Re: [PATCH 1/3] gpio: omap: Add level wakeup handling for omap4 based SoCs

[Keerthy <j-keerthy@xxxxxx>]

On Friday 21 September 2018 01:05 AM, Tony Lindgren wrote:
> I noticed that unlike omap2 and 3 based SoCs, omap4 based SoCs keep
> the GPIO clocks enabled for GPIO level interrupts with wakeup enabled.
> This blocks deeper idle states as the whole domain will stay busy.
> 
> The GPIO functional clock seems to stay enabled if the wakeup register
> is enabled and a level interrupt is triggered. In that case the only
> way to have the GPIO module idle is to reset it. It is possible this
> has gone unnoticed with OSWR (Open SWitch Retention) and off mode
> during idle resetting GPIO context most GPIO instances in the earlier
> Android trees for example.
> 
> Looks like the way to deal with this is to have omap4 based SoCs
> only set wake for the duration of idle for level interrupts, and clear
> level registers for the idle. With level interrupts we can do this as
> the level interrupt from device will be still there on resume.
> 
> I've taken the long path to fixing this to avoid yet more hard to
> read code. I've set up a quirks flag, and a struct for function
> pointers so we can use these to clean up other quirk handling easier
> in the later patches. The current level quirk handling is moved to
> the new functions.
> 

Tested for Deep Sleep 0 on AM437x-GP-EVM and AM335x-Boneblack for all
the three patches of the series.

Tested-by: Keerthy <j-keerthy@xxxxxx>

> Cc: Aaro Koskinen <aaro.koskinen@xxxxxx>
> Cc: Grygorii Strashko <grygorii.strashko@xxxxxx>
> Cc: Keerthy <j-keerthy@xxxxxx>
> Cc: Ladislav Michl <ladis@xxxxxxxxxxxxxx>
> Cc: Tero Kristo <t-kristo@xxxxxx>
> Signed-off-by: Tony Lindgren <tony@xxxxxxxxxxx>
> ---
>  drivers/gpio/gpio-omap.c                | 151 ++++++++++++++++++------
>  include/linux/platform_data/gpio-omap.h |   2 +
>  2 files changed, 120 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -31,6 +31,8 @@
>  #define OFF_MODE	1
>  #define OMAP4_GPIO_DEBOUNCINGTIME_MASK 0xFF
>  
> +#define OMAP_GPIO_QUIRK_DEFERRED_WKUP_EN	BIT(1)
> +
>  static LIST_HEAD(omap_gpio_list);
>  
>  struct gpio_regs {
> @@ -48,6 +50,13 @@ struct gpio_regs {
>  	u32 debounce_en;
>  };
>  
> +struct gpio_bank;
> +
> +struct gpio_omap_funcs {
> +	void (*idle_enable_level_quirk)(struct gpio_bank *bank);
> +	void (*idle_disable_level_quirk)(struct gpio_bank *bank);
> +};
> +
>  struct gpio_bank {
>  	struct list_head node;
>  	void __iomem *base;
> @@ -55,6 +64,7 @@ struct gpio_bank {
>  	u32 non_wakeup_gpios;
>  	u32 enabled_non_wakeup_gpios;
>  	struct gpio_regs context;
> +	struct gpio_omap_funcs funcs;
>  	u32 saved_datain;
>  	u32 level_mask;
>  	u32 toggle_mask;
> @@ -75,6 +85,7 @@ struct gpio_bank {
>  	int context_loss_count;
>  	int power_mode;
>  	bool workaround_enabled;
> +	u32 quirks;
>  
>  	void (*set_dataout)(struct gpio_bank *bank, unsigned gpio, int enable);
>  	void (*set_dataout_multiple)(struct gpio_bank *bank,
> @@ -368,9 +379,18 @@ static inline void omap_set_gpio_trigger(struct gpio_bank *bank, int gpio,
>  			readl_relaxed(bank->base + bank->regs->fallingdetect);
>  
>  	if (likely(!(bank->non_wakeup_gpios & gpio_bit))) {
> -		omap_gpio_rmw(base, bank->regs->wkup_en, gpio_bit, trigger != 0);
> -		bank->context.wake_en =
> -			readl_relaxed(bank->base + bank->regs->wkup_en);
> +		/* Defer wkup_en register update until we idle? */
> +		if (bank->quirks & OMAP_GPIO_QUIRK_DEFERRED_WKUP_EN) {
> +			if (trigger)
> +				bank->context.wake_en |= gpio_bit;
> +			else
> +				bank->context.wake_en &= ~gpio_bit;
> +		} else {
> +			omap_gpio_rmw(base, bank->regs->wkup_en, gpio_bit,
> +				      trigger != 0);
> +			bank->context.wake_en =
> +				readl_relaxed(bank->base + bank->regs->wkup_en);
> +		}
>  	}
>  
>  	/* This part needs to be executed always for OMAP{34xx, 44xx} */
> @@ -899,6 +919,82 @@ static void omap_gpio_unmask_irq(struct irq_data *d)
>  	raw_spin_unlock_irqrestore(&bank->lock, flags);
>  }
>  
> +/*
> + * Only edges can generate a wakeup event to the PRCM.
> + *
> + * Therefore, ensure any wake-up capable GPIOs have
> + * edge-detection enabled before going idle to ensure a wakeup
> + * to the PRCM is generated on a GPIO transition. (c.f. 34xx
> + * NDA TRM 25.5.3.1)
> + *
> + * The normal values will be restored upon ->runtime_resume()
> + * by writing back the values saved in bank->context.
> + */
> +static void __maybe_unused
> +omap2_gpio_enable_level_quirk(struct gpio_bank *bank)
> +{
> +	u32 wake_low, wake_hi;
> +
> +	/* Enable additional edge detection for level gpios for idle */
> +	wake_low = bank->context.leveldetect0 & bank->context.wake_en;
> +	if (wake_low)
> +		writel_relaxed(wake_low | bank->context.fallingdetect,
> +			       bank->base + bank->regs->fallingdetect);
> +
> +	wake_hi = bank->context.leveldetect1 & bank->context.wake_en;
> +	if (wake_hi)
> +		writel_relaxed(wake_hi | bank->context.risingdetect,
> +			       bank->base + bank->regs->risingdetect);
> +}
> +
> +static void __maybe_unused
> +omap2_gpio_disable_level_quirk(struct gpio_bank *bank)
> +{
> +	/* Disable edge detection for level gpios after idle */
> +	writel_relaxed(bank->context.fallingdetect,
> +		       bank->base + bank->regs->fallingdetect);
> +	writel_relaxed(bank->context.risingdetect,
> +		       bank->base + bank->regs->risingdetect);
> +}
> +
> +/*
> + * On omap4 and later SoC variants a level interrupt with wkup_en
> + * enabled blocks the GPIO functional clock from idling until the GPIO
> + * instance has been reset. To avoid that, we must set wkup_en only for
> + * idle for level interrupts, and clear level registers for the duration
> + * of idle. The level interrupts will be still there on wakeup by their
> + * nature.
> + */
> +static void __maybe_unused
> +omap4_gpio_enable_level_quirk(struct gpio_bank *bank)
> +{
> +	/* Update wake register for idle, edge bits might be already set */
> +	writel_relaxed(bank->context.wake_en,
> +		       bank->base + bank->regs->wkup_en);
> +
> +	/* Clear level registers for idle */
> +	writel_relaxed(0, bank->base + bank->regs->leveldetect0);
> +	writel_relaxed(0, bank->base + bank->regs->leveldetect1);
> +}
> +
> +static void __maybe_unused
> +omap4_gpio_disable_level_quirk(struct gpio_bank *bank)
> +{
> +	/* Restore level registers after idle */
> +	writel_relaxed(bank->context.leveldetect0,
> +		       bank->base + bank->regs->leveldetect0);
> +	writel_relaxed(bank->context.leveldetect1,
> +		       bank->base + bank->regs->leveldetect1);
> +
> +	/* Clear saved wkup_en for level, it will be set for next idle again */
> +	bank->context.wake_en &= ~(bank->context.leveldetect0 |
> +				   bank->context.leveldetect1);
> +
> +	/* Update wake with only edge configuration */
> +	writel_relaxed(bank->context.wake_en,
> +		       bank->base + bank->regs->wkup_en);
> +}
> +
>  /*---------------------------------------------------------------------*/
>  
>  static int omap_mpuio_suspend_noirq(struct device *dev)
> @@ -1270,6 +1366,7 @@ static int omap_gpio_probe(struct platform_device *pdev)
>  	bank->chip.parent = dev;
>  	bank->chip.owner = THIS_MODULE;
>  	bank->dbck_flag = pdata->dbck_flag;
> +	bank->quirks = pdata->quirks;
>  	bank->stride = pdata->bank_stride;
>  	bank->width = pdata->bank_width;
>  	bank->is_mpuio = pdata->is_mpuio;
> @@ -1278,6 +1375,7 @@ static int omap_gpio_probe(struct platform_device *pdev)
>  #ifdef CONFIG_OF_GPIO
>  	bank->chip.of_node = of_node_get(node);
>  #endif
> +
>  	if (node) {
>  		if (!of_property_read_bool(node, "ti,gpio-always-on"))
>  			bank->loses_context = true;
> @@ -1298,6 +1396,18 @@ static int omap_gpio_probe(struct platform_device *pdev)
>  				omap_set_gpio_dataout_mask_multiple;
>  	}
>  
> +	if (bank->quirks & OMAP_GPIO_QUIRK_DEFERRED_WKUP_EN) {
> +		bank->funcs.idle_enable_level_quirk =
> +			omap4_gpio_enable_level_quirk;
> +		bank->funcs.idle_disable_level_quirk =
> +			omap4_gpio_disable_level_quirk;
> +	} else {
> +		bank->funcs.idle_enable_level_quirk =
> +			omap2_gpio_enable_level_quirk;
> +		bank->funcs.idle_disable_level_quirk =
> +			omap2_gpio_disable_level_quirk;
> +	}
> +
>  	raw_spin_lock_init(&bank->lock);
>  	raw_spin_lock_init(&bank->wa_lock);
>  
> @@ -1372,29 +1482,11 @@ static int omap_gpio_runtime_suspend(struct device *dev)
>  	struct gpio_bank *bank = platform_get_drvdata(pdev);
>  	u32 l1 = 0, l2 = 0;
>  	unsigned long flags;
> -	u32 wake_low, wake_hi;
>  
>  	raw_spin_lock_irqsave(&bank->lock, flags);
>  
> -	/*
> -	 * Only edges can generate a wakeup event to the PRCM.
> -	 *
> -	 * Therefore, ensure any wake-up capable GPIOs have
> -	 * edge-detection enabled before going idle to ensure a wakeup
> -	 * to the PRCM is generated on a GPIO transition. (c.f. 34xx
> -	 * NDA TRM 25.5.3.1)
> -	 *
> -	 * The normal values will be restored upon ->runtime_resume()
> -	 * by writing back the values saved in bank->context.
> -	 */
> -	wake_low = bank->context.leveldetect0 & bank->context.wake_en;
> -	if (wake_low)
> -		writel_relaxed(wake_low | bank->context.fallingdetect,
> -			     bank->base + bank->regs->fallingdetect);
> -	wake_hi = bank->context.leveldetect1 & bank->context.wake_en;
> -	if (wake_hi)
> -		writel_relaxed(wake_hi | bank->context.risingdetect,
> -			     bank->base + bank->regs->risingdetect);
> +	if (bank->funcs.idle_enable_level_quirk)
> +		bank->funcs.idle_enable_level_quirk(bank);
>  
>  	if (!bank->enabled_non_wakeup_gpios)
>  		goto update_gpio_context_count;
> @@ -1459,16 +1551,8 @@ static int omap_gpio_runtime_resume(struct device *dev)
>  
>  	omap_gpio_dbck_enable(bank);
>  
> -	/*
> -	 * In ->runtime_suspend(), level-triggered, wakeup-enabled
> -	 * GPIOs were set to edge trigger also in order to be able to
> -	 * generate a PRCM wakeup.  Here we restore the
> -	 * pre-runtime_suspend() values for edge triggering.
> -	 */
> -	writel_relaxed(bank->context.fallingdetect,
> -		     bank->base + bank->regs->fallingdetect);
> -	writel_relaxed(bank->context.risingdetect,
> -		     bank->base + bank->regs->risingdetect);
> +	if (bank->funcs.idle_disable_level_quirk)
> +		bank->funcs.idle_disable_level_quirk(bank);
>  
>  	if (bank->loses_context) {
>  		if (!bank->get_context_loss_count) {
> @@ -1706,6 +1790,7 @@ static const struct omap_gpio_platform_data omap4_pdata = {
>  	.regs = &omap4_gpio_regs,
>  	.bank_width = 32,
>  	.dbck_flag = true,
> +	.quirks = OMAP_GPIO_QUIRK_DEFERRED_WKUP_EN,
>  };
>  
>  static const struct of_device_id omap_gpio_match[] = {
> diff --git a/include/linux/platform_data/gpio-omap.h b/include/linux/platform_data/gpio-omap.h
> --- a/include/linux/platform_data/gpio-omap.h
> +++ b/include/linux/platform_data/gpio-omap.h
> @@ -197,6 +197,8 @@ struct omap_gpio_platform_data {
>  	bool is_mpuio;		/* whether the bank is of type MPUIO */
>  	u32 non_wakeup_gpios;
>  
> +	u32 quirks;		/* Version specific quirks mask */
> +
>  	struct omap_gpio_reg_offs *regs;
>  
>  	/* Return context loss count due to PM states changing */
>