[PATCH v2 0/2] Pinctrl fixes for rockchip

[PATCH v2 0/2] Pinctrl fixes for rockchip

[Doug Anderson]

These two patches fix some pinctrl issues on rockchip.  The first
fixes a real issue where interrupts were being left on in
suspend/resume and waking the system up when they shouldn't.  The
second fixes purely theoretical problems but seems clean.

I've tested these patches more extensively on a non-mainline tree but
have done basic compile and boot testing on mainline.  I don't have
suspend/resume working so well on mainline yet, so I haven't tested
fully there.

Changes in v2:
- Spread out code (heiko)

Doug Anderson (2):
  pinctrl: rockchip: Handle wakeup pins
  pinctrl: rockchip: Fix enable/disable/mask/unmask

 drivers/pinctrl/pinctrl-rockchip.c | 57 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 54 insertions(+), 3 deletions(-)

-- 
2.1.0.rc2.206.gedb03e5

[PATCH v2 1/2] pinctrl: rockchip: Handle wakeup pins

[Doug Anderson]

The rockchip pinctrl driver was using irq_gc_set_wake() as its
implementation of irq_set_wake() but was totally ignoring everything
that irq_gc_set_wake() did (which is to upkeep gc->wake_active).

Let's fix that by setting gc->wake_active as GPIO_INTEN at suspend
time and restoring GPIO_INTEN at resume time.

NOTE a few quirks when thinking about this patch:
- Rockchip pinctrl hardware supports both "disable/enable" and
  "mask/unmask".  Right now we only use "disable/enable" and present
  those to Linux as "mask/unmask".  This should be OK because
  enable/disable is optional and Linux will implement it in terms of
  mask/unmask.  At the moment we always tell hardware all interrupts
  are unmasked (the boot default).
- At suspend time Linux tries to call "disable" on all interrupts and
  also enables wakeup on all wakeup interrupts.  One would think that
  since "disable" is implemented as "mask" when "disable" isn't
  provided and that since we were ignoring gc->wake_active that
  nothing would have woken us up.  That's not the case since Linux
  "optimizes" things and just leaves interrutps unmasked, assuming it
  could mask them later when they go off.  That meant that at suspend
  time all interrupts were actually being left enabled.

With this patch random non-wakeup interrupts no longer wake the system
up.  Wakeup interrupts still wake the system up.

Signed-off-by: Doug Anderson <dianders@chromium.org>
Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Reviewed-by: Heiko Stuebner <heiko@sntech.de>
---
Changes in v2: None

 drivers/pinctrl/pinctrl-rockchip.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index ba74f0a..e91e845 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -89,6 +89,7 @@ struct rockchip_iomux {
  * @reg_pull: optional separate register for additional pull settings
  * @clk: clock of the gpio bank
  * @irq: interrupt of the gpio bank
+ * @saved_enables: Saved content of GPIO_INTEN at suspend time.
  * @pin_base: first pin number
  * @nr_pins: number of pins in this bank
  * @name: name of the bank
@@ -107,6 +108,7 @@ struct rockchip_pin_bank {
 	struct regmap			*regmap_pull;
 	struct clk			*clk;
 	int				irq;
+	u32				saved_enables;
 	u32				pin_base;
 	u8				nr_pins;
 	char				*name;
@@ -1543,6 +1545,23 @@ static int rockchip_irq_set_type(struct irq_data *d, unsigned int type)
 	return 0;
 }
 
+static void rockchip_irq_suspend(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct rockchip_pin_bank *bank = gc->private;
+
+	bank->saved_enables = irq_reg_readl(gc, GPIO_INTEN);
+	irq_reg_writel(gc, gc->wake_active, GPIO_INTEN);
+}
+
+static void rockchip_irq_resume(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct rockchip_pin_bank *bank = gc->private;
+
+	irq_reg_writel(gc, bank->saved_enables, GPIO_INTEN);
+}
+
 static int rockchip_interrupts_register(struct platform_device *pdev,
 						struct rockchip_pinctrl *info)
 {
@@ -1587,6 +1606,8 @@ static int rockchip_interrupts_register(struct platform_device *pdev,
 		gc->chip_types[0].chip.irq_mask = irq_gc_mask_clr_bit;
 		gc->chip_types[0].chip.irq_unmask = irq_gc_mask_set_bit;
 		gc->chip_types[0].chip.irq_set_wake = irq_gc_set_wake;
+		gc->chip_types[0].chip.irq_suspend = rockchip_irq_suspend;
+		gc->chip_types[0].chip.irq_resume = rockchip_irq_resume;
 		gc->chip_types[0].chip.irq_set_type = rockchip_irq_set_type;
 		gc->wake_enabled = IRQ_MSK(bank->nr_pins);
 
-- 
2.1.0.rc2.206.gedb03e5

[PATCH v2 2/2] pinctrl: rockchip: Fix enable/disable/mask/unmask

[Doug Anderson]

The Rockchip pinctrl driver was only implementing the "mask" and
"unmask" operations though the hardware actually has two distinct
things: enable/disable and mask/unmask.  It was implementing the
"mask" operations as a hardware enable/disable and always leaving all
interrupts unmasked.

I believe that the old system had some downsides, specifically:
- (Untested) if an interrupt went off while interrupts were "masked"
  it would be lost.  Now it will be kept track of.
- If someone wanted to change an interrupt back into a GPIO (is such a
  thing sensible?) by calling irq_disable() it wouldn't actually take
  effect.  That's because Linux does some extra optimizations when
  there's no true "disable" function: it does a lazy mask.

Let's actually implement enable/disable/mask/unmask properly.

Signed-off-by: Doug Anderson <dianders@chromium.org>
Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Reviewed-by: Heiko Stuebner <heiko@sntech.de>
---
Changes in v2:
- Spread out code (heiko)

 drivers/pinctrl/pinctrl-rockchip.c | 36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index e91e845..3c22dbe 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -1562,6 +1562,34 @@ static void rockchip_irq_resume(struct irq_data *d)
 	irq_reg_writel(gc, bank->saved_enables, GPIO_INTEN);
 }
 
+static void rockchip_irq_disable(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	u32 val;
+
+	irq_gc_lock(gc);
+
+	val = irq_reg_readl(gc, GPIO_INTEN);
+	val &= ~d->mask;
+	irq_reg_writel(gc, val, GPIO_INTEN);
+
+	irq_gc_unlock(gc);
+}
+
+static void rockchip_irq_enable(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	u32 val;
+
+	irq_gc_lock(gc);
+
+	val = irq_reg_readl(gc, GPIO_INTEN);
+	val |= d->mask;
+	irq_reg_writel(gc, val, GPIO_INTEN);
+
+	irq_gc_unlock(gc);
+}
+
 static int rockchip_interrupts_register(struct platform_device *pdev,
 						struct rockchip_pinctrl *info)
 {
@@ -1600,11 +1628,13 @@ static int rockchip_interrupts_register(struct platform_device *pdev,
 		gc = irq_get_domain_generic_chip(bank->domain, 0);
 		gc->reg_base = bank->reg_base;
 		gc->private = bank;
-		gc->chip_types[0].regs.mask = GPIO_INTEN;
+		gc->chip_types[0].regs.mask = GPIO_INTMASK;
 		gc->chip_types[0].regs.ack = GPIO_PORTS_EOI;
 		gc->chip_types[0].chip.irq_ack = irq_gc_ack_set_bit;
-		gc->chip_types[0].chip.irq_mask = irq_gc_mask_clr_bit;
-		gc->chip_types[0].chip.irq_unmask = irq_gc_mask_set_bit;
+		gc->chip_types[0].chip.irq_mask = irq_gc_mask_set_bit;
+		gc->chip_types[0].chip.irq_unmask = irq_gc_mask_clr_bit;
+		gc->chip_types[0].chip.irq_enable = rockchip_irq_enable;
+		gc->chip_types[0].chip.irq_disable = rockchip_irq_disable;
 		gc->chip_types[0].chip.irq_set_wake = irq_gc_set_wake;
 		gc->chip_types[0].chip.irq_suspend = rockchip_irq_suspend;
 		gc->chip_types[0].chip.irq_resume = rockchip_irq_resume;
-- 
2.1.0.rc2.206.gedb03e5

Re: [PATCH v2 0/2] Pinctrl fixes for rockchip

[Heiko Stübner]

Linus,

Am Mittwoch, 19. November 2014, 14:51:31 schrieb Doug Anderson:
> These two patches fix some pinctrl issues on rockchip.  The first
> fixes a real issue where interrupts were being left on in
> suspend/resume and waking the system up when they shouldn't.  The
> second fixes purely theoretical problems but seems clean.
> 
> I've tested these patches more extensively on a non-mainline tree but
> have done basic compile and boot testing on mainline.  I don't have
> suspend/resume working so well on mainline yet, so I haven't tested
> fully there.

As indicated by my review-tag in the patches, these look good to me.
Do you want to take them normally [they should simply go on top of the other 
patches] or do you prefer another pull request - which feels strange for only 
two patches :-) .


Thanks
Heiko

Re: [PATCH v2 1/2] pinctrl: rockchip: Handle wakeup pins

[Linus Walleij]

On Wed, Nov 19, 2014 at 11:51 PM, Doug Anderson <dianders@chromium.org> wrote:

> The rockchip pinctrl driver was using irq_gc_set_wake() as its
> implementation of irq_set_wake() but was totally ignoring everything
> that irq_gc_set_wake() did (which is to upkeep gc->wake_active).
>
> Let's fix that by setting gc->wake_active as GPIO_INTEN at suspend
> time and restoring GPIO_INTEN at resume time.
>
> NOTE a few quirks when thinking about this patch:
> - Rockchip pinctrl hardware supports both "disable/enable" and
>   "mask/unmask".  Right now we only use "disable/enable" and present
>   those to Linux as "mask/unmask".  This should be OK because
>   enable/disable is optional and Linux will implement it in terms of
>   mask/unmask.  At the moment we always tell hardware all interrupts
>   are unmasked (the boot default).
> - At suspend time Linux tries to call "disable" on all interrupts and
>   also enables wakeup on all wakeup interrupts.  One would think that
>   since "disable" is implemented as "mask" when "disable" isn't
>   provided and that since we were ignoring gc->wake_active that
>   nothing would have woken us up.  That's not the case since Linux
>   "optimizes" things and just leaves interrutps unmasked, assuming it
>   could mask them later when they go off.  That meant that at suspend
>   time all interrupts were actually being left enabled.
>
> With this patch random non-wakeup interrupts no longer wake the system
> up.  Wakeup interrupts still wake the system up.
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> ---
> Changes in v2: None

Patch applied.

Yours,
Linus Walleij

Re: [PATCH v2 2/2] pinctrl: rockchip: Fix enable/disable/mask/unmask

[Linus Walleij]

On Wed, Nov 19, 2014 at 11:51 PM, Doug Anderson <dianders@chromium.org> wrote:

> The Rockchip pinctrl driver was only implementing the "mask" and
> "unmask" operations though the hardware actually has two distinct
> things: enable/disable and mask/unmask.  It was implementing the
> "mask" operations as a hardware enable/disable and always leaving all
> interrupts unmasked.
>
> I believe that the old system had some downsides, specifically:
> - (Untested) if an interrupt went off while interrupts were "masked"
>   it would be lost.  Now it will be kept track of.
> - If someone wanted to change an interrupt back into a GPIO (is such a
>   thing sensible?) by calling irq_disable() it wouldn't actually take
>   effect.  That's because Linux does some extra optimizations when
>   there's no true "disable" function: it does a lazy mask.
>
> Let's actually implement enable/disable/mask/unmask properly.
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> ---
> Changes in v2:
> - Spread out code (heiko)

Patch applied.

Yours,
Linus Walleij

Re: [PATCH v2 0/2] Pinctrl fixes for rockchip

[Linus Walleij]

On Wed, Nov 26, 2014 at 6:56 PM, Heiko Stübner <heiko@sntech.de> wrote:

> As indicated by my review-tag in the patches, these look good to me.
> Do you want to take them normally [they should simply go on top of the other
> patches] or do you prefer another pull request - which feels strange for only
> two patches :-) .

NP I've applied them now, just lagging behind on mail.

Yours,
Linus Walleij