Hi Kevin,
Thanks for the detailed review.
On Sat, Mar 5, 2011 at 03:29, Kevin Hilman <khilman@xxxxxx> wrote:
> Charulatha V <charu@xxxxxx> writes:
>
>> Call runtime pm APIs pm_runtime_put_sync() and pm_runtime_get()
>
> Minor: I think you mean _get_sync() and _put()
Yes, thanks for catching it. It was a typo :-(
>
>> for enabling/disabling the clocks, sysconfig settings instead of using
>> clock FW APIs.
>> Note: OMAP16xx & OMAP2 has interface and functional clocks whereas
>> OMAP3&4 has interface and debounce clocks.
>>
>> GPIO driver is modified to use dev_pm_ops instead of sysdev_class.
>> With this approach, gpio_bank_suspend() & gpio_bank_resume()
>> are not part of sys_dev_class.
>>
>> Usage of PM runtime get/put APIs in GPIO driver is as given below:
>> pm_runtime_get_sync():
>> * In the probe before accessing the GPIO registers
>> * at the beginning of omap_gpio_request()
>> -only when one of the gpios is requested on a bank, in which,
>> no other gpio is being used (when mod_usage becomes non-zero).
>
> When using runtime PM, bank->mod_usage acutally becomes redundant with
> usage counting already done at the runtime PM level. IOW, checking
> bank->mod_usage is he equivalent of checking pm_runtime_suspended(), so
> I think you can totally get rid of bank->mod_usage.
I wish to differ here. bank->mod_usage is filled for each GPIO pin in a bank.
Hence during request/free if pm_runtime_get_sync() is called for each GPIO
pin, then the count gets increased for each GPIO pin in a bank. But
gpio_prepare_for_idle/suspend calls pm_runtime_put() only once for
each bank. Hence there will be a count mismatch and hence this will lead
to problems and never a bank will get suspended.
IMO it is required to have bank->mod_usage checks. Please correct
me if I am wrong.
>
> pm_runtime_get* on an already active device is harmless and will just
> increment the runtime PM internal use counting. It does however have
> the additional benefit of taking advantage of the runtime PM statistics
> so using tools like powertop, we will be able to see stats for *all*
> GPIO users, not just the first (and last) ones to use a given bank.
> IMO, This is a big win for PM debug.
>
> More on the implementation of this below...
>
>> * at the beginning of gpio_resume_after_idle()
>> - only if the GPIO bank is under use
>> (and)
>> - if the bank is in non-wkup power domain
>> * at the beginning of gpio_irq_handler()
>> - only if the specific GPIO bank is pm_runtime_suspended()
>> * at the beginning of omap_gpio_resume()
>> - only if the GPIO bank is under use
>>
>> pm_runtime_put():
>> * In the probe after completing GPIO register access
>> * at the end of omap_gpio_free()
>> - only when the last used gpio in the gpio bank is
>> freed (when mod_usage becomes 0).
>> * at the end of gpio_prepare_for_idle()
>> - only if the GPIO bank is under use
>> (and)
>> - if the bank is in non-wkup power domain
>> * at the end of gpio_irq_handler()
>> - only if a corresponding pm_runtime_get_sync() was done
>> in gpio_irq_handler()
>> * at the end of omap_gpio_suspend()
>> - only if the GPIO bank is under use
>>
>> OMAP GPIO Request/ Free:
>> *During a gpio_request when mod_usage becomes non-zero, the bank
>> registers are configured with init time configurations inorder to
>> make sure that the GPIO init time configurations are not lost if
>> the context was earlier lost when the GPIO bank was not in use.
>>
>> TODO:
>> Cleanup GPIO driver to avoid usage of gpio_bank_count &
>> cpu_is_* checks
>>
>> Signed-off-by: Charulatha V <charu@xxxxxx>
>> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@xxxxxx>
>> ---
>> arch/arm/plat-omap/gpio.c | 305 +++++++++++++++++++++++++--------------------
>> 1 files changed, 167 insertions(+), 138 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/gpio.c b/arch/arm/plat-omap/gpio.c
>> index 10792b6..908bad2 100644
>> --- a/arch/arm/plat-omap/gpio.c
>> +++ b/arch/arm/plat-omap/gpio.c
>> @@ -177,6 +177,7 @@ struct gpio_bank {
>>
>> static void omap_gpio_save_context(struct gpio_bank *bank);
>> static void omap_gpio_restore_context(struct gpio_bank *bank);
>> +static void omap_gpio_mod_init(struct gpio_bank *bank, int id);
>>
>> /*
>> * TODO: Cleanup gpio_bank usage as it is having information
>> @@ -1042,8 +1043,28 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
>> struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
>> unsigned long flags;
>>
>> + /*
>> + * If this is the first gpio_request for the bank,
>> + * enable the bank module
>> + */
>> + if (!bank->mod_usage) {
>> + struct platform_device *pdev = to_platform_device(bank->dev);
>> +
>> + if (unlikely(pm_runtime_get_sync(bank->dev) < 0)) {
>> + dev_err(bank->dev, "%s: GPIO bank %d "
>> + "pm_runtime_get_sync failed\n",
>> + __func__, pdev->id);
>> + return -EINVAL;
>> + }
>> +
>> + /* Initialize the gpio bank registers to init time value */
>> + omap_gpio_mod_init(bank, pdev->id);
>> + }
>> +
>
> This could all be replaced by:
> if (!pm_runtime_get_sync(bank->dev))
> omap_gpio_mod_init(bank, pdev->id);
>
> since the first 'get' that actually resumes the device will return zero,
> all the others will return 1.
>
> Actually, even better (and my prefernce) would be to just do the
> _get_sync() for every request as above, but put the omap_gpio_mod_init()
> in the ->runtime_resume() hook so it gets called whenever the first GPIO
> in the bank is activated.
The problem is only about the count mismatch I mentioned above.
>
>
>> spin_lock_irqsave(&bank->lock, flags);
>>
>> + bank->mod_usage |= 1 << offset;
>> +
>
>> /* Set trigger to none. You need to enable the desired trigger with
>> * request_irq() or set_irq_type().
>> */
>> @@ -1058,22 +1079,7 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
>> __raw_writel(__raw_readl(reg) | (1 << offset), reg);
>> }
>> #endif
>> - if (!cpu_class_is_omap1()) {
>> - if (!bank->mod_usage) {
>> - void __iomem *reg = bank->base;
>> - u32 ctrl;
>> -
>> - if (cpu_is_omap24xx() || cpu_is_omap34xx())
>> - reg += OMAP24XX_GPIO_CTRL;
>> - else if (cpu_is_omap44xx())
>> - reg += OMAP4_GPIO_CTRL;
>> - ctrl = __raw_readl(reg);
>> - /* Module is enabled, clocks are not gated */
>> - ctrl &= 0xFFFFFFFE;
>> - __raw_writel(ctrl, reg);
>> - }
>> - bank->mod_usage |= 1 << offset;
>> - }
>
> Where did this code go? I expected it to be moved, but not removed completely.
It is only moved and not removed.
bank->mod_usage |= 1 << offset; is done above and the rest is done below.
> This code also belongs in the ->runtime_resume() method so it happens
> when the first GPIO in a bank is activated.
Okay.
>
>> +
>> spin_unlock_irqrestore(&bank->lock, flags);
>>
>> return 0;
>> @@ -1106,24 +1112,39 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
>> __raw_writel(1 << offset, reg);
>> }
>> #endif
>> - if (!cpu_class_is_omap1()) {
>> - bank->mod_usage &= ~(1 << offset);
>> - if (!bank->mod_usage) {
>> - void __iomem *reg = bank->base;
>> - u32 ctrl;
>> -
>> - if (cpu_is_omap24xx() || cpu_is_omap34xx())
>> - reg += OMAP24XX_GPIO_CTRL;
>> - else if (cpu_is_omap44xx())
>> - reg += OMAP4_GPIO_CTRL;
>> - ctrl = __raw_readl(reg);
>> - /* Module is disabled, clocks are gated */
>> - ctrl |= 1;
>> - __raw_writel(ctrl, reg);
>> - }
>> + bank->mod_usage &= ~(1 << offset);
>> + if (!bank->mod_usage) {
>> + void __iomem *reg = bank->base;
>> + u32 ctrl;
>> +
>> + if (bank->method == METHOD_GPIO_24XX)
>> + reg += OMAP24XX_GPIO_CTRL;
>> + else if (bank->method == METHOD_GPIO_44XX)
>> + reg += OMAP4_GPIO_CTRL;
>> + else
>> + goto reset_gpio;
>> +
>> + ctrl = __raw_readl(reg);
>> + /* Module is disabled, clocks are gated */
>> + ctrl |= 1;
>> + __raw_writel(ctrl, reg);
>
> And here, rather than updating bank->mod_usage, just move this code
> into a ->runtime_suspend hook which will then be called whenever the
> bank is actually suspended.
Pls see above explanation for bank->mod_usage.
>
>> }
>> +reset_gpio:
>> _reset_gpio(bank, bank->chip.base + offset);
>> spin_unlock_irqrestore(&bank->lock, flags);
>> +
>> + /*
>> + * If this is the last gpio to be freed in the bank,
>> + * disable the bank module
>> + */
>> + if (!bank->mod_usage) {
>> + if (unlikely(pm_runtime_put(bank->dev) < 0)) {
>> + struct platform_device *pdev =
>> + to_platform_device(bank->dev);
>> + dev_err(bank->dev, "%s: GPIO bank %d pm_runtime_put "
>> + "failed\n", __func__, pdev->id);
>> + }
>> + }
>
> and this can just be a pm_runtime_put(bank->dev)
ditto.
>
>> }
>>
>> /*
>> @@ -1143,10 +1164,17 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
>> struct gpio_bank *bank;
>> u32 retrigger = 0;
>> int unmasked = 0;
>> + int enable_gpio = 0;
>>
>> desc->irq_data.chip->irq_ack(&desc->irq_data);
>>
>> bank = get_irq_data(irq);
>> +
>> + if (pm_runtime_suspended(bank->dev)) {
>
> Why do you need the check here?
>
> If the device is already suspended, and you call _get_sync(), it
> just increments the usecount, and returns.
Yes, you are right. I will remove pm_runtime_suspended() checks
and also the enable_gpio flag.
>
>> + if (unlikely(pm_runtime_get_sync(bank->dev) == 0))
>> + enable_gpio = 1;
>> + }
>> +
>> #ifdef CONFIG_ARCH_OMAP1
>> if (bank->method == METHOD_MPUIO)
>> isr_reg = bank->base +
>> @@ -1238,6 +1266,9 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
>> exit:
>> if (!unmasked)
>> desc->irq_data.chip->irq_unmask(&desc->irq_data);
>> +
>> + if (enable_gpio)
>> + pm_runtime_put(bank->dev);
>
> Likewise, I think you could just always do a 'put' here without having
> to have a flag.
Sure, will change this.
>
>> }
>>
>> static void gpio_irq_shutdown(struct irq_data *d)
>> @@ -1742,126 +1773,121 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
>> }
>>
>> pm_runtime_enable(bank->dev);
>> - pm_runtime_get_sync(bank->dev);
>> + pm_runtime_irq_safe(bank->dev);
>> +
>> + if (unlikely(pm_runtime_get_sync(bank->dev) < 0)) {
>> + dev_err(bank->dev, "%s: GPIO bank %d pm_runtime_get_sync "
>> + "failed\n", __func__, id);
>> + iounmap(bank->base);
>> + return -EINVAL;
>> + }
>>
>> - omap_gpio_mod_init(bank, id);
>> omap_gpio_chip_init(bank);
>> omap_gpio_show_rev(bank);
>>
>> + if (unlikely(pm_runtime_put(bank->dev) < 0)) {
>
> use IS_ERR_VALUE() instead of '< 0'
Okay.
>
>> + dev_err(bank->dev, "%s: GPIO bank %d pm_runtime_put "
>> + "failed\n", __func__, id);
>> + iounmap(bank->base);
>> + return -EINVAL;
>> + }
>> +
>> if (!gpio_init_done)
>> gpio_init_done = 1;
>>
>> return 0;
>> }
>>
>> -#if defined(CONFIG_ARCH_OMAP16XX) || defined(CONFIG_ARCH_OMAP2PLUS)
>> -static int omap_gpio_suspend(struct sys_device *dev, pm_message_t mesg)
>> +static int omap_gpio_suspend(struct device *dev)
>> {
>> - int i;
>> + struct platform_device *pdev = to_platform_device(dev);
>> + void __iomem *wake_status;
>> + void __iomem *wake_clear;
>> + void __iomem *wake_set;
>> + unsigned long flags;
>> + struct gpio_bank *bank = &gpio_bank[pdev->id];
>>
>> - if (!cpu_class_is_omap2() && !cpu_is_omap16xx())
>> + /* If the gpio bank is not used, do nothing */
>> + if (!bank->mod_usage)
>
> if (pm_runtime_suspended(bank-dev))
>
>> return 0;
>>
>> - for (i = 0; i < gpio_bank_count; i++) {
>> - struct gpio_bank *bank = &gpio_bank[i];
>> - void __iomem *wake_status;
>> - void __iomem *wake_clear;
>> - void __iomem *wake_set;
>> - unsigned long flags;
>> + switch (bank->method) {
>> + case METHOD_GPIO_1610:
>> + wake_status = bank->base + OMAP1610_GPIO_WAKEUPENABLE;
>> + wake_clear = bank->base + OMAP1610_GPIO_CLEAR_WAKEUPENA;
>> + wake_set = bank->base + OMAP1610_GPIO_SET_WAKEUPENA;
>> + break;
>> + case METHOD_GPIO_24XX:
>> + wake_status = bank->base + OMAP24XX_GPIO_WAKE_EN;
>> + wake_clear = bank->base + OMAP24XX_GPIO_CLEARWKUENA;
>> + wake_set = bank->base + OMAP24XX_GPIO_SETWKUENA;
>> + break;
>> + case METHOD_GPIO_44XX:
>> + wake_status = bank->base + OMAP4_GPIO_IRQWAKEN0;
>> + wake_clear = bank->base + OMAP4_GPIO_IRQWAKEN0;
>> + wake_set = bank->base + OMAP4_GPIO_IRQWAKEN0;
>> + break;
>> + default:
>> + return 0;
>> + }
>>
>> - switch (bank->method) {
>> -#ifdef CONFIG_ARCH_OMAP16XX
>> - case METHOD_GPIO_1610:
>> - wake_status = bank->base + OMAP1610_GPIO_WAKEUPENABLE;
>> - wake_clear = bank->base + OMAP1610_GPIO_CLEAR_WAKEUPENA;
>> - wake_set = bank->base + OMAP1610_GPIO_SET_WAKEUPENA;
>> - break;
>> -#endif
>> -#if defined(CONFIG_ARCH_OMAP2) || defined(CONFIG_ARCH_OMAP3)
>> - case METHOD_GPIO_24XX:
>> - wake_status = bank->base + OMAP24XX_GPIO_WAKE_EN;
>> - wake_clear = bank->base + OMAP24XX_GPIO_CLEARWKUENA;
>> - wake_set = bank->base + OMAP24XX_GPIO_SETWKUENA;
>> - break;
>> -#endif
>> -#ifdef CONFIG_ARCH_OMAP4
>> - case METHOD_GPIO_44XX:
>> - wake_status = bank->base + OMAP4_GPIO_IRQWAKEN0;
>> - wake_clear = bank->base + OMAP4_GPIO_IRQWAKEN0;
>> - wake_set = bank->base + OMAP4_GPIO_IRQWAKEN0;
>> - break;
>> -#endif
>> - default:
>> - continue;
>> - }
>> + if (strcmp(bank->pwrdm_name, "wkup_pwrdm"))
>> + omap_gpio_save_context(bank);
>
> see comments on patch 2.
I agree.
>
>> - spin_lock_irqsave(&bank->lock, flags);
>> - bank->saved_wakeup = __raw_readl(wake_status);
>> - __raw_writel(0xffffffff, wake_clear);
>> - __raw_writel(bank->suspend_wakeup, wake_set);
>> - spin_unlock_irqrestore(&bank->lock, flags);
>> - }
>> + spin_lock_irqsave(&bank->lock, flags);
>> + bank->saved_wakeup = __raw_readl(wake_status);
>> + __raw_writel(0xffffffff, wake_clear);
>> + __raw_writel(bank->suspend_wakeup, wake_set);
>> + spin_unlock_irqrestore(&bank->lock, flags);
>> +
>> + if (unlikely(pm_runtime_put(bank->dev) < 0))
>> + return -EINVAL;
>>
>> return 0;
>> }
>>
<<snip>>
--
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
