From: Thomas Petazzoni <thomas.petazzoni@bootlin.com> To: Michal Simek <michal.simek@xilinx.com>, Linus Walleij <linus.walleij@linaro.org> Cc: "Nava kishore Manne" <navam@xilinx.com>, "Josh Cartwright" <josh.cartwright@ni.com>, "monstr@monstr.eu" <monstr@monstr.eu>, "Peter Crosthwaite" <peter.crosthwaite@xilinx.com>, "Borsodi Petr" <Petr.Borsodi@i.cz>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>, "Rob Herring" <robherring2@gmail.com>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, "Steffen Trumtrar" <s.trumtrar@pengutronix.de>, "Sören Brinkmann" <soren.brinkmann@xilinx.com>, "Shubhrajyoti Datta" <shubhraj@xilinx.com> Subject: Re: [PATCH 2/8] gpio: zynq: Wakeup gpio controller when it is used as IRQ controller Date: Mon, 7 Jan 2019 16:42:10 +0100 [thread overview] Message-ID: <20190107164210.3ecf37e8@windsurf> (raw) In-Reply-To: <6da5fd79-fbc8-b613-954f-dcbe2ef8d6c5@xilinx.com> Hello, I am reviving this old thread, because the proposed patch (almost) solves the problem I recently reported with the bad interaction of runtime PM with the Zynq GPIO driver (see https://www.spinics.net/lists/linux-gpio/msg35437.html). On Mon, 14 Aug 2017 16:33:09 +0200, Michal Simek wrote: > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 9568708a550b..a08a044fa4aa 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -1647,14 +1647,22 @@ static void gpiochip_irq_unmap(struct irq_domain > *d, unsigned int irq) > static int gpiochip_irq_reqres(struct irq_data *d) > { > struct gpio_chip *chip = irq_data_get_irq_chip_data(d); > + int ret; > > if (!try_module_get(chip->gpiodev->owner)) > return -ENODEV; > > + ret = pm_runtime_get_sync(chip->parent); > + if (ret < 0) { > + module_put(chip->gpiodev->owner); > + return ret; > + } > + > if (gpiochip_lock_as_irq(chip, d->hwirq)) { > chip_err(chip, > "unable to lock HW IRQ %lu for IRQ\n", > d->hwirq); > + pm_runtime_put(chip->parent); > module_put(chip->gpiodev->owner); > return -EINVAL; > } > @@ -1666,6 +1674,7 @@ static void gpiochip_irq_relres(struct irq_data *d) > struct gpio_chip *chip = irq_data_get_irq_chip_data(d); > > gpiochip_unlock_as_irq(chip, d->hwirq); > + pm_runtime_put(chip->parent); > module_put(chip->gpiodev->owner); > } This patch almost solves the problem. It doesn't work as-is, because it assumes that runtime PM is used by all GPIO controllers, which is not the case. When runtime PM is not enabled, pm_runtime_get_sync() fails with -EACCES, and the whole gpiochip_irq_reqres() function aborts. The following patch works fine in my case (a MMC card detect signal is connected to a pin of a PCA GPIO expander over I2C, whose INT# pin is itself connected to a GPIO pin of the Zynq SoC). diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 20887c62fbb3..bd9a81fc8d56 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -27,6 +27,7 @@ #include <linux/kfifo.h> #include <linux/poll.h> #include <linux/timekeeping.h> +#include <linux/pm_runtime.h> #include <uapi/linux/gpio.h> #include "gpiolib.h" @@ -3540,12 +3541,23 @@ int gpiochip_reqres_irq(struct gpio_chip *chip, unsigned int offset) if (!try_module_get(chip->gpiodev->owner)) return -ENODEV; + if (pm_runtime_enabled(chip->parent)) { + ret = pm_runtime_get_sync(chip->parent); + if (ret < 0) { + module_put(chip->gpiodev->owner); + return ret; + } + } + ret = gpiochip_lock_as_irq(chip, offset); if (ret) { chip_err(chip, "unable to lock HW IRQ %u for IRQ\n", offset); + if (pm_runtime_enabled(chip->parent)) + pm_runtime_put(chip->parent); module_put(chip->gpiodev->owner); return ret; } + return 0; } EXPORT_SYMBOL_GPL(gpiochip_reqres_irq); @@ -3553,6 +3565,8 @@ EXPORT_SYMBOL_GPL(gpiochip_reqres_irq); void gpiochip_relres_irq(struct gpio_chip *chip, unsigned int offset) { gpiochip_unlock_as_irq(chip, offset); + if (pm_runtime_enabled(chip->parent)) + pm_runtime_put(chip->parent); module_put(chip->gpiodev->owner); } EXPORT_SYMBOL_GPL(gpiochip_relres_irq); However, I must say that from a design perspective, I am not a big fan of this solution. Indeed for the normal GPIO ->request() and ->free() hooks, it is currently the GPIO driver itself that is responsible for runtime PM get/put, so it would be weird to have the runtime PM get/put for the IRQ request/free be done by the GPIO core. I believe that either the GPIO core should be in charge of the entire runtime PM interaction, or it should entirely be the responsibility of each GPIO controller driver. Having a mixed solution seems very confusing. Let me know which direction should be taken so that I can submit a proper patch to hopefully resolve this issue. Best regards, Thomas -- Thomas Petazzoni, CTO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
WARNING: multiple messages have this Message-ID (diff)
From: Thomas Petazzoni <thomas.petazzoni@bootlin.com> To: Michal Simek <michal.simek@xilinx.com>, Linus Walleij <linus.walleij@linaro.org> Cc: "Josh Cartwright" <josh.cartwright@ni.com>, "monstr@monstr.eu" <monstr@monstr.eu>, "Nava kishore Manne" <navam@xilinx.com>, "Peter Crosthwaite" <peter.crosthwaite@xilinx.com>, "Borsodi Petr" <Petr.Borsodi@i.cz>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>, "Rob Herring" <robherring2@gmail.com>, "Sören Brinkmann" <soren.brinkmann@xilinx.com>, "Steffen Trumtrar" <s.trumtrar@pengutronix.de>, "Shubhrajyoti Datta" <shubhraj@xilinx.com>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org> Subject: Re: [PATCH 2/8] gpio: zynq: Wakeup gpio controller when it is used as IRQ controller Date: Mon, 7 Jan 2019 16:42:10 +0100 [thread overview] Message-ID: <20190107164210.3ecf37e8@windsurf> (raw) In-Reply-To: <6da5fd79-fbc8-b613-954f-dcbe2ef8d6c5@xilinx.com> Hello, I am reviving this old thread, because the proposed patch (almost) solves the problem I recently reported with the bad interaction of runtime PM with the Zynq GPIO driver (see https://www.spinics.net/lists/linux-gpio/msg35437.html). On Mon, 14 Aug 2017 16:33:09 +0200, Michal Simek wrote: > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 9568708a550b..a08a044fa4aa 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -1647,14 +1647,22 @@ static void gpiochip_irq_unmap(struct irq_domain > *d, unsigned int irq) > static int gpiochip_irq_reqres(struct irq_data *d) > { > struct gpio_chip *chip = irq_data_get_irq_chip_data(d); > + int ret; > > if (!try_module_get(chip->gpiodev->owner)) > return -ENODEV; > > + ret = pm_runtime_get_sync(chip->parent); > + if (ret < 0) { > + module_put(chip->gpiodev->owner); > + return ret; > + } > + > if (gpiochip_lock_as_irq(chip, d->hwirq)) { > chip_err(chip, > "unable to lock HW IRQ %lu for IRQ\n", > d->hwirq); > + pm_runtime_put(chip->parent); > module_put(chip->gpiodev->owner); > return -EINVAL; > } > @@ -1666,6 +1674,7 @@ static void gpiochip_irq_relres(struct irq_data *d) > struct gpio_chip *chip = irq_data_get_irq_chip_data(d); > > gpiochip_unlock_as_irq(chip, d->hwirq); > + pm_runtime_put(chip->parent); > module_put(chip->gpiodev->owner); > } This patch almost solves the problem. It doesn't work as-is, because it assumes that runtime PM is used by all GPIO controllers, which is not the case. When runtime PM is not enabled, pm_runtime_get_sync() fails with -EACCES, and the whole gpiochip_irq_reqres() function aborts. The following patch works fine in my case (a MMC card detect signal is connected to a pin of a PCA GPIO expander over I2C, whose INT# pin is itself connected to a GPIO pin of the Zynq SoC). diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 20887c62fbb3..bd9a81fc8d56 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -27,6 +27,7 @@ #include <linux/kfifo.h> #include <linux/poll.h> #include <linux/timekeeping.h> +#include <linux/pm_runtime.h> #include <uapi/linux/gpio.h> #include "gpiolib.h" @@ -3540,12 +3541,23 @@ int gpiochip_reqres_irq(struct gpio_chip *chip, unsigned int offset) if (!try_module_get(chip->gpiodev->owner)) return -ENODEV; + if (pm_runtime_enabled(chip->parent)) { + ret = pm_runtime_get_sync(chip->parent); + if (ret < 0) { + module_put(chip->gpiodev->owner); + return ret; + } + } + ret = gpiochip_lock_as_irq(chip, offset); if (ret) { chip_err(chip, "unable to lock HW IRQ %u for IRQ\n", offset); + if (pm_runtime_enabled(chip->parent)) + pm_runtime_put(chip->parent); module_put(chip->gpiodev->owner); return ret; } + return 0; } EXPORT_SYMBOL_GPL(gpiochip_reqres_irq); @@ -3553,6 +3565,8 @@ EXPORT_SYMBOL_GPL(gpiochip_reqres_irq); void gpiochip_relres_irq(struct gpio_chip *chip, unsigned int offset) { gpiochip_unlock_as_irq(chip, offset); + if (pm_runtime_enabled(chip->parent)) + pm_runtime_put(chip->parent); module_put(chip->gpiodev->owner); } EXPORT_SYMBOL_GPL(gpiochip_relres_irq); However, I must say that from a design perspective, I am not a big fan of this solution. Indeed for the normal GPIO ->request() and ->free() hooks, it is currently the GPIO driver itself that is responsible for runtime PM get/put, so it would be weird to have the runtime PM get/put for the IRQ request/free be done by the GPIO core. I believe that either the GPIO core should be in charge of the entire runtime PM interaction, or it should entirely be the responsibility of each GPIO controller driver. Having a mixed solution seems very confusing. Let me know which direction should be taken so that I can submit a proper patch to hopefully resolve this issue. Best regards, Thomas -- Thomas Petazzoni, CTO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-01-07 15:42 UTC|newest] Thread overview: 61+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-08-07 11:01 [PATCH 0/8] Zynq GPIO driver changes Michal Simek 2017-08-07 11:01 ` Michal Simek 2017-08-07 11:01 ` [PATCH 1/8] gpio: zynq: Add support for suspend resume Michal Simek 2017-08-07 11:01 ` Michal Simek 2017-08-07 11:01 ` Michal Simek 2017-08-14 13:44 ` Linus Walleij 2017-08-14 13:44 ` Linus Walleij 2017-08-07 11:01 ` [PATCH 2/8] gpio: zynq: Wakeup gpio controller when it is used as IRQ controller Michal Simek 2017-08-07 11:01 ` Michal Simek 2017-08-14 13:53 ` Linus Walleij 2017-08-14 13:53 ` Linus Walleij 2017-08-14 14:33 ` Michal Simek 2017-08-14 14:33 ` Michal Simek 2017-08-22 12:57 ` Linus Walleij 2017-08-22 12:57 ` Linus Walleij 2019-01-07 15:42 ` Thomas Petazzoni [this message] 2019-01-07 15:42 ` Thomas Petazzoni 2019-01-08 13:21 ` Michal Simek 2019-01-08 13:21 ` Michal Simek 2019-01-11 9:54 ` Linus Walleij 2019-01-11 9:54 ` Linus Walleij 2019-01-11 12:54 ` Thomas Petazzoni 2019-01-11 12:54 ` Thomas Petazzoni 2019-01-11 14:37 ` Linus Walleij 2019-01-11 14:37 ` Linus Walleij 2019-01-11 14:37 ` Linus Walleij 2019-01-21 6:11 ` Shubhrajyoti Datta 2019-01-21 6:11 ` Shubhrajyoti Datta 2019-01-21 6:11 ` Shubhrajyoti Datta 2017-08-07 11:01 ` [PATCH 3/8] gpio: zynq: Shift zynq_gpio_init() to subsys_initcall level Michal Simek 2017-08-07 11:01 ` Michal Simek 2017-08-14 13:55 ` Linus Walleij 2017-08-14 13:55 ` Linus Walleij 2017-08-14 14:15 ` Michal Simek 2017-08-14 14:15 ` Michal Simek 2017-08-22 13:02 ` Linus Walleij 2017-08-22 13:02 ` Linus Walleij 2017-08-07 11:01 ` [PATCH 4/8] gpio: zynq: Provided workaround for GPIO Michal Simek 2017-08-07 11:01 ` Michal Simek 2017-08-14 13:57 ` Linus Walleij 2017-08-14 13:57 ` Linus Walleij 2017-08-14 14:01 ` Michal Simek 2017-08-14 14:01 ` Michal Simek 2017-08-07 11:01 ` [PATCH 5/8] gpio: zynq: Fix kernel doc warnings Michal Simek 2017-08-07 11:01 ` Michal Simek 2017-08-14 13:58 ` Linus Walleij 2017-08-14 13:58 ` Linus Walleij 2017-08-07 11:01 ` [PATCH 6/8] gpio: zynq: Fix empty lines in driver Michal Simek 2017-08-07 11:01 ` Michal Simek 2017-08-14 13:59 ` Linus Walleij 2017-08-14 13:59 ` Linus Walleij 2017-08-07 11:02 ` [PATCH 7/8] gpio: zynq: Fix warnings in the driver Michal Simek 2017-08-07 11:02 ` Michal Simek 2017-08-14 14:00 ` Linus Walleij 2017-08-14 14:00 ` Linus Walleij 2017-08-07 11:02 ` [PATCH 8/8] gpio: zynq: Fix driver function parameters alignment Michal Simek 2017-08-07 11:02 ` Michal Simek 2017-08-14 14:01 ` Linus Walleij 2017-08-14 14:01 ` Linus Walleij 2017-08-14 14:03 ` Michal Simek 2017-08-14 14:03 ` Michal Simek
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190107164210.3ecf37e8@windsurf \ --to=thomas.petazzoni@bootlin.com \ --cc=Petr.Borsodi@i.cz \ --cc=josh.cartwright@ni.com \ --cc=linus.walleij@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-gpio@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=michal.simek@xilinx.com \ --cc=monstr@monstr.eu \ --cc=navam@xilinx.com \ --cc=peter.crosthwaite@xilinx.com \ --cc=robherring2@gmail.com \ --cc=s.trumtrar@pengutronix.de \ --cc=shubhraj@xilinx.com \ --cc=soren.brinkmann@xilinx.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.