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

[netdev-bot+sashiko@kernel.org]

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

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

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] The single example in the new binding is self-inconsistent and
  contradicts the semantics it inherits: the package node is named…
- [Medium] The binding redefines the standard, MAC-side property `phy-
  mode` on a PHY *package* node with its own type reference (`$ref:…
- [Medium] The package-level `phy-mode` enum is limited to `[internal,
  usxgmii]`.
- [Low] The new binding file
  Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml is not…
- [Low] Concern (judged a false positive at end of series): the binding
  requires `phy-mode` on the package node and defines compatible…

--

dt-bindings: net: Document Motorcomm YT8824 PHY package

This adds a new binding for the Motorcomm YT8824, described as a PHY
package containing four PHYs. The binding references
ethernet-phy-package.yaml and adds a package-level phy-mode property with
the values internal and usxgmii.

A few questions about the new schema below.

> diff --git a/Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml b/Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml
> new file mode 100644
> index 0000000000000..9737364744a07
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/motorcomm,yt8824.yaml
> @@ -0,0 +1,62 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/motorcomm,yt8824.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MotorComm YT8824 Ethernet PHY
> +
> +maintainers:
> +  - Kyle Switch <kyle.switch@motor-comm.com>

[Severity: Low]
Should MAINTAINERS gain an entry for this file?  The MOTORCOMM PHY DRIVER
section currently lists the binding literally:

  MOTORCOMM PHY DRIVER
  M:	Frank <Frank.Sae@motor-comm.com>
  L:	netdev@vger.kernel.org
  S:	Maintained
  F:	Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml
  F:	drivers/net/phy/motorcomm.c

That pattern does not match motorcomm,yt8824.yaml, and no other section
matches it either, so get_maintainer.pl will not report the Motorcomm PHY
maintainer for future changes to this file.  Widening the existing entry to
Documentation/devicetree/bindings/net/motorcomm,yt8*.yaml, or adding a new
F: line, would cover it.

> +
> +description:
> +  Motorcomm YT8824 Ethernet PHY is a PHY package of 4 PHYs.
> +
> +$ref: ethernet-phy-package.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - motorcomm,yt8824-package
> +  phy-mode:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    enum: [ internal, usxgmii ]

[Severity: Medium]
Is phy-mode the right property name here, and is internal a valid value for
this device?

phy-mode is owned by ethernet-controller.yaml, as an alias of
phy-connection-type, and internal is defined there 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.
      - internal

The YT8824 is a discrete package addressed over MDIO, so that definition
does not appear to describe it.  The driver added later in the series keys
hardware bring-up off this value:

  drivers/net/phy/motorcomm.c:yt8824_config_init() {
      if (priv->interface_mode == PHY_INTERFACE_MODE_INTERNAL) {
          ret = yt8824_internal_config_init_paged(phydev);
      ...
  }

so the standard property name is being given a vendor-specific meaning that
the schema does not state.  The property is also added with no description
and placed in required:, which freezes it as ABI.

The closest peer binding, qcom,qca807x.yaml (which also references
ethernet-phy-package.yaml), uses a vendor-prefixed property with an explicit
mode table instead:

  qcom,package-mode:
    description: |
      PHY package can be configured in 3 mode following this table:

Would a described, vendor-prefixed property such as motorcomm,package-mode
be a better fit than overloading phy-mode?

[Severity: Medium]
Separately, is usxgmii the correct wire format for this package?

If the four 2.5G ports are multiplexed onto a single 10G SerDes uplink, the
kernel's interface mode for that format is 10g-qxgmii
(PHY_INTERFACE_MODE_10G_QXGMII), and ethernet-controller.yaml lists the two
as distinct values:

      - usxgmii
      - 10gbase-r
      - 25gbase-r
      - 10g-qxgmii

A MAC configured from phy-mode = "usxgmii" would program a single-channel
USXGMII PCS, which would leave the remaining three ports without a channel.

If the chip instead exposes four independent USXGMII lanes, a single
package-wide string cannot express which PHY sits on which lane, and the
binding defines no property for that mapping.  I could not determine the
host topology from the series: the only SerDes setup for the non-internal
mode is preamble padding in yt8824_extern_config_sds_init_paged(), which is
consistent with either arrangement.

Could the description: state the host-side topology explicitly?

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

[Severity: Medium]
Two things about this node.

First, the unit address is @0 while reg = <9>.  Should the node be named
ethernet-phy-package@9 so the unit address matches the first reg entry?  As
written, two YT8824 packages on the same bus would both be named
ethernet-phy-package@0, which collides under the
-Wunique_unit_address_if_enabled used by
Documentation/devicetree/bindings/Makefile.

Second, is reg = <9> consistent with the meaning inherited from
ethernet-phy-package.yaml?

  reg:
    minimum: 0
    maximum: 31
    description:
      The base ID number for the PHY package.
      Commonly the ID of the first PHY in the PHY package.

Here all four members are at 4, 5, 6 and 7, i.e. below the declared base, and
phylib only ever forms member addresses by adding an unsigned offset to the
base:

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

      if (addr_offset >= PHY_MAX_ADDR - base_addr)
          return -EIO;
      return base_addr + addr_offset;
  }

so no member of this example is reachable as base + offset.

The driver added later in the series treats the package reg as the absolute
MDIO address of the shared top-extended register block, at offset 0:

  drivers/net/phy/motorcomm.c:ytphy_write_top_ext() {
      /*
       * The package base address is used here to read the top ext registers.
       * For example, in internal mode, the top ext address is fixed at 9;
       */
      ret = __phy_package_write(phydev, 0, YTPHY_PAGE_SELECT, regnum);
      ...
  }

If that is the intended meaning, should this binding add its own reg:
description overriding the base-ID wording?  A DT author following the
inherited text and writing reg = <4> would have of_phy_package_join() store
4 as base_addr, and the page-select and page-data writes above would then
land on the member PHY at address 4 instead of the package register block.

> +
> +            phy-mode = "internal";
> +
> +            ethernet-phy@4 {
> +                reg = <4>;
> +            };
> +
> +            ethernet-phy@5 {
> +                reg = <5>;
> +            };

[ ... ]

For reference, the binding file is unchanged by the remaining two patches in
the series, so nothing later adjusts the example or the phy-mode wording.

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