Re: [PATCH net-next v25 02/10] dt-bindings: net: Document support for AN8855 Switch Internal PHY

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In the example of
  Documentation/devicetree/bindings/net/airoha,an8855-phy.yaml, both PHY…
- [Low] In the example of
  Documentation/devicetree/bindings/net/airoha,an8855-phy.yaml,…

--

dt-bindings: net: Document support for AN8855 Switch Internal PHY

This adds a YAML binding for the internal PHYs of the Airoha AN8855
5-port Gigabit Switch. These PHYs are exposed on the MDIO bus. The binding
also lets each PHY node reference the SoC eFUSE NVMEM cells (tx_a..tx_d)
used for calibration.

> diff --git a/Documentation/devicetree/bindings/net/airoha,an8855-phy.yaml b/Documentation/devicetree/bindings/net/airoha,an8855-phy.yaml
> new file mode 100644
> index 0000000000000..d2f86116badf4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/airoha,an8855-phy.yaml
> @@ -0,0 +1,83 @@

[ ... ]

> +examples:
> +  - |
> +    mdio {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        ethernet-phy@1 {
> +            compatible = "ethernet-phy-idc0ff.0410",
> +                         "ethernet-phy-ieee802.3-c45";

[Severity: Medium]
Should this be "ethernet-phy-ieee802.3-c22" (or just the ID compatible)?
The same applies to ethernet-phy@2 below.

Later in the series, an8855_core_probe() in drivers/mfd/airoha-an8855.c
registers each internal PHY on a bus created by
devm_mdio_regmap_register(). That bus only provides C22 accessors:

drivers/net/mdio/mdio-regmap.c:devm_mdio_regmap_register() {
    ...
    mii->read = mdio_regmap_read_c22;
    mii->write = mdio_regmap_write_c22;
    ...
}

When the c45 compatible is present, fwnode_mdiobus_register_phy() ignores
the explicit c0ff.0410 ID and forces Clause 45 probing:

    is_c45 = fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45");
    if (is_c45 || fwnode_get_phy_id(child, &phy_id))
        phy = get_phy_device(bus, addr, is_c45);

bus->read_c45 is NULL, so __mdiobus_c45_read() returns -EOPNOTSUPP. The
call chain is:

get_phy_device()->get_phy_c45_ids()->get_phy_c45_devs_in_pkg()

and get_phy_c45_ids() turns that error into -EIO. an8855_core_probe()
treats a fwnode_mdiobus_register_phy() failure as fatal
(goto err_put_fwnodes). So a DT copied from this example would make the
whole AN8855 MFD fail to probe, and neither the switch nor the eFUSE
device would be created.

The PHY driver later in the series (drivers/net/phy/air_an8855.c) also
uses C22 paged access, and the MFD binding in the same series
("dt-bindings: mfd: Document support for Airoha AN8855 Switch SoC") uses
"ethernet-phy-ieee802.3-c22" for these same PHYs. ethernet-phy.yaml
accepts both strings, so dt_binding_check won't flag the mismatch.

> +
> +            reg = <1>;
> +        };
> +
> +        ethernet-phy@2 {
> +            compatible = "ethernet-phy-idc0ff.0410",
> +                         "ethernet-phy-ieee802.3-c45";
> +
> +            reg = <2>;
> +
> +            nvmem-cells = <&shift_sel_port0_tx_a>,
> +                          <&shift_sel_port0_tx_b>,
> +                          <&shift_sel_port0_tx_c>,
> +                          <&shift_sel_port0_tx_d>;

[Severity: Low]
Is the PHY at reg 2 meant to use port 0's calibration cells here? In this
example ethernet-phy@1 has no cells and ethernet-phy@2 uses
shift_sel_port0_tx_*.

The MFD binding later in the series maps them differently:
ethernet-phy@1 (internal_phy1) uses shift_sel_port0_tx_*, ethernet-phy@2
(internal_phy2) uses shift_sel_port1_tx_*, port@0 is internal_phy1 and
port@1 is internal_phy2.

an8855_phy_bus_register() computes the port from the address:

    phy_offset = addr - priv->switch_addr;
    if (phy_offset >= AN8855_MAX_PHY_PORT)
        return -EINVAL;

So when both reg 1 and reg 2 are present, the PHY at reg 2 can't be
port 0.

air_an8855.c reads tx_a..tx_d in an8855_probe() and writes them into the
VEND1 R50OHM_RSEL registers in an8855_config_init(). A DTS copied from this
example would put port 0's trim on a different PHY and leave the port 0 PHY
uncalibrated.

Would it make sense to match the MFD example, with port0 cells on
ethernet-phy@1 and port1 cells on ethernet-phy@2?

> +            nvmem-cell-names = "tx_a", "tx_b", "tx_c", "tx_d";
> +        };
> +    };

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009161102.38961-1-ansuelsmth%40gmail.com