Re: [PATCH 13/15] rockchip: rk3328: Add support to build bootable SPI image

[Dragan Simic <dsimic@manjaro.org>]

Hello Jonas,

On 2024-02-08 22:54, Jonas Karlman wrote:
> On 2024-02-08 04:53, Dragan Simic wrote:
>> On 2024-02-07 01:02, Jonas Karlman wrote:
>>> Similar to RK35xx the BootRom in RK3328 can read all data and look 
>>> for
>>> idbloader at 0x8000, same as on SD and eMMC.
>>> 
>>> Use the rksd format and modify the mkimage offset to generate a
>>> bootable
>>> u-boot-rockchip-spi.bin that can be written to 0x0 of SPI NOR flash.
>>> 
>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>> 
>> Could you, please, clarify a bit why the "rkspi" format isn't used
>> instead of "rksd"?
> 
> Not really sure how I can clarify this.
> 
> As stated in commit message, the bootrom will read all bytes and use
> same boot offset as the sd/emmc-format. The "legacy" spi-format only
> read first 2kb of each 4kb of storage, see tools/rkspi.c.
> 
> A few bootrom versions:
> - rk3288: 320A 20131116 V100
> - rk3399: 330C 20160118 V100
> - rk3328: 320C 20161117 V100
> - rk3308: 330E 20180203 V100
> 
> I am not sure if the rkspi format must be used for all bootrom prior to
> and including rk3399 and all versions after that should use the rksd
> format. I have only ever used boot from spi flash on rk3399, rk3328 and
> rk35xx. And for those that has been true.

Ah, thank you very much for refreshing my memory.  I somehow forgot 
about
the bug that the RK3399 BROM suffers from.

Please see also one small suggestion below.

>>> ---
>>>  arch/arm/dts/rk3328-u-boot.dtsi        | 11 +++++++++++
>>>  arch/arm/mach-rockchip/rk3328/rk3328.c |  1 +
>>>  2 files changed, 12 insertions(+)
>>> 
>>> diff --git a/arch/arm/dts/rk3328-u-boot.dtsi
>>> b/arch/arm/dts/rk3328-u-boot.dtsi
>>> index b90d78878d77..2a5dca97dd4b 100644
>>> --- a/arch/arm/dts/rk3328-u-boot.dtsi
>>> +++ b/arch/arm/dts/rk3328-u-boot.dtsi
>>> @@ -120,3 +120,14 @@
>>>  &usb20_otg {
>>>  	hnp-srp-disable;
>>>  };
>>> +
>>> +#ifdef CONFIG_ROCKCHIP_SPI_IMAGE
>>> +&binman {
>>> +	simple-bin-spi {
>>> +		mkimage {
>>> +			args = "-n", CONFIG_SYS_SOC, "-T", "rksd";

Perhaps there's no need to use CONFIG_SYS_SOC there, because this is an
U-Boot SoC dtsi specific to the RK3328.  Furthermore, I find it much 
more
readable (and grep-able, which is also important) when the exact name of
the SoC is specified here.

This note also applies to the other U-Boot SoC dtsi files.

>>> +			offset = <0x8000>;
>>> +		};
>>> +	};
>>> +};
>>> +#endif
>>> diff --git a/arch/arm/mach-rockchip/rk3328/rk3328.c
>>> b/arch/arm/mach-rockchip/rk3328/rk3328.c
>>> index b591d38fe412..b82b209de9e2 100644
>>> --- a/arch/arm/mach-rockchip/rk3328/rk3328.c
>>> +++ b/arch/arm/mach-rockchip/rk3328/rk3328.c
>>> @@ -36,6 +36,7 @@
>>> 
>>>  const char * const boot_devices[BROM_LAST_BOOTSOURCE + 1] = {
>>>  	[BROM_BOOTSOURCE_EMMC] = "/mmc@ff520000",
>>> +	[BROM_BOOTSOURCE_SPINOR] = "/spi@ff190000/flash@0",
>>>  	[BROM_BOOTSOURCE_SD] = "/mmc@ff500000",
>>>  };