[PATCH 0/4] rockchip: rk3588: Fix SPI flash bootsource id values

[PATCH 0/4] rockchip: rk3588: Fix SPI flash bootsource id values

[Jonas Karlman]

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.

Since then the following bootsource id values have been observed on
different RK3588 devices:

- Radxa ROCK 5 Model A - fspim0_pins - rk3588s
- Xunlong Orange Pi 5 - fspim0_pins - rk3588s
    brom_bootdevice_id: 3

- FriendlyElec NanoPC-T6 - fspim1_pins - rk3588
- Xunlong Orange Pi 5 Plus - fspim1_pins - rk3588
    brom_bootdevice_id: 4

- Radxa ROCK 5 Model B - fspim2_pins - rk3588
    brom_bootdevice_id: 6

It has now been confirmed 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.

This series adjusts the bootsource enum values, updates the boot_devices
array to fix booting from SPI flash on RK3588 devices using fspim1_pins.
It also enable building of a bootable SPI image for affected devices.

John Clark (1):
  rockchip: rk3588-nanopc-t6: Build SPI image

Jonas Karlman (2):
  rockchip: rk3588: Fix boot from SPI flash
  rockchip: rk3588s-orangepi-5: Build SPI image

Slawomir Stepien (1):
  rockchip: rk3588-orangepi-5-plus: Build SPI image

 arch/arm/include/asm/arch-rockchip/bootrom.h | 4 +++-
 arch/arm/mach-rockchip/rk3588/rk3588.c       | 5 +++--
 configs/nanopc-t6-rk3588_defconfig           | 1 +
 configs/orangepi-5-plus-rk3588_defconfig     | 1 +
 configs/orangepi-5-rk3588s_defconfig         | 1 +
 5 files changed, 9 insertions(+), 3 deletions(-)

-- 
2.42.0

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

[Jonas Karlman]

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,
+	BROM_BOOTSOURCE_FSPI_M2 = 6,
 	BROM_BOOTSOURCE_SD = 5,
-	BROM_BOOTSOURCE_SPINOR_RK3588 = 6,
 	BROM_BOOTSOURCE_USB = 10,
 	BROM_LAST_BOOTSOURCE = BROM_BOOTSOURCE_USB
 };
diff --git a/arch/arm/mach-rockchip/rk3588/rk3588.c b/arch/arm/mach-rockchip/rk3588/rk3588.c
index b1f535fad505..322164e9b307 100644
--- a/arch/arm/mach-rockchip/rk3588/rk3588.c
+++ b/arch/arm/mach-rockchip/rk3588/rk3588.c
@@ -39,9 +39,10 @@ DECLARE_GLOBAL_DATA_PTR;
 
 const char * const boot_devices[BROM_LAST_BOOTSOURCE + 1] = {
 	[BROM_BOOTSOURCE_EMMC] = "/mmc@fe2e0000",
-	[BROM_BOOTSOURCE_SPINOR] = "/spi@fe2b0000/flash@0",
+	[BROM_BOOTSOURCE_FSPI_M0] = "/spi@fe2b0000/flash@0",
+	[BROM_BOOTSOURCE_FSPI_M1] = "/spi@fe2b0000/flash@0",
+	[BROM_BOOTSOURCE_FSPI_M2] = "/spi@fe2b0000/flash@0",
 	[BROM_BOOTSOURCE_SD] = "/mmc@fe2c0000",
-	[BROM_BOOTSOURCE_SPINOR_RK3588] = "/spi@fe2b0000/flash@0",
 };
 
 static struct mm_region rk3588_mem_map[] = {
-- 
2.42.0

[PATCH 2/4] rockchip: rk3588-nanopc-t6: Build SPI image

[Jonas Karlman]

From: John Clark <inindev@gmail.com>

Enable building of the SPI image, u-boot-rockchip-spi.bin, now that we
know what bootsource id values BootRom use for SPI flash on RK3588.

Fixes: b0b8086898f8 ("board: rockchip: add FriendlyElec NanoPC-T6 rk3588 board")
Signed-off-by: John Clark <inindev@gmail.com>
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 configs/nanopc-t6-rk3588_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configs/nanopc-t6-rk3588_defconfig b/configs/nanopc-t6-rk3588_defconfig
index 070399ce2a86..daa13a4ffff8 100644
--- a/configs/nanopc-t6-rk3588_defconfig
+++ b/configs/nanopc-t6-rk3588_defconfig
@@ -14,6 +14,7 @@ CONFIG_SF_DEFAULT_MODE=0x2000
 CONFIG_DEFAULT_DEVICE_TREE="rk3588-nanopc-t6"
 CONFIG_ROCKCHIP_RK3588=y
 CONFIG_SPL_ROCKCHIP_COMMON_BOARD=y
+CONFIG_ROCKCHIP_SPI_IMAGE=y
 CONFIG_SPL_SERIAL=y
 CONFIG_SPL_STACK_R_ADDR=0x600000
 CONFIG_TARGET_NANOPCT6_RK3588=y
-- 
2.42.0

[PATCH 3/4] rockchip: rk3588-orangepi-5-plus: Build SPI image

[Jonas Karlman]

From: Slawomir Stepien <sst@poczta.fm>

Enable building of the SPI image, u-boot-rockchip-spi.bin, now that we
know what bootsource id values BootRom use for SPI flash on RK3588.

Fixes: b51cf8bb09b6 ("board: rockchip: Add Xunlong Orange Pi 5 Plus")
Signed-off-by: Slawomir Stepien <sst@poczta.fm>
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 configs/orangepi-5-plus-rk3588_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configs/orangepi-5-plus-rk3588_defconfig b/configs/orangepi-5-plus-rk3588_defconfig
index 04736996217e..e1cd753a6d18 100644
--- a/configs/orangepi-5-plus-rk3588_defconfig
+++ b/configs/orangepi-5-plus-rk3588_defconfig
@@ -14,6 +14,7 @@ CONFIG_SF_DEFAULT_MODE=0x2000
 CONFIG_DEFAULT_DEVICE_TREE="rk3588-orangepi-5-plus"
 CONFIG_ROCKCHIP_RK3588=y
 CONFIG_SPL_ROCKCHIP_COMMON_BOARD=y
+CONFIG_ROCKCHIP_SPI_IMAGE=y
 CONFIG_SPL_SERIAL=y
 CONFIG_SPL_STACK_R_ADDR=0x600000
 CONFIG_TARGET_EVB_RK3588=y
-- 
2.42.0

[PATCH 4/4] rockchip: rk3588s-orangepi-5: Build SPI image

[Jonas Karlman]

Enable building of the SPI image, u-boot-rockchip-spi.bin, now that we
know what bootsource id values BootRom use for SPI flash on RK3588.

Fixes: 28c5f941edf7 ("board: rockchip: Add Xunlong Orange Pi 5")
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 configs/orangepi-5-rk3588s_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configs/orangepi-5-rk3588s_defconfig b/configs/orangepi-5-rk3588s_defconfig
index feb45a53853b..1e33c832ec67 100644
--- a/configs/orangepi-5-rk3588s_defconfig
+++ b/configs/orangepi-5-rk3588s_defconfig
@@ -13,6 +13,7 @@ CONFIG_SF_DEFAULT_MODE=0x2000
 CONFIG_DEFAULT_DEVICE_TREE="rk3588s-orangepi-5"
 CONFIG_ROCKCHIP_RK3588=y
 CONFIG_SPL_ROCKCHIP_COMMON_BOARD=y
+CONFIG_ROCKCHIP_SPI_IMAGE=y
 CONFIG_SPL_SERIAL=y
 CONFIG_SPL_STACK_R_ADDR=0x600000
 CONFIG_TARGET_EVB_RK3588=y
-- 
2.42.0

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

[Slawomir Stepien]

On lis 12, 2023 10: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

Tested-by: Slawomir Stepien <sst@poczta.fm>

> 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,
> +	BROM_BOOTSOURCE_FSPI_M2 = 6,
>  	BROM_BOOTSOURCE_SD = 5,
> -	BROM_BOOTSOURCE_SPINOR_RK3588 = 6,
>  	BROM_BOOTSOURCE_USB = 10,
>  	BROM_LAST_BOOTSOURCE = BROM_BOOTSOURCE_USB
>  };
> diff --git a/arch/arm/mach-rockchip/rk3588/rk3588.c b/arch/arm/mach-rockchip/rk3588/rk3588.c
> index b1f535fad505..322164e9b307 100644
> --- a/arch/arm/mach-rockchip/rk3588/rk3588.c
> +++ b/arch/arm/mach-rockchip/rk3588/rk3588.c
> @@ -39,9 +39,10 @@ DECLARE_GLOBAL_DATA_PTR;
>  
>  const char * const boot_devices[BROM_LAST_BOOTSOURCE + 1] = {
>  	[BROM_BOOTSOURCE_EMMC] = "/mmc@fe2e0000",
> -	[BROM_BOOTSOURCE_SPINOR] = "/spi@fe2b0000/flash@0",
> +	[BROM_BOOTSOURCE_FSPI_M0] = "/spi@fe2b0000/flash@0",
> +	[BROM_BOOTSOURCE_FSPI_M1] = "/spi@fe2b0000/flash@0",
> +	[BROM_BOOTSOURCE_FSPI_M2] = "/spi@fe2b0000/flash@0",
>  	[BROM_BOOTSOURCE_SD] = "/mmc@fe2c0000",
> -	[BROM_BOOTSOURCE_SPINOR_RK3588] = "/spi@fe2b0000/flash@0",
>  };
>  
>  static struct mm_region rk3588_mem_map[] = {

-- 
Slawomir Stepien

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

[Quentin Schulz]

Hi Jonas,

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)?

Cheers,
Quentin

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

[Slawomir Stepien]

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.

-- 
Slawomir Stepien

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

[Tom Rini]

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

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.

-- 
Tom

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

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

[John Clark]

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

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

[Tom Rini]

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

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.

-- 
Tom

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

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

[Jonas Karlman]

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.

Regards,
Jonas

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

[Tom Rini]

[-- 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 --]