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@xxxxxx> 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@xxxxxxxxx>
>> Cc: Chris Gorman <chrisjohgorman@xxxxxxxxx>
>> Cc: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
>> Cc: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@xxxxxx>
>> Reported-by: Chris Gorman <chrisjohgorman@xxxxxxxxx>
>> Reported-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> 
> 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