Quantcast

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

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

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

Sergei Shtylyov-4
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

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

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

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

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

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

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

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

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

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

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

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

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 <[hidden email]>
>
> ---
>  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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sergei Shtylyov-4
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 <[hidden email]>
>>
>> ---
>>   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

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

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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sergei Shtylyov-4
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

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

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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sergei Shtylyov-4
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

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

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

Rob Herring-3
In reply to this post by Sergei Shtylyov-4
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 <[hidden email]>
>
> ---
>  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;
>  
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sergei Shtylyov-4
On 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 <[hidden email]>
>>
>> ---
>>   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

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

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

Rob Herring-3
On Mon, Apr 11, 2016 at 2:28 PM, Sergei Shtylyov
<[hidden email]> 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 <[hidden email]>
>>>
>>> ---
>>>   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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Nicolas Ferre-2
In reply to this post by Andrew Lunn
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 <[hidden email]>
>>
>> ---
>>  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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Nicolas Ferre-2
In reply to this post by Andrew Lunn
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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Andrew Lunn
In reply to this post by Nicolas Ferre-2
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 <[hidden email]>
> >>
> >> ---
> >>  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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sergei Shtylyov-4
In reply to this post by Nicolas Ferre-2
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 <[hidden email]>
>>>
>>> ---
>>>   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

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

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

Nicolas Ferre-2
In reply to this post by Andrew Lunn
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 <[hidden email]>
>>>>
>>>> ---
>>>>  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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Nicolas Ferre-2
In reply to this post by Sergei Shtylyov-4
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 <[hidden email]>
>>>>
>>>> ---
>>>>   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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Nicolas Ferre-2
In reply to this post by Sergei Shtylyov-4
Fix gpio active flag for the phy reset-gpios property. The line is
active low instead of active high.
Actually, this flags was never used by the macb driver.

Reported-by: Sergei Shtylyov <[hidden email]>
Cc: Andrew Lunn <[hidden email]>
Cc: David Miller <[hidden email]>
Signed-off-by: Nicolas Ferre <[hidden email]>
---
Hi,

Thanks for having reported this bug to me.
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

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

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

David Miller-13
From: Nicolas Ferre <[hidden email]>
Date: Tue, 26 Apr 2016 12:24:32 +0200

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

Ok.
123
Loading...