Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ot] hw/opentitan: ot_spi_host: SPI Host V3 changes #119

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions hw/opentitan/ot_spi_host.c
Original file line number Diff line number Diff line change
@@ -111,10 +111,10 @@ REG32(CONFIGOPTS, 0x18u)
REG32(CSID, 0x1cu)
FIELD(CSID, CSID, 0u, 32u)
REG32(COMMAND, 0x20u)
FIELD(COMMAND, LEN, 0u, 9u)
FIELD(COMMAND, CSAAT, 9u, 1u)
FIELD(COMMAND, SPEED, 10u, 2u)
FIELD(COMMAND, DIRECTION, 12u, 2u)
FIELD(COMMAND, CSAAT, 0u, 1u)
FIELD(COMMAND, SPEED, 1u, 2u)
FIELD(COMMAND, DIRECTION, 3u, 2u)
FIELD(COMMAND, LEN, 5u, 20u)
REG32(RXDATA, 0x24u)
REG32(TXDATA, 0x28u)
REG32(ERROR_ENABLE, 0x2cu)
@@ -370,7 +370,7 @@ struct TxFifo {
*/
typedef struct {
uint32_t opts; /* configopts */
uint16_t command; /* command[15:0] */

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above, it was designed to fit into a 64-bit word.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW the comment had been fixed earlier, need to check whether it got lost between DG et EG branches

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW the comment had been fixed earlier, need to check whether it got lost between DG et EG branches

Fixes in DJ branches have not been backported to EG branch. I'm working on it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, in that case I'm not sure how you think would be best to address this without breaking the 64-bit word size. The command size increase in V3 I think cannot be implemented with how few bits there are available (it requires 25 bits now), and if we were to increase the size of the CmdFifoSlot then we may as well support the rest of the CS lines.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we cannot use 64-bit storage anymore, without relying on ugly C bitfields or even more ugly macros.

There's more to do with SPI flash support, as the tests seem to use both SPI buses, and the way the Earlgrey machine used to support the SPI dataflash device does not fit well this model and collides with embedded flash.

I'm working on these different topics, I should have post to Slack sooner :(

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies - I didn't see your Slack message until just now. I had this change from a bit ago, and only opened the PR now to get spi_host_smoketest passing. If you're working on the SPI host please do feel free to close this PR in favour of your fixes.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My fault, I've started to work on this last friday and only posted this morning.
Your patches were indeed useful as I initially missed the 64-bit problem.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #120 (only a draft for now)

uint32_t command; /* command[31:0] */
uint8_t csid; /* csid[7:0] */
bool ongoing; /* command is being processed */
} CmdFifoSlot;
@@ -752,7 +752,7 @@ static void ot_spi_host_step_fsm(OtSPIHostState *s, const char *cause)
s->fsm.active = true;
ot_spi_host_update_event(s);

uint32_t command = (uint32_t)headcmd->command;
uint32_t command = headcmd->command;
bool read = ot_spi_host_is_rx(command);
bool write = ot_spi_host_is_tx(command);
unsigned speed = FIELD_EX32(command, COMMAND, SPEED);
@@ -831,7 +831,7 @@ static void ot_spi_host_step_fsm(OtSPIHostState *s, const char *cause)
ongoing = false;
}

headcmd->command = (uint16_t)command;
headcmd->command = command;
headcmd->ongoing = ongoing;

timer_mod(s->fsm_delay,
@@ -860,7 +860,7 @@ static void ot_spi_host_post_fsm(void *opaque)
trace_ot_spi_host_fsm(s->ot_id, "post");

CmdFifoSlot *headcmd = cmdfifo_peek(s->cmd_fifo);
uint32_t command = (uint32_t)headcmd->command;
uint32_t command = headcmd->command;
bool ongoing = headcmd->ongoing;

ot_spi_host_trace_status(s->ot_id, "P>", ot_spi_host_get_status(s));
@@ -1130,7 +1130,7 @@ static void ot_spi_host_io_write(void *opaque, hwaddr addr, uint64_t val64,
uint16_t csid = (uint16_t)s->regs[R_CSID];
CmdFifoSlot slot = {
.opts = s->config_opts[csid],
.command = (uint16_t)val32, /* only b15..b0 are meaningful */
.command = val32,
.csid = csid,
.ongoing = false,
};