-
Notifications
You must be signed in to change notification settings - Fork 59
Add rock3b docs. Fix typos #311
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
Conversation
Signed-off-by: Jakub Duchniewicz <j.duchniewicz@unsw.edu.au>
|
Looking not bad, please break long lines at 80. We're missing the |
Hardware/rock3b.md
Outdated
| @@ -0,0 +1,129 @@ | |||
| --- | |||
| arm_hardware: true | |||
| cmake_plat: rock3b | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to seL4/seL4#1553, the cmake platform name is rk3568
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, what's the config_set(KernelARMPlatform ARM_PLAT rock3b) for? It's also set by platforms not having any sub-platform.
In this case I'm wondering whether rock3b should be a sub-platform of rockpro64 instead of being it's own kernel platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, but setting rock3b did not work for compilation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
KernelARMPlatform is used for variations, e.g. for imx6, the declare_platform is imx6 and KernelARMPlatform can be one of sabre, wandq, nitrogen6sx, with sabre being the default.
The only correct value for KernelPlatform in our case is rk3568 . If we expect further variations in the future it makes sense to have rock3b as KernelARMPlatform as the currently only value (which is being set automatically).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But you are right that maybe it should be a sub platform for rockpro64 -- I'm not exactly an expert on how that is set up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it to rk3568 for the time being. Happy to change it to rock3b if we decide to treat it as a child of overarching rockchip family
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(we could also follow just the imx8 platforms and just call it rock3b and not rk3568 at all; the main thing we care about is memory and uart setup which is specific to the board not the SOC)
This is where we disagree: Those are easily fixed by users with a custom DTS overlay, we really don't want one "platform" per UART/memory configuration. So platforms should be per SoC, not per board IMHO.
Main thing boards have different is all the peripheral hardware in addition to the SoC, but those don't matter for seL4 support. Only clock configuration can be relevant, but that's fixed in the future if we switch to clock ticks instead of us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only clock configuration can be relevant, but that's fixed in the future if we switch to clock ticks instead of us.
That's not going away with the switch, we still do need to know the same info as before if we want to provide the same functionality as now in libsel4.
But it still makes sense to me to group by SoC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in principle rockpro64 should be then renamed to rk3399 to adhere to new conventions of naming by SoC right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only clock configuration can be relevant, but that's fixed in the future if we switch to clock ticks instead of us.
That's not going away with the switch, we still do need to know the same info as before if we want to provide the same functionality as now in libsel4.
It goes away in the sense that it can be just a simple (user changeable) config option and we won't have the CLK_MAGIC and CLK_SHIFT any more.
Then there is very little left that can't be configured by the user and a platform would be just a convenient set of settings for a known board, instead of a requirement before being able to build seL4.
My view is that long-term we only need a generic platform per architecture that can be used pretty much for all newer hardware by tweaking config options as a user, as long as their hardware is a known combination of elements. In that sense Aarch64 and RISC-V could work the same as x86 does now.
The next step would be to parse the DTB at boot to configure the memory and actually have a binary that can run on multiple boards.
So in principle rockpro64 should be then renamed to rk3399 to adhere to new conventions of naming by SoC right?
Yes and no. Yes in the sense that it should have been named that, but no in the sense you can't just change the name without confusing existing users. And we just did a new release, so it is too late for that I think.
It doesn't matter hugely, but it makes it harder for people with a different board based on the same SoC to discover that it's already supported by seL4. This is partially solved by mentioning the SoC on the supported HW webpage. But if the board is more popular than the SoC, trade-offs can be different too, so it's not black and white.
Yup, they do, so you can add |
Signed-off-by: Jakub Duchniewicz <j.duchniewicz@unsw.edu.au>
No description provided.