[earlgrey,dv] Remove bogus dmi device from TopEarlgrey

rswarbrick · rswarbrick · commit 9a296996e76c · 2026-03-29T20:04:32.000+01:00
This was added for Darjeeling and it appears the engineers concerned
didn't think about the fact this would convince various bits of
tooling that the phantom interface exists for Earlgrey too.

The bulk of this commit is tidying up the logic in topgen's lib.py,
merge.py and top_uvm_reg.sv.tpl to allow the possibility that an
instance of a block doesn't connect up all the interfaces that the
block defines. To do this properly, it also fixes some incorrect type
annotations in the code (which seem to have been introduced when first
adding multiple address maps).

The Earlgrey-specific change is tiny: it's just remove the "dmi" line
in hw/top_earlgrey/data/top_earlgrey.hjson, together with the comment
that shows the author realises it didn't make much sense...

Signed-off-by: Rupert Swarbrick &lt;rswarbrick@lowrisc.org&gt;
diff --git a/hw/top_earlgrey/data/autogen/top_earlgrey.gen.hjson b/hw/top_earlgrey/data/autogen/top_earlgrey.gen.hjson
@@ -2424,10 +2424,6 @@
         {
           hart: 0x40140000
         }
-        dmi:
-        {
-          hart: 0x0
-        }
       }
       param_decl:
       {
diff --git a/hw/top_earlgrey/data/autogen/top_earlgrey.secrets.testing.gen.hjson b/hw/top_earlgrey/data/autogen/top_earlgrey.secrets.testing.gen.hjson
@@ -2873,10 +2873,6 @@
         {
           hart: 0x40140000
         }
-        dmi:
-        {
-          hart: 0x0
-        }
       }
       memory: {}
       param_list:
diff --git a/hw/top_earlgrey/data/top_earlgrey.hjson b/hw/top_earlgrey/data/top_earlgrey.hjson
@@ -453,7 +453,6 @@
       reset_connections: {rst_ni: "lc_io_div4", rst_kmac_ni: "lc"},
       base_addrs: {
         regs: {hart: "0x40140000"},
-        dmi:  {hart: "0x0"}, // DMI is not used in EarlGrey
       },
       param_decl: {
         // NOTE THAT THIS IS A FEATURE FOR TEST CHIPS ONLY TO MITIGATE
diff --git a/hw/top_earlgrey/doc/memory_map.md b/hw/top_earlgrey/doc/memory_map.md
@@ -28,7 +28,6 @@ The main address space, shared between the CPU and DM
 | otp_ctrl          | core        | `0x40130000`   | `0x1000`       | `0x400`        | core device on otp_ctrl          |
 | otp_macro         | prim        | `0x40138000`   | `0x20`         | `0x8`          | prim device on otp_macro         |
 | lc_ctrl           | regs        | `0x40140000`   | `0x100`        | `0x40`         | regs device on lc_ctrl           |
-| lc_ctrl           | dmi         | `0x0`          | `0x1000`       | `0x400`        | dmi device on lc_ctrl            |
 | alert_handler     | default     | `0x40150000`   | `0x800`        | `0x200`        | alert_handler                    |
 | spi_host0         | default     | `0x40300000`   | `0x40`         | `0x10`         | spi_host0                        |
 | spi_host1         | default     | `0x40310000`   | `0x40`         | `0x10`         | spi_host1                        |
diff --git a/hw/top_earlgrey/rtl/autogen/top_earlgrey_pkg.sv b/hw/top_earlgrey/rtl/autogen/top_earlgrey_pkg.sv
@@ -149,16 +149,6 @@ package top_earlgrey_pkg;
    */
   parameter int unsigned TOP_EARLGREY_LC_CTRL_REGS_SIZE_BYTES = 32'h100;
 
-  /**
-   * Peripheral base address for dmi device on lc_ctrl in top earlgrey.
-   */
-  parameter int unsigned TOP_EARLGREY_LC_CTRL_DMI_BASE_ADDR = 32'h0;
-
-  /**
-   * Peripheral size in bytes for dmi device on lc_ctrl in top earlgrey.
-   */
-  parameter int unsigned TOP_EARLGREY_LC_CTRL_DMI_SIZE_BYTES = 32'h1000;
-
   /**
    * Peripheral base address for alert_handler in top earlgrey.
    */
diff --git a/hw/top_earlgrey/sw/autogen/chip/top_earlgrey.rs b/hw/top_earlgrey/sw/autogen/chip/top_earlgrey.rs
@@ -217,20 +217,6 @@ pub const LC_CTRL_REGS_BASE_ADDR: usize = 0x40140000;
 /// `LC_CTRL_REGS_BASE_ADDR + LC_CTRL_REGS_SIZE_BYTES`.
 pub const LC_CTRL_REGS_SIZE_BYTES: usize = 0x100;
 
-/// Peripheral base address for dmi device on lc_ctrl in top earlgrey.
-///
-/// This should be used with #mmio_region_from_addr to access the memory-mapped
-/// registers associated with the peripheral (usually via a DIF).
-pub const LC_CTRL_DMI_BASE_ADDR: usize = 0x0;
-
-/// Peripheral size for dmi device on lc_ctrl in top earlgrey.
-///
-/// This is the size (in bytes) of the peripheral's reserved memory area. All
-/// memory-mapped registers associated with this peripheral should have an
-/// address between #LC_CTRL_DMI_BASE_ADDR and
-/// `LC_CTRL_DMI_BASE_ADDR + LC_CTRL_DMI_SIZE_BYTES`.
-pub const LC_CTRL_DMI_SIZE_BYTES: usize = 0x1000;
-
 /// Peripheral base address for alert_handler in top earlgrey.
 ///
 /// This should be used with #mmio_region_from_addr to access the memory-mapped
diff --git a/hw/top_earlgrey/sw/autogen/top_earlgrey.h b/hw/top_earlgrey/sw/autogen/top_earlgrey.h
@@ -282,24 +282,6 @@ extern "C" {
  */
 #define TOP_EARLGREY_LC_CTRL_REGS_SIZE_BYTES 0x100u
 
-/**
- * Peripheral base address for dmi device on lc_ctrl in top earlgrey.
- *
- * This should be used with #mmio_region_from_addr to access the memory-mapped
- * registers associated with the peripheral (usually via a DIF).
- */
-#define TOP_EARLGREY_LC_CTRL_DMI_BASE_ADDR 0x0u
-
-/**
- * Peripheral size for dmi device on lc_ctrl in top earlgrey.
- *
- * This is the size (in bytes) of the peripheral's reserved memory area. All
- * memory-mapped registers associated with this peripheral should have an
- * address between #TOP_EARLGREY_LC_CTRL_DMI_BASE_ADDR and
- * `TOP_EARLGREY_LC_CTRL_DMI_BASE_ADDR + TOP_EARLGREY_LC_CTRL_DMI_SIZE_BYTES`.
- */
-#define TOP_EARLGREY_LC_CTRL_DMI_SIZE_BYTES 0x1000u
-
 /**
  * Peripheral base address for alert_handler in top earlgrey.
  *
diff --git a/hw/top_earlgrey/sw/autogen/top_earlgrey_memory.h b/hw/top_earlgrey/sw/autogen/top_earlgrey_memory.h
@@ -304,23 +304,6 @@
  * `TOP_EARLGREY_LC_CTRL_REGS_BASE_ADDR + TOP_EARLGREY_LC_CTRL_REGS_SIZE_BYTES`.
  */
 #define TOP_EARLGREY_LC_CTRL_REGS_SIZE_BYTES 0x100
-/**
- * Peripheral base address for dmi device on lc_ctrl in top earlgrey.
- *
- * This should be used with #mmio_region_from_addr to access the memory-mapped
- * registers associated with the peripheral (usually via a DIF).
- */
-#define TOP_EARLGREY_LC_CTRL_DMI_BASE_ADDR 0x0
-
-/**
- * Peripheral size for dmi device on lc_ctrl in top earlgrey.
- *
- * This is the size (in bytes) of the peripheral's reserved memory area. All
- * memory-mapped registers associated with this peripheral should have an
- * address between #TOP_EARLGREY_LC_CTRL_DMI_BASE_ADDR and
- * `TOP_EARLGREY_LC_CTRL_DMI_BASE_ADDR + TOP_EARLGREY_LC_CTRL_DMI_SIZE_BYTES`.
- */
-#define TOP_EARLGREY_LC_CTRL_DMI_SIZE_BYTES 0x1000
 /**
  * Peripheral base address for alert_handler in top earlgrey.
  *
diff --git a/util/dtgen/helper.py b/util/dtgen/helper.py
@@ -1066,25 +1066,55 @@ def _create_instance(self, m):
                     rb = self.UNNAMED_REG_BLOCK_NAME
                     rb_key = "null"  # Due to json serializing, None appears as null.
                 rb = Name.from_snake_case(rb)
-                # It is possible that this module is not accessible in this
-                # address space. In this case, return a dummy value.
-                # FIXME Maybe find a better way of doing this.
-                assert rb_key in m["base_addrs"]
-                reg_block_map[rb] = m["base_addrs"][rb_key].get(self._addr_space, "0xffffffff")
+
+                # The register block may not actually be accessible in this
+                # address space. Indeed, the top-level might not connect up the
+                # register block at all, because it also passes a parameter
+                # value that turns off the interface that this address space
+                # would be using.
+                #
+                # If so, use the same behaviour here: don't add the block to
+                # reg_block_map.
+                asid_to_base_addr = m["base_addrs"].get(rb_key)
+                if asid_to_base_addr is not None:
+                    reg_block_map[rb] = asid_to_base_addr.get(self._addr_space,
+                                                              "0xffffffff")
+
             inst_desc[self.REG_BLOCK_ADDR_FIELD_NAME] = reg_block_map
+
         # Memories.
         if self.has_memories():
             mem_addr_map = OrderedDict()
             mem_size_map = OrderedDict()
             for mem in self.ip.memories.keys():
                 mem_name = Name.from_snake_case(mem)
-                # It is possible that this module is not accessible in this
-                # address space. In this case, return a dummy value.
-                # FIXME Maybe find a better way of doing this.
-                assert mem in m["base_addrs"]
-                mem_addr_map[mem_name] = m["base_addrs"][mem].get(self._addr_space, "0xffffffff")
-                assert mem in m["memory"] and "size" in m["memory"][mem]
-                mem_size_map[mem_name] = m["memory"][mem]["size"]
+
+                # As with register blocks, the memory might not be accessible
+                # in this address space. As before, don't add the memory to
+                # mem_addr_map and mem_size_map.
+                #
+                # Note that we *do* expect m["memory"][mem] to exist if the
+                # memory is accessible. If it doesn't, the configuration
+                # requested a connection for a memory without describing what
+                # was connected.
+                asid_to_base_addr = m["base_addrs"].get(mem)
+                if asid_to_base_addr is not None:
+                    mem_addr_map[mem_name] = asid_to_base_addr.get(self._addr_space, "0xffffffff")
+
+                    memory_mem = m["memory"].get(mem)
+                    if memory_mem is None:
+                        raise RuntimeError(f"Connection described for memory "
+                                           f"{mem_name}, but the memory has "
+                                           f"no description.")
+
+                    memory_mem_size = memory_mem.get("size")
+                    if memory_mem_size is None:
+                        raise RuntimeError(f"The memory {mem_name} has a "
+                                           f"connection described, but the "
+                                           f"size is not defined.")
+
+                    mem_size_map[mem_name] = memory_mem_size
+
             inst_desc[self.MEM_ADDR_FIELD_NAME] = mem_addr_map
             inst_desc[self.MEM_SIZE_FIELD_NAME] = mem_size_map
         # Clock map.
diff --git a/util/topgen.py b/util/topgen.py
@@ -856,10 +856,12 @@ def generate_top_ral(topname: str, top: ConfigT, name_to_block: IpBlocksT,
 
         inst_to_block[inst_name] = block_name
         for if_name in block.reg_blocks.keys():
-            if_addr = {
-                asid: int(addr, 0)
-                for (asid, addr) in module["base_addrs"][if_name].items()
-            }
+            base_addrs = module["base_addrs"].get(if_name)
+            if base_addrs is None:
+                continue
+
+            if_addr = {asid: int(addr, 0)
+                       for (asid, addr) in base_addrs.items()}
             if_addrs[(inst_name, if_name)] = if_addr
 
     # Top-level may override the mem setting. Store the new type to
diff --git a/util/topgen/lib.py b/util/topgen/lib.py
@@ -721,11 +721,29 @@ def is_inst(module: ConfigT) -> bool:
     return top_level_mem or top_level_module
 
 
-def get_base_and_size(name_to_block: IpBlocksT, inst: ConfigT,
-                      ifname: Optional[str]) -> Tuple[int, int]:
+def get_base_and_size(block: IpBlock,
+                      inst: ConfigT,
+                      ifname: Optional[str]
+                      ) -> Tuple[Dict[str, int], int] | None:
+    '''Return (base_addrs, size) to describe addresses inst uses on ifname.
+
+    It may be that this instance doesn't actually use the interface at all in
+    the top-level (perhaps because its support for the interface is disabled by
+    a parameter). If so, return None.
+
+    If the instance *does* use the interface, base_addrs is a map from address
+    space ID to the base address of the block in that address space.
+
+    size is the size in bytes that the block uses on the interface named
+    ifname.
+    '''
+
+    # Check whether the top-level connects up this interface. If not, return
+    # None.
+    base_addrs = inst['base_addrs'].get(ifname)
+    if base_addrs is None:
+        return None
 
-    block = name_to_block.get(inst['type'])
-    assert block, f"No module named {inst['type']} (coming from instance {inst['name']})"
     # If inst is the instantiation of some block, find the register block
     # that corresponds to ifname
     if rb := block.reg_blocks.get(ifname):
@@ -739,7 +757,7 @@ def get_base_and_size(name_to_block: IpBlocksT, inst: ConfigT,
                 'default' if ifname is None else repr(ifname),
                 inst['name'], block.name))
 
-    base_addrs = deepcopy(inst['base_addrs'][ifname])
+    base_addrs = deepcopy(base_addrs)
 
     for (asid, base_addr) in base_addrs.items():
         if isinstance(base_addr, str):
@@ -981,19 +999,28 @@ def _init_device_regions(self, addr_space):
         for inst in self.top['module']:
             block = self._name_to_block[inst['type']]
             for if_name in block.reg_blocks.keys():
+                bases_size = get_base_and_size(block, inst, if_name)
+                if bases_size is None:
+                    # Nothing to do for this interface: it is not used in this
+                    # instance.
+                    continue
+
+                bases, size = bases_size
+                base = bases.get(addr_space)
+                if base is None:
+                    # Again, nothing to do for this interface: it does not
+                    # define anything mapped into addr_space.
+                    continue
+
                 full_if = (inst['name'], if_name)
                 full_if_name = Name.from_snake_case(full_if[0])
                 if if_name is not None:
                     full_if_name += Name.from_snake_case(if_name)
 
                 name = full_if_name
-                base, size = get_base_and_size(self._name_to_block, inst,
-                                               if_name)
-                if addr_space not in base:
-                    continue
 
                 region = MemoryRegion(self._top_name, name, addr_space,
-                                      base[addr_space], size)
+                                      base, size)
                 device_region[inst['name']].update({if_name: region})
 
         self.device_regions[addr_space] = device_region
@@ -1039,19 +1066,36 @@ def _init_device_memories(self, addr_space):
         device_memories = defaultdict(dict)
 
         for inst in self.top['module']:
-            if "memory" in inst:
-                for if_name, val in inst["memory"].items():
-                    base, size = get_base_and_size(self._name_to_block, inst,
-                                                   if_name)
-                    if addr_space not in base:
-                        continue
-
-                    full_if_name = Name.from_snake_case(inst['name']) + \
-                        Name.from_snake_case(if_name)
-                    region = MemoryRegion(self._top_name, full_if_name, addr_space,
-                                          base[addr_space], size)
-
-                    device_memories[inst['name']].update({if_name: region})
+            block = self._name_to_block.get(inst['type'])
+            if block is None:
+                raise RuntimeError(f"No block defined for instance "
+                                   f"type {inst['type']} (with name "
+                                   f"{inst['name']})")
+
+            for if_name, val in inst.get("memory", {}).items():
+                bases_size = get_base_and_size(block, inst, if_name)
+
+                # We expect bases_size not to be None. If it is None, then the
+                # instance defines the memory but doesn't give it a base
+                # address (which seems unlikely to be right).
+                if bases_size is None:
+                    raise RuntimeError(f"The instance named {inst['name']} "
+                                       f"defines a memory for interface "
+                                       f"{if_name}, but doesn't give it a "
+                                       f"base address.")
+
+                bases, size = bases_size
+                base = bases.get(addr_space)
+
+                if base is None:
+                    continue
+
+                full_if_name = Name.from_snake_case(inst['name']) + \
+                    Name.from_snake_case(if_name)
+                region = MemoryRegion(self._top_name, full_if_name, addr_space,
+                                      base, size)
+
+                device_memories[inst['name']].update({if_name: region})
 
         self.device_memories[addr_space] = device_memories
 
diff --git a/util/topgen/merge.py b/util/topgen/merge.py
@@ -245,16 +245,21 @@ def elaborate_instance(instance, block: IpBlock):
             log.error(f'Instance {instance["name"]} has both a base_addr '
                       'and a base_addrs field.')
 
-        # Since the instance already has a base_addrs field, make sure that
-        # it's got the same set of keys as the name of the interfaces in the
-        # block.
+        # The base_addrs field for the instance might not have an entry for
+        # every one of the block's interfaces: some of those interfaces might
+        # be disabled through a parameter and thus we don't want to wire them
+        # up.
+        #
+        # But we do expect that every interface that *is* given a base address
+        # has an associated reg_block or memory in the block.
         inst_if_names = set(base_addrs.keys())
         block_if_names = set(block.reg_blocks.keys()) | set(block.memories.keys())
-        if block_if_names != inst_if_names:
-            log.error('Instance {!r} has a base_addrs field with keys {} '
-                      'but the block it instantiates ({!r}) has device '
-                      'interfaces {}.'.format(instance['name'], inst_if_names,
-                                              block.name, block_if_names))
+
+        if inst_if_names - block_if_names:
+            log.error(f"Instance {instance['name']} has a base_addrs field "
+                      f"that defines base addresses for interfaces not named "
+                      f"in the block itself. Extra items: "
+                      f"{list(inst_if_names - block_if_names)}")
 
     if 'base_addr' in instance:
         del instance['base_addr']
@@ -436,8 +441,21 @@ def xbar_adddevice(top: ConfigT, name_to_block: IpBlocksT, xbar: ConfigT,
     # If we get here, inst points an instance of some block or memory. It
     # shouldn't point at a crossbar (because that would imply a naming clash)
     assert device_base not in other_xbars
-    base_addrs, size_byte = lib.get_base_and_size(name_to_block, inst,
-                                                  device_ifname)
+
+    block = name_to_block.get(inst['type'])
+    if block is None:
+        raise RuntimeError(f"No block defined for instance "
+                           f"type {inst['type']} (with name "
+                           f"{inst['name']})")
+
+    bases_size = lib.get_base_and_size(block, inst, device_ifname)
+    if bases_size is None:
+        # If get_base_and_size returned None, that means the instance doesn't
+        # actually connect with this interface and thus doesn't connect to this
+        # crossbar. Nothing more to do.
+        return
+
+    base_addrs, size_byte = bases_size
     addr_range = {
         "base_addrs":
         {asid: hex(base_addr)
diff --git a/util/topgen/top_uvm_reg.sv.tpl b/util/topgen/top_uvm_reg.sv.tpl

Original file line number	Diff line number	Diff line change
`@@ -2424,10 +2424,6 @@`
`2424`	`2424`	`{`
`2425`	`2425`	`hart: 0x40140000`
`2426`	`2426`	`}`
`2427`		`- dmi:`
`2428`		`- {`
`2429`		`- hart: 0x0`
`2430`		`- }`
`2431`	`2427`	`}`
`2432`	`2428`	`param_decl:`
`2433`	`2429`	`{`
Original file line number	Diff line number	Diff line change
`@@ -2873,10 +2873,6 @@`
`2873`	`2873`	`{`
`2874`	`2874`	`hart: 0x40140000`
`2875`	`2875`	`}`
`2876`		`- dmi:`
`2877`		`- {`
`2878`		`- hart: 0x0`
`2879`		`- }`
`2880`	`2876`	`}`
`2881`	`2877`	`memory: {}`
`2882`	`2878`	`param_list:`