Date:	Fri, 17 Jul 2015 19:29:03 -0700
From:	Florian Fainelli <f.fainelli@...il.com>
To:	Stas Sergeev <stsp@...t.ru>, netdev <netdev@...r.kernel.org>
CC:	Linux kernel <linux-kernel@...r.kernel.org>,
	Sebastien Rannou <mxs@...k.org>,
	Arnaud Ebalard <arno@...isbad.org>,
	Stas Sergeev <stsp@...rs.sourceforge.net>
Subject: 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 majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists