| Message ID | 1321015093-13715-1-git-send-email-leoli@freescale.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Fri, Nov 11, 2011 at 08:38:13PM +0800, Li Yang wrote: > The original implementation of dequeuing a request in progress > is not correct. Change to use a correct process and also clean > up the related functions a little bit. > > Signed-off-by: Li Yang <leoli@freescale.com> > --- > drivers/usb/gadget/fsl_udc_core.c | 62 +++++++++++++++++------------------- > 1 files changed, 29 insertions(+), 33 deletions(-) > > diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c > index b2c44e1..beef9b7 100644 > --- a/drivers/usb/gadget/fsl_udc_core.c > +++ b/drivers/usb/gadget/fsl_udc_core.c > @@ -696,12 +696,31 @@ static void fsl_free_request(struct usb_ep *_ep, struct usb_request *_req) > kfree(req); > } > > -/*-------------------------------------------------------------------------*/ > +/* Actually add a dTD chain to an empty dQH and let go */ > +static void fsl_prime_ep(struct fsl_ep *ep, struct ep_td_struct *td) > +{ > + struct ep_queue_head *qh = ep->qh; It seems to can't get the correct qh pointer, you may still need to use below code to get it int i = ep_index(ep) * 2 + ep_is_in(ep); struct ep_queue_head *dQH = &ep->udc->ep_qh[i]; > + > + /* Write dQH next pointer and terminate bit to 0 */ > + qh->next_dtd_ptr = cpu_to_hc32(td->td_dma > + & EP_QUEUE_HEAD_NEXT_POINTER_MASK); > + > + /* Clear active and halt bit */ > + qh->size_ioc_int_sts &= cpu_to_hc32(~(EP_QUEUE_HEAD_STATUS_ACTIVE > + | EP_QUEUE_HEAD_STATUS_HALT)); > + > + /* Ensure that updates to the QH will occur before priming. */ > + wmb(); > + > + /* Prime endpoint by writing correct bit to ENDPTPRIME */ > + fsl_writel(ep_is_in(ep) ? (1 << (ep_index(ep) + 16)) > + : (1 << (ep_index(ep))), &dr_regs->endpointprime); > +} > + > +/* Add dTD chain to the dQH of an EP */ > static void fsl_queue_td(struct fsl_ep *ep, struct fsl_req *req) > { > - int i = ep_index(ep) * 2 + ep_is_in(ep); > u32 temp, bitmask, tmp_stat; > - struct ep_queue_head *dQH = &ep->udc->ep_qh[i]; > > /* VDBG("QH addr Register 0x%8x", dr_regs->endpointlistaddr); > VDBG("ep_qh[%d] addr is 0x%8x", i, (u32)&(ep->udc->ep_qh[i])); */ > @@ -719,7 +738,7 @@ static void fsl_queue_td(struct fsl_ep *ep, struct fsl_req *req) > cpu_to_hc32(req->head->td_dma & DTD_ADDR_MASK); > /* Read prime bit, if 1 goto done */ > if (fsl_readl(&dr_regs->endpointprime) & bitmask) > - goto out; > + return; > > do { > /* Set ATDTW bit in USBCMD */ > @@ -736,28 +755,10 @@ static void fsl_queue_td(struct fsl_ep *ep, struct fsl_req *req) > fsl_writel(temp & ~USB_CMD_ATDTW, &dr_regs->usbcmd); > > if (tmp_stat) > - goto out; > + return; > } > > - /* Write dQH next pointer and terminate bit to 0 */ > - temp = req->head->td_dma & EP_QUEUE_HEAD_NEXT_POINTER_MASK; > - dQH->next_dtd_ptr = cpu_to_hc32(temp); > - > - /* Clear active and halt bit */ > - temp = cpu_to_hc32(~(EP_QUEUE_HEAD_STATUS_ACTIVE > - | EP_QUEUE_HEAD_STATUS_HALT)); > - dQH->size_ioc_int_sts &= temp; > - > - /* Ensure that updates to the QH will occur before priming. */ > - wmb(); > - > - /* Prime endpoint by writing 1 to ENDPTPRIME */ > - temp = ep_is_in(ep) > - ? (1 << (ep_index(ep) + 16)) > - : (1 << (ep_index(ep))); > - fsl_writel(temp, &dr_regs->endpointprime); > -out: > - return; > + fsl_prime_ep(ep, req->head); > } > > /* Fill in the dTD structure > @@ -973,25 +974,20 @@ static int fsl_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req) > > /* The request isn't the last request in this ep queue */ > if (req->queue.next != &ep->queue) { > - struct ep_queue_head *qh; > struct fsl_req *next_req; > > - qh = ep->qh; > next_req = list_entry(req->queue.next, struct fsl_req, > queue); > > - /* Point the QH to the first TD of next request */ > - fsl_writel((u32) next_req->head, &qh->curr_dtd_ptr); > + /* prime with dTD of next request */ > + fsl_prime_ep(ep, next_req->head); > } > - > - /* The request hasn't been processed, patch up the TD chain */ > + /* The request hasn't been processed, patch up the TD chain */ > } else { > struct fsl_req *prev_req; > > prev_req = list_entry(req->queue.prev, struct fsl_req, queue); > - fsl_writel(fsl_readl(&req->tail->next_td_ptr), > - &prev_req->tail->next_td_ptr); > - > + prev_req->tail->next_td_ptr = req->tail->next_td_ptr; > } > > done(ep, req, -ECONNRESET); After fixing above error, others are ok. I have tested it at i.mx51 bbg board using 3.2.0-rc2+. > -- > 1.5.4.3 > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
>Subject: Re: [PATCH] usb/fsl_udc: fix dequeuing a request in progress > >On Fri, Nov 11, 2011 at 08:38:13PM +0800, Li Yang wrote: >> The original implementation of dequeuing a request in progress is not >> correct. Change to use a correct process and also clean up the >> related functions a little bit. >> >> Signed-off-by: Li Yang <leoli@freescale.com> >> --- >> drivers/usb/gadget/fsl_udc_core.c | 62 +++++++++++++++++------------- >------ >> 1 files changed, 29 insertions(+), 33 deletions(-) >> >> diff --git a/drivers/usb/gadget/fsl_udc_core.c >> b/drivers/usb/gadget/fsl_udc_core.c >> index b2c44e1..beef9b7 100644 >> --- a/drivers/usb/gadget/fsl_udc_core.c >> +++ b/drivers/usb/gadget/fsl_udc_core.c >> @@ -696,12 +696,31 @@ static void fsl_free_request(struct usb_ep *_ep, >struct usb_request *_req) >> kfree(req); >> } >> >> -/*------------------------------------------------------------------- >> ------*/ >> +/* Actually add a dTD chain to an empty dQH and let go */ static void >> +fsl_prime_ep(struct fsl_ep *ep, struct ep_td_struct *td) { >> + struct ep_queue_head *qh = ep->qh; >It seems to can't get the correct qh pointer, you may still need to use >below code to get it > int i = ep_index(ep) * 2 + ep_is_in(ep); > struct ep_queue_head *dQH = &ep->udc->ep_qh[i]; Thanks for trying. It will be much easier if we can dereference QH from the ep structure. It is really strange that the ep->qh pointer is not working somehow. We have initialized it in struct_ep_setup(): ep->qh = &udc->ep_qh[index]; Can you do me a favor on investigating why it's failing? Thanks, Leo
>>It seems to can't get the correct qh pointer, you may still need to use >>below code to get it >> int i = ep_index(ep) * 2 + ep_is_in(ep); >> struct ep_queue_head *dQH = &ep->udc->ep_qh[i]; > > Thanks for trying. It will be much easier if we can dereference QH from the ep structure. It is really strange that the ep->qh pointer is not working somehow. > Seems only ep0-out's qh pointer is assigned at ep structure. At probe: /* setup udc->eps[] for ep0 */ struct_ep_setup(udc_controller, 0, "ep0", 0); > We have initialized it in struct_ep_setup(): > ep->qh = &udc->ep_qh[index]; > > Can you do me a favor on investigating why it's failing? > > Thanks, > Leo > > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Tue, Nov 22, 2011 at 7:48 PM, Peter Chen <hzpeterchen@gmail.com> wrote: >>>It seems to can't get the correct qh pointer, you may still need to use >>>below code to get it >>> int i = ep_index(ep) * 2 + ep_is_in(ep); >>> struct ep_queue_head *dQH = &ep->udc->ep_qh[i]; >> >> Thanks for trying. It will be much easier if we can dereference QH from the ep structure. It is really strange that the ep->qh pointer is not working somehow. >> > > Seems only ep0-out's qh pointer is assigned at ep structure. > At probe: > /* setup udc->eps[] for ep0 */ > struct_ep_setup(udc_controller, 0, "ep0", 0); > > >> We have initialized it in struct_ep_setup(): >> ep->qh = &udc->ep_qh[index]; >> >> Can you do me a favor on investigating why it's failing? >> Leo, I have debugged this issue at my board just now, the reason of failure is we only have one ep struct for ep0, so when talking about ep0, it always pointers to udc->ep[0]. So even we initialize the current qh address for ep0in at probe, it still can't get the ep0in's qh address through ep structure, as ep0 is always udc->ep[0]. >> Thanks, >> Leo >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-usb" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > > > > -- > BR, > Peter Chen >
>-----Original Message----- >From: Peter Chen [mailto:hzpeterchen@gmail.com] >Sent: Wednesday, November 23, 2011 11:02 AM >To: Li Yang-R58472 >Cc: Chen Peter-B29397; balbi@ti.com; gregkh@suse.de; linux- >usb@vger.kernel.org; linuxppc-dev@lists.ozlabs.org >Subject: Re: [PATCH] usb/fsl_udc: fix dequeuing a request in progress > >On Tue, Nov 22, 2011 at 7:48 PM, Peter Chen <hzpeterchen@gmail.com> wrote: >>>>It seems to can't get the correct qh pointer, you may still need to >>>>use below code to get it >>>> int i = ep_index(ep) * 2 + ep_is_in(ep); >>>> struct ep_queue_head *dQH = &ep->udc->ep_qh[i]; >>> >>> Thanks for trying. It will be much easier if we can dereference QH >from the ep structure. It is really strange that the ep->qh pointer is >not working somehow. >>> >> >> Seems only ep0-out's qh pointer is assigned at ep structure. >> At probe: >> /* setup udc->eps[] for ep0 */ >> struct_ep_setup(udc_controller, 0, "ep0", 0); >> >> >>> We have initialized it in struct_ep_setup(): >>> ep->qh = &udc->ep_qh[index]; >>> >>> Can you do me a favor on investigating why it's failing? >>> >Leo, I have debugged this issue at my board just now, the reason of >failure is we only have one ep struct for ep0, so when talking about ep0, >it always pointers to udc->ep[0]. So even we initialize the current qh >address for ep0in at probe, it still can't get the ep0in's qh address >through ep structure, as ep0 is always udc->ep[0]. Oops. I forgot the fact that we used a single ep structure to handle both IN and OUT ep0 because the gadget layer only knows about one ep0 structure. I guess currently we have no other way out unless the gadget layer do honor ep0 with direction. IMHO, it is a limitation to current gadget APIs that each udc driver need to take too much care of the protocol related stuff on ep0. Thanks, Leo
On Wed, Nov 23, 2011 at 08:20:54AM +0000, Li Yang-R58472 wrote: > >Leo, I have debugged this issue at my board just now, the reason of > >failure is we only have one ep struct for ep0, so when talking about ep0, > >it always pointers to udc->ep[0]. So even we initialize the current qh > >address for ep0in at probe, it still can't get the ep0in's qh address > >through ep structure, as ep0 is always udc->ep[0]. > > Oops. I forgot the fact that we used a single ep structure to handle > both IN and OUT ep0 because the gadget layer only knows about one ep0 > structure. the gadget framework knows about USB endpoints which are bidirectional logical entities. One USB endpoint is composed of two unidirectional HW endpoints. > I guess currently we have no other way out unless the gadget layer do > honor ep0 with direction. IMHO, it is a limitation to current gadget > APIs that each udc driver need to take too much care of the protocol > related stuff on ep0. indeed, if you can come up with a sensible patch we're here to review ;-)
diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c index b2c44e1..beef9b7 100644 --- a/drivers/usb/gadget/fsl_udc_core.c +++ b/drivers/usb/gadget/fsl_udc_core.c @@ -696,12 +696,31 @@ static void fsl_free_request(struct usb_ep *_ep, struct usb_request *_req) kfree(req); } -/*-------------------------------------------------------------------------*/ +/* Actually add a dTD chain to an empty dQH and let go */ +static void fsl_prime_ep(struct fsl_ep *ep, struct ep_td_struct *td) +{ + struct ep_queue_head *qh = ep->qh; + + /* Write dQH next pointer and terminate bit to 0 */ + qh->next_dtd_ptr = cpu_to_hc32(td->td_dma + & EP_QUEUE_HEAD_NEXT_POINTER_MASK); + + /* Clear active and halt bit */ + qh->size_ioc_int_sts &= cpu_to_hc32(~(EP_QUEUE_HEAD_STATUS_ACTIVE + | EP_QUEUE_HEAD_STATUS_HALT)); + + /* Ensure that updates to the QH will occur before priming. */ + wmb(); + + /* Prime endpoint by writing correct bit to ENDPTPRIME */ + fsl_writel(ep_is_in(ep) ? (1 << (ep_index(ep) + 16)) + : (1 << (ep_index(ep))), &dr_regs->endpointprime); +} + +/* Add dTD chain to the dQH of an EP */ static void fsl_queue_td(struct fsl_ep *ep, struct fsl_req *req) { - int i = ep_index(ep) * 2 + ep_is_in(ep); u32 temp, bitmask, tmp_stat; - struct ep_queue_head *dQH = &ep->udc->ep_qh[i]; /* VDBG("QH addr Register 0x%8x", dr_regs->endpointlistaddr); VDBG("ep_qh[%d] addr is 0x%8x", i, (u32)&(ep->udc->ep_qh[i])); */ @@ -719,7 +738,7 @@ static void fsl_queue_td(struct fsl_ep *ep, struct fsl_req *req) cpu_to_hc32(req->head->td_dma & DTD_ADDR_MASK); /* Read prime bit, if 1 goto done */ if (fsl_readl(&dr_regs->endpointprime) & bitmask) - goto out; + return; do { /* Set ATDTW bit in USBCMD */ @@ -736,28 +755,10 @@ static void fsl_queue_td(struct fsl_ep *ep, struct fsl_req *req) fsl_writel(temp & ~USB_CMD_ATDTW, &dr_regs->usbcmd); if (tmp_stat) - goto out; + return; } - /* Write dQH next pointer and terminate bit to 0 */ - temp = req->head->td_dma & EP_QUEUE_HEAD_NEXT_POINTER_MASK; - dQH->next_dtd_ptr = cpu_to_hc32(temp); - - /* Clear active and halt bit */ - temp = cpu_to_hc32(~(EP_QUEUE_HEAD_STATUS_ACTIVE - | EP_QUEUE_HEAD_STATUS_HALT)); - dQH->size_ioc_int_sts &= temp; - - /* Ensure that updates to the QH will occur before priming. */ - wmb(); - - /* Prime endpoint by writing 1 to ENDPTPRIME */ - temp = ep_is_in(ep) - ? (1 << (ep_index(ep) + 16)) - : (1 << (ep_index(ep))); - fsl_writel(temp, &dr_regs->endpointprime); -out: - return; + fsl_prime_ep(ep, req->head); } /* Fill in the dTD structure @@ -973,25 +974,20 @@ static int fsl_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req) /* The request isn't the last request in this ep queue */ if (req->queue.next != &ep->queue) { - struct ep_queue_head *qh; struct fsl_req *next_req; - qh = ep->qh; next_req = list_entry(req->queue.next, struct fsl_req, queue); - /* Point the QH to the first TD of next request */ - fsl_writel((u32) next_req->head, &qh->curr_dtd_ptr); + /* prime with dTD of next request */ + fsl_prime_ep(ep, next_req->head); } - - /* The request hasn't been processed, patch up the TD chain */ + /* The request hasn't been processed, patch up the TD chain */ } else { struct fsl_req *prev_req; prev_req = list_entry(req->queue.prev, struct fsl_req, queue); - fsl_writel(fsl_readl(&req->tail->next_td_ptr), - &prev_req->tail->next_td_ptr); - + prev_req->tail->next_td_ptr = req->tail->next_td_ptr; } done(ep, req, -ECONNRESET);
The original implementation of dequeuing a request in progress is not correct. Change to use a correct process and also clean up the related functions a little bit. Signed-off-by: Li Yang <leoli@freescale.com> --- drivers/usb/gadget/fsl_udc_core.c | 62 +++++++++++++++++------------------- 1 files changed, 29 insertions(+), 33 deletions(-)