| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d01e6b5c-8e7a-bcba-477b-cfe3a9088964@rock-chips.com>
Date: Thu, 21 Jul 2016 10:49:53 +0800
From: Frank Wang <frank.wang@...k-chips.com>
To: Doug Anderson <dianders@...omium.org>,
Heiko Stübner <heiko@...ech.de>
Cc: Guenter Roeck <linux@...ck-us.net>,
Guenter Roeck <groeck@...omium.org>,
Julius Werner <jwerner@...omium.org>,
Kishon Vijay Abraham I <kishon@...com>,
Rob Herring <robh+dt@...nel.org>,
Pawel Moll <pawel.moll@....com>,
Mark Rutland <mark.rutland@....com>,
Ian Campbell <ijc+devicetree@...lion.org.uk>,
Kumar Gala <galak@...eaurora.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>,
Ziyuan Xu <xzy.xu@...k-chips.com>,
Kever Yang <kever.yang@...k-chips.com>,
Tao Huang <huangtao@...k-chips.com>,
吴良峰 <william.wu@...k-chips.com>,
daniel.meng@...k-chips.com, frank.wang@...k-chips.com
Subject: Re: [PATCH v8 3/3] arm64: dts: rockchip: add usb2-phy support for
rk3399
Hi Doug,
On 2016/7/21 5:33, Doug Anderson wrote:
> Hi,
>
> On Tue, Jul 19, 2016 at 12:28 AM, Frank Wang <frank.wang@...k-chips.com> 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 <frank.wang@...k-chips.com>
>> ---
>> 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 = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
>> 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 = <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>;
>> 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@...70000 {
>> - 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@...0 {
>> + 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 = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
>> + interrupt-names = "linestate";
>> + status = "disabled";
>> + };
>> + };
>> +
>> + u2phy1: usb2-phy@...0 {
>> + 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 = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
>> + interrupt-names = "linestate";
>> + status = "disabled";
>> + };
>> + };
>> };
>>
>> watchdog@...40000 {
>> @@ -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
Powered by blists - more mailing lists