Add ARMv5 and NUC980 support

[MDr164]

This adds the Nuvoton NUC980 and one of its reference boards the NuMaker-IIoT-NUC980 which is a low cost ARM9 (ARMv5) based SoC used in some iBMC aka industrial or 'edge' BMC solutions by several manufacturers. The datasheet and software (u-boot + Linux) are publicly available from Nuvoton. I'll add a guide on how to test this on hardware including logs a bit later once everything works as intended. Opening a draft PR for initial discussion.

[abarisani]

Please target development branch.

[abarisani]

I am not sure I like the overrides and linkhwinit0 flag (the less build flags there are the better), I will ponder a cleaner (at least to me) approach.

Go has the GOARM variable which maybe is exposed by runtime to have better conditionals in both .go and .s files? It should be set differently for this target I think.

I will check this and propose a different layout.

Ideally I would like to keep the current ARMv8 code as identical as possible to the current tested one.

[MDr164]

Yes, I was also wondering if there is a better approach to split of the ARMv5 quirks and I'm not fully contempt with this solution either. It does compile fine and I do get code execution. It still gets stuck somewhere on my board so I'm currently debugging this with JTAG but the development pipeline of using JTAG with Go code is still a bit tricky. It was easier to get working on my other port than on this one here.

[MDr164]

It looks like there are somewhat underdocumented build tags I could use, so instead of adding new tags I would simply rely on GOARM=5 which should trigger //go:build arm.5

[MDr164]

Added two commits to experiment with that existing build tag

[MDr164]

Good news: I got everything working on real hardware now and got TamaGo talking to me on the UART! I did run into an issue with upstream Go though regarding ARMv5 atomics. Might be worth fixing downstream first I guess. I assume I can open a PR to tamago-go so we can discuss if this should go upstream or not?

Hello from TamaGo on NUC980!

Runtime   : go1.26.1 tamago/arm
Board     : NuMaker-IIoT-NUC980G2
SoC       : NUC980 PDID 0x0
CPU       : ARM926EJ-S MIDR 0x41069265
  impl    : 0x41 (ARM)
  variant : 0x0
  arch    : 0x6
  part    : 0x926
  rev     : 0x5
RAM       : 128 MB @ 0x00000000

tick 0
tick 1
tick 2
tick 3
tick 4

EDIT: This was the needed fix for ARMv5: MDr164/tamago-go@14103fd

[abarisani]

Good news: I got everything working on real hardware now and got TamaGo talking to me on the UART! I did run into an issue with upstream Go though regarding ARMv5 atomics. Might be worth fixing downstream first I guess. I assume I can open a PR to tamago-go so we can discuss if this should go upstream or not?

Hello from TamaGo on NUC980!

Runtime   : go1.26.1 tamago/arm
Board     : NuMaker-IIoT-NUC980G2
SoC       : NUC980 PDID 0x0
CPU       : ARM926EJ-S MIDR 0x41069265
  impl    : 0x41 (ARM)
  variant : 0x0
  arch    : 0x6
  part    : 0x926
  rev     : 0x5
RAM       : 128 MB @ 0x00000000

tick 0
tick 1
tick 2
tick 3
tick 4

EDIT: This was the needed fix for ARMv5: MDr164/tamago-go@14103fd

Any tamago-go contribution must be good for potential upstreaming inclusion, to accept it I'd ask:

• Do not remove the existing commented code (the TODO) from original Go.
• Patch ·armcas in atomic_arm.s has it alread has a GOARM conditional
• Explain why ·armcas in atomic_arm.s doesn't work as is? Interrupt handlers should never execute code within the Go runtime, so why is your implementation required?

As it stands your patch is not suitable as it is too specific to hardware and the Go upstream implementation has been designed to be agnostic.

As Go already supports ARM5 I'd like to understand what is the issue that you are trying to solve.

Thanks!

[MDr164]

Thanks for the review, I updated the toolchain patch here now: MDr164/tamago-go@5aef39e

Do not remove the existing commented code (the TODO) from original Go.

Done

Patch ·armcas in atomic_arm.s has it alread has a GOARM conditional

Good idea, done

Explain why ·armcas in atomic_arm.s doesn't work as is? Interrupt handlers should never execute code within the Go runtime, so why is your implementation required?

I updated the commit message as well. I ran into some undefined instructions on my target as it's ARMv5 and TamaGo is non-linux so it ran into the TODO item I previously removed. Are you suggesting rewriting the interrupt handlers instead?

As Go already supports ARM5 I'd like to understand what is the issue that you are trying to solve.

I suppose it did not really support the situation of having ARMv5 under a non-linux OS working 100%. There was just this one spot in the atomics assembly which was only working for ARMv6+.

[abarisani]

Thanks for the review, I updated the toolchain patch here now: MDr164/tamago-go@5aef39e

Do not remove the existing commented code (the TODO) from original Go.

Done

Patch ·armcas in atomic_arm.s has it alread has a GOARM conditional

Good idea, done

Explain why ·armcas in atomic_arm.s doesn't work as is? Interrupt handlers should never execute code within the Go runtime, so why is your implementation required?

I updated the commit message as well. I ran into some undefined instructions on my target as it's ARMv5 and TamaGo is non-linux so it ran into the TODO item I previously removed. Are you suggesting rewriting the interrupt handlers instead?

As Go already supports ARM5 I'd like to understand what is the issue that you are trying to solve.

I suppose it did not really support the situation of having ARMv5 under a non-linux OS working 100%. There was just this one spot in the atomics assembly which was only working for ARMv6+.

Looks much better thanks.

The interrupt handler should not run Go code which requires the runtime, please see the current pattern used in tamago and tamago-go.

In fact I am working on riscv64 interrupt handling as we speak for other RV64 targets which have just been included, so allow to me finish that and you can leverage on it.

Thanks!

[abarisani]

Please see d7cf6ac, 1ad5e0c for interrupt handling.

An example implementation is at https://github.com/usbarmory/kotama/blob/5ea39b280fc7b3939d6d93132d276c1e043354fc/cmd/sifive_u.go#L64

[MDr164]

I'm not yet 100% certain the newly added buildtags for either arm.5 or arm.6 are the best way to do this. Other than that I suppose the port is working pretty well now.

[MDr164]

Btw I created a small CLI tool that is able to create images that can directly be flashed to the onboard flash or an sd-card. This board does require some DDR2 training data to be present. I pulled that data from their GitHub.

[abarisani]

Thanks, I'll start my review next week!

[abarisani]

Haven't forgotten about this, just a little swamped but I promise I'll get to it.

[MDr164]

No worries, I just noticed a merge conflict with the development branch so I just updated the code to match the past commits. I can at least report that the code has been working well so far and I'm right now working on more drivers and the other PR so I'm also busy in the meantime :)

[abarisani]

This week I have time to work on this, can you see and address the pending comments? There have been also some changes that need sync up. Thanks!

[MDr164]

Quick summary once you get around reviewing it: I currently use a patched tamago-go toolchain based on your 1.26.2 tag with one commit on top: MDr164/tamago-go@f4bcfd4

Additionally I created a small script that I run locally to create correctly formatted sd-card images for this platform. I have not tested booting it from its internal NAND flash yet but that should work with the current code just fine I assume. I did not push the script into this PR as it felt to be out of scope to me.

One thing that tripped me up about GOARM buildtags is that they are apparently cumulative which the doc.go file they are described in doesn't claim. So:
GOARM=5 ~= arm.5
GOARM=6 ~= arm.5 || arm.6
GOARM=7 ~= arm.5 || arm.6 || arm.7

That's why I opted for using !arm.6 instead of going for the arm.5 buildtag.

[MDr164]

I'll update the code by moving everything to a common place for drivers inside the nuvoton package. I assume if there are other nuvoton chips using different IP cores we can still move things around. Do you think the one commit I got for the Go toolchain is worth adding to tamago-go? I guess if I send it upstream there would be confusion about why I run into this edgecase as it only really affects baremetal targets which aren't upstream yet.

[abarisani]

Yes, for starters we can add that commit to tamago-go, I'll ponder it a little more but in time for v1.26.3.

[abarisani]

Sorry for the delay, I am testing your tamago-go patch for inclusion, however it fails to pass this test (it hangs):

GOOS=tamago GOARCH=arm GOARM=5 bin/go test sync/atomic internal/sync internal/runtime/atomic

[abarisani]

I think the issue spawns from the fact that the testsuite is not designed for single-core processors without atomic operations, I had to workaround this issue here to enable lack of atomics on smaller RISC-V targets for kotama.

I think a similar workaround is needed though I am slightly uncomfortable in having the same approach for upstreaming it as I'd favor a conditional patch which is never touched on other architectures.

[MDr164]

Thanks for the feedback, I applied your suggestions and also reorganized the asm as mentioned by moving everything out of the board pkg except the pinmux which is specific to this board. For some reason doing the pinmux and the EarlyClockInit not via asm it fails to apply correctly. The new code has also been validated on my board here.

[abarisani]

pinmux.s still could use constants for register address and value, I'd still mux nanotime with the chip UID to have better seed value. The rest looks good and I'll be happy to merge it!

[MDr164]

I can change pinmux.s to use constants, sure. For the seed value of the PRNG peripheral of the soc I don't think it is worth using the UID. It is not really unique in that sense as all NUC980 chips share the same value from what I read in the datasheet. The closely related NUC970 would have the same peripheral but a different ID for that matter but I'm not sure if having a chip sku specific seed provide any benefit over simply using the timer value. I think even better would be to query one of the ADCs but I think those are also connected on the pinmux only and not internally to some temp sensor of the chip itself.

[abarisani]

I can change pinmux.s to use constants, sure. For the seed value of the PRNG peripheral of the soc I don't think it is worth using the UID. It is not really unique in that sense as all NUC980 chips share the same value from what I read in the datasheet. The closely related NUC970 would have the same peripheral but a different ID for that matter but I'm not sure if having a chip sku specific seed provide any benefit over simply using the timer value. I think even better would be to query one of the ADCs but I think those are also connected on the pinmux only and not internally to some temp sensor of the chip itself.

I don't quite understand if the PRNG becomes predictable with the same Seed or not.

If so we might want to consider panicking if Seed is empty and delegate that decision to board packages.

[MDr164]

I think a similar workaround is needed though I am slightly uncomfortable in having the same approach for upstreaming it as I'd favor a conditional patch which is never touched on other architectures.

I looked deeper into this and found an instruction that is available on real hardware as well as on Qemu that doesn't cause issues. This commit here now works on the actual board and on Qemu. I added the rational in the code as a lenghty comment which you're not a fan of so feel free to strip it out: MDr164/tamago-go@b32a132

[MDr164]

I don't quite understand if the PRNG becomes predictable with the same Seed or not.

The datasheet is not really very descriptive to be honest so maybe it is.

If so we might want to consider panicking if Seed is empty and delegate that decision to board packages.

I think that would be a fair compromise I'm willing to make, just have the user provide something (e.g. an ADC readout if they know their board as the pinux set with ADC inputs).

[abarisani]

I think a similar workaround is needed though I am slightly uncomfortable in having the same approach for upstreaming it as I'd favor a conditional patch which is never touched on other architectures.

I looked deeper into this and found an instruction that is available on real hardware as well as on Qemu that doesn't cause issues. This commit here now works on the actual board and on Qemu. I added the rational in the code as a lenghty comment which you're not a fan of so feel free to strip it out: MDr164/tamago-go@b32a132

Please use Go assembly for the ORR, R0, 0xc0 instruction rather than WORD. I see that Go stdlib has SWPW R3, (R7), R9 so kindly check if that's available as well.

Please make a PR against https://github.com/usbarmory/tamago-go/tree/tamago1.26.4 for testing and inclusion in the next release, also please consider making an upstream PR to Go given that GOARM=5 is officially supported and I feel this is not specific to GOOS=tamago ?

[abarisani]

I don't quite understand if the PRNG becomes predictable with the same Seed or not.

The datasheet is not really very descriptive to be honest so maybe it is.

If so we might want to consider panicking if Seed is empty and delegate that decision to board packages.

I think that would be a fair compromise I'm willing to make, just have the user provide something (e.g. an ADC readout if they know their board as the pinux set with ADC inputs).

For now I'd simply use nanotime in the SoC package, which is required as you need to start timers first, only if PRNG.Seed == 0, so that a previous board override is respected.

[abarisani]

Additionally I think PRNG should allow re-seeding at runtime, so probably PRNG.Seed should become a non-exported field to have PRNG.Seed() to set it and negate seeded.

[MDr164]

Also please consider making an upstream PR to Go given that GOARM=5 is officially supported and I feel this is not specific to GOOS=tamago?

Yes, it's an upstream issue but as my commit message mentioned it current doesn't show up upstream as Linux has a helper to mitigate the problem. Tamago/baremetal would be the first user of this path. While I agree that it should land upstream for sure I'm not 100% certain if it's accepted if there are not situations upstream where this assembly fixes an issue other than in baremetal environments. I can try though.

[MDr164]

I don't quite understand if the PRNG becomes predictable with the same Seed or not.

The datasheet is not really very descriptive to be honest so maybe it is.

If so we might want to consider panicking if Seed is empty and delegate that decision to board packages.

I think that would be a fair compromise I'm willing to make, just have the user provide something (e.g. an ADC readout if they know their board as the pinux set with ADC inputs).

For now I'd simply use nanotime in the SoC package, which is required as you need to start timers first, only if PRNG.Seed == 0, so that a previous board override is respected.

Makes sense, the vendors Linux driver simply uses the kernel jiffies as a seed with no way to change/override it. So in some sense we got parity but I agree that we could do better.

[abarisani]

Also please consider making an upstream PR to Go given that GOARM=5 is officially supported and I feel this is not specific to GOOS=tamago?

Yes, it's an upstream issue but as my commit message mentioned it current doesn't show up upstream as Linux has a helper to mitigate the problem. Tamago/baremetal would be the first user of this path. While I agree that it should land upstream for sure I'm not 100% certain if it's accepted if there are not situations upstream where this assembly fixes an issue other than in baremetal environments. I can try though.

Then let us keep it as part of our upstreaming proposal given that it's only specific to us. Thanks for clarifying.

I assume there are no !Linux GOARM=5 supported platforms upstream then?

[abarisani]

I don't quite understand if the PRNG becomes predictable with the same Seed or not.

The datasheet is not really very descriptive to be honest so maybe it is.

If so we might want to consider panicking if Seed is empty and delegate that decision to board packages.

I think that would be a fair compromise I'm willing to make, just have the user provide something (e.g. an ADC readout if they know their board as the pinux set with ADC inputs).

For now I'd simply use nanotime in the SoC package, which is required as you need to start timers first, only if PRNG.Seed == 0, so that a previous board override is respected.

Makes sense, the vendors Linux driver simply uses the kernel jiffies as a seed with no way to change/override it. So in some sense we got parity but I agree that we could do better.

By the way FIPS mode is implemented under TamaGo, just like in Go, and uses memory jiffies for entropy. Though this is for compliance and I cannot comment on its security, still an option for users to enable.

[MDr164]

Then let us keep it as part of our upstreaming proposal given that it's only specific to us.

Well it's specific to non-linux armv5 which tamago seems to be the first candidate.

I assume there are no !Linux GOARM=5 supported platforms upstream then?

Looks like it, or other operating systems also install helpers to catch the instructions that wouldn't work standalone but I only saw Linux mentioned upstream in the source.

[MDr164]

Added a commit to allow calling *PRNG.Seed(seed uint32) at runtime and seeded using nanotime by default now and also the toolchain commit. I think that covers everything now?

[abarisani]

Merged, thanks for your patience in addressing my review and this contribution overall!

[MDr164]

Merged, thanks for your patience in addressing my review and this contribution overall!

Likewise thanks for the reviews and patience with my iterations. With the learnings I'll make sure the next ports will be much more smooth :)

[abarisani]

We broke GoTEE by initializing features after the vector table.

Can you test if this patch doesn't break ARMv5/NUC980 please, it fixes GoTEE and it seems to me the best way to go about this:

diff --git a/arm/arm.go b/arm/arm.go
index cbae403..a82e4ba 100644
--- a/arm/arm.go
+++ b/arm/arm.go
@@ -86,8 +86,8 @@ func (cpu *CPU) Init() {
 	goos.Exit = exit
 	goos.Idle = cpu.DefaultIdleGovernor
 
-	cpu.initVectorTable()
 	cpu.initFeatures()
+	cpu.initVectorTable()
 }
 
 // Mode returns the processor mode.
diff --git a/arm/features.go b/arm/features.go
index da6781b..491ecb9 100644
--- a/arm/features.go
+++ b/arm/features.go
@@ -22,6 +22,11 @@ const (
 	ID_PFR1_GENERIC_TIMER_MASK     = 0xf0000
 )
 
+// ARM architecture versions
+const (
+	MIDR_ARCH_ARMV6 = 0b0111
+)
+
 // defined in features.s
 func read_midr() uint32
 func read_idpfr0() uint32
@@ -36,7 +41,7 @@ func (cpu *CPU) MIDR() uint32 {
 func (cpu *CPU) initFeatures() {
 	// Do not set read features on cores with fixed exception vectors (e.g.
 	// ARMv5) which have no CP15 feature ID registers.
-	if cpu.vbar == 0 {
+	if read_midr() < MIDR_ARCH_ARMV6 {
 		return
 	}
 

[MDr164]

The MIDR call returns a value that is higher for some reason on the NUC980. Let me quickly try to create a better comparison check in initFeatures asap to get GoTEE fixed again.

EDIT ok found it, the MIDR value contains several fields and we only need to compare bits 16 to 19 and not all of them: https://arm.jonpalmisc.com/latest_sysreg/AArch32-midr