Quantcast

[PATCH RFT 0/2] Teach phylib hard-resetting devices

classic Classic list List threaded Threaded
49 messages Options
123
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] ARM: dts: at91: VInCo: fix phy reset gpio flag

Sergei Shtylyov-4
Hello.

On 04/26/2016 01:24 PM, Nicolas Ferre wrote:

> Fix gpio active flag for the phy reset-gpios property. The line is
> active low instead of active high.
> Actually, this flags was never used by the macb driver.
>
> Reported-by: Sergei Shtylyov <[hidden email]>
> Cc: Andrew Lunn <[hidden email]>
> Cc: David Miller <[hidden email]>
> Signed-off-by: Nicolas Ferre <[hidden email]>
> ---
> Hi,
>
> Thanks for having reported this bug to me.

    And thank you for beating me to it and doing the patch. I'm still busy
with other stuff. :-(

> I hope that we will be able to move forward for changing the phy
> reset code in the macb driver.

    I meant to delete it entirely.

> I plan to queue this patch through arm-soc for 4.7.

    Hm, that way we get that board broken if my phylib/macb patches get merged
before your patch. Perhaps it would be better to merge this patch thru DaveM's
tree (before my patches) to keep the kernel bisectable?

> Thanks, best regards,
>    Nicolas

MBR, Sergei

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] ARM: dts: at91: VInCo: fix phy reset gpio flag

Sergei Shtylyov-4
In reply to this post by David Miller-13
On 04/26/2016 08:17 PM, David Miller wrote:

>> I plan to queue this patch through arm-soc for 4.7.
>
> Ok.

    How about this patch going thru your net-next repo instead?
I'd like to keep the kernel bisectable... if my phylib/macb patches get merged
earlier than this one, that board would be broken...

MBR, Sergei

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] ARM: dts: at91: VInCo: fix phy reset gpio flag

Nicolas Ferre-2
Le 26/04/2016 20:27, Sergei Shtylyov a écrit :
> On 04/26/2016 08:17 PM, David Miller wrote:
>
>>> I plan to queue this patch through arm-soc for 4.7.
>>
>> Ok.
>
>     How about this patch going thru your net-next repo instead?
> I'd like to keep the kernel bisectable... if my phylib/macb patches get merged
> earlier than this one, that board would be broken...

Sergei, David,

I don't think that there is a big risk for this board to be tested in
the meantime as it's not widely deployed yet.
And as I'm aware of the issue and basically maintaining this DT file, I
think that I'll be informed if people try an unlikely arrangement of
patches on this board.

So either way, I'm okay. But I do think it's not worth thinking too much
about this case.

Bye,
--
Nicolas Ferre
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sergei Shtylyov-4
In reply to this post by Sergei Shtylyov-4
The PHY devices sometimes do have their reset signal (maybe even power
supply?) tied to some GPIO and sometimes it also does happen that a boot
loader does not leave it deasserted. So far this issue has been attacked
from (as I believe) a wrong angle: by teaching the MAC driver to manipulate
the GPIO in question; that solution, when applied to the device trees, led
to adding the PHY reset GPIO properties to the MAC device node, with one
exception: Cadence MACB driver which could handle the "reset-gpios" prop
in a PHY device subnode. I believe that the correct approach is to teach
the 'phylib' to get the MDIO device reset GPIO from the device tree node
corresponding to this device -- which this patch is doing...

Note that I had to modify the  AT803x PHY driver as it would stop working
otherwise as it made use of the reset GPIO for its own purposes...

Signed-off-by: Sergei Shtylyov <[hidden email]>

---
Changes in version 2:
- reformatted the changelog;
- resolved rejects, refreshed the patch.

Documentation/devicetree/bindings/net/phy.txt |    2 +
 Documentation/devicetree/bindings/net/phy.txt |    2 +
 drivers/net/phy/at803x.c                      |   19 ++------------
 drivers/net/phy/mdio_bus.c                    |    4 +++
 drivers/net/phy/mdio_device.c                 |   27 +++++++++++++++++++--
 drivers/net/phy/phy_device.c                  |   33 ++++++++++++++++++++++++--
 drivers/of/of_mdio.c                          |   16 ++++++++++++
 include/linux/mdio.h                          |    3 ++
 include/linux/phy.h                           |    5 +++
 8 files changed, 89 insertions(+), 20 deletions(-)

Index: net-next/Documentation/devicetree/bindings/net/phy.txt
===================================================================
--- 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 {
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");
 
 struct at803x_priv {
  bool phy_reset:1;
- struct gpio_desc *gpiod_reset;
 };
 
 struct at803x_context {
@@ -271,22 +270,10 @@ static int at803x_probe(struct phy_devic
 {
  struct device *dev = &phydev->mdio.dev;
  struct at803x_priv *priv;
- struct gpio_desc *gpiod_reset;
 
  priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
  if (!priv)
  return -ENOMEM;
-
- if (phydev->drv->phy_id != ATH8030_PHY_ID)
- goto does_not_require_reset_workaround;
-
- gpiod_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
- if (IS_ERR(gpiod_reset))
- return PTR_ERR(gpiod_reset);
-
- priv->gpiod_reset = gpiod_reset;
-
-does_not_require_reset_workaround:
  phydev->priv = priv;
 
  return 0;
@@ -361,14 +348,14 @@ static void at803x_link_change_notify(st
  */
  if (phydev->drv->phy_id == ATH8030_PHY_ID) {
  if (phydev->state == PHY_NOLINK) {
- if (priv->gpiod_reset && !priv->phy_reset) {
+ if (phydev->mdio.reset && !priv->phy_reset) {
  struct at803x_context context;
 
  at803x_context_save(phydev, &context);
 
- gpiod_set_value(priv->gpiod_reset, 1);
+ phy_device_reset(phydev, 1);
  msleep(1);
- gpiod_set_value(priv->gpiod_reset, 0);
+ phy_device_reset(phydev, 0);
  msleep(1);
 
  at803x_context_restore(phydev, &context);
Index: net-next/drivers/net/phy/mdio_bus.c
===================================================================
--- net-next.orig/drivers/net/phy/mdio_bus.c
+++ net-next/drivers/net/phy/mdio_bus.c
@@ -35,6 +35,7 @@
 #include <linux/phy.h>
 #include <linux/io.h>
 #include <linux/uaccess.h>
+#include <linux/gpio/consumer.h>
 
 #include <asm/irq.h>
 
@@ -371,6 +372,9 @@ void mdiobus_unregister(struct mii_bus *
  if (!mdiodev)
  continue;
 
+ if (mdiodev->reset)
+ gpiod_put(mdiodev->reset);
+
  mdiodev->device_remove(mdiodev);
  mdiodev->device_free(mdiodev);
  }
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
@@ -12,6 +12,8 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/errno.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
@@ -103,6 +105,13 @@ void mdio_device_remove(struct mdio_devi
 }
 EXPORT_SYMBOL(mdio_device_remove);
 
+void mdio_device_reset(struct mdio_device *mdiodev, int value)
+{
+ if (mdiodev->reset)
+ gpiod_set_value(mdiodev->reset, value);
+}
+EXPORT_SYMBOL(mdio_device_reset);
+
 /**
  * mdio_probe - probe an MDIO device
  * @dev: device to probe
@@ -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);
+ }
+
  return err;
 }
 
@@ -129,9 +145,16 @@ static int mdio_remove(struct device *de
  struct device_driver *drv = mdiodev->dev.driver;
  struct mdio_driver *mdiodrv = to_mdio_driver(drv);
 
- if (mdiodrv->remove)
+ if (mdiodrv->remove) {
+ /* Deassert the reset signal */
+ mdio_device_reset(mdiodev, 0);
+
  mdiodrv->remove(mdiodev);
 
+ /* Assert the reset signal */
+ mdio_device_reset(mdiodev, 1);
+ }
+
  return 0;
 }
 
Index: net-next/drivers/net/phy/phy_device.c
===================================================================
--- net-next.orig/drivers/net/phy/phy_device.c
+++ net-next/drivers/net/phy/phy_device.c
@@ -589,6 +589,9 @@ int phy_device_register(struct phy_devic
  if (err)
  return err;
 
+ /* Deassert the reset signal */
+ phy_device_reset(phydev, 0);
+
  /* Run all of the fixups for this PHY */
  err = phy_scan_fixups(phydev);
  if (err) {
@@ -604,9 +607,15 @@ int phy_device_register(struct phy_devic
  goto out;
  }
 
+ /* Assert the reset signal */
+ phy_device_reset(phydev, 1);
+
  return 0;
 
  out:
+ /* Assert the reset signal */
+ phy_device_reset(phydev, 1);
+
  mdiobus_unregister_device(&phydev->mdio);
  return err;
 }
@@ -792,6 +801,9 @@ int phy_init_hw(struct phy_device *phyde
 {
  int ret = 0;
 
+ /* Deassert the reset signal */
+ phy_device_reset(phydev, 0);
+
  if (!phydev->drv || !phydev->drv->config_init)
  return 0;
 
@@ -997,6 +1009,9 @@ void phy_detach(struct phy_device *phyde
 
  put_device(&phydev->mdio.dev);
  module_put(bus->owner);
+
+ /* Assert the reset signal */
+ phy_device_reset(phydev, 1);
 }
 EXPORT_SYMBOL(phy_detach);
 
@@ -1596,9 +1611,16 @@ static int phy_probe(struct device *dev)
  /* Set the state to READY by default */
  phydev->state = PHY_READY;
 
- if (phydev->drv->probe)
+ if (phydev->drv->probe) {
+ /* Deassert the reset signal */
+ phy_device_reset(phydev, 0);
+
  err = phydev->drv->probe(phydev);
 
+ /* Assert the reset signal */
+ phy_device_reset(phydev, 1);
+ }
+
  mutex_unlock(&phydev->lock);
 
  return err;
@@ -1612,8 +1634,15 @@ static int phy_remove(struct device *dev
  phydev->state = PHY_DOWN;
  mutex_unlock(&phydev->lock);
 
- if (phydev->drv->remove)
+ if (phydev->drv->remove) {
+ /* Deassert the reset signal */
+ phy_device_reset(phydev, 0);
+
  phydev->drv->remove(phydev);
+
+ /* Assert the reset signal */
+ phy_device_reset(phydev, 1);
+ }
  phydev->drv = NULL;
 
  return 0;
Index: net-next/drivers/of/of_mdio.c
===================================================================
--- net-next.orig/drivers/of/of_mdio.c
+++ net-next/drivers/of/of_mdio.c
@@ -44,6 +44,7 @@ static int of_get_phy_id(struct device_n
 static void of_mdiobus_register_phy(struct mii_bus *mdio,
     struct device_node *child, u32 addr)
 {
+ struct gpio_desc *gpiod;
  struct phy_device *phy;
  bool is_c45;
  int rc;
@@ -52,10 +53,17 @@ static void of_mdiobus_register_phy(stru
  is_c45 = of_device_is_compatible(child,
  "ethernet-phy-ieee802.3-c45");
 
+ gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios");
+ /* Deassert the reset signal */
+ if (!IS_ERR(gpiod))
+ gpiod_direction_output(gpiod, 0);
  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);
+ /* Assert the reset signal again */
+ if (!IS_ERR(gpiod))
+ gpiod_set_value(gpiod, 1);
  if (IS_ERR(phy))
  return;
 
@@ -75,6 +83,9 @@ static void of_mdiobus_register_phy(stru
  of_node_get(child);
  phy->mdio.dev.of_node = child;
 
+ if (!IS_ERR(gpiod))
+ phy->mdio.reset = gpiod;
+
  /* All data is now stored in the phy struct;
  * register it */
  rc = phy_device_register(phy);
@@ -92,6 +103,7 @@ static void of_mdiobus_register_device(s
        struct device_node *child, u32 addr)
 {
  struct mdio_device *mdiodev;
+ struct gpio_desc *gpiod;
  int rc;
 
  mdiodev = mdio_device_create(mdio, addr);
@@ -104,6 +116,10 @@ static void of_mdiobus_register_device(s
  of_node_get(child);
  mdiodev->dev.of_node = child;
 
+ gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios");
+ if (!IS_ERR(gpiod))
+ mdiodev->reset = gpiod;
+
  /* All data is now stored in the mdiodev struct; register it. */
  rc = mdio_device_register(mdiodev);
  if (rc) {
Index: net-next/include/linux/mdio.h
===================================================================
--- net-next.orig/include/linux/mdio.h
+++ net-next/include/linux/mdio.h
@@ -11,6 +11,7 @@
 
 #include <uapi/linux/mdio.h>
 
+struct gpio_desc;
 struct mii_bus;
 
 /* Multiple levels of nesting are possible. However typically this is
@@ -37,6 +38,7 @@ struct mdio_device {
  /* Bus address of the MDIO device (0-31) */
  int addr;
  int flags;
+ struct gpio_desc *reset;
 };
 #define to_mdio_device(d) container_of(d, struct mdio_device, dev)
 
@@ -69,6 +71,7 @@ void mdio_device_free(struct mdio_device
 struct mdio_device *mdio_device_create(struct mii_bus *bus, int addr);
 int mdio_device_register(struct mdio_device *mdiodev);
 void mdio_device_remove(struct mdio_device *mdiodev);
+void mdio_device_reset(struct mdio_device *mdiodev, int value);
 int mdio_driver_register(struct mdio_driver *drv);
 void mdio_driver_unregister(struct mdio_driver *drv);
 
Index: net-next/include/linux/phy.h
===================================================================
--- net-next.orig/include/linux/phy.h
+++ net-next/include/linux/phy.h
@@ -769,6 +769,11 @@ static inline int phy_read_status(struct
  return phydev->drv->read_status(phydev);
 }
 
+static inline void phy_device_reset(struct phy_device *phydev, int value)
+{
+ mdio_device_reset(&phydev->mdio, value);
+}
+
 #define phydev_err(_phydev, format, args...) \
  dev_err(&_phydev->mdio.dev, format, ##args)
 

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Rob Herring-3
On Fri, Apr 29, 2016 at 01:12:54AM +0300, Sergei Shtylyov wrote:

> The PHY devices sometimes do have their reset signal (maybe even power
> supply?) tied to some GPIO and sometimes it also does happen that a boot
> loader does not leave it deasserted. So far this issue has been attacked
> from (as I believe) a wrong angle: by teaching the MAC driver to manipulate
> the GPIO in question; that solution, when applied to the device trees, led
> to adding the PHY reset GPIO properties to the MAC device node, with one
> exception: Cadence MACB driver which could handle the "reset-gpios" prop
> in a PHY device subnode. I believe that the correct approach is to teach
> the 'phylib' to get the MDIO device reset GPIO from the device tree node
> corresponding to this device -- which this patch is doing...
>
> Note that I had to modify the  AT803x PHY driver as it would stop working
> otherwise as it made use of the reset GPIO for its own purposes...
>
> Signed-off-by: Sergei Shtylyov <[hidden email]>
>
> ---
> Changes in version 2:
> - reformatted the changelog;
> - resolved rejects, refreshed the patch.
>
> Documentation/devicetree/bindings/net/phy.txt |    2 +
>  Documentation/devicetree/bindings/net/phy.txt |    2 +

Acked-by: Rob Herring <[hidden email]>

>  drivers/net/phy/at803x.c                      |   19 ++------------
>  drivers/net/phy/mdio_bus.c                    |    4 +++
>  drivers/net/phy/mdio_device.c                 |   27 +++++++++++++++++++--
>  drivers/net/phy/phy_device.c                  |   33 ++++++++++++++++++++++++--
>  drivers/of/of_mdio.c                          |   16 ++++++++++++
>  include/linux/mdio.h                          |    3 ++
>  include/linux/phy.h                           |    5 +++
>  8 files changed, 89 insertions(+), 20 deletions(-)
>
> Index: net-next/Documentation/devicetree/bindings/net/phy.txt
> ===================================================================
> --- 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 {
> 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");
>  
>  struct at803x_priv {
>   bool phy_reset:1;
> - struct gpio_desc *gpiod_reset;
>  };
>  
>  struct at803x_context {
> @@ -271,22 +270,10 @@ static int at803x_probe(struct phy_devic
>  {
>   struct device *dev = &phydev->mdio.dev;
>   struct at803x_priv *priv;
> - struct gpio_desc *gpiod_reset;
>  
>   priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>   if (!priv)
>   return -ENOMEM;
> -
> - if (phydev->drv->phy_id != ATH8030_PHY_ID)
> - goto does_not_require_reset_workaround;
> -
> - gpiod_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> - if (IS_ERR(gpiod_reset))
> - return PTR_ERR(gpiod_reset);
> -
> - priv->gpiod_reset = gpiod_reset;
> -
> -does_not_require_reset_workaround:
>   phydev->priv = priv;
>  
>   return 0;
> @@ -361,14 +348,14 @@ static void at803x_link_change_notify(st
>   */
>   if (phydev->drv->phy_id == ATH8030_PHY_ID) {
>   if (phydev->state == PHY_NOLINK) {
> - if (priv->gpiod_reset && !priv->phy_reset) {
> + if (phydev->mdio.reset && !priv->phy_reset) {
>   struct at803x_context context;
>  
>   at803x_context_save(phydev, &context);
>  
> - gpiod_set_value(priv->gpiod_reset, 1);
> + phy_device_reset(phydev, 1);
>   msleep(1);
> - gpiod_set_value(priv->gpiod_reset, 0);
> + phy_device_reset(phydev, 0);
>   msleep(1);
>  
>   at803x_context_restore(phydev, &context);
> Index: net-next/drivers/net/phy/mdio_bus.c
> ===================================================================
> --- net-next.orig/drivers/net/phy/mdio_bus.c
> +++ net-next/drivers/net/phy/mdio_bus.c
> @@ -35,6 +35,7 @@
>  #include <linux/phy.h>
>  #include <linux/io.h>
>  #include <linux/uaccess.h>
> +#include <linux/gpio/consumer.h>
>  
>  #include <asm/irq.h>
>  
> @@ -371,6 +372,9 @@ void mdiobus_unregister(struct mii_bus *
>   if (!mdiodev)
>   continue;
>  
> + if (mdiodev->reset)
> + gpiod_put(mdiodev->reset);
> +
>   mdiodev->device_remove(mdiodev);
>   mdiodev->device_free(mdiodev);
>   }
> 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
> @@ -12,6 +12,8 @@
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
>  #include <linux/errno.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/init.h>
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
> @@ -103,6 +105,13 @@ void mdio_device_remove(struct mdio_devi
>  }
>  EXPORT_SYMBOL(mdio_device_remove);
>  
> +void mdio_device_reset(struct mdio_device *mdiodev, int value)
> +{
> + if (mdiodev->reset)
> + gpiod_set_value(mdiodev->reset, value);
> +}
> +EXPORT_SYMBOL(mdio_device_reset);
> +
>  /**
>   * mdio_probe - probe an MDIO device
>   * @dev: device to probe
> @@ -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);
> + }
> +
>   return err;
>  }
>  
> @@ -129,9 +145,16 @@ static int mdio_remove(struct device *de
>   struct device_driver *drv = mdiodev->dev.driver;
>   struct mdio_driver *mdiodrv = to_mdio_driver(drv);
>  
> - if (mdiodrv->remove)
> + if (mdiodrv->remove) {
> + /* Deassert the reset signal */
> + mdio_device_reset(mdiodev, 0);
> +
>   mdiodrv->remove(mdiodev);
>  
> + /* Assert the reset signal */
> + mdio_device_reset(mdiodev, 1);
> + }
> +
>   return 0;
>  }
>  
> Index: net-next/drivers/net/phy/phy_device.c
> ===================================================================
> --- net-next.orig/drivers/net/phy/phy_device.c
> +++ net-next/drivers/net/phy/phy_device.c
> @@ -589,6 +589,9 @@ int phy_device_register(struct phy_devic
>   if (err)
>   return err;
>  
> + /* Deassert the reset signal */
> + phy_device_reset(phydev, 0);
> +
>   /* Run all of the fixups for this PHY */
>   err = phy_scan_fixups(phydev);
>   if (err) {
> @@ -604,9 +607,15 @@ int phy_device_register(struct phy_devic
>   goto out;
>   }
>  
> + /* Assert the reset signal */
> + phy_device_reset(phydev, 1);
> +
>   return 0;
>  
>   out:
> + /* Assert the reset signal */
> + phy_device_reset(phydev, 1);
> +
>   mdiobus_unregister_device(&phydev->mdio);
>   return err;
>  }
> @@ -792,6 +801,9 @@ int phy_init_hw(struct phy_device *phyde
>  {
>   int ret = 0;
>  
> + /* Deassert the reset signal */
> + phy_device_reset(phydev, 0);
> +
>   if (!phydev->drv || !phydev->drv->config_init)
>   return 0;
>  
> @@ -997,6 +1009,9 @@ void phy_detach(struct phy_device *phyde
>  
>   put_device(&phydev->mdio.dev);
>   module_put(bus->owner);
> +
> + /* Assert the reset signal */
> + phy_device_reset(phydev, 1);
>  }
>  EXPORT_SYMBOL(phy_detach);
>  
> @@ -1596,9 +1611,16 @@ static int phy_probe(struct device *dev)
>   /* Set the state to READY by default */
>   phydev->state = PHY_READY;
>  
> - if (phydev->drv->probe)
> + if (phydev->drv->probe) {
> + /* Deassert the reset signal */
> + phy_device_reset(phydev, 0);
> +
>   err = phydev->drv->probe(phydev);
>  
> + /* Assert the reset signal */
> + phy_device_reset(phydev, 1);
> + }
> +
>   mutex_unlock(&phydev->lock);
>  
>   return err;
> @@ -1612,8 +1634,15 @@ static int phy_remove(struct device *dev
>   phydev->state = PHY_DOWN;
>   mutex_unlock(&phydev->lock);
>  
> - if (phydev->drv->remove)
> + if (phydev->drv->remove) {
> + /* Deassert the reset signal */
> + phy_device_reset(phydev, 0);
> +
>   phydev->drv->remove(phydev);
> +
> + /* Assert the reset signal */
> + phy_device_reset(phydev, 1);
> + }
>   phydev->drv = NULL;
>  
>   return 0;
> Index: net-next/drivers/of/of_mdio.c
> ===================================================================
> --- net-next.orig/drivers/of/of_mdio.c
> +++ net-next/drivers/of/of_mdio.c
> @@ -44,6 +44,7 @@ static int of_get_phy_id(struct device_n
>  static void of_mdiobus_register_phy(struct mii_bus *mdio,
>      struct device_node *child, u32 addr)
>  {
> + struct gpio_desc *gpiod;
>   struct phy_device *phy;
>   bool is_c45;
>   int rc;
> @@ -52,10 +53,17 @@ static void of_mdiobus_register_phy(stru
>   is_c45 = of_device_is_compatible(child,
>   "ethernet-phy-ieee802.3-c45");
>  
> + gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios");
> + /* Deassert the reset signal */
> + if (!IS_ERR(gpiod))
> + gpiod_direction_output(gpiod, 0);
>   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);
> + /* Assert the reset signal again */
> + if (!IS_ERR(gpiod))
> + gpiod_set_value(gpiod, 1);
>   if (IS_ERR(phy))
>   return;
>  
> @@ -75,6 +83,9 @@ static void of_mdiobus_register_phy(stru
>   of_node_get(child);
>   phy->mdio.dev.of_node = child;
>  
> + if (!IS_ERR(gpiod))
> + phy->mdio.reset = gpiod;
> +
>   /* All data is now stored in the phy struct;
>   * register it */
>   rc = phy_device_register(phy);
> @@ -92,6 +103,7 @@ static void of_mdiobus_register_device(s
>         struct device_node *child, u32 addr)
>  {
>   struct mdio_device *mdiodev;
> + struct gpio_desc *gpiod;
>   int rc;
>  
>   mdiodev = mdio_device_create(mdio, addr);
> @@ -104,6 +116,10 @@ static void of_mdiobus_register_device(s
>   of_node_get(child);
>   mdiodev->dev.of_node = child;
>  
> + gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios");
> + if (!IS_ERR(gpiod))
> + mdiodev->reset = gpiod;
> +
>   /* All data is now stored in the mdiodev struct; register it. */
>   rc = mdio_device_register(mdiodev);
>   if (rc) {
> Index: net-next/include/linux/mdio.h
> ===================================================================
> --- net-next.orig/include/linux/mdio.h
> +++ net-next/include/linux/mdio.h
> @@ -11,6 +11,7 @@
>  
>  #include <uapi/linux/mdio.h>
>  
> +struct gpio_desc;
>  struct mii_bus;
>  
>  /* Multiple levels of nesting are possible. However typically this is
> @@ -37,6 +38,7 @@ struct mdio_device {
>   /* Bus address of the MDIO device (0-31) */
>   int addr;
>   int flags;
> + struct gpio_desc *reset;
>  };
>  #define to_mdio_device(d) container_of(d, struct mdio_device, dev)
>  
> @@ -69,6 +71,7 @@ void mdio_device_free(struct mdio_device
>  struct mdio_device *mdio_device_create(struct mii_bus *bus, int addr);
>  int mdio_device_register(struct mdio_device *mdiodev);
>  void mdio_device_remove(struct mdio_device *mdiodev);
> +void mdio_device_reset(struct mdio_device *mdiodev, int value);
>  int mdio_driver_register(struct mdio_driver *drv);
>  void mdio_driver_unregister(struct mdio_driver *drv);
>  
> Index: net-next/include/linux/phy.h
> ===================================================================
> --- net-next.orig/include/linux/phy.h
> +++ net-next/include/linux/phy.h
> @@ -769,6 +769,11 @@ static inline int phy_read_status(struct
>   return phydev->drv->read_status(phydev);
>  }
>  
> +static inline void phy_device_reset(struct phy_device *phydev, int value)
> +{
> + mdio_device_reset(&phydev->mdio, value);
> +}
> +
>  #define phydev_err(_phydev, format, args...) \
>   dev_err(&_phydev->mdio.dev, format, ##args)
>  
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Florian Fainelli-5
In reply to this post by Sergei Shtylyov-4
On 04/28/2016 03:12 PM, Sergei Shtylyov wrote:

> The PHY devices sometimes do have their reset signal (maybe even power
> supply?) tied to some GPIO and sometimes it also does happen that a boot
> loader does not leave it deasserted. So far this issue has been attacked
> from (as I believe) a wrong angle: by teaching the MAC driver to manipulate
> the GPIO in question; that solution, when applied to the device trees, led
> to adding the PHY reset GPIO properties to the MAC device node, with one
> exception: Cadence MACB driver which could handle the "reset-gpios" prop
> in a PHY device subnode. I believe that the correct approach is to teach
> the 'phylib' to get the MDIO device reset GPIO from the device tree node
> corresponding to this device -- which this patch is doing...
>
> Note that I had to modify the  AT803x PHY driver as it would stop working
> otherwise as it made use of the reset GPIO for its own purposes...
>
> Signed-off-by: Sergei Shtylyov <[hidden email]>

This looks good to me:

Acked-by: Florian Fainelli <[hidden email]>

Can you follow up with changes in phy_{suspend,resume} if that is also
an use case that you have?
--
Florian
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sergei Shtylyov-4
Hello.

On 05/10/2016 09:32 PM, Florian Fainelli wrote:

>> The PHY devices sometimes do have their reset signal (maybe even power
>> supply?) tied to some GPIO and sometimes it also does happen that a boot
>> loader does not leave it deasserted. So far this issue has been attacked
>> from (as I believe) a wrong angle: by teaching the MAC driver to manipulate
>> the GPIO in question; that solution, when applied to the device trees, led
>> to adding the PHY reset GPIO properties to the MAC device node, with one
>> exception: Cadence MACB driver which could handle the "reset-gpios" prop
>> in a PHY device subnode. I believe that the correct approach is to teach
>> the 'phylib' to get the MDIO device reset GPIO from the device tree node
>> corresponding to this device -- which this patch is doing...
>>
>> Note that I had to modify the  AT803x PHY driver as it would stop working
>> otherwise as it made use of the reset GPIO for its own purposes...
>>
>> Signed-off-by: Sergei Shtylyov <[hidden email]>
>
> This looks good to me:
>
> Acked-by: Florian Fainelli <[hidden email]>

    Thank you! I'll send v3 without [RFT] then.

> Can you follow up with changes in phy_{suspend,resume}

    I'm not sure what changes you mean -- powering down the PHYs?

> if that is also
> an use case that you have?

    No, I'm not into power management.

MBR, Sergei

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Florian Fainelli-5
On 05/10/2016 12:11 PM, Sergei Shtylyov wrote:

> Hello.
>
> On 05/10/2016 09:32 PM, Florian Fainelli wrote:
>
>>> The PHY devices sometimes do have their reset signal (maybe even power
>>> supply?) tied to some GPIO and sometimes it also does happen that a boot
>>> loader does not leave it deasserted. So far this issue has been attacked
>>> from (as I believe) a wrong angle: by teaching the MAC driver to
>>> manipulate
>>> the GPIO in question; that solution, when applied to the device
>>> trees, led
>>> to adding the PHY reset GPIO properties to the MAC device node, with one
>>> exception: Cadence MACB driver which could handle the "reset-gpios" prop
>>> in a PHY device subnode. I believe that the correct approach is to teach
>>> the 'phylib' to get the MDIO device reset GPIO from the device tree node
>>> corresponding to this device -- which this patch is doing...
>>>
>>> Note that I had to modify the  AT803x PHY driver as it would stop
>>> working
>>> otherwise as it made use of the reset GPIO for its own purposes...
>>>
>>> Signed-off-by: Sergei Shtylyov <[hidden email]>
>>
>> This looks good to me:
>>
>> Acked-by: Florian Fainelli <[hidden email]>
>
>    Thank you! I'll send v3 without [RFT] then.
>
>> Can you follow up with changes in phy_{suspend,resume}
>
>    I'm not sure what changes you mean -- powering down the PHYs?

Yes, powering down, conversely up the PHY. The whole point of putting
this in PHYLIB is to be able to perform things like that. We do not need
this right now, but it would be nice if we saw that materialize at some
point.
--
Florian
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Uwe Kleine-König-5
In reply to this post by Sergei Shtylyov-4
Hello Sergei,

[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.

> [...]
> 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
> [...]
> @@ -103,6 +105,13 @@ void mdio_device_remove(struct mdio_devi
>  }
>  EXPORT_SYMBOL(mdio_device_remove);
>  
> +void mdio_device_reset(struct mdio_device *mdiodev, int value)
> +{
> + if (mdiodev->reset)
> + gpiod_set_value(mdiodev->reset, value);

Before v4.6-rc1~108^2~91 it was not necessary to check for the first
parameter being non-NULL before calling gpiod_set_value. Linus, did you
change this on purpose?

> +}
> +EXPORT_SYMBOL(mdio_device_reset);
> +
>  /**
>   * mdio_probe - probe an MDIO device
>   * @dev: device to probe
> @@ -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?

> + }
> +
>   return err;
>  }
> [...]
> Index: net-next/drivers/of/of_mdio.c
> ===================================================================
> --- net-next.orig/drivers/of/of_mdio.c
> +++ net-next/drivers/of/of_mdio.c
> @@ -44,6 +44,7 @@ static int of_get_phy_id(struct device_n
>  static void of_mdiobus_register_phy(struct mii_bus *mdio,
>      struct device_node *child, u32 addr)
>  {
> + struct gpio_desc *gpiod;
>   struct phy_device *phy;
>   bool is_c45;
>   int rc;
> @@ -52,10 +53,17 @@ static void of_mdiobus_register_phy(stru
>   is_c45 = of_device_is_compatible(child,
>   "ethernet-phy-ieee802.3-c45");
>  
> + gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios");
> + /* Deassert the reset signal */
> + if (!IS_ERR(gpiod))
> + gpiod_direction_output(gpiod, 0);

This is wrong I think. You must only ignore -ENODEV, all other error
codes should be passed to the caller. (I see that's not trivial because
of_mdiobus_register_phy returns void.)

In my patch I used devm_gpiod_get_array which has the nice property that
I can already pass GPIOD_OUT_LOW in flags. Also this binds the lifetime
of the gpio to the device which is nice and IMHO the right direction for
the phylib (i.e. better embracing of the device model).

This cannot be used here easily however because there is no struct
device yet and this is only created after the phy id is determined. The
phy id is either read from the device tree or must be read from the phy
which might fail if reset is not deasserted.

Principally there is no reason however that the phy_id must be known
before the struct device is created however.

Best regards
Uwe

--
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sergei Shtylyov-4
Hello.

On 05/12/2016 09:42 PM, Uwe Kleine-König 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...

[...]
>> 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...

[...]

>> Index: net-next/drivers/of/of_mdio.c
>> ===================================================================
>> --- net-next.orig/drivers/of/of_mdio.c
>> +++ net-next/drivers/of/of_mdio.c
>> @@ -44,6 +44,7 @@ static int of_get_phy_id(struct device_n
>>  static void of_mdiobus_register_phy(struct mii_bus *mdio,
>>      struct device_node *child, u32 addr)
>>  {
>> + struct gpio_desc *gpiod;
>>   struct phy_device *phy;
>>   bool is_c45;
>>   int rc;
>> @@ -52,10 +53,17 @@ static void of_mdiobus_register_phy(stru
>>   is_c45 = of_device_is_compatible(child,
>>   "ethernet-phy-ieee802.3-c45");
>>
>> + gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios");
>> + /* Deassert the reset signal */
>> + if (!IS_ERR(gpiod))
>> + gpiod_direction_output(gpiod, 0);
>
> This is wrong I think. You must only ignore -ENODEV, all other error

    At least -ENOSYS should also be ignored (it's returned when gpiolib is not
configured), right? When does -ENODEV gets returned (it's not easy to follow)?

> codes should be passed to the caller.

    The caller doesn't care anyway...

> (I see that's not trivial because
> of_mdiobus_register_phy returns void.)

    I've made this function *void* in net-next.

> In my patch I used devm_gpiod_get_array which has the nice property that
> I can already pass GPIOD_OUT_LOW in flags. Also this binds the lifetime
> of the gpio to the device which is nice and IMHO the right direction for
> the phylib (i.e. better embracing of the device model).
>
> This cannot be used here easily however because there is no struct
> device yet and this is only created after the phy id is determined.

    Your last patch [1] didn't make use of the managed device API (devm)
either, I didn't quite get to the bottom of that...

> The
> phy id is either read from the device tree or must be read from the phy
> which might fail if reset is not deasserted.

> Principally there is no reason however that the phy_id must be known
> before the struct device is created however.

    It's just that the code is cleaner that way...

[1] http://paste.debian.net/683630/

> Best regards
> Uwe

MBR, Sergei

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Andrew Lunn
> >>+ gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios");
> >>+ /* Deassert the reset signal */
> >>+ if (!IS_ERR(gpiod))
> >>+ gpiod_direction_output(gpiod, 0);
> >
> >This is wrong I think. You must only ignore -ENODEV, all other error
>
>    At least -ENOSYS should also be ignored (it's returned when
> gpiolib is not configured), right? When does -ENODEV gets returned
> (it's not easy to follow)?
>
> >codes should be passed to the caller.
>
>    The caller doesn't care anyway...

It should do. What if fwnode_get_named_gpiod() returns -EPROBE_DEFER
because the GPIO driver has not been loaded yet?

        Andrew
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Uwe Kleine-König-5
In reply to this post by Sergei Shtylyov-4
Hello Sergei,

On Fri, May 13, 2016 at 12:35:50AM +0300, Sergei Shtylyov wrote:

> On 05/12/2016 09:42 PM, Uwe Kleine-König wrote:
> >>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...
>
> [...]
> >>Index: net-next/drivers/of/of_mdio.c
> >>===================================================================
> >>--- net-next.orig/drivers/of/of_mdio.c
> >>+++ net-next/drivers/of/of_mdio.c
> >>@@ -44,6 +44,7 @@ static int of_get_phy_id(struct device_n
> >> static void of_mdiobus_register_phy(struct mii_bus *mdio,
> >>    struct device_node *child, u32 addr)
> >> {
> >>+ struct gpio_desc *gpiod;
> >> struct phy_device *phy;
> >> bool is_c45;
> >> int rc;
> >>@@ -52,10 +53,17 @@ static void of_mdiobus_register_phy(stru
> >> is_c45 = of_device_is_compatible(child,
> >> "ethernet-phy-ieee802.3-c45");
> >>
> >>+ gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios");
> >>+ /* Deassert the reset signal */
> >>+ if (!IS_ERR(gpiod))
> >>+ gpiod_direction_output(gpiod, 0);
> >
> >This is wrong I think. You must only ignore -ENODEV, all other error
>
>    At least -ENOSYS should also be ignored (it's returned when gpiolib is
> not configured), right?

No, that's a common misconception. If GPIOLIB is off you cannot
determine if dt specified a reset gpio. So you have the choice between:

 a) ignore -ENOSYS and so don't handle the reset line in the cases where
    it's necessary probably yielding an "Error: phy not found" message.
 b) fail to probe even if a reset handling isn't necessary, yielding
    "Error: failed to get hold of reset gpio".

I say b) is the better one. It's easier to debug because the error
message is better, GPIOLIB is enabled in most cases anyhow (still maybe
select it?) and it's ensured that we're operating in the limits of the
hardware specs (maybe a phy returns a random value when a register is
read while reset is applied?).

> When does -ENODEV gets returned (it's not easy to follow)?

I don't know for sure for fwnode_get_named_gpiod, but the gpiod_get*()
family returns -ENODEV if the node doesn't have a reset-gpio property.

> >codes should be passed to the caller.
>
>    The caller doesn't care anyway...
>
> >(I see that's not trivial because
> >of_mdiobus_register_phy returns void.)
>
>    I've made this function *void* in net-next.

I'd say this is a step in the wrong direction. For example this makes it
impossible to handle the case where the phy should be probed, the gpio
driver isn't available yet, though. Normally you'd want to return
-EPROBE_DEFER in this case and retry probing later.

> >In my patch I used devm_gpiod_get_array which has the nice property that
> >I can already pass GPIOD_OUT_LOW in flags. Also this binds the lifetime
> >of the gpio to the device which is nice and IMHO the right direction for
> >the phylib (i.e. better embracing of the device model).
> >
> >This cannot be used here easily however because there is no struct
> >device yet and this is only created after the phy id is determined.
>
>    Your last patch [1] didn't make use of the managed device API (devm)
> either, I didn't quite get to the bottom of that...

Right, devm didn't work. My patch was a prototype for a way that allowed
to bind the lifetime of the gpio to the device. This might be longer
than the call to mdiobus_unregister. See the problems that i2c handles
because it doesn't handle lifetimes correctly in drivers/i2c/i2c-core.c
at the end of i2c_del_adapter.

> >The phy id is either read from the device tree or must be read from
> >the phy which might fail if reset is not deasserted.
>
> >Principally there is no reason however that the phy_id must be known
> >before the struct device is created however.
>
>    It's just that the code is cleaner that way...

I don't agree, I don't see that

        determine_phyid()
        allocate_device()

is better than

        allocate_device()
        determine_phyid()

. The former is maybe easier because that's the status quo and it
doesn't need patching. But IMHO the result is on a similar level of
cleanliness.

Best regards
Uwe

> [1] http://paste.debian.net/683630/

--
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Roger Quadros
In reply to this post by Uwe Kleine-König-5
Hi Sergei,

On 12/05/16 21:42, Uwe Kleine-König wrote:

> Hello Sergei,
>
> [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.
>
>> [...]
>> 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
>> [...]
>> @@ -103,6 +105,13 @@ void mdio_device_remove(struct mdio_devi
>>  }
>>  EXPORT_SYMBOL(mdio_device_remove);
>>  
>> +void mdio_device_reset(struct mdio_device *mdiodev, int value)
>> +{
>> + if (mdiodev->reset)
>> + gpiod_set_value(mdiodev->reset, value);
>
> Before v4.6-rc1~108^2~91 it was not necessary to check for the first
> parameter being non-NULL before calling gpiod_set_value. Linus, did you
> change this on purpose?
>
>> +}
>> +EXPORT_SYMBOL(mdio_device_reset);
>> +
>>  /**
>>   * mdio_probe - probe an MDIO device
>>   * @dev: device to probe
>> @@ -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?

mdio_probe is called for non PHY devices only, right?

I'm a bit lost as to why we're de-asserting reset at multiple places. i.e.
mdio_probe(), phy_device_register(), phy_init_hw(), phy_probe(), of_mdiobus_register_phy().

Isn't it simpler to just de-assert it once at the topmost level?
i.e. of_mdiobus_register_device() f and of_mdiobus_register_phy()?

Also, how about issuing a reset pulse instead of just de-asserting it.
This would tackle the case where PHY is in invalid state with reset already
de-asserted.

Another issue is that on some boards we have one reset line tied to
multiple PHYs. How do we prevent multiple resets being taking place when each of
the PHYs are registered? Do we just specify the reset line only for one PHY in
the DT or can we have the reset gpio in the mdio_bus node for such case?

cheers,
-roger

>
>> + }
>> +
>>   return err;
>>  }
>> [...]
>> Index: net-next/drivers/of/of_mdio.c
>> ===================================================================
>> --- net-next.orig/drivers/of/of_mdio.c
>> +++ net-next/drivers/of/of_mdio.c
>> @@ -44,6 +44,7 @@ static int of_get_phy_id(struct device_n
>>  static void of_mdiobus_register_phy(struct mii_bus *mdio,
>>      struct device_node *child, u32 addr)
>>  {
>> + struct gpio_desc *gpiod;
>>   struct phy_device *phy;
>>   bool is_c45;
>>   int rc;
>> @@ -52,10 +53,17 @@ static void of_mdiobus_register_phy(stru
>>   is_c45 = of_device_is_compatible(child,
>>   "ethernet-phy-ieee802.3-c45");
>>  
>> + gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios");
>> + /* Deassert the reset signal */
>> + if (!IS_ERR(gpiod))
>> + gpiod_direction_output(gpiod, 0);
>
> This is wrong I think. You must only ignore -ENODEV, all other error
> codes should be passed to the caller. (I see that's not trivial because
> of_mdiobus_register_phy returns void.)
>
> In my patch I used devm_gpiod_get_array which has the nice property that
> I can already pass GPIOD_OUT_LOW in flags. Also this binds the lifetime
> of the gpio to the device which is nice and IMHO the right direction for
> the phylib (i.e. better embracing of the device model).
>
> This cannot be used here easily however because there is no struct
> device yet and this is only created after the phy id is determined. The
> phy id is either read from the device tree or must be read from the phy
> which might fail if reset is not deasserted.
>
> Principally there is no reason however that the phy_id must be known
> before the struct device is created however.
>
> Best regards
> Uwe
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sergei Shtylyov-4
In reply to this post by Sergei Shtylyov-4
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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sergei Shtylyov-4
In reply to this post by Roger Quadros
Hello.

On 05/13/2016 12:07 PM, Roger Quadros wrote:

[...]

>>> +}
>>> +EXPORT_SYMBOL(mdio_device_reset);
>>> +
>>>  /**
>>>   * mdio_probe - probe an MDIO device
>>>   * @dev: device to probe
>>> @@ -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?
>
> mdio_probe is called for non PHY devices only, right?

    Yes, those also can have a reset signal.

> I'm a bit lost as to why we're de-asserting reset at multiple places. i.e.
> mdio_probe(), phy_device_register(), phy_init_hw(), phy_probe(), of_mdiobus_register_phy().

> Isn't it simpler to just de-assert it once at the topmost level?

    I wasn't after simplicity here. The intent was to save some power putting
the device at reset when it's not needed. Florian Fainelly (the phylib
maintainer) actually wanted me to go even further with that and assert reset
in phy_suspend() but it was too much for me.

> i.e. of_mdiobus_register_device() f and of_mdiobus_register_phy()?
>
> Also, how about issuing a reset pulse instead of just de-asserting it.

    If it's already held at reset, my assumption is that it's enough time
passed already.

> This would tackle the case where PHY is in invalid state with reset already
> de-asserted.

     Good question. I haven't yet met such cases though.

> Another issue is that on some boards we have one reset line tied to
> multiple PHYs.How do we prevent multiple resets being taking place when each of
> the PHYs are registered?

    My patch just doesn't address this case -- it's about the individual
resets only.

> Do we just specify the reset line only for one PHY in
> the DT or can we have the reset gpio in the mdio_bus node for such case?

    I think there's mii_bus::reset() method for that case. Some Ethernet
drivers even use it
(mostly instead of the code being suggested here).

> cheers,
> -roger

MBR, Sergei

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Andrew Lunn
> >Another issue is that on some boards we have one reset line tied to
> >multiple PHYs.How do we prevent multiple resets being taking place when each of
> >the PHYs are registered?
>
>    My patch just doesn't address this case -- it's about the
> individual resets only.

This actually needs to be addresses a layer above. What you have is a
bus reset, not a device reset. So the gpio line is associated to the
mdio bus, not a PHY. Either your MDIO driver needs to handle the gpio
line, or in __mdio_register(), before it starts looking at the
children.

        Andrew
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sergei Shtylyov-4
On 05/13/2016 11:44 PM, Andrew Lunn wrote:

>>> Another issue is that on some boards we have one reset line tied to
>>> multiple PHYs.How do we prevent multiple resets being taking place when each of
>>> the PHYs are registered?
>>
>>    My patch just doesn't address this case -- it's about the
>> individual resets only.
>
> This actually needs to be addresses a layer above. What you have is a
> bus reset, not a device reset.

    No.
    There's simply no such thing as a bus reset for the xMII/MDIO busses,
there's simply no reset signaling on them. Every device has its own reset
signal and its own timing requirements.

> So the gpio line is associated to the mdio bus, not a PHY.

    No.

> Either your MDIO driver needs to handle the gpio
> line,  or in __mdio_register(),

    __mdiobus_register(), you mean?

> before it starts looking at the
> children.

    It's basically the same thing.
    The MDIO bus reset is a misconception.

>
> Andrew

MBR, Sergei

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sergei Shtylyov-4
In reply to this post by Andrew Lunn
Hello.

On 05/13/2016 07:06 AM, Andrew Lunn wrote:

>>>> + gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios");
>>>> + /* Deassert the reset signal */
>>>> + if (!IS_ERR(gpiod))
>>>> + gpiod_direction_output(gpiod, 0);
>>>
>>> This is wrong I think. You must only ignore -ENODEV, all other error
>>
>>    At least -ENOSYS should also be ignored (it's returned when
>> gpiolib is not configured), right? When does -ENODEV gets returned
>> (it's not easy to follow)?
>>
>>> codes should be passed to the caller.
>>
>>    The caller doesn't care anyway...
>
> It should do.

    Tell that to Florian. So far, everybody has been happy with
of_mdiobus_register(). Until I had to touch this code. :-)

> What if fwnode_get_named_gpiod() returns -EPROBE_DEFER
> because the GPIO driver has not been loaded yet?

    Bad luck. :-)
    Seriously, I'll see what I can do but it's not a trivial case.

> Andrew

MBR, Sergei

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sergei Shtylyov-4
In reply to this post by Uwe Kleine-König-5
Hello.

On 05/13/2016 10:06 AM, Uwe Kleine-König wrote:

[...]

>>>> Index: net-next/drivers/of/of_mdio.c
>>>> ===================================================================
>>>> --- net-next.orig/drivers/of/of_mdio.c
>>>> +++ net-next/drivers/of/of_mdio.c
>>>> @@ -44,6 +44,7 @@ static int of_get_phy_id(struct device_n
>>>> static void of_mdiobus_register_phy(struct mii_bus *mdio,
>>>>    struct device_node *child, u32 addr)
>>>> {
>>>> + struct gpio_desc *gpiod;
>>>> struct phy_device *phy;
>>>> bool is_c45;
>>>> int rc;
>>>> @@ -52,10 +53,17 @@ static void of_mdiobus_register_phy(stru
>>>> is_c45 = of_device_is_compatible(child,
>>>> "ethernet-phy-ieee802.3-c45");
>>>>
>>>> + gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios");
>>>> + /* Deassert the reset signal */
>>>> + if (!IS_ERR(gpiod))
>>>> + gpiod_direction_output(gpiod, 0);
>>>
>>> This is wrong I think. You must only ignore -ENODEV, all other error
>>
>>    At least -ENOSYS should also be ignored (it's returned when gpiolib is
>> not configured), right?
>
> No, that's a common misconception. If GPIOLIB is off you cannot
> determine if dt specified a reset gpio. So you have the choice between:
>
>  a) ignore -ENOSYS and so don't handle the reset line in the cases where
>     it's necessary probably yielding an "Error: phy not found" message.
>  b) fail to probe even if a reset handling isn't necessary, yielding
>     "Error: failed to get hold of reset gpio".
>
> I say b) is the better one. It's easier to debug because the error
> message is better, GPIOLIB is enabled in most cases anyhow (still maybe
> select it?) and it's ensured that we're operating in the limits of the
> hardware specs (maybe a phy returns a random value when a register is
> read while reset is applied?).

    It returns all ones in my case.

>> When does -ENODEV gets returned (it's not easy to follow)?
>
> I don't know for sure for fwnode_get_named_gpiod, but the gpiod_get*()
> family returns -ENODEV if the node doesn't have a reset-gpio property.

    Are you sure it's not -ENOENT?

>>> codes should be passed to the caller.
>>
>>    The caller doesn't care anyway...
>>
>>> (I see that's not trivial because
>>> of_mdiobus_register_phy returns void.)
>>
>>    I've made this function *void* in net-next.
>
> I'd say this is a step in the wrong direction. For example this makes it
> impossible to handle the case where the phy should be probed, the gpio
> driver isn't available yet, though. Normally you'd want to return
> -EPROBE_DEFER in this case and retry probing later.

    Well, of_mdiobus_register() is not an easy function to add the checks
whiuch were never there (and undo the done stuff on failure). I'll see what I
can do but no promises...

>>> In my patch I used devm_gpiod_get_array which has the nice property that
>>> I can already pass GPIOD_OUT_LOW in flags. Also this binds the lifetime
>>> of the gpio to the device which is nice and IMHO the right direction for
>>> the phylib (i.e. better embracing of the device model).
>>>
>>> This cannot be used here easily however because there is no struct
>>> device yet and this is only created after the phy id is determined.
>>
>>    Your last patch [1] didn't make use of the managed device API (devm)
>> either, I didn't quite get to the bottom of that...
>
> Right, devm didn't work. My patch was a prototype for a way that allowed
> to bind the lifetime of the gpio to the device. This might be longer
> than the call to mdiobus_unregister.

    Ah, that was the reason... Well, then you hardly achieved anything with
rehashing the code...

> See the problems that i2c handles
> because it doesn't handle lifetimes correctly in drivers/i2c/i2c-core.c
> at the end of i2c_del_adapter.
>
>>> The phy id is either read from the device tree or must be read from
>>> the phy which might fail if reset is not deasserted.
>>
>>> Principally there is no reason however that the phy_id must be known
>>> before the struct device is created however.
>>
>>    It's just that the code is cleaner that way...
>
> I don't agree, I don't see that
>
> determine_phyid()
> allocate_device()
>
> is better than
>
> allocate_device()
> determine_phyid()

    This is an oversimplified view. Actually, it is:

        error = determine_phyid()
        if (error)
                return
        allocate_device()

versus

        allocate_device()
        error = determine_phyid()
        if (error)
                free_device()

> . The former is maybe easier because that's the status quo and it
> doesn't need patching. But IMHO the result is on a similar level of
> cleanliness.

    I disagree. And I don't see why it's necessary at all. Just to use another
gpiolib API?

> Best regards
> Uwe

MBR, Sergei

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Andrew Lunn
In reply to this post by Sergei Shtylyov-4
On Fri, May 13, 2016 at 11:56:12PM +0300, Sergei Shtylyov wrote:

> On 05/13/2016 11:44 PM, Andrew Lunn wrote:
>
> >>>Another issue is that on some boards we have one reset line tied to
> >>>multiple PHYs.How do we prevent multiple resets being taking place when each of
> >>>the PHYs are registered?
> >>
> >>   My patch just doesn't address this case -- it's about the
> >>individual resets only.
> >
> >This actually needs to be addresses a layer above. What you have is a
> >bus reset, not a device reset.
>
>    No.
>    There's simply no such thing as a bus reset for the xMII/MDIO
> busses, there's simply no reset signaling on them. Every device has
> its own reset signal and its own timing requirements.

Except in the case above, where two phys are sharing the same reset
signal. So although it is not part of the mdio standard to have a bus
reset, this is in effect what the gpio line is doing, resetting all
devices on the bus. If you don't model that as a bus reset, how do you
model it?

      Andrew
123
Loading...