diff mbox series

pinctrl: cherryview: fix issues caused by dynamic gpio irqs mapping

Message ID 20171003170049.24480-1-grygorii.strashko@ti.com
State New
Headers show
Series pinctrl: cherryview: fix issues caused by dynamic gpio irqs mapping | expand

Commit Message

Grygorii Strashko Oct. 3, 2017, 5 p.m. UTC
New GPIO IRQs are allocated and mapped dynamically by default when
GPIO IRQ infrastructure is used by cherryview-pinctrl driver.
This causes issues on some Intel platforms [1][2] with broken BIOS which
hardcodes Linux IRQ numbers in their ACPI tables.

On such platforms cherryview-pinctrl driver should allocate and map all
GPIO IRQs at probe time.
Side effect - "Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n"
can be seen at boot log.

NOTE. It still may fail if boot sequence will changed and some interrupt
controller will be probed before cherryview-pinctrl which will shift Linux IRQ
numbering (expected with CONFIG_SPARCE_IRQ enabled).

[1] https://bugzilla.kernel.org/show_bug.cgi?id=194945
[2] https://lkml.org/lkml/2017/9/28/153
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Chris Gorman <chrisjohgorman@gmail.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> 
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
Reported-by: Chris Gorman <chrisjohgorman@gmail.com>
Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-cherryview.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

Comments

Andy Shevchenko Oct. 3, 2017, 5:54 p.m. UTC | #1
On Tue, Oct 3, 2017 at 8:00 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
> New GPIO IRQs are allocated and mapped dynamically by default when
> GPIO IRQ infrastructure is used by cherryview-pinctrl driver.
> This causes issues on some Intel platforms [1][2] with broken BIOS which
> hardcodes Linux IRQ numbers in their ACPI tables.
>
> On such platforms cherryview-pinctrl driver should allocate and map all
> GPIO IRQs at probe time.
> Side effect - "Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n"
> can be seen at boot log.
>
> NOTE. It still may fail if boot sequence will changed and some interrupt
> controller will be probed before cherryview-pinctrl which will shift Linux IRQ
> numbering (expected with CONFIG_SPARCE_IRQ enabled).
>
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=194945
> [2] https://lkml.org/lkml/2017/9/28/153

Btw, you might want to use one of

Buglink:
Bugzilla:
Link:

tags.

> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Chris Gorman <chrisjohgorman@gmail.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> Reported-by: Chris Gorman <chrisjohgorman@gmail.com>
> Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com>

I'm not sure about this approach. It might be some other broken BIOS
discovered with some other driver (like pinctrl-baytrail.c as an
hypothetical example).

Alternative is to deselect SPARSE_IRQ (though it makes it non-usable
in entire x86 world).

>  drivers/pinctrl/intel/pinctrl-cherryview.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
> index 04e929f..fadbca9 100644
> --- a/drivers/pinctrl/intel/pinctrl-cherryview.c
> +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
> @@ -1577,6 +1577,7 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
>         struct gpio_chip *chip = &pctrl->chip;
>         bool need_valid_mask = !dmi_check_system(chv_no_valid_mask);
>         int ret, i, offset;
> +       int irq_base;
>
>         *chip = chv_gpio_chip;
>
> @@ -1622,7 +1623,18 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
>         /* Clear all interrupts */
>         chv_writel(0xffff, pctrl->regs + CHV_INTSTAT);
>
> -       ret = gpiochip_irqchip_add(chip, &chv_gpio_irqchip, 0,
> +       if (!need_valid_mask) {
> +               irq_base = devm_irq_alloc_descs(pctrl->dev, -1, 0,
> +                                               chip->ngpio, NUMA_NO_NODE);
> +               if (irq_base < 0) {
> +                       dev_err(pctrl->dev, "Failed to allocate IRQ numbers\n");
> +                       return irq_base;
> +               }
> +       } else {
> +               irq_base = 0;
> +       }
> +
> +       ret = gpiochip_irqchip_add(chip, &chv_gpio_irqchip, irq_base,
>                                    handle_bad_irq, IRQ_TYPE_NONE);
>         if (ret) {
>                 dev_err(pctrl->dev, "failed to add IRQ chip\n");
> --
> 2.10.1
>
Grygorii Strashko Oct. 3, 2017, 6:20 p.m. UTC | #2
On 10/03/2017 12:54 PM, Andy Shevchenko wrote:
> On Tue, Oct 3, 2017 at 8:00 PM, Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
>> New GPIO IRQs are allocated and mapped dynamically by default when
>> GPIO IRQ infrastructure is used by cherryview-pinctrl driver.
>> This causes issues on some Intel platforms [1][2] with broken BIOS which
>> hardcodes Linux IRQ numbers in their ACPI tables.
>>
>> On such platforms cherryview-pinctrl driver should allocate and map all
>> GPIO IRQs at probe time.
>> Side effect - "Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n"
>> can be seen at boot log.
>>
>> NOTE. It still may fail if boot sequence will changed and some interrupt
>> controller will be probed before cherryview-pinctrl which will shift Linux IRQ
>> numbering (expected with CONFIG_SPARCE_IRQ enabled).
>>
>> [1] https://bugzilla.kernel.org/show_bug.cgi?id=194945
>> [2] https://lkml.org/lkml/2017/9/28/153
> 
> Btw, you might want to use one of
> 
> Buglink:
> Bugzilla:
> Link:
> 
> tags.
> 
>> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
>> Cc: Chris Gorman <chrisjohgorman@gmail.com>
>> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
>> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> Reported-by: Chris Gorman <chrisjohgorman@gmail.com>
>> Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> I'm not sure about this approach. It might be some other broken BIOS
> discovered with some other driver (like pinctrl-baytrail.c as an
> hypothetical example).

It can be made more safe by fixing gpio chip base irq numbers, but this info
should be passed to drivers somehow. Of course, above note will be still valid
- as W/A, required IRQ ranges can be reserved very early during boot in platform code.

> 
> Alternative is to deselect SPARSE_IRQ (though it makes it non-usable
> in entire x86 world).

:) nuke mix.

> 
>>   drivers/pinctrl/intel/pinctrl-cherryview.c | 14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
>> index 04e929f..fadbca9 100644
>> --- a/drivers/pinctrl/intel/pinctrl-cherryview.c
>> +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
>> @@ -1577,6 +1577,7 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
>>          struct gpio_chip *chip = &pctrl->chip;
>>          bool need_valid_mask = !dmi_check_system(chv_no_valid_mask);
>>          int ret, i, offset;
>> +       int irq_base;
>>
>>          *chip = chv_gpio_chip;
>>
>> @@ -1622,7 +1623,18 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
>>          /* Clear all interrupts */
>>          chv_writel(0xffff, pctrl->regs + CHV_INTSTAT);
>>
>> -       ret = gpiochip_irqchip_add(chip, &chv_gpio_irqchip, 0,
>> +       if (!need_valid_mask) {
>> +               irq_base = devm_irq_alloc_descs(pctrl->dev, -1, 0,
>> +                                               chip->ngpio, NUMA_NO_NODE);
>> +               if (irq_base < 0) {
>> +                       dev_err(pctrl->dev, "Failed to allocate IRQ numbers\n");
>> +                       return irq_base;
>> +               }
>> +       } else {
>> +               irq_base = 0;
>> +       }
>> +
>> +       ret = gpiochip_irqchip_add(chip, &chv_gpio_irqchip, irq_base,
>>                                     handle_bad_irq, IRQ_TYPE_NONE);
>>          if (ret) {
>>                  dev_err(pctrl->dev, "failed to add IRQ chip\n");
>> --
>> 2.10.1
>>
> 
> 
>
Mika Westerberg Oct. 4, 2017, 6:41 a.m. UTC | #3
On Tue, Oct 03, 2017 at 08:54:30PM +0300, Andy Shevchenko wrote:
> I'm not sure about this approach. It might be some other broken BIOS
> discovered with some other driver (like pinctrl-baytrail.c as an
> hypothetical example).

Let's deal that separately if it ever happens.

> Alternative is to deselect SPARSE_IRQ (though it makes it non-usable
> in entire x86 world).

No way.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mika Westerberg Oct. 4, 2017, 6:42 a.m. UTC | #4
On Tue, Oct 03, 2017 at 12:00:49PM -0500, Grygorii Strashko wrote:
> New GPIO IRQs are allocated and mapped dynamically by default when
> GPIO IRQ infrastructure is used by cherryview-pinctrl driver.
> This causes issues on some Intel platforms [1][2] with broken BIOS which
> hardcodes Linux IRQ numbers in their ACPI tables.
> 
> On such platforms cherryview-pinctrl driver should allocate and map all
> GPIO IRQs at probe time.
> Side effect - "Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n"
> can be seen at boot log.
> 
> NOTE. It still may fail if boot sequence will changed and some interrupt
> controller will be probed before cherryview-pinctrl which will shift Linux IRQ
> numbering (expected with CONFIG_SPARCE_IRQ enabled).
> 
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=194945
> [2] https://lkml.org/lkml/2017/9/28/153
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Chris Gorman <chrisjohgorman@gmail.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> 
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> Reported-by: Chris Gorman <chrisjohgorman@gmail.com>
> Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Looks reasonable to me. Thanks for taking care of this!

Chris, can you try if this fixes the issue and provide your Tested-by?
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chris Gorman Oct. 4, 2017, 5:01 p.m. UTC | #5
Tested on 4.14.0-rc3 and this works for me.

Tested by: Chris Gorman <chrisjohgorman@gmail.com>

On Tue, Oct 3, 2017 at 1:00 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
> New GPIO IRQs are allocated and mapped dynamically by default when
> GPIO IRQ infrastructure is used by cherryview-pinctrl driver.
> This causes issues on some Intel platforms [1][2] with broken BIOS which
> hardcodes Linux IRQ numbers in their ACPI tables.
>
> On such platforms cherryview-pinctrl driver should allocate and map all
> GPIO IRQs at probe time.
> Side effect - "Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n"
> can be seen at boot log.
>
> NOTE. It still may fail if boot sequence will changed and some interrupt
> controller will be probed before cherryview-pinctrl which will shift Linux IRQ
> numbering (expected with CONFIG_SPARCE_IRQ enabled).
>
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=194945
> [2] https://lkml.org/lkml/2017/9/28/153
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Chris Gorman <chrisjohgorman@gmail.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> Reported-by: Chris Gorman <chrisjohgorman@gmail.com>
> Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-cherryview.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
> index 04e929f..fadbca9 100644
> --- a/drivers/pinctrl/intel/pinctrl-cherryview.c
> +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
> @@ -1577,6 +1577,7 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
>         struct gpio_chip *chip = &pctrl->chip;
>         bool need_valid_mask = !dmi_check_system(chv_no_valid_mask);
>         int ret, i, offset;
> +       int irq_base;
>
>         *chip = chv_gpio_chip;
>
> @@ -1622,7 +1623,18 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
>         /* Clear all interrupts */
>         chv_writel(0xffff, pctrl->regs + CHV_INTSTAT);
>
> -       ret = gpiochip_irqchip_add(chip, &chv_gpio_irqchip, 0,
> +       if (!need_valid_mask) {
> +               irq_base = devm_irq_alloc_descs(pctrl->dev, -1, 0,
> +                                               chip->ngpio, NUMA_NO_NODE);
> +               if (irq_base < 0) {
> +                       dev_err(pctrl->dev, "Failed to allocate IRQ numbers\n");
> +                       return irq_base;
> +               }
> +       } else {
> +               irq_base = 0;
> +       }
> +
> +       ret = gpiochip_irqchip_add(chip, &chv_gpio_irqchip, irq_base,
>                                    handle_bad_irq, IRQ_TYPE_NONE);
>         if (ret) {
>                 dev_err(pctrl->dev, "failed to add IRQ chip\n");
> --
> 2.10.1
>
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mika Westerberg Oct. 6, 2017, 8:19 a.m. UTC | #6
On Wed, Oct 04, 2017 at 09:42:49AM +0300, Mika Westerberg wrote:
> On Tue, Oct 03, 2017 at 12:00:49PM -0500, Grygorii Strashko wrote:
> > New GPIO IRQs are allocated and mapped dynamically by default when
> > GPIO IRQ infrastructure is used by cherryview-pinctrl driver.
> > This causes issues on some Intel platforms [1][2] with broken BIOS which
> > hardcodes Linux IRQ numbers in their ACPI tables.
> > 
> > On such platforms cherryview-pinctrl driver should allocate and map all
> > GPIO IRQs at probe time.
> > Side effect - "Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n"
> > can be seen at boot log.
> > 
> > NOTE. It still may fail if boot sequence will changed and some interrupt
> > controller will be probed before cherryview-pinctrl which will shift Linux IRQ
> > numbering (expected with CONFIG_SPARCE_IRQ enabled).
> > 
> > [1] https://bugzilla.kernel.org/show_bug.cgi?id=194945
> > [2] https://lkml.org/lkml/2017/9/28/153
> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Cc: Chris Gorman <chrisjohgorman@gmail.com>
> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> 
> > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> > Reported-by: Chris Gorman <chrisjohgorman@gmail.com>
> > Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> Looks reasonable to me. Thanks for taking care of this!
> 
> Chris, can you try if this fixes the issue and provide your Tested-by?

Linus,

Chris gave his tested-by in another thread:

  https://patchwork.kernel.org/patch/9985087/

Chris, please let me know if that was not your intention.

I'm fine with the patch as well,

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

I guess this requires stable tag because if I understand comments in
that bug right, it affects the whole v4.13.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chris Gorman Oct. 6, 2017, 1:02 p.m. UTC | #7
Hello all,

Apologies if you are receiving this acknowledgement a second time.  I
have tested this patch on an Intel_Strago chromebook (Acer cb3-532)
and it works to enable the keyboard.  I tested it on 4.14.0-rc3.  Let
me know if you want me to test the patch on the stable branch.

Tested-by: Chris Gorman <chrisjohgorman@gmail.com>

On Fri, Oct 6, 2017 at 4:19 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Wed, Oct 04, 2017 at 09:42:49AM +0300, Mika Westerberg wrote:
>> On Tue, Oct 03, 2017 at 12:00:49PM -0500, Grygorii Strashko wrote:
>> > New GPIO IRQs are allocated and mapped dynamically by default when
>> > GPIO IRQ infrastructure is used by cherryview-pinctrl driver.
>> > This causes issues on some Intel platforms [1][2] with broken BIOS which
>> > hardcodes Linux IRQ numbers in their ACPI tables.
>> >
>> > On such platforms cherryview-pinctrl driver should allocate and map all
>> > GPIO IRQs at probe time.
>> > Side effect - "Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n"
>> > can be seen at boot log.
>> >
>> > NOTE. It still may fail if boot sequence will changed and some interrupt
>> > controller will be probed before cherryview-pinctrl which will shift Linux IRQ
>> > numbering (expected with CONFIG_SPARCE_IRQ enabled).
>> >
>> > [1] https://bugzilla.kernel.org/show_bug.cgi?id=194945
>> > [2] https://lkml.org/lkml/2017/9/28/153
>> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
>> > Cc: Chris Gorman <chrisjohgorman@gmail.com>
>> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
>> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>> > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> > Reported-by: Chris Gorman <chrisjohgorman@gmail.com>
>> > Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>>
>> Looks reasonable to me. Thanks for taking care of this!
>>
>> Chris, can you try if this fixes the issue and provide your Tested-by?
>
> Linus,
>
> Chris gave his tested-by in another thread:
>
>   https://patchwork.kernel.org/patch/9985087/
>
> Chris, please let me know if that was not your intention.
>
> I'm fine with the patch as well,
>
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>
> I guess this requires stable tag because if I understand comments in
> that bug right, it affects the whole v4.13.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Oct. 8, 2017, 12:42 a.m. UTC | #8
On Tue, Oct 3, 2017 at 7:00 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> New GPIO IRQs are allocated and mapped dynamically by default when
> GPIO IRQ infrastructure is used by cherryview-pinctrl driver.
> This causes issues on some Intel platforms [1][2] with broken BIOS which
> hardcodes Linux IRQ numbers in their ACPI tables.
>
> On such platforms cherryview-pinctrl driver should allocate and map all
> GPIO IRQs at probe time.
> Side effect - "Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n"
> can be seen at boot log.
>
> NOTE. It still may fail if boot sequence will changed and some interrupt
> controller will be probed before cherryview-pinctrl which will shift Linux IRQ
> numbering (expected with CONFIG_SPARCE_IRQ enabled).
>
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=194945
> [2] https://lkml.org/lkml/2017/9/28/153
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Chris Gorman <chrisjohgorman@gmail.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> Reported-by: Chris Gorman <chrisjohgorman@gmail.com>
> Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com>

OK patch applied for fixes.

But I'mm still very sceptical about this.

Look at the following (just from grep irq_base drivers/gpio/):

drivers/gpio/gpio-ml-ioh.c:

static int ioh_gpio_to_irq(struct gpio_chip *gpio, unsigned offset)
{
        struct ioh_gpio *chip = gpiochip_get_data(gpio);
        return chip->irq_base + offset;
}

(...)
        ch = irq - chip->irq_base;
        if (irq <= chip->irq_base + 7) {
                im_reg = &chip->reg->regs[chip->ch].im_0;
                im_pos = ch;
(...)

drivers/gpio/gpio-sta2x11.c:

static int gsta_gpio_to_irq(struct gpio_chip *gpio, unsigned offset)
{
        struct gsta_gpio *chip = gpiochip_get_data(gpio);
        return chip->irq_base + offset;
}

(etc)

The thing is that the lines you deleted from gpiolib were the only
thing ever assigning chip->irq_base. This patch, if performed
properly should have removed the .irq_base field from struct
gpio_chip altogether without regressions.

As it is not, anything using .irq_base is regressing.

Either you need to convince me you can quickfix all of these
users, or we need to simply revert this change.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Grygorii Strashko Oct. 9, 2017, 6:04 p.m. UTC | #9
On 10/07/2017 07:42 PM, Linus Walleij wrote:
> On Tue, Oct 3, 2017 at 7:00 PM, Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
> 
>> New GPIO IRQs are allocated and mapped dynamically by default when
>> GPIO IRQ infrastructure is used by cherryview-pinctrl driver.
>> This causes issues on some Intel platforms [1][2] with broken BIOS which
>> hardcodes Linux IRQ numbers in their ACPI tables.
>>
>> On such platforms cherryview-pinctrl driver should allocate and map all
>> GPIO IRQs at probe time.
>> Side effect - "Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n"
>> can be seen at boot log.
>>
>> NOTE. It still may fail if boot sequence will changed and some interrupt
>> controller will be probed before cherryview-pinctrl which will shift Linux IRQ
>> numbering (expected with CONFIG_SPARCE_IRQ enabled).
>>
>> [1] https://bugzilla.kernel.org/show_bug.cgi?id=194945
>> [2] https://lkml.org/lkml/2017/9/28/153
>> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
>> Cc: Chris Gorman <chrisjohgorman@gmail.com>
>> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
>> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> Reported-by: Chris Gorman <chrisjohgorman@gmail.com>
>> Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> OK patch applied for fixes.
> 
> But I'mm still very sceptical about this.
> 
> Look at the following (just from grep irq_base drivers/gpio/):
> 
> drivers/gpio/gpio-ml-ioh.c:
> 
> static int ioh_gpio_to_irq(struct gpio_chip *gpio, unsigned offset)
> {
>          struct ioh_gpio *chip = gpiochip_get_data(gpio);

It is ioh_gpio - not  gpio_chip ;)

>          return chip->irq_base + offset;
> }
> 
> (...)
>          ch = irq - chip->irq_base;
>          if (irq <= chip->irq_base + 7) {
>                  im_reg = &chip->reg->regs[chip->ch].im_0;
>                  im_pos = ch;
> (...)

not an issue gpio-ml-ioh.c:
ioh_gpio_probe()
 		irq_base = devm_irq_alloc_descs(&pdev->dev, -1, IOH_IRQ_BASE,
						num_ports[j], NUMA_NO_NODE);
...
		chip->irq_base = irq_base;
 


> 
> drivers/gpio/gpio-sta2x11.c:
> 
> static int gsta_gpio_to_irq(struct gpio_chip *gpio, unsigned offset)
> {
>          struct gsta_gpio *chip = gpiochip_get_data(gpio);
>          return chip->irq_base + offset;

same. It is gsta_gpio - not  gpio_chip ;)

> }
> 
> (etc)
> 
> The thing is that the lines you deleted from gpiolib were the only
> thing ever assigning chip->irq_base. This patch, if performed
> properly should have removed the .irq_base field from struct
> gpio_chip altogether without regressions.
> 
> As it is not, anything using .irq_base is regressing.
> 
> Either you need to convince me you can quickfix all of these
> users, or we need to simply revert this change.
diff mbox series

Patch

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index 04e929f..fadbca9 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -1577,6 +1577,7 @@  static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
 	struct gpio_chip *chip = &pctrl->chip;
 	bool need_valid_mask = !dmi_check_system(chv_no_valid_mask);
 	int ret, i, offset;
+	int irq_base;
 
 	*chip = chv_gpio_chip;
 
@@ -1622,7 +1623,18 @@  static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
 	/* Clear all interrupts */
 	chv_writel(0xffff, pctrl->regs + CHV_INTSTAT);
 
-	ret = gpiochip_irqchip_add(chip, &chv_gpio_irqchip, 0,
+	if (!need_valid_mask) {
+		irq_base = devm_irq_alloc_descs(pctrl->dev, -1, 0,
+						chip->ngpio, NUMA_NO_NODE);
+		if (irq_base < 0) {
+			dev_err(pctrl->dev, "Failed to allocate IRQ numbers\n");
+			return irq_base;
+		}
+	} else {
+		irq_base = 0;
+	}
+
+	ret = gpiochip_irqchip_add(chip, &chv_gpio_irqchip, irq_base,
 				   handle_bad_irq, IRQ_TYPE_NONE);
 	if (ret) {
 		dev_err(pctrl->dev, "failed to add IRQ chip\n");