[PATCH] pinctrl: cherryview: fix issues caused by dynamic gpio irqs mapping

[PATCH] pinctrl: cherryview: fix issues caused by dynamic gpio irqs mapping

[Grygorii Strashko]

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

[PATCH] pinctrl: cherryview: fix issues caused by dynamic gpio irqs mapping

[Grygorii Strashko]

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

Re: [PATCH] pinctrl: cherryview: fix issues caused by dynamic gpio irqs mapping

[Andy Shevchenko]

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
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] pinctrl: cherryview: fix issues caused by dynamic gpio irqs mapping

[Grygorii Strashko]

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

-- 
regards,
-grygorii

Re: [PATCH] pinctrl: cherryview: fix issues caused by dynamic gpio irqs mapping

[Grygorii Strashko]

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

-- 
regards,
-grygorii

Re: [PATCH] pinctrl: cherryview: fix issues caused by dynamic gpio irqs mapping

[Mika Westerberg]

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.

Re: [PATCH] pinctrl: cherryview: fix issues caused by dynamic gpio irqs mapping

[Mika Westerberg]

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?

Re: [PATCH] pinctrl: cherryview: fix issues caused by dynamic gpio irqs mapping

[Chris Gorman]

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
>

Re: [PATCH] pinctrl: cherryview: fix issues caused by dynamic gpio irqs mapping

[Mika Westerberg]

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.

Re: [PATCH] pinctrl: cherryview: fix issues caused by dynamic gpio irqs mapping

[Chris Gorman]

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.

Re: [PATCH] pinctrl: cherryview: fix issues caused by dynamic gpio irqs mapping

[Linus Walleij]

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

Re: [PATCH] pinctrl: cherryview: fix issues caused by dynamic gpio irqs mapping

[Grygorii Strashko]

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.



-- 
regards,
-grygorii

Re: [PATCH] pinctrl: cherryview: fix issues caused by dynamic gpio irqs mapping

[Mika Westerberg]

On Wed, Oct 04, 2017 at 12:07:34PM -0400, Chris Gorman wrote:
> Tested-by: Chris Gorman <chrisjohgorman@gmail.com>

I guess you sent this because of the "Tested-by", right? It is enough if
you reply to the original thread (the one posted by Grygorii yesterday)
and say something like:

  Works for me,
  Tested-by: ...

Then Linus W can add that tag to the patch when he applies it to his
tree.

[PATCH] pinctrl: cherryview: fix issues caused by dynamic gpio irqs mapping

[Chris Gorman]

From: Grygorii Strashko <grygorii.strashko@ti.com>

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] with broken BIOS which
hardcodes Linux IRQ numbers in their ACPI tables instead of GPIO number.

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 any new interrupt
controller will be probed before cherryview-pinctrl.

[1] https://bugzilla.kernel.org/show_bug.cgi?id=194945
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
Tested-by: Chris Gorman <chrisjohgorman@gmail.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