Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754452AbbGIVQy (ORCPT ); Thu, 9 Jul 2015 17:16:54 -0400 Received: from mail-pa0-f53.google.com ([209.85.220.53]:33209 "EHLO mail-pa0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753847AbbGIVQv (ORCPT ); Thu, 9 Jul 2015 17:16:51 -0400 Message-ID: <559EE45C.4040408@gmail.com> Date: Thu, 09 Jul 2015 14:15:08 -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 CC: Linux kernel , Sebastien Rannou , Arnaud Ebalard , Stas Sergeev , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Grant Likely , devicetree@vger.kernel.org, netdev Subject: Re: [PATCH 1/2] of_mdio: add new DT property 'link' for fixed-link References: <559EB0A4.5080101@list.ru> <559EB1A6.1090008@list.ru> <559EBC59.6020003@gmail.com> <559EDD0C.7020907@list.ru> In-Reply-To: <559EDD0C.7020907@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: 4553 Lines: 106 On 09/07/15 13:43, Stas Sergeev wrote: > 09.07.2015 21:24, Florian Fainelli пишет: >> (there is no such thing as linux-net@vger.kernel.org, please remove it >> from your future submissions). >> >> On 09/07/15 10:38, Stas Sergeev wrote: >>> Currently for fixed-link the link state is always set to UP. >> Not quite true, this is always a driver decision to make. > But what about this part of of_mdio.c:of_phy_register_fixed_link(): > --- > > fixed_link_node = of_get_child_by_name(np, "fixed-link"); > if (fixed_link_node) { > status.link = 1 > > --- This seems like a logical consequence of finding a "fixed-link" property for the DT node of interest. If no such property exist, then we do not set anything. > >>> This patch introduces the new property 'link' that accepts the >>> following string arguments: "up", "down" and "auto". >>> "down" may be needed if the link is physically unconnected. >> In which case you probably do not even care about inserting such a >> property in the first place, do you? What would be the value of forcibly >> having a link permanently down (not counting loopback)? > The DTs have a common parts that are included by other > parts. So if you include the definition of your SoC that have > all ethernets defined, and you only set up the external things > like PHYs, then I would see a potential use for "down". "down" is equivalent to using a status = "disabled", in fact the latter is much better since you can even conserve energy and resources by not enabling something which is not usable. > Other than that, it is probably not a big deal. > Please note that I haven't even hard-coded it anywhere: > whatever is not "up" or "auto", is down. > I can remove it from the description if you think that way, > but I'd rather leave it for consistency and for a small but > possible use. Eg my board has 4 ethernets and only 2 are > connected. I feel its right to include the SoC definition and > set the unconnected ones to "down", but other approaches > are possible too. > Should I remove it? What you describe about your board is the perfect example of how a "status" property should be used. > >>> "auto" is needed to enable the link paramaters auto-negotiation, >>> that is built into some MII protocols, namely SGMII. >> RGMII also has an in-band status FWIW. > Thanks, will take that into account in v2. > >>> The appropriate documentation is added and explicitly states that >>> "auto" is very specific (protocol, HW and driver-specific), and >>> is therefore should be used with care. >> And therefore probably be made a device (and driver) specific decision >> whether this is the right thing to do. > This doesn't work. > It appears even if the driver supports it and wants to use it, the > PHY HW may simply not generate the inband status. This is actually > the whole point why we have a regression now. It is _currently_ > a driver decision, and that doesn't work for some people. > The point of this patch set is to make it a DT decision instead. Then, if the in-band status indication is not reliable (which really should be completely understood), you can just ignore the in-band status and use all the parameter in a 'fixed-link' property, should not we? If in-band status can be used, then you can decide this with a separate property which is not in 'fixed-link', would that seem reasonable? > >>> - return -EINVAL; >>> + if (of_property_read_u32(fixed_link_node, "speed", >>> + &status.speed) != 0) { >>> + /* in auto mode just set to some sane value: >>> + * it will be changed by MAC later */ >>> + if (link_auto) >>> + status.speed = 1000; >> This is a completely arbitrary speed, that does not more or less sense >> than defaulting to 100 or anything else, > Exactly. > But if I leave it to 0, then fixed-phy driver will return an error, > so I took an arbitrary value. > But if it obscures the code, I'll hack fixed-phy to accept 0 instead, > to get something cleaner. So in v2. > >> a driver should be able to set >> the speed it wants, based on the parsing of a 'phy-mode' property for >> instance. > It actually does, that value is just to "cheat" fixed-phy. > I'll make things more obvious next time. -- 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/