Re: [PATCH v1 2/2] of: property: fw_devlink: Add support for "phy-handle" property

[Saravana Kannan <saravanak@google.com>]

On Mon, Aug 16, 2021 at 2:15 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Mon, Aug 16, 2021 at 01:43:19PM -0700, Saravana Kannan wrote:
> > On Sat, Aug 14, 2021 at 8:22 AM Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > Hi Saravana
> > >
> > > > Hi Andrew,
> > > >
> > >
> > > > Also there
> > > > are so many phy related properties that my head is spinning. Is there a
> > > > "phy" property (which is different from "phys") that treated exactly as
> > > > "phy-handle"?
> > >
> > > Sorry, i don't understand your question.
> >
> > Sorry. I was just saying I understand the "phy-handle" DT property
> > (seems specific to ethernet PHY) and "phys" DT property (seems to be
> > for generic PHYs -- used mostly by display and USB?). But I noticed
> > there's yet another "phy" DT property which I'm not sure I understand.
> > It seems to be used by display and ethernet and seems to be a
> > deprecated property. If you can explain that DT property in the
> > context of networking and how to interpret it as a human, that'd be
> > nice.
>
> Ah, i think i understand:
>
> Documentation/devicetree/bindings/net/ethernet-controller.yaml
>
>   phy:
>     $ref: "#/properties/phy-handle"
>     deprecated: true
>
> So it is used the same as phy-handle. I doubt there are many examples
> of it, it has been deprecated a long time. Maybe look in the powerpc
> dts files?
>
> > > > +     /*
> > > > +      * Device tree nodes pointed to by phy-handle never have struct devices
> > > > +      * created for them even if they have a "compatible" property. So
> > > > +      * return the parent node pointer.
> > > > +      */
> > >
> > > We have a classic bus with devices on it. The bus master is registers
> > > with linux using one of the mdiobus_register() calls. That then
> > > enumerates the bus, looking at the 32 possible address on the bus,
> > > using mdiobus_scan. It then gets a little complex, due to
> > > history.
> > >
> > > Originally, the only thing you could have on an MDIO bus was a
> > > PHY. But devices on MDIO busses are more generic, and Linux gained
> > > support for Ethernet switches on an MDIO bus, and there are a few
> > > other sort device. So to keep the PHY API untouched, but to add these
> > > extra devices, we added the generic struct mdio_device which
> > > represents any sort of device on an MDIO bus. This has a struct device
> > > embedded in it.
> > >
> > > When we scan the bus and find a PHY, a struct phy_device is created,
> > > which has an embedded struct mdio_device. The struct device in that is
> > > then registered with the driver core.
> > >
> > > So a phy-handle does point to a device, but you need to do an object
> > > orientated style look at the base class to find it.
> >
> > Thanks for the detailed explanation. I didn't notice a phy_device had
> > an mdio_device inside it. Makes sense. I think my comment is not
> > worded accurately and it really should be:
> >
> > Device tree nodes pointed to by phy-handle (even if they have a
> > "compatible" property) will never have struct devices probed and bound
> > to a driver through the driver framework. It's the parent node/device
> > that gets bound to a driver and initializes the PHY. So return the
> > parent node pointer instead.
> >
> > Does this sound right? As opposed to PHYs the other generic mdio
> > devices seem to actually have drivers that'll bind to them through the
> > driver framework.
>
> That sounds wrong. The MDIO bus master is a linux device and has a
> driver. Same as an I2C bus master, or an SPI bus master, or a USB
> host. All these busses have devices on them, same as an MDIO bus. The
> devices on the bus are found and registered with the driver
> framework. The driver framework, with some help from the mdio bus
> class, with then find the correct driver of the device, and probe
> it. During probe, it gets initialized by the PHY driver.
>
> So for me, the parent of a PHY would be the MDIO bus master, and the
> bus master is not driving the PHY, in the same way an I2C bus master
> does not drive the tmp100 temperature sensor on an i2c bus.
>
> But maybe i don't understand your terminology here?
>
> Maybe this will help:
>
> root@370rd:/sys/class/mdio_bus# ls -l
> total 0
> lrwxrwxrwx 1 root root 0 Jan  2  2021 '!soc!internal-regs!mdio@72004!switch@10!mdio' -> '../../devices/platform/soc/soc:internal-regs/f1072004.mdio/mdio_bus/f1072004.mdio-mii/f1072004.mdio-mii:10/mdio_bus/!soc!internal-regs!mdio@72004!switch@10!mdio'
> lrwxrwxrwx 1 root root 0 Jan  2  2021  f1072004.mdio-mii -> ../../devices/platform/soc/soc:internal-regs/f1072004.mdio/mdio_bus/f1072004.mdio-mii
> lrwxrwxrwx 1 root root 0 Jan  2  2021  fixed-0 -> '../../devices/platform/Fixed MDIO bus.0/mdio_bus/fixed-0'
>
> So there are three MDIO bus masters.
>
> Going into f1072004.mdio-mii, we see there are two PHYs on this bus:
>
> root@370rd:/sys/class/mdio_bus/f1072004.mdio-mii# ls -l
> total 0
> lrwxrwxrwx 1 root root    0 Aug 16 21:03 device -> ../../../f1072004.mdio
> drwxr-xr-x 5 root root    0 Jan  2  2021 f1072004.mdio-mii:00
> drwxr-xr-x 6 root root    0 Jan  2  2021 f1072004.mdio-mii:10
> lrwxrwxrwx 1 root root    0 Aug 16 21:03 of_node -> ../../../../../../../firmware/devicetree/base/soc/internal-regs/mdio@72004
> drwxr-xr-x 2 root root    0 Aug 16 21:03 power
> drwxr-xr-x 2 root root    0 Aug 16 21:03 statistics
> lrwxrwxrwx 1 root root    0 Jan  2  2021 subsystem -> ../../../../../../../class/mdio_bus
> -rw-r--r-- 1 root root 4096 Jan  2  2021 uevent
> -r--r--r-- 1 root root 4096 Aug 16 21:03 waiting_for_supplier
>
> and going into one of the PHYs f1072004.mdio-mii:00
>
> lrwxrwxrwx 1 root root    0 Aug 16 20:54 attached_dev -> ../../../../f1070000.ethernet/net/eth0
> lrwxrwxrwx 1 root root    0 Aug 16 20:54 driver -> '../../../../../../../../bus/mdio_bus/drivers/Marvell 88E1510'
> drwxr-xr-x 3 root root    0 Jan  2  2021 hwmon
> lrwxrwxrwx 1 root root    0 Aug 16 20:54 of_node -> ../../../../../../../../firmware/devicetree/base/soc/internal-regs/mdio@72004/ethernet-phy@0
> -r--r--r-- 1 root root 4096 Aug 16 20:54 phy_dev_flags
> -r--r--r-- 1 root root 4096 Aug 16 20:54 phy_has_fixups
> -r--r--r-- 1 root root 4096 Aug 16 20:54 phy_id
> -r--r--r-- 1 root root 4096 Aug 16 20:54 phy_interface
> drwxr-xr-x 2 root root    0 Aug 16 20:54 power
> drwxr-xr-x 2 root root    0 Aug 16 20:54 statistics
> lrwxrwxrwx 1 root root    0 Jan  2  2021 subsystem -> ../../../../../../../../bus/mdio_bus
> -rw-r--r-- 1 root root 4096 Jan  2  2021 uevent
>
> The phy-handle in the MAC node points to ethernet-phy@0.

Thanks! Looks like I got confused a bit based on some misconceptions I
got when working on unrelated patches. I read through the code and now
understand how this works. I'll fix up this patch and send out a v2
(it actually makes the patch simpler).

On an unrelated note, I'm still a bit confused on what's going on in
get_phy_device() when one PHY C45 device has multiple IDs found by
get_phy_c45_ids(). Based on the comments, it looks like each of those
IDs might be separate devices? Or am I misunderstanding the comment?
If they are separate devices, how is one phy_device representing them
all?

-Saravana