[spi_device] Remove generic mode #20856
Conversation
a-will
force-pushed
the
spid-remove-fwmode
branch
3 times, most recently
from
9e1cbf9 to
4c901c8
Compare
a-will
force-pushed
the
spid-remove-fwmode
branch
4 times, most recently
from
a2f6535 to
f153f91
Compare
a-will
requested review from
timothytrippel,
msfschaffner,
engdoreis and
hcallahan-lowrisc
|
Sorry for the massive PR, but I'm hoping the commit breakdown will help direct folks to the areas that they're most concerned about. This is primarily deletion, with some swaps where appropriate (primarily for looping from the start of the interrupt block). Here's the plan: Once each of the areas of DD, DV, and SW are represented, I'll squash all the commits down to one, write some reasonable error message, then Edit: The first commit now has the intended commit message, so I've pulled this out of draft state. The other commits will all be squashed into the first once reviews are complete. |
| "spi_device_dummy_item_extra_dly_vseq", | ||
| "spi_device_intr_vseq", | ||
| "spi_device_perf_vseq", | ||
| string seq_names[$] = { |
There was a problem hiding this comment.
"stress_all" doesn't really stress much anymore, does it? 😁
| tags = [ | ||
| "broken", | ||
| "manual", | ||
| ], |
There was a problem hiding this comment.
The Tock test is disabled for now, but most likely, this isn't the best repo for it. I believe RTL changes in this repo should not be gated by a simultaneous Tock support requirement. In terms of scheduling and resources, Tock support is downstream (and probably should depend on this repo's releases, rather than keeping pace with each PR).
There was a problem hiding this comment.
Does this deserve a comment inline so that it is clear why this has been marked disabled/broken?
There was a problem hiding this comment.
I'm not sure. I think we should remove this test from the repo altogether, really.
It's broken because a third-party software repo doesn't support earlgrey*, but it seems a little funky to have this repo carry issues for that one. Tock is important to us, but I believe it should be positioned downstream of this repo.
*I'm being a little obtuse with the naming here, but from the perspective of the master branch, what was called "earlgrey ES" is left to history.
There was a problem hiding this comment.
I'm ok with removing it, since it creates a weird dependency loop.
a-will
force-pushed
the
spid-remove-fwmode
branch
from
f153f91 to
f916d6b
Compare
| tags = [ | ||
| "broken", | ||
| "manual", | ||
| ], |
There was a problem hiding this comment.
Does this deserve a comment inline so that it is clear why this has been marked disabled/broken?
|
|
||
| ////////////////////// | ||
| // Interrupt Enable // | ||
| ////////////////////// | ||
| for (genvar s = 0; s < 185; s++) begin : gen_ie0 | ||
| for (genvar s = 0; s < 179; s++) begin : gen_ie0 |
There was a problem hiding this comment.
Silly question, I've seen this number repeating throughout the code base, Isn't there a easy way to get this number from the hjson file?
There was a problem hiding this comment.
I'm not sure what you mean, specifically. This file is generated by topgen / ipgen, and this number already comes from the top-level hjson file.
However, if you are saying that stylistically, it'd be better if the rv_plic ipgen templates used a named constant throughout instead of a magic number, then I do agree!
(And really, the constant is already available. ${module_instance_name}_reg_pkg::NumSrc is that number)
There was a problem hiding this comment.
Afaik, this code is generated (so all these numbers should be automatically updated through the template).
We could make it look nicer by converting to parameters I agree - but the numbers are not hand edited.
Remove generic mode from spi_device. Generic mode has a very different timing model than the relatively high throughput, high latency flash and tpm modes. Generic mode requires data to be available on the MISO pin *before* the first SCK edge, whereas flash and tpm modes have a significant command phase that comes first. In addition, generic mode requires support for a number of different idle clock polarities and sampling phases, whereas flash and TPM modes have one clearly defined set. The complexity for supporting both timing models for one IP led to a compromised performance for flash and TPM. Remove generic mode here to permit optimizations that target the latter modes. Signed-off-by: Alexander Williams <[email protected]>
a-will
force-pushed
the
spid-remove-fwmode
branch
from
f916d6b to
30ff93f
Compare
Remove generic mode from spi_device.
Generic mode has a very different timing model than the relatively high throughput, high latency flash and tpm modes. Generic mode requires data to be available on the MISO pin before the first SCK edge, whereas flash and tpm modes have a significant command phase that comes first. In addition, generic mode requires support for a number of different idle clock polarities and sampling phases, whereas flash and TPM modes have one clearly defined set.
The complexity for supporting both timing models for one IP led to a compromised performance for flash and TPM. Remove generic mode here to permit optimizations that target the latter modes.
Resolves #15452