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

From: Heiko Stübner
Date: Wed Nov 19 2014 - 13:33:09 EST


Am Dienstag, 18. November 2014, 15:49:55 schrieb 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@xxxxxxxxxxxx>

Reviewed-by: Heiko Stuebner <heiko@xxxxxxxxx>


> ---
> 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);

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