[PATCH v1 1/3] usb: dwc2: reset AHB hclk domain before init

[PATCH v1 1/3] usb: dwc2: reset AHB hclk domain before init

[Yunzhi Li]

We initiate dwc2 usb controller in BIOS, when kernel driver
start-up we should reset AHB hclk domain to reset all AHB
interface registers to default. Without this the FIFO value
setting might be incorrect because calculating FIFO size need the
power-on value of GRXFSIZ/GNPTXFSIZ/HPTXFSIZ registers.

This patch could avoid warnning massage like in rk3288 platform:
[    2.074764] dwc2 ff580000.usb: 256 invalid for
host_perio_tx_fifo_size. Check HW configuration.

Signed-off-by: Yunzhi Li <lyz@rock-chips.com>
---

 drivers/usb/dwc2/platform.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 9093530..ec6bf6b 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -42,6 +42,7 @@
 #include <linux/of_device.h>
 #include <linux/mutex.h>
 #include <linux/platform_device.h>
+#include <linux/reset.h>
 
 #include <linux/usb/of.h>
 
@@ -165,6 +166,7 @@ static int dwc2_driver_probe(struct platform_device *dev)
 	struct resource *res;
 	struct phy *phy;
 	struct usb_phy *uphy;
+	struct reset_control *rst;
 	int retval;
 	int irq;
 
@@ -189,6 +191,16 @@ static int dwc2_driver_probe(struct platform_device *dev)
 
 	hsotg->dev = &dev->dev;
 
+	/* AHB hclk domain reset, set all AHB interface registers to default */
+	rst = devm_reset_control_get_optional(&dev->dev, "ahb");
+	if (IS_ERR(rst)) {
+		dev_dbg(&dev->dev, "Can't get dwc2 AHB reset\n");
+	} else {
+		reset_control_assert(rst);
+		udelay(5);
+		reset_control_deassert(rst);
+	}
+
 	/*
 	 * Use reasonable defaults so platforms don't have to provide these.
 	 */
-- 
2.0.0

[PATCH v1 2/3] Documentation: dt-bindings: add dt binding info for dwc2 reset control

[Yunzhi Li]

Signed-off-by: Yunzhi Li <lyz@rock-chips.com>
---

 Documentation/devicetree/bindings/usb/dwc2.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/dwc2.txt b/Documentation/devicetree/bindings/usb/dwc2.txt
index fd132cb..6a84099 100644
--- a/Documentation/devicetree/bindings/usb/dwc2.txt
+++ b/Documentation/devicetree/bindings/usb/dwc2.txt
@@ -24,6 +24,12 @@ Refer to phy/phy-bindings.txt for generic phy consumer properties
 - g-rx-fifo-size: size of rx fifo size in gadget mode.
 - g-np-tx-fifo-size: size of non-periodic tx fifo size in gadget mode.
 - g-tx-fifo-size: size of periodic tx fifo per endpoint (except ep0) in gadget mode.
+- resets: A list of phandle + reset-specifier pairs for the resets listed in
+  reset-names
+- reset-names: Should contain the following:
+  "ahb": AHB interface reset
+  "phy": PHY reset
+  "con": controller reset
 
 Example:
 
-- 
2.0.0

Re: [PATCH v1 2/3] Documentation: dt-bindings: add dt binding info for dwc2 reset control

[Doug Anderson]

lyz,

On Tue, Aug 11, 2015 at 12:56 AM, Yunzhi Li <lyz@rock-chips.com> wrote:
> Signed-off-by: Yunzhi Li <lyz@rock-chips.com>
> ---
>
>  Documentation/devicetree/bindings/usb/dwc2.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/dwc2.txt b/Documentation/devicetree/bindings/usb/dwc2.txt
> index fd132cb..6a84099 100644
> --- a/Documentation/devicetree/bindings/usb/dwc2.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc2.txt
> @@ -24,6 +24,12 @@ Refer to phy/phy-bindings.txt for generic phy consumer properties
>  - g-rx-fifo-size: size of rx fifo size in gadget mode.
>  - g-np-tx-fifo-size: size of non-periodic tx fifo size in gadget mode.
>  - g-tx-fifo-size: size of periodic tx fifo per endpoint (except ep0) in gadget mode.
> +- resets: A list of phandle + reset-specifier pairs for the resets listed in
> +  reset-names
> +- reset-names: Should contain the following:
> +  "ahb": AHB interface reset
> +  "phy": PHY reset
> +  "con": controller reset

Would it be worth it to leave the "phy" out?  You don't use it in the
driver yet and it's unclear to me whether it belongs here or as part
of "rockchip,rk3288-usb-phy"?

-Doug

[PATCH v1 3/3] ARM: dts: rockchip: add dwc2 ahb reset property for rk3288

[Yunzhi Li]

This patch adds dwc2 reset property for rk3288 dwc2 usb
controller to fix FIFO setting bug

Signed-off-by: Yunzhi Li <lyz@rock-chips.com>

---

 arch/arm/boot/dts/rk3288.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index 22316d0..440aa42 100644
--- a/arch/arm/boot/dts/rk3288.dtsi
+++ b/arch/arm/boot/dts/rk3288.dtsi
@@ -473,6 +473,10 @@
 		dr_mode = "host";
 		phys = <&usbphy2>;
 		phy-names = "usb2-phy";
+		resets = <&cru SRST_USBHOST1_AHB>,
+			 <&cru SRST_USBHOST1_PHY>,
+			 <&cru SRST_USBHOST1_CON>;
+		reset-names = "ahb", "phy", "con";
 		status = "disabled";
 	};
 
@@ -490,6 +494,10 @@
 		g-use-dma;
 		phys = <&usbphy0>;
 		phy-names = "usb2-phy";
+		resets = <&cru SRST_USBOTG_AHB>,
+			 <&cru SRST_USBOTG_PHY>,
+			 <&cru SRST_USBOTG_CON>;
+		reset-names = "ahb", "phy", "con";
 		status = "disabled";
 	};
 
-- 
2.0.0

Re: [PATCH v1 1/3] usb: dwc2: reset AHB hclk domain before init

[John Youn]

On 8/11/2015 12:57 AM, Yunzhi Li wrote:
> We initiate dwc2 usb controller in BIOS, when kernel driver
> start-up we should reset AHB hclk domain to reset all AHB
> interface registers to default. Without this the FIFO value
> setting might be incorrect because calculating FIFO size need the
> power-on value of GRXFSIZ/GNPTXFSIZ/HPTXFSIZ registers.
> 
> This patch could avoid warnning massage like in rk3288 platform:
> [    2.074764] dwc2 ff580000.usb: 256 invalid for
> host_perio_tx_fifo_size. Check HW configuration.
> 
> Signed-off-by: Yunzhi Li <lyz@rock-chips.com>
> ---
> 
>  drivers/usb/dwc2/platform.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index 9093530..ec6bf6b 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -42,6 +42,7 @@
>  #include <linux/of_device.h>
>  #include <linux/mutex.h>
>  #include <linux/platform_device.h>
> +#include <linux/reset.h>
>  
>  #include <linux/usb/of.h>
>  
> @@ -165,6 +166,7 @@ static int dwc2_driver_probe(struct platform_device *dev)
>  	struct resource *res;
>  	struct phy *phy;
>  	struct usb_phy *uphy;
> +	struct reset_control *rst;
>  	int retval;
>  	int irq;
>  
> @@ -189,6 +191,16 @@ static int dwc2_driver_probe(struct platform_device *dev)
>  
>  	hsotg->dev = &dev->dev;
>  
> +	/* AHB hclk domain reset, set all AHB interface registers to default */
> +	rst = devm_reset_control_get_optional(&dev->dev, "ahb");
> +	if (IS_ERR(rst)) {
> +		dev_dbg(&dev->dev, "Can't get dwc2 AHB reset\n");
> +	} else {
> +		reset_control_assert(rst);
> +		udelay(5);
> +		reset_control_deassert(rst);
> +	}
> +
>  	/*
>  	 * Use reasonable defaults so platforms don't have to provide these.
>  	 */
> 

I didn't receive the other two patches in this series so I was
confused about where the "ahb_reset" was coming from when I
replied to your other patch.

I see you changed the name and documented the DT so never mind.

Another thing is that there probably shouldn't be a debug
message on the IS_ERR condition since that is the common case
and of no interest to other platforms.

The other two resets you added aren't used by the driver
anywhere right? Maybe those should be left out until they are.

John

Re: [PATCH v1 1/3] usb: dwc2: reset AHB hclk domain before init

[Yunzhi Li]

在 2015/8/14 8:09, John Youn 写道:
> On 8/11/2015 12:57 AM, Yunzhi Li wrote:
>> We initiate dwc2 usb controller in BIOS, when kernel driver
>> start-up we should reset AHB hclk domain to reset all AHB
>> interface registers to default. Without this the FIFO value
>> setting might be incorrect because calculating FIFO size need the
>> power-on value of GRXFSIZ/GNPTXFSIZ/HPTXFSIZ registers.
>>
>> This patch could avoid warnning massage like in rk3288 platform:
>> [    2.074764] dwc2 ff580000.usb: 256 invalid for
>> host_perio_tx_fifo_size. Check HW configuration.
>>
>> ......
> I didn't receive the other two patches in this series so I was
> confused about where the "ahb_reset" was coming from when I
> replied to your other patch.
>
> I see you changed the name and documented the DT so never mind.
>
> Another thing is that there probably shouldn't be a debug
> message on the IS_ERR condition since that is the common case
> and of no interest to other platforms.
>
> The other two resets you added aren't used by the driver
> anywhere right? Maybe those should be left out until they are.
>
> John
>
Hi John ,

   Here is the other two patches :
     https://patchwork.kernel.org/patch/6989541/
     https://patchwork.kernel.org/patch/6989531/

   ahb_reset is hreset_n signal of dwc2 IP. Our rk3288 SoC implement 
connect this signal to a special
register in clock ang reset unit (CRU) module, set this register will 
reset dwc2 control and status registers(CSR)
to default value. You could find more info in <<DesignWare Cores USB 2.0 
Hi Speed On-TheGo (OTG) Databook 3.10a>>
4.4.1 System Clock and Reset Signals.

   Our problem is that dwc2_get_hwparams() reads fifo size registers and 
reguards it as the power-on reset value,
then dwc2_set_param_host_perio_tx_fifo_size() will check this value and 
make sure the new fifo size value is no bigger
than the power-on reset value. But we init and set these fifo registers 
in BIOS, so here hw->xxx_fifo_size is not the
real power-on reset vaule. So we hope to reset CSR before 
dwc2_get_hwparams().

I have another ideal: we might use GRSTCTL.CSftRst instead of hreset_n 
to reset dwc2 CSR.

Re: [PATCH v1 1/3] usb: dwc2: reset AHB hclk domain before init

[John Youn]

On 8/13/2015 8:29 PM, Yunzhi Li wrote:
> 
> 
> 在 2015/8/14 8:09, John Youn 写道:
>> On 8/11/2015 12:57 AM, Yunzhi Li wrote:
>>> We initiate dwc2 usb controller in BIOS, when kernel driver
>>> start-up we should reset AHB hclk domain to reset all AHB
>>> interface registers to default. Without this the FIFO value
>>> setting might be incorrect because calculating FIFO size need the
>>> power-on value of GRXFSIZ/GNPTXFSIZ/HPTXFSIZ registers.
>>>
>>> This patch could avoid warnning massage like in rk3288 platform:
>>> [    2.074764] dwc2 ff580000.usb: 256 invalid for
>>> host_perio_tx_fifo_size. Check HW configuration.
>>>
>>> ......
>> I didn't receive the other two patches in this series so I was
>> confused about where the "ahb_reset" was coming from when I
>> replied to your other patch.
>>
>> I see you changed the name and documented the DT so never mind.
>>
>> Another thing is that there probably shouldn't be a debug
>> message on the IS_ERR condition since that is the common case
>> and of no interest to other platforms.
>>
>> The other two resets you added aren't used by the driver
>> anywhere right? Maybe those should be left out until they are.
>>
>> John
>>
> Hi John ,
> 
>    Here is the other two patches :
>      https://patchwork.kernel.org/patch/6989541/
>      https://patchwork.kernel.org/patch/6989531/
> 
>    ahb_reset is hreset_n signal of dwc2 IP. Our rk3288 SoC implement 
> connect this signal to a special
> register in clock ang reset unit (CRU) module, set this register will 
> reset dwc2 control and status registers(CSR)
> to default value. You could find more info in <<DesignWare Cores USB 2.0 
> Hi Speed On-TheGo (OTG) Databook 3.10a>>
> 4.4.1 System Clock and Reset Signals.
> 
>    Our problem is that dwc2_get_hwparams() reads fifo size registers and 
> reguards it as the power-on reset value,
> then dwc2_set_param_host_perio_tx_fifo_size() will check this value and 
> make sure the new fifo size value is no bigger
> than the power-on reset value. But we init and set these fifo registers 
> in BIOS, so here hw->xxx_fifo_size is not the
> real power-on reset vaule. So we hope to reset CSR before 
> dwc2_get_hwparams().
> 
> I have another ideal: we might use GRSTCTL.CSftRst instead of hreset_n 
> to reset dwc2 CSR.


Yes, please try doing that before calling dwc2_get_hwparams().
Maybe by calling dwc2_core_reset(). If that works for you, I think
it would be better.

John

Re: [PATCH v1 1/3] usb: dwc2: reset AHB hclk domain before init

[Yunzhi Li]

Hi John,

在 2015/8/15 3:41, John Youn 写道:
> On 8/13/2015 8:29 PM, Yunzhi Li wrote:
>>
>> 在 2015/8/14 8:09, John Youn 写道:
>>> On 8/11/2015 12:57 AM, Yunzhi Li wrote:
>>>> We initiate dwc2 usb controller in BIOS, when kernel driver
>>>> start-up we should reset AHB hclk domain to reset all AHB
>>>> interface registers to default. Without this the FIFO value
>>>> setting might be incorrect because calculating FIFO size need the
>>>> power-on value of GRXFSIZ/GNPTXFSIZ/HPTXFSIZ registers.
>>>>
>>>> This patch could avoid warnning massage like in rk3288 platform:
>>>> [    2.074764] dwc2 ff580000.usb: 256 invalid for
>>>> host_perio_tx_fifo_size. Check HW configuration.
>>>>
>>>> ......
>>> I didn't receive the other two patches in this series so I was
>>> confused about where the "ahb_reset" was coming from when I
>>> replied to your other patch.
>>>
>>> I see you changed the name and documented the DT so never mind.
>>>
>>> Another thing is that there probably shouldn't be a debug
>>> message on the IS_ERR condition since that is the common case
>>> and of no interest to other platforms.
>>>
>>> The other two resets you added aren't used by the driver
>>> anywhere right? Maybe those should be left out until they are.
>>>
>>> John
>>>
>> Hi John ,
>>
>>    Here is the other two patches :
>>      https://patchwork.kernel.org/patch/6989541/
>>      https://patchwork.kernel.org/patch/6989531/
>>
>>    ahb_reset is hreset_n signal of dwc2 IP. Our rk3288 SoC implement 
>> connect this signal to a special
>> register in clock ang reset unit (CRU) module, set this register will 
>> reset dwc2 control and status registers(CSR)
>> to default value. You could find more info in <<DesignWare Cores USB 2.0 
>> Hi Speed On-TheGo (OTG) Databook 3.10a>>
>> 4.4.1 System Clock and Reset Signals.
>>
>>    Our problem is that dwc2_get_hwparams() reads fifo size registers and 
>> reguards it as the power-on reset value,
>> then dwc2_set_param_host_perio_tx_fifo_size() will check this value and 
>> make sure the new fifo size value is no bigger
>> than the power-on reset value. But we init and set these fifo registers 
>> in BIOS, so here hw->xxx_fifo_size is not the
>> real power-on reset vaule. So we hope to reset CSR before 
>> dwc2_get_hwparams().
>>
>> I have another ideal: we might use GRSTCTL.CSftRst instead of hreset_n 
>> to reset dwc2 CSR.
>
> Yes, please try doing that before calling dwc2_get_hwparams().
> Maybe by calling dwc2_core_reset(). If that works for you, I think
> it would be better.
>
> John
>
>
It works and please help review the new patch.
Thanks.