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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Dec 5, 2011 at 7:27 PM, DebBarma, Tarun Kanti
<tarun.kanti@xxxxxx> wrote:
> On Mon, Nov 7, 2011 at 7:19 PM, DebBarma, Tarun Kanti
> <tarun.kanti@xxxxxx> wrote:
>> On Fri, Nov 4, 2011 at 10:10 PM, Kevin Hilman <khilman@xxxxxx> wrote:
>>> Tarun Kanti DebBarma <tarun.kanti@xxxxxx> 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@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?
>> 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?
Sorry for the silly mistake.
I used !off_mode check before calling pm_runtime_put_sync_suspend()
in *_prepare_for_idle(). This resulted in use count mismatch with
*_get_sync() call in *_resume_after_idle() which is called both for
offmode and retention. I have removed the off_mode and made it unused.


void omap2_gpio_prepare_for_idle(int off_mode)
{
 	struct gpio_bank *bank;

 	list_for_each_entry(bank, &omap_gpio_list, node) {
 		if (!bank->mod_usage || !off_mode)
 			continue;

		pm_runtime_put_sync_suspend(bank->dev);
 	}
 }

void omap2_gpio_resume_after_idle(void)
 	struct gpio_bank *bank;

 	list_for_each_entry(bank, &omap_gpio_list, node) {
 		if (!bank->mod_usage)
 			continue;

		pm_runtime_get_sync(bank->dev);
 	}
 }

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


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux