Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753580AbbGRC3J (ORCPT ); Fri, 17 Jul 2015 22:29:09 -0400 Received: from mail-oi0-f49.google.com ([209.85.218.49]:35463 "EHLO mail-oi0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752778AbbGRC3G (ORCPT ); Fri, 17 Jul 2015 22:29:06 -0400 Message-ID: <55A9B9EF.2030308@gmail.com> Date: Fri, 17 Jul 2015 19:29:03 -0700 From: Florian Fainelli User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Stas Sergeev , netdev CC: Linux kernel , Sebastien Rannou , Arnaud Ebalard , Stas Sergeev Subject: Re: [PATCH 1/3] fixed_phy: handle link-down case References: <55A7C45F.1070501@list.ru> <55A7C49E.2020803@list.ru> <55A83D86.2030505@gmail.com> <55A8E64A.3040009@list.ru> <55A94E5A.9010104@gmail.com> <55A95F83.8010900@list.ru> <55A97B51.3030508@gmail.com> <55A98EC5.4040805@list.ru> <55A99138.2030905@gmail.com> <55A9956E.4070205@list.ru> In-Reply-To: <55A9956E.4070205@list.ru> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4755 Lines: 106 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 linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/