Re: [PATCH] arm64: dts: qcom: glymur-qcb: Add PCIe3a support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] arm64: dts: qcom: glymur-qcb: pinctrl output-high glitches PCIe slot rails
--

commit 396ad8048cc24879fc2c5466438a8ef1a184d1eb
Author: Qiang Yu <qiang.yu@oss.qualcomm.com>

arm64: dts: qcom: glymur-qcb: Add PCIe3a support

This commit adds device tree nodes for the PCIe3a x8 slot on the Glymur QCB
board, including fixed regulators, pinctrl states, and PHY/port power
supplies.

> diff --git a/arch/arm64/boot/dts/qcom/glymur-qcb.dts b/arch/arm64/boot/dts/qcom/glymur-qcb.dts
> index 17d8ff3aed23b..741a24454e80d 100644
> --- a/arch/arm64/boot/dts/qcom/glymur-qcb.dts
> +++ b/arch/arm64/boot/dts/qcom/glymur-qcb.dts
> @@ -78,6 +78,48 @@ vreg_nvme: regulator-nvme {
>  		regulator-boot-on;
>  	};
>  
> +	vreg_pcie_12v: regulator-pcie-12v {
> +		compatible = "regulator-fixed";
> +
> +		regulator-name = "VREG_PCIE_12V";
> +		regulator-min-microvolt = <12000000>;
> +		regulator-max-microvolt = <12000000>;
> +
> +		gpio = <&tlmm 26 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +
> +		pinctrl-0 = <&pcie_x8_12v>;
> +		pinctrl-names = "default";
> +	};
> +
> +	vreg_pcie_3v3: regulator-pcie-3v3 {
> +		compatible = "regulator-fixed";
> +
> +		regulator-name = "VREG_PCIE_3P3";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +
> +		gpio = <&tlmm 135 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +
> +		pinctrl-0 = <&pm_main_3p3_en>;
> +		pinctrl-names = "default";
> +	};
> +
> +	vreg_pcie_3v3_aux: regulator-pcie-3v3-aux {
> +		compatible = "regulator-fixed";
> +
> +		regulator-name = "VREG_PCIE_3P3_AUX";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +
> +		gpio = <&tlmm 136 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +
> +		pinctrl-0 = <&pm_aux_3p3_en>;
> +		pinctrl-names = "default";
> +	};
> +
>  	vreg_wcn_0p95: regulator-wcn-0p95 {
>  		compatible = "regulator-fixed";
>  

[ ... ]

> @@ -659,6 +749,33 @@ wake-n-pins {
>  		};
>  	};
>  
> +	pcie_x8_12v: pcie-12v-default-state {
> +		pins = "gpio26";
> +		function = "gpio";
> +		output-enable;
> +		output-high;
> +		drive-strength = <16>;
> +		bias-disable;
> +	};
> +
> +	pm_aux_3p3_en: pcie-aux-3p3-default-state {
> +		pins = "gpio136";
> +		function = "gpio";
> +		output-enable;
> +		output-high;
> +		drive-strength = <16>;
> +		bias-disable;
> +	};
> +
> +	pm_main_3p3_en: pcie-main-3p3-default-state {
> +		pins = "gpio135";
> +		function = "gpio";
> +		output-enable;
> +		output-high;
> +		drive-strength = <16>;
> +		bias-disable;
> +	};

[Severity: Medium]
Does setting output-enable and output-high in pcie_x8_12v, pm_aux_3p3_en, and
pm_main_3p3_en while omitting regulator-boot-on in vreg_pcie_12v,
vreg_pcie_3v3, and vreg_pcie_3v3_aux cause a power glitch on the PCIe3a slot
rails during regulator probe?

When each regulator-fixed platform device probes, really_probe() calls
pinctrl_bind_pins() -> msm_config_group_set() before
reg_fixed_voltage_probe(). Because pcie_x8_12v, pm_aux_3p3_en, and
pm_main_3p3_en specify output-enable and output-high, msm_config_group_set()
drives gpio26, gpio136, and gpio135 high and turns on the 12V, 3.3V_AUX, and
3.3V rails.

Immediately afterward, reg_fixed_voltage_probe() in drivers/regulator/fixed.c
checks config->enabled_at_boot:

	if (config->enabled_at_boot)
		gflags = GPIOD_OUT_HIGH;
	else
		gflags = GPIOD_OUT_LOW;

Because regulator-boot-on is not set on vreg_pcie_12v, vreg_pcie_3v3, or
vreg_pcie_3v3_aux, reg_fixed_voltage_probe() requests the enable GPIOs with
GPIOD_OUT_LOW, driving gpio26, gpio135, and gpio136 low again and turning off
the rails until slot_pwrctrl_probe() later enables the supplies for
pcie3a_port0.

Should output-enable and output-high be dropped from pcie_x8_12v,
pm_aux_3p3_en, and pm_main_3p3_en (matching wcn_sw_en and wwan_reg_en in this
file) so the regulator core manages the GPIO direction and level, or should
regulator-boot-on be added to the regulator nodes if these rails are expected
to remain enabled from boot?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-pcie3a_x8_1105-v1-1-d09aa0601186@oss.qualcomm.com?part=1