RE: [PATCH v6 13/13] dts: hi3660: Add support for usb on Hikey960

["shufan_lee(李書帆)" <shufan_lee@richtek.com>]

Hi Yu Chen,

From: Chen Yu <chenyu56@huawei.com>
Sent: Tuesday, April 30, 2019 3:16 PM
To: Rob Herring <robh@kernel.org>
Cc: Liuyu (R) <liuyu712@hisilicon.com>; linux-usb@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; john.stultz@linaro.org; suzhuangluan@hisilicon.com; kongfei@hisilicon.com; wanghu17@hisilicon.com; butao@hisilicon.com; chenyao11@huawei.com; fangshengzhou@hisilicon.com; lipengcheng8@huawei.com; songxiaowei@hisilicon.com; xuyiping@hisilicon.com; xuyoujun4@huawei.com; yudongbin@hisilicon.com; zangleigang@hisilicon.com; Chunfeng Yun <chunfeng.yun@mediatek.com>; Wei Xu <xuwei5@hisilicon.com>; Mark Rutland <mark.rutland@arm.com>; linux-arm-kernel@lists.infradead.org; Binghui Wang <wangbinghui@hisilicon.com>; shufan_lee(李書帆) <shufan_lee@richtek.com>
Subject: Re: [PATCH v6 13/13] dts: hi3660: Add support for usb on Hikey960

Hi Rob,

On 2019/4/26 6:00, Rob Herring wrote:
>> On Sat, Apr 20, 2019 at 02:40:19PM +0800, Yu Chen wrote:
>>> This patch adds support for usb on Hikey960.
>>>
>>> Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
>>> Cc: Wei Xu <xuwei5@hisilicon.com>
>>> Cc: Rob Herring <robh+dt@kernel.org>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: linux-arm-kernel@lists.infradead.org
>>> Cc: John Stultz <john.stultz@linaro.org>
>>> Cc: Binghui Wang <wangbinghui@hisilicon.com>
>>> Signed-off-by: Yu Chen <chenyu56@huawei.com>
>>> ---
>>> v2:
>>> * Remove device_type property.
>>> * Add property "usb-role-switch".
>> v3:
>>> * Make node "usb_phy" a subnode of usb3_otg_bc register.
>>> * Remove property "typec-vbus-enable-val" of hisi_hikey_usb.
>>> v4:
>>> * Remove property "hisilicon,usb3-otg-bc-syscon" of usb-phy.
>>> ---
>>> ---
>>>  arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts | 53 ++++++++++++++++
>>>  arch/arm64/boot/dts/hisilicon/hi3660.dtsi         | 73 +++++++++++++++++++++++
>>>  2 files changed, 126 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts
>>> b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts
>>> index e035cf195b19..d4e11c56b250 100644
>>> --- a/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts
>>> +++ b/arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts
>>> @@ -13,6 +13,7 @@
>>>  #include <dt-bindings/gpio/gpio.h>
>>>  #include <dt-bindings/input/input.h>  #include
>>> <dt-bindings/interrupt-controller/irq.h>
>>> +#include <dt-bindings/usb/pd.h>
>>>
>>>  / {
>>>  model = "HiKey960";
>>> @@ -196,6 +197,26 @@
>>>  method = "smc";
>>>  };
>>>  };
>>> +
>>> +hisi_hikey_usb: hisi_hikey_usb {
>>> +compatible = "hisilicon,hikey960_usb";
>>> +typec-vbus-gpios = <&gpio25 2 GPIO_ACTIVE_HIGH>;
>>> +otg-switch-gpios = <&gpio25 6 GPIO_ACTIVE_HIGH>;
>>> +hub-vdd33-en-gpios = <&gpio5 6 GPIO_ACTIVE_HIGH>;
>>> +pinctrl-names = "default";
>>> +pinctrl-0 = <&usbhub5734_pmx_func>;
>>> +
>>> +port {
>>> +#address-cells = <1>;
>>> +#size-cells = <0>;
>>> +
>>> +hikey_usb_ep: endpoint@0 {
>>> +reg = <0>;
>>> +remote-endpoint = <&dwc3_role_switch_notify>;
>>> +};
>>> +};
>>> +};
>>> +
>>>  };
>>>
>>>  /*
>>> @@ -526,6 +547,38 @@
>>>  &i2c1 {
>>>  status = "okay";
>>>
>>> +rt1711h: rt1711h@4e {
>>> +compatible = "richtek,rt1711h";
>>
>> The binding doc for this should state it should have a 'connector' node.
>>
> Hi shufan,
> Is the 'connector' node an essential node of rt1711h?
> Currently it is missing in Documentation/devicetree/bindings/usb/richtek,rt1711h.txt.
> Do you think the 'connector' node should add as this patch in the binding doc?

Yes.
For rt1711h, the parsing work is done when calling tcpci_register_port and tcpm_register_port.
The parsing flow is uploaded by Jun Li after rt1711h is uploaded, so richtek,rt1711h.txt hasn't modified yet.
To add rt1711h node and modify the binding doc, richtek,rt1711h.txt, you can refer to Documentation/devicetree/bindings/usb/typec-tcpci.txt

>>> +reg = <0x4e>;
>>> +status = "ok";
>>
>> Can drop this, it is the default.
>>
> OK.

>>> +interrupt-parent = <&gpio27>;
>>> +interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
>>> +pinctrl-names = "default";
>>> +pinctrl-0 = <&usb_cfg_func>;
>>> +
>>> +usb_con: connector {
>>> +compatible = "usb-c-connector";
>>> +label = "USB-C";
>>> +data-role = "dual";
>>> +power-role = "dual";
>>> +try-power-role = "sink";
>>> +source-pdos = <PDO_FIXED(5000, 500, PDO_FIXED_USB_COMM)>;
>>> +sink-pdos = <PDO_FIXED(5000, 500, PDO_FIXED_USB_COMM)
>>> +PDO_VAR(5000, 5000, 1000)>;
>>> +op-sink-microwatt = <10000000>;
>>> +};
>>> +
>>> +port {
>>
>> The connector node should have a 'ports' child with 'port@0' being the
>> HS connection.
>>
> This port and endpoint of the port are used for role_switch matching by fwnode_graph_devcon_match. I'm not too sure the 'ports' under connector is used in rt1711h driver？
> Hi shufan_lee,
>     Can you confirm this?

The 'ports' is not used in rt1711h's driver.
I'm not sure if this is helpful but the usage of 'ports' may refer to the following patch.
https://lore.kernel.org/patchwork/patch/879945/

>>> +#address-cells = <1>;
>>> +#size-cells = <0>;
>>> +
>>> +rt1711h_ep: endpoint@0 {
>>> +reg = <0>;
>>> +remote-endpoint = <&dwc3_role_switch>;
>>> +};
>>> +};
>>> +};
>>> +
>>>  adv7533: adv7533@39 {
>>>  status = "ok";
>>>  compatible = "adi,adv7533";
>>> diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
>>> b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
>>> index 2f19e0e5b7cf..173467505ada 100644
>>> --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
>>> +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
>>> @@ -355,6 +355,12 @@
>>>  #clock-cells = <1>;
>>>  };
>>>
>>> +pmctrl: pmctrl@fff31000 {
>>> +compatible = "hisilicon,hi3660-pmctrl", "syscon";
>>> +reg = <0x0 0xfff31000 0x0 0x1000>;
>>> +#clock-cells = <1>;
>>> +};
>>> +
>>>  pmuctrl: crg_ctrl@fff34000 {
>>>  compatible = "hisilicon,hi3660-pmuctrl", "syscon";
>>>  reg = <0x0 0xfff34000 0x0 0x1000>; @@ -1134,5 +1140,72 @@
>>>  };
>>>  };
>>>  };
>>> +
>>> +usb3_otg_bc: usb3_otg_bc@ff200000 {
>>> +compatible = "syscon", "simple-mfd";
>>> +reg = <0x0 0xff200000 0x0 0x1000>;
>>> +
>>> +usb_phy: usb-phy {
>>> +compatible = "hisilicon,hi3660-usb-phy";
>>> +#phy-cells = <0>;
>>> +hisilicon,pericrg-syscon = <&crg_ctrl>;
>>> +hisilicon,pctrl-syscon = <&pctrl>;
>>> +hisilicon,eye-diagram-param = <0x22466e4>;
>>> +};
>>> +};
>>> +
>>> +usb3: hisi_dwc3 {
>>> +compatible = "hisilicon,hi3660-dwc3";
>>> +#address-cells = <2>;
>>> +#size-cells = <2>;
>>> +ranges;
>>
>> If there are not any wrapper registers, then get rid of the hisi_dwc3
>> node. For just clocks and resets we just put everything in one node.
>>
> I will try to fix this.
>>> +
>>> +clocks = <&crg_ctrl HI3660_CLK_ABB_USB>,
>>> + <&crg_ctrl HI3660_ACLK_GATE_USB3OTG>;
>>> +clock-names = "clk_usb3phy_ref", "aclk_usb3otg";
>>> +
>>> +assigned-clocks = <&crg_ctrl HI3660_ACLK_GATE_USB3OTG>;
>>> +assigned-clock-rates = <229000000>;
>>> +resets = <&crg_rst 0x90 8>,
>>> + <&crg_rst 0x90 7>,
>>> + <&crg_rst 0x90 6>,
>>> + <&crg_rst 0x90 5>;
>>> +
>>> +dwc3: dwc3@ff100000 {
>>> +compatible = "snps,dwc3";
>>> +reg = <0x0 0xff100000 0x0 0x100000>;
>>> +interrupts = <0 159 4>, <0 161 4>;
>>> +phys = <&usb_phy>;
>>> +phy-names = "usb3-phy";
>>> +dr_mode = "otg";
>>> +maximum-speed = "super-speed";
>>> +phy_type = "utmi";
>>> +snps,dis-del-phy-power-chg-quirk;
>>
>>> +snps,lfps_filter_quirk;
>>> +snps,dis_u2_susphy_quirk;
>>> +snps,dis_u3_susphy_quirk;
>>> +snps,tx_de_emphasis_quirk;
>>> +snps,tx_de_emphasis = <1>;
>>> +snps,dis_enblslpm_quirk;
>>
>> Pretty sure these aren't documented because we don't use '_' in
>> property names.
>>
> Do you mean property as "snps,dis_enblslpm_quirk"? These properties are documented in Documentation/devicetree/bindings/usb/dwc3.txt.

>>> +snps,gctl-reset-quirk;
>>> +usb-role-switch;
>>> +role-switch-default-host;
>>> +
>>> +port {
>>> +#address-cells = <1>;
>>> +#size-cells = <0>;
>>> +
>>> +dwc3_role_switch: endpoint@0 {
>>> +reg = <0>;
>>> +remote-endpoint = <&rt1711h_ep>;
>>> +};
>>> +
>>> +dwc3_role_switch_notify: endpoint@1 {
>>> +reg = <1>;
>>> +remote-endpoint = <&hikey_usb_ep>;
>>> +};
>>> +};
>>> +};
>>> +};
>>>  };
>>>  };
>>> --
>>> 2.15.0-rc2
>>>
>>
>> .
>>

> Thanks
> - Yu Chen

************* Email Confidentiality Notice ********************

The information contained in this e-mail message (including any attachments) may be confidential, proprietary, privileged, or otherwise exempt from disclosure under applicable laws. It is intended to be conveyed only to the designated recipient(s). Any use, dissemination, distribution, printing, retaining or copying of this e-mail (including its attachments) by unintended recipient(s) is strictly prohibited and may be unlawful. If you are not an intended recipient of this e-mail, or believe that you have received this e-mail in error, please notify the sender immediately (by replying to this e-mail), delete any and all copies of this e-mail (including any attachments) from your system, and do not disclose the content of this e-mail to any other person. Thank you!