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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) 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: missing unit-address on hsgmii_phy0 and hsgmii_phy1
--

commit 1c47953bff4f7712f8c14ac2bdb120d7a18d16d2
Author: Gopikishan Thawre <gopikishan.thawre@oss.qualcomm.com>

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

This commit adds the device tree for the LeMans Draco EVK platform, combining
the LeMans Draco EVK SoM and Draco carrier board with board-specific
peripherals including GPIO expanders, EEPROM, DisplayPort, Ethernet, TPM, and
fan controller.

> 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..5238157ad0029
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/lemans-draco-evk.dts
> @@ -0,0 +1,543 @@

[ ... ]

> +	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]
Should vreg_dp0_pwr, vreg_dp1_pwr, vreg_dp2_pwr, and vreg_dp3_pwr include the
enable-active-high property?

When reg_fixed_voltage_probe() requests the gpio descriptor for a
regulator-fixed node, the call chain:

  of_find_gpio()
    -> of_gpio_try_fixup_polarity()
      -> of_gpio_set_polarity_by_property()
        -> of_gpio_quirk_polarity()

in drivers/gpio/gpiolib-of.c ignores the GPIO_ACTIVE_HIGH phandle flag and
checks only for the presence of enable-active-high:

	/*
	 * The regulator GPIO handles are specified such that the
	 * presence or absence of "enable-active-high" solely controls
	 * the polarity of the GPIO line. Any phandle flags must
	 * be actively ignored.
	 */
#if IS_ENABLED(CONFIG_REGULATOR_FIXED_VOLTAGE)
	{ "regulator-fixed",   "gpios",        "enable-active-high" },
	{ "regulator-fixed",   "gpio",         "enable-active-high" },

Without enable-active-high, of_gpio_quirk_polarity() overrides the polarity to
active-low, inverting the enable signal on all four DisplayPort 3.3V power
regulators.

[ ... ]

> +&ethernet0 {
> +	phy-handle = <&hsgmii_phy0>;
> +	phy-mode = "2500base-x";
> +
> +	pinctrl-0 = <&ethernet0_default>;
> +	pinctrl-names = "default";
> +
> +	snps,mtl-rx-config = <&mtl_rx_setup>;
> +	snps,mtl-tx-config = <&mtl_tx_setup>;
> +
> +	nvmem-cells = <&mac_addr0>;
> +	nvmem-cell-names = "mac-address";
> +
> +	status = "okay";
> +
> +	mdio {
> +		compatible = "snps,dwmac-mdio";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		hsgmii_phy0: ethernet-phy {
> +			compatible = "ethernet-phy-id004d.d101";
> +			reg = <0x1c>;
> +			reset-gpios = <&pmm8654au_2_gpios 8 GPIO_ACTIVE_LOW>;
> +			reset-assert-us = <11000>;
> +			reset-deassert-us = <70000>;
> +		};
> +	};

[ ... ]

> +&ethernet1 {
> +	phy-handle = <&hsgmii_phy1>;
> +	phy-mode = "2500base-x";
> +
> +	pinctrl-0 = <&ethernet1_default>;
> +	pinctrl-names = "default";
> +
> +	snps,mtl-rx-config = <&mtl_rx_setup1>;
> +	snps,mtl-tx-config = <&mtl_tx_setup1>;
> +
> +	nvmem-cells = <&mac_addr1>;
> +	nvmem-cell-names = "mac-address";
> +
> +	status = "okay";
> +
> +	mdio {
> +		compatible = "snps,dwmac-mdio";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		hsgmii_phy1: ethernet-phy {
> +			compatible = "ethernet-phy-id004d.d101";
> +			reg = <0x1c>;
> +			reset-gpios = <&pmm8654au_2_gpios 9 GPIO_ACTIVE_LOW>;
> +			reset-assert-us = <11000>;
> +			reset-deassert-us = <70000>;
> +		};
> +	};

[Severity: Low]
Should the hsgmii_phy0 and hsgmii_phy1 nodes be named ethernet-phy@1c instead
of ethernet-phy?

Both nodes define reg = <0x1c> under an mdio bus with #address-cells = <1>, so
omitting the unit-address triggers a check_unit_address_vs_reg() warning in
dtc ("node has a reg or ranges property, but no unit name") and fails
dtbs_check against Documentation/devicetree/bindings/net/mdio.yaml.

[ ... ]

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

[Severity: High]
Does configuring serdes0 and serdes1 with phy-supply = <&vreg_l5a> leave the
SGMII PHY without its required 0.9V and 1.2V regulator votes?

In Documentation/devicetree/bindings/phy/qcom,sa8775p-dwmac-sgmii-phy.yaml,
phy-supply is deprecated in favor of vdda-0p9-supply and vdda-1p2-supply. In
drivers/phy/qualcomm/phy-qcom-sgmii-eth.c, qcom_dwmac_sgmii_phy_probe() calls
devm_regulator_bulk_get_const() for qcom_dwmac_sgmii_phy_vregs[]:

	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 },
	};

The driver does not request phy-supply, so dummy regulators will be
substituted when qcom_dwmac_sgmii_phy_power_on() enables the bulk supplies
prior to qcom_dwmac_sgmii_phy_calibrate().

As updated for other LeMans boards in commits 4f81684a1d10, 457cf65a7997, and
0d567fb7def9, should serdes0 and serdes1 use:

	vdda-0p9-supply = <&vreg_l4a>;
	vdda-1p2-supply = <&vreg_l1c>;

instead of phy-supply = <&vreg_l5a>?

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