[earlgrey,dv] Remove bogus dmi device from TopEarlgrey

rswarbrick · rswarbrick · commit fddaec322294 · 2026-03-29T17:56:08.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/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
@@ -64,7 +64,8 @@ package ${ral_id}_ral_pkg;
     block = top.blocks[block_name]
     for inst_name in inst_names:
       for if_name in block.reg_blocks:
-        if addr_space not in top.if_addrs[(inst_name, if_name)]: continue
+        asid_to_addr = top.if_addrs.get((inst_name, if_name), {})
+        if addr_space not in asid_to_addr: continue
         if_suffix = '' if if_name is None else '_' + if_name
         if_packages.add('{}{}_ral_pkg'.format(block_name.lower(), if_suffix))
   if_packages = sorted(if_packages)
@@ -86,7 +87,8 @@ ${make_ral_pkg_window_class(block_dv_base_names.mem, ral_id, window)}
 %     for inst_name in inst_names:
 %       for if_name, rb in block.reg_blocks.items():
 <%
-          if addr_space not in top.if_addrs[(inst_name, if_name)]: continue
+          asid_to_addr = top.if_addrs.get((inst_name, if_name), {})
+          if addr_space not in asid_to_addr: continue
           if_suffix = '' if if_name is None else '_' + if_name
           esc_if_name = block_name.lower() + if_suffix
           if_inst = inst_name + if_suffix
@@ -128,7 +130,8 @@ ${make_ral_pkg_window_class(block_dv_base_names.mem, ral_id, window)}
 %     for inst_name in inst_names:
 %       for if_name, rb in block.reg_blocks.items():
 <%
-          if addr_space not in top.if_addrs[(inst_name, if_name)]: continue
+          asid_to_addr = top.if_addrs.get((inst_name, if_name), {})
+          if addr_space not in asid_to_addr: continue
           if_suffix = '' if if_name is None else '_' + if_name
           esc_if_name = block_name.lower() + if_suffix
           if_inst = inst_name + if_suffix

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:`