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

2013-04-23 Thread Bibek Basu
On 23/04/2013 11:25 PM, Bibek Basu wrote:
> -Original Message-
> From: Linus Walleij [mailto:linus.wall...@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  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2013-04-03 Thread Linus Walleij
On Wed, Apr 3, 2013 at 7:16 PM, Stephen Warren  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2013-04-03 Thread Stephen Warren
On 04/03/2013 08:09 AM, Linus Walleij wrote:
> On Thu, Mar 28, 2013 at 6:48 PM, Stephen Warren  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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2013-04-03 Thread Linus Walleij
On Thu, Mar 28, 2013 at 6:11 PM, Bibek Basu  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2013-04-03 Thread Linus Walleij
On Thu, Mar 28, 2013 at 6:48 PM, Stephen Warren  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2013-04-01 Thread 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2013-03-31 Thread Bibek Basu
Hi Stephen,

My response inline.

Regards
Bibek


-Original Message-
From: Stephen Warren [mailto:swar...@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 
> 
> This patch adds suspend and resume callbacks to the pinctrl-tegra driver.
> 
> Based on work by:
> Pritesh Raithatha 

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 
> Signed-off-by: Bibek Basu 

> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

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

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 
> Signed-off-by: Bibek Basu 

> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2013-03-28 Thread Bibek Basu
From: Pritesh Raithatha 

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

Based on work by:
Pritesh Raithatha 
Signed-off-by: Pritesh Raithatha 
Signed-off-by: Bibek Basu 
---
 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 
 #include 
 #include 
+#include 
 
 #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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2012-11-01 Thread Stephen Warren
On 11/01/2012 09:53 AM, Dmitry Osipenko wrote:
> Added suspend/resume pm ops. We need to store current regs vals on suspend and
> restore them on resume.

Interesting. Just literally a couple days ago, I was reviewing a patch
to our internal kernel tree that implemented the same thing for the
pinctrl driver, in almost the same way!

> ---
> Tested on my tablet.

I'm curious how; in what environment. As far as I know, the Tegra
support in the mainline kernel doesn't actually support suspend/resume.
I assume you cherry-picked this pinctrl driver into some other kernel,
and tested this patch there?

> +#ifdef CONFIG_PM_SLEEP
> +static int tegra_pinctrl_suspend_noirq(struct device *dev)

The one major different between this patch and the downstream patch I
reviewed is how suspend/resume is triggered. This uses suspend_noirq,
whereas the downstream patch registers the callbacks using
register_syscore_ops(). Apparently the latter is required (at least in
our downstream kernel) in order to ensure that pinctrl gets suspended
after all other drivers.

I Cc'd Pritesh to comment on this.

Still, perhaps device probe ordering should ensure this upstream so
using register_syscore_ops() might not be necessary, although that
relies on drivers probing in the correct order, which they may not
without explicitly pinctrl_get() calls... back to that same problem again!

...
> +static const struct dev_pm_ops tegra_pinctrl_pm_ops = {
> + .suspend_noirq = tegra_pinctrl_suspend_noirq,
> + .resume_noirq = tegra_pinctrl_resume_noirq,
> +};
> +#define TEGRA_PINCTRL_PM (&tegra_pinctrl_pm_ops)
> +#else
> +#define TEGRA_PINCTRL_PM NULL
> +#endif

I was going to suggest using something like SET_SYSTEM_SLEEP_PM_OPS, but
I guess there's no equivalent for the _noirq variants.

> @@ -752,6 +838,8 @@ int __devinit tegra_pinctrl_probe(struct platform_device 
> *pdev,
>  
>   platform_set_drvdata(pdev, pmx);
>  
> + pdev->dev.driver->pm = TEGRA_PINCTRL_PM;

That might be better done in the struct platform_driver initialization
in pinctrl-tegra*.c?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2012-11-01 Thread Dmitry Osipenko
Added suspend/resume pm ops. We need to store current regs vals on suspend and
restore them on resume.

Signed-off-by: Dmitry Osipenko 
---
Tested on my tablet.

 drivers/pinctrl/pinctrl-tegra.c | 94 +++--
 1 file changed, 91 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-tegra.c b/drivers/pinctrl/pinctrl-tegra.c
index 7da0b37..03a3afb 100644
--- a/drivers/pinctrl/pinctrl-tegra.c
+++ b/drivers/pinctrl/pinctrl-tegra.c
@@ -41,6 +41,11 @@ struct tegra_pmx {
 
int nbanks;
void __iomem **regs;
+
+   int *bank_size;
+#ifdef CONFIG_PM_SLEEP
+   u32 *regs_store;
+#endif
 };
 
 static inline u32 pmx_readl(struct tegra_pmx *pmx, u32 bank, u32 reg)
@@ -685,11 +690,72 @@ static struct pinctrl_desc tegra_pinctrl_desc = {
.owner = THIS_MODULE,
 };
 
+#ifdef CONFIG_PM_SLEEP
+static int tegra_pinctrl_suspend_noirq(struct device *dev)
+{
+   struct tegra_pmx *pmx = dev_get_drvdata(dev);
+   int store_offset = 0;
+   int i, reg;
+   u32 val;
+
+   if (!pmx)
+   return -ENOMEM;
+
+   for (i = 0; i < pmx->nbanks; i++) {
+
+   for (reg = 0; reg < pmx->bank_size[i]; reg += 4) {
+   val = pmx_readl(pmx, i, reg);
+   pmx->regs_store[store_offset] = val;
+   store_offset++;
+
+   dev_dbg(dev, "stored val: 0x%x bank: %d reg: 0x%x\n",
+   val, i, reg);
+   }
+   }
+
+   return 0;
+}
+
+static int tegra_pinctrl_resume_noirq(struct device *dev)
+{
+   struct tegra_pmx *pmx = dev_get_drvdata(dev);
+   int store_offset = 0;
+   int i, reg;
+   u32 val;
+
+   if (!pmx)
+   return -ENOMEM;
+
+   for (i = 0; i < pmx->nbanks; i++) {
+
+   for (reg = 0; reg < pmx->bank_size[i]; reg += 4) {
+   val = pmx->regs_store[store_offset];
+   pmx_writel(pmx, val, i, reg);
+   store_offset++;
+
+   dev_dbg(dev, "restored val: 0x%x bank: %d reg: 0x%x\n",
+   val, i, reg);
+   }
+   }
+
+   return 0;
+}
+
+static const struct dev_pm_ops tegra_pinctrl_pm_ops = {
+   .suspend_noirq = tegra_pinctrl_suspend_noirq,
+   .resume_noirq = tegra_pinctrl_resume_noirq,
+};
+#define TEGRA_PINCTRL_PM   (&tegra_pinctrl_pm_ops)
+#else
+#define TEGRA_PINCTRL_PM   NULL
+#endif
+
 int __devinit tegra_pinctrl_probe(struct platform_device *pdev,
const struct tegra_pinctrl_soc_data *soc_data)
 {
struct tegra_pmx *pmx;
struct resource *res;
+   int nregs = 0;
int i;
 
pmx = devm_kzalloc(&pdev->dev, sizeof(*pmx), GFP_KERNEL);
@@ -712,6 +778,13 @@ int __devinit tegra_pinctrl_probe(struct platform_device 
*pdev,
}
pmx->nbanks = i;
 
+   pmx->bank_size = devm_kzalloc(&pdev->dev, pmx->nbanks * sizeof(int),
+ GFP_KERNEL);
+   if (!pmx->bank_size) {
+   dev_err(&pdev->dev, "Can't alloc banks sizes pointer\n");
+   return -ENODEV;
+   }
+
pmx->regs = devm_kzalloc(&pdev->dev, pmx->nbanks * sizeof(*pmx->regs),
 GFP_KERNEL);
if (!pmx->regs) {
@@ -726,22 +799,35 @@ int __devinit tegra_pinctrl_probe(struct platform_device 
*pdev,
return -ENODEV;
}
 
+   pmx->bank_size[i] = resource_size(res);
+
if (!devm_request_mem_region(&pdev->dev, res->start,
-   resource_size(res),
-   dev_name(&pdev->dev))) {
+pmx->bank_size[i],
+dev_name(&pdev->dev))) {
dev_err(&pdev->dev,
"Couldn't request MEM resource %d\n", i);
return -ENODEV;
}
 
pmx->regs[i] = devm_ioremap(&pdev->dev, res->start,
-   resource_size(res));
+   pmx->bank_size[i]);
if (!pmx->regs[i]) {
dev_err(&pdev->dev, "Couldn't ioremap regs %d\n", i);
return -ENODEV;
}
+
+   nregs += pmx->bank_size[i] / 4;
}
 
+#ifdef CONFIG_PM_SLEEP
+   pmx->regs_store = devm_kzalloc(&pdev->dev, nregs * sizeof(u32),
+  GFP_KERNEL);
+   if (!pmx->regs_store) {
+   dev_err(&pdev->dev, "Can't alloc regs store pointer\n");
+   return -ENODEV;
+   }
+#endif
+
pmx->pctl = pinctrl_register(&tegra_pinctrl_desc, &pdev->dev, pmx);
if (!pmx->pctl) {
dev_err(&pdev->dev, "Couldn't register pinctrl driver\n");
@@