From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753289AbcGUCvA (ORCPT ); Wed, 20 Jul 2016 22:51:00 -0400 Received: from regular1.263xmail.com ([211.150.99.137]:57571 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752153AbcGUCu6 (ORCPT ); Wed, 20 Jul 2016 22:50:58 -0400 X-263anti-spam: KSV:0; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 X-ABS-CHECKED: 4 X-ADDR-CHECKED: 0 X-RL-SENDER: frank.wang@rock-chips.com X-FST-TO: frank.wang@rock-chips.com X-SENDER-IP: 58.22.7.114 X-LOGIN-NAME: frank.wang@rock-chips.com X-UNIQUE-TAG: <3ce4ea976d766401d3390e7b974bbbfa> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Subject: Re: [PATCH v8 3/3] arm64: dts: rockchip: add usb2-phy support for rk3399 To: Doug Anderson , =?UTF-8?Q?Heiko_St=c3=bcbner?= References: <1468913311-23977-1-git-send-email-frank.wang@rock-chips.com> Cc: Guenter Roeck , Guenter Roeck , Julius Werner , Kishon Vijay Abraham I , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , "linux-kernel@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-usb@vger.kernel.org" , "open list:ARM/Rockchip SoC..." , Ziyuan Xu , Kever Yang , Tao Huang , =?UTF-8?B?5ZC06Imv5bOw?= , daniel.meng@rock-chips.com, frank.wang@rock-chips.com From: Frank Wang Message-ID: Date: Thu, 21 Jul 2016 10:49:53 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Doug, On 2016/7/21 5:33, Doug Anderson wrote: > Hi, > > On Tue, Jul 19, 2016 at 12:28 AM, Frank Wang wrote: > > You need a patch description here, even for simple patches. All you > have now is a subject. > OK, I will describe it next version. >> Signed-off-by: Frank Wang >> --- >> arch/arm64/boot/dts/rockchip/rk3399-evb.dts | 19 +++++++++++ >> arch/arm64/boot/dts/rockchip/rk3399.dtsi | 47 ++++++++++++++++++++++++++- > Personally I'd prefer to see EVB in a separate patch. > Yep, I would like to separate them :-) . >> 2 files changed, 65 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-evb.dts b/arch/arm64/boot/dts/rockchip/rk3399-evb.dts >> index 1a3eb14..31d4828 100644 >> --- a/arch/arm64/boot/dts/rockchip/rk3399-evb.dts >> +++ b/arch/arm64/boot/dts/rockchip/rk3399-evb.dts >> @@ -69,6 +69,15 @@ >> regulator-max-microvolt = <3300000>; >> }; >> >> + vbus_host: vbus-host-regulator { >> + compatible = "regulator-fixed"; >> + enable-active-high; >> + gpio = <&gpio4 25 GPIO_ACTIVE_HIGH>; >> + pinctrl-names = "default"; >> + pinctrl-0 = <&host_vbus_drv>; >> + regulator-name = "vbus_host"; >> + }; >> + > To match my schematics, this would probably be "vcc5v0_host". > Technically there are two regulators but since they are the same > voltage and enabled by the same GPIO it seems like modeling it as one > regulator is fine. Yep, you are right, I will rename it. > If you really wanted to model things you could also include the input > supply (VCC5V0_SYS). Not sure how much you care to model in EVB. > Actually, from "Documentation/devicetree/bindings/regulator/fixed-regulator.txt" show, input supply name is just optional property, and it seems that only do assign "vin" value for input_supply (the second member of struct fixed_voltage_config) if "vin-supply" is specified. So is input supply name (VCC5V0_SYS) required here? Would you like to give more comments please? >> vcc_phy: vcc-phy-regulator { >> compatible = "regulator-fixed"; >> regulator-name = "vcc_phy"; >> @@ -93,6 +102,16 @@ >> status = "okay"; >> }; >> >> +&u2phy0_host { >> + phy-supply = <&vbus_host>; >> + status = "okay"; >> +}; >> + >> +&u2phy1_host { >> + phy-supply = <&vbus_host>; >> + status = "okay"; >> +}; >> + > Technically "u2" sorts alphabetically before "uart". > Well, It will be sorted next version. >> &usb_host0_ehci { >> status = "okay"; >> }; >> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi >> index d7f8e06..0383785 100644 >> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi >> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi >> @@ -221,6 +221,8 @@ >> interrupts = ; >> clocks = <&cru HCLK_HOST0>, <&cru HCLK_HOST0_ARB>; >> clock-names = "hclk_host0", "hclk_host0_arb"; >> + phys = <&u2phy0_host>; >> + phy-names = "u2phy0"; > This is wrong. From > "Documentation/devicetree/bindings/usb/usb-ehci.txt" phy-names should > be "usb". done. >> status = "disabled"; >> }; >> >> @@ -239,6 +241,8 @@ >> interrupts = ; >> clocks = <&cru HCLK_HOST1>, <&cru HCLK_HOST1_ARB>; >> clock-names = "hclk_host1", "hclk_host1_arb"; >> + phys = <&u2phy1_host>; >> + phy-names = "u2phy1"; > This is wrong. From > "Documentation/devicetree/bindings/usb/usb-ehci.txt" phy-names should > be "usb". done >> status = "disabled"; >> }; >> >> @@ -481,8 +485,42 @@ >> }; >> >> grf: syscon@ff770000 { >> - compatible = "rockchip,rk3399-grf", "syscon"; >> + compatible = "rockchip,rk3399-grf", "syscon", "simple-mfd"; >> reg = <0x0 0xff770000 0x0 0x10000>; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + >> + u2phy0: usb2-phy@e450 { >> + compatible = "rockchip,rk3399-usb2phy"; >> + reg = <0xe450 0x10>; >> + clocks = <&cru SCLK_USB2PHY0_REF>; >> + clock-names = "phyclk"; >> + #clock-cells = <0>; >> + clock-output-names = "clk_usbphy0_480m"; > Any reason why there isn't a 'status = "disabled";' here? > Refer to some explains from Heiko in another mail which sent at 6:07 AM on 21th July, anyway, I will add ' status = "disabled" ' property next version. >> + u2phy0_host: host-port { >> + #phy-cells = <0>; >> + interrupts = ; >> + interrupt-names = "linestate"; >> + status = "disabled"; >> + }; >> + }; >> + >> + u2phy1: usb2-phy@e460 { >> + compatible = "rockchip,rk3399-usb2phy"; >> + reg = <0xe460 0x10>; >> + clocks = <&cru SCLK_USB2PHY1_REF>; >> + clock-names = "phyclk"; >> + #clock-cells = <0>; >> + clock-output-names = "clk_usbphy1_480m"; >> + >> + u2phy1_host: host-port { >> + #phy-cells = <0>; >> + interrupts = ; >> + interrupt-names = "linestate"; >> + status = "disabled"; >> + }; >> + }; >> }; >> >> watchdog@ff840000 { >> @@ -1009,5 +1047,12 @@ >> <1 14 RK_FUNC_1 &pcfg_pull_none>; >> }; >> }; >> + >> + usb2 { >> + host_vbus_drv: host-vbus-drv { >> + rockchip,pins = >> + <4 25 RK_FUNC_GPIO &pcfg_pull_none>; >> + }; >> + }; > Are you certain this belongs in rk3399.dtsi? It seems like it should > be in the EVB file. > All right, It will be moved to EVB file next version. BR. Frank