| Message ID | 20190207185917.167829-1-egranata@google.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | driver: platform: Add support for GpioInt() ACPI to platform_get_irq() | expand |
+Mika Westerberg +Andy Shevchenko +Hans de Goede On Thu, Feb 7, 2019 at 7:59 PM <egranata@google.com> wrote: > > From: Enrico Granata <egranata@chromium.org> > > ACPI 5 added support for GpioInt resources as a way to provide > information about interrupts mediated via a GPIO controller. > > Several device buses (e.g. SPI, I2C) have support for retrieving > an IRQ specified via this type of resource, and providing it > directly to the driver as an IRQ number. > This is not currently done for the platform drivers, as platform_get_irq() > does not try to parse GpioInt() resources. > > This commit adds that functionality. > > Signed-off-by: Enrico Granata <egranata@chromium.org> > --- > drivers/base/platform.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > index 1c958eb33ef4d..c50c4f9033aef 100644 > --- a/drivers/base/platform.c > +++ b/drivers/base/platform.c > @@ -127,7 +127,17 @@ int platform_get_irq(struct platform_device *dev, unsigned int num) > irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS); > } > > - return r ? r->start : -ENXIO; > + if (r) > + return r->start; > + > + /* > + * If no IRQ was found, try to parse ACPI GpioInt resources > + * as a last resort. > + */ > + if (has_acpi_companion(&dev->dev)) > + return acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num); > + > + return -ENXIO; > #endif > } > EXPORT_SYMBOL_GPL(platform_get_irq); > -- > 2.20.1.611.gfbb209baf1-goog >
On Thu, Feb 7, 2019 at 9:04 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > +Mika Westerberg > +Andy Shevchenko > +Hans de Goede Thanks, Rafael. My comments below. > On Thu, Feb 7, 2019 at 7:59 PM <egranata@google.com> wrote: > > > > From: Enrico Granata <egranata@chromium.org> > > > > ACPI 5 added support for GpioInt resources as a way to provide > > information about interrupts mediated via a GPIO controller. > > > > Several device buses (e.g. SPI, I2C) have support for retrieving > > an IRQ specified via this type of resource, and providing it > > directly to the driver as an IRQ number. > > This is not currently done for the platform drivers, as platform_get_irq() > > does not try to parse GpioInt() resources. And why is this a problem? > > This commit adds that functionality. How that can override the configuration / BIOS flavour when driver needs to register an interrupt out of GpioIo() resource? P.S. Have you looked at drivers/platform/x86/i2c-multi-instantiate.c and intel_cht_int33fe.c? They are the only drivers which needs something like this for now and I would rather say it's a bad BIOS decision to write a table like that.
On Thu, Feb 7, 2019 at 9:39 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Feb 7, 2019 at 9:04 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > This commit adds that functionality. > > How that can override the configuration / BIOS flavour when driver > needs to register an interrupt out of GpioIo() resource? One more missed example, Two variants of the IRQ resources in the table for the _same_ device but on different platforms. --Variant1-- Device(XYZ) _CRS1() { Interrupt() GpioInt() Interrupt() } --Variant2-- Device(XYZ) _CRS2() { GpioInt() Interrupt() Interrupt() } How to always get second resource?
Resending as plain text; comments inlined. On Thu, Feb 7, 2019 at 11:39 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Thu, Feb 7, 2019 at 9:04 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > +Mika Westerberg > > +Andy Shevchenko > > +Hans de Goede > > Thanks, Rafael. > My comments below. > > > On Thu, Feb 7, 2019 at 7:59 PM <egranata@google.com> wrote: > > > > > > From: Enrico Granata <egranata@chromium.org> > > > > > > ACPI 5 added support for GpioInt resources as a way to provide > > > information about interrupts mediated via a GPIO controller. > > > > > > Several device buses (e.g. SPI, I2C) have support for retrieving > > > an IRQ specified via this type of resource, and providing it > > > directly to the driver as an IRQ number. > > > > This is not currently done for the platform drivers, as platform_get_irq() > > > does not try to parse GpioInt() resources. > > And why is this a problem? In ChromeOS, we have a driver (cros_ec_lpc) which can run either on systems that directly expose the interrupt, or systems where the interrupt goes through a GPIO controller. On the former, firmware provides an Interrupt resource and platform_get_irq() finds it. On the latter, firmware provides a GpioInt resource and platform_get_irq does not find it. We could work around this in the driver by probing both paths, but since other subsystems seem to directly look for GpioInt resources, it seemed to us to make more sense to extend platform_get_irq() instead. > > > This commit adds that functionality. > > How that can override the configuration / BIOS flavour when driver > needs to register an interrupt out of GpioIo() resource? > > P.S. Have you looked at drivers/platform/x86/i2c-multi-instantiate.c > and intel_cht_int33fe.c? Those drivers seem to only ever expect a GpioInt(). Our case is one where both are possible, depending on the system we're running on. We are trying not to put ad-hoc logic in the driver, but if you think that doing so would be our best course of action, please do let us know. > They are the only drivers which needs something like this for now and > I would rather say it's a bad BIOS decision to write a table like > that. Do you have any suggestions to write proper firmware tables to avoid this issue? > > -- > With Best Regards, > Andy Shevchenko
On Thu, Feb 7, 2019 at 9:45 PM Enrico Granata <egranata@google.com> wrote: > On Thu, Feb 7, 2019 at 11:39 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >> On Thu, Feb 7, 2019 at 9:04 PM Rafael J. Wysocki <rafael@kernel.org> wrote: >> > > This is not currently done for the platform drivers, as platform_get_irq() >> > > does not try to parse GpioInt() resources. >> >> And why is this a problem? > In ChromeOS, we have a driver (cros_ec_lpc) which can run either on systems that directly expose the interrupt, > or systems where the interrupt goes through a GPIO controller. On the former, firmware provides an Interrupt resource > and platform_get_irq() finds it. On the latter, firmware provides a GpioInt resource and platform_get_irq does not > find it. We could work around this in the driver by probing both paths, but since other subsystems seem to directly > look for GpioInt resources, it seemed to us to make more sense to extend platform_get_irq() instead. Looking briefly into the driver I found third scenario — no resource at all. So, you already have a quirk for that. Now it's the question either you go for global quirk (trying to find an IRQ via iterating over GpioInt() resources like in this patch, but in the driver), or use DMI table for more stricter rules. Either way you choose, I don't see a necessity to put this to the driver core for now since it would be the only (let's assume properly written ACPI tables) driver needs such. > Do you have a suggestion as to how to write ACPI tables to avoid the issue? 1. Allocate new ID and use it (perhaps not the best path). 2. Use GPE(s). -- With Best Regards, Andy Shevchenko
On Thu, Feb 7, 2019 at 12:18 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Thu, Feb 7, 2019 at 9:45 PM Enrico Granata <egranata@google.com> wrote: > > On Thu, Feb 7, 2019 at 11:39 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > >> On Thu, Feb 7, 2019 at 9:04 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > >> > > This is not currently done for the platform drivers, as platform_get_irq() > >> > > does not try to parse GpioInt() resources. > >> > >> And why is this a problem? > > > In ChromeOS, we have a driver (cros_ec_lpc) which can run either on systems that directly expose the interrupt, > > or systems where the interrupt goes through a GPIO controller. On the former, firmware provides an Interrupt resource > > and platform_get_irq() finds it. On the latter, firmware provides a GpioInt resource and platform_get_irq does not > > find it. We could work around this in the driver by probing both paths, but since other subsystems seem to directly > > look for GpioInt resources, it seemed to us to make more sense to extend platform_get_irq() instead. > > Looking briefly into the driver I found third scenario — no resource at all. > So, you already have a quirk for that. Now it's the question either > you go for global quirk (trying to find an IRQ via iterating over > GpioInt() resources like in this patch, but in the driver), or use DMI > table for more stricter rules. No, no DMI rules please, they are more pain than anything. > > Either way you choose, I don't see a necessity to put this to the > driver core for now since it would be the only (let's assume properly > written ACPI tables) driver needs such. This is simply kicking the can down the road. > > > Do you have a suggestion as to how to write ACPI tables to avoid the issue? > > 1. Allocate new ID and use it (perhaps not the best path). > 2. Use GPE(s). > Or just solve the issue of intermixing Interrupt() with GpioInt(). We have similar issue with i2c and spi, but we sidestep that there as we only parse the first interrupt and do not give option of fetching 2nd, 3rd, etc. Maybe we should only GpioInt parsing for the first interrupt in platform_get_irq() as well for the first iteration and then see if we need to improve it if we see devices with multiple interrupts. Thanks, Dmitry
Hi, On Thu, Feb 07, 2019 at 12:29:17PM -0800, Dmitry Torokhov wrote: > > > > > Do you have a suggestion as to how to write ACPI tables to avoid the issue? > > > > 1. Allocate new ID and use it (perhaps not the best path). > > 2. Use GPE(s). > > > > Or just solve the issue of intermixing Interrupt() with GpioInt(). We > have similar issue with i2c and spi, but we sidestep that there as we > only parse the first interrupt and do not give option of fetching 2nd, > 3rd, etc. Maybe we should only GpioInt parsing for the first interrupt > in platform_get_irq() as well for the first iteration and then see if > we need to improve it if we see devices with multiple interrupts. I think it should be fine to intermix them or do what you suggest and start supporting index 0 for now and then maybe extend it in the future to cover more.
Hi, On 11-02-19 11:30, Mika Westerberg wrote: > Hi, > > On Thu, Feb 07, 2019 at 12:29:17PM -0800, Dmitry Torokhov wrote: >>> >>>> Do you have a suggestion as to how to write ACPI tables to avoid the issue? >>> >>> 1. Allocate new ID and use it (perhaps not the best path). >>> 2. Use GPE(s). >>> >> >> Or just solve the issue of intermixing Interrupt() with GpioInt(). We >> have similar issue with i2c and spi, but we sidestep that there as we >> only parse the first interrupt and do not give option of fetching 2nd, >> 3rd, etc. Maybe we should only GpioInt parsing for the first interrupt >> in platform_get_irq() as well for the first iteration and then see if >> we need to improve it if we see devices with multiple interrupts. > > I think it should be fine to intermix them or do what you suggest and > start supporting index 0 for now and then maybe extend it in the future > to cover more. I think only support fallback to GpioInt for index 0 for now is probably the best solution. A device could have both Interrupt and GpioInt resources, as soon as that is the case then the meaning of index becomes ambiguous. We are already seeing something similar with mixed use of GpioInt + Gpio resources on some devices, where we need the GpioInt for the IRQ and the Gpio resource to toggle something else and the ACPI tables on different devices have them in a different order, see: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/?id=4d1f7a6eabd45639d9de22a8a004f3c208d13c1a I suspect that on Windows device drivers specifically specify if they want a Gpio or a GpioInt; or in this case if they want an Interrupt or a GpioInt. Regards, Hans
diff --git a/drivers/base/platform.c b/drivers/base/platform.c index 1c958eb33ef4d..c50c4f9033aef 100644 --- a/drivers/base/platform.c +++ b/drivers/base/platform.c @@ -127,7 +127,17 @@ int platform_get_irq(struct platform_device *dev, unsigned int num) irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS); } - return r ? r->start : -ENXIO; + if (r) + return r->start; + + /* + * If no IRQ was found, try to parse ACPI GpioInt resources + * as a last resort. + */ + if (has_acpi_companion(&dev->dev)) + return acpi_dev_gpio_irq_get(ACPI_COMPANION(&dev->dev), num); + + return -ENXIO; #endif } EXPORT_SYMBOL_GPL(platform_get_irq);