diff mbox

[U-Boot,4/4] USB:gadget:designware Fix memory nonalignment issue

Message ID 1329393818-24552-5-git-send-email-amit.virdi@st.com
State Superseded
Delegated to: Marek Vasut
Headers show

Commit Message

Amit Virdi Feb. 16, 2012, 12:03 p.m. UTC 
From: Shiraz Hashim <shiraz.hashim@st.com>

While receiving packets from FIFO sometimes the buffer provided was
nonaligned. Fix this by taking a temporary aligned buffer and then
copying the content to nonaligned buffer.

Signed-off-by: Shiraz Hashim <shiraz.hashim@st.com>
Signed-off-by: Amit Virdi <amit.virdi@st.com>
---
 drivers/usb/gadget/designware_udc.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

Comments

Marek Vasut March 5, 2012, 6:21 p.m. UTC | #1 
Dear Amit Virdi,

> From: Shiraz Hashim <shiraz.hashim@st.com>
> 
> While receiving packets from FIFO sometimes the buffer provided was
> nonaligned. Fix this by taking a temporary aligned buffer and then
> copying the content to nonaligned buffer.
> 
> Signed-off-by: Shiraz Hashim <shiraz.hashim@st.com>
> Signed-off-by: Amit Virdi <amit.virdi@st.com>
> ---
>  drivers/usb/gadget/designware_udc.c |   15 ++++++++++++++-
>  1 files changed, 14 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/usb/gadget/designware_udc.c
> b/drivers/usb/gadget/designware_udc.c index d4b53a2..878123c 100644
> --- a/drivers/usb/gadget/designware_udc.c
> +++ b/drivers/usb/gadget/designware_udc.c
> @@ -202,6 +202,7 @@ static int usbgetpckfromfifo(int epNum, u8 *bufp, u32
> len) u32 i, nw, nb;
>  	u32 *wrdp;
>  	u8 *bytp;
> +	u32 tmp[128];
> 
>  	if (readl(&udc_regs_p->dev_stat) & DEV_STAT_RXFIFO_EMPTY)
>  		return -1;
> @@ -209,7 +210,12 @@ static int usbgetpckfromfifo(int epNum, u8 *bufp, u32
> len) nw = len / sizeof(u32);
>  	nb = len % sizeof(u32);
> 
> -	wrdp = (u32 *)bufp;
> +	/* use tmp buf if bufp is not word aligned */
> +	if ((int)bufp & 0x3)
> +		wrdp = (u32 *)&tmp[0];
> +	else
> +		wrdp = (u32 *)bufp;
> +
>  	for (i = 0; i < nw; i++) {
>  		writel(readl(fifo_ptr), wrdp);
>  		wrdp++;
> @@ -223,6 +229,13 @@ static int usbgetpckfromfifo(int epNum, u8 *bufp, u32
> len) }
>  	readl(&outep_regs_p[epNum].write_done);
> 
> +	/* copy back tmp buffer to bufp if bufp is not word aligned */
> +	if ((int)bufp & 0x3) {
> +		bytp = (u8 *)&tmp[0];
> +		for (i = 0; i < len; i++)
> +			bufp[i] = bytp[i];
> +	}
> +
>  	return 0;
>  }

This addresses EHCI cache problem, that's why you need bounce buffer, right? 

This is the patch from Puneet, can you give it a go and tell me if EHCI works 
for you without 4/4 patch from your series or do you still see issues?

http://permalink.gmane.org/gmane.comp.boot-loaders.u-boot/126447

Thanks in advance!

Best regards,
Marek Vasut
Amit Virdi March 6, 2012, 9:36 a.m. UTC | #2 
Dear Marek,

On 3/5/2012 11:51 PM, Marek Vasut wrote:
> Dear Amit Virdi,
>
>> From: Shiraz Hashim<shiraz.hashim@st.com>
>>
>> While receiving packets from FIFO sometimes the buffer provided was
>> nonaligned. Fix this by taking a temporary aligned buffer and then
>> copying the content to nonaligned buffer.
>>
>> Signed-off-by: Shiraz Hashim<shiraz.hashim@st.com>
>> Signed-off-by: Amit Virdi<amit.virdi@st.com>
>> ---
>>   drivers/usb/gadget/designware_udc.c |   15 ++++++++++++++-
>>   1 files changed, 14 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/designware_udc.c
>> b/drivers/usb/gadget/designware_udc.c index d4b53a2..878123c 100644
>> --- a/drivers/usb/gadget/designware_udc.c
>> +++ b/drivers/usb/gadget/designware_udc.c
>> @@ -202,6 +202,7 @@ static int usbgetpckfromfifo(int epNum, u8 *bufp, u32
>> len) u32 i, nw, nb;
>>   	u32 *wrdp;
>>   	u8 *bytp;
>> +	u32 tmp[128];
>>
>>   	if (readl(&udc_regs_p->dev_stat)&  DEV_STAT_RXFIFO_EMPTY)
>>   		return -1;
>> @@ -209,7 +210,12 @@ static int usbgetpckfromfifo(int epNum, u8 *bufp, u32
>> len) nw = len / sizeof(u32);
>>   	nb = len % sizeof(u32);
>>
>> -	wrdp = (u32 *)bufp;
>> +	/* use tmp buf if bufp is not word aligned */
>> +	if ((int)bufp&  0x3)
>> +		wrdp = (u32 *)&tmp[0];
>> +	else
>> +		wrdp = (u32 *)bufp;
>> +
>>   	for (i = 0; i<  nw; i++) {
>>   		writel(readl(fifo_ptr), wrdp);
>>   		wrdp++;
>> @@ -223,6 +229,13 @@ static int usbgetpckfromfifo(int epNum, u8 *bufp, u32
>> len) }
>>   	readl(&outep_regs_p[epNum].write_done);
>>
>> +	/* copy back tmp buffer to bufp if bufp is not word aligned */
>> +	if ((int)bufp&  0x3) {
>> +		bytp = (u8 *)&tmp[0];
>> +		for (i = 0; i<  len; i++)
>> +			bufp[i] = bytp[i];
>> +	}
>> +
>>   	return 0;
>>   }
>
> This addresses EHCI cache problem, that's why you need bounce buffer, right?
>

No. The problem was we were copying data word-by-word to a non-word 
aligned memory in the USB gadget. So, this is different from the USB 
host controller issue.


Thanks n Regards
Amit Virdi

> This is the patch from Puneet, can you give it a go and tell me if EHCI works
> for you without 4/4 patch from your series or do you still see issues?
>
> http://permalink.gmane.org/gmane.comp.boot-loaders.u-boot/126447
>
> Thanks in advance!
>
> Best regards,
> Marek Vasut
> .
>
Marek Vasut March 6, 2012, 9:51 a.m. UTC | #3 
Dear Amit Virdi,

> Dear Marek,
> 
> On 3/5/2012 11:51 PM, Marek Vasut wrote:
> > Dear Amit Virdi,
> > 
> >> From: Shiraz Hashim<shiraz.hashim@st.com>
> >> 
> >> While receiving packets from FIFO sometimes the buffer provided was
> >> nonaligned. Fix this by taking a temporary aligned buffer and then
> >> copying the content to nonaligned buffer.
> >> 
> >> Signed-off-by: Shiraz Hashim<shiraz.hashim@st.com>
> >> Signed-off-by: Amit Virdi<amit.virdi@st.com>
> >> ---
> >> 
> >>   drivers/usb/gadget/designware_udc.c |   15 ++++++++++++++-
> >>   1 files changed, 14 insertions(+), 1 deletions(-)
> >> 
> >> diff --git a/drivers/usb/gadget/designware_udc.c
> >> b/drivers/usb/gadget/designware_udc.c index d4b53a2..878123c 100644
> >> --- a/drivers/usb/gadget/designware_udc.c
> >> +++ b/drivers/usb/gadget/designware_udc.c
> >> @@ -202,6 +202,7 @@ static int usbgetpckfromfifo(int epNum, u8 *bufp,
> >> u32 len) u32 i, nw, nb;
> >> 
> >>   	u32 *wrdp;
> >>   	u8 *bytp;
> >> 
> >> +	u32 tmp[128];
> >> 
> >>   	if (readl(&udc_regs_p->dev_stat)&  DEV_STAT_RXFIFO_EMPTY)
> >>   	
> >>   		return -1;
> >> 
> >> @@ -209,7 +210,12 @@ static int usbgetpckfromfifo(int epNum, u8 *bufp,
> >> u32 len) nw = len / sizeof(u32);
> >> 
> >>   	nb = len % sizeof(u32);
> >> 
> >> -	wrdp = (u32 *)bufp;
> >> +	/* use tmp buf if bufp is not word aligned */
> >> +	if ((int)bufp&  0x3)
> >> +		wrdp = (u32 *)&tmp[0];
> >> +	else
> >> +		wrdp = (u32 *)bufp;
> >> +
> >> 
> >>   	for (i = 0; i<  nw; i++) {
> >>   	
> >>   		writel(readl(fifo_ptr), wrdp);
> >>   		wrdp++;
> >> 
> >> @@ -223,6 +229,13 @@ static int usbgetpckfromfifo(int epNum, u8 *bufp,
> >> u32 len) }
> >> 
> >>   	readl(&outep_regs_p[epNum].write_done);
> >> 
> >> +	/* copy back tmp buffer to bufp if bufp is not word aligned */
> >> +	if ((int)bufp&  0x3) {
> >> +		bytp = (u8 *)&tmp[0];
> >> +		for (i = 0; i<  len; i++)
> >> +			bufp[i] = bytp[i];
> >> +	}
> >> +
> >> 
> >>   	return 0;
> >>   
> >>   }
> > 
> > This addresses EHCI cache problem, that's why you need bounce buffer,
> > right?
> 
> No. The problem was we were copying data word-by-word to a non-word
> aligned memory in the USB gadget. So, this is different from the USB
> host controller issue.
> 

I see ... why isn't buffer aligned by the usb stack then?

Thanks!

> 
> Thanks n Regards
> Amit Virdi
> 
> > This is the patch from Puneet, can you give it a go and tell me if EHCI
> > works for you without 4/4 patch from your series or do you still see
> > issues?
> > 
> > http://permalink.gmane.org/gmane.comp.boot-loaders.u-boot/126447
> > 
> > Thanks in advance!
> > 
> > Best regards,
> > Marek Vasut
> > .
Mike Frysinger March 6, 2012, 4:08 p.m. UTC | #4 
On Tuesday 06 March 2012 04:51:57 Marek Vasut wrote:
> > On 3/5/2012 11:51 PM, Marek Vasut wrote:
> > > Amit Virdi wrote:
> > >> While receiving packets from FIFO sometimes the buffer provided was
> > >> nonaligned. Fix this by taking a temporary aligned buffer and then
> > >> copying the content to nonaligned buffer.
> > >> 
> > >> --- a/drivers/usb/gadget/designware_udc.c
> > >> +++ b/drivers/usb/gadget/designware_udc.c
> > >> @@ -202,6 +202,7 @@ static int usbgetpckfromfifo(int epNum, u8 *bufp,
> > >> u32 len) u32 i, nw, nb;
> > >> 
> > >>   	u32 *wrdp;
> > >>   	u8 *bytp;
> > >> 
> > >> +	u32 tmp[128];
> > >> 
> > >>   	if (readl(&udc_regs_p->dev_stat)&  DEV_STAT_RXFIFO_EMPTY)
> > >>   	
> > >>   		return -1;
> > >> 
> > >> @@ -209,7 +210,12 @@ static int usbgetpckfromfifo(int epNum, u8 *bufp,
> > >> u32 len) nw = len / sizeof(u32);
> > >> 
> > >>   	nb = len % sizeof(u32);
> > >> 
> > >> -	wrdp = (u32 *)bufp;
> > >> +	/* use tmp buf if bufp is not word aligned */
> > >> +	if ((int)bufp&  0x3)
> > >> +		wrdp = (u32 *)&tmp[0];
> > >> +	else
> > >> +		wrdp = (u32 *)bufp;
> > >> +
> > >> 
> > >>   	for (i = 0; i<  nw; i++) {
> > >>   	
> > >>   		writel(readl(fifo_ptr), wrdp);
> > >>   		wrdp++;
> > >> 
> > >> @@ -223,6 +229,13 @@ static int usbgetpckfromfifo(int epNum, u8 *bufp,
> > >> u32 len) }
> > >> 
> > >>   	readl(&outep_regs_p[epNum].write_done);
> > >> 
> > >> +	/* copy back tmp buffer to bufp if bufp is not word aligned */
> > >> +	if ((int)bufp&  0x3) {
> > >> +		bytp = (u8 *)&tmp[0];
> > >> +		for (i = 0; i<  len; i++)
> > >> +			bufp[i] = bytp[i];
> > >> +	}
> > >> +
> > >> 
> > >>   	return 0;
> > >>   
> > >>   }
> > > 
> > > This addresses EHCI cache problem, that's why you need bounce buffer,
> > > right?
> > 
> > No. The problem was we were copying data word-by-word to a non-word
> > aligned memory in the USB gadget. So, this is different from the USB
> > host controller issue.
> 
> I see ... why isn't buffer aligned by the usb stack then?

because it might not be a requirement higher up.  i.e. it's dealing with a 
data byte stream.  forcing all higher layers to use 32bit alignment because 
this host controller requires 32bit alignment in its FIFOs is incorrect.
-mike
Mike Frysinger March 6, 2012, 4:09 p.m. UTC | #5 
On Thursday 16 February 2012 07:03:38 Amit Virdi wrote:
> --- a/drivers/usb/gadget/designware_udc.c
> +++ b/drivers/usb/gadget/designware_udc.c
> 
> +	/* copy back tmp buffer to bufp if bufp is not word aligned */
> +	if ((int)bufp & 0x3) {
> +		bytp = (u8 *)&tmp[0];
> +		for (i = 0; i < len; i++)
> +			bufp[i] = bytp[i];
> +	}

memcpy(bufp, tmp, len) ?
-mike
Amit Virdi March 7, 2012, 7:04 a.m. UTC | #6 
Hey Mike,

On 3/6/2012 9:39 PM, Mike Frysinger wrote:
> On Thursday 16 February 2012 07:03:38 Amit Virdi wrote:
>> --- a/drivers/usb/gadget/designware_udc.c
>> +++ b/drivers/usb/gadget/designware_udc.c
>>
>> +	/* copy back tmp buffer to bufp if bufp is not word aligned */
>> +	if ((int)bufp&  0x3) {
>> +		bytp = (u8 *)&tmp[0];
>> +		for (i = 0; i<  len; i++)
>> +			bufp[i] = bytp[i];
>> +	}
>
> memcpy(bufp, tmp, len) ?

Yes, memcpy can be used. I'll amend the code.

Thanks
Amit Virdi
Amit Virdi March 7, 2012, 7:14 a.m. UTC | #7 
Dear Marek,

>>>>>    }
>>>>
>>>> This addresses EHCI cache problem, that's why you need bounce buffer,
>>>> right?
>>>
>>> No. The problem was we were copying data word-by-word to a non-word
>>> aligned memory in the USB gadget. So, this is different from the USB
>>> host controller issue.
>>
>> I see ... why isn't buffer aligned by the usb stack then?
>
> because it might not be a requirement higher up.  i.e. it's dealing with a
> data byte stream.  forcing all higher layers to use 32bit alignment because
> this host controller requires 32bit alignment in its FIFOs is incorrect.

The usb gadget is used by the usbtty so we can receive any number of 
bytes and, as Mike explains, we cannot force 32-bit alignment.

Thanks
Amit Virdi
diff mbox

Patch

diff --git a/drivers/usb/gadget/designware_udc.c b/drivers/usb/gadget/designware_udc.c
index d4b53a2..878123c 100644
--- a/drivers/usb/gadget/designware_udc.c
+++ b/drivers/usb/gadget/designware_udc.c
@@ -202,6 +202,7 @@  static int usbgetpckfromfifo(int epNum, u8 *bufp, u32 len)
 	u32 i, nw, nb;
 	u32 *wrdp;
 	u8 *bytp;
+	u32 tmp[128];
 
 	if (readl(&udc_regs_p->dev_stat) & DEV_STAT_RXFIFO_EMPTY)
 		return -1;
@@ -209,7 +210,12 @@  static int usbgetpckfromfifo(int epNum, u8 *bufp, u32 len)
 	nw = len / sizeof(u32);
 	nb = len % sizeof(u32);
 
-	wrdp = (u32 *)bufp;
+	/* use tmp buf if bufp is not word aligned */
+	if ((int)bufp & 0x3)
+		wrdp = (u32 *)&tmp[0];
+	else
+		wrdp = (u32 *)bufp;
+
 	for (i = 0; i < nw; i++) {
 		writel(readl(fifo_ptr), wrdp);
 		wrdp++;
@@ -223,6 +229,13 @@  static int usbgetpckfromfifo(int epNum, u8 *bufp, u32 len)
 	}
 	readl(&outep_regs_p[epNum].write_done);
 
+	/* copy back tmp buffer to bufp if bufp is not word aligned */
+	if ((int)bufp & 0x3) {
+		bytp = (u8 *)&tmp[0];
+		for (i = 0; i < len; i++)
+			bufp[i] = bytp[i];
+	}
+
 	return 0;
 }