On 09.04.19 07:36, Keerthy wrote:
>
>
> On 09/04/19 1:15 AM, Tony Lindgren wrote:
>> From: Russell King <rmk+kernel@xxxxxxxxxxxxxxx>
>>
>> Commit b764a5863fd8 ("gpio: omap: Remove custom PM calls and use cpu_pm
>> instead") moved interrupt using GPIO banks to idle with cpu_pm in order
>> to drop the use of pm_runtime_irq_safe() in a later patch. The GPIO
>> banks with no interrupts claimed are still being idled based on PM
>> runtime calls. However this caused a regression for am437x suspend for
>> rtc+ddr idle mode as reported by Keerthy <j-keerthy@xxxxxx>. The fix
>> to this is:
>>
>> Bump the pm_runtime usage count while interrupts are in use, rather
>> than failing the pm_runtime callbacks. The logic here is a little
>> non-obvious - the calling order will be:
>>
>> omap_gpio_irq_bus_lock()
>> (optionally) raw_spin_lock_irqsave(desc->lock)
>> omap_gpio_irq_startup()
>> (optionally) raw_spin_unlock_irqrestore(desc->lock)
>> gpio_irq_bus_sync_unlock()
>>
>> As the irq_startup method may be called with interrupts disabled, we
>> must not use pm_runtime_get() here, so as the bus lock takes an initial
>> pm reference count us, use pm_runtime_get_if_in_use() which will merely
>> increment the use count.
>
> Tony,
>
> Applied cleanly on linux-next. I had to manually apply this on your for-next branch. Tested for AM4 RTC+DDR and DS0 back and forth.
I'm very sorry, but what was the regression exactly?
Can't enter RTC+DDR state? some crash?
>
> Regards,
> Keerthy
>>
>> Cc: Aaro Koskinen <aaro.koskinen@xxxxxx>
>> Cc: Grygorii Strashko <grygorii.strashko@xxxxxx>
>> Cc: Keerthy <j-keerthy@xxxxxx>
>> Cc: Peter Ujfalusi <peter.ujfalusi@xxxxxx>
>> Cc: Tero Kristo <t-kristo@xxxxxx>
>> Fixes: b764a5863fd8 ("gpio: omap: Remove custom PM calls and use cpu_pm instead")
>> Reported-by: Keerthy <j-keerthy@xxxxxx>
>> Signed-off-by: Russell King <rmk+kernel@xxxxxxxxxxxxxxx>
>> [tony@xxxxxxxxxxx: updated patch description for regression fix]
>> Signed-off-by: Tony Lindgren <tony@xxxxxxxxxxx>
>> ---
>>
>> AFAIK this is only needed with patches heading to v5.2, so this is based on
>> Linux next. We can always backport as needed.
>>
>> ---
>> drivers/gpio/gpio-omap.c | 23 +++++++----------------
>> 1 file changed, 7 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> --- a/drivers/gpio/gpio-omap.c
>> +++ b/drivers/gpio/gpio-omap.c
>> @@ -811,6 +811,9 @@ static unsigned int omap_gpio_irq_startup(struct irq_data *d)
>> unsigned long flags;
>> unsigned offset = d->hwirq;
>> + /* Take a reference on the runtime PM to prevent RPM suspends */
>> + WARN_ON(pm_runtime_get_if_in_use(bank->chip.parent) == 0);
>> +
We have in driver:
irqc->parent_device = dev;
which means:
request_threaded_irq()
irq_chip_pm_get()
if (IS_ENABLED(CONFIG_PM) && data->chip->parent_device) {
retval = pm_runtime_get_sync(data->chip->parent_device);
and power.usage_count will be incremented every time GPIO irq is requested.
only in free_irq() (or in case of error) power.usage_count is decremented.
Now above change will introduce just another incrementation of power.usage_count
How is it helping?
>> raw_spin_lock_irqsave(&bank->lock, flags);
>> if (!LINE_USED(bank->mod_usage, offset))
>> @@ -844,6 +847,8 @@ static void omap_gpio_irq_shutdown(struct irq_data *d)
>> omap_clear_gpio_debounce(bank, offset);
>> omap_disable_gpio_module(bank, offset);
>> raw_spin_unlock_irqrestore(&bank->lock, flags);
>> +
>> + pm_runtime_put(bank->chip.parent);
>> }
>> static void omap_gpio_irq_bus_lock(struct irq_data *data)
>> @@ -1719,40 +1724,26 @@ static int __maybe_unused omap_gpio_runtime_suspend(struct device *dev)
>> {
>> struct gpio_bank *bank = dev_get_drvdata(dev);
>> unsigned long flags;
>> - int error = 0;
>> raw_spin_lock_irqsave(&bank->lock, flags);
>> - /* Must be idled only by CPU_CLUSTER_PM_ENTER? */
>> - if (bank->irq_usage) {
>> - error = -EBUSY;
>> - goto unlock;
>> - }
>> omap_gpio_idle(bank, true);
>> bank->is_suspended = true;
>> -unlock:
>> raw_spin_unlock_irqrestore(&bank->lock, flags);
>> - return error;
>> + return 0;
>> }
>> static int __maybe_unused omap_gpio_runtime_resume(struct device *dev)
>> {
>> struct gpio_bank *bank = dev_get_drvdata(dev);
>> unsigned long flags;
>> - int error = 0;
>> raw_spin_lock_irqsave(&bank->lock, flags);
>> - /* Must be unidled only by CPU_CLUSTER_PM_ENTER? */
>> - if (bank->irq_usage) {
>> - error = -EBUSY;
>> - goto unlock;
>> - }
>> omap_gpio_unidle(bank);
>> bank->is_suspended = false;
>> -unlock:
>> raw_spin_unlock_irqrestore(&bank->lock, flags);
>> - return error;
>> + return 0;
>> }
>> static const struct dev_pm_ops gpio_pm_ops = {
>>
--
Best regards,
grygorii
