Re: [PATCH 1/3] fixed_phy: handle link-down case
18.07.2015 05:29, Florian Fainelli пишет:
Le 07/17/15 16:53, Stas Sergeev a écrit :
18.07.2015 02:35, Florian Fainelli пишет:
On 17/07/15 16:24, Stas Sergeev wrote:
18.07.2015 01:01, Florian Fainelli пишет:
On 17/07/15 13:03, Stas Sergeev wrote:
17.07.2015 21:50, Florian Fainelli пишет:
On 17/07/15 04:26, Stas Sergeev wrote:
17.07.2015 02:25, Florian Fainelli пишет:
On 16/07/15 07:50, Stas Sergeev wrote:
Currently fixed_phy driver recognizes only the link-up state.
This simple patch adds an implementation of link-down state.
It fixes the status registers when link is down, and also allows
to register the fixed-phy with link down without specifying the
speed.
This patch still breaks my setups here, e.g:
drivers/net/dsa/bcm_sf2.c,
but I will look into it.
Do we really need this for now for your two other patches to work
properly, or is it just nicer to have?
Yes, absolutely.
Otherwise registering fixed phy will return -EINVAL
because of the missing link speed (even though the link
is down).
Ok, I see the problem that you have now. Arguably you could say that
according to the fixed-link binding, speed needs to be specified and
the
code correctly errors out with such an error if you do not specify
it. I
Aren't you missing the fact that .link=0?
I think what you say is true only for the link-up case, no?
.speed==0 is valid for link-down IMHO: no link - zero speed.
Pardon me being very dense and stupid here, but your problem is that
the
speed parameter is not specified in your DT,
Not even a fixed-link at all, since the latest patches.
I removed fixed-link defs from my DT.
Hummm, okay, so you just have the inband-status property and that's it,
not even a fixed-link node anymore, right? How does
mvneta_fixed_link_update() work then since it needs a fixed PHY to be
registered?
You can see it from my patch:
---
+err = of_property_read_string(np, managed, managed);
+if (err == 0) {
+if (strcmp(managed, in-band-status) == 0) {
+/* status is zeroed, namely its .link member */
+phy = fixed_phy_register(PHY_POLL, status, np);
+return IS_ERR(phy) ? PTR_ERR(phy) : 0;
+}
+}
---
which is the hunk added to the of_phy_register_fixed_link().
So in that case we register fixed-phy, but do not parse the fixed-link.
Ok, I missed that part. Could not you just override everything that is
needed here to get past the point where you register your fixed PHY even
with link = 0, this will be discarded anyway once you start in-band
negotiation.
Maybe my English is bad, but I have problems understanding
some of your senteneces. What do you mean?
If you meant to re-use the existing registration code instead
of adding a new hunk, please note that there is no fixed-link
node at all, so we do not even enter the parsing code block.
As such, there is nothing to override.
I will work on something anyway.
Thanks, hope to hear from you soon.
This stream of regressions is disturbing. :)
Should finally be fixed for real.
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] fixed_phy: handle link-down case
17.07.2015 02:25, Florian Fainelli пишет: On 16/07/15 07:50, Stas Sergeev wrote: Currently fixed_phy driver recognizes only the link-up state. This simple patch adds an implementation of link-down state. It fixes the status registers when link is down, and also allows to register the fixed-phy with link down without specifying the speed. This patch still breaks my setups here, e.g: drivers/net/dsa/bcm_sf2.c, but I will look into it. Do we really need this for now for your two other patches to work properly, or is it just nicer to have? Yes, absolutely. Otherwise registering fixed phy will return -EINVAL because of the missing link speed (even though the link is down). Please, see what makes a problem. I can't reproduce what you report. -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] fixed_phy: handle link-down case
On 17/07/15 04:26, Stas Sergeev wrote: 17.07.2015 02:25, Florian Fainelli пишет: On 16/07/15 07:50, Stas Sergeev wrote: Currently fixed_phy driver recognizes only the link-up state. This simple patch adds an implementation of link-down state. It fixes the status registers when link is down, and also allows to register the fixed-phy with link down without specifying the speed. This patch still breaks my setups here, e.g: drivers/net/dsa/bcm_sf2.c, but I will look into it. Do we really need this for now for your two other patches to work properly, or is it just nicer to have? Yes, absolutely. Otherwise registering fixed phy will return -EINVAL because of the missing link speed (even though the link is down). Ok, I see the problem that you have now. Arguably you could say that according to the fixed-link binding, speed needs to be specified and the code correctly errors out with such an error if you do not specify it. I also agree that having to specify speed and duplex for something you will end-up auto-negotiating has no useful purpose. Please, see what makes a problem. I can't reproduce what you report. So is different is that I use a link_update callback, and so we rely on at least one call of this function to initialize the hardware in drivers/net/dsa/bcm_sf2.c for this to work, after that, the hardware reflects the fixed link parameters we configured, and we feed the fixed_phy_status information from the hardware directly. From there I see two different ways to fix this: - we ignore the fixed_phy_update_regs return value in fixed_phy_add(), but that will make us avoid doing verification on the speed, which is not so great, but is essentially what your patch does anyway - we update the use of the fixed PHY link_update in drivers using it and convert them to use fixed_phy_update_state instead, which can take some time and effort to convert What do you think? I would go with option 1 and eventually introduce a special switch() case on the speed settings just to validate we know them. Thanks -- Florian -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] fixed_phy: handle link-down case
Le 07/17/15 16:53, Stas Sergeev a écrit :
18.07.2015 02:35, Florian Fainelli пишет:
On 17/07/15 16:24, Stas Sergeev wrote:
18.07.2015 01:01, Florian Fainelli пишет:
On 17/07/15 13:03, Stas Sergeev wrote:
17.07.2015 21:50, Florian Fainelli пишет:
On 17/07/15 04:26, Stas Sergeev wrote:
17.07.2015 02:25, Florian Fainelli пишет:
On 16/07/15 07:50, Stas Sergeev wrote:
Currently fixed_phy driver recognizes only the link-up state.
This simple patch adds an implementation of link-down state.
It fixes the status registers when link is down, and also allows
to register the fixed-phy with link down without specifying the
speed.
This patch still breaks my setups here, e.g:
drivers/net/dsa/bcm_sf2.c,
but I will look into it.
Do we really need this for now for your two other patches to work
properly, or is it just nicer to have?
Yes, absolutely.
Otherwise registering fixed phy will return -EINVAL
because of the missing link speed (even though the link
is down).
Ok, I see the problem that you have now. Arguably you could say that
according to the fixed-link binding, speed needs to be specified and
the
code correctly errors out with such an error if you do not specify
it. I
Aren't you missing the fact that .link=0?
I think what you say is true only for the link-up case, no?
.speed==0 is valid for link-down IMHO: no link - zero speed.
Pardon me being very dense and stupid here, but your problem is that
the
speed parameter is not specified in your DT,
Not even a fixed-link at all, since the latest patches.
I removed fixed-link defs from my DT.
Hummm, okay, so you just have the inband-status property and that's it,
not even a fixed-link node anymore, right? How does
mvneta_fixed_link_update() work then since it needs a fixed PHY to be
registered?
You can see it from my patch:
---
+err = of_property_read_string(np, managed, managed);
+if (err == 0) {
+if (strcmp(managed, in-band-status) == 0) {
+/* status is zeroed, namely its .link member */
+phy = fixed_phy_register(PHY_POLL, status, np);
+return IS_ERR(phy) ? PTR_ERR(phy) : 0;
+}
+}
---
which is the hunk added to the of_phy_register_fixed_link().
So in that case we register fixed-phy, but do not parse the fixed-link.
Ok, I missed that part. Could not you just override everything that is
needed here to get past the point where you register your fixed PHY even
with link = 0, this will be discarded anyway once you start in-band
negotiation.
AFAIK when link is down, you are not allowed to rely on the PHY
status registers to read speed from, or am I wrong? So if my
understanding is correct, this was working by a pure luck.
Well, it's more like it is undefined, and before this patch, the fixed
PHY would update everything except the link status indication.
And what about the real MDIO PHY? Or does it never hit this
undefined code path?
Anyway, if you call it undefined, I guess you automatically agree
this needs to be fixed. :)
I should have been clearer; it is undefined for real PHYs it was not for
fixed PHYs, you can rely on the configuration that was done during
registration. Maybe not the best assumption; but it worked, and with
this patch it no longer works, so we want to find something here.
As for the quick fix - why not to do this pre-init in
fixed_link_update()
instead of adjust_link()? In fixed_link_update() you'll get the speed
right from DT, so it will be correct.
fixed_link_update() only gets called once you start your PHY state
machine, unfortunately, not upon fixed PHY device registration, and it
runs before your adjust_link callback does,
So you say fixed_link_update() runs before adjust_link callback does,
which looks logical. Why would you need it to run on device registration,
if it runs earlier than adjust_link (which you use for init) even now?
There could be multiple reasons:
- device might be clock gated, until you open it you cannot
necessarily start making register accesses
- interfaces can be brought up/down separately so you want to stop the
PHY state machine accordingly
I will work on something anyway.
--
Florian
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] fixed_phy: handle link-down case
On 17/07/15 13:03, Stas Sergeev wrote: 17.07.2015 21:50, Florian Fainelli пишет: On 17/07/15 04:26, Stas Sergeev wrote: 17.07.2015 02:25, Florian Fainelli пишет: On 16/07/15 07:50, Stas Sergeev wrote: Currently fixed_phy driver recognizes only the link-up state. This simple patch adds an implementation of link-down state. It fixes the status registers when link is down, and also allows to register the fixed-phy with link down without specifying the speed. This patch still breaks my setups here, e.g: drivers/net/dsa/bcm_sf2.c, but I will look into it. Do we really need this for now for your two other patches to work properly, or is it just nicer to have? Yes, absolutely. Otherwise registering fixed phy will return -EINVAL because of the missing link speed (even though the link is down). Ok, I see the problem that you have now. Arguably you could say that according to the fixed-link binding, speed needs to be specified and the code correctly errors out with such an error if you do not specify it. I Aren't you missing the fact that .link=0? I think what you say is true only for the link-up case, no? .speed==0 is valid for link-down IMHO: no link - zero speed. Pardon me being very dense and stupid here, but your problem is that the speed parameter is not specified in your DT, and we end-up returning -EINVAL from of_phy_register_fixed_link(), is that what is happening? And even if we silenced that error, we would end-up calling fixed_phy_add() which would also return -EINVAL because then, we would have status.link = 1, but no speed. So I better understand what is it that you are after here, and that is also a broken Device Tree, is not it? So this was the reason why in earlier versions of the patchset you ended-up with a given speed which would make us pass this condition, right? So is different is that I use a link_update callback, and so we rely on at least one call of this function to initialize the hardware in drivers/net/dsa/bcm_sf2.c Do you mean this?: core_writel(priv, reg, CORE_STS_OVERRIDE_GMIIP_PORT(port)); Maybe just moving the HW initialization bits to some init func will be a quick fix? Well, the problem with that is that to know how we should be configuring the hardware in the adjust_link function, we need to run the link_update function first. By default, there is no auto-negotiation on these fixed links at all, so we cannot rely on any value being programmed other than those specified in DT. for this to work, after that, the hardware reflects the fixed link parameters we configured, and we feed the fixed_phy_status information from the hardware directly. From there I see two different ways to fix this: - we ignore the fixed_phy_update_regs return value in fixed_phy_add(), but that will make us avoid doing verification on the speed, which is not so great, but is essentially what your patch does anyway No, it does not. All it does is to allow no speed _when link is down_, which is IMHO a very logical fix. The speed checks for the link-up case are all still there. - we update the use of the fixed PHY link_update in drivers using it IMHO just 2 drivers: bcmii.c and bcm_sf2.c, and the change is likely trivial, although of course I am not sure in details. The changes are not trivial, it took a while to get that logic done correctly, and this would increase the number of patches to backport to -stable, which is not ideal. and convert them to use fixed_phy_update_state instead, which can take some time and effort to convert Maybe just move the initialization bits out of the link_update callback, but still use the callback for now? Should be simple, no? Let me see if I have a smart idea other the weekend on how to do this. -- Florian -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] fixed_phy: handle link-down case
18.07.2015 01:01, Florian Fainelli пишет: On 17/07/15 13:03, Stas Sergeev wrote: 17.07.2015 21:50, Florian Fainelli пишет: On 17/07/15 04:26, Stas Sergeev wrote: 17.07.2015 02:25, Florian Fainelli пишет: On 16/07/15 07:50, Stas Sergeev wrote: Currently fixed_phy driver recognizes only the link-up state. This simple patch adds an implementation of link-down state. It fixes the status registers when link is down, and also allows to register the fixed-phy with link down without specifying the speed. This patch still breaks my setups here, e.g: drivers/net/dsa/bcm_sf2.c, but I will look into it. Do we really need this for now for your two other patches to work properly, or is it just nicer to have? Yes, absolutely. Otherwise registering fixed phy will return -EINVAL because of the missing link speed (even though the link is down). Ok, I see the problem that you have now. Arguably you could say that according to the fixed-link binding, speed needs to be specified and the code correctly errors out with such an error if you do not specify it. I Aren't you missing the fact that .link=0? I think what you say is true only for the link-up case, no? .speed==0 is valid for link-down IMHO: no link - zero speed. Pardon me being very dense and stupid here, but your problem is that the speed parameter is not specified in your DT, Not even a fixed-link at all, since the latest patches. I removed fixed-link defs from my DT. and we end-up returning -EINVAL from of_phy_register_fixed_link(), is that what is happening? Yes. And even if we silenced that error, I don't agree with calling it an error silencing. To me it is a fix. It will also return a more correct status when link is down. we would end-up calling fixed_phy_add() which would also return -EINVAL because then, we would have status.link = 1, but no speed. Why link=1 and no speed? This is not valid, should never be used. The error checking is still there to prevent it. So I better understand what is it that you are after here, and that is also a broken Device Tree, is not it? I don't understand. If you didn't specify the in-band status, you _must_ set the speed. There is no broken DT in either case. So this was the reason why in earlier versions of the patchset you ended-up with a given speed which would make us pass this condition, right? As explained earlier, yes. So is different is that I use a link_update callback, and so we rely on at least one call of this function to initialize the hardware in drivers/net/dsa/bcm_sf2.c Do you mean this?: core_writel(priv, reg, CORE_STS_OVERRIDE_GMIIP_PORT(port)); Maybe just moving the HW initialization bits to some init func will be a quick fix? Well, the problem with that is that to know how we should be configuring the hardware in the adjust_link function, we need to run the link_update function first. By default, there is no auto-negotiation on these fixed links at all, so we cannot rely on any value being programmed other than those specified in DT. Ah, so is my understanding correct that in fixed_link_update() you set .link=0 and as a result get wrong speed in adjust_link(), which gets then written to init HW? AFAIK when link is down, you are not allowed to rely on the PHY status registers to read speed from, or am I wrong? So if my understanding is correct, this was working by a pure luck. As for the quick fix - why not to do this pre-init in fixed_link_update() instead of adjust_link()? In fixed_link_update() you'll get the speed right from DT, so it will be correct. The changes are not trivial, it took a while to get that logic done For a longer term fix, how about adding a *status arg to of_phy_register_fixed_link() to always get the status back to the driver, unless NULL is provided? Using an update callback for that doesn't look like the best thing to do. And besides, if we move to my fixed_phy_update_state(), this will be needed anyway. -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] fixed_phy: handle link-down case
18.07.2015 02:35, Florian Fainelli пишет:
On 17/07/15 16:24, Stas Sergeev wrote:
18.07.2015 01:01, Florian Fainelli пишет:
On 17/07/15 13:03, Stas Sergeev wrote:
17.07.2015 21:50, Florian Fainelli пишет:
On 17/07/15 04:26, Stas Sergeev wrote:
17.07.2015 02:25, Florian Fainelli пишет:
On 16/07/15 07:50, Stas Sergeev wrote:
Currently fixed_phy driver recognizes only the link-up state.
This simple patch adds an implementation of link-down state.
It fixes the status registers when link is down, and also allows
to register the fixed-phy with link down without specifying the
speed.
This patch still breaks my setups here, e.g:
drivers/net/dsa/bcm_sf2.c,
but I will look into it.
Do we really need this for now for your two other patches to work
properly, or is it just nicer to have?
Yes, absolutely.
Otherwise registering fixed phy will return -EINVAL
because of the missing link speed (even though the link
is down).
Ok, I see the problem that you have now. Arguably you could say that
according to the fixed-link binding, speed needs to be specified and
the
code correctly errors out with such an error if you do not specify
it. I
Aren't you missing the fact that .link=0?
I think what you say is true only for the link-up case, no?
.speed==0 is valid for link-down IMHO: no link - zero speed.
Pardon me being very dense and stupid here, but your problem is that the
speed parameter is not specified in your DT,
Not even a fixed-link at all, since the latest patches.
I removed fixed-link defs from my DT.
Hummm, okay, so you just have the inband-status property and that's it,
not even a fixed-link node anymore, right? How does
mvneta_fixed_link_update() work then since it needs a fixed PHY to be
registered?
You can see it from my patch:
---
+ err = of_property_read_string(np, managed, managed);
+ if (err == 0) {
+ if (strcmp(managed, in-band-status) == 0) {
+ /* status is zeroed, namely its .link member */
+ phy = fixed_phy_register(PHY_POLL, status, np);
+ return IS_ERR(phy) ? PTR_ERR(phy) : 0;
+ }
+ }
---
which is the hunk added to the of_phy_register_fixed_link().
So in that case we register fixed-phy, but do not parse the fixed-link.
AFAIK when link is down, you are not allowed to rely on the PHY
status registers to read speed from, or am I wrong? So if my
understanding is correct, this was working by a pure luck.
Well, it's more like it is undefined, and before this patch, the fixed
PHY would update everything except the link status indication.
And what about the real MDIO PHY? Or does it never hit this
undefined code path?
Anyway, if you call it undefined, I guess you automatically agree
this needs to be fixed. :)
As for the quick fix - why not to do this pre-init in fixed_link_update()
instead of adjust_link()? In fixed_link_update() you'll get the speed
right from DT, so it will be correct.
fixed_link_update() only gets called once you start your PHY state
machine, unfortunately, not upon fixed PHY device registration, and it
runs before your adjust_link callback does,
So you say fixed_link_update() runs before adjust_link callback does,
which looks logical. Why would you need it to run on device registration,
if it runs earlier than adjust_link (which you use for init) even now?
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] fixed_phy: handle link-down case
17.07.2015 21:50, Florian Fainelli пишет: On 17/07/15 04:26, Stas Sergeev wrote: 17.07.2015 02:25, Florian Fainelli пишет: On 16/07/15 07:50, Stas Sergeev wrote: Currently fixed_phy driver recognizes only the link-up state. This simple patch adds an implementation of link-down state. It fixes the status registers when link is down, and also allows to register the fixed-phy with link down without specifying the speed. This patch still breaks my setups here, e.g: drivers/net/dsa/bcm_sf2.c, but I will look into it. Do we really need this for now for your two other patches to work properly, or is it just nicer to have? Yes, absolutely. Otherwise registering fixed phy will return -EINVAL because of the missing link speed (even though the link is down). Ok, I see the problem that you have now. Arguably you could say that according to the fixed-link binding, speed needs to be specified and the code correctly errors out with such an error if you do not specify it. I Aren't you missing the fact that .link=0? I think what you say is true only for the link-up case, no? .speed==0 is valid for link-down IMHO: no link - zero speed. So is different is that I use a link_update callback, and so we rely on at least one call of this function to initialize the hardware in drivers/net/dsa/bcm_sf2.c Do you mean this?: core_writel(priv, reg, CORE_STS_OVERRIDE_GMIIP_PORT(port)); Maybe just moving the HW initialization bits to some init func will be a quick fix? for this to work, after that, the hardware reflects the fixed link parameters we configured, and we feed the fixed_phy_status information from the hardware directly. From there I see two different ways to fix this: - we ignore the fixed_phy_update_regs return value in fixed_phy_add(), but that will make us avoid doing verification on the speed, which is not so great, but is essentially what your patch does anyway No, it does not. All it does is to allow no speed _when link is down_, which is IMHO a very logical fix. The speed checks for the link-up case are all still there. - we update the use of the fixed PHY link_update in drivers using it IMHO just 2 drivers: bcmii.c and bcm_sf2.c, and the change is likely trivial, although of course I am not sure in details. and convert them to use fixed_phy_update_state instead, which can take some time and effort to convert Maybe just move the initialization bits out of the link_update callback, but still use the callback for now? Should be simple, no? -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] fixed_phy: handle link-down case
On 17/07/15 16:24, Stas Sergeev wrote: 18.07.2015 01:01, Florian Fainelli пишет: On 17/07/15 13:03, Stas Sergeev wrote: 17.07.2015 21:50, Florian Fainelli пишет: On 17/07/15 04:26, Stas Sergeev wrote: 17.07.2015 02:25, Florian Fainelli пишет: On 16/07/15 07:50, Stas Sergeev wrote: Currently fixed_phy driver recognizes only the link-up state. This simple patch adds an implementation of link-down state. It fixes the status registers when link is down, and also allows to register the fixed-phy with link down without specifying the speed. This patch still breaks my setups here, e.g: drivers/net/dsa/bcm_sf2.c, but I will look into it. Do we really need this for now for your two other patches to work properly, or is it just nicer to have? Yes, absolutely. Otherwise registering fixed phy will return -EINVAL because of the missing link speed (even though the link is down). Ok, I see the problem that you have now. Arguably you could say that according to the fixed-link binding, speed needs to be specified and the code correctly errors out with such an error if you do not specify it. I Aren't you missing the fact that .link=0? I think what you say is true only for the link-up case, no? .speed==0 is valid for link-down IMHO: no link - zero speed. Pardon me being very dense and stupid here, but your problem is that the speed parameter is not specified in your DT, Not even a fixed-link at all, since the latest patches. I removed fixed-link defs from my DT. Hummm, okay, so you just have the inband-status property and that's it, not even a fixed-link node anymore, right? How does mvneta_fixed_link_update() work then since it needs a fixed PHY to be registered? and we end-up returning -EINVAL from of_phy_register_fixed_link(), is that what is happening? Yes. And even if we silenced that error, I don't agree with calling it an error silencing. To me it is a fix. It will also return a more correct status when link is down. we would end-up calling fixed_phy_add() which would also return -EINVAL because then, we would have status.link = 1, but no speed. Why link=1 and no speed? This is not valid, should never be used. The error checking is still there to prevent it. So I better understand what is it that you are after here, and that is also a broken Device Tree, is not it? I don't understand. If you didn't specify the in-band status, you _must_ set the speed. There is no broken DT in either case. So this was the reason why in earlier versions of the patchset you ended-up with a given speed which would make us pass this condition, right? As explained earlier, yes. So is different is that I use a link_update callback, and so we rely on at least one call of this function to initialize the hardware in drivers/net/dsa/bcm_sf2.c Do you mean this?: core_writel(priv, reg, CORE_STS_OVERRIDE_GMIIP_PORT(port)); Maybe just moving the HW initialization bits to some init func will be a quick fix? Well, the problem with that is that to know how we should be configuring the hardware in the adjust_link function, we need to run the link_update function first. By default, there is no auto-negotiation on these fixed links at all, so we cannot rely on any value being programmed other than those specified in DT. Ah, so is my understanding correct that in fixed_link_update() you set .link=0 and as a result get wrong speed in adjust_link(), which gets then written to init HW? Yes, that's what happens. AFAIK when link is down, you are not allowed to rely on the PHY status registers to read speed from, or am I wrong? So if my understanding is correct, this was working by a pure luck. Well, it's more like it is undefined, and before this patch, the fixed PHY would update everything except the link status indication. As for the quick fix - why not to do this pre-init in fixed_link_update() instead of adjust_link()? In fixed_link_update() you'll get the speed right from DT, so it will be correct. fixed_link_update() only gets called once you start your PHY state machine, unfortunately, not upon fixed PHY device registration, and it runs before your adjust_link callback does, that's why starting with correct parameters is kind of important here. Of course, this could be fixed. The changes are not trivial, it took a while to get that logic done For a longer term fix, how about adding a *status arg to of_phy_register_fixed_link() to always get the status back to the driver, unless NULL is provided? Using an update callback for that doesn't look like the best thing to do. And besides, if we move to my fixed_phy_update_state(), this will be needed anyway. I agree that the link_update callback is not the best thing, it polls the hardware and comes with that problem that it may or may not have yet run to configure your fixed_phy_status appropriately. -- Florian -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a
Re: [PATCH 1/3] fixed_phy: handle link-down case
On 16/07/15 07:50, Stas Sergeev wrote:
Currently fixed_phy driver recognizes only the link-up state.
This simple patch adds an implementation of link-down state.
It fixes the status registers when link is down, and also allows
to register the fixed-phy with link down without specifying the speed.
This patch still breaks my setups here, e.g: drivers/net/dsa/bcm_sf2.c,
but I will look into it.
Do we really need this for now for your two other patches to work
properly, or is it just nicer to have?
Signed-off-by: Stas Sergeev s...@users.sourceforge.net
CC: Florian Fainelli f.faine...@gmail.com
CC: netdev@vger.kernel.org
CC: linux-ker...@vger.kernel.org
---
drivers/net/phy/fixed_phy.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
index 1960b46..479b93f 100644
--- a/drivers/net/phy/fixed_phy.c
+++ b/drivers/net/phy/fixed_phy.c
@@ -52,6 +52,10 @@ static int fixed_phy_update_regs(struct fixed_phy *fp)
u16 lpagb = 0;
u16 lpa = 0;
+ if (!fp-status.link)
+ goto done;
+ bmsr |= BMSR_LSTATUS | BMSR_ANEGCOMPLETE;
+
if (fp-status.duplex) {
bmcr |= BMCR_FULLDPLX;
@@ -96,15 +100,13 @@ static int fixed_phy_update_regs(struct fixed_phy *fp)
}
}
- if (fp-status.link)
- bmsr |= BMSR_LSTATUS | BMSR_ANEGCOMPLETE;
-
if (fp-status.pause)
lpa |= LPA_PAUSE_CAP;
if (fp-status.asym_pause)
lpa |= LPA_PAUSE_ASYM;
+done:
fp-regs[MII_PHYSID1] = 0;
fp-regs[MII_PHYSID2] = 0;
--
Florian
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] fixed_phy: handle link-down case
On 14/07/15 10:11, Stas Sergeev wrote:
Currently fixed_phy driver recognizes only the link-up state.
This simple patch adds an implementation of link-down state.
It fixes the status registers when link is down, and also allows
to register the fixed-phy with link down without specifying the speed.
Signed-off-by: Stas Sergeev s...@users.sourceforge.net
This does not quite seem to work for me here on two different setups
that use fixed PHYs:
Before patch link up:
# ethtool moca
Settings for moca:
Supported ports: [ TP AUI BNC MII FIBRE ]
Supported link modes: 1000baseT/Half 1000baseT/Full
Supported pause frame use: No
Supports auto-negotiation: Yes
Advertised link modes: 1000baseT/Half 1000baseT/Full
Advertised pause frame use: No
Advertised auto-negotiation: Yes
Link partner advertised link modes: 1000baseT/Full
Link partner advertised pause frame use: No
Link partner advertised auto-negotiation: No
Speed: 1000Mb/s
Duplex: Full
Port: BNC
PHYAD: 2
Transceiver: external
Auto-negotiation: on
Supports Wake-on: gs
Wake-on: d
SecureOn password: 00:00:00:00:00:00
Link detected: yes
#
Before patch link down:
# ethtool moca
Settings for moca:
Supported ports: [ TP AUI BNC MII FIBRE ]
Supported link modes: 1000baseT/Half 1000baseT/Full
Supported pause frame use: No
Supports auto-negotiation: Yes
Advertised link modes: 1000baseT/Half 1000baseT/Full
Advertised pause frame use: No
Advertised auto-negotiation: No
Speed: 1000Mb/s
Duplex: Full
Port: BNC
PHYAD: 2
Transceiver: external
Auto-negotiation: off
Supports Wake-on: gs
Wake-on: d
SecureOn password: 00:00:00:00:00:00
Link detected: no
#
After patch link up:
# ethtool moca
Settings for moca:
Supported ports: [ TP AUI BNC MII FIBRE ]
Supported link modes: 1000baseT/Half 1000baseT/Full
Supported pause frame use: No
Supports auto-negotiation: Yes
Advertised link modes: 1000baseT/Half 1000baseT/Full
Advertised pause frame use: No
Advertised auto-negotiation: Yes
Link partner advertised link modes: 10baseT/Full
Link partner advertised pause frame use: No
Link partner advertised auto-negotiation: No
Speed: 10Mb/s this is not quite the speed we want
Duplex: Full
Port: BNC
PHYAD: 2
Transceiver: external
Auto-negotiation: on
Supports Wake-on: gs
Wake-on: d
SecureOn password: 00:00:00:00:00:00
Link detected: yes
#
After patch link down:
# ethtool moca
Settings for moca:
Supported ports: [ TP AUI BNC MII FIBRE ]
Supported link modes: 1000baseT/Half 1000baseT/Full
Supported pause frame use: No
Supports auto-negotiation: Yes
Advertised link modes: 1000baseT/Half 1000baseT/Full
Advertised pause frame use: No
Advertised auto-negotiation: No
Speed: 10Mb/s
Duplex: Half
Port: BNC
PHYAD: 2
Transceiver: external
Auto-negotiation: off
Supports Wake-on: gs
Wake-on: d
SecureOn password: 00:00:00:00:00:00
Link detected: no
#
Does it behave properly for you?
CC: Florian Fainelli f.faine...@gmail.com
CC: netdev@vger.kernel.org
CC: linux-ker...@vger.kernel.org
---
drivers/net/phy/fixed_phy.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
index 1960b46..479b93f 100644
--- a/drivers/net/phy/fixed_phy.c
+++ b/drivers/net/phy/fixed_phy.c
@@ -52,6 +52,10 @@ static int fixed_phy_update_regs(struct fixed_phy *fp)
u16 lpagb = 0;
u16 lpa = 0;
+ if (!fp-status.link)
+ goto done;
+ bmsr |= BMSR_LSTATUS | BMSR_ANEGCOMPLETE;
+
if (fp-status.duplex) {
bmcr |= BMCR_FULLDPLX;
@@ -96,15 +100,13 @@ static int fixed_phy_update_regs(struct fixed_phy *fp)
}
}
- if (fp-status.link)
- bmsr |= BMSR_LSTATUS | BMSR_ANEGCOMPLETE;
-
if (fp-status.pause)
lpa |= LPA_PAUSE_CAP;
if (fp-status.asym_pause)
lpa |= LPA_PAUSE_ASYM;
+done:
fp-regs[MII_PHYSID1] = 0;
fp-regs[MII_PHYSID2] = 0;
--
Florian
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] fixed_phy: handle link-down case
On 10/07/15 14:14, Stas Sergeev wrote: 10.07.2015 23:44, Florian Fainelli пишет: On 10/07/15 09:41, Stas Sergeev wrote: Currently fixed_phy driver recognizes only the link-up state. This simple patch adds an implementation of link-down state. The actual change is 1-line, the rest is an indentation. It is not clear to me how this is useful, if you have a link_update callback manipulating the link state, the fixed PHY driver returns appropriate MII_BMSR values and always re-initializes everything. It returns the appropriate values only for link status (when its down), but it still returns speed, duplex etc as if the link is up. I had hard times finding the relevant specs, but from what I have googled, when link is down, the speed/duplex/etc status fields should _also_ be zero, which is what my patch does. What is more important is that fixed_phy_add() would return -EINVAL if you didn't specify speed while the link is down. This is an absolute must-fix, or I will have to add an arbitrary speed value again, on which you already objected. Ok, but that does not seems to be a code path that you can hit, unless you are already modifying drivers/of/of_mdio.c::of_fixed_phy_register_link() and overriding how status.link is defined, am I missing something? Is this meant to be some sort of optimization? If so, you could just avoid the re-intendation completely and do a goto instead? Oh, c'mon... Adding goto just to keep the _patch_ smaller? Well, yes, so it's easy to audit the changes? (not smaller code, just a smaller patch) Well, this is certainly something that can be done, feel free to request that explicitly and I'll release v3 next week. I hereby explicitly request that you make this a new iteration using a goto. Thank you. -- Florian -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] fixed_phy: handle link-down case
10.07.2015 23:44, Florian Fainelli пишет: On 10/07/15 09:41, Stas Sergeev wrote: Currently fixed_phy driver recognizes only the link-up state. This simple patch adds an implementation of link-down state. The actual change is 1-line, the rest is an indentation. It is not clear to me how this is useful, if you have a link_update callback manipulating the link state, the fixed PHY driver returns appropriate MII_BMSR values and always re-initializes everything. It returns the appropriate values only for link status (when its down), but it still returns speed, duplex etc as if the link is up. I had hard times finding the relevant specs, but from what I have googled, when link is down, the speed/duplex/etc status fields should _also_ be zero, which is what my patch does. What is more important is that fixed_phy_add() would return -EINVAL if you didn't specify speed while the link is down. This is an absolute must-fix, or I will have to add an arbitrary speed value again, on which you already objected. Is this meant to be some sort of optimization? If so, you could just avoid the re-intendation completely and do a goto instead? Oh, c'mon... Adding goto just to keep the _patch_ smaller? (not smaller code, just a smaller patch) Well, this is certainly something that can be done, feel free to request that explicitly and I'll release v3 next week. -- To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] fixed_phy: handle link-down case
On 10/07/15 09:41, Stas Sergeev wrote:
Currently fixed_phy driver recognizes only the link-up state.
This simple patch adds an implementation of link-down state.
The actual change is 1-line, the rest is an indentation.
It is not clear to me how this is useful, if you have a link_update
callback manipulating the link state, the fixed PHY driver returns
appropriate MII_BMSR values and always re-initializes everything.
Is this meant to be some sort of optimization? If so, you could just
avoid the re-intendation completely and do a goto instead?
Signed-off-by: Stas Sergeev s...@users.sourceforge.net
CC: Florian Fainelli f.faine...@gmail.com
CC: netdev@vger.kernel.org
CC: linux-ker...@vger.kernel.org
---
drivers/net/phy/fixed_phy.c | 99
+++--
1 file changed, 50 insertions(+), 49 deletions(-)
diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
index 1960b46..a5d93cf 100644
--- a/drivers/net/phy/fixed_phy.c
+++ b/drivers/net/phy/fixed_phy.c
@@ -52,58 +52,59 @@ static int fixed_phy_update_regs(struct fixed_phy *fp)
u16 lpagb = 0;
u16 lpa = 0;
- if (fp-status.duplex) {
- bmcr |= BMCR_FULLDPLX;
-
- switch (fp-status.speed) {
- case 1000:
- bmsr |= BMSR_ESTATEN;
- bmcr |= BMCR_SPEED1000;
- lpagb |= LPA_1000FULL;
- break;
- case 100:
- bmsr |= BMSR_100FULL;
- bmcr |= BMCR_SPEED100;
- lpa |= LPA_100FULL;
- break;
- case 10:
- bmsr |= BMSR_10FULL;
- lpa |= LPA_10FULL;
- break;
- default:
- pr_warn(fixed phy: unknown speed\n);
- return -EINVAL;
- }
- } else {
- switch (fp-status.speed) {
- case 1000:
- bmsr |= BMSR_ESTATEN;
- bmcr |= BMCR_SPEED1000;
- lpagb |= LPA_1000HALF;
- break;
- case 100:
- bmsr |= BMSR_100HALF;
- bmcr |= BMCR_SPEED100;
- lpa |= LPA_100HALF;
- break;
- case 10:
- bmsr |= BMSR_10HALF;
- lpa |= LPA_10HALF;
- break;
- default:
- pr_warn(fixed phy: unknown speed\n);
- return -EINVAL;
- }
- }
-
- if (fp-status.link)
+ if (fp-status.link) {
bmsr |= BMSR_LSTATUS | BMSR_ANEGCOMPLETE;
- if (fp-status.pause)
- lpa |= LPA_PAUSE_CAP;
+ if (fp-status.duplex) {
+ bmcr |= BMCR_FULLDPLX;
+
+ switch (fp-status.speed) {
+ case 1000:
+ bmsr |= BMSR_ESTATEN;
+ bmcr |= BMCR_SPEED1000;
+ lpagb |= LPA_1000FULL;
+ break;
+ case 100:
+ bmsr |= BMSR_100FULL;
+ bmcr |= BMCR_SPEED100;
+ lpa |= LPA_100FULL;
+ break;
+ case 10:
+ bmsr |= BMSR_10FULL;
+ lpa |= LPA_10FULL;
+ break;
+ default:
+ pr_warn(fixed phy: unknown speed\n);
+ return -EINVAL;
+ }
+ } else {
+ switch (fp-status.speed) {
+ case 1000:
+ bmsr |= BMSR_ESTATEN;
+ bmcr |= BMCR_SPEED1000;
+ lpagb |= LPA_1000HALF;
+ break;
+ case 100:
+ bmsr |= BMSR_100HALF;
+ bmcr |= BMCR_SPEED100;
+ lpa |= LPA_100HALF;
+ break;
+ case 10:
+ bmsr |= BMSR_10HALF;
+ lpa |= LPA_10HALF;
+ break;
+ default:
+ pr_warn(fixed phy: unknown speed\n);
+ return -EINVAL;
+ }
+ }
- if (fp-status.asym_pause)
- lpa |= LPA_PAUSE_ASYM;
+ if (fp-status.pause)
+ lpa |= LPA_PAUSE_CAP;
+
+ if (fp-status.asym_pause)
+ lpa |= LPA_PAUSE_ASYM;
+ }
fp-regs[MII_PHYSID1] = 0;
