diff mbox

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

Message ID 1368807872-2601-7-git-send-email-t.figa@samsung.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tomasz Figa May 17, 2013, 4:24 p.m. UTC 
Some GPIO EINT control registers needs to be preserved across
suspend/resume cycle. This patch extends the driver to take care of
this.

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

Comments

Doug Anderson May 17, 2013, 7:25 p.m. UTC | #1 
Tomasz,

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

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

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

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

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


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

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

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

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

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


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

Optional: debug statements:

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

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

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

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

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

Right, seems reasonable.

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

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

Best regards,
Tomasz
Doug Anderson May 17, 2013, 9:18 p.m. UTC | #3 
Tomasz,

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

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

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


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

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


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


-Doug
diff mbox

Patch

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