[PATCH v8 REPOST 17/24] gpio/omap: fix debounce clock handling

DebBarma, Tarun Kanti tarun.kanti at ti.com
Mon Dec 5 08:57:09 EST 2011


On Mon, Nov 7, 2011 at 7:19 PM, DebBarma, Tarun Kanti
<tarun.kanti at ti.com> wrote:
> On Fri, Nov 4, 2011 at 10:10 PM, Kevin Hilman <khilman at ti.com> wrote:
>> Tarun Kanti DebBarma <tarun.kanti at ti.com> writes:
>>
>>> Currently debounce clock state is not tracked in the system.
>>
>> ??
>>
>> bank->dbck_enable_mask ?
> As I understand, this only tells which all GPIOs have debounce timeout enabled.
> But, during system operation as debounce clocks are enabled and disabled I need
> additional flag to keep track of current state (enabled/disabled).
> This is what I meant.
>
>>
>>> 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.
> You are right. I need to rephrase my description.
>
>>
>>> 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.
> Yes, as I explained above, it is more about the tracking the debounce clock
> enabled/disabled state for those GPIOs whose debounce timeouts are enabled.
> I will modify the patch description.
>
>>
>>> In _set_gpio_debounce, enable debounce clock before accessing
>>> registers.
>>
>> This is a separate issue/bug and wants its own patch with descriptive
>> changelog.
> OK.
>
>>
>>> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti at ti.com>
>>> ---
>>> 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?
> Yes, I can move there and test.
>
>>
>>> +     }
>>>  }
>>>
>>>  /*
>>> @@ -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.
> Yes.
>
>>
>>>       }
>>>
>>>       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.
> Right.
>
>>
>> Using the runtime PM callbacks, all the _gpio_dbck_* calls below would
>> not be needed.
> Yes.
I removed the  _gpio_dbck_disable/enable calls are removed from
*_prepare_for_idle()
and *_resume_after_idle(). But I am not seeing runtime callbacks being
called and
hence the  _gpio_dbck_* calls are not getting called.

I have also tried making explicit pm_runtime_put() and pm_runtime_get_sync() in
*_prepare_for_idle() and *_resume_after_idle(). Still, I do not see
the runtime callbacks
getting called.

Is there anything that I could be missing?
--
Tarun
>>
>>>               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
>>



More information about the linux-arm-kernel mailing list