linux4sam / at91bootstrap

Second level bootloader for Microchip SoC (aka AT91)
https://www.linux4sam.org/linux4sam/bin/view/Linux4SAM/AT91Bootstrap4
112 stars 232 forks source link

crt0_gnu.S: always pass along r4 to next stage #159

Closed a3f closed 1 year ago

a3f commented 1 year ago

At least on the SAMA5D2, D3 and D4, the BootROM encodes information about the boot source into the r4 register. AT91Bootstrap discards this information on all SoCs except for the SAMA5D3. Fix this by storing r4 at a universally applicable location (first word of stack) and passing it along to the next boot stage, so it can decide how to act on this.

In order to keep backwards compatibility with next stage boot firmware that looks at the GPBR instead of r4, we keep the store to GPBR unchanged and replace only the load with aforementioned stack top.

Acked-by: Nicolas Ferre nicolas.ferre@microchip.com
Signed-off-by: Ahmad Fatoum a.fatoum@pengutronix.de

noglitch commented 1 year ago

Hi, Thanks for this patch @a3f, I agree with it: Acked-by: Nicolas Ferre nicolas.ferre@microchip.com

Thanks, best regards, Nicolas

ehristev commented 1 year ago

I see that right now for SAMA5D3 the R4 information is being read for a dedicated GPBR register. With your change, the value of R4 is being used. I am not sure why it was decided to read from there instead of using the R4 as you do now.

ehristev commented 1 year ago

I see that right now for SAMA5D3 the R4 information is being read for a dedicated GPBR register. With your change, the value of R4 is being used. I am not sure why it was decided to read from there instead of using the R4 as you do now.

I misread the code. It appears the R4 is being saved to the GPBR instead of read from it. So maybe someone is expecting a value there ? Can't we keep this behavior ? Also, it would be interesting to see if we could set R4 back to it's original romcode value before booting the next stage, or this register is maybe used by the next stage for some other purpose. For linux, this may be accurate:

r0 = 0,

r1 = machine type number discovered in (3) above.

r2 = physical address of tagged list in system RAM, or physical address of device tree block (dtb) in system RAM

So r4 might be set to the romcode boot value ?

a3f commented 1 year ago

So maybe someone is expecting a value there ? Can't we keep this behavior ?

Sure, I can do that if you prefer.

Also, it would be interesting to see if we could set R4 back to it's original romcode value before booting the next stage, or this register is maybe used by the next stage for some other purpose.

This is what I am already doing here?

So r4 might be set to the romcode boot value [for Linux]?

No. Platform data is meant to be passed via device tree. Hacking the kernel to store extra registers isn't something that should be encouraged. For my use case I have this: https://lore.barebox.org/barebox/20221214105831.844421-11-a.fatoum@pengutronix.de/T/#u

This results in $bootsource,$bootsource_instance variables that can be evaluated on the bootloader shell and in /chosen/bootsource, /chosen/bootsource-instance and possibly /chosen/bootsource-device properties in the fixed up Linux device trees.

If someone wants the equivalent with AT91Bootstrap booting Linux directly, they should either fix up the DT or put a nvmem-rmem DT node on top of the GPBR or SRAM location where the bootsource is stored. That's a bit more involved though than the straight forward change here, which enables interoperation with barebox or U-Boot.

a3f commented 1 year ago

v2:

Thanks for the review.

ehristev commented 1 year ago

Applied, thanks !