Re: [PATCH v7 6/7] ARM: dts: stm32: Add boot phase tags for STMicroelectronics mp15 boards

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Missing boot phase tag on the secondary flash node.
- [High] Conflicting pinctrl properties due to missing `/delete-property/`.
- [Medium] Meaningful code and commit message mismatch regarding the scope of added tags.
- [Low] Redundant boot phase properties on the same node.
--

commit 14d5fc0d2bb2bdd48adf7a7f9c0b21cba6b63b86
Author: Patrice Chotard <patrice.chotard@foss.st.com>
ARM: dts: stm32: Add boot phase tags for STMicroelectronics mp15 boards

The bootph-all flag was introduced in dt-schema to define node usage across
different boot phases. This patch adds these flags to ensure SD boot for
various stm32 boards.

> 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

[Severity: Medium]
Is this commit message accurate regarding the scope of the changes? The text
only mentions adding the bootph-all flag to timer, gpio, syscfg, clock and
uart nodes. However, the patch also introduces bootph-pre-ram and
bootph-some-ram tags to other subsystems like QSPI, PMIC, LTDC, OPTEE, SCMI,
PSCI, and SDMMC. Could the message be updated to reflect the full scope of
the modifications?

> diff --git a/arch/arm/boot/dts/st/stm32mp157a-icore-stm32mp1-ctouch2.dts b/arch/arm/boot/dts/st/stm32mp157a-icore-stm32mp1-ctouch2.dts
> index 60ce4425a7fdd..ac4e313ca3716 100644
> --- a/arch/arm/boot/dts/st/stm32mp157a-icore-stm32mp1-ctouch2.dts
> +++ b/arch/arm/boot/dts/st/stm32mp157a-icore-stm32mp1-ctouch2.dts

[ ... ]

> @@ -53,6 +53,16 @@ &uart4 {
>  	status = "okay";
>  };
> +
> +&uart4_pins_a {
> +	bootph-all;
> +	pins1 {
> +		bootph-all;
> +	};
> +
> +	pins2 {
> +		bootph-all;
> +		bias-pull-up;
> +	};
> +};

[Severity: High]
Does this introduce mutually exclusive electrical configurations? The base
definition for these pins in stm32mp15-pinctrl.dtsi already specifies
bias-disable. Because Device Tree append semantics merge node contents, the
resulting tree will contain both bias-disable and bias-pull-up. Should this
include /delete-property/ bias-disable before setting the pull-up?

> diff --git a/arch/arm/boot/dts/st/stm32mp157c-ev1.dts b/arch/arm/boot/dts/st/stm32mp157c-ev1.dts
> index 0e65a1862eb53..c4be802ef1e78 100644
> --- a/arch/arm/boot/dts/st/stm32mp157c-ev1.dts
> +++ b/arch/arm/boot/dts/st/stm32mp157c-ev1.dts

[ ... ]

> @@ -314,6 +315,7 @@ &qspi_bk2_sleep_pins_a
>  	reg = <0x58003000 0x1000>, <0x70000000 0x4000000>;
>  	#address-cells = <1>;
>  	#size-cells = <0>;
> +	bootph-pre-ram;
>  	status = "okay";
>  
>  	flash0: flash@0 {
> @@ -323,6 +325,7 @@ flash0: flash@0 {
>  		spi-max-frequency = <108000000>;
>  		#address-cells = <1>;
>  		#size-cells = <1>;
> +		bootph-pre-ram;
>  	};
>  
>  	flash1: flash@1 {
> @@ -335,6 +338,41 @@ flash1: flash@1 {
>  	};
>  };

[ ... ]

> +&qspi_bk2_pins_a {
> +	bootph-pre-ram;
> +	pins {
> +		bootph-pre-ram;
> +	};
> +};
> +
> +&qspi_cs2_pins_a {
> +	bootph-pre-ram;
> +	pins {
> +		bootph-pre-ram;
> +	};
> +};

[Severity: High]
Will this omission prevent the secondary flash from being accessible during
the SPL boot phase? The patch adds the bootph-pre-ram tag to the QSPI
controller, flash0, and the pinmux definitions dedicated to the secondary
flash (qspi_bk2_pins_a and qspi_cs2_pins_a), but it is omitted from the
flash1 node itself. Should the tag be added to flash1 as well?

> diff --git a/arch/arm/boot/dts/st/stm32mp15xx-dhcom-som.dtsi b/arch/arm/boot/dts/st/stm32mp15xx-dhcom-som.dtsi
> index 4cc633683c6b7..04e91d02cc287 100644
> --- a/arch/arm/boot/dts/st/stm32mp15xx-dhcom-som.dtsi
> +++ b/arch/arm/boot/dts/st/stm32mp15xx-dhcom-som.dtsi

[ ... ]

> @@ -253,6 +299,8 @@ &i2c4 {
>  	pinctrl-0 = <&i2c4_pins_a>;
>  	i2c-scl-rising-time-ns = <185>;
>  	i2c-scl-falling-time-ns = <20>;
> +	bootph-all;
> +	bootph-pre-ram;

[Severity: Low]
Are both of these properties necessary? Since the bootph-all property
inherently specifies that a node is present in all boot phases, adding
bootph-pre-ram to the same node appears redundant.

>  	status = "okay";
>  	/* spare dmas for other usage */
>  	/delete-property/dmas;

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