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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 0

- [Medium] Self-contradictory addressing in the new binding example: the
  package node is named `ethernet-phy-package@0`, carries `reg = <9>`,…
- [Medium] `phy-mode` is placed in `required` for the YT8824 PHY-package
  node…

--

dt-bindings: net: Document Motorcomm YT8824 PHY package

This adds a new binding for the Motorcomm YT8824, a package of four
Ethernet PHYs.  The schema references the common ethernet-phy-package.yaml
and requires compatible, phy-mode and reg.

> diff --git a/Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml b/Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml
> new file mode 100644
> index 0000000000000..93e9f765404a3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml
> @@ -0,0 +1,59 @@

[ ... ]

> +$ref: ethernet-phy-package.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - motorcomm,yt8824-package
> +
> +required:
> +  - compatible
> +  - phy-mode
> +  - reg

[Severity: Medium]
Is phy-mode defined anywhere in the schema chain that applies to this node?

The properties: block here defines only compatible.  The $ref'd
ethernet-phy-package.yaml defines only $nodename, reg, #address-cells,
#size-cells and the ^ethernet-phy@[a-f0-9]+$ children.  phy-mode is defined
by ethernet-controller.yaml:

  phy-mode:
    $ref: "#/properties/phy-connection-type"

which is a MAC-side binding and is not referenced here, so its enum never
applies to this package node.  Does that mean only the presence of phy-mode
is checked, and phy-mode = "sgmii" or even a non-string phy-mode = <0> would
pass make dt_binding_check for this compatible?

The driver added by the companion patch "net: phy: Add driver for Motorcomm
Quad 2.5GbE phy" accepts only two values in
yt8824_phy_package_probe_once():

  if (!of_property_read_string(np, "phy-mode", &interface_mode_name)) {
      if (!strcasecmp(interface_mode_name, phy_modes(PHY_INTERFACE_MODE_USXGMII))) ...
      else if (!strcasecmp(interface_mode_name, phy_modes(PHY_INTERFACE_MODE_INTERNAL))) ...
      else
          return -EINVAL;

so anything the unconstrained schema lets through fails probe with -EINVAL
and takes all four ports down.  Would it make sense to define the property
locally with an explicit type and the enum of modes the YT8824 supports?

For comparison, qcom,qca807x.yaml expresses the package-wide interface
setting as a locally defined and constrained vendor property:

  qcom,package-mode:
    $ref: /schemas/types.yaml#/definitions/string
    enum:
      - qsgmii
      - psgmii

and requires only compatible.

> +
> +unevaluatedProperties: false

[Severity: Low]
Does unevaluatedProperties: false have any effect here?  The referenced
ethernet-phy-package.yaml ends with additionalProperties: true, which looks
like it neutralises the restriction on property names for this node.

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

[Severity: Medium]
Three different addresses appear for the same package in this example: the
unit address @0, reg = <9>, and member PHYs at 4, 5, 6 and 7.  Which one is
correct?

The unit address should match the first reg cell, so the node would be
ethernet-phy-package@9 if reg really is 9.

The referenced ethernet-phy-package.yaml describes reg as:

      The base ID number for the PHY package.
      Commonly the ID of the first PHY in the PHY package.

and says each member PHY reg is the absolute MDIO address.  With a base of 9
and members at 4-7, every member is at a negative offset from the base.  Can
phy_package_address() express that?

drivers/net/phy/phy_package.c:phy_package_address() {
    u8 base_addr = shared->base_addr;
    ...
    return base_addr + addr_offset;
}

There is also a runtime consequence for a board DT copied from this example.
of_phy_package_join() reads this reg into base_addr, and the YT8824 driver
from the companion patch does all package-global accesses at offset 0:

drivers/net/phy/motorcomm.c:ytphy_write_top_ext() {
    ret = __phy_package_write(phydev, 0, YTPHY_PAGE_SELECT, regnum);
    ...
}

so package-global configuration would be driven at MDIO address 9, outside
the 4-7 block the package occupies, and bus->shared[9] would collide with
any other package legitimately based at 9.

Note that dtc does not catch the mismatch: the unit_address_vs_reg check in
scripts/dtc/checks.c only verifies that a unit name and a reg are both
present or both absent, not that they match, and scripts/dtc/dt-check-style
only checks unit-address formatting.  The in-tree user of the same common
schema, qcom,qca807x.yaml, keeps all three consistent
(ethernet-phy-package@0, reg = <0>, first child ethernet-phy@0).

> +
> +            phy-mode = "internal";

[Severity: Low]
Is "internal" the intended value to document for an external package?
ethernet-controller.yaml describes internal as "There is not a standard bus
between the MAC and the PHY, something proprietary is being used to embed
the PHY in the MAC", which does not seem to describe a discrete 4-port PHY
package attached over MDIO.  Can a single package-level value describe the
four individual MAC-facing links?

[ ... ]

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