Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752283AbdHJHse (ORCPT ); Thu, 10 Aug 2017 03:48:34 -0400 Received: from lucky1.263xmail.com ([211.157.147.133]:48412 "EHLO lucky1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751391AbdHJHsc (ORCPT ); Thu, 10 Aug 2017 03:48:32 -0400 X-263anti-spam: KSV:0; X-MAIL-GRAY: 1 X-MAIL-DELIVERY: 0 X-KSVirus-check: 0 X-ABS-CHECKED: 4 X-RL-SENDER: david.wu@rock-chips.com X-FST-TO: linux-arm-kernel@lists.infradead.org X-SENDER-IP: 58.22.7.114 X-LOGIN-NAME: david.wu@rock-chips.com X-UNIQUE-TAG: <532a2324b72834cbfd27037b5321bc0f> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Subject: Re: [PATCH v3 05/11] net: stmmac: dwmac-rk: Add internal phy support To: Chen-Yu Tsai Cc: Corentin Labbe , Florian Fainelli , Andrew Lunn , David Miller , =?UTF-8?Q?Heiko_St=c3=bcbner?= , 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 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> <20170809084541.GB7137@Red> From: "David.Wu" Message-ID: <8ae2c84e-f828-c434-d609-e55bc716ec99@rock-chips.com> Date: Thu, 10 Aug 2017 15:48:18 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5553 Lines: 132 Hi Chen-Yu, 在 2017/8/10 10:40, Chen-Yu Tsai 写道: > Hi David, > > On Wed, Aug 9, 2017 at 5:38 PM, David.Wu wrote: >> Hello Corentin, Chen-Yu >> >> >> 在 2017/8/9 16:45, Corentin Labbe 写道: >>> >>> 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. >>> >> >> I think we should be specific to the situation, for us we have the >> possibility that the Mac only picks up internal PHY, so this can be fixed at >> the. DTSi level, also possible INTERNL PHY's Mac can also be used to connect >> external PHY, while cutting off the connection with the internal PHY, so we >> should define the internal PHY at the DTS level, so I think Florian's >> approach is acceptable. > > So it looks like you have the clock for the internal/integrated PHY at the > MAC level. I think this lets you define/add the PHY at the board level more > easily without a lot of duplication? > > Sunxi has the clock and reset in the PHY node, which is defined in the dtsi > file. (This part is already done.) Okay, that is better difined in the PHY node, i'll move them later. > > ChenYu > > >