Re: [PATCH 1/4] rockchip: rk3588: Fix boot from SPI flash

[John Clark <inindev@gmail.com>]

On 11/17/23 12:50 PM, Tom Rini wrote:
> On Fri, Nov 17, 2023 at 03:03:39PM +0100, Slawomir Stepien wrote:
>> On lis 14, 2023 15:06, Quentin Schulz wrote:
>>> Hi Jonas,
>> Hi Quentin
>>
>>> On 11/12/23 11:26, Jonas Karlman wrote:
>>>> The commit fd6e425be243 ("rockchip: rk3588-rock-5b: Enable boot from SPI
>>>> NOR flash") added a new BROM_BOOTSOURCE_SPINOR_RK3588 with value 6.
>>>>
>>>> At the time the reason for this new bootsource id value 6 was unknown.
>>>>
>>>> We now know that the BootRom on RK3588 use different bootsource id
>>>> values depending on the iomux used by the flash spi controller, and not
>>>> by the type of spi nor or spi nand flash used.
>>>>
>>>> Add the following defines and use them for RK3588 boot_devices.
>>>>
>>>> - BROM_BOOTSOURCE_FSPI_M0 = 3
>>>> - BROM_BOOTSOURCE_FSPI_M1 = 4
>>>> - BROM_BOOTSOURCE_FSPI_M2 = 6
>>>>
>>>> Fixes: fd6e425be243 ("rockchip: rk3588-rock-5b: Enable boot from SPI NOR flash")
>>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>>> ---
>>>>    arch/arm/include/asm/arch-rockchip/bootrom.h | 4 +++-
>>>>    arch/arm/mach-rockchip/rk3588/rk3588.c       | 5 +++--
>>>>    2 files changed, 6 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/arch/arm/include/asm/arch-rockchip/bootrom.h b/arch/arm/include/asm/arch-rockchip/bootrom.h
>>>> index 7dab18fbc3fb..f78337397d63 100644
>>>> --- a/arch/arm/include/asm/arch-rockchip/bootrom.h
>>>> +++ b/arch/arm/include/asm/arch-rockchip/bootrom.h
>>>> @@ -47,8 +47,10 @@ enum {
>>>>    	BROM_BOOTSOURCE_EMMC = 2,
>>>>    	BROM_BOOTSOURCE_SPINOR = 3,
>>>>    	BROM_BOOTSOURCE_SPINAND = 4,
>>>> +	BROM_BOOTSOURCE_FSPI_M0 = 3,
>>>> +	BROM_BOOTSOURCE_FSPI_M1 = 4,
>>> I'm a bit wary of two pairs of enums sharing the same value, especially when
>>> we want to use them as offset in a static definition of an array.
>>>
>>> Should we #ifdef it (meh) for RK3588?
>>> Should we add a suffix like before for identifying RK3588-specific options?
>>>
>>> At the very least explicit that those are RK3588-specific in a comment for
>>> both conflicts (the ones that apply to everything except RK3588 to say to
>>> use only for !RK3588, and the ones that apply to RK3588 only)?
>> Can you say why it is so important to know that given enum is specific to given CPU here in the
>> header file? I think that the enums in the bootrom.h should be as generic as possible.
>>
>> By using the possible enums in a static array, "solves" the problem of assigning the boot source to
>> specific CPU. There is not need to make such grouping in the bootrom.h.
> Do we have any insight as to why Rockchip re-used those values? Looking
> at the header I see RK3588 has a different SPINOR value than others.
> Does RK3588 share the same value for other sources? How much of
> bootrom.h is still correct for RK3588? I'd rather not have to move to
> having bootrom_${soc}.h like so many other headers are, and if it's just
> these values, it might be cleaner to #if .. #else .. #endif the whole
> enum, and then re-evaluate things abased on whatever the next new SoC
> does here.
>
Which c specification does u-boot follow?  Duplicate values in 
enumerations are explicitly allowed in c as far as I ever have known.

"The use of enumerators with = may produce enumeration constants with 
values that duplicate other values in the same enumeration."
https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2310.pdf