Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
On Thu, May 26, 2016 at 9:00 PM, Uwe Kleine-König
wrote:
> 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
>> 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);
Grrr OK I see, it's explicit from the _optional() call that it may be NULL
and then it should be ignored. So subsequent functions should ignore
that and bail out. My bad, sorry.
> If not (as I assume) you really changed something as this might trigger
> the warning.
Making a patch.
Yours,
Linus Walleij
Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
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
> 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/ |
Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
On Thu, May 12, 2016 at 8:42 PM, Uwe Kleine-König
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
Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
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
Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
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
Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
> >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
Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
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
Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
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
Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
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
Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
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
Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
On Fri, May 13, 2016 at 11:56:12PM +0300, Sergei Shtylyov wrote: > On 05/13/2016 11:44 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? Andrew
Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
Hello.
On 05/13/2016 10:06 AM, Uwe Kleine-König wrote:
[...]
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
At least -ENOSYS should also be ignored (it's returned when gpiolib is
not configured), right?
No, that's a common misconception. If GPIOLIB is off you cannot
determine if dt specified a reset gpio. So you have the choice between:
a) ignore -ENOSYS and so don't handle the reset line in the cases where
it's necessary probably yielding an "Error: phy not found" message.
b) fail to probe even if a reset handling isn't necessary, yielding
"Error: failed to get hold of reset gpio".
I say b) is the better one. It's easier to debug because the error
message is better, GPIOLIB is enabled in most cases anyhow (still maybe
select it?) and it's ensured that we're operating in the limits of the
hardware specs (maybe a phy returns a random value when a register is
read while reset is applied?).
It returns all ones in my case.
When does -ENODEV gets returned (it's not easy to follow)?
I don't know for sure for fwnode_get_named_gpiod, but the gpiod_get*()
family returns -ENODEV if the node doesn't have a reset-gpio property.
Are you sure it's not -ENOENT?
codes should be passed to the caller.
The caller doesn't care anyway...
(I see that's not trivial because
of_mdiobus_register_phy returns void.)
I've made this function *void* in net-next.
I'd say this is a step in the wrong direction. For example this makes it
impossible to handle the case where the phy should be probed, the gpio
driver isn't available yet, though. Normally you'd want to return
-EPROBE_DEFER in this case and retry probing later.
Well, of_mdiobus_register() is not an easy function to add the checks
whiuch were never there (and undo the done stuff on failure). I'll see what I
can do but no promises...
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.
Your last patch [1] didn't make use of the managed device API (devm)
either, I didn't quite get to the bottom of that...
Right, devm didn't work. My patch was a prototype for a way that allowed
to bind the lifetime of the gpio to the device. This might be longer
than the call to mdiobus_unregister.
Ah, that was the reason... Well, then you hardly achieved anything with
rehashing the code...
See the problems that i2c handles
because it doesn't handle lifetimes correctly in drivers/i2c/i2c-core.c
at the end of i2c_del_adapter.
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.
It's just that the code is cleaner that way...
I don't agree, I don't see that
determine_phyid()
allocate_device()
is better than
allocate_device()
determine_phyid()
This is an oversimplified view. Actually, it is:
error = determine_phyid()
if (error)
return
allocate_device()
versus
allocate_device()
error = determine_phyid()
if (error)
free_device()
. The former is maybe easier because that's the status quo and it
doesn't need patching. But IMHO the result is on a similar level of
cleanliness.
I disagree. And I don't see why it's necessary at all. Just to use another
gpiolib API?
Best regards
Uwe
MBR, Sergei
Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
Hello. On 05/13/2016 07:06 AM, Andrew Lunn wrote: + 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 At least -ENOSYS should also be ignored (it's returned when gpiolib is not configured), right? When does -ENODEV gets returned (it's not easy to follow)? codes should be passed to the caller. The caller doesn't care anyway... It should do. Tell that to Florian. So far, everybody has been happy with of_mdiobus_register(). Until I had to touch this code. :-) What if fwnode_get_named_gpiod() returns -EPROBE_DEFER because the GPIO driver has not been loaded yet? Bad luck. :-) Seriously, I'll see what I can do but it's not a trivial case. Andrew MBR, Sergei
Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
On 05/13/2016 11:44 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. So the gpio line is associated to the mdio bus, not a PHY. No. Either your MDIO driver needs to handle the gpio line, or in __mdio_register(), __mdiobus_register(), you mean? before it starts looking at the children. It's basically the same thing. The MDIO bus reset is a misconception. Andrew MBR, Sergei
Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
> >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. So the gpio line is associated to the mdio bus, not a PHY. Either your MDIO driver needs to handle the gpio line, or in __mdio_register(), before it starts looking at the children. Andrew
Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
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.
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
MBR, Sergei
Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
Hello.
On 05/13/2016 12:35 AM, 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
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. Looks like I should also
look into fixing lxt.c. Florian, what do you think?
[...]
MBR, Sergei
Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
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
Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
Hello Sergei,
On Fri, May 13, 2016 at 12:35:50AM +0300, Sergei Shtylyov wrote:
> On 05/12/2016 09:42 PM, Uwe Kleine-König wrote:
> >>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...
>
> [...]
> >>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
>
>At least -ENOSYS should also be ignored (it's returned when gpiolib is
> not configured), right?
No, that's a common misconception. If GPIOLIB is off you cannot
determine if dt specified a reset gpio. So you have the choice between:
a) ignore -ENOSYS and so don't handle the reset line in the cases where
it's necessary probably yielding an "Error: phy not found" message.
b) fail to probe even if a reset handling isn't necessary, yielding
"Error: failed to get hold of reset gpio".
I say b) is the better one. It's easier to debug because the error
message is better, GPIOLIB is enabled in most cases anyhow (still maybe
select it?) and it's ensured that we're operating in the limits of the
hardware specs (maybe a phy returns a random value when a register is
read while reset is applied?).
> When does -ENODEV gets returned (it's not easy to follow)?
I don't know for sure for fwnode_get_named_gpiod, but the gpiod_get*()
family returns -ENODEV if the node doesn't have a reset-gpio property.
> >codes should be passed to the caller.
>
>The caller doesn't care anyway...
>
> >(I see that's not trivial because
> >of_mdiobus_register_phy returns void.)
>
>I've made this function *void* in net-next.
I'd say this is a step in the wrong direction. For example this makes it
impossible to handle the case where the phy should be probed, the gpio
driver isn't available yet, though. Normally you'd want to return
-EPROBE_DEFER in this case and retry probing later.
> >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.
>
>Your last patch [1] didn't make use of the managed device API (devm)
> either, I didn't quite get to the bottom of that...
Right, devm didn't work. My patch was a prototype for a way that allowed
to bind the lifetime of the gpio to the device. This might be longer
than the call to mdiobus_unregister. See the problems that i2c handles
because it doesn't handle lifetimes correctly in drivers/i2c/i2c-core.c
at the end of i2c_del_adapter.
> >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.
>
>It's just that the code is cleaner that way...
I don't agree, I don't see that
determine_phyid()
allocate_device()
is better than
allocate_device()
determine_phyid()
. The former is maybe easier be
Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
> >>+ 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 > >At least -ENOSYS should also be ignored (it's returned when > gpiolib is not configured), right? When does -ENODEV gets returned > (it's not easy to follow)? > > >codes should be passed to the caller. > >The caller doesn't care anyway... It should do. What if fwnode_get_named_gpiod() returns -EPROBE_DEFER because the GPIO driver has not been loaded yet? Andrew
Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
Hello.
On 05/12/2016 09:42 PM, Uwe Kleine-König 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...
[...]
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...
[...]
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
At least -ENOSYS should also be ignored (it's returned when gpiolib is not
configured), right? When does -ENODEV gets returned (it's not easy to follow)?
codes should be passed to the caller.
The caller doesn't care anyway...
(I see that's not trivial because
of_mdiobus_register_phy returns void.)
I've made this function *void* in net-next.
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.
Your last patch [1] didn't make use of the managed device API (devm)
either, I didn't quite get to the bottom of that...
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.
It's just that the code is cleaner that way...
[1] http://paste.debian.net/683630/
Best regards
Uwe
MBR, Sergei
Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
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?
> + }
> +
> 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
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/ |
Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
On 05/10/2016 12:11 PM, Sergei Shtylyov wrote:
> Hello.
>
> On 05/10/2016 09:32 PM, Florian Fainelli wrote:
>
>>> 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 as it made use of the reset GPIO for its own purposes...
>>>
>>> Signed-off-by: Sergei Shtylyov
>>
>> This looks good to me:
>>
>> Acked-by: Florian Fainelli
>
>Thank you! I'll send v3 without [RFT] then.
>
>> Can you follow up with changes in phy_{suspend,resume}
>
>I'm not sure what changes you mean -- powering down the PHYs?
Yes, powering down, conversely up the PHY. The whole point of putting
this in PHYLIB is to be able to perform things like that. We do not need
this right now, but it would be nice if we saw that materialize at some
point.
--
Florian
Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
Hello.
On 05/10/2016 09:32 PM, Florian Fainelli wrote:
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 as it made use of the reset GPIO for its own purposes...
Signed-off-by: Sergei Shtylyov
This looks good to me:
Acked-by: Florian Fainelli
Thank you! I'll send v3 without [RFT] then.
Can you follow up with changes in phy_{suspend,resume}
I'm not sure what changes you mean -- powering down the PHYs?
if that is also
an use case that you have?
No, I'm not into power management.
MBR, Sergei
Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
On 04/28/2016 03:12 PM, Sergei Shtylyov wrote:
> 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 as it made use of the reset GPIO for its own purposes...
>
> Signed-off-by: Sergei Shtylyov
This looks good to me:
Acked-by: Florian Fainelli
Can you follow up with changes in phy_{suspend,resume} if that is also
an use case that you have?
--
Florian
Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
On Fri, Apr 29, 2016 at 01:12:54AM +0300, Sergei Shtylyov wrote:
> 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 as it made use of the reset GPIO for its own purposes...
>
> Signed-off-by: Sergei Shtylyov
>
> ---
> Changes in version 2:
> - reformatted the changelog;
> - resolved rejects, refreshed the patch.
>
> Documentation/devicetree/bindings/net/phy.txt |2 +
> Documentation/devicetree/bindings/net/phy.txt |2 +
Acked-by: Rob Herring
> drivers/net/phy/at803x.c | 19 ++
> drivers/net/phy/mdio_bus.c|4 +++
> drivers/net/phy/mdio_device.c | 27 +++--
> drivers/net/phy/phy_device.c | 33
> --
> drivers/of/of_mdio.c | 16
> include/linux/mdio.h |3 ++
> include/linux/phy.h |5 +++
> 8 files changed, 89 insertions(+), 20 deletions(-)
>
> Index: net-next/Documentation/devicetree/bindings/net/phy.txt
> ===
> --- 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 {
> 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");
>
> struct at803x_priv {
> bool phy_reset:1;
> - struct gpio_desc *gpiod_reset;
> };
>
> struct at803x_context {
> @@ -271,22 +270,10 @@ static int at803x_probe(struct phy_devic
> {
> 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;
> -
> - 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;
> @@ -361,14 +348,14 @@ static void at803x_link_change_notify(st
>*/
> if (phydev->drv->phy_id == ATH8030_PHY_ID) {
> if (phydev->state == PHY_NOLINK) {
> - if (priv->gpiod_reset && !priv->phy_reset) {
> + if (phydev->mdio.reset && !priv->phy_reset) {
> struct at803x_context context;
>
> at803x_context_save(phydev, &context);
>
> - gpiod_set_value(priv->gpiod_reset, 1);
> + phy_device_reset(phydev, 1);
> msleep(1);
> - gpiod_set_value(priv->gpiod_reset, 0);
> + phy_device_reset(phydev, 0);
> msleep(1);
>
> at803x_context_restore(phydev, &context);
> Index: net-next/drivers/net/phy/mdio_bus.c
> ===
> --- net-next.orig/drivers/net/phy/mdio_bus.c
> +++ net-next/drivers/net/phy/mdio_bus.c
> @@ -35,6 +35,7 @@
> #include
> #include
> #include
> +#include
>
> #include
>
> @@ -371,6 +372,9 @@ void mdiobus_unregister(struct mii_bus *
> if (!mdiodev)
> continue;
>
> + if (mdiodev->reset)
> + gpiod_put(mdiodev->reset);
> +
> mdiodev->device_remove(mdiodev);
> mdiodev->device_free(mdiodev);
> }
> Index: net-next/drivers/net/phy/mdio_device.c
[PATCH RFT 1/2] phylib: add device reset GPIO support
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 as it made use of the reset GPIO for its own purposes...
Signed-off-by: Sergei Shtylyov
---
Changes in version 2:
- reformatted the changelog;
- resolved rejects, refreshed the patch.
Documentation/devicetree/bindings/net/phy.txt |2 +
Documentation/devicetree/bindings/net/phy.txt |2 +
drivers/net/phy/at803x.c | 19 ++
drivers/net/phy/mdio_bus.c|4 +++
drivers/net/phy/mdio_device.c | 27 +++--
drivers/net/phy/phy_device.c | 33 --
drivers/of/of_mdio.c | 16
include/linux/mdio.h |3 ++
include/linux/phy.h |5 +++
8 files changed, 89 insertions(+), 20 deletions(-)
Index: net-next/Documentation/devicetree/bindings/net/phy.txt
===
--- 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 {
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");
struct at803x_priv {
bool phy_reset:1;
- struct gpio_desc *gpiod_reset;
};
struct at803x_context {
@@ -271,22 +270,10 @@ static int at803x_probe(struct phy_devic
{
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;
-
- 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;
@@ -361,14 +348,14 @@ static void at803x_link_change_notify(st
*/
if (phydev->drv->phy_id == ATH8030_PHY_ID) {
if (phydev->state == PHY_NOLINK) {
- if (priv->gpiod_reset && !priv->phy_reset) {
+ if (phydev->mdio.reset && !priv->phy_reset) {
struct at803x_context context;
at803x_context_save(phydev, &context);
- gpiod_set_value(priv->gpiod_reset, 1);
+ phy_device_reset(phydev, 1);
msleep(1);
- gpiod_set_value(priv->gpiod_reset, 0);
+ phy_device_reset(phydev, 0);
msleep(1);
at803x_context_restore(phydev, &context);
Index: net-next/drivers/net/phy/mdio_bus.c
===
--- net-next.orig/drivers/net/phy/mdio_bus.c
+++ net-next/drivers/net/phy/mdio_bus.c
@@ -35,6 +35,7 @@
#include
#include
#include
+#include
#include
@@ -371,6 +372,9 @@ void mdiobus_unregister(struct mii_bus *
if (!mdiodev)
continue;
+ if (mdiodev->reset)
+ gpiod_put(mdiodev->reset);
+
mdiodev->device_remove(mdiodev);
mdiodev->device_free(mdiodev);
}
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
@@ -12,6 +12,8 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#include
+#include
+#include
#in
Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
On Mon, Apr 11, 2016 at 2:28 PM, Sergei Shtylyov
wrote:
> On 04/11/2016 10:25 PM, Rob Herring wrote:
>
>>> 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 as it made use of the reset GPIO for its own purposes...
>>
>>
>> Lots of double spaces in here. Please fix.
>
>
>Oh, it's you again! :-D
Yep, one of those picky kernel maintainers that like a bad rash just
won't go away. :)
>>> Signed-off-by: Sergei Shtylyov
>>>
>>> ---
>>> Documentation/devicetree/bindings/net/phy.txt |2 +
>>> drivers/net/phy/at803x.c | 19 ++
>>> drivers/net/phy/mdio_bus.c|4 +++
>>> drivers/net/phy/mdio_device.c | 27
>>> +++--
>>> drivers/net/phy/phy_device.c | 33
>>> --
>>> drivers/of/of_mdio.c | 16
>>> include/linux/mdio.h |3 ++
>>> include/linux/phy.h |5 +++
>>> 8 files changed, 89 insertions(+), 20 deletions(-)
>>
>>
>> [...]
>>
>>> 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 int 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 int of_mdiobus_register_phy(struc
>>> is_c45 = of_device_is_compatible(child,
>>> "ethernet-phy-ieee802.3-c45");
>>>
>>> + gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios");
>>
>>
>> Calling fwnode_* functions in a DT specific file/function? That doesn't
>> make sense.
>
>
>Really?! 8-)
>Where is a DT-only analog I wonder...
Ah, you're right. NM.
Rob
Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
On 04/11/2016 10:25 PM, Rob Herring wrote:
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 as it made use of the reset GPIO for its own purposes...
Lots of double spaces in here. Please fix.
Oh, it's you again! :-D
Signed-off-by: Sergei Shtylyov
---
Documentation/devicetree/bindings/net/phy.txt |2 +
drivers/net/phy/at803x.c | 19 ++
drivers/net/phy/mdio_bus.c|4 +++
drivers/net/phy/mdio_device.c | 27 +++--
drivers/net/phy/phy_device.c | 33
--
drivers/of/of_mdio.c | 16
include/linux/mdio.h |3 ++
include/linux/phy.h |5 +++
8 files changed, 89 insertions(+), 20 deletions(-)
[...]
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 int 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 int of_mdiobus_register_phy(struc
is_c45 = of_device_is_compatible(child,
"ethernet-phy-ieee802.3-c45");
+ gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios");
Calling fwnode_* functions in a DT specific file/function? That doesn't
make sense.
Really?! 8-)
Where is a DT-only analog I wonder...
MBR, Sergei
Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
On Sat, Apr 09, 2016 at 01:22:47AM +0300, Sergei Shtylyov wrote:
> 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 as it made use of the reset GPIO for its own purposes...
Lots of double spaces in here. Please fix.
>
> Signed-off-by: Sergei Shtylyov
>
> ---
> Documentation/devicetree/bindings/net/phy.txt |2 +
> drivers/net/phy/at803x.c | 19 ++
> drivers/net/phy/mdio_bus.c|4 +++
> drivers/net/phy/mdio_device.c | 27 +++--
> drivers/net/phy/phy_device.c | 33
> --
> drivers/of/of_mdio.c | 16
> include/linux/mdio.h |3 ++
> include/linux/phy.h |5 +++
> 8 files changed, 89 insertions(+), 20 deletions(-)
[...]
> 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 int 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 int of_mdiobus_register_phy(struc
> is_c45 = of_device_is_compatible(child,
>"ethernet-phy-ieee802.3-c45");
>
> + gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios");
Calling fwnode_* functions in a DT specific file/function? That doesn't
make sense.
> + /* Deassert the reset signal */
> + if (!IS_ERR(gpiod))
> + gpiod_direction_output(gpiod, 0);
> 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 */
> + if (!IS_ERR(gpiod))
> + gpiod_set_value(gpiod, 1);
> if (IS_ERR_OR_NULL(phy))
> return 1;
>
[PATCH RFT 1/2] phylib: add device reset GPIO support
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 as it made use of the reset GPIO for its own purposes...
Signed-off-by: Sergei Shtylyov
---
Documentation/devicetree/bindings/net/phy.txt |2 +
drivers/net/phy/at803x.c | 19 ++
drivers/net/phy/mdio_bus.c|4 +++
drivers/net/phy/mdio_device.c | 27 +++--
drivers/net/phy/phy_device.c | 33 --
drivers/of/of_mdio.c | 16
include/linux/mdio.h |3 ++
include/linux/phy.h |5 +++
8 files changed, 89 insertions(+), 20 deletions(-)
Index: net-next/Documentation/devicetree/bindings/net/phy.txt
===
--- 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 {
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");
struct at803x_priv {
bool phy_reset:1;
- struct gpio_desc *gpiod_reset;
};
struct at803x_context {
@@ -271,22 +270,10 @@ static int at803x_probe(struct phy_devic
{
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;
-
- 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;
@@ -361,14 +348,14 @@ static void at803x_link_change_notify(st
*/
if (phydev->drv->phy_id == ATH8030_PHY_ID) {
if (phydev->state == PHY_NOLINK) {
- if (priv->gpiod_reset && !priv->phy_reset) {
+ if (phydev->mdio.reset && !priv->phy_reset) {
struct at803x_context context;
at803x_context_save(phydev, &context);
- gpiod_set_value(priv->gpiod_reset, 1);
+ phy_device_reset(phydev, 1);
msleep(1);
- gpiod_set_value(priv->gpiod_reset, 0);
+ phy_device_reset(phydev, 0);
msleep(1);
at803x_context_restore(phydev, &context);
Index: net-next/drivers/net/phy/mdio_bus.c
===
--- net-next.orig/drivers/net/phy/mdio_bus.c
+++ net-next/drivers/net/phy/mdio_bus.c
@@ -35,6 +35,7 @@
#include
#include
#include
+#include
#include
@@ -371,6 +372,9 @@ void mdiobus_unregister(struct mii_bus *
if (!mdiodev)
continue;
+ if (mdiodev->reset)
+ gpiod_put(mdiodev->reset);
+
mdiodev->device_remove(mdiodev);
mdiodev->device_free(mdiodev);
}
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
@@ -12,6 +12,8 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#include
+#include
+#include
#include
#include
#include
@@ -103,6 +105,13 @@ void mdio_device_remove(struct mdio_devi
}
EXPORT_SYMBOL(mdio_device_remove);
+void mdio
