[PATCH v4 5/5] usb: dwc3: rockchip: add devicetree bindings documentation

[PATCH v4 5/5] usb: dwc3: rockchip: add devicetree bindings documentation

[William Wu]

This patch adds the devicetree documentation required for Rockchip
USB3.0 core wrapper consisting of USB3.0 IP from Synopsys.

It supports DRD mode, and could operate in device mode (SS, HS, FS)
and host mode (SS, HS, FS, LS).

Signed-off-by: William Wu <william.wu@rock-chips.com>
---
Changes in v4:
- modify commit log, and add phy documentation location (Sergei)

Changes in v3:
- add dwc3 address (balbi)

Changes in v2:
- add rockchip,dwc3.txt to Documentation/devicetree/bindings/ (balbi, Brian)

 .../devicetree/bindings/usb/rockchip,dwc3.txt      | 46 ++++++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/rockchip,dwc3.txt

diff --git a/Documentation/devicetree/bindings/usb/rockchip,dwc3.txt b/Documentation/devicetree/bindings/usb/rockchip,dwc3.txt
new file mode 100644
index 0000000..0edf013
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/rockchip,dwc3.txt
@@ -0,0 +1,46 @@
+Rockchip SuperSpeed DWC3 USB SoC controller
+
+Required properties:
+- compatible:	should contain "rockchip,dwc3"
+- clocks:		A list of phandle + clock-specifier pairs for the
+				clocks listed in clock-names
+- clock-names:	Should contain the following:
+  "clk_usb3otg0_ref"	Controller reference clk
+  "clk_usb3otg0_suspend"Controller suspend clk, can use 24 MHz or 32 KHz
+  "aclk_usb3"		Master/Core clock, have to be >= 62.5 MHz for SS operation
+
+
+Optional clocks:
+  "aclk_usb3otg0"	Aclk for specific usb controller clock.
+  "aclk_usb3_rksoc_axi_perf"  USB AXI perf clock.  Not present on all platforms.
+  "aclk_usb3_grf"	USB grf clock.  Not present on all platforms.
+
+Required child node:
+A child node must exist to represent the core DWC3 IP block. The name of
+the node is not important. The content of the node is defined in dwc3.txt.
+
+Phy documentation is provided in the following places:
+Documentation/devicetree/bindings/phy/rockchip,dwc3-usb-phy.txt
+
+Example device nodes:
+
+	usbdrd3_0: usb@fe800000 {
+		compatible = "rockchip,dwc3";
+		clocks = <&cru SCLK_USB3OTG0_REF>, <&cru SCLK_USB3OTG0_SUSPEND>,
+			 <&cru ACLK_USB3>, <&cru ACLK_USB3OTG0>,
+			 <&cru ACLK_USB3_RKSOC_AXI_PERF>, <&cru ACLK_USB3_GRF>;
+		clock-names = "clk_usb3otg0_ref", "clk_usb3otg0_suspend",
+			      "aclk_usb3", "aclk_usb3otg0",
+			      "aclk_usb3_rksoc_axi_perf", "aclk_usb3_grf";
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+		status = "disabled";
+		usbdrd_dwc3_0: dwc3@fe800000 {
+			compatible = "snps,dwc3";
+			reg = <0x0 0xfe800000 0x0 0x100000>;
+			interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>;
+			dr_mode = "otg";
+			status = "disabled";
+		};
+	};
-- 
1.9.1

Re: [PATCH v4 5/5] usb: dwc3: rockchip: add devicetree bindings documentation

[Heiko Stübner]

Hi William,

Am Donnerstag, 2. Juni 2016, 20:34:56 schrieb William Wu:
> This patch adds the devicetree documentation required for Rockchip
> USB3.0 core wrapper consisting of USB3.0 IP from Synopsys.
> 
> It supports DRD mode, and could operate in device mode (SS, HS, FS)
> and host mode (SS, HS, FS, LS).
> 
> Signed-off-by: William Wu <william.wu@rock-chips.com>

devicetree binding documentation patches should include the devicetree 
maintainers (scripts/get_maintainer.pl)

> ---
> Changes in v4:
> - modify commit log, and add phy documentation location (Sergei)
> 
> Changes in v3:
> - add dwc3 address (balbi)
> 
> Changes in v2:
> - add rockchip,dwc3.txt to Documentation/devicetree/bindings/ (balbi, 
Brian)
> 
>  .../devicetree/bindings/usb/rockchip,dwc3.txt      | 46
> ++++++++++++++++++++++ 1 file changed, 46 insertions(+)
>  create mode 100644 
Documentation/devicetree/bindings/usb/rockchip,dwc3.txt
> 
> diff --git a/Documentation/devicetree/bindings/usb/rockchip,dwc3.txt
> b/Documentation/devicetree/bindings/usb/rockchip,dwc3.txt new file mode
> 100644
> index 0000000..0edf013
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/rockchip,dwc3.txt
> @@ -0,0 +1,46 @@
> +Rockchip SuperSpeed DWC3 USB SoC controller
> +
> +Required properties:
> +- compatible:	should contain "rockchip,dwc3"

are you sure this will work for all future socs in the same way? I guess 
doing this as rockchip,rk3399-dwc3 might make our lifes easier down the road 
:-) [both the xilinx and st dwc3 bindings do already that]

> +- clocks:		A list of phandle + clock-specifier pairs for the
> +				clocks listed in clock-names
> +- clock-names:	Should contain the following:
> +  "clk_usb3otg0_ref"	Controller reference clk
> +  "clk_usb3otg0_suspend"Controller suspend clk, can use 24 MHz or 32 KHz
> +  "aclk_usb3"		Master/Core clock, have to be >= 62.5 MHz for SS 
operation

clock names should always be in the scope of the device block (named after 
what it supplies). And looking at the dwc3-xilinx.txt binding, I'd suggest 
getting inspiration from their clock names (bus_clk, ref_clk, suspend_clk or 
so)

> +Optional clocks:
> +  "aclk_usb3otg0"	Aclk for specific usb controller clock.
> +  "aclk_usb3_rksoc_axi_perf"  USB AXI perf clock.  Not present on all
> platforms. 

The clock names looks pretty strange. What are they for? Especially as 
nothing seems to use them right now.


> +  "aclk_usb3_grf"	USB grf clock.  Not present on all platforms.

for my own education, which part of the GRF does this clock supply?


> +
> +Required child node:
> +A child node must exist to represent the core DWC3 IP block. The name of
> +the node is not important. The content of the node is defined in dwc3.txt.
> +
> +Phy documentation is provided in the following places:
> +Documentation/devicetree/bindings/phy/rockchip,dwc3-usb-phy.txt
> +
> +Example device nodes:
> +
> +	usbdrd3_0: usb@fe800000 {
> +		compatible = "rockchip,dwc3";
> +		clocks = <&cru SCLK_USB3OTG0_REF>, <&cru SCLK_USB3OTG0_SUSPEND>,
> +			 <&cru ACLK_USB3>, <&cru ACLK_USB3OTG0>,
> +			 <&cru ACLK_USB3_RKSOC_AXI_PERF>, <&cru ACLK_USB3_GRF>;
> +		clock-names = "clk_usb3otg0_ref", "clk_usb3otg0_suspend",
> +			      "aclk_usb3", "aclk_usb3otg0",
> +			      "aclk_usb3_rksoc_axi_perf", "aclk_usb3_grf";
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +		status = "disabled";
> +		usbdrd_dwc3_0: dwc3@fe800000 {
> +			compatible = "snps,dwc3";
> +			reg = <0x0 0xfe800000 0x0 0x100000>;
> +			interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>;
> +			dr_mode = "otg";
> +			status = "disabled";
> +		};
> +	};

Re: [PATCH v4 5/5] usb: dwc3: rockchip: add devicetree bindings documentation

[William Wu]

Dear Heiko,

On 06/17/2016 07:15 AM, Heiko Stübner wrote:
> Hi William,
>
> Am Donnerstag, 2. Juni 2016, 20:34:56 schrieb William Wu:
>> This patch adds the devicetree documentation required for Rockchip
>> USB3.0 core wrapper consisting of USB3.0 IP from Synopsys.
>>
>> It supports DRD mode, and could operate in device mode (SS, HS, FS)
>> and host mode (SS, HS, FS, LS).
>>
>> Signed-off-by: William Wu <william.wu@rock-chips.com>
> devicetree binding documentation patches should include the devicetree
> maintainers (scripts/get_maintainer.pl)
I'll add devicetree maintainers in next patch v5.
>
>> ---
>> Changes in v4:
>> - modify commit log, and add phy documentation location (Sergei)
>>
>> Changes in v3:
>> - add dwc3 address (balbi)
>>
>> Changes in v2:
>> - add rockchip,dwc3.txt to Documentation/devicetree/bindings/ (balbi,
> Brian)
>>   .../devicetree/bindings/usb/rockchip,dwc3.txt      | 46
>> ++++++++++++++++++++++ 1 file changed, 46 insertions(+)
>>   create mode 100644
> Documentation/devicetree/bindings/usb/rockchip,dwc3.txt
>> diff --git a/Documentation/devicetree/bindings/usb/rockchip,dwc3.txt
>> b/Documentation/devicetree/bindings/usb/rockchip,dwc3.txt new file mode
>> 100644
>> index 0000000..0edf013
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/rockchip,dwc3.txt
>> @@ -0,0 +1,46 @@
>> +Rockchip SuperSpeed DWC3 USB SoC controller
>> +
>> +Required properties:
>> +- compatible:	should contain "rockchip,dwc3"
> are you sure this will work for all future socs in the same way? I guess
> doing this as rockchip,rk3399-dwc3 might make our lifes easier down the road
> :-) [both the xilinx and st dwc3 bindings do already that]
I'm not sure that whether our future socs dwc3 will work well in the 
same way.
So I think "rockchip,rk3399-dwc3" is more appropriate.
Thanks very much for your suggestion.
>
>> +- clocks:		A list of phandle + clock-specifier pairs for the
>> +				clocks listed in clock-names
>> +- clock-names:	Should contain the following:
>> +  "clk_usb3otg0_ref"	Controller reference clk
>> +  "clk_usb3otg0_suspend"Controller suspend clk, can use 24 MHz or 32 KHz
>> +  "aclk_usb3"		Master/Core clock, have to be >= 62.5 MHz for SS
> operation
>
> clock names should always be in the scope of the device block (named after
> what it supplies). And looking at the dwc3-xilinx.txt binding, I'd suggest
> getting inspiration from their clock names (bus_clk, ref_clk, suspend_clk or
> so)
I'll fix the clock names next patch v5.
>
>> +Optional clocks:
>> +  "aclk_usb3otg0"	Aclk for specific usb controller clock.
>> +  "aclk_usb3_rksoc_axi_perf"  USB AXI perf clock.  Not present on all
>> platforms.
> The clock names looks pretty strange. What are they for? Especially as
> nothing seems to use them right now.

"aclk_usb3_rksoc_axi_perf", it's the clk for usb3 performance monitor module,
you can refer to the GRF_USB3_PERF_xxx. And we don't use the usb3 performance
monitor control registers right now.

>
>
>> +  "aclk_usb3_grf"	USB grf clock.  Not present on all platforms.
> for my own education, which part of the GRF does this clock supply?

"aclk_usb3_grf", it's the clk for USB3 grf, e.g. GRF_USB3OTGX_CONX

>
>
>> +
>> +Required child node:
>> +A child node must exist to represent the core DWC3 IP block. The name of
>> +the node is not important. The content of the node is defined in dwc3.txt.
>> +
>> +Phy documentation is provided in the following places:
>> +Documentation/devicetree/bindings/phy/rockchip,dwc3-usb-phy.txt
>> +
>> +Example device nodes:
>> +
>> +	usbdrd3_0: usb@fe800000 {
>> +		compatible = "rockchip,dwc3";
>> +		clocks = <&cru SCLK_USB3OTG0_REF>, <&cru SCLK_USB3OTG0_SUSPEND>,
>> +			 <&cru ACLK_USB3>, <&cru ACLK_USB3OTG0>,
>> +			 <&cru ACLK_USB3_RKSOC_AXI_PERF>, <&cru ACLK_USB3_GRF>;
>> +		clock-names = "clk_usb3otg0_ref", "clk_usb3otg0_suspend",
>> +			      "aclk_usb3", "aclk_usb3otg0",
>> +			      "aclk_usb3_rksoc_axi_perf", "aclk_usb3_grf";
>> +		#address-cells = <2>;
>> +		#size-cells = <2>;
>> +		ranges;
>> +		status = "disabled";
>> +		usbdrd_dwc3_0: dwc3@fe800000 {
>> +			compatible = "snps,dwc3";
>> +			reg = <0x0 0xfe800000 0x0 0x100000>;
>> +			interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>;
>> +			dr_mode = "otg";
>> +			status = "disabled";
>> +		};
>> +	};
>
>

Re: [PATCH v4 5/5] usb: dwc3: rockchip: add devicetree bindings documentation

[Heiko Stübner]

Hi William,

Am Freitag, 17. Juni 2016, 17:18:59 schrieb William Wu:
> On 06/17/2016 07:15 AM, Heiko Stübner wrote:
> > Am Donnerstag, 2. Juni 2016, 20:34:56 schrieb William Wu:
> >> This patch adds the devicetree documentation required for Rockchip
> >> USB3.0 core wrapper consisting of USB3.0 IP from Synopsys.
> >> 
> >> It supports DRD mode, and could operate in device mode (SS, HS, FS)
> >> and host mode (SS, HS, FS, LS).
> >> 
> >> Signed-off-by: William Wu <william.wu@rock-chips.com>

[...]

> >> +Optional clocks:
> >> +  "aclk_usb3otg0"	Aclk for specific usb controller clock.

what does this clock do? Also most likely same argument as below.


> >> +  "aclk_usb3_rksoc_axi_perf"  USB AXI perf clock.  Not present on all
> >> platforms.
> > 
> > The clock names looks pretty strange. What are they for? Especially as
> > nothing seems to use them right now.
> 
> "aclk_usb3_rksoc_axi_perf", it's the clk for usb3 performance monitor
> module, you can refer to the GRF_USB3_PERF_xxx. And we don't use the usb3
> performance monitor control registers right now.

ok, then I'd suggest not defining the clock for now.

For one, there are more perf blocks in the GRF (usb3, pcie, hdcp22, gmac, gpu, 
etc) which all seem to share a somewhat similar design, so they will maybe 
result in a separate driver of some form for the performance monitors.

And secondly, it is somewhat easy to add new optional properties, but you 
cannot remove anything defined previously. So if we later decide to handle all 
the performance monitors differently, you can't remove that clock from the 
binding again. (or at least only with quite a bit of hassle).

So as this clock isn't used at all yet, I guess it should not get included 
now.


> >> +  "aclk_usb3_grf"	USB grf clock.  Not present on all platforms.
> > 
> > for my own education, which part of the GRF does this clock supply?
> 
> "aclk_usb3_grf", it's the clk for USB3 grf, e.g. GRF_USB3OTGX_CONX

Hmm, this looks more like it belongs to the otg phy? 
Anyway, also seems unused right now, so same argument as above applies here.


Heiko

Re: [PATCH v4 5/5] usb: dwc3: rockchip: add devicetree bindings documentation

[William Wu]

Dear Heiko,

On 06/20/2016 10:44 PM, Heiko Stübner wrote:
> Hi William,
>
> Am Freitag, 17. Juni 2016, 17:18:59 schrieb William Wu:
>> On 06/17/2016 07:15 AM, Heiko Stübner wrote:
>>> Am Donnerstag, 2. Juni 2016, 20:34:56 schrieb William Wu:
>>>> This patch adds the devicetree documentation required for Rockchip
>>>> USB3.0 core wrapper consisting of USB3.0 IP from Synopsys.
>>>>
>>>> It supports DRD mode, and could operate in device mode (SS, HS, FS)
>>>> and host mode (SS, HS, FS, LS).
>>>>
>>>> Signed-off-by: William Wu <william.wu@rock-chips.com>
> [...]
>
>>>> +Optional clocks:
>>>> +  "aclk_usb3otg0"	Aclk for specific usb controller clock.
> what does this clock do? Also most likely same argument as below.
Here is partial rk3399 clk tree about usb3:

aclk_usb3                       7            7   297000000 0 0
              aclk_usb3_grf                1            1 
297000000          0 0
              aclk_usb3_rksoc_axi_perf     1            1 
297000000          0 0
              aclk_usb3otg1                1            1 
297000000          0 0
              aclk_usb3otg0                1            1 
297000000          0 0
              aclk_usb3_noc                1            1 
297000000          0 0

from the clk tree, and check with our IC designers, we can see that:
1. aclk_usb3 is the parent clk of aclk_usb3****
2. aclk_usb3_grf  is used for both otg0 and otg1 grf, and these usb3 grf 
can be set
to control otg0 and otg1 controller, but not the phy.
3. aclk_usb3otg1 is otg1 controller clk,  aclk_usb3otg0 is otg0 
controller clk.
4. aclk_usb3_rksoc_axi_perf is the clk for usb3 performance monitor module.
5. aclk_usb3_noc is the clk for soc bus interconnect.

>
>>>> +  "aclk_usb3_rksoc_axi_perf"  USB AXI perf clock.  Not present on all
>>>> platforms.
>>> The clock names looks pretty strange. What are they for? Especially as
>>> nothing seems to use them right now.
>> "aclk_usb3_rksoc_axi_perf", it's the clk for usb3 performance monitor
>> module, you can refer to the GRF_USB3_PERF_xxx. And we don't use the usb3
>> performance monitor control registers right now.
> ok, then I'd suggest not defining the clock for now.
>
> For one, there are more perf blocks in the GRF (usb3, pcie, hdcp22, gmac, gpu,
> etc) which all seem to share a somewhat similar design, so they will maybe
> result in a separate driver of some form for the performance monitors.
>
> And secondly, it is somewhat easy to add new optional properties, but you
> cannot remove anything defined previously. So if we later decide to handle all
> the performance monitors differently, you can't remove that clock from the
> binding again. (or at least only with quite a bit of hassle).
>
> So as this clock isn't used at all yet, I guess it should not get included
> now.
Yes, I agree with you. We can remove the aclk_usb3_rksoc_axi_perf right now.
>
>>>> +  "aclk_usb3_grf"	USB grf clock.  Not present on all platforms.
>>> for my own education, which part of the GRF does this clock supply?
>> "aclk_usb3_grf", it's the clk for USB3 grf, e.g. GRF_USB3OTGX_CONX
> Hmm, this looks more like it belongs to the otg phy?
> Anyway, also seems unused right now, so same argument as above applies here.
As I have described above, the "aclk_usb3_grf" is  used for both otg0 
and otg1 grf,
and these usb3 grf can be set to control otg0 and otg1 controller, but 
not the phy.
And we have done a test that remove the grf clk, and the result showed 
that usb3
controller can't work normally. So I think we need the usb3 grf clk.

So about the usb3 controller clk management, I think it should contain 
the following clk:
1.  aclk_usb3otg1
2.  aclk_usb3otg0
3.  aclk_usb3_grf
4.  aclk_usb3_noc
For "aclk_usb3_noc", I discuss with our clk manager, the clk is always 
on before,
but according to upstream maintainer's suggestion, we need to manage the 
noc clk by each module.

and the follow two clk can be removed:
1. aclk_usb3
2. aclk_usb3_rksoc_axi_perf

Is it ok?
>
>
> Heiko
>
>
>

Re: [PATCH v4 5/5] usb: dwc3: rockchip: add devicetree bindings documentation

[Heiko Stuebner]

Hi William,

Am Dienstag, 21. Juni 2016, 17:11:44 schrieb William Wu:
> On 06/20/2016 10:44 PM, Heiko Stübner wrote:
> > Am Freitag, 17. Juni 2016, 17:18:59 schrieb William Wu:
> >> On 06/17/2016 07:15 AM, Heiko Stübner wrote:
> >>> Am Donnerstag, 2. Juni 2016, 20:34:56 schrieb William Wu:
> >>>> This patch adds the devicetree documentation required for Rockchip
> >>>> USB3.0 core wrapper consisting of USB3.0 IP from Synopsys.
> >>>> 
> >>>> It supports DRD mode, and could operate in device mode (SS, HS, FS)
> >>>> and host mode (SS, HS, FS, LS).
> >>>> 
> >>>> Signed-off-by: William Wu <william.wu@rock-chips.com>
> > 
> > [...]
> > 
> >>>> +Optional clocks:
> >>>> +  "aclk_usb3otg0"	Aclk for specific usb controller clock.
> > 
> > what does this clock do? Also most likely same argument as below.
> 
> Here is partial rk3399 clk tree about usb3:
> 
> aclk_usb3                       7            7   297000000 0 0
>               aclk_usb3_grf                1            1
> 297000000          0 0
>               aclk_usb3_rksoc_axi_perf     1            1
> 297000000          0 0
>               aclk_usb3otg1                1            1
> 297000000          0 0
>               aclk_usb3otg0                1            1
> 297000000          0 0
>               aclk_usb3_noc                1            1
> 297000000          0 0
> 
> from the clk tree, and check with our IC designers, we can see that:
> 1. aclk_usb3 is the parent clk of aclk_usb3****
> 2. aclk_usb3_grf  is used for both otg0 and otg1 grf, and these usb3 grf
> can be set
> to control otg0 and otg1 controller, but not the phy.
> 3. aclk_usb3otg1 is otg1 controller clk,  aclk_usb3otg0 is otg0
> controller clk.
> 4. aclk_usb3_rksoc_axi_perf is the clk for usb3 performance monitor
> module. 5. aclk_usb3_noc is the clk for soc bus interconnect.
> 
> >>>> +  "aclk_usb3_rksoc_axi_perf"  USB AXI perf clock.  Not present on
> >>>> all
> >>>> platforms.
> >>> 
> >>> The clock names looks pretty strange. What are they for? Especially as
> >>> nothing seems to use them right now.
> >> 
> >> "aclk_usb3_rksoc_axi_perf", it's the clk for usb3 performance monitor
> >> module, you can refer to the GRF_USB3_PERF_xxx. And we don't use the
> >> usb3
> >> performance monitor control registers right now.
> > 
> > ok, then I'd suggest not defining the clock for now.
> > 
> > For one, there are more perf blocks in the GRF (usb3, pcie, hdcp22,
> > gmac, gpu, etc) which all seem to share a somewhat similar design, so
> > they will maybe result in a separate driver of some form for the
> > performance monitors.
> > 
> > And secondly, it is somewhat easy to add new optional properties, but
> > you
> > cannot remove anything defined previously. So if we later decide to
> > handle all the performance monitors differently, you can't remove that
> > clock from the binding again. (or at least only with quite a bit of
> > hassle).
> > 
> > So as this clock isn't used at all yet, I guess it should not get
> > included now.
> 
> Yes, I agree with you. We can remove the aclk_usb3_rksoc_axi_perf right
> now.
> >>>> +  "aclk_usb3_grf"	USB grf clock.  Not present on all platforms.
> >>> 
> >>> for my own education, which part of the GRF does this clock supply?
> >> 
> >> "aclk_usb3_grf", it's the clk for USB3 grf, e.g. GRF_USB3OTGX_CONX
> > 
> > Hmm, this looks more like it belongs to the otg phy?
> > Anyway, also seems unused right now, so same argument as above applies
> > here.
> As I have described above, the "aclk_usb3_grf" is  used for both otg0
> and otg1 grf,
> and these usb3 grf can be set to control otg0 and otg1 controller, but
> not the phy.
> And we have done a test that remove the grf clk, and the result showed
> that usb3
> controller can't work normally. So I think we need the usb3 grf clk.
> 
> So about the usb3 controller clk management, I think it should contain
> the following clk:
> 1.  aclk_usb3otg1
> 2.  aclk_usb3otg0
> 3.  aclk_usb3_grf

correct, aclk_usb3otgX would then be the busclk for each controller if I'm 
not mistaken and the grf clock should also get enabled, like we also plan on 
doing for the vio_grf clock in the display area.

> 4.  aclk_usb3_noc
> For "aclk_usb3_noc", I discuss with our clk manager, the clk is always
> on before,
> but according to upstream maintainer's suggestion, we need to manage the
> noc clk by each module.

can you point me to this discussion? The bus-interconnect is a very separate 
component, which we don't model so far and thus we have all the noc clocks 
simply marked as critical.

As this clock doesn't belong to the actual usb controller block, but said 
separate component, handling it in the controller seems somehow wrong to me.

So my (current) opinion would again be to mark the noc clock as critical for 
the time being.


> and the follow two clk can be removed:
> 1. aclk_usb3
> 2. aclk_usb3_rksoc_axi_perf
> 
> Is it ok?

yep, apart from the noc-clock, this looks great.


Heiko

Re: [PATCH v4 5/5] usb: dwc3: rockchip: add devicetree bindings documentation

[William Wu]

Dear Heiko,

On 06/25/2016 03:50 AM, Heiko Stuebner wrote:
> Hi William,
>
> Am Dienstag, 21. Juni 2016, 17:11:44 schrieb William Wu:
>> On 06/20/2016 10:44 PM, Heiko Stübner wrote:
>>> Am Freitag, 17. Juni 2016, 17:18:59 schrieb William Wu:
>>>> On 06/17/2016 07:15 AM, Heiko Stübner wrote:
>>>>> Am Donnerstag, 2. Juni 2016, 20:34:56 schrieb William Wu:
>>>>>> This patch adds the devicetree documentation required for Rockchip
>>>>>> USB3.0 core wrapper consisting of USB3.0 IP from Synopsys.
>>>>>>
>>>>>> It supports DRD mode, and could operate in device mode (SS, HS, FS)
>>>>>> and host mode (SS, HS, FS, LS).
>>>>>>
>>>>>> Signed-off-by: William Wu <william.wu@rock-chips.com>
>>> [...]
>>>
>>>>>> +Optional clocks:
>>>>>> +  "aclk_usb3otg0"	Aclk for specific usb controller clock.
>>> what does this clock do? Also most likely same argument as below.
>> Here is partial rk3399 clk tree about usb3:
>>
>> aclk_usb3                       7            7   297000000 0 0
>>                aclk_usb3_grf                1            1
>> 297000000          0 0
>>                aclk_usb3_rksoc_axi_perf     1            1
>> 297000000          0 0
>>                aclk_usb3otg1                1            1
>> 297000000          0 0
>>                aclk_usb3otg0                1            1
>> 297000000          0 0
>>                aclk_usb3_noc                1            1
>> 297000000          0 0
>>
>> from the clk tree, and check with our IC designers, we can see that:
>> 1. aclk_usb3 is the parent clk of aclk_usb3****
>> 2. aclk_usb3_grf  is used for both otg0 and otg1 grf, and these usb3 grf
>> can be set
>> to control otg0 and otg1 controller, but not the phy.
>> 3. aclk_usb3otg1 is otg1 controller clk,  aclk_usb3otg0 is otg0
>> controller clk.
>> 4. aclk_usb3_rksoc_axi_perf is the clk for usb3 performance monitor
>> module. 5. aclk_usb3_noc is the clk for soc bus interconnect.
>>
>>>>>> +  "aclk_usb3_rksoc_axi_perf"  USB AXI perf clock.  Not present on
>>>>>> all
>>>>>> platforms.
>>>>> The clock names looks pretty strange. What are they for? Especially as
>>>>> nothing seems to use them right now.
>>>> "aclk_usb3_rksoc_axi_perf", it's the clk for usb3 performance monitor
>>>> module, you can refer to the GRF_USB3_PERF_xxx. And we don't use the
>>>> usb3
>>>> performance monitor control registers right now.
>>> ok, then I'd suggest not defining the clock for now.
>>>
>>> For one, there are more perf blocks in the GRF (usb3, pcie, hdcp22,
>>> gmac, gpu, etc) which all seem to share a somewhat similar design, so
>>> they will maybe result in a separate driver of some form for the
>>> performance monitors.
>>>
>>> And secondly, it is somewhat easy to add new optional properties, but
>>> you
>>> cannot remove anything defined previously. So if we later decide to
>>> handle all the performance monitors differently, you can't remove that
>>> clock from the binding again. (or at least only with quite a bit of
>>> hassle).
>>>
>>> So as this clock isn't used at all yet, I guess it should not get
>>> included now.
>> Yes, I agree with you. We can remove the aclk_usb3_rksoc_axi_perf right
>> now.
>>>>>> +  "aclk_usb3_grf"	USB grf clock.  Not present on all platforms.
>>>>> for my own education, which part of the GRF does this clock supply?
>>>> "aclk_usb3_grf", it's the clk for USB3 grf, e.g. GRF_USB3OTGX_CONX
>>> Hmm, this looks more like it belongs to the otg phy?
>>> Anyway, also seems unused right now, so same argument as above applies
>>> here.
>> As I have described above, the "aclk_usb3_grf" is  used for both otg0
>> and otg1 grf,
>> and these usb3 grf can be set to control otg0 and otg1 controller, but
>> not the phy.
>> And we have done a test that remove the grf clk, and the result showed
>> that usb3
>> controller can't work normally. So I think we need the usb3 grf clk.
>>
>> So about the usb3 controller clk management, I think it should contain
>> the following clk:
>> 1.  aclk_usb3otg1
>> 2.  aclk_usb3otg0
>> 3.  aclk_usb3_grf
> correct, aclk_usb3otgX would then be the busclk for each controller if I'm
> not mistaken and the grf clock should also get enabled, like we also plan on
> doing for the vio_grf clock in the display area.
OK, so  aclk_usb3_grf should be marked as critical, right?

I found that most of the grf clocks haven't been marked as critical, 
apart from vio_grf. So may I keep the aclk_usb3_grf in usb3 dts, and 
remove it after clock manager adds it to critical clocks?
>
>> 4.  aclk_usb3_noc
>> For "aclk_usb3_noc", I discuss with our clk manager, the clk is always
>> on before,
>> but according to upstream maintainer's suggestion, we need to manage the
>> noc clk by each module.
> can you point me to this discussion? The bus-interconnect is a very separate
> component, which we don't model so far and thus we have all the noc clocks
> simply marked as critical.
>
> As this clock doesn't belong to the actual usb controller block, but said
> separate component, handling it in the controller seems somehow wrong to me.
>
> So my (current) opinion would again be to mark the noc clock as critical for
> the time being.
Sorry, it seems that I get the new information about clk management too 
late.:-)

There's no dedicated discussion about noc clk, but similar to grf clock, 
please refer to "https://patchwork.kernel.org/patch/9171467/" for add 
pclk_vio_grf to critical clock on the RK3399, and you have agreed on 
that mark vio grf clk as critical. So I agree with your opinion, thanks~
>
>> and the follow two clk can be removed:
>> 1. aclk_usb3
>> 2. aclk_usb3_rksoc_axi_perf
>>
>> Is it ok?
> yep, apart from the noc-clock, this looks great.
>
>
> Heiko
Best regards,
      William Wu
>
>
>

Re: [PATCH v4 5/5] usb: dwc3: rockchip: add devicetree bindings documentation

[Heiko Stuebner]

Hi William,

Am Dienstag, 28. Juni 2016, 11:18:04 schrieb William Wu:
> >> So about the usb3 controller clk management, I think it should contain
> >> the following clk:
> >> 1.  aclk_usb3otg1
> >> 2.  aclk_usb3otg0
> >> 3.  aclk_usb3_grf
> > 
> > correct, aclk_usb3otgX would then be the busclk for each controller if
> > I'm not mistaken and the grf clock should also get enabled, like we
> > also plan on doing for the vio_grf clock in the display area.
> 
> OK, so  aclk_usb3_grf should be marked as critical, right?

nope ... the consensus for the vio_grf clock was that the driver accessing 
it should control it. So for the usb3_grf clock this would be your dwc3-
based driver being responsible for the grf-clock.


> I found that most of the grf clocks haven't been marked as critical,
> apart from vio_grf. So may I keep the aclk_usb3_grf in usb3 dts, and
> remove it after clock manager adds it to critical clocks?

you should keep the usb3-grf clock in the dts all the time.


> >> 4.  aclk_usb3_noc
> >> For "aclk_usb3_noc", I discuss with our clk manager, the clk is always
> >> on before,
> >> but according to upstream maintainer's suggestion, we need to manage
> >> the
> >> noc clk by each module.
> > 
> > can you point me to this discussion? The bus-interconnect is a very
> > separate component, which we don't model so far and thus we have all
> > the noc clocks simply marked as critical.
> > 
> > As this clock doesn't belong to the actual usb controller block, but
> > said
> > separate component, handling it in the controller seems somehow wrong to
> > me.
> > 
> > So my (current) opinion would again be to mark the noc clock as critical
> > for the time being.
> 
> Sorry, it seems that I get the new information about clk management too
> late.:-)
> 
> There's no dedicated discussion about noc clk, but similar to grf clock,
> please refer to "https://patchwork.kernel.org/patch/9171467/" for add
> pclk_vio_grf to critical clock on the RK3399, and you have agreed on
> that mark vio grf clk as critical. So I agree with your opinion, thanks~

The difference as I see it is, that the GRF parts are _part_ of the usb3 
controller, while the interconnect really is a separate component, 
connecting the controller to the rest of the system.

----------	
|  USB3  |----[Interconnect]----[rest of the system]
| [+GRF] |	
----------	

So the controller binding + driver should handle all clocks it needs to 
operate. We currently don't model and handle the separate interconnect 
though, so that noc clock should be critical for the time being.


Heiko

Re: [PATCH v4 5/5] usb: dwc3: rockchip: add devicetree bindings documentation

[William Wu]

Dear Heiko,

On 06/29/2016 12:41 AM, Heiko Stuebner wrote:
> Hi William,
>
> Am Dienstag, 28. Juni 2016, 11:18:04 schrieb William Wu:
>>>> So about the usb3 controller clk management, I think it should contain
>>>> the following clk:
>>>> 1.  aclk_usb3otg1
>>>> 2.  aclk_usb3otg0
>>>> 3.  aclk_usb3_grf
>>> correct, aclk_usb3otgX would then be the busclk for each controller if
>>> I'm not mistaken and the grf clock should also get enabled, like we
>>> also plan on doing for the vio_grf clock in the display area.
>> OK, so  aclk_usb3_grf should be marked as critical, right?
> nope ... the consensus for the vio_grf clock was that the driver accessing
> it should control it. So for the usb3_grf clock this would be your dwc3-
> based driver being responsible for the grf-clock.
Ah, I misunderstand before. I'll control usb3_grf in dwc3 driver.
>
>
>> I found that most of the grf clocks haven't been marked as critical,
>> apart from vio_grf. So may I keep the aclk_usb3_grf in usb3 dts, and
>> remove it after clock manager adds it to critical clocks?
> you should keep the usb3-grf clock in the dts all the time.
OK, I'll keep it in the dts.
>
>
>>>> 4.  aclk_usb3_noc
>>>> For "aclk_usb3_noc", I discuss with our clk manager, the clk is always
>>>> on before,
>>>> but according to upstream maintainer's suggestion, we need to manage
>>>> the
>>>> noc clk by each module.
>>> can you point me to this discussion? The bus-interconnect is a very
>>> separate component, which we don't model so far and thus we have all
>>> the noc clocks simply marked as critical.
>>>
>>> As this clock doesn't belong to the actual usb controller block, but
>>> said
>>> separate component, handling it in the controller seems somehow wrong to
>>> me.
>>>
>>> So my (current) opinion would again be to mark the noc clock as critical
>>> for the time being.
>> Sorry, it seems that I get the new information about clk management too
>> late.:-)
>>
>> There's no dedicated discussion about noc clk, but similar to grf clock,
>> please refer to "https://patchwork.kernel.org/patch/9171467/" for add
>> pclk_vio_grf to critical clock on the RK3399, and you have agreed on
>> that mark vio grf clk as critical. So I agree with your opinion, thanks~
> The difference as I see it is, that the GRF parts are _part_ of the usb3
> controller, while the interconnect really is a separate component,
> connecting the controller to the rest of the system.
>
> ----------	
> |  USB3  |----[Interconnect]----[rest of the system]
> | [+GRF] |	
> ----------	
>
> So the controller binding + driver should handle all clocks it needs to
> operate. We currently don't model and handle the separate interconnect
> though, so that noc clock should be critical for the time being.
Thanks for your professional explanation. Now I think I have
more clearly understood the clocks. And I'll upload a new
patch later.
>
> Heiko
>
>
>
>

Re: [PATCH v4 5/5] usb: dwc3: rockchip: add devicetree bindings documentation

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 431 bytes --]


Hi,

William Wu <william.wu@rock-chips.com> writes:
> This patch adds the devicetree documentation required for Rockchip
> USB3.0 core wrapper consisting of USB3.0 IP from Synopsys.
>
> It supports DRD mode, and could operate in device mode (SS, HS, FS)
> and host mode (SS, HS, FS, LS).
>
> Signed-off-by: William Wu <william.wu@rock-chips.com>

I'm dropping this series until comments are sorted out.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]