Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752776AbdHIIpv (ORCPT ); Wed, 9 Aug 2017 04:45:51 -0400 Received: from mail-wr0-f195.google.com ([209.85.128.195]:38627 "EHLO mail-wr0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752081AbdHIIps (ORCPT ); Wed, 9 Aug 2017 04:45:48 -0400 Date: Wed, 9 Aug 2017 10:45:41 +0200 From: Corentin Labbe To: f.fainelli@gmail.com, andrew@lunn.ch, davem@davemloft.net Cc: David Wu , Heiko =?iso-8859-1?Q?St=FCbner?= , Rob Herring , Mark Rutland , Catalin Marinas , Will Deacon , Olof Johansson , Russell King , Arnd Bergmann , Tao Huang , hwg@rock-chips.com, alexandre.torgue@st.com, devicetree , netdev , linux-kernel , "open list:ARM/Rockchip SoC..." , Giuseppe Cavallaro , linux-arm-kernel , wens@csie.org Subject: Re: [PATCH v3 05/11] net: stmmac: dwmac-rk: Add internal phy support Message-ID: <20170809084541.GB7137@Red> References: <1501654546-17292-1-git-send-email-david.wu@rock-chips.com> <1501654888-19514-1-git-send-email-david.wu@rock-chips.com> <6b1ebf64-6903-6d35-b1fc-92ec47334425@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.7.2 (2016-11-26) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4189 Lines: 83 On Thu, Aug 03, 2017 at 07:06:33PM +0800, Chen-Yu Tsai wrote: > On Thu, Aug 3, 2017 at 1:38 AM, Florian Fainelli wrote: > > On 08/01/2017 11:21 PM, David Wu wrote: > >> To make internal phy work, need to configure the phy_clock, > >> phy cru_reset and related registers. > >> > >> Signed-off-by: David Wu > >> --- > >> .../devicetree/bindings/net/rockchip-dwmac.txt | 6 +- > >> drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 81 ++++++++++++++++++++++ > >> 2 files changed, 86 insertions(+), 1 deletion(-) > >> > >> diff --git a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt > >> index 8f42755..ec39b31 100644 > >> --- a/Documentation/devicetree/bindings/net/rockchip-dwmac.txt > >> +++ b/Documentation/devicetree/bindings/net/rockchip-dwmac.txt > >> @@ -25,7 +25,8 @@ Required properties: > >> - clock-names: One name for each entry in the clocks property. > >> - phy-mode: See ethernet.txt file in the same directory. > >> - pinctrl-names: Names corresponding to the numbered pinctrl states. > >> - - pinctrl-0: pin-control mode. can be <&rgmii_pins> or <&rmii_pins>. > >> + - pinctrl-0: pin-control mode. can be <&rgmii_pins>, <&rmii_pins> or led pins > >> + for internal phy mode. > >> - clock_in_out: For RGMII, it must be "input", means main clock(125MHz) > >> is not sourced from SoC's PLL, but input from PHY; For RMII, "input" means > >> PHY provides the reference clock(50MHz), "output" means GMAC provides the > >> @@ -40,6 +41,9 @@ Optional properties: > >> - tx_delay: Delay value for TXD timing. Range value is 0~0x7F, 0x30 as default. > >> - rx_delay: Delay value for RXD timing. Range value is 0~0x7F, 0x10 as default. > >> - phy-supply: phandle to a regulator if the PHY needs one > >> + - clocks: <&cru MAC_PHY>: Clock selector for internal macphy > >> + - phy-is-internal: A boolean property allows us to know that MAC will connect to > >> + internal phy. > > > > This is incorrect in two ways: > > > > - this is a property of the PHY, so it should be documented as such in > > Documentation/devicetree/bindings/net/phy.txt so other bindings can > > re-use it > > > > - if it was specific to your MAC you would expect a vendor prefix to > > this property, which is not there > > > > An alternative way to implement the external/internal logic selection > > would be mandate that your Ethernet PHY node have a compatible string > > like this: > > > > phy@0 { > > compatible = "ethernet-phy-id1234.d400", "ethernet-phy-802.3-c22"; > > }; > > > > Then you don't need this phy-is-internal property, because you can > > locate the PHY node by the phy-handle (see more about that in a reply to > > patch 10) and you can determine ahead of time whether this PHY is > > internal or not based on its compatible string. > > This doesn't really work for us (sunxi). The "internal" PHY of the H3 > is also available in the X-Powers AC200 combo chip, which would be > external. Same ID. So if someone were to be stupid and put the two > together, you wouldn't know which one it actually is. Generically > it doesn't make sense to match against the ID either. The PHY is > only integrated or inlined into the SoC, meaning it could be moved > elsewhere or even be a discreet part. > > The way I see it is more like a reversed pinmux. The system should > select either the internal set or external set of xMII pins to use. > > A phy-is-internal property in the PHY node would work for us. We > already need a PHY node to describe its clocks and resets. > > A side note to this solution is that, since the internal PHY is defined > at the .dtsi level, any external PHYs at the same address would need to > make sure to delete the property, which is kind of counterintuitive, but > it is how device tree works. One would want to put the internal PHY's > address, assuming it is configurable, on something that is rarely used. > Hello David, Florian, Andrew Could someone ack this ? or nack if you think that the chance for having two PHY id both internal and external is too low. Anyway, we need a choice. Regards