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

[Roger Quadros <rogerq@ti.com>]

Hi Sergei,

On 12/05/16 21:42, Uwe Kleine-König wrote:
> Hello Sergei,
> 
> [we already talked about this patch in #armlinux, I'm now just
> forwarding my comments on the list. Background was that I sent an easier
> and less complete patch with the same idea. See
> http://patchwork.ozlabs.org/patch/621418/]
> 
> [added Linus Walleij to Cc, there is a question for you/him below]
> 
> On Fri, Apr 29, 2016 at 01:12:54AM +0300, Sergei Shtylyov wrote:
>> --- 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 {
> 
> This is great.
> 
>> 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");
>> [...]
> 
> My patch breaks this driver. I wasn't aware of it.
> 
>> [...]
>> 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
>> [...]
>> @@ -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);
> 
> Before v4.6-rc1~108^2~91 it was not necessary to check for the first
> parameter being non-NULL before calling gpiod_set_value. Linus, did you
> change this on purpose?
> 
>> +}
>> +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);
> 
> I wonder if it's safe to do this in general. What if ->probe does
> something with the phy that is lost by resetting but that is relied on
> later?

mdio_probe is called for non PHY devices only, right?

I'm a bit lost as to why we're de-asserting reset at multiple places. i.e.
mdio_probe(), phy_device_register(), phy_init_hw(), phy_probe(), of_mdiobus_register_phy().

Isn't it simpler to just de-assert it once at the topmost level?
i.e. of_mdiobus_register_device() f and of_mdiobus_register_phy()?

Also, how about issuing a reset pulse instead of just de-asserting it.
This would tackle the case where PHY is in invalid state with reset already
de-asserted.

Another issue is that on some boards we have one reset line tied to
multiple PHYs. How do we prevent multiple resets being taking place when each of
the PHYs are registered? Do we just specify the reset line only for one PHY in
the DT or can we have the reset gpio in the mdio_bus node for such case?

cheers,
-roger

> 
>> +	}
>> +
>>  	return err;
>>  }
>> [...]
>> 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 void 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 void of_mdiobus_register_phy(stru
>>  	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);
> 
> This is wrong I think. You must only ignore -ENODEV, all other error
> codes should be passed to the caller. (I see that's not trivial because
> of_mdiobus_register_phy returns void.)
> 
> In my patch I used devm_gpiod_get_array which has the nice property that
> I can already pass GPIOD_OUT_LOW in flags. Also this binds the lifetime
> of the gpio to the device which is nice and IMHO the right direction for
> the phylib (i.e. better embracing of the device model).
> 
> This cannot be used here easily however because there is no struct
> device yet and this is only created after the phy id is determined. The
> phy id is either read from the device tree or must be read from the phy
> which might fail if reset is not deasserted.
> 
> Principally there is no reason however that the phy_id must be known
> before the struct device is created however.
> 
> Best regards
> Uwe
>