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

[Jonas Karlman]

Hi Dragan,

On 2024-02-10 18:18, Dragan Simic wrote:
> 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 at 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.

Consistency is my main concern so I would rather keep using SYS_SOC, it
is already being used at the two other override locations.

If we can get confirmation that it is brom in SoCs chronologically up to
and including rk3399 that is affected by the read 2kb per 4kb of spi
issue we should be able to rework this into a more proper and future
proof way.

Regards,
Jonas

> 
>>>> +			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 at ff520000",
>>>> +	[BROM_BOOTSOURCE_SPINOR] = "/spi at ff190000/flash at 0",
>>>>  	[BROM_BOOTSOURCE_SD] = "/mmc at ff500000",
>>>>  };