Hi Felipe,
On Thu, Jun 13, 2013 at 02:01:04AM +0800, Felipe Balbi wrote:
> Hi,
>
> On Fri, Jun 07, 2013 at 03:50:17PM +0800, Huang Rui wrote:
> > Hi Felipe,
> >
> > I was reading dwc3 codes and found that during the process of
> > configuring event buffer (dwc3_event_buffers_setup), it only write the
> > size of the buffer and doesn't write interrupt mask bit into GEVNTSIZ
> > register like below,
> >
> > dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(n),
> > evt->length & 0xffff);
> >
> > But in spec, it suggests that write this bit to prevent the interrupt
> > from being generated in an event buffer configuration. So need we set
>
> where does it say that ? I just re-read details about that bit and all
> it says is that it can be used to mask the interrupt, but events will
> still be queued. Maybe I'm missing some part. Which revision of the
> databook are you reading ?
Thanks a lot to look back into this issue. I read version 2.50a, in
section 8.2.2, the step 3 to configure an Event Buffer describes:
"Writes the size of the buffer and interrupt mask into GEVNTSIZn.
Depending on your system interrupt latency, enough Event Buffer space
must be allocated to avoid lost interrupts or reduced performance."
If I misunderstood, please correct me.
>
> Anyway, we don't really need that bit right now because linux will only
> enable the IRQ line after request_*irq() has been called and we're
> setting up our event buffers before calling that.
>
Yeah, when the event buffers are set up, it must not encounter any
interrupts.
> OTOH, we could use that bit as means to get rid of IRQF_ONESHOT from
> DWC3 driver.
>
Thanks to teach me. The IRQF_ONESHOT interrupt should also prevent the
other interrupts until the current thread has been run, just like the
function of interrupt mask bit, am I right?
> Can you test a patch for me ? I don't have access to HW right now. I
> assume the patch below works fine, does it ?
>
Sorry, I haven't got the board yet. Your patch looks good, and I will
test it as soon as I get HW.
> (it's not final yet, I will still break it down into proper patches)
>
> 8<----------------------- cut here --------------------------------
>
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index c35d49d..be74df6 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -236,7 +236,7 @@ static int dwc3_event_buffers_setup(struct dwc3 *dwc)
> dwc3_writel(dwc->regs, DWC3_GEVNTADRHI(n),
> upper_32_bits(evt->dma));
> dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(n),
> - evt->length & 0xffff);
> + DWC3_GEVNTSIZ_SIZE(evt->length));
> dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(n), 0);
> }
>
> @@ -255,7 +255,8 @@ static void dwc3_event_buffers_cleanup(struct dwc3 *dwc)
>
> dwc3_writel(dwc->regs, DWC3_GEVNTADRLO(n), 0);
> dwc3_writel(dwc->regs, DWC3_GEVNTADRHI(n), 0);
> - dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(n), 0);
> + dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(n), DWC3_GEVNTSIZ_INTMASK
> + | DWC3_GEVNTSIZ_SIZE(0));
> dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(n), 0);
> }
> }
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index b69d322..d2ceb82 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -194,6 +194,10 @@
> #define DWC3_GTXFIFOSIZ_TXFDEF(n) ((n) & 0xffff)
> #define DWC3_GTXFIFOSIZ_TXFSTADDR(n) ((n) & 0xffff0000)
>
> +/* Global Event Size Registers */
> +#define DWC3_GEVNTSIZ_INTMASK (1 << 31)
> +#define DWC3_GEVNTSIZ_SIZE(n) ((n) & 0xffff)
> +
> /* Global HWPARAMS1 Register */
> #define DWC3_GHWPARAMS1_EN_PWROPT(n) (((n) & (3 << 24)) >> 24)
> #define DWC3_GHWPARAMS1_EN_PWROPT_NO 0
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 2b6e7e0..dc7ee3d 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1567,7 +1567,7 @@ static int dwc3_gadget_start(struct usb_gadget *g,
>
> irq = platform_get_irq(to_platform_device(dwc->dev), 0);
> ret = request_threaded_irq(irq, dwc3_interrupt, dwc3_thread_interrupt,
> - IRQF_SHARED | IRQF_ONESHOT, "dwc3", dwc);
> + IRQF_SHARED, "dwc3", dwc);
> if (ret) {
> dev_err(dwc->dev, "failed to request irq #%d --> %d\n",
> irq, ret);
> @@ -2491,6 +2491,7 @@ static irqreturn_t dwc3_thread_interrupt(int irq, void *_dwc)
> struct dwc3 *dwc = _dwc;
> unsigned long flags;
> irqreturn_t ret = IRQ_NONE;
> + u32 reg;
> int i;
>
> spin_lock_irqsave(&dwc->lock, flags);
> @@ -2530,6 +2531,11 @@ static irqreturn_t dwc3_thread_interrupt(int irq, void *_dwc)
> evt->count = 0;
> evt->flags &= ~DWC3_EVENT_PENDING;
> ret = IRQ_HANDLED;
> +
> + /* Unask interrupt */
> + reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(i));
> + reg &= ~DWC3_GEVNTSIZ_INTMASK;
> + dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(i), reg);
> }
>
> spin_unlock_irqrestore(&dwc->lock, flags);
> @@ -2541,6 +2547,7 @@ static irqreturn_t dwc3_process_event_buf(struct dwc3 *dwc, u32 buf)
> {
> struct dwc3_event_buffer *evt;
> u32 count;
> + u32 reg;
>
> evt = dwc->ev_buffs[buf];
>
> @@ -2552,6 +2559,11 @@ static irqreturn_t dwc3_process_event_buf(struct dwc3 *dwc, u32 buf)
> evt->count = count;
> evt->flags |= DWC3_EVENT_PENDING;
>
> + /* Mask interrupt */
> + reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(buf));
> + reg |= DWC3_GEVNTSIZ_INTMASK;
> + dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(buf), reg);
> +
> return IRQ_WAKE_THREAD;
> }
>
>
> --
> balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
