[PATCH COVER] rockchip: rk3399: fix SPI-NOR flash not found in U-Boot pre-reloc
From: Quentin Schulz quentin.schulz@cherry.de
In commit 100f489f58a6 ("rockchip: rk3399: Fix loading FIT from SD-card when booting from eMMC"), the spi1 bootph properties were mistakenly removed meaning, so re-add them back to fix SPI-NOR flash not being found in U-Boot pre-reloc as required for RK3399 Puma.
Fixes: 100f489f58a6 ("rockchip: rk3399: Fix loading FIT from SD-card when booting from eMMC") Signed-off-by: Quentin Schulz quentin.schulz@cherry.de --- arch/arm/dts/rk3399-u-boot.dtsi | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/arch/arm/dts/rk3399-u-boot.dtsi b/arch/arm/dts/rk3399-u-boot.dtsi index b6b43271172..2bec139d833 100644 --- a/arch/arm/dts/rk3399-u-boot.dtsi +++ b/arch/arm/dts/rk3399-u-boot.dtsi @@ -145,6 +145,11 @@ bootph-some-ram; };
+&spi1 { + bootph-pre-ram; + bootph-some-ram; +}; + &spi1_clk { bootph-pre-ram; bootph-some-ram;
--- base-commit: 2248c96ea1cf0b65377040d9f87ce7d8cf534c63 change-id: 20240617-rk3399-fix-d8c8d3ce844e
Best regards,
Hi all,
This is a patch, even if the mail subject says COVER, some regression in new b4 release.
Cheers, Quentin
On 6/17/24 3:10 PM, Quentin Schulz wrote:
From: Quentin Schulz quentin.schulz@cherry.de
In commit 100f489f58a6 ("rockchip: rk3399: Fix loading FIT from SD-card when booting from eMMC"), the spi1 bootph properties were mistakenly removed meaning, so re-add them back to fix SPI-NOR flash not being found in U-Boot pre-reloc as required for RK3399 Puma.
Fixes: 100f489f58a6 ("rockchip: rk3399: Fix loading FIT from SD-card when booting from eMMC") Signed-off-by: Quentin Schulz quentin.schulz@cherry.de
arch/arm/dts/rk3399-u-boot.dtsi | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/arch/arm/dts/rk3399-u-boot.dtsi b/arch/arm/dts/rk3399-u-boot.dtsi index b6b43271172..2bec139d833 100644 --- a/arch/arm/dts/rk3399-u-boot.dtsi +++ b/arch/arm/dts/rk3399-u-boot.dtsi @@ -145,6 +145,11 @@ bootph-some-ram; };
+&spi1 {
- bootph-pre-ram;
- bootph-some-ram;
+};
- &spi1_clk { bootph-pre-ram; bootph-some-ram;
base-commit: 2248c96ea1cf0b65377040d9f87ce7d8cf534c63 change-id: 20240617-rk3399-fix-d8c8d3ce844e
Best regards,
Hi Quentin,
On 2024-06-17 15:10, Quentin Schulz wrote:
From: Quentin Schulz quentin.schulz@cherry.de
In commit 100f489f58a6 ("rockchip: rk3399: Fix loading FIT from SD-card when booting from eMMC"), the spi1 bootph properties were mistakenly removed meaning, so re-add them back to fix SPI-NOR flash not being found in U-Boot pre-reloc as required for RK3399 Puma.
Good catch, for TPL/SPL the bootph props is propagated, something that is not done for pre-reloc.
Fixes: 100f489f58a6 ("rockchip: rk3399: Fix loading FIT from SD-card when booting from eMMC") Signed-off-by: Quentin Schulz quentin.schulz@cherry.de
Reviewed-by: Jonas Karlman jonas@kwiboo.se
Regards, Jonas
arch/arm/dts/rk3399-u-boot.dtsi | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/arch/arm/dts/rk3399-u-boot.dtsi b/arch/arm/dts/rk3399-u-boot.dtsi index b6b43271172..2bec139d833 100644 --- a/arch/arm/dts/rk3399-u-boot.dtsi +++ b/arch/arm/dts/rk3399-u-boot.dtsi @@ -145,6 +145,11 @@ bootph-some-ram; };
+&spi1 {
- bootph-pre-ram;
- bootph-some-ram;
+};
&spi1_clk { bootph-pre-ram; bootph-some-ram;
base-commit: 2248c96ea1cf0b65377040d9f87ce7d8cf534c63 change-id: 20240617-rk3399-fix-d8c8d3ce844e
Best regards,
Hi Jonas,
On 6/17/24 3:26 PM, Jonas Karlman wrote:
Hi Quentin,
On 2024-06-17 15:10, Quentin Schulz wrote:
From: Quentin Schulz quentin.schulz@cherry.de
In commit 100f489f58a6 ("rockchip: rk3399: Fix loading FIT from SD-card when booting from eMMC"), the spi1 bootph properties were mistakenly removed meaning, so re-add them back to fix SPI-NOR flash not being found in U-Boot pre-reloc as required for RK3399 Puma.
Good catch, for TPL/SPL the bootph props is propagated, something that is not done for pre-reloc.
Can you tell us a bit more about this? I know that the pinctrl nodes marked for pre-reloc recursively apply the same to their parent, but couldn't find anything similar for other subsystems for example. What did I miss in my 5m search?
Cheers, Quentin
Hi Quentin,
On 2024-06-17 15:29, Quentin Schulz wrote:
Hi Jonas,
On 6/17/24 3:26 PM, Jonas Karlman wrote:
Hi Quentin,
On 2024-06-17 15:10, Quentin Schulz wrote:
From: Quentin Schulz quentin.schulz@cherry.de
In commit 100f489f58a6 ("rockchip: rk3399: Fix loading FIT from SD-card when booting from eMMC"), the spi1 bootph properties were mistakenly removed meaning, so re-add them back to fix SPI-NOR flash not being found in U-Boot pre-reloc as required for RK3399 Puma.
Good catch, for TPL/SPL the bootph props is propagated, something that is not done for pre-reloc.
Can you tell us a bit more about this? I know that the pinctrl nodes marked for pre-reloc recursively apply the same to their parent, but couldn't find anything similar for other subsystems for example. What did I miss in my 5m search?
v2024.04-rc1 added support to propagate bootph props using fdtgrep for the u-boot-tpl/spl.dtb files, however pre-reloc the main u-boot.dtb (or possible the FDT included in FIT) that is not processed by fdtgrep.
fdtgrep: Allow propagating properties up to supernodes https://source.denx.de/u-boot/u-boot/-/commit/7a06cc2027c0169c462da63a68fa26...
Makefile: Use the fdtgrep -u flag https://source.denx.de/u-boot/u-boot/-/commit/aca95282c1b72c41d8e72984b1dceb...
I solely relied, wrongly, on this new propagation to justify the removal of the spi1 node, without ever thinking about that pre-reloc uses an unprocessed version of the device tree.
Regards, Jonas
Cheers, Quentin
Hi Jonas,
On 6/17/24 3:44 PM, Jonas Karlman wrote:
Hi Quentin,
On 2024-06-17 15:29, Quentin Schulz wrote:
Hi Jonas,
On 6/17/24 3:26 PM, Jonas Karlman wrote:
Hi Quentin,
On 2024-06-17 15:10, Quentin Schulz wrote:
From: Quentin Schulz quentin.schulz@cherry.de
In commit 100f489f58a6 ("rockchip: rk3399: Fix loading FIT from SD-card when booting from eMMC"), the spi1 bootph properties were mistakenly removed meaning, so re-add them back to fix SPI-NOR flash not being found in U-Boot pre-reloc as required for RK3399 Puma.
Good catch, for TPL/SPL the bootph props is propagated, something that is not done for pre-reloc.
Can you tell us a bit more about this? I know that the pinctrl nodes marked for pre-reloc recursively apply the same to their parent, but couldn't find anything similar for other subsystems for example. What did I miss in my 5m search?
v2024.04-rc1 added support to propagate bootph props using fdtgrep for the u-boot-tpl/spl.dtb files, however pre-reloc the main u-boot.dtb (or possible the FDT included in FIT) that is not processed by fdtgrep.
fdtgrep: Allow propagating properties up to supernodes https://source.denx.de/u-boot/u-boot/-/commit/7a06cc2027c0169c462da63a68fa26...
Makefile: Use the fdtgrep -u flag https://source.denx.de/u-boot/u-boot/-/commit/aca95282c1b72c41d8e72984b1dceb...
Thanks for the pointers!
I'm wondering if we shouldn't do the same for U-Boot proper DTB as well?
i.e.
cmd_fdt_rm_props = cat $< | $(objtree)/tools/fdtgrep -r -O dtb - -o $@
to
cmd_fdt_rm_props = $(objtree)/tools/fdtgrep $(fdtgrep_props) -u $< | $(objtree)/tools/fdtgrep -r -O dtb - -o $@
I assume we need to add a bunch more options though.
But that would make sense to me, the fact that there's a pre-reloc stage in U-Boot proper is already a bit odd (to me), but if we have something that behaves differently than the pre-reloc stage in earlier stages... I'm not sure this is a good idea?
While at it, I'm wondering if we didn't miss the removal of bootph-some-ram in cmd_fdtgrep for the VPL/SPL/TPL?
Additionally, we should be able to remove bootph-pre-sram and bootph-pre-ram in U-Boot proper DT since those only apply to non-U-Boot proper stages, don't you think?
Cheers, Quentin
Hi Quentin,
On 2024-06-17 16:03, Quentin Schulz wrote:
Hi Jonas,
On 6/17/24 3:44 PM, Jonas Karlman wrote:
Hi Quentin,
On 2024-06-17 15:29, Quentin Schulz wrote:
Hi Jonas,
On 6/17/24 3:26 PM, Jonas Karlman wrote:
Hi Quentin,
On 2024-06-17 15:10, Quentin Schulz wrote:
From: Quentin Schulz quentin.schulz@cherry.de
In commit 100f489f58a6 ("rockchip: rk3399: Fix loading FIT from SD-card when booting from eMMC"), the spi1 bootph properties were mistakenly removed meaning, so re-add them back to fix SPI-NOR flash not being found in U-Boot pre-reloc as required for RK3399 Puma.
Good catch, for TPL/SPL the bootph props is propagated, something that is not done for pre-reloc.
Can you tell us a bit more about this? I know that the pinctrl nodes marked for pre-reloc recursively apply the same to their parent, but couldn't find anything similar for other subsystems for example. What did I miss in my 5m search?
v2024.04-rc1 added support to propagate bootph props using fdtgrep for the u-boot-tpl/spl.dtb files, however pre-reloc the main u-boot.dtb (or possible the FDT included in FIT) that is not processed by fdtgrep.
fdtgrep: Allow propagating properties up to supernodes https://source.denx.de/u-boot/u-boot/-/commit/7a06cc2027c0169c462da63a68fa26...
Makefile: Use the fdtgrep -u flag https://source.denx.de/u-boot/u-boot/-/commit/aca95282c1b72c41d8e72984b1dceb...
Thanks for the pointers!
I'm wondering if we shouldn't do the same for U-Boot proper DTB as well?
I think that would be good, and should also remove the need for the recursive pinctrl lookup for bootph props during pre-reloc.
i.e.
cmd_fdt_rm_props = cat $< | $(objtree)/tools/fdtgrep -r -O dtb - -o $@
to
cmd_fdt_rm_props = $(objtree)/tools/fdtgrep $(fdtgrep_props) -u $< | $(objtree)/tools/fdtgrep -r -O dtb - -o $@
I assume we need to add a bunch more options though.
But that would make sense to me, the fact that there's a pre-reloc stage in U-Boot proper is already a bit odd (to me), but if we have something that behaves differently than the pre-reloc stage in earlier stages... I'm not sure this is a good idea?
Fully agree that part of the pre-reloc stage make little sense at least on Rockchip platform where full memory is available before pre-reloc.
(I am hoping to get an initial series to remove time wasted during pre-reloc for Rockchip platform out in near future, e.g. ~250ms is currently wasted looking up e.g. CRU_BASE on rk3399, a constant that is known at compile time).
While at it, I'm wondering if we didn't miss the removal of bootph-some-ram in cmd_fdtgrep for the VPL/SPL/TPL?
I think so too, the bootph-some-ram should be safe to remove.
We can/should probably also extend fdtgrep to remove disabled nodes for the VPL/SPL/TPL.
Additionally, we should be able to remove bootph-pre-sram and bootph-pre-ram in U-Boot proper DT since those only apply to non-U-Boot proper stages, don't you think?
I do not fully think we can remove them, due to the dual use of ofnode_pre_reloc(). During pre-reloc it will return true if node is required, and after pre-reloc it return true if the node was required during any prior phase. Unsure if anything depend on this secondry use.
Regards, Jonas
Cheers, Quentin
Hi Jonas,
On 6/17/24 8:53 PM, Jonas Karlman wrote: [...]
I'm wondering if we shouldn't do the same for U-Boot proper DTB as well?
I think that would be good, and should also remove the need for the recursive pinctrl lookup for bootph props during pre-reloc.
i.e.
cmd_fdt_rm_props = cat $< | $(objtree)/tools/fdtgrep -r -O dtb - -o $@
to
cmd_fdt_rm_props = $(objtree)/tools/fdtgrep $(fdtgrep_props) -u $< | $(objtree)/tools/fdtgrep -r -O dtb - -o $@
I assume we need to add a bunch more options though.
But that would make sense to me, the fact that there's a pre-reloc stage in U-Boot proper is already a bit odd (to me), but if we have something that behaves differently than the pre-reloc stage in earlier stages... I'm not sure this is a good idea?
Fully agree that part of the pre-reloc stage make little sense at least on Rockchip platform where full memory is available before pre-reloc.
Will try to come up with something.
[...]
We can/should probably also extend fdtgrep to remove disabled nodes for the VPL/SPL/TPL.
I didn't find any disabled node in SPL DTB for RK3588 Tiger and RK3399 Puma, so maybe not worth the added complexity to fdtgrep right now. But that's a good idea :)
Cheers, Quentin
Hi,
On 6/19/24 12:33 PM, Quentin Schulz wrote:
Hi Jonas,
On 6/17/24 8:53 PM, Jonas Karlman wrote: [...]
I'm wondering if we shouldn't do the same for U-Boot proper DTB as well?
I think that would be good, and should also remove the need for the recursive pinctrl lookup for bootph props during pre-reloc.
i.e.
cmd_fdt_rm_props = cat $< | $(objtree)/tools/fdtgrep -r -O dtb - -o $@
to
cmd_fdt_rm_props = $(objtree)/tools/fdtgrep $(fdtgrep_props) -u $< | $(objtree)/tools/fdtgrep -r -O dtb - -o $@
I assume we need to add a bunch more options though.
But that would make sense to me, the fact that there's a pre-reloc stage in U-Boot proper is already a bit odd (to me), but if we have something that behaves differently than the pre-reloc stage in earlier stages... I'm not sure this is a good idea?
Fully agree that part of the pre-reloc stage make little sense at least on Rockchip platform where full memory is available before pre-reloc.
Will try to come up with something.
Ok so nothing straightforward. The issue is that the SPL/TPL/VPL DTB is built this way:
1) take full DTB 2) include all nodes which have the bootph-all, bootph-pre-sram/pre-ram, bootph-verify 3) include all parent nodes recursively of nodes from 2) 4) include /, /config, /chosen, /aliases
This generates a DTB. The parent nodes of nodes matching 2) and nodes matching 2) and / and /config and /chosen and /aliases are there, nothing else. No change made to DTB.
Step 2.
5) take DTB from 4) 6) remove bootph-all, bootph-pre-sram/pre-ram, bootph-verify properties and the ones in CONFIG_OF_SPL_REMOVE_PROPS
So.... '-u' option documentation is a lie :) It's not adding the property to the parents, it's including the parents of nodes matching 2).
I'm also a bit perplexed with the documentation stating it's the '-p' properties that are propagated as this is used for SPL/TPL/VPL and we don't have a '-p' option for it, only '-b' (the sounds being close in English, maybe just an unfortunate typo).
For U-Boot proper DTB, nothing is done except removing properties listed in CONFIG_OF_REMOVE_PROPS. And we would need to **add** boopth-some-ram properties to parent nodes instead.
From the look of it, this isn't a straightforward change to fdtgrep as fdtgrep only cares if a node needs to be included not really which property actually matched in order for it to be included.
Cheers, Quentin
On 2024/6/17 21:10, Quentin Schulz wrote:
From: Quentin Schulz quentin.schulz@cherry.de
In commit 100f489f58a6 ("rockchip: rk3399: Fix loading FIT from SD-card when booting from eMMC"), the spi1 bootph properties were mistakenly removed meaning, so re-add them back to fix SPI-NOR flash not being found in U-Boot pre-reloc as required for RK3399 Puma.
Fixes: 100f489f58a6 ("rockchip: rk3399: Fix loading FIT from SD-card when booting from eMMC") Signed-off-by: Quentin Schulz quentin.schulz@cherry.de
Reviewed-by: Kever Yang kever.yang@rock-chips.com
Thanks, - Kever
arch/arm/dts/rk3399-u-boot.dtsi | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/arch/arm/dts/rk3399-u-boot.dtsi b/arch/arm/dts/rk3399-u-boot.dtsi index b6b43271172..2bec139d833 100644 --- a/arch/arm/dts/rk3399-u-boot.dtsi +++ b/arch/arm/dts/rk3399-u-boot.dtsi @@ -145,6 +145,11 @@ bootph-some-ram; };
+&spi1 {
- bootph-pre-ram;
- bootph-some-ram;
+};
- &spi1_clk { bootph-pre-ram; bootph-some-ram;
base-commit: 2248c96ea1cf0b65377040d9f87ce7d8cf534c63 change-id: 20240617-rk3399-fix-d8c8d3ce844e
Best regards,
participants (4)
-
Jonas Karlman -
Kever Yang -
Quentin Schulz -
Quentin Schulz