[PATCH v2 1/2] dt: Document a compatible entry for MDIO ethernet Phys

[PATCH v2 1/2] dt: Document a compatible entry for MDIO ethernet Phys

[Jason Gunthorpe]

This describes a compatible entry of the form:
  ethernet-phy-idAAAA,BBBB
Which is modelled after the PCI structured compatible entry
(pciVVVV,DDDD.SSSS.ssss.RR)

If present the OF core will be able to use this information to
directly create the correct phy without auto probing the bus.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/net/phy.txt | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
Cc: devicetree@vger.kernel.org

diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
index 7cd18fb..989122c 100644
--- a/Documentation/devicetree/bindings/net/phy.txt
+++ b/Documentation/devicetree/bindings/net/phy.txt
@@ -23,10 +23,18 @@ Optional Properties:
   assume clause 22. The compatible list may also contain other
   elements.
 
+  If the phy's identifier is known then the list may contain an entry
+  of the form: "ethernet-phy-idAAAA.BBBB" where
+     AAAA - The value of the 16 bit Phy Identifier 1 register as
+            4 hex digits. This is the chip vendor OUI bits 3:18
+     BBBB - The value of the 16 bit Phy Identifier 2 register as
+            4 hex digits. This is the chip vendor OUI bits 19:24,
+            followed by 10 bits of a vendor specific ID.
+
 Example:
 
 ethernet-phy@0 {
-	compatible = "ethernet-phy-ieee802.3-c22";
+	compatible = "ethernet-phy-id0141.0e90", "ethernet-phy-ieee802.3-c22";
 	linux,phandle = <2452000>;
 	interrupt-parent = <40000>;
 	interrupts = <35 1>;
-- 
1.8.1.2

[PATCH 2/2] of_mdio: Allow the DT to specify the phy ID and avoid autoprobing

[Jason Gunthorpe]

This makes the generic of_mdiobus_register parse the DT compatible string for
the pattern ethernet-phy-idAAAA.BBBB. If present it should be a value that
matches the phy-id register normally readable through MDIO.

When the ID is given the phy autoprobing is defeated and the phy is
created directly.

This is necessary to support phy's that cannot be autoprobed when
of_mdiobus_register is called. Specifically, my case has the phy in reset at
of_mdiobus_register, the reset is only released once the ethernet driver
starts, before it attaches to the phy.

Tested on ARM Kirkwood with phy id 0x01410e90 (Marvell 88E1318)

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 drivers/of/of_mdio.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index d5a57a9..c93287e 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -22,6 +22,30 @@
 MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
 MODULE_LICENSE("GPL");
 
+/* Extract the clause 22 phy ID from the compatible string of the form
+ * ethernet-phy-idAAAA.BBBB */
+static int of_get_phy_id(struct device_node *device, u32 *phy_id)
+{
+	const char *cp;
+	int cplen, l;
+	unsigned int upper, lower;
+
+	cp = of_get_property(device, "compatible", &cplen);
+	if (cp == NULL)
+		return -EINVAL;
+	while (cplen > 0) {
+		if (sscanf(cp, "ethernet-phy-id%4x.%4x", &upper, &lower) == 2) {
+			*phy_id = ((upper & 0xFFFF) << 16) | (lower & 0xFFFF);
+			return 0;
+		}
+
+		l = strlen(cp) + 1;
+		cp += l;
+		cplen -= l;
+	}
+	return -EINVAL;
+}
+
 /**
  * of_mdiobus_register - Register mii_bus and create PHYs from the device tree
  * @mdio: pointer to mii_bus structure
@@ -36,6 +60,7 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 	struct device_node *child;
 	const __be32 *paddr;
 	u32 addr;
+	u32 phy_id;
 	bool is_c45, scanphys = false;
 	int rc, i, len;
 
@@ -81,7 +106,10 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 
 		is_c45 = of_device_is_compatible(child,
 						 "ethernet-phy-ieee802.3-c45");
-		phy = get_phy_device(mdio, addr, is_c45);
+		if (!is_c45 && !of_get_phy_id(child, &phy_id))
+			phy = phy_device_create(mdio, addr, phy_id, 0, NULL);
+		else
+			phy = get_phy_device(mdio, addr, is_c45);
 
 		if (!phy || IS_ERR(phy)) {
 			dev_err(&mdio->dev,
-- 
1.8.1.2

Re: [PATCH 2/2] of_mdio: Allow the DT to specify the phy ID and avoid autoprobing

[Florian Fainelli]

Hi,

2014-01-31 Jason Gunthorpe <jgunthorpe@obsidianresearch.com>:
> This makes the generic of_mdiobus_register parse the DT compatible string for
> the pattern ethernet-phy-idAAAA.BBBB. If present it should be a value that
> matches the phy-id register normally readable through MDIO.
>
> When the ID is given the phy autoprobing is defeated and the phy is
> created directly.
>
> This is necessary to support phy's that cannot be autoprobed when
> of_mdiobus_register is called. Specifically, my case has the phy in reset at
> of_mdiobus_register, the reset is only released once the ethernet driver
> starts, before it attaches to the phy.

Who is responsible for bringing the PHY out of reset, is it the
Ethernet/MDIO bus driver? Is the PHY put into reset using, e.g a GPIO
line or any sort of reset controller, if so, should not we have some
sort of reset handle node and handle that in a generic manner?

Is your DTS or DTB hardcoding the PHY id, or are you having your
bootloader detect the exact PHY for you, then putting back the PHY
into reset to save power, until someone uses that PHY again?

>
> Tested on ARM Kirkwood with phy id 0x01410e90 (Marvell 88E1318)
>
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>  drivers/of/of_mdio.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index d5a57a9..c93287e 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -22,6 +22,30 @@
>  MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
>  MODULE_LICENSE("GPL");
>
> +/* Extract the clause 22 phy ID from the compatible string of the form
> + * ethernet-phy-idAAAA.BBBB */
> +static int of_get_phy_id(struct device_node *device, u32 *phy_id)
> +{
> +       const char *cp;
> +       int cplen, l;
> +       unsigned int upper, lower;
> +
> +       cp = of_get_property(device, "compatible", &cplen);
> +       if (cp == NULL)
> +               return -EINVAL;
> +       while (cplen > 0) {
> +               if (sscanf(cp, "ethernet-phy-id%4x.%4x", &upper, &lower) == 2) {

You might want to guard against 0x0 and 0xffff just in case whoever
fills this information in the Device Tree was reading bogus data out
of the MDIO bus, otherwise, chances are that the "Generic PHY" driver
will be picked up, and it might still not be appropriate for driving
your PHY chip.

> +                       *phy_id = ((upper & 0xFFFF) << 16) | (lower & 0xFFFF);
> +                       return 0;
> +               }
> +
> +               l = strlen(cp) + 1;
> +               cp += l;
> +               cplen -= l;
> +       }
> +       return -EINVAL;
> +}
> +
>  /**
>   * of_mdiobus_register - Register mii_bus and create PHYs from the device tree
>   * @mdio: pointer to mii_bus structure
> @@ -36,6 +60,7 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
>         struct device_node *child;
>         const __be32 *paddr;
>         u32 addr;
> +       u32 phy_id;
>         bool is_c45, scanphys = false;
>         int rc, i, len;
>
> @@ -81,7 +106,10 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
>
>                 is_c45 = of_device_is_compatible(child,
>                                                  "ethernet-phy-ieee802.3-c45");
> -               phy = get_phy_device(mdio, addr, is_c45);
> +               if (!is_c45 && !of_get_phy_id(child, &phy_id))
> +                       phy = phy_device_create(mdio, addr, phy_id, 0, NULL);
> +               else
> +                       phy = get_phy_device(mdio, addr, is_c45);
>
>                 if (!phy || IS_ERR(phy)) {
>                         dev_err(&mdio->dev,
> --
> 1.8.1.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
Florian

Re: [PATCH v2 1/2] dt: Document a compatible entry for MDIO ethernet Phys

[Florian Fainelli]

2014-01-31 Jason Gunthorpe <jgunthorpe@obsidianresearch.com>:
> This describes a compatible entry of the form:
>   ethernet-phy-idAAAA,BBBB
> Which is modelled after the PCI structured compatible entry
> (pciVVVV,DDDD.SSSS.ssss.RR)
>
> If present the OF core will be able to use this information to
> directly create the correct phy without auto probing the bus.
>
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Acked-by: Rob Herring <robh@kernel.org>

Looks good, thanks!

Acked-by: Florian Fainelli <f.fainelli@gmail.com>

> ---
>  Documentation/devicetree/bindings/net/phy.txt | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> Cc: devicetree@vger.kernel.org
>
> diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
> index 7cd18fb..989122c 100644
> --- a/Documentation/devicetree/bindings/net/phy.txt
> +++ b/Documentation/devicetree/bindings/net/phy.txt
> @@ -23,10 +23,18 @@ Optional Properties:
>    assume clause 22. The compatible list may also contain other
>    elements.
>
> +  If the phy's identifier is known then the list may contain an entry
> +  of the form: "ethernet-phy-idAAAA.BBBB" where
> +     AAAA - The value of the 16 bit Phy Identifier 1 register as
> +            4 hex digits. This is the chip vendor OUI bits 3:18
> +     BBBB - The value of the 16 bit Phy Identifier 2 register as
> +            4 hex digits. This is the chip vendor OUI bits 19:24,
> +            followed by 10 bits of a vendor specific ID.
> +
>  Example:
>
>  ethernet-phy@0 {
> -       compatible = "ethernet-phy-ieee802.3-c22";
> +       compatible = "ethernet-phy-id0141.0e90", "ethernet-phy-ieee802.3-c22";
>         linux,phandle = <2452000>;
>         interrupt-parent = <40000>;
>         interrupts = <35 1>;
> --
> 1.8.1.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Florian

Re: [PATCH 2/2] of_mdio: Allow the DT to specify the phy ID and avoid autoprobing

[Jason Gunthorpe]

On Fri, Jan 31, 2014 at 02:24:52PM -0800, Florian Fainelli wrote:

> > This is necessary to support phy's that cannot be autoprobed when
> > of_mdiobus_register is called. Specifically, my case has the phy in reset at
> > of_mdiobus_register, the reset is only released once the ethernet driver
> > starts, before it attaches to the phy.
> 
> Who is responsible for bringing the PHY out of reset, is it the
> Ethernet/MDIO bus driver? Is the PHY put into reset using, e.g a GPIO
> line or any sort of reset controller, if so, should not we have some
> sort of reset handle node and handle that in a generic manner?

The Phy Reset is connected to a GPIO, and the ethernet driver has code
to switch the GPIO out of reset. The phy is kept in reset until the
ethernet device is opened, and Linux is booted with the phy reset
asserted.

> Is your DTS or DTB hardcoding the PHY id, or are you having your
> bootloader detect the exact PHY for you, then putting back the PHY
> into reset to save power, until someone uses that PHY again?

For our uses the Phy ID is hardcoded. There is only a single part that
will fit on the board. So the bootloader doesn't touch the phy. If
there were alternate parts we'd get the part kind from the EEPROM that
stores the MAC address/etc.

> > +       while (cplen > 0) {
> > +               if (sscanf(cp, "ethernet-phy-id%4x.%4x", &upper, &lower) == 2) {
> 
> You might want to guard against 0x0 and 0xffff just in case whoever
> fills this information in the Device Tree was reading bogus data out
> of the MDIO bus, otherwise, chances are that the "Generic PHY" driver
> will be picked up, and it might still not be appropriate for driving
> your PHY chip.

Having the bootloader read the phy ID just to fill in this compatible
string isn't really the point. In every normal case I think it makes
sense to let Linux autoprobe the phy id. The use for this compatible
string is to defeat the autoprobe for situations where it is not
appropriate.

Thanks,
Jason

Re: [PATCH 2/2] of_mdio: Allow the DT to specify the phy ID and avoid autoprobing

[Florian Fainelli]

2014-01-31 Jason Gunthorpe <jgunthorpe@obsidianresearch.com>:
> On Fri, Jan 31, 2014 at 02:24:52PM -0800, Florian Fainelli wrote:
>
>> > This is necessary to support phy's that cannot be autoprobed when
>> > of_mdiobus_register is called. Specifically, my case has the phy in reset at
>> > of_mdiobus_register, the reset is only released once the ethernet driver
>> > starts, before it attaches to the phy.
>>
>> Who is responsible for bringing the PHY out of reset, is it the
>> Ethernet/MDIO bus driver? Is the PHY put into reset using, e.g a GPIO
>> line or any sort of reset controller, if so, should not we have some
>> sort of reset handle node and handle that in a generic manner?
>
> The Phy Reset is connected to a GPIO, and the ethernet driver has code
> to switch the GPIO out of reset. The phy is kept in reset until the
> ethernet device is opened, and Linux is booted with the phy reset
> asserted.
>
>> Is your DTS or DTB hardcoding the PHY id, or are you having your
>> bootloader detect the exact PHY for you, then putting back the PHY
>> into reset to save power, until someone uses that PHY again?
>
> For our uses the Phy ID is hardcoded. There is only a single part that
> will fit on the board. So the bootloader doesn't touch the phy. If
> there were alternate parts we'd get the part kind from the EEPROM that
> stores the MAC address/etc.
>
>> > +       while (cplen > 0) {
>> > +               if (sscanf(cp, "ethernet-phy-id%4x.%4x", &upper, &lower) == 2) {
>>
>> You might want to guard against 0x0 and 0xffff just in case whoever
>> fills this information in the Device Tree was reading bogus data out
>> of the MDIO bus, otherwise, chances are that the "Generic PHY" driver
>> will be picked up, and it might still not be appropriate for driving
>> your PHY chip.
>
> Having the bootloader read the phy ID just to fill in this compatible
> string isn't really the point. In every normal case I think it makes
> sense to let Linux autoprobe the phy id. The use for this compatible
> string is to defeat the autoprobe for situations where it is not
> appropriate.

This is well understood, I just think there a few different ways to
proceed with your use case here:

- you allow for a compatible string to defeat auto-probing like you just did
- you tell of_mdiobus_register() to look for a reset phandle and have
a reset controller release the PHY from reset before it tries to probe
for it, because doing that could avoid doing the PHY out of reset
sequence in a few dozen Ethernet drivers

The auto-probing bypass logic looks simple enough, and it does not
require burying the reset logic behind a reset controller, which uses
regmap and a few dozens layers down the road to toggle a bit somewhere
in a register, although this might be the desired way to move forward
if we want that to be generalized.

Anyway:

Acked-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH 2/2] of_mdio: Allow the DT to specify the phy ID and avoid autoprobing

[Jason Gunthorpe]

On Fri, Jan 31, 2014 at 03:28:10PM -0800, Florian Fainelli wrote:
> - you tell of_mdiobus_register() to look for a reset phandle and have
> a reset controller release the PHY from reset before it tries to probe
> for it, because doing that could avoid doing the PHY out of reset
> sequence in a few dozen Ethernet drivers

I think this would be interesting, but difficult. Ideally you'd want
to keep the phy in reset until an ethernet driver attaches to it, and
reset it again once it detaches, so Linux would have to defer probing
the phy driver until an attach. Not sure what this would do for power
consumption, but reset might be lower power on phy's that don't have
an internal power down.

> The auto-probing bypass logic looks simple enough, and it does not
> require burying the reset logic behind a reset controller, which
> uses

Right, it closely matches what was happening in board files. FWIW,
this system was designed in 2005 for Linux 2.16 and this patch set fell
out of porting forward to 3.13 w/ DT.

Jason