[PATCH] pinctrl: tegra: add suspend-resume support

[PATCH] pinctrl: tegra: add suspend-resume support

[Bibek Basu]

From: Pritesh Raithatha <praithatha@nvidia.com>

This patch adds suspend and resume callbacks to the pinctrl-tegra driver.

Based on work by:
Pritesh Raithatha <praithatha@nvidia.com>
Signed-off-by: Pritesh Raithatha <praithatha@nvidia.com>
Signed-off-by: Bibek Basu <bbasu@nvidia.com>
---
 drivers/pinctrl/pinctrl-tegra.c | 71 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 68 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-tegra.c b/drivers/pinctrl/pinctrl-tegra.c
index f195d77..c789326 100644
--- a/drivers/pinctrl/pinctrl-tegra.c
+++ b/drivers/pinctrl/pinctrl-tegra.c
@@ -29,6 +29,7 @@
 #include <linux/pinctrl/pinmux.h>
 #include <linux/pinctrl/pinconf.h>
 #include <linux/slab.h>
+#include <linux/syscore_ops.h>
 
 #include "core.h"
 #include "pinctrl-tegra.h"
@@ -41,8 +42,13 @@ struct tegra_pmx {
 
 	int nbanks;
 	void __iomem **regs;
+	int *regs_size;
+
+	u32 *pg_data;
 };
 
+static struct tegra_pmx *pmx;
+
 static inline u32 pmx_readl(struct tegra_pmx *pmx, u32 bank, u32 reg)
 {
 	return readl(pmx->regs[bank] + reg);
@@ -701,12 +707,47 @@ static struct pinctrl_desc tegra_pinctrl_desc = {
 	.owner = THIS_MODULE,
 };
 
+#ifdef CONFIG_PM_SLEEP
+
+static int pinctrl_suspend(void)
+{
+	int i, j;
+	u32 *pg_data = pmx->pg_data;
+	u32 *regs;
+
+	for (i = 0; i < pmx->nbanks; i++) {
+		regs = pmx->regs[i];
+		for (j = 0; j < pmx->regs_size[i] / 4; j++)
+			*pg_data++ = readl(regs++);
+	}
+	return 0;
+}
+
+static void pinctrl_resume(void)
+{
+	int i, j;
+	u32 *pg_data = pmx->pg_data;
+	u32 *regs;
+
+	for (i = 0; i < pmx->nbanks; i++) {
+		regs = pmx->regs[i];
+		for (j = 0; j < pmx->regs_size[i] / 4; j++)
+			writel(*pg_data++, regs++);
+	}
+}
+
+static struct syscore_ops pinctrl_syscore_ops = {
+	.suspend = pinctrl_suspend,
+	.resume = pinctrl_resume,
+};
+
+#endif
+
 int tegra_pinctrl_probe(struct platform_device *pdev,
 			const struct tegra_pinctrl_soc_data *soc_data)
-{
-	struct tegra_pmx *pmx;
+	{
 	struct resource *res;
-	int i;
+	int i, pg_data_size = 0;
 
 	pmx = devm_kzalloc(&pdev->dev, sizeof(*pmx), GFP_KERNEL);
 	if (!pmx) {
@@ -725,6 +766,7 @@ int tegra_pinctrl_probe(struct platform_device *pdev,
 		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
 		if (!res)
 			break;
+		pg_data_size += resource_size(res);
 	}
 	pmx->nbanks = i;
 
@@ -735,6 +777,22 @@ int tegra_pinctrl_probe(struct platform_device *pdev,
 		return -ENODEV;
 	}
 
+#ifdef CONFIG_PM_SLEEP
+	pmx->regs_size = devm_kzalloc(&pdev->dev,
+				pmx->nbanks * sizeof(*(pmx->regs_size)),
+				GFP_KERNEL);
+	if (!pmx->regs_size) {
+		dev_err(&pdev->dev, "Can't alloc regs pointer\n");
+		return -ENODEV;
+	}
+
+	pmx->pg_data = devm_kzalloc(&pdev->dev, pg_data_size, GFP_KERNEL);
+	if (!pmx->pg_data) {
+		dev_err(&pdev->dev, "Can't alloc pingroup data pointer\n");
+		return -ENODEV;
+	}
+#endif
+
 	for (i = 0; i < pmx->nbanks; i++) {
 		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
 		if (!res) {
@@ -756,6 +814,10 @@ int tegra_pinctrl_probe(struct platform_device *pdev,
 			dev_err(&pdev->dev, "Couldn't ioremap regs %d\n", i);
 			return -ENODEV;
 		}
+
+#ifdef CONFIG_PM_SLEEP
+		pmx->regs_size[i] = resource_size(res);
+#endif
 	}
 
 	pmx->pctl = pinctrl_register(&tegra_pinctrl_desc, &pdev->dev, pmx);
@@ -768,6 +830,9 @@ int tegra_pinctrl_probe(struct platform_device *pdev,
 
 	platform_set_drvdata(pdev, pmx);
 
+#ifdef CONFIG_PM_SLEEP
+	register_syscore_ops(&pinctrl_syscore_ops);
+#endif
 	dev_dbg(&pdev->dev, "Probed Tegra pinctrl driver\n");
 
 	return 0;
-- 
1.8.1.5

Re: [PATCH] pinctrl: tegra: add suspend-resume support

[Stephen Warren]

On 03/28/2013 11:11 AM, Bibek Basu wrote:
> From: Pritesh Raithatha <praithatha@nvidia.com>
> 
> This patch adds suspend and resume callbacks to the pinctrl-tegra driver.
> 
> Based on work by:
> Pritesh Raithatha <praithatha@nvidia.com>

Those two lines are somewhat implied by the fact the commit's git author
field is Pritesh, and his s-o-b line is included below.

> Signed-off-by: Pritesh Raithatha <praithatha@nvidia.com>
> Signed-off-by: Bibek Basu <bbasu@nvidia.com>

> diff --git a/drivers/pinctrl/pinctrl-tegra.c b/drivers/pinctrl/pinctrl-tegra.c

> +static struct tegra_pmx *pmx;

Isn't there any way to pass data into the suspend/resume functions so
that this global isn't needed?

Why can't we just use the device suspend/resume functions rather than
global (syscore) suspend/resume functions? Presumably this is to ensure
that all other drivers suspend first, then the pinctrl driver does, and
the reverse for resume. Can't we rely on deferred probe to ensure that
instead?

To make that work, we might need every affected driver to define a dummy
pinmux state in DT that references the pinctrl driver, to make sure they
all get probed after the pinctrl driver.

> +#ifdef CONFIG_PM_SLEEP

Perhaps we can remove the ifdefs, and always compile this code. The
compiler should remove any unused functions from the final linked image.

>  int tegra_pinctrl_probe(struct platform_device *pdev,
>  			const struct tegra_pinctrl_soc_data *soc_data)
> -{
> -	struct tegra_pmx *pmx;
> +	{

The indentation of the { is wrong there.

> +#ifdef CONFIG_PM_SLEEP

I think you can use if (IS_ENABLED(CONFIG_PM_SLEEP)) here instead of an
ifdef. This will allow the code to be compiled in all cases (to allow
better compile coverage), but it will be optimized out if
CONFIG_PM_SLEEP isn't enabled. Can you please make that change, and
verify that it works?

The same comment applies to the other ifdefs below.

RE: [PATCH] pinctrl: tegra: add suspend-resume support

[Bibek Basu]

Hi Stephen,

My response inline.

Regards
Bibek


-----Original Message-----
From: Stephen Warren [mailto:swarren@wwwdotorg.org] 
Sent: Thursday, March 28, 2013 11:19 PM
To: Bibek Basu
Cc: Linus Walleij; linux-kernel@vger.kernel.org; Pritesh Raithatha
Subject: Re: [PATCH] pinctrl: tegra: add suspend-resume support

On 03/28/2013 11:11 AM, Bibek Basu wrote:
> From: Pritesh Raithatha <praithatha@nvidia.com>
> 
> This patch adds suspend and resume callbacks to the pinctrl-tegra driver.
> 
> Based on work by:
> Pritesh Raithatha <praithatha@nvidia.com>

Those two lines are somewhat implied by the fact the commit's git author field is Pritesh, and his s-o-b line is included below.
[BB]: Will get rid of the above lines.

> Signed-off-by: Pritesh Raithatha <praithatha@nvidia.com>
> Signed-off-by: Bibek Basu <bbasu@nvidia.com>

> diff --git a/drivers/pinctrl/pinctrl-tegra.c 
> b/drivers/pinctrl/pinctrl-tegra.c

> +static struct tegra_pmx *pmx;

Isn't there any way to pass data into the suspend/resume functions so that this global isn't needed?

Why can't we just use the device suspend/resume functions rather than global (syscore) suspend/resume functions? Presumably this is to ensure that all other drivers suspend first, then the pinctrl driver does, and the reverse for resume. Can't we rely on deferred probe to ensure that instead?

To make that work, we might need every affected driver to define a dummy pinmux state in DT that references the pinctrl driver, to make sure they all get probed after the pinctrl driver.
[BB]: Before I add dummy pinmux state in DT of affected driver, I would like to know the following:
    1> The usage of syscore api needs  global data. So, are you suggesting that syscore APIs are not appropriate to be used or syscore implementation is not proper?
    2> Adding dummy DT states may give scope for BUGS. Reason being there must be someone checking that every time some new driver refrences pinmux driver, should put a dummy entry in device tree. Isnt it more time costing   then having "static struct tegra_pmx *pmx;"?

> +#ifdef CONFIG_PM_SLEEP

Perhaps we can remove the ifdefs, and always compile this code. The compiler should remove any unused functions from the final linked image.
[BB]: Will do

>  int tegra_pinctrl_probe(struct platform_device *pdev,
>  			const struct tegra_pinctrl_soc_data *soc_data) -{
> -	struct tegra_pmx *pmx;
> +	{

The indentation of the { is wrong there.
[BB]: Will fix

> +#ifdef CONFIG_PM_SLEEP

I think you can use if (IS_ENABLED(CONFIG_PM_SLEEP)) here instead of an ifdef. This will allow the code to be compiled in all cases (to allow better compile coverage), but it will be optimized out if CONFIG_PM_SLEEP isn't enabled. Can you please make that change, and verify that it works?
[BB]: Will do.

The same comment applies to the other ifdefs below.

Re: [PATCH] pinctrl: tegra: add suspend-resume support

[Stephen Warren]

On 03/31/2013 10:34 PM, Bibek Basu wrote:
> Hi Stephen,
> 
> My response inline.

Upstream, responses should always be inline, so there's no need to
mention that.

By the way, can you please configure your email client to:

a) Not re-wrap the email you're replying to; email should be wrapped to
about 74 characters.

b) Prefix all the lines you're quoting with "> " so that we can
differentiate the text you quoted from the text you wrote. If you need
to write "[BB]" to differentiate those pieces of text, something is
wrong. Thanks.

I've tried to fix these below in my reply.

> Stephen Warren wrote at Thursday, March 28, 2013 11:19 PM:
> > On 03/28/2013 11:11 AM, Bibek Basu wrote:
> >> This patch adds suspend and resume callbacks to the pinctrl-tegra driver.

> >> diff --git a/drivers/pinctrl/pinctrl-tegra.c 
> >> b/drivers/pinctrl/pinctrl-tegra.c
> > 
> >> +static struct tegra_pmx *pmx;
> > 
> > Isn't there any way to pass data into the suspend/resume functions so that this global isn't needed?
> > 
> > Why can't we just use the device suspend/resume functions rather than
> > global (syscore) suspend/resume functions? Presumably this is to
> > ensure that all other drivers suspend first, then the pinctrl driver
> > does, and the reverse for resume. Can't we rely on deferred probe to
> >  ensure that instead?
> > 
> > To make that work, we might need every affected driver to define a
> > dummy pinmux state in DT that references the pinctrl driver, to make
> > sure they all get probed after the pinctrl driver.
>
> [BB]: Before I add dummy pinmux state in DT of affected driver, I would
> like to know the following:
>
> 1> The usage of syscore api needs global data. So, are you suggesting
> that syscore APIs are not appropriate to be used or syscore
> implementation is not proper?

Yes. The code here is a driver, and drivers shouldn't be using global
data where possible.

> 2> Adding dummy DT states may give scope for BUGS. Reason being there
> must be someone checking that every time some new driver refrences
> pinmux driver, should put a dummy entry in device tree. Isnt it more
> time costing then having "static struct tegra_pmx *pmx;"?

The overhead isn't high, and fixing that particular bug is trivial. And
yes, sometimes doing things the right way is more work than a
quick-and-dirty solution.

Re: [PATCH] pinctrl: tegra: add suspend-resume support

[Linus Walleij]

On Thu, Mar 28, 2013 at 6:48 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:

> Why can't we just use the device suspend/resume functions rather than
> global (syscore) suspend/resume functions? Presumably this is to ensure
> that all other drivers suspend first, then the pinctrl driver does, and
> the reverse for resume. Can't we rely on deferred probe to ensure that
> instead?
>
> To make that work, we might need every affected driver to define a dummy
> pinmux state in DT that references the pinctrl driver, to make sure they
> all get probed after the pinctrl driver.

Hm that reminds me of that policy change I suggested a while back to
do this instead of using hogs where possible.

It has the nice side-effect that when we inspect the debugfs info
all pins will be properly owned by respective consuming device.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: tegra: add suspend-resume support

[Stephen Warren]

On 04/03/2013 08:09 AM, Linus Walleij wrote:
> On Thu, Mar 28, 2013 at 6:48 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> 
>> Why can't we just use the device suspend/resume functions rather than
>> global (syscore) suspend/resume functions? Presumably this is to ensure
>> that all other drivers suspend first, then the pinctrl driver does, and
>> the reverse for resume. Can't we rely on deferred probe to ensure that
>> instead?
>>
>> To make that work, we might need every affected driver to define a dummy
>> pinmux state in DT that references the pinctrl driver, to make sure they
>> all get probed after the pinctrl driver.
> 
> Hm that reminds me of that policy change I suggested a while back to
> do this instead of using hogs where possible.
> 
> It has the nice side-effect that when we inspect the debugfs info
> all pins will be properly owned by respective consuming device.

True, in theory that would also work.

However, in practice with Tegra's pinmux, it has to all be set up at
once to avoid any conflicts, so hogging is really the only practical way
to use it in most cases.

This is because in many cases, a single controller could have its
signals routed out to many different pins (or sets of pins), rather than
just having one possible location where each controller could be routed
to. In other words, the pinmux is m:n rather than m:1.

It's possible program the registers so that the same signal is connected
to (or from depending on signal direction) multiple pins at once. If
this is done, the behaviour is unspecified; who knows which pin will
actually receive (or provide) that signal?

This can easily happen if the whole pinmux is not initialized fully in
one pass, i.e. through hogs. For example, the HW default may be for e.g.
UART1 to get routed to pins A, and B, whereas a particular board may
assume that UART1 is routed to pins X, Y. So, SW must program pins X, Y
to mux in UART1. However, if pins A, B aren't also re-programmed away
from the UART1 option, then UART 1 on X, Y may not actually work. In
this case, we can't rely on some other driver having
acquire/re-programmed pins A, B, unless it's the hog of the pin
controller itself. Hence, the only sensible solution is for the pin
controller to hog absolutely everything.

The only exception would be for dynamic pin-muxing (e.g. pinctrl-based
I2C muxing), where hopefully everything is chosen carefully to avoid
this kind of issue.

Re: [PATCH] pinctrl: tegra: add suspend-resume support

[Linus Walleij]

On Wed, Apr 3, 2013 at 7:16 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:

> It's possible program the registers so that the same signal is connected
> to (or from depending on signal direction) multiple pins at once. If
> this is done, the behaviour is unspecified; who knows which pin will
> actually receive (or provide) that signal?

In the dbx500 this will actually make the signal come out on both
pins. So there are several muxes connected to the same internal
rail. I can imagine a few odd ways that would work differently though...

We could use this to connect two UARTs to the console, not very
useful, but hey, dual screens...

I wonder what happens if both consoles hit enter at the same instant.
Probably nasty electronic states are possible.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: tegra: add suspend-resume support

[Linus Walleij]

On Thu, Mar 28, 2013 at 6:11 PM, Bibek Basu <bbasu@nvidia.com> wrote:

Hm I recognize this name :-)

> This patch adds suspend and resume callbacks to the pinctrl-tegra driver.

Please be more verbose. How is this achieved? I have to
guess what the code is doing..

> +#ifdef CONFIG_PM_SLEEP
> +
> +static int pinctrl_suspend(void)
> +{
> +       int i, j;
> +       u32 *pg_data = pmx->pg_data;
> +       u32 *regs;
> +
> +       for (i = 0; i < pmx->nbanks; i++) {
> +               regs = pmx->regs[i];
> +               for (j = 0; j < pmx->regs_size[i] / 4; j++)
> +                       *pg_data++ = readl(regs++);
> +       }
> +       return 0;
> +}
> +
> +static void pinctrl_resume(void)
> +{
> +       int i, j;
> +       u32 *pg_data = pmx->pg_data;
> +       u32 *regs;
> +
> +       for (i = 0; i < pmx->nbanks; i++) {
> +               regs = pmx->regs[i];
> +               for (j = 0; j < pmx->regs_size[i] / 4; j++)
> +                       writel(*pg_data++, regs++);
> +       }
> +}
> +
> +static struct syscore_ops pinctrl_syscore_ops = {
> +       .suspend = pinctrl_suspend,
> +       .resume = pinctrl_resume,
> +};
> +
> +#endif
(...)
> +#ifdef CONFIG_PM_SLEEP
> +       register_syscore_ops(&pinctrl_syscore_ops);
> +#endif

So Stephen already commented that syscore ops is maybe too big
a sledgehammer for a fine-granular problem.

I mainly want to know what is happening above, it looks like
a state save/restore for all registers or something like this?

Yours,
Linus Walleij

RE: [PATCH] pinctrl: tegra: add suspend-resume support

[Bibek Basu]

On 23/04/2013 11:25 PM, Bibek Basu wrote:
> -----Original Message-----
> From: Linus Walleij [mailto:linus.walleij@linaro.org]
> Sent: Wednesday, April 03, 2013 9:52 PM
> To: Bibek Basu
> Cc: linux-kernel@vger.kernel.org; Pritesh Raithatha
> Subject: Re: [PATCH] pinctrl: tegra: add suspend-resume support
> 
> On Thu, Mar 28, 2013 at 6:11 PM, Bibek Basu <bbasu@nvidia.com> wrote:
> 
> Hm I recognize this name :-)
> 
You recognized me correctly. After all it's a small world:-)
> > This patch adds suspend and resume callbacks to the pinctrl-tegra driver.
> 
> Please be more verbose. How is this achieved? I have to guess what the code
> is doing..
> 
> > +#ifdef CONFIG_PM_SLEEP
> > +
> > +static int pinctrl_suspend(void)
> > +{
> > +       int i, j;
> > +       u32 *pg_data = pmx->pg_data;
> > +       u32 *regs;
> > +
> > +       for (i = 0; i < pmx->nbanks; i++) {
> > +               regs = pmx->regs[i];
> > +               for (j = 0; j < pmx->regs_size[i] / 4; j++)
> > +                       *pg_data++ = readl(regs++);
> > +       }
> > +       return 0;
> > +}
> > +
> > +static void pinctrl_resume(void)
> > +{
> > +       int i, j;
> > +       u32 *pg_data = pmx->pg_data;
> > +       u32 *regs;
> > +
> > +       for (i = 0; i < pmx->nbanks; i++) {
> > +               regs = pmx->regs[i];
> > +               for (j = 0; j < pmx->regs_size[i] / 4; j++)
> > +                       writel(*pg_data++, regs++);
> > +       }
> > +}
> > +
> > +static struct syscore_ops pinctrl_syscore_ops = {
> > +       .suspend = pinctrl_suspend,
> > +       .resume = pinctrl_resume,
> > +};
> > +
> > +#endif
> (...)
> > +#ifdef CONFIG_PM_SLEEP
> > +       register_syscore_ops(&pinctrl_syscore_ops);
> > +#endif
> 
> So Stephen already commented that syscore ops is maybe too big a
> sledgehammer for a fine-granular problem.
> 
> I mainly want to know what is happening above, it looks like a state
> save/restore for all registers or something like this?
> 
Yeah, patch mainly saves and restores the registers.
I am re-pushing the patch after incorporating stephen's changes.

regards
Bibek
> Yours,
> Linus Walleij