| Message ID | 1310804152-9243-19-git-send-email-tarun.kanti@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Jul 16, 2011 at 01:45:50PM +0530, Tarun Kanti DebBarma wrote: > Call runtime pm APIs pm_runtime_get_sync() and pm_runtime_put_sync() > for enabling/disabling clocks appropriately. Remove syscore_ops and > instead use dev_pm_ops now. > ... > @@ -481,6 +483,22 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset) > > spin_lock_irqsave(&bank->lock, flags); > > + /* > + * If this is the first gpio_request for the bank, > + * enable the bank module. > + */ > + if (!bank->mod_usage) { > + if (IS_ERR_VALUE(pm_runtime_get_sync(bank->dev) < 0)) { > + dev_err(bank->dev, "%s: GPIO bank %d " > + "pm_runtime_get_sync failed\n", > + __func__, bank->id); > + return -EINVAL; Must spin_unlock_irqrestore() first. > + } > + > + /* Initialize the gpio bank registers to init time value */ > + omap_gpio_mod_init(bank); omap_gpio_mod_init calls mpuio_init calls platform_driver_register which can't be called in an IRQs off and spinlocked atomic context, for example, device_private_init calls kzalloc with GFP_KERNEL. Concurrency protection for this will need to happen prior to the spinlock (assuming it really does need to be an IRQ saving spinlock and not a mutex). Possibly a new mutex is needed to protect the check for first usage and init'ing the bank (and blocking a racing second caller until the init is done). The omap_gpio_mod_init may be unbalanced with the code performed below on last free of a GPIO for the bank? If all GPIOs are freed and then a new GPIO used, does omap_gpio_mod_init do the right thing? Need a separate flag to indicate whether one-time init has ever been performed, vs. needing runtime PM enable/disable? > + } > + > /* Set trigger to none. You need to enable the desired trigger with > * request_irq() or set_irq_type(). > */ > @@ -517,7 +535,6 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset) > unsigned long flags; > > spin_lock_irqsave(&bank->lock, flags); > - > if (bank->regs->wkup_status) > /* Disable wake-up during idle for dynamic tick */ > _gpio_rmw(base, bank->regs->wkup_status, 1 << offset, 0); > @@ -535,6 +552,18 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset) > } > > _reset_gpio(bank, bank->chip.base + offset); > + > + /* > + * If this is the last gpio to be freed in the bank, > + * disable the bank module. > + */ > + if (!bank->mod_usage) { > + if (IS_ERR_VALUE(pm_runtime_put_sync(bank->dev) < 0)) { > + dev_err(bank->dev, "%s: GPIO bank %d " > + "pm_runtime_put_sync failed\n", > + __func__, bank->id); > + } > + } > spin_unlock_irqrestore(&bank->lock, flags); Todd
[...] > > > > + /* > > + * If this is the first gpio_request for the bank, > > + * enable the bank module. > > + */ > > + if (!bank->mod_usage) { > > + if (IS_ERR_VALUE(pm_runtime_get_sync(bank->dev) < 0)) { > > + dev_err(bank->dev, "%s: GPIO bank %d " > > + "pm_runtime_get_sync failed\n", > > + __func__, bank->id); > > + return -EINVAL; > > > Must spin_unlock_irqrestore() first. Yes. > > > + } > > + > > + /* Initialize the gpio bank registers to init time value */ > > + omap_gpio_mod_init(bank); > > omap_gpio_mod_init calls mpuio_init calls platform_driver_register > which can't be called in an IRQs off and spinlocked atomic context, > for example, device_private_init calls kzalloc with GFP_KERNEL. Right! The present form of *_mod_init() is not suitable here. > > Concurrency protection for this will need to happen prior to the > spinlock (assuming it really does need to be an IRQ saving spinlock > and not a mutex). Possibly a new mutex is needed to protect the > check for first usage and init'ing the bank (and blocking a racing > second caller until the init is done). I looked at the implementation once again with the comments at background. There are gaps which require fixing. Thanks. > > The omap_gpio_mod_init may be unbalanced with the code performed below > on last free of a GPIO for the bank? If all GPIOs are freed and then > a new GPIO used, does omap_gpio_mod_init do the right thing? Need a > separate flag to indicate whether one-time init has ever been > performed, vs. needing runtime PM enable/disable? Yes, there is imbalance. I am re-looking and fixing. -- Tarun [...]
Todd, [...] > > + /* Initialize the gpio bank registers to init time value */ > > + omap_gpio_mod_init(bank); > > omap_gpio_mod_init calls mpuio_init calls platform_driver_register > which can't be called in an IRQs off and spinlocked atomic context, > for example, device_private_init calls kzalloc with GFP_KERNEL. > > Concurrency protection for this will need to happen prior to the > spinlock (assuming it really does need to be an IRQ saving spinlock > and not a mutex). Possibly a new mutex is needed to protect the > check for first usage and init'ing the bank (and blocking a racing > second caller until the init is done). I have isolated mpuio_init() from omap_gpio_mod_init(). mpuio_init() is now called once in omap_gpio_probe(). > > The omap_gpio_mod_init may be unbalanced with the code performed below > on last free of a GPIO for the bank? If all GPIOs are freed and then > a new GPIO used, does omap_gpio_mod_init do the right thing? Need a > separate flag to indicate whether one-time init has ever been > performed, vs. needing runtime PM enable/disable? With the above changes I am seeing omap_gpio_mod_init() is balanced. Let me know if I am still missing something. -- Tarun > > > + } > > + > > /* Set trigger to none. You need to enable the desired trigger with > > * request_irq() or set_irq_type(). > > */ > > @@ -517,7 +535,6 @@ static void omap_gpio_free(struct gpio_chip *chip, > unsigned offset) > > unsigned long flags; > > > > spin_lock_irqsave(&bank->lock, flags); > > - > > if (bank->regs->wkup_status) > > /* Disable wake-up during idle for dynamic tick */ > > _gpio_rmw(base, bank->regs->wkup_status, 1 << offset, 0); > > @@ -535,6 +552,18 @@ static void omap_gpio_free(struct gpio_chip *chip, > unsigned offset) > > } > > > > _reset_gpio(bank, bank->chip.base + offset); > > + > > + /* > > + * If this is the last gpio to be freed in the bank, > > + * disable the bank module. > > + */ > > + if (!bank->mod_usage) { > > + if (IS_ERR_VALUE(pm_runtime_put_sync(bank->dev) < 0)) { > > + dev_err(bank->dev, "%s: GPIO bank %d " > > + "pm_runtime_put_sync failed\n", > > + __func__, bank->id); > > + } > > + } > > spin_unlock_irqrestore(&bank->lock, flags); > > > Todd
On Wed, Jul 27, 2011 at 05:14:58PM +0530, DebBarma, Tarun Kanti wrote: ... > > omap_gpio_mod_init calls mpuio_init calls platform_driver_register > > which can't be called in an IRQs off and spinlocked atomic context, ... > I have isolated mpuio_init() from omap_gpio_mod_init(). > mpuio_init() is now called once in omap_gpio_probe(). Looking at omap_gpio_mod_init() it seems like much of its processing could probably be done once at probe time (or at pm_runtime_get_sync time) as well, namely setting the IRQ enable masks. Ungating the interface clock, enabling wakeup, setting smart idle for the module, and enabling the (old-style) system clock seem like either one-time actions at probe, or a part of the pm_runtime_get_sync callback. Or is there some other reason these power management actions should be taken each time a GPIO is requested in the block (when none were previously requested), after the runtime PM get_sync callback, but not as part of the get_sync callback? If so, what caused the smart idle setting to be lost, or the interface clock gated, if not the pm_runtime_put_sync? Todd > > The omap_gpio_mod_init may be unbalanced with the code performed below > > on last free of a GPIO for the bank? If all GPIOs are freed and then > > a new GPIO used, does omap_gpio_mod_init do the right thing? Need a > > separate flag to indicate whether one-time init has ever been > > performed, vs. needing runtime PM enable/disable? > With the above changes I am seeing omap_gpio_mod_init() is balanced. > Let me know if I am still missing something. > -- > Tarun
Todd, [...] > > > omap_gpio_mod_init calls mpuio_init calls platform_driver_register > > > which can't be called in an IRQs off and spinlocked atomic context, > ... > > I have isolated mpuio_init() from omap_gpio_mod_init(). > > mpuio_init() is now called once in omap_gpio_probe(). > > Looking at omap_gpio_mod_init() it seems like much of its processing > could probably be done once at probe time (or at pm_runtime_get_sync > time) as well, namely setting the IRQ enable masks. This must be called at the beginning of bank access to get reset state. Otherwise, it would contain settings related to previous bank access. > > Ungating the interface clock, enabling wakeup, setting smart idle for > the module, and enabling the (old-style) system clock seem like either > one-time actions at probe, or a part of the pm_runtime_get_sync > callback. Yes... sysconfig related settings are done by hwmod framework. Debounce clocks are enabled in callback. > > Or is there some other reason these power management actions should be > taken each time a GPIO is requested in the block (when none were > previously requested), after the runtime PM get_sync callback, but > not as part of the get_sync callback? If so, what caused > the smart idle setting to be lost, or the interface clock gated, if > not the pm_runtime_put_sync? I am not sure which smart idle setting you are referring to. Most stuffs done in *_get_sync() callback can skip first time. Omap_gpio_mod_init() does resetting irqenable settings to reset value. This should not be part of callback. -- Tarun > > > Todd > > > > The omap_gpio_mod_init may be unbalanced with the code performed below > > > on last free of a GPIO for the bank? If all GPIOs are freed and then > > > a new GPIO used, does omap_gpio_mod_init do the right thing? Need a > > > separate flag to indicate whether one-time init has ever been > > > performed, vs. needing runtime PM enable/disable? > > With the above changes I am seeing omap_gpio_mod_init() is balanced. > > Let me know if I am still missing something. > > -- > > Tarun
On Thu, Jul 28, 2011 at 03:05:29PM +0530, DebBarma, Tarun Kanti wrote: > > ... > > Looking at omap_gpio_mod_init() it seems like much of its processing > > could probably be done once at probe time (or at pm_runtime_get_sync > > time) as well, namely setting the IRQ enable masks. > This must be called at the beginning of bank access to get reset state. > Otherwise, it would contain settings related to previous bank access. What state may have changed between the last time a pin was released from the bank and the next time a pin is requested in the bank? Prior to this patch, omap_gpio_mod_init() is called once at probe time, and initializing irqenable masks etc. isn't reset at the beginning of bank access, if I understand correctly. If it's not actually necessary to do this on each first request to the bank then it's not a large overhead or anything, but want to make sure the PM operations being performed are correct. This patch is basically to runtime PM enable the GPIO bank when a pin is requested, and disable when no pin is requested. If other actions beyond the runtime PM enable are needed, now that a runtime PM disable is added, then it calls into question whether the enable method is missing something. More troubling are the PM actions performed in addition to pm_runtime_get_sync... > > Ungating the interface clock, enabling wakeup, setting smart idle for > > the module, and enabling the (old-style) system clock seem like either > > one-time actions at probe, or a part of the pm_runtime_get_sync > > callback. > Yes... sysconfig related settings are done by hwmod framework. > Debounce clocks are enabled in callback. > > > > > Or is there some other reason these power management actions should be > > taken each time a GPIO is requested in the block (when none were > > previously requested), after the runtime PM get_sync callback, but > > not as part of the get_sync callback? If so, what caused > > the smart idle setting to be lost, or the interface clock gated, if > > not the pm_runtime_put_sync? > I am not sure which smart idle setting you are referring to. That's part of what the magic constant in this statement is doing: __raw_writew(0x0014, bank->base + OMAP1610_GPIO_SYSCONFIG); > Most stuffs done in *_get_sync() callback can skip first time. > Omap_gpio_mod_init() does resetting irqenable settings to reset value. > This should not be part of callback. To be clear, it seems like resetting irqenable settings should be a one-time action at probe time. The power management actions such as enabling debounce clocks, setting module PRCM idle protocol behavior, and ungating interface clocks seem like they should either be one-time probe actions, or a part of the runtime PM callbacks, or balanced with corresponding actions when the last pin in the bank is released. Else what caused the interface clock to gate, or the slave idle control to change, or the debounce clock to be cut? The only thing added here that would do that is the pm_runtime_put_sync. If it can cause those actions then do other calls to pm_runtime_get_sync need to fix these up? Anyhow, mainly wanted to point that out as a possible optimization. It's pretty unlikely there's an actual problem here. Todd
[...] > > > Looking at omap_gpio_mod_init() it seems like much of its processing > > > could probably be done once at probe time (or at pm_runtime_get_sync > > > time) as well, namely setting the IRQ enable masks. > > This must be called at the beginning of bank access to get reset state. > > Otherwise, it would contain settings related to previous bank access. > > What state may have changed between the last time a pin was released > from the bank and the next time a pin is requested in the bank? Prior > to this patch, omap_gpio_mod_init() is called once at probe time, and > initializing irqenable masks etc. isn't reset at the beginning of bank > access, if I understand correctly. Call to *_gpio_mod_init() is overhead if a client driver request/release same pin(s) and no other pins in the same bank are used by others. This is just a special case. We normally expect multiple multiple clients Access pins within the bank. Here *_mod_init() would be called just once. If a bank was used temporarily just during boot it is fair to reset it Using *_mod_init() by new client driver. > > If it's not actually necessary to do this on each first request to the > bank then it's not a large overhead or anything, but want to make sure > the PM operations being performed are correct. This patch is > basically to runtime PM enable the GPIO bank when a pin is requested, > and disable when no pin is requested. If other actions beyond the > runtime PM enable are needed, now that a runtime PM disable is added, > then it calls into question whether the enable method is missing > something. More troubling are the PM actions performed in addition to > pm_runtime_get_sync... Your concern is valid. But overhead is not likely from *_gpio_request/free() for reason explained. We have inherent overhead in runtime callbacks. But as I said, we can avoid Many of the operations when bank is accessed the first time. > > > > Ungating the interface clock, enabling wakeup, setting smart idle for > > > the module, and enabling the (old-style) system clock seem like either > > > one-time actions at probe, or a part of the pm_runtime_get_sync > > > callback. > > Yes... sysconfig related settings are done by hwmod framework. > > Debounce clocks are enabled in callback. > > > > > > > > Or is there some other reason these power management actions should be > > > taken each time a GPIO is requested in the block (when none were > > > previously requested), after the runtime PM get_sync callback, but > > > not as part of the get_sync callback? If so, what caused > > > the smart idle setting to be lost, or the interface clock gated, if > > > not the pm_runtime_put_sync? > > I am not sure which smart idle setting you are referring to. > > That's part of what the magic constant in this statement is doing: > > __raw_writew(0x0014, bank->base > + OMAP1610_GPIO_SYSCONFIG); Ok, now this is moved to init and is called only once. > > > Most stuffs done in *_get_sync() callback can skip first time. > > Omap_gpio_mod_init() does resetting irqenable settings to reset value. > > This should not be part of callback. > > To be clear, it seems like resetting irqenable settings should be a > one-time action at probe time. The power management actions such as > enabling debounce clocks, setting module PRCM idle protocol behavior, > and ungating interface clocks seem like they should either be one-time > probe actions, or a part of the runtime PM callbacks, or balanced with > corresponding actions when the last pin in the bank is released. > Else what caused the interface clock to gate, or the slave idle > control to change, or the debounce clock to be cut? The only thing > added here that would do that is the pm_runtime_put_sync. If it can > cause those actions then do other calls to pm_runtime_get_sync need to > fix these up? Debounce clk enable/disable has to be associated with *_get/put_sync(). Idle mode setting and interface clock gating is a one time operation. This is done during init. We can however look at optimization of the Callbacks. > > Anyhow, mainly wanted to point that out as a possible optimization. > It's pretty unlikely there's an actual problem here. Ok, thanks. -- Tarun > > > Todd
[...] > To be clear, it seems like resetting irqenable settings should be a > one-time action at probe time. The power management actions such as Looking at it again, well we could probably avoid *_mod_init() in request. I will test and confirm. Thanks. -- Tarun [...] > > Anyhow, mainly wanted to point that out as a possible optimization. > It's pretty unlikely there's an actual problem here. > > > Todd
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c index 4191555..6f52395 100644 --- a/drivers/gpio/gpio-omap.c +++ b/drivers/gpio/gpio-omap.c @@ -79,6 +79,8 @@ struct gpio_bank { struct omap_gpio_reg_offs *regs; }; +static void omap_gpio_mod_init(struct gpio_bank *bank); + #define GPIO_INDEX(bank, gpio) (gpio % bank->width) #define GPIO_BIT(bank, gpio) (1 << GPIO_INDEX(bank, gpio)) #define GPIO_MOD_CTRL_BIT BIT(0) @@ -481,6 +483,22 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset) spin_lock_irqsave(&bank->lock, flags); + /* + * If this is the first gpio_request for the bank, + * enable the bank module. + */ + if (!bank->mod_usage) { + if (IS_ERR_VALUE(pm_runtime_get_sync(bank->dev) < 0)) { + dev_err(bank->dev, "%s: GPIO bank %d " + "pm_runtime_get_sync failed\n", + __func__, bank->id); + return -EINVAL; + } + + /* Initialize the gpio bank registers to init time value */ + omap_gpio_mod_init(bank); + } + /* Set trigger to none. You need to enable the desired trigger with * request_irq() or set_irq_type(). */ @@ -517,7 +535,6 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset) unsigned long flags; spin_lock_irqsave(&bank->lock, flags); - if (bank->regs->wkup_status) /* Disable wake-up during idle for dynamic tick */ _gpio_rmw(base, bank->regs->wkup_status, 1 << offset, 0); @@ -535,6 +552,18 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset) } _reset_gpio(bank, bank->chip.base + offset); + + /* + * If this is the last gpio to be freed in the bank, + * disable the bank module. + */ + if (!bank->mod_usage) { + if (IS_ERR_VALUE(pm_runtime_put_sync(bank->dev) < 0)) { + dev_err(bank->dev, "%s: GPIO bank %d " + "pm_runtime_put_sync failed\n", + __func__, bank->id); + } + } spin_unlock_irqrestore(&bank->lock, flags); } @@ -560,6 +589,9 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc) chained_irq_enter(chip, desc); bank = irq_get_handler_data(irq); + + pm_runtime_get_sync(bank->dev); + isr_reg = bank->base + bank->regs->irqstatus; if (WARN_ON(!isr_reg)) @@ -622,6 +654,8 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc) exit: if (!unmasked) chained_irq_exit(chip, desc); + + pm_runtime_put_sync(bank->dev); } static void gpio_irq_shutdown(struct irq_data *d) @@ -1028,12 +1062,25 @@ 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 (IS_ERR_VALUE(pm_runtime_get_sync(bank->dev) < 0)) { + dev_err(bank->dev, "%s: GPIO bank %d pm_runtime_get_sync " + "failed\n", __func__, bank->id); + iounmap(bank->base); + return -EINVAL; + } omap_gpio_mod_init(bank); omap_gpio_chip_init(bank); omap_gpio_show_rev(bank); + if (IS_ERR_VALUE(pm_runtime_put_sync(bank->dev) < 0)) { + dev_err(bank->dev, "%s: GPIO bank %d pm_runtime_put_sync " + "failed\n", __func__, bank->id); + iounmap(bank->base); + return -EINVAL; + } + list_add_tail(&bank->node, &omap_gpio_list); return ret; @@ -1044,10 +1091,12 @@ err_exit: return ret; } -static int omap_gpio_suspend(void) +static int omap_gpio_suspend(struct device *dev) { struct gpio_bank *bank; + pm_runtime_get_sync(dev); + list_for_each_entry(bank, &omap_gpio_list, node) { void __iomem *base = bank->base; void __iomem *wake_status; @@ -1065,30 +1114,33 @@ static int omap_gpio_suspend(void) spin_unlock_irqrestore(&bank->lock, flags); } + pm_runtime_put_sync(dev); + return 0; } -static void omap_gpio_resume(void) +static int omap_gpio_resume(struct device *dev) { struct gpio_bank *bank; + pm_runtime_get_sync(dev); + list_for_each_entry(bank, &omap_gpio_list, node) { void __iomem *base = bank->base; unsigned long flags; if (!bank->regs->wkup_status) - return; + return 0; spin_lock_irqsave(&bank->lock, flags); _gpio_rmw(base, bank->regs->wkup_status, bank->saved_wakeup, 1); spin_unlock_irqrestore(&bank->lock, flags); } -} -static struct syscore_ops omap_gpio_syscore_ops = { - .suspend = omap_gpio_suspend, - .resume = omap_gpio_resume, -}; + pm_runtime_put_sync(dev); + + return 0; +} #ifdef CONFIG_ARCH_OMAP2PLUS @@ -1112,6 +1164,11 @@ void omap2_gpio_prepare_for_idle(int off_mode) if (!off_mode) continue; + if (IS_ERR_VALUE(pm_runtime_put_sync(bank->dev) < 0)) + dev_err(bank->dev, "%s: GPIO bank %d " + "pm_runtime_put_sync failed\n", + __func__, bank->id); + /* If going to OFF, remove triggering for all * non-wakeup GPIOs. Otherwise spurious IRQs will be * generated. See OMAP2420 Errata item 1.101. */ @@ -1152,6 +1209,11 @@ void omap2_gpio_resume_after_idle(void) if (!bank->loses_context) continue; + if (IS_ERR_VALUE(pm_runtime_get_sync(bank->dev) < 0)) + dev_err(bank->dev, "%s: GPIO bank %d " + "pm_runtime_get_sync failed\n", + __func__, bank->id); + for (j = 0; j < hweight_long(bank->dbck_enable_mask); j++) clk_enable(bank->dbck); @@ -1267,10 +1329,16 @@ static void omap_gpio_restore_context(struct gpio_bank *bank) } #endif +static const struct dev_pm_ops gpio_pm_ops = { + .suspend = omap_gpio_suspend, + .resume = omap_gpio_resume, +}; + static struct platform_driver omap_gpio_driver = { .probe = omap_gpio_probe, .driver = { .name = "omap_gpio", + .pm = &gpio_pm_ops, }, }; @@ -1285,15 +1353,3 @@ static int __init omap_gpio_drv_reg(void) } postcore_initcall(omap_gpio_drv_reg); -static int __init omap_gpio_sysinit(void) -{ - -#if defined(CONFIG_ARCH_OMAP16XX) || defined(CONFIG_ARCH_OMAP2PLUS) - if (cpu_is_omap16xx() || cpu_class_is_omap2()) - register_syscore_ops(&omap_gpio_syscore_ops); -#endif - - return 0; -} - -arch_initcall(omap_gpio_sysinit);