Message ID | 1329393818-24552-5-git-send-email-amit.virdi@st.com |
---|---|
State | Superseded |
Delegated to: | Marek Vasut |
Headers | show |
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
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 > . >
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 > > .
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
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
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
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 --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; }