diff mbox series

[v1,01/12] dt-bindings: usb: add support for dwc3 controller on HiSilicon SoCs

Message ID 20181203034515.91412-2-chenyu56@huawei.com
State Changes Requested, archived
Headers show
Series Add support for usb on Hikey960 | expand

Checks

Context Check Description
robh/checkpatch success

Commit Message

Chen Yu Dec. 3, 2018, 3:45 a.m. UTC
This patch adds binding descriptions to support the dwc3 controller
on HiSilicon SoCs and boards like the HiKey960.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: John Stultz <john.stultz@linaro.org>
Signed-off-by: Yu Chen <chenyu56@huawei.com>
---
 .../devicetree/bindings/usb/dwc3-hisi.txt          | 67 ++++++++++++++++++++++
 1 file changed, 67 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/dwc3-hisi.txt

Comments

Sergei Shtylyov Dec. 3, 2018, 8:35 a.m. UTC | #1
Hello!

On 03.12.2018 6:45, Yu Chen wrote:

> This patch adds binding descriptions to support the dwc3 controller
> on HiSilicon SoCs and boards like the HiKey960.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: John Stultz <john.stultz@linaro.org>
> Signed-off-by: Yu Chen <chenyu56@huawei.com>
> ---
>   .../devicetree/bindings/usb/dwc3-hisi.txt          | 67 ++++++++++++++++++++++
>   1 file changed, 67 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/usb/dwc3-hisi.txt
> 
> diff --git a/Documentation/devicetree/bindings/usb/dwc3-hisi.txt b/Documentation/devicetree/bindings/usb/dwc3-hisi.txt
> new file mode 100644
> index 000000000000..d32d2299a0a1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/dwc3-hisi.txt
> @@ -0,0 +1,67 @@
> +HiSilicon DWC3 USB SoC controller
> +
> +This file documents the parameters for the dwc3-hisi driver.
> +
> +Required properties:
> +- compatible:	should be "hisilicon,hi3660-dwc3"
> +- clocks:	A list of phandle + clock-specifier pairs for the
> +		clocks listed in clock-names
> +- clock-names:	Specify clock names
> +- resets:	list of phandle and reset specifier pairs.
> +
> +Sub-nodes:
> +The dwc3 core should be added as subnode to HiSilicon DWC3 as shown in the
> +example below. The DT binding details of dwc3 can be found in:
> +Documentation/devicetree/bindings/usb/dwc3.txt
> +
> +Example:
> +	usb3: hisi_dwc3 {
> +		compatible = "hisilicon,hi3660-dwc3";
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		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 {

     According to the DT spec, the node names should be generic, not chip 
specific, i.e. usb@ff100000 in this case.

[...]

MBR, Sergei
Chen Yu Dec. 3, 2018, 8:51 a.m. UTC | #2
Hi,

On 2018/12/3 16:35, Sergei Shtylyov wrote:
> Hello!
> 
> On 03.12.2018 6:45, Yu Chen wrote:
> 
>> This patch adds binding descriptions to support the dwc3 controller
>> on HiSilicon SoCs and boards like the HiKey960.
>>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: John Stultz <john.stultz@linaro.org>
>> Signed-off-by: Yu Chen <chenyu56@huawei.com>
>> ---
>>   .../devicetree/bindings/usb/dwc3-hisi.txt          | 67 ++++++++++++++++++++++
>>   1 file changed, 67 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/usb/dwc3-hisi.txt
>>
>> diff --git a/Documentation/devicetree/bindings/usb/dwc3-hisi.txt b/Documentation/devicetree/bindings/usb/dwc3-hisi.txt
>> new file mode 100644
>> index 000000000000..d32d2299a0a1
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/dwc3-hisi.txt
>> @@ -0,0 +1,67 @@
>> +HiSilicon DWC3 USB SoC controller
>> +
>> +This file documents the parameters for the dwc3-hisi driver.
>> +
>> +Required properties:
>> +- compatible:    should be "hisilicon,hi3660-dwc3"
>> +- clocks:    A list of phandle + clock-specifier pairs for the
>> +        clocks listed in clock-names
>> +- clock-names:    Specify clock names
>> +- resets:    list of phandle and reset specifier pairs.
>> +
>> +Sub-nodes:
>> +The dwc3 core should be added as subnode to HiSilicon DWC3 as shown in the
>> +example below. The DT binding details of dwc3 can be found in:
>> +Documentation/devicetree/bindings/usb/dwc3.txt
>> +
>> +Example:
>> +    usb3: hisi_dwc3 {
>> +        compatible = "hisilicon,hi3660-dwc3";
>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +        ranges;
>> +
>> +        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 {
> 
>     According to the DT spec, the node names should be generic, not chip specific, i.e. usb@ff100000 in this case.
> 

Do you mean it should be usb@ff100000: dwc3@ff100000 ?

Thanks!

> [...]
> 
> MBR, Sergei
> 
> .
>
Sergei Shtylyov Dec. 3, 2018, 8:59 a.m. UTC | #3
On 03.12.2018 11:51, Chen Yu wrote:

>>> This patch adds binding descriptions to support the dwc3 controller
>>> on HiSilicon SoCs and boards like the HiKey960.
>>>
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Cc: Rob Herring <robh+dt@kernel.org>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: John Stultz <john.stultz@linaro.org>
>>> Signed-off-by: Yu Chen <chenyu56@huawei.com>
>>> ---
>>>    .../devicetree/bindings/usb/dwc3-hisi.txt          | 67 ++++++++++++++++++++++
>>>    1 file changed, 67 insertions(+)
>>>    create mode 100644 Documentation/devicetree/bindings/usb/dwc3-hisi.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3-hisi.txt b/Documentation/devicetree/bindings/usb/dwc3-hisi.txt
>>> new file mode 100644
>>> index 000000000000..d32d2299a0a1
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/usb/dwc3-hisi.txt
>>> @@ -0,0 +1,67 @@
>>> +HiSilicon DWC3 USB SoC controller
>>> +
>>> +This file documents the parameters for the dwc3-hisi driver.
>>> +
>>> +Required properties:
>>> +- compatible:    should be "hisilicon,hi3660-dwc3"
>>> +- clocks:    A list of phandle + clock-specifier pairs for the
>>> +        clocks listed in clock-names
>>> +- clock-names:    Specify clock names
>>> +- resets:    list of phandle and reset specifier pairs.
>>> +
>>> +Sub-nodes:
>>> +The dwc3 core should be added as subnode to HiSilicon DWC3 as shown in the
>>> +example below. The DT binding details of dwc3 can be found in:
>>> +Documentation/devicetree/bindings/usb/dwc3.txt
>>> +
>>> +Example:
>>> +    usb3: hisi_dwc3 {
>>> +        compatible = "hisilicon,hi3660-dwc3";
>>> +        #address-cells = <2>;
>>> +        #size-cells = <2>;
>>> +        ranges;
>>> +
>>> +        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 {
>>
>>      According to the DT spec, the node names should be generic, not chip specific, i.e. usb@ff100000 in this case.
>>
> 
> Do you mean it should be usb@ff100000: dwc3@ff100000 ?

	dwc3: usb@ff100000

    "dwc3:" is a label, not name.

> Thanks!
> 
>> [...]

MBR, Sergei
Chen Yu Dec. 3, 2018, 9:28 a.m. UTC | #4
Hi,

On 2018/12/3 16:59, Sergei Shtylyov wrote:
> On 03.12.2018 11:51, Chen Yu wrote:
> 
>>>> This patch adds binding descriptions to support the dwc3 controller
>>>> on HiSilicon SoCs and boards like the HiKey960.
>>>>
>>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Cc: John Stultz <john.stultz@linaro.org>
>>>> Signed-off-by: Yu Chen <chenyu56@huawei.com>
>>>> ---
>>>>    .../devicetree/bindings/usb/dwc3-hisi.txt          | 67 ++++++++++++++++++++++
>>>>    1 file changed, 67 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/usb/dwc3-hisi.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3-hisi.txt b/Documentation/devicetree/bindings/usb/dwc3-hisi.txt
>>>> new file mode 100644
>>>> index 000000000000..d32d2299a0a1
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3-hisi.txt
>>>> @@ -0,0 +1,67 @@
>>>> +HiSilicon DWC3 USB SoC controller
>>>> +
>>>> +This file documents the parameters for the dwc3-hisi driver.
>>>> +
>>>> +Required properties:
>>>> +- compatible:    should be "hisilicon,hi3660-dwc3"
>>>> +- clocks:    A list of phandle + clock-specifier pairs for the
>>>> +        clocks listed in clock-names
>>>> +- clock-names:    Specify clock names
>>>> +- resets:    list of phandle and reset specifier pairs.
>>>> +
>>>> +Sub-nodes:
>>>> +The dwc3 core should be added as subnode to HiSilicon DWC3 as shown in the
>>>> +example below. The DT binding details of dwc3 can be found in:
>>>> +Documentation/devicetree/bindings/usb/dwc3.txt
>>>> +
>>>> +Example:
>>>> +    usb3: hisi_dwc3 {
>>>> +        compatible = "hisilicon,hi3660-dwc3";
>>>> +        #address-cells = <2>;
>>>> +        #size-cells = <2>;
>>>> +        ranges;
>>>> +
>>>> +        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 {
>>>
>>>      According to the DT spec, the node names should be generic, not chip specific, i.e. usb@ff100000 in this case.
>>>
>>
>> Do you mean it should be usb@ff100000: dwc3@ff100000 ?
> 
>     dwc3: usb@ff100000
> 
>    "dwc3:" is a label, not name.

I use the node name "dwc3@ff100000" according to Documentation/devicetree/bindings/usb/dwc3.txt
and documentations of vendor drivers, i.e. qcom,dwc3.txt, rockchip,dwc3.txt.

In these documentations, the dwc3 sub-node name uses "dwc3@xxxxxxxx".

I think it is better to be same as the other vendor's dwc3 drivers.
> 
>> Thanks!
>>
>>> [...]
> 
> MBR, Sergei
> 
> .
>
Rob Herring Dec. 19, 2018, 2:09 p.m. UTC | #5
On Mon, Dec 03, 2018 at 05:28:56PM +0800, Chen Yu wrote:
> Hi,
> 
> On 2018/12/3 16:59, Sergei Shtylyov wrote:
> > On 03.12.2018 11:51, Chen Yu wrote:
> > 
> >>>> This patch adds binding descriptions to support the dwc3 controller
> >>>> on HiSilicon SoCs and boards like the HiKey960.
> >>>>
> >>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >>>> Cc: Rob Herring <robh+dt@kernel.org>
> >>>> Cc: Mark Rutland <mark.rutland@arm.com>
> >>>> Cc: John Stultz <john.stultz@linaro.org>
> >>>> Signed-off-by: Yu Chen <chenyu56@huawei.com>
> >>>> ---
> >>>>    .../devicetree/bindings/usb/dwc3-hisi.txt          | 67 ++++++++++++++++++++++
> >>>>    1 file changed, 67 insertions(+)
> >>>>    create mode 100644 Documentation/devicetree/bindings/usb/dwc3-hisi.txt
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3-hisi.txt b/Documentation/devicetree/bindings/usb/dwc3-hisi.txt
> >>>> new file mode 100644
> >>>> index 000000000000..d32d2299a0a1
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/usb/dwc3-hisi.txt
> >>>> @@ -0,0 +1,67 @@
> >>>> +HiSilicon DWC3 USB SoC controller
> >>>> +
> >>>> +This file documents the parameters for the dwc3-hisi driver.
> >>>> +
> >>>> +Required properties:
> >>>> +- compatible:    should be "hisilicon,hi3660-dwc3"
> >>>> +- clocks:    A list of phandle + clock-specifier pairs for the
> >>>> +        clocks listed in clock-names
> >>>> +- clock-names:    Specify clock names
> >>>> +- resets:    list of phandle and reset specifier pairs.
> >>>> +
> >>>> +Sub-nodes:
> >>>> +The dwc3 core should be added as subnode to HiSilicon DWC3 as shown in the
> >>>> +example below. The DT binding details of dwc3 can be found in:
> >>>> +Documentation/devicetree/bindings/usb/dwc3.txt
> >>>> +
> >>>> +Example:
> >>>> +    usb3: hisi_dwc3 {
> >>>> +        compatible = "hisilicon,hi3660-dwc3";
> >>>> +        #address-cells = <2>;
> >>>> +        #size-cells = <2>;
> >>>> +        ranges;
> >>>> +
> >>>> +        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 {

Please combine these into a single node. Unless you have a wrapper with 
registers, you don't need these 2 nodes. Clocks and reset can go in the 
dwc3 node.

> >>>
> >>>      According to the DT spec, the node names should be generic, not chip specific, i.e. usb@ff100000 in this case.
> >>>
> >>
> >> Do you mean it should be usb@ff100000: dwc3@ff100000 ?
> > 
> >     dwc3: usb@ff100000
> > 
> >    "dwc3:" is a label, not name.
> 
> I use the node name "dwc3@ff100000" according to Documentation/devicetree/bindings/usb/dwc3.txt
> and documentations of vendor drivers, i.e. qcom,dwc3.txt, rockchip,dwc3.txt.
> 
> In these documentations, the dwc3 sub-node name uses "dwc3@xxxxxxxx".
> 
> I think it is better to be same as the other vendor's dwc3 drivers.

It's not. The other bindings are wrong. Follow the DT Spec.

Rob
Chen Yu Dec. 20, 2018, 1:33 a.m. UTC | #6
Hi,

On 2018/12/19 22:09, Rob Herring wrote:
> On Mon, Dec 03, 2018 at 05:28:56PM +0800, Chen Yu wrote:
>> Hi,
>>
>> On 2018/12/3 16:59, Sergei Shtylyov wrote:
>>> On 03.12.2018 11:51, Chen Yu wrote:
>>>
>>>>>> This patch adds binding descriptions to support the dwc3 controller
>>>>>> on HiSilicon SoCs and boards like the HiKey960.
>>>>>>
>>>>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>>>> Cc: John Stultz <john.stultz@linaro.org>
>>>>>> Signed-off-by: Yu Chen <chenyu56@huawei.com>
>>>>>> ---
>>>>>>    .../devicetree/bindings/usb/dwc3-hisi.txt          | 67 ++++++++++++++++++++++
>>>>>>    1 file changed, 67 insertions(+)
>>>>>>    create mode 100644 Documentation/devicetree/bindings/usb/dwc3-hisi.txt
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3-hisi.txt b/Documentation/devicetree/bindings/usb/dwc3-hisi.txt
>>>>>> new file mode 100644
>>>>>> index 000000000000..d32d2299a0a1
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3-hisi.txt
>>>>>> @@ -0,0 +1,67 @@
>>>>>> +HiSilicon DWC3 USB SoC controller
>>>>>> +
>>>>>> +This file documents the parameters for the dwc3-hisi driver.
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible:    should be "hisilicon,hi3660-dwc3"
>>>>>> +- clocks:    A list of phandle + clock-specifier pairs for the
>>>>>> +        clocks listed in clock-names
>>>>>> +- clock-names:    Specify clock names
>>>>>> +- resets:    list of phandle and reset specifier pairs.
>>>>>> +
>>>>>> +Sub-nodes:
>>>>>> +The dwc3 core should be added as subnode to HiSilicon DWC3 as shown in the
>>>>>> +example below. The DT binding details of dwc3 can be found in:
>>>>>> +Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>> +
>>>>>> +Example:
>>>>>> +    usb3: hisi_dwc3 {
>>>>>> +        compatible = "hisilicon,hi3660-dwc3";
>>>>>> +        #address-cells = <2>;
>>>>>> +        #size-cells = <2>;
>>>>>> +        ranges;
>>>>>> +
>>>>>> +        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 {
> 
> Please combine these into a single node. Unless you have a wrapper with 
> registers, you don't need these 2 nodes. Clocks and reset can go in the 
> dwc3 node.

Yes, clocks can go in the dwc3 node, but there are four resets need to be
deasserted for dwc3 usb of Hi3660 Soc and there is only one reset handled
in dwc3 driver.

> 
>>>>>
>>>>>      According to the DT spec, the node names should be generic, not chip specific, i.e. usb@ff100000 in this case.
>>>>>
>>>>
>>>> Do you mean it should be usb@ff100000: dwc3@ff100000 ?
>>>
>>>     dwc3: usb@ff100000
>>>
>>>    "dwc3:" is a label, not name.
>>
>> I use the node name "dwc3@ff100000" according to Documentation/devicetree/bindings/usb/dwc3.txt
>> and documentations of vendor drivers, i.e. qcom,dwc3.txt, rockchip,dwc3.txt.
>>
>> In these documentations, the dwc3 sub-node name uses "dwc3@xxxxxxxx".
>>
>> I think it is better to be same as the other vendor's dwc3 drivers.
> 
> It's not. The other bindings are wrong. Follow the DT Spec.
> 
> Rob
> 
> .
>
Felipe Balbi Dec. 20, 2018, 6:46 a.m. UTC | #7
Hi,

Rob Herring <robh@kernel.org> writes:
>> >>>> +Example:
>> >>>> +    usb3: hisi_dwc3 {
>> >>>> +        compatible = "hisilicon,hi3660-dwc3";
>> >>>> +        #address-cells = <2>;
>> >>>> +        #size-cells = <2>;
>> >>>> +        ranges;
>> >>>> +
>> >>>> +        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 {
>
> Please combine these into a single node. Unless you have a wrapper with 
> registers, you don't need these 2 nodes. Clocks and reset can go in the 
> dwc3 node.
>
>> >>>
>> >>>      According to the DT spec, the node names should be generic, not chip specific, i.e. usb@ff100000 in this case.
>> >>>
>> >>
>> >> Do you mean it should be usb@ff100000: dwc3@ff100000 ?
>> > 
>> >     dwc3: usb@ff100000
>> > 
>> >    "dwc3:" is a label, not name.
>> 
>> I use the node name "dwc3@ff100000" according to Documentation/devicetree/bindings/usb/dwc3.txt
>> and documentations of vendor drivers, i.e. qcom,dwc3.txt, rockchip,dwc3.txt.
>> 
>> In these documentations, the dwc3 sub-node name uses "dwc3@xxxxxxxx".
>> 
>> I think it is better to be same as the other vendor's dwc3 drivers.
>
> It's not. The other bindings are wrong. Follow the DT Spec.

what's wrong about them? They clearly describe the HW:

1) a company-specific glue/adaptation/integration IP
2) a generic licensed IP inside it

dwc3.ko is compatible with Synopsys' documentation and there's only one
incarnation of dwc3. Everything that can be detected in runtime, we do
so. Everything that can't, we use quirk flags. Keep in mind dwc3.ko is
also used as is by non-DT systems where we can't simply change a
compatible flag.
Rob Herring Dec. 20, 2018, 1:48 p.m. UTC | #8
On Thu, Dec 20, 2018 at 12:46 AM Felipe Balbi
<felipe.balbi@linux.intel.com> wrote:
>
>
> Hi,
>
> Rob Herring <robh@kernel.org> writes:
> >> >>>> +Example:
> >> >>>> +    usb3: hisi_dwc3 {
> >> >>>> +        compatible = "hisilicon,hi3660-dwc3";
> >> >>>> +        #address-cells = <2>;
> >> >>>> +        #size-cells = <2>;
> >> >>>> +        ranges;
> >> >>>> +
> >> >>>> +        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 {
> >
> > Please combine these into a single node. Unless you have a wrapper with
> > registers, you don't need these 2 nodes. Clocks and reset can go in the
> > dwc3 node.
> >
> >> >>>
> >> >>>      According to the DT spec, the node names should be generic, not chip specific, i.e. usb@ff100000 in this case.
> >> >>>
> >> >>
> >> >> Do you mean it should be usb@ff100000: dwc3@ff100000 ?
> >> >
> >> >     dwc3: usb@ff100000
> >> >
> >> >    "dwc3:" is a label, not name.
> >>
> >> I use the node name "dwc3@ff100000" according to Documentation/devicetree/bindings/usb/dwc3.txt
> >> and documentations of vendor drivers, i.e. qcom,dwc3.txt, rockchip,dwc3.txt.
> >>
> >> In these documentations, the dwc3 sub-node name uses "dwc3@xxxxxxxx".
> >>
> >> I think it is better to be same as the other vendor's dwc3 drivers.
> >
> > It's not. The other bindings are wrong. Follow the DT Spec.
>
> what's wrong about them? They clearly describe the HW:
>
> 1) a company-specific glue/adaptation/integration IP
> 2) a generic licensed IP inside it

That is *every* licensed IP block and DWC3 is the oddball where we did
this 2 node thing. It is not a pattern we should continue. If there's
registers in the wrapper, then yes, having 2 nodes makes sense. But
just additional clocks or resets, no. I would guess these extra clocks
and resets are inter-connect related and are needed as an artifact of
not describing and managing inter-connects.

I can just as easily argue it doesn't describe the hardware. I'm
pretty sure the DWC3 has clocks and resets yet there are none in the
DWC3 node. How can it operate with no clocks?

> dwc3.ko is compatible with Synopsys' documentation and there's only one
> incarnation of dwc3. Everything that can be detected in runtime, we do
> so. Everything that can't, we use quirk flags. Keep in mind dwc3.ko is
> also used as is by non-DT systems where we can't simply change a
> compatible flag.

Linux driver architecture doesn't dictate bindings.

Rob
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/usb/dwc3-hisi.txt b/Documentation/devicetree/bindings/usb/dwc3-hisi.txt
new file mode 100644
index 000000000000..d32d2299a0a1
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/dwc3-hisi.txt
@@ -0,0 +1,67 @@ 
+HiSilicon DWC3 USB SoC controller
+
+This file documents the parameters for the dwc3-hisi driver.
+
+Required properties:
+- compatible:	should be "hisilicon,hi3660-dwc3"
+- clocks:	A list of phandle + clock-specifier pairs for the
+		clocks listed in clock-names
+- clock-names:	Specify clock names
+- resets:	list of phandle and reset specifier pairs.
+
+Sub-nodes:
+The dwc3 core should be added as subnode to HiSilicon DWC3 as shown in the
+example below. The DT binding details of dwc3 can be found in:
+Documentation/devicetree/bindings/usb/dwc3.txt
+
+Example:
+	usb3: hisi_dwc3 {
+		compatible = "hisilicon,hi3660-dwc3";
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		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;
+			snps,gctl-reset-quirk;
+
+			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>;
+				};
+			};
+		};
+	};