[PATCH 0/6] Fix suspend/resume issues created by pinmux on exynos, part 2

[PATCH 0/6] Fix suspend/resume issues created by pinmux on exynos, part 2

[Tomasz Figa]

This series aims at fixing problems with suspend/resume on Exynos SoCs
introduced by migration of GPIO and EINT support to pin control driver
on DT-enabled platforms.

The patches fix following issues:
 - missing support for IRQ wake of pinctrl-exynos driver
 - legacy GPIO and EINT save/restore handling in platform PM code
   inappropriate on pinctrl-enabled platforms
 - several EINT-related registers must be saved as well
 
The series is based on a patch by Doug Anderson that adds suspend/restore
of pinctrl registers to pinctrl-samsung driver:
[PATCH v3] pinctrl: samsung: fix suspend/resume functionality
http://www.spinics.net/lists/kernel/msg1534119.html

On Exynos4210-based Trats board:
Tested-by: Tomasz Figa <t.figa@samsung.com>

Tomasz Figa (6):
  pinctrl: exynos: Add support for set_irq_wake of wake-up EINTs
  ARM: EXYNOS: Fix EINT wake-up mask configuration when pinctrl is used
  ARM: SAMSUNG: pm: Adjust for pinctrl- and DT-enabled platforms
  pinctrl: samsung: Add support for SoC-specific suspend/resume
    callbacks
  pinctrl: samsung: Allow per-bank SoC-specific private data
  pinctrl: exynos: Handle suspend/resume of GPIO EINT registers

 arch/arm/mach-exynos/include/mach/pm-core.h |  14 +++-
 arch/arm/plat-samsung/pm.c                  |  17 +++--
 drivers/pinctrl/pinctrl-exynos.c            | 106 ++++++++++++++++++++++++++++
 drivers/pinctrl/pinctrl-exynos.h            |   1 +
 drivers/pinctrl/pinctrl-samsung.c           |   6 ++
 drivers/pinctrl/pinctrl-samsung.h           |   4 ++
 6 files changed, 142 insertions(+), 6 deletions(-)

-- 
1.8.2.1

[PATCH 1/6] pinctrl: exynos: Add support for set_irq_wake of wake-up EINTs

[Tomasz Figa]

This patch adds support of IRQ wake-up ability configuration for
wake-up EINTs on Exynos SoCs.

Signed-off-by: Tomasz Figa <t.figa@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/pinctrl/pinctrl-exynos.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-exynos.c b/drivers/pinctrl/pinctrl-exynos.c
index ac74281..4f868e5 100644
--- a/drivers/pinctrl/pinctrl-exynos.c
+++ b/drivers/pinctrl/pinctrl-exynos.c
@@ -326,6 +326,28 @@ static int exynos_wkup_irq_set_type(struct irq_data *irqd, unsigned int type)
 	return 0;
 }
 
+static u32 exynos_eint_wake_mask = 0xffffffff;
+
+u32 exynos_get_eint_wake_mask(void)
+{
+	return exynos_eint_wake_mask;
+}
+
+static int exynos_wkup_irq_set_wake(struct irq_data *irqd, unsigned int on)
+{
+	struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd);
+	unsigned long bit = 1UL << (2 * bank->eint_offset + irqd->hwirq);
+
+	pr_info("wake %s for irq %d\n", on ? "enabled" : "disabled", irqd->irq);
+
+	if (!on)
+		exynos_eint_wake_mask |= bit;
+	else
+		exynos_eint_wake_mask &= ~bit;
+
+	return 0;
+}
+
 /*
  * irq_chip for wakeup interrupts
  */
@@ -335,6 +357,7 @@ static struct irq_chip exynos_wkup_irq_chip = {
 	.irq_mask	= exynos_wkup_irq_mask,
 	.irq_ack	= exynos_wkup_irq_ack,
 	.irq_set_type	= exynos_wkup_irq_set_type,
+	.irq_set_wake	= exynos_wkup_irq_set_wake,
 };
 
 /* interrupt handler for wakeup interrupts 0..15 */
-- 
1.8.2.1

[PATCH 1/6] pinctrl: exynos: Add support for set_irq_wake of wake-up EINTs

[Doug Anderson]

Tomasz,

On Fri, May 17, 2013 at 9:24 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> This patch adds support of IRQ wake-up ability configuration for
> wake-up EINTs on Exynos SoCs.
>
> Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/pinctrl/pinctrl-exynos.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)

On exynos5250-snow (pinmux backported to 3.8):

Tested-by: Doug Anderson <dianders@chromium.org>

Reviewed-by: Doug Anderson <dianders@chromium.org>

[PATCH 1/6] pinctrl: exynos: Add support for set_irq_wake of wake-up EINTs

[Linus Walleij]

On Fri, May 17, 2013 at 6:24 PM, Tomasz Figa <t.figa@samsung.com> wrote:

> This patch adds support of IRQ wake-up ability configuration for
> wake-up EINTs on Exynos SoCs.
>
> Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

Patch applied on my fixes branch for v3.10

Yours,
Linus Walleij

[PATCH 2/6] ARM: EXYNOS: Fix EINT wake-up mask configuration when pinctrl is used

[Tomasz Figa]

On DT-enabled systems pinctrl-exynos driver is responsible for handling
of wake-up EINT interrupts. This patch adjusts wake-up mask
configuration code to take wake-up mask value from pinctrl-exynos driver
on DT-enabled systems.

Signed-off-by: Tomasz Figa <t.figa@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/mach-exynos/include/mach/pm-core.h | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-exynos/include/mach/pm-core.h b/arch/arm/mach-exynos/include/mach/pm-core.h
index 7dbbfec..296090e 100644
--- a/arch/arm/mach-exynos/include/mach/pm-core.h
+++ b/arch/arm/mach-exynos/include/mach/pm-core.h
@@ -18,8 +18,15 @@
 #ifndef __ASM_ARCH_PM_CORE_H
 #define __ASM_ARCH_PM_CORE_H __FILE__
 
+#include <linux/of.h>
 #include <mach/regs-pmu.h>
 
+#ifdef CONFIG_PINCTRL_EXYNOS
+extern u32 exynos_get_eint_wake_mask(void);
+#else
+static inline u32 exynos_get_eint_wake_mask(void) { return 0xffffffff; }
+#endif
+
 static inline void s3c_pm_debug_init_uart(void)
 {
 	/* nothing here yet */
@@ -27,7 +34,12 @@ static inline void s3c_pm_debug_init_uart(void)
 
 static inline void s3c_pm_arch_prepare_irqs(void)
 {
-	__raw_writel(s3c_irqwake_eintmask, S5P_EINT_WAKEUP_MASK);
+	u32 eintmask = s3c_irqwake_eintmask;
+
+	if (of_have_populated_dt())
+		eintmask = exynos_get_eint_wake_mask();
+
+	__raw_writel(eintmask, S5P_EINT_WAKEUP_MASK);
 	__raw_writel(s3c_irqwake_intmask & ~(1 << 31), S5P_WAKEUP_MASK);
 }
 
-- 
1.8.2.1

[PATCH 2/6] ARM: EXYNOS: Fix EINT wake-up mask configuration when pinctrl is used

[Doug Anderson]

Tomasz,

On Fri, May 17, 2013 at 9:24 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> On DT-enabled systems pinctrl-exynos driver is responsible for handling
> of wake-up EINT interrupts. This patch adjusts wake-up mask
> configuration code to take wake-up mask value from pinctrl-exynos driver
> on DT-enabled systems.
>
> Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  arch/arm/mach-exynos/include/mach/pm-core.h | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)

A little ugly because of the need to support old non-device tree
boards, but seems reasonable.  Means that if you have a device tree
you'd better be using pincontrol.  Assuming that's true now someone
needs to go through and remove all of the device tree support (and
bindings Documentation) for gpio-samsung.  Maybe someone has already
started?


On exynos5250-snow (pinmux backported to 3.8):

Tested-by: Doug Anderson <dianders@chromium.org>

Reviewed-by: Doug Anderson <dianders@chromium.org>

On Fri, May 17, 2013 at 9:24 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> On DT-enabled systems pinctrl-exynos driver is responsible for handling
> of wake-up EINT interrupts. This patch adjusts wake-up mask
> configuration code to take wake-up mask value from pinctrl-exynos driver
> on DT-enabled systems.
>
> Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  arch/arm/mach-exynos/include/mach/pm-core.h | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-exynos/include/mach/pm-core.h b/arch/arm/mach-exynos/include/mach/pm-core.h
> index 7dbbfec..296090e 100644
> --- a/arch/arm/mach-exynos/include/mach/pm-core.h
> +++ b/arch/arm/mach-exynos/include/mach/pm-core.h
> @@ -18,8 +18,15 @@
>  #ifndef __ASM_ARCH_PM_CORE_H
>  #define __ASM_ARCH_PM_CORE_H __FILE__
>
> +#include <linux/of.h>
>  #include <mach/regs-pmu.h>
>
> +#ifdef CONFIG_PINCTRL_EXYNOS
> +extern u32 exynos_get_eint_wake_mask(void);
> +#else
> +static inline u32 exynos_get_eint_wake_mask(void) { return 0xffffffff; }
> +#endif
> +
>  static inline void s3c_pm_debug_init_uart(void)
>  {
>         /* nothing here yet */
> @@ -27,7 +34,12 @@ static inline void s3c_pm_debug_init_uart(void)
>
>  static inline void s3c_pm_arch_prepare_irqs(void)
>  {
> -       __raw_writel(s3c_irqwake_eintmask, S5P_EINT_WAKEUP_MASK);
> +       u32 eintmask = s3c_irqwake_eintmask;
> +
> +       if (of_have_populated_dt())
> +               eintmask = exynos_get_eint_wake_mask();
> +
> +       __raw_writel(eintmask, S5P_EINT_WAKEUP_MASK);
>         __raw_writel(s3c_irqwake_intmask & ~(1 << 31), S5P_WAKEUP_MASK);
>  }
>
> --
> 1.8.2.1
>

[PATCH 2/6] ARM: EXYNOS: Fix EINT wake-up mask configuration when pinctrl is used

[Tomasz Figa]

Hi Doug,

On Friday 17 of May 2013 12:22:36 Doug Anderson wrote:
> Tomasz,
> 
> On Fri, May 17, 2013 at 9:24 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> > On DT-enabled systems pinctrl-exynos driver is responsible for
> > handling
> > of wake-up EINT interrupts. This patch adjusts wake-up mask
> > configuration code to take wake-up mask value from pinctrl-exynos
> > driver on DT-enabled systems.
> > 
> > Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> > 
> >  arch/arm/mach-exynos/include/mach/pm-core.h | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> A little ugly

Hehe. I thought exactly the same, but I don't see any better solution at 
the moment, except persuading the whole suspend/resume support in plat-
samsung to be a friend of multiplatform, which will have to be done 
eventually, but at the moment I don't have time to work on it.

> because of the need to support old non-device tree
> boards, but seems reasonable.  Means that if you have a device tree
> you'd better be using pincontrol.  Assuming that's true now someone
> needs to go through and remove all of the device tree support (and
> bindings Documentation) for gpio-samsung.  Maybe someone has already
> started?

If I remember correctly Sylwester Nawrocki had some patches to remove 
that. He's on a leave right now, so he won't send them for a while, but I 
guess it's nothing urgent.

> On exynos5250-snow (pinmux backported to 3.8):
> 
> Tested-by: Doug Anderson <dianders@chromium.org>
> 
> Reviewed-by: Doug Anderson <dianders@chromium.org>

Thanks.

Best regards,
Tomasz

> On Fri, May 17, 2013 at 9:24 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> > On DT-enabled systems pinctrl-exynos driver is responsible for
> > handling
> > of wake-up EINT interrupts. This patch adjusts wake-up mask
> > configuration code to take wake-up mask value from pinctrl-exynos
> > driver on DT-enabled systems.
> > 
> > Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> > 
> >  arch/arm/mach-exynos/include/mach/pm-core.h | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/mach-exynos/include/mach/pm-core.h
> > b/arch/arm/mach-exynos/include/mach/pm-core.h index 7dbbfec..296090e
> > 100644
> > --- a/arch/arm/mach-exynos/include/mach/pm-core.h
> > +++ b/arch/arm/mach-exynos/include/mach/pm-core.h
> > @@ -18,8 +18,15 @@
> > 
> >  #ifndef __ASM_ARCH_PM_CORE_H
> >  #define __ASM_ARCH_PM_CORE_H __FILE__
> > 
> > +#include <linux/of.h>
> > 
> >  #include <mach/regs-pmu.h>
> > 
> > +#ifdef CONFIG_PINCTRL_EXYNOS
> > +extern u32 exynos_get_eint_wake_mask(void);
> > +#else
> > +static inline u32 exynos_get_eint_wake_mask(void) { return
> > 0xffffffff; } +#endif
> > +
> > 
> >  static inline void s3c_pm_debug_init_uart(void)
> >  {
> >  
> >         /* nothing here yet */
> > 
> > @@ -27,7 +34,12 @@ static inline void s3c_pm_debug_init_uart(void)
> > 
> >  static inline void s3c_pm_arch_prepare_irqs(void)
> >  {
> > 
> > -       __raw_writel(s3c_irqwake_eintmask, S5P_EINT_WAKEUP_MASK);
> > +       u32 eintmask = s3c_irqwake_eintmask;
> > +
> > +       if (of_have_populated_dt())
> > +               eintmask = exynos_get_eint_wake_mask();
> > +
> > +       __raw_writel(eintmask, S5P_EINT_WAKEUP_MASK);
> > 
> >         __raw_writel(s3c_irqwake_intmask & ~(1 << 31),
> >         S5P_WAKEUP_MASK);
> >  
> >  }
> > 
> > --
> > 1.8.2.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-samsung-soc" in the body of a message to
> majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/6] ARM: EXYNOS: Fix EINT wake-up mask configuration when pinctrl is used

[Linus Walleij]

On Fri, May 17, 2013 at 6:24 PM, Tomasz Figa <t.figa@samsung.com> wrote:

> On DT-enabled systems pinctrl-exynos driver is responsible for handling
> of wake-up EINT interrupts. This patch adjusts wake-up mask
> configuration code to take wake-up mask value from pinctrl-exynos driver
> on DT-enabled systems.
>
> Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

Patch applied to my fixes branch for v3.10.

Yours,
Linus Walleij

[PATCH 3/6] ARM: SAMSUNG: pm: Adjust for pinctrl- and DT-enabled platforms

[Tomasz Figa]

This patch makes legacy code on suspend/resume path being executed
conditionally, on non-DT platforms only, to fix suspend/resume of
DT-enabled systems, for which the code is inappropriate.

Signed-off-by: Tomasz Figa <t.figa@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/plat-samsung/pm.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/arch/arm/plat-samsung/pm.c b/arch/arm/plat-samsung/pm.c
index 53210ec..8ac2b2d 100644
--- a/arch/arm/plat-samsung/pm.c
+++ b/arch/arm/plat-samsung/pm.c
@@ -261,7 +261,8 @@ static int s3c_pm_enter(suspend_state_t state)
 	 * require a full power-cycle)
 	*/
 
-	if (!any_allowed(s3c_irqwake_intmask, s3c_irqwake_intallow) &&
+	if (!of_have_populated_dt() &&
+	    !any_allowed(s3c_irqwake_intmask, s3c_irqwake_intallow) &&
 	    !any_allowed(s3c_irqwake_eintmask, s3c_irqwake_eintallow)) {
 		printk(KERN_ERR "%s: No wake-up sources!\n", __func__);
 		printk(KERN_ERR "%s: Aborting sleep\n", __func__);
@@ -270,8 +271,11 @@ static int s3c_pm_enter(suspend_state_t state)
 
 	/* save all necessary core registers not covered by the drivers */
 
-	samsung_pm_save_gpios();
-	samsung_pm_saved_gpios();
+	if (!of_have_populated_dt()) {
+		samsung_pm_save_gpios();
+		samsung_pm_saved_gpios();
+	}
+
 	s3c_pm_save_uarts();
 	s3c_pm_save_core();
 
@@ -310,8 +314,11 @@ static int s3c_pm_enter(suspend_state_t state)
 
 	s3c_pm_restore_core();
 	s3c_pm_restore_uarts();
-	samsung_pm_restore_gpios();
-	s3c_pm_restored_gpios();
+
+	if (!of_have_populated_dt()) {
+		samsung_pm_restore_gpios();
+		s3c_pm_restored_gpios();
+	}
 
 	s3c_pm_debug_init();
 
-- 
1.8.2.1

[PATCH 3/6] ARM: SAMSUNG: pm: Adjust for pinctrl- and DT-enabled platforms

[Doug Anderson]

Tomasz,

On Fri, May 17, 2013 at 9:24 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> This patch makes legacy code on suspend/resume path being executed
> conditionally, on non-DT platforms only, to fix suspend/resume of
> DT-enabled systems, for which the code is inappropriate.
>
> Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  arch/arm/plat-samsung/pm.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/plat-samsung/pm.c b/arch/arm/plat-samsung/pm.c
> index 53210ec..8ac2b2d 100644
> --- a/arch/arm/plat-samsung/pm.c
> +++ b/arch/arm/plat-samsung/pm.c
> @@ -261,7 +261,8 @@ static int s3c_pm_enter(suspend_state_t state)
>          * require a full power-cycle)
>         */
>
> -       if (!any_allowed(s3c_irqwake_intmask, s3c_irqwake_intallow) &&
> +       if (!of_have_populated_dt() &&
> +           !any_allowed(s3c_irqwake_intmask, s3c_irqwake_intallow) &&

I'd rather see you modify patch set #2 to provide some function to
retrieve just the eint mask and then use it here.  Your patch removes
this test and doesn't replace it with anything.  If you moved this
test to pinctrl directly you'd lose the test against intallow.

...or do you think this test is no longer useful for some reason?


>             !any_allowed(s3c_irqwake_eintmask, s3c_irqwake_eintallow)) {
>                 printk(KERN_ERR "%s: No wake-up sources!\n", __func__);
>                 printk(KERN_ERR "%s: Aborting sleep\n", __func__);
> @@ -270,8 +271,11 @@ static int s3c_pm_enter(suspend_state_t state)
>
>         /* save all necessary core registers not covered by the drivers */
>
> -       samsung_pm_save_gpios();
> -       samsung_pm_saved_gpios();
> +       if (!of_have_populated_dt()) {
> +               samsung_pm_save_gpios();
> +               samsung_pm_saved_gpios();
> +       }
> +

Ah, the important part here is the "saved", not the "save"!  The
"save" should get a NULL chip for all GPIOs and effectively be a
no-op.

Skipping the "saved" is important of s3c64xx and s5p64x0 where the
"saved" seems to transition things over to powerdown mode.  Hopefully
a future patch of yours adds that back for those platforms in the
pinmux world.  ...same for restore.


Summary: I've tested this on exynos5250-snow and it's reasonable but
I'd love a response about the missing test before adding Reviewed-by.

-Doug

[PATCH 3/6] ARM: SAMSUNG: pm: Adjust for pinctrl- and DT-enabled platforms

[Tomasz Figa]

On Friday 17 of May 2013 12:24:04 Doug Anderson wrote:
> Tomasz,
> 
> On Fri, May 17, 2013 at 9:24 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> > This patch makes legacy code on suspend/resume path being executed
> > conditionally, on non-DT platforms only, to fix suspend/resume of
> > DT-enabled systems, for which the code is inappropriate.
> > 
> > Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> > 
> >  arch/arm/plat-samsung/pm.c | 17 ++++++++++++-----
> >  1 file changed, 12 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/arm/plat-samsung/pm.c b/arch/arm/plat-samsung/pm.c
> > index 53210ec..8ac2b2d 100644
> > --- a/arch/arm/plat-samsung/pm.c
> > +++ b/arch/arm/plat-samsung/pm.c
> > @@ -261,7 +261,8 @@ static int s3c_pm_enter(suspend_state_t state)
> > 
> >          * require a full power-cycle)
> >         
> >         */
> > 
> > -       if (!any_allowed(s3c_irqwake_intmask, s3c_irqwake_intallow) &&
> > +       if (!of_have_populated_dt() &&
> > +           !any_allowed(s3c_irqwake_intmask, s3c_irqwake_intallow) &&
> 
> I'd rather see you modify patch set #2 to provide some function to
> retrieve just the eint mask and then use it here.  Your patch removes
> this test and doesn't replace it with anything.  If you moved this
> test to pinctrl directly you'd lose the test against intallow.

Well, looking from the perspective of status before my patch, it just 
bypasses a test that is incorrect on DT-enabled platforms.

I agree that this test is rather reasonable to have, but it would require 
defining a new interface and moving all platforms to it, which for now 
would be a bit of overkill.

IMHO a separate series that sanitizes the whole PM handling in plat-
samsung, including a rework of this check to make it cover all platforms 
in a generic and multiplatform-friendly way. What do you think?

> ...or do you think this test is no longer useful for some reason?
> 
> >             !any_allowed(s3c_irqwake_eintmask, s3c_irqwake_eintallow))
> >             {
> >             
> >                 printk(KERN_ERR "%s: No wake-up sources!\n",
> >                 __func__);
> >                 printk(KERN_ERR "%s: Aborting sleep\n", __func__);
> > 
> > @@ -270,8 +271,11 @@ static int s3c_pm_enter(suspend_state_t state)
> > 
> >         /* save all necessary core registers not covered by the
> >         drivers */
> > 
> > -       samsung_pm_save_gpios();
> > -       samsung_pm_saved_gpios();
> > +       if (!of_have_populated_dt()) {
> > +               samsung_pm_save_gpios();
> > +               samsung_pm_saved_gpios();
> > +       }
> > +
> 
> Ah, the important part here is the "saved", not the "save"!  The
> "save" should get a NULL chip for all GPIOs and effectively be a
> no-op.
> 
> Skipping the "saved" is important of s3c64xx and s5p64x0 where the
> "saved" seems to transition things over to powerdown mode.  Hopefully
> a future patch of yours adds that back for those platforms in the
> pinmux world.  ...same for restore.

S3C64xx can be switched to power down pin configuration manually, but if 
you don't do it, it will activate it automatically after entering sleep 
mode.

> Summary: I've tested this on exynos5250-snow and it's reasonable but
> I'd love a response about the missing test before adding Reviewed-by.

Thanks.

Best regards,
Tomasz

[PATCH 3/6] ARM: SAMSUNG: pm: Adjust for pinctrl- and DT-enabled platforms

[Doug Anderson]

Tomasz,

On Fri, May 17, 2013 at 1:23 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> I'd rather see you modify patch set #2 to provide some function to
>> retrieve just the eint mask and then use it here.  Your patch removes
>> this test and doesn't replace it with anything.  If you moved this
>> test to pinctrl directly you'd lose the test against intallow.
>
> Well, looking from the perspective of status before my patch, it just
> bypasses a test that is incorrect on DT-enabled platforms.

True.  What was there before was broken and this avoids the broken code.


> I agree that this test is rather reasonable to have, but it would require
> defining a new interface and moving all platforms to it, which for now
> would be a bit of overkill.

It seems like you could use the same type of solution as patch set #2?

...oh, but that's only for exynos!  I guess we would need something
similar for other platforms.  ...and until we do those other platforms
(including S3C64xx, I think) are still using the old
s3c_irqwake_eintmask, right?

...so overall your patch series only fully fixes exynos, though it
does make other platforms less broken which is a plus.  :)


> IMHO a separate series that sanitizes the whole PM handling in plat-
> samsung, including a rework of this check to make it cover all platforms
> in a generic and multiplatform-friendly way. What do you think?

Sure, I think that would be OK.


>> Ah, the important part here is the "saved", not the "save"!  The
>> "save" should get a NULL chip for all GPIOs and effectively be a
>> no-op.
>>
>> Skipping the "saved" is important of s3c64xx and s5p64x0 where the
>> "saved" seems to transition things over to powerdown mode.  Hopefully
>> a future patch of yours adds that back for those platforms in the
>> pinmux world.  ...same for restore.
>
> S3C64xx can be switched to power down pin configuration manually, but if
> you don't do it, it will activate it automatically after entering sleep
> mode.

How would restore work in that case?  I'd imagine that it would
automatically switch out of the powerdown config at wakeup before
running software?  That doesn't seem like a great idea.

I think that to make S3C64xx work properly we'd need to solve.


I wouldn't be opposed to a re-spin to address some of the above, but I
also won't object to this landing and remaining issues being addressed
in future patches.  This patch definitely does make things better and
no worse.  :)

On exynos5250-snow (pinmux backported to 3.8):

Tested-by: Doug Anderson <dianders@chromium.org>

Reviewed-by: Doug Anderson <dianders@chromium.org>

[PATCH 3/6] ARM: SAMSUNG: pm: Adjust for pinctrl- and DT-enabled platforms

[Tomasz Figa]

On Friday 17 of May 2013 13:56:39 Doug Anderson wrote:
> Tomasz,
> 
> On Fri, May 17, 2013 at 1:23 PM, Tomasz Figa <tomasz.figa@gmail.com> 
wrote:
> >> I'd rather see you modify patch set #2 to provide some function to
> >> retrieve just the eint mask and then use it here.  Your patch removes
> >> this test and doesn't replace it with anything.  If you moved this
> >> test to pinctrl directly you'd lose the test against intallow.
> > 
> > Well, looking from the perspective of status before my patch, it just
> > bypasses a test that is incorrect on DT-enabled platforms.
> 
> True.  What was there before was broken and this avoids the broken code.
> > I agree that this test is rather reasonable to have, but it would
> > require defining a new interface and moving all platforms to it,
> > which for now would be a bit of overkill.
> 
> It seems like you could use the same type of solution as patch set #2?
> 
> ...oh, but that's only for exynos!  I guess we would need something
> similar for other platforms.  ...and until we do those other platforms
> (including S3C64xx, I think) are still using the old
> s3c_irqwake_eintmask, right?

Correct.

Also as an extra side note, intallow is used here as a mask of valid 
eintmask bits on given platform. On Exynos SoCs there are 32 EINTs, so 
this extra mask is not needed.

> ...so overall your patch series only fully fixes exynos, though it
> does make other platforms less broken which is a plus.  :)
> 
> > IMHO a separate series that sanitizes the whole PM handling in plat-
> > samsung, including a rework of this check to make it cover all
> > platforms in a generic and multiplatform-friendly way. What do you
> > think?
> Sure, I think that would be OK.
> 
> >> Ah, the important part here is the "saved", not the "save"!  The
> >> "save" should get a NULL chip for all GPIOs and effectively be a
> >> no-op.
> >> 
> >> Skipping the "saved" is important of s3c64xx and s5p64x0 where the
> >> "saved" seems to transition things over to powerdown mode.  Hopefully
> >> a future patch of yours adds that back for those platforms in the
> >> pinmux world.  ...same for restore.
> > 
> > S3C64xx can be switched to power down pin configuration manually, but
> > if you don't do it, it will activate it automatically after entering
> > sleep mode.
> 
> How would restore work in that case?  I'd imagine that it would
> automatically switch out of the powerdown config at wakeup before
> running software?  That doesn't seem like a great idea.

This is configurable. There is a bit that determines whether it should 
switch back to normal config automatically or not. What's interesting is 
that AFAIR current code uses automatic switch, which even with the glitch 
prevention in resume can glitch things, due to the period of time between 
wake-up and resume when pins are misconfigured.

I think we should just configure this bit for manual switch back and 
trigger the switch from .resumed() callback of the pin controller in 
pinctrl-s3c64xx driver. I will post a patch for this some day.

> I think that to make S3C64xx work properly we'd need to solve.
> 
> 
> I wouldn't be opposed to a re-spin to address some of the above, but I
> also won't object to this landing and remaining issues being addressed
> in future patches.  This patch definitely does make things better and
> no worse.  :)

Let's see. If nobody takes this until Monday, which is very likely, I will 
send new version.

> On exynos5250-snow (pinmux backported to 3.8):
> 
> Tested-by: Doug Anderson <dianders@chromium.org>
> 
> Reviewed-by: Doug Anderson <dianders@chromium.org>

Thanks.

Best regards,
Tomasz

[PATCH 3/6] ARM: SAMSUNG: pm: Adjust for pinctrl- and DT-enabled platforms

[Linus Walleij]

On Fri, May 17, 2013 at 11:07 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> On Friday 17 of May 2013 13:56:39 Doug Anderson wrote:
>> I wouldn't be opposed to a re-spin to address some of the above, but I
>> also won't object to this landing and remaining issues being addressed
>> in future patches.  This patch definitely does make things better and
>> no worse.  :)
>
> Let's see. If nobody takes this until Monday, which is very likely, I will
> send new version.

I applied 1 & 2 so pls repost your updated patches 3,4,5,6 on
monday and let's continue fixing.

Yours,
Linus Walleij

[PATCH 3/6] ARM: SAMSUNG: pm: Adjust for pinctrl- and DT-enabled platforms

[Tomasz Figa]

Hi Linus,

On Tuesday 21 of May 2013 13:29:23 Linus Walleij wrote:
> On Fri, May 17, 2013 at 11:07 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> > On Friday 17 of May 2013 13:56:39 Doug Anderson wrote:
> >> I wouldn't be opposed to a re-spin to address some of the above, but I
> >> also won't object to this landing and remaining issues being addressed
> >> in future patches.  This patch definitely does make things better and
> >> no worse.  :)
> > 
> > Let's see. If nobody takes this until Monday, which is very likely, I will
> > send new version.
> 
> I applied 1 & 2 so pls repost your updated patches 3,4,5,6 on
> monday and let's continue fixing.

Thanks.

Feel free to apply current version of patches 3, 4 and 5 as well. I will post 
updated patch 6 in a while.

Best regards,
-- 
Tomasz Figa
Linux Kernel Developer
Samsung R&D Institute Poland
Samsung Electronics
 
> Yours,
> Linus Walleij
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc"
> in the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/6] ARM: SAMSUNG: pm: Adjust for pinctrl- and DT-enabled platforms

[Tomasz Figa]

Just posted updated version of patch 6/6.

Best regards,
-- 
Tomasz Figa
Linux Kernel Developer
Samsung R&D Institute Poland
Samsung Electronics

On Tuesday 21 of May 2013 15:15:45 Tomasz Figa wrote:
> Hi Linus,
> 
> On Tuesday 21 of May 2013 13:29:23 Linus Walleij wrote:
> > On Fri, May 17, 2013 at 11:07 PM, Tomasz Figa <tomasz.figa@gmail.com> 
wrote:
> > > On Friday 17 of May 2013 13:56:39 Doug Anderson wrote:
> > >> I wouldn't be opposed to a re-spin to address some of the above, but I
> > >> also won't object to this landing and remaining issues being addressed
> > >> in future patches.  This patch definitely does make things better and
> > >> no worse.  :)
> > > 
> > > Let's see. If nobody takes this until Monday, which is very likely, I
> > > will
> > > send new version.
> > 
> > I applied 1 & 2 so pls repost your updated patches 3,4,5,6 on
> > monday and let's continue fixing.
> 
> Thanks.
> 
> Feel free to apply current version of patches 3, 4 and 5 as well. I will
> post updated patch 6 in a while.
> 
> Best regards,
> 
> > Yours,
> > Linus Walleij
> > --
> > To unsubscribe from this list: send the line "unsubscribe
> > linux-samsung-soc" in the body of a message to majordomo at vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc"
> in the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/6] ARM: SAMSUNG: pm: Adjust for pinctrl- and DT-enabled platforms

[Tomasz Figa]

Hi,

On Friday 17 of May 2013 18:24:29 Tomasz Figa wrote:
> This patch makes legacy code on suspend/resume path being executed
> conditionally, on non-DT platforms only, to fix suspend/resume of
> DT-enabled systems, for which the code is inappropriate.
> 
> Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  arch/arm/plat-samsung/pm.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)

It seems like this patch did not make it into 3.10-rc5, while rest of patches 
did, which ended up with suspend/resume still being broken.

What should we do in this case? 

Best regards,
Tomasz

> diff --git a/arch/arm/plat-samsung/pm.c b/arch/arm/plat-samsung/pm.c
> index 53210ec..8ac2b2d 100644
> --- a/arch/arm/plat-samsung/pm.c
> +++ b/arch/arm/plat-samsung/pm.c
> @@ -261,7 +261,8 @@ static int s3c_pm_enter(suspend_state_t state)
>  	 * require a full power-cycle)
>  	*/
> 
> -	if (!any_allowed(s3c_irqwake_intmask, s3c_irqwake_intallow) &&
> +	if (!of_have_populated_dt() &&
> +	    !any_allowed(s3c_irqwake_intmask, s3c_irqwake_intallow) &&
>  	    !any_allowed(s3c_irqwake_eintmask, s3c_irqwake_eintallow)) {
>  		printk(KERN_ERR "%s: No wake-up sources!\n", __func__);
>  		printk(KERN_ERR "%s: Aborting sleep\n", __func__);
> @@ -270,8 +271,11 @@ static int s3c_pm_enter(suspend_state_t state)
> 
>  	/* save all necessary core registers not covered by the drivers */
> 
> -	samsung_pm_save_gpios();
> -	samsung_pm_saved_gpios();
> +	if (!of_have_populated_dt()) {
> +		samsung_pm_save_gpios();
> +		samsung_pm_saved_gpios();
> +	}
> +
>  	s3c_pm_save_uarts();
>  	s3c_pm_save_core();
> 
> @@ -310,8 +314,11 @@ static int s3c_pm_enter(suspend_state_t state)
> 
>  	s3c_pm_restore_core();
>  	s3c_pm_restore_uarts();
> -	samsung_pm_restore_gpios();
> -	s3c_pm_restored_gpios();
> +
> +	if (!of_have_populated_dt()) {
> +		samsung_pm_restore_gpios();
> +		s3c_pm_restored_gpios();
> +	}
> 
>  	s3c_pm_debug_init();

[PATCH 3/6] ARM: SAMSUNG: pm: Adjust for pinctrl- and DT-enabled platforms

[Linus Walleij]

On Mon, Jun 10, 2013 at 4:45 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> On Friday 17 of May 2013 18:24:29 Tomasz Figa wrote:
>> This patch makes legacy code on suspend/resume path being executed
>> conditionally, on non-DT platforms only, to fix suspend/resume of
>> DT-enabled systems, for which the code is inappropriate.
>>
>> Signed-off-by: Tomasz Figa <t.figa@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>  arch/arm/plat-samsung/pm.c | 17 ++++++++++++-----
>>  1 file changed, 12 insertions(+), 5 deletions(-)
>
> It seems like this patch did not make it into 3.10-rc5, while rest of patches
> did, which ended up with suspend/resume still being broken.
>
> What should we do in this case?

I did not apply this patch to the pinctrl tree because it begins
with ARM: but it appears I promised to carry it anyway...

I need an ACK from the ARM SoC people to do that though,
or you can take it through ARM SoC fixes directly.

I would just ask Olof/Arnd to apply it directly as a separate
patch.

Yours,
Linus Walleij

[PATCH 3/6] ARM: SAMSUNG: pm: Adjust for pinctrl- and DT-enabled platforms

[Olof Johansson]

On Mon, Jun 10, 2013 at 06:13:10PM +0200, Linus Walleij wrote:
> On Mon, Jun 10, 2013 at 4:45 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> > On Friday 17 of May 2013 18:24:29 Tomasz Figa wrote:
> >> This patch makes legacy code on suspend/resume path being executed
> >> conditionally, on non-DT platforms only, to fix suspend/resume of
> >> DT-enabled systems, for which the code is inappropriate.
> >>
> >> Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >> ---
> >>  arch/arm/plat-samsung/pm.c | 17 ++++++++++++-----
> >>  1 file changed, 12 insertions(+), 5 deletions(-)
> >
> > It seems like this patch did not make it into 3.10-rc5, while rest of patches
> > did, which ended up with suspend/resume still being broken.
> >
> > What should we do in this case?
> 
> I did not apply this patch to the pinctrl tree because it begins
> with ARM: but it appears I promised to carry it anyway...
> 
> I need an ACK from the ARM SoC people to do that though,
> or you can take it through ARM SoC fixes directly.
> 
> I would just ask Olof/Arnd to apply it directly as a separate
> patch.

It makes sense to take this through arm-soc since it's no longer
dependent on anything else.

Applied to fixes. Please do follow through with the promised cleanups at some
point, Tomasz. :-)


-Olof

[PATCH 3/6] ARM: SAMSUNG: pm: Adjust for pinctrl- and DT-enabled platforms

[Olof Johansson]

On Tue, Jun 11, 2013 at 12:45 AM, Olof Johansson <olof@lixom.net> wrote:
> On Mon, Jun 10, 2013 at 06:13:10PM +0200, Linus Walleij wrote:
>> On Mon, Jun 10, 2013 at 4:45 PM, Tomasz Figa <t.figa@samsung.com> wrote:
>> > On Friday 17 of May 2013 18:24:29 Tomasz Figa wrote:
>> >> This patch makes legacy code on suspend/resume path being executed
>> >> conditionally, on non-DT platforms only, to fix suspend/resume of
>> >> DT-enabled systems, for which the code is inappropriate.
>> >>
>> >> Signed-off-by: Tomasz Figa <t.figa@samsung.com>
>> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> >> ---
>> >>  arch/arm/plat-samsung/pm.c | 17 ++++++++++++-----
>> >>  1 file changed, 12 insertions(+), 5 deletions(-)
>> >
>> > It seems like this patch did not make it into 3.10-rc5, while rest of patches
>> > did, which ended up with suspend/resume still being broken.
>> >
>> > What should we do in this case?
>>
>> I did not apply this patch to the pinctrl tree because it begins
>> with ARM: but it appears I promised to carry it anyway...
>>
>> I need an ACK from the ARM SoC people to do that though,
>> or you can take it through ARM SoC fixes directly.
>>
>> I would just ask Olof/Arnd to apply it directly as a separate
>> patch.
>
> It makes sense to take this through arm-soc since it's no longer
> dependent on anything else.
>
> Applied to fixes. Please do follow through with the promised cleanups at some
> point, Tomasz. :-)


Btw, I had to add an include of <linux/of.h> or else all non-DT builds broke...


-Olof

[PATCH 3/6] ARM: SAMSUNG: pm: Adjust for pinctrl- and DT-enabled platforms

[Tomasz Figa]

Hi Olof,

On Tuesday 11 of June 2013 01:21:59 Olof Johansson wrote:
> On Tue, Jun 11, 2013 at 12:45 AM, Olof Johansson <olof@lixom.net> wrote:
> > On Mon, Jun 10, 2013 at 06:13:10PM +0200, Linus Walleij wrote:
> >> On Mon, Jun 10, 2013 at 4:45 PM, Tomasz Figa <t.figa@samsung.com> 
wrote:
> >> > On Friday 17 of May 2013 18:24:29 Tomasz Figa wrote:
> >> >> This patch makes legacy code on suspend/resume path being executed
> >> >> conditionally, on non-DT platforms only, to fix suspend/resume of
> >> >> DT-enabled systems, for which the code is inappropriate.
> >> >> 
> >> >> Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> >> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >> >> ---
> >> >> 
> >> >>  arch/arm/plat-samsung/pm.c | 17 ++++++++++++-----
> >> >>  1 file changed, 12 insertions(+), 5 deletions(-)
> >> > 
> >> > It seems like this patch did not make it into 3.10-rc5, while rest
> >> > of patches did, which ended up with suspend/resume still being
> >> > broken.
> >> > 
> >> > What should we do in this case?
> >> 
> >> I did not apply this patch to the pinctrl tree because it begins
> >> with ARM: but it appears I promised to carry it anyway...
> >> 
> >> I need an ACK from the ARM SoC people to do that though,
> >> or you can take it through ARM SoC fixes directly.
> >> 
> >> I would just ask Olof/Arnd to apply it directly as a separate
> >> patch.
> > 
> > It makes sense to take this through arm-soc since it's no longer
> > dependent on anything else.
> > 
> > Applied to fixes. 

Thanks.

> > Please do follow through with the promised cleanups
> > at some point, Tomasz. :-)

Yeah, just managed to reserve some time for it, so stay tuned ;) .

> 
> Btw, I had to add an include of <linux/of.h> or else all non-DT builds
> broke...

Sorry, I must have missed it.

I guess it's the right time to set up some autobuild framework to check 
all patches with several most interesting configs. I guess some git hooks 
would be enough...

Best regards,
Tomasz

> 
> -Olof
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/6] ARM: SAMSUNG: pm: Adjust for pinctrl- and DT-enabled platforms

[Olof Johansson]

On Tue, Jun 11, 2013 at 5:15 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:

>> > Please do follow through with the promised cleanups
>> > at some point, Tomasz. :-)
>
> Yeah, just managed to reserve some time for it, so stay tuned ;) .

Cool.

>> Btw, I had to add an include of <linux/of.h> or else all non-DT builds
>> broke...
>
> Sorry, I must have missed it.
>
> I guess it's the right time to set up some autobuild framework to check
> all patches with several most interesting configs. I guess some git hooks
> would be enough...

No worries. I tend to build all defconfigs after doing a batch of
pulls, and that's when we find these issues. I also boot on the few
boards that I have to make sure nothing catastrophic is broken, but
there's obviously much less coverage on that side.


-Olof

[PATCH 4/6] pinctrl: samsung: Add support for SoC-specific suspend/resume callbacks

[Tomasz Figa]

SoC-specific driver might require additional save and restore of
registers. This patch adds pair of SoC-specific callbacks per pinctrl
device to account for this.

Signed-off-by: Tomasz Figa <t.figa@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/pinctrl/pinctrl-samsung.c | 6 ++++++
 drivers/pinctrl/pinctrl-samsung.h | 3 +++
 2 files changed, 9 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-samsung.c b/drivers/pinctrl/pinctrl-samsung.c
index 15db258..63ac22e 100644
--- a/drivers/pinctrl/pinctrl-samsung.c
+++ b/drivers/pinctrl/pinctrl-samsung.c
@@ -1010,6 +1010,9 @@ static void samsung_pinctrl_suspend_dev(
 				 reg, bank->pm_save[PINCFG_TYPE_FUNC]);
 		}
 	}
+
+	if (ctrl->suspend)
+		ctrl->suspend(drvdata);
 }
 
 /**
@@ -1026,6 +1029,9 @@ static void samsung_pinctrl_resume_dev(struct samsung_pinctrl_drv_data *drvdata)
 	void __iomem *virt_base = drvdata->virt_base;
 	int i;
 
+	if (ctrl->resume)
+		ctrl->resume(drvdata);
+
 	for (i = 0; i < ctrl->nr_banks; i++) {
 		struct samsung_pin_bank *bank = &ctrl->pin_banks[i];
 		void __iomem *reg = virt_base + bank->pctl_offset;
diff --git a/drivers/pinctrl/pinctrl-samsung.h b/drivers/pinctrl/pinctrl-samsung.h
index 9f5cc81..b316d9f 100644
--- a/drivers/pinctrl/pinctrl-samsung.h
+++ b/drivers/pinctrl/pinctrl-samsung.h
@@ -187,6 +187,9 @@ struct samsung_pin_ctrl {
 
 	int		(*eint_gpio_init)(struct samsung_pinctrl_drv_data *);
 	int		(*eint_wkup_init)(struct samsung_pinctrl_drv_data *);
+	void		(*suspend)(struct samsung_pinctrl_drv_data *);
+	void		(*resume)(struct samsung_pinctrl_drv_data *);
+
 	char		*label;
 };
 
-- 
1.8.2.1

[PATCH 4/6] pinctrl: samsung: Add support for SoC-specific suspend/resume callbacks

[Doug Anderson]

Tomasz,

On Fri, May 17, 2013 at 9:24 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> +       if (ctrl->resume)
> +               ctrl->resume(drvdata);
> +

Having this resume at the beginning of the function seems right for
restoring eint settings like you do in patch #6.

...but if you need to try to handle the old s3c64xx and s5p64x0
concept of "restored" to actually take GPIOs out of powerdown mode
then you'll also need a callback at the end.

Does it make sense to add a second callback at the end of this function?


...since it's unclear how we'll handle s3c64xx/s5p64x0 (or even if I'm
misunderstanding and they're already handled somehow), I don't see any
problems with this patch, so...


On exynos5250-snow (pinmux backported to 3.8):

Tested-by: Doug Anderson <dianders@chromium.org>

Reviewed-by: Doug Anderson <dianders@chromium.org>

[PATCH 4/6] pinctrl: samsung: Add support for SoC-specific suspend/resume callbacks

[Tomasz Figa]

On Friday 17 of May 2013 12:24:29 Doug Anderson wrote:
> Tomasz,
> 
> On Fri, May 17, 2013 at 9:24 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> > +       if (ctrl->resume)
> > +               ctrl->resume(drvdata);
> > +
> 
> Having this resume at the beginning of the function seems right for
> restoring eint settings like you do in patch #6.
> 
> ...but if you need to try to handle the old s3c64xx and s5p64x0
> concept of "restored" to actually take GPIOs out of powerdown mode
> then you'll also need a callback at the end.
> 
> Does it make sense to add a second callback at the end of this function?

Right. I haven't thought of this. It might make sense to add .resumed() 
callback as well to handle this.

I guess it could be added as a part of patches for S3C64xx-specific 
pinctrl suspend/resume, that I will post some day.

> ...since it's unclear how we'll handle s3c64xx/s5p64x0 (or even if I'm
> misunderstanding and they're already handled somehow), I don't see any
> problems with this patch, so...
> 
> 
> On exynos5250-snow (pinmux backported to 3.8):
> 
> Tested-by: Doug Anderson <dianders@chromium.org>
> 
> Reviewed-by: Doug Anderson <dianders@chromium.org>

Thanks.

Best regards,
Tomasz

[PATCH 4/6] pinctrl: samsung: Add support for SoC-specific suspend/resume callbacks

[Linus Walleij]

On Fri, May 17, 2013 at 6:24 PM, Tomasz Figa <t.figa@samsung.com> wrote:

> SoC-specific driver might require additional save and restore of
> registers. This patch adds pair of SoC-specific callbacks per pinctrl
> device to account for this.
>
> Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

Patch applied for fixes.

Hm this is quite a lot of code for "fixes", can you confirm that
the system is really unusable without all these patches?

Yours,
Linus Walleij

[PATCH 4/6] pinctrl: samsung: Add support for SoC-specific suspend/resume callbacks

[Tomasz Figa]

Hi Linus,

On Friday 24 of May 2013 11:07:41 Linus Walleij wrote:
> On Fri, May 17, 2013 at 6:24 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> > SoC-specific driver might require additional save and restore of
> > registers. This patch adds pair of SoC-specific callbacks per pinctrl
> > device to account for this.
> > 
> > Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> 
> Patch applied for fixes.

Thanks.

> Hm this is quite a lot of code for "fixes", can you confirm that
> the system is really unusable without all these patches?

Well, it is something that should have been already sent as a fix for 3.8, when 
this pin control driver was added, because since then suspend/resume has been 
broken on DT-enabled Exynos boards.

Without this, suspending the board and then trying to wake it up is rather 
unpredictable, leading usually to board reset, interrupt storm or at least 
some devices failing to resume.

Best regards,
-- 
Tomasz Figa
Linux Kernel Developer
Samsung R&D Institute Poland
Samsung Electronics

[PATCH 5/6] pinctrl: samsung: Allow per-bank SoC-specific private data

[Tomasz Figa]

This patch extends pin bank descriptor structure with SoC-specific
private data field that allows SoC-specific drivers to store their own
private data.

Signed-off-by: Tomasz Figa <t.figa@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/pinctrl/pinctrl-samsung.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pinctrl/pinctrl-samsung.h b/drivers/pinctrl/pinctrl-samsung.h
index b316d9f..26d3519 100644
--- a/drivers/pinctrl/pinctrl-samsung.h
+++ b/drivers/pinctrl/pinctrl-samsung.h
@@ -139,6 +139,7 @@ struct samsung_pin_bank {
 	u32		eint_mask;
 	u32		eint_offset;
 	char		*name;
+	void		*soc_priv;
 	struct device_node *of_node;
 	struct samsung_pinctrl_drv_data *drvdata;
 	struct irq_domain *irq_domain;
-- 
1.8.2.1

[PATCH 5/6] pinctrl: samsung: Allow per-bank SoC-specific private data

[Doug Anderson]

Tomasz,

On Fri, May 17, 2013 at 9:24 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> This patch extends pin bank descriptor structure with SoC-specific
> private data field that allows SoC-specific drivers to store their own
> private data.
>
> Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/pinctrl/pinctrl-samsung.h | 1 +
>  1 file changed, 1 insertion(+)

Sure.  Seems like it could be squashed into the next patch, but I
think it looks fine separate too.

On exynos5250-snow (pinmux backported to 3.8):

Tested-by: Doug Anderson <dianders@chromium.org>

Reviewed-by: Doug Anderson <dianders@chromium.org>

[PATCH 5/6] pinctrl: samsung: Allow per-bank SoC-specific private data

[Linus Walleij]

On Fri, May 17, 2013 at 6:24 PM, Tomasz Figa <t.figa@samsung.com> wrote:

> This patch extends pin bank descriptor structure with SoC-specific
> private data field that allows SoC-specific drivers to store their own
> private data.
>
> Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

Patch applied for fixes.

Yours,
Linus Walleij

[PATCH 6/6] pinctrl: exynos: Handle suspend/resume of GPIO EINT registers

[Tomasz Figa]

Some GPIO EINT control registers needs to be preserved across
suspend/resume cycle. This patch extends the driver to take care of
this.

Signed-off-by: Tomasz Figa <t.figa@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/pinctrl/pinctrl-exynos.c | 83 ++++++++++++++++++++++++++++++++++++++++
 drivers/pinctrl/pinctrl-exynos.h |  1 +
 2 files changed, 84 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-exynos.c b/drivers/pinctrl/pinctrl-exynos.c
index 4f868e5..908d24a 100644
--- a/drivers/pinctrl/pinctrl-exynos.c
+++ b/drivers/pinctrl/pinctrl-exynos.c
@@ -196,6 +196,12 @@ static irqreturn_t exynos_eint_gpio_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+struct exynos_eint_gpio_save {
+	u32 eint_con;
+	u32 eint_fltcon0;
+	u32 eint_fltcon1;
+};
+
 /*
  * exynos_eint_gpio_init() - setup handling of external gpio interrupts.
  * @d: driver data of samsung pinctrl driver.
@@ -229,6 +235,11 @@ static int exynos_eint_gpio_init(struct samsung_pinctrl_drv_data *d)
 			dev_err(dev, "gpio irq domain add failed\n");
 			return -ENXIO;
 		}
+
+		bank->soc_priv = devm_kzalloc(d->dev,
+			sizeof(struct exynos_eint_gpio_save), GFP_KERNEL);
+		if (!bank->soc_priv)
+			return -ENOMEM;
 	}
 
 	return 0;
@@ -528,6 +539,58 @@ static int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d)
 	return 0;
 }
 
+static void exynos_pinctrl_suspend_bank(
+				struct samsung_pinctrl_drv_data *drvdata,
+				struct samsung_pin_bank *bank)
+{
+	struct exynos_eint_gpio_save *save = bank->soc_priv;
+	void __iomem *regs = drvdata->virt_base;
+
+	save->eint_con = readl(regs + EXYNOS_GPIO_ECON_OFFSET
+						+ bank->eint_offset);
+	save->eint_fltcon0 = readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
+						+ 2 * bank->eint_offset);
+	save->eint_fltcon1 = readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
+						+ 2 * bank->eint_offset + 4);
+}
+
+static void exynos_pinctrl_suspend(struct samsung_pinctrl_drv_data *drvdata)
+{
+	struct samsung_pin_ctrl *ctrl = drvdata->ctrl;
+	struct samsung_pin_bank *bank = ctrl->pin_banks;
+	int i;
+
+	for (i = 0; i < ctrl->nr_banks; ++i, ++bank)
+		if (bank->eint_type == EINT_TYPE_GPIO)
+			exynos_pinctrl_suspend_bank(drvdata, bank);
+}
+
+static void exynos_pinctrl_resume_bank(
+				struct samsung_pinctrl_drv_data *drvdata,
+				struct samsung_pin_bank *bank)
+{
+	struct exynos_eint_gpio_save *save = bank->soc_priv;
+	void __iomem *regs = drvdata->virt_base;
+
+	writel(save->eint_con, regs + EXYNOS_GPIO_ECON_OFFSET
+						+ bank->eint_offset);
+	writel(save->eint_fltcon0, regs + EXYNOS_GPIO_EFLTCON_OFFSET
+						+ 2 * bank->eint_offset);
+	writel(save->eint_fltcon1, regs + EXYNOS_GPIO_EFLTCON_OFFSET
+						+ 2 * bank->eint_offset + 4);
+}
+
+static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
+{
+	struct samsung_pin_ctrl *ctrl = drvdata->ctrl;
+	struct samsung_pin_bank *bank = ctrl->pin_banks;
+	int i;
+
+	for (i = 0; i < ctrl->nr_banks; ++i, ++bank)
+		if (bank->eint_type == EINT_TYPE_GPIO)
+			exynos_pinctrl_resume_bank(drvdata, bank);
+}
+
 /* pin banks of exynos4210 pin-controller 0 */
 static struct samsung_pin_bank exynos4210_pin_banks0[] = {
 	EXYNOS_PIN_BANK_EINTG(8, 0x000, "gpa0", 0x00),
@@ -591,6 +654,8 @@ struct samsung_pin_ctrl exynos4210_pin_ctrl[] = {
 		.geint_pend	= EXYNOS_GPIO_EPEND_OFFSET,
 		.svc		= EXYNOS_SVC_OFFSET,
 		.eint_gpio_init = exynos_eint_gpio_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
 		.label		= "exynos4210-gpio-ctrl0",
 	}, {
 		/* pin-controller instance 1 data */
@@ -605,6 +670,8 @@ struct samsung_pin_ctrl exynos4210_pin_ctrl[] = {
 		.svc		= EXYNOS_SVC_OFFSET,
 		.eint_gpio_init = exynos_eint_gpio_init,
 		.eint_wkup_init = exynos_eint_wkup_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
 		.label		= "exynos4210-gpio-ctrl1",
 	}, {
 		/* pin-controller instance 2 data */
@@ -686,6 +753,8 @@ struct samsung_pin_ctrl exynos4x12_pin_ctrl[] = {
 		.geint_pend	= EXYNOS_GPIO_EPEND_OFFSET,
 		.svc		= EXYNOS_SVC_OFFSET,
 		.eint_gpio_init = exynos_eint_gpio_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
 		.label		= "exynos4x12-gpio-ctrl0",
 	}, {
 		/* pin-controller instance 1 data */
@@ -700,6 +769,8 @@ struct samsung_pin_ctrl exynos4x12_pin_ctrl[] = {
 		.svc		= EXYNOS_SVC_OFFSET,
 		.eint_gpio_init = exynos_eint_gpio_init,
 		.eint_wkup_init = exynos_eint_wkup_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
 		.label		= "exynos4x12-gpio-ctrl1",
 	}, {
 		/* pin-controller instance 2 data */
@@ -710,6 +781,8 @@ struct samsung_pin_ctrl exynos4x12_pin_ctrl[] = {
 		.geint_pend	= EXYNOS_GPIO_EPEND_OFFSET,
 		.svc		= EXYNOS_SVC_OFFSET,
 		.eint_gpio_init = exynos_eint_gpio_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
 		.label		= "exynos4x12-gpio-ctrl2",
 	}, {
 		/* pin-controller instance 3 data */
@@ -720,6 +793,8 @@ struct samsung_pin_ctrl exynos4x12_pin_ctrl[] = {
 		.geint_pend	= EXYNOS_GPIO_EPEND_OFFSET,
 		.svc		= EXYNOS_SVC_OFFSET,
 		.eint_gpio_init = exynos_eint_gpio_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
 		.label		= "exynos4x12-gpio-ctrl3",
 	},
 };
@@ -798,6 +873,8 @@ struct samsung_pin_ctrl exynos5250_pin_ctrl[] = {
 		.svc		= EXYNOS_SVC_OFFSET,
 		.eint_gpio_init = exynos_eint_gpio_init,
 		.eint_wkup_init = exynos_eint_wkup_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
 		.label		= "exynos5250-gpio-ctrl0",
 	}, {
 		/* pin-controller instance 1 data */
@@ -808,6 +885,8 @@ struct samsung_pin_ctrl exynos5250_pin_ctrl[] = {
 		.geint_pend	= EXYNOS_GPIO_EPEND_OFFSET,
 		.svc		= EXYNOS_SVC_OFFSET,
 		.eint_gpio_init = exynos_eint_gpio_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
 		.label		= "exynos5250-gpio-ctrl1",
 	}, {
 		/* pin-controller instance 2 data */
@@ -818,6 +897,8 @@ struct samsung_pin_ctrl exynos5250_pin_ctrl[] = {
 		.geint_pend	= EXYNOS_GPIO_EPEND_OFFSET,
 		.svc		= EXYNOS_SVC_OFFSET,
 		.eint_gpio_init = exynos_eint_gpio_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
 		.label		= "exynos5250-gpio-ctrl2",
 	}, {
 		/* pin-controller instance 3 data */
@@ -828,6 +909,8 @@ struct samsung_pin_ctrl exynos5250_pin_ctrl[] = {
 		.geint_pend	= EXYNOS_GPIO_EPEND_OFFSET,
 		.svc		= EXYNOS_SVC_OFFSET,
 		.eint_gpio_init = exynos_eint_gpio_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
 		.label		= "exynos5250-gpio-ctrl3",
 	},
 };
diff --git a/drivers/pinctrl/pinctrl-exynos.h b/drivers/pinctrl/pinctrl-exynos.h
index 9b1f77a..3c91c35 100644
--- a/drivers/pinctrl/pinctrl-exynos.h
+++ b/drivers/pinctrl/pinctrl-exynos.h
@@ -19,6 +19,7 @@
 
 /* External GPIO and wakeup interrupt related definitions */
 #define EXYNOS_GPIO_ECON_OFFSET		0x700
+#define EXYNOS_GPIO_EFLTCON_OFFSET	0x800
 #define EXYNOS_GPIO_EMASK_OFFSET	0x900
 #define EXYNOS_GPIO_EPEND_OFFSET	0xA00
 #define EXYNOS_WKUP_ECON_OFFSET		0xE00
-- 
1.8.2.1

[PATCH 6/6] pinctrl: exynos: Handle suspend/resume of GPIO EINT registers

[Doug Anderson]

Tomasz,

On Fri, May 17, 2013 at 9:24 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> Some GPIO EINT control registers needs to be preserved across
> suspend/resume cycle. This patch extends the driver to take care of
> this.

I was confused why we didn't seem to need this on exynos5250-snow but
figured it out.  We only use interrupts on GPX lines which don't need
this code.  ...but on any exynos5250 boards that use one of the other
banks for interrupts you'd need it.  I tested by setting one of the
registers related to GPA0 and found that this code is indeed needed on
exynos5250.  :)

Just nits / optional comments below, so on exynos5250-snow (pinmux
backported to 3.8):

Tested-by: Doug Anderson <dianders@chromium.org>

Reviewed-by: Doug Anderson <dianders@chromium.org>


> @@ -229,6 +235,11 @@ static int exynos_eint_gpio_init(struct samsung_pinctrl_drv_data *d)
>                         dev_err(dev, "gpio irq domain add failed\n");
>                         return -ENXIO;
>                 }
> +
> +               bank->soc_priv = devm_kzalloc(d->dev,
> +                       sizeof(struct exynos_eint_gpio_save), GFP_KERNEL);
> +               if (!bank->soc_priv)
> +                       return -ENOMEM;

Slight nit to add this before the call to irq_domain_add_linear().
demv() will handle freeing your memory but nothing will handle undoing
the irq_domain_add_linear() if you return an error.

>         }
>
>         return 0;
> @@ -528,6 +539,58 @@ static int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d)
>         return 0;
>  }
>
> +static void exynos_pinctrl_suspend_bank(
> +                               struct samsung_pinctrl_drv_data *drvdata,
> +                               struct samsung_pin_bank *bank)
> +{
> +       struct exynos_eint_gpio_save *save = bank->soc_priv;
> +       void __iomem *regs = drvdata->virt_base;
> +
> +       save->eint_con = readl(regs + EXYNOS_GPIO_ECON_OFFSET
> +                                               + bank->eint_offset);
> +       save->eint_fltcon0 = readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
> +                                               + 2 * bank->eint_offset);
> +       save->eint_fltcon1 = readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
> +                                               + 2 * bank->eint_offset + 4);

Optional: I wish there were debug statements to help debug, like:

pr_debug("%s: save     con %#010x\n", bank->name, save->eint_con);
pr_debug("%s: save fltcon0 %#010x\n", bank->name, save->eint_fltcon0);
pr_debug("%s: save fltcon1 %#010x\n", bank->name, save->eint_fltcon1);


> +}
> +
> +static void exynos_pinctrl_suspend(struct samsung_pinctrl_drv_data *drvdata)
> +{
> +       struct samsung_pin_ctrl *ctrl = drvdata->ctrl;
> +       struct samsung_pin_bank *bank = ctrl->pin_banks;
> +       int i;
> +
> +       for (i = 0; i < ctrl->nr_banks; ++i, ++bank)
> +               if (bank->eint_type == EINT_TYPE_GPIO)
> +                       exynos_pinctrl_suspend_bank(drvdata, bank);
> +}
> +
> +static void exynos_pinctrl_resume_bank(
> +                               struct samsung_pinctrl_drv_data *drvdata,
> +                               struct samsung_pin_bank *bank)
> +{
> +       struct exynos_eint_gpio_save *save = bank->soc_priv;
> +       void __iomem *regs = drvdata->virt_base;
> +

Optional: debug statements:

pr_debug("%s:     con %#010x => %#010x\n", bank->name,
  readl(regs + EXYNOS_GPIO_ECON_OFFSET + bank->eint_offset),
  save->eint_con);
pr_debug("%s: fltcon0 %#010x => %#010x\n", bank->name,
  readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET + 2 * bank->eint_offset),
  save->eint_fltcon0);
pr_debug("%s: fltcon1 %#010x => %#010x\n", bank->name,
  readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET + 2 * bank->eint_offset + 4),
  save->eint_fltcon1);

-Doug

[PATCH 6/6] pinctrl: exynos: Handle suspend/resume of GPIO EINT registers

[Tomasz Figa]

On Friday 17 of May 2013 12:25:02 Doug Anderson wrote:
> Tomasz,
> 
> On Fri, May 17, 2013 at 9:24 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> > Some GPIO EINT control registers needs to be preserved across
> > suspend/resume cycle. This patch extends the driver to take care of
> > this.
> 
> I was confused why we didn't seem to need this on exynos5250-snow but
> figured it out.  We only use interrupts on GPX lines which don't need
> this code.  ...but on any exynos5250 boards that use one of the other
> banks for interrupts you'd need it.  I tested by setting one of the
> registers related to GPA0 and found that this code is indeed needed on
> exynos5250.  :)
> 
> Just nits / optional comments below, so on exynos5250-snow (pinmux
> backported to 3.8):
> 
> Tested-by: Doug Anderson <dianders@chromium.org>
> 
> Reviewed-by: Doug Anderson <dianders@chromium.org>
> 
> > @@ -229,6 +235,11 @@ static int exynos_eint_gpio_init(struct
> > samsung_pinctrl_drv_data *d)> 
> >                         dev_err(dev, "gpio irq domain add failed\n");
> >                         return -ENXIO;
> >                 
> >                 }
> > 
> > +
> > +               bank->soc_priv = devm_kzalloc(d->dev,
> > +                       sizeof(struct exynos_eint_gpio_save),
> > GFP_KERNEL); +               if (!bank->soc_priv)
> > +                       return -ENOMEM;
> 
> Slight nit to add this before the call to irq_domain_add_linear().
> demv() will handle freeing your memory but nothing will handle undoing
> the irq_domain_add_linear() if you return an error.

I'm a bit sceptical when it is about error handling in such cases. 
Basically if interrupt initialization fails, something is seriously wrong, 
either with your kernel config or with some code.

Since such case has been already unhandled in the driver (with nr_banks > 
1 = always), so I didn't bother to add any undoing here.

> >         }
> >         
> >         return 0;
> > 
> > @@ -528,6 +539,58 @@ static int exynos_eint_wkup_init(struct
> > samsung_pinctrl_drv_data *d)> 
> >         return 0;
> >  
> >  }
> > 
> > +static void exynos_pinctrl_suspend_bank(
> > +                               struct samsung_pinctrl_drv_data
> > *drvdata, +                               struct samsung_pin_bank
> > *bank) +{
> > +       struct exynos_eint_gpio_save *save = bank->soc_priv;
> > +       void __iomem *regs = drvdata->virt_base;
> > +
> > +       save->eint_con = readl(regs + EXYNOS_GPIO_ECON_OFFSET
> > +                                               + bank->eint_offset);
> > +       save->eint_fltcon0 = readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
> > +                                               + 2 *
> > bank->eint_offset); +       save->eint_fltcon1 = readl(regs +
> > EXYNOS_GPIO_EFLTCON_OFFSET +                                         
> >      + 2 * bank->eint_offset + 4);
> Optional: I wish there were debug statements to help debug, like:
> 
> pr_debug("%s: save     con %#010x\n", bank->name, save->eint_con);
> pr_debug("%s: save fltcon0 %#010x\n", bank->name, save->eint_fltcon0);
> pr_debug("%s: save fltcon1 %#010x\n", bank->name, save->eint_fltcon1);
> 

Right, seems reasonable.

> > +}
> > +
> > +static void exynos_pinctrl_suspend(struct samsung_pinctrl_drv_data
> > *drvdata) +{
> > +       struct samsung_pin_ctrl *ctrl = drvdata->ctrl;
> > +       struct samsung_pin_bank *bank = ctrl->pin_banks;
> > +       int i;
> > +
> > +       for (i = 0; i < ctrl->nr_banks; ++i, ++bank)
> > +               if (bank->eint_type == EINT_TYPE_GPIO)
> > +                       exynos_pinctrl_suspend_bank(drvdata, bank);
> > +}
> > +
> > +static void exynos_pinctrl_resume_bank(
> > +                               struct samsung_pinctrl_drv_data
> > *drvdata, +                               struct samsung_pin_bank
> > *bank) +{
> > +       struct exynos_eint_gpio_save *save = bank->soc_priv;
> > +       void __iomem *regs = drvdata->virt_base;
> > +
> 
> Optional: debug statements:
> 
> pr_debug("%s:     con %#010x => %#010x\n", bank->name,
>   readl(regs + EXYNOS_GPIO_ECON_OFFSET + bank->eint_offset),
>   save->eint_con);
> pr_debug("%s: fltcon0 %#010x => %#010x\n", bank->name,
>   readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET + 2 * bank->eint_offset),
>   save->eint_fltcon0);
> pr_debug("%s: fltcon1 %#010x => %#010x\n", bank->name,
>   readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET + 2 * bank->eint_offset + 4),
>   save->eint_fltcon1);

OK. I wonder if this could be added in a separate patch or I should rather 
send v2 on Monday?

Best regards,
Tomasz

[PATCH 6/6] pinctrl: exynos: Handle suspend/resume of GPIO EINT registers

[Doug Anderson]

Tomasz,

On Fri, May 17, 2013 at 1:34 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> Slight nit to add this before the call to irq_domain_add_linear().
>> demv() will handle freeing your memory but nothing will handle undoing
>> the irq_domain_add_linear() if you return an error.
>
> I'm a bit sceptical when it is about error handling in such cases.
> Basically if interrupt initialization fails, something is seriously wrong,
> either with your kernel config or with some code.
>
> Since such case has been already unhandled in the driver (with nr_banks >
> 1 = always), so I didn't bother to add any undoing here.

Yeah, not all drivers handle it well.  I'm always surprised by the
number of drivers that seem have it right, though.  Certainly it seems
awfully unlikely that allocating a small number of bytes in a probe
function would fail.

...but changing the order doesn't hurt anything and would make it more
correct, even if it's not fully correct.


>> Optional: debug statements:
>>
>> pr_debug("%s:     con %#010x => %#010x\n", bank->name,
>>   readl(regs + EXYNOS_GPIO_ECON_OFFSET + bank->eint_offset),
>>   save->eint_con);
>> pr_debug("%s: fltcon0 %#010x => %#010x\n", bank->name,
>>   readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET + 2 * bank->eint_offset),
>>   save->eint_fltcon0);
>> pr_debug("%s: fltcon1 %#010x => %#010x\n", bank->name,
>>   readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET + 2 * bank->eint_offset + 4),
>>   save->eint_fltcon1);
>
> OK. I wonder if this could be added in a separate patch or I should rather
> send v2 on Monday?

Definitely optional to add these, so up to you whether to spin the
patch, ignore my suggestion, or do a separate patch.  :)


Thanks for sending all of these up, by the way!  If things look good
next week I'll probably revert my local version of this (the V2 of my
original series) and pull in your series to keep us on the same page.
:)


-Doug

[PATCH v2 6/6] pinctrl: exynos: Handle suspend/resume of GPIO EINT registers

[Tomasz Figa]

Some GPIO EINT control registers needs to be preserved across
suspend/resume cycle. This patch extends the driver to take care of
this.

Signed-off-by: Tomasz Figa <t.figa@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---

Changes since v1:
 - Added optional debugging messages
 - Added proper error path in initialization

 drivers/pinctrl/pinctrl-exynos.c | 116 ++++++++++++++++++++++++++++++++++++++-
 drivers/pinctrl/pinctrl-exynos.h |   1 +
 2 files changed, 115 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-exynos.c b/drivers/pinctrl/pinctrl-exynos.c
index 4f868e5..864e2c0 100644
--- a/drivers/pinctrl/pinctrl-exynos.c
+++ b/drivers/pinctrl/pinctrl-exynos.c
@@ -196,6 +196,12 @@ static irqreturn_t exynos_eint_gpio_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+struct exynos_eint_gpio_save {
+	u32 eint_con;
+	u32 eint_fltcon0;
+	u32 eint_fltcon1;
+};
+
 /*
  * exynos_eint_gpio_init() - setup handling of external gpio interrupts.
  * @d: driver data of samsung pinctrl driver.
@@ -204,8 +210,9 @@ static int exynos_eint_gpio_init(struct samsung_pinctrl_drv_data *d)
 {
 	struct samsung_pin_bank *bank;
 	struct device *dev = d->dev;
-	unsigned int ret;
+	int ret;
 	unsigned int i;
+	unsigned int undo;
 
 	if (!d->irq) {
 		dev_err(dev, "irq number not available\n");
@@ -227,11 +234,30 @@ static int exynos_eint_gpio_init(struct samsung_pinctrl_drv_data *d)
 				bank->nr_pins, &exynos_gpio_irqd_ops, bank);
 		if (!bank->irq_domain) {
 			dev_err(dev, "gpio irq domain add failed\n");
-			return -ENXIO;
+			ret = -ENXIO;
+			goto err_domains;
+		}
+
+		bank->soc_priv = devm_kzalloc(d->dev,
+			sizeof(struct exynos_eint_gpio_save), GFP_KERNEL);
+		if (!bank->soc_priv) {
+			++i;
+			ret = -ENOMEM;
+			goto err_domains;
 		}
 	}
 
 	return 0;
+
+err_domains:
+	bank = d->ctrl->pin_banks;
+	for (undo = 0; undo < i; ++undo) {
+		if (bank->eint_type != EINT_TYPE_GPIO)
+			continue;
+		irq_domain_remove(bank->irq_domain);
+	}
+
+	return ret;
 }
 
 static void exynos_wkup_irq_unmask(struct irq_data *irqd)
@@ -528,6 +554,72 @@ static int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d)
 	return 0;
 }
 
+static void exynos_pinctrl_suspend_bank(
+				struct samsung_pinctrl_drv_data *drvdata,
+				struct samsung_pin_bank *bank)
+{
+	struct exynos_eint_gpio_save *save = bank->soc_priv;
+	void __iomem *regs = drvdata->virt_base;
+
+	save->eint_con = readl(regs + EXYNOS_GPIO_ECON_OFFSET
+						+ bank->eint_offset);
+	save->eint_fltcon0 = readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
+						+ 2 * bank->eint_offset);
+	save->eint_fltcon1 = readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
+						+ 2 * bank->eint_offset + 4);
+
+	pr_debug("%s: save     con %#010x\n", bank->name, save->eint_con);
+	pr_debug("%s: save fltcon0 %#010x\n", bank->name, save->eint_fltcon0);
+	pr_debug("%s: save fltcon1 %#010x\n", bank->name, save->eint_fltcon1);
+}
+
+static void exynos_pinctrl_suspend(struct samsung_pinctrl_drv_data *drvdata)
+{
+	struct samsung_pin_ctrl *ctrl = drvdata->ctrl;
+	struct samsung_pin_bank *bank = ctrl->pin_banks;
+	int i;
+
+	for (i = 0; i < ctrl->nr_banks; ++i, ++bank)
+		if (bank->eint_type == EINT_TYPE_GPIO)
+			exynos_pinctrl_suspend_bank(drvdata, bank);
+}
+
+static void exynos_pinctrl_resume_bank(
+				struct samsung_pinctrl_drv_data *drvdata,
+				struct samsung_pin_bank *bank)
+{
+	struct exynos_eint_gpio_save *save = bank->soc_priv;
+	void __iomem *regs = drvdata->virt_base;
+
+	pr_debug("%s:     con %#010x => %#010x\n", bank->name,
+			readl(regs + EXYNOS_GPIO_ECON_OFFSET
+			+ bank->eint_offset), save->eint_con);
+	pr_debug("%s: fltcon0 %#010x => %#010x\n", bank->name,
+			readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
+			+ 2 * bank->eint_offset), save->eint_fltcon0);
+	pr_debug("%s: fltcon1 %#010x => %#010x\n", bank->name,
+			readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
+			+ 2 * bank->eint_offset + 4), save->eint_fltcon1);
+
+	writel(save->eint_con, regs + EXYNOS_GPIO_ECON_OFFSET
+						+ bank->eint_offset);
+	writel(save->eint_fltcon0, regs + EXYNOS_GPIO_EFLTCON_OFFSET
+						+ 2 * bank->eint_offset);
+	writel(save->eint_fltcon1, regs + EXYNOS_GPIO_EFLTCON_OFFSET
+						+ 2 * bank->eint_offset + 4);
+}
+
+static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
+{
+	struct samsung_pin_ctrl *ctrl = drvdata->ctrl;
+	struct samsung_pin_bank *bank = ctrl->pin_banks;
+	int i;
+
+	for (i = 0; i < ctrl->nr_banks; ++i, ++bank)
+		if (bank->eint_type == EINT_TYPE_GPIO)
+			exynos_pinctrl_resume_bank(drvdata, bank);
+}
+
 /* pin banks of exynos4210 pin-controller 0 */
 static struct samsung_pin_bank exynos4210_pin_banks0[] = {
 	EXYNOS_PIN_BANK_EINTG(8, 0x000, "gpa0", 0x00),
@@ -591,6 +683,8 @@ struct samsung_pin_ctrl exynos4210_pin_ctrl[] = {
 		.geint_pend	= EXYNOS_GPIO_EPEND_OFFSET,
 		.svc		= EXYNOS_SVC_OFFSET,
 		.eint_gpio_init = exynos_eint_gpio_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
 		.label		= "exynos4210-gpio-ctrl0",
 	}, {
 		/* pin-controller instance 1 data */
@@ -605,6 +699,8 @@ struct samsung_pin_ctrl exynos4210_pin_ctrl[] = {
 		.svc		= EXYNOS_SVC_OFFSET,
 		.eint_gpio_init = exynos_eint_gpio_init,
 		.eint_wkup_init = exynos_eint_wkup_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
 		.label		= "exynos4210-gpio-ctrl1",
 	}, {
 		/* pin-controller instance 2 data */
@@ -686,6 +782,8 @@ struct samsung_pin_ctrl exynos4x12_pin_ctrl[] = {
 		.geint_pend	= EXYNOS_GPIO_EPEND_OFFSET,
 		.svc		= EXYNOS_SVC_OFFSET,
 		.eint_gpio_init = exynos_eint_gpio_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
 		.label		= "exynos4x12-gpio-ctrl0",
 	}, {
 		/* pin-controller instance 1 data */
@@ -700,6 +798,8 @@ struct samsung_pin_ctrl exynos4x12_pin_ctrl[] = {
 		.svc		= EXYNOS_SVC_OFFSET,
 		.eint_gpio_init = exynos_eint_gpio_init,
 		.eint_wkup_init = exynos_eint_wkup_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
 		.label		= "exynos4x12-gpio-ctrl1",
 	}, {
 		/* pin-controller instance 2 data */
@@ -710,6 +810,8 @@ struct samsung_pin_ctrl exynos4x12_pin_ctrl[] = {
 		.geint_pend	= EXYNOS_GPIO_EPEND_OFFSET,
 		.svc		= EXYNOS_SVC_OFFSET,
 		.eint_gpio_init = exynos_eint_gpio_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
 		.label		= "exynos4x12-gpio-ctrl2",
 	}, {
 		/* pin-controller instance 3 data */
@@ -720,6 +822,8 @@ struct samsung_pin_ctrl exynos4x12_pin_ctrl[] = {
 		.geint_pend	= EXYNOS_GPIO_EPEND_OFFSET,
 		.svc		= EXYNOS_SVC_OFFSET,
 		.eint_gpio_init = exynos_eint_gpio_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
 		.label		= "exynos4x12-gpio-ctrl3",
 	},
 };
@@ -798,6 +902,8 @@ struct samsung_pin_ctrl exynos5250_pin_ctrl[] = {
 		.svc		= EXYNOS_SVC_OFFSET,
 		.eint_gpio_init = exynos_eint_gpio_init,
 		.eint_wkup_init = exynos_eint_wkup_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
 		.label		= "exynos5250-gpio-ctrl0",
 	}, {
 		/* pin-controller instance 1 data */
@@ -808,6 +914,8 @@ struct samsung_pin_ctrl exynos5250_pin_ctrl[] = {
 		.geint_pend	= EXYNOS_GPIO_EPEND_OFFSET,
 		.svc		= EXYNOS_SVC_OFFSET,
 		.eint_gpio_init = exynos_eint_gpio_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
 		.label		= "exynos5250-gpio-ctrl1",
 	}, {
 		/* pin-controller instance 2 data */
@@ -818,6 +926,8 @@ struct samsung_pin_ctrl exynos5250_pin_ctrl[] = {
 		.geint_pend	= EXYNOS_GPIO_EPEND_OFFSET,
 		.svc		= EXYNOS_SVC_OFFSET,
 		.eint_gpio_init = exynos_eint_gpio_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
 		.label		= "exynos5250-gpio-ctrl2",
 	}, {
 		/* pin-controller instance 3 data */
@@ -828,6 +938,8 @@ struct samsung_pin_ctrl exynos5250_pin_ctrl[] = {
 		.geint_pend	= EXYNOS_GPIO_EPEND_OFFSET,
 		.svc		= EXYNOS_SVC_OFFSET,
 		.eint_gpio_init = exynos_eint_gpio_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
 		.label		= "exynos5250-gpio-ctrl3",
 	},
 };
diff --git a/drivers/pinctrl/pinctrl-exynos.h b/drivers/pinctrl/pinctrl-exynos.h
index 9b1f77a..3c91c35 100644
--- a/drivers/pinctrl/pinctrl-exynos.h
+++ b/drivers/pinctrl/pinctrl-exynos.h
@@ -19,6 +19,7 @@
 
 /* External GPIO and wakeup interrupt related definitions */
 #define EXYNOS_GPIO_ECON_OFFSET		0x700
+#define EXYNOS_GPIO_EFLTCON_OFFSET	0x800
 #define EXYNOS_GPIO_EMASK_OFFSET	0x900
 #define EXYNOS_GPIO_EPEND_OFFSET	0xA00
 #define EXYNOS_WKUP_ECON_OFFSET		0xE00
-- 
1.8.2.1

[PATCH v2 6/6] pinctrl: exynos: Handle suspend/resume of GPIO EINT registers

[Doug Anderson]

Tomasz,

On Tue, May 21, 2013 at 10:05 AM, Tomasz Figa <t.figa@samsung.com> wrote:
>         return 0;
> +
> +err_domains:
> +       bank = d->ctrl->pin_banks;
> +       for (undo = 0; undo < i; ++undo) {

++bank is missing, I think.  This will free the same bank many times.

optional: could also avoid extra 'undo' variable and remove in reverse
order by changing i to signed and counting down to 0.

-Doug

[PATCH v2 6/6] pinctrl: exynos: Handle suspend/resume of GPIO EINT registers

[Tomasz Figa]

On Tuesday 21 of May 2013 21:46:35 Doug Anderson wrote:
> Tomasz,
> 
> On Tue, May 21, 2013 at 10:05 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> >         return 0;
> > 
> > +
> > +err_domains:
> > +       bank = d->ctrl->pin_banks;
> > +       for (undo = 0; undo < i; ++undo) {
> 
> ++bank is missing, I think.  This will free the same bank many times.

Shit, this is what happens when you post patches so late and in a hurry. Will 
send something sane in a while.

> optional: could also avoid extra 'undo' variable and remove in reverse
> order by changing i to signed and counting down to 0.

Sure.

Best regards,
Tomasz

> -Doug
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc"
> in the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 6/6] pinctrl: exynos: Handle suspend/resume of GPIO EINT registers

[Tomasz Figa]

Some GPIO EINT control registers needs to be preserved across
suspend/resume cycle. This patch extends the driver to take care of
this.

Signed-off-by: Tomasz Figa <t.figa@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---

Changes since v2:
 - Fixed error path
Changes since v1:
 - Added optional debugging messages
 - Added proper error path in initialization

 drivers/pinctrl/pinctrl-exynos.c | 116 ++++++++++++++++++++++++++++++++++++++-
 drivers/pinctrl/pinctrl-exynos.h |   1 +
 2 files changed, 114 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-exynos.c b/drivers/pinctrl/pinctrl-exynos.c
index 4f868e5..2d76f66 100644
--- a/drivers/pinctrl/pinctrl-exynos.c
+++ b/drivers/pinctrl/pinctrl-exynos.c
@@ -196,6 +196,12 @@ static irqreturn_t exynos_eint_gpio_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+struct exynos_eint_gpio_save {
+	u32 eint_con;
+	u32 eint_fltcon0;
+	u32 eint_fltcon1;
+};
+
 /*
  * exynos_eint_gpio_init() - setup handling of external gpio interrupts.
  * @d: driver data of samsung pinctrl driver.
@@ -204,8 +210,8 @@ static int exynos_eint_gpio_init(struct samsung_pinctrl_drv_data *d)
 {
 	struct samsung_pin_bank *bank;
 	struct device *dev = d->dev;
-	unsigned int ret;
-	unsigned int i;
+	int ret;
+	int i;
 
 	if (!d->irq) {
 		dev_err(dev, "irq number not available\n");
@@ -227,11 +233,29 @@ static int exynos_eint_gpio_init(struct samsung_pinctrl_drv_data *d)
 				bank->nr_pins, &exynos_gpio_irqd_ops, bank);
 		if (!bank->irq_domain) {
 			dev_err(dev, "gpio irq domain add failed\n");
-			return -ENXIO;
+			ret = -ENXIO;
+			goto err_domains;
+		}
+
+		bank->soc_priv = devm_kzalloc(d->dev,
+			sizeof(struct exynos_eint_gpio_save), GFP_KERNEL);
+		if (!bank->soc_priv) {
+			irq_domain_remove(bank->irq_domain);
+			ret = -ENOMEM;
+			goto err_domains;
 		}
 	}
 
 	return 0;
+
+err_domains:
+	for (--i, --bank; i >= 0; --i, --bank) {
+		if (bank->eint_type != EINT_TYPE_GPIO)
+			continue;
+		irq_domain_remove(bank->irq_domain);
+	}
+
+	return ret;
 }
 
 static void exynos_wkup_irq_unmask(struct irq_data *irqd)
@@ -528,6 +552,72 @@ static int exynos_eint_wkup_init(struct samsung_pinctrl_drv_data *d)
 	return 0;
 }
 
+static void exynos_pinctrl_suspend_bank(
+				struct samsung_pinctrl_drv_data *drvdata,
+				struct samsung_pin_bank *bank)
+{
+	struct exynos_eint_gpio_save *save = bank->soc_priv;
+	void __iomem *regs = drvdata->virt_base;
+
+	save->eint_con = readl(regs + EXYNOS_GPIO_ECON_OFFSET
+						+ bank->eint_offset);
+	save->eint_fltcon0 = readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
+						+ 2 * bank->eint_offset);
+	save->eint_fltcon1 = readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
+						+ 2 * bank->eint_offset + 4);
+
+	pr_debug("%s: save     con %#010x\n", bank->name, save->eint_con);
+	pr_debug("%s: save fltcon0 %#010x\n", bank->name, save->eint_fltcon0);
+	pr_debug("%s: save fltcon1 %#010x\n", bank->name, save->eint_fltcon1);
+}
+
+static void exynos_pinctrl_suspend(struct samsung_pinctrl_drv_data *drvdata)
+{
+	struct samsung_pin_ctrl *ctrl = drvdata->ctrl;
+	struct samsung_pin_bank *bank = ctrl->pin_banks;
+	int i;
+
+	for (i = 0; i < ctrl->nr_banks; ++i, ++bank)
+		if (bank->eint_type == EINT_TYPE_GPIO)
+			exynos_pinctrl_suspend_bank(drvdata, bank);
+}
+
+static void exynos_pinctrl_resume_bank(
+				struct samsung_pinctrl_drv_data *drvdata,
+				struct samsung_pin_bank *bank)
+{
+	struct exynos_eint_gpio_save *save = bank->soc_priv;
+	void __iomem *regs = drvdata->virt_base;
+
+	pr_debug("%s:     con %#010x => %#010x\n", bank->name,
+			readl(regs + EXYNOS_GPIO_ECON_OFFSET
+			+ bank->eint_offset), save->eint_con);
+	pr_debug("%s: fltcon0 %#010x => %#010x\n", bank->name,
+			readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
+			+ 2 * bank->eint_offset), save->eint_fltcon0);
+	pr_debug("%s: fltcon1 %#010x => %#010x\n", bank->name,
+			readl(regs + EXYNOS_GPIO_EFLTCON_OFFSET
+			+ 2 * bank->eint_offset + 4), save->eint_fltcon1);
+
+	writel(save->eint_con, regs + EXYNOS_GPIO_ECON_OFFSET
+						+ bank->eint_offset);
+	writel(save->eint_fltcon0, regs + EXYNOS_GPIO_EFLTCON_OFFSET
+						+ 2 * bank->eint_offset);
+	writel(save->eint_fltcon1, regs + EXYNOS_GPIO_EFLTCON_OFFSET
+						+ 2 * bank->eint_offset + 4);
+}
+
+static void exynos_pinctrl_resume(struct samsung_pinctrl_drv_data *drvdata)
+{
+	struct samsung_pin_ctrl *ctrl = drvdata->ctrl;
+	struct samsung_pin_bank *bank = ctrl->pin_banks;
+	int i;
+
+	for (i = 0; i < ctrl->nr_banks; ++i, ++bank)
+		if (bank->eint_type == EINT_TYPE_GPIO)
+			exynos_pinctrl_resume_bank(drvdata, bank);
+}
+
 /* pin banks of exynos4210 pin-controller 0 */
 static struct samsung_pin_bank exynos4210_pin_banks0[] = {
 	EXYNOS_PIN_BANK_EINTG(8, 0x000, "gpa0", 0x00),
@@ -591,6 +681,8 @@ struct samsung_pin_ctrl exynos4210_pin_ctrl[] = {
 		.geint_pend	= EXYNOS_GPIO_EPEND_OFFSET,
 		.svc		= EXYNOS_SVC_OFFSET,
 		.eint_gpio_init = exynos_eint_gpio_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
 		.label		= "exynos4210-gpio-ctrl0",
 	}, {
 		/* pin-controller instance 1 data */
@@ -605,6 +697,8 @@ struct samsung_pin_ctrl exynos4210_pin_ctrl[] = {
 		.svc		= EXYNOS_SVC_OFFSET,
 		.eint_gpio_init = exynos_eint_gpio_init,
 		.eint_wkup_init = exynos_eint_wkup_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
 		.label		= "exynos4210-gpio-ctrl1",
 	}, {
 		/* pin-controller instance 2 data */
@@ -686,6 +780,8 @@ struct samsung_pin_ctrl exynos4x12_pin_ctrl[] = {
 		.geint_pend	= EXYNOS_GPIO_EPEND_OFFSET,
 		.svc		= EXYNOS_SVC_OFFSET,
 		.eint_gpio_init = exynos_eint_gpio_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
 		.label		= "exynos4x12-gpio-ctrl0",
 	}, {
 		/* pin-controller instance 1 data */
@@ -700,6 +796,8 @@ struct samsung_pin_ctrl exynos4x12_pin_ctrl[] = {
 		.svc		= EXYNOS_SVC_OFFSET,
 		.eint_gpio_init = exynos_eint_gpio_init,
 		.eint_wkup_init = exynos_eint_wkup_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
 		.label		= "exynos4x12-gpio-ctrl1",
 	}, {
 		/* pin-controller instance 2 data */
@@ -710,6 +808,8 @@ struct samsung_pin_ctrl exynos4x12_pin_ctrl[] = {
 		.geint_pend	= EXYNOS_GPIO_EPEND_OFFSET,
 		.svc		= EXYNOS_SVC_OFFSET,
 		.eint_gpio_init = exynos_eint_gpio_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
 		.label		= "exynos4x12-gpio-ctrl2",
 	}, {
 		/* pin-controller instance 3 data */
@@ -720,6 +820,8 @@ struct samsung_pin_ctrl exynos4x12_pin_ctrl[] = {
 		.geint_pend	= EXYNOS_GPIO_EPEND_OFFSET,
 		.svc		= EXYNOS_SVC_OFFSET,
 		.eint_gpio_init = exynos_eint_gpio_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
 		.label		= "exynos4x12-gpio-ctrl3",
 	},
 };
@@ -798,6 +900,8 @@ struct samsung_pin_ctrl exynos5250_pin_ctrl[] = {
 		.svc		= EXYNOS_SVC_OFFSET,
 		.eint_gpio_init = exynos_eint_gpio_init,
 		.eint_wkup_init = exynos_eint_wkup_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
 		.label		= "exynos5250-gpio-ctrl0",
 	}, {
 		/* pin-controller instance 1 data */
@@ -808,6 +912,8 @@ struct samsung_pin_ctrl exynos5250_pin_ctrl[] = {
 		.geint_pend	= EXYNOS_GPIO_EPEND_OFFSET,
 		.svc		= EXYNOS_SVC_OFFSET,
 		.eint_gpio_init = exynos_eint_gpio_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
 		.label		= "exynos5250-gpio-ctrl1",
 	}, {
 		/* pin-controller instance 2 data */
@@ -818,6 +924,8 @@ struct samsung_pin_ctrl exynos5250_pin_ctrl[] = {
 		.geint_pend	= EXYNOS_GPIO_EPEND_OFFSET,
 		.svc		= EXYNOS_SVC_OFFSET,
 		.eint_gpio_init = exynos_eint_gpio_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
 		.label		= "exynos5250-gpio-ctrl2",
 	}, {
 		/* pin-controller instance 3 data */
@@ -828,6 +936,8 @@ struct samsung_pin_ctrl exynos5250_pin_ctrl[] = {
 		.geint_pend	= EXYNOS_GPIO_EPEND_OFFSET,
 		.svc		= EXYNOS_SVC_OFFSET,
 		.eint_gpio_init = exynos_eint_gpio_init,
+		.suspend	= exynos_pinctrl_suspend,
+		.resume		= exynos_pinctrl_resume,
 		.label		= "exynos5250-gpio-ctrl3",
 	},
 };
diff --git a/drivers/pinctrl/pinctrl-exynos.h b/drivers/pinctrl/pinctrl-exynos.h
index 9b1f77a..3c91c35 100644
--- a/drivers/pinctrl/pinctrl-exynos.h
+++ b/drivers/pinctrl/pinctrl-exynos.h
@@ -19,6 +19,7 @@
 
 /* External GPIO and wakeup interrupt related definitions */
 #define EXYNOS_GPIO_ECON_OFFSET		0x700
+#define EXYNOS_GPIO_EFLTCON_OFFSET	0x800
 #define EXYNOS_GPIO_EMASK_OFFSET	0x900
 #define EXYNOS_GPIO_EPEND_OFFSET	0xA00
 #define EXYNOS_WKUP_ECON_OFFSET		0xE00
-- 
1.8.2.1

[PATCH v3 6/6] pinctrl: exynos: Handle suspend/resume of GPIO EINT registers

[Doug Anderson]

Tomasz,

On Wed, May 22, 2013 at 7:03 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> Some GPIO EINT control registers needs to be preserved across
> suspend/resume cycle. This patch extends the driver to take care of
> this.
>
> Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>
> Changes since v2:
>  - Fixed error path
> Changes since v1:
>  - Added optional debugging messages
>  - Added proper error path in initialization
>
>  drivers/pinctrl/pinctrl-exynos.c | 116 ++++++++++++++++++++++++++++++++++++++-
>  drivers/pinctrl/pinctrl-exynos.h |   1 +
>  2 files changed, 114 insertions(+), 3 deletions(-)

Looks good to me.  Thanks!

Good thing I tried the printouts too--they pointed to the fact that
"git am" had somehow applied part 4 ("Add support for SoC-specific
suspend/resume callbacks") incorrectly and with no warning!  It had
put the resume() call at the beginning of the suspend function.
Weird.  I had to fall back to using "patch -p1 to apply".  :-/


On exynos5250-snow (pinmux backported to 3.8):

Tested-by: Doug Anderson <dianders@chromium.org>

Reviewed-by: Doug Anderson <dianders@chromium.org>

[PATCH v3 6/6] pinctrl: exynos: Handle suspend/resume of GPIO EINT registers

[Linus Walleij]

On Wed, May 22, 2013 at 4:03 PM, Tomasz Figa <t.figa@samsung.com> wrote:

> Some GPIO EINT control registers needs to be preserved across
> suspend/resume cycle. This patch extends the driver to take care of
> this.
>
> Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>
> Changes since v2:
>  - Fixed error path
> Changes since v1:
>  - Added optional debugging messages
>  - Added proper error path in initialization

v2 version applied for fixes.

Again confirm you must really have all this. It is not looking
good with diffstats like this for fixing regressions.

Yours,
Linus Walleij

[PATCH v3 6/6] pinctrl: exynos: Handle suspend/resume of GPIO EINT registers

[Tomasz Figa]

Hi Linus,

On Friday 24 of May 2013 11:12:00 Linus Walleij wrote:
> On Wed, May 22, 2013 at 4:03 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> > Some GPIO EINT control registers needs to be preserved across
> > suspend/resume cycle. This patch extends the driver to take care of
> > this.
> > 
> > Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> > 
> > Changes since v2:
> >  - Fixed error path
> > 
> > Changes since v1:
> >  - Added optional debugging messages
> >  - Added proper error path in initialization
> 
> v2 version applied for fixes.

I posted v3 two days ago. It fixes a stupid mistake in error path I made in a 
hurry and also addresses some suggestions from Doug.

> Again confirm you must really have all this. It is not looking
> good with diffstats like this for fixing regressions.

Yes. The whole series is necessary to have correct suspend/resume support of 
DT-enabled Exynos boards.

Best regards,
-- 
Tomasz Figa
Linux Kernel Developer
Samsung R&D Institute Poland
Samsung Electronics

[PATCH 0/6] Fix suspend/resume issues created by pinmux on exynos, part 2

[Tushar Behera]

On 05/17/2013 09:54 PM, Tomasz Figa wrote:
> This series aims at fixing problems with suspend/resume on Exynos SoCs
> introduced by migration of GPIO and EINT support to pin control driver
> on DT-enabled platforms.
> 
> The patches fix following issues:
>  - missing support for IRQ wake of pinctrl-exynos driver
>  - legacy GPIO and EINT save/restore handling in platform PM code
>    inappropriate on pinctrl-enabled platforms
>  - several EINT-related registers must be saved as well
>  
> The series is based on a patch by Doug Anderson that adds suspend/restore
> of pinctrl registers to pinctrl-samsung driver:
> [PATCH v3] pinctrl: samsung: fix suspend/resume functionality
> http://www.spinics.net/lists/kernel/msg1534119.html
> 
> On Exynos4210-based Trats board:
> Tested-by: Tomasz Figa <t.figa@samsung.com>
> 

On Exynos5250-based Arndale board:
Tested-by: Tushar Behera <tushar.behera@linaro.org>

> Tomasz Figa (6):
>   pinctrl: exynos: Add support for set_irq_wake of wake-up EINTs
>   ARM: EXYNOS: Fix EINT wake-up mask configuration when pinctrl is used
>   ARM: SAMSUNG: pm: Adjust for pinctrl- and DT-enabled platforms
>   pinctrl: samsung: Add support for SoC-specific suspend/resume
>     callbacks
>   pinctrl: samsung: Allow per-bank SoC-specific private data
>   pinctrl: exynos: Handle suspend/resume of GPIO EINT registers
> 
>  arch/arm/mach-exynos/include/mach/pm-core.h |  14 +++-
>  arch/arm/plat-samsung/pm.c                  |  17 +++--
>  drivers/pinctrl/pinctrl-exynos.c            | 106 ++++++++++++++++++++++++++++
>  drivers/pinctrl/pinctrl-exynos.h            |   1 +
>  drivers/pinctrl/pinctrl-samsung.c           |   6 ++
>  drivers/pinctrl/pinctrl-samsung.h           |   4 ++
>  6 files changed, 142 insertions(+), 6 deletions(-)
> 


-- 
Tushar Behera