| Message ID | 1344256323-10484-1-git-send-email-lee.jones@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Lee, On Mon, Aug 06, 2012 at 01:32:03PM +0100, Lee Jones wrote: > If we're booting with Device Tree enabled, we want the IRQ numbers to > be taken and translated from the Device Tree binary. If not, they > should be taken from the resource allocation defined in the AB8500 MFD > core driver. > > Tested-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Lee Jones <lee.jones@linaro.org> > --- > drivers/input/misc/ab8500-ponkey.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/input/misc/ab8500-ponkey.c b/drivers/input/misc/ab8500-ponkey.c > index 1a1d974..afcd87f 100644 > --- a/drivers/input/misc/ab8500-ponkey.c > +++ b/drivers/input/misc/ab8500-ponkey.c > @@ -47,6 +47,7 @@ static irqreturn_t ab8500_ponkey_handler(int irq, void *data) > static int __devinit ab8500_ponkey_probe(struct platform_device *pdev) > { > struct ab8500 *ab8500 = dev_get_drvdata(pdev->dev.parent); > + struct device_node *np = pdev->dev.of_node; > struct ab8500_ponkey *ponkey; > struct input_dev *input; > int irq_dbf, irq_dbr; > @@ -73,8 +74,9 @@ static int __devinit ab8500_ponkey_probe(struct platform_device *pdev) > > ponkey->idev = input; > ponkey->ab8500 = ab8500; > - ponkey->irq_dbf = ab8500_irq_get_virq(ab8500, irq_dbf); > - ponkey->irq_dbr = ab8500_irq_get_virq(ab8500, irq_dbr); > + > + ponkey->irq_dbf = (np) ? ab8500_irq_get_virq(ab8500, irq_dbf) : irq_dbf; > + ponkey->irq_dbr = (np) ? ab8500_irq_get_virq(ab8500, irq_dbr) : irq_dbr; Why this isn't done inside ab8500_irq_get_virq()? Thanks.
On Mon, Aug 06, 2012 at 01:19:15AM -0700, Dmitry Torokhov wrote: > > - ponkey->irq_dbf = ab8500_irq_get_virq(ab8500, irq_dbf); > > - ponkey->irq_dbr = ab8500_irq_get_virq(ab8500, irq_dbr); > > + > > + ponkey->irq_dbf = (np) ? ab8500_irq_get_virq(ab8500, irq_dbf) : irq_dbf; > > + ponkey->irq_dbr = (np) ? ab8500_irq_get_virq(ab8500, irq_dbr) : irq_dbr; > > Why this isn't done inside ab8500_irq_get_virq()? There's no reason why it can't be. My first version of the patch did just that in fact. Would that be your preference?
On Mon, Aug 06, 2012 at 04:37:52PM +0100, Lee Jones wrote: > On Mon, Aug 06, 2012 at 01:19:15AM -0700, Dmitry Torokhov wrote: > > > + ponkey->irq_dbf = (np) ? ab8500_irq_get_virq(ab8500, irq_dbf) : irq_dbf; > > > + ponkey->irq_dbr = (np) ? ab8500_irq_get_virq(ab8500, irq_dbr) : irq_dbr; > > Why this isn't done inside ab8500_irq_get_virq()? > There's no reason why it can't be. > My first version of the patch did just that in fact. > Would that be your preference? Restating my comment elsewhere... why do we even need to do this in _get_virq() - I'd *really* expect this to be handled by the irq domain code.
On Mon, Aug 06, 2012 at 05:02:26PM +0100, Mark Brown wrote: > On Mon, Aug 06, 2012 at 04:37:52PM +0100, Lee Jones wrote: > > On Mon, Aug 06, 2012 at 01:19:15AM -0700, Dmitry Torokhov wrote: > > > > > + ponkey->irq_dbf = (np) ? ab8500_irq_get_virq(ab8500, irq_dbf) : irq_dbf; > > > > + ponkey->irq_dbr = (np) ? ab8500_irq_get_virq(ab8500, irq_dbr) : irq_dbr; > > > > Why this isn't done inside ab8500_irq_get_virq()? > > > There's no reason why it can't be. > > > My first version of the patch did just that in fact. > > > Would that be your preference? > > Restating my comment elsewhere... why do we even need to do this in > _get_virq() - I'd *really* expect this to be handled by the irq domain > code. I really should stop reading my emails backwards. :) I'll look into this tomorrow.
On Mon, Aug 06, 2012 at 05:02:26PM +0100, Mark Brown wrote: > On Mon, Aug 06, 2012 at 04:37:52PM +0100, Lee Jones wrote: > > On Mon, Aug 06, 2012 at 01:19:15AM -0700, Dmitry Torokhov wrote: > > > > > + ponkey->irq_dbf = (np) ? ab8500_irq_get_virq(ab8500, irq_dbf) : irq_dbf; > > > > + ponkey->irq_dbr = (np) ? ab8500_irq_get_virq(ab8500, irq_dbr) : irq_dbr; > > > > Why this isn't done inside ab8500_irq_get_virq()? > > > There's no reason why it can't be. > > > My first version of the patch did just that in fact. > > > Would that be your preference? > > Restating my comment elsewhere... why do we even need to do this in > _get_virq() - I'd *really* expect this to be handled by the irq domain > code. Okay, so I've just spent a small amount of time looking at this. I think the best place for this would be in *_get_virq(), using the same logic that selected a *_legacy or *_linear domain in the first place. The only thing the domain can test for is the 'type' of domain and the requested IRQ. This is where the issue lies. If a hwirq to virq conversion is requested, but a virq is passed (which happens in the non-DT case) a WARN() is triggered because the irq passed is bigger than first_irq + size. I think *_get_virq() should ensure that only a hwirq is passed to irq_create_mapping(). Let me know if you had other ideas.
On Tue, Aug 07, 2012 at 06:01:30PM +0100, Lee Jones wrote: > Okay, so I've just spent a small amount of time looking at this. I think > the best place for this would be in *_get_virq(), using the same logic that > selected a *_legacy or *_linear domain in the first place. The only thing > the domain can test for is the 'type' of domain and the requested IRQ. This > is where the issue lies. If a hwirq to virq conversion is requested, but a > virq is passed (which happens in the non-DT case) a WARN() is triggered > because the irq passed is bigger than first_irq + size. I think *_get_virq() > should ensure that only a hwirq is passed to irq_create_mapping(). > Let me know if you had other ideas. I'd expect your driver to always pass a hwirq into _get_virq() here.
> Restating my comment elsewhere... why do we even need to do this in > _get_virq() - I'd *really* expect this to be handled by the irq domain > code. > > Okay, so I've just spent a small amount of time looking at this. I think > > the best place for this would be in *_get_virq(), using the same logic that > > selected a *_legacy or *_linear domain in the first place. > I'd expect your driver to always pass a hwirq into _get_virq() here. I actually had this thought last night. (I really must learn to switch off in the evenings.) ;) That's even easier then. I'll make the changes and submit.
On Tue, Aug 07, 2012 at 06:03:34PM +0100, Mark Brown wrote: > On Tue, Aug 07, 2012 at 06:01:30PM +0100, Lee Jones wrote: > > > Okay, so I've just spent a small amount of time looking at this. I think > > the best place for this would be in *_get_virq(), using the same logic that > > selected a *_legacy or *_linear domain in the first place. The only thing > > the domain can test for is the 'type' of domain and the requested IRQ. This > > is where the issue lies. If a hwirq to virq conversion is requested, but a > > virq is passed (which happens in the non-DT case) a WARN() is triggered > > because the irq passed is bigger than first_irq + size. I think *_get_virq() > > should ensure that only a hwirq is passed to irq_create_mapping(). > > > Let me know if you had other ideas. > > I'd expect your driver to always pass a hwirq into _get_virq() here. Okay, actually this isn't so easy. Currently we have: During DT boot: - No platform data is passed, hence no IRQ base for AB8500 is either - No IRQ base means we register a Linear IRQ Domain - MFD sees there is no base and leaves the IRQ resource as a hwirq - AB8500 child devices use *_get_virq() to convert the hwirq to a virq During non-DT boot: - Platform data is passed, which contains an IRQ base - If an IRQ base is requested we use it to register a Legacy IRQ Domain - MFD adds the IRQ base to the hwirq and registers it as a virq - AB8500 child devices use *_get_virq() to convert virq to virq - *ERROR* I guess my suggestion falls-back to placing logic in *_get_virq() to only call irq_create_mapping() when when !ab8500->irq_base.
On Wednesday 08 August 2012, Lee Jones wrote: > Okay, actually this isn't so easy. Currently we have: > > During DT boot: > - No platform data is passed, hence no IRQ base for AB8500 is either > - No IRQ base means we register a Linear IRQ Domain > - MFD sees there is no base and leaves the IRQ resource as a hwirq > - AB8500 child devices use *_get_virq() to convert the hwirq to a virq > > During non-DT boot: > - Platform data is passed, which contains an IRQ base > - If an IRQ base is requested we use it to register a Legacy IRQ Domain > - MFD adds the IRQ base to the hwirq and registers it as a virq > - AB8500 child devices use *_get_virq() to convert virq to virq - ERROR > > I guess my suggestion falls-back to placing logic in *_get_virq() to only > call irq_create_mapping() when when !ab8500->irq_base. In general, it seems easier to use the same domain type for both cases. I don't think that MOP500_AB8500_VIR_GPIO_IRQ_BASE is used anywhere else besides the .irq_base definition in board-mop500.c, so I would guess that you can just remove that identifier and always use the linear domain. Arnd
On Wed, Aug 08, 2012 at 09:04:12AM +0100, Lee Jones wrote: > During non-DT boot: > - Platform data is passed, which contains an IRQ base > - If an IRQ base is requested we use it to register a Legacy IRQ Domain > - MFD adds the IRQ base to the hwirq and registers it as a virq Just don't do this step - the only reason to do it is for mapping back into a linear domain but if you're going to do this... > - AB8500 child devices use *_get_virq() to convert virq to virq - *ERROR* ...then it's redundant. The mapping functions in the domain code replace this functionality.
On Wed, Aug 08, 2012 at 08:28:35AM +0000, Arnd Bergmann wrote: > In general, it seems easier to use the same domain type for both cases. > I don't think that MOP500_AB8500_VIR_GPIO_IRQ_BASE is used anywhere > else besides the .irq_base definition in board-mop500.c, so I would guess > that you can just remove that identifier and always use the linear > domain. Given how easy this is to get working it doesn't seem sensible to avoid it - there's a clear pattern already for maintaining support for legacy domains.
On Wed, Aug 08, 2012 at 10:49:52AM +0100, Mark Brown wrote: > On Wed, Aug 08, 2012 at 09:04:12AM +0100, Lee Jones wrote: > > > During non-DT boot: > > - Platform data is passed, which contains an IRQ base > > - If an IRQ base is requested we use it to register a Legacy IRQ Domain > > - MFD adds the IRQ base to the hwirq and registers it as a virq > > Just don't do this step - the only reason to do it is for mapping back > into a linear domain but if you're going to do this... > > > - AB8500 child devices use *_get_virq() to convert virq to virq - *ERROR* > > ...then it's redundant. The mapping functions in the domain code > replace this functionality. No, the other way round. This is now required all the time. Now we force the use of hwirq, the driver needs to convert that into a virq before requesting the resource. So we need to put *_get_virq()'s into every child device that requests an IRQ.
On Wed, Aug 08, 2012 at 12:40:38PM +0100, Lee Jones wrote: > On Wed, Aug 08, 2012 at 10:49:52AM +0100, Mark Brown wrote: > > > - MFD adds the IRQ base to the hwirq and registers it as a virq > > Just don't do this step - the only reason to do it is for mapping back > > into a linear domain but if you're going to do this... > > > - AB8500 child devices use *_get_virq() to convert virq to virq - *ERROR* > > ...then it's redundant. The mapping functions in the domain code > > replace this functionality. > No, the other way round. This is now required all the time. > Now we force the use of hwirq, the driver needs to convert that into a > virq before requesting the resource. So we need to put *_get_virq()'s into > every child device that requests an IRQ. Yes, that's exactly what I said - and as a result of doing this the remapping in the MFD core becomes redundant.
On Mon, Aug 6, 2012 at 2:32 PM, Lee Jones <lee.jones@linaro.org> wrote: > If we're booting with Device Tree enabled, we want the IRQ numbers to > be taken and translated from the Device Tree binary. If not, they > should be taken from the resource allocation defined in the AB8500 MFD > core driver. > > Tested-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Lee Jones <lee.jones@linaro.org> Not having this patch in v3.6-rcN gives the following boot noise (and the key does not work): ------------[ cut here ]------------ WARNING: at /home/elinwal/linux-stericsson/kernel/irq/irqdomain.c:137 irq_domain_legacy_revmap+0x20/0x48() Modules linked in: [<c0014710>] (unwind_backtrace+0x0/0xf8) from [<c001d37c>] (warn_slowpath_common+0x4c/0x64) [<c001d37c>] (warn_slowpath_common+0x4c/0x64) from [<c001d3b0>] (warn_slowpath_null+0x1c/0x24) [<c001d3b0>] (warn_slowpath_null+0x1c/0x24) from [<c0064200>] (irq_domain_legacy_revmap+0x20/0x48) [<c0064200>] (irq_domain_legacy_revmap+0x20/0x48) from [<c02cea28>] (ab8500_ponkey_probe+0xd0/0x1f8) [<c02cea28>] (ab8500_ponkey_probe+0xd0/0x1f8) from [<c01a1e20>] (platform_drv_probe+0x14/0x18) [<c01a1e20>] (platform_drv_probe+0x14/0x18) from [<c01a0be4>] (driver_probe_device+0x78/0x208) [<c01a0be4>] (driver_probe_device+0x78/0x208) from [<c01a0e00>] (__driver_attach+0x8c/0x90) [<c01a0e00>] (__driver_attach+0x8c/0x90) from [<c019f514>] (bus_for_each_dev+0x50/0x7c) [<c019f514>] (bus_for_each_dev+0x50/0x7c) from [<c01a0424>] (bus_add_driver+0x170/0x23c) [<c01a0424>] (bus_add_driver+0x170/0x23c) from [<c01a12b4>] (driver_register+0x78/0x144) [<c01a12b4>] (driver_register+0x78/0x144) from [<c0008598>] (do_one_initcall+0x34/0x174) [<c0008598>] (do_one_initcall+0x34/0x174) from [<c03df8e8>] (kernel_init+0xfc/0x1bc) [<c03df8e8>] (kernel_init+0xfc/0x1bc) from [<c000f1d4>] (kernel_thread_exit+0x0/0x8) ---[ end trace d77aa0db848f0e28 ]--- ab8500-core ab8500-core.0: Failed to request dbf IRQ#0: -22 ab8500-poweron-key: probe of ab8500-poweron-key.0 failed with error -22 So how do we proceed to not release v3.6 with this regression? Shall all of the MFD IRQdomain stuff be pulled into the -rc series? (Linux-next seems to be working, so the real fix is in there) Yours, Linus Walleij
On Thu, Sep 13, 2012 at 11:35:43AM +0200, Linus Walleij wrote: > On Mon, Aug 6, 2012 at 2:32 PM, Lee Jones <lee.jones@linaro.org> wrote: > > > If we're booting with Device Tree enabled, we want the IRQ numbers to > > be taken and translated from the Device Tree binary. If not, they > > should be taken from the resource allocation defined in the AB8500 MFD > > core driver. > > > > Tested-by: Linus Walleij <linus.walleij@linaro.org> > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > Not having this patch in v3.6-rcN gives the following boot noise (and > the key does not work): > > ------------[ cut here ]------------ > WARNING: at /home/elinwal/linux-stericsson/kernel/irq/irqdomain.c:137 > irq_domain_legacy_revmap+0x20/0x48() > Modules linked in: > [<c0014710>] (unwind_backtrace+0x0/0xf8) from [<c001d37c>] > (warn_slowpath_common+0x4c/0x64) > [<c001d37c>] (warn_slowpath_common+0x4c/0x64) from [<c001d3b0>] > (warn_slowpath_null+0x1c/0x24) > [<c001d3b0>] (warn_slowpath_null+0x1c/0x24) from [<c0064200>] > (irq_domain_legacy_revmap+0x20/0x48) > [<c0064200>] (irq_domain_legacy_revmap+0x20/0x48) from [<c02cea28>] > (ab8500_ponkey_probe+0xd0/0x1f8) > [<c02cea28>] (ab8500_ponkey_probe+0xd0/0x1f8) from [<c01a1e20>] > (platform_drv_probe+0x14/0x18) > [<c01a1e20>] (platform_drv_probe+0x14/0x18) from [<c01a0be4>] > (driver_probe_device+0x78/0x208) > [<c01a0be4>] (driver_probe_device+0x78/0x208) from [<c01a0e00>] > (__driver_attach+0x8c/0x90) > [<c01a0e00>] (__driver_attach+0x8c/0x90) from [<c019f514>] > (bus_for_each_dev+0x50/0x7c) > [<c019f514>] (bus_for_each_dev+0x50/0x7c) from [<c01a0424>] > (bus_add_driver+0x170/0x23c) > [<c01a0424>] (bus_add_driver+0x170/0x23c) from [<c01a12b4>] > (driver_register+0x78/0x144) > [<c01a12b4>] (driver_register+0x78/0x144) from [<c0008598>] > (do_one_initcall+0x34/0x174) > [<c0008598>] (do_one_initcall+0x34/0x174) from [<c03df8e8>] > (kernel_init+0xfc/0x1bc) > [<c03df8e8>] (kernel_init+0xfc/0x1bc) from [<c000f1d4>] > (kernel_thread_exit+0x0/0x8) > ---[ end trace d77aa0db848f0e28 ]--- > ab8500-core ab8500-core.0: Failed to request dbf IRQ#0: -22 > ab8500-poweron-key: probe of ab8500-poweron-key.0 failed with error -22 > > So how do we proceed to not release v3.6 with this regression? > > Shall all of the MFD IRQdomain stuff be pulled into the -rc series? > > (Linux-next seems to be working, so the real fix is in there) I haven't tested this, but I think this patch should be pulled out of -next and pushed into the -rcs to fix the issue you see above. The real fix along with the revert to this patch also resides in -next and will be applied during the next merge window.
On Fri, Sep 14, 2012 at 10:03 AM, Lee Jones <lee.jones@linaro.org> wrote: > On Thu, Sep 13, 2012 at 11:35:43AM +0200, Linus Walleij wrote: >> >> Not having this patch in v3.6-rcN gives the following boot noise (and >> the key does not work): >> >> ------------[ cut here ]------------ >> WARNING: at /home/elinwal/linux-stericsson/kernel/irq/irqdomain.c:137 >> irq_domain_legacy_revmap+0x20/0x48() (...) > I haven't tested this, but I think this patch should be pulled out > of -next and pushed into the -rcs to fix the issue you see above. > The real fix along with the revert to this patch also resides in > -next and will be applied during the next merge window. Dmitry, can you fix this? v3.6-rc6 is regressing on us... Yours, Linus Walleij
On Tue, Sep 18, 2012 at 01:22:06PM +0200, Linus Walleij wrote: > On Fri, Sep 14, 2012 at 10:03 AM, Lee Jones <lee.jones@linaro.org> wrote: > > On Thu, Sep 13, 2012 at 11:35:43AM +0200, Linus Walleij wrote: > >> > >> Not having this patch in v3.6-rcN gives the following boot noise (and > >> the key does not work): > >> > >> ------------[ cut here ]------------ > >> WARNING: at /home/elinwal/linux-stericsson/kernel/irq/irqdomain.c:137 > >> irq_domain_legacy_revmap+0x20/0x48() > (...) > > I haven't tested this, but I think this patch should be pulled out > > of -next and pushed into the -rcs to fix the issue you see above. > > The real fix along with the revert to this patch also resides in > > -next and will be applied during the next merge window. > > Dmitry, can you fix this? > > v3.6-rc6 is regressing on us... OK, I'll revert and send a pull request tonight. Thanks.
diff --git a/drivers/input/misc/ab8500-ponkey.c b/drivers/input/misc/ab8500-ponkey.c index 1a1d974..afcd87f 100644 --- a/drivers/input/misc/ab8500-ponkey.c +++ b/drivers/input/misc/ab8500-ponkey.c @@ -47,6 +47,7 @@ static irqreturn_t ab8500_ponkey_handler(int irq, void *data) static int __devinit ab8500_ponkey_probe(struct platform_device *pdev) { struct ab8500 *ab8500 = dev_get_drvdata(pdev->dev.parent); + struct device_node *np = pdev->dev.of_node; struct ab8500_ponkey *ponkey; struct input_dev *input; int irq_dbf, irq_dbr; @@ -73,8 +74,9 @@ static int __devinit ab8500_ponkey_probe(struct platform_device *pdev) ponkey->idev = input; ponkey->ab8500 = ab8500; - ponkey->irq_dbf = ab8500_irq_get_virq(ab8500, irq_dbf); - ponkey->irq_dbr = ab8500_irq_get_virq(ab8500, irq_dbr); + + ponkey->irq_dbf = (np) ? ab8500_irq_get_virq(ab8500, irq_dbf) : irq_dbf; + ponkey->irq_dbr = (np) ? ab8500_irq_get_virq(ab8500, irq_dbr) : irq_dbr; input->name = "AB8500 POn(PowerOn) Key"; input->dev.parent = &pdev->dev;