Re: [PATCH 1/4] rockchip: rk3588: Fix boot from SPI flash
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
> > ---
> > 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
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
> Fixes: fd6e425be243 ("rockchip: rk3588-rock-5b: Enable boot from SPI NOR
> flash")
> Signed-off-by: Jonas Karlman
> ---
> 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@fe2e",
> - [BROM_BOOTSOURCE_SPINOR] = "/spi@fe2b/flash@0",
> + [BROM_BOOTSOURCE_FSPI_M0] = "/spi@fe2b/flash@0",
> + [BROM_BOOTSOURCE_FSPI_M1] = "/spi@fe2b/flash@0",
> + [BROM_BOOTSOURCE_FSPI_M2] = "/spi@fe2b/flash@0",
> [BROM_BOOTSOURCE_SD] = "/mmc@fe2c",
> - [BROM_BOOTSOURCE_SPINOR_RK3588] = "/spi@fe2b/flash@0",
> };
>
> static struct mm_region rk3588_mem_map[] = {
--
Slawomir Stepien
Re: [PATCH 1/2] rockchip: rk3588: Add SPINAND as boot device
On lis 03, 2023 11:03, Jonas Karlman wrote:
> On 2023-11-03 10:34, Slawomir Stepien wrote:
> > Hi Jonas and Kever
> >
> > Thanks for clarifying the situation Kever! Please see below my proposition.
> > What do you think?
>
> Yes, this matches my proposition at [1], and I have a series queued now
> that we have gotten the answer.
>
> Have included your commit to enable ROCKCHIP_SPI_IMAGE on Orange Pi 5
> Plus, John Clark's commit to enable on NanoPC-T6 my own to enable on
> Orange Pi 5.
>
> Should hit the list later today after full runtime testing.
Lovely, thank you! I have read all your comments from below. I'm trusting your
judgment.
> > On lis 02, 2023 15:27, Kever Yang wrote:
> >> Hi Jonas,
> >>
> >> On 2023/11/1 00:45, Jonas Karlman wrote:
> >>> On 2023-10-31 17:13, Slawomir Stepien wrote:
> >>>> Mark the flash@0 as BROM_BOOTSOURCE_SPINAND.
> >>>> Fixes use of same-as-spl in u-boot,spl-boot-order prop on Orange Pi 5
> >>>> Plus.
> >>>>
> >>>> Signed-off-by: Slawomir Stepien
> >>>> ---
> >>>> arch/arm/mach-rockchip/rk3588/rk3588.c | 1 +
> >>>> 1 file changed, 1 insertion(+)
> >>>>
> >>>> diff --git a/arch/arm/mach-rockchip/rk3588/rk3588.c
> >>>> b/arch/arm/mach-rockchip/rk3588/rk3588.c
> >>>> index b1f535fad5..0c8f10a97c 100644
> >>>> --- a/arch/arm/mach-rockchip/rk3588/rk3588.c
> >>>> +++ b/arch/arm/mach-rockchip/rk3588/rk3588.c
> >>>> @@ -42,6 +42,7 @@ const char * const boot_devices[BROM_LAST_BOOTSOURCE +
> >>>> 1] = {
> >>>> [BROM_BOOTSOURCE_SPINOR] = "/spi@fe2b/flash@0",
> >>>> [BROM_BOOTSOURCE_SD] = "/mmc@fe2c",
> >>>> [BROM_BOOTSOURCE_SPINOR_RK3588] = "/spi@fe2b/flash@0",
> >>>> +[BROM_BOOTSOURCE_SPINAND] = "/spi@fe2b/flash@0",
> >>> This is not correct, please see [1] for an open discussion on this issue.
> >>>
> >>> Kever: Do you have any more insights into the question on [1].
> >>
> >> As I reply in another mail, the FSPI M0~M2 is all the option for SPI
> >> NAND/NOR in rk3588.
> >
> > So based on that info, something like this?:
> >
> > 1. BROM_BOOTSOURCE_SPINAND - removed (not used at all).
>
> Should not be removed, because this is still valid for old generation
> SoCs.
>
> > 2. BROM_BOOTSOURCE_SPINOR_RK3588 - removed (not needed after this patch).
>
> Yes, this should be replaced with FSPI_M2.
>
> > 3. Updated also the rk3568.c since it also has FSPI M0 (only this one mux),
> > so I would assume it
> > will be value of 3 there too.
>
> No need to change this, the RK356x does not have iomux on fspi, so lets
> treat this as an old generation SoCs.
>
> > 4. The BROM_BOOTSOURCE_SPINOR is still there, since the rk3399 does not use
> > the FSPI (that is my
> > understanding after checking the TRM doc).
>
> Agreed, should not be changed for same reasons as SPINAND.
>
> >
> > diff --git a/arch/arm/include/asm/arch-rockchip/bootrom.h
> > b/arch/arm/include/asm/arch-rockchip/bootrom.h
> > index 7dab18fbc3..9f16d28aec 100644
> > --- a/arch/arm/include/asm/arch-rockchip/bootrom.h
> > +++ b/arch/arm/include/asm/arch-rockchip/bootrom.h
> > @@ -46,9 +46,10 @@ enum {
> > BROM_BOOTSOURCE_NAND = 1,
> > BROM_BOOTSOURCE_EMMC = 2,
> > BROM_BOOTSOURCE_SPINOR = 3,
> > - BROM_BOOTSOURCE_SPINAND = 4,
> > + BROM_BOOTSOURCE_FSPI_M0 = 3,
> > + BROM_BOOTSOURCE_FSPI_M1 = 4,
> > BROM_BOOTSOURCE_SD = 5,
> > - BROM_BOOTSOURCE_SPINOR_RK3588 = 6,
> > + BROM_BOOTSOURCE_FSPI_M2 = 6,
> > BROM_BOOTSOURCE_USB = 10,
> > BROM_LAST_BOOTSOURCE = BROM_BOOTSOURCE_USB
> > };
> > diff --git a/arch/arm/mach-rockchip/rk3568/rk3568.c
> > b/arch/arm/mach-rockchip/rk3568/rk3568.c
> > index 69ef19cc85..ccb63c6a88 100644
> > --- a/arch/arm/mach-rockchip/rk3568/rk3568.c
> > +++ b/arch/arm/mach-rockchip/rk3568/rk3568.c
> > @@ -83,7 +83,7 @@ static struct mm_region rk3568_mem_map[] = {
> >
> > const char * const boot_devices[BROM_LAST_BOOTSOURCE + 1] = {
> > [BROM_BOOTSOURCE_EMMC] = "/mmc@fe31",
> > - [BROM_BOOTSOURCE_SPINOR] = "/spi@fe30/flash@0",
> > + [BROM_BOOTSOURCE_FSPI_M0] = "/spi@fe30/flash@0",
> > [BROM_BOOTSOURCE_SD] = "/mmc@fe2b",
> > };
> >
Re: [PATCH 1/2] rockchip: rk3588: Add SPINAND as boot device
Hi Jonas and Kever
Thanks for clarifying the situation Kever! Please see below my proposition.
What do you think?
On lis 02, 2023 15:27, Kever Yang wrote:
> Hi Jonas,
>
> On 2023/11/1 00:45, Jonas Karlman wrote:
> > On 2023-10-31 17:13, Slawomir Stepien wrote:
> > > Mark the flash@0 as BROM_BOOTSOURCE_SPINAND.
> > > Fixes use of same-as-spl in u-boot,spl-boot-order prop on Orange Pi 5
> > > Plus.
> > >
> > > Signed-off-by: Slawomir Stepien
> > > ---
> > > arch/arm/mach-rockchip/rk3588/rk3588.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/arch/arm/mach-rockchip/rk3588/rk3588.c
> > > b/arch/arm/mach-rockchip/rk3588/rk3588.c
> > > index b1f535fad5..0c8f10a97c 100644
> > > --- a/arch/arm/mach-rockchip/rk3588/rk3588.c
> > > +++ b/arch/arm/mach-rockchip/rk3588/rk3588.c
> > > @@ -42,6 +42,7 @@ const char * const boot_devices[BROM_LAST_BOOTSOURCE +
> > > 1] = {
> > > [BROM_BOOTSOURCE_SPINOR] = "/spi@fe2b/flash@0",
> > > [BROM_BOOTSOURCE_SD] = "/mmc@fe2c",
> > > [BROM_BOOTSOURCE_SPINOR_RK3588] = "/spi@fe2b/flash@0",
> > > + [BROM_BOOTSOURCE_SPINAND] = "/spi@fe2b/flash@0",
> > This is not correct, please see [1] for an open discussion on this issue.
> >
> > Kever: Do you have any more insights into the question on [1].
>
> As I reply in another mail, the FSPI M0~M2 is all the option for SPI
> NAND/NOR in rk3588.
So based on that info, something like this?:
1. BROM_BOOTSOURCE_SPINAND - removed (not used at all).
2. BROM_BOOTSOURCE_SPINOR_RK3588 - removed (not needed after this patch).
3. Updated also the rk3568.c since it also has FSPI M0 (only this one mux), so
I would assume it
will be value of 3 there too.
4. The BROM_BOOTSOURCE_SPINOR is still there, since the rk3399 does not use the
FSPI (that is my
understanding after checking the TRM doc).
diff --git a/arch/arm/include/asm/arch-rockchip/bootrom.h
b/arch/arm/include/asm/arch-rockchip/bootrom.h
index 7dab18fbc3..9f16d28aec 100644
--- a/arch/arm/include/asm/arch-rockchip/bootrom.h
+++ b/arch/arm/include/asm/arch-rockchip/bootrom.h
@@ -46,9 +46,10 @@ enum {
BROM_BOOTSOURCE_NAND = 1,
BROM_BOOTSOURCE_EMMC = 2,
BROM_BOOTSOURCE_SPINOR = 3,
- BROM_BOOTSOURCE_SPINAND = 4,
+ BROM_BOOTSOURCE_FSPI_M0 = 3,
+ BROM_BOOTSOURCE_FSPI_M1 = 4,
BROM_BOOTSOURCE_SD = 5,
- BROM_BOOTSOURCE_SPINOR_RK3588 = 6,
+ BROM_BOOTSOURCE_FSPI_M2 = 6,
BROM_BOOTSOURCE_USB = 10,
BROM_LAST_BOOTSOURCE = BROM_BOOTSOURCE_USB
};
diff --git a/arch/arm/mach-rockchip/rk3568/rk3568.c
b/arch/arm/mach-rockchip/rk3568/rk3568.c
index 69ef19cc85..ccb63c6a88 100644
--- a/arch/arm/mach-rockchip/rk3568/rk3568.c
+++ b/arch/arm/mach-rockchip/rk3568/rk3568.c
@@ -83,7 +83,7 @@ static struct mm_region rk3568_mem_map[] = {
const char * const boot_devices[BROM_LAST_BOOTSOURCE + 1] = {
[BROM_BOOTSOURCE_EMMC] = "/mmc@fe31",
- [BROM_BOOTSOURCE_SPINOR] = "/spi@fe30/flash@0",
+ [BROM_BOOTSOURCE_FSPI_M0] = "/spi@fe30/flash@0",
[BROM_BOOTSOURCE_SD] = "/mmc@fe2b",
};
diff --git a/arch/arm/mach-rockchip/rk3588/rk3588.c
b/arch/arm/mach-rockchip/rk3588/rk3588.c
index b1f535fad5..c36f783d29 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@fe2e",
- [BROM_BOOTSOURCE_SPINOR] = "/spi@fe2b/flash@0",
+ [BROM_BOOTSOURCE_FSPI_M0] = "/spi@fe2b/flash@0",
[BROM_BOOTSOURCE_SD] = "/mmc@fe2c",
- [BROM_BOOTSOURCE_SPINOR_RK3588] = "/spi@fe2b/flash@0",
+ [BROM_BOOTSOURCE_FSPI_M1] = "/spi@fe2b/flash@0",
+ [BROM_BOOTSOURCE_FSPI_M2] = "/spi@fe2b/flash@0",
};
static struct mm_region rk3588_mem_map[] = {
> > Is my assessment about the value BootRom writes to bootsource reg
> > correct, see [1]?
> >
> > - FSPI (M0): 3
> > - FSPI (M1): 4
> > - FSPI (M2): 6
> >
> > [1]
> > https://lore.kernel.org/u-boot/b82c02c0-0a37-8553-f498-b03f85394...@kwiboo.se/
> >
> > Regards,
> > Jonas
> >
> > > };
> > > static struct mm_region rk3588_mem_map[] = {
--
Slawomir Stepien
Re: [PATCH 2/2] configs: orangepi-5-plus-rk3588: Build SPI image by default
On paź 31, 2023 17:49, Jonas Karlman wrote: > On 2023-10-31 17:13, Slawomir Stepien wrote: > > Flashing this image to the on-board flash is useful when you want to > > start Linux from a medium (e.g. nvme) without flashing u-boot elements > > to it. > > > > Signed-off-by: Slawomir Stepien > > --- > > 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 0473699621..9af65026a0 100644 > > --- a/configs/orangepi-5-plus-rk3588_defconfig > > +++ b/configs/orangepi-5-plus-rk3588_defconfig > > @@ -103,3 +103,4 @@ CONFIG_USB_OHCI_GENERIC=y > > CONFIG_USB_DWC3=y > > CONFIG_USB_DWC3_GENERIC=y > > CONFIG_ERRNO_STR=y > > +CONFIG_ROCKCHIP_SPI_IMAGE=y > > This was intentionally left out from original submission due to an > unfinished discussion at [1]. > > Also note that this option is not added at correct position in defconfig > file. Next time, please re-sync defconfig changes with a make > savedefconfig generated file. Ah yes. I did forget about it. Sorry. I will regenerate it correctly...if there will be v2. > [1] > https://lore.kernel.org/u-boot/b82c02c0-0a37-8553-f498-b03f85394...@kwiboo.se/ > > Regards, > Jonas -- Slawomir Stepien
[PATCH 2/2] configs: orangepi-5-plus-rk3588: Build SPI image by default
Flashing this image to the on-board flash is useful when you want to start Linux from a medium (e.g. nvme) without flashing u-boot elements to it. Signed-off-by: Slawomir Stepien --- 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 0473699621..9af65026a0 100644 --- a/configs/orangepi-5-plus-rk3588_defconfig +++ b/configs/orangepi-5-plus-rk3588_defconfig @@ -103,3 +103,4 @@ CONFIG_USB_OHCI_GENERIC=y CONFIG_USB_DWC3=y CONFIG_USB_DWC3_GENERIC=y CONFIG_ERRNO_STR=y +CONFIG_ROCKCHIP_SPI_IMAGE=y -- 2.42.0
[PATCH 1/2] rockchip: rk3588: Add SPINAND as boot device
Mark the flash@0 as BROM_BOOTSOURCE_SPINAND.
Fixes use of same-as-spl in u-boot,spl-boot-order prop on Orange Pi 5
Plus.
Signed-off-by: Slawomir Stepien
---
arch/arm/mach-rockchip/rk3588/rk3588.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/mach-rockchip/rk3588/rk3588.c
b/arch/arm/mach-rockchip/rk3588/rk3588.c
index b1f535fad5..0c8f10a97c 100644
--- a/arch/arm/mach-rockchip/rk3588/rk3588.c
+++ b/arch/arm/mach-rockchip/rk3588/rk3588.c
@@ -42,6 +42,7 @@ const char * const boot_devices[BROM_LAST_BOOTSOURCE + 1] = {
[BROM_BOOTSOURCE_SPINOR] = "/spi@fe2b/flash@0",
[BROM_BOOTSOURCE_SD] = "/mmc@fe2c",
[BROM_BOOTSOURCE_SPINOR_RK3588] = "/spi@fe2b/flash@0",
+ [BROM_BOOTSOURCE_SPINAND] = "/spi@fe2b/flash@0",
};
static struct mm_region rk3588_mem_map[] = {
--
2.42.0
