[topgen] Generically filter for MMIO region visible devices

[Razer6]

Remove hardcoded devices and interfaces for MMIO region filtering by adding two new optional module attributes cpu_visible and cpu_not_visible.

By unifying the implementation of the C and Rust generator, an inconsistency between the two implementations was uncovered. Ideally, TopGenC and TopGenRust will be refactored with a common base class to remove as much of the code duplication. I'm not sure though if that is the right PR to fix that.

This fixes #14345

[rswarbrick]

It's a bit sad to have the names appearing twice. Could we switch it so that we end up with something like this?

      // Note that this module also contains a bus host.
      base_addrs: {
        mem:  {hart: "0x00040000", cpu_visible: false},
        regs: {hart: "0x21200000", cpu_visible: false},
        dbg:  {soc_dbg: "0x00000000", cpu_visible: false},
      },

and we could make it so a region has a default visibility attribute (true, presumably)

[Razer6]

I changed the implementation according to that. It was required to add some special cases when iterating over that struct. Should we add a top-level validation, to ensure that there is no address space named cpu_visible?

[a-will]

Would it make more sense to formalize memory region groups? The "MMIO region" is not a property of an individual IP instance. Instead, it's a contiguous grouping of nodes in the address space.

Validating memory region groups (...or "subspaces"? Not sure what would be a good name yet...) would mean ensuring that all nodes between the two ends are included in the named group, and we'd have to be explicit about each node that is intended to be in the group. topgen would need to error out if any node is missing, since that would be an inconsistent subspace.

I kind of feel like topgen is a dubious place to do this... but if we really want the internal check for minimizing ePMP entries, I guess it makes sense. If there's no DRC, then it might make more sense to kick this out to downstream software completely. Well... maybe it's also convenient to define regions with just the two ends and abandon the check /shrug

[a-will]

The name of this key and how it's used don't really match. We also shouldn't need a designation of whether a node is "visible" to a CPU, since that is indicated by the address space (or could be determined by the xbar config).

The "regs" are very much visible to the CPU, for example.

[Razer6]

That is totally right. This resulted from the fact, that everything except memories was included by default. I removed that base, and explicitly excluded memories from the MMIO range (except for the retention RAM).

[Razer6]

Would it make more sense to formalize memory region groups? The "MMIO region" is not a property of an individual IP instance. Instead, it's a contiguous grouping of nodes in the address space.

Validating memory region groups (...or "subspaces"? Not sure what would be a good name yet...) would mean ensuring that all nodes between the two ends are included in the named group, and we'd have to be explicit about each node that is intended to be in the group. topgen would need to error out if any node is missing, since that would be an inconsistent subspace.

Not sure, if that is in the scope of this PR. I guess we can support multiple MMIO regions in a different PR if needed.

I kind of feel like topgen is a dubious place to do this... but if we really want the internal check for minimizing ePMP entries, I guess it makes sense. If there's no DRC, then it might make more sense to kick this out to downstream software completely. Well... maybe it's also convenient to define regions with just the two ends and abandon the check /shrug

I'm not sure if downstream software actually uses those defines? If not, we maybe can remove that completely?

[a-will]

Would it make more sense to formalize memory region groups? The "MMIO region" is not a property of an individual IP instance. Instead, it's a contiguous grouping of nodes in the address space.
Validating memory region groups (...or "subspaces"? Not sure what would be a good name yet...) would mean ensuring that all nodes between the two ends are included in the named group, and we'd have to be explicit about each node that is intended to be in the group. topgen would need to error out if any node is missing, since that would be an inconsistent subspace.

Not sure, if that is in the scope of this PR. I guess we can support multiple MMIO regions in a different PR if needed.

This is exactly the scope of what you are modifying, though. The "MMIO region" is merely a macro defined in a software header. The code changed here has no relationship with the CPU's visibility of node.

[a-will]

I think we've got some misunderstandings of what the original code does here. It defines a contiguous region of the address map and calls it the "MMIO region" so software can program a single entry for it in the ePMP.

CPU visibility doesn't factor into it. Instead, there are hard-coded exceptions in topgen's output helpers that try to define the bounds of the region. If you want to remove those hard-coded exceptions, we would need to either explicitly define non-MMIO nodes for each addr_space, or we could generalize and define multiple such regions, with "MMIO" just being one of them.

[a-will]

Please no hard-coded address space name exceptions. This violates the data structure's layout.

[Razer6]

Exactly, I thought about that too. What about an address space validation as proposed in #23274 (comment) ?

Hit the wrong button, hehe

[a-will]

Whoops, meant to request changes on that last review. Doh!

[Razer6]

What about defining it like this:

addr_group defines the address range, the node is associated to. If nothing provided, it associates them to the default mmio group. If set to none or false, not sure what works, they will be excluded. The helper would then determine all addr_groups per address space generates the definitions for them.

base_addrs: {
        mem:  {hart: "0x00040000", addr_range: none},
        regs: {hart: "0x21200000", addr_range: "mmio"},
        dbg:  {soc_dbg: "0x00000000", addr_range: none},
      },

[a-will]

For the option where we generically define subspaces of an addr_space and explicitly include each node, it could look like this:

  addr_spaces: [
    { name: "hart",
      desc: "The main address space, shared between the CPU and DM"},
      subspaces: [
        { name: "mmio",
          desc: "The memory region for peripherals",
          nodes: [
            "uart0", "gpio", "spi_device", "i2c0", "rv_timer",
            "otp_ctrl", // Shorthand for all otp_ctrl nodes
            "lc_ctrl.regs", // Can select specific nodes with a dot separator
            "alert_handler", "spi_host0", "pwrmgr_aon",  // And so on
          ],
        },
      ],
    },
    { name: "soc_mbx", desc: "SoC address space for mailbox access"},
    { name: "soc_dbg", desc: "SoC address space for debug module interfaces"},
  ]

Actually, we could even choose to move all the base_addr definitions into this three-level tree, and that would make us have compatible definitions with IP-XACT (where a "subspace" is the same as an IP-XACT "memoryMap"). SystemRDL seems to allow nesting addrmap definitions, so it's a little more free. But anyway, that would look like this:

  addr_spaces: [
    { name: "hart",
      desc: "The main address space, shared between the CPU and DM"},
      subspaces: [
        { name: "mmio",
          desc: "The memory region for peripherals",
          nodes: {
            uart0:                "0x30010000",
            gpio:                 "0x30000000",
            spi_device:           "0x30310000",
            i2c0:                 "0x30080000",
            rv_timer:             "0x30100000",
            otp_ctrl.core:        "0x30130000",
            otp_ctrl.prim:        "0x30138000",
            lc_ctrl.regs:         "0x30140000",
            // And so on
          },
        },
      ],
      nodes: [ // Same as the "nodes" in the subspaces, but these are not in a named subspace.
      ],
    },

[Razer6]

Looks good to me. The first one seems easy to me. Whats quite attractive to the second one is to have a base address table basically on one screen. It makes it much more readable for the base addresses, which already annoyed me a few times. However, the changes are of course much more intrusive though.

[a-will]

Looks good to me. The first one seems easy to me. Whats quite attractive to the second one is to have a base address table basically on one screen. It makes it much more readable for the base addresses, which already annoyed me a few times. However, the changes are of course much more intrusive though.

Would it make sense to start with the first one? We could choose to move to the second one later.

The first solution annoyingly adds some burden to update yet another place in the file when you assign new base addresses. That might motivate us to eventually get to the second solution instead of forever punting it, though, haha.

[Razer6]

Would it make sense to start with the first one? We could choose to move to the second one later.

That sounds good. The second one is an improvement on top of it, but touches a lot of places.

The first solution annoyingly adds some burden to update yet another place in the file when you assign new base addresses. That might motivate us to eventually get to the second solution instead of forever punting it, though, haha.

Totally agree. And it really shows you the memory map in one place. Quite hard to scroll down if there are many more devices ;-)

[a-will]

Oh, this looks great!

[a-will]

Sorry you had to type all of that 😆

[Razer6]

Haha, no worries 😆

[a-will]

CHANGE AUTHORIZED: hw/top_darjeeling/data/top_darjeeling.hjson

[andreaskurth]

CHANGE AUTHORIZED: hw/top_darjeeling/data/top_darjeeling.hjson