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

[spi_device] Disallow certain state changes when busy #21700

Closed
a-will opened this issue Feb 27, 2024 · 8 comments · Fixed by #23546
Closed

[spi_device] Disallow certain state changes when busy #21700

a-will opened this issue Feb 27, 2024 · 8 comments · Fixed by #23546

Comments

@a-will
Copy link
Contributor

a-will commented Feb 27, 2024

Description

On a real SPI flash device, many commands are ignored when the device is busy. We might want to consider adding this behavior to our IP as well.

Currently, command uploads, WREN, WRDI, EN4B, and EX4B all are allowed while the busy bit is set.

@a-will
Copy link
Contributor Author

a-will commented Feb 27, 2024

I have created this primarily for future reference (and evaluation in the D2 sign-off meeting). I don't think this is necessary for the next earlgrey release--It primarily helps for achieving robust behavior when the host is doing something illegal, likely due to a loss of state from a reset or similar.

@a-will
Copy link
Contributor Author

a-will commented Feb 27, 2024

A similar discussion about this happened here: #12053 (comment)

@msfschaffner msfschaffner modified the milestones: Backlog, Earlgrey-PROD.M4 Mar 1, 2024
@andreaskurth andreaskurth added the Triage Priority Issue to be discussed with priority in the next triage meeting label Mar 25, 2024
@andreaskurth
Copy link
Contributor

@a-will: This is currently in M4, but your proposal would be rather to not do this for this release (thus backlog), right?

@a-will
Copy link
Contributor Author

a-will commented Apr 2, 2024

In the D2 sign-off, I think we decided to just get it done.

@andreaskurth andreaskurth removed the Triage Priority Issue to be discussed with priority in the next triage meeting label Apr 12, 2024
@andreaskurth
Copy link
Contributor

@moidx: I think we already have this feature, so this may only be a coverage gap. Needs to be taken into account for DV.

@andreaskurth andreaskurth changed the title [spi_device] Disallow certain state changes when busy [spi_device/dv] Disallow certain state changes when busy Apr 12, 2024
@antmarzam
Copy link
Contributor

@moidx: I think we already have this feature, so this may only be a coverage gap. Needs to be taken into account for DV.

@andreaskurth - This needs doing in both RTL and TB (I just checked with @a-will)

@a-will
Copy link
Contributor Author

a-will commented Apr 12, 2024

Off the top of my head, I'd probably simply add the current "busy" value as an input to spi_cmdparse, then use it to block the commands that should be ignored. For example, if busy is 1, we could prevent decoding the opcode here:

assign opcode_en4b = cmd_info_i[CmdInfoEn4B].valid
&& (data_i == cmd_info_i[CmdInfoEn4B].opcode);

There are likely a number of equivalent means of doing it, though.

@andreaskurth
Copy link
Contributor

Ah, in that case we were wrong about what needs to happen here during today's triage -- sorry! Thanks for clarifying 👍

@antmarzam, as current coordinator for spi_device, may I leave it to you to find someone to do the RTL change (or do it yourself)?

@andreaskurth andreaskurth changed the title [spi_device/dv] Disallow certain state changes when busy [spi_device] Disallow certain state changes when busy Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants