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

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

[Sergei Shtylyov]

Hello.

   Here's the set of 2 patches against DaveM's 'net-next.git' repo. They add to
'phylib' support for resetting devices via GPIO and do some clean up after
doing that...

[1/2] phylib: add device reset GPIO support
[2/2] macb: kill PHY reset code

MBR, Sergei

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

[Sergei Shtylyov]

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 <sergei.shtylyov@cogentembedded.com>

---
 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);
 
@@ -1595,9 +1610,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;
@@ -1611,8 +1633,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 int 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 int of_mdiobus_register_phy(struc
 	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_OR_NULL(phy))
 		return 1;
 
@@ -75,6 +83,9 @@ static int of_mdiobus_register_phy(struc
 	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);
@@ -95,6 +106,7 @@ static int of_mdiobus_register_device(st
 				      u32 addr)
 {
 	struct mdio_device *mdiodev;
+	struct gpio_desc *gpiod;
 	int rc;
 
 	mdiodev = mdio_device_create(mdio, addr);
@@ -107,6 +119,10 @@ static int of_mdiobus_register_device(st
 	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;
 
 struct mdio_device {
@@ -26,6 +27,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)
 
@@ -58,6 +60,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)
 

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

[Rob Herring]

On Sat, Apr 09, 2016 at 01:22:47AM +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...

Lots of double spaces in here. Please fix.

> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> ---
>  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/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 int 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 int of_mdiobus_register_phy(struc
>  	is_c45 = of_device_is_compatible(child,
>  					 "ethernet-phy-ieee802.3-c45");
>  
> +	gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios");

Calling fwnode_* functions in a DT specific file/function? That doesn't 
make sense.

> +	/* 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_OR_NULL(phy))
>  		return 1;
>  

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

[Sergei Shtylyov]

On 04/11/2016 10:25 PM, Rob Herring 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...
>
> Lots of double spaces in here. Please fix.

    Oh, it's you again! :-D

>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org>
>>
>> ---
>>   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/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 int 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 int of_mdiobus_register_phy(struc
>>   	is_c45 = of_device_is_compatible(child,
>>   					 "ethernet-phy-ieee802.3-c45");
>>
>> +	gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios");
>
> Calling fwnode_* functions in a DT specific file/function? That doesn't
> make sense.

    Really?! 8-)
    Where is a DT-only analog I wonder...

MBR, Sergei

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

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

[Rob Herring]

On Mon, Apr 11, 2016 at 2:28 PM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> On 04/11/2016 10:25 PM, Rob Herring 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...
>>
>>
>> Lots of double spaces in here. Please fix.
>
>
>    Oh, it's you again! :-D

Yep, one of those picky kernel maintainers that like a bad rash just
won't go away. :)

>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>>
>>> ---
>>>   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/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 int 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 int of_mdiobus_register_phy(struc
>>>         is_c45 = of_device_is_compatible(child,
>>>                                          "ethernet-phy-ieee802.3-c45");
>>>
>>> +       gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios");
>>
>>
>> Calling fwnode_* functions in a DT specific file/function? That doesn't
>> make sense.
>
>
>    Really?! 8-)
>    Where is a DT-only analog I wonder...

Ah, you're right. NM.

Rob

[PATCH RFT 2/2] macb: kill PHY reset code

[Sergei Shtylyov]

With  the 'phylib' now  being aware of  the "reset-gpios" PHY node property,
there should be no need to frob the PHY reset in this  driver anymore...

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
 drivers/net/ethernet/cadence/macb.c |   17 -----------------
 drivers/net/ethernet/cadence/macb.h |    1 -
 2 files changed, 18 deletions(-)

Index: net-next/drivers/net/ethernet/cadence/macb.c
===================================================================
--- net-next.orig/drivers/net/ethernet/cadence/macb.c
+++ net-next/drivers/net/ethernet/cadence/macb.c
@@ -2884,7 +2884,6 @@ static int macb_probe(struct platform_de
 					      = macb_clk_init;
 	int (*init)(struct platform_device *) = macb_init;
 	struct device_node *np = pdev->dev.of_node;
-	struct device_node *phy_node;
 	const struct macb_config *macb_config = NULL;
 	struct clk *pclk, *hclk = NULL, *tx_clk = NULL;
 	unsigned int queue_mask, num_queues;
@@ -2977,18 +2976,6 @@ static int macb_probe(struct platform_de
 	else
 		macb_get_hwaddr(bp);
 
-	/* Power up the PHY if there is a GPIO reset */
-	phy_node =  of_get_next_available_child(np, NULL);
-	if (phy_node) {
-		int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0);
-
-		if (gpio_is_valid(gpio)) {
-			bp->reset_gpio = gpio_to_desc(gpio);
-			gpiod_direction_output(bp->reset_gpio, 1);
-		}
-	}
-	of_node_put(phy_node);
-
 	err = of_get_phy_mode(np);
 	if (err < 0) {
 		pdata = dev_get_platdata(&pdev->dev);
@@ -3054,10 +3041,6 @@ static int macb_remove(struct platform_d
 		mdiobus_unregister(bp->mii_bus);
 		mdiobus_free(bp->mii_bus);
 
-		/* Shutdown the PHY if there is a GPIO reset */
-		if (bp->reset_gpio)
-			gpiod_set_value(bp->reset_gpio, 0);
-
 		unregister_netdev(dev);
 		clk_disable_unprepare(bp->tx_clk);
 		clk_disable_unprepare(bp->hclk);
Index: net-next/drivers/net/ethernet/cadence/macb.h
===================================================================
--- net-next.orig/drivers/net/ethernet/cadence/macb.h
+++ net-next/drivers/net/ethernet/cadence/macb.h
@@ -832,7 +832,6 @@ struct macb {
 	unsigned int		dma_burst_length;
 
 	phy_interface_t		phy_interface;
-	struct gpio_desc	*reset_gpio;
 
 	/* AT91RM9200 transmit */
 	struct sk_buff *skb;			/* holds skb until xmit interrupt completes */

Re: [PATCH RFT 2/2] macb: kill PHY reset code

[Andrew Lunn]

On Sat, Apr 09, 2016 at 01:25:03AM +0300, Sergei Shtylyov wrote:
> With  the 'phylib' now  being aware of  the "reset-gpios" PHY node property,
> there should be no need to frob the PHY reset in this  driver anymore...
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> ---
>  drivers/net/ethernet/cadence/macb.c |   17 -----------------
>  drivers/net/ethernet/cadence/macb.h |    1 -
>  2 files changed, 18 deletions(-)
> 
> Index: net-next/drivers/net/ethernet/cadence/macb.c
> ===================================================================
> --- net-next.orig/drivers/net/ethernet/cadence/macb.c
> +++ net-next/drivers/net/ethernet/cadence/macb.c
> @@ -2884,7 +2884,6 @@ static int macb_probe(struct platform_de
>  					      = macb_clk_init;
>  	int (*init)(struct platform_device *) = macb_init;
>  	struct device_node *np = pdev->dev.of_node;
> -	struct device_node *phy_node;
>  	const struct macb_config *macb_config = NULL;
>  	struct clk *pclk, *hclk = NULL, *tx_clk = NULL;
>  	unsigned int queue_mask, num_queues;
> @@ -2977,18 +2976,6 @@ static int macb_probe(struct platform_de
>  	else
>  		macb_get_hwaddr(bp);
>  
> -	/* Power up the PHY if there is a GPIO reset */
> -	phy_node =  of_get_next_available_child(np, NULL);
> -	if (phy_node) {
> -		int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0);
> -
> -		if (gpio_is_valid(gpio)) {
> -			bp->reset_gpio = gpio_to_desc(gpio);
> -			gpiod_direction_output(bp->reset_gpio, 1);

Hi Sergei

The code you are deleting would of ignored the flags in the gpio
property, i.e. active low. The new code in the previous patch does
however take the flags into account. Did you check if there are any
device trees which have flags, which were never used, but are now
going to be used and thus break...

      Andrew

Re: [PATCH RFT 2/2] macb: kill PHY reset code

[Sergei Shtylyov]

Hello.

On 04/11/2016 05:28 AM, Andrew Lunn wrote:

>> With  the 'phylib' now  being aware of  the "reset-gpios" PHY node property,
>> there should be no need to frob the PHY reset in this  driver anymore...
>>
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>
>> ---
>>   drivers/net/ethernet/cadence/macb.c |   17 -----------------
>>   drivers/net/ethernet/cadence/macb.h |    1 -
>>   2 files changed, 18 deletions(-)
>>
>> Index: net-next/drivers/net/ethernet/cadence/macb.c
>> ===================================================================
>> --- net-next.orig/drivers/net/ethernet/cadence/macb.c
>> +++ net-next/drivers/net/ethernet/cadence/macb.c
[...]
>> @@ -2977,18 +2976,6 @@ static int macb_probe(struct platform_de
>>   	else
>>   		macb_get_hwaddr(bp);
>>
>> -	/* Power up the PHY if there is a GPIO reset */
>> -	phy_node =  of_get_next_available_child(np, NULL);
>> -	if (phy_node) {
>> -		int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0);
>> -
>> -		if (gpio_is_valid(gpio)) {
>> -			bp->reset_gpio = gpio_to_desc(gpio);
>> -			gpiod_direction_output(bp->reset_gpio, 1);
>
> Hi Sergei
>
> The code you are deleting would of ignored the flags in the gpio
> property, i.e. active low.

    Hm, you're right -- I forgot about that... :-/

> The new code in the previous patch does
> however take the flags into account. Did you check if there are any
> device trees which have flags, which were never used, but are now
> going to be used and thus break...

    Checked this now and found out arch/arm/boot/dts/ar91-vinco.dts. Looks 
like it needs to be fixed indeed...

>        Andrew

MBR, Sergei

Re: [PATCH RFT 2/2] macb: kill PHY reset code

[Andrew Lunn]

> >The code you are deleting would of ignored the flags in the gpio
> >property, i.e. active low.
> 
>    Hm, you're right -- I forgot about that... :-/
> 
> >The new code in the previous patch does
> >however take the flags into account. Did you check if there are any
> >device trees which have flags, which were never used, but are now
> >going to be used and thus break...
> 
>    Checked this now and found out arch/arm/boot/dts/ar91-vinco.dts.
> Looks like it needs to be fixed indeed...

And this is where it gets tricky. You are breaking backwards
compatibility by now respecting the flag. An old DT blob is not going
to work.

You potentially need to add a new property and deprecate the old one.

    Andrew

Re: [PATCH RFT 2/2] macb: kill PHY reset code

[Sergei Shtylyov]

Hello.

On 04/11/2016 09:19 PM, Andrew Lunn wrote:

>>> The code you are deleting would of ignored the flags in the gpio
>>> property, i.e. active low.
>>
>>     Hm, you're right -- I forgot about that... :-/
>>
>>> The new code in the previous patch does
>>> however take the flags into account. Did you check if there are any
>>> device trees which have flags, which were never used, but are now
>>> going to be used and thus break...
>>
>>     Checked this now and found out arch/arm/boot/dts/ar91-vinco.dts.
>> Looks like it needs to be fixed indeed...
>>
> And this is where it gets tricky. You are breaking backwards
> compatibility by now respecting the flag. An old DT blob is not going
> to work.

    Do we care that much about the DT blobs that are just *wrong*?

> You potentially need to add a new property and deprecate the old one.

    I would like to avoid that...

>      Andrew

MBR, Sergei

Re: [PATCH RFT 2/2] macb: kill PHY reset code

[Andrew Lunn]

On Mon, Apr 11, 2016 at 09:39:02PM +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 04/11/2016 09:19 PM, Andrew Lunn wrote:
> 
> >>>The code you are deleting would of ignored the flags in the gpio
> >>>property, i.e. active low.
> >>
> >>    Hm, you're right -- I forgot about that... :-/
> >>
> >>>The new code in the previous patch does
> >>>however take the flags into account. Did you check if there are any
> >>>device trees which have flags, which were never used, but are now
> >>>going to be used and thus break...
> >>
> >>    Checked this now and found out arch/arm/boot/dts/ar91-vinco.dts.
> >>Looks like it needs to be fixed indeed...
> >>
> >And this is where it gets tricky. You are breaking backwards
> >compatibility by now respecting the flag. An old DT blob is not going
> >to work.
> 
>    Do we care that much about the DT blobs that are just *wrong*?

Wrong, but currently works.

> >You potentially need to add a new property and deprecate the old one.
> 
>    I would like to avoid that...

You will need the agreement from the at91-vinco maintainer.

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

Re: [PATCH RFT 2/2] macb: kill PHY reset code

[Sergei Shtylyov]

On 04/11/2016 09:51 PM, Andrew Lunn wrote:

>>>>> The code you are deleting would of ignored the flags in the gpio
>>>>> property, i.e. active low.
>>>>
>>>>     Hm, you're right -- I forgot about that... :-/
>>>>
>>>>> The new code in the previous patch does
>>>>> however take the flags into account. Did you check if there are any
>>>>> device trees which have flags, which were never used, but are now
>>>>> going to be used and thus break...
>>>>
>>>>     Checked this now and found out arch/arm/boot/dts/ar91-vinco.dts.
>>>> Looks like it needs to be fixed indeed...
>>>>
>>> And this is where it gets tricky. You are breaking backwards
>>> compatibility by now respecting the flag. An old DT blob is not going
>>> to work.
>>
>>     Do we care that much about the DT blobs that are just *wrong*?
>
> Wrong, but currently works.

    Note that it's not only using GPIO_ACTIVE_HIGH but does that against what 
the MACB binding documents.

>>> You potentially need to add a new property and deprecate the old one.
>>
>>     I would like to avoid that...
>
> You will need the agreement from the at91-vinco maintainer.

   I'll try submitting a formal DT patch...

>      Andrew

MBR, Sergei

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

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

[Nicolas Ferre]

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 <sergei.shtylyov@cogentembedded.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: David Miller <davem@davemloft.net>
Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
Hi,

Thanks for having reported this bug to me.
I hope that we will be able to move forward for changing the phy
reset code in the macb driver.

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

Thanks, best regards,
  Nicolas

 arch/arm/boot/dts/at91-vinco.dts | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/at91-vinco.dts b/arch/arm/boot/dts/at91-vinco.dts
index 79aec55e1ebc..6a366ee952a8 100644
--- a/arch/arm/boot/dts/at91-vinco.dts
+++ b/arch/arm/boot/dts/at91-vinco.dts
@@ -118,7 +118,7 @@
 
 				ethernet-phy@1 {
 					reg = <0x1>;
-					reset-gpios = <&pioE 8 GPIO_ACTIVE_HIGH>;
+					reset-gpios = <&pioE 8 GPIO_ACTIVE_LOW>;
 					interrupt-parent = <&pioB>;
 					interrupts = <15 IRQ_TYPE_EDGE_FALLING>;
 				};
@@ -162,7 +162,7 @@
 					reg = <0x1>;
 					interrupt-parent = <&pioB>;
 					interrupts = <31 IRQ_TYPE_EDGE_FALLING>;
-					reset-gpios = <&pioE 6 GPIO_ACTIVE_HIGH>;
+					reset-gpios = <&pioE 6 GPIO_ACTIVE_LOW>;
 				};
 			};
 
-- 
2.1.3

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

[David Miller]

From: Nicolas Ferre <nicolas.ferre@atmel.com>
Date: Tue, 26 Apr 2016 12:24:32 +0200

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

Ok.

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

[Sergei Shtylyov]

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

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

[Nicolas Ferre]

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

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

[Sergei Shtylyov]

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 <sergei.shtylyov@cogentembedded.com>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: David Miller <davem@davemloft.net>
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
> 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

Re: [PATCH RFT 2/2] macb: kill PHY reset code

[Nicolas Ferre]

Le 11/04/2016 20:51, Andrew Lunn a écrit :
> On Mon, Apr 11, 2016 at 09:39:02PM +0300, Sergei Shtylyov wrote:
>> Hello.
>>
>> On 04/11/2016 09:19 PM, Andrew Lunn wrote:
>>
>>>>> The code you are deleting would of ignored the flags in the gpio
>>>>> property, i.e. active low.
>>>>
>>>>    Hm, you're right -- I forgot about that... :-/
>>>>
>>>>> The new code in the previous patch does
>>>>> however take the flags into account. Did you check if there are any
>>>>> device trees which have flags, which were never used, but are now
>>>>> going to be used and thus break...
>>>>
>>>>    Checked this now and found out arch/arm/boot/dts/ar91-vinco.dts.
>>>> Looks like it needs to be fixed indeed...
>>>>
>>> And this is where it gets tricky. You are breaking backwards
>>> compatibility by now respecting the flag. An old DT blob is not going
>>> to work.
>>
>>    Do we care that much about the DT blobs that are just *wrong*?
> 
> Wrong, but currently works.
> 
>>> You potentially need to add a new property and deprecate the old one.
>>
>>    I would like to avoid that...
> 
> You will need the agreement from the at91-vinco maintainer.

If the at91-vinco has to be modified, you have my agreement that it can
be modified.

Bye,
-- 
Nicolas Ferre
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFT 2/2] macb: kill PHY reset code

[Nicolas Ferre]

Le 11/04/2016 04:28, Andrew Lunn a écrit :
> On Sat, Apr 09, 2016 at 01:25:03AM +0300, Sergei Shtylyov wrote:
>> With  the 'phylib' now  being aware of  the "reset-gpios" PHY node property,
>> there should be no need to frob the PHY reset in this  driver anymore...
>>
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>
>> ---
>>  drivers/net/ethernet/cadence/macb.c |   17 -----------------
>>  drivers/net/ethernet/cadence/macb.h |    1 -
>>  2 files changed, 18 deletions(-)
>>
>> Index: net-next/drivers/net/ethernet/cadence/macb.c
>> ===================================================================
>> --- net-next.orig/drivers/net/ethernet/cadence/macb.c
>> +++ net-next/drivers/net/ethernet/cadence/macb.c
>> @@ -2884,7 +2884,6 @@ static int macb_probe(struct platform_de
>>  					      = macb_clk_init;
>>  	int (*init)(struct platform_device *) = macb_init;
>>  	struct device_node *np = pdev->dev.of_node;
>> -	struct device_node *phy_node;
>>  	const struct macb_config *macb_config = NULL;
>>  	struct clk *pclk, *hclk = NULL, *tx_clk = NULL;
>>  	unsigned int queue_mask, num_queues;
>> @@ -2977,18 +2976,6 @@ static int macb_probe(struct platform_de
>>  	else
>>  		macb_get_hwaddr(bp);
>>  
>> -	/* Power up the PHY if there is a GPIO reset */
>> -	phy_node =  of_get_next_available_child(np, NULL);
>> -	if (phy_node) {
>> -		int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0);
>> -
>> -		if (gpio_is_valid(gpio)) {
>> -			bp->reset_gpio = gpio_to_desc(gpio);
>> -			gpiod_direction_output(bp->reset_gpio, 1);
> 
> Hi Sergei
> 
> The code you are deleting would of ignored the flags in the gpio

I don't parse this.

The code deleted does take the flag into account. And the DT property
associated to it seems correct to me (I mean, with proper flag
specification).

> property, i.e. active low. The new code in the previous patch does
> however take the flags into account. Did you check if there are any
> device trees which have flags, which were never used, but are now
> going to be used and thus break...

Flag was used and you are saying that it's taken into account in new
code... So, what's the issue?

I see a difference in the way the "value" of gpiod_* functions is used.
There may be a misunderstanding here...

Bye,
-- 
Nicolas Ferre

Re: [PATCH RFT 2/2] macb: kill PHY reset code

[Andrew Lunn]

On Tue, Apr 12, 2016 at 11:22:10AM +0200, Nicolas Ferre wrote:
> Le 11/04/2016 04:28, Andrew Lunn a écrit :
> > On Sat, Apr 09, 2016 at 01:25:03AM +0300, Sergei Shtylyov wrote:
> >> With  the 'phylib' now  being aware of  the "reset-gpios" PHY node property,
> >> there should be no need to frob the PHY reset in this  driver anymore...
> >>
> >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> >>
> >> ---
> >>  drivers/net/ethernet/cadence/macb.c |   17 -----------------
> >>  drivers/net/ethernet/cadence/macb.h |    1 -
> >>  2 files changed, 18 deletions(-)
> >>
> >> Index: net-next/drivers/net/ethernet/cadence/macb.c
> >> ===================================================================
> >> --- net-next.orig/drivers/net/ethernet/cadence/macb.c
> >> +++ net-next/drivers/net/ethernet/cadence/macb.c
> >> @@ -2884,7 +2884,6 @@ static int macb_probe(struct platform_de
> >>  					      = macb_clk_init;
> >>  	int (*init)(struct platform_device *) = macb_init;
> >>  	struct device_node *np = pdev->dev.of_node;
> >> -	struct device_node *phy_node;
> >>  	const struct macb_config *macb_config = NULL;
> >>  	struct clk *pclk, *hclk = NULL, *tx_clk = NULL;
> >>  	unsigned int queue_mask, num_queues;
> >> @@ -2977,18 +2976,6 @@ static int macb_probe(struct platform_de
> >>  	else
> >>  		macb_get_hwaddr(bp);
> >>  
> >> -	/* Power up the PHY if there is a GPIO reset */
> >> -	phy_node =  of_get_next_available_child(np, NULL);
> >> -	if (phy_node) {
> >> -		int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0);
> >> -
> >> -		if (gpio_is_valid(gpio)) {
> >> -			bp->reset_gpio = gpio_to_desc(gpio);
> >> -			gpiod_direction_output(bp->reset_gpio, 1);
> > 
> > Hi Sergei
> > 
> > The code you are deleting would of ignored the flags in the gpio
> I don't parse this.
> 
> The code deleted does take the flag into account. And the DT property
> associated to it seems correct to me (I mean, with proper flag
> specification).

Hi Nicolas
 
of_get_named_gpio() does not do anything with the flags. So for
example,

			gpios = <&gpio0 12 GPIO_ACTIVE_LOW>;

the GPIO_ACTIVE_LOW would be ignored. If you want the flags to be
respected, you need to use the gpiod API for all calls, in particular,
you need to use something which calls gpiod_get_index(), since that is
the only function to call gpiod_parse_flags() to translate
GPIO_ACTIVE_LOW into a flag within the gpio descriptor.

	Andrew

Re: [PATCH RFT 2/2] macb: kill PHY reset code

[Nicolas Ferre]

Le 12/04/2016 15:40, Andrew Lunn a écrit :
> On Tue, Apr 12, 2016 at 11:22:10AM +0200, Nicolas Ferre wrote:
>> Le 11/04/2016 04:28, Andrew Lunn a écrit :
>>> On Sat, Apr 09, 2016 at 01:25:03AM +0300, Sergei Shtylyov wrote:
>>>> With  the 'phylib' now  being aware of  the "reset-gpios" PHY node property,
>>>> there should be no need to frob the PHY reset in this  driver anymore...
>>>>
>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>>>
>>>> ---
>>>>  drivers/net/ethernet/cadence/macb.c |   17 -----------------
>>>>  drivers/net/ethernet/cadence/macb.h |    1 -
>>>>  2 files changed, 18 deletions(-)
>>>>
>>>> Index: net-next/drivers/net/ethernet/cadence/macb.c
>>>> ===================================================================
>>>> --- net-next.orig/drivers/net/ethernet/cadence/macb.c
>>>> +++ net-next/drivers/net/ethernet/cadence/macb.c
>>>> @@ -2884,7 +2884,6 @@ static int macb_probe(struct platform_de
>>>>  					      = macb_clk_init;
>>>>  	int (*init)(struct platform_device *) = macb_init;
>>>>  	struct device_node *np = pdev->dev.of_node;
>>>> -	struct device_node *phy_node;
>>>>  	const struct macb_config *macb_config = NULL;
>>>>  	struct clk *pclk, *hclk = NULL, *tx_clk = NULL;
>>>>  	unsigned int queue_mask, num_queues;
>>>> @@ -2977,18 +2976,6 @@ static int macb_probe(struct platform_de
>>>>  	else
>>>>  		macb_get_hwaddr(bp);
>>>>  
>>>> -	/* Power up the PHY if there is a GPIO reset */
>>>> -	phy_node =  of_get_next_available_child(np, NULL);
>>>> -	if (phy_node) {
>>>> -		int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0);
>>>> -
>>>> -		if (gpio_is_valid(gpio)) {
>>>> -			bp->reset_gpio = gpio_to_desc(gpio);
>>>> -			gpiod_direction_output(bp->reset_gpio, 1);
>>>
>>> Hi Sergei
>>>
>>> The code you are deleting would of ignored the flags in the gpio
>> I don't parse this.
>>
>> The code deleted does take the flag into account. And the DT property
>> associated to it seems correct to me (I mean, with proper flag
>> specification).
> 
> Hi Nicolas
>  
> of_get_named_gpio() does not do anything with the flags. So for
> example,
> 
> 			gpios = <&gpio0 12 GPIO_ACTIVE_LOW>;
> 
> the GPIO_ACTIVE_LOW would be ignored. If you want the flags to be
> respected, you need to use the gpiod API for all calls, in particular,
> you need to use something which calls gpiod_get_index(), since that is
> the only function to call gpiod_parse_flags() to translate
> GPIO_ACTIVE_LOW into a flag within the gpio descriptor.

Ok, I remember what confused me now: this code, used to be something around:
devm_gpiod_get_optional(&bp->pdev->dev, "phy-reset", GPIOD_OUT_HIGH);
before it has been changed to the chunk above... So, yes, the DT flag
was not handled anyway...

Sorry for the noise and thanks for the clarification.

Bye,
-- 
Nicolas Ferre

Re: [PATCH RFT 2/2] macb: kill PHY reset code

[Sergei Shtylyov]

Hello.

On 4/12/2016 12:22 PM, Nicolas Ferre wrote:

>>> With  the 'phylib' now  being aware of  the "reset-gpios" PHY node property,
>>> there should be no need to frob the PHY reset in this  driver anymore...
>>>
>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>>
>>> ---
>>>   drivers/net/ethernet/cadence/macb.c |   17 -----------------
>>>   drivers/net/ethernet/cadence/macb.h |    1 -
>>>   2 files changed, 18 deletions(-)
>>>
>>> Index: net-next/drivers/net/ethernet/cadence/macb.c
>>> ===================================================================
>>> --- net-next.orig/drivers/net/ethernet/cadence/macb.c
>>> +++ net-next/drivers/net/ethernet/cadence/macb.c
[...]
>>> @@ -2977,18 +2976,6 @@ static int macb_probe(struct platform_de
>>>   	else
>>>   		macb_get_hwaddr(bp);
>>>
>>> -	/* Power up the PHY if there is a GPIO reset */
>>> -	phy_node =  of_get_next_available_child(np, NULL);
>>> -	if (phy_node) {
>>> -		int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0);
>>> -
>>> -		if (gpio_is_valid(gpio)) {
>>> -			bp->reset_gpio = gpio_to_desc(gpio);
>>> -			gpiod_direction_output(bp->reset_gpio, 1);
>>
>> Hi Sergei
>>
>> The code you are deleting would of ignored the flags in the gpio
>
> I don't parse this.

> The code deleted does take the flag into account.

    Not really -- you need to call of_get_named_gpio_flags() (with a valid 
last argument) for that.

> And the DT property
> associated to it seems correct to me (I mean, with proper flag
> specification).

    It apparently is not as it have GPIO_ACTIVE_HIGH and the driver assumes 
active-low reset signal.

[...]
> Bye,

MBR, Sergei

Re: [PATCH RFT 2/2] macb: kill PHY reset code

[Nicolas Ferre]

Le 12/04/2016 15:54, Sergei Shtylyov a écrit :
> Hello.
> 
> On 4/12/2016 12:22 PM, Nicolas Ferre wrote:
> 
>>>> With  the 'phylib' now  being aware of  the "reset-gpios" PHY node property,
>>>> there should be no need to frob the PHY reset in this  driver anymore...
>>>>
>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>>>
>>>> ---
>>>>   drivers/net/ethernet/cadence/macb.c |   17 -----------------
>>>>   drivers/net/ethernet/cadence/macb.h |    1 -
>>>>   2 files changed, 18 deletions(-)
>>>>
>>>> Index: net-next/drivers/net/ethernet/cadence/macb.c
>>>> ===================================================================
>>>> --- net-next.orig/drivers/net/ethernet/cadence/macb.c
>>>> +++ net-next/drivers/net/ethernet/cadence/macb.c
> [...]
>>>> @@ -2977,18 +2976,6 @@ static int macb_probe(struct platform_de
>>>>   	else
>>>>   		macb_get_hwaddr(bp);
>>>>
>>>> -	/* Power up the PHY if there is a GPIO reset */
>>>> -	phy_node =  of_get_next_available_child(np, NULL);
>>>> -	if (phy_node) {
>>>> -		int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0);
>>>> -
>>>> -		if (gpio_is_valid(gpio)) {
>>>> -			bp->reset_gpio = gpio_to_desc(gpio);
>>>> -			gpiod_direction_output(bp->reset_gpio, 1);
>>>
>>> Hi Sergei
>>>
>>> The code you are deleting would of ignored the flags in the gpio
>>
>> I don't parse this.
> 
>> The code deleted does take the flag into account.
> 
>     Not really -- you need to call of_get_named_gpio_flags() (with a valid 
> last argument) for that.

Yep,

>> And the DT property
>> associated to it seems correct to me (I mean, with proper flag
>> specification).
> 
>     It apparently is not as it have GPIO_ACTIVE_HIGH and the driver assumes 
> active-low reset signal.

Yes, logic was inverted and... anyway, the flag never used for real...
Thanks Sergei.

No problem for me accepting a patch for the at91-vinco.dts.

Bye,
-- 
Nicolas Ferre

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

[Sergei Shtylyov]

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 <sergei.shtylyov@cogentembedded.com>

---
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)
 

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

[Rob Herring]

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 <sergei.shtylyov@cogentembedded.com>
> 
> ---
> 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 <robh@kernel.org>

>  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)
>  
> 

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

[Florian Fainelli]

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 <sergei.shtylyov@cogentembedded.com>

This looks good to me:

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

Can you follow up with changes in phy_{suspend,resume} if that is also
an use case that you have?
-- 
Florian

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

[Sergei Shtylyov]

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 <sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org>
>
> This looks good to me:
>
> Acked-by: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

    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

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

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

[Florian Fainelli]

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 <sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org>
>>
>> This looks good to me:
>>
>> Acked-by: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
>    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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Uwe Kleine-König]

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/  |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Sergei Shtylyov]

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

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

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

[Sergei Shtylyov]

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

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

[Uwe Kleine-König]

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/  |

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

[Sergei Shtylyov]

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

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

[Sergei Shtylyov]

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

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

[Sergei Shtylyov]

Hello.

On 05/13/2016 10:18 PM, 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

    Only iff the config_init() method exists in the PHY driver...

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

    No, actually it doesn't. :-(

> Looks like I should alsolook into fixing lxt.c.

    It took me to actually do a patch to understand my fault. Sigh... :-/

> Florian, what do you think?

    Florian, is phy_init_hw() logic correct?

MBR, Sergei

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

[Roger Quadros]

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
> 

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

[Sergei Shtylyov]

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

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

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

[Sergei Shtylyov]

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

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

[Andrew Lunn]

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

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

[Sergei Shtylyov]

Hello.

On 05/14/2016 02:44 AM, 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?

    I'm not suggesting that the shared reset should be handled by my patch. 
Contrariwise, I suggested to use the mii_bus::reset() method -- I see it as a 
necessary evil. However, in the more common case of a single PHY, this method 
simply doesn't scale -- you'd have to teach each and every individual MAC/ 
MDIO driver to do the GPIO reset trick.

>       Andrew

MBR, Sergei

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

[Andrew Lunn]

On Sat, May 14, 2016 at 10:36:38PM +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 05/14/2016 02:44 AM, 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?
> 
>    I'm not suggesting that the shared reset should be handled by my
> patch. Contrariwise, I suggested to use the mii_bus::reset() method

I think we miss understood each other somewhere.

Your code is great for one gpio reset line for one phy.

I think there could be similar code one layer above to handle one gpio
line for multiple phys.

     Andrew

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

[Sergei Shtylyov]

Hello.

On 05/14/2016 10:50 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?
>>
>>    I'm not suggesting that the shared reset should be handled by my
>> patch. Contrariwise, I suggested to use the mii_bus::reset() method
>
> I think we miss understood each other somewhere.
>
> Your code is great for one gpio reset line for one phy.
>
> I think there could be similar code one layer above to handle one gpio
> line for multiple phys.

    Ah, you want me to recognize some MAC/MDIO bound prop (e.g. 
"mdio-reset-gpios") in of_mdiobus_register()? I'll think about it now that my 
patch needs fixing anyway...

>      Andrew

MBR, Sergei

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

[Andrew Lunn]

> >I think there could be similar code one layer above to handle one gpio
> >line for multiple phys.
> 
>    Ah, you want me to recognize some MAC/MDIO bound prop (e.g.
> "mdio-reset-gpios") in of_mdiobus_register()? I'll think about it
> now that my patch needs fixing anyway...

Hi Sergi

It does not need to be you implementing this, your hardware does not
need it. However, if you do feel like doing it, great.

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

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

[Sergei Shtylyov]

Hello.

On 5/15/2016 6:23 PM, Andrew Lunn wrote:

>>> I think there could be similar code one layer above to handle one gpio
>>> line for multiple phys.
>>
>>    Ah, you want me to recognize some MAC/MDIO bound prop (e.g.
>> "mdio-reset-gpios") in of_mdiobus_register()? I'll think about it
>> now that my patch needs fixing anyway...
>
> Hi Sergi
>
> It does not need to be you implementing this, your hardware does not
> need it. However, if you do feel like doing it, great.

    It would cover my "single PHY" case anyway.

>      Andrew

MBR, Sergei

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

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

[Roger Quadros]

On 13/05/16 22:36, Sergei Shtylyov wrote:
> 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.

Is using RESET for power saving a standard practice? Isn't there some register interface for power saving?
My concern here is that RESET does a number of things that might be undesired for normal operation.
e.g. PHY's will use bootstrap settings on RESET and we need to ensure that bootstrap pins are always in
the right setting before issuing a RESET.

What happens to the PHY link? Is it lost while PHY RESET is asserted?
Is this really desirable?


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

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

[Linus Walleij]

On Thu, May 12, 2016 at 8:42 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:

> [added Linus Walleij to Cc, there is a question for you/him below]
(...)
>> +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?

Not really. And AFAICT it is still not necessary: what changed is that
an error message will be printed by VALIDATE_DESC() if you do that.
And that is proper I guess? I think it's sloppy code to randomly pass in
NULL to a call and just expect it to bail out, it seems more like
exercising the error path than something you'd normally rely on.

Or am I getting things wrong?

Yours,
Linus Walleij

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

[Uwe Kleine-König]

On Thu, May 26, 2016 at 11:00:55AM +0200, Linus Walleij wrote:
> On Thu, May 12, 2016 at 8:42 PM, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> 
> > [added Linus Walleij to Cc, there is a question for you/him below]
> (...)
> >> +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?
> 
> Not really. And AFAICT it is still not necessary: what changed is that
> an error message will be printed by VALIDATE_DESC() if you do that.
> And that is proper I guess? I think it's sloppy code to randomly pass in
> NULL to a call and just expect it to bail out, it seems more like
> exercising the error path than something you'd normally rely on.
> 
> Or am I getting things wrong?

is the following sloppy?:

	somegpio = gpiod_get_optional(dev, "some", GPIOD_OUT_LOW);
	if (IS_ERR(somegpio))
		return PTR_ERR(somegpio);
	gpiod_set_value(somegpio, 1);

If not (as I assume) you really changed something as this might trigger
the warning.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Linus Walleij]

On Thu, May 26, 2016 at 9:00 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Thu, May 26, 2016 at 11:00:55AM +0200, Linus Walleij wrote:
>> On Thu, May 12, 2016 at 8:42 PM, Uwe Kleine-König
>> <u.kleine-koenig@pengutronix.de> wrote:
>>
>> > [added Linus Walleij to Cc, there is a question for you/him below]
>> (...)
>> >> +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?
>>
>> Not really. And AFAICT it is still not necessary: what changed is that
>> an error message will be printed by VALIDATE_DESC() if you do that.
>> And that is proper I guess? I think it's sloppy code to randomly pass in
>> NULL to a call and just expect it to bail out, it seems more like
>> exercising the error path than something you'd normally rely on.
>>
>> Or am I getting things wrong?
>
> is the following sloppy?:
>
>         somegpio = gpiod_get_optional(dev, "some", GPIOD_OUT_LOW);
>         if (IS_ERR(somegpio))
>                 return PTR_ERR(somegpio);
>         gpiod_set_value(somegpio, 1);

Grrr OK I see, it's explicit from the _optional() call that it may be NULL
and then it should be ignored. So subsequent functions should ignore
that and bail out. My bad, sorry.

> If not (as I assume) you really changed something as this might trigger
> the warning.

Making a patch.

Yours,
Linus Walleij