Re: [PATCH net-next v27 1/3] dt-bindings: net: Document Motorcomm YT8824 PHY package

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml,
  the package 'reg' is redefined as the absolute MDIO address of the…

--

dt-bindings: net: Document Motorcomm YT8824 PHY package

This commit adds a DT binding schema for the Motorcomm YT8824, a package
of four PHYs, built on ethernet-phy-package.yaml. It adds a
motorcomm,package-mode property and defines the package reg as the MDIO
address of the shared top extend register block. It also adds the new
file and a second maintainer to the MOTORCOMM PHY DRIVER entry in
MAINTAINERS.

> diff --git a/Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml b/Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml
> new file mode 100644
> index 000000000000..4c67f8afcf27
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml

[ ... ]

> +  reg:
> +    description:
> +      The absolute MDIO address of the YT8824 shared top extend
> +      register block.
> +      For internal mode, this is fixed at 9;
> +      For external mode, it is the base MDIO address of the four
> +      member PHYs plus 4, where base is typically 0x0.
> +      Note that this is NOT the reg of the first member PHY.
> +
> +required:
> +  - compatible
> +  - motorcomm,package-mode
> +  - reg
> +
> +unevaluatedProperties: false

[Severity: Low]
Does the schema enforce "fixed at 9" for internal mode anywhere? I could
not find an allOf/if-then on motorcomm,package-mode that sets reg to
const 9. So a node with package-mode = "internal" and any reg in the
0..31 range inherited from ethernet-phy-package.yaml would still pass
dt_binding_check.

The two address rules also don't line up. The internal mode example below
has the member PHYs at 4..7 and reg = <9>:

    ethernet-phy-package@9 {
        ...
        reg = <9>;

        motorcomm,package-mode = "internal";

        ethernet-phy@4 {

For that layout, the "base plus 4" rule from external mode would give 8.
Could the description explain why the top extend block sits at a different
offset in internal mode?

The reg value matters at runtime. Later in this series,
of_phy_package_join() in drivers/net/phy/phy_package.c reads the package
node's reg into base_addr:

    if (of_property_read_u32(package_node, "reg", &base_addr)) {

and ytphy_read_top_ext()/ytphy_write_top_ext() in drivers/net/phy/motorcomm.c
access offset 0 from that base:

    ret = __phy_package_write(phydev, 0, YTPHY_PAGE_SELECT, regnum);

With a wrong reg, every top extend access (page select, SerDes PLL control
and so on) would go to a member PHY or to an empty MDIO address.

Would it make sense to add an if/then on motorcomm,package-mode that
constrains reg to const 9 for internal mode?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261020021929.3028490-1-kyle.switch%40motor-comm.com