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

[Tom Rini]

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 at 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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20231117/f5c28eae/attachment.sig>