Tarun Kanti DebBarma <tarun.kanti@xxxxxx> writes:
> Currently debounce clock state is not tracked in the system.
??
bank->dbck_enable_mask ?
> The bank->dbck
> is enabled/disabled in suspend/idle paths irrespective of whether debounce
> interval has been set or not.
No. It's conditional based on bank->dbck_enable_mask, which is based on
whether or not debounce has been enabled.
> Ideally, it should be handled only for those
> gpio banks where the debounce is enabled.
AFAICT, it is. Please explain more what is actually happening in the
patch, and why.
> In _set_gpio_debounce, enable debounce clock before accessing
> registers.
This is a separate issue/bug and wants its own patch with descriptive
changelog.
> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@xxxxxx>
> ---
> During further internal testing it was found that image was crashing within
> _set_gpio_debounce(). It is observed that we are trying to access registers
> without enabling debounce clock. This patch incorporates the change whereby
> the debounce clock is enabled before accessing registers and disabled at the
> end of the function.
>
> drivers/gpio/gpio-omap.c | 60 ++++++++++++++++++++++++++++++++-------------
> 1 files changed, 42 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index fa6c9c5..85e9c2a 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -65,6 +65,7 @@ struct gpio_bank {
> struct clk *dbck;
> u32 mod_usage;
> u32 dbck_enable_mask;
> + bool dbck_enabled;
> struct device *dev;
> bool is_mpuio;
> bool dbck_flag;
> @@ -156,6 +157,22 @@ static inline void _gpio_rmw(void __iomem *base, u32 reg, u32 mask, bool set)
> __raw_writel(l, base + reg);
> }
>
> +static inline void _gpio_dbck_enable(struct gpio_bank *bank)
> +{
> + if (bank->dbck_enable_mask && !bank->dbck_enabled) {
> + clk_enable(bank->dbck);
> + bank->dbck_enabled = true;
> + }
> +}
> +
> +static inline void _gpio_dbck_disable(struct gpio_bank *bank)
> +{
> + if (bank->dbck_enable_mask && bank->dbck_enabled) {
> + clk_disable(bank->dbck);
> + bank->dbck_enabled = false;
> + }
> +}
> +
> /**
> * _set_gpio_debounce - low level gpio debounce time
> * @bank: the gpio bank we're acting upon
> @@ -184,22 +201,22 @@ static void _set_gpio_debounce(struct gpio_bank *bank, unsigned gpio,
>
> l = GPIO_BIT(bank, gpio);
>
> + clk_enable(bank->dbck);
> reg = bank->base + bank->regs->debounce;
> __raw_writel(debounce, reg);
>
> reg = bank->base + bank->regs->debounce_en;
> val = __raw_readl(reg);
>
> - if (debounce) {
> + if (debounce)
> val |= l;
> - clk_enable(bank->dbck);
> - } else {
> + else
> val &= ~l;
> - clk_disable(bank->dbck);
> - }
> +
> bank->dbck_enable_mask = val;
>
> __raw_writel(val, reg);
> + clk_disable(bank->dbck);
> }
>
> static inline void set_gpio_trigger(struct gpio_bank *bank, int gpio,
> @@ -485,8 +502,10 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
> * If this is the first gpio_request for the bank,
> * enable the bank module.
> */
> - if (!bank->mod_usage)
> + if (!bank->mod_usage) {
> + _gpio_dbck_enable(bank);
> pm_runtime_get_sync(bank->dev);
> + }
>
> spin_lock_irqsave(&bank->lock, flags);
> /* Set trigger to none. You need to enable the desired trigger with
> @@ -549,8 +568,10 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
> * If this is the last gpio to be freed in the bank,
> * disable the bank module.
> */
> - if (!bank->mod_usage)
> + if (!bank->mod_usage) {
> pm_runtime_put_sync(bank->dev);
> + _gpio_dbck_disable(bank);
Why not add this to the ->runtime_suspend() callback?
> + }
> }
>
> /*
> @@ -829,8 +850,10 @@ static int gpio_debounce(struct gpio_chip *chip, unsigned offset,
>
> if (!bank->dbck) {
> bank->dbck = clk_get(bank->dev, "dbclk");
> - if (IS_ERR(bank->dbck))
> + if (IS_ERR(bank->dbck)) {
> dev_err(bank->dev, "Could not get gpio dbck\n");
> + return -EINVAL;
> + }
> }
>
> spin_lock_irqsave(&bank->lock, flags);
> @@ -1086,6 +1109,8 @@ static int omap_gpio_suspend(struct device *dev)
> bank->saved_wakeup = __raw_readl(wake_status);
> _gpio_rmw(base, bank->regs->wkup_en, bank->suspend_wakeup, 1);
> spin_unlock_irqrestore(&bank->lock, flags);
> +
> + _gpio_dbck_disable(bank);
If this call was in the ->runtime_suspend() callback, you wouldn't need
it here.
> }
>
> return 0;
> @@ -1102,6 +1127,8 @@ static int omap_gpio_resume(struct device *dev)
> if (!bank->regs->wkup_en)
> return 0;
>
> + _gpio_dbck_enable(bank);
Similarily, this call should be in the ->runtime_resume() callback and
it wouldn't be needed here.
Using the runtime PM callbacks, all the _gpio_dbck_* calls below would
not be needed.
> spin_lock_irqsave(&bank->lock, flags);
> _gpio_rmw(base, bank->regs->wkup_en, bank->saved_wakeup, 1);
> spin_unlock_irqrestore(&bank->lock, flags);
> @@ -1120,16 +1147,14 @@ void omap2_gpio_prepare_for_idle(int off_mode)
>
> list_for_each_entry(bank, &omap_gpio_list, node) {
> u32 l1 = 0, l2 = 0;
> - int j;
>
> if (!bank->loses_context)
> continue;
>
> - for (j = 0; j < hweight_long(bank->dbck_enable_mask); j++)
> - clk_disable(bank->dbck);
> -
> - if (!off_mode)
> + if (!off_mode) {
> + _gpio_dbck_disable(bank);
> continue;
> + }
>
> /* If going to OFF, remove triggering for all
> * non-wakeup GPIOs. Otherwise spurious IRQs will be
> @@ -1151,15 +1176,16 @@ void omap2_gpio_prepare_for_idle(int off_mode)
> __raw_writel(l2, bank->base + bank->regs->risingdetect);
>
> save_gpio_context:
> -
> if (bank->get_context_loss_count)
> bank->context_loss_count =
> bank->get_context_loss_count(bank->dev);
>
> omap_gpio_save_context(bank);
>
> - if (!pm_runtime_suspended(bank->dev))
> + if (!pm_runtime_suspended(bank->dev)) {
> pm_runtime_put_sync(bank->dev);
> + _gpio_dbck_disable(bank);
> + }
> }
> }
>
> @@ -1170,13 +1196,11 @@ void omap2_gpio_resume_after_idle(void)
> list_for_each_entry(bank, &omap_gpio_list, node) {
> u32 context_lost_cnt_after;
> u32 l = 0, gen, gen0, gen1;
> - int j;
>
> if (!bank->loses_context)
> continue;
>
> - for (j = 0; j < hweight_long(bank->dbck_enable_mask); j++)
> - clk_enable(bank->dbck);
> + _gpio_dbck_enable(bank);
> if (pm_runtime_suspended(bank->dev))
> pm_runtime_get_sync(bank->dev);
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
