Re: [PATCH v4.1] phylib: Add device reset GPIO support

2017-12-08 Thread Sergei Shtylyov 

Hello!

On 12/08/2017 12:53 PM, Geert Uytterhoeven wrote:


On 12/04/2017 03:35 PM, Geert Uytterhoeven wrote:

From: 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 -- it made use of the reset GPIO for its own purposes...

Signed-off-by: Sergei Shtylyov 
Acked-by: Rob Herring 
[geert: Propagate actual errors from fwnode_get_named_gpiod()]
[geert: Avoid destroying initial setup]
[geert: Consolidate GPIO descriptor acquiring code]
Signed-off-by: Geert Uytterhoeven 


[...]


diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 2df7b62c1a36811e..8f8b7747c54bc478 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c


[...]


@@ -48,9 +49,26 @@
 int mdiobus_register_device(struct mdio_device *mdiodev)
   {
+   struct gpio_desc *gpiod = NULL;
+
 if (mdiodev->bus->mdio_map[mdiodev->addr])
 return -EBUSY;
   + /* Deassert the optional reset signal */



Umm, but why deassert it here for such a short time?


That's a consequence of moving it from drivers/of/of_mdio.c to here.


   Well, you shouldn't do code moves without some thinking. ;-)


Not that it was deasserted that much longer in drivers/of/of_mdio.c, though...


   There it had a reason, here I'm not seeing one. Perhaps using GPIOD_ASIS 
(or GPIOD_OUT_HIGH) instead of GPIOD_OUT_LOW and dropping 
mdio_device_reset(mdiodev, 1) afterwards would make more sense here?



Gr{oetje,eeting}s,

 Geert


MBR, Sergei

Re: [PATCH v4.1] phylib: Add device reset GPIO support

2017-12-08 Thread Geert Uytterhoeven 
Hi Sergei,

On Thu, Dec 7, 2017 at 6:20 PM, Sergei Shtylyov
 wrote:
> On 12/04/2017 03:35 PM, Geert Uytterhoeven wrote:
>> From: 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 -- it made use of the reset GPIO for its own purposes...
>>
>> Signed-off-by: Sergei Shtylyov 
>> Acked-by: Rob Herring 
>> [geert: Propagate actual errors from fwnode_get_named_gpiod()]
>> [geert: Avoid destroying initial setup]
>> [geert: Consolidate GPIO descriptor acquiring code]
>> Signed-off-by: Geert Uytterhoeven 
>
> [...]
>>
>> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
>> index 2df7b62c1a36811e..8f8b7747c54bc478 100644
>> --- a/drivers/net/phy/mdio_bus.c
>> +++ b/drivers/net/phy/mdio_bus.c
>
> [...]
>>
>> @@ -48,9 +49,26 @@
>> int mdiobus_register_device(struct mdio_device *mdiodev)
>>   {
>> +   struct gpio_desc *gpiod = NULL;
>> +
>> if (mdiodev->bus->mdio_map[mdiodev->addr])
>> return -EBUSY;
>>   + /* Deassert the optional reset signal */
>
>
>Umm, but why deassert it here for such a short time?

That's a consequence of moving it from drivers/of/of_mdio.c to here.
Not that it was deasserted that much longer in drivers/of/of_mdio.c, though...

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH v4.1] phylib: Add device reset GPIO support

2017-12-07 Thread Sergei Shtylyov 

On 12/07/2017 08:20 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 -- it made use of the reset GPIO for its own purposes...

Signed-off-by: Sergei Shtylyov 
Acked-by: Rob Herring 
[geert: Propagate actual errors from fwnode_get_named_gpiod()]
[geert: Avoid destroying initial setup]
[geert: Consolidate GPIO descriptor acquiring code]
Signed-off-by: Geert Uytterhoeven 

[...]

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 2df7b62c1a36811e..8f8b7747c54bc478 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c

[...]

@@ -48,9 +49,26 @@
  int mdiobus_register_device(struct mdio_device *mdiodev)
  {
+struct gpio_desc *gpiod = NULL;
+
  if (mdiodev->bus->mdio_map[mdiodev->addr])
  return -EBUSY;
+/* Deassert the optional reset signal */


Umm, but why deassert it here for such a short time?


+if (mdiodev->dev.of_node)
+gpiod = fwnode_get_named_gpiod(&mdiodev->dev.of_node->fwnode,
+   "reset-gpios", 0, GPIOD_OUT_LOW,
+   "PHY reset");
+if (PTR_ERR(gpiod) == -ENOENT)
+gpiod = NULL;
+else if (IS_ERR(gpiod))
+return PTR_ERR(gpiod);


Hm, returning on error with reset deasserted?


   Oops, error means we couldn't drive the GPIO at all...
   The 1st question remains though...

[...]

MBR, Sergei

Re: [PATCH v4.1] phylib: Add device reset GPIO support

2017-12-07 Thread Sergei Shtylyov 

Hello!

On 12/04/2017 03:35 PM, Geert Uytterhoeven wrote:


From: 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 -- it made use of the reset GPIO for its own purposes...

Signed-off-by: Sergei Shtylyov 
Acked-by: Rob Herring 
[geert: Propagate actual errors from fwnode_get_named_gpiod()]
[geert: Avoid destroying initial setup]
[geert: Consolidate GPIO descriptor acquiring code]
Signed-off-by: Geert Uytterhoeven 

[...]

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 2df7b62c1a36811e..8f8b7747c54bc478 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c

[...]

@@ -48,9 +49,26 @@
  
  int mdiobus_register_device(struct mdio_device *mdiodev)

  {
+   struct gpio_desc *gpiod = NULL;
+
if (mdiodev->bus->mdio_map[mdiodev->addr])
return -EBUSY;
  
+	/* Deassert the optional reset signal */


   Umm, but why deassert it here for such a short time?


+   if (mdiodev->dev.of_node)
+   gpiod = fwnode_get_named_gpiod(&mdiodev->dev.of_node->fwnode,
+  "reset-gpios", 0, GPIOD_OUT_LOW,
+  "PHY reset");
+   if (PTR_ERR(gpiod) == -ENOENT)
+   gpiod = NULL;
+   else if (IS_ERR(gpiod))
+   return PTR_ERR(gpiod);


   Hm, returning on error with reset deasserted?


+
+   mdiodev->reset = gpiod;
+
+   /* Assert the reset signal again */
+   mdio_device_reset(mdiodev, 1);
+
mdiodev->bus->mdio_map[mdiodev->addr] = mdiodev;
  
  	return 0;

[...]

MBR, Sergei

Re: [PATCH v4.1] phylib: Add device reset GPIO support

2017-12-04 Thread Richard Leitner 

On 12/04/2017 01:35 PM, Geert Uytterhoeven wrote:
> From: 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 -- it made use of the reset GPIO for its own purposes...
> 
> Signed-off-by: Sergei Shtylyov 
> Acked-by: Rob Herring 
> [geert: Propagate actual errors from fwnode_get_named_gpiod()]
> [geert: Avoid destroying initial setup]
> [geert: Consolidate GPIO descriptor acquiring code]
> Signed-off-by: Geert Uytterhoeven 
> ---

Successfully tested this patch on a i.MX6SOLO based board containing a
LAN8710 PHY:

Tested-by: Richard Leitner

[PATCH v4.1] phylib: Add device reset GPIO support

2017-12-04 Thread Geert Uytterhoeven 
From: 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 -- it made use of the reset GPIO for its own purposes...

Signed-off-by: Sergei Shtylyov 
Acked-by: Rob Herring 
[geert: Propagate actual errors from fwnode_get_named_gpiod()]
[geert: Avoid destroying initial setup]
[geert: Consolidate GPIO descriptor acquiring code]
Signed-off-by: Geert Uytterhoeven 
---
v4.1:
  - Fix uninitialized variable in mdiobus_register_device(),

v4:
  - Remove Florian's Acked-by,
  - Add missing #include ,
  - Re-add the gpiod check, as the dummy gpiod_set_value() for !GPIOLIB
does not ignore NULL, and calls WARN_ON(1),
  - Do not reassert the reset signal if {mdio,phy}_probe() or
phy_device_register() succeeded, as that may destroy initial setup,
  - Do not deassert the reset signal in {mdio,phy}_remove(), as it
should already be deasserted,
  - Bring the PHY back into reset state in phy_device_remove(),
  - Move/consolidate GPIO descriptor acquiring code from
of_mdiobus_register_phy() and of_mdiobus_register_device() to
mdiobus_register_device().
Note that this changes behavior slightly, in that the reset signal
is now also asserted when called from of_mdiobus_register_device().

v3:
  - Fix fwnode_get_named_gpiod() call due to added parameters (which
allowed to eliminate the gpiod_direction_output() call),
  - Undelete one blank line in the AT803x driver,
  - Resolve rejects, refresh patch,
  - Reword/reformat changelog,
  - Take over from Sergei,
  - Add Acked-by,
  - Remove unneeded gpiod check, as gpiod_set_value() handles NULL fine,
  - Handle fwnode_get_named_gpiod() errors correctly:
  - -ENOENT is ignored (the GPIO is optional), and turned into NULL,
which allowed to remove all later !IS_ERR() checks,
  - Other errors (incl. -EPROBE_DEFER) are propagated [*],

v2:
  - Reformat changelog,
  - Resolve rejects, refresh patch.
---
 Documentation/devicetree/bindings/net/phy.txt |  2 ++
 drivers/net/phy/at803x.c  | 18 +++
 drivers/net/phy/mdio_bus.c| 21 ++
 drivers/net/phy/mdio_device.c | 25 +++--
 drivers/net/phy/phy_device.c  | 32 +--
 include/linux/mdio.h  |  3 +++
 include/linux/phy.h   |  5 +
 7 files changed, 87 insertions(+), 19 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/phy.txt 
b/Documentation/devicetree/bindings/net/phy.txt
index 77d0b2a61ffa96fc..c05479f5ac7cc837 100644
--- a/Documentation/devicetree/bindings/net/phy.txt
+++ b/Documentation/devicetree/bindings/net/phy.txt
@@ -53,6 +53,8 @@ Optional Properties:
   to ensure the integrated PHY is used. The absence of this property indicates
   the muxers should be configured so that the external PHY is used.
 
+- reset-gpios: The GPIO phandle and specifier for the PHY reset signal.
+
 Example:
 
 ethernet-phy@0 {
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index de7dd6566df7a364..29da7a3c7a3761c0 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -71,7 +71,6 @@ MODULE_LICENSE("GPL");
 
 struct at803x_priv {
bool phy_reset:1;
-   struct gpio_desc *gpiod_reset;
 };
 
 struct at803x_context {
@@ -254,22 +253,11 @@ static int at803x_probe(struct phy_device *phydev)
 {
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;
@@ -343,14 +331,14 @@ static void at803x_link_change_notify(struct phy_device 
*phydev)
 * cannot recover from by software.
 */
if (phydev->state == PHY_NOLINK) {
-   if (priv->gpi