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 DDR4 RDIMMs #189

Merged
merged 1 commit into from
May 15, 2020

Conversation

daveshah1
Copy link
Collaborator

I would appreciate some feedback on the structure, it seems like perhaps is_rdimm and drive strengths should be a module property, but I can't find a nice way of glueing that in.

This also still needs regression testing on a non-RDIMM platform too, I can do that if you want.

@daveshah1
Copy link
Collaborator Author

See also enjoy-digital/litex#490

Signed-off-by: David Shah <dave@ds0.me>
@enjoy-digital
Copy link
Owner

Thanks! i'll review it in the next days.

@enjoy-digital
Copy link
Owner

In modules.py, we should probably create specialized SDRAMModules:

class SDRAMRegisteredModule(SDRAMModule): registered = True

class SDRModule(SDRAMModule): memtype = "SDR"
class SDRRegisteredModule(SDRAMRegisteredModule): memtype = "SDR"

...

class DDR4Module(SDRAMModule): memtype = "DDR4"
class DDR4RegisteredModule(SDRAMRegisteredModule): memtype = "DDR4"

And use them as the base class for the modules. This would simplify the definition and would provide the registered attribute that would be used by the PHY/Core.

In PhySettings, we could rename is_rdimm to registered attribute. Does it make sense for you? If so, i'm happy to do the changes and do the regression testing on targets with Unbuffered memory. (unless you want to do it, but the improvements here are more related to LiteDRAM itself than your PR).

@daveshah1
Copy link
Collaborator Author

Yes, that sounds good. The RCD drive strength settings come from the SPD data and so should also be a property of DDR4RegisteredModule. Please feel free to make these changes.

@enjoy-digital enjoy-digital merged commit b2a5685 into enjoy-digital:master May 15, 2020
@enjoy-digital
Copy link
Owner

Thanks, merged. I'll do the adaptations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants