Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handling resource name collisions inside a MemoryMap #20

Closed
jfng opened this issue Jun 29, 2020 · 1 comment · Fixed by #27
Closed

Handling resource name collisions inside a MemoryMap #20

jfng opened this issue Jun 29, 2020 · 1 comment · Fixed by #27
Labels
bug

Comments

@jfng
Copy link
Member

jfng commented Jun 29, 2020

Dumping CSR names from a MemoryMap by iterating over .all_resources() is subject to name collisions.
These can happen in two scenarios:

  1. registers in separate windows share the same name
class X:
    def __init__(self):
        foo = csr.Element(1, "r")
        mux = csr.Multiplexer(addr_width=1, data_width=8)
        mux.add(foo)
        self.bus = mux.bus

a = X()
b = X()
decoder = csr.Decoder(addr_width=2, data_width=8)
decoder.add(a.bus)
decoder.add(b.bus)

for elem, elem_range in decoder.bus.memory_map.all_resources():
    print(elem.name, elem_range)

# output:
# foo (0, 1)
# foo (1, 2)

In this case, we could add a name attribute to MemoryMap. Each window would then have a namespace for its resources. Resolving the full resource name would be the responsibility of the BSP generator, by walking through the memory hierarchy.
The result could look like this :

class X:
    def __init__(self, *, name=None, src_loc_at=0):
        foo = csr.Element(1, "r")
        mux = csr.Multiplexer(addr_width=1, data_width=8, name=name, src_loc_at=1 + src_loc_at)
        mux.add(foo)
        self.bus = mux.bus
a = X()
b = X()
  1. registers in the same window share the same name
class Y:
    def __init__(self):
        self.foo = csr.Element(1, "r")

c = Y()
d = Y()
mux = csr.Multiplexer(addr_width=1, data_width=8)
mux.add(c.foo)
mux.add(d.foo)

In this case, I don't see an easy way to disambiguate the two names. The BSP generator would have to detect this and throw an error.
@whitequark
Copy link
Member

In this case, we could add a name attribute to MemoryMap. Each window would then have a namespace for its resources.

Sounds good--I have missed this somehow.

The BSP generator would have to detect this and throw an error.

I think the MemoryMap itself should enforce this, but otherwise yes.

jfng pushed a commit to jfng/amaranth-soc that referenced this issue Aug 31, 2020
memory: provide a naming mechanism for memory map resources and windows.
14f4a0c 
Breaking changes:
* MemoryMap.add_resource() now takes a mandatory ``name`` parameter.
* MemoryMap.resources() now returns a ``resource, name, (start, end)``
  tuple.
* MemoryMap.all_resources() and MemoryMap.find_resource() return an
  instance of ResourceInfo describing the resource and its assigned name
  and address range, instead of a tuple.
* Freezing a memory map now prevents the further addition of *any*
  resource or window, instead of only the ones that require an extension
  of the memory map address bits. Allowing these could prevent us from
  detecting name conflicts in some cases.

Other changes:
* MemoryMap.__init__() now takes an optional ``name`` parameter, which
  is then visible as a property.
* csr.Multiplexer, csr.Decoder, WishboneCSRBridge and wishbone.Decoder
  now take an optional ``name`` parameter in __init__(), which is passed
  to their underlying memory map.

Fixes amaranth-lang#20.
jfng pushed a commit to jfng/amaranth-soc that referenced this issue Sep 1, 2020
memory: use a ResourceInfo data class to answer resource queries.
c21da61 
Breaking changes:
* MemoryMap.all_resources() and MemoryMap.find_resource() return an
  instance of ResourceInfo describing the resource and its assigned name
  and address range, instead of a tuple.

Fixes amaranth-lang#20.
@jfng jfng mentioned this issue Sep 1, 2020
Merged
@jfng jfng closed this as completed in #27 Oct 29, 2021
jfng pushed a commit that referenced this issue Oct 29, 2021
@jfng
memory: use a ResourceInfo data class to answer resource queries.
ec7d19e 
Breaking changes:
* MemoryMap.all_resources() and MemoryMap.find_resource() return an
  instance of ResourceInfo describing the resource and its assigned name
  and address range, instead of a tuple.

Fixes #20.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Provide a naming mechanism for memory map resources and windows jfng/amaranth-soc
2 participants
@whitequark @jfng