[...] > > > 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 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
