Re: [PATCH v3 1/4] phylib: Add device reset GPIO support

[Sergei Shtylyov]

Hello!

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

> > From: Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxxxx>
> > Acked-by: Rob Herring <robh@xxxxxxxxxx>
> > Acked-by: Florian Fainelli <f.fainelli@xxxxxxxxx>
>
> 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@xxxxxxxxx>
[...]
> > 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