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

[permalink] [raw]

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

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 <[email protected]>

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

2016-04-08 22:25:10

[permalink] [raw]

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

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 <[email protected]>

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

2016-04-11 02:28:07

[permalink] [raw]

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

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 <[email protected]>
>
> ---
> 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

2016-04-11 17:41:32

[permalink] [raw]

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

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 <[email protected]>
>>
>> ---
>> 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

2016-04-11 18:19:10

[permalink] [raw]

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

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

2016-04-11 18:39:10

[permalink] [raw]

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

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

2016-04-11 18:51:21

[permalink] [raw]

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

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

2016-04-11 19:01:42

[permalink] [raw]

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

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

2016-04-11 19:25:36

by Rob Herring

[permalink] [raw]

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

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 <[email protected]>
>
> ---
> 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;
>

2016-04-11 19:28:22

[permalink] [raw]

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

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 <[email protected]>
>>
>> ---
>> 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

2016-04-11 22:46:53

by Rob Herring

[permalink] [raw]

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

On Mon, Apr 11, 2016 at 2:28 PM, Sergei Shtylyov
<[email protected]> 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 <[email protected]>
>>>
>>> ---
>>> 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

2016-04-12 09:21:54

[permalink] [raw]

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

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 <[email protected]>
>>
>> ---
>> 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

2016-04-12 09:23:14

[permalink] [raw]

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

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

2016-04-12 13:40:07

[permalink] [raw]

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

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 <[email protected]>
> >>
> >> ---
> >> 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

2016-04-12 13:54:33

[permalink] [raw]

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

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 <[email protected]>
>>>
>>> ---
>>> 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

2016-04-12 14:45:15

[permalink] [raw]

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

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 <[email protected]>
>>>>
>>>> ---
>>>> 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

2016-04-12 14:57:07

[permalink] [raw]

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

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 <[email protected]>
>>>>
>>>> ---
>>>> 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

2016-04-26 10:24:03

[permalink] [raw]

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

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 <[email protected]>
Cc: Andrew Lunn <[email protected]>
Cc: David Miller <[email protected]>
Signed-off-by: Nicolas Ferre <[email protected]>
---
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

2016-04-26 17:17:55

by David Miller

[permalink] [raw]

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

From: Nicolas Ferre <[email protected]>
Date: Tue, 26 Apr 2016 12:24:32 +0200

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

Ok.

2016-04-26 18:25:43

[permalink] [raw]

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

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 <[email protected]>
> Cc: Andrew Lunn <[email protected]>
> Cc: David Miller <[email protected]>
> Signed-off-by: Nicolas Ferre <[email protected]>
> ---
> 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

2016-04-26 18:27:26

[permalink] [raw]

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

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

2016-04-27 07:15:29

[permalink] [raw]

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

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

2016-04-28 22:13:00