Re: [PATCH v2 2/2] arm64: dts: qcom: lemans-sa8255p: add ethernet nodes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Low] arm64: dts: qcom: lemans-gearvm: missing SCMI power-domains on ethernet nodes
- [Low] arm64: dts: qcom: lemans-sa8255p-ride: non-compliant MDIO PHY nodes
- [Low] arm64: dts: qcom: lemans-sa8255p: out-of-order soc and serdes nodes
- [Low] arm64: dts: qcom: lemans-sa8255p-ride: invalid snps,ps-speed = <2500>
--

commit 81e11e3cdbf7c26506804bb78d74a7572b18ad5a
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

arm64: dts: qcom: lemans-sa8255p: add ethernet nodes

This commit adds the ethernet0 and ethernet1 nodes along with their serdes PHY
nodes for the gearvm variant of the lemans SA8255P Ride board, and enables
scmi7 and scmi8 to serve the EMACs.

> diff --git a/arch/arm64/boot/dts/qcom/lemans-gearvm.dtsi b/arch/arm64/boot/dts/qcom/lemans-gearvm.dtsi
> index 91c97a55f2787..96a54f32f0eeb 100644
> --- a/arch/arm64/boot/dts/qcom/lemans-gearvm.dtsi
> +++ b/arch/arm64/boot/dts/qcom/lemans-gearvm.dtsi
> @@ -58,6 +58,18 @@ &cpufreq_hw {
>  	clock-names = "xo", "alternate";
>  };
>  
> +&ethernet0 {
> +	compatible = "qcom,sa8255p-ethqos";
> +	phys = <&serdes0>;
> +	phy-names = "serdes";
> +};
> +
> +&ethernet1 {
> +	compatible = "qcom,sa8255p-ethqos";
> +	phys = <&serdes1>;
> +	phy-names = "serdes";
> +};

[Severity: Low]
Should power-domains and power-domain-names for ethernet0 and ethernet1 be
placed here in arch/arm64/boot/dts/qcom/lemans-gearvm.dtsi rather than in
arch/arm64/boot/dts/qcom/lemans-sa8255p-ride.dts?

Documentation/devicetree/bindings/net/qcom,sa8255p-ethqos.yaml marks both
power-domains and power-domain-names as required properties when compatible is
set to qcom,sa8255p-ethqos.

Because scmi7_pd and scmi8_pd are defined in lemans-gearvm.dtsi, and all other
peripherals in lemans-gearvm.dtsi (including serdes0, serdes1, and ufs_mem_hc,
as well as ethernet0 and ethernet1 in lemans-embedded.dtsi) define their
SoC/VM power-domains in the DTSI file, any other board DTS including
lemans-gearvm.dtsi without duplicating those properties would fail devicetree
schema validation.

[ ... ]

> @@ -3218,6 +3230,24 @@ &uart21 {
>  			     "perf";
>  };
>  
> +&soc {
> +	serdes0: phy@8901000 {
> +		compatible = "qcom,sa8255p-dwmac-sgmii-phy";
> +		reg = <0x0 0x08901000 0x0 0xe10>;
> +		#phy-cells = <0>;
> +		power-domains = <&scmi7_dvfs 0>;
> +		status = "disabled";
> +	};
> +
> +	serdes1: phy@8902000 {
> +		compatible = "qcom,sa8255p-dwmac-sgmii-phy";
> +		reg = <0x0 0x08902000 0x0 0xe10>;
> +		#phy-cells = <0>;
> +		power-domains = <&scmi8_dvfs 0>;
> +		status = "disabled";
> +	};
> +};

[Severity: Low]
This isn't a bug, but both arch/arm64/boot/dts/qcom/lemans-gearvm.dtsi and
arch/arm64/boot/dts/qcom/lemans-sa8255p-ride.dts sort top-level label
references alphabetically.

In lemans-gearvm.dtsi, &soc is placed after &uart21 instead of between &scm
and &spi0, and status = "disabled" in serdes0 and serdes1 is missing a
preceding blank line. Similarly, in lemans-sa8255p-ride.dts, &serdes0 and
&serdes1 are placed after &tlmm instead of between &scmi23 and &sleep_clk.
Could these nodes be moved into alphabetical order?

> diff --git a/arch/arm64/boot/dts/qcom/lemans-sa8255p-ride.dts b/arch/arm64/boot/dts/qcom/lemans-sa8255p-ride.dts
> index 6f19064997a4b..2b3c9e90802bf 100644
> --- a/arch/arm64/boot/dts/qcom/lemans-sa8255p-ride.dts
> +++ b/arch/arm64/boot/dts/qcom/lemans-sa8255p-ride.dts

[ ... ]

> @@ -101,6 +103,179 @@ trip1 {
>  	};
>  };
>  
> +&ethernet0 {
> +	phy-handle = <&sgmii_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>;
> +	snps,ps-speed = <2500>;

[Severity: Low]
Is snps,ps-speed = <2500> on ethernet0 and ethernet1 intended here?

In drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c,
stmmac_probe_config_dt() parses snps,ps-speed into plat->mac_port_sel_speed,
which is only consumed by stmmac_check_pcs_mode() in
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:

    static void stmmac_check_pcs_mode(struct stmmac_priv *priv)
    {
        int interface = priv->plat->interface;

        if (priv->dma_cap.pcs) {
            if ((interface == PHY_INTERFACE_MODE_RGMII) ||
                (interface == PHY_INTERFACE_MODE_RGMII_ID) ||
                (interface == PHY_INTERFACE_MODE_RGMII_RXID) ||
                (interface == PHY_INTERFACE_MODE_RGMII_TXID)) {
                netdev_dbg(priv->dev, "PCS RGMII support enabled\n");
                priv->hw->pcs = STMMAC_PCS_RGMII;
            } else if (interface == PHY_INTERFACE_MODE_SGMII) {
                netdev_dbg(priv->dev, "PCS SGMII support enabled\n");
                priv->hw->pcs = STMMAC_PCS_SGMII;
            }
        }
    }

When evaluated, stmmac_check_pcs_mode() only checks PHY_INTERFACE_MODE_SGMII
and only accepts speeds of 10, 100, or 1000. Because ethernet0 and ethernet1
use external PHYs with phy-mode = "2500base-x", snps,ps-speed = <2500> is
unused and specifies an unsupported speed value. Should snps,ps-speed be
dropped from both ethernet0 and ethernet1?

> +
> +	power-domains = <&scmi7_pd 0>, <&scmi7_pd 1>;
> +	power-domain-names = "core", "mdio";
> +
> +	status = "okay";
> +
> +	mdio {
> +		compatible = "snps,dwmac-mdio";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		sgmii_phy0: phy@8 {
> +			compatible = "ethernet-phy-id31c3.1c33";
> +			reg = <0x8>;
> +			device_type = "ethernet-phy";
> +			resets = <&scmi7_rst 0>;
> +			reset-names = "phy";
> +			reset-assert-us = <11000>;
> +			reset-deassert-us = <70000>;
> +		};
> +
> +		sgmii_phy1: phy@0 {
> +			compatible = "ethernet-phy-id31c3.1c33";
> +			reg = <0x0>;
> +			device_type = "ethernet-phy";
> +			resets = <&scmi8_rst 0>;
> +			reset-names = "phy";
> +			reset-assert-us = <11000>;
> +			reset-deassert-us = <70000>;
> +		};
> +	};

[Severity: Low]
Will sgmii_phy0 and sgmii_phy1 under ethernet0->mdio pass make dtbs_check with
this node naming and property set?

Documentation/devicetree/bindings/net/ethernet-phy.yaml selects Ethernet PHY
nodes via the nodename pattern ^ethernet-phy(@[a-f0-9]+)?$, while
Documentation/devicetree/bindings/net/snps,dwmac.yaml references mdio.yaml#
with unevaluatedProperties: false. Naming the child nodes phy@8 and phy@0
prevents ethernet-phy.yaml from matching them, leaving compatible, resets, and
reset-names unevaluated against mdio.yaml.

In addition, both nodes include the deprecated pre-FDT property
device_type = "ethernet-phy" (which is not allowed by dtschema and is not
needed by of_mdiobus_child_is_phy() in drivers/net/mdio/of_mdio.c), and unit
address 8 is placed before unit address 0.

Should these be renamed to ethernet-phy@0 and ethernet-phy@8, ordered by
ascending unit address, and have device_type = "ethernet-phy" removed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-sa8255p-emac-dts-v2-0-05f9355df966@oss.qualcomm.com?part=2