Re: [PATCH v2 3/3] arm64: dts: qcom: lemans: Add LeMans Draco EVK board

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] arm64: dts: qcom: wrong deprecated phy-supply on serdes0 and
  serdes1
- [High] arm64: dts: qcom: missing enable-active-high on DP power
  regulators
- [Low] arm64: dts: qcom: Makefile entries for lemans-draco-evk out of
  order
--

commit 262a58ee1d4a0c406732397254080c92d772bece
Author: Gopikishan Thawre <gopikishan.thawre@oss.qualcomm.com>

arm64: dts: qcom: lemans: Add LeMans Draco EVK board

This commit adds the device tree for the LeMans Draco EVK platform on top
of the LeMans Draco EVK SoM.

> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> index 4e714251d5e6b..f1713d53af5f6 100644
> --- a/arch/arm64/boot/dts/qcom/Makefile
> +++ b/arch/arm64/boot/dts/qcom/Makefile
> @@ -50,6 +50,11 @@ dtb-$(CONFIG_ARCH_QCOM)	+= ipq9650-rdp488.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= kaanapali-mtp.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= kaanapali-qrd.dtb
>  dtb-$(CONFIG_ARCH_QCOM)	+= lemans-evk.dtb
> +dtb-$(CONFIG_ARCH_QCOM)	+= lemans-draco-evk.dtb
> +
> +lemans-draco-evk-el2-dtbs := lemans-draco-evk.dtb lemans-el2.dtbo
> +
> +dtb-$(CONFIG_ARCH_QCOM)	+= lemans-draco-evk-el2.dtb
>  
>  lemans-evk-camera-csi1-imx577-dtbs	:= lemans-evk.dtb lemans-evk-camera-csi1-imx577.dtbo
>  lemans-evk-camera-dtbs	:= lemans-evk.dtb lemans-evk-camera.dtbo

[Severity: Low]
This isn't a bug, but the lemans-draco-evk.dtb and lemans-draco-evk-el2.dtb
entries sort alphabetically before lemans-evk.dtb.

> diff --git a/arch/arm64/boot/dts/qcom/lemans-draco-evk.dts b/arch/arm64/boot/dts/qcom/lemans-draco-evk.dts
> new file mode 100644
> index 0000000000000..6dfbc8e2d8ff6
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/lemans-draco-evk.dts

[ ... ]

> +	vreg_dp0_pwr: regulator-dp0-pwr {
> +		compatible = "regulator-fixed";
> +		regulator-name = "VREG_DP0_PWR_3P3";
> +
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +
> +		gpio = <&expander1 7 GPIO_ACTIVE_HIGH>;
> +	};
> +
> +	vreg_dp1_pwr: regulator-dp1-pwr {
> +		compatible = "regulator-fixed";
> +		regulator-name = "VREG_DP1_PWR_3P3";
> +
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +
> +		gpio = <&expander1 8 GPIO_ACTIVE_HIGH>;
> +	};
> +
> +	vreg_dp2_pwr: regulator-dp2-pwr {
> +		compatible = "regulator-fixed";
> +		regulator-name = "VREG_DP2_PWR_3P3";
> +
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +
> +		gpio = <&expander1 9 GPIO_ACTIVE_HIGH>;
> +	};
> +
> +	vreg_dp3_pwr: regulator-dp3-pwr {
> +		compatible = "regulator-fixed";
> +		regulator-name = "VREG_DP3_PWR_3P3";
> +
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +
> +		gpio = <&expander1 10 GPIO_ACTIVE_HIGH>;
> +	};

[Severity: High]
Are vreg_dp0_pwr through vreg_dp3_pwr missing the enable-active-high
property?

For regulator-fixed nodes, of_gpio_set_polarity_by_property() ignores
GPIO_ACTIVE_HIGH in the gpio specifier and forces OF_GPIO_ACTIVE_LOW unless
enable-active-high is present, which will invert the enable polarity on
expander1 pins 7-10 and turn off power to DP0-DP3 when enabled.

[ ... ]

> +&serdes0 {
> +	phy-supply = <&vreg_l5a>;
> +
> +	status = "okay";
> +};
> +
> +&serdes1 {
> +	phy-supply = <&vreg_l5a>;
> +
> +	status = "okay";
> +};

[Severity: High]
Should serdes0 and serdes1 specify vdda-0p9-supply = <&vreg_l4a> and
vdda-1p2-supply = <&vreg_l1c> instead of the deprecated phy-supply?

In drivers/phy/qualcomm/phy-qcom-sgmii-eth.c, qcom_dwmac_sgmii_phy_probe()
only requests the vdda-0p9 and vdda-1p2 supplies:

drivers/phy/qualcomm/phy-qcom-sgmii-eth.c:
	static const struct regulator_bulk_data qcom_dwmac_sgmii_phy_vregs[] = {
		{ .supply = "vdda-0p9", .init_load_uA = 46000 },
		{ .supply = "vdda-1p2", .init_load_uA = 15000 },
	};

With only phy-supply specified in the DT, the regulator framework
substitutes dummy regulators for vdda-0p9 and vdda-1p2, leaving the 0.9V
and 1.2V SerDes rails unvoted when ethernet0 or ethernet1 powers on the PHY
and causing SerDes calibration to fail.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-lemans-draco-final-changes-bringup-v2-0-3ae8cd1c9932@oss.qualcomm.com?part=3