lkml.org 
[lkml]   [2017]   [Oct]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3 1/4] phylib: Add device reset GPIO support
From
Date
Hello!

On 10/20/2017 9:11 PM, Florian Fainelli wrote:

>> From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>
>> 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 <sergei.shtylyov@cogentembedded.com>
>> Acked-by: Rob Herring <robh@kernel.org>
>> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
>
> This is a new patch as far as PHYLIB is concerned, so not quite accurate

No, it is not new. No changes in logic since v2.

> anymore. Thanks for picking that up, this looks good, with a few minor
> things here and there.

>> [geert: Propagate actual errors from fwnode_get_named_gpiod()]
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
[...]
>> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
>> index 5f93e6add56394f2..15b4560aeb5de759 100644
>> --- a/drivers/net/phy/at803x.c
>> +++ b/drivers/net/phy/at803x.c
>
> You most certainly want to make the conversion of the at803x.c driver as
> a separate patch, there is no reason why it should be folded within the

No, it can't be a separate patch, otherwise I would have posted it
separately. We cannot request the same GPIO 2 times.

> patch teaching PHYLIB about PHY reset through GPIOs. More on that below.
>
[...]
>> @@ -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;
>
> We have lost that bit now, see below.

Hm... I'm still seeing this in net-next.

>> -
>> - 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->gpiod_reset && !priv->phy_reset) {
>> + if (phydev->mdio.reset && !priv->phy_reset) {
>
> and phydev->drv->phy_id == ATH8030_PHY_ID, the workaround is not
> applicable to all PHY devices.

You are clearly looking at an outdated tree -- this check was removed,
This method is populated only for 8030 instead.

[...]
>> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
>> index 98258583abb0b405..3d044119c032d176 100644
>> --- a/drivers/of/of_mdio.c
>> +++ b/drivers/of/of_mdio.c
[...]
>> @@ -55,10 +56,22 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
>> is_c45 = of_device_is_compatible(child,
>> "ethernet-phy-ieee802.3-c45");
>>
>> + /* Deassert the optional reset signal */
>> + gpiod = fwnode_get_named_gpiod(&child->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);
>> +
>> 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 */
>> + gpiod_set_value(gpiod, 1);
>
> You have a phy_device reference now, so why not call phy_device_reset()
> directly here?

Symmetry, perhaps? (There was a gpiod_set_value(gpiod, 0) call above it...

[...]
>> @@ -112,6 +127,14 @@ static int of_mdiobus_register_device(struct mii_bus *mdio,
>> of_node_get(child);
>> mdiodev->dev.of_node = child;
>>
>> + gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios", 0,
>> + GPIOD_ASIS, "PHY reset");
>> + if (PTR_ERR(gpiod) == -ENOENT)
>> + gpiod = NULL;
>> + else if (IS_ERR(gpiod))
>> + return PTR_ERR(gpiod);
>> + mdiodev->reset = gpiod;
>
> There is some obvious and non desireable duplication of code between
> "pure" mdio_device and phy_device paths here, can you factor this such
> that there is a common function storing gpiod into a mdio_device's reset
> member in both cases?
>
> Also, there is some asymetry being exposed here, the GPIO descriptor is
> acquired from OF specific code, but is released in the generic MDIO
> device layer, can we find a way to make that symmetrical?

Finally move of_mdio.c into drivers/net/phy/? ;-)

MBR, Sergei

\
 
 \ /
  Last update: 2017-10-22 17:49    [W:1.884 / U:0.876 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site