Re: [PATCH 08/10] arm64: dts: st: Add pinmux nodes for DH electronics STM32MP23xx/STM32MP25xx DHCOS SoM and Breakout Board

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Conflicting alternate function (AF) configurations for SDMMC3_D3 between active and open-drain states.
- [High] The `eth2_rgmii_pins_b` node is missing the `st,io-sync = "data on both edges";` property required for Double Data Rate (DDR) signaling in RGMII.
- [Medium] Active and sleep pinmux configurations for ETH2 MDIO use conflicting pins for ETH_MDC.
- [Low] Misleading inline comments in DT pinmux nodes copy-pasted from other nodes.
--

--- Patch [8]: [PATCH 08/10] arm64: dts: st: Add pinmux nodes for DH electronics STM32MP23xx/STM32MP25xx DHCOS SoM and Breakout Board ---
commit c98eed614c4613fe217412814e7be996b10ee304
Author: Marek Vasut <marex@nabladev.com>

arm64: dts: st: Add pinmux nodes for DH electronics STM32MP23xx/STM32MP25xx DHCOS SoM and Breakout Board

This patch adds new pinmux nodes for the DH electronics STM32MP2 DHCOS SoM
and Breakout Board. It includes configurations for ETH2, I2C8, MCO1, SDMMC,
SPI, UART, and USART interfaces.

> diff --git a/arch/arm64/boot/dts/st/stm32mp25-pinctrl.dtsi b/arch/arm64/boot/dts/st/stm32mp25-pinctrl.dtsi
> index 456ece7f8ebc3..1aadbc6b47da9 100644
> --- a/arch/arm64/boot/dts/st/stm32mp25-pinctrl.dtsi
> +++ b/arch/arm64/boot/dts/st/stm32mp25-pinctrl.dtsi
> @@ -182,6 +182,30 @@ pins5 {
>  		};
>  	};
>  
> +	/omit-if-no-ref/
> +	eth2_mdio_pins_a: eth2-mdio-0 {
> +		pins1 {
> +			pinmux = <STM32_PINMUX('G', 4, AF11)>; /* ETH_MDC */
> +			bias-disable;
> +			drive-push-pull;
> +			slew-rate = <3>;
> +		};

[ ... ]

> +	/omit-if-no-ref/
> +	eth2_mdio_sleep_pins_a: eth2-mdio-sleep-0 {
> +		pins {
> +			pinmux = <STM32_PINMUX('C', 6, ANALOG)>, /* ETH_MDC */

[Severity: Medium]
In eth2_mdio_pins_a, ETH_MDC is mapped to PG4. Does this mismatched
pinmux configuration leave PG4 misconfigured and incorrectly apply
analog mode to PC6 instead of PG4 during power management transitions?

> +				 <STM32_PINMUX('C', 5, ANALOG)>; /* ETH_MDIO */
> +		};
> +	};

[ ... ]

> +	/omit-if-no-ref/
> +	eth2_rgmii_pins_b: eth2-rgmii-1 {
> +		pins1 {

[Severity: High]
This node configures the RGMII data pins, but it doesn't include the
st,io-sync = "data on both edges"; property required for Double
Data Rate (DDR) signaling. Will this cause the MAC to use Single Data Rate
sampling instead of RGMII DDR mode, resulting in timing violations?

> +			pinmux = <STM32_PINMUX('C', 7, AF10)>, /* ETH_RGMII_TXD0 */
> +				 <STM32_PINMUX('C', 8, AF10)>, /* ETH_RGMII_TXD1 */
> +				 <STM32_PINMUX('C', 9, AF10)>, /* ETH_RGMII_TXD2 */
> +				 <STM32_PINMUX('C', 10, AF10)>, /* ETH_RGMII_TXD3 */
> +				 <STM32_PINMUX('C', 4, AF10)>; /* ETH_RGMII_TX_CTL */
> +			bias-disable;
> +			drive-push-pull;
> +			slew-rate = <3>;
> +		};
> +		pins2 {
> +			pinmux = <STM32_PINMUX('F', 8, AF10)>, /* ETH_RGMII_CLK125 */
> +				 <STM32_PINMUX('F', 7, AF10)>; /* ETH_MDC */

[Severity: Low]
This isn't a bug, but should this be labeled ETH_RGMII_GTX_CLK rather
than ETH_MDC, to match the sleep state configuration for PF7 below?

> +			bias-disable;
> +			drive-push-pull;
> +			slew-rate = <3>;
> +		};

[ ... ]

> +	/omit-if-no-ref/
> +	sdmmc3_b4_pins_b: sdmmc3-b4-1 {
> +		pins1 {
> +			pinmux = <STM32_PINMUX('B', 14, AF10)>, /* SDMMC3_D0 */
> +				 <STM32_PINMUX('D', 13, AF10)>, /* SDMMC3_D1 */
> +				 <STM32_PINMUX('B', 12, AF10)>, /* SDMMC3_D2 */
> +				 <STM32_PINMUX('D', 14, AF12)>, /* SDMMC3_D3 */
> +				 <STM32_PINMUX('D', 12, AF10)>; /* SDMMC3_CMD */

[ ... ]

> +	/omit-if-no-ref/
> +	sdmmc3_b4_od_pins_b: sdmmc3-b4-od-1 {
> +		pins1 {
> +			pinmux = <STM32_PINMUX('B', 14, AF10)>, /* SDMMC3_D0 */
> +				 <STM32_PINMUX('D', 13, AF10)>, /* SDMMC3_D1 */
> +				 <STM32_PINMUX('B', 12, AF10)>, /* SDMMC3_D2 */
> +				 <STM32_PINMUX('D', 14, AF10)>; /* SDMMC3_D3 */

[Severity: High]
The active state node (sdmmc3_b4_pins_b) configures PD14 with AF12 for
SDMMC3_D3. Does configuring PD14 with AF10 here in the open-drain state
disconnect the data line from the SDMMC controller during operations
that use open-drain mode?

> +			slew-rate = <2>;
> +			drive-push-pull;
> +			bias-disable;
> +		};

[ ... ]

> +	/omit-if-no-ref/
> +	i2c8_pins_b: i2c8-1 {
> +		pins {
> +			pinmux = <STM32_PINMUX('Z', 4, AF8)>, /* I2C1_SCL */
> +				 <STM32_PINMUX('Z', 9, AF8)>; /* I2C1_SDA */

[Severity: Low]
This isn't a bug, but the comments label these pins as I2C1 instead
of I2C8. Were these comments copied from an earlier I2C1 node?

> +			bias-disable;
> +			drive-open-drain;
> +			slew-rate = <0>;
> +		};
> +	};
> +
> +	/omit-if-no-ref/
> +	i2c8_sleep_pins_b: i2c8-sleep-1 {
> +		pins {
> +			pinmux = <STM32_PINMUX('Z', 4, ANALOG)>, /* I2C1_SCL */
> +				 <STM32_PINMUX('Z', 9, ANALOG)>; /* I2C1_SDA */

[Severity: Low]
This isn't a bug, but the same typo labels these pins as I2C1 instead of I2C8.

> +		};
> +	};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711210131.236025-1-marex@nabladev.com?part=8