Re: [PATCH PRELIMINARY 2/5] arm64: dts: qcom: shikra: add ethernet nodes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The 'qcom,shikra-ethqos' compatible string is undocumented in DT bindings, lacks a generic fallback, and has no driver support, rendering the nodes dead code.
- [Medium] The ethernet nodes define 6 clocks including 'axi-noc' and 'pcie-tile-axi-noc', violating the 4-clock limit in bindings and introducing a copy-paste error.
- [Low] Incorrect interconnect tag macro `QCOM_ICC_TAG_ALWAYS` used for RPM-based interconnects.
--

commit ab9a1a40b65e83ff44f2ceb6b0d365afac0a9e34
Author: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>

arm64: dts: qcom: shikra: add ethernet nodes

Add the two Gigabit Ethernet controllers present on Shikra (ethernet0
at 0x5d00000, ethernet1 at 0x5d20000). Both nodes are left disabled;
board files supply the PHY details.

> diff --git a/arch/arm64/boot/dts/qcom/shikra.dtsi b/arch/arm64/boot/dts/qcom/shikra.dtsi
> index c1f06f692f664..a55e20be83fe2 100644
> --- a/arch/arm64/boot/dts/qcom/shikra.dtsi
> +++ b/arch/arm64/boot/dts/qcom/shikra.dtsi

[ ... ]

> @@ -1977,6 +2027,222 @@ &clk_virt SLAVE_QUP_CORE_0 RPM_ALWAYS_TAG>,
>  			};
>  		};
>  
> +		ethernet0: ethernet@5d00000 {
> +			compatible = "qcom,shikra-ethqos";

[Severity: High]
Does this compatible string exist in the dt-bindings and does it have driver
support?

Without a standard fallback string like "qcom,sc8280xp-ethqos" and no driver
support in dwmac-qcom-ethqos.c, won't these nodes fail to probe and remain
completely uninitialized when board files try to enable them?

> +			reg = <0x0 0x05d00000 0x0 0x10000>,
> +			      <0x0 0x05d16000 0x0 0x100>;
> +			reg-names = "stmmaceth", "rgmii";
> +
> +			interrupts = <GIC_SPI 478 IRQ_TYPE_LEVEL_HIGH 0>;
> +			interrupt-names = "macirq";
> +
> +			clocks = <&gcc GCC_EMAC0_AXI_CLK>,
> +				 <&gcc GCC_EMAC0_AHB_CLK>,
> +				 <&gcc GCC_EMAC0_PTP_CLK>,
> +				 <&gcc GCC_EMAC0_RGMII_CLK>,
> +				 <&gcc GCC_EMAC0_AXI_SYS_NOC_CLK>,
> +				 <&gcc GCC_PCIE_TILE_AXI_SYS_NOC_CLK>;
> +			clock-names = "stmmaceth", "pclk", "ptp_ref", "rgmii",
> +				      "axi-noc", "pcie-tile-axi-noc";

[Severity: Medium]
Does the qcom,ethqos.yaml binding allow 6 clocks?

The binding strictly limits the clocks array to 4 items. Additionally, does it
make sense to assign a PCIe clock to the Ethernet MAC? NOC clock scaling is
usually handled by the interconnect framework via the interconnects property,
so this might be a copy-paste error that breaks static validation.

> +
> +			power-domains = <&gcc GCC_EMAC0_GDSC>;
> +			resets = <&gcc GCC_EMAC0_BCR>;
> +			iommus = <&apps_smmu 0x0380 0x0007>;
> +
> +			interconnects = <&mem_noc MASTER_AMPSS_M0 QCOM_ICC_TAG_ALWAYS
> +					 &config_noc SLAVE_EMAC0_CFG QCOM_ICC_TAG_ALWAYS>,

[Severity: Low]
Is QCOM_ICC_TAG_ALWAYS the correct tag macro here?

The Shikra SoC uses an RPM-based interconnect provider, which expects tags
from <dt-bindings/interconnect/qcom,rpm-icc.h> such as RPM_ALWAYS_TAG.
QCOM_ICC_TAG_ALWAYS is an RPMh macro defined in
<dt-bindings/interconnect/qcom,icc.h>.

[ ... ]

> +		ethernet1: ethernet@5d20000 {
> +			compatible = "qcom,shikra-ethqos";

[Severity: High]
Does this compatible string exist in the dt-bindings and does it have driver
support?

Without a standard fallback string like "qcom,sc8280xp-ethqos" and no driver
support in dwmac-qcom-ethqos.c, won't these nodes fail to probe and remain
completely uninitialized when board files try to enable them?

> +			reg = <0x0 0x05d20000 0x0 0x10000>,
> +			      <0x0 0x05d36000 0x0 0x100>;
> +			reg-names = "stmmaceth", "rgmii";
> +
> +			interrupts = <GIC_SPI 458 IRQ_TYPE_LEVEL_HIGH 0>;
> +			interrupt-names = "macirq";
> +
> +			clocks = <&gcc GCC_EMAC1_AXI_CLK>,
> +				 <&gcc GCC_EMAC1_AHB_CLK>,
> +				 <&gcc GCC_EMAC1_PTP_CLK>,
> +				 <&gcc GCC_EMAC1_RGMII_CLK>,
> +				 <&gcc GCC_EMAC1_AXI_SYS_NOC_CLK>,
> +				 <&gcc GCC_PCIE_TILE_AXI_SYS_NOC_CLK>;
> +			clock-names = "stmmaceth", "pclk", "ptp_ref", "rgmii",
> +				      "axi-noc", "pcie-tile-axi-noc";

[Severity: Medium]
Does the qcom,ethqos.yaml binding allow 6 clocks?

The binding strictly limits the clocks array to 4 items. Additionally, does it
make sense to assign a PCIe clock to the Ethernet MAC? NOC clock scaling is
usually handled by the interconnect framework via the interconnects property,
so this might be a copy-paste error that breaks static validation.

> +
> +			power-domains = <&gcc GCC_EMAC1_GDSC>;
> +			resets = <&gcc GCC_EMAC1_BCR>;
> +			iommus = <&apps_smmu 0x03a0 0x0007>;
> +
> +			interconnects = <&mem_noc MASTER_AMPSS_M0 QCOM_ICC_TAG_ALWAYS
> +					 &config_noc SLAVE_EMAC1_CFG QCOM_ICC_TAG_ALWAYS>,

[Severity: Low]
Is QCOM_ICC_TAG_ALWAYS the correct tag macro here?

The Shikra SoC uses an RPM-based interconnect provider, which expects tags
from <dt-bindings/interconnect/qcom,rpm-icc.h> such as RPM_ALWAYS_TAG.
QCOM_ICC_TAG_ALWAYS is an RPMh macro defined in
<dt-bindings/interconnect/qcom,icc.h>.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-shikra_ethernet_dts-v1-0-69c0c5c7c124@oss.qualcomm.com?part=2