[hw,pinmux,lint] Read unused signals to avoid lint errors #26020
Conversation
There was a problem hiding this comment.
I want to make sure that I understand this. If I've got it right, this mux is sized to be the first power of two that is at least NMioPads+2 and NDioPads.
Presumably (for the configuration you're interested in), NDioPads is much larger so the index in this line:
assign mio_to_periph_o[k] = mio_mux[reg2hw.mio_periph_insel[k].q];can never be all that big? (as checked by a lint tool)
Is that correct? If so, can you add a note to the commit message that explains things (and I'm very happy to take the change!)
|
We've discussed that it would be good to add a comment / commit message and that ideally we can change the template to not insert the code if it is unneeded. I.e., we also won't need to authorize any code change for Earlgrey. |
|
@vogelpi As discussed, I gated the read of the unused signals as part of the template rendering. |
There was a problem hiding this comment.
Thanks for making the proposed changes @Razer6 .
After taking another look at this and also reading @rswarbrick comment and questions (thanks for the thorough review Rupert), I realized that once more the lint tool is right here and these waivers shouldn't be added in the first place.
What actually happens is that you have either 128 MIOs or DIOs (I haven't checked which one is bigger, it doesn't matter) but from the register file the padsel signals are only 6 bits wide. That's why the mux signals with indices 127 to 64 can never be selected.
What should be changed instead is the pinmux hjson for Darjeeling. At least the WKUP_DETECTOR_PADSEL register should be extend from 5:0 to 6:0 bits.
| % if n_dio_pads > n_mio_pads + 2: | ||
| logic unused_mio_wkup_mux_signals; | ||
| assign unused_mio_wkup_mux_signals = ^{mio_wkup_mux[(AlignedMuxSize - 1):(NMioPads + 2)]}; | ||
| logic unused_dio_wkup_mux_signals; | ||
| assign unused_dio_wkup_mux_signals = ^{dio_wkup_mux[(AlignedMuxSize - 1):(NMioPads + 2)]}; | ||
|
|
||
| % endif |
There was a problem hiding this comment.
After taking another look at this I think this actually doesn't make sense. Either you have more MIOs or more DIOs, so it should only be required to waive either unused DIOs or unused MIOs but not both.
|
@Razer6 , I've now implemented the change to correctly size the PADSEL registers. Interestingly, part of your lint waivers are still required. I've now updated your commit accordingly and added comments as requested by @rswarbrick . |
For pinmux configurations with NMioPads + 2 < NDioPads, the mio_in input signal is zero-extended to NDioPads bits but simplify the RTL code. However, depending on the actual widths, most of the zero bits can actually not be selected, as mio_periph_insel is sized to only select NMioPads + 2 bits. Signed-off-by: Robert Schilling <[email protected]>
The `dio_in_i` signal going into the wakeup detector multiplexer is of width `NDioPads`, i.e., it contains both inputs and outputs. Previously, ipgen was using the number of DIO input pads to compute the width of the padsel register fields. As a result, only a subset of the DIO pads could be selected in Darjeeling. Signed-off-by: Pirmin Vogel <[email protected]>
|
@vogelpi Great. That seems it was a bug in the HJSON? but it never triggered because of the specific pad configuration of Earlgrey? |
Yes exactly! |
Depending on the pin config of the pinmux, some signals of the MUX might be unused (It is aligned to the largest DIO size). Read the unused signals to avoid linting errors.