| Message ID | 20120509000221.GA19525@lovely.krouter (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
Hi Christoph, > -----Original Message----- > From: Christoph Fritz [mailto:chf.fritz@googlemail.com] > Sent: 2012年5月9日 8:02 > To: Fabio Estevam; Neil Zhang; Li Yang; Felipe Balbi; Greg Kroah- > Hartman; linux-usb@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; Hans > J. Koch; Daniel Mack > Cc: Thomas Dahlmann; Nicolas Ferre; Eric Miao; Russell King; Haojian > Zhuang; Ben Dooks; Kukjin Kim; Sascha Hauer; Oliver Neukum; Chen Peter- > B29397; Ido Shayevitz; Estevam Fabio-R49496 > Subject: [RFC] [PATCH] usb: gadget: fix dtd dma confusion > > On Wed, Apr 11, 2012 at 10:15:49AM -0300, Fabio Estevam wrote: > > On Wed, Apr 11, 2012 at 4:39 AM, Christoph Fritz > > <chf.fritz@googlemail.com> wrote: > > > > > Can you approve this behaviour of g_ether/usbnet when using iperf? > > > > I am not very familiar with iperf, so I will let others comment. > > Hi to All, > > after a while of testing and searching I can come up with a patch > that fixes g_ether <-> iperf for fsl_udc on ARM i.MX35. > > The sad part is that this kind of fix is already implemented for > marvell mv_udc driver since last year but still _not_ in the ~15 > other *udc.c drivers. > > See here: > daec765da767e4a6a30e1298862b28f2cae9a73f > usb: gadget: mv_udc: fix dtd dma confusion > > So hereby I'm CC-ing all *udc.c maintainers to point out that this > issue maybe affects you too. > > And maybe someone can explain if this: > + if (curr_td->next_td_ptr == > EP_QUEUE_HEAD_NEXT_TERMINATE) { > is necessary and why (please see below). > The outer loop is to check whether the controller still point to the current dtd. If so, we should wait it to move onto the next dtd. But there is an exception, if the current dtd is the last dtd on this ep queue, then the controller can never move on. Fortunately, we can check register epstatus (offset: 0x1b8) to see whether the buffer is free again. > Thanks, > -- Christoph > > --- > Subject: [PATCH] usb: gadget: fsl_udc: fix dtd dma confusion > > The controller will hang when doing testings with g_ether and iperf > (tool for measuring maximum TCP and UDP bandwidth). This patch adds a > delay to wait for controller to release dtd dma before freeing it. > > Signed-off-by: Christoph Fritz <chf.fritz@googlemail.com> > --- > drivers/usb/gadget/fsl_udc_core.c | 9 +++++++++ > 1 files changed, 9 insertions(+), 0 deletions(-) > > diff --git a/drivers/usb/gadget/fsl_udc_core.c > b/drivers/usb/gadget/fsl_udc_core.c > index 55abfb6..fc86108 100644 > --- a/drivers/usb/gadget/fsl_udc_core.c > +++ b/drivers/usb/gadget/fsl_udc_core.c > @@ -1638,6 +1638,15 @@ static int process_ep_req(struct fsl_udc *udc, > int pipe, > status = REQ_UNCOMPLETE; > return status; > } else if (remaining_length) { > + /* wait controller release dtd dma */ > + while ((curr_qh->curr_dtd_ptr == curr_td->td_dma)) { > + if (curr_td->next_td_ptr == > + EP_QUEUE_HEAD_NEXT_TERMINATE) { > + udelay(100); > + break; > + } > + udelay(1); > + } > if (direction) { > VDBG("Transmit dTD remaining length not zero"); > status = -EPROTO; > -- > 1.7.2.5 > Best Regards, Neil Zhang
> > > > after a while of testing and searching I can come up with a patch > > that fixes g_ether <-> iperf for fsl_udc on ARM i.MX35. > > > > The sad part is that this kind of fix is already implemented for > > marvell mv_udc driver since last year but still _not_ in the ~15 > > other *udc.c drivers. > > It is a controller limitation (bug?) for chipidea controller when using dtd list to handle multi-dtds situation. The controller will read dtd's next pointer after this dtd's active bit has already been cleared. Freescale has an errata for this problem's description and solution. http://cache.freescale.com/files/dsp/doc/errata/IMX23CE.pdf See 2858 BR, Peter
Hi Peter, thanks you for your help. On Wed, May 09, 2012 at 02:11:56AM +0000, Chen Peter-B29397 wrote: > > > > > > > > after a while of testing and searching I can come up with a patch > > > that fixes g_ether <-> iperf for fsl_udc on ARM i.MX35. > > > > > > The sad part is that this kind of fix is already implemented for > > > marvell mv_udc driver since last year but still _not_ in the ~15 > > > other *udc.c drivers. > > > > > It is a controller limitation (bug?) for chipidea controller when using > dtd list to handle multi-dtds situation. The controller will read dtd's > next pointer after this dtd's active bit has already been cleared. > > Freescale has an errata for this problem's description and solution. > http://cache.freescale.com/files/dsp/doc/errata/IMX23CE.pdf > See 2858 I'm hacking here an i.MX35 not an i.MX23. So I already had a look at its errata sheet http://cache.freescale.com/files/dsp/doc/errata/IMX35CE.pdf but didn't find a similar "limitation" as you now pointed out by errata number 2858 for i.MX23. What about the fix for mv_udc, shouldn't they have a similar "limitation"? How do you explain? Or maybe someone from Marvell has an answer? Thanks, -- Christoph
> > I'm hacking here an i.MX35 not an i.MX23. So I already had a look at its > errata sheet http://cache.freescale.com/files/dsp/doc/errata/IMX35CE.pdf > but didn't find a similar "limitation" as you now pointed out by errata > number 2858 for i.MX23. > i.mx35 and i.mx23 uses the same controller. > What about the fix for mv_udc, shouldn't they have a similar > "limitation"? How do you explain? > Or maybe someone from Marvell has an answer? > It is probably that others who use chipidea core have the same problem, but it needs to get confirm from them. > > Thanks, > -- Christoph
> -----Original Message----- > From: Christoph Fritz [mailto:chf.fritz@googlemail.com] > Sent: 2012年5月9日 13:38 > To: Chen Peter-B29397; Chao Xie > Cc: Neil Zhang; Fabio Estevam; Li Yang-R58472; Felipe Balbi; Greg > Kroah-Hartman; linux-usb@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; > Hans J. Koch; Daniel Mack; Thomas Dahlmann; Nicolas Ferre; Eric Miao; > Russell King; Haojian Zhuang; Ben Dooks; Kukjin Kim; Sascha Hauer; > Oliver Neukum; Ido Shayevitz; Estevam Fabio-R49496 > Subject: Re: [RFC] [PATCH] usb: gadget: fix dtd dma confusion > > Hi Peter, > > thanks you for your help. > > On Wed, May 09, 2012 at 02:11:56AM +0000, Chen Peter-B29397 wrote: > > > > > > > > > > > > after a while of testing and searching I can come up with a patch > > > > that fixes g_ether <-> iperf for fsl_udc on ARM i.MX35. > > > > > > > > The sad part is that this kind of fix is already implemented for > > > > marvell mv_udc driver since last year but still _not_ in the ~15 > > > > other *udc.c drivers. > > > > > > > > It is a controller limitation (bug?) for chipidea controller when > using > > dtd list to handle multi-dtds situation. The controller will read > dtd's > > next pointer after this dtd's active bit has already been cleared. > > > > Freescale has an errata for this problem's description and solution. > > http://cache.freescale.com/files/dsp/doc/errata/IMX23CE.pdf > > See 2858 > > I'm hacking here an i.MX35 not an i.MX23. So I already had a look at > its > errata sheet > http://cache.freescale.com/files/dsp/doc/errata/IMX35CE.pdf > but didn't find a similar "limitation" as you now pointed out by errata > number 2858 for i.MX23. > > What about the fix for mv_udc, shouldn't they have a similar > "limitation"? How do you explain? > Or maybe someone from Marvell has an answer? > Yes, we encountered the same issue, and I have answered your question several hours ago. Please have a look. > > Thanks, > -- Christoph Best Regards, Neil Zhang
On Wed, May 09, 2012 at 05:43:11AM +0000, Chen Peter-B29397 wrote: > > > > > I'm hacking here an i.MX35 not an i.MX23. So I already had a look at its > > errata sheet http://cache.freescale.com/files/dsp/doc/errata/IMX35CE.pdf > > but didn't find a similar "limitation" as you now pointed out by errata > > number 2858 for i.MX23. > > > > i.mx35 and i.mx23 uses the same controller. A link or short note in i.MX35 its errata sheet to the i.MX23 one would be nice.
Hi Neil, thanks for you help. > > > > What about the fix for mv_udc, shouldn't they have a similar > > "limitation"? How do you explain? > > Or maybe someone from Marvell has an answer? > > > > Yes, we encountered the same issue, and I have answered your question several hours ago. > Please have a look. Thanks for that. Relating to the other *udc.c driver, do you think they should implement a similar workaround there too or at least check their errataS? Thanks, -- Christoph
On Wed, May 09, 2012 at 02:02:22AM +0200, Christoph Fritz wrote: > > Hi to All, > > after a while of testing and searching I can come up with a patch > that fixes g_ether <-> iperf for fsl_udc on ARM i.MX35. > > The sad part is that this kind of fix is already implemented for > marvell mv_udc driver since last year but still _not_ in the ~15 > other *udc.c drivers. > > See here: > daec765da767e4a6a30e1298862b28f2cae9a73f > usb: gadget: mv_udc: fix dtd dma confusion > > So hereby I'm CC-ing all *udc.c maintainers to point out that this > issue maybe affects you too. > > > --- > Subject: [PATCH] usb: gadget: fsl_udc: fix dtd dma confusion > > The controller will hang when doing testings with g_ether and iperf > (tool for measuring maximum TCP and UDP bandwidth). This patch adds a > delay to wait for controller to release dtd dma before freeing it. > > Signed-off-by: Christoph Fritz <chf.fritz@googlemail.com> > --- > drivers/usb/gadget/fsl_udc_core.c | 9 +++++++++ > 1 files changed, 9 insertions(+), 0 deletions(-) > > diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c > index 55abfb6..fc86108 100644 > --- a/drivers/usb/gadget/fsl_udc_core.c > +++ b/drivers/usb/gadget/fsl_udc_core.c > @@ -1638,6 +1638,15 @@ static int process_ep_req(struct fsl_udc *udc, int pipe, > status = REQ_UNCOMPLETE; > return status; > } else if (remaining_length) { > + /* wait controller release dtd dma */ > + while ((curr_qh->curr_dtd_ptr == curr_td->td_dma)) { > + if (curr_td->next_td_ptr == > + EP_QUEUE_HEAD_NEXT_TERMINATE) { > + udelay(100); > + break; > + } > + udelay(1); > + } > if (direction) { > VDBG("Transmit dTD remaining length not zero"); > status = -EPROTO; > -- > 1.7.2.5 ping - what about this patch? Will it be applied? Thanks, -- Christoph
> > See here: > > daec765da767e4a6a30e1298862b28f2cae9a73f > > usb: gadget: mv_udc: fix dtd dma confusion > > > > So hereby I'm CC-ing all *udc.c maintainers to point out that this > > issue maybe affects you too. > > > > > > --- > > Subject: [PATCH] usb: gadget: fsl_udc: fix dtd dma confusion > > > > The controller will hang when doing testings with g_ether and iperf > > (tool for measuring maximum TCP and UDP bandwidth). This patch adds a > > delay to wait for controller to release dtd dma before freeing it. > > > > Signed-off-by: Christoph Fritz <chf.fritz@googlemail.com> > > --- > > drivers/usb/gadget/fsl_udc_core.c | 9 +++++++++ > > 1 files changed, 9 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/usb/gadget/fsl_udc_core.c > b/drivers/usb/gadget/fsl_udc_core.c > > index 55abfb6..fc86108 100644 > > --- a/drivers/usb/gadget/fsl_udc_core.c > > +++ b/drivers/usb/gadget/fsl_udc_core.c > > @@ -1638,6 +1638,15 @@ static int process_ep_req(struct fsl_udc *udc, > int pipe, > > status = REQ_UNCOMPLETE; > > return status; > > } else if (remaining_length) { > > + /* wait controller release dtd dma */ > > + while ((curr_qh->curr_dtd_ptr == curr_td->td_dma)) { > > + if (curr_td->next_td_ptr == > > + EP_QUEUE_HEAD_NEXT_TERMINATE) { > > + udelay(100); > > + break; > > + } > > + udelay(1); > > + } > > if (direction) { > > VDBG("Transmit dTD remaining length not zero"); > > status = -EPROTO; > > -- > > 1.7.2.5 > > ping - what about this patch? Will it be applied? > Christoph, it is better use errata suggests workaround (postpone free dtd memory), and you can find related code at: http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/ drivers/usb/gadget/arcotg_udc.c?h=imx_3.0.15 The code is defined with POSTPONE_FREE_LAST_DTD > Thanks, > -- Christoph >
On Mon, May 14, 2012 at 12:51:26AM +0200, Christoph Fritz wrote: > On Wed, May 09, 2012 at 02:02:22AM +0200, Christoph Fritz wrote: > > > > Hi to All, > > > > after a while of testing and searching I can come up with a patch > > that fixes g_ether <-> iperf for fsl_udc on ARM i.MX35. > > > > The sad part is that this kind of fix is already implemented for > > marvell mv_udc driver since last year but still _not_ in the ~15 > > other *udc.c drivers. > > > > See here: > > daec765da767e4a6a30e1298862b28f2cae9a73f > > usb: gadget: mv_udc: fix dtd dma confusion > > > > So hereby I'm CC-ing all *udc.c maintainers to point out that this > > issue maybe affects you too. > > > > > > --- > > Subject: [PATCH] usb: gadget: fsl_udc: fix dtd dma confusion > > > > The controller will hang when doing testings with g_ether and iperf > > (tool for measuring maximum TCP and UDP bandwidth). This patch adds a > > delay to wait for controller to release dtd dma before freeing it. > > > > Signed-off-by: Christoph Fritz <chf.fritz@googlemail.com> > > --- > > drivers/usb/gadget/fsl_udc_core.c | 9 +++++++++ > > 1 files changed, 9 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c > > index 55abfb6..fc86108 100644 > > --- a/drivers/usb/gadget/fsl_udc_core.c > > +++ b/drivers/usb/gadget/fsl_udc_core.c > > @@ -1638,6 +1638,15 @@ static int process_ep_req(struct fsl_udc *udc, int pipe, > > status = REQ_UNCOMPLETE; > > return status; > > } else if (remaining_length) { > > + /* wait controller release dtd dma */ > > + while ((curr_qh->curr_dtd_ptr == curr_td->td_dma)) { > > + if (curr_td->next_td_ptr == > > + EP_QUEUE_HEAD_NEXT_TERMINATE) { > > + udelay(100); > > + break; > > + } > > + udelay(1); > > + } > > if (direction) { > > VDBG("Transmit dTD remaining length not zero"); > > status = -EPROTO; > > -- > > 1.7.2.5 > > ping - what about this patch? Will it be applied? RFC patches are never applied, as you are asking for comments, not for the patch to actually be applied... Submit it properly if you wish it to be accepted. thanks, greg k-h
diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c index 55abfb6..fc86108 100644 --- a/drivers/usb/gadget/fsl_udc_core.c +++ b/drivers/usb/gadget/fsl_udc_core.c @@ -1638,6 +1638,15 @@ static int process_ep_req(struct fsl_udc *udc, int pipe, status = REQ_UNCOMPLETE; return status; } else if (remaining_length) { + /* wait controller release dtd dma */ + while ((curr_qh->curr_dtd_ptr == curr_td->td_dma)) { + if (curr_td->next_td_ptr == + EP_QUEUE_HEAD_NEXT_TERMINATE) { + udelay(100); + break; + } + udelay(1); + } if (direction) { VDBG("Transmit dTD remaining length not zero"); status = -EPROTO;