[topgen] Improve support for multiple address spaces #22961
Conversation
Razer6
requested review from
timothytrippel,
a-will and
andreaskurth
and removed request for
a team
util/topgen/c.py
Outdated
| @@ -519,21 +517,49 @@ def _init_mmio_region(self): | |||
| space, i.e. ROM, main SRAM, and flash are excluded but retention SRAM, | |||
| spi_device memory, or usbdev memory are included. | |||
| """ | |||
There was a problem hiding this comment.
That code should be factored out and merged with the same rust code.
Razer6
added
Component:Tooling
Razer6
force-pushed
the
multiple-addr-space
branch
from
cc2f56f to
a431268
Compare
| @@ -687,7 +688,8 @@ | |||
| banks: 2, | |||
| pages_per_bank: 256, | |||
| program_resolution: 8, // maximum number of flash words allowed to program at one time | |||
| } | |||
| }, | |||
| addr_space: "hart" | |||
There was a problem hiding this comment.
This doesn't look right to me. This memory section isn't describing addresses--It describes properties of the named "mem" region above, which already describes the connected address spaces in the base_addrs dictionary.
Instead of specifying the addr_space here, can we do a lookup in the relevant generators to get the needed info?
There was a problem hiding this comment.
That makes totally sense. I removed the new entry and use the base address field to determine the address space the memory is in.
util/topgen/c.py
Outdated
| # TODO: Don't hardcode the address space used for software. | ||
| self.addr_space = "hart" | ||
|
|
There was a problem hiding this comment.
I think there needs to be an address space selected somewhere, but it could be provided as an argument to the generator object's constructor. The header file generated is not intended to spit out macros for more than one address space at a time.
We could possibly consider doing that, but then the macro names would need to start specifying their address spaces, and it's a lot of irrelevant info for the compilation target, right?
Or does not having the address space in the macro name end up causing issues when one CPU actually connects to one node's multiple address spaces through translation layers? (In the SoC context, the CPU might need multiple headers, one for each address space, and that might be fine only if there aren't any name clashes.)
util/topgen/c.py
Outdated
| int(m["size"], 0)))) | ||
| int(m["base_addr"][m["addr_space"]], 0), | ||
| int(m["size"], 0))), | ||
| m["addr_space"]) |
There was a problem hiding this comment.
Because TopGenC should be generating for a specific addr space, this could just be...
| m["addr_space"]) | |
| self.addr_space) |
and we should keep that as a member field.
Razer6
force-pushed
the
multiple-addr-space
branch
4 times, most recently
from
571e8ca to
49b6e37
Compare
|
It might help if I could understand the desired feature set and where topgen currently falls short. For this case of "multiple CPUs within the OpenTitan complex," what sort of topology are we talking about for the interconnect? Do each of these CPUs actually have different maps? How should the C and Rust output look when there are multiple address spaces of interest? Do we have 1 file per address space or a single file that puts all address spaces together? If it's the former, do we need the name of the address space to be part of the identifier? For a CPU inside the OpenTitan complex, I'd think each CPU would only need the one address space (i.e. the one attached to its host node in the xbar definition). Are there cases where a CPU would bridge into other address spaces? |
Right now, I envision multiple CPUs that have their own maps but share common infrastructure from the RoT CPU. For example, the RoT Ibex has one alert manager connected and is in charge of configuring it. IPs from other maps are attached to this one alert manager. There might be a shared bus, connecting the current egress ports of Darjeeling and a second CPU, as well as some shared IPs.
Currently, I used a single TopgGenC call to generate a common C/Rust output for all address spaces. This is the main shortcoming of topgen right now. However, the more I think about it, it may make more sense to perform multiple TopGenC calls for every address space, generating individual C and Rust outputs for every address space. It would leave the outputs not polluted with outputs from other address spaces. But I think FW teams should maybe chime in here?
Currently, we think the same. The CPU needs only one address space, which is orthogonal to other CPUs. |
So in this case, the shared infrastructure may be at different addresses vs. the RoT CPU, I suppose. That's fine, but that might mean we'll want them to have generated output that is tailored to their specific maps. And that leads us to the next bit... :)
Hehe, I'm also part of the FW team, but agreed on all points. I figure it would be best to generate a different file for each address space. For a given CPU, it should only use the file that is for its own address space. Ultimately, if a CPU has a different map, then it can't be treated as the exact same compilation target--We can hide the differences behind a library interface and avoid duplicating an entire build graph, though! If possible, it'd be nice to not need the address space name to be part of the C macro name, to need to disambiguate between address spaces. As long as there is no bridging across spaces, I think that should work fine, assuming I'm not missing something. Rust's namespaces are nicer to work with than C's one, big, global namespace, so it might matter less there.
Thanks for clarifying! |
Individually generate C and Rust collateral for all address spaces Signed-off-by: Robert Schilling <[email protected]>
Razer6
force-pushed
the
multiple-addr-space
branch
from
49b6e37 to
821e459
Compare
|
Closing as superseded by #23122 |
I'm working towards adding support for multiple CPUs within the OpenTitan complex. With this change,
topgensupports MMIO regions in multiple address spaces. It requires associating each memory with an address space, which is now required to be defined in the top-level HJSON definition.Further,
topgengenerates defines for the peripheral interface for all address spaces. For Darjeeling, that means the SOC-facing interfaces, e.g., the SOC port of the mailbox, get their base address definitions and the size of the address window.It further fixes a bug to correctly compute
TOP_DARJEELING_MMIO_SIZE_BYTESfor the rust definitions, which are the same now as the C definitions. In general, the code here is duplicated. I suggest factoring that out and follow a DRY principle.