diff mbox

[v5] gpio: Add driver for ACPI INT0002 Virtual GPIO device

Message ID 20170524104216.23643-1-hdegoede@redhat.com
State New
Headers show

Commit Message

Hans de Goede May 24, 2017, 10:42 a.m. UTC
Some peripherals on Bay Trail and Cherry Trail platforms signal PME to the
PMC to wakeup the system. When this happens software needs to clear the
PME_B0_STS bit in the GPE0a_STS register to avoid an IRQ storm on IRQ 9.

This is modeled in ACPI through the INT0002 ACPI Virtual GPIO device.

This commit adds a driver which registers the Virtual GPIOs expected
by the DSDT on these devices, letting gpiolib-acpi claim the
virtual GPIO and install a GPIO-interrupt handler which call the _L02
handler as it would for a real GPIO controller.

Cc: joeyli <jlee@suse.com>
Cc: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
Changes in v2:
-Remove dev_err after malloc failure
-Remove unused empty runtime pm callbacks
-s/GPE0A_PME_/GPE0A_PME_B0_/
-Fixed some checkpatch warnings (I forgot to run checkpatch on v1)
Changes in v3:
-Rewrite as gpiochip driver letting gpiolib-acpi deal with claiming the pin
 0x0002 and calling the _L02 event handler when the virtual gpio-irq triggers
-Rebase on 4.12-rc1
Changes in v4:
-Drop device_init_wakeup() from _probe(), use pm_system_wakeup() instead
 of pm_wakeup_hard_event(chip->parent)
-Improve commit message
Changes in v5:
-Use BIT() macro for FOO_BIT defines
-Drop unneeded ACPI_PTR macro usage
---
 drivers/gpio/Kconfig        |  14 +++
 drivers/gpio/Makefile       |   1 +
 drivers/gpio/gpio-int0002.c | 210 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 225 insertions(+)
 create mode 100644 drivers/gpio/gpio-int0002.c

Comments

Andy Shevchenko May 24, 2017, 11:23 a.m. UTC | #1
On Wed, 2017-05-24 at 12:42 +0200, Hans de Goede wrote:
> Some peripherals on Bay Trail and Cherry Trail platforms signal PME to
> the
> PMC to wakeup the system. When this happens software needs to clear
> the
> PME_B0_STS bit in the GPE0a_STS register to avoid an IRQ storm on IRQ
> 9.
> 
> This is modeled in ACPI through the INT0002 ACPI Virtual GPIO device.
> 
> This commit adds a driver which registers the Virtual GPIOs expected
> by the DSDT on these devices, letting gpiolib-acpi claim the
> virtual GPIO and install a GPIO-interrupt handler which call the _L02
> handler as it would for a real GPIO controller.
> 
> Cc: joeyli <jlee@suse.com>
> Cc: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

In addition to my comment to v4 (if you are going to apply it) couple of
nits below. (I'm fine with current version, as tag says, though consider
them anyway)

> +#define GPE0A_PME_B0_STS_BIT		BIT(13)
> +#define GPE0A_PME_B0_EN_BIT		BIT(13)
> +#define GPE0A_STS_PORT			0x420
> +#define GPE0A_EN_PORT			0x428

Perhaps squeeze bits after actual ports like

ADDRESSa
ADDRa_BITa

ADDRESSb
ADDRb_BITa

> +	for (i = 0; i < GPE0A_PME_B0_VIRT_GPIO_PIN; i++)
> +		clear_bit(i, chip->irq_valid_mask);

bitmap_zero() ?
Hans de Goede May 24, 2017, 12:10 p.m. UTC | #2
Hi,

On 24-05-17 13:23, Andy Shevchenko wrote:
> On Wed, 2017-05-24 at 12:42 +0200, Hans de Goede wrote:
>> Some peripherals on Bay Trail and Cherry Trail platforms signal PME to
>> the
>> PMC to wakeup the system. When this happens software needs to clear
>> the
>> PME_B0_STS bit in the GPE0a_STS register to avoid an IRQ storm on IRQ
>> 9.
>>
>> This is modeled in ACPI through the INT0002 ACPI Virtual GPIO device.
>>
>> This commit adds a driver which registers the Virtual GPIOs expected
>> by the DSDT on these devices, letting gpiolib-acpi claim the
>> virtual GPIO and install a GPIO-interrupt handler which call the _L02
>> handler as it would for a real GPIO controller.
>>
>> Cc: joeyli <jlee@suse.com>
>> Cc: Takashi Iwai <tiwai@suse.de>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> In addition to my comment to v4 (if you are going to apply it) couple of
> nits below. (I'm fine with current version, as tag says, though consider
> them anyway)

I prefer to stick with the current version (v5) unless something
bigger comes up.

Regards,

Hans



> 
>> +#define GPE0A_PME_B0_STS_BIT		BIT(13)
>> +#define GPE0A_PME_B0_EN_BIT		BIT(13)
>> +#define GPE0A_STS_PORT			0x420
>> +#define GPE0A_EN_PORT			0x428
> 
> Perhaps squeeze bits after actual ports like
> 
> ADDRESSa
> ADDRa_BITa
> 
> ADDRESSb
> ADDRb_BITa
> 
>> +	for (i = 0; i < GPE0A_PME_B0_VIRT_GPIO_PIN; i++)
>> +		clear_bit(i, chip->irq_valid_mask);
> 
> bitmap_zero() ?
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki May 24, 2017, 11:12 p.m. UTC | #3
On Wed, May 24, 2017 at 12:42 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Some peripherals on Bay Trail and Cherry Trail platforms signal PME to the
> PMC to wakeup the system. When this happens software needs to clear the
> PME_B0_STS bit in the GPE0a_STS register to avoid an IRQ storm on IRQ 9.
>
> This is modeled in ACPI through the INT0002 ACPI Virtual GPIO device.
>
> This commit adds a driver which registers the Virtual GPIOs expected
> by the DSDT on these devices, letting gpiolib-acpi claim the
> virtual GPIO and install a GPIO-interrupt handler which call the _L02
> handler as it would for a real GPIO controller.
>
> Cc: joeyli <jlee@suse.com>
> Cc: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> Changes in v2:
> -Remove dev_err after malloc failure
> -Remove unused empty runtime pm callbacks
> -s/GPE0A_PME_/GPE0A_PME_B0_/
> -Fixed some checkpatch warnings (I forgot to run checkpatch on v1)
> Changes in v3:
> -Rewrite as gpiochip driver letting gpiolib-acpi deal with claiming the pin
>  0x0002 and calling the _L02 event handler when the virtual gpio-irq triggers
> -Rebase on 4.12-rc1
> Changes in v4:
> -Drop device_init_wakeup() from _probe(), use pm_system_wakeup() instead
>  of pm_wakeup_hard_event(chip->parent)
> -Improve commit message
> Changes in v5:
> -Use BIT() macro for FOO_BIT defines
> -Drop unneeded ACPI_PTR macro usage

LGTM

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki May 27, 2017, 7:28 p.m. UTC | #4
On Thursday, May 25, 2017 01:12:37 AM Rafael J. Wysocki wrote:
> On Wed, May 24, 2017 at 12:42 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> > Some peripherals on Bay Trail and Cherry Trail platforms signal PME to the
> > PMC to wakeup the system. When this happens software needs to clear the
> > PME_B0_STS bit in the GPE0a_STS register to avoid an IRQ storm on IRQ 9.
> >
> > This is modeled in ACPI through the INT0002 ACPI Virtual GPIO device.
> >
> > This commit adds a driver which registers the Virtual GPIOs expected
> > by the DSDT on these devices, letting gpiolib-acpi claim the
> > virtual GPIO and install a GPIO-interrupt handler which call the _L02
> > handler as it would for a real GPIO controller.
> >
> > Cc: joeyli <jlee@suse.com>
> > Cc: Takashi Iwai <tiwai@suse.de>
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> > Changes in v2:
> > -Remove dev_err after malloc failure
> > -Remove unused empty runtime pm callbacks
> > -s/GPE0A_PME_/GPE0A_PME_B0_/
> > -Fixed some checkpatch warnings (I forgot to run checkpatch on v1)
> > Changes in v3:
> > -Rewrite as gpiochip driver letting gpiolib-acpi deal with claiming the pin
> >  0x0002 and calling the _L02 event handler when the virtual gpio-irq triggers
> > -Rebase on 4.12-rc1
> > Changes in v4:
> > -Drop device_init_wakeup() from _probe(), use pm_system_wakeup() instead
> >  of pm_wakeup_hard_event(chip->parent)
> > -Improve commit message
> > Changes in v5:
> > -Use BIT() macro for FOO_BIT defines
> > -Drop unneeded ACPI_PTR macro usage
> 
> LGTM
> 
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Linus,

This would be nice to have in 4.12 to unbreak things in there after some recent
ACPI changes (the feature in question have never worked entirely correctly to
be precise, but with this patch on top of the current -rc it actually does work).

I can queue it up if that's not a problem, but need your ACK for that.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij June 1, 2017, 3:11 p.m. UTC | #5
On Wed, May 24, 2017 at 12:42 PM, Hans de Goede <hdegoede@redhat.com> wrote:

> Some peripherals on Bay Trail and Cherry Trail platforms signal PME to the
> PMC to wakeup the system. When this happens software needs to clear the
> PME_B0_STS bit in the GPE0a_STS register to avoid an IRQ storm on IRQ 9.

Spell out these acronyms so I have at least a faint chance of understanding
what is going on here.

> This is modeled in ACPI through the INT0002 ACPI Virtual GPIO device.

So are you saying that some firmware dork has modeled something that
*is* *actually* a port to a PMC (I guess power management controller) to
send a PME (I guess power managment enable signal?) by shoehorning that
into something purporting to be a GPIO (general purpose input/output)
controller?

Well I've been approached before by people all over that just wanna stick
something quick and dirty into the GPIO subsystem because it's so
convenient. ("Come on, just take it", etc.)

I usually just say no. General purpose I/O is not special-purpose I/O.
I bet a million to one these are just a few bits in a register, right?
And someone chose to model that as a "GPIO"?

This takes the price.

Now we have not just lazy kernel programmers but lazy ACPI firmware
authors having to have their sloppy stuff cleaned up by Hans in kernelspace.

And I bet the resource resolution for this thing is completely dependent on
dirvers/gpio/gpiolib-acpi.c so it must be a using GPIO for that reason?

I guess that just force me to not NACK this because I don't want to screw
up all the worlds Intel cherryviews etc.

:(

> +static const struct acpi_device_id int0002_acpi_ids[] = {
> +       { "INT0002", 0 },
> +       { },

So again, the reason this - which is not a GPIO controller at all -
should anyways
be in drivers/gpio/ is that some firmware person think it's convenient?

Shouldn't this rather be in drivers/platform/x86?

You can still use the gpio driver abstraction.

I have some small comments on the code as well, will send in a separate
mail, otherwise good work as always Hans.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans de Goede June 1, 2017, 3:20 p.m. UTC | #6
Hi,

On 01-06-17 17:11, Linus Walleij wrote:
> On Wed, May 24, 2017 at 12:42 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> Some peripherals on Bay Trail and Cherry Trail platforms signal PME to the
>> PMC to wakeup the system. When this happens software needs to clear the
>> PME_B0_STS bit in the GPE0a_STS register to avoid an IRQ storm on IRQ 9.
> 
> Spell out these acronyms so I have at least a faint chance of understanding
> what is going on here.

Sorry, I cannot spell them out without guessing myself, there is no
documentation this commit message is based on the explanation found
in the patches from here:

https://github.com/01org/ProductionKernelQuilts/tree/master/uefi/cht-m1stable/patches

Warning there are over 5000 patches there (yes really that seems to
be how android on x86 gets shipped).

Well I guess I could go over the PDF with the register documentation
that may if we're lucky explain the acronyms, but often it just uses
the acronyms and that's it.

>> This is modeled in ACPI through the INT0002 ACPI Virtual GPIO device.
> 
> So are you saying that some firmware dork has modeled something that
> *is* *actually* a port to a PMC (I guess power management controller) to
> send a PME (I guess power managment enable signal?) by shoehorning that
> into something purporting to be a GPIO (general purpose input/output)
> controller?

Yes, you've got the gist of it right this has nothing to do with GPIOs
at all, but the ACPI firmware representation of the AML event handler
which needs to be called when the PME_B0 interrupt fires is in the
form of a GpioInt and the ACPI node has _AEI and _L02 methods to
match.

> Well I've been approached before by people all over that just wanna stick
> something quick and dirty into the GPIO subsystem because it's so
> convenient. ("Come on, just take it", etc.)
> 
> I usually just say no. General purpose I/O is not special-purpose I/O.
> I bet a million to one these are just a few bits in a register, right?

Right.

> And someone chose to model that as a "GPIO"?

Right.

> 
> This takes the price.
> 
> Now we have not just lazy kernel programmers but lazy ACPI firmware
> authors having to have their sloppy stuff cleaned up by Hans in kernelspace. >
> And I bet the resource resolution for this thing is completely dependent on
> dirvers/gpio/gpiolib-acpi.c so it must be a using GPIO for that reason?

Actually I started out with a stand-alone platform driver which duplicated
some code from gpiolib-acpi.c (not copy pasted but I ended up writing more
or less the same code) you never saw that version as it was not send to
you but to the platform/x86 maintainers.

That version got review comments asking me to redo it in a way which avoided
the duplication and the version you are seeing now is the result of that.

> I guess that just force me to not NACK this because I don't want to screw
> up all the worlds Intel cherryviews etc.
> 
> :(
> 
>> +static const struct acpi_device_id int0002_acpi_ids[] = {
>> +       { "INT0002", 0 },
>> +       { },
> 
> So again, the reason this - which is not a GPIO controller at all -
> should anyways
> be in drivers/gpio/ is that some firmware person think it's convenien >
> Shouldn't this rather be in drivers/platform/x86?

That is where it started, I'm fine with putting it back there.

> You can still use the gpio driver abstraction.

Ack, if you're ok with using the gpio driver abstraction while
putting the driver in drivers/platform/x86 that might be the
best way to deal with this.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij June 1, 2017, 3:23 p.m. UTC | #7
On Wed, May 24, 2017 at 12:42 PM, Hans de Goede <hdegoede@redhat.com> wrote:

> Some peripherals on Bay Trail and Cherry Trail platforms signal PME to the
> PMC to wakeup the system. When this happens software needs to clear the
> PME_B0_STS bit in the GPE0a_STS register to avoid an IRQ storm on IRQ 9.
>
> This is modeled in ACPI through the INT0002 ACPI Virtual GPIO device.
>
> This commit adds a driver which registers the Virtual GPIOs expected
> by the DSDT on these devices, letting gpiolib-acpi claim the
> virtual GPIO and install a GPIO-interrupt handler which call the _L02
> handler as it would for a real GPIO controller.
>
> Cc: joeyli <jlee@suse.com>
> Cc: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig

Please move this thing to drivers/platform/x86.

> +config GPIO_INT0002
> +       tristate "Intel ACPI INT0002 Virtual GPIO"
> +       depends on ACPI
> +       select GPIOLIB_IRQCHIP
> +       ---help---
> +         Some peripherals on Baytrail and Cherrytrail platforms signal
> +         PME to the PMC to wakeup the system. When this happens software

Spell out the acronyms.

> +         needs to explicitly clear the interrupt source to avoid an IRQ
> +         storm on IRQ 9. This is modelled in ACPI through the INT0002
> +         Virtual GPIO ACPI device.

I.e. it is not really GPIO.

Virtual GPIO == not GPIO at all, obviously.

Please write something about this weird newspeak here.

> +++ b/drivers/gpio/gpio-int0002.c
(...)
> + * Some peripherals on Bay Trail and Cherry Trail platforms signal PME to the
> + * PMC to wakeup the system. When this happens software needs to clear the
> + * PME_B0_STS bit in the GPE0a_STS register to avoid an IRQ storm on IRQ 9.

Spell out acronyms.

> +#include <asm/cpu_device_id.h>
> +#include <asm/intel-family.h>
> +#include <linux/acpi.h>
> +#include <linux/gpio.h>

No <linux/gpio.h>, only:
#include <linux/gpio/driver.h>

in drivers.

> +/* For some reason the virtual GPIO pin tied to the GPE is numbered pin 2 */
> +#define GPE0A_PME_B0_VIRT_GPIO_PIN     2

I think it's not weird at all, it is a 32bit word which is not a GPIO
register at all, but
misc IPC register, where bits 0 and 1 does something else.

> +static int int0002_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +       return 0;
> +}
> +
> +static void int0002_gpio_set(struct gpio_chip *chip, unsigned int offset,
> +                            int value)
> +{
> +}
> +
> +static int int0002_gpio_direction_output(struct gpio_chip *chip,
> +                                        unsigned int offset, int value)
> +{
> +       return 0;
> +}

If you're anyways pretending to be a GPIO chip, then you could implement
these. But I guess you're not implementing them, because this is not
a GPIO chip, so let's add a big fat comment above these stating clearly
why they are not implemented proper.

> +static irqreturn_t int0002_irq(int irq, void *data)
> +{
> +       struct gpio_chip *chip = data;
> +       u32 gpe_sts_reg;
> +
> +       gpe_sts_reg = inl(GPE0A_STS_PORT);
> +       if (!(gpe_sts_reg & GPE0A_PME_B0_STS_BIT))
> +               return IRQ_NONE;

I wonder what happens the day an IRQ start arriving on the other
bits. Oh well, that day we handle that.

> +       for (i = 0; i < GPE0A_PME_B0_VIRT_GPIO_PIN; i++)
> +               clear_bit(i, chip->irq_valid_mask);

That's neat.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij June 1, 2017, 3:29 p.m. UTC | #8
On Thu, Jun 1, 2017 at 5:20 PM, Hans de Goede <hdegoede@redhat.com> wrote:

>> So again, the reason this - which is not a GPIO controller at all -
>> should anyways
>> be in drivers/gpio/ is that some firmware person think it's convenien >
>> Shouldn't this rather be in drivers/platform/x86?
>
>
> That is where it started, I'm fine with putting it back there.
>
>> You can still use the gpio driver abstraction.
>
> Ack, if you're ok with using the gpio driver abstraction while
> putting the driver in drivers/platform/x86 that might be the
> best way to deal with this.

OK let's proceed like that.

I guess I could be hopeless and require that it reimplement
the ACPI parser and whatnot just because it's not GPIO but
code duplication is a greater evil and thus modelling this as a
"GPIO" is the lesser evil, so I just have to accept it as such.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki June 1, 2017, 4:35 p.m. UTC | #9
On Thu, Jun 1, 2017 at 5:29 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Jun 1, 2017 at 5:20 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>
>>> So again, the reason this - which is not a GPIO controller at all -
>>> should anyways
>>> be in drivers/gpio/ is that some firmware person think it's convenien >
>>> Shouldn't this rather be in drivers/platform/x86?
>>
>>
>> That is where it started, I'm fine with putting it back there.
>>
>>> You can still use the gpio driver abstraction.
>>
>> Ack, if you're ok with using the gpio driver abstraction while
>> putting the driver in drivers/platform/x86 that might be the
>> best way to deal with this.
>
> OK let's proceed like that.

Cool.

Well, I actually overlooked the "virtual" part somehow, sorry about that.

Yes, it totally should be platform/x86.

> I guess I could be hopeless and require that it reimplement
> the ACPI parser and whatnot just because it's not GPIO but
> code duplication is a greater evil and thus modelling this as a
> "GPIO" is the lesser evil, so I just have to accept it as such.

Thanks for looking into this!

Cheers,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Shevchenko June 2, 2017, 11:10 a.m. UTC | #10
On Thu, 2017-06-01 at 17:23 +0200, Linus Walleij wrote:
> On Wed, May 24, 2017 at 12:42 PM, Hans de Goede <hdegoede@redhat.com>
> wrote:
> 
> Please move this thing to drivers/platform/x86.

I'm fine to take it as long it has your Ack.

> > +         Some peripherals on Baytrail and Cherrytrail platforms
> > signal
> > +         PME to the PMC to wakeup the system. When this happens
> > software
> 
> Spell out the acronyms.

PME is standard of PCI, Power Management Event.

PMC — Power Management Controller

PME_B0 — PME Bus 0

STS — status (obviously :-) )

GPE — General Purpose Event (in ACPI terms)

> (...)
> > + * Some peripherals on Bay Trail and Cherry Trail platforms signal
> > PME to the
> > + * PMC to wakeup the system. When this happens software needs to
> > clear the
> > + * PME_B0_STS bit in the GPE0a_STS register to avoid an IRQ storm
> > on IRQ 9.
> 
> Spell out acronyms.
Hans de Goede June 2, 2017, 2:01 p.m. UTC | #11
Hi,

On 06/01/2017 05:23 PM, Linus Walleij wrote:
> On Wed, May 24, 2017 at 12:42 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> Some peripherals on Bay Trail and Cherry Trail platforms signal PME to the
>> PMC to wakeup the system. When this happens software needs to clear the
>> PME_B0_STS bit in the GPE0a_STS register to avoid an IRQ storm on IRQ 9.
>>
>> This is modeled in ACPI through the INT0002 ACPI Virtual GPIO device.
>>
>> This commit adds a driver which registers the Virtual GPIOs expected
>> by the DSDT on these devices, letting gpiolib-acpi claim the
>> virtual GPIO and install a GPIO-interrupt handler which call the _L02
>> handler as it would for a real GPIO controller.
>>
>> Cc: joeyli <jlee@suse.com>
>> Cc: Takashi Iwai <tiwai@suse.de>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> 
> Please move this thing to drivers/platform/x86.
> 
>> +config GPIO_INT0002
>> +       tristate "Intel ACPI INT0002 Virtual GPIO"
>> +       depends on ACPI
>> +       select GPIOLIB_IRQCHIP
>> +       ---help---
>> +         Some peripherals on Baytrail and Cherrytrail platforms signal
>> +         PME to the PMC to wakeup the system. When this happens software
> 
> Spell out the acronyms.
> 
>> +         needs to explicitly clear the interrupt source to avoid an IRQ
>> +         storm on IRQ 9. This is modelled in ACPI through the INT0002
>> +         Virtual GPIO ACPI device.
> 
> I.e. it is not really GPIO.
> 
> Virtual GPIO == not GPIO at all, obviously.
> 
> Please write something about this weird newspeak here.
> 
>> +++ b/drivers/gpio/gpio-int0002.c
> (...)
>> + * Some peripherals on Bay Trail and Cherry Trail platforms signal PME to the
>> + * PMC to wakeup the system. When this happens software needs to clear the
>> + * PME_B0_STS bit in the GPE0a_STS register to avoid an IRQ storm on IRQ 9.
> 
> Spell out acronyms.
> 
>> +#include <asm/cpu_device_id.h>
>> +#include <asm/intel-family.h>
>> +#include <linux/acpi.h>
>> +#include <linux/gpio.h>
> 
> No <linux/gpio.h>, only:
> #include <linux/gpio/driver.h>
> 
> in drivers.

The above is all fixed for the next version.

>> +/* For some reason the virtual GPIO pin tied to the GPE is numbered pin 2 */
>> +#define GPE0A_PME_B0_VIRT_GPIO_PIN     2
> 
> I think it's not weird at all, it is a 32bit word which is not a GPIO
> register at all, but
> misc IPC register, where bits 0 and 1 does something else.

That is not true the bus 0 status bit we're reacting to and which we're
clearing in this driver is bit 13 ...

> 
>> +static int int0002_gpio_get(struct gpio_chip *chip, unsigned int offset)
>> +{
>> +       return 0;
>> +}
>> +
>> +static void int0002_gpio_set(struct gpio_chip *chip, unsigned int offset,
>> +                            int value)
>> +{
>> +}
>> +
>> +static int int0002_gpio_direction_output(struct gpio_chip *chip,
>> +                                        unsigned int offset, int value)
>> +{
>> +       return 0;
>> +}
> 
> If you're anyways pretending to be a GPIO chip, then you could implement
> these. But I guess you're not implementing them, because this is not
> a GPIO chip, so let's add a big fat comment above these stating clearly
> why they are not implemented proper.

Done for v6.

> 
>> +static irqreturn_t int0002_irq(int irq, void *data)
>> +{
>> +       struct gpio_chip *chip = data;
>> +       u32 gpe_sts_reg;
>> +
>> +       gpe_sts_reg = inl(GPE0A_STS_PORT);
>> +       if (!(gpe_sts_reg & GPE0A_PME_B0_STS_BIT))
>> +               return IRQ_NONE;
> 
> I wonder what happens the day an IRQ start arriving on the other
> bits. Oh well, that day we handle that.
> 
>> +       for (i = 0; i < GPE0A_PME_B0_VIRT_GPIO_PIN; i++)
>> +               clear_bit(i, chip->irq_valid_mask);
> 
> That's neat.
> 
> Yours,
> Linus Walleij
> 

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 23ca51ee6b28..11e7f9b464d4 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -608,6 +608,20 @@  config GPIO_GPIO_MM
 	  The base port addresses for the devices may be configured via the base
 	  array module parameter.
 
+config GPIO_INT0002
+	tristate "Intel ACPI INT0002 Virtual GPIO"
+	depends on ACPI
+	select GPIOLIB_IRQCHIP
+	---help---
+	  Some peripherals on Baytrail and Cherrytrail platforms signal
+	  PME to the PMC to wakeup the system. When this happens software
+	  needs to explicitly clear the interrupt source to avoid an IRQ
+	  storm on IRQ 9. This is modelled in ACPI through the INT0002
+	  Virtual GPIO ACPI device.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called gpio_int0002.
+
 config GPIO_IT87
 	tristate "IT87xx GPIO support"
 	help
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 68b96277d9fa..1f76c5694079 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -55,6 +55,7 @@  obj-$(CONFIG_GPIO_GPIO_MM)	+= gpio-gpio-mm.o
 obj-$(CONFIG_GPIO_GRGPIO)	+= gpio-grgpio.o
 obj-$(CONFIG_HTC_EGPIO)		+= gpio-htc-egpio.o
 obj-$(CONFIG_GPIO_ICH)		+= gpio-ich.o
+obj-$(CONFIG_GPIO_INT0002)	+= gpio-int0002.o
 obj-$(CONFIG_GPIO_IOP)		+= gpio-iop.o
 obj-$(CONFIG_GPIO_IT87)		+= gpio-it87.o
 obj-$(CONFIG_GPIO_JANZ_TTL)	+= gpio-janz-ttl.o
diff --git a/drivers/gpio/gpio-int0002.c b/drivers/gpio/gpio-int0002.c
new file mode 100644
index 000000000000..50b4e0a213d0
--- /dev/null
+++ b/drivers/gpio/gpio-int0002.c
@@ -0,0 +1,210 @@ 
+/*
+ * Intel INT0002 "Virtual GPIO" driver
+ *
+ * Copyright (C) 2017 Hans de Goede <hdegoede@redhat.com>
+ *
+ * Loosely based on android x86 kernel code which is:
+ *
+ * Copyright (c) 2014, Intel Corporation.
+ *
+ * Author: Dyut Kumar Sil <dyut.k.sil@intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Some peripherals on Bay Trail and Cherry Trail platforms signal PME to the
+ * PMC to wakeup the system. When this happens software needs to clear the
+ * PME_B0_STS bit in the GPE0a_STS register to avoid an IRQ storm on IRQ 9.
+ *
+ * This is modelled in ACPI through the INT0002 ACPI device, which is
+ * called a "Virtual GPIO controller" in ACPI because it defines the event
+ * handler to call when the PME triggers through _AEI and _L02 / _E02
+ * methods as would be done for a real GPIO interrupt in ACPI.
+ *
+ * This driver will bind to the INT0002 device, and register as a GPIO
+ * controller, letting gpiolib-acpi.c call the _L02 handler as it would
+ * for a real GPIO controller.
+ */
+
+#include <asm/cpu_device_id.h>
+#include <asm/intel-family.h>
+#include <linux/acpi.h>
+#include <linux/gpio.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/suspend.h>
+
+#define DRV_NAME			"INT0002 Virtual GPIO"
+
+/* For some reason the virtual GPIO pin tied to the GPE is numbered pin 2 */
+#define GPE0A_PME_B0_VIRT_GPIO_PIN	2
+
+#define GPE0A_PME_B0_STS_BIT		BIT(13)
+#define GPE0A_PME_B0_EN_BIT		BIT(13)
+#define GPE0A_STS_PORT			0x420
+#define GPE0A_EN_PORT			0x428
+
+#define ICPU(model)	{ X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY, }
+
+static const struct x86_cpu_id int0002_cpu_ids[] = {
+/*
+ * Limit ourselves to Cherry Trail for now, until testing shows we
+ * need to handle the INT0002 device on Baytrail too.
+ *	ICPU(INTEL_FAM6_ATOM_SILVERMONT1),	 * Valleyview, Bay Trail *
+ */
+	ICPU(INTEL_FAM6_ATOM_AIRMONT),		/* Braswell, Cherry Trail */
+	{}
+};
+
+static int int0002_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	return 0;
+}
+
+static void int0002_gpio_set(struct gpio_chip *chip, unsigned int offset,
+			     int value)
+{
+}
+
+static int int0002_gpio_direction_output(struct gpio_chip *chip,
+					 unsigned int offset, int value)
+{
+	return 0;
+}
+
+static void int0002_irq_ack(struct irq_data *data)
+{
+	outl(GPE0A_PME_B0_STS_BIT, GPE0A_STS_PORT);
+}
+
+static void int0002_irq_unmask(struct irq_data *data)
+{
+	u32 gpe_en_reg;
+
+	gpe_en_reg = inl(GPE0A_EN_PORT);
+	gpe_en_reg |= GPE0A_PME_B0_EN_BIT;
+	outl(gpe_en_reg, GPE0A_EN_PORT);
+}
+
+static void int0002_irq_mask(struct irq_data *data)
+{
+	u32 gpe_en_reg;
+
+	gpe_en_reg = inl(GPE0A_EN_PORT);
+	gpe_en_reg &= ~GPE0A_PME_B0_EN_BIT;
+	outl(gpe_en_reg, GPE0A_EN_PORT);
+}
+
+static irqreturn_t int0002_irq(int irq, void *data)
+{
+	struct gpio_chip *chip = data;
+	u32 gpe_sts_reg;
+
+	gpe_sts_reg = inl(GPE0A_STS_PORT);
+	if (!(gpe_sts_reg & GPE0A_PME_B0_STS_BIT))
+		return IRQ_NONE;
+
+	generic_handle_irq(irq_find_mapping(chip->irqdomain,
+					    GPE0A_PME_B0_VIRT_GPIO_PIN));
+
+	pm_system_wakeup();
+
+	return IRQ_HANDLED;
+}
+
+static struct irq_chip int0002_irqchip = {
+	.name			= DRV_NAME,
+	.irq_ack		= int0002_irq_ack,
+	.irq_mask		= int0002_irq_mask,
+	.irq_unmask		= int0002_irq_unmask,
+};
+
+static int int0002_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	const struct x86_cpu_id *cpu_id;
+	struct gpio_chip *chip;
+	int i, irq, ret;
+
+	/* Menlow has a different INT0002 device? <sigh> */
+	cpu_id = x86_match_cpu(int0002_cpu_ids);
+	if (!cpu_id)
+		return -ENODEV;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(dev, "Error getting IRQ: %d\n", irq);
+		return irq;
+	}
+
+	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	chip->label = DRV_NAME;
+	chip->parent = dev;
+	chip->owner = THIS_MODULE;
+	chip->get = int0002_gpio_get;
+	chip->set = int0002_gpio_set;
+	chip->direction_input = int0002_gpio_get;
+	chip->direction_output = int0002_gpio_direction_output;
+	chip->base = -1;
+	chip->ngpio = GPE0A_PME_B0_VIRT_GPIO_PIN + 1;
+	chip->irq_need_valid_mask = true;
+
+	ret = devm_gpiochip_add_data(&pdev->dev, chip, NULL);
+	if (ret) {
+		dev_err(dev, "Error adding gpio chip: %d\n", ret);
+		return ret;
+	}
+
+	for (i = 0; i < GPE0A_PME_B0_VIRT_GPIO_PIN; i++)
+		clear_bit(i, chip->irq_valid_mask);
+
+	/*
+	 * We manually request the irq here instead of passing a flow-handler
+	 * to gpiochip_set_chained_irqchip, because the irq is shared.
+	 */
+	ret = devm_request_irq(dev, irq, int0002_irq,
+			       IRQF_SHARED | IRQF_NO_THREAD, "INT0002", chip);
+	if (ret) {
+		dev_err(dev, "Error requesting IRQ %d: %d\n", irq, ret);
+		return ret;
+	}
+
+	ret = gpiochip_irqchip_add(chip, &int0002_irqchip, 0, handle_edge_irq,
+				   IRQ_TYPE_NONE);
+	if (ret) {
+		dev_err(dev, "Error adding irqchip: %d\n", ret);
+		return ret;
+	}
+
+	gpiochip_set_chained_irqchip(chip, &int0002_irqchip, irq, NULL);
+
+	return 0;
+}
+
+static const struct acpi_device_id int0002_acpi_ids[] = {
+	{ "INT0002", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, int0002_acpi_ids);
+
+static struct platform_driver int0002_driver = {
+	.driver = {
+		.name			= DRV_NAME,
+		.acpi_match_table	= int0002_acpi_ids,
+	},
+	.probe	= int0002_probe,
+};
+
+module_platform_driver(int0002_driver);
+
+MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
+MODULE_DESCRIPTION("Intel INT0002 Virtual GPIO driver");
+MODULE_LICENSE("GPL");