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

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

[Doug Anderson]

This set of patches fixes some problems with suspend/resume that were
introduced by the switch from the old gpio code to the new pinmux
code.  Specifically:
* It adds saving and restoring of pincontrol registers.
* It fixes eint wakeups.

This set of two patches was verified on a backport of the current
pinmux code onto 3.8 on a Samsung ARM Chromebook.  Suspend/resume does
not seem functional on the ARM Chromebook on current ToT Linux so I
couldn't validate there.  This gets us one step closer, though!  Since
patches applied cleanly I'm fairly certain that they will work on ToT
as well as they do in our tree.

These patches have only been tested on exynos5250.  I have made an
effort to support other samsung boards (even those with two CONF
registers), but that support is untested.

Tomasz Figa has said that he has similar patches in development.  I'm
posting what we have here but if Tomasz's patches end up being more
suitable I have no objections to taking them over these (or of Tomasz
wants to merge the two?).

If you'd like to see the gerrit reviews of these in the Chrome OS tree,
you can see:
* https://gerrit.chromium.org/gerrit/#/c/51336/4
* https://gerrit.chromium.org/gerrit/#/c/51342/3


Doug Anderson (1):
  pinctrl: samsung: fix suspend/resume functionality

Prathyush K (1):
  pinctrl: exynos: fix eint wakeup by using irq_set_wake()

 drivers/pinctrl/pinctrl-exynos.c  |  45 ++++++---
 drivers/pinctrl/pinctrl-exynos.h  |   3 +-
 drivers/pinctrl/pinctrl-samsung.c | 199 ++++++++++++++++++++++++++++++++++++++
 drivers/pinctrl/pinctrl-samsung.h |  13 +++
 4 files changed, 247 insertions(+), 13 deletions(-)

-- 
1.8.2.1

[PATCH 1/2] pinctrl: samsung: fix suspend/resume functionality

[Doug Anderson]

The GPIO states need to be restored after s2r and this is not currently
supported in the pinctrl driver. This patch saves the gpio states before
suspend and restores them after resume.

Logic and commenting for samsung_pinctrl_resume_noirq() is heavily
borrowed from the old samsung_gpio_pm_2bit_resume(), which seemed to
do this a reasonable way.

Patch originally from Prathyush K <prathyush.k@samsung.com> but
rewritten by Doug Anderson <dianders@chromium.org>.

Signed-off-by: Prathyush K <prathyush.k@samsung.com>
Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 drivers/pinctrl/pinctrl-samsung.c | 199 ++++++++++++++++++++++++++++++++++++++
 drivers/pinctrl/pinctrl-samsung.h |  11 +++
 2 files changed, 210 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-samsung.c b/drivers/pinctrl/pinctrl-samsung.c
index 9763668..0891667 100644
--- a/drivers/pinctrl/pinctrl-samsung.c
+++ b/drivers/pinctrl/pinctrl-samsung.c
@@ -964,6 +964,204 @@ static int samsung_pinctrl_probe(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM
+
+/**
+ * samsung_pinctrl_resume_noirq - save pinctrl state for suspend
+ *
+ * Save data for all banks handled by this device.
+ */
+static int samsung_pinctrl_suspend_noirq(struct device *dev)
+{
+	struct samsung_pinctrl_drv_data *drvdata = dev_get_drvdata(dev);
+	struct samsung_pin_ctrl *ctrl = drvdata->ctrl;
+	void __iomem * const virt_base = drvdata->virt_base;
+	int i;
+
+	for (i = 0; i < ctrl->nr_banks; i++) {
+		struct samsung_pin_bank *bank = &ctrl->pin_banks[i];
+		const struct samsung_pin_bank_type *type = bank->type;
+		void __iomem * const reg = virt_base + bank->pctl_offset;
+
+		bank->pm_save.con = readl(reg +
+					  type->reg_offset[PINCFG_TYPE_FUNC]);
+		if (type->fld_width[PINCFG_TYPE_FUNC] > 4)
+			bank->pm_save.con |= (u64)readl(reg + 4 +
+				      type->reg_offset[PINCFG_TYPE_FUNC]) << 32;
+		bank->pm_save.dat = readl(reg +
+					  type->reg_offset[PINCFG_TYPE_DAT]);
+		bank->pm_save.pud = readl(reg +
+					  type->reg_offset[PINCFG_TYPE_PUD]);
+		bank->pm_save.drv = readl(reg +
+					  type->reg_offset[PINCFG_TYPE_DRV]);
+
+		if (type->fld_width[PINCFG_TYPE_CON_PDN]) {
+			bank->pm_save.conpdn = readl(reg +
+				type->reg_offset[PINCFG_TYPE_CON_PDN]);
+			bank->pm_save.pudpdn = readl(reg +
+				type->reg_offset[PINCFG_TYPE_PUD_PDN]);
+		}
+
+		dev_dbg(dev, "Save %s @ %p (con %#010llx)\n",
+			bank->name, reg, bank->pm_save.con);
+	}
+
+	return 0;
+}
+
+/**
+ * is_sfn - test whether a pin config represents special function.
+ *
+ * Test whether the given masked+shifted bits of an GPIO configuration
+ * are one of the SFN (special function) modes.
+ */
+static inline int is_sfn(u32 con)
+{
+	return con >= 2;
+}
+
+/**
+ * is_in - test if the given masked+shifted GPIO configuration is an input.
+ */
+static inline int is_in(u32 con)
+{
+	return con == 0;
+}
+
+/**
+ * is_out - test if the given masked+shifted GPIO configuration is an output.
+ */
+static inline int is_out(u32 con)
+{
+	return con == 1;
+}
+
+/**
+ * samsung_pinctrl_resume_noirq - restore pinctrl state from suspend
+ *
+ * Restore one of the GPIO banks that was saved during suspend. This is
+ * not as simple as once thought, due to the possibility of glitches
+ * from the order that the CON and DAT registers are set in.
+ *
+ * The three states the pin can be are {IN,OUT,SFN} which gives us 9
+ * combinations of changes to check. Three of these, if the pin stays
+ * in the same configuration can be discounted. This leaves us with
+ * the following:
+ *
+ * { IN => OUT }  Change DAT first
+ * { IN => SFN }  Change CON first
+ * { OUT => SFN } Change CON first, so new data will not glitch
+ * { OUT => IN }  Change CON first, so new data will not glitch
+ * { SFN => IN }  Change CON first
+ * { SFN => OUT } Change DAT first, so new data will not glitch [1]
+ *
+ * We do not currently deal with the UP registers as these control
+ * weak resistors, so a small delay in change should not need to bring
+ * these into the calculations.
+ *
+ * [1] this assumes that writing to a pin DAT whilst in SFN will set the
+ *     state for when it is next output.
+ */
+static int samsung_pinctrl_resume_noirq(struct device *dev)
+{
+	struct samsung_pinctrl_drv_data *drvdata = dev_get_drvdata(dev);
+	struct samsung_pin_ctrl *ctrl = drvdata->ctrl;
+	void __iomem * const virt_base = drvdata->virt_base;
+	int i;
+
+	for (i = 0; i < ctrl->nr_banks; i++) {
+		const struct samsung_pin_bank *bank = &ctrl->pin_banks[i];
+		void __iomem * const reg = virt_base + bank->pctl_offset;
+		const struct samsung_pin_bank_type *type = bank->type;
+		const u8 func_offset = type->reg_offset[PINCFG_TYPE_FUNC];
+		const u32 func_width = type->fld_width[PINCFG_TYPE_FUNC];
+		const u32 func_mask = (1 << func_width) - 1;
+		const bool is64bit = type->fld_width[PINCFG_TYPE_FUNC] > 4;
+		const u64 to_con = bank->pm_save.con;
+		u64 from_con = readl(reg + func_offset);
+		u64 change_mask = 0;
+		u64 to_write;
+		int pin;
+
+		if (is64bit)
+			from_con |= (u64)readl(reg + 4 + func_offset) << 32;
+
+		/* Easy--the PUD and DRV can go first */
+		writel(bank->pm_save.pud,
+		       reg + type->reg_offset[PINCFG_TYPE_PUD]);
+		writel(bank->pm_save.drv,
+		       reg + type->reg_offset[PINCFG_TYPE_DRV]);
+
+		/*
+		 * Create a change_mask of all the items that need to have
+		 * their CON value changed before their DAT value, so that
+		 * we minimise the work between the two settings.
+		 */
+		for (pin = 0; pin < bank->nr_pins; pin++) {
+			u32 shift = pin * func_width;
+			u32 from_func = (from_con >> shift) & func_mask;
+			u32 to_func = (to_con >> shift) & func_mask;
+
+			/* If there is no change, then skip */
+			if (from_func == to_func)
+				continue;
+
+			/* If both are special function, then skip */
+			if (is_sfn(from_func) && is_sfn(to_func))
+				continue;
+
+			/* Change is IN => OUT, do not change now */
+			if (is_in(from_func) && is_out(to_func))
+				continue;
+
+			/* Change is SFN => OUT, do not change now */
+			if (is_sfn(from_func) && is_out(to_func))
+				continue;
+
+			/*
+			 * We should now be at the case of:
+			 *   IN=>SFN, OUT=>SFN, OUT=>IN, SFN=>IN.
+			 */
+			change_mask |= (func_mask << shift);
+		}
+
+		/* Write the new CON settings */
+		to_write = (from_con & ~change_mask) | (to_con & change_mask);
+		writel((u32)to_write, reg + func_offset);
+		if (is64bit)
+			writel((u32)(to_write >> 32), reg + 4 + func_offset);
+
+		/* Now change any items that require DAT,CON */
+		writel(bank->pm_save.dat,
+		       reg + type->reg_offset[PINCFG_TYPE_DAT]);
+		writel((u32)to_con, reg + func_offset);
+		if (is64bit)
+			writel((u32)(to_con >> 32), reg + 4 + to_con);
+
+		if (type->fld_width[PINCFG_TYPE_CON_PDN]) {
+			writel(bank->pm_save.conpdn,
+			       reg + type->reg_offset[PINCFG_TYPE_CON_PDN]);
+			writel(bank->pm_save.pudpdn,
+			       reg + type->reg_offset[PINCFG_TYPE_PUD_PDN]);
+		}
+
+		dev_dbg(dev, "Restore %s@%p (%#010llx=>%#010llx ch %#010llx)\n",
+			bank->name, reg, from_con, to_con, change_mask);
+	}
+
+	return 0;
+}
+
+#else
+#define samsung_pinctrl_suspend_noirq	NULL
+#define samsung_pinctrl_resume_noirq	NULL
+#endif
+
+static const struct dev_pm_ops samsung_pinctrl_dev_pm_ops = {
+	.suspend_noirq = samsung_pinctrl_suspend_noirq,
+	.resume_noirq = samsung_pinctrl_resume_noirq,
+};
+
 static const struct of_device_id samsung_pinctrl_dt_match[] = {
 #ifdef CONFIG_PINCTRL_EXYNOS
 	{ .compatible = "samsung,exynos4210-pinctrl",
@@ -987,6 +1185,7 @@ static struct platform_driver samsung_pinctrl_driver = {
 		.name	= "samsung-pinctrl",
 		.owner	= THIS_MODULE,
 		.of_match_table = of_match_ptr(samsung_pinctrl_dt_match),
+		.pm = &samsung_pinctrl_dev_pm_ops,
 	},
 };
 
diff --git a/drivers/pinctrl/pinctrl-samsung.h b/drivers/pinctrl/pinctrl-samsung.h
index 7c7f9eb..c9a7b6e 100644
--- a/drivers/pinctrl/pinctrl-samsung.h
+++ b/drivers/pinctrl/pinctrl-samsung.h
@@ -127,6 +127,7 @@ struct samsung_pin_bank_type {
  * @gpio_chip: GPIO chip of the bank.
  * @grange: linux gpio pin range supported by this bank.
  * @slock: spinlock protecting bank registers
+ * @pm_save: saved register values during suspend
  */
 struct samsung_pin_bank {
 	struct samsung_pin_bank_type *type;
@@ -144,6 +145,16 @@ struct samsung_pin_bank {
 	struct gpio_chip gpio_chip;
 	struct pinctrl_gpio_range grange;
 	spinlock_t slock;
+#ifdef CONFIG_PM
+	struct {
+		u64	con;
+		u32	dat;
+		u32	pud;
+		u32	drv;
+		u32	conpdn;
+		u32	pudpdn;
+	} pm_save;
+#endif
 };
 
 /**
-- 
1.8.2.1

[PATCH 2/2] pinctrl: exynos: fix eint wakeup by using irq_set_wake()

[Doug Anderson]

From: Prathyush K <prathyush.k@samsung.com>

Add the irq_set_wake function for exynos pinctrl to configure the
external interrupt wakeup mask register.

[dianders: minor nit fixes; port to ToT]

Signed-off-by: Prathyush K <prathyush.k@samsung.com>
Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 drivers/pinctrl/pinctrl-exynos.c  | 45 ++++++++++++++++++++++++++++-----------
 drivers/pinctrl/pinctrl-exynos.h  |  3 ++-
 drivers/pinctrl/pinctrl-samsung.h |  2 ++
 3 files changed, 37 insertions(+), 13 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-exynos.c b/drivers/pinctrl/pinctrl-exynos.c
index ac74281..3ebb2ff 100644
--- a/drivers/pinctrl/pinctrl-exynos.c
+++ b/drivers/pinctrl/pinctrl-exynos.c
@@ -30,6 +30,8 @@
 #include <linux/spinlock.h>
 #include <linux/err.h>
 
+#include <plat/pm.h>
+
 #include "pinctrl-samsung.h"
 #include "pinctrl-exynos.h"
 
@@ -326,6 +328,24 @@ static int exynos_wkup_irq_set_type(struct irq_data *irqd, unsigned int type)
 	return 0;
 }
 
+static int exynos_wkup_irq_set_wake(struct irq_data *irqd, unsigned int state)
+{
+	struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd);
+	const int eint_num = bank->eint_base + irqd->hwirq;
+	unsigned long bit = 1L << eint_num;
+
+	pr_info("wake %s for eint %d / %s[%ld]\n",
+		state ? "enabled" : "disabled",
+		eint_num, bank->name, irqd->hwirq);
+
+	if (!state)
+		s3c_irqwake_eintmask |= bit;
+	else
+		s3c_irqwake_eintmask &= ~bit;
+
+	return 0;
+}
+
 /*
  * irq_chip for wakeup interrupts
  */
@@ -335,6 +355,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 */
@@ -543,10 +564,10 @@ static struct samsung_pin_bank exynos4210_pin_banks1[] = {
 	EXYNOS_PIN_BANK_EINTN(8, 0x1A0, "gpy4"),
 	EXYNOS_PIN_BANK_EINTN(8, 0x1C0, "gpy5"),
 	EXYNOS_PIN_BANK_EINTN(8, 0x1E0, "gpy6"),
-	EXYNOS_PIN_BANK_EINTW(8, 0xC00, "gpx0", 0x00),
-	EXYNOS_PIN_BANK_EINTW(8, 0xC20, "gpx1", 0x04),
-	EXYNOS_PIN_BANK_EINTW(8, 0xC40, "gpx2", 0x08),
-	EXYNOS_PIN_BANK_EINTW(8, 0xC60, "gpx3", 0x0c),
+	EXYNOS_PIN_BANK_EINTW(8, 0xC00, "gpx0", 0x00, 0),
+	EXYNOS_PIN_BANK_EINTW(8, 0xC20, "gpx1", 0x04, 8),
+	EXYNOS_PIN_BANK_EINTW(8, 0xC40, "gpx2", 0x08, 16),
+	EXYNOS_PIN_BANK_EINTW(8, 0xC60, "gpx3", 0x0c, 24),
 };
 
 /* pin banks of exynos4210 pin-controller 2 */
@@ -629,10 +650,10 @@ static struct samsung_pin_bank exynos4x12_pin_banks1[] = {
 	EXYNOS_PIN_BANK_EINTN(8, 0x1A0, "gpy4"),
 	EXYNOS_PIN_BANK_EINTN(8, 0x1C0, "gpy5"),
 	EXYNOS_PIN_BANK_EINTN(8, 0x1E0, "gpy6"),
-	EXYNOS_PIN_BANK_EINTW(8, 0xC00, "gpx0", 0x00),
-	EXYNOS_PIN_BANK_EINTW(8, 0xC20, "gpx1", 0x04),
-	EXYNOS_PIN_BANK_EINTW(8, 0xC40, "gpx2", 0x08),
-	EXYNOS_PIN_BANK_EINTW(8, 0xC60, "gpx3", 0x0c),
+	EXYNOS_PIN_BANK_EINTW(8, 0xC00, "gpx0", 0x00, 0),
+	EXYNOS_PIN_BANK_EINTW(8, 0xC20, "gpx1", 0x04, 8),
+	EXYNOS_PIN_BANK_EINTW(8, 0xC40, "gpx2", 0x08, 16),
+	EXYNOS_PIN_BANK_EINTW(8, 0xC60, "gpx3", 0x0c, 24),
 };
 
 /* pin banks of exynos4x12 pin-controller 2 */
@@ -724,10 +745,10 @@ static struct samsung_pin_bank exynos5250_pin_banks0[] = {
 	EXYNOS_PIN_BANK_EINTN(8, 0x220, "gpy4"),
 	EXYNOS_PIN_BANK_EINTN(8, 0x240, "gpy5"),
 	EXYNOS_PIN_BANK_EINTN(8, 0x260, "gpy6"),
-	EXYNOS_PIN_BANK_EINTW(8, 0xC00, "gpx0", 0x00),
-	EXYNOS_PIN_BANK_EINTW(8, 0xC20, "gpx1", 0x04),
-	EXYNOS_PIN_BANK_EINTW(8, 0xC40, "gpx2", 0x08),
-	EXYNOS_PIN_BANK_EINTW(8, 0xC60, "gpx3", 0x0c),
+	EXYNOS_PIN_BANK_EINTW(8, 0xC00, "gpx0", 0x00, 0),
+	EXYNOS_PIN_BANK_EINTW(8, 0xC20, "gpx1", 0x04, 8),
+	EXYNOS_PIN_BANK_EINTW(8, 0xC40, "gpx2", 0x08, 16),
+	EXYNOS_PIN_BANK_EINTW(8, 0xC60, "gpx3", 0x0c, 24),
 };
 
 /* pin banks of exynos5250 pin-controller 1 */
diff --git a/drivers/pinctrl/pinctrl-exynos.h b/drivers/pinctrl/pinctrl-exynos.h
index 9b1f77a..d98e9ff 100644
--- a/drivers/pinctrl/pinctrl-exynos.h
+++ b/drivers/pinctrl/pinctrl-exynos.h
@@ -65,13 +65,14 @@
 		.name		= id			\
 	}
 
-#define EXYNOS_PIN_BANK_EINTW(pins, reg, id, offs)	\
+#define EXYNOS_PIN_BANK_EINTW(pins, reg, id, offs, base)\
 	{						\
 		.type		= &bank_type_alive,	\
 		.pctl_offset	= reg,			\
 		.nr_pins	= pins,			\
 		.eint_type	= EINT_TYPE_WKUP,	\
 		.eint_offset	= offs,			\
+		.eint_base	= base,			\
 		.name		= id			\
 	}
 
diff --git a/drivers/pinctrl/pinctrl-samsung.h b/drivers/pinctrl/pinctrl-samsung.h
index c9a7b6e..1e033ae 100644
--- a/drivers/pinctrl/pinctrl-samsung.h
+++ b/drivers/pinctrl/pinctrl-samsung.h
@@ -120,6 +120,7 @@ struct samsung_pin_bank_type {
  * @eint_func: function to set in CON register to configure pin as EINT.
  * @eint_type: type of the external interrupt supported by the bank.
  * @eint_mask: bit mask of pins which support EINT function.
+ * @eint_base: number of external wakeup interrupts from start to this bank.
  * @name: name to be prefixed for each pin in this pin bank.
  * @of_node: OF node of the bank.
  * @drvdata: link to controller driver data
@@ -138,6 +139,7 @@ struct samsung_pin_bank {
 	enum eint_type	eint_type;
 	u32		eint_mask;
 	u32		eint_offset;
+	u32		eint_base;
 	char		*name;
 	struct device_node *of_node;
 	struct samsung_pinctrl_drv_data *drvdata;
-- 
1.8.2.1

Re: [PATCH 1/2] pinctrl: samsung: fix suspend/resume functionality

[Tomasz Figa]

Hi Doug,

Thanks for the patch. See my comments inline.

On Thursday 16 of May 2013 10:12:31 Doug Anderson wrote:
> The GPIO states need to be restored after s2r and this is not currently
> supported in the pinctrl driver. This patch saves the gpio states before
> suspend and restores them after resume.
> 
> Logic and commenting for samsung_pinctrl_resume_noirq() is heavily
> borrowed from the old samsung_gpio_pm_2bit_resume(), which seemed to
> do this a reasonable way.
> 
> Patch originally from Prathyush K <prathyush.k@samsung.com> but
> rewritten by Doug Anderson <dianders@chromium.org>.
> 
> Signed-off-by: Prathyush K <prathyush.k@samsung.com>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
>  drivers/pinctrl/pinctrl-samsung.c | 199
> ++++++++++++++++++++++++++++++++++++++
> drivers/pinctrl/pinctrl-samsung.h |  11 +++
>  2 files changed, 210 insertions(+)
> 
> diff --git a/drivers/pinctrl/pinctrl-samsung.c
> b/drivers/pinctrl/pinctrl-samsung.c index 9763668..0891667 100644
> --- a/drivers/pinctrl/pinctrl-samsung.c
> +++ b/drivers/pinctrl/pinctrl-samsung.c
> @@ -964,6 +964,204 @@ static int samsung_pinctrl_probe(struct
> platform_device *pdev) return 0;
>  }
> 
> +#ifdef CONFIG_PM
> +
> +/**
> + * samsung_pinctrl_resume_noirq - save pinctrl state for suspend
> + *
> + * Save data for all banks handled by this device.
> + */
> +static int samsung_pinctrl_suspend_noirq(struct device *dev)
> +{
> +	struct samsung_pinctrl_drv_data *drvdata = dev_get_drvdata(dev);
> +	struct samsung_pin_ctrl *ctrl = drvdata->ctrl;
> +	void __iomem * const virt_base = drvdata->virt_base;

Nit: This const is ugly :) . Is it needed for anything?

> +	int i;
> +
> +	for (i = 0; i < ctrl->nr_banks; i++) {
> +		struct samsung_pin_bank *bank = &ctrl->pin_banks[i];
> +		const struct samsung_pin_bank_type *type = bank->type;
> +		void __iomem * const reg = virt_base + bank->pctl_offset;

Nit: This one is not pretty either.

> +		bank->pm_save.con = readl(reg +
> +					  type-
>reg_offset[PINCFG_TYPE_FUNC]);
> +		if (type->fld_width[PINCFG_TYPE_FUNC] > 4)

What is this condition supposed to check?

> +			bank->pm_save.con |= (u64)readl(reg + 4 +
> +				      type->reg_offset[PINCFG_TYPE_FUNC]) 
<< 32;

This looks ugly. Whatever is going on here, wouldn't it be better to use 
separate field, like con2 or something?

> +		bank->pm_save.dat = readl(reg +
> +					  type-
>reg_offset[PINCFG_TYPE_DAT]);
> +		bank->pm_save.pud = readl(reg +
> +					  type-
>reg_offset[PINCFG_TYPE_PUD]);
> +		bank->pm_save.drv = readl(reg +
> +					  type-
>reg_offset[PINCFG_TYPE_DRV]);
> +
> +		if (type->fld_width[PINCFG_TYPE_CON_PDN]) {
> +			bank->pm_save.conpdn = readl(reg +
> +				type->reg_offset[PINCFG_TYPE_CON_PDN]);
> +			bank->pm_save.pudpdn = readl(reg +
> +				type->reg_offset[PINCFG_TYPE_PUD_PDN]);
> +		}

I wonder if you couldn't do all the saving here in a single loop over all 
pin control types, like:

	unsigned int offsets = bank->type->reg_offsets;
	unsigned int widths = bank->type->fld_width;

	for (i = 0; i < PINCFG_TYPE_NUM; ++i)
		if (widths[i])
			bank->pm_save[i] = readl(reg + offsets[i]);

The only thing not handled by this loop is second CON registers in banks 
with two of them. I can't think of any better solution for this other than 
just adding a special case after the loop.

> +		dev_dbg(dev, "Save %s @ %p (con %#010llx)\n",
> +			bank->name, reg, bank->pm_save.con);
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * is_sfn - test whether a pin config represents special function.
> + *
> + * Test whether the given masked+shifted bits of an GPIO configuration
> + * are one of the SFN (special function) modes.
> + */
> +static inline int is_sfn(u32 con)
> +{
> +	return con >= 2;
> +}
> +
> +/**
> + * is_in - test if the given masked+shifted GPIO configuration is an
> input. + */
> +static inline int is_in(u32 con)
> +{
> +	return con == 0;
> +}
> +
> +/**
> + * is_out - test if the given masked+shifted GPIO configuration is an
> output. + */
> +static inline int is_out(u32 con)
> +{
> +	return con == 1;
> +}
> +
> +/**
> + * samsung_pinctrl_resume_noirq - restore pinctrl state from suspend
> + *
> + * Restore one of the GPIO banks that was saved during suspend. This is
> + * not as simple as once thought, due to the possibility of glitches +
> * from the order that the CON and DAT registers are set in.
> + *
> + * The three states the pin can be are {IN,OUT,SFN} which gives us 9
> + * combinations of changes to check. Three of these, if the pin stays
> + * in the same configuration can be discounted. This leaves us with
> + * the following:
> + *
> + * { IN => OUT }  Change DAT first
> + * { IN => SFN }  Change CON first
> + * { OUT => SFN } Change CON first, so new data will not glitch
> + * { OUT => IN }  Change CON first, so new data will not glitch
> + * { SFN => IN }  Change CON first
> + * { SFN => OUT } Change DAT first, so new data will not glitch [1]
> + *
> + * We do not currently deal with the UP registers as these control
> + * weak resistors, so a small delay in change should not need to bring
> + * these into the calculations.
> + *
> + * [1] this assumes that writing to a pin DAT whilst in SFN will set
> the + *     state for when it is next output.
> + */
> +static int samsung_pinctrl_resume_noirq(struct device *dev)
> +{
> +	struct samsung_pinctrl_drv_data *drvdata = dev_get_drvdata(dev);
> +	struct samsung_pin_ctrl *ctrl = drvdata->ctrl;
> +	void __iomem * const virt_base = drvdata->virt_base;
> +	int i;
> +
> +	for (i = 0; i < ctrl->nr_banks; i++) {
> +		const struct samsung_pin_bank *bank = &ctrl->pin_banks[i];
> +		void __iomem * const reg = virt_base + bank->pctl_offset;
> +		const struct samsung_pin_bank_type *type = bank->type;
> +		const u8 func_offset = type->reg_offset[PINCFG_TYPE_FUNC];
> +		const u32 func_width = type->fld_width[PINCFG_TYPE_FUNC];
> +		const u32 func_mask = (1 << func_width) - 1;

I'm constantly seeing so many consts in this patch. Is this constraint 
relevant here in any way? Git grep doesn't show too many instances of this 
construct used for local variables around the kernel.

> +		const bool is64bit = type->fld_width[PINCFG_TYPE_FUNC] > 
4;

Again this check. Is it supposed to handle banks with two CON registers? 
If yes, it is incorrect, as they have 4 bits per pin.

> +		const u64 to_con = bank->pm_save.con;
> +		u64 from_con = readl(reg + func_offset);
> +		u64 change_mask = 0;
> +		u64 to_write;
> +		int pin;
> +
> +		if (is64bit)
> +			from_con |= (u64)readl(reg + 4 + func_offset) << 
32;

Same comment as in save.

> +
> +		/* Easy--the PUD and DRV can go first */
> +		writel(bank->pm_save.pud,
> +		       reg + type->reg_offset[PINCFG_TYPE_PUD]);
> +		writel(bank->pm_save.drv,
> +		       reg + type->reg_offset[PINCFG_TYPE_DRV]);
> +
> +		/*
> +		 * Create a change_mask of all the items that need to have
> +		 * their CON value changed before their DAT value, so that
> +		 * we minimise the work between the two settings.
> +		 */
> +		for (pin = 0; pin < bank->nr_pins; pin++) {

This is a bit more tricky that in case of save, so CON register as a 
special case is justified here.

> +			u32 shift = pin * func_width;
> +			u32 from_func = (from_con >> shift) & func_mask;
> +			u32 to_func = (to_con >> shift) & func_mask;
> +
> +			/* If there is no change, then skip */
> +			if (from_func == to_func)
> +				continue;
> +
> +			/* If both are special function, then skip */
> +			if (is_sfn(from_func) && is_sfn(to_func))
> +				continue;
> +
> +			/* Change is IN => OUT, do not change now */
> +			if (is_in(from_func) && is_out(to_func))
> +				continue;
> +
> +			/* Change is SFN => OUT, do not change now */
> +			if (is_sfn(from_func) && is_out(to_func))
> +				continue;
> +
> +			/*
> +			 * We should now be at the case of:
> +			 *   IN=>SFN, OUT=>SFN, OUT=>IN, SFN=>IN.
> +			 */
> +			change_mask |= (func_mask << shift);
> +		}
> +
> +		/* Write the new CON settings */
> +		to_write = (from_con & ~change_mask) | (to_con & 
change_mask);
> +		writel((u32)to_write, reg + func_offset);
> +		if (is64bit)
> +			writel((u32)(to_write >> 32), reg + 4 + 
func_offset);
> +
> +		/* Now change any items that require DAT,CON */
> +		writel(bank->pm_save.dat,
> +		       reg + type->reg_offset[PINCFG_TYPE_DAT]);
> +		writel((u32)to_con, reg + func_offset);
> +		if (is64bit)
> +			writel((u32)(to_con >> 32), reg + 4 + to_con);
> +
> +		if (type->fld_width[PINCFG_TYPE_CON_PDN]) {
> +			writel(bank->pm_save.conpdn,
> +			       reg + type-
>reg_offset[PINCFG_TYPE_CON_PDN]);
> +			writel(bank->pm_save.pudpdn,
> +			       reg + type-
>reg_offset[PINCFG_TYPE_PUD_PDN]);
> +		}

Now as I think of it, do CON_PDN and PUD_PDN really need to be saved? I 
couldn't find in the documentation if they are preserved or lost in sleep 
mode. Do you have some information on this?

My experiments on Exynos4210 and 4x12 showed that they are preserved, but 
I don't know what about other SoCs.

> +		dev_dbg(dev, "Restore %s@%p (%#010llx=>%#010llx ch 
%#010llx)\n",
> +			bank->name, reg, from_con, to_con, change_mask);
> +	}
> +
> +	return 0;
> +}

I wonder if the whole restoration procedure couldn't be simplified. I 
don't remember my version being so complicated, but I don't have my patch 
on my screen at the moment, so I might be wrong on this.

> +#else
> +#define samsung_pinctrl_suspend_noirq	NULL
> +#define samsung_pinctrl_resume_noirq	NULL
> +#endif
> +
> +static const struct dev_pm_ops samsung_pinctrl_dev_pm_ops = {
> +	.suspend_noirq = samsung_pinctrl_suspend_noirq,
> +	.resume_noirq = samsung_pinctrl_resume_noirq,
> +};

I'm not sure if resume_noirq is really early enough. Some drivers might 
already need correct pin configuration in their resume_noirq callback.

In my patch I have used syscore_ops to register very late suspend and very 
early resume callbacks for the whole pinctrl-samsung driver and a global 
list of registered pin controllers, that is iterated over in suspend and 
resume.

>  static const struct of_device_id samsung_pinctrl_dt_match[] = {
>  #ifdef CONFIG_PINCTRL_EXYNOS
>  	{ .compatible = "samsung,exynos4210-pinctrl",
> @@ -987,6 +1185,7 @@ static struct platform_driver
> samsung_pinctrl_driver = { .name	= "samsung-pinctrl",
>  		.owner	= THIS_MODULE,
>  		.of_match_table = of_match_ptr(samsung_pinctrl_dt_match),
> +		.pm = &samsung_pinctrl_dev_pm_ops,
>  	},
>  };
> 
> diff --git a/drivers/pinctrl/pinctrl-samsung.h
> b/drivers/pinctrl/pinctrl-samsung.h index 7c7f9eb..c9a7b6e 100644
> --- a/drivers/pinctrl/pinctrl-samsung.h
> +++ b/drivers/pinctrl/pinctrl-samsung.h
> @@ -127,6 +127,7 @@ struct samsung_pin_bank_type {
>   * @gpio_chip: GPIO chip of the bank.
>   * @grange: linux gpio pin range supported by this bank.
>   * @slock: spinlock protecting bank registers
> + * @pm_save: saved register values during suspend
>   */
>  struct samsung_pin_bank {
>  	struct samsung_pin_bank_type *type;
> @@ -144,6 +145,16 @@ struct samsung_pin_bank {
>  	struct gpio_chip gpio_chip;
>  	struct pinctrl_gpio_range grange;
>  	spinlock_t slock;
> +#ifdef CONFIG_PM
> +	struct {
> +		u64	con;
> +		u32	dat;
> +		u32	pud;
> +		u32	drv;
> +		u32	conpdn;
> +		u32	pudpdn;

This could be changed into an array.

Best regards,
Tomasz

Re: [PATCH 2/2] pinctrl: exynos: fix eint wakeup by using irq_set_wake()

[Tomasz Figa]

On Thursday 16 of May 2013 10:12:32 Doug Anderson wrote:
> From: Prathyush K <prathyush.k@samsung.com>
> 
> Add the irq_set_wake function for exynos pinctrl to configure the
> external interrupt wakeup mask register.
> 
> [dianders: minor nit fixes; port to ToT]
> 
> Signed-off-by: Prathyush K <prathyush.k@samsung.com>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
>  drivers/pinctrl/pinctrl-exynos.c  | 45
> ++++++++++++++++++++++++++++-----------
> drivers/pinctrl/pinctrl-exynos.h  |  3 ++-
>  drivers/pinctrl/pinctrl-samsung.h |  2 ++
>  3 files changed, 37 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-exynos.c
> b/drivers/pinctrl/pinctrl-exynos.c index ac74281..3ebb2ff 100644
> --- a/drivers/pinctrl/pinctrl-exynos.c
> +++ b/drivers/pinctrl/pinctrl-exynos.c
> @@ -30,6 +30,8 @@
>  #include <linux/spinlock.h>
>  #include <linux/err.h>
> 
> +#include <plat/pm.h>
> +

This is not going to work with CONFIG_MULTIPLATFORM.

Now this raises a question what is the preferred way to pass some data 
from generic driver to platform code.

I would suggest adding a function called exynos_pinctrl_get_eintmask() (or 
whatever) that would return the wake-up mask configured in the driver and 
then modify platform code to use it.

>  #include "pinctrl-samsung.h"
>  #include "pinctrl-exynos.h"
> 
> @@ -326,6 +328,24 @@ static int exynos_wkup_irq_set_type(struct irq_data
> *irqd, unsigned int type) return 0;
>  }
> 
> +static int exynos_wkup_irq_set_wake(struct irq_data *irqd, unsigned int
> state) +{
> +	struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd);
> +	const int eint_num = bank->eint_base + irqd->hwirq;
> +	unsigned long bit = 1L << eint_num;
> +
> +	pr_info("wake %s for eint %d / %s[%ld]\n",
> +		state ? "enabled" : "disabled",
> +		eint_num, bank->name, irqd->hwirq);
> +
> +	if (!state)
> +		s3c_irqwake_eintmask |= bit;
> +	else
> +		s3c_irqwake_eintmask &= ~bit;
> +
> +	return 0;
> +}
> +
>  /*
>   * irq_chip for wakeup interrupts
>   */
> @@ -335,6 +355,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 */
> @@ -543,10 +564,10 @@ static struct samsung_pin_bank
> exynos4210_pin_banks1[] = { EXYNOS_PIN_BANK_EINTN(8, 0x1A0, "gpy4"),
>  	EXYNOS_PIN_BANK_EINTN(8, 0x1C0, "gpy5"),
>  	EXYNOS_PIN_BANK_EINTN(8, 0x1E0, "gpy6"),
> -	EXYNOS_PIN_BANK_EINTW(8, 0xC00, "gpx0", 0x00),
> -	EXYNOS_PIN_BANK_EINTW(8, 0xC20, "gpx1", 0x04),
> -	EXYNOS_PIN_BANK_EINTW(8, 0xC40, "gpx2", 0x08),
> -	EXYNOS_PIN_BANK_EINTW(8, 0xC60, "gpx3", 0x0c),
> +	EXYNOS_PIN_BANK_EINTW(8, 0xC00, "gpx0", 0x00, 0),
> +	EXYNOS_PIN_BANK_EINTW(8, 0xC20, "gpx1", 0x04, 8),
> +	EXYNOS_PIN_BANK_EINTW(8, 0xC40, "gpx2", 0x08, 16),
> +	EXYNOS_PIN_BANK_EINTW(8, 0xC60, "gpx3", 0x0c, 24),
>  };
> 
>  /* pin banks of exynos4210 pin-controller 2 */
> @@ -629,10 +650,10 @@ static struct samsung_pin_bank
> exynos4x12_pin_banks1[] = { EXYNOS_PIN_BANK_EINTN(8, 0x1A0, "gpy4"),
>  	EXYNOS_PIN_BANK_EINTN(8, 0x1C0, "gpy5"),
>  	EXYNOS_PIN_BANK_EINTN(8, 0x1E0, "gpy6"),
> -	EXYNOS_PIN_BANK_EINTW(8, 0xC00, "gpx0", 0x00),
> -	EXYNOS_PIN_BANK_EINTW(8, 0xC20, "gpx1", 0x04),
> -	EXYNOS_PIN_BANK_EINTW(8, 0xC40, "gpx2", 0x08),
> -	EXYNOS_PIN_BANK_EINTW(8, 0xC60, "gpx3", 0x0c),
> +	EXYNOS_PIN_BANK_EINTW(8, 0xC00, "gpx0", 0x00, 0),
> +	EXYNOS_PIN_BANK_EINTW(8, 0xC20, "gpx1", 0x04, 8),
> +	EXYNOS_PIN_BANK_EINTW(8, 0xC40, "gpx2", 0x08, 16),
> +	EXYNOS_PIN_BANK_EINTW(8, 0xC60, "gpx3", 0x0c, 24),
>  };
> 
>  /* pin banks of exynos4x12 pin-controller 2 */
> @@ -724,10 +745,10 @@ static struct samsung_pin_bank
> exynos5250_pin_banks0[] = { EXYNOS_PIN_BANK_EINTN(8, 0x220, "gpy4"),
>  	EXYNOS_PIN_BANK_EINTN(8, 0x240, "gpy5"),
>  	EXYNOS_PIN_BANK_EINTN(8, 0x260, "gpy6"),
> -	EXYNOS_PIN_BANK_EINTW(8, 0xC00, "gpx0", 0x00),
> -	EXYNOS_PIN_BANK_EINTW(8, 0xC20, "gpx1", 0x04),
> -	EXYNOS_PIN_BANK_EINTW(8, 0xC40, "gpx2", 0x08),
> -	EXYNOS_PIN_BANK_EINTW(8, 0xC60, "gpx3", 0x0c),
> +	EXYNOS_PIN_BANK_EINTW(8, 0xC00, "gpx0", 0x00, 0),
> +	EXYNOS_PIN_BANK_EINTW(8, 0xC20, "gpx1", 0x04, 8),
> +	EXYNOS_PIN_BANK_EINTW(8, 0xC40, "gpx2", 0x08, 16),
> +	EXYNOS_PIN_BANK_EINTW(8, 0xC60, "gpx3", 0x0c, 24),
>  };
> 
>  /* pin banks of exynos5250 pin-controller 1 */
> diff --git a/drivers/pinctrl/pinctrl-exynos.h
> b/drivers/pinctrl/pinctrl-exynos.h index 9b1f77a..d98e9ff 100644
> --- a/drivers/pinctrl/pinctrl-exynos.h
> +++ b/drivers/pinctrl/pinctrl-exynos.h
> @@ -65,13 +65,14 @@
>  		.name		= id			\
>  	}
> 
> -#define EXYNOS_PIN_BANK_EINTW(pins, reg, id, offs)	\
> +#define EXYNOS_PIN_BANK_EINTW(pins, reg, id, offs, base)\
>  	{						\
>  		.type		= &bank_type_alive,	\
>  		.pctl_offset	= reg,			\
>  		.nr_pins	= pins,			\
>  		.eint_type	= EINT_TYPE_WKUP,	\
>  		.eint_offset	= offs,			\
> +		.eint_base	= base,			\

I can't look at my patch at the moment, but I think I have managed to get 
EINT index without adding this extra field.

Best regards,
Tomasz

Re: [PATCH 1/2] pinctrl: samsung: fix suspend/resume functionality

[Doug Anderson]

Tomasz,

Thanks for the review!  I'll get a new patch out either today or tomorrow...

On Thu, May 16, 2013 at 12:19 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> +/**
>> + * samsung_pinctrl_resume_noirq - save pinctrl state for suspend
>> + *
>> + * Save data for all banks handled by this device.
>> + */
>> +static int samsung_pinctrl_suspend_noirq(struct device *dev)
>> +{
>> +     struct samsung_pinctrl_drv_data *drvdata = dev_get_drvdata(dev);
>> +     struct samsung_pin_ctrl *ctrl = drvdata->ctrl;
>> +     void __iomem * const virt_base = drvdata->virt_base;
>
> Nit: This const is ugly :) . Is it needed for anything?

Not anything really.  I just got in the habit of adding them for
variables that are simple shorthand variables: AKA I'm only creating
this variable to avoid typing some long complicated thing below.  It's
a hint to someone reading the code that they don't need to think about
it.  I have also occasionally caught bugs by doing this.

...but I can understand the dislike.  I'll remove this and other
similar (but keep const pointers).


>> +             if (type->fld_width[PINCFG_TYPE_FUNC] > 4)
>
> What is this condition supposed to check?

It is supposed to be checking whether there are two CON registers in
use.  ...oh, but that's probably not the right way to do it now that I
think about it.  I need to check (bank->nr_pins *
type->fld_width[PINCFG_TYPE_FUNC]).

I will fix.


>> +                     bank->pm_save.con |= (u64)readl(reg + 4 +
>> +                                   type->reg_offset[PINCFG_TYPE_FUNC])
> << 32;
>
> This looks ugly. Whatever is going on here, wouldn't it be better to use
> separate field, like con2 or something?

Probably.  The resume code seemed cleaner with a 64-bit value, but I
think I could make it nearly as clean with two 32-bit ones by using a
helper function.


> I wonder if you couldn't do all the saving here in a single loop over all
> pin control types, like:
>
>         unsigned int offsets = bank->type->reg_offsets;
>         unsigned int widths = bank->type->fld_width;
>
>         for (i = 0; i < PINCFG_TYPE_NUM; ++i)
>                 if (widths[i])
>                         bank->pm_save[i] = readl(reg + offsets[i]);
>
> The only thing not handled by this loop is second CON registers in banks
> with two of them. I can't think of any better solution for this other than
> just adding a special case after the loop.

Yes, that would work.  I think it wasn't possible when I first wrote
the code against an older code base that didn't have the arrays.  I
can give it a shot if it doesn't make restore too bad...


> Now as I think of it, do CON_PDN and PUD_PDN really need to be saved? I
> couldn't find in the documentation if they are preserved or lost in sleep
> mode. Do you have some information on this?

I remember it being important.  Running a test now.  Yes, it's
important on exynos5250.  As an example:

[   62.220000] samsung-pinctrl 11400000.pinctrl: Restore gpa2@f0048040
(0x22220000=>0x22123333 ch 0x0000ffff)
[   62.220000] samsung-pinctrl 11400000.pinctrl: Restore gpb0@f0048060
CON_PDN (0x00000000=>0x000002aa)
[   62.220000] samsung-pinctrl 11400000.pinctrl: Restore gpb0@f0048060
PUD_PDN (0x00000000=>0x00000155)


> I wonder if the whole restoration procedure couldn't be simplified. I
> don't remember my version being so complicated, but I don't have my patch
> on my screen at the moment, so I might be wrong on this.

I debated about this a bunch.  Perhaps I should just delete it.  I saw
that it was there in the old "2-bit" code and it also seemed quite
reasonable, so I kept it.  Things seem to work OK without it, but most
things are pretty tolerant to their lines glitching (or even driving
high to low for a short period of time).

...but your question made me check again.

>From previous experimentation I'm pretty certain that most pins on the
exynos are held in the powerdown state even during early bootup of the
SoC.  The hope is that they are released from powerdown _after_ the
GPIO init is called.  If not then we're already glitching somewhat as
we transition from powerdown state to default state before this
function finally gets called.

Looking at exynos, that's probably done in exynos_pm_resume(), maybe
by mucking with the pad retention options?

Oh, that probably means taht no_irq() is too late and that I need to
figure out how to get my code called earlier...  The
exynos_pm_resume() is called by syscore.


>> +#else
>> +#define samsung_pinctrl_suspend_noirq        NULL
>> +#define samsung_pinctrl_resume_noirq NULL
>> +#endif
>> +
>> +static const struct dev_pm_ops samsung_pinctrl_dev_pm_ops = {
>> +     .suspend_noirq = samsung_pinctrl_suspend_noirq,
>> +     .resume_noirq = samsung_pinctrl_resume_noirq,
>> +};
>
> I'm not sure if resume_noirq is really early enough. Some drivers might
> already need correct pin configuration in their resume_noirq callback.
>
> In my patch I have used syscore_ops to register very late suspend and very
> early resume callbacks for the whole pinctrl-samsung driver and a global
> list of registered pin controllers, that is iterated over in suspend and
> resume.

OK, as per above syscore is actually important.  ...and in fact I need
to figure out how to ensure that this is called _before_
exynos_pm_resume(), which is also syscore.  I guess I also need to
stash an array of devices in an global somewhere, since syscore passes
no params...

-Doug

Re: [PATCH 1/2] pinctrl: samsung: fix suspend/resume functionality

[Heiko Stübner]

Am Donnerstag, 16. Mai 2013, 21:19:20 schrieb Tomasz Figa:
[...]
> > +
> > +		if (type->fld_width[PINCFG_TYPE_CON_PDN]) {
> > +			bank->pm_save.conpdn = readl(reg +
> > +				type->reg_offset[PINCFG_TYPE_CON_PDN]);
> > +			bank->pm_save.pudpdn = readl(reg +
> > +				type->reg_offset[PINCFG_TYPE_PUD_PDN]);
> > +		}
> 
> I wonder if you couldn't do all the saving here in a single loop over all
> pin control types, like:
> 
> 	unsigned int offsets = bank->type->reg_offsets;
> 	unsigned int widths = bank->type->fld_width;
> 
> 	for (i = 0; i < PINCFG_TYPE_NUM; ++i)
> 		if (widths[i])
> 			bank->pm_save[i] = readl(reg + offsets[i]);
> 
> The only thing not handled by this loop is second CON registers in banks
> with two of them. I can't think of any better solution for this other than
> just adding a special case after the loop.

doing this in the loop over the pinctrl types like Tomasz suggests, also 
nicely fixes the problem of s3c24xx [0] only having FUNC, DAT and PUD and some 
(gpa) not even having the PUD, which was not checked in the original patch.


Heiko

[0] patch is with Kgene currently, so should make it into 3.11

Re: [PATCH 1/2] pinctrl: samsung: fix suspend/resume functionality

[Tomasz Figa]

On Thursday 16 of May 2013 13:32:48 Doug Anderson wrote:
> Tomasz,
> 
> Thanks for the review!  I'll get a new patch out either today or
> tomorrow...

OK. I will be fine to go with your patches, after addressing the comments. 
In the end it's good that you posted them, as reviewing them allowed me to 
find even better ways of doing some things than I had in mine ;) .

[snip]
> 
> > I wonder if you couldn't do all the saving here in a single loop over
> > all> 
> > pin control types, like:
> >         unsigned int offsets = bank->type->reg_offsets;
> >         unsigned int widths = bank->type->fld_width;
> >         
> >         for (i = 0; i < PINCFG_TYPE_NUM; ++i)
> >         
> >                 if (widths[i])
> >                 
> >                         bank->pm_save[i] = readl(reg + offsets[i]);
> > 
> > The only thing not handled by this loop is second CON registers in
> > banks with two of them. I can't think of any better solution for this
> > other than just adding a special case after the loop.
> 
> Yes, that would work.  I think it wasn't possible when I first wrote
> the code against an older code base that didn't have the arrays.  I
> can give it a shot if it doesn't make restore too bad...

OK. I wonder if resume couldn't somehow benefit from this as well, but it 
probably depends on how much it can be altered without breaking the anti-
glitch functionality.

> > Now as I think of it, do CON_PDN and PUD_PDN really need to be saved?
> > I
> > couldn't find in the documentation if they are preserved or lost in
> > sleep mode. Do you have some information on this?
> 
> I remember it being important.  Running a test now.  Yes, it's
> important on exynos5250.  As an example:
> 
> [   62.220000] samsung-pinctrl 11400000.pinctrl: Restore gpa2@f0048040
> (0x22220000=>0x22123333 ch 0x0000ffff)
> [   62.220000] samsung-pinctrl 11400000.pinctrl: Restore gpb0@f0048060
> CON_PDN (0x00000000=>0x000002aa)
> [   62.220000] samsung-pinctrl 11400000.pinctrl: Restore gpb0@f0048060
> PUD_PDN (0x00000000=>0x00000155)

OK. It's good to know.

> > I wonder if the whole restoration procedure couldn't be simplified. I
> > don't remember my version being so complicated, but I don't have my
> > patch on my screen at the moment, so I might be wrong on this.
> 
> I debated about this a bunch.  Perhaps I should just delete it.  I saw
> that it was there in the old "2-bit" code and it also seemed quite
> reasonable, so I kept it.  Things seem to work OK without it, but most
> things are pretty tolerant to their lines glitching (or even driving
> high to low for a short period of time).
> 
> ...but your question made me check again.
> 
> From previous experimentation I'm pretty certain that most pins on the
> exynos are held in the powerdown state even during early bootup of the
> SoC.  The hope is that they are released from powerdown _after_ the
> GPIO init is called.  If not then we're already glitching somewhat as
> we transition from powerdown state to default state before this
> function finally gets called.
> 
> Looking at exynos, that's probably done in exynos_pm_resume(), maybe
> by mucking with the pad retention options?
> 
> Oh, that probably means taht no_irq() is too late and that I need to
> figure out how to get my code called earlier...  The
> exynos_pm_resume() is called by syscore.

How all of this works is basically a good question. I couldn't find any 
mention about pins switching from power down to normal mode in the 
documentation, but maybe there is a small side note somewhere, which I 
could miss.

On S3C6410, for example, there are two modes. State is switched to power 
down mode automatically, but can be switched out either automatically on 
wake-up (exact timing is unknown to me) or by clearing a special bit, 
depending on value of other special bit.

IMHO this is rather important, so we should find out how it work on other 
SoCs and make the code account for it.

Best regards,
Tomasz

Re: [PATCH 1/2] pinctrl: samsung: fix suspend/resume functionality

[Doug Anderson]

Tomasz,

On Thu, May 16, 2013 at 2:27 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> OK. I will be fine to go with your patches, after addressing the comments.
> In the end it's good that you posted them, as reviewing them allowed me to
> find even better ways of doing some things than I had in mine ;) .

Yes.  I often find that the best way to review code is to think about
how I would implement it myself.  Certainly I think we've ended up
with something better / less buggy this way.  ;)


> How all of this works is basically a good question. I couldn't find any
> mention about pins switching from power down to normal mode in the
> documentation, but maybe there is a small side note somewhere, which I
> could miss.
>
> On S3C6410, for example, there are two modes. State is switched to power
> down mode automatically, but can be switched out either automatically on
> wake-up (exact timing is unknown to me) or by clearing a special bit,
> depending on value of other special bit.
>
> IMHO this is rather important, so we should find out how it work on other
> SoCs and make the code account for it.

Agreed that it's important.  ...but it's also good not to have tons of
complexity when it's not needed.  It sounds like S3C6410 could be
handled OK by just using the special bits and waiting to take things
out of power down mode.

...thinking about it, all SoCs that have power down modes (which you
_must_ have if your pinctrl state is lost across a low power) would be
slightly broken if they didn't have a bit to switch out of power down
mode.  Otherwise you're asking for at least some type of glitch
because you'll end up in the default state of pins for a little while
during resume.

That's not to say that there aren't broken SoCs out there and it's
entirely possible that people even designed systems around them
(knowing that the default state of each pin after wakeup is not
harmful to whatever is connected to that pin).  If there are any cases
like this then they would need the special code like my V1 patch had.
Do you know of any SoCs like this that we need to support on kernel
3.10 and higher?


I'm planning on going back to the "simpler" code for my next patchset
unless I can find a problem with it.


-Doug

Re: [PATCH 1/2] pinctrl: samsung: fix suspend/resume functionality

[Tomasz Figa]

On Thursday 16 of May 2013 14:51:53 Doug Anderson wrote:
> Tomasz,
> 
> On Thu, May 16, 2013 at 2:27 PM, Tomasz Figa <tomasz.figa@gmail.com> 
wrote:
> > OK. I will be fine to go with your patches, after addressing the
> > comments. In the end it's good that you posted them, as reviewing
> > them allowed me to find even better ways of doing some things than I
> > had in mine ;) .
> Yes.  I often find that the best way to review code is to think about
> how I would implement it myself.  Certainly I think we've ended up
> with something better / less buggy this way.  ;)
> 
> > How all of this works is basically a good question. I couldn't find
> > any
> > mention about pins switching from power down to normal mode in the
> > documentation, but maybe there is a small side note somewhere, which I
> > could miss.
> > 
> > On S3C6410, for example, there are two modes. State is switched to
> > power down mode automatically, but can be switched out either
> > automatically on wake-up (exact timing is unknown to me) or by
> > clearing a special bit, depending on value of other special bit.
> > 
> > IMHO this is rather important, so we should find out how it work on
> > other SoCs and make the code account for it.
> 
> Agreed that it's important.  ...but it's also good not to have tons of
> complexity when it's not needed.  It sounds like S3C6410 could be
> handled OK by just using the special bits and waiting to take things
> out of power down mode.
> 
> ...thinking about it, all SoCs that have power down modes (which you
> _must_ have if your pinctrl state is lost across a low power) would be
> slightly broken if they didn't have a bit to switch out of power down
> mode.  Otherwise you're asking for at least some type of glitch
> because you'll end up in the default state of pins for a little while
> during resume.
> 
> That's not to say that there aren't broken SoCs out there and it's
> entirely possible that people even designed systems around them
> (knowing that the default state of each pin after wakeup is not
> harmful to whatever is connected to that pin).  If there are any cases
> like this then they would need the special code like my V1 patch had.
> Do you know of any SoCs like this that we need to support on kernel
> 3.10 and higher?

Hmm, I just checked documentation of S3C2440 and S3C2416 they seem to 
retain GPIO settings completely in sleep mode. This would mean that they 
don't require any suspend/resume support in pinctrl driver. Heiko, can you 
confirm this?

Best regards,
Tomasz

Re: [PATCH 2/2] pinctrl: exynos: fix eint wakeup by using irq_set_wake()

[Doug Anderson]

Tomasz,

On Thu, May 16, 2013 at 12:26 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> On Thursday 16 of May 2013 10:12:32 Doug Anderson wrote:
>> From: Prathyush K <prathyush.k@samsung.com>
>>
>> Add the irq_set_wake function for exynos pinctrl to configure the
>> external interrupt wakeup mask register.
>>
>> [dianders: minor nit fixes; port to ToT]
>>
>> Signed-off-by: Prathyush K <prathyush.k@samsung.com>
>> Signed-off-by: Doug Anderson <dianders@chromium.org>
>> ---
>>  drivers/pinctrl/pinctrl-exynos.c  | 45
>> ++++++++++++++++++++++++++++-----------
>> drivers/pinctrl/pinctrl-exynos.h  |  3 ++-
>>  drivers/pinctrl/pinctrl-samsung.h |  2 ++
>>  3 files changed, 37 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/pinctrl/pinctrl-exynos.c
>> b/drivers/pinctrl/pinctrl-exynos.c index ac74281..3ebb2ff 100644
>> --- a/drivers/pinctrl/pinctrl-exynos.c
>> +++ b/drivers/pinctrl/pinctrl-exynos.c
>> @@ -30,6 +30,8 @@
>>  #include <linux/spinlock.h>
>>  #include <linux/err.h>
>>
>> +#include <plat/pm.h>
>> +
>
> This is not going to work with CONFIG_MULTIPLATFORM.

Hmm, this sounds like it might be a bit of a long path, especially
since I haven't been keeping up with what's been going on with
MULTIPLATFORM and I'm currently midway through making 3.8 work (which
has no MULTIPLATFORM).

Perhaps for this patch it makes more sense for you to post your
version and I can review it?  We may end up just keeping our version
of this patch for 3.8 and pick up yours when we do our next rebase.
Does that sound OK?


>> -#define EXYNOS_PIN_BANK_EINTW(pins, reg, id, offs)   \
>> +#define EXYNOS_PIN_BANK_EINTW(pins, reg, id, offs, base)\
>>       {                                               \
>>               .type           = &bank_type_alive,     \
>>               .pctl_offset    = reg,                  \
>>               .nr_pins        = pins,                 \
>>               .eint_type      = EINT_TYPE_WKUP,       \
>>               .eint_offset    = offs,                 \
>> +             .eint_base      = base,                 \
>
> I can't look at my patch at the moment, but I think I have managed to get
> EINT index without adding this extra field.

It looks like this is always 2 * eint_offset in the code above.  Maybe
you just multiplied?  The multiplication works fine although I think
specifying eint_base like this might be more generic and handle future
chips better?  Ya never know...

-Doug

Re: [PATCH 1/2] pinctrl: samsung: fix suspend/resume functionality

[Heiko Stübner]

Am Freitag, 17. Mai 2013, 00:08:34 schrieb Tomasz Figa:
> On Thursday 16 of May 2013 14:51:53 Doug Anderson wrote:
> > Tomasz,
> > 
> > On Thu, May 16, 2013 at 2:27 PM, Tomasz Figa <tomasz.figa@gmail.com>
> 
> wrote:
> > > OK. I will be fine to go with your patches, after addressing the
> > > comments. In the end it's good that you posted them, as reviewing
> > > them allowed me to find even better ways of doing some things than I
> > > had in mine ;) .
> > 
> > Yes.  I often find that the best way to review code is to think about
> > how I would implement it myself.  Certainly I think we've ended up
> > with something better / less buggy this way.  ;)
> > 
> > > How all of this works is basically a good question. I couldn't find
> > > any
> > > mention about pins switching from power down to normal mode in the
> > > documentation, but maybe there is a small side note somewhere, which I
> > > could miss.
> > > 
> > > On S3C6410, for example, there are two modes. State is switched to
> > > power down mode automatically, but can be switched out either
> > > automatically on wake-up (exact timing is unknown to me) or by
> > > clearing a special bit, depending on value of other special bit.
> > > 
> > > IMHO this is rather important, so we should find out how it work on
> > > other SoCs and make the code account for it.
> > 
> > Agreed that it's important.  ...but it's also good not to have tons of
> > complexity when it's not needed.  It sounds like S3C6410 could be
> > handled OK by just using the special bits and waiting to take things
> > out of power down mode.
> > 
> > ...thinking about it, all SoCs that have power down modes (which you
> > _must_ have if your pinctrl state is lost across a low power) would be
> > slightly broken if they didn't have a bit to switch out of power down
> > mode.  Otherwise you're asking for at least some type of glitch
> > because you'll end up in the default state of pins for a little while
> > during resume.
> > 
> > That's not to say that there aren't broken SoCs out there and it's
> > entirely possible that people even designed systems around them
> > (knowing that the default state of each pin after wakeup is not
> > harmful to whatever is connected to that pin).  If there are any cases
> > like this then they would need the special code like my V1 patch had.
> > Do you know of any SoCs like this that we need to support on kernel
> > 3.10 and higher?
> 
> Hmm, I just checked documentation of S3C2440 and S3C2416 they seem to
> retain GPIO settings completely in sleep mode. This would mean that they
> don't require any suspend/resume support in pinctrl driver. Heiko, can you
> confirm this?

Hmm, my system does not have a working suspend right now, but looking at the 
legacy code (mach-s3c24xx/pm.c, etc) tells me that the gpio banks were never 
saved during suspend.

And as there were (and still are) systems with working suspend around, I'd 
assume that you're correct that the pins retain their state.

Is the same true for the s3c64xx, as I didn't find any gpio suspend handling 
for it either.


Heiko

Re: [PATCH 2/2] pinctrl: exynos: fix eint wakeup by using irq_set_wake()

[Tomasz Figa]

On Thursday 16 of May 2013 15:25:15 Doug Anderson wrote:
> Tomasz,
> 
> On Thu, May 16, 2013 at 12:26 PM, Tomasz Figa <tomasz.figa@gmail.com> 
wrote:
> > On Thursday 16 of May 2013 10:12:32 Doug Anderson wrote:
> >> From: Prathyush K <prathyush.k@samsung.com>
> >> 
> >> Add the irq_set_wake function for exynos pinctrl to configure the
> >> external interrupt wakeup mask register.
> >> 
> >> [dianders: minor nit fixes; port to ToT]
> >> 
> >> Signed-off-by: Prathyush K <prathyush.k@samsung.com>
> >> Signed-off-by: Doug Anderson <dianders@chromium.org>
> >> ---
> >> 
> >>  drivers/pinctrl/pinctrl-exynos.c  | 45
> >> 
> >> ++++++++++++++++++++++++++++-----------
> >> drivers/pinctrl/pinctrl-exynos.h  |  3 ++-
> >> 
> >>  drivers/pinctrl/pinctrl-samsung.h |  2 ++
> >>  3 files changed, 37 insertions(+), 13 deletions(-)
> >> 
> >> diff --git a/drivers/pinctrl/pinctrl-exynos.c
> >> b/drivers/pinctrl/pinctrl-exynos.c index ac74281..3ebb2ff 100644
> >> --- a/drivers/pinctrl/pinctrl-exynos.c
> >> +++ b/drivers/pinctrl/pinctrl-exynos.c
> >> @@ -30,6 +30,8 @@
> >> 
> >>  #include <linux/spinlock.h>
> >>  #include <linux/err.h>
> >> 
> >> +#include <plat/pm.h>
> >> +
> > 
> > This is not going to work with CONFIG_MULTIPLATFORM.
> 
> Hmm, this sounds like it might be a bit of a long path, especially
> since I haven't been keeping up with what's been going on with
> MULTIPLATFORM and I'm currently midway through making 3.8 work (which
> has no MULTIPLATFORM).

Well, to make long story short, including headers from plat/ and mach/ 
from files outside plat/ or mach/ is no longer valid with 
CONFIG_MULTIPLATFORM, because more than one plat and/or mach can be 
enabled at the same time. In addition to this, care must be taken for code 
to not break platforms other than written for, when compiled into the 
resulting kernel.

> Perhaps for this patch it makes more sense for you to post your
> version and I can review it?  We may end up just keeping our version
> of this patch for 3.8 and pick up yours when we do our next rebase.
> Does that sound OK?

Fine. I will also send a patch adding save and restore for several EINT 
registers that need it.

> >> -#define EXYNOS_PIN_BANK_EINTW(pins, reg, id, offs)   \
> >> +#define EXYNOS_PIN_BANK_EINTW(pins, reg, id, offs, base)\
> >> 
> >>       {                                               \
> >>       
> >>               .type           = &bank_type_alive,     \
> >>               .pctl_offset    = reg,                  \
> >>               .nr_pins        = pins,                 \
> >>               .eint_type      = EINT_TYPE_WKUP,       \
> >>               .eint_offset    = offs,                 \
> >> 
> >> +             .eint_base      = base,                 \
> > 
> > I can't look at my patch at the moment, but I think I have managed to
> > get EINT index without adding this extra field.
> 
> It looks like this is always 2 * eint_offset in the code above.  Maybe
> you just multiplied?  The multiplication works fine although I think
> specifying eint_base like this might be more generic and handle future
> chips better?  Ya never know...

Since EINT handling is highly SoC-specific (i.e. done in pinctrl-exynos, 
not pinctrl-samsung), such assumption wouldn't be a problem. Let me see 
how I solved this problem in my version tomorrow at work.

Best regards,
Tomasz

Re: [PATCH 1/2] pinctrl: samsung: fix suspend/resume functionality

[Tomasz Figa]

On Friday 17 of May 2013 00:30:38 Heiko Stübner wrote:
> Am Freitag, 17. Mai 2013, 00:08:34 schrieb Tomasz Figa:
> > On Thursday 16 of May 2013 14:51:53 Doug Anderson wrote:
> > > Tomasz,
> > > 
> > > On Thu, May 16, 2013 at 2:27 PM, Tomasz Figa <tomasz.figa@gmail.com>
> > 
> > wrote:
> > > > OK. I will be fine to go with your patches, after addressing the
> > > > comments. In the end it's good that you posted them, as reviewing
> > > > them allowed me to find even better ways of doing some things than
> > > > I
> > > > had in mine ;) .
> > > 
> > > Yes.  I often find that the best way to review code is to think
> > > about
> > > how I would implement it myself.  Certainly I think we've ended up
> > > with something better / less buggy this way.  ;)
> > > 
> > > > How all of this works is basically a good question. I couldn't
> > > > find
> > > > any
> > > > mention about pins switching from power down to normal mode in the
> > > > documentation, but maybe there is a small side note somewhere,
> > > > which I
> > > > could miss.
> > > > 
> > > > On S3C6410, for example, there are two modes. State is switched to
> > > > power down mode automatically, but can be switched out either
> > > > automatically on wake-up (exact timing is unknown to me) or by
> > > > clearing a special bit, depending on value of other special bit.
> > > > 
> > > > IMHO this is rather important, so we should find out how it work
> > > > on
> > > > other SoCs and make the code account for it.
> > > 
> > > Agreed that it's important.  ...but it's also good not to have tons
> > > of
> > > complexity when it's not needed.  It sounds like S3C6410 could be
> > > handled OK by just using the special bits and waiting to take things
> > > out of power down mode.
> > > 
> > > ...thinking about it, all SoCs that have power down modes (which you
> > > _must_ have if your pinctrl state is lost across a low power) would
> > > be
> > > slightly broken if they didn't have a bit to switch out of power
> > > down
> > > mode.  Otherwise you're asking for at least some type of glitch
> > > because you'll end up in the default state of pins for a little
> > > while
> > > during resume.
> > > 
> > > That's not to say that there aren't broken SoCs out there and it's
> > > entirely possible that people even designed systems around them
> > > (knowing that the default state of each pin after wakeup is not
> > > harmful to whatever is connected to that pin).  If there are any
> > > cases
> > > like this then they would need the special code like my V1 patch
> > > had.
> > > Do you know of any SoCs like this that we need to support on kernel
> > > 3.10 and higher?
> > 
> > Hmm, I just checked documentation of S3C2440 and S3C2416 they seem to
> > retain GPIO settings completely in sleep mode. This would mean that
> > they don't require any suspend/resume support in pinctrl driver.
> > Heiko, can you confirm this?
> 
> Hmm, my system does not have a working suspend right now, but looking at
> the legacy code (mach-s3c24xx/pm.c, etc) tells me that the gpio banks
> were never saved during suspend.
> 
> And as there were (and still are) systems with working suspend around,
> I'd assume that you're correct that the pins retain their state.
> 
> Is the same true for the s3c64xx, as I didn't find any gpio suspend
> handling for it either.

Seems like I need some sleep, as I'm already starting to overlook large 
blobs of code. 

Originally, GPIO suspend/resume handlers have been configured in 
drivers/gpio/gpio-samsung.c, by setting pm field of samsung_gpio_chip 
struct to point to appropriate samsung_gpio_pm struct, which contains 
pointers to save and resume callbacks.

In result, samsung_gpio_pm_2bit_* or samsung_gpio_pm_4bit_* have been 
used, depending on bank type, on all SoCs.

Now since the documentation states that wake-up reset doesn't reset GPIO 
registers (at least on S3C2440 and S3C2416), I wonder what is the correct 
way of handling them.

Best regards,
Tomasz

Re: [PATCH 1/2] pinctrl: samsung: fix suspend/resume functionality

[Doug Anderson]

Tomasz,

On Thu, May 16, 2013 at 3:56 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Seems like I need some sleep, as I'm already starting to overlook large
> blobs of code.
>
> Originally, GPIO suspend/resume handlers have been configured in
> drivers/gpio/gpio-samsung.c, by setting pm field of samsung_gpio_chip
> struct to point to appropriate samsung_gpio_pm struct, which contains
> pointers to save and resume callbacks.
>
> In result, samsung_gpio_pm_2bit_* or samsung_gpio_pm_4bit_* have been
> used, depending on bank type, on all SoCs.
>
> Now since the documentation states that wake-up reset doesn't reset GPIO
> registers (at least on S3C2440 and S3C2416), I wonder what is the correct
> way of handling them.

If state of these registers isn't lost on those SoCs then running the
save/restore shouldn't _hurt_ though, right?  If you can run the old
GPIO code on one of those systems and do a suspend/resume you could
check...

-Doug

[PATCH v2] pinctrl: samsung: fix suspend/resume functionality

[Doug Anderson]

The GPIO states need to be restored after s2r and this is not currently
supported in the pinctrl driver. This patch saves the gpio states before
suspend and restores them after resume.

Saving and restoring is done very early using syscore_ops and must
happen before pins are released from their powerdown state.

Patch originally from Prathyush K <prathyush.k@samsung.com> but
rewritten by Doug Anderson <dianders@chromium.org>.

Signed-off-by: Prathyush K <prathyush.k@samsung.com>
Signed-off-by: Doug Anderson <dianders@chromium.org>
---
Changes in v2:
- Now uses sycore_ops to make sure we're early enough.
- Try to handle two CON registers better.
- Should handle s3c24xx better as per Heiko.
- Simpler code; no longer tries to avoid glitching lines since
  we _think_ all current SoCs should have pins in power down state
  when the restore is called.
- Dropped eint patch for now; Tomasz will post his version.

 drivers/pinctrl/pinctrl-samsung.c | 140 ++++++++++++++++++++++++++++++++++++++
 drivers/pinctrl/pinctrl-samsung.h |   5 ++
 2 files changed, 145 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-samsung.c b/drivers/pinctrl/pinctrl-samsung.c
index 9763668..851eada 100644
--- a/drivers/pinctrl/pinctrl-samsung.c
+++ b/drivers/pinctrl/pinctrl-samsung.c
@@ -28,6 +28,7 @@
 #include <linux/gpio.h>
 #include <linux/irqdomain.h>
 #include <linux/spinlock.h>
+#include <linux/syscore_ops.h>
 
 #include "core.h"
 #include "pinctrl-samsung.h"
@@ -48,6 +49,9 @@ static struct pin_config {
 	{ "samsung,pin-pud-pdn", PINCFG_TYPE_PUD_PDN },
 };
 
+/* Global list of devices (struct samsung_pinctrl_drv_data) */
+LIST_HEAD(drvdata_list);
+
 static unsigned int pin_base;
 
 static inline struct samsung_pin_bank *gc_to_pin_bank(struct gpio_chip *gc)
@@ -961,9 +965,137 @@ static int samsung_pinctrl_probe(struct platform_device *pdev)
 		ctrl->eint_wkup_init(drvdata);
 
 	platform_set_drvdata(pdev, drvdata);
+
+	/* Add to the global list */
+	list_add_tail(&drvdata->node, &drvdata_list);
+
 	return 0;
 }
 
+#ifdef CONFIG_PM
+
+/**
+ * samsung_pinctrl_suspend_dev - save pinctrl state for suspend for a device
+ *
+ * Save data for all banks handled by this device.
+ */
+static void samsung_pinctrl_suspend_dev(
+	struct samsung_pinctrl_drv_data *drvdata)
+{
+	struct samsung_pin_ctrl *ctrl = drvdata->ctrl;
+	void __iomem *virt_base = drvdata->virt_base;
+	int i;
+
+	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;
+
+		u8 *offs = bank->type->reg_offset;
+		u8 *widths = bank->type->fld_width;
+		enum pincfg_type type;
+
+		for (type = 0; type < PINCFG_TYPE_NUM; type++)
+			if (widths[type])
+				bank->pm_save[type] = readl(reg + offs[type]);
+
+		if (widths[PINCFG_TYPE_FUNC] * bank->nr_pins > 32) {
+			/* Some banks have two config registers */
+			bank->pm_save[PINCFG_TYPE_NUM] =
+				readl(reg + offs[PINCFG_TYPE_FUNC] + 4);
+			pr_debug("Save %s @ %p (con %#010x %08x)\n",
+				 bank->name, reg,
+				 bank->pm_save[PINCFG_TYPE_FUNC],
+				 bank->pm_save[PINCFG_TYPE_NUM]);
+		} else {
+			pr_debug("Save %s @ %p (con %#010x)\n", bank->name,
+				 reg, bank->pm_save[PINCFG_TYPE_FUNC]);
+		}
+	}
+}
+
+/**
+ * samsung_pinctrl_resume_dev - restore pinctrl state from suspend for a device
+ *
+ * Restore one of the banks that was saved during suspend.
+ *
+ * We don't bother doing anything complicated to avoid glitching lines since
+ * we're called before pad retention is turned off.
+ */
+static void samsung_pinctrl_resume_dev(struct samsung_pinctrl_drv_data *drvdata)
+{
+	struct samsung_pin_ctrl *ctrl = drvdata->ctrl;
+	void __iomem *virt_base = drvdata->virt_base;
+	int i;
+
+	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;
+
+		u8 *offs = bank->type->reg_offset;
+		u8 *widths = bank->type->fld_width;
+		enum pincfg_type type;
+
+		if (widths[PINCFG_TYPE_FUNC] * bank->nr_pins > 32) {
+			/* Some banks have two config registers */
+			pr_debug("%s @ %p (con %#010x %08x => %#010x %08x)\n",
+				 bank->name, reg,
+				 readl(reg + offs[PINCFG_TYPE_FUNC]),
+				 readl(reg + offs[PINCFG_TYPE_FUNC] + 4),
+				 bank->pm_save[PINCFG_TYPE_FUNC],
+				 bank->pm_save[PINCFG_TYPE_NUM]);
+			writel(bank->pm_save[PINCFG_TYPE_NUM],
+			       reg + offs[PINCFG_TYPE_FUNC] + 4);
+		} else {
+			pr_debug("%s @ %p (con %#010x => %#010x)\n", bank->name,
+				 reg, readl(reg + offs[PINCFG_TYPE_FUNC]),
+				 bank->pm_save[PINCFG_TYPE_FUNC]);
+		}
+		for (type = 0; type < PINCFG_TYPE_NUM; type++)
+			if (widths[type])
+				writel(bank->pm_save[type], reg + offs[type]);
+	}
+}
+
+/**
+ * samsung_pinctrl_suspend - save pinctrl state for suspend
+ *
+ * Save data for all banks across all devices.
+ */
+static int samsung_pinctrl_suspend(void)
+{
+	struct samsung_pinctrl_drv_data *drvdata;
+
+	list_for_each_entry(drvdata, &drvdata_list, node) {
+		samsung_pinctrl_suspend_dev(drvdata);
+	}
+
+	return 0;
+}
+
+/**
+ * samsung_pinctrl_resume - restore pinctrl state for suspend
+ *
+ * Restore data for all banks across all devices.
+ */
+static void samsung_pinctrl_resume(void)
+{
+	struct samsung_pinctrl_drv_data *drvdata;
+
+	list_for_each_entry_reverse(drvdata, &drvdata_list, node) {
+		samsung_pinctrl_resume_dev(drvdata);
+	}
+}
+
+#else
+#define samsung_pinctrl_suspend		NULL
+#define samsung_pinctrl_resume		NULL
+#endif
+
+static struct syscore_ops samsung_pinctrl_syscore_ops = {
+	.suspend	= samsung_pinctrl_suspend,
+	.resume		= samsung_pinctrl_resume,
+};
+
 static const struct of_device_id samsung_pinctrl_dt_match[] = {
 #ifdef CONFIG_PINCTRL_EXYNOS
 	{ .compatible = "samsung,exynos4210-pinctrl",
@@ -992,6 +1124,14 @@ static struct platform_driver samsung_pinctrl_driver = {
 
 static int __init samsung_pinctrl_drv_register(void)
 {
+	/*
+	 * Register syscore ops for save/restore of registers across suspend.
+	 * It's important to ensure that this driver is running at an earlier
+	 * initcall level than any arch-specific init calls that install syscore
+	 * ops that turn off pad retention (like exynos_pm_resume).
+	 */
+	register_syscore_ops(&samsung_pinctrl_syscore_ops);
+
 	return platform_driver_register(&samsung_pinctrl_driver);
 }
 postcore_initcall(samsung_pinctrl_drv_register);
diff --git a/drivers/pinctrl/pinctrl-samsung.h b/drivers/pinctrl/pinctrl-samsung.h
index 7c7f9eb..9f5cc81 100644
--- a/drivers/pinctrl/pinctrl-samsung.h
+++ b/drivers/pinctrl/pinctrl-samsung.h
@@ -127,6 +127,7 @@ struct samsung_pin_bank_type {
  * @gpio_chip: GPIO chip of the bank.
  * @grange: linux gpio pin range supported by this bank.
  * @slock: spinlock protecting bank registers
+ * @pm_save: saved register values during suspend
  */
 struct samsung_pin_bank {
 	struct samsung_pin_bank_type *type;
@@ -144,6 +145,8 @@ struct samsung_pin_bank {
 	struct gpio_chip gpio_chip;
 	struct pinctrl_gpio_range grange;
 	spinlock_t slock;
+
+	u32 pm_save[PINCFG_TYPE_NUM + 1]; /* +1 to handle double CON registers*/
 };
 
 /**
@@ -189,6 +192,7 @@ struct samsung_pin_ctrl {
 
 /**
  * struct samsung_pinctrl_drv_data: wrapper for holding driver data together.
+ * @node: global list node
  * @virt_base: register base address of the controller.
  * @dev: device instance representing the controller.
  * @irq: interrpt number used by the controller to notify gpio interrupts.
@@ -201,6 +205,7 @@ struct samsung_pin_ctrl {
  * @nr_function: number of such pin functions.
  */
 struct samsung_pinctrl_drv_data {
+	struct list_head		node;
 	void __iomem			*virt_base;
 	struct device			*dev;
 	int				irq;
-- 
1.8.2.1

Re: [PATCH 2/2] pinctrl: exynos: fix eint wakeup by using irq_set_wake()

[Doug Anderson]

Tomasz,

On Thu, May 16, 2013 at 3:37 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Well, to make long story short, including headers from plat/ and mach/
> from files outside plat/ or mach/ is no longer valid with
> CONFIG_MULTIPLATFORM, because more than one plat and/or mach can be
> enabled at the same time. In addition to this, care must be taken for code
> to not break platforms other than written for, when compiled into the
> resulting kernel.

Right.  That makes sense.  That's also why it would be a bit of a
longshot for me to get this done right now.  I'd imagine that there
would be a number of changes to the samsung pm infrastructure that are
needed to make this work and I don't have all of those in my tree
right now.  We've already picked back a lot to 3.8, but multiplatform
seems too much.


>> Perhaps for this patch it makes more sense for you to post your
>> version and I can review it?  We may end up just keeping our version
>> of this patch for 3.8 and pick up yours when we do our next rebase.
>> Does that sound OK?
>
> Fine. I will also send a patch adding save and restore for several EINT
> registers that need it.

OK, sounds good.  I was trying to figure out why we didn't seem to
have those in our 3.4 stuff and that it seems to work without
saving/restoring.  I assumed that maybe higher level code was
masking/unmasking interrupts but didn't dig.


> Since EINT handling is highly SoC-specific (i.e. done in pinctrl-exynos,
> not pinctrl-samsung), such assumption wouldn't be a problem. Let me see
> how I solved this problem in my version tomorrow at work.

Fair enough.  :)  Looking forward to seeing your patch!

-Doug

Re: [PATCH 1/2] pinctrl: samsung: fix suspend/resume functionality

[Doug Anderson]

Tomasz,

On Thu, May 16, 2013 at 4:10 PM, Doug Anderson <dianders@chromium.org> wrote:
> If state of these registers isn't lost on those SoCs then running the
> save/restore shouldn't _hurt_ though, right?  If you can run the old
> GPIO code on one of those systems and do a suspend/resume you could
> check...

I think it's been too long of a day for me, too.

I just thought about this and realized that there is no "powerdown"
registers for the GPX banks on exynos5250.  ...and they don't lose
their state at sleep!  ...so maybe a reasonable thing to do would be
to skip save/restore in any case where there are no powerdown
registers?

You can see a printout in my case:

[  412.840000] gpx0 @ f004ac00 (con 0x30000110 => 0x30000110)
[  412.840000] gpx1 @ f004ac20 (con 0x1f10ff10 => 0x1f10ff10)
[  412.840000] gpx2 @ f004ac40 (con 0x1f000f0f => 0x1f000f0f)
[  412.840000] gpx3 @ f004ac60 (con 0x00f00f01 => 0x00f00f01)

-Doug

[PATCH v3] pinctrl: samsung: fix suspend/resume functionality

[Doug Anderson]

The GPIO states need to be restored after s2r and this is not currently
supported in the pinctrl driver. This patch saves the gpio states before
suspend and restores them after resume.

Saving and restoring is done very early using syscore_ops and must
happen before pins are released from their powerdown state.

Patch originally from Prathyush K <prathyush.k@samsung.com> but
rewritten by Doug Anderson <dianders@chromium.org>.

Signed-off-by: Prathyush K <prathyush.k@samsung.com>
Signed-off-by: Doug Anderson <dianders@chromium.org>
---
Changes in v3:
- Skip save and restore for banks with no powerdown config.

Changes in v2:
- Now uses sycore_ops to make sure we're early enough.
- Try to handle two CON registers better.
- Should handle s3c24xx better as per Heiko.
- Simpler code; no longer tries to avoid glitching lines since
  we _think_ all current SoCs should have pins in power down state
  when the restore is called.
- Dropped eint patch for now; Tomasz will post his version.

 drivers/pinctrl/pinctrl-samsung.c | 148 ++++++++++++++++++++++++++++++++++++++
 drivers/pinctrl/pinctrl-samsung.h |   5 ++
 2 files changed, 153 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-samsung.c b/drivers/pinctrl/pinctrl-samsung.c
index 9763668..d45e36f 100644
--- a/drivers/pinctrl/pinctrl-samsung.c
+++ b/drivers/pinctrl/pinctrl-samsung.c
@@ -28,6 +28,7 @@
 #include <linux/gpio.h>
 #include <linux/irqdomain.h>
 #include <linux/spinlock.h>
+#include <linux/syscore_ops.h>
 
 #include "core.h"
 #include "pinctrl-samsung.h"
@@ -48,6 +49,9 @@ static struct pin_config {
 	{ "samsung,pin-pud-pdn", PINCFG_TYPE_PUD_PDN },
 };
 
+/* Global list of devices (struct samsung_pinctrl_drv_data) */
+LIST_HEAD(drvdata_list);
+
 static unsigned int pin_base;
 
 static inline struct samsung_pin_bank *gc_to_pin_bank(struct gpio_chip *gc)
@@ -961,9 +965,145 @@ static int samsung_pinctrl_probe(struct platform_device *pdev)
 		ctrl->eint_wkup_init(drvdata);
 
 	platform_set_drvdata(pdev, drvdata);
+
+	/* Add to the global list */
+	list_add_tail(&drvdata->node, &drvdata_list);
+
 	return 0;
 }
 
+#ifdef CONFIG_PM
+
+/**
+ * samsung_pinctrl_suspend_dev - save pinctrl state for suspend for a device
+ *
+ * Save data for all banks handled by this device.
+ */
+static void samsung_pinctrl_suspend_dev(
+	struct samsung_pinctrl_drv_data *drvdata)
+{
+	struct samsung_pin_ctrl *ctrl = drvdata->ctrl;
+	void __iomem *virt_base = drvdata->virt_base;
+	int i;
+
+	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;
+
+		u8 *offs = bank->type->reg_offset;
+		u8 *widths = bank->type->fld_width;
+		enum pincfg_type type;
+
+		/* Registers without a powerdown config aren't lost */
+		if (!widths[PINCFG_TYPE_CON_PDN])
+			continue;
+
+		for (type = 0; type < PINCFG_TYPE_NUM; type++)
+			if (widths[type])
+				bank->pm_save[type] = readl(reg + offs[type]);
+
+		if (widths[PINCFG_TYPE_FUNC] * bank->nr_pins > 32) {
+			/* Some banks have two config registers */
+			bank->pm_save[PINCFG_TYPE_NUM] =
+				readl(reg + offs[PINCFG_TYPE_FUNC] + 4);
+			pr_debug("Save %s @ %p (con %#010x %08x)\n",
+				 bank->name, reg,
+				 bank->pm_save[PINCFG_TYPE_FUNC],
+				 bank->pm_save[PINCFG_TYPE_NUM]);
+		} else {
+			pr_debug("Save %s @ %p (con %#010x)\n", bank->name,
+				 reg, bank->pm_save[PINCFG_TYPE_FUNC]);
+		}
+	}
+}
+
+/**
+ * samsung_pinctrl_resume_dev - restore pinctrl state from suspend for a device
+ *
+ * Restore one of the banks that was saved during suspend.
+ *
+ * We don't bother doing anything complicated to avoid glitching lines since
+ * we're called before pad retention is turned off.
+ */
+static void samsung_pinctrl_resume_dev(struct samsung_pinctrl_drv_data *drvdata)
+{
+	struct samsung_pin_ctrl *ctrl = drvdata->ctrl;
+	void __iomem *virt_base = drvdata->virt_base;
+	int i;
+
+	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;
+
+		u8 *offs = bank->type->reg_offset;
+		u8 *widths = bank->type->fld_width;
+		enum pincfg_type type;
+
+		/* Registers without a powerdown config aren't lost */
+		if (!widths[PINCFG_TYPE_CON_PDN])
+			continue;
+
+		if (widths[PINCFG_TYPE_FUNC] * bank->nr_pins > 32) {
+			/* Some banks have two config registers */
+			pr_debug("%s @ %p (con %#010x %08x => %#010x %08x)\n",
+				 bank->name, reg,
+				 readl(reg + offs[PINCFG_TYPE_FUNC]),
+				 readl(reg + offs[PINCFG_TYPE_FUNC] + 4),
+				 bank->pm_save[PINCFG_TYPE_FUNC],
+				 bank->pm_save[PINCFG_TYPE_NUM]);
+			writel(bank->pm_save[PINCFG_TYPE_NUM],
+			       reg + offs[PINCFG_TYPE_FUNC] + 4);
+		} else {
+			pr_debug("%s @ %p (con %#010x => %#010x)\n", bank->name,
+				 reg, readl(reg + offs[PINCFG_TYPE_FUNC]),
+				 bank->pm_save[PINCFG_TYPE_FUNC]);
+		}
+		for (type = 0; type < PINCFG_TYPE_NUM; type++)
+			if (widths[type])
+				writel(bank->pm_save[type], reg + offs[type]);
+	}
+}
+
+/**
+ * samsung_pinctrl_suspend - save pinctrl state for suspend
+ *
+ * Save data for all banks across all devices.
+ */
+static int samsung_pinctrl_suspend(void)
+{
+	struct samsung_pinctrl_drv_data *drvdata;
+
+	list_for_each_entry(drvdata, &drvdata_list, node) {
+		samsung_pinctrl_suspend_dev(drvdata);
+	}
+
+	return 0;
+}
+
+/**
+ * samsung_pinctrl_resume - restore pinctrl state for suspend
+ *
+ * Restore data for all banks across all devices.
+ */
+static void samsung_pinctrl_resume(void)
+{
+	struct samsung_pinctrl_drv_data *drvdata;
+
+	list_for_each_entry_reverse(drvdata, &drvdata_list, node) {
+		samsung_pinctrl_resume_dev(drvdata);
+	}
+}
+
+#else
+#define samsung_pinctrl_suspend		NULL
+#define samsung_pinctrl_resume		NULL
+#endif
+
+static struct syscore_ops samsung_pinctrl_syscore_ops = {
+	.suspend	= samsung_pinctrl_suspend,
+	.resume		= samsung_pinctrl_resume,
+};
+
 static const struct of_device_id samsung_pinctrl_dt_match[] = {
 #ifdef CONFIG_PINCTRL_EXYNOS
 	{ .compatible = "samsung,exynos4210-pinctrl",
@@ -992,6 +1132,14 @@ static struct platform_driver samsung_pinctrl_driver = {
 
 static int __init samsung_pinctrl_drv_register(void)
 {
+	/*
+	 * Register syscore ops for save/restore of registers across suspend.
+	 * It's important to ensure that this driver is running at an earlier
+	 * initcall level than any arch-specific init calls that install syscore
+	 * ops that turn off pad retention (like exynos_pm_resume).
+	 */
+	register_syscore_ops(&samsung_pinctrl_syscore_ops);
+
 	return platform_driver_register(&samsung_pinctrl_driver);
 }
 postcore_initcall(samsung_pinctrl_drv_register);
diff --git a/drivers/pinctrl/pinctrl-samsung.h b/drivers/pinctrl/pinctrl-samsung.h
index 7c7f9eb..9f5cc81 100644
--- a/drivers/pinctrl/pinctrl-samsung.h
+++ b/drivers/pinctrl/pinctrl-samsung.h
@@ -127,6 +127,7 @@ struct samsung_pin_bank_type {
  * @gpio_chip: GPIO chip of the bank.
  * @grange: linux gpio pin range supported by this bank.
  * @slock: spinlock protecting bank registers
+ * @pm_save: saved register values during suspend
  */
 struct samsung_pin_bank {
 	struct samsung_pin_bank_type *type;
@@ -144,6 +145,8 @@ struct samsung_pin_bank {
 	struct gpio_chip gpio_chip;
 	struct pinctrl_gpio_range grange;
 	spinlock_t slock;
+
+	u32 pm_save[PINCFG_TYPE_NUM + 1]; /* +1 to handle double CON registers*/
 };
 
 /**
@@ -189,6 +192,7 @@ struct samsung_pin_ctrl {
 
 /**
  * struct samsung_pinctrl_drv_data: wrapper for holding driver data together.
+ * @node: global list node
  * @virt_base: register base address of the controller.
  * @dev: device instance representing the controller.
  * @irq: interrpt number used by the controller to notify gpio interrupts.
@@ -201,6 +205,7 @@ struct samsung_pin_ctrl {
  * @nr_function: number of such pin functions.
  */
 struct samsung_pinctrl_drv_data {
+	struct list_head		node;
 	void __iomem			*virt_base;
 	struct device			*dev;
 	int				irq;
-- 
1.8.2.1

Re: [PATCH v3] pinctrl: samsung: fix suspend/resume functionality

[Tomasz Figa]

Hi Doug,

On Thursday 16 of May 2013 21:33:18 Doug Anderson wrote:
> The GPIO states need to be restored after s2r and this is not currently
> supported in the pinctrl driver. This patch saves the gpio states before
> suspend and restores them after resume.
> 
> Saving and restoring is done very early using syscore_ops and must
> happen before pins are released from their powerdown state.
> 
> Patch originally from Prathyush K <prathyush.k@samsung.com> but
> rewritten by Doug Anderson <dianders@chromium.org>.
> 
> Signed-off-by: Prathyush K <prathyush.k@samsung.com>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
> Changes in v3:
> - Skip save and restore for banks with no powerdown config.
> 
> Changes in v2:
> - Now uses sycore_ops to make sure we're early enough.
> - Try to handle two CON registers better.
> - Should handle s3c24xx better as per Heiko.
> - Simpler code; no longer tries to avoid glitching lines since
>   we _think_ all current SoCs should have pins in power down state
>   when the restore is called.
> - Dropped eint patch for now; Tomasz will post his version.
> 
>  drivers/pinctrl/pinctrl-samsung.c | 148
> ++++++++++++++++++++++++++++++++++++++ drivers/pinctrl/pinctrl-samsung.h | 
>  5 ++
>  2 files changed, 153 insertions(+)

On Exynos4210-based Trats board:

Tested-by: Tomasz Figa <t.figa@samsung.com>

Acked-by: Tomasz Figa <t.figa@samsung.com>

I will send several complementary patches in a while.

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

> diff --git a/drivers/pinctrl/pinctrl-samsung.c
> b/drivers/pinctrl/pinctrl-samsung.c index 9763668..d45e36f 100644
> --- a/drivers/pinctrl/pinctrl-samsung.c
> +++ b/drivers/pinctrl/pinctrl-samsung.c
> @@ -28,6 +28,7 @@
>  #include <linux/gpio.h>
>  #include <linux/irqdomain.h>
>  #include <linux/spinlock.h>
> +#include <linux/syscore_ops.h>
> 
>  #include "core.h"
>  #include "pinctrl-samsung.h"
> @@ -48,6 +49,9 @@ static struct pin_config {
>  	{ "samsung,pin-pud-pdn", PINCFG_TYPE_PUD_PDN },
>  };
> 
> +/* Global list of devices (struct samsung_pinctrl_drv_data) */
> +LIST_HEAD(drvdata_list);
> +
>  static unsigned int pin_base;
> 
>  static inline struct samsung_pin_bank *gc_to_pin_bank(struct gpio_chip *gc)
> @@ -961,9 +965,145 @@ static int samsung_pinctrl_probe(struct
> platform_device *pdev) ctrl->eint_wkup_init(drvdata);
> 
>  	platform_set_drvdata(pdev, drvdata);
> +
> +	/* Add to the global list */
> +	list_add_tail(&drvdata->node, &drvdata_list);
> +
>  	return 0;
>  }
> 
> +#ifdef CONFIG_PM
> +
> +/**
> + * samsung_pinctrl_suspend_dev - save pinctrl state for suspend for a
> device + *
> + * Save data for all banks handled by this device.
> + */
> +static void samsung_pinctrl_suspend_dev(
> +	struct samsung_pinctrl_drv_data *drvdata)
> +{
> +	struct samsung_pin_ctrl *ctrl = drvdata->ctrl;
> +	void __iomem *virt_base = drvdata->virt_base;
> +	int i;
> +
> +	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;
> +
> +		u8 *offs = bank->type->reg_offset;
> +		u8 *widths = bank->type->fld_width;
> +		enum pincfg_type type;
> +
> +		/* Registers without a powerdown config aren't lost */
> +		if (!widths[PINCFG_TYPE_CON_PDN])
> +			continue;
> +
> +		for (type = 0; type < PINCFG_TYPE_NUM; type++)
> +			if (widths[type])
> +				bank->pm_save[type] = readl(reg + offs[type]);
> +
> +		if (widths[PINCFG_TYPE_FUNC] * bank->nr_pins > 32) {
> +			/* Some banks have two config registers */
> +			bank->pm_save[PINCFG_TYPE_NUM] =
> +				readl(reg + offs[PINCFG_TYPE_FUNC] + 4);
> +			pr_debug("Save %s @ %p (con %#010x %08x)\n",
> +				 bank->name, reg,
> +				 bank->pm_save[PINCFG_TYPE_FUNC],
> +				 bank->pm_save[PINCFG_TYPE_NUM]);
> +		} else {
> +			pr_debug("Save %s @ %p (con %#010x)\n", bank->name,
> +				 reg, bank->pm_save[PINCFG_TYPE_FUNC]);
> +		}
> +	}
> +}
> +
> +/**
> + * samsung_pinctrl_resume_dev - restore pinctrl state from suspend for a
> device + *
> + * Restore one of the banks that was saved during suspend.
> + *
> + * We don't bother doing anything complicated to avoid glitching lines
> since + * we're called before pad retention is turned off.
> + */
> +static void samsung_pinctrl_resume_dev(struct samsung_pinctrl_drv_data
> *drvdata) +{
> +	struct samsung_pin_ctrl *ctrl = drvdata->ctrl;
> +	void __iomem *virt_base = drvdata->virt_base;
> +	int i;
> +
> +	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;
> +
> +		u8 *offs = bank->type->reg_offset;
> +		u8 *widths = bank->type->fld_width;
> +		enum pincfg_type type;
> +
> +		/* Registers without a powerdown config aren't lost */
> +		if (!widths[PINCFG_TYPE_CON_PDN])
> +			continue;
> +
> +		if (widths[PINCFG_TYPE_FUNC] * bank->nr_pins > 32) {
> +			/* Some banks have two config registers */
> +			pr_debug("%s @ %p (con %#010x %08x => %#010x %08x)\n",
> +				 bank->name, reg,
> +				 readl(reg + offs[PINCFG_TYPE_FUNC]),
> +				 readl(reg + offs[PINCFG_TYPE_FUNC] + 4),
> +				 bank->pm_save[PINCFG_TYPE_FUNC],
> +				 bank->pm_save[PINCFG_TYPE_NUM]);
> +			writel(bank->pm_save[PINCFG_TYPE_NUM],
> +			       reg + offs[PINCFG_TYPE_FUNC] + 4);
> +		} else {
> +			pr_debug("%s @ %p (con %#010x => %#010x)\n", bank->name,
> +				 reg, readl(reg + offs[PINCFG_TYPE_FUNC]),
> +				 bank->pm_save[PINCFG_TYPE_FUNC]);
> +		}
> +		for (type = 0; type < PINCFG_TYPE_NUM; type++)
> +			if (widths[type])
> +				writel(bank->pm_save[type], reg + offs[type]);
> +	}
> +}
> +
> +/**
> + * samsung_pinctrl_suspend - save pinctrl state for suspend
> + *
> + * Save data for all banks across all devices.
> + */
> +static int samsung_pinctrl_suspend(void)
> +{
> +	struct samsung_pinctrl_drv_data *drvdata;
> +
> +	list_for_each_entry(drvdata, &drvdata_list, node) {
> +		samsung_pinctrl_suspend_dev(drvdata);
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * samsung_pinctrl_resume - restore pinctrl state for suspend
> + *
> + * Restore data for all banks across all devices.
> + */
> +static void samsung_pinctrl_resume(void)
> +{
> +	struct samsung_pinctrl_drv_data *drvdata;
> +
> +	list_for_each_entry_reverse(drvdata, &drvdata_list, node) {
> +		samsung_pinctrl_resume_dev(drvdata);
> +	}
> +}
> +
> +#else
> +#define samsung_pinctrl_suspend		NULL
> +#define samsung_pinctrl_resume		NULL
> +#endif
> +
> +static struct syscore_ops samsung_pinctrl_syscore_ops = {
> +	.suspend	= samsung_pinctrl_suspend,
> +	.resume		= samsung_pinctrl_resume,
> +};
> +
>  static const struct of_device_id samsung_pinctrl_dt_match[] = {
>  #ifdef CONFIG_PINCTRL_EXYNOS
>  	{ .compatible = "samsung,exynos4210-pinctrl",
> @@ -992,6 +1132,14 @@ static struct platform_driver samsung_pinctrl_driver =
> {
> 
>  static int __init samsung_pinctrl_drv_register(void)
>  {
> +	/*
> +	 * Register syscore ops for save/restore of registers across suspend.
> +	 * It's important to ensure that this driver is running at an earlier
> +	 * initcall level than any arch-specific init calls that install syscore
> +	 * ops that turn off pad retention (like exynos_pm_resume).
> +	 */
> +	register_syscore_ops(&samsung_pinctrl_syscore_ops);
> +
>  	return platform_driver_register(&samsung_pinctrl_driver);
>  }
>  postcore_initcall(samsung_pinctrl_drv_register);
> diff --git a/drivers/pinctrl/pinctrl-samsung.h
> b/drivers/pinctrl/pinctrl-samsung.h index 7c7f9eb..9f5cc81 100644
> --- a/drivers/pinctrl/pinctrl-samsung.h
> +++ b/drivers/pinctrl/pinctrl-samsung.h
> @@ -127,6 +127,7 @@ struct samsung_pin_bank_type {
>   * @gpio_chip: GPIO chip of the bank.
>   * @grange: linux gpio pin range supported by this bank.
>   * @slock: spinlock protecting bank registers
> + * @pm_save: saved register values during suspend
>   */
>  struct samsung_pin_bank {
>  	struct samsung_pin_bank_type *type;
> @@ -144,6 +145,8 @@ struct samsung_pin_bank {
>  	struct gpio_chip gpio_chip;
>  	struct pinctrl_gpio_range grange;
>  	spinlock_t slock;
> +
> +	u32 pm_save[PINCFG_TYPE_NUM + 1]; /* +1 to handle double CON registers*/
>  };
> 
>  /**
> @@ -189,6 +192,7 @@ struct samsung_pin_ctrl {
> 
>  /**
>   * struct samsung_pinctrl_drv_data: wrapper for holding driver data
> together. + * @node: global list node
>   * @virt_base: register base address of the controller.
>   * @dev: device instance representing the controller.
>   * @irq: interrpt number used by the controller to notify gpio interrupts.
> @@ -201,6 +205,7 @@ struct samsung_pin_ctrl {
>   * @nr_function: number of such pin functions.
>   */
>  struct samsung_pinctrl_drv_data {
> +	struct list_head		node;
>  	void __iomem			*virt_base;
>  	struct device			*dev;
>  	int				irq;

Re: [PATCH v3] pinctrl: samsung: fix suspend/resume functionality

[Linus Walleij]

On Fri, May 17, 2013 at 6:33 AM, Doug Anderson <dianders@chromium.org> wrote:

> The GPIO states need to be restored after s2r and this is not currently
> supported in the pinctrl driver. This patch saves the gpio states before
> suspend and restores them after resume.
>
> Saving and restoring is done very early using syscore_ops and must
> happen before pins are released from their powerdown state.
>
> Patch originally from Prathyush K <prathyush.k@samsung.com> but
> rewritten by Doug Anderson <dianders@chromium.org>.
>
> Signed-off-by: Prathyush K <prathyush.k@samsung.com>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
> Changes in v3:
> - Skip save and restore for banks with no powerdown config.
>
> Changes in v2:
> - Now uses sycore_ops to make sure we're early enough.
> - Try to handle two CON registers better.
> - Should handle s3c24xx better as per Heiko.
> - Simpler code; no longer tries to avoid glitching lines since
>   we _think_ all current SoCs should have pins in power down state
>   when the restore is called.
> - Dropped eint patch for now; Tomasz will post his version.

Looks good to me.
Acked-by: Linus Walleij <linus.walleij@linaro.org>

How are you going to merge this?

Samsung tree?

My pinctrl development tree?

Or my fixes tree, if it's a regression for v3.10?

Yours,
Linus Walleij

Re: [PATCH v3] pinctrl: samsung: fix suspend/resume functionality

[Doug Anderson]

Linus,

On Mon, May 20, 2013 at 4:47 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> Looks good to me.
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>
> How are you going to merge this?
>
> Samsung tree?
>
> My pinctrl development tree?
>
> Or my fixes tree, if it's a regression for v3.10?

It is a regression for v3.10 so it would be nice to get it in there
(Tomasz's patches, too).

It looks like the majority of recent commits to this file went through
your tree and that seems fine to me unless anyone else on this thread
has objections.

Thanks!

-Doug

Re: [PATCH v3] pinctrl: samsung: fix suspend/resume functionality

[Kukjin Kim]

On 05/20/13 23:59, Doug Anderson wrote:
> Linus,
>
> On Mon, May 20, 2013 at 4:47 AM, Linus Walleij<linus.walleij@linaro.org>  wrote:
>> Looks good to me.
>> Acked-by: Linus Walleij<linus.walleij@linaro.org>
>>
>> How are you going to merge this?
>>
>> Samsung tree?
>>
>> My pinctrl development tree?
>>
>> Or my fixes tree, if it's a regression for v3.10?
>
> It is a regression for v3.10 so it would be nice to get it in there
> (Tomasz's patches, too).
>
> It looks like the majority of recent commits to this file went through
> your tree and that seems fine to me unless anyone else on this thread
> has objections.
>
Linus, I'm fine on this. Please take this into your fixes tree with my 
ack, if you want:

Acked-by: Kukjin Kim <kgene.kim@samsung.com>

Thanks.

- Kukjin

Re: [PATCH v3] pinctrl: samsung: fix suspend/resume functionality

[Olof Johansson]

On Mon, May 20, 2013 at 07:59:27AM -0700, Doug Anderson wrote:
> Linus,
> 
> On Mon, May 20, 2013 at 4:47 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > Looks good to me.
> > Acked-by: Linus Walleij <linus.walleij@linaro.org>
> >
> > How are you going to merge this?
> >
> > Samsung tree?
> >
> > My pinctrl development tree?
> >
> > Or my fixes tree, if it's a regression for v3.10?
> 
> It is a regression for v3.10 so it would be nice to get it in there
> (Tomasz's patches, too).
> 
> It looks like the majority of recent commits to this file went through
> your tree and that seems fine to me unless anyone else on this thread
> has objections.

Sounds good to me.


-Olof

Re: [PATCH v3] pinctrl: samsung: fix suspend/resume functionality

[Linus Walleij]

On Fri, May 17, 2013 at 6:33 AM, Doug Anderson <dianders@chromium.org> wrote:

> The GPIO states need to be restored after s2r and this is not currently
> supported in the pinctrl driver. This patch saves the gpio states before
> suspend and restores them after resume.
>
> Saving and restoring is done very early using syscore_ops and must
> happen before pins are released from their powerdown state.
>
> Patch originally from Prathyush K <prathyush.k@samsung.com> but
> rewritten by Doug Anderson <dianders@chromium.org>.
>
> Signed-off-by: Prathyush K <prathyush.k@samsung.com>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
> Changes in v3:

Applied to my fixes branch for v3.10 with some collected
Tested-bys/Acks.

Yours,
Linus Walleij