Re: [PATCH 3/3] arm64: dts: renesas: rzg3l-smarc-som: Enable SDHI2

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] arm64: dts: renesas: rzg3l-smarc-som: no-op UHS pinctrl
  switch on sdhi2
--

commit fa36790e3b28d4844cd7f51756a420dcdb4538ff
Author: Biju Das <biju.das.jz@bp.renesas.com>

arm64: dts: renesas: rzg3l-smarc-som: Enable SDHI2

This commit adds pin control and device tree nodes to enable SDHI2 on the
RZ/G3L SMARC EVK platform when the SW_SD2_EN macro is set.

> diff --git a/arch/arm64/boot/dts/renesas/rzg3l-smarc-som.dtsi b/arch/arm64/boot/dts/renesas/rzg3l-smarc-som.dtsi
> index f6f4eed9a2ea6..e8fc9cddf288f 100644
> --- a/arch/arm64/boot/dts/renesas/rzg3l-smarc-som.dtsi
> +++ b/arch/arm64/boot/dts/renesas/rzg3l-smarc-som.dtsi

[ ... ]

> @@ -330,6 +331,74 @@ sd0-data {
>  			power-source = <1800>;
>  		};
>  	};
> +
> +	sdhi2_pins: sd2 {
> +		sd2-cd {
> +			pinmux = <RZG3L_PORT_PINMUX(K, 0, 1)>; /* SD2_CD */
> +		};
> +
> +		sd2-clk {
> +			pinmux = <RZG3L_PORT_PINMUX(H, 0, 1)>; /* SD2_CLK */
> +			power-source = <3300>;
> +		};
> +
> +		sd2-cmd {
> +			pinmux = <RZG3L_PORT_PINMUX(H, 1, 1)>; /* SD2_CMD */
> +			input-enable;
> +			power-source = <3300>;
> +		};
> +
> +		sd2-data {
> +			pinmux = <RZG3L_PORT_PINMUX(H, 2, 1)>, /* SD2_DAT0 */
> +				 <RZG3L_PORT_PINMUX(H, 3, 1)>, /* SD2_DAT1 */
> +				 <RZG3L_PORT_PINMUX(H, 4, 1)>, /* SD2_DAT2 */
> +				 <RZG3L_PORT_PINMUX(H, 5, 1)>; /* SD2_DAT3 */
> +			input-enable;
> +			power-source = <3300>;
> +		};
> +
> +		sd2-iovs {
> +			pinmux = <RZG3L_PORT_PINMUX(K, 1, 1)>; /* SD2_IOVS */
> +		};
> +
> +		sd2-pwen {
> +			pinmux = <RZG3L_PORT_PINMUX(K, 2, 1)>; /* SD2_PWEN */
> +		};
> +	};
> +
> +	sdhi2_pins_uhs: sd2-uhs {
> +		sd2-cd {
> +			pinmux = <RZG3L_PORT_PINMUX(K, 0, 1)>; /* SD2_CD */
> +		};
> +
> +		sd2-clk {
> +			pinmux = <RZG3L_PORT_PINMUX(H, 0, 1)>; /* SD2_CLK */
> +			power-source = <1800>;
> +		};
> +
> +		sd2-cmd {
> +			pinmux = <RZG3L_PORT_PINMUX(H, 1, 1)>; /* SD2_CMD */
> +			input-enable;
> +			power-source = <1800>;
> +		};
> +
> +		sd2-data {
> +			pinmux = <RZG3L_PORT_PINMUX(H, 2, 1)>, /* SD2_DAT0 */
> +				 <RZG3L_PORT_PINMUX(H, 3, 1)>, /* SD2_DAT1 */
> +				 <RZG3L_PORT_PINMUX(H, 4, 1)>, /* SD2_DAT2 */
> +				 <RZG3L_PORT_PINMUX(H, 5, 1)>; /* SD2_DAT3 */
> +			input-enable;
> +			power-source = <1800>;
> +		};
> +
> +		sd2-iovs {
> +			pinmux = <RZG3L_PORT_PINMUX(K, 1, 1)>; /* SD2_IOVS */
> +		};
> +
> +		sd2-pwen {
> +			pinmux = <RZG3L_PORT_PINMUX(K, 2, 1)>; /* SD2_PWEN */
> +		};
> +	};
>  };

[Severity: Medium]
Does switching sdhi2 between sdhi2_pins and sdhi2_pins_uhs write any hardware
pinctrl registers for the sd2 pins?

sdhi2_pins and sdhi2_pins_uhs differ only by power-source (<3300> vs <1800>),
and neither specifies drive-strength-microamp. In r9a08g046_variable_pin_cfg,
those pins use software power-source tracking instead of a hardware
power-output-control register:

drivers/pinctrl/renesas/pinctrl-rzg2l.c:r9a08g046_variable_pin_cfg[] = {
    ...
	RZG2L_VARIABLE_PIN_CFG_PACK(RZG3L_PH, 0, RZG3L_MPXED_PIN_FUNCS(B)),
	RZG2L_VARIABLE_PIN_CFG_PACK(RZG3L_PH, 1, RZG3L_MPXED_PIN_FUNCS(B) | PIN_CFG_IEN),
    ...
};

drivers/pinctrl/renesas/pinctrl-rzg2l.c:rzg2l_set_power_source() {
    ...
	if (caps & PIN_CFG_SOFT_PS) {
		pctrl->settings[pin].power_source = ps;
		return 0;
	}
    ...
}

When renesas_sdhi_start_signal_voltage_switch() selects state_uhs,
rzg2l_set_power_source() only updates pctrl->settings[pin].power_source in
memory and returns 0 without writing any pinctrl register while the internal
regulator switches the external signal voltage.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-sdhi_rzg3l-v1-0-ca30b209615d@bp.renesas.com?part=3