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

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

Merged
merged 1 commit into from
Feb 10, 2020

Conversation

jfng
Copy link
Member

@jfng jfng commented Feb 7, 2020

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:

  • csr.Multiplexer
  • csr.Decoder
  • wishbone.Decoder

Contents

  • 7f27d90 adds support for address space extension to nmigen_soc.memory
  • fa244cd ports nmigen_soc.csr and nmigen_soc.wishbone to the updated API

@codecov
Copy link

codecov bot commented Feb 7, 2020

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%> (ø) ⬆️
nmigen_soc/csr/wishbone.py 100% <100%> (ø) ⬆️
nmigen_soc/wishbone/bus.py 98.52% <96.15%> (-1.48%) ⬇️

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
Copy link
Member

I've cherry-picked 7f27d90 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
Copy link
Member Author

jfng commented Feb 10, 2020

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
Copy link
Member

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.

Before this commit, the bus interface of a Decoder or a Multiplexer
would be created during __init__. A bus interface would always be tied
to an underlying memory map.

After this commit, the following changes are introduced:

* The bus interface of a Decoder (or a Multiplexer) is lazily created
  upon request. This allows the record fields of the bus interface to
  be up-to-date with any address space extension that may have occured.

* The memory_map attribute of a bus interface is no longer assigned
  during __init__. It is instead assigned externally at a later time.
  Decoupling the memory map from the bus interface allows a Decoder (or
  a Multiplexer) to first create the memory map, extend it, and then
  use it to create the bus interface, once requested.

* Decoder.add(extend=True) (or Multiplexer.add) can be used to add
  a window (or resource) that would otherwise not fit inside its
  underlying memory map.
@jfng jfng merged commit f8f8982 into amaranth-lang:master Feb 10, 2020
antoinevg added a commit to antoinevg/luna that referenced this pull request Dec 22, 2022
The semantics for adding resources to a MemoryMap has changed in amaranth-soc with the addition of support for dynamically extending the address space of a memory map:

    amaranth-lang/amaranth-soc#7

This PR brings the Luna WishboneRAM and WishboneROM peripherals up to date with the new semantics.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants