Re: [PATCH RFT 1/2] phylib: add device reset GPIO support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hello.

On 05/13/2016 12:35 AM, Sergei Shtylyov wrote:

[we already talked about this patch in #armlinux, I'm now just
forwarding my comments on the list. Background was that I sent an easier
and less complete patch with the same idea. See
http://patchwork.ozlabs.org/patch/621418/]

[added Linus Walleij to Cc, there is a question for you/him below]

On Fri, Apr 29, 2016 at 01:12:54AM +0300, Sergei Shtylyov wrote:
--- net-next.orig/Documentation/devicetree/bindings/net/phy.txt
+++ net-next/Documentation/devicetree/bindings/net/phy.txt
@@ -35,6 +35,8 @@ Optional Properties:
 - broken-turn-around: If set, indicates the PHY device does not correctly
   release the turn around line low at the end of a MDIO transaction.

+- reset-gpios: The GPIO phandle and specifier for the PHY reset signal.
+
 Example:

 ethernet-phy@0 {

This is great.

Index: net-next/drivers/net/phy/at803x.c
===================================================================
--- net-next.orig/drivers/net/phy/at803x.c
+++ net-next/drivers/net/phy/at803x.c
@@ -65,7 +65,6 @@ MODULE_LICENSE("GPL");
[...]

My patch breaks this driver. I wasn't aware of it.

   I tried to be as careful as I could but still it looks that I didn't
succeed at that too...

   Hm, I'm starting to forget the vital details about my patch...

[...]
Index: net-next/drivers/net/phy/mdio_device.c
===================================================================
--- net-next.orig/drivers/net/phy/mdio_device.c
+++ net-next/drivers/net/phy/mdio_device.c
[...]
@@ -117,9 +126,16 @@ static int mdio_probe(struct device *dev
     struct mdio_driver *mdiodrv = to_mdio_driver(drv);
     int err = 0;

-    if (mdiodrv->probe)
+    if (mdiodrv->probe) {
+        /* Deassert the reset signal */
+        mdio_device_reset(mdiodev, 0);
+
         err = mdiodrv->probe(mdiodev);

+        /* Assert the reset signal */
+        mdio_device_reset(mdiodev, 1);

I wonder if it's safe to do this in general. What if ->probe does
something with the phy that is lost by resetting but that is relied on
later?

   Well, I thought that config_init() method is designed for that but indeed
the LXT driver writes to BMCR in its probe() method and hence is broken.
> Thank you for noticing...

It's broken even without my patch. The phylib will cause a PHY soft reset when attaching to the PHY device, so all BMCR programming dpone in the probe() method will be lost. My patch does make sense as is. Looks like I should also look into fixing lxt.c. Florian, what do you think?

[...]

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux