csr: Proposal to accept CSRs with the same name owned by different submodules

[HarryMakes]

This PR is an attempt to tackle an issue where multiple CSRs with the same name are instantiated within different submodules, and then collected only at some ancestor at higher levels indirectly.

Background

Normally, a CSR should be made collectible by some CSR bank implementations (e.g. misoc.interconnect.csr_bus.CSRBankArray) by instantiating it as a member in an AutoCSR object that, itself, is added as a member of the CSR bank object. The implication is that a CSR should be instantiated within a named submodule of the source module of the bank. For example, in SoCCore implementations, the hierarchy of CSRs is normally like this:

class DummyAutoCSR(Module, AutoCSR):
    def __init__(self):
        self.dummy_csr = CSRStorage()

class DummyBaseSoc(SoCCore):
    def __init__(self, platform):
        SoCCore.__init__(self, platform, 100e6)
        # By naming the DummyAutoCSR submodule, when finalising SoCCore,
        # the submodule is absorbed as an attribute of the SoCCore, so that
        # the CSRBankArray can detect this AutoCSR object when scanning through
        # the list of attributes in the SoCCore object.
        self.submodules.dummy = DummyAutoCSR()
        # By appending to self.csr_devices, when finalising SoCCore, the CSRBankArray
        # can add the CSR device to the memory as SRAM, thus completing the 
        # memory-mapping procedure.
        self.csr_devices += ["dummy"]

Possible Scenario

The user of MiSoC might want to add a CSR several levels below a module representing its device, while still being able to allocate memory for the CSR. While the gatherer functions like get_csrs() and get_memories() can recursively collect CSRs, these functions require that the submodule at each sub-level must be named (i.e. using self.submodules.xyz = ...). If the user does not intend to name the submodule, these gatherers would fail. Consider the following scenario:

class DummyAutoCSR(Module, AutoCSR):
    def __init__(self):
        self.dummy_csr = CSRStorage()

class DummyDevice(Module, AutoCSR):
    def __init__(self):
        self.submodules.dummy = DummyAutoCSR()    # dummy_csr in this can be collected
        dummy_list = []
        for i in range(2):
            dummy = DummyAutoCSR()
            self.submodule += dummy               # dummy_csr's in these 2 cannot be collected
            dummy_list.append(dummy)
        self.special_csr = dummies[0].dummy_csr   # user attempts to add one of the 
                                                  # unnamed dummies for collection

class DummyBaseSoc(SoCCore):
    def __init__(self, platform):
        SoCCore.__init__(self, platform, 100e6)
        self.submodules.dummy = DummyDevice()
        self.csr_devices += ["dummy"]

This code would force the gatherer functions to collect the underlying CSR in dummies[0]. However, the current implementation of the gatherers would only add a unique prefix to the CSR name (e.g. for the bank on the top level) if its owner is a named submodule. Thus, while the CSR referred by dummy.dummy_csr would be renamed as dummy_dummy_csr, the CSR referred by special_csr would simply be called dummy_csr by the gatherer. Consider adding the following line to DummyDevice:

        self.another_special_csr = dummies[1].dummy_csr

Instead of another_special_csr, the gatherer would call it dummy_csr without prefixing. Now, there will be two dummy_csr in the list of the CSR collection, which would result in name duplication error when misoc.integration.builder is generating the csr.h or csr.rs for the SoCCore.

Solution

While I am unsure whether or not the user should be prohibited from adding CSRs to the collection in this manner, there should be a way to have the gatherer functions uniquely naming the CSRs. My proposal is to make these gatherers scan through all the submodules to check if the CSR is owned by one (or some submodule at an even lower level). If so, the original attribute name assigned as a reference to the CSR at the current module level will be used as a unique identifier. This means for the above code, the get_csrs() gatherer called on the DummyDevice object would return the following list of CSR names: ["dummy_dummy_csr", "special_csr", "another_special_csr"]. An example of the generated csr.rs would now contain the following:

pub mod csr {
  pub const DUMMY_BASE: *mut u32 = ...... as *mut u32;

  pub mod dummy {
    #[allow(unused_imports)]
    use core::ptr::{read_volatile, write_volatile};

    pub const DUMMY_DUMMY_CSR_ADDR: *mut u32 = ...... as *mut u32;
    pub const DUMMY_DUMMY_CSR_SIZE: usize = 1;

    #[inline(always)]
    pub unsafe fn dummy_dummy_csr_read() -> u8 {
      read_volatile(DUMMY_DUMMY_CSR_ADDR) as u8
    }

    #[inline(always)]
    pub unsafe fn dummy_dummy_csr_write(w: u8) {
      write_volatile(DUMMY_DUMMY_CSR_ADDR.offset(0), (w) as u32);
    }

    pub const SPECIAL_CSR_ADDR: *mut u32 = ...... as *mut u32;
    pub const SPECIAL_CSR_SIZE: usize = 1;

    #[inline(always)]
    pub unsafe fn special_csr_read() -> u8 {
      read_volatile(SPECIAL_CSR_ADDR) as u8
    }

    #[inline(always)]
    pub unsafe fn special_csr_write(w: u8) {
      write_volatile(SPECIAL_CSR_ADDR.offset(0), (w) as u32);
    }

    pub const ANOTHER_SPECIAL_CSR_ADDR: *mut u32 = ...... as *mut u32;
    pub const ANOTHER_SPECIAL_CSR_SIZE: usize = 1;

    #[inline(always)]
    pub unsafe fn another_special_csr_read() -> u8 {
      read_volatile(ANOTHER_SPECIAL_CSR_ADDR) as u8
    }

    #[inline(always)]
    pub unsafe fn another_special_csr_write(w: u8) {
      write_volatile(ANOTHER_SPECIAL_CSR_ADDR.offset(0), (w) as u32);
    }
  }
}

[sbourdeauducq]

It would become difficult to determine which module a CSR belong to, and this seems prone to errors IMO.

I suggest doing things explicitly:

        for i in range(2):
            dummy = DummyAutoCSR()
            setattr(self.submodules, "dummy_{}".format(i), dummy)

[sbourdeauducq]

And instead of using AutoCSR, it is also possible to implement get_csrs manually, e.g.

def get_csrs(self):
  return self.submodule1.get_csrs() + self.submodule2.get_csrs()

[HarryMakes]

Yes, anyone who use MiSoC should be careful with their CSRs and understand how each helper method is currently implemented. Either by adding each AutoCSR object as a named submodule, or overriding get_csrs() should suffice to avoid this kind of CSR name conflicts.

That said, what I'm proposing is assuming that the user doesn't know deeply how our SoC builder works with CSR structures, and hence defining objects in an unexpected way while still passing the Python compile-time requirements. If my assumption is valid, then there should be a way to either stop the user from doing so, or adopt this kind of CSR instantiation as a new feature to bring CSRs from deeper levels to the current module level without setting attributes to self.submodules.

If my assumption doesn't really hold, then perhaps my proposal isn't helpful, and I'd better work on the documentations instead. :thinking: