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

[Stas Sergeev <stsp@list.ru>]

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.