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

Sonata Pinmux driver #360

Merged
merged 2 commits into from
Jan 7, 2025
Merged

Sonata Pinmux driver #360

merged 2 commits into from
Jan 7, 2025

Conversation

HU90m
Copy link
Collaborator

@HU90m HU90m commented Nov 27, 2024

A driver for Sonata v1.0's Pinmux

Copy link
Contributor

@marnovandermaas marnovandermaas left a comment

Choose a reason for hiding this comment

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

Thanks Hugo, this looks correct to me from a code review.

@HU90m
Copy link
Collaborator Author

HU90m commented Nov 28, 2024

I think in the case of the Pinmux it's best to make sure sink names match the canonical name used documentation, RTL and schematic. The sinks are enum classes, so breaking the code style shouldn't cause much confusion here.

Copy link
Member

@nwf nwf left a comment

Choose a reason for hiding this comment

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

It looks like platform-pinmux.hh is generated from sonata-system's platform-pinmux.hh.tpl via its top_gen. It'd be good to...

  • have a comment to that effect in the file
  • update the .hh.tpl in light of the changes made here (NOLINT, names of some function arguments and locals, &c).

FWIW, I ran git diff --word-diff having clobbered sonata-system v0.3-403-g658a741's sw/cheri/common/platform-pinmux.hh with the one from this PR; that might be the easiest way to find the requisite changes to the template?

I have not attempted to validate that top_gen is consistent between the RTL and C++ it generates, much preferring to merely assume so. :)

@HU90m
Copy link
Collaborator Author

HU90m commented Jan 6, 2025

↑ rebased onto main, ↓ added comment pointing to what generates the file.

See lowRISC/sonata-system#376

@nwf
Copy link
Member

nwf commented Jan 6, 2025

Thanks for doing that. One nit aside, this LGTM.

@marnovandermaas
Copy link
Contributor

lowRISC/sonata-system#376 is now merged.

@nwf
Copy link
Member

nwf commented Jan 6, 2025

Want to update the platform-pinmux.hh herein with the result of that PR? I think it's good to go after that.

@HU90m
Copy link
Collaborator Author

HU90m commented Jan 7, 2025

Want to update the platform-pinmux.hh herein with the result of that PR? I think it's good to go after that.

The sonata-system repo uses the lowRISC C++ formatting, unlike sonata-software, so the output of the topgen script has to be lowRISC formatted.

This PR has already been updated with the updated with the latest output, only run through clang-format with the CHERIoT settings.

@HU90m HU90m merged commit b1b3509 into CHERIoT-Platform:main Jan 7, 2025
7 checks passed
@davidchisnall
Copy link
Collaborator

I missed in review, but this seems to have an Apache license. Is it possible to relicense to match the rest of the repository?

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.

4 participants