Re: [PATCH net-next v24 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] Both PHY nodes in the example in
  Documentation/devicetree/bindings/net/airoha,an8855-phy.yaml list…
- [Low] The example in
  Documentation/devicetree/bindings/net/airoha,an8855-phy.yaml gives…

--

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

This patch adds a DT binding for the internal PHYs of the Airoha AN8855
5-port Gigabit Switch, which are exposed on the MDIO bus. Each PHY can
optionally reference four eFUSE NVMEM cells (tx_a..tx_d) that hold TX
calibration data.

> 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

[ ... ]

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

[Severity: Medium]
Should these PHYs be described as "ethernet-phy-ieee802.3-c22" and not
c45? Both example nodes use the c45 compatible.

In fwnode_mdiobus_register_phy(), the c45 compatible makes phylib ignore
the explicit PHY ID and probe the device using Clause 45:

drivers/net/mdio/fwnode_mdio.c:fwnode_mdiobus_register_phy() {
    ...
    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);
    ...
}

Each AN8855 PHY is on a per-PHY bus that an8855_phy_bus_register()
creates via devm_mdio_regmap_register(). That bus provides only C22
accessors:

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

That means get_phy_c45_ids()->get_phy_c45_devs_in_pkg() gets -EOPNOTSUPP
and returns -EIO. The PHY registration then fails, and
an8855_core_probe() in drivers/mfd/airoha-an8855.c goes to
err_put_fwnodes. As a result, neither the PHYs nor the switch/efuse MFD
cells are registered.

The PHY driver in drivers/net/phy/air_an8855.c matches the C22 ID with
PHY_ID_MATCH_EXACT(0xc0ff0410) and uses C22 paged access. Also, the MFD
binding example in the same series (airoha,an8855.yaml) uses
"ethernet-phy-ieee802.3-c22" for these PHYs.

If a board DT follows this example, won't the whole AN8855 probe fail?

> +
> +            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 ethernet-phy@2 meant to get port 0's calibration cells here, while
ethernet-phy@1 gets none?

an8855_phy_bus_register() in drivers/mfd/airoha-an8855.c maps the MDIO
address to a port relative to the switch address:

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

Because ethernet-phy@1 is present, switch_addr has to be 0 or 1. That
makes address 2 port 1 or port 2, and never port 0.

The eFUSE binding (airoha,an8855-efuse.yaml) has a separate tx_a..tx_d
set for each port, for example shift_sel_port0_tx_a@c and
shift_sel_port1_tx_a@1c. The MFD binding example in the same series
wires internal_phy1 (reg 1) to shift_sel_port0_* and internal_phy2
(reg 2) to shift_sel_port1_*, which does not match this example.

A DT copied from this example would have an8855_probe() read port 0's
R50-ohm values. an8855_config_init() would then write them into the
second PHY's AN8855_PHY_R50OHM_RSEL_TX_AB/CD registers, and the first
PHY would stay uncalibrated. No error would be reported.

Should the cells move to ethernet-phy@1, or should ethernet-phy@2 use
the shift_sel_port1_* cells?

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

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