| Message ID | 1460374506-9779-3-git-send-email-rogerq@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, Roger Quadros <rogerq@ti.com> writes: > The wrapper interrupt is shared with OTG core so mark it IRQF_SHARED. > > Use request_threaded_irq() to ensure that irqflags match for the > shared interrupt handlers. If we don't use request_treaded_irq() then > forced threaded irq will set IRQF_ONESHOT and this won't match with > the OTG irq handler. then it seems you need to _first_ fix the OTG IRQ handler. Why does it defer ? At a minimum, first switch to threaded IRQ handler, then (in another patch) switch to IRQF_SHARED > Signed-off-by: Roger Quadros <rogerq@ti.com> > --- > drivers/usb/dwc3/dwc3-omap.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c > index 22e9606..51ca098 100644 > --- a/drivers/usb/dwc3/dwc3-omap.c > +++ b/drivers/usb/dwc3/dwc3-omap.c > @@ -274,19 +274,25 @@ static irqreturn_t dwc3_omap_interrupt(int irq, void *_omap) > { > struct dwc3_omap *omap = _omap; > u32 reg; > + int ret = IRQ_NONE; > > reg = dwc3_omap_read_irqmisc_status(omap); > > + if (reg) > + ret = IRQ_HANDLED; you can avoid the local variable by returning early here. if (!reg) return IRQ_NONE; > if (reg & USBOTGSS_IRQMISC_DMADISABLECLR) > omap->dma_status = false; > > dwc3_omap_write_irqmisc_status(omap, reg); > > reg = dwc3_omap_read_irq0_status(omap); > + if (reg) > + ret = IRQ_HANDLED; and this check is probably unnecessary. > @@ -506,8 +512,8 @@ static int dwc3_omap_probe(struct platform_device *pdev) > reg = dwc3_omap_readl(omap->base, USBOTGSS_SYSCONFIG); > omap->dma_status = !!(reg & USBOTGSS_SYSCONFIG_DMADISABLE); > > - ret = devm_request_irq(dev, omap->irq, dwc3_omap_interrupt, 0, > - "dwc3-omap", omap); > + ret = devm_request_threaded_irq(dev, omap->irq, dwc3_omap_interrupt, this switch to threaded IRQ is not part of $subject.
On 11/04/16 15:13, Felipe Balbi wrote: > > Hi, > > Roger Quadros <rogerq@ti.com> writes: >> The wrapper interrupt is shared with OTG core so mark it IRQF_SHARED. >> >> Use request_threaded_irq() to ensure that irqflags match for the >> shared interrupt handlers. If we don't use request_treaded_irq() then >> forced threaded irq will set IRQF_ONESHOT and this won't match with >> the OTG irq handler. > > then it seems you need to _first_ fix the OTG IRQ handler. Why does it There is no OTG irq handler yet. > defer ? At a minimum, first switch to threaded IRQ handler, then (in > another patch) switch to IRQF_SHARED OK. > >> Signed-off-by: Roger Quadros <rogerq@ti.com> >> --- >> drivers/usb/dwc3/dwc3-omap.c | 12 +++++++++--- >> 1 file changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c >> index 22e9606..51ca098 100644 >> --- a/drivers/usb/dwc3/dwc3-omap.c >> +++ b/drivers/usb/dwc3/dwc3-omap.c >> @@ -274,19 +274,25 @@ static irqreturn_t dwc3_omap_interrupt(int irq, void *_omap) >> { >> struct dwc3_omap *omap = _omap; >> u32 reg; >> + int ret = IRQ_NONE; >> >> reg = dwc3_omap_read_irqmisc_status(omap); >> >> + if (reg) >> + ret = IRQ_HANDLED; > > you can avoid the local variable by returning early here. How can we return early? we need to check irq0_status as well right? > > if (!reg) > return IRQ_NONE; > >> if (reg & USBOTGSS_IRQMISC_DMADISABLECLR) >> omap->dma_status = false; >> >> dwc3_omap_write_irqmisc_status(omap, reg); >> >> reg = dwc3_omap_read_irq0_status(omap); >> + if (reg) >> + ret = IRQ_HANDLED; > > and this check is probably unnecessary. > >> @@ -506,8 +512,8 @@ static int dwc3_omap_probe(struct platform_device *pdev) >> reg = dwc3_omap_readl(omap->base, USBOTGSS_SYSCONFIG); >> omap->dma_status = !!(reg & USBOTGSS_SYSCONFIG_DMADISABLE); >> >> - ret = devm_request_irq(dev, omap->irq, dwc3_omap_interrupt, 0, >> - "dwc3-omap", omap); >> + ret = devm_request_threaded_irq(dev, omap->irq, dwc3_omap_interrupt, > > this switch to threaded IRQ is not part of $subject. > OK. cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Roger Quadros <rogerq@ti.com> writes: >>> diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c >>> index 22e9606..51ca098 100644 >>> --- a/drivers/usb/dwc3/dwc3-omap.c >>> +++ b/drivers/usb/dwc3/dwc3-omap.c >>> @@ -274,19 +274,25 @@ static irqreturn_t dwc3_omap_interrupt(int irq, void *_omap) >>> { >>> struct dwc3_omap *omap = _omap; >>> u32 reg; >>> + int ret = IRQ_NONE; >>> >>> reg = dwc3_omap_read_irqmisc_status(omap); >>> >>> + if (reg) >>> + ret = IRQ_HANDLED; >> >> you can avoid the local variable by returning early here. > > How can we return early? we need to check irq0_status as well right? Oh, that's true. There's one thing that I noticed though. dma_status is only written to, never read, so you should be able to remove it completely (a bit off-topic, sorry).
On 11/04/16 15:58, Felipe Balbi wrote: > > Hi, > > Roger Quadros <rogerq@ti.com> writes: >>>> diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c >>>> index 22e9606..51ca098 100644 >>>> --- a/drivers/usb/dwc3/dwc3-omap.c >>>> +++ b/drivers/usb/dwc3/dwc3-omap.c >>>> @@ -274,19 +274,25 @@ static irqreturn_t dwc3_omap_interrupt(int irq, void *_omap) >>>> { >>>> struct dwc3_omap *omap = _omap; >>>> u32 reg; >>>> + int ret = IRQ_NONE; >>>> >>>> reg = dwc3_omap_read_irqmisc_status(omap); >>>> >>>> + if (reg) >>>> + ret = IRQ_HANDLED; >>> >>> you can avoid the local variable by returning early here. >> >> How can we return early? we need to check irq0_status as well right? > > Oh, that's true. > > There's one thing that I noticed though. dma_status is only written to, > never read, so you should be able to remove it completely (a bit > off-topic, sorry). > I'll send a patch for that. Can it go in -rc as well along with the other 2? cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Roger Quadros <rogerq@ti.com> writes: > On 11/04/16 15:58, Felipe Balbi wrote: >> >> Hi, >> >> Roger Quadros <rogerq@ti.com> writes: >>>>> diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c >>>>> index 22e9606..51ca098 100644 >>>>> --- a/drivers/usb/dwc3/dwc3-omap.c >>>>> +++ b/drivers/usb/dwc3/dwc3-omap.c >>>>> @@ -274,19 +274,25 @@ static irqreturn_t dwc3_omap_interrupt(int irq, void *_omap) >>>>> { >>>>> struct dwc3_omap *omap = _omap; >>>>> u32 reg; >>>>> + int ret = IRQ_NONE; >>>>> >>>>> reg = dwc3_omap_read_irqmisc_status(omap); >>>>> >>>>> + if (reg) >>>>> + ret = IRQ_HANDLED; >>>> >>>> you can avoid the local variable by returning early here. >>> >>> How can we return early? we need to check irq0_status as well right? >> >> Oh, that's true. >> >> There's one thing that I noticed though. dma_status is only written to, >> never read, so you should be able to remove it completely (a bit >> off-topic, sorry). >> > I'll send a patch for that. Can it go in -rc as well along with the other 2? probably not, as it's not fixing any bug ;-)
diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c index 22e9606..51ca098 100644 --- a/drivers/usb/dwc3/dwc3-omap.c +++ b/drivers/usb/dwc3/dwc3-omap.c @@ -274,19 +274,25 @@ static irqreturn_t dwc3_omap_interrupt(int irq, void *_omap) { struct dwc3_omap *omap = _omap; u32 reg; + int ret = IRQ_NONE; reg = dwc3_omap_read_irqmisc_status(omap); + if (reg) + ret = IRQ_HANDLED; + if (reg & USBOTGSS_IRQMISC_DMADISABLECLR) omap->dma_status = false; dwc3_omap_write_irqmisc_status(omap, reg); reg = dwc3_omap_read_irq0_status(omap); + if (reg) + ret = IRQ_HANDLED; dwc3_omap_write_irq0_status(omap, reg); - return IRQ_HANDLED; + return ret; } static void dwc3_omap_enable_irqs(struct dwc3_omap *omap) @@ -506,8 +512,8 @@ static int dwc3_omap_probe(struct platform_device *pdev) reg = dwc3_omap_readl(omap->base, USBOTGSS_SYSCONFIG); omap->dma_status = !!(reg & USBOTGSS_SYSCONFIG_DMADISABLE); - ret = devm_request_irq(dev, omap->irq, dwc3_omap_interrupt, 0, - "dwc3-omap", omap); + ret = devm_request_threaded_irq(dev, omap->irq, dwc3_omap_interrupt, + NULL, IRQF_SHARED, "dwc3-omap", omap); if (ret) { dev_err(dev, "failed to request IRQ #%d --> %d\n", omap->irq, ret);
The wrapper interrupt is shared with OTG core so mark it IRQF_SHARED. Use request_threaded_irq() to ensure that irqflags match for the shared interrupt handlers. If we don't use request_treaded_irq() then forced threaded irq will set IRQF_ONESHOT and this won't match with the OTG irq handler. Signed-off-by: Roger Quadros <rogerq@ti.com> --- drivers/usb/dwc3/dwc3-omap.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)