All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again
@ 2017-03-29  9:59 Mika Westerberg
  2017-03-29 12:58 ` Linus Walleij
  2017-04-03 18:52 ` Dmitry Torokhov
  0 siblings, 2 replies; 15+ messages in thread
From: Mika Westerberg @ 2017-03-29  9:59 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Heikki Krogerus, Adam S Levy, Dmitry Torokhov, Mika Westerberg,
	linux-gpio

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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again
  2017-03-29  9:59 [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again Mika Westerberg
@ 2017-03-29 12:58 ` Linus Walleij
  2017-03-29 13:19   ` Mika Westerberg
  2017-03-29 16:43   ` Marc Zyngier
  2017-04-03 18:52 ` Dmitry Torokhov
  1 sibling, 2 replies; 15+ messages in thread
From: Linus Walleij @ 2017-03-29 12:58 UTC (permalink / raw)
  To: Mika Westerberg, Grant Likely, Thomas Gleixner, Marc Zyngier
  Cc: Heikki Krogerus, Adam S Levy, Dmitry Torokhov, linux-gpio

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again
  2017-03-29 12:58 ` Linus Walleij
@ 2017-03-29 13:19   ` Mika Westerberg
  2017-03-29 13:24     ` Linus Walleij
  2017-03-29 16:43   ` Marc Zyngier
  1 sibling, 1 reply; 15+ messages in thread
From: Mika Westerberg @ 2017-03-29 13:19 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Grant Likely, Thomas Gleixner, Marc Zyngier, Heikki Krogerus,
	Adam S Levy, Dmitry Torokhov, linux-gpio

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again
  2017-03-29 13:19   ` Mika Westerberg
@ 2017-03-29 13:24     ` Linus Walleij
  0 siblings, 0 replies; 15+ messages in thread
From: Linus Walleij @ 2017-03-29 13:24 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: Grant Likely, Thomas Gleixner, Marc Zyngier, Heikki Krogerus,
	Adam S Levy, Dmitry Torokhov, linux-gpio

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again
  2017-03-29 12:58 ` Linus Walleij
  2017-03-29 13:19   ` Mika Westerberg
@ 2017-03-29 16:43   ` Marc Zyngier
  2017-03-30  9:00     ` Mika Westerberg
  1 sibling, 1 reply; 15+ messages in thread
From: Marc Zyngier @ 2017-03-29 16:43 UTC (permalink / raw)
  To: Linus Walleij, Mika Westerberg, Grant Likely, Thomas Gleixner
  Cc: Heikki Krogerus, Adam S Levy, Dmitry Torokhov, linux-gpio

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again
  2017-03-29 16:43   ` Marc Zyngier
@ 2017-03-30  9:00     ` Mika Westerberg
  2017-04-06  9:59       ` Mika Westerberg
  0 siblings, 1 reply; 15+ messages in thread
From: Mika Westerberg @ 2017-03-30  9:00 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Linus Walleij, Grant Likely, Thomas Gleixner, Heikki Krogerus,
	Adam S Levy, Dmitry Torokhov, linux-gpio

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again
  2017-03-29  9:59 [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again Mika Westerberg
  2017-03-29 12:58 ` Linus Walleij
@ 2017-04-03 18:52 ` Dmitry Torokhov
  2017-04-06  8:19   ` Mika Westerberg
  1 sibling, 1 reply; 15+ messages in thread
From: Dmitry Torokhov @ 2017-04-03 18:52 UTC (permalink / raw)
  To: Mika Westerberg; +Cc: Linus Walleij, Heikki Krogerus, Adam S Levy, linux-gpio

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again
  2017-04-03 18:52 ` Dmitry Torokhov
@ 2017-04-06  8:19   ` Mika Westerberg
  0 siblings, 0 replies; 15+ messages in thread
From: Mika Westerberg @ 2017-04-06  8:19 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Linus Walleij, Heikki Krogerus, Adam S Levy, linux-gpio

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again
  2017-03-30  9:00     ` Mika Westerberg
@ 2017-04-06  9:59       ` Mika Westerberg
  2017-04-06 10:32         ` Marc Zyngier
  0 siblings, 1 reply; 15+ messages in thread
From: Mika Westerberg @ 2017-04-06  9:59 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Linus Walleij, Grant Likely, Thomas Gleixner, Heikki Krogerus,
	Adam S Levy, Dmitry Torokhov, linux-gpio

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again
  2017-04-06  9:59       ` Mika Westerberg
@ 2017-04-06 10:32         ` Marc Zyngier
  2017-04-06 10:50           ` Mika Westerberg
  0 siblings, 1 reply; 15+ messages in thread
From: Marc Zyngier @ 2017-04-06 10:32 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: Linus Walleij, Grant Likely, Thomas Gleixner, Heikki Krogerus,
	Adam S Levy, Dmitry Torokhov, linux-gpio

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again
  2017-04-06 10:32         ` Marc Zyngier
@ 2017-04-06 10:50           ` Mika Westerberg
  2017-04-06 11:05             ` Marc Zyngier
  0 siblings, 1 reply; 15+ messages in thread
From: Mika Westerberg @ 2017-04-06 10:50 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Linus Walleij, Grant Likely, Thomas Gleixner, Heikki Krogerus,
	Adam S Levy, Dmitry Torokhov, linux-gpio

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again
  2017-04-06 10:50           ` Mika Westerberg
@ 2017-04-06 11:05             ` Marc Zyngier
  2017-04-06 13:18               ` Mika Westerberg
  0 siblings, 1 reply; 15+ messages in thread
From: Marc Zyngier @ 2017-04-06 11:05 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: Linus Walleij, Grant Likely, Thomas Gleixner, Heikki Krogerus,
	Adam S Levy, Dmitry Torokhov, linux-gpio

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again
  2017-04-06 11:05             ` Marc Zyngier
@ 2017-04-06 13:18               ` Mika Westerberg
  2017-04-06 15:15                 ` Marc Zyngier
  2017-04-07  7:27                 ` Linus Walleij
  0 siblings, 2 replies; 15+ messages in thread
From: Mika Westerberg @ 2017-04-06 13:18 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Linus Walleij, Grant Likely, Thomas Gleixner, Heikki Krogerus,
	Adam S Levy, Dmitry Torokhov, linux-gpio

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again
  2017-04-06 13:18               ` Mika Westerberg
@ 2017-04-06 15:15                 ` Marc Zyngier
  2017-04-07  7:27                 ` Linus Walleij
  1 sibling, 0 replies; 15+ messages in thread
From: Marc Zyngier @ 2017-04-06 15:15 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: Linus Walleij, Grant Likely, Thomas Gleixner, Heikki Krogerus,
	Adam S Levy, Dmitry Torokhov, linux-gpio

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again
  2017-04-06 13:18               ` Mika Westerberg
  2017-04-06 15:15                 ` Marc Zyngier
@ 2017-04-07  7:27                 ` Linus Walleij
  1 sibling, 0 replies; 15+ messages in thread
From: Linus Walleij @ 2017-04-07  7:27 UTC (permalink / raw)
  To: Mika Westerberg, thierry.reding
  Cc: Marc Zyngier, Grant Likely, Thomas Gleixner, Heikki Krogerus,
	Adam S Levy, Dmitry Torokhov, linux-gpio

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2017-04-07  7:27 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-29  9:59 [PATCH] pinctrl: cherryview: Add a quirk to make Acer Chromebook keyboard work again Mika Westerberg
2017-03-29 12:58 ` Linus Walleij
2017-03-29 13:19   ` Mika Westerberg
2017-03-29 13:24     ` Linus Walleij
2017-03-29 16:43   ` Marc Zyngier
2017-03-30  9:00     ` Mika Westerberg
2017-04-06  9:59       ` Mika Westerberg
2017-04-06 10:32         ` Marc Zyngier
2017-04-06 10:50           ` Mika Westerberg
2017-04-06 11:05             ` Marc Zyngier
2017-04-06 13:18               ` Mika Westerberg
2017-04-06 15:15                 ` Marc Zyngier
2017-04-07  7:27                 ` Linus Walleij
2017-04-03 18:52 ` Dmitry Torokhov
2017-04-06  8:19   ` Mika Westerberg

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.