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

[sw,multitop] Port spi_device_passthrough and spi_host_testutils to DT #26284

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

AlexJones0
Copy link
Contributor

@AlexJones0 AlexJones0 commented Feb 13, 2025

Fix #26234

This PR ports the spi_device_passthrough test to use the devicetables API so that it no longer depends on Earlgrey-specific constants. The test remains passing on Earlgrey in the sim_dv environment, and will compile for Darjeeling via

bazel build //sw/device/tests/sim_dv:chip_sw_spi_device_pass_through_sim_dv --//hw/top=darjeeling

As part of porting this test, a small issue with a special case in top_earlgrey.hjson was encountered, with dtgen not generating necessary constants, so a fix for this is introduced. Porting of this test also required porting of the spi_host_testutils library, and refactoring of the pad_attributes/configure_pad interfaces in the pinmux utils also. Note that specifically for the spi_host_testutils and the spi_passthrough_test itself, several pinmux mappings are used which are very top-specific. I've tried to introduce some conditional compilation based on the EARLGREY_IS_TOP defines that should throw appropriate errors when compiling the test on an unsupported top.

@AlexJones0 AlexJones0 requested review from msfschaffner and a team as code owners February 13, 2025 17:47
@AlexJones0 AlexJones0 requested review from jon-flatley and removed request for a team February 13, 2025 17:47
@AlexJones0 AlexJones0 changed the title Dt spi passthrough [sw,multitop] Port spi_device_passthrough and the spi_host_testutils to DT Feb 13, 2025
@AlexJones0 AlexJones0 requested review from pamaury, nbdd0121 and jwnrt and removed request for msfschaffner and jon-flatley February 13, 2025 17:51
@AlexJones0 AlexJones0 changed the title [sw,multitop] Port spi_device_passthrough and the spi_host_testutils to DT [sw,multitop] Port spi_device_passthrough and spi_host_testutils to DT Feb 13, 2025
Copy link
Contributor

@jwnrt jwnrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The port looks okay to me, though I don't know much about this strange pin type.

sw/device/tests/pmod/spi_host_gigadevice1Gb_flash_test.c Outdated Show resolved Hide resolved
@AlexJones0 AlexJones0 added the CI:Rerun Rerun failed CI jobs label Feb 14, 2025
@github-actions github-actions bot removed the CI:Rerun Rerun failed CI jobs label Feb 14, 2025
@AlexJones0
Copy link
Contributor Author

Force push is just a rebase to pull in CI fixes

@AlexJones0
Copy link
Contributor Author

Force push is a rebase again due to Github runner issues

@@ -578,50 +579,50 @@ static void configure_pinmux_sim(void) {
const pinmux_pad_attributes_t pinmux_pad_attributes[] = {
// Enable pull-ups for spi_host_0 data pins to avoid floating inputs.
{
.pad = kTopEarlgreyDirectPadsSpiHost0Sd0,
.pad = kDtPadSpiHost0Sd0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not for this PR but I feel this function is doing duplicate work. If the pins are already configured (ie the DIOs/MIOs are connected) then I think that with the DT the function could just go through the list of pins, figure out the pads that they are connected to and configure them. This would avoid repeating the list of pads in several places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable, but due to the scope I think should be addressed outside this PR.

@@ -43,37 +44,37 @@ static dif_spi_host_t spi_host1;
// Enable pull-ups for spi_host data pins to avoid floating inputs.
static const pinmux_pad_attributes_t pinmux_pad_config[] = {
{
.pad = kTopEarlgreyMuxedPadsIob1,
.pad = kDtPadIob1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

In `top_earlgrey.hjson`, the IOA2 and IOA3 pads have their port type
overriden from the default `inout wire` to the value `INOUT_A0.
According to the relevant comment, this is only used for simulating ADC
connections right now, where these ports are switched to a real type
when the SystemVerilog ANALOGSIM macro is defined (i.e. this would work
if we parsed a pre-processed SV file, but not when generating from this
HJSON).

Since this appears to be the one unique case of a macro being used for a
port-type in a hacky way, we just hard-code this into the relevant lists
in dtgen/helper.py. An extra check/assertation is added to check that
the port type is in a list of known port types, to avoid this issue
silently appearing again in the future.

Signed-off-by: Alex Jones <[email protected]>
@AlexJones0 AlexJones0 force-pushed the dt_spi_passthrough branch 2 times, most recently from 1f198db to bf47a8c Compare February 14, 2025 16:26
@AlexJones0 AlexJones0 requested a review from nbdd0121 February 14, 2025 16:46
@AlexJones0 AlexJones0 requested a review from pamaury February 14, 2025 16:47
Copy link
Contributor

@pamaury pamaury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks modulo the dif_pinmux_pad_write_attrs -> dif_pinmux_pad_write_attrs_dt that might simplify things a bit.

Ports the spi_host_testutils utilities to use the devicetable Pinmux
interface, so that it no longer relies on Earlgrey constants. Also
updates the corresponding tests that use this library to provide a
CSB pin through the DT interface, though the remainder of these
tests are not yet ported to devicetables.

This library uses pinmux mappings that are quite specific to
Earlgrey/Darjeeling, so relevant conditional compilation is introduced
to ensure the testutils only build information for relevant tops, and
error in the case of an undefined top (to make tracking down errors
easier).

Signed-off-by: Alex Jones <[email protected]>
Add a dif function for getting the attributes from a devicetables pad,
like the corresponding `dif_pinmux_pad_write_attrs_dt` that already
exists, but for getting attributes.

Signed-off-by: Alex Jones <[email protected]>
Makes the pinmux testutils pad configuration API take pad attributes
defined using DT pads, rather than Earlgrey top-specific tabs. This
allows more tests to be defined using only devicetables, without
depending on constants from any specific top.

Signed-off-by: Alex Jones <[email protected]>
This test requires more significant changes to be suited to running on
Darjeeling, as it uses some Earlgrey-specific pinmux mappings, and uses
two SPI Hosts instead of just one. Only the first SPI Host is actually
used, so we enable the test to scale to initialise any number of SPI
hosts per top but still only use the first. Pin mappings are hard-coded
for Earlgrey/Darjeeling based on the top-level using conditional
compilation.

When porting to DT, more generic logic for enabling all the interrupts
for the Spi Host and Spi Device blocks is introduced, to avoid
hard-coding the name of interrupts when the test itself does not require
it.

Signed-off-by: Alex Jones <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Multitop, test] chip_sw_spi_device_pass_through
5 participants