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 <jo...@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
