Re: [PATCH v7 7/7] arm64: dts: st: Add boot phase tags for STMicroelectronics mp2 boards

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The patch aims to support SD boot by adding `bootph-all` tags to necessary nodes, but it fails to add them to the SDMMC node (`&sdmmc1`), its pinctrl configurations (e.g., `sdmmc1_b4_pins_a`), and its required regulators (e.g., `scmi_vdd_sdcard`).
- [High] The commit message explicitly claims to add the `bootph-all` tag to timer nodes, but the patch completely omits them.
- [Medium] Display controller nodes (`&ltdc` and `&lvds`) in `stm32mp257f-ev1.dts` are inappropriately tagged with `bootph-all`.
--

commit ece19a3ebae334de3035a4297c45927575316f46
Author: Patrice Chotard <patrice.chotard@foss.st.com>

arm64: dts: st: Add boot phase tags for STMicroelectronics mp2 boards

This commit introduces bootph-all tags to multiple device tree nodes across
STMicroelectronics mp2 boards. The stated goal is to preserve essential nodes
like timer, gpio, syscfg, clock, and uart across all boot phases to ensure
SD boot works.

> To ensure SD boot, timer, gpio, syscfg, clock and uart nodes need to be
> present in all boot stages, so add missing bootph-all phase flag
> to these nodes to support SD boot.

[Severity: High]
Does this patch actually add the bootph-all property to the timer nodes?
Looking through the diff, the base arm architected timer and other timer
nodes don't appear to be updated. If the early boot stages rely on the
device tree for timer initialization, will this omission prevent SD boot
from working properly?

> diff --git a/arch/arm64/boot/dts/st/stm32mp235f-dk.dts b/arch/arm64/boot/dts/st/stm32mp235f-dk.dts
> index dd4efbe5a46e8..0608b978cbe5a 100644
> --- a/arch/arm64/boot/dts/st/stm32mp235f-dk.dts
> +++ b/arch/arm64/boot/dts/st/stm32mp235f-dk.dts
[ ... ]
> @@ -258,6 +334,10 @@ scmi_vdd_sdcard: regulator@23 {
>  	};
>  };
>  
> +&scmi_reset {
> +	bootph-all;
> +};
> +
>  &sdmmc1 {
>  	pinctrl-names = "default", "opendrain", "sleep";
>  	pinctrl-0 = <&sdmmc1_b4_pins_a>;
> @@ -272,12 +352,27 @@ &sdmmc1 {
>  	status = "okay";
>  };

[Severity: High]
Should the &sdmmc1 node, along with its required regulators (like
scmi_vdd_sdcard) and pin configurations (like sdmmc1_b4_pins_a), also
receive the bootph-all tag?

Since the boot loader will remove device tree nodes without early boot tags,
it seems the SD controller itself might be missing from the early device
tree, which would break the SD boot support.

[ ... ]
> diff --git a/arch/arm64/boot/dts/st/stm32mp257f-ev1.dts b/arch/arm64/boot/dts/st/stm32mp257f-ev1.dts
> index 14e033f365e39..dab54742e01c1 100644
> --- a/arch/arm64/boot/dts/st/stm32mp257f-ev1.dts
> +++ b/arch/arm64/boot/dts/st/stm32mp257f-ev1.dts
[ ... ]
> @@ -344,6 +396,7 @@ timer {
>  };
>  
>  &ltdc {
> +	bootph-all;
>  	status = "okay";
>  	port {
>  		ltdc_ep0_out: endpoint {

[Severity: Medium]
Are display controller nodes like &ltdc required during the early SD boot
stages?

Including these nodes in the heavily memory-constrained early device tree
might cause unnecessary binary bloat. Could this safely be left out if it
isn't strictly needed for early boot?

> @@ -353,6 +406,7 @@ ltdc_ep0_out: endpoint {
>  };
>  
>  &lvds {
> +	bootph-all;
>  	status = "okay";
>  	ports {
>  		#address-cells = <1>;

[Severity: Medium]
Similar to the &ltdc node above, does the &lvds node need to be present
in all boot stages?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260526-upstream_uboot_properties-v7-0-e17cd424d5db@foss.st.com?part=7