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

Implement RFC 16 (CSR register API) #40

Merged
merged 19 commits into from
Feb 9, 2024
Merged

Conversation

jfng
Copy link
Member

@jfng jfng commented Jul 14, 2023

This PR implements the CSR register RFC.

  • Migrate csr.Element away from Record
  • Add Field
  • Add built-in fields (csr.action.R,csr.action.W, csr.action.RW, csr.action.RW1C, csr.action.RW1S)
  • Add reserved fields
  • Add field collections (FieldActionMap, FieldActionArray)
  • Add Register
  • Add Builder
  • Add Bridge

@jfng jfng changed the title CSR register API prototype Implement RFC 16 (CSR register API) Sep 1, 2023
@jfng jfng marked this pull request as ready for review September 1, 2023 17:02
@jfng jfng changed the base branch from main to lib-wiring September 12, 2023 12:20
@jfng
Copy link
Member Author

jfng commented Sep 12, 2023

Rebased on the lib-wiring branch to use the Amaranth interface API.

@jfng jfng changed the base branch from lib-wiring to main November 29, 2023 10:46
@jfng
Copy link
Member Author

jfng commented Nov 29, 2023

Rebased on the lib-wiring branch to use the Amaranth interface API.

Rebased back to main.

Copy link
Contributor

@tpwrules tpwrules left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really excited to see this. It will remove a lot of fiddly and annoying code work, and there are a lot of interesting things that can be done by combining with the component metadata work, among other things.

But it's still rather hard to get into. It felt like an uphill battle trying to inventory all the pieces (Registers, Fields, Elements, Signatures, Interfaces, csr.Interfaces, Multiplexers, Bridges) and figure out who is supposed to connect to what and how. I figured it out after a while by diving through the source along with some brute forcing but that's not a pleasant experience.

While all the bits do seem to work logically and properly (once I could see the logic), I would have a hard time personally recommending a merge without at least an illustrated example design to show how the pieces are supposed to fit together, as now there are even more pieces and even more ways to get things wrong compared to just managing Elements. I'm happy to share my design and help guide such an example.

The RFC text could use an update too, to correct for the changes introduced by the incorporation of lib.wiring.

I've put a few comments on simple changes that would make things more convenient to use too.

amaranth_soc/csr/field.py Outdated Show resolved Hide resolved
amaranth_soc/csr/field.py Outdated Show resolved Hide resolved
amaranth_soc/csr/__init__.py Show resolved Hide resolved
amaranth_soc/csr/field.py Outdated Show resolved Hide resolved
amaranth_soc/csr/field.py Outdated Show resolved Hide resolved
@jfng
Copy link
Member Author

jfng commented Feb 8, 2024

Updated to implement the final version of amaranth-lang/rfcs#16.

Before this commit, CSR elements were added with `Multiplexer.add()`.

Now, a memory map of elements is populated beforehand, then given as
argument to `Multiplexer.__init__()`.

This decouples the definition of a group of neighboring registers from
their implementation by a CSR Multiplexer.
Also, fix FieldPort signature direction.
Before this commit, a Register whose fields were variable annotations
couldn't be instantiated more than once, as the latter belonged to the
global scope.

The Field class is now a factory for user-defined field components,
instead of a base class. Every FieldMap or FieldArray sharing a
Field will use a different instance of its component, obtained from
Field.create().

Also:
* update to follow RFCs 37 and 38
* docstring and diagnostics improvements.
This allows csr.Register subclasses that use variable annotations to
define their access mode without overriding __init__().

In addition, the default value of access= (which was "rw") is removed.
The access mode of a register has an influence on the rest of the SoC
and should therefore be explicitly set.

As a consequence of access= being a required __init_subclass__() kwarg,
the csr.Register class can no longer be directly instantiated and must
be subclassed first.
The access= parameter can now be set in either __init_subclass__() or
__init__(). It has a default value of None, and __init__() will error
out if set in both sites or none of them.
Also, csr.Multiplexer now requires the signature of its memory map
resources to contain a csr.Element.Signature member, named "element"
and oriented as output.
@jfng jfng enabled auto-merge February 9, 2024 16:50
@jfng jfng added this pull request to the merge queue Feb 9, 2024
Merged via the queue into amaranth-lang:main with commit 1e1490e Feb 9, 2024
12 checks passed
@jfng jfng deleted the csr-reg-rfc branch February 9, 2024 16:53
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

3 participants