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 Star64 SBC #1019

Merged
merged 5 commits into from
Nov 8, 2023
Merged

Add support for Star64 SBC #1019

merged 5 commits into from
Nov 8, 2023

Conversation

Ivan-Velickovic
Copy link
Contributor

@Ivan-Velickovic Ivan-Velickovic commented Apr 21, 2023

This PR adds initial support for the Pine64 Star64 SBC. This is a draft as there are still a couple things to sort out before this is ready for mainlining, however, I thought I would make the changes public in case anyone else is experimenting with the Star64.

The interesting thing about the Star64 is it has SiFive U74 cores which have ASID support unlike the HiFive so hopefully the sel4bench results will be much better on this board, but I am yet to test that.

Couple points for reviewers to consider:

  • This board is pretty experimental, not many were made and the software support for the board is pretty lackluster. The SoC however has decent software support and I think the Linux patches are making their way into the kernel.
  • Support for OpenSBI 1.3 will be needed for those that boot sel4test with the OpenSBI payload. Booting with OpenSBI is not strictly necessary as the board's boot process when you get it looks like U-Boot SPL -> OpenSBI -> U-Boot.

@axel-h
Copy link
Member

axel-h commented Apr 28, 2023

So, is the plan now to merge this? With a few changes this is also usable with the VisionFive board then, that has the same SOC.

@axel-h
Copy link
Member

axel-h commented Apr 28, 2023

Is there any good reason not to update to OpenSBI 1.3 actually? We have been at 0.8 for ages and just recently (#383) switched to 0.9 finally. If 1.3 works on all the (new) hardware and QEMU, then let's just use this by default.

@lsf37
Copy link
Member

lsf37 commented Apr 28, 2023

Is there any good reason not to update to OpenSBI 1.3 actually? We have been at 0.8 for ages and just recently (#383) switched to 0.9 finally. If 1.3 works on all the (new) hardware and QEMU, then let's just use this by default.

0.9 is actually not working, sel4bench has been failing for RISC-V since we merged that PR. If 1.3 does work everywhere I would be for using it and figuring out what exactly the problem is on 1.3 instead of 0.9.

So, how do we check that 1.3 works for everything? Is everything we care about in CI already?

@axel-h
Copy link
Member

axel-h commented Apr 28, 2023

So, how do we check that 1.3 works for everything? Is everything we care about in CI already?

Well, things should be there. However, trying to use OpenSBI v1.2 in seL4/sel4bench-manifest#12 failed either due to an outdated workflow or I lack some rights: https://github.com/seL4/sel4bench-manifest/actions/runs/4826147244/jobs/8597908000

@Ivan-Velickovic
Copy link
Contributor Author

Just to note in order to get seL4/sel4test working, compiling OpenSBI is not required, so updating OpenSBI will not have an effect for this particular platform, unless someone was to change the default booting process. So, I (think) there is nothing blocking this PR.

@axel-h
Copy link
Member

axel-h commented Apr 28, 2023

Just to note in order to get seL4/sel4test working, compiling OpenSBI is not required, so updating OpenSBI will not have an effect for this particular platform, unless someone was to change the default booting process. So, I (think) there is nothing blocking this PR.

Could you explain in detail at seL4/docs#181 how you currently set up things? Seems - for quite practical development reasons - you are not building a "OpenSBI+sel4"-package that SPL loads. But instead have SPL boot OpenSBI and then U-Boot from some Flash, so a sel4 system can be loaded via TFTP then. Question is, if we want both way to work, so also how that U-Boot is not needed there at all except for convenience.

Copy link
Member

@axel-h axel-h May 2, 2023

Choose a reason for hiding this comment

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

We have tools/dts/update-dts.sh, is there a trivial way to extend this so it can grab this DTS from the linux sources?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As is tradition with software, the script is broken so I will have to fix that first then I should be able to add RISC-V device trees to it.

Copy link
Member

@lsf37 lsf37 left a comment

Choose a reason for hiding this comment

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

If the boot process is sorted out, this looks good to go from my side. It'll be nice to have another RISC-V board in CI and a potential future verification option with actual ASID support.

Comment on lines +22 to +25
# The value for TIMER_FREQUENCY is from the "timebase-frequency" field on
# the "cpus" node in the Star64 device tree.
# The value for PLIC_MAX_NUM_INT comes from the DTS "plic" node which says
# "riscv,ndev = <0x88>".
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for documenting those.

@Ivan-Velickovic
Copy link
Contributor Author

Happy to adjust this PR (or make a new one once this is merged) to adhere to #1036

@canarysnort01
Copy link
Contributor

I've successfully tested this commit with sel4test with mcs and smp enabled, in combination with seL4/util_libs#167, seL4/seL4_libs#78, and seL4/seL4_tools#174, on pinetab-v hardware, which is based on the same SoC as the star64.

I configured sel4test with these option:

./init-build.sh -DPLATFORM=star64 -DKernelRiscvExtD=true -DUseRiscVOpenSBI=false -DSIMULATION=false -DMCS=true -DSMP=true -DNUM_NODES=4

This is the sel4test console output: sel4test-mcs-smp-success.log

Signed-off-by: Ivan-Velickovic <i.velickovic@unsw.edu.au>
Similarly to the U54-MC, the U74-MC has a S-core that does not run in
supervisor mode.

Signed-off-by: Ivan-Velickovic <i.velickovic@unsw.edu.au>
Signed-off-by: Ivan-Velickovic <i.velickovic@unsw.edu.au>
@Ivan-Velickovic
Copy link
Contributor Author

Adding set(KernelRiscvUseClintMtime ON) in the configuration causes the kernel to fail booting. Which is odd, given that the mtime offset is (according to the documentation) correct.

When trying to load the value of CLINT_PPTR + CLINT_MTIME_OFFSET in riscv_read_time, the execution of the kernel just stops.

@Ivan-Velickovic
Copy link
Contributor Author

Adding set(KernelRiscvUseClintMtime ON) in the configuration causes the kernel to fail booting. Which is odd, given that the mtime offset is (according to the documentation) correct.

When trying to load the value of CLINT_PPTR + CLINT_MTIME_OFFSET in riscv_read_time, the execution of the kernel just stops.

It seems that I am missing something, as I would have thought that OpenSBI is doing the exact same access when we use rdtime. I guess the first thing to do is actually confirm that assumption.

@canarysnort01
Copy link
Contributor

When trying to load the value of CLINT_PPTR + CLINT_MTIME_OFFSET in riscv_read_time, the execution of the kernel just stops.

I get the same behavior on the pinetab-v.

@canarysnort01
Copy link
Contributor

I see this information printed by OpenSBI when I boot:

Domain0 Region00          : 0x0000000002000000-0x000000000200ffff M: (I,R,W) S/U: ()
Domain0 Region01          : 0x0000000040000000-0x000000004003ffff M: (R,X) S/U: ()
Domain0 Region02          : 0x0000000040040000-0x000000004007ffff M: (R,W) S/U: ()
Domain0 Region03          : 0x0000000000000000-0xffffffffffffffff M: (R,W,X) S/U: (R,W,X)

Does Domain0 Region00 mean it is protecting the CLINT from being accessed from supervisor mode?

@canarysnort01
Copy link
Contributor

I tried accessing mtime from the u-boot console and I got this:

StarFive # md.q 0x200bff8
Unhandled exception: Load access fault
EPC: 00000000cce0a60c RA: 00000000cce0a522 TVAL: 000000000200bff8
EPC: 000000004026660c RA: 0000000040266522 reloc adjusted

SP:  00000000cc593890 GP:  00000000cc593dd0 TP:  0000000000000001
T0:  00000000cc593b10 T1:  0000000000000039 T2:  0000000000000000
S0:  0000000000000002 S1:  0000000000000008 A0:  0000000000000002
A1:  00000000cce281b0 A2:  00000000cce26ab6 A3:  fffffffffffffffe
A4:  0000000000000008 A5:  0000000000000000 A6:  0000000000000002
A7:  0000000000000008 S2:  00000000cc593a69 S3:  0000000000000002
S4:  00000000cc5939a8 S5:  000000000200bff8 S6:  0000000000000000
S7:  00000000cc5939a8 S8:  0000000000000010 S9:  00000000cc5939a8
S10: 0000000000000010 S11: 0000000000000004 T3:  0000000000000010
T4:  0000000000000000 T5:  000000000001869f T6:  00000000cc593af0

Code: 4721 e597 0001 8593 bb25 bf81 9763 00e4 (b683 000a)

@Ivan-Velickovic
Copy link
Contributor Author

I see this information printed by OpenSBI when I boot:

Domain0 Region00          : 0x0000000002000000-0x000000000200ffff M: (I,R,W) S/U: ()
Domain0 Region01          : 0x0000000040000000-0x000000004003ffff M: (R,X) S/U: ()
Domain0 Region02          : 0x0000000040040000-0x000000004007ffff M: (R,W) S/U: ()
Domain0 Region03          : 0x0000000000000000-0xffffffffffffffff M: (R,W,X) S/U: (R,W,X)

Does Domain0 Region00 mean it is protecting the CLINT from being accessed from supervisor mode?

Yes, that looks like what is happening. I don't know what Domain means, it seems to be OpenSBI specific terminology. I guess the next question is why it is protecting access to it.

@Ivan-Velickovic
Copy link
Contributor Author

Nevermind, they have documentation on it https://github.com/riscv-software-src/opensbi/blob/master/docs/domain_support.md.

@Ivan-Velickovic
Copy link
Contributor Author

The only place I can see in OpenSBI where it's setting the domain regions is here, https://github.com/riscv-software-src/opensbi/blob/ee1f83ca848d3639b808e52719bc7111f5a1be7c/lib/utils/fdt/fdt_domain.c#L355. This indicates that the previous booting stage (either U-Boot SPL or the firmware) has decided that the CLINT should only be M-Mode accessible.

@Ivan-Velickovic
Copy link
Contributor Author

There doesn't seem to be anything in the U-Boot source code that is setting the domain regions (which makes sense).

I assume then the firmware has decided to mark it as M-Mode only?

In either case, this issue is not blocking this PR and I think there are no other comments to address

@Ivan-Velickovic
Copy link
Contributor Author

Can this be merged?

@lsf37
Copy link
Member

lsf37 commented Nov 7, 2023

Yes, I think this is good to go when the checks have passed again.

Copy link
Member

@axel-h axel-h left a comment

Choose a reason for hiding this comment

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

Look good to merge, assuming this boots on your board. Maybe mention in the commit comment that this needs OpenSBI 1.3, so we have a reference there.

@Indanz Indanz merged commit baacd44 into seL4:master Nov 8, 2023
40 checks passed
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.

None yet

6 participants