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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 5455 bytes --]

On Fri, Nov 17, 2023 at 08:53:49PM +0100, Jonas Karlman wrote:
> On 2023-11-17 20:07, Tom Rini wrote:
> > On Fri, Nov 17, 2023 at 01:50:58PM -0500, John Clark wrote:
> >>
> >> 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
> > 
> > Sorry I wasn't clear. It's less about C specification and more about
> > readability. We could do, but I feel is odd readability-wise is:
> > enum {
> >         BROM_BOOTSOURCE_NAND = 1,
> >         BROM_BOOTSOURCE_EMMC = 2,
> >         BROM_BOOTSOURCE_SPINOR = 3,
> >         BROM_BOOTSOURCE_SPINAND = 4,
> >         BROM_BOOTSOURCE_SD = 5,
> >         BROM_BOOTSOURCE_FSPI_M0_RK3588 = 3,
> > 	BROM_BOOTSOURCE_FSPI_M1_RK3588 = 4,
> >         BROM_BOOTSOURCE_FSPI_M2_RK3588 = 5,
> >         BROM_BOOTSOURCE_SPINOR_RK3588 = 6,
> >         BROM_BOOTSOURCE_USB = 10,
> >         BROM_LAST_BOOTSOURCE = BROM_BOOTSOURCE_USB
> > };
> > 
> > But how many of the other values are still correct for RK3588? The TRM I
> > found quickly does say that NAND and SD/eMMC are valid options, and USB,
> > but I didn't see a table that mapped back to the values here.
> > 
> 
> Remaining values that is supported should be same, the only difference
> for RK3588 is that the old SPINOR=3/SPINAND=4 values now map to flash
> spi iomux M0 and M1, and compared to bootrom in other SoCs cannot be
> used to determine if SPI NOR or SPI NAND was the boot source. The new
> value 6 maps to flash spi iomux M2. What type of flash spi media the
> device was booted from, nor/nand, does not matter for RK3588.
> 
> Guess I can add a enum for the rk3588 FSPI_M0/M1/M2 values directly in
> rk3588.c where they are used, and when next SoC re-use iomux for flash
> spi controller they can be moved to bootrom.h.

OK, thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]