Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1375728 > unrolled thread

[PATCH v6 02/10] usb: dwc3: omap: Make the wrapper interrupt shared

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v6 02/10] usb: dwc3: omap: Make the wrapper interrupt shared Roger Quadros <rogerq@ti.com> - 2016-04-11 13:40 +0200
    Re: [PATCH v6 02/10] usb: dwc3: omap: Make the wrapper interrupt shared Felipe Balbi <balbi@kernel.org> - 2016-04-11 14:20 +0200
      Re: [PATCH v6 02/10] usb: dwc3: omap: Make the wrapper interrupt  shared Roger Quadros <rogerq@ti.com> - 2016-04-11 15:00 +0200
        Re: [PATCH v6 02/10] usb: dwc3: omap: Make the wrapper interrupt shared Felipe Balbi <balbi@kernel.org> - 2016-04-11 15:10 +0200
          Re: [PATCH v6 02/10] usb: dwc3: omap: Make the wrapper interrupt  shared Roger Quadros <rogerq@ti.com> - 2016-04-11 15:20 +0200
            Re: [PATCH v6 02/10] usb: dwc3: omap: Make the wrapper interrupt shared Felipe Balbi <balbi@kernel.org> - 2016-04-11 15:30 +0200

#1375728 — [PATCH v6 02/10] usb: dwc3: omap: Make the wrapper interrupt shared

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(-)

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);
-- 
2.5.0

[toc] | [next] | [standalone]

#1375767

[Multipart message — attachments visible in raw view] — view raw

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.

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1375808 — Re: [PATCH v6 02/10] usb: dwc3: omap: Make the wrapper interrupt shared

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

[toc] | [prev] | [next] | [standalone]

#1375826

[Multipart message — attachments visible in raw view] — view raw

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).

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1375833 — Re: [PATCH v6 02/10] usb: dwc3: omap: Make the wrapper interrupt shared

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

[toc] | [prev] | [next] | [standalone]

#1375844

[Multipart message — attachments visible in raw view] — view raw

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 ;-)

-- 
balbi

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web