Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754605AbbGIUok (ORCPT ); Thu, 9 Jul 2015 16:44:40 -0400 Received: from smtp34.i.mail.ru ([94.100.177.94]:49658 "EHLO smtp34.i.mail.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754569AbbGIUoO (ORCPT ); Thu, 9 Jul 2015 16:44:14 -0400 X-Greylist: delayed 11116 seconds by postgrey-1.27 at vger.kernel.org; Thu, 09 Jul 2015 16:44:13 EDT Subject: Re: [PATCH 1/2] of_mdio: add new DT property 'link' for fixed-link To: Florian Fainelli References: <559EB0A4.5080101@list.ru> <559EB1A6.1090008@list.ru> <559EBC59.6020003@gmail.com> 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 From: Stas Sergeev Message-ID: <559EDD0C.7020907@list.ru> Date: Thu, 9 Jul 2015 23:43:56 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.0.1 MIME-Version: 1.0 In-Reply-To: <559EBC59.6020003@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam: Not detected X-Mras: Ok Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3504 Lines: 79 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 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". 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? >> "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. >> - 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. -- 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/