Hi, On Wed, Jul 22, 2015 at 06:27:29PM -0400, Alan Cooper wrote: > On Wed, Jul 22, 2015 at 5:29 PM, Felipe Balbi <balbi@xxxxxx> wrote: > > On Wed, Jul 22, 2015 at 04:26:57PM -0500, Felipe Balbi wrote: > >> On Wed, Jul 22, 2015 at 04:58:07PM -0400, Al Cooper wrote: > >> > V2 - Fix a compiler bug that happend when the config options > >> > CONFIG_USB_GADGET_DEBUG and CONFIG_USB_GADGET_VERBOSE > >> > were enabled. > >> > > >> > ep_dequeue() in bdc_ep.c was capturing the hw dequeue pointer > >> > incorrectly by reading the wrong register for the upper 32 bits. > >> > The header file defining the registers was incorrect. > >> > >> btw, the header file was really "incorrect" as long as you passed 0 to > >> the argument :-p > > > > in fact, the minimal fix for this bug would be the one below: > > > > diff --git a/drivers/usb/gadget/udc/bdc/bdc_ep.c b/drivers/usb/gadget/udc/bdc/bdc_ep.c > > index b04980cf6dc4..1efa61265d8d 100644 > > --- a/drivers/usb/gadget/udc/bdc/bdc_ep.c > > +++ b/drivers/usb/gadget/udc/bdc/bdc_ep.c > > @@ -779,7 +779,7 @@ static int ep_dequeue(struct bdc_ep *ep, struct bdc_req *req) > > /* The current hw dequeue pointer */ > > tmp_32 = bdc_readl(bdc->regs, BDC_EPSTS0(0)); > > deq_ptr_64 = tmp_32; > > - tmp_32 = bdc_readl(bdc->regs, BDC_EPSTS0(1)); > > + tmp_32 = bdc_readl(bdc->regs, BDC_EPSTS1(0)); > > deq_ptr_64 |= ((u64)tmp_32 << 32); > > > > /* we have the dma addr of next bd that will be fetched by hardware */ > > > > And $subject becomes a cleanup patch for v4.3. Can you make these > > changes, please ? > > Yes, I can make these changes, but first let me explain why I did it this way. > The 8 End Point Status registers are 32 bit consecutive registers, so > defining them as: > #define BDC_EPSTS0(n) (0x60 + (n * 0x10)) > #define BDC_EPSTS1(n) (0x64 + (n * 0x10)) > #define BDC_EPSTS2(n) (0x68 + (n * 0x10)) > #define BDC_EPSTS3(n) (0x6c + (n * 0x10)) > #define BDC_EPSTS4(n) (0x70 + (n * 0x10)) > #define BDC_EPSTS5(n) (0x74 + (n * 0x10)) > #define BDC_EPSTS6(n) (0x78 + (n * 0x10)) > #define BDC_EPSTS7(n) (0x7c + (n * 0x10)) > seems misleading, does not reflect the hardware and using anything > other than (0) would get you to some other unexpected register and > should be considered a coding error. it sure is, but the minimal patch for -rc is what I sent above :-) As long as you pass 0 as parameters, all your offsets are correct, so removing the parameter (which must be always zero) is, actually, refactoring happening. > I think the original hardware spec had each End Point Status as a > block of registers but the first silicon had them as single registers > and they are expected to stay that way. I believe they wanted to have: #define BDC_EPSTS(n) (0x60 + (n * 0x4)) and that way you can access each EP by passing the correct argument to that macro. -- balbi
Attachment:
signature.asc
Description: Digital signature
