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 annotations for memory maps, wishbone and CSR primitives. #58

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jfng
Copy link
Member

@jfng jfng commented Dec 5, 2023

Depends on PR amaranth-lang/amaranth#978.

MemoryMap resources (e.g. csr.Elements) are now assumed to have a .signature property, which lets them hold annotations. I am not sure whether we should enforce this in MemoryMap.add_resource(), or introduce a base class instead.

@jfng jfng requested a review from whitequark December 5, 2023 12:25
@jfng jfng mentioned this pull request Dec 13, 2023
3 tasks
@jfng
Copy link
Member Author

jfng commented Jan 19, 2024

Updated, mainly to track changes in amaranth-lang/amaranth#978:

  • replace annotation names with their $id URLs;
  • replace the Signature.annotations property with Signature.annotations(self, interface, /) to access memory maps, which are assigned to bus interfaces;
  • improve URL paths of amaranth-soc schemas:
    • csr.Interface: from /csr.json to /csr/bus.json;
    • csr.Element: from /csr-element.json to /csr/element.json;
    • wishbone.Interface: from /wishbone.json to /wishbone/bus.json;
    • memory.MemoryMap: from /memory-map.json to /memory/memory-map.json.

"""
if not isinstance(element, Element):
raise TypeError(f"Element must be a csr.Element object, not {element!r}")
if element.signature != self:
Copy link
Member

Choose a reason for hiding this comment

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

This check should probably be in Signature.annotations.

"""
if not isinstance(interface, Interface):
raise TypeError(f"Interface must be a csr.Interface object, not {interface!r}")
if interface.signature != self:
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

@@ -264,7 +269,8 @@ def add_resource(self, resource, *, name, size, addr=None, alignment=None):
Arguments
---------
resource : object
Arbitrary object representing a resource.
Arbitrary object representing a resource. It must have a 'signature' attribute that is
Copy link
Member

Choose a reason for hiding this comment

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

must be an interface object

?

},
"alignment": {
"type": "integer",
"minimum": 0,
Copy link
Member

Choose a reason for hiding this comment

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

You should add a comment here explaining what the value means. (It is a power of 2, right? Where the alignment in words is 1 << alignment.)

"type": "integer",
"minimum": 0,
},
"ratio": {
Copy link
Member

Choose a reason for hiding this comment

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

This would benefit from a comment as well.

if not isinstance(interface, Interface):
raise TypeError(f"Interface must be a wishbone.Interface object, not {interface!r}")
if interface.signature != self:
raise ValueError(f"Interface signature is not equal to this signature")
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

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.

2 participants