-
Notifications
You must be signed in to change notification settings - Fork 10
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
Conversation
Update spi host to match V3 changes Signed-off-by: Alex Jones <[email protected]>
@@ -370,7 +370,7 @@ struct TxFifo { | |||
*/ | |||
typedef struct { | |||
uint32_t opts; /* configopts */ | |||
uint16_t command; /* command[15:0] */ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :(
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
Superseded with #120. |
Version 3 of OpenTitan's SPI Host introduces changes to the command register, increasing its size. See: lowRISC/opentitan#25199
This PR updates the relevant register/field definitions to ensure that
spi_host_smoketest
remains passing on OpenTitan'smaster
.