[PATCH RFT 0/2] Teach phylib hard-resetting devices

classic Classic list List threaded Threaded
49 messages Options
123
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sergei Shtylyov-4
Hello.

On 05/14/2016 02:44 AM, Andrew Lunn wrote:

>>>>> 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.
>>>
>>> This actually needs to be addresses a layer above. What you have is a
>>> bus reset, not a device reset.
>>
>>    No.
>>    There's simply no such thing as a bus reset for the xMII/MDIO
>> busses, there's simply no reset signaling on them. Every device has
>> its own reset signal and its own timing requirements.
>
> Except in the case above, where two phys are sharing the same reset
> signal. So although it is not part of the mdio standard to have a bus
> reset, this is in effect what the gpio line is doing, resetting all
> devices on the bus. If you don't model that as a bus reset, how do you
> model it?

    I'm not suggesting that the shared reset should be handled by my patch.
Contrariwise, I suggested to use the mii_bus::reset() method -- I see it as a
necessary evil. However, in the more common case of a single PHY, this method
simply doesn't scale -- you'd have to teach each and every individual MAC/
MDIO driver to do the GPIO reset trick.

>       Andrew

MBR, Sergei

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Andrew Lunn
On Sat, May 14, 2016 at 10:36:38PM +0300, Sergei Shtylyov wrote:

> Hello.
>
> On 05/14/2016 02:44 AM, Andrew Lunn wrote:
>
> >>>>>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.
> >>>
> >>>This actually needs to be addresses a layer above. What you have is a
> >>>bus reset, not a device reset.
> >>
> >>   No.
> >>   There's simply no such thing as a bus reset for the xMII/MDIO
> >>busses, there's simply no reset signaling on them. Every device has
> >>its own reset signal and its own timing requirements.
> >
> >Except in the case above, where two phys are sharing the same reset
> >signal. So although it is not part of the mdio standard to have a bus
> >reset, this is in effect what the gpio line is doing, resetting all
> >devices on the bus. If you don't model that as a bus reset, how do you
> >model it?
>
>    I'm not suggesting that the shared reset should be handled by my
> patch. Contrariwise, I suggested to use the mii_bus::reset() method

I think we miss understood each other somewhere.

Your code is great for one gpio reset line for one phy.

I think there could be similar code one layer above to handle one gpio
line for multiple phys.

     Andrew
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sergei Shtylyov-4
In reply to this post by Sergei Shtylyov-4
Hello.

On 05/13/2016 10:18 PM, Sergei Shtylyov wrote:

>>> [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.
>>
>>    I tried to be as careful as I could but still it looks that I didn't
>> succeed at that too...
>
>    Hm, I'm starting to forget the vital details about my patch...
>
>> [...]
>>>> 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
>> [...]
>>>> @@ -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?
>>
>>    Well, I thought that config_init() method is designed for that but indeed
>> the LXT driver writes to BMCR in its probe() method and hence is broken.
>> Thank you for noticing...
>
>    It's broken even without my patch. The phylib will cause a PHY soft reset

    Only iff the config_init() method exists in the PHY driver...

> when attaching to the PHY device, so all BMCR programming dpone in the probe()
> method will be lost. My patch does make sense as is.

    No, actually it doesn't. :-(

> Looks like I should alsolook into fixing lxt.c.

    It took me to actually do a patch to understand my fault. Sigh... :-/

> Florian, what do you think?

    Florian, is phy_init_hw() logic correct?

MBR, Sergei

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sergei Shtylyov-4
In reply to this post by Andrew Lunn
Hello.

On 05/14/2016 10:50 PM, Andrew Lunn wrote:

>>>>>>> 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.
>>>>>
>>>>> This actually needs to be addresses a layer above. What you have is a
>>>>> bus reset, not a device reset.
>>>>
>>>>   No.
>>>>   There's simply no such thing as a bus reset for the xMII/MDIO
>>>> busses, there's simply no reset signaling on them. Every device has
>>>> its own reset signal and its own timing requirements.
>>>
>>> Except in the case above, where two phys are sharing the same reset
>>> signal. So although it is not part of the mdio standard to have a bus
>>> reset, this is in effect what the gpio line is doing, resetting all
>>> devices on the bus. If you don't model that as a bus reset, how do you
>>> model it?
>>
>>    I'm not suggesting that the shared reset should be handled by my
>> patch. Contrariwise, I suggested to use the mii_bus::reset() method
>
> I think we miss understood each other somewhere.
>
> Your code is great for one gpio reset line for one phy.
>
> I think there could be similar code one layer above to handle one gpio
> line for multiple phys.

    Ah, you want me to recognize some MAC/MDIO bound prop (e.g.
"mdio-reset-gpios") in of_mdiobus_register()? I'll think about it now that my
patch needs fixing anyway...

>      Andrew

MBR, Sergei

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Andrew Lunn
> >I think there could be similar code one layer above to handle one gpio
> >line for multiple phys.
>
>    Ah, you want me to recognize some MAC/MDIO bound prop (e.g.
> "mdio-reset-gpios") in of_mdiobus_register()? I'll think about it
> now that my patch needs fixing anyway...

Hi Sergi

It does not need to be you implementing this, your hardware does not
need it. However, if you do feel like doing it, great.

     Andrew
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Roger Quadros
In reply to this post by Sergei Shtylyov-4
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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Sergei Shtylyov-4
In reply to this post by Andrew Lunn
Hello.

On 5/15/2016 6:23 PM, Andrew Lunn wrote:

>>> I think there could be similar code one layer above to handle one gpio
>>> line for multiple phys.
>>
>>    Ah, you want me to recognize some MAC/MDIO bound prop (e.g.
>> "mdio-reset-gpios") in of_mdiobus_register()? I'll think about it
>> now that my patch needs fixing anyway...
>
> Hi Sergi
>
> It does not need to be you implementing this, your hardware does not
> need it. However, if you do feel like doing it, great.

    It would cover my "single PHY" case anyway.

>      Andrew

MBR, Sergei

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Linus Walleij
In reply to this post by Uwe Kleine-König-5
On Thu, May 12, 2016 at 8:42 PM, Uwe Kleine-König
<[hidden email]> wrote:

> [added Linus Walleij to Cc, there is a question for you/him below]
(...)
>> +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?

Not really. And AFAICT it is still not necessary: what changed is that
an error message will be printed by VALIDATE_DESC() if you do that.
And that is proper I guess? I think it's sloppy code to randomly pass in
NULL to a call and just expect it to bail out, it seems more like
exercising the error path than something you'd normally rely on.

Or am I getting things wrong?

Yours,
Linus Walleij
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Uwe Kleine-König-5
On Thu, May 26, 2016 at 11:00:55AM +0200, Linus Walleij wrote:

> On Thu, May 12, 2016 at 8:42 PM, Uwe Kleine-König
> <[hidden email]> wrote:
>
> > [added Linus Walleij to Cc, there is a question for you/him below]
> (...)
> >> +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?
>
> Not really. And AFAICT it is still not necessary: what changed is that
> an error message will be printed by VALIDATE_DESC() if you do that.
> And that is proper I guess? I think it's sloppy code to randomly pass in
> NULL to a call and just expect it to bail out, it seems more like
> exercising the error path than something you'd normally rely on.
>
> Or am I getting things wrong?

is the following sloppy?:

        somegpio = gpiod_get_optional(dev, "some", GPIOD_OUT_LOW);
        if (IS_ERR(somegpio))
                return PTR_ERR(somegpio);
        gpiod_set_value(somegpio, 1);

If not (as I assume) you really changed something as this might trigger
the warning.

Best regards
Uwe

--
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
123
Loading...