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

[multitop] Port DIF to multitop, port two tests to multitop #25943

Merged
merged 13 commits into from
Jan 26, 2025

Conversation

pamaury
Copy link
Contributor

@pamaury pamaury commented Jan 20, 2025

Follow up PR on #25807, this handles the DIF and shows how to port two tests.

This PR also does one notable change to the DT which was discussed on #25807 and pushed to this PR: rename "signal" to "peripheral I/O" to match the CIP specification.

@pamaury pamaury requested review from a-will, engdoreis and jwnrt January 20, 2025 10:25
@pamaury pamaury force-pushed the multitop_sw_dt2 branch 3 times, most recently from c68ad03 to cde0ac5 Compare January 23, 2025 10:28
jwnrt
jwnrt previously approved these changes Jan 23, 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 code LGTM.

It looks like the compiler did pick up on the enum = int + 1 problem from the previous PR. I think the other CI errors are casting problems. LGTM once these are fixed and it passes CI.

@pamaury pamaury changed the title [multitop] Port DIF to multitp, port two tests to multitop [multitop] Port DIF to multitop, port two tests to multitop Jan 23, 2025
@pamaury pamaury force-pushed the multitop_sw_dt2 branch 4 times, most recently from c7e03e0 to 090e969 Compare January 23, 2025 15:47
The term peripheral I/O is the one used in the comportable IO spec
so it's better to stay consistent. It's abbreviated as periph_io
to make it shorter.

Signed-off-by: Amaury Pouly <amaury.pouly@lowrisc.org>
Previously, the code was using specifially crafted values to indicate
the I/O direction. This causes problems with the compiler since we were
using values output of the enumeration range. This was also more of hack.
This commit introduces properly an enum type for the peripheral I/O
direction.

Signed-off-by: Amaury Pouly <amaury.pouly@lowrisc.org>
@pamaury pamaury force-pushed the multitop_sw_dt2 branch 3 times, most recently from e980564 to 52cab17 Compare January 23, 2025 21:21
@pamaury pamaury marked this pull request as ready for review January 23, 2025 21:26
@pamaury pamaury requested review from msfschaffner and a team as code owners January 23, 2025 21:26
@pamaury pamaury requested review from Razer6 and cfrantz January 23, 2025 21:26
@@ -173,6 +177,7 @@ cc_library(
deps = [
":base",
"//hw/top:clkmgr_c_regs",
"//hw/top:dt_clkmgr",
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll see if a single set of clkmgr DIFs can really hold up for multi-top. ipgen templated IPs may not all work with this sort of abstraction. For that matter, the OTP DIFs use named partitions in places, so that probably won't due...

Stuff to ponder for the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes future will tell if a single DIF can handle that but it's definitely not the job of this PR to change the current approach ;)

sw/device/tests/aon_timer_irq_test.c Show resolved Hide resolved
sw/device/tests/aon_timer_irq_test.c Outdated Show resolved Hide resolved
sw/device/tests/aon_timer_irq_test.c Show resolved Hide resolved
Instead of exposing the structure, expose indices and make all API
take indices as input. This produce the same result with LTO enabled
since constants indices will propagate to load of compile-time known
constants.

Signed-off-by: Amaury Pouly <amaury.pouly@lowrisc.org>
Hide the `dt_<ip>` type (which is a struct) and replace it by an
enum of instance names. The APIs are changed to takes indices as
input and the structure is made local to a new `dt_<ip>.c` file.

Signed-off-by: Amaury Pouly <amaury.pouly@lowrisc.org>
This commit changes the generator as follows:
- a new `dif_<ip>_init_from_dt` function to initialize from DT
  instead of from an IO region
- the dif IRQ names and types are now defines/typedefs to the DT
  types: this ensures compatibility with existing code and an
  incremental transition

Signed-off-by: Amaury Pouly <amaury.pouly@lowrisc.org>
The DT commit changed the type `dif_rv_plic_irq_id_t` from `uint32_t`
to an enumeration. This makes certain implicit casts in the C++ code
invalid now.

Signed-off-by: Amaury Pouly <amaury.pouly@lowrisc.org>
pamaury and others added 4 commits January 24, 2025 15:30
This is for documentation purposes, to make it clear that DT IRQ
IDs are PLIC IRQ IDs.

Signed-off-by: Amaury Pouly <amaury.pouly@lowrisc.org>
The compiler is not happy about a missing case for kDtI2cIrqCount
which is obviously invalid. Add a default to please the compiler.

Signed-off-by: Amaury Pouly <amaury.pouly@lowrisc.org>
This is necessary since the autogen part of DIFs now depends on DT.

Signed-off-by: Amaury Pouly <amaury.pouly@lowrisc.org>
Co-authored-by: Alexander Williams <awill@opentitan.org>
Signed-off-by: Amaury Pouly <amaury.pouly@lowrisc.org>
@pamaury
Copy link
Contributor Author

pamaury commented Jan 24, 2025

I have done a substantial overall of the PR:

  • Now the pads and dt structures are access by indices, instead of pointers. Concretely, instead dt_pad_t and dt_<ip>_t become enums, like
/**
 * List of instances.
 */
typedef enum dt_uart {
  kDtUart0 = 0, /**<  */
  kDtUart1 = 1, /**<  */
  kDtUart2 = 2, /**<  */
  kDtUart3 = 3, /**<  */
  kDtUartCount = 4, /**< Number of instances */
} dt_uart_t;

and all APIs functions take a dt_uart_t as input. The actual struct and table is move to a dt_<ip>.c file and the types are made private. For the perspective of API users, the only change is that it's not a pointer type anymore.

  • I added two functions to the pinmux DIF to make it easy to connect a MIO using the new dt_periph_io_t and dt_pad_t types.
  • the dt/dt.h header is gone, everything is moved to either dt/dt_api.h or dt/dt_<ip>.h. This also means one needs to include one header per IP, like for the DIFs.
  • the very large dt/dt.c is gone, it is now either moved to a new dt/dt_api.c or split into dt/dt_<ip>.c which makes more sense.

Signed-off-by: Amaury Pouly <amaury.pouly@lowrisc.org>
Co-authored-by: Alexander Williams <awill@opentitan.org>
@pamaury pamaury force-pushed the multitop_sw_dt2 branch 2 times, most recently from bfa997c to 43b9922 Compare January 24, 2025 14:50
Signed-off-by: Amaury Pouly <amaury.pouly@lowrisc.org>
@pamaury
Copy link
Contributor Author

pamaury commented Jan 24, 2025

So this PR is going to break Earlgrey whatever I do because the patching replaces top_earlgrey.h by the EB header but the DT headers are still generated for earlgrey and those use constants from top_earlgrey.h. We know that it is inevitable that EB breaks at some point and will need to be repaired when we can compile more code for EB.

I suggest we disable temporarily the EB CI tests for the time being? @vogelpi

@vogelpi
Copy link
Contributor

vogelpi commented Jan 24, 2025

So this PR is going to break Earlgrey whatever I do because the patching replaces top_earlgrey.h by the EB header but the DT headers are still generated for earlgrey and those use constants from top_earlgrey.h. We know that it is inevitable that EB breaks at some point and will need to be repaired when we can compile more code for EB.

I suggest we disable temporarily the EB CI tests for the time being? @vogelpi

SGTM, but let's make sure @a-will is in the loop as well as he used EB as a prototype, too.

static dif_aon_timer_t aon_timer;
static dt_aon_timer_t kAonTimerDt = kDtAonTimerAon;
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, these enums are still top-specific, since these are constructed from the instance names in the module section of the top hjson, right?

But we can adjust this whenever.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The most logical thing to do is usually to go over all instances of this IP and test them one by one. In this case, test each aon_timer (even though it's almost sure that there is only one). Alternatively, we could just test the first one (set the index to 0).

/**
* Description of instances.
*/
${helper.inst_struct.render()}
Copy link
Contributor

Choose a reason for hiding this comment

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

At first glance, it doesn't look like this enum has a unique namespace. I'd wager that name collisions are unlikely, but we might want to adjust this at some point. The suffix is controlled by the user's hjson file, so it's less controlled. (We could choose to provide naming guidelines instead, though.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, it's named dt_<something>_desc_t so technically could get a collision. We could alternatively name it dt_desc_<something>, this should be enough to avoid collisions. Side node: this type is local to the file so we could technically give it a fixed name, ie dt_this_ip_desc_t and it would also solve the problem.

@a-will
Copy link
Contributor

a-will commented Jan 24, 2025

So this PR is going to break Earlgrey whatever I do because the patching replaces top_earlgrey.h by the EB header but the DT headers are still generated for earlgrey and those use constants from top_earlgrey.h. We know that it is inevitable that EB breaks at some point and will need to be repaired when we can compile more code for EB.
I suggest we disable temporarily the EB CI tests for the time being? @vogelpi

SGTM, but let's make sure @a-will is in the loop as well as he used EB as a prototype, too.

Yes, this is fine. I hear @pamaury has already solved this in the multitop_dev branch, so it'll only be a temporary hiatus. 😄

@pamaury pamaury requested a review from rswarbrick as a code owner January 25, 2025 10:21
Signed-off-by: Amaury Pouly <amaury.pouly@lowrisc.org>
@jwnrt jwnrt dismissed their stale review January 25, 2025 12:36

stale

@pamaury pamaury merged commit ecd5b8c into lowRISC:master Jan 26, 2025
38 checks passed
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.

4 participants