[PATCH 0/4] usb: dwc2: add suport for Rockchip dwc2 controller

[PATCH 0/4] usb: dwc2: add suport for Rockchip dwc2 controller

[Kever Yang]

This path is to add the support for dwc2 controller found ind
Rockchip processors rk3066, rk3188 and rk3288

This patch also add dr_mode for dwc2 driver.

Kever Yang (4):
  Documentation: dt-bindings: add dt binding info for Rockchip dwc2
  ARM: dts: add rk3288 dwc2 controller support
  usb: dwc2: add compatible data for rockchip soc
  usb: dwc2: add dr_mode support for dwc2

 Documentation/devicetree/bindings/usb/dwc2.txt |    5 ++++
 arch/arm/boot/dts/rk3288.dtsi                  |   20 ++++++++++++++
 drivers/usb/dwc2/core.c                        |   13 ++++++++++
 drivers/usb/dwc2/core.h                        |    2 ++
 drivers/usb/dwc2/platform.c                    |   33 ++++++++++++++++++++++++
 5 files changed, 73 insertions(+)

-- 
1.7.9.5

[PATCH 1/4] Documentation: dt-bindings: add dt binding info for Rockchip dwc2

[Kever Yang]

This add necessary dwc2 binding documentation for Rockchip socs:
rk3066, rk3188 and rk3288

add dr_mode as optional properties.

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---
 Documentation/devicetree/bindings/usb/dwc2.txt |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/dwc2.txt b/Documentation/devicetree/bindings/usb/dwc2.txt
index aa91034..eb80d7b 100644
--- a/Documentation/devicetree/bindings/usb/dwc2.txt
+++ b/Documentation/devicetree/bindings/usb/dwc2.txt
@@ -4,6 +4,9 @@ Platform DesignWare HS OTG USB 2.0 controller
 Required properties:
 - compatible : One of:
   - brcm,bcm2835-usb: The DWC2 USB controller instance in the BCM2835 SoC.
+  - rockchip,rk3066-usb: The DWC2 USB controller instance in the rk3066 Soc;
+  - "rockchip,rk3188-usb","rockchip,rk3066-usb": for rk3188 Soc;
+  - "rockchip,rk3288-usb","rockchip,rk3066-usb": for rk3288 Soc;
   - snps,dwc2: A generic DWC2 USB controller with default parameters.
 - reg : Should contain 1 register range (address and length)
 - interrupts : Should contain 1 interrupt
@@ -15,6 +18,8 @@ Optional properties:
 - phys: phy provider specifier
 - phy-names: shall be "usb2-phy"
 Refer to phy/phy-bindings.txt for generic phy consumer properties
+- dr_mode: shall be one of "host", "peripheral" and "otg"
+  Refer to usb/generic.txt
 
 Example:
 
-- 
1.7.9.5

[PATCH 1/4] Documentation: dt-bindings: add dt binding info for Rockchip dwc2

[Doug Anderson]

Kever,

On Tue, Jul 29, 2014 at 6:31 PM, Kever Yang <kever.yang@rock-chips.com> wrote:
> This add necessary dwc2 binding documentation for Rockchip socs:
> rk3066, rk3188 and rk3288
>
> add dr_mode as optional properties.
>
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
>  Documentation/devicetree/bindings/usb/dwc2.txt |    5 +++++
>  1 file changed, 5 insertions(+)

Reviewed-by: Doug Anderson <dianders@chromium.org>

[PATCH 1/4] Documentation: dt-bindings: add dt binding info for Rockchip dwc2

[Doug Anderson]

Kever,

On Tue, Jul 29, 2014 at 6:31 PM, Kever Yang <kever.yang@rock-chips.com> wrote:
> This add necessary dwc2 binding documentation for Rockchip socs:
> rk3066, rk3188 and rk3288
>
> add dr_mode as optional properties.
>
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
>  Documentation/devicetree/bindings/usb/dwc2.txt |    5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/dwc2.txt b/Documentation/devicetree/bindings/usb/dwc2.txt
> index aa91034..eb80d7b 100644
> --- a/Documentation/devicetree/bindings/usb/dwc2.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc2.txt
> @@ -4,6 +4,9 @@ Platform DesignWare HS OTG USB 2.0 controller
>  Required properties:
>  - compatible : One of:
>    - brcm,bcm2835-usb: The DWC2 USB controller instance in the BCM2835 SoC.
> +  - rockchip,rk3066-usb: The DWC2 USB controller instance in the rk3066 Soc;
> +  - "rockchip,rk3188-usb","rockchip,rk3066-usb": for rk3188 Soc;
> +  - "rockchip,rk3288-usb","rockchip,rk3066-usb": for rk3288 Soc;
>    - snps,dwc2: A generic DWC2 USB controller with default parameters.
>  - reg : Should contain 1 register range (address and length)
>  - interrupts : Should contain 1 interrupt
> @@ -15,6 +18,8 @@ Optional properties:
>  - phys: phy provider specifier
>  - phy-names: shall be "usb2-phy"
>  Refer to phy/phy-bindings.txt for generic phy consumer properties
> +- dr_mode: shall be one of "host", "peripheral" and "otg"
> +  Refer to usb/generic.txt

One other note: Maybe we should break this series into two different
patch series'.  I think that the patches for doing "dr_mode" can be
totally separate from the ones adding support for rockchip, right?

-Doug

[PATCH 1/4] Documentation: dt-bindings: add dt binding info for Rockchip dwc2

[Paul Zimmerman]

> From: Kever Yang [mailto:kever.yang at rock-chips.com]
> Sent: Tuesday, July 29, 2014 6:31 PM
> 
> This add necessary dwc2 binding documentation for Rockchip socs:
> rk3066, rk3188 and rk3288
> 
> add dr_mode as optional properties.
> 
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
>  Documentation/devicetree/bindings/usb/dwc2.txt |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/dwc2.txt
> b/Documentation/devicetree/bindings/usb/dwc2.txt
> index aa91034..eb80d7b 100644
> --- a/Documentation/devicetree/bindings/usb/dwc2.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc2.txt
> @@ -4,6 +4,9 @@ Platform DesignWare HS OTG USB 2.0 controller
>  Required properties:
>  - compatible : One of:
>    - brcm,bcm2835-usb: The DWC2 USB controller instance in the BCM2835 SoC.
> +  - rockchip,rk3066-usb: The DWC2 USB controller instance in the rk3066 Soc;
> +  - "rockchip,rk3188-usb","rockchip,rk3066-usb": for rk3188 Soc;
> +  - "rockchip,rk3288-usb","rockchip,rk3066-usb": for rk3288 Soc;
>    - snps,dwc2: A generic DWC2 USB controller with default parameters.

Do you really need three different bindings here? I believe the 
recommended approach is to define one binding for the common case, and
reuse it for similar SOCs. Additional bindings should only be added if
there is some difference in the SOC that requires it.

>  - reg : Should contain 1 register range (address and length)
>  - interrupts : Should contain 1 interrupt
> @@ -15,6 +18,8 @@ Optional properties:
>  - phys: phy provider specifier
>  - phy-names: shall be "usb2-phy"
>  Refer to phy/phy-bindings.txt for generic phy consumer properties
> +- dr_mode: shall be one of "host", "peripheral" and "otg"

I don't see where you use 'dr_mode' in any of the DTS files. Are you
going to add the uses later? And please us a more descriptive name,
such as 'dual-role-mode'.

-- 
Paul

[PATCH 1/4] Documentation: dt-bindings: add dt binding info for Rockchip dwc2

[Doug Anderson]

Paul,

On Wed, Jul 30, 2014 at 11:53 AM, Paul Zimmerman
<Paul.Zimmerman@synopsys.com> wrote:
>> From: Kever Yang [mailto:kever.yang at rock-chips.com]
>> Sent: Tuesday, July 29, 2014 6:31 PM
>>
>> This add necessary dwc2 binding documentation for Rockchip socs:
>> rk3066, rk3188 and rk3288
>>
>> add dr_mode as optional properties.
>>
>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>> ---
>>  Documentation/devicetree/bindings/usb/dwc2.txt |    5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/dwc2.txt
>> b/Documentation/devicetree/bindings/usb/dwc2.txt
>> index aa91034..eb80d7b 100644
>> --- a/Documentation/devicetree/bindings/usb/dwc2.txt
>> +++ b/Documentation/devicetree/bindings/usb/dwc2.txt
>> @@ -4,6 +4,9 @@ Platform DesignWare HS OTG USB 2.0 controller
>>  Required properties:
>>  - compatible : One of:
>>    - brcm,bcm2835-usb: The DWC2 USB controller instance in the BCM2835 SoC.
>> +  - rockchip,rk3066-usb: The DWC2 USB controller instance in the rk3066 Soc;
>> +  - "rockchip,rk3188-usb","rockchip,rk3066-usb": for rk3188 Soc;
>> +  - "rockchip,rk3288-usb","rockchip,rk3066-usb": for rk3288 Soc;
>>    - snps,dwc2: A generic DWC2 USB controller with default parameters.
>
> Do you really need three different bindings here? I believe the
> recommended approach is to define one binding for the common case, and
> reuse it for similar SOCs. Additional bindings should only be added if
> there is some difference in the SOC that requires it.

I think that Kever is doing the right thing.  Although we might think
that we are 100% 3066 compatible right now, we might later decide that
there's some difference that we didn't think about.  The standard way
to handle this is to list several different modes like this.  In other
words, we say:

* We are really a rk3288.

* We're either exactly compatible with a rk3066 or mostly compatible
with an rk3066.  We certainly know that if we don't have a special
rk3288 driver that the rk3066 driver works pretty well.

* If we didn't have an rk3066 driver, we might be able to get some
functionality with the generic "snps,dwc2" driver (in fact I can mount
a USB stick and read files with the generic driver).


>>  - reg : Should contain 1 register range (address and length)
>>  - interrupts : Should contain 1 interrupt
>> @@ -15,6 +18,8 @@ Optional properties:
>>  - phys: phy provider specifier
>>  - phy-names: shall be "usb2-phy"
>>  Refer to phy/phy-bindings.txt for generic phy consumer properties
>> +- dr_mode: shall be one of "host", "peripheral" and "otg"
>
> I don't see where you use 'dr_mode' in any of the DTS files. Are you
> going to add the uses later? And please us a more descriptive name,
> such as 'dual-role-mode'.

We have some local patches that do this, since we have another board
where the OTG port needs to be forced into host mode.  As I said above
I think this should be split into a separate patch series, though,
since it's not rk3288-related.

Using dr_mode is the right thing because it matches all of the other
drivers and also the generic bindings.  See Kever's note saying "Refer
to usb/generic.txt".

-Doug

[PATCH 1/4] Documentation: dt-bindings: add dt binding info for Rockchip dwc2

[Paul Zimmerman]

> From: dianders at google.com [mailto:dianders at google.com] On Behalf Of Doug Anderson
> Sent: Wednesday, July 30, 2014 3:29 PM
> 
> On Wed, Jul 30, 2014 at 11:53 AM, Paul Zimmerman
> <Paul.Zimmerman@synopsys.com> wrote:
> >> From: Kever Yang [mailto:kever.yang at rock-chips.com]
> >> Sent: Tuesday, July 29, 2014 6:31 PM
> >>
> >> This add necessary dwc2 binding documentation for Rockchip socs:
> >> rk3066, rk3188 and rk3288
> >>
> >> add dr_mode as optional properties.
> >>
> >> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> >> ---
> >>  Documentation/devicetree/bindings/usb/dwc2.txt |    5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/usb/dwc2.txt
> >> b/Documentation/devicetree/bindings/usb/dwc2.txt
> >> index aa91034..eb80d7b 100644
> >> --- a/Documentation/devicetree/bindings/usb/dwc2.txt
> >> +++ b/Documentation/devicetree/bindings/usb/dwc2.txt
> >> @@ -4,6 +4,9 @@ Platform DesignWare HS OTG USB 2.0 controller
> >>  Required properties:
> >>  - compatible : One of:
> >>    - brcm,bcm2835-usb: The DWC2 USB controller instance in the BCM2835 SoC.
> >> +  - rockchip,rk3066-usb: The DWC2 USB controller instance in the rk3066 Soc;
> >> +  - "rockchip,rk3188-usb","rockchip,rk3066-usb": for rk3188 Soc;
> >> +  - "rockchip,rk3288-usb","rockchip,rk3066-usb": for rk3288 Soc;
> >>    - snps,dwc2: A generic DWC2 USB controller with default parameters.
> >
> > Do you really need three different bindings here? I believe the
> > recommended approach is to define one binding for the common case, and
> > reuse it for similar SOCs. Additional bindings should only be added if
> > there is some difference in the SOC that requires it.
> 
> I think that Kever is doing the right thing.  Although we might think
> that we are 100% 3066 compatible right now, we might later decide that
> there's some difference that we didn't think about.  The standard way
> to handle this is to list several different modes like this.  In other
> words, we say:
> 
> * We are really a rk3288.
> 
> * We're either exactly compatible with a rk3066 or mostly compatible
> with an rk3066.  We certainly know that if we don't have a special
> rk3288 driver that the rk3066 driver works pretty well.
> 
> * If we didn't have an rk3066 driver, we might be able to get some
> functionality with the generic "snps,dwc2" driver (in fact I can mount
> a USB stick and read files with the generic driver).

OK, I'll take your word for it, unless someone else objects.

> >>  - reg : Should contain 1 register range (address and length)
> >>  - interrupts : Should contain 1 interrupt
> >> @@ -15,6 +18,8 @@ Optional properties:
> >>  - phys: phy provider specifier
> >>  - phy-names: shall be "usb2-phy"
> >>  Refer to phy/phy-bindings.txt for generic phy consumer properties
> >> +- dr_mode: shall be one of "host", "peripheral" and "otg"
> >
> > I don't see where you use 'dr_mode' in any of the DTS files. Are you
> > going to add the uses later? And please us a more descriptive name,
> > such as 'dual-role-mode'.
> 
> We have some local patches that do this, since we have another board
> where the OTG port needs to be forced into host mode.  As I said above
> I think this should be split into a separate patch series, though,
> since it's not rk3288-related.
> 
> Using dr_mode is the right thing because it matches all of the other
> drivers and also the generic bindings.  See Kever's note saying "Refer
> to usb/generic.txt".

Whoops, yes, I see a lot of other platforms us 'dr_mode' as well. Never
mind me, then :)

-- 
Paul

[PATCH 2/4] ARM: dts: add rk3288 dwc2 controller support

[Kever Yang]

rk3288 has two kind of usb controller, this add the dwc2 controller
for otg and host1.

Controller can works with usb PHY default setting and Vbus on.

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---
 arch/arm/boot/dts/rk3288.dtsi |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index abc51f5..4309c4f 100644
--- a/arch/arm/boot/dts/rk3288.dtsi
+++ b/arch/arm/boot/dts/rk3288.dtsi
@@ -646,5 +646,25 @@
 			clock-names = "baudclk", "apb_pclk";
 			status = "disabled";
 		};
+
+		usb_otg: dwc2 at ff580000 {
+			compatible = "rockchip,rk3288-usb", "rockchip,rk3066-usb",
+					"snps,dwc2";
+			reg = <0xff580000 0x40000>;
+			interrupts = <GIC_SPI 23 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cru HCLK_OTG0>;
+			clock-names = "otg";
+			status = "disabled";
+		};
+
+		usb_host1: dwc2 at ff540000 {
+			compatible = "rockchip,rk3288-usb", "rockchip,rk3066-usb",
+					"snps,dwc2";
+			reg = <0xff540000 0x40000>;
+			interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cru HCLK_USBHOST1>;
+			clock-names = "otg";
+			status = "disabled";
+		};
 	};
 };
-- 
1.7.9.5

[PATCH 2/4] ARM: dts: add rk3288 dwc2 controller support

[Doug Anderson]

Kever,

On Tue, Jul 29, 2014 at 6:34 PM, Kever Yang <kever.yang@rock-chips.com> wrote:
> rk3288 has two kind of usb controller, this add the dwc2 controller
> for otg and host1.
>
> Controller can works with usb PHY default setting and Vbus on.
>
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
>  arch/arm/boot/dts/rk3288.dtsi |   20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
> index abc51f5..4309c4f 100644
> --- a/arch/arm/boot/dts/rk3288.dtsi
> +++ b/arch/arm/boot/dts/rk3288.dtsi

Maybe move the dtsi patch to the 3rd or 4th patch in the series?
Things won't work super well without patch #3, right?

Also: do you want to add a 5th patch in the series that enables the
ports like <https://chromium-review.googlesource.com/#/c/210066/>


> @@ -646,5 +646,25 @@
>                         clock-names = "baudclk", "apb_pclk";
>                         status = "disabled";
>                 };
> +
> +               usb_otg: dwc2 at ff580000 {
> +                       compatible = "rockchip,rk3288-usb", "rockchip,rk3066-usb",
> +                                       "snps,dwc2";

You included the snps,dwc2 (at my request) here.  ...but you didn't
add it to the bindings (patch #1).  You should probably add it to the
bindings in the next version unless someone else thinks it shouldn't
be here.


> +                       reg = <0xff580000 0x40000>;
> +                       interrupts = <GIC_SPI 23 IRQ_TYPE_LEVEL_HIGH>;
> +                       clocks = <&cru HCLK_OTG0>;
> +                       clock-names = "otg";
> +                       status = "disabled";
> +               };
> +
> +               usb_host1: dwc2 at ff540000 {

Please sort by base address.  ...and if Heiko Acks my EHCI device tree
patches maybe rebase atop my patches to avoid merge conflicts?


> +                       compatible = "rockchip,rk3288-usb", "rockchip,rk3066-usb",
> +                                       "snps,dwc2";
> +                       reg = <0xff540000 0x40000>;
> +                       interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>;
> +                       clocks = <&cru HCLK_USBHOST1>;
> +                       clock-names = "otg";
> +                       status = "disabled";
> +               };
>         };
>  };

[PATCH 2/4] ARM: dts: add rk3288 dwc2 controller support

[Sergei Shtylyov]

Hello.

On 07/30/2014 05:34 AM, Kever Yang wrote:

> rk3288 has two kind of usb controller, this add the dwc2 controller
> for otg and host1.

> Controller can works with usb PHY default setting and Vbus on.

> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
>   arch/arm/boot/dts/rk3288.dtsi |   20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)

> diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
> index abc51f5..4309c4f 100644
> --- a/arch/arm/boot/dts/rk3288.dtsi
> +++ b/arch/arm/boot/dts/rk3288.dtsi
> @@ -646,5 +646,25 @@
>   			clock-names = "baudclk", "apb_pclk";
>   			status = "disabled";
>   		};
> +
> +		usb_otg: dwc2 at ff580000 {

    The ePAPR standard [1] says:

The name of a node should be somewhat generic, reflecting the function of the 
device and not its precise programming model. If appropriate, the name should 
be one of the following choices:

[...]
    ? usb

> +			compatible = "rockchip,rk3288-usb", "rockchip,rk3066-usb",
> +					"snps,dwc2";
> +			reg = <0xff580000 0x40000>;
> +			interrupts = <GIC_SPI 23 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&cru HCLK_OTG0>;
> +			clock-names = "otg";
> +			status = "disabled";
> +		};
> +
> +		usb_host1: dwc2 at ff540000 {

    Same here.

WBR, Sergei

[PATCH 2/4] ARM: dts: add rk3288 dwc2 controller support

[Sergei Shtylyov]

On 07/30/2014 07:18 PM, Sergei Shtylyov wrote:

>> rk3288 has two kind of usb controller, this add the dwc2 controller
>> for otg and host1.

>> Controller can works with usb PHY default setting and Vbus on.

>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>> ---
>>   arch/arm/boot/dts/rk3288.dtsi |   20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)

>> diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
>> index abc51f5..4309c4f 100644
>> --- a/arch/arm/boot/dts/rk3288.dtsi
>> +++ b/arch/arm/boot/dts/rk3288.dtsi
>> @@ -646,5 +646,25 @@
>>               clock-names = "baudclk", "apb_pclk";
>>               status = "disabled";
>>           };
>> +
>> +        usb_otg: dwc2 at ff580000 {

>     The ePAPR standard [1] says:

> The name of a node should be somewhat generic, reflecting the function of the
> device and not its precise programming model. If appropriate, the name should
> be one of the following choices:

> [...]
>     ? usb

>> +            compatible = "rockchip,rk3288-usb", "rockchip,rk3066-usb",
>> +                    "snps,dwc2";
>> +            reg = <0xff580000 0x40000>;
>> +            interrupts = <GIC_SPI 23 IRQ_TYPE_LEVEL_HIGH>;
>> +            clocks = <&cru HCLK_OTG0>;
>> +            clock-names = "otg";
>> +            status = "disabled";
>> +        };
>> +
>> +        usb_host1: dwc2 at ff540000 {

>     Same here.

    Oops, forgot to give the ePAPR link:

[1] http://www.power.org/resources/downloads/Power_ePAPR_APPROVED_v1.0.pdf

WBR, Sergei

[PATCH 3/4] usb: dwc2: add compatible data for rockchip soc

[Kever Yang]

This patch add compatible data for dwc2 controller found on
rk3066, rk3188 and rk3288 processors from rockchip.

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---
 drivers/usb/dwc2/platform.c |   29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index a10e7a3..cc5983c 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -75,6 +75,34 @@ static const struct dwc2_core_params params_bcm2835 = {
 	.uframe_sched			= 0,
 };
 
+static const struct dwc2_core_params params_rk3066 = {
+	.otg_cap			= 2,	/* no HNP/SRP capable */
+	.otg_ver			= 0,	/* 1.3 */
+	.dma_enable			= 1,
+	.dma_desc_enable		= 0,
+	.speed				= 0,	/* High Speed */
+	.enable_dynamic_fifo		= 1,
+	.en_multiple_tx_fifo		= 1,
+	.host_rx_fifo_size		= 520,	/* 520 DWORDs */
+	.host_nperio_tx_fifo_size	= 128,	/* 128 DWORDs */
+	.host_perio_tx_fifo_size	= 256,	/* 256 DWORDs */
+	.max_transfer_size		= 65536,
+	.max_packet_count		= 512,
+	.host_channels			= 9,
+	.phy_type			= 1,	/* UTMI */
+	.phy_utmi_width			= 16,	/* 8 bits */
+	.phy_ulpi_ddr			= 0,	/* Single */
+	.phy_ulpi_ext_vbus		= 0,
+	.i2c_enable			= 0,
+	.ulpi_fs_ls			= 0,
+	.host_support_fs_ls_low_power	= 0,
+	.host_ls_low_power_phy_clk	= 0,	/* 48 MHz */
+	.ts_dline			= 0,
+	.reload_ctl			= 1,
+	.ahbcfg				= 0x17, /* dma enable &  INCR16 */
+	.uframe_sched			= 1,
+};
+
 /**
  * dwc2_driver_remove() - Called when the DWC_otg core is unregistered with the
  * DWC_otg driver
@@ -97,6 +125,7 @@ static int dwc2_driver_remove(struct platform_device *dev)
 
 static const struct of_device_id dwc2_of_match_table[] = {
 	{ .compatible = "brcm,bcm2835-usb", .data = &params_bcm2835 },
+	{ .compatible = "rockchip,rk3066-usb", .data = &params_rk3066 },
 	{ .compatible = "snps,dwc2", .data = NULL },
 	{},
 };
-- 
1.7.9.5

[PATCH 3/4] usb: dwc2: add compatible data for rockchip soc

[Doug Anderson]

Kever,

On Tue, Jul 29, 2014 at 6:35 PM, Kever Yang <kever.yang@rock-chips.com> wrote:
> This patch add compatible data for dwc2 controller found on
> rk3066, rk3188 and rk3288 processors from rockchip.
>
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
>  drivers/usb/dwc2/platform.c |   29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)

I'm nowhere an expert here, but...


> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index a10e7a3..cc5983c 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -75,6 +75,34 @@ static const struct dwc2_core_params params_bcm2835 = {
>         .uframe_sched                   = 0,
>  };
>
> +static const struct dwc2_core_params params_rk3066 = {
> +       .otg_cap                        = 2,    /* no HNP/SRP capable */

Are you sure HNP/SRP is not available?  Do things break if you leave this at 0?


> +       .otg_ver                        = 0,    /* 1.3 */
> +       .dma_enable                     = 1,
> +       .dma_desc_enable                = 0,
> +       .speed                          = 0,    /* High Speed */
> +       .enable_dynamic_fifo            = 1,
> +       .en_multiple_tx_fifo            = 1,
> +       .host_rx_fifo_size              = 520,  /* 520 DWORDs */
> +       .host_nperio_tx_fifo_size       = 128,  /* 128 DWORDs */
> +       .host_perio_tx_fifo_size        = 256,  /* 256 DWORDs */
> +       .max_transfer_size              = 65536,
> +       .max_packet_count               = 512,

Header file says max_packet_count should be max of 511.


> +       .host_channels                  = 9,
> +       .phy_type                       = 1,    /* UTMI */
> +       .phy_utmi_width                 = 16,   /* 8 bits */

Either comment or value is wrong since 16 != 8 bits.


> +       .phy_ulpi_ddr                   = 0,    /* Single */
> +       .phy_ulpi_ext_vbus              = 0,
> +       .i2c_enable                     = 0,
> +       .ulpi_fs_ls                     = 0,
> +       .host_support_fs_ls_low_power   = 0,
> +       .host_ls_low_power_phy_clk      = 0,    /* 48 MHz */
> +       .ts_dline                       = 0,
> +       .reload_ctl                     = 1,
> +       .ahbcfg                         = 0x17, /* dma enable &  INCR16 */
> +       .uframe_sched                   = 1,
> +};

Many of these values could just be -1 to autodetect / use default.
Should we consider doing that?

-Doug

[PATCH 3/4] usb: dwc2: add compatible data for rockchip soc

[Kever Yang]

Hi Doug:

On 07/29/2014 09:21 PM, Doug Anderson wrote:
> Kever,
>
> On Tue, Jul 29, 2014 at 6:35 PM, Kever Yang <kever.yang@rock-chips.com> wrote:
>> This patch add compatible data for dwc2 controller found on
>> rk3066, rk3188 and rk3288 processors from rockchip.
>>
>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>> ---
>>   drivers/usb/dwc2/platform.c |   29 +++++++++++++++++++++++++++++
>>   1 file changed, 29 insertions(+)
> I'm nowhere an expert here, but...
>
>
>> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
>> index a10e7a3..cc5983c 100644
>> --- a/drivers/usb/dwc2/platform.c
>> +++ b/drivers/usb/dwc2/platform.c
>> @@ -75,6 +75,34 @@ static const struct dwc2_core_params params_bcm2835 = {
>>          .uframe_sched                   = 0,
>>   };
>>
>> +static const struct dwc2_core_params params_rk3066 = {
>> +       .otg_cap                        = 2,    /* no HNP/SRP capable */
> Are you sure HNP/SRP is not available?  Do things break if you leave this at 0?
1. HNP/SRP is not need right now, I didn't see it used in mobile devices.
2. If HNP/SRP is enabled, the controller will monitor the otg_vbus about 5V,
     overcurrent change will happen if vbus not detect after switch to host
     for a period of time.
     If not, the controller will monitor b_valid signal about 3V instead 
of 5V vbus,
     which match the hardware reference design from Rockchip.
>
>
>> +       .otg_ver                        = 0,    /* 1.3 */
>> +       .dma_enable                     = 1,
>> +       .dma_desc_enable                = 0,
>> +       .speed                          = 0,    /* High Speed */
>> +       .enable_dynamic_fifo            = 1,
>> +       .en_multiple_tx_fifo            = 1,
>> +       .host_rx_fifo_size              = 520,  /* 520 DWORDs */
>> +       .host_nperio_tx_fifo_size       = 128,  /* 128 DWORDs */
>> +       .host_perio_tx_fifo_size        = 256,  /* 256 DWORDs */
>> +       .max_transfer_size              = 65536,
>> +       .max_packet_count               = 512,
> Header file says max_packet_count should be max of 511.
Yeap, you are right, I will fix this.
>
>
>> +       .host_channels                  = 9,
>> +       .phy_type                       = 1,    /* UTMI */
>> +       .phy_utmi_width                 = 16,   /* 8 bits */
> Either comment or value is wrong since 16 != 8 bits.
I will change this to auto detect.
>
>
>> +       .phy_ulpi_ddr                   = 0,    /* Single */
>> +       .phy_ulpi_ext_vbus              = 0,
>> +       .i2c_enable                     = 0,
>> +       .ulpi_fs_ls                     = 0,
>> +       .host_support_fs_ls_low_power   = 0,
>> +       .host_ls_low_power_phy_clk      = 0,    /* 48 MHz */
>> +       .ts_dline                       = 0,
>> +       .reload_ctl                     = 1,
>> +       .ahbcfg                         = 0x17, /* dma enable &  INCR16 */
>> +       .uframe_sched                   = 1,
>> +};
> Many of these values could just be -1 to autodetect / use default.
> Should we consider doing that?
Most of the parameter can be auto-detect right except some have more than
one choice, so I will leave the parameters to driver if it can do it right.
>
> -Doug
>
>
>

[PATCH 3/4] usb: dwc2: add compatible data for rockchip soc

[Doug Anderson]

Kever,

On Tue, Jul 29, 2014 at 11:40 PM, Kever Yang <kever.yang@rock-chips.com> wrote:
>> Are you sure HNP/SRP is not available?  Do things break if you leave this
>> at 0?
>
> 1. HNP/SRP is not need right now, I didn't see it used in mobile devices.
> 2. If HNP/SRP is enabled, the controller will monitor the otg_vbus about 5V,
>     overcurrent change will happen if vbus not detect after switch to host
>     for a period of time.
>     If not, the controller will monitor b_valid signal about 3V instead of
> 5V vbus,
>     which match the hardware reference design from Rockchip.

OK.  USB is pretty far away from my realm of expertise and I knew even
less about OTG/gadget.  It does sound like there's no fundamental
issue with HNP/SRP on the SoC itself but that we're working around an
issue with the board, though.  If that's true, then perhaps we need
another device tree property?

-Doug

[PATCH 3/4] usb: dwc2: add compatible data for rockchip soc

[Paul Zimmerman]

> From: Kever Yang [mailto:kever.yang at rock-chips.com]
> Sent: Tuesday, July 29, 2014 6:35 PM
> 
> This patch add compatible data for dwc2 controller found on
> rk3066, rk3188 and rk3288 processors from rockchip.
> 
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
>  drivers/usb/dwc2/platform.c |   29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index a10e7a3..cc5983c 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -75,6 +75,34 @@ static const struct dwc2_core_params params_bcm2835 = {
>  	.uframe_sched			= 0,
>  };
> 
> +static const struct dwc2_core_params params_rk3066 = {
> +	.otg_cap			= 2,	/* no HNP/SRP capable */
> +	.otg_ver			= 0,	/* 1.3 */
> +	.dma_enable			= 1,
> +	.dma_desc_enable		= 0,
> +	.speed				= 0,	/* High Speed */
> +	.enable_dynamic_fifo		= 1,
> +	.en_multiple_tx_fifo		= 1,
> +	.host_rx_fifo_size		= 520,	/* 520 DWORDs */
> +	.host_nperio_tx_fifo_size	= 128,	/* 128 DWORDs */
> +	.host_perio_tx_fifo_size	= 256,	/* 256 DWORDs */
> +	.max_transfer_size		= 65536,
> +	.max_packet_count		= 512,
> +	.host_channels			= 9,
> +	.phy_type			= 1,	/* UTMI */
> +	.phy_utmi_width			= 16,	/* 8 bits */

The comment doesn't match the value.

> +	.phy_ulpi_ddr			= 0,	/* Single */
> +	.phy_ulpi_ext_vbus		= 0,
> +	.i2c_enable			= 0,
> +	.ulpi_fs_ls			= 0,
> +	.host_support_fs_ls_low_power	= 0,
> +	.host_ls_low_power_phy_clk	= 0,	/* 48 MHz */
> +	.ts_dline			= 0,
> +	.reload_ctl			= 1,
> +	.ahbcfg				= 0x17, /* dma enable &  INCR16 */

Don't set the dma enable bit here, the driver will set that bit
according to the '.dma_enable' member above.

-- 
Paul

[PATCH 4/4] usb: dwc2: add dr_mode support for dwc2

[Kever Yang]

Some devices with A female host port and without use of usb_id pin
will need this for the otg controller works as device role
during firmware period and works as host role in rich os.

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---
 drivers/usb/dwc2/core.c     |   13 +++++++++++++
 drivers/usb/dwc2/core.h     |    2 ++
 drivers/usb/dwc2/platform.c |    4 ++++
 3 files changed, 19 insertions(+)

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index 27d2c9b..6688951 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -118,6 +118,7 @@ static int dwc2_core_reset(struct dwc2_hsotg *hsotg)
 {
 	u32 greset;
 	int count = 0;
+	u32 gusbcfg;
 
 	dev_vdbg(hsotg->dev, "%s()\n", __func__);
 
@@ -148,6 +149,18 @@ static int dwc2_core_reset(struct dwc2_hsotg *hsotg)
 		}
 	} while (greset & GRSTCTL_CSFTRST);
 
+	if (hsotg->dr_mode == USB_DR_MODE_HOST) {
+		gusbcfg = readl(hsotg->regs+GUSBCFG);
+		gusbcfg &= ~GUSBCFG_FORCEDEVMODE;
+		gusbcfg |= GUSBCFG_FORCEHOSTMODE;
+		writel(gusbcfg, hsotg->regs+GUSBCFG);
+	} else if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL) {
+		gusbcfg = readl(hsotg->regs+GUSBCFG);
+		gusbcfg &= ~GUSBCFG_FORCEHOSTMODE;
+		gusbcfg |= GUSBCFG_FORCEDEVMODE;
+		writel(gusbcfg, hsotg->regs+GUSBCFG);
+	}
+
 	/*
 	 * NOTE: This long sleep is _very_ important, otherwise the core will
 	 * not stay in host mode after a connector ID change!
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 1efd10c..9fe960b 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -501,6 +501,7 @@ struct dwc2_hw_params {
  *                      a_peripheral and b_device=>b_host) this may not match
  *                      the core, but allows the software to determine
  *                      transitions
+ * @dr_mode:            requested mode of operation
  * @queuing_high_bandwidth: True if multiple packets of a high-bandwidth
  *                      transfer are in process of being queued
  * @srp_success:        Stores status of SRP request in the case of a FS PHY
@@ -592,6 +593,7 @@ struct dwc2_hsotg {
 	/** Params to actually use */
 	struct dwc2_core_params *core_params;
 	enum usb_otg_state op_state;
+	enum usb_dr_mode        dr_mode;
 
 	unsigned int queuing_high_bandwidth:1;
 	unsigned int srp_success:1;
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index cc5983c..a2ac1ea 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -42,6 +42,8 @@
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 
+#include <linux/usb/of.h>
+
 #include "core.h"
 #include "hcd.h"
 
@@ -200,6 +202,8 @@ static int dwc2_driver_probe(struct platform_device *dev)
 	dev_dbg(&dev->dev, "mapped PA %08lx to VA %p\n",
 		(unsigned long)res->start, hsotg->regs);
 
+	hsotg->dr_mode = of_usb_get_dr_mode(dev->dev.of_node);
+
 	retval = dwc2_hcd_init(hsotg, irq, params);
 	if (retval)
 		return retval;
-- 
1.7.9.5

[PATCH 4/4] usb: dwc2: add dr_mode support for dwc2

[Doug Anderson]

Kever,

On Tue, Jul 29, 2014 at 6:35 PM, Kever Yang <kever.yang@rock-chips.com> wrote:
> Some devices with A female host port and without use of usb_id pin
> will need this for the otg controller works as device role
> during firmware period and works as host role in rich os.
>
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
>  drivers/usb/dwc2/core.c     |   13 +++++++++++++
>  drivers/usb/dwc2/core.h     |    2 ++
>  drivers/usb/dwc2/platform.c |    4 ++++
>  3 files changed, 19 insertions(+)
>
> diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
> index 27d2c9b..6688951 100644
> --- a/drivers/usb/dwc2/core.c
> +++ b/drivers/usb/dwc2/core.c
> @@ -118,6 +118,7 @@ static int dwc2_core_reset(struct dwc2_hsotg *hsotg)
>  {
>         u32 greset;
>         int count = 0;
> +       u32 gusbcfg;
>
>         dev_vdbg(hsotg->dev, "%s()\n", __func__);
>
> @@ -148,6 +149,18 @@ static int dwc2_core_reset(struct dwc2_hsotg *hsotg)
>                 }
>         } while (greset & GRSTCTL_CSFTRST);
>
> +       if (hsotg->dr_mode == USB_DR_MODE_HOST) {
> +               gusbcfg = readl(hsotg->regs+GUSBCFG);
> +               gusbcfg &= ~GUSBCFG_FORCEDEVMODE;
> +               gusbcfg |= GUSBCFG_FORCEHOSTMODE;
> +               writel(gusbcfg, hsotg->regs+GUSBCFG);
> +       } else if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL) {
> +               gusbcfg = readl(hsotg->regs+GUSBCFG);
> +               gusbcfg &= ~GUSBCFG_FORCEHOSTMODE;
> +               gusbcfg |= GUSBCFG_FORCEDEVMODE;
> +               writel(gusbcfg, hsotg->regs+GUSBCFG);
> +       }

Don't you want to handle "dr_mode == USB_DR_MODE_OTG" here too?

[PATCH 4/4] usb: dwc2: add dr_mode support for dwc2

[Paul Zimmerman]

> From: Kever Yang [mailto:kever.yang at rock-chips.com]
> Sent: Tuesday, July 29, 2014 6:35 PM
> 
> Some devices with A female host port and without use of usb_id pin
> will need this for the otg controller works as device role
> during firmware period and works as host role in rich os.
> 
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
>  drivers/usb/dwc2/core.c     |   13 +++++++++++++
>  drivers/usb/dwc2/core.h     |    2 ++
>  drivers/usb/dwc2/platform.c |    4 ++++
>  3 files changed, 19 insertions(+)
> 
> diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
> index 27d2c9b..6688951 100644
> --- a/drivers/usb/dwc2/core.c
> +++ b/drivers/usb/dwc2/core.c
> @@ -118,6 +118,7 @@ static int dwc2_core_reset(struct dwc2_hsotg *hsotg)
>  {
>  	u32 greset;
>  	int count = 0;
> +	u32 gusbcfg;
> 
>  	dev_vdbg(hsotg->dev, "%s()\n", __func__);
> 
> @@ -148,6 +149,18 @@ static int dwc2_core_reset(struct dwc2_hsotg *hsotg)
>  		}
>  	} while (greset & GRSTCTL_CSFTRST);
> 
> +	if (hsotg->dr_mode == USB_DR_MODE_HOST) {
> +		gusbcfg = readl(hsotg->regs+GUSBCFG);
> +		gusbcfg &= ~GUSBCFG_FORCEDEVMODE;
> +		gusbcfg |= GUSBCFG_FORCEHOSTMODE;
> +		writel(gusbcfg, hsotg->regs+GUSBCFG);
> +	} else if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL) {
> +		gusbcfg = readl(hsotg->regs+GUSBCFG);
> +		gusbcfg &= ~GUSBCFG_FORCEHOSTMODE;
> +		gusbcfg |= GUSBCFG_FORCEDEVMODE;
> +		writel(gusbcfg, hsotg->regs+GUSBCFG);
> +	}

Please put spaces around the '+' operator. Did you run these patches
through checkpatch?

> +
>  	/*
>  	 * NOTE: This long sleep is _very_ important, otherwise the core will
>  	 * not stay in host mode after a connector ID change!
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 1efd10c..9fe960b 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -501,6 +501,7 @@ struct dwc2_hw_params {
>   *                      a_peripheral and b_device=>b_host) this may not match
>   *                      the core, but allows the software to determine
>   *                      transitions
> + * @dr_mode:            requested mode of operation

Please expand this comment, e.g.
"Requested mode of operation (host/peripheral/dual-role)"

-- 
Paul