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] Remove Generic (a.k.a. FwMode) from SPI_DEVICE HWIP #15452

Closed
eunchan opened this issue Oct 12, 2022 · 7 comments · Fixed by #20856
Closed

[spi_device] Remove Generic (a.k.a. FwMode) from SPI_DEVICE HWIP #15452

eunchan opened this issue Oct 12, 2022 · 7 comments · Fixed by #20856
Assignees
Labels
Earlgrey-PROD Candidate Temporary label to triage issues into Earlgrey-PROD Milestones IP:spi_device Priority:P0 Priority: critical Type:FutureRelease Not relevant to currently planned releases/milestones

Comments

@eunchan
Copy link
Contributor

eunchan commented Oct 12, 2022

This is a topic brought up multiple times. As TPM can behave almost identical but limited frame size (64B in TPM vs 1kB or more), it is OK to remove Generic mode. TPM protocol, however, has better flow control compared to Generic mode.

The benefit of removing Generic mode is to clean up the complex CDC path on SRAM B port in SPI_DEVICE. It raises many CDC warnings and errors, so that we waived many of them. By removing the Generic mode, SRAM b datapath sits in SPI_CLK always, which simplifies the clock domain and makes easier to verify CDC.

CC: @tjaychen @weicaiyang @jeoongp

@eunchan eunchan added the Priority:P3 Priority: low label Oct 12, 2022
@eunchan eunchan self-assigned this Oct 12, 2022
@a-will
Copy link
Contributor

a-will commented Oct 12, 2022

The benefit of removing Generic mode is to clean up the complex CDC path on SRAM B port in SPI_DEVICE. It raises many CDC warnings and errors, so that we waived many of them. By removing the Generic mode, SRAM b datapath sits in SPI_CLK always, which simplifies the clock domain and makes easier to verify CDC.

I'll point out that adjusting the generic mode logic so it doesn't use asynchronous FIFOs in front of the SRAM would also take care of that issue. Removing generic mode altogether may be the best choice, but it's not the only way to get rid of the complex CDC.

@eunchan
Copy link
Contributor Author

eunchan commented Oct 12, 2022

Yep. you are right. We can fix the issue as DPRPAM now supports byte enable (it wasn't when GenericMode was designed. That's why DPSRAM was in SYS_CLK and async fifo was there). But as I mentioned, it is not worth to have two almost identical modules in SPI_DEVICE.

@tjaychen
Copy link

if we remove generic mode, does that mean spi device would have no way of constructing a full duplex protocol if someone wanted to use it that way?

@eunchan
Copy link
Contributor Author

eunchan commented Oct 13, 2022 via email

@tjaychen
Copy link

i've honestly only ever seen 2 use cases of someone doing that.
@a-will may have seen others.

@andreaskurth
Copy link
Contributor

Triaged for spi_device. Labeling Type:FutureRelease Not relevant to currently planned releases/milestones because removing Generic / FW mode seems out of scope for this release. Verification of the mentioned CDC path is tracked in #15457.

@andreaskurth andreaskurth added Type:FutureRelease Not relevant to currently planned releases/milestones Triaged labels Feb 23, 2023
@msfschaffner
Copy link
Contributor

Indeed, removing / changing generic mode is out of scope for M2.5.
CDC will be closed separately as noted above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Earlgrey-PROD Candidate Temporary label to triage issues into Earlgrey-PROD Milestones IP:spi_device Priority:P0 Priority: critical Type:FutureRelease Not relevant to currently planned releases/milestones
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants