[BISECTED] 3.10-rc1 OMAP1 GPIO IRQ regression

Commit Message

Tony Lindgren May 20, 2013, 5:46 p.m. UTC

* Tony Lindgren <[e-mail]> [130516 14:50]:
> * Aaro Koskinen <[e-mail]> [130516 14:05]:
> > On Thu, May 16, 2013 at 11:09:34AM -0700, Tony Lindgren wrote:
> > > * Aaro Koskinen <[e-mail]> [130513 13:58]:
> > > > I tested 3.10-rc1 on OMAP1 / Nokia 770, and Retu MFD probe is broken:
> > > > 
> > > > [    2.264221] retu-mfd 2-0001: Retu v3.2 found
> > > > [    2.281951] retu-mfd 2-0001: Failed to allocate IRQs: -12
> > > > [    2.300140] retu-mfd: probe of 2-0001 failed with error -12
> > > > 
> > > > The error is coming from regmap code. According to git bisect, it is
> > > > caused by:
> > > > 
> > > > 	commit ede4d7a5b9835510fd1f724367f68d2fa4128453
> > > > 	Author: Jon Hunter <[e-mail]>
> > > > 	Date:   Fri Mar 1 11:22:47 2013 -0600
> > > > 
> > > > 	    gpio/omap: convert gpio irq domain to linear mapping
> > > > 
> > > > The commit does not anymore revert cleanly, and I haven't yet tried
> > > > crafting a manual revert, so any fix proposals/ideas are welcome...
> > > 
> > > Hmm this might be a bit trickier to fix. Obviously the real solution
> > > is to convert omap1 to SPARSE_IRQ like we did for omap2+.
> > > 
> > > For the -rc cycle, it might be possible to fix this by adding a
> > > different irq_to_gpio() and gpio_to_irq() functions for omap1.
> > 
> > The commit reverts cleanly if we also revert
> > 3513cdeccc647d41c4a9ff923af17deaaac04a66 (gpio/omap: optimise interrupt
> > service routine), which seems to be just some minor optimization. The
> > result is below, and with it 770 works again.
> 
> Hmm in this case it seems that we should just fix it rather than go back
> to the old code, so let's take a look at that first.

Does the following fix it for you or do we need to fix something else
there too?

Regards,

Tony


--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Aaro Koskinen May 21, 2013, 5:39 p.m. UTC | #1

On Mon, May 20, 2013 at 10:46:21AM -0700, Tony Lindgren wrote:
> * Tony Lindgren <[e-mail]> [130516 14:50]:
> > * Aaro Koskinen <[e-mail]> [130516 14:05]:
> > > On Thu, May 16, 2013 at 11:09:34AM -0700, Tony Lindgren wrote:
> > > > * Aaro Koskinen <[e-mail]> [130513 13:58]:
> > > > > I tested 3.10-rc1 on OMAP1 / Nokia 770, and Retu MFD probe is broken:
> > > > > 
> > > > > [    2.264221] retu-mfd 2-0001: Retu v3.2 found
> > > > > [    2.281951] retu-mfd 2-0001: Failed to allocate IRQs: -12
> > > > > [    2.300140] retu-mfd: probe of 2-0001 failed with error -12
> > > > > 
> > > > > The error is coming from regmap code. According to git bisect, it is
> > > > > caused by:
> > > > > 
> > > > > 	commit ede4d7a5b9835510fd1f724367f68d2fa4128453
> > > > > 	Author: Jon Hunter <[e-mail]>
> > > > > 	Date:   Fri Mar 1 11:22:47 2013 -0600
> > > > > 
> > > > > 	    gpio/omap: convert gpio irq domain to linear mapping
> > > > > 
> > > > > The commit does not anymore revert cleanly, and I haven't yet tried
> > > > > crafting a manual revert, so any fix proposals/ideas are welcome...
> > > > 
> > > > Hmm this might be a bit trickier to fix. Obviously the real solution
> > > > is to convert omap1 to SPARSE_IRQ like we did for omap2+.
> > > > 
> > > > For the -rc cycle, it might be possible to fix this by adding a
> > > > different irq_to_gpio() and gpio_to_irq() functions for omap1.
> > > 
> > > The commit reverts cleanly if we also revert
> > > 3513cdeccc647d41c4a9ff923af17deaaac04a66 (gpio/omap: optimise interrupt
> > > service routine), which seems to be just some minor optimization. The
> > > result is below, and with it 770 works again.
> > 
> > Hmm in this case it seems that we should just fix it rather than go back
> > to the old code, so let's take a look at that first.
> 
> Does the following fix it for you or do we need to fix something else
> there too?

Thanks, that fixes Retu probe on 770.

Tested-by: Aaro Koskinen <[e-mail]>

A.

> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -1094,6 +1094,7 @@ static int omap_gpio_probe(struct platform_device *pdev)
>  	const struct omap_gpio_platform_data *pdata;
>  	struct resource *res;
>  	struct gpio_bank *bank;
> +	int irq_base;
>  
>  	match = of_match_device(of_match_ptr(omap_gpio_match), dev);
>  
> @@ -1135,11 +1136,23 @@ static int omap_gpio_probe(struct platform_device *pdev)
>  				pdata->get_context_loss_count;
>  	}
>  
> -
> -	bank->domain = irq_domain_add_linear(node, bank->width,
> -					     &irq_domain_simple_ops, NULL);
> -	if (!bank->domain)
> +	/*
> +	 * REVISIT: Once we have omap1 supporting  SPARSE_IRQ, we can drop
> +	 * irq_alloc_descs() and irq_domain_add_legacy() and just do:
> +	 *
> +	 * bank->domain = irq_domain_add_linear(node, bank->width,
> +	 *				     &irq_domain_simple_ops, NULL);
> +	 * if (!bank->domain)
> +	 *	return -ENODEV;
> +	 */
> +	irq_base = irq_alloc_descs(-1, 0, bank->width, 0);
> +	if (irq_base < 0) {
> +		dev_err(dev, "Couldn't allocate IRQ numbers\n");
>  		return -ENODEV;
> +	}
> +
> +	bank->domain = irq_domain_add_legacy(node, bank->width, irq_base,
> +					     0, &irq_domain_simple_ops, NULL);
>  
>  	if (bank->regs->set_dataout && bank->regs->clr_dataout)
>  		bank->set_dataout = _set_gpio_dataout_reg;
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Grant Likely June 5, 2013, 10:33 p.m. UTC | #2

On Mon, 20 May 2013 10:46:21 -0700, Tony Lindgren <[e-mail]> wrote:
> * Tony Lindgren <[e-mail]> [130516 14:50]:
> > * Aaro Koskinen <[e-mail]> [130516 14:05]:
> > > On Thu, May 16, 2013 at 11:09:34AM -0700, Tony Lindgren wrote:
> > > > * Aaro Koskinen <[e-mail]> [130513 13:58]:
> > > > > I tested 3.10-rc1 on OMAP1 / Nokia 770, and Retu MFD probe is broken:
> > > > > 
> > > > > [    2.264221] retu-mfd 2-0001: Retu v3.2 found
> > > > > [    2.281951] retu-mfd 2-0001: Failed to allocate IRQs: -12
> > > > > [    2.300140] retu-mfd: probe of 2-0001 failed with error -12
> > > > > 
> > > > > The error is coming from regmap code. According to git bisect, it is
> > > > > caused by:
> > > > > 
> > > > > 	commit ede4d7a5b9835510fd1f724367f68d2fa4128453
> > > > > 	Author: Jon Hunter <[e-mail]>
> > > > > 	Date:   Fri Mar 1 11:22:47 2013 -0600
> > > > > 
> > > > > 	    gpio/omap: convert gpio irq domain to linear mapping
> > > > > 
> > > > > The commit does not anymore revert cleanly, and I haven't yet tried
> > > > > crafting a manual revert, so any fix proposals/ideas are welcome...
> > > > 
> > > > Hmm this might be a bit trickier to fix. Obviously the real solution
> > > > is to convert omap1 to SPARSE_IRQ like we did for omap2+.
> > > > 
> > > > For the -rc cycle, it might be possible to fix this by adding a
> > > > different irq_to_gpio() and gpio_to_irq() functions for omap1.
> > > 
> > > The commit reverts cleanly if we also revert
> > > 3513cdeccc647d41c4a9ff923af17deaaac04a66 (gpio/omap: optimise interrupt
> > > service routine), which seems to be just some minor optimization. The
> > > result is below, and with it 770 works again.
> > 
> > Hmm in this case it seems that we should just fix it rather than go back
> > to the old code, so let's take a look at that first.
> 
> Does the following fix it for you or do we need to fix something else
> there too?
> 

Hi Tony,

Do you want me to apply this fix? It sounds like it solves the symptoms,
but I'd like to know more about what the root cause is.

Send me your s-o-b line and I'll apply the patch

g.

> 
> 
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -1094,6 +1094,7 @@ static int omap_gpio_probe(struct platform_device *pdev)
>  	const struct omap_gpio_platform_data *pdata;
>  	struct resource *res;
>  	struct gpio_bank *bank;
> +	int irq_base;
>  
>  	match = of_match_device(of_match_ptr(omap_gpio_match), dev);
>  
> @@ -1135,11 +1136,23 @@ static int omap_gpio_probe(struct platform_device *pdev)
>  				pdata->get_context_loss_count;
>  	}
>  
> -
> -	bank->domain = irq_domain_add_linear(node, bank->width,
> -					     &irq_domain_simple_ops, NULL);
> -	if (!bank->domain)
> +	/*
> +	 * REVISIT: Once we have omap1 supporting  SPARSE_IRQ, we can drop
> +	 * irq_alloc_descs() and irq_domain_add_legacy() and just do:
> +	 *
> +	 * bank->domain = irq_domain_add_linear(node, bank->width,
> +	 *				     &irq_domain_simple_ops, NULL);
> +	 * if (!bank->domain)
> +	 *	return -ENODEV;
> +	 */
> +	irq_base = irq_alloc_descs(-1, 0, bank->width, 0);
> +	if (irq_base < 0) {
> +		dev_err(dev, "Couldn't allocate IRQ numbers\n");
>  		return -ENODEV;
> +	}
> +
> +	bank->domain = irq_domain_add_legacy(node, bank->width, irq_base,
> +					     0, &irq_domain_simple_ops, NULL);
>  
>  	if (bank->regs->set_dataout && bank->regs->clr_dataout)
>  		bank->set_dataout = _set_gpio_dataout_reg;

Tony Lindgren June 6, 2013, 3:53 p.m. UTC | #3

* Grant Likely <[e-mail]> [130605 15:39]:
> On Mon, 20 May 2013 10:46:21 -0700, Tony Lindgren <[e-mail]> wrote:
> > * Tony Lindgren <[e-mail]> [130516 14:50]:
> > > * Aaro Koskinen <[e-mail]> [130516 14:05]:
> > > > On Thu, May 16, 2013 at 11:09:34AM -0700, Tony Lindgren wrote:
> > > > > * Aaro Koskinen <[e-mail]> [130513 13:58]:
> > > > > > I tested 3.10-rc1 on OMAP1 / Nokia 770, and Retu MFD probe is broken:
> > > > > > 
> > > > > > [    2.264221] retu-mfd 2-0001: Retu v3.2 found
> > > > > > [    2.281951] retu-mfd 2-0001: Failed to allocate IRQs: -12
> > > > > > [    2.300140] retu-mfd: probe of 2-0001 failed with error -12
> > > > > > 
> > > > > > The error is coming from regmap code. According to git bisect, it is
> > > > > > caused by:
> > > > > > 
> > > > > > 	commit ede4d7a5b9835510fd1f724367f68d2fa4128453
> > > > > > 	Author: Jon Hunter <[e-mail]>
> > > > > > 	Date:   Fri Mar 1 11:22:47 2013 -0600
> > > > > > 
> > > > > > 	    gpio/omap: convert gpio irq domain to linear mapping
> > > > > > 
> > > > > > The commit does not anymore revert cleanly, and I haven't yet tried
> > > > > > crafting a manual revert, so any fix proposals/ideas are welcome...
> > > > > 
> > > > > Hmm this might be a bit trickier to fix. Obviously the real solution
> > > > > is to convert omap1 to SPARSE_IRQ like we did for omap2+.
> > > > > 
> > > > > For the -rc cycle, it might be possible to fix this by adding a
> > > > > different irq_to_gpio() and gpio_to_irq() functions for omap1.
> > > > 
> > > > The commit reverts cleanly if we also revert
> > > > 3513cdeccc647d41c4a9ff923af17deaaac04a66 (gpio/omap: optimise interrupt
> > > > service routine), which seems to be just some minor optimization. The
> > > > result is below, and with it 770 works again.
> > > 
> > > Hmm in this case it seems that we should just fix it rather than go back
> > > to the old code, so let's take a look at that first.
> > 
> > Does the following fix it for you or do we need to fix something else
> > there too?
> > 
> 
> Hi Tony,
> 
> Do you want me to apply this fix? It sounds like it solves the symptoms,
> but I'd like to know more about what the root cause is.
> 
> Send me your s-o-b line and I'll apply the patch

Yes sorry was meaning to send it as a proper patch, but got distracted.
Please go ahead and apply it thanks:

Signed-off-by: Tony Lindgren <[e-mail]>

 
> > --- a/drivers/gpio/gpio-omap.c
> > +++ b/drivers/gpio/gpio-omap.c
> > @@ -1094,6 +1094,7 @@ static int omap_gpio_probe(struct platform_device *pdev)
> >  	const struct omap_gpio_platform_data *pdata;
> >  	struct resource *res;
> >  	struct gpio_bank *bank;
> > +	int irq_base;
> >  
> >  	match = of_match_device(of_match_ptr(omap_gpio_match), dev);
> >  
> > @@ -1135,11 +1136,23 @@ static int omap_gpio_probe(struct platform_device *pdev)
> >  				pdata->get_context_loss_count;
> >  	}
> >  
> > -
> > -	bank->domain = irq_domain_add_linear(node, bank->width,
> > -					     &irq_domain_simple_ops, NULL);
> > -	if (!bank->domain)
> > +	/*
> > +	 * REVISIT: Once we have omap1 supporting  SPARSE_IRQ, we can drop
> > +	 * irq_alloc_descs() and irq_domain_add_legacy() and just do:
> > +	 *
> > +	 * bank->domain = irq_domain_add_linear(node, bank->width,
> > +	 *				     &irq_domain_simple_ops, NULL);
> > +	 * if (!bank->domain)
> > +	 *	return -ENODEV;
> > +	 */
> > +	irq_base = irq_alloc_descs(-1, 0, bank->width, 0);
> > +	if (irq_base < 0) {
> > +		dev_err(dev, "Couldn't allocate IRQ numbers\n");
> >  		return -ENODEV;
> > +	}
> > +
> > +	bank->domain = irq_domain_add_legacy(node, bank->width, irq_base,
> > +					     0, &irq_domain_simple_ops, NULL);
> >  
> >  	if (bank->regs->set_dataout && bank->regs->clr_dataout)
> >  		bank->set_dataout = _set_gpio_dataout_reg;
> 
> -- 
> Grant Likely, B.Sc, P.Eng.
> Secret Lab Technologies, Ltd.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Aaro Koskinen June 23, 2013, 10:16 p.m. UTC | #4

On Thu, Jun 06, 2013 at 08:53:41AM -0700, Tony Lindgren wrote:
> * Grant Likely <[e-mail]> [130605 15:39]:
> > On Mon, 20 May 2013 10:46:21 -0700, Tony Lindgren <[e-mail]> wrote:
> > > * Tony Lindgren <[e-mail]> [130516 14:50]:
> > > > * Aaro Koskinen <[e-mail]> [130516 14:05]:
> > > > > On Thu, May 16, 2013 at 11:09:34AM -0700, Tony Lindgren wrote:
> > > > > > * Aaro Koskinen <[e-mail]> [130513 13:58]:
> > > > > > > I tested 3.10-rc1 on OMAP1 / Nokia 770, and Retu MFD probe is broken:
> > > > > > > 
> > > > > > > [    2.264221] retu-mfd 2-0001: Retu v3.2 found
> > > > > > > [    2.281951] retu-mfd 2-0001: Failed to allocate IRQs: -12
> > > > > > > [    2.300140] retu-mfd: probe of 2-0001 failed with error -12
> > > > > > > 
> > > > > > > The error is coming from regmap code. According to git bisect, it is
> > > > > > > caused by:
> > > > > > > 
> > > > > > > 	commit ede4d7a5b9835510fd1f724367f68d2fa4128453
> > > > > > > 	Author: Jon Hunter <[e-mail]>
> > > > > > > 	Date:   Fri Mar 1 11:22:47 2013 -0600
> > > > > > > 
> > > > > > > 	    gpio/omap: convert gpio irq domain to linear mapping
> > > > > > > 
> > > > > > > The commit does not anymore revert cleanly, and I haven't yet tried
> > > > > > > crafting a manual revert, so any fix proposals/ideas are welcome...
> > > > > > 
> > > > > > Hmm this might be a bit trickier to fix. Obviously the real solution
> > > > > > is to convert omap1 to SPARSE_IRQ like we did for omap2+.
> > > > > > 
> > > > > > For the -rc cycle, it might be possible to fix this by adding a
> > > > > > different irq_to_gpio() and gpio_to_irq() functions for omap1.
> > > > > 
> > > > > The commit reverts cleanly if we also revert
> > > > > 3513cdeccc647d41c4a9ff923af17deaaac04a66 (gpio/omap: optimise interrupt
> > > > > service routine), which seems to be just some minor optimization. The
> > > > > result is below, and with it 770 works again.
> > > > 
> > > > Hmm in this case it seems that we should just fix it rather than go back
> > > > to the old code, so let's take a look at that first.
> > > 
> > > Does the following fix it for you or do we need to fix something else
> > > there too?
> > > 
> > 
> > Hi Tony,
> > 
> > Do you want me to apply this fix? It sounds like it solves the symptoms,
> > but I'd like to know more about what the root cause is.
> > 
> > Send me your s-o-b line and I'll apply the patch
> 
> Yes sorry was meaning to send it as a proper patch, but got distracted.
> Please go ahead and apply it thanks:
> 
> Signed-off-by: Tony Lindgren <[e-mail]>

What is the status of this patch? We're already at 3.10-rc7 and GPIO
IRQs are still broken on OMAP1.

A.

> > > --- a/drivers/gpio/gpio-omap.c
> > > +++ b/drivers/gpio/gpio-omap.c
> > > @@ -1094,6 +1094,7 @@ static int omap_gpio_probe(struct platform_device *pdev)
> > >  	const struct omap_gpio_platform_data *pdata;
> > >  	struct resource *res;
> > >  	struct gpio_bank *bank;
> > > +	int irq_base;
> > >  
> > >  	match = of_match_device(of_match_ptr(omap_gpio_match), dev);
> > >  
> > > @@ -1135,11 +1136,23 @@ static int omap_gpio_probe(struct platform_device *pdev)
> > >  				pdata->get_context_loss_count;
> > >  	}
> > >  
> > > -
> > > -	bank->domain = irq_domain_add_linear(node, bank->width,
> > > -					     &irq_domain_simple_ops, NULL);
> > > -	if (!bank->domain)
> > > +	/*
> > > +	 * REVISIT: Once we have omap1 supporting  SPARSE_IRQ, we can drop
> > > +	 * irq_alloc_descs() and irq_domain_add_legacy() and just do:
> > > +	 *
> > > +	 * bank->domain = irq_domain_add_linear(node, bank->width,
> > > +	 *				     &irq_domain_simple_ops, NULL);
> > > +	 * if (!bank->domain)
> > > +	 *	return -ENODEV;
> > > +	 */
> > > +	irq_base = irq_alloc_descs(-1, 0, bank->width, 0);
> > > +	if (irq_base < 0) {
> > > +		dev_err(dev, "Couldn't allocate IRQ numbers\n");
> > >  		return -ENODEV;
> > > +	}
> > > +
> > > +	bank->domain = irq_domain_add_legacy(node, bank->width, irq_base,
> > > +					     0, &irq_domain_simple_ops, NULL);
> > >  
> > >  	if (bank->regs->set_dataout && bank->regs->clr_dataout)
> > >  		bank->set_dataout = _set_gpio_dataout_reg;
> > 
> > -- 
> > Grant Likely, B.Sc, P.Eng.
> > Secret Lab Technologies, Ltd.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Javier Martinez Canillas June 23, 2013, 11:06 p.m. UTC | #5

On Mon, Jun 24, 2013 at 12:16 AM, Aaro Koskinen <[e-mail]> wrote:
> On Thu, Jun 06, 2013 at 08:53:41AM -0700, Tony Lindgren wrote:
>> * Grant Likely <[e-mail]> [130605 15:39]:
>> > On Mon, 20 May 2013 10:46:21 -0700, Tony Lindgren <[e-mail]> wrote:
>> > > * Tony Lindgren <[e-mail]> [130516 14:50]:
>> > > > * Aaro Koskinen <[e-mail]> [130516 14:05]:
>> > > > > On Thu, May 16, 2013 at 11:09:34AM -0700, Tony Lindgren wrote:
>> > > > > > * Aaro Koskinen <[e-mail]> [130513 13:58]:
>> > > > > > > I tested 3.10-rc1 on OMAP1 / Nokia 770, and Retu MFD probe is broken:
>> > > > > > >
>> > > > > > > [    2.264221] retu-mfd 2-0001: Retu v3.2 found
>> > > > > > > [    2.281951] retu-mfd 2-0001: Failed to allocate IRQs: -12
>> > > > > > > [    2.300140] retu-mfd: probe of 2-0001 failed with error -12
>> > > > > > >
>> > > > > > > The error is coming from regmap code. According to git bisect, it is
>> > > > > > > caused by:
>> > > > > > >
>> > > > > > >   commit ede4d7a5b9835510fd1f724367f68d2fa4128453
>> > > > > > >   Author: Jon Hunter <[e-mail]>
>> > > > > > >   Date:   Fri Mar 1 11:22:47 2013 -0600
>> > > > > > >
>> > > > > > >       gpio/omap: convert gpio irq domain to linear mapping
>> > > > > > >
>> > > > > > > The commit does not anymore revert cleanly, and I haven't yet tried
>> > > > > > > crafting a manual revert, so any fix proposals/ideas are welcome...
>> > > > > >
>> > > > > > Hmm this might be a bit trickier to fix. Obviously the real solution
>> > > > > > is to convert omap1 to SPARSE_IRQ like we did for omap2+.
>> > > > > >
>> > > > > > For the -rc cycle, it might be possible to fix this by adding a
>> > > > > > different irq_to_gpio() and gpio_to_irq() functions for omap1.
>> > > > >
>> > > > > The commit reverts cleanly if we also revert
>> > > > > 3513cdeccc647d41c4a9ff923af17deaaac04a66 (gpio/omap: optimise interrupt
>> > > > > service routine), which seems to be just some minor optimization. The
>> > > > > result is below, and with it 770 works again.
>> > > >
>> > > > Hmm in this case it seems that we should just fix it rather than go back
>> > > > to the old code, so let's take a look at that first.
>> > >
>> > > Does the following fix it for you or do we need to fix something else
>> > > there too?
>> > >
>> >
>> > Hi Tony,
>> >
>> > Do you want me to apply this fix? It sounds like it solves the symptoms,
>> > but I'd like to know more about what the root cause is.
>> >
>> > Send me your s-o-b line and I'll apply the patch
>>
>> Yes sorry was meaning to send it as a proper patch, but got distracted.
>> Please go ahead and apply it thanks:
>>
>> Signed-off-by: Tony Lindgren <[e-mail]>
>
> What is the status of this patch? We're already at 3.10-rc7 and GPIO
> IRQs are still broken on OMAP1.
>
> A.
>

Hello,

There is a problem with this patch.

Currently with DeviceTree, using an OMAP GPIO as an IRQ is broken
since before a driver can request the IRQ line, the GPIO bank has to
be enabled and also the GPIO line has to be setup and configured as
input with gpio_request().

This means that when defining:

                gpio6: gpio@49058000 {
                        compatible = "ti,omap3-gpio";
                        reg = <0x49058000 0x200>;
                        interrupts = <34>;
                        ti,hwmods = "gpio6";
                        gpio-controller;
                        #gpio-cells = <2>;
                        interrupt-controller;
                        #interrupt-cells = <2>;
                };

                interrupt-parent = <&gpio6>;
                interrupts = <16 8>;

The GPIO is correctly mapped as an IRQ but a call to gpio_request() is
never made and thus a all to request_irq() from the driver of the
device using the GPIO-IRQ line fails.

I posted the following patches that adds a custom .map function
handler that calls gpio_request() automatically when an OMAP GPIO line
is mapped as an IRQ:

[v2,1/2] gpio/omap: don't create an IRQ mapping for every GPIO on DT [1].
[v2,2/2] gpio/omap: auto request GPIO as input if used as IRQ via DT [2].

Now, this won't work if we revert to the legacy domain mapping. Of
course we can came with a different solution but it will even more
hack-ish than the one I posted.

So I think that the correct solution is to add SPARSE_IRQ support to
omap1 and not reverting Jon's patch. Of course this may not be
possible since we are so close to 3.10 and most OMAP patches already
merged for 3.11 but we should definitely try to have this at least for
3.12. Otherwise we won't be able to move to DT-only booting for
OMAP2+.

Thanks a lot and best regards,
Javier

[1]: https://patchwork.kernel.org/patch/2764821/
[2]: https://patchwork.kernel.org/patch/2764811/
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Aaro Koskinen June 23, 2013, 11:43 p.m. UTC | #6

Hi,

On Mon, Jun 24, 2013 at 01:06:37AM +0200, Javier Martinez Canillas wrote:
> On Mon, Jun 24, 2013 at 12:16 AM, Aaro Koskinen <[e-mail]> wrote:
> > What is the status of this patch? We're already at 3.10-rc7 and GPIO
> > IRQs are still broken on OMAP1.

[...]

> There is a problem with this patch.

[...]

> So I think that the correct solution is to add SPARSE_IRQ support to
> omap1 and not reverting Jon's patch. Of course this may not be
> possible since we are so close to 3.10 and most OMAP patches already
> merged for 3.11 but we should definitely try to have this at least for
> 3.12. Otherwise we won't be able to move to DT-only booting for
> OMAP2+.

OMAP1 does not use DT. So we could put this code under #ifdef
CONFIG_ARCH_OMAP1 or similar. It's just a few lines of code. OMAP2+
work should not regress OMAP1.

Demanding SPARSE_IRQ support for OMAP1 should have been discussed before
these changes were made. It's not reasonable to assume such things can
be made during rc-cycle. Also, now, I don't think it's reasonable to
wait for that to be done, as it would take until 3.12 or even later to
get OMAP1 functional again.

A.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Javier Martinez Canillas June 24, 2013, 1:01 a.m. UTC | #7

On Mon, Jun 24, 2013 at 1:43 AM, Aaro Koskinen <[e-mail]> wrote:
> Hi,
>
> On Mon, Jun 24, 2013 at 01:06:37AM +0200, Javier Martinez Canillas wrote:
>> On Mon, Jun 24, 2013 at 12:16 AM, Aaro Koskinen <[e-mail]> wrote:
>> > What is the status of this patch? We're already at 3.10-rc7 and GPIO
>> > IRQs are still broken on OMAP1.
>
> [...]
>
>> There is a problem with this patch.
>
> [...]
>
>> So I think that the correct solution is to add SPARSE_IRQ support to
>> omap1 and not reverting Jon's patch. Of course this may not be
>> possible since we are so close to 3.10 and most OMAP patches already
>> merged for 3.11 but we should definitely try to have this at least for
>> 3.12. Otherwise we won't be able to move to DT-only booting for
>> OMAP2+.
>
> OMAP1 does not use DT. So we could put this code under #ifdef
> CONFIG_ARCH_OMAP1 or similar. It's just a few lines of code. OMAP2+
> work should not regress OMAP1.
>
> Demanding SPARSE_IRQ support for OMAP1 should have been discussed before
> these changes were made. It's not reasonable to assume such things can
> be made during rc-cycle. Also, now, I don't think it's reasonable to
> wait for that to be done, as it would take until 3.12 or even later to
> get OMAP1 functional again.
>
> A.

Hi,

Yes, since we are so late in the -rc cycle and OMAP1 is currently
broken I agree that the only sensible solution is to revert the patch
for now.

I just wanted to point out the issue that keeping the OMAP GPIO driver
using legacy mapping domain represents a blocker to have GPIO-IRQ
working with Device Tree for OMAP2+

Thanks a lot and best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tony Lindgren June 24, 2013, 7:21 a.m. UTC | #8

* Javier Martinez Canillas <[e-mail]> [130623 18:08]:
> On Mon, Jun 24, 2013 at 1:43 AM, Aaro Koskinen <[e-mail]> wrote:
> > Hi,
> >
> > On Mon, Jun 24, 2013 at 01:06:37AM +0200, Javier Martinez Canillas wrote:
> >> On Mon, Jun 24, 2013 at 12:16 AM, Aaro Koskinen <[e-mail]> wrote:
> >> > What is the status of this patch? We're already at 3.10-rc7 and GPIO
> >> > IRQs are still broken on OMAP1.
> >
> > [...]
> >
> >> There is a problem with this patch.
> >
> > [...]
> >
> >> So I think that the correct solution is to add SPARSE_IRQ support to
> >> omap1 and not reverting Jon's patch. Of course this may not be
> >> possible since we are so close to 3.10 and most OMAP patches already
> >> merged for 3.11 but we should definitely try to have this at least for
> >> 3.12. Otherwise we won't be able to move to DT-only booting for
> >> OMAP2+.
> >
> > OMAP1 does not use DT. So we could put this code under #ifdef
> > CONFIG_ARCH_OMAP1 or similar. It's just a few lines of code. OMAP2+
> > work should not regress OMAP1.
> >
> > Demanding SPARSE_IRQ support for OMAP1 should have been discussed before
> > these changes were made. It's not reasonable to assume such things can
> > be made during rc-cycle. Also, now, I don't think it's reasonable to
> > wait for that to be done, as it would take until 3.12 or even later to
> > get OMAP1 functional again.
> >
> > A.
> 
> Hi,
> 
> Yes, since we are so late in the -rc cycle and OMAP1 is currently
> broken I agree that the only sensible solution is to revert the patch
> for now.

Agreed.
 
> I just wanted to point out the issue that keeping the OMAP GPIO driver
> using legacy mapping domain represents a blocker to have GPIO-IRQ
> working with Device Tree for OMAP2+

Yes. We can do the ifdef Aaro suggested, and let's also plan on
converting omap1 to use SPARSE_IRQ. But with the ifdef we can cut
away the dependency between these two.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Grant Likely June 24, 2013, 3:53 p.m. UTC | #9

On Mon, Jun 24, 2013 at 8:21 AM, Tony Lindgren <[e-mail]> wrote:
> * Javier Martinez Canillas <[e-mail]> [130623 18:08]:
>> On Mon, Jun 24, 2013 at 1:43 AM, Aaro Koskinen <[e-mail]> wrote:
>> > Hi,
>> >
>> > On Mon, Jun 24, 2013 at 01:06:37AM +0200, Javier Martinez Canillas wrote:
>> >> On Mon, Jun 24, 2013 at 12:16 AM, Aaro Koskinen <[e-mail]> wrote:
>> >> > What is the status of this patch? We're already at 3.10-rc7 and GPIO
>> >> > IRQs are still broken on OMAP1.
>> >
>> > [...]
>> >
>> >> There is a problem with this patch.
>> >
>> > [...]
>> >
>> >> So I think that the correct solution is to add SPARSE_IRQ support to
>> >> omap1 and not reverting Jon's patch. Of course this may not be
>> >> possible since we are so close to 3.10 and most OMAP patches already
>> >> merged for 3.11 but we should definitely try to have this at least for
>> >> 3.12. Otherwise we won't be able to move to DT-only booting for
>> >> OMAP2+.
>> >
>> > OMAP1 does not use DT. So we could put this code under #ifdef
>> > CONFIG_ARCH_OMAP1 or similar. It's just a few lines of code. OMAP2+
>> > work should not regress OMAP1.
>> >
>> > Demanding SPARSE_IRQ support for OMAP1 should have been discussed before
>> > these changes were made. It's not reasonable to assume such things can
>> > be made during rc-cycle. Also, now, I don't think it's reasonable to
>> > wait for that to be done, as it would take until 3.12 or even later to
>> > get OMAP1 functional again.
>> >
>> > A.
>>
>> Hi,
>>
>> Yes, since we are so late in the -rc cycle and OMAP1 is currently
>> broken I agree that the only sensible solution is to revert the patch
>> for now.
>
> Agreed.
>
>> I just wanted to point out the issue that keeping the OMAP GPIO driver
>> using legacy mapping domain represents a blocker to have GPIO-IRQ
>> working with Device Tree for OMAP2+
>
> Yes. We can do the ifdef Aaro suggested, and let's also plan on
> converting omap1 to use SPARSE_IRQ. But with the ifdef we can cut
> away the dependency between these two.

Alright. Sorry I dropped the ball on this one. I've lost track of
which patch needs to get applied, but given that it is so late in the
cycle, it would be better for someone else to apply the change, test
and send a pull request to Linus. I'm okay with it going through the
OMAP tree if that is the most expedient. Alternately, send me the pull
request and I'll pass it on to Linus.

g.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tony Lindgren June 25, 2013, 7:04 a.m. UTC | #10

* Grant Likely <[e-mail]> [130624 09:00]:
> On Mon, Jun 24, 2013 at 8:21 AM, Tony Lindgren <[e-mail]> wrote:
> > * Javier Martinez Canillas <[e-mail]> [130623 18:08]:
> >> On Mon, Jun 24, 2013 at 1:43 AM, Aaro Koskinen <[e-mail]> wrote:
> >> > Hi,
> >> >
> >> > On Mon, Jun 24, 2013 at 01:06:37AM +0200, Javier Martinez Canillas wrote:
> >> >> On Mon, Jun 24, 2013 at 12:16 AM, Aaro Koskinen <[e-mail]> wrote:
> >> >> > What is the status of this patch? We're already at 3.10-rc7 and GPIO
> >> >> > IRQs are still broken on OMAP1.
> >> >
> >> > [...]
> >> >
> >> >> There is a problem with this patch.
> >> >
> >> > [...]
> >> >
> >> >> So I think that the correct solution is to add SPARSE_IRQ support to
> >> >> omap1 and not reverting Jon's patch. Of course this may not be
> >> >> possible since we are so close to 3.10 and most OMAP patches already
> >> >> merged for 3.11 but we should definitely try to have this at least for
> >> >> 3.12. Otherwise we won't be able to move to DT-only booting for
> >> >> OMAP2+.
> >> >
> >> > OMAP1 does not use DT. So we could put this code under #ifdef
> >> > CONFIG_ARCH_OMAP1 or similar. It's just a few lines of code. OMAP2+
> >> > work should not regress OMAP1.
> >> >
> >> > Demanding SPARSE_IRQ support for OMAP1 should have been discussed before
> >> > these changes were made. It's not reasonable to assume such things can
> >> > be made during rc-cycle. Also, now, I don't think it's reasonable to
> >> > wait for that to be done, as it would take until 3.12 or even later to
> >> > get OMAP1 functional again.
> >> >
> >> > A.
> >>
> >> Hi,
> >>
> >> Yes, since we are so late in the -rc cycle and OMAP1 is currently
> >> broken I agree that the only sensible solution is to revert the patch
> >> for now.
> >
> > Agreed.
> >
> >> I just wanted to point out the issue that keeping the OMAP GPIO driver
> >> using legacy mapping domain represents a blocker to have GPIO-IRQ
> >> working with Device Tree for OMAP2+
> >
> > Yes. We can do the ifdef Aaro suggested, and let's also plan on
> > converting omap1 to use SPARSE_IRQ. But with the ifdef we can cut
> > away the dependency between these two.
> 
> Alright. Sorry I dropped the ball on this one. I've lost track of
> which patch needs to get applied, but given that it is so late in the
> cycle, it would be better for someone else to apply the change, test
> and send a pull request to Linus. I'm okay with it going through the
> OMAP tree if that is the most expedient. Alternately, send me the pull
> request and I'll pass it on to Linus.

OK, I'll wait for Aaro's ack on Javier's patch and then put it into a
branch for you.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Grant Likely June 25, 2013, 11:49 a.m. UTC | #11

On Tue, Jun 25, 2013 at 8:04 AM, Tony Lindgren <[e-mail]> wrote:
> * Grant Likely <[e-mail]> [130624 09:00]:
>> On Mon, Jun 24, 2013 at 8:21 AM, Tony Lindgren <[e-mail]> wrote:
>> > * Javier Martinez Canillas <[e-mail]> [130623 18:08]:
>> >> On Mon, Jun 24, 2013 at 1:43 AM, Aaro Koskinen <[e-mail]> wrote:
>> >> > Hi,
>> >> >
>> >> > On Mon, Jun 24, 2013 at 01:06:37AM +0200, Javier Martinez Canillas wrote:
>> >> >> On Mon, Jun 24, 2013 at 12:16 AM, Aaro Koskinen <[e-mail]> wrote:
>> >> >> > What is the status of this patch? We're already at 3.10-rc7 and GPIO
>> >> >> > IRQs are still broken on OMAP1.
>> >> >
>> >> > [...]
>> >> >
>> >> >> There is a problem with this patch.
>> >> >
>> >> > [...]
>> >> >
>> >> >> So I think that the correct solution is to add SPARSE_IRQ support to
>> >> >> omap1 and not reverting Jon's patch. Of course this may not be
>> >> >> possible since we are so close to 3.10 and most OMAP patches already
>> >> >> merged for 3.11 but we should definitely try to have this at least for
>> >> >> 3.12. Otherwise we won't be able to move to DT-only booting for
>> >> >> OMAP2+.
>> >> >
>> >> > OMAP1 does not use DT. So we could put this code under #ifdef
>> >> > CONFIG_ARCH_OMAP1 or similar. It's just a few lines of code. OMAP2+
>> >> > work should not regress OMAP1.
>> >> >
>> >> > Demanding SPARSE_IRQ support for OMAP1 should have been discussed before
>> >> > these changes were made. It's not reasonable to assume such things can
>> >> > be made during rc-cycle. Also, now, I don't think it's reasonable to
>> >> > wait for that to be done, as it would take until 3.12 or even later to
>> >> > get OMAP1 functional again.
>> >> >
>> >> > A.
>> >>
>> >> Hi,
>> >>
>> >> Yes, since we are so late in the -rc cycle and OMAP1 is currently
>> >> broken I agree that the only sensible solution is to revert the patch
>> >> for now.
>> >
>> > Agreed.
>> >
>> >> I just wanted to point out the issue that keeping the OMAP GPIO driver
>> >> using legacy mapping domain represents a blocker to have GPIO-IRQ
>> >> working with Device Tree for OMAP2+
>> >
>> > Yes. We can do the ifdef Aaro suggested, and let's also plan on
>> > converting omap1 to use SPARSE_IRQ. But with the ifdef we can cut
>> > away the dependency between these two.
>>
>> Alright. Sorry I dropped the ball on this one. I've lost track of
>> which patch needs to get applied, but given that it is so late in the
>> cycle, it would be better for someone else to apply the change, test
>> and send a pull request to Linus. I'm okay with it going through the
>> OMAP tree if that is the most expedient. Alternately, send me the pull
>> request and I'll pass it on to Linus.
>
> OK, I'll wait for Aaro's ack on Javier's patch and then put it into a
> branch for you.

Thanks Tony.

g.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tony Lindgren June 26, 2013, 7:06 a.m. UTC | #12

* Grant Likely <[e-mail]> [130625 04:56]:
> On Tue, Jun 25, 2013 at 8:04 AM, Tony Lindgren <[e-mail]> wrote:
> >
> > OK, I'll wait for Aaro's ack on Javier's patch and then put it into a
> > branch for you.
> 
> Thanks Tony.

Pull request sent as "[GIT PULL] GPIO regression fix for omap1 for v3.10-rc".

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1094,6 +1094,7 @@  static int omap_gpio_probe(struct platform_device *pdev)
 	const struct omap_gpio_platform_data *pdata;
 	struct resource *res;
 	struct gpio_bank *bank;
+	int irq_base;
 
 	match = of_match_device(of_match_ptr(omap_gpio_match), dev);
 
@@ -1135,11 +1136,23 @@  static int omap_gpio_probe(struct platform_device *pdev)
 				pdata->get_context_loss_count;
 	}
 
-
-	bank->domain = irq_domain_add_linear(node, bank->width,
-					     &irq_domain_simple_ops, NULL);
-	if (!bank->domain)
+	/*
+	 * REVISIT: Once we have omap1 supporting  SPARSE_IRQ, we can drop
+	 * irq_alloc_descs() and irq_domain_add_legacy() and just do:
+	 *
+	 * bank->domain = irq_domain_add_linear(node, bank->width,
+	 *				     &irq_domain_simple_ops, NULL);
+	 * if (!bank->domain)
+	 *	return -ENODEV;
+	 */
+	irq_base = irq_alloc_descs(-1, 0, bank->width, 0);
+	if (irq_base < 0) {
+		dev_err(dev, "Couldn't allocate IRQ numbers\n");
 		return -ENODEV;
+	}
+
+	bank->domain = irq_domain_add_legacy(node, bank->width, irq_base,
+					     0, &irq_domain_simple_ops, NULL);
 
 	if (bank->regs->set_dataout && bank->regs->clr_dataout)
 		bank->set_dataout = _set_gpio_dataout_reg;