[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