Re: [PATCH net-next v23 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 motorcomm,yt8824.yaml, the rules for the package `reg` exist
  only as prose, and the schema does not enforce them.

--

dt-bindings: net: Document Motorcomm YT8824 PHY package

This commit adds a new DT binding schema for the Motorcomm YT8824 quad
PHY package. The schema is based on ethernet-phy-package.yaml, and a
vendor property, motorcomm,package-mode, selects internal or 10g-qxgmii
mode. The commit also adds the new file and a co-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 0000000000000..4c67f8afcf27a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml
> @@ -0,0 +1,78 @@

[ ... ]

> +  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.

[Severity: Low]
The rules for reg are only in the description text. No allOf/if-then
block ties reg to motorcomm,package-mode, so this node would pass
dt_binding_check:

    motorcomm,package-mode = "internal";
    reg = <4>;

That passes even though the description says reg is fixed at 9 in
internal mode. Could an if/then be added that requires reg to be 9 when
motorcomm,package-mode is "internal"?

The value matters at runtime, because of_phy_package_join() uses the
package reg as base_addr:

drivers/net/phy/phy_package.c:of_phy_package_join() {
    ...
	if (of_property_read_u32(package_node, "reg", &base_addr)) {
		ret = -EINVAL;
		goto exit;
	}

	ret = phy_package_join(phydev, base_addr, priv_size);
    ...
}

The driver added later in the series ("net: phy: Add driver for
Motorcomm Quad 2.5GbE phy") has ytphy_read_top_ext() and
ytphy_write_top_ext(). Both access the shared top block through
__phy_package_read() and __phy_package_write() with offset 0, so they go
to exactly that address.

The inherited ethernet-phy-package.yaml describes reg as "Commonly the
ID of the first PHY in the PHY package". That is the value this binding
says not to use.

If a board DT sets reg to the first member PHY's address, would the
top-block page select and data writes end up in that member PHY's
register space?

The two modes also seem to use different address layouts. In the
internal-mode example below, the member PHYs are at 4-7 and the top
block is at 9. That is base plus 5, with 8 unused. External mode is
described as base plus 4.

Is that difference real in the hardware? If so, could the description
say so explicitly?

[ ... ]

> +        ethernet-phy-package@9 {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            compatible = "motorcomm,yt8824-package";
> +            reg = <9>;
> +
> +            motorcomm,package-mode = "internal";
> +
> +            ethernet-phy@4 {
> +                reg = <4>;
> +            };

[ ... ]

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