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

[Roger Quadros]

On 13/05/16 22:36, Sergei Shtylyov wrote:
> Hello.
> 
> On 05/13/2016 12:07 PM, Roger Quadros wrote:
> 
> [...]
> 
>>>> +}
>>>> +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?
> 
>    Yes, those also can have a reset signal.
> 
>> 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 wasn't after simplicity here. The intent was to save some power putting the device at reset when it's not needed. Florian Fainelly (the phylib maintainer) actually wanted me to go even further with that and assert reset in phy_suspend() but it was too much for me.

Is using RESET for power saving a standard practice? Isn't there some register interface for power saving?
My concern here is that RESET does a number of things that might be undesired for normal operation.
e.g. PHY's will use bootstrap settings on RESET and we need to ensure that bootstrap pins are always in
the right setting before issuing a RESET.

What happens to the PHY link? Is it lost while PHY RESET is asserted?
Is this really desirable?


> 
>> 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.
> 
>    If it's already held at reset, my assumption is that it's enough time passed already.
> 
>> This would tackle the case where PHY is in invalid state with reset already
>> de-asserted.
> 
>     Good question. I haven't yet met such cases though.
> 
>> 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?
> 
>    My patch just doesn't address this case -- it's about the individual resets only.
> 
>> 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?
> 
>    I think there's mii_bus::reset() method for that case. Some Ethernet drivers even use it
> (mostly instead of the code being suggested here).
> 

--
cheers,
-roger