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
boards/riscv: Add support for PINE64 Ox64 BL808 SBC #11377
Conversation
This PR adds support for PINE64 Ox64 64-bit RISC-V SBC, based on Bouffalo Lab BL808 SoC (T-Head C906 Core). Most of the code is derived from NuttX for Star64 JH7110. The source files are explained in the articles here: https://github.com/lupyuen/nuttx-ox64 ### Modified Files `boards/Kconfig`: Added Ox64 board ### New Files in boards/risc-v/bl808/ox64 `src/bl808_appinit.c`: Startup Code `include/board.h`: Ox64 Definitions `include/board_memorymap.h`: Memory Map `src/etc/init.d/rc.sysinit`, `rcS`: Startup Script `src/.gitignore`: Ignore the tmp filesystem `scripts/ld.script`: Linker Script `scripts/Make.defs`: Ox64 Makefile `src/Makefile`: Ox64 Makefile `Kconfig`: Ox64 Config `configs/nsh/defconfig`: Build Config for `ox64:nsh` ### Updated Documentation `platforms/risc-v/bl808/index.rst`: New page for Bouffalo Lab BL808 SoC `platforms/risc-v/bl808/boards/ox64/index.rst`: Building and booting NuttX for Ox64 `platforms/risc-v/jh7110/boards/star64/index.rst`: Fix typo
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.
Drive-by review for @lupyuen . You absolutely have more domain experience here, friend, but perhaps fresh eyes with experience on adjacent systems might be helpful. Oddly my experience may both be too new (languages standardized in the last generation...) AND too old (POSIX 89's shell didn't have pushd/popd) at the same time. I know that NuttX and I live in very different time zones sometimes, so this might just be noise.
You don't need to defend or justify anything involved. Please do at least think about the individual issues raised, though, and if you think they're fine, they're fine. I'll readily admit to not being a NuttX internals expert, but I may have some related experience that might help NuttX from implementing the same mistakes I've made in the past. :-)
Based on non-trivial RISC-V familiarity and doing similar ports within Nuttx , though, this all seems quite reasonable to me. My "approval" officially means nothing, so I won't click that button, but if it were my project and Lup said "yes, i'm sure" to this whole thing, I'd accept this into MY tree without further blinking.
Thanx for making RISC-V/NuttX more awesome, Lup!
.. code:: console | ||
|
||
$ riscv64-unknown-elf-gcc -v | ||
|
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.
... and expect output like:
Using built-in specs.
COLLECT_GCC=riscv64-unknown-elf-gcc
COLLECT_LTO_WRAPPER=/usr/local/Cellar/riscv-gnu-toolchain/main/libexec/gcc/riscv64-unknown-elf/12.2.0/lto-wrapper
Target: riscv64-unknown-elf
[ ... ]
And not an error.
Sure, it seems obvious if you know, but SOMEONE will "check the toolchain" and verify that it printed an error. :-/
.. code:: console | ||
|
||
$ make export | ||
$ pushd ../apps |
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.
Pushd isn't available in all shells. Maybe it's available in "enough" shells, and that's OK. Maybe Bash/zsh rules enough of the world that Ksh can kiss it. There's a chance that pushd might make the BSD crowd grumpy, but its probably on them to find it and suggest an alternative.
My knowledge on this topic is admittedly aged and there may be other requirements within Nuttx that have forced this issue before anyone gets to this point.
Basic configuration that runs NuttShell (nsh). | ||
This configuration is focused on low level, command-line driver testing. | ||
Built-in applications are supported, but none are enabled. | ||
Serial Console is enabled on UART3 at 2 Mbps. |
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.
Is it obvious to the target audience which serial pins correspond to UART3 if they're wiring up a cable? If not, a callout might be helpful.
- 32-bit / 16-bit Mixed Instruction Set | ||
- RISC-V Machine Mode and User Mode | ||
- 32 x 32-bit Integer General Purpose Registers (GPR) | ||
- 32 x 32-bit / 64-bit Floating-Point GPRs |
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 don't doubt this is true, but what an interesting choice to put an FPU on a baby-sitting core... Wild times!
|
||
.pgtables (NOLOAD) : ALIGN(0x1000) { | ||
*(.pgtables) | ||
. = ALIGN(4); |
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.
Is that redundant?
_ebss = ABSOLUTE(.); | ||
} > ksram | ||
|
||
/* Stabs debugging sections. */ |
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.
Are stabs even a thing in toolchains modern enough to support SoCs like this? The release notes for GCC 13, for example, say "Support for emitting the STABS debugging format was removed. GCC supports DWARF in almost all configurations." Citation needed? https://gcc.gnu.org/gcc-13/changes.html
DWARF has been with us longer than RISC-V has.
int ret; | ||
struct boardioc_romdisk_s desc; | ||
|
||
desc.minor = RAMDISK_DEVICE_MINOR; |
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.
C99 is also older than every RISC-V chip out there. Are designated initializers still out of the question? This is code that doesn't have to be built on a PDP.
struct b_r_s desc {
.minor = RAMDIS_DEVICE_MINOR,
.nsectors = ...
.sectsize = ...
};
It widens the pool of the programmers that can read the code and allows the front end to warn about potential ordering problems, for example.
ret = boardctl(BOARDIOC_ROMDISK, (uintptr_t)&desc); | ||
if (ret < 0) | ||
{ | ||
syslog(LOG_ERR, "Ramdisk register failed: %s\n", strerror(errno)); |
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 get that optimizing an error path is questionable, but might this be easier to read as ONE syslog call instead of three? It'd stand a lower chance of being interrupted and interleaved in a confusing way.
syslog(LOG_ERR, "Ramdisk register failed: %s\n", strerror(errno)); | ||
syslog(LOG_ERR, "Ramdisk mountpoint /dev/ram%d\n", | ||
RAMDISK_DEVICE_MINOR); | ||
syslog(LOG_ERR, "Ramdisk length %lu, origin %lx\n", |
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.
About 25 years ago, a size_t became %z when it might be either a 32 or 64 bit value on either a 64 or a 32-bit host. (On a 32-bit core, it might be a long long. On a 64-bit core, it might not be a long at all, but just a plain int...)
I'd hope that syslog() has caught up by now, but don't really know the context; I just know there are safety flares and danger in this area.
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.
Thanks for the feedback Robert! We'll need to do a thorough analysis and regression test on the other RISC-V ports of NuttX, so that the code works consistently across QEMU RISC-V 32-bit + 64-bit, Star64 JH7110, Ox64 BL808, etc.
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.
This was introduced back when 64-bit CPUs were starting to become mainstream and 64-bit filesystems were definitely mainstream. The premise was that the printf caller didn't really want to know if a size_t was a long, a long int, a long long and thus didn't know if casting up or down was appropriate.
Since this code runs totally within Nuttx, it probably depends on the Nuttx syslog (which probably depends on [vsn]printf family.
I dug into it. It's actually been required in POSIX since SUSv3 back in 2001. I know that NuttX has a love/hate relationship with new standards, but I'd be shocked (well, kinda...) if you don't have access to this.
Edit: I think you do.
Citations:
https://en.cppreference.com/w/c/io/vfprintf (search for size_t sideways)
https://unix.stackexchange.com/questions/378890/does-posix-mention-cc-or-only-c99 (and upstream specs) on SUSv3 and V4 using C99 as specs commonly include specs via reference.
nuttx/libs/libc/stdio/lib_libvsprintf.c
Line 390 in 5f631e2
if (c == 'z' || c == 't') |
TL;DR: Formatting a size_t in this century should be done with %zu and friends. But you're right - if we're the first ones to notice this in NuttX, it's probably worth checking.
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.
Thanks Robert, I hope we're not holding up this PR from getting merged :-)
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 wouldn't think so. My review notes are technically powerless here. I'm just a bystander that cares about code quality/maintainability and knows a dash about RISC-V and NuttX.
Scanning my comments, the alignment and reliance on pushd/popd are the only thing I'd consider potential actual bugs (instead of improvements) that might be worth holding it up. But I lack the Nuttx ecosystem knowledge to know if only shells supporting pushed are already a requirement, for example.
Alan has already dropped an LGTM on this, so I'd consider that authoritative. If you have reason to not use %z width modifiers or just consider it out of scope for this PR, commit away.
I just responded to your call for reviewers and came in from the outer edges of the usual suspects to review th PR. If that's bad for the process, it's certainly easy for me to avoid gumming things up in the future.
From my view, if you're really sure that 64-bit code/data with 4-byte alignment is OK and something else already requires pushd/popd, go for it.
Summary
This PR adds support for PINE64 Ox64 64-bit RISC-V SBC, based on Bouffalo Lab BL808 SoC (T-Head C906 Core). Most of the code is derived from NuttX for Star64 JH7110. The source files are explained in the articles here
Modified Files
boards/Kconfig
: Added Ox64 boardNew Files in boards/risc-v/bl808/ox64
src/bl808_appinit.c
: Startup Codeinclude/board.h
: Ox64 Definitionsinclude/board_memorymap.h
: Memory Mapsrc/etc/init.d/rc.sysinit
,rcS
: Startup Scriptsrc/.gitignore
: Ignore the tmp filesystemscripts/ld.script
: Linker Scriptscripts/Make.defs
: Ox64 Makefilesrc/Makefile
: Ox64 MakefileKconfig
: Ox64 Configconfigs/nsh/defconfig
: Build Config forox64:nsh
Updated Documentation
platforms/risc-v/bl808/index.rst
: New page for Bouffalo Lab BL808 SoCplatforms/risc-v/bl808/boards/ox64/index.rst
: Building and booting NuttX for Ox64platforms/risc-v/jh7110/boards/star64/index.rst
: Fix typoImpact
This PR is needed to support NuttX on Ox64 SBC.
No impact on existing code, since the Ox64 source files are not used by existing code.
Testing
We tested the NuttX Build on Ox64 SBC via a microSD Card:
NuttX boots correctly to NSH Shell:
NuttX Log on Ox64
Watch the Demo Video
See the Build Outputs
See the Build Log