[PATCH] fusb300_udc: modify stall clear and idma reset procedure

[PATCH] fusb300_udc: modify stall clear and idma reset procedure

[Yuan-Hsin Chen]

From: Yuan-Hsin Chen <yuanlmm@gmail.com>

Due to fusb300 controller modification, stall clear procedure should be
modified consistantly. This patch also fixes software bugs: only
enter IDMA_RESET when the condition matched and disable corresponding
PRD interrupt in IDMA_RESET.


Signed-off-by: Yuan-Hsin Chen <yhchen@faraday-tech.com>
---
 drivers/usb/gadget/fusb300_udc.c |    9 ++++++---
 drivers/usb/gadget/fusb300_udc.h |    2 +-
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/gadget/fusb300_udc.c b/drivers/usb/gadget/fusb300_udc.c
index 72cd5e6..109cab1 100644
--- a/drivers/usb/gadget/fusb300_udc.c
+++ b/drivers/usb/gadget/fusb300_udc.c
@@ -394,7 +394,7 @@ static void fusb300_clear_epnstall(struct fusb300 *fusb300, u8 ep)
 
 	if (reg & FUSB300_EPSET0_STL) {
 		printk(KERN_DEBUG "EP%d stall... Clear!!\n", ep);
-		reg &= ~FUSB300_EPSET0_STL;
+		reg |= FUSB300_EPSET0_STL_CLR;
 		iowrite32(reg, fusb300->reg + FUSB300_OFFSET_EPSET0(ep));
 	}
 }
@@ -930,9 +930,12 @@ static void fusb300_wait_idma_finished(struct fusb300_ep *ep)
 
 	fusb300_clear_int(ep->fusb300, FUSB300_OFFSET_IGR0,
 		FUSB300_IGR0_EPn_PRD_INT(ep->epnum));
+	return;
+
 IDMA_RESET:
-	fusb300_clear_int(ep->fusb300, FUSB300_OFFSET_IGER0,
-		FUSB300_IGER0_EEPn_PRD_INT(ep->epnum));
+	reg = ioread32(ep->fusb300->reg + FUSB300_OFFSET_IGER0);
+	reg &= ~FUSB300_IGER0_EEPn_PRD_INT(ep->epnum);
+	iowrite32(reg, ep->fusb300->reg + FUSB300_OFFSET_IGER0);
 }
 
 static void  fusb300_set_idma(struct fusb300_ep *ep,
diff --git a/drivers/usb/gadget/fusb300_udc.h b/drivers/usb/gadget/fusb300_udc.h
index 542cd83..ccae1b5 100644
--- a/drivers/usb/gadget/fusb300_udc.h
+++ b/drivers/usb/gadget/fusb300_udc.h
@@ -111,8 +111,8 @@
 /*
  * * EPn Setting 0 (EPn_SET0, offset = 020H+(n-1)*30H, n=1~15 )
  * */
+#define FUSB300_EPSET0_STL_CLR		(1 << 3)
 #define FUSB300_EPSET0_CLRSEQNUM	(1 << 2)
-#define FUSB300_EPSET0_EPn_TX0BYTE	(1 << 1)
 #define FUSB300_EPSET0_STL		(1 << 0)
 
 /*
-- 
1.7.4.1

Re: [PATCH] fusb300_udc: modify stall clear and idma reset procedure

[Felipe Balbi]

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

Hi,

On Fri, Feb 22, 2013 at 07:09:39AM +0000, Yuan-Hsin Chen wrote:
> From: Yuan-Hsin Chen <yuanlmm@gmail.com>
> 
> Due to fusb300 controller modification, stall clear procedure should be
> modified consistantly. This patch also fixes software bugs: only
> enter IDMA_RESET when the condition matched and disable corresponding
> PRD interrupt in IDMA_RESET.
> 
> 
> Signed-off-by: Yuan-Hsin Chen <yhchen@faraday-tech.com>

I have been trying to understand if this is a bug fix or if it can wait
until v3.10.

Also, according to your commit log, it seems like this patch is doing
more than one thing, which is really frowned upon. Please split it up
into separate logically self-contained changes. While doing that, make
sure that your commit log explains the problem and the solution very
well. Saying that because of a controller modification you need to
change stall clear consistently isn't enough.

Don't forget to mention how and with which platforms you tested.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] fusb300_udc: modify stall clear and idma reset procedure

[Yuan-Hsin Chen]

Hi,

On Wed, Feb 27, 2013 at 1:59 AM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Fri, Feb 22, 2013 at 07:09:39AM +0000, Yuan-Hsin Chen wrote:
>> From: Yuan-Hsin Chen <yuanlmm@gmail.com>
>>
>> Due to fusb300 controller modification, stall clear procedure should be
>> modified consistantly. This patch also fixes software bugs: only
>> enter IDMA_RESET when the condition matched and disable corresponding
>> PRD interrupt in IDMA_RESET.
>>
>>
>> Signed-off-by: Yuan-Hsin Chen <yhchen@faraday-tech.com>
>
> I have been trying to understand if this is a bug fix or if it can wait
> until v3.10.

There are a software bug fix and two corresponding fixes to hw register
 modification in the patch. Should it wait until v3.10?
>
> Also, according to your commit log, it seems like this patch is doing
> more than one thing, which is really frowned upon. Please split it up
> into separate logically self-contained changes. While doing that, make
> sure that your commit log explains the problem and the solution very
> well. Saying that because of a controller modification you need to
> change stall clear consistently isn't enough.

I will split it up and explain more clearly.
Thanks.
>
> Don't forget to mention how and with which platforms you tested.
>
> --
> balbi

[PATCH v2 1/2] usb: gadget: fusb300_udc: add FUSB300_EPSET0_STL_CLR for clearing EP0 stall

[Yuan-Hsin Chen]

The final version of fusb300 controller adds EPSET0_STL_CLR
for clearing EP0 stall and also removes EPSET0_EPn_TX0BYTE.

fusb300_udc driver is tested on FARADAY platform a369 with
FUSB300 FPGA v1.8

Signed-off-by: Yuan-Hsin Chen <yhchen@faraday-tech.com>
---

v2:
split patch

 drivers/usb/gadget/fusb300_udc.c |    2 +-
 drivers/usb/gadget/fusb300_udc.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/fusb300_udc.c b/drivers/usb/gadget/fusb300_udc.c
index 72cd5e6..c20cc58 100644
--- a/drivers/usb/gadget/fusb300_udc.c
+++ b/drivers/usb/gadget/fusb300_udc.c
@@ -394,7 +394,7 @@ static void fusb300_clear_epnstall(struct fusb300 *fusb300, u8 ep)
 
 	if (reg & FUSB300_EPSET0_STL) {
 		printk(KERN_DEBUG "EP%d stall... Clear!!\n", ep);
-		reg &= ~FUSB300_EPSET0_STL;
+		reg |= FUSB300_EPSET0_STL_CLR;
 		iowrite32(reg, fusb300->reg + FUSB300_OFFSET_EPSET0(ep));
 	}
 }
diff --git a/drivers/usb/gadget/fusb300_udc.h b/drivers/usb/gadget/fusb300_udc.h
index 542cd83..ccae1b5 100644
--- a/drivers/usb/gadget/fusb300_udc.h
+++ b/drivers/usb/gadget/fusb300_udc.h
@@ -111,8 +111,8 @@
 /*
  * * EPn Setting 0 (EPn_SET0, offset = 020H+(n-1)*30H, n=1~15 )
  * */
+#define FUSB300_EPSET0_STL_CLR		(1 << 3)
 #define FUSB300_EPSET0_CLRSEQNUM	(1 << 2)
-#define FUSB300_EPSET0_EPn_TX0BYTE	(1 << 1)
 #define FUSB300_EPSET0_STL		(1 << 0)
 
 /*
-- 
1.7.4.1

[PATCH v2 2/2] usb: gadget: fusb300_udc: bug fix of not doing idma reset for each time

[Yuan-Hsin Chen]

Enter IDMA_RESET only when the controller has been reset or
the device has been plugged in to or out from a host. In
IDMA_RESET, we should disable the corresponding PRD interrupt.
Also there is a redundant space eliminated.

fusb300_udc driver is tested on FARADAY platform a369 with
FUSB300 FPGA v1.8

Signed-off-by: Yuan-Hsin Chen <yhchen@faraday-tech.com>
---

v2:
split patch

 drivers/usb/gadget/fusb300_udc.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/fusb300_udc.c b/drivers/usb/gadget/fusb300_udc.c
index c20cc58..c4dc906 100644
--- a/drivers/usb/gadget/fusb300_udc.c
+++ b/drivers/usb/gadget/fusb300_udc.c
@@ -930,12 +930,15 @@ static void fusb300_wait_idma_finished(struct fusb300_ep *ep)
 
 	fusb300_clear_int(ep->fusb300, FUSB300_OFFSET_IGR0,
 		FUSB300_IGR0_EPn_PRD_INT(ep->epnum));
+	return;
+
 IDMA_RESET:
-	fusb300_clear_int(ep->fusb300, FUSB300_OFFSET_IGER0,
-		FUSB300_IGER0_EEPn_PRD_INT(ep->epnum));
+	reg = ioread32(ep->fusb300->reg + FUSB300_OFFSET_IGER0);
+	reg &= ~FUSB300_IGER0_EEPn_PRD_INT(ep->epnum);
+	iowrite32(reg, ep->fusb300->reg + FUSB300_OFFSET_IGER0);
 }
 
-static void  fusb300_set_idma(struct fusb300_ep *ep,
+static void fusb300_set_idma(struct fusb300_ep *ep,
 			struct fusb300_request *req)
 {
 	dma_addr_t d;
-- 
1.7.4.1