Re: [PATCH v8 3/3] arm64: dts: rockchip: add usb2-phy support for rk3399

[Frank Wang <frank.wang@rock-chips.com>]

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@rock-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@rock-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@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 = <GIC_SPI 27 IRQ_TYPE_LEVEL_HIGH>;
>> +                               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 = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
>> +                               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