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 linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Reply via email to