net: enable inband link state negotiation only when explicitly requested

PATCH In review Stas Sergeev 2 participants 14 messages Jul 16, 2015 – Jul 18, 2015

Subsystems (6)

Networking Network drivers Device tree bindings Ethernet PHY +2 more subsystems

lore

Share & tools

lore

Stas Sergeev

author

2015-07-16 14:49 UTC #0 lore raw

v4 cover net: enable inband link state negotiation only when explicitly requested

Hello.

Currently the link status auto-negotiation is enabled for any SGMII link with fixed-link DT binding.
The regression was reported:
https://lkml.org/lkml/2015/7/8/865
Apparently not all HW that implements SGMII protocol, generates the inband status for the auto-negotiation to work.
More details here:
https://lkml.org/lkml/2015/7/10/206

The following patches reverts to the old behavior by default, which is to not enable the auto-negotiation for fixed-link. The new DT property is added that allows to explicitly request the auto-negotiation.

Those who were affected by the change, please send your Tested-by, Thanks!

Stas Sergeev

author

2015-07-16 14:50 UTC #1 lore raw

PATCH v4 1/3 fixed_phy: handle link-down case

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 <[email protected]>

CC Florian Fainelli <[email protected]>

CC [email protected]

---
 drivers/net/phy/fixed_phy.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

drivers/net/phy/fixed_phy.c +5 -3

index 1960b46..479b93f 100644
@@ -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;

Signature · 1 line

1.9.1

Florian Fainelli

2015-07-16 23:25 UTC #2 lore raw

Re: [PATCH 1/3] fixed_phy: handle link-down case

Quoted 5 lines · Stas Sergeev

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?

Quoted 42 lines

Signed-off-by Stas Sergeev <[email protected]>

CC Florian Fainelli <[email protected]>

CC [email protected]

---
 drivers/net/phy/fixed_phy.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

drivers/net/phy/fixed_phy.c +5 -3

index 1960b46..479b93f 100644
@@ -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;

Signature · 1 line

Florian

Stas Sergeev

author

2015-07-17 11:26 UTC #3 lore raw

Re: [PATCH 1/3] fixed_phy: handle link-down case

17.07.2015 02:25, Florian Fainelli пишет:

Quoted 7 lines · Stas Sergeev

Quoted 5 lines
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.

Quoted 5 lines

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.

Florian Fainelli

2015-07-17 18:50 UTC #4 lore raw

Re: [PATCH 1/3] fixed_phy: handle link-down case

Quoted 11 lines · Stas Sergeev

17.07.2015 02:25, Florian Fainelli пишет:
Quoted 10 lines · Stas Sergeev
Quoted 5 lines
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.

Quoted 6 lines

Quoted 2 lines
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.

Quoted 3 lines

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

Signature · 1 line

Florian

Stas Sergeev

author

2015-07-17 20:03 UTC #5 lore raw

Re: [PATCH 1/3] fixed_phy: handle link-down case

17.07.2015 21:50, Florian Fainelli пишет:

Quoted 12 lines · Stas Sergeev

Quoted 11 lines
17.07.2015 02:25, Florian Fainelli пишет:
Quoted 10 lines · Stas Sergeev
Quoted 4 lines
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?

Quoted 7 lines

Quoted 4 lines
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.

Quoted 3 lines

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?

Quoted 9 lines

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.
Quoted 1 line
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.

Quoted 1 line

- 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.

Quoted 3 lines

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?

Florian Fainelli

2015-07-17 22:01 UTC #6 lore raw

Re: [PATCH 1/3] fixed_phy: handle link-down case

Quoted 17 lines · Stas Sergeev

17.07.2015 21:50, Florian Fainelli пишет:
Quoted 16 lines · Stas Sergeev
Quoted 15 lines
17.07.2015 02:25, Florian Fainelli пишет:
Quoted 10 lines · Stas Sergeev
Quoted 4 lines
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).

Quoted 6 lines

Quoted 3 lines
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?

Quoted 8 lines

Quoted 4 lines
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.

Quoted 10 lines

Quoted 10 lines
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.
Quoted 1 line
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

Quoted 7 lines

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.
Quoted 1 line
- 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.

Quoted 6 lines

Quoted 4 lines
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.

Signature · 1 line

Florian

Stas Sergeev

author

2015-07-17 23:24 UTC #7 lore raw

Re: [PATCH 1/3] fixed_phy: handle link-down case

18.07.2015 01:01, Florian Fainelli пишет:

Quoted 18 lines · Stas Sergeev

Quoted 17 lines
17.07.2015 21:50, Florian Fainelli пишет:
Quoted 16 lines · Stas Sergeev
Quoted 15 lines
17.07.2015 02:25, Florian Fainelli пишет:
Quoted 10 lines · Stas Sergeev
Quoted 4 lines
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).

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
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.

Quoted 2 lines

and we end-up returning
-EINVAL from of_phy_register_fixed_link(), is that what is happening?

Yes.

Quoted 1 line

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.

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 7 lines

Quoted 7 lines
Quoted 3 lines
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?

Quoted 5 lines

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.

Quoted 1 line

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.

Florian Fainelli

2015-07-17 23:35 UTC #8 lore raw

Re: [PATCH 1/3] fixed_phy: handle link-down case

Quoted 26 lines · Stas Sergeev

18.07.2015 01:01, Florian Fainelli пишет:
Quoted 25 lines · Stas Sergeev
Quoted 24 lines
17.07.2015 21:50, Florian Fainelli пишет:
Quoted 23 lines · Stas Sergeev
Quoted 11 lines
17.07.2015 02:25, Florian Fainelli пишет:
Quoted 10 lines · Stas Sergeev
Quoted 5 lines
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.
Quoted 6 lines
Quoted 2 lines
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

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
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?

Quoted 38 lines

Quoted 3 lines
and we end-up returning
-EINVAL from of_phy_register_fixed_link(), is that what is happening?
Yes.
Quoted 1 line
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.
Quoted 3 lines
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.
Quoted 3 lines
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.
Quoted 3 lines
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.
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
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?

Quoted 8 lines

Quoted 5 lines
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.

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 8 lines

Quoted 2 lines
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.

Signature · 1 line

Florian

Stas Sergeev

author

2015-07-17 23:53 UTC #9 lore raw

Re: [PATCH 1/3] fixed_phy: handle link-down case

18.07.2015 02:35, Florian Fainelli пишет:

Quoted 30 lines · Stas Sergeev

Quoted 29 lines
18.07.2015 01:01, Florian Fainelli пишет:
Quoted 28 lines · Stas Sergeev
Quoted 27 lines
17.07.2015 21:50, Florian Fainelli пишет:
Quoted 18 lines · Stas Sergeev
Quoted 11 lines
17.07.2015 02:25, Florian Fainelli пишет:
Quoted 10 lines · Stas Sergeev
Quoted 5 lines
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.
Quoted 6 lines
Quoted 2 lines
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).
Quoted 5 lines
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.

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
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.

Quoted 5 lines

Quoted 3 lines
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. :)

Quoted 6 lines

Quoted 3 lines
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?

Florian Fainelli

2015-07-18 02:29 UTC #10 lore raw

Re: [PATCH 1/3] fixed_phy: handle link-down case

Quoted 53 lines · Stas Sergeev

18.07.2015 02:35, Florian Fainelli пишет:
Quoted 33 lines · Stas Sergeev
Quoted 32 lines
18.07.2015 01:01, Florian Fainelli пишет:
Quoted 31 lines · Stas Sergeev
Quoted 19 lines
17.07.2015 21:50, Florian Fainelli пишет:
Quoted 18 lines · Stas Sergeev
Quoted 17 lines
17.07.2015 02:25, Florian Fainelli пишет:
Quoted 6 lines · Stas Sergeev
Quoted 5 lines
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.
Quoted 6 lines
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).
Quoted 8 lines
Quoted 5 lines
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,
Quoted 6 lines
Quoted 2 lines
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:
---
(quoted diff) +8 -1
+    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;
+        }
+    }
 
---

Quoted 2 lines

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.

Quoted 10 lines

Quoted 6 lines
Quoted 4 lines
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.

Quoted 11 lines

Quoted 8 lines
Quoted 5 lines
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.

Signature · 1 line

Florian

Stas Sergeev

author

2015-07-18 21:16 UTC #11 lore raw

Re: [PATCH 1/3] fixed_phy: handle link-down case

18.07.2015 05:29, Florian Fainelli пишет:

Quoted 53 lines · Stas Sergeev

Quoted 52 lines
18.07.2015 02:35, Florian Fainelli пишет:
Quoted 33 lines · Stas Sergeev
Quoted 32 lines
18.07.2015 01:01, Florian Fainelli пишет:
Quoted 31 lines · Stas Sergeev
Quoted 19 lines
17.07.2015 21:50, Florian Fainelli пишет:
Quoted 18 lines · Stas Sergeev
Quoted 17 lines
17.07.2015 02:25, Florian Fainelli пишет:
Quoted 6 lines · Stas Sergeev
Quoted 5 lines
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.
Quoted 6 lines
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).
Quoted 8 lines
Quoted 5 lines
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,
Quoted 6 lines
Quoted 2 lines
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:
---
(quoted diff) +8 -0
+    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;
+        }
+    }

Quoted 7 lines

Quoted 3 lines
---
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.

Quoted 1 line

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.

Stas Sergeev

author

2015-07-16 14:52 UTC #12 lore raw

PATCH v4 2/3 of_mdio: add new DT property 'managed' to specify the PHY management type

In reply to Stas Sergeev

Currently the PHY management type is selected by the MAC driver arbitrary. The decision is based on the presence of the "fixed-link" node and on a will of the driver's authors.
This caused a regression recently, when mvneta driver suddenly started to use the in-band status for auto-negotiation on fixed links.
It appears the auto-negotiation may not work when expected by the MAC driver. Sebastien Rannou explains:
<< Yes, I confirm that my HW does not generate an in-band status. AFAIK, it's a PHY that aggregates 4xSGMIIs to 1xQSGMII ; the MAC side of the PHY (with inband status) is connected to the switch through QSGMII, and in this context we are on the media side of the PHY. >>
https://lkml.org/lkml/2015/7/10/206

This patch introduces the new string property 'managed' that allows the user to set the management type explicitly.
The supported values are:
"auto" - default. Uses either MDIO or nothing, depending on the presence of the fixed-link node
"in-band-status" - use in-band status

Signed-off-by Stas Sergeev <[email protected]>

CC Rob Herring <[email protected]>

CC Pawel Moll <[email protected]>

CC Mark Rutland <[email protected]>

CC Ian Campbell <[email protected]>

CC Kumar Gala <[email protected]>

CC Florian Fainelli <[email protected]>

CC Grant Likely <[email protected]>

CC [email protected]

---
 Documentation/devicetree/bindings/net/ethernet.txt |  4 ++++
 drivers/of/of_mdio.c                               | 19 +++++++++++++++++--
 2 files changed, 21 insertions(+), 2 deletions(-)

Documentation/devicetree/bindings/net/ethernet.txt +4 -0

index 3fc3605..cb115a3 100644
@@ -19,7 +19,11 @@ The following properties are common to the Ethernet controllers:
 - phy: the same as "phy-handle" property, not recommended for new bindings.
 - phy-device: the same as "phy-handle" property, not recommended for new
   bindings.
+- managed: string, specifies the PHY management type. Supported values are:
+  "auto", "in-band-status". "auto" is the default, it usess MDIO for
+  management if fixed-link is not specified.
 
 Child nodes of the Ethernet controller are typically the individual PHY devices
 connected via the MDIO bus (sometimes the MDIO bus controller is separate).
 They are described in the phy.txt file in this same directory.
+For non-MDIO PHY management see fixed-link.txt.

drivers/of/of_mdio.c +17 -2

index 1bd4305..5dc1ef95 100644
@@ -262,7 +262,8 @@ EXPORT_SYMBOL(of_phy_attach);
 bool of_phy_is_fixed_link(struct device_node *np)
 {
 	struct device_node *dn;
-	int len;
+	int len, err;
+	const char *managed;
 
 	/* New binding */
 	dn = of_get_child_by_name(np, "fixed-link");

@@ -271,6 +272,10 @@ bool of_phy_is_fixed_link(struct device_node *np)
 		return true;
 	}
 
+	err = of_property_read_string(np, "managed", &managed);
+	if (err == 0 && strcmp(managed, "auto") != 0)
+		return true;
+
 	/* Old binding */
 	if (of_get_property(np, "fixed-link", &len) &&
 	    len == (5 * sizeof(__be32)))

@@ -285,8 +290,18 @@ int of_phy_register_fixed_link(struct device_node *np)
 	struct fixed_phy_status status = {};
 	struct device_node *fixed_link_node;
 	const __be32 *fixed_link_prop;
-	int len;
+	int len, err;
 	struct phy_device *phy;
+	const char *managed;
+
+	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;
+		}
+	}
 
 	/* New binding */
 	fixed_link_node = of_get_child_by_name(np, "fixed-link");

Signature · 5 lines

1.9.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stas Sergeev

author

2015-07-16 14:53 UTC #13 lore raw

PATCH v4 3/3 mvneta: use inband status only when explicitly enabled

In reply to Stas Sergeev

The commit 898b2970e2c9 ("mvneta: implement SGMII-based in-band link state signaling") implemented the link parameters auto-negotiation unconditionally. Unfortunately it appears that some HW that implements SGMII protocol, doesn't generate the inband status, so it is not possible to auto-negotiate anything with such HW.

This patch enables the auto-negotiation only if explicitly requested with the 'managed' DT property.

This patch fixes the following regression:
https://lkml.org/lkml/2015/7/8/865

Signed-off-by Stas Sergeev <[email protected]>

CC Thomas Petazzoni <[email protected]>

CC [email protected]

---
 drivers/net/ethernet/marvell/mvneta.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

drivers/net/ethernet/marvell/mvneta.c +5 -4

index 74176ec..7a1deee 100644
@@ -3008,8 +3008,8 @@ static int mvneta_probe(struct platform_device *pdev)
 	const char *dt_mac_addr;
 	char hw_mac_addr[ETH_ALEN];
 	const char *mac_from;
+	const char *managed;
 	int phy_mode;
-	int fixed_phy = 0;
 	int err;
 
 	/* Our multiqueue support is not complete, so for now, only

@@ -3043,7 +3043,6 @@ static int mvneta_probe(struct platform_device *pdev)
 			dev_err(&pdev->dev, "cannot register fixed PHY\n");
 			goto err_free_irq;
 		}
-		fixed_phy = 1;
 
 		/* In the case of a fixed PHY, the DT node associated
 		 * to the PHY is the Ethernet MAC DT node.

@@ -3067,8 +3066,10 @@ static int mvneta_probe(struct platform_device *pdev)
 	pp = netdev_priv(dev);
 	pp->phy_node = phy_node;
 	pp->phy_interface = phy_mode;
-	pp->use_inband_status = (phy_mode == PHY_INTERFACE_MODE_SGMII) &&
-				fixed_phy;
+
+	err = of_property_read_string(dn, "managed", &managed);
+	pp->use_inband_status = (err == 0 &&
+				 strcmp(managed, "in-band-status") == 0);
 
 	pp->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(pp->clk)) {

Signature · 1 line

1.9.1