Re: [PATCH v4 6/7] arm64: dts: amlogic: t7: add the ethernet controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] arm64: dts: amlogic: t7: missing status disabled on untested internal_ephy
--

commit 8b67e94cfb48fb74bfbb5053e5ce7da85b48466a
Author: Lucas Tanure <tanure@linux.com>

arm64: dts: amlogic: t7: add the ethernet controller

This patch adds the Synopsys DWMAC ethernet controller, the MDIO multiplexer,
and the internal 100M PHY nodes to the Amlogic T7 SoC device tree.

> diff --git a/arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi b/arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi
> index 7bd53ec166071..94ea225145015 100644
> --- a/arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi

[ ... ]

> @@ -645,6 +672,37 @@ gpio_intc: interrupt-controller@4080 {
>  					<10 11 12 13 14 15 16 17 18 19 20 21>;
>  			};
>  
> +			eth_phy: mdio-multiplexer@28000 {
> +				compatible = "amlogic,g12a-mdio-mux";
> +				reg = <0x0 0x28000 0x0 0xa4>;
> +				clocks = <&clkc_periphs CLKID_SYS_ETHPHY>,
> +					 <&xtal>,
> +					 <&scmi_clk CLKID_FCLK_50M>;
> +				clock-names = "pclk", "clkin0", "clkin1";
> +				mdio-parent-bus = <&mdio0>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +
> +				ext_mdio: mdio@0 {
> +					reg = <0>;
> +					#address-cells = <1>;
> +					#size-cells = <0>;
> +				};
> +
> +				int_mdio: mdio@1 {
> +					reg = <1>;
> +					#address-cells = <1>;
> +					#size-cells = <0>;
> +
> +					internal_ephy: ethernet-phy@8 {
> +						compatible = "ethernet-phy-id0180.3301",
> +							     "ethernet-phy-ieee802.3-c22";
> +						interrupts = <GIC_SPI 340 IRQ_TYPE_LEVEL_HIGH>;
> +						reg = <8>;
> +					};
> +				};
> +			};

[Severity: Medium]
The commit message states:

  The on-chip PHY follows the G12A; no board in tree uses it, so it is
  untested. It stays disabled for boards to enable.

However, in arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi, only ethmac has
status = "disabled"; while eth_phy, int_mdio, and internal_ephy are all left
enabled by default.

When a board enables ethmac (even to use an external RGMII PHY on ext_mdio),
g12a_mdio_mux_probe() calls mdio_mux_init() in drivers/net/mdio/mdio-mux.c,
which registers every available child bus:

    for_each_available_child_of_node(mux_node, child_bus_node) {
        ...
        r = of_mdiobus_register(cb->mii_bus, child_bus_node);

Because internal_ephy under int_mdio is available and specifies a hardcoded
PHY ID compatible string ("ethernet-phy-id0180.3301"),
__of_mdiobus_parse_phys() in drivers/net/mdio/of_mdio.c creates and registers
the PHY device without reading hardware ID registers:

    of_mdiobus_register()
      -> __of_mdiobus_parse_phys()
        -> fwnode_mdiobus_register_phy()
          -> phy_device_register()
            -> device_add()
              -> phy_probe()

During phy_probe() in drivers/net/phy/phy_device.c, MDIO reads issued by
phy_disable_interrupts() and genphy_read_abilities() switch the mux to child
bus 1 via g12a_mdio_switch_fn() -> g12a_enable_internal_mdio() in
drivers/net/mdio/mdio-mux-meson-g12a.c:

    if (!__clk_is_enabled(priv->pll)) {
        ret = clk_prepare_enable(priv->pll);
        if (ret)
            return ret;
    }
    ...
    writel(PHY_CNTL2_USE_INTERNAL |
           PHY_CNTL2_SMI_SRC_MAC |
           PHY_CNTL2_RX_CLK_EPHY,
           priv->regs + ETH_PHY_CNTL2);

    value |= PHY_CNTL1_PHY_ENB;
    writel(value, priv->regs + ETH_PHY_CNTL1);

This powers on the untested internal EPHY PLL and enables the internal PHY in
ETH_PHY_CNTL1 and ETH_PHY_CNTL2 even on boards using an external PHY.

Additionally, on T7 boards that leave ethmac disabled, leaving eth_phy
enabled causes g12a_mdio_mux_probe() to enable pclk, program the PLL
registers in g12a_ephy_pll_init(), and then fail in mdio_mux_init() with
-EPROBE_DEFER because mdio0 is never registered.

Should internal_ephy (or int_mdio) and eth_phy have status = "disabled"; so
that the untested internal PHY stays disabled until a board explicitly
enables it?

>  
>  			uart_a: serial@78000 {
>  				compatible = "amlogic,t7-uart", "amlogic,meson-s4-uart";
>  				reg = <0x0 0x78000 0x0 0x18>;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022200640.3101932-1-tanure@linux.com?part=6