[PATCH] gpio: mxc: add runtime pm support

[PATCH] gpio: mxc: add runtime pm support

[Shenwei Wang]

Adds runtime PM support and allow the GPIO controller to enter
into runtime suspend automatically when not in use to save power.
However, it will automatically resume and enable clocks when a
GPIO or IRQ is requested.

Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com>
---
 drivers/gpio/gpio-mxc.c | 67 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 65 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index 9d0cec4b82a3..10387655a903 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c
@@ -17,6 +17,7 @@
 #include <linux/irqchip/chained_irq.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/syscore_ops.h>
@@ -382,6 +383,24 @@ static int mxc_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
 	return irq_find_mapping(port->domain, offset);
 }
 
+static int mxc_gpio_request(struct gpio_chip *chip, unsigned int offset)
+{
+	int ret;
+
+	ret = gpiochip_generic_request(chip, offset);
+	if (ret)
+		return ret;
+
+	ret = pm_runtime_get_sync(chip->parent);
+	return ret < 0 ? ret : 0;
+}
+
+static void mxc_gpio_free(struct gpio_chip *chip, unsigned int offset)
+{
+	gpiochip_generic_free(chip, offset);
+	pm_runtime_put(chip->parent);
+}
+
 static int mxc_gpio_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -429,6 +448,12 @@ static int mxc_gpio_probe(struct platform_device *pdev)
 	if (of_device_is_compatible(np, "fsl,imx7d-gpio"))
 		port->power_off = true;
 
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+	err = pm_runtime_get_sync(&pdev->dev);
+	if (err < 0)
+		goto out_pm_dis;
+
 	/* disable the interrupt and clear the status */
 	writel(0, port->base + GPIO_IMR);
 	writel(~0, port->base + GPIO_ISR);
@@ -459,8 +484,8 @@ static int mxc_gpio_probe(struct platform_device *pdev)
 	if (err)
 		goto out_bgio;
 
-	port->gc.request = gpiochip_generic_request;
-	port->gc.free = gpiochip_generic_free;
+	port->gc.request = mxc_gpio_request;
+	port->gc.free = mxc_gpio_free;
 	port->gc.to_irq = mxc_gpio_to_irq;
 	port->gc.base = (pdev->id < 0) ? of_alias_get_id(np, "gpio") * 32 :
 					     pdev->id * 32;
@@ -482,6 +507,8 @@ static int mxc_gpio_probe(struct platform_device *pdev)
 		goto out_bgio;
 	}
 
+	irq_domain_set_pm_device(port->domain, &pdev->dev);
+
 	/* gpio-mxc can be a generic irq chip */
 	err = mxc_gpio_init_gc(port, irq_base);
 	if (err < 0)
@@ -490,9 +517,13 @@ static int mxc_gpio_probe(struct platform_device *pdev)
 	list_add_tail(&port->node, &mxc_gpio_ports);
 
 	platform_set_drvdata(pdev, port);
+	pm_runtime_put_autosuspend(&pdev->dev);
 
 	return 0;
 
+out_pm_dis:
+	pm_runtime_disable(&pdev->dev);
+	clk_disable_unprepare(port->clk);
 out_irqdomain_remove:
 	irq_domain_remove(port->domain);
 out_bgio:
@@ -572,6 +603,32 @@ static bool mxc_gpio_set_pad_wakeup(struct mxc_gpio_port *port, bool enable)
 	return ret;
 }
 
+static int __maybe_unused mxc_gpio_runtime_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct mxc_gpio_port *port = platform_get_drvdata(pdev);
+
+	mxc_gpio_save_regs(port);
+	clk_disable_unprepare(port->clk);
+
+	return 0;
+}
+
+static int __maybe_unused mxc_gpio_runtime_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct mxc_gpio_port *port = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = clk_prepare_enable(port->clk);
+	if (ret)
+		return ret;
+
+	mxc_gpio_restore_regs(port);
+
+	return 0;
+}
+
 static int __maybe_unused mxc_gpio_noirq_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
@@ -597,14 +654,19 @@ static int __maybe_unused mxc_gpio_noirq_resume(struct device *dev)
 
 static const struct dev_pm_ops mxc_gpio_dev_pm_ops = {
 	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mxc_gpio_noirq_suspend, mxc_gpio_noirq_resume)
+	SET_RUNTIME_PM_OPS(mxc_gpio_runtime_suspend, mxc_gpio_runtime_resume, NULL)
 };
 
 static int mxc_gpio_syscore_suspend(void)
 {
 	struct mxc_gpio_port *port;
+	int ret;
 
 	/* walk through all ports */
 	list_for_each_entry(port, &mxc_gpio_ports, node) {
+		ret = clk_prepare_enable(port->clk);
+		if (ret)
+			return ret;
 		mxc_gpio_save_regs(port);
 		clk_disable_unprepare(port->clk);
 	}
@@ -625,6 +687,7 @@ static void mxc_gpio_syscore_resume(void)
 			return;
 		}
 		mxc_gpio_restore_regs(port);
+		clk_disable_unprepare(port->clk);
 	}
 }
 
-- 
2.34.1

Re: [PATCH] gpio: mxc: add runtime pm support

[Andy Shevchenko]

On Thu, Jun 29, 2023 at 10:19 PM Shenwei Wang <shenwei.wang@nxp.com> wrote:
>
> Adds runtime PM support and allow the GPIO controller to enter
> into runtime suspend automatically when not in use to save power.
> However, it will automatically resume and enable clocks when a
> GPIO or IRQ is requested.

...

> +static int mxc_gpio_request(struct gpio_chip *chip, unsigned int offset)
> +{
> +       int ret;
> +
> +       ret = gpiochip_generic_request(chip, offset);
> +       if (ret)
> +               return ret;
> +
> +       ret = pm_runtime_get_sync(chip->parent);

reference count disbalance here.

> +       return ret < 0 ? ret : 0;
> +}
> +
> +static void mxc_gpio_free(struct gpio_chip *chip, unsigned int offset)
> +{
> +       gpiochip_generic_free(chip, offset);
> +       pm_runtime_put(chip->parent);
> +}

So, you want to have this to track the amount of GPIO lines requested, right?
Calling PM runtime after the first request makes little sense.

But here is the question: does your controller support wake from IRQ?

Have you tried to see if the lines that are used for IRQ with
gpiod_to_irq() really work with this?

...

> +       err = pm_runtime_get_sync(&pdev->dev);
> +       if (err < 0)

reference count leak here.

> +               goto out_pm_dis;


> +static int __maybe_unused mxc_gpio_runtime_suspend(struct device *dev)

Please, no __maybe_unused. Use new PM macros for that.

> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct mxc_gpio_port *port = platform_get_drvdata(pdev);

What's wrong with dev_get_drvdata()?

> +       mxc_gpio_save_regs(port);
> +       clk_disable_unprepare(port->clk);
> +
> +       return 0;
> +}
> +
> +static int __maybe_unused mxc_gpio_runtime_resume(struct device *dev)
> +{

Same comments as per above function.

> +}

...

Personal view on this is that it makes little sense to do and is prone
to subtle bugs with wake sources or other, not so obvious, uses of the
GPIO lines. Can you provide the numbers of the current dissipation if
the controller is on and no line is requested? Also is there any real
example of hardware (existing DTS) that has no GPIO in use?

-- 
With Best Regards,
Andy Shevchenko

RE: [EXT] Re: [PATCH] gpio: mxc: add runtime pm support

[Shenwei Wang]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Friday, June 30, 2023 4:19 AM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>; Bartosz Golaszewski
> <brgl@bgdev.pl>; Andy Shevchenko <andy@kernel.org>; linux-
> gpio@vger.kernel.org; imx@lists.linux.dev; dl-linux-imx <linux-imx@nxp.com>
> Subject: [EXT] Re: [PATCH] gpio: mxc: add runtime pm support
> > +static int mxc_gpio_request(struct gpio_chip *chip, unsigned int
> > +offset) {
> > +       int ret;
> > +
> > +       ret = gpiochip_generic_request(chip, offset);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = pm_runtime_get_sync(chip->parent);
> 
> reference count disbalance here.

Seems we shouldn't check the return value here and simply return 0.
Or should it be changed to " pm_runtime_resume_and_get" ?

> 
> > +       return ret < 0 ? ret : 0;
> > +}
> > +
> > +static void mxc_gpio_free(struct gpio_chip *chip, unsigned int
> > +offset) {
> > +       gpiochip_generic_free(chip, offset);
> > +       pm_runtime_put(chip->parent);
> > +}
> 
> So, you want to have this to track the amount of GPIO lines requested, right?
> Calling PM runtime after the first request makes little sense.
> 

Yes, that's the purpose. Once a GPIO line is requested, the GPIO controller
should be in active state. 

> But here is the question: does your controller support wake from IRQ?
> 
> Have you tried to see if the lines that are used for IRQ with
> gpiod_to_irq() really work with this?
> 

Yes, the controller supports wake from IRQ. This patch has been
Verified with the SDCARD plug-in/out use case which use a GPIO line as CD PIN.

> ...
> 
> > +       err = pm_runtime_get_sync(&pdev->dev);
> > +       if (err < 0)
> 
> reference count leak here.

Change to pm_runtime_resume_and_get?

> 
> > +               goto out_pm_dis;
> 
> 
> > +static int __maybe_unused mxc_gpio_runtime_suspend(struct device
> > +*dev)
> 
> Please, no __maybe_unused. Use new PM macros for that.
> 
> > +{
> > +       struct platform_device *pdev = to_platform_device(dev);
> > +       struct mxc_gpio_port *port = platform_get_drvdata(pdev);
> 
> What's wrong with dev_get_drvdata()?

Yes, dev_get_drvdata is better.

> 
> > +       mxc_gpio_save_regs(port);
> > +       clk_disable_unprepare(port->clk);
> > +
> > +       return 0;
> > +}
> > +
> > +static int __maybe_unused mxc_gpio_runtime_resume(struct device *dev)
> > +{
> 
> Same comments as per above function.
> 
> > +}
> 
> ...
> 
> Personal view on this is that it makes little sense to do and is prone to subtle
> bugs with wake sources or other, not so obvious, uses of the GPIO lines. Can you
> provide the numbers of the current dissipation if the controller is on and no line
> is requested? Also is there any real example of hardware (existing DTS) that has
> no GPIO in use?
> 

While putting the GPIO module itself into power saving mode may not have an obvious impact 
on current dissipation, the function is necessary because the GPIO module disables its clock when idle. 
This enables the system an opportunity to power off the parent subsystem, and this conserves more 
power. The typical i.MX8 SoC features up to 8 GPIO controllers, but most of the controllers often 
remain unused.

Thanks,
Shenwei

> --
> With Best Regards,
> Andy Shevchenko

Re: [EXT] Re: [PATCH] gpio: mxc: add runtime pm support

[Andy Shevchenko]

On Fri, Jun 30, 2023 at 05:01:10PM +0000, Shenwei Wang wrote:
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Sent: Friday, June 30, 2023 4:19 AM

...

> > > +       ret = pm_runtime_get_sync(chip->parent);
> > 
> > reference count disbalance here.
> 
> Seems we shouldn't check the return value here and simply return 0.
> Or should it be changed to " pm_runtime_resume_and_get" ?

It depends. I don't know the goal and what the case will be if PM fails
and you still go with that.

> > > +       return ret < 0 ? ret : 0;

...

> > But here is the question: does your controller support wake from IRQ?
> > 
> > Have you tried to see if the lines that are used for IRQ with
> > gpiod_to_irq() really work with this?
> 
> Yes, the controller supports wake from IRQ. This patch has been
> Verified with the SDCARD plug-in/out use case which use a GPIO line as CD PIN.

Yes, but in that case it has been probably requested. What I'm telling is when
the GPIO IRQ is went via IRQ chip and hence never been requested by GPIO.

...

> > > +       err = pm_runtime_get_sync(&pdev->dev);
> > > +       if (err < 0)
> > 
> > reference count leak here.
> 
> Change to pm_runtime_resume_and_get?

No idea. You know it better than me. See above.

> > > +               goto out_pm_dis;

...

> > Personal view on this is that it makes little sense to do and is prone to
> > subtle bugs with wake sources or other, not so obvious, uses of the GPIO
> > lines. Can you provide the numbers of the current dissipation if the
> > controller is on and no line is requested? Also is there any real example
> > of hardware (existing DTS) that has no GPIO in use?
> 
> While putting the GPIO module itself into power saving mode may not have an
> obvious impact on current dissipation, the function is necessary because the
> GPIO module disables its clock when idle.  This enables the system an
> opportunity to power off the parent subsystem, and this conserves more power.
> The typical i.MX8 SoC features up to 8 GPIO controllers, but most of the
> controllers often remain unused.

Maybe you need to summarize above in the commit message to improve your
selling point.

-- 
With Best Regards,
Andy Shevchenko

RE: [EXT] Re: [PATCH] gpio: mxc: add runtime pm support

[Shenwei Wang]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Friday, June 30, 2023 12:29 PM
> To: Shenwei Wang <shenwei.wang@nxp.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>; Bartosz Golaszewski
> <brgl@bgdev.pl>; linux-gpio@vger.kernel.org; imx@lists.linux.dev; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [EXT] Re: [PATCH] gpio: mxc: add runtime pm support
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report this
> email' button
> 
> 
> On Fri, Jun 30, 2023 at 05:01:10PM +0000, Shenwei Wang wrote:
> > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > Sent: Friday, June 30, 2023 4:19 AM
> 
> ...
> 
> > > > +       ret = pm_runtime_get_sync(chip->parent);
> > >
> > > reference count disbalance here.
> >
> > Seems we shouldn't check the return value here and simply return 0.
> > Or should it be changed to " pm_runtime_resume_and_get" ?
> 
> It depends. I don't know the goal and what the case will be if PM fails and you
> still go with that.
> 
> > > > +       return ret < 0 ? ret : 0;
> 
> ...
> 
> > > But here is the question: does your controller support wake from IRQ?
> > >
> > > Have you tried to see if the lines that are used for IRQ with
> > > gpiod_to_irq() really work with this?
> >
> > Yes, the controller supports wake from IRQ. This patch has been
> > Verified with the SDCARD plug-in/out use case which use a GPIO line as CD PIN.
> 
> Yes, but in that case it has been probably requested. What I'm telling is when the
> GPIO IRQ is went via IRQ chip and hence never been requested by GPIO.
> 

Just did a quick testing, and it still works even without requesting the GPIO line.
This is because we have specified the gpio controller as the pm_dev for the irq_chip in 
the probe function.

+	irq_domain_set_pm_device(port->domain, &pdev->dev);
+

Thanks,
Shenwei

> ...
> 
> > > > +       err = pm_runtime_get_sync(&pdev->dev);
> > > > +       if (err < 0)
> > >
> > > reference count leak here.
> >
> > Change to pm_runtime_resume_and_get?
> 
> No idea. You know it better than me. See above.
> 
> > > > +               goto out_pm_dis;
> 
> ...
> 
> > > Personal view on this is that it makes little sense to do and is
> > > prone to subtle bugs with wake sources or other, not so obvious,
> > > uses of the GPIO lines. Can you provide the numbers of the current
> > > dissipation if the controller is on and no line is requested? Also
> > > is there any real example of hardware (existing DTS) that has no GPIO in use?
> >
> > While putting the GPIO module itself into power saving mode may not
> > have an obvious impact on current dissipation, the function is
> > necessary because the GPIO module disables its clock when idle.  This
> > enables the system an opportunity to power off the parent subsystem, and this
> conserves more power.
> > The typical i.MX8 SoC features up to 8 GPIO controllers, but most of
> > the controllers often remain unused.
> 
> Maybe you need to summarize above in the commit message to improve your
> selling point.
> 
> --
> With Best Regards,
> Andy Shevchenko
>