[PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again

[PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again

[Mika Westerberg]

After commit 47c950d10202 ("pinctrl: cherryview: Do not add all
southwest and north GPIOs to IRQ domain") the driver does not add all
GPIOs to the irqdomain. The reason for that is that those GPIOs cannot
generate IRQs at all, only GPEs (General Purpose Events). This causes
Linux virtual IRQ numbering to change.

However, it seems some CYAN Chromebooks, including Acer Chromebook
hardcodes these Linux IRQ numbers in the ACPI tables of the machine.
Since the numbering is different now, the IRQ meant for keyboard does
not match the Linux virtual IRQ number anymore making the keyboard
non-functional.

Work this around by adding special quirk just for these machines where
we add back all GPIOs to the irqdomain. Rest of the Cherryview/Braswell
based machines will not be affected by the change.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=194945
Fixes: 47c950d10202 ("pinctrl: cherryview: Do not add all southwest and north GPIOs to IRQ domain")
Reported-by: Adam S Levy <theadamlevy@gmail.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-cherryview.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index f80134e3e0b6..bbe9cdf36ca4 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -13,6 +13,7 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/dmi.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/init.h>
@@ -1524,10 +1525,30 @@ static void chv_gpio_irq_handler(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
+/*
+ * Certain machines seem to hardcode Linux IRQ numbers in their ACPI
+ * tables. Since we leave GPIOs that are not capable of generating
+ * interrupts out of the irqdomain the numbering will be different and
+ * cause devices using the hardcoded IRQ numbers fail. In order not to
+ * break such machines we will only mask pins from irqdomain if the machine
+ * is not listed below.
+ */
+static const struct dmi_system_id chv_no_valid_mask[] = {
+	{
+		/* See https://bugzilla.kernel.org/show_bug.cgi?id=194945 */
+		.ident = "Acer Chromebook (CYAN)",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "GOOGLE"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Edgar"),
+		},
+	}
+};
+
 static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
 {
 	const struct chv_gpio_pinrange *range;
 	struct gpio_chip *chip = &pctrl->chip;
+	bool need_valid_mask = !dmi_check_system(chv_no_valid_mask);
 	int ret, i, offset;
 
 	*chip = chv_gpio_chip;
@@ -1536,7 +1557,7 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
 	chip->label = dev_name(pctrl->dev);
 	chip->parent = pctrl->dev;
 	chip->base = -1;
-	chip->irq_need_valid_mask = true;
+	chip->irq_need_valid_mask = need_valid_mask;
 
 	ret = devm_gpiochip_add_data(pctrl->dev, chip, pctrl);
 	if (ret) {
@@ -1567,7 +1588,7 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
 		intsel &= CHV_PADCTRL0_INTSEL_MASK;
 		intsel >>= CHV_PADCTRL0_INTSEL_SHIFT;
 
-		if (intsel >= pctrl->community->nirqs)
+		if (need_valid_mask && intsel >= pctrl->community->nirqs)
 			clear_bit(i, chip->irq_valid_mask);
 	}
 
-- 
2.11.0

Re: [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again

[Linus Walleij]

On Wed, Mar 29, 2017 at 11:59 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> However, it seems some CYAN Chromebooks, including Acer Chromebook
> hardcodes these Linux IRQ numbers in the ACPI tables of the machine.
> Since the numbering is different now, the IRQ meant for keyboard does
> not match the Linux virtual IRQ number anymore making the keyboard
> non-functional.

Well ain't that great. They made the totally unstable Linux IRQ number
space into an ABI.

I wonder what the irqchip people think about that. I think Grant warned us
that this could happen.

> Work this around by adding special quirk just for these machines where
> we add back all GPIOs to the irqdomain. Rest of the Cherryview/Braswell
> based machines will not be affected by the change.

Quirking seems right. But:

> +/*
> + * Certain machines seem to hardcode Linux IRQ numbers in their ACPI
> + * tables. Since we leave GPIOs that are not capable of generating
> + * interrupts out of the irqdomain the numbering will be different and
> + * cause devices using the hardcoded IRQ numbers fail. In order not to
> + * break such machines we will only mask pins from irqdomain if the machine
> + * is not listed below.
> + */
> +static const struct dmi_system_id chv_no_valid_mask[] = {
> +       {
> +               /* See https://bugzilla.kernel.org/show_bug.cgi?id=194945 */
> +               .ident = "Acer Chromebook (CYAN)",
> +               .matches = {
> +                       DMI_MATCH(DMI_SYS_VENDOR, "GOOGLE"),
> +                       DMI_MATCH(DMI_PRODUCT_NAME, "Edgar"),
> +               },
> +       }
> +};

We match but...

>         const struct chv_gpio_pinrange *range;
>         struct gpio_chip *chip = &pctrl->chip;
> +       bool need_valid_mask = !dmi_check_system(chv_no_valid_mask);
>         int ret, i, offset;
>
>         *chip = chv_gpio_chip;
> @@ -1536,7 +1557,7 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
>         chip->label = dev_name(pctrl->dev);
>         chip->parent = pctrl->dev;
>         chip->base = -1;
> -       chip->irq_need_valid_mask = true;
> +       chip->irq_need_valid_mask = need_valid_mask;

Isn't the right solution to translate this back to the offset from the "Linux
IRQ" and use that offset? This quirk seems pretty violent.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again

[Mika Westerberg]

On Wed, Mar 29, 2017 at 02:58:19PM +0200, Linus Walleij wrote:
> On Wed, Mar 29, 2017 at 11:59 AM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> 
> > However, it seems some CYAN Chromebooks, including Acer Chromebook
> > hardcodes these Linux IRQ numbers in the ACPI tables of the machine.
> > Since the numbering is different now, the IRQ meant for keyboard does
> > not match the Linux virtual IRQ number anymore making the keyboard
> > non-functional.
> 
> Well ain't that great. They made the totally unstable Linux IRQ number
> space into an ABI.
> 
> I wonder what the irqchip people think about that. I think Grant warned us
> that this could happen.
> 
> > Work this around by adding special quirk just for these machines where
> > we add back all GPIOs to the irqdomain. Rest of the Cherryview/Braswell
> > based machines will not be affected by the change.
> 
> Quirking seems right. But:
> 
> > +/*
> > + * Certain machines seem to hardcode Linux IRQ numbers in their ACPI
> > + * tables. Since we leave GPIOs that are not capable of generating
> > + * interrupts out of the irqdomain the numbering will be different and
> > + * cause devices using the hardcoded IRQ numbers fail. In order not to
> > + * break such machines we will only mask pins from irqdomain if the machine
> > + * is not listed below.
> > + */
> > +static const struct dmi_system_id chv_no_valid_mask[] = {
> > +       {
> > +               /* See https://bugzilla.kernel.org/show_bug.cgi?id=194945 */
> > +               .ident = "Acer Chromebook (CYAN)",
> > +               .matches = {
> > +                       DMI_MATCH(DMI_SYS_VENDOR, "GOOGLE"),
> > +                       DMI_MATCH(DMI_PRODUCT_NAME, "Edgar"),
> > +               },
> > +       }
> > +};
> 
> We match but...
> 
> >         const struct chv_gpio_pinrange *range;
> >         struct gpio_chip *chip = &pctrl->chip;
> > +       bool need_valid_mask = !dmi_check_system(chv_no_valid_mask);
> >         int ret, i, offset;
> >
> >         *chip = chv_gpio_chip;
> > @@ -1536,7 +1557,7 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
> >         chip->label = dev_name(pctrl->dev);
> >         chip->parent = pctrl->dev;
> >         chip->base = -1;
> > -       chip->irq_need_valid_mask = true;
> > +       chip->irq_need_valid_mask = need_valid_mask;
> 
> Isn't the right solution to translate this back to the offset from the "Linux
> IRQ" and use that offset? This quirk seems pretty violent.

So based on DMI strings instead of doing this, just correct the offset
and be done with it? Works for me.

Re: [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again

[Linus Walleij]

On Wed, Mar 29, 2017 at 3:19 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Wed, Mar 29, 2017 at 02:58:19PM +0200, Linus Walleij wrote:
>> On Wed, Mar 29, 2017 at 11:59 AM, Mika Westerberg
>> > +       chip->irq_need_valid_mask = need_valid_mask;
>>
>> Isn't the right solution to translate this back to the offset from the "Linux
>> IRQ" and use that offset? This quirk seems pretty violent.
>
> So based on DMI strings instead of doing this, just correct the offset
> and be done with it? Works for me.

Yeah I imagine? We just fix up the Linux magic numbers to the hardware
magic numbers.

This is what a poor soul trying to port this chromebook to use BSD or
Windows will have to do anyway, so I don't see why we should be
different.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again

[Marc Zyngier]

On 29/03/17 13:58, Linus Walleij wrote:
> On Wed, Mar 29, 2017 at 11:59 AM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> 
>> However, it seems some CYAN Chromebooks, including Acer Chromebook
>> hardcodes these Linux IRQ numbers in the ACPI tables of the machine.
>> Since the numbering is different now, the IRQ meant for keyboard does
>> not match the Linux virtual IRQ number anymore making the keyboard
>> non-functional.
> 
> Well ain't that great. They made the totally unstable Linux IRQ number
> space into an ABI.

I'm puzzled as to how this could have ever worked. Is that because we
used to have irq == hwirq, and that assertion doesn't hold true anymore?

> 
> I wonder what the irqchip people think about that. I think Grant warned us
> that this could happen.
> 
>> Work this around by adding special quirk just for these machines where
>> we add back all GPIOs to the irqdomain. Rest of the Cherryview/Braswell
>> based machines will not be affected by the change.
> 
> Quirking seems right. But:
> 
>> +/*
>> + * Certain machines seem to hardcode Linux IRQ numbers in their ACPI
>> + * tables. Since we leave GPIOs that are not capable of generating
>> + * interrupts out of the irqdomain the numbering will be different and
>> + * cause devices using the hardcoded IRQ numbers fail. In order not to
>> + * break such machines we will only mask pins from irqdomain if the machine
>> + * is not listed below.
>> + */
>> +static const struct dmi_system_id chv_no_valid_mask[] = {
>> +       {
>> +               /* See https://bugzilla.kernel.org/show_bug.cgi?id=194945 */
>> +               .ident = "Acer Chromebook (CYAN)",
>> +               .matches = {
>> +                       DMI_MATCH(DMI_SYS_VENDOR, "GOOGLE"),
>> +                       DMI_MATCH(DMI_PRODUCT_NAME, "Edgar"),
>> +               },
>> +       }
>> +};
> 
> We match but...
> 
>>         const struct chv_gpio_pinrange *range;
>>         struct gpio_chip *chip = &pctrl->chip;
>> +       bool need_valid_mask = !dmi_check_system(chv_no_valid_mask);
>>         int ret, i, offset;
>>
>>         *chip = chv_gpio_chip;
>> @@ -1536,7 +1557,7 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
>>         chip->label = dev_name(pctrl->dev);
>>         chip->parent = pctrl->dev;
>>         chip->base = -1;
>> -       chip->irq_need_valid_mask = true;
>> +       chip->irq_need_valid_mask = need_valid_mask;
> 
> Isn't the right solution to translate this back to the offset from the "Linux
> IRQ" and use that offset? This quirk seems pretty violent.

I'm not sure I understand the quirk here, but my personal approach would
be to provide an inverse mapping oldIRQ->hwirq, and use the usual
irqdomain lookup to get back to the real thing.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again

[Mika Westerberg]

On Wed, Mar 29, 2017 at 05:43:49PM +0100, Marc Zyngier wrote:
> On 29/03/17 13:58, Linus Walleij wrote:
> > On Wed, Mar 29, 2017 at 11:59 AM, Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > 
> >> However, it seems some CYAN Chromebooks, including Acer Chromebook
> >> hardcodes these Linux IRQ numbers in the ACPI tables of the machine.
> >> Since the numbering is different now, the IRQ meant for keyboard does
> >> not match the Linux virtual IRQ number anymore making the keyboard
> >> non-functional.
> > 
> > Well ain't that great. They made the totally unstable Linux IRQ number
> > space into an ABI.
> 
> I'm puzzled as to how this could have ever worked. Is that because we
> used to have irq == hwirq, and that assertion doesn't hold true anymore?

For legacy and I/O-APIC IRQs this still is true (on x86). However,
anything else which can be loaded as a module, like the GPIO driver here
gets the next free slot from the global virtual IRQ number space. If
someone happens to load another GPIO driver for example for a GPIO
expander before the SoC GPIO driver is loaded the numbering will be
different than without the driver.

At least that's my understanding.

> > I wonder what the irqchip people think about that. I think Grant warned us
> > that this could happen.
> > 
> >> Work this around by adding special quirk just for these machines where
> >> we add back all GPIOs to the irqdomain. Rest of the Cherryview/Braswell
> >> based machines will not be affected by the change.
> > 
> > Quirking seems right. But:
> > 
> >> +/*
> >> + * Certain machines seem to hardcode Linux IRQ numbers in their ACPI
> >> + * tables. Since we leave GPIOs that are not capable of generating
> >> + * interrupts out of the irqdomain the numbering will be different and
> >> + * cause devices using the hardcoded IRQ numbers fail. In order not to
> >> + * break such machines we will only mask pins from irqdomain if the machine
> >> + * is not listed below.
> >> + */
> >> +static const struct dmi_system_id chv_no_valid_mask[] = {
> >> +       {
> >> +               /* See https://bugzilla.kernel.org/show_bug.cgi?id=194945 */
> >> +               .ident = "Acer Chromebook (CYAN)",
> >> +               .matches = {
> >> +                       DMI_MATCH(DMI_SYS_VENDOR, "GOOGLE"),
> >> +                       DMI_MATCH(DMI_PRODUCT_NAME, "Edgar"),
> >> +               },
> >> +       }
> >> +};
> > 
> > We match but...
> > 
> >>         const struct chv_gpio_pinrange *range;
> >>         struct gpio_chip *chip = &pctrl->chip;
> >> +       bool need_valid_mask = !dmi_check_system(chv_no_valid_mask);
> >>         int ret, i, offset;
> >>
> >>         *chip = chv_gpio_chip;
> >> @@ -1536,7 +1557,7 @@ static int chv_gpio_probe(struct chv_pinctrl *pctrl, int irq)
> >>         chip->label = dev_name(pctrl->dev);
> >>         chip->parent = pctrl->dev;
> >>         chip->base = -1;
> >> -       chip->irq_need_valid_mask = true;
> >> +       chip->irq_need_valid_mask = need_valid_mask;
> > 
> > Isn't the right solution to translate this back to the offset from the "Linux
> > IRQ" and use that offset? This quirk seems pretty violent.
> 
> I'm not sure I understand the quirk here, but my personal approach would
> be to provide an inverse mapping oldIRQ->hwirq, and use the usual
> irqdomain lookup to get back to the real thing.

OK, thanks for tip. Let me try both approaches and see which one works
better :)

Re: [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again

[Mika Westerberg]

On Thu, Mar 30, 2017 at 12:00:47PM +0300, Mika Westerberg wrote:
> > > Isn't the right solution to translate this back to the offset from the "Linux
> > > IRQ" and use that offset? This quirk seems pretty violent.
> > 
> > I'm not sure I understand the quirk here, but my personal approach would
> > be to provide an inverse mapping oldIRQ->hwirq, and use the usual
> > irqdomain lookup to get back to the real thing.
> 
> OK, thanks for tip. Let me try both approaches and see which one works
> better :)

I've now looked at the issue again and I'm actually not sure if we can
use either of the proposals here :/

Essentially all the Linux IRQ<->hwirq mappings are created when the
gpiochip is initialized in gpiochip_irqchip_add(). Because of that, I
don't see how we can change those afterwards when the i8042 keyboard
driver asks for IRQ 182. Maybe I'm missing something obvious but to me
the only way to go forward is to do what this patch does and add all the
GPIOs to the irqdomain (if we don't want to add a custom IRQ domain for
cherryview).

Re: [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again

[Marc Zyngier]

On 06/04/17 10:59, Mika Westerberg wrote:
> On Thu, Mar 30, 2017 at 12:00:47PM +0300, Mika Westerberg wrote:
>>>> Isn't the right solution to translate this back to the offset from the "Linux
>>>> IRQ" and use that offset? This quirk seems pretty violent.
>>>
>>> I'm not sure I understand the quirk here, but my personal approach would
>>> be to provide an inverse mapping oldIRQ->hwirq, and use the usual
>>> irqdomain lookup to get back to the real thing.
>>
>> OK, thanks for tip. Let me try both approaches and see which one works
>> better :)
> 
> I've now looked at the issue again and I'm actually not sure if we can
> use either of the proposals here :/
> 
> Essentially all the Linux IRQ<->hwirq mappings are created when the
> gpiochip is initialized in gpiochip_irqchip_add(). Because of that, I
> don't see how we can change those afterwards when the i8042 keyboard
> driver asks for IRQ 182. Maybe I'm missing something obvious but to me
> the only way to go forward is to do what this patch does and add all the
> GPIOs to the irqdomain (if we don't want to add a custom IRQ domain for
> cherryview).

Something is very fishy here: You say that the i8042 asks for IRQ 182.
But surely that's a hwirq. Or is it directly poking at the ACPI table,
extracting a number and do a request_irq() on it? If that's the case,
this is a bug (or at least a severe limitation) in the i8042 driver.

If the latter, even adding all the GPIOs to the domain do not guarantee
that the allocated interrupts will always be in the range you expect
(depending on the order of things being initialized).

It all feels extremely fragile to me...

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again

[Mika Westerberg]

On Thu, Apr 06, 2017 at 11:32:59AM +0100, Marc Zyngier wrote:
> On 06/04/17 10:59, Mika Westerberg wrote:
> > On Thu, Mar 30, 2017 at 12:00:47PM +0300, Mika Westerberg wrote:
> >>>> Isn't the right solution to translate this back to the offset from the "Linux
> >>>> IRQ" and use that offset? This quirk seems pretty violent.
> >>>
> >>> I'm not sure I understand the quirk here, but my personal approach would
> >>> be to provide an inverse mapping oldIRQ->hwirq, and use the usual
> >>> irqdomain lookup to get back to the real thing.
> >>
> >> OK, thanks for tip. Let me try both approaches and see which one works
> >> better :)
> > 
> > I've now looked at the issue again and I'm actually not sure if we can
> > use either of the proposals here :/
> > 
> > Essentially all the Linux IRQ<->hwirq mappings are created when the
> > gpiochip is initialized in gpiochip_irqchip_add(). Because of that, I
> > don't see how we can change those afterwards when the i8042 keyboard
> > driver asks for IRQ 182. Maybe I'm missing something obvious but to me
> > the only way to go forward is to do what this patch does and add all the
> > GPIOs to the irqdomain (if we don't want to add a custom IRQ domain for
> > cherryview).
> 
> Something is very fishy here: You say that the i8042 asks for IRQ 182.
> But surely that's a hwirq. Or is it directly poking at the ACPI table,
> extracting a number and do a request_irq() on it? If that's the case,
> this is a bug (or at least a severe limitation) in the i8042 driver.

Yes, it gets the number 182 from ACPI tables but that's not hwirq but
instead it is the Linux IRQ number the cherryview-pinctrl driver has
assigned for the GPIO in question (as far as I can tell).

This is already reported as a bug in coreboot for the particular machine
but we need to work it around in Linux anyway, even though it may break
again if Linux IRQ numbering changes.

> If the latter, even adding all the GPIOs to the domain do not guarantee
> that the allocated interrupts will always be in the range you expect
> (depending on the order of things being initialized).
> 
> It all feels extremely fragile to me...

Yes, it is unfortunately.

There is a proper way to use GPIOs as interrupts in ACPI, namely GpioInt
resource but it has not been used in that particular machine for some
reason.

Re: [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again

[Marc Zyngier]

On 06/04/17 11:50, Mika Westerberg wrote:
> On Thu, Apr 06, 2017 at 11:32:59AM +0100, Marc Zyngier wrote:
>> On 06/04/17 10:59, Mika Westerberg wrote:
>>> On Thu, Mar 30, 2017 at 12:00:47PM +0300, Mika Westerberg wrote:
>>>>>> Isn't the right solution to translate this back to the offset from the "Linux
>>>>>> IRQ" and use that offset? This quirk seems pretty violent.
>>>>>
>>>>> I'm not sure I understand the quirk here, but my personal approach would
>>>>> be to provide an inverse mapping oldIRQ->hwirq, and use the usual
>>>>> irqdomain lookup to get back to the real thing.
>>>>
>>>> OK, thanks for tip. Let me try both approaches and see which one works
>>>> better :)
>>>
>>> I've now looked at the issue again and I'm actually not sure if we can
>>> use either of the proposals here :/
>>>
>>> Essentially all the Linux IRQ<->hwirq mappings are created when the
>>> gpiochip is initialized in gpiochip_irqchip_add(). Because of that, I
>>> don't see how we can change those afterwards when the i8042 keyboard
>>> driver asks for IRQ 182. Maybe I'm missing something obvious but to me
>>> the only way to go forward is to do what this patch does and add all the
>>> GPIOs to the irqdomain (if we don't want to add a custom IRQ domain for
>>> cherryview).
>>
>> Something is very fishy here: You say that the i8042 asks for IRQ 182.
>> But surely that's a hwirq. Or is it directly poking at the ACPI table,
>> extracting a number and do a request_irq() on it? If that's the case,
>> this is a bug (or at least a severe limitation) in the i8042 driver.
> 
> Yes, it gets the number 182 from ACPI tables but that's not hwirq but
> instead it is the Linux IRQ number the cherryview-pinctrl driver has
> assigned for the GPIO in question (as far as I can tell).

But whatever number gets put there is by definition wrong. There is no
"right" number, except for something that identifies the HW connection.

> 
> This is already reported as a bug in coreboot for the particular machine
> but we need to work it around in Linux anyway, even though it may break
> again if Linux IRQ numbering changes.
> 
>> If the latter, even adding all the GPIOs to the domain do not guarantee
>> that the allocated interrupts will always be in the range you expect
>> (depending on the order of things being initialized).
>>
>> It all feels extremely fragile to me...
> 
> Yes, it is unfortunately.
> 
> There is a proper way to use GPIOs as interrupts in ACPI, namely GpioInt
> resource but it has not been used in that particular machine for some
> reason.

OK, that explains it. One thing you could try would be to allocate the
domain as a legacy one, which allows you to specify the virtual
interrupt range you require. Not sure that's any better though.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again

[Mika Westerberg]

On Thu, Apr 06, 2017 at 12:05:28PM +0100, Marc Zyngier wrote:
> > There is a proper way to use GPIOs as interrupts in ACPI, namely GpioInt
> > resource but it has not been used in that particular machine for some
> > reason.
> 
> OK, that explains it. One thing you could try would be to allocate the
> domain as a legacy one, which allows you to specify the virtual
> interrupt range you require. Not sure that's any better though.

Yeah, then I would have to add a whole bunch of code to handle custom
irqdomain just to support this one machine (fingers crossed that it is
limited to only this particular machine).

If no objections, I would rather stick to the approach used in this
patch. I will just update the DMI strings to include BIOS version/date.

Re: [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again

[Marc Zyngier]

On 06/04/17 14:18, Mika Westerberg wrote:
> On Thu, Apr 06, 2017 at 12:05:28PM +0100, Marc Zyngier wrote:
>>> There is a proper way to use GPIOs as interrupts in ACPI, namely GpioInt
>>> resource but it has not been used in that particular machine for some
>>> reason.
>>
>> OK, that explains it. One thing you could try would be to allocate the
>> domain as a legacy one, which allows you to specify the virtual
>> interrupt range you require. Not sure that's any better though.
> 
> Yeah, then I would have to add a whole bunch of code to handle custom
> irqdomain just to support this one machine (fingers crossed that it is
> limited to only this particular machine).
> 
> If no objections, I would rather stick to the approach used in this
> patch. I will just update the DMI strings to include BIOS version/date.

Fair enough. Though I have the ugly feeling that we're going to see this
cropping up again in a couple of releases, when some ordering changes
affect the interrupts allocated to this domain.

Another thing worth exploring would be to actually quirk the i8042
driver to override the ACPI interrupt. That would have a better chance
of working in the long term.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again

[Linus Walleij]

On Thu, Apr 6, 2017 at 3:18 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Thu, Apr 06, 2017 at 12:05:28PM +0100, Marc Zyngier wrote:
>> > There is a proper way to use GPIOs as interrupts in ACPI, namely GpioInt
>> > resource but it has not been used in that particular machine for some
>> > reason.
>>
>> OK, that explains it. One thing you could try would be to allocate the
>> domain as a legacy one, which allows you to specify the virtual
>> interrupt range you require. Not sure that's any better though.
>
> Yeah, then I would have to add a whole bunch of code to handle custom
> irqdomain just to support this one machine (fingers crossed that it is
> limited to only this particular machine).

Thierry is working on better generic/custom irqdomains for the GPIOlib
core IRQ support. But right now the quirk is maybe a better solution.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again

[Dmitry Torokhov]

On Wed, Mar 29, 2017 at 12:59:32PM +0300, Mika Westerberg wrote:
> After commit 47c950d10202 ("pinctrl: cherryview: Do not add all
> southwest and north GPIOs to IRQ domain") the driver does not add all
> GPIOs to the irqdomain. The reason for that is that those GPIOs cannot
> generate IRQs at all, only GPEs (General Purpose Events). This causes
> Linux virtual IRQ numbering to change.
> 
> However, it seems some CYAN Chromebooks, including Acer Chromebook
> hardcodes these Linux IRQ numbers in the ACPI tables of the machine.
> Since the numbering is different now, the IRQ meant for keyboard does
> not match the Linux virtual IRQ number anymore making the keyboard
> non-functional.
> 
> Work this around by adding special quirk just for these machines where
> we add back all GPIOs to the irqdomain. Rest of the Cherryview/Braswell
> based machines will not be affected by the change.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=194945
> Fixes: 47c950d10202 ("pinctrl: cherryview: Do not add all southwest and north GPIOs to IRQ domain")
> Reported-by: Adam S Levy <theadamlevy@gmail.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-cherryview.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
> index f80134e3e0b6..bbe9cdf36ca4 100644
> --- a/drivers/pinctrl/intel/pinctrl-cherryview.c
> +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
> @@ -13,6 +13,7 @@
>   * published by the Free Software Foundation.
>   */
>  
> +#include <linux/dmi.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
> @@ -1524,10 +1525,30 @@ static void chv_gpio_irq_handler(struct irq_desc *desc)
>  	chained_irq_exit(chip, desc);
>  }
>  
> +/*
> + * Certain machines seem to hardcode Linux IRQ numbers in their ACPI
> + * tables. Since we leave GPIOs that are not capable of generating
> + * interrupts out of the irqdomain the numbering will be different and
> + * cause devices using the hardcoded IRQ numbers fail. In order not to
> + * break such machines we will only mask pins from irqdomain if the machine
> + * is not listed below.
> + */
> +static const struct dmi_system_id chv_no_valid_mask[] = {
> +	{
> +		/* See https://bugzilla.kernel.org/show_bug.cgi?id=194945 */
> +		.ident = "Acer Chromebook (CYAN)",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "GOOGLE"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Edgar"),

Is there a BIOS version (I do not have my Cyan with me)? Because if
Intel happens to release a fixes for this hard-coded mapping, we would
not want to continue applying this quirk, would we?

Thanks.

-- 
Dmitry

Re: [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again

[Mika Westerberg]

On Mon, Apr 03, 2017 at 11:52:18AM -0700, Dmitry Torokhov wrote:
> Is there a BIOS version (I do not have my Cyan with me)? Because if
> Intel happens to release a fixes for this hard-coded mapping, we would
> not want to continue applying this quirk, would we?

Good point. I think there is a BIOS version string in the DMI
information as well. I'll include it in the next revision of the patch.