[BUG] [QUESTION] - Importer API support for signals

[benoitdenkinger]

Describe the bug

Dear systemrdl-compiler community,
I'm encountering what I believe is a bug. The RDLImporter class as a method called add_child() but it does not support adding signals to an addrmap because the method checks that the child is an AddressableComponent.

def add_child(self, parent: comp.Component, child: comp.Component) -> None:
        """
        Add a child component instance to an existing parent definition or instance

        .. versionadded:: 1.16
        """
        bad = True
        if isinstance(parent, comp.Addrmap):
            if isinstance(child, comp.AddressableComponent):
                bad = False
        elif isinstance(parent, comp.Regfile):
            if isinstance(child, (comp.Regfile, comp.Reg)):
                bad = False
        elif isinstance(parent, comp.Mem):
            if isinstance(child, comp.Reg):
                bad = False
        elif isinstance(parent, comp.Reg):
            if isinstance(child, comp.Field):
                bad = False
        if bad:
            raise TypeError("Parent %s cannot be assigned child %s" % (repr(parent), repr(child)))

        if not child.is_instance:
            raise ValueError("Child must be an instance if adding to a parent")

        parent.children.append(child)

On the other hand, the Node class as a method called children() (which calls the _factory() method below) which returns signal components:

def _factory(inst: comp.Component, env: 'RDLEnvironment', parent: Optional['Node']=None) -> 'Node':
        if isinstance(inst, comp.Field):
            return FieldNode(inst, env, parent)
        elif isinstance(inst, comp.Reg):
            return RegNode(inst, env, parent)
        elif isinstance(inst, comp.Regfile):
            return RegfileNode(inst, env, parent)
        elif isinstance(inst, comp.Addrmap):
            return AddrmapNode(inst, env, parent)
        elif isinstance(inst, comp.Mem):
            return MemNode(inst, env, parent)
        elif isinstance(inst, comp.Signal):
            return SignalNode(inst, env, parent)
        else:
            raise RuntimeError

systemrdl-compiler version: 1.27.3

Additional context
I'm converting an hjson description from the opentitan project to systemrdl and I want to add signal inside an addrmap node. Currently I'm overloading the add_child method to change the behavior with the one I want, but to me it seems the original method should provide this possibility.

[amykyta3]

Currently the import API doesn't support the full set of SystemRDL concepts - notably references. Something I hope to improve in the future but given some competing projects, I don't have the bandwidth to work on it.

I'm curious - what type of register structure are you attempting to import that requires a signal? I ask in case there is an alternate way to describe it that does not require signals. Personally, I have not found the need to use signals all too often, so I want to make sure we're not missing something.

[benoitdenkinger]

Hello,

Thanks for the reply. I'm using SystemRDL to describe full systems (i.e., multiple IP blocks interconnected). In this scenario, SystemRDL is used to define an IP block and signals are used to specify the inputs and outputs of the IP. It is similar to what the opentitan project is doing but using hjson files and a set of python scripts to generate register files and other more higher level modules (e.g., a complete IP block with IOs, interrupts). I'm taking their hjson file and translate it to SystemRDL. These signals are usually external wires used to generate connections or anything else. In a hierarchical description of a complete system, signals are very useful to connect different modules together.

The change is quite simple to do (if it does not break anything else), so if this is of interest I'll be happy to create a merge request.