Re: [net-next RFC PATCH 03/14] dt-bindings: net: document ethernet PHY package nodes

[Vladimir Oltean <olteanv@gmail.com>]

On Wed, Nov 22, 2023 at 07:32:22PM +0100, Christian Marangi wrote:
> Sooooo.... Sorry if I insist but I would really love to have something
> ""stable"" to move this further. (the changes are easy enough so it's
> really a matter of finding a good DT structure)
> 
> Maybe a good idea would be summarize the concern and see what solution
> was proposed:

Sorry, I didn't follow the entire discussion. I hope I'm not too far off
with my understanding of your problems.

I think you are hitting some of the same points I have hit with DSA.
The PHY package could be considered an SoC with lots of peripherals on
it, for which you'd want separate drivers. Just like a DSA switch would.
I don't think it's exactly phylib's place to deal with that, just like
it's not DSA's place to deal with complex SoCs, just with the switching
IP (like the Ethernet PHY IP for phylib).
https://lore.kernel.org/lkml/20221222134844.lbzyx5hz7z5n763n@skbuf/

Why does the ethernet-phy-package DT binding attempt to be so grand and
generic? I would expect the 180 degree opposite. Make it have a _single_
compatible of "qcom,qca807x" (but don't use an "x" wildcard, do specify
the full package name).

Make it have a "reg" property which is the base MDIO address of the package.

Write an mdio_device driver that probes on that. The PHY core already
knows that if a child on the MDIO bus has a compatible string of the
normal form (not like "ethernet-phy-id004d.d0b2"), then it's looking at
an mdio_device.

Make the OF node of the package have an "mdio" child with its own
compatible string, which represents the place where PHYs are. The driver
for the "mdio" child has a very simple implementation of the mii_bus
ops, which just calls into the device parent (it can assume a certain
parent implementation and private data structures).

Lateral to the "mdio" child node of the "qcom,qca807x" package node, you
could put any other device tree properties that you want.

Make the mdio_device driver for "qcom,qca807x" use shared code if you
want - but keep the device tree structure hardware-specific. Look at the
compatible strings that e.g. the drivers/mfd/simple-mfd-i2c.c driver
probes on. You could always change the driver implementation for a
certain compatible string, but you'll be stuck with the ultra-generic
compatible = "ethernet-phy-package", which has the problems that you
mention.

> 
> Concern list:
> 1. ethernet-phy-package MUST be placed in mdio node (not in ethernet,
>    the example was wrong anyway) and MUST have an addr
> 
>    Current example doesn't have an addr. I would prefer this way but
>    no problem in changing this.
> 
>    Solution:
>      - Add reg to the ethernet-phy-package node with the base address of
>        the PHY package (base address = the first PHY address of the
>        package)

Correct, what I'm saying is compatible with this.

> 
>        We will have a PHY node with the same address of the PHY package
>        node. Each PHY node in the PHY package node will have reg set to
>        the REAL address in the mdio bus.

If the real PHY IPs are children of the package rather than on the same
level with it, I don't think this will be a problem. I wonder if some
address translation could be done with the "ranges" device tree property.
I've never seen this with MDIO though.

> 4. Not finding a correct place to put PHY package info.
> 
>    I'm still convinced the mdio node is the correct place.
>    - PHY package are PHY in bundle so they are actual PHY
>    - We already have in the mdio node special handling (every DSA switch
>      use custom compatible and PHY ID is not used to probe them
>      normally)
>    - Node this way won't be treated as PHY as they won't match the PHY
>      node name pattern and also won't have the compatible pattern for
>      PHY.
> 
>    Solution:
>     - ethernet-phy-package node is OK given a reg is defined.

I agree that it should sit under the MDIO node. I disagree with the idea
of a standardized binding for PHY packages.