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

Factor out DDR3. #162

Merged
merged 1 commit into from Jun 13, 2021
Merged

Factor out DDR3. #162

merged 1 commit into from Jun 13, 2021

Conversation

StephanvanSchaik
Copy link
Contributor

The following commit factors out any instance of the Resource function specifying a 'ddr3' resource with a call to DDR3Resource. See also issue #15. The following boards have been updated to use DDR3Resource:

  • Alchitry Au
  • Arty A7
  • Arty S7
  • ECPIX5
  • Genesys2
  • OrangeCrab r0.1, r0.2
  • versa_ecp5, versa_ecp5_5g

As DDR3 uses differential signalling, DDR3Resource accepts diff_attrs in addition to attrs. Some board also specify different attributes for specific pins like the rst, dq and dqs pins.

Copy link
Member

@whitequark whitequark left a comment

Choose a reason for hiding this comment

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

The main concern I have with this PR is the complex interplay between the various attrs parameters. It seems hard to understand looking at the call site. Moreover, the way attributes are overridden both implicitly (by the platform code) and explicitly (by the or clauses) seems like it would make writing correct definitions challenging.

@StephanvanSchaik
Copy link
Contributor Author

The main concern I have with this PR is the complex interplay between the various attrs parameters. It seems hard to understand looking at the call site. Moreover, the way attributes are overridden both implicitly (by the platform code) and explicitly (by the or clauses) seems like it would make writing correct definitions challenging.

I agree, I am not sure myself what the best way forward is if we want to have a DDR3Resource abstraction. One way is to keep it simple and only have an attribute for the general resource and another for the differential signals, as there are a few boards where it is that simple, and to just write out the Resource for anything more complicated.

Similarly with the Arrow DECA platform I have been noticing that the peripherals are not as straight-forward, as most of the resources have pins that require two different I/O standards (e.g. 1.2V and 1.8V).

@whitequark
Copy link
Member

One way is to keep it simple and only have an attribute for the general resource and another for the differential signals, as there are a few boards where it is that simple, and to just write out the Resource for anything more complicated.

This is what I've leaned towards so far. There's nothing inherently wrong with writing out the Resource explicitly within the board definition framework as it's currently designed. Ideally we'd have some kind of type system for resources, though.

@StephanvanSchaik
Copy link
Contributor Author

One way is to keep it simple and only have an attribute for the general resource and another for the differential signals, as there are a few boards where it is that simple, and to just write out the Resource for anything more complicated.

This is what I've leaned towards so far. There's nothing inherently wrong with writing out the Resource explicitly within the board definition framework as it's currently designed. Ideally we'd have some kind of type system for resources, though.

All right, that is clear! I will work on this in my next cycle.

@StephanvanSchaik
Copy link
Contributor Author

This is a simplified version of the previous commit where the DDR3Resource only has the usual attrs, as well as diff_attrs for the differential signalling. As a consequence, only the Alchitry Au board uses DDR3Resource, whereas every other board defines the 'ddr3' resource explicitly, because they need more explicit control over the attributes of clk, dq and/or dqs`.

@whitequark whitequark merged commit 9d24c51 into amaranth-lang:master Jun 13, 2021
@whitequark
Copy link
Member

Thank you!

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