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
