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

API has superfluous arguments #16

Open
ed-xmos opened this issue Feb 21, 2017 · 3 comments
Open

API has superfluous arguments #16

ed-xmos opened this issue Feb 21, 2017 · 3 comments
Labels
type:cleanup Clean up the code base

Comments

@ed-xmos
Copy link
Contributor

ed-xmos commented Feb 21, 2017

In addition to the ports, the sdram server API is currently passed:

  • const static unsigned cas_latency,
  • const static unsigned row_words,
  • const static unsigned col_bits,
  • const static unsigned col_address_bits,
  • const static unsigned row_address_bits,
  • const static unsigned bank_address_bits,
  • const static unsigned refresh_ms,
  • const static unsigned refresh_cycles,
  • const static unsigned clock_divider);
  • col_bits is unused by the server so should be removed
  • row_words is misleading because it's actually the 32b row words (not the SDRAM row words) and can actually be calculated from col_address_bits (2 ^ (col - 1)) since API width is fixed at 32b and SDRAM width is fixed at 16b
  • refresh_cycles in every SDRAM I have seen is (2 ^ row_address_bits) so could be removed

Finally, it makes sense to wrap up both of the port list (5 ports) and SDRAM config arguments into typedef-ed structures.

@samchesney
Copy link
Contributor

We might want to reconsider wrapping ports into a structure as it will introduce an inconsistent style across our libraries (see IP-CS116), the rational being:

Structures work well if passed around to multiple places. In this case they are only passed in at the top level so adding a structure is an extra level of indirection. Without this port allocations in applications are clearer. This system also allows easy sharing of clock arguments with other components (if marked const).

If the ports are bound together in some way with a default initializer (e.g. OTP or XAB libraries) then a structure may still be appropriate.

@andrewstanfordjason
Copy link
Collaborator

The reason everything was exposed was to be inline with the coding guidelines at the time. I prefer all things like that in a struct

@ed-xmos
Copy link
Contributor Author

ed-xmos commented Feb 21, 2017

I can see the arguments for both approaches.
I think the SDRAM parameters should definitely be in a type-deffed struct. That way, you can just pass in an initialised struct for a part number like "sdram_IS42S16400D". We can provide a few examples of common parts.
For the ports, it's probably clearer for the user to have less indirection but more convenient for us to have a struct when you have cases like slice kit where there are multiple slots that are supported (as well as multiple boards). So you could have an initialised struct such as ports_slicekit_X200_triangle_tile_0 or similar.
I propose going for struct for the sdram parameters and resource IDs for the ports/clockblk unless anyone has strong objections, and keep the permutations of the examples to a minimum to keep it cleaner.

@oscarbailey-xmos oscarbailey-xmos added the type:cleanup Clean up the code base label Jan 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:cleanup Clean up the code base
Projects
None yet
Development

No branches or pull requests

4 participants