amaranth-lang / amaranth-soc

System on Chip toolkit for Amaranth HDL
BSD 2-Clause "Simplified" License
75 stars 29 forks source link

Add support for extending the address space of a memory map #7

Closed jfng closed 4 years ago

jfng commented 4 years ago

This PR adds support for extending the address space of a MemoryMap, after it has been instantiated.

Use case

Before this PR, the addr_width parameter of a csr.Multiplexer would be the definitive address width of its underlying memory map.

This is inconvenient, because it forces the user to be aware of the size and address of each csr.Element that will be added to the multiplexer upfront.

from nmigen_soc import csr

mux = csr.Multiplexer(addr_width=2, data_width=8)
mux.add(csr.Element(8, "rw"))
mux.add(csr.Element(8, "rw"))
mux.add(csr.Element(8, "rw"))

This PR allows the address space covered by the csr.Multiplexer to be extended as resources are added to it.

mux = csr.Multiplexer(addr_width=1, data_width=8)
mux.add(csr.Element(8, "rw"), extend=True)
mux.add(csr.Element(8, "rw"), extend=True)
mux.add(csr.Element(8, "rw"), extend=True)

print(mux.bus.addr_width) # mux.bus.addr_width is now 2

The following primitives can support this use case:

Contents

codecov[bot] commented 4 years ago

Codecov Report

Merging #7 into master will decrease coverage by 0.4%. The diff coverage is 97.8%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #7      +/-   ##
==========================================
- Coverage     100%   99.59%   -0.41%     
==========================================
  Files           4        4              
  Lines         436      493      +57     
  Branches       95      107      +12     
==========================================
+ Hits          436      491      +55     
- Misses          0        1       +1     
- Partials        0        1       +1
Impacted Files Coverage Δ
nmigen_soc/csr/bus.py 100% <100%> (ø) :arrow_up:
nmigen_soc/csr/wishbone.py 100% <100%> (ø) :arrow_up:
nmigen_soc/wishbone/bus.py 98.52% <96.15%> (-1.48%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 987aeb0...e583fb1. Read the comment docs.

whitequark commented 4 years ago

I've cherry-picked 7f27d90e66fcede136caaeeb6a1d3f76a115ca16 since it's just what we discussed and it seems correct.

For the other commit, could you explain why you made the memory map externally assignable, rather than created by the parent object?

jfng commented 4 years ago

I've cherry-picked 7f27d90 since it's just what we discussed and it seems correct.

Thanks !

For the other commit, could you explain why you made the memory map externally assignable, rather than created by the parent object?

Let's consider the case where the MemoryMap is created by its parent bus interface. The memory map is extendable, and the bus interface is owned by e.g. a multiplexer. The following happens:

  1. a resource is added to the multiplexer with extend=True
  2. the resource is then added to bus interface memory map with extend=True
  3. the resource does not fit, so the address space is extended, thus addr_width increases

We have a problem: the updated addr_width value is now different than the one used to shape the record fields of the bus interface.

A solution is to lazily create the bus interface, only when it is requested from the multiplexer. It would then have up-to-date record fields. The memory map is freezed at the same time.

In order to do this, the memory map has to be created by the multiplexer before the bus interface. The multiplexer adds resources to the map, and uses it to create the bus interface, once requested.

I favored external assignation over passing the memory map to __init__, because creating a memory map beforehand is not always desirable (e.g. if the interface is tied to a bus initiator, it doesn't need one).

whitequark commented 4 years ago

Thanks for the explanation. I agree there is no better way to do this. Please amend the commit to include the rationale, and feel free to merge it once it's done.