diff mbox series

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

Message ID 20210814023132.2729731-3-saravanak@google.com
State New
Headers show
Series of: property: Support for few more properties | expand

Commit Message

Saravana Kannan Aug. 14, 2021, 2:31 a.m. UTC
Allows tracking dependencies between consumers of an Ethernet PHY and
the parent devices that own the PHY.

Cc: Andrew Lunn <andrew@lunn.ch>
Cc: netdev@vger.kernel.org
Signed-off-by: Saravana Kannan <saravanak@google.com>
---

Hi Andrew,

I spent a few hours looking at most/all uses of phy-handle and my
comment in the code seems valid. Can you confirm that please? 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"?

-Saravana

 drivers/of/property.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

Saravana Kannan Aug. 16, 2021, 8:43 p.m. UTC | #1
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.

>

> > +     /*

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

-Saravana
Rob Herring Aug. 16, 2021, 9:11 p.m. UTC | #2
On Mon, Aug 16, 2021 at 3:43 PM Saravana Kannan <saravanak@google.com> 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.


For net devices, you can have 2 PHYs. 'phys' is the serdes phy and
'phy-handle' is the ethernet (typically) phy. On some chips, a serdes
phy can do PCS (ethernet), SATA, PCIe.

'phy' is deprecated, so ignore it. The one case for displays I see in
display/exynos/exynos_hdmi.txt should be deprecated as well.

There's also 'usb-phy' which should be deprecated.

Rob
Andrew Lunn Aug. 16, 2021, 9:15 p.m. UTC | #3
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.

    Andrew
Saravana Kannan Aug. 17, 2021, 6:18 p.m. UTC | #4
On Mon, Aug 16, 2021 at 2:11 PM Rob Herring <robh+dt@kernel.org> wrote:
>

> On Mon, Aug 16, 2021 at 3:43 PM Saravana Kannan <saravanak@google.com> 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.

>

> For net devices, you can have 2 PHYs. 'phys' is the serdes phy and

> 'phy-handle' is the ethernet (typically) phy. On some chips, a serdes

> phy can do PCS (ethernet), SATA, PCIe.

>

> 'phy' is deprecated, so ignore it. The one case for displays I see in

> display/exynos/exynos_hdmi.txt should be deprecated as well.

>

> There's also 'usb-phy' which should be deprecated.


Thanks for the explanation Rob. I'll ignore phy and usb-phy unless it
becomes an issue for any future changes/improvements.

-Saravana


-Saravana
Saravana Kannan Aug. 18, 2021, 2:13 a.m. UTC | #5
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
diff mbox series

Patch

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 931340329414..70d9843fd4bf 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1350,6 +1350,20 @@  static struct device_node *parse_interrupts(struct device_node *np,
 	return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np;
 }
 
+static struct device_node *parse_phy_handle(struct device_node *np,
+					    const char *prop_name, int index)
+{
+	if (strcmp(prop_name, "phy-handle"))
+		return NULL;
+
+	/*
+	 * 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.
+	 */
+	return of_get_next_parent(of_parse_phandle(np, prop_name, index));
+}
+
 static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_clocks, },
 	{ .parse_prop = parse_interconnects, },
@@ -1379,6 +1393,7 @@  static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_resets, },
 	{ .parse_prop = parse_leds, },
 	{ .parse_prop = parse_backlight, },
+	{ .parse_prop = parse_phy_handle, },
 	{ .parse_prop = parse_gpio_compat, },
 	{ .parse_prop = parse_interrupts, },
 	{ .parse_prop = parse_regulators, },