[dv,spi] Set strong drive strength and fast slew rate for SPI pads

[sha-ron]

SPI pads should use strong drive strength and fast slew rate.

[moidx]

CI has the following error:

E00003 power_virus_systemtest.c:1468] DIF-fail: dif_pinmux_pad_write_attrs(&pinmux, kTopEarlgreyDirectPadsSpiHost0Sck, kDifPinmuxPadKindDio, in_attr, &out_attr) returns 13

Code: https://github.com/lowRISC/opentitan/blob/7e34e67ade0717418930ecbfa4bee30ba44979ed/sw/device/lib/dif/dif_pinmux.c#L307-L309

This only happens in the sival_rom_ext configuration. Need to check if we are locking pinmux register in that case

[a-will]

CI has the following error:

E00003 power_virus_systemtest.c:1468] DIF-fail: dif_pinmux_pad_write_attrs(&pinmux, kTopEarlgreyDirectPadsSpiHost0Sck, kDifPinmuxPadKindDio, in_attr, &out_attr) returns 13

Code:

https://github.com/lowRISC/opentitan/blob/7e34e67ade0717418930ecbfa4bee30ba44979ed/sw/device/lib/dif/dif_pinmux.c#L307-L309

This only happens in the sival_rom_ext configuration. Need to check if we are locking pinmux register in that case

It's happening in all FPGA configurations because these pad attributes are not available. Pad attributes should not be written without proper matching against the configuration.

[moidx]

Thanks @a-will for your quick response.

@sha-ron, you will probably want to constraint this configuration to the follow device types:

• kDeviceSilicon
• kDeviceSimDV

[sha-ron]

Thank you @a-will, @moidx for the review and help with the CI failures. I added constrains on the configurations as suggested. Hopefully Ci will pass now.

[timothytrippel]

@sha-ron should this change be eventually cherry-picked over to the earlgrey_1.0.0 branch too?

[sha-ron]

@sha-ron should this change be eventually cherry-picked over to the earlgrey_1.0.0 branch too?

Yes, it should be merged also to earlgrey_1.0.0 branch (it is required for GLS simulation).

[timothytrippel]

@sha-ron : looks like this breaks CI. Can you fix the failures? Then we can get this merged.

[sha-ron]

Thank you @timothytrippel. It looks like CI is failing over FPGA checks. I use 'if (kDeviceType == kDeviceSilicon || kDeviceType == kDeviceSimDV)' so I am not sure if/how it is related to the changes in the PR. Do you understand what it is failing on?

[timothytrippel]

@sha-ron It looks like there are also DV failures, see the "Private CI" jobs

[sha-ron]

I see now:( It looks like it is related to the pad attribute definition in the open source https://github.com/lowRISC/opentitan/blob/master/hw/ip/prim_generic/rtl/prim_generic_pad_attr.sv#L53 that is different. Not sure what is the reason for this definition but at stage maybe it will be better to add all the code under some `ifdef. WDYT?

[vogelpi]

You're right @sha-ron , depending on the platform (silicon, open source DV, or Verilator), a different number of drive strength bits are supported. Other bits are tied to zero. Some other pad attributes are treated similarly.

I think the real issue here is that these pad attribute registers are actually defined as WARL (write any, read legal) while the DIF treats them as RW (read and write): There is a check in the DIF which errors out in case an unsupported bit is written. This doesn't make sense for a WARL register. I've now filed #24688 to change this.

I also tested your PR on top of the #24688 and it works. So once #24688 is merged, we can rebase this one and it should pass CI.

[vogelpi]

CI has the following error:

E00003 power_virus_systemtest.c:1468] DIF-fail: dif_pinmux_pad_write_attrs(&pinmux, kTopEarlgreyDirectPadsSpiHost0Sck, kDifPinmuxPadKindDio, in_attr, &out_attr) returns 13

Code: https://github.com/lowRISC/opentitan/blob/7e34e67ade0717418930ecbfa4bee30ba44979ed/sw/device/lib/dif/dif_pinmux.c#L307-L309

This only happens in the sival_rom_ext configuration. Need to check if we are locking pinmux register in that case

It's happening in all FPGA configurations because these pad attributes are not available. Pad attributes should not be written without proper matching against the configuration.

Ah, sorry @a-will , I only saw your message now. So the approach I took in #24688 is probably wrong then. Can someone please provide some guidance on how to match the attributes against the configuration? We could provide a platform-dependent mask similar to how we specify clock frequencies for FPGA, simulation and silicon. But the main issue here is that there are differences between the different pad implementations between closed and open source. I don't know how we should handle this if we want to keep the check referenced by Alex above.

[a-will]

CI has the following error:

E00003 power_virus_systemtest.c:1468] DIF-fail: dif_pinmux_pad_write_attrs(&pinmux, kTopEarlgreyDirectPadsSpiHost0Sck, kDifPinmuxPadKindDio, in_attr, &out_attr) returns 13

Code: https://github.com/lowRISC/opentitan/blob/7e34e67ade0717418930ecbfa4bee30ba44979ed/sw/device/lib/dif/dif_pinmux.c#L307-L309

This only happens in the sival_rom_ext configuration. Need to check if we are locking pinmux register in that case

It's happening in all FPGA configurations because these pad attributes are not available. Pad attributes should not be written without proper matching against the configuration.

Ah, sorry @a-will , I only saw your message now. So the approach I took in #24688 is probably wrong then. Can someone please provide some guidance on how to match the attributes against the configuration? We could provide a platform-dependent mask similar to how we specify clock frequencies for FPGA, simulation and silicon. But the main issue here is that there are differences between the different pad implementations between closed and open source. I don't know how we should handle this if we want to keep the check referenced by Alex above.

This is one of those special pieces of top-specific issues that we have been ignoring. Is this the first time we have had a mismatch between generic DV and GLS where the same code paths can't be used? In some ways, the target platform is closer to the "silicon" one than the generic "dv" one... Should this be a silicon device type for software, but tests run with DV tooling?

For now, I'd probably just choose to check the readback value under the DV device type and explicitly ignore mismatches for drive strength. Then add a TODO to improve this with multi-top. Pad attributes are going to be one of those gnarly areas to solve. sw/device/lib/arch/device.h is a kludge.

[pamaury]

I wonder if this should be part of the spi_device testutils? This would avoid duplicating code in several places.

[vogelpi]

Thanks a lot @a-will for your feedback, this is really appreciated!

@sha-ron , I took the liberty to directly update this PR with an implementation that follows the approach suggested by Alex and that we also use in other files. The idea is to configure the pad attributes and then compare the returned / accepted attributes against the specified ones. As previously discussed, different target platforms may use pad with different max slew rate and drive strength, so we can kind of expect that there might be a mismatch for these attributes. The implementation now accepts a mismatch in the drive strength and slew rate but not for other attributes.

After reworking these checks, I ran into issues with the power virus and the passthrough tests: I got assertion failures at the top-level and inside SPI host. It seems we should not enable the pull up for SPI device for these tests. I am no expert on this obviously, but does that make sense to you?

[vogelpi]

I wonder if this should be part of the spi_device testutils? This would avoid duplicating code in several places.

I thought about that, too. But I think we should merge this PR and cherry pick it into the TO release branch as soon as possible to unblock @sha-ron . Maybe the restructuring can be done as a follow-up? I am happy for someone else to take this on but I should move on to another higher prio task.

[pamaury]

Sure, just create an issue to remember to do it, it's not urgent to refactor now.

[vogelpi]

@pamaury , I've now added two more comments to factor our that part of the pad attributes configuration which can be shared between tests into spi_host/device_testutils.

[pamaury]

@pamaury , I've now added two more comments to factor our that part of the pad attributes configuration which can be shared between tests into spi_host/device_testutils.

Thanks, it looks great. Is there any reason you have not converted spi_host_tx_rx.c?

[vogelpi]

Thanks @pamaury , the configuration of SPI Host 0 in spi_host_tx_rx.c should now be shared. SPI Host 1 is a bit different in the sense that we set up pad attributes but also the muxing. I could probably share some of that logic as well, but it's not used in another test yet. So we probably don't gain much right now by doing this.

[sha-ron]

Thanks a lot @a-will for your feedback, this is really appreciated!

@sha-ron , I took the liberty to directly update this PR with an implementation that follows the approach suggested by Alex and that we also use in other files. The idea is to configure the pad attributes and then compare the returned / accepted attributes against the specified ones. As previously discussed, different target platforms may use pad with different max slew rate and drive strength, so we can kind of expect that there might be a mismatch for these attributes. The implementation now accepts a mismatch in the drive strength and slew rate but not for other attributes.

After reworking these checks, I ran into issues with the power virus and the passthrough tests: I got assertion failures at the top-level and inside SPI host. It seems we should not enable the pull up for SPI device for these tests. I am no expert on this obviously, but does that make sense to you?

Yes I don't see a reason to enable pull up for SPI device.

[vogelpi]

Okay, thanks for the feedback @sha-ron !