"Varadarajan, Charulatha" <charu@xxxxxx> writes: > Kevin, > > On Tue, Mar 8, 2011 at 00:25, Kevin Hilman <khilman@xxxxxx> wrote: >> "Varadarajan, Charulatha" <charu@xxxxxx> writes: >> >> [...] >> >>>>> 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. >> >> You're right, I see what you're saying now. ÂThanks for clarifying. > > Okay. > >> >> In that case, keeping bank->mod_usage should be OK for now. > > Okay. > >> >> That being said, as I'm looking at the idle prepare/resume hooks >> something else came to mind. >> >> Most of what the idle prepare/resume mehods do should actually be done >> in the ->runtime_suspend() and ->runtime_resume() hooks (e.g. debounce >> clock disable, edge-detect stuff, context save/restore). ÂIOW, that >> stuff does not need to be done until the bank is actually >> disabled/enabled. ÂFor example, prepare_for_idle itself could be a >> relatively simple check for bank->mod_usage and a call to >> pm_runtime_put_sync(). >> >> What do you think? > > I also thought about this and my very old implementation with hwmod > series was like this. But, > a. prepare_for_idle has an Erratum 1.101 handling, debounce clock disable, > context save based on offmode flag > b. omap_gpio_suspend has wkup related code handling in it along > with context save w/o any flag Don't forget that the suspend path also calls prepare_for_idle (and resume path calls resume_from_idle) so that (b) actually includes (a). In fact, looking closer at the code, it appears we save context twice on a static suspend. > c. gpio_free need not do any of the above mentioned stuff. But it would be harmless if the ->runtime_suspend/resume methods were called. If bank->mod_usage were zero, these hooks would just return. > Similar for resume_after_idle, gpio_request, omap_resume. > > But the runtime_suspend/resume hooks would be called for all the above. > > Hence I thought that it might not be correct to move all the code from > prepare_for_idle() to runtime_suspend hook of GPIO. Similar for > resume_after_idle() > and runtime_resume hook. You're right, there are currently different paths for the 3 different users of the runtime PM API (your a, b & c above), but to me that leads to serious readability problems. (NOTE: this isn't your fault, the current code suffers from this already, I'm just hoping we can clean it up with the runtime PM conversion.) I think this could be much cleaner if everything was moved to the ->runtime_suspend/resume hooks and a couple checks were added. For example, the runtime_suspend would look like: for each bank: /* this handles the gpio_free case */ if (!bank->mod_usage) continue; /* debounce clock disable */ /* off-mode: remove triggering */ /* save context */ /* Extra stuff for static suspend */ if (bank->is_suspending) /* set wakeup bits */ > Also, from implementation point of view it needs to be taken care to > pass off_mode flag to runtime_suspend hook and use it only for > prepare_for idle and not for other cases > (omap_gpio_suspend/gpio_free). Actually, I'm not a big fan of the off_mode flag passed from the PM core. Here's what would be much nicer. Each bank can get it's pwrdm from its hwmod. So the ->runtime_suspend method should just read the next_state of its powerdomain to see if it's going off, and save context (and do errata workarounds) accordingly. In addition, it will _get_context_loss_count() and store the counter. The resume method then does _get_context_loss_count() again and compare to see if context needs to be restored. > Do you still think that it is appropriate to do this code movement from > prepare_for_idle() to GPIO's runtime_suspend? Based on the above suggestions, yes. >> >> [...] >> >>>>> @@ -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. >> >> I found the set of bank->mod_usage, but I do not see the clock un-gating >> in the resulting code. ÂCan you please share the line number in the >> resulting code where this is done? Â I just grep'd for 'Module is >> enabled' and the 'ctrl &= 0xFFFFFFFE' line and could not find them. > > This is done as part of omap_gpio_mod_init() (which writes zero into > ctrl register) > and it is called from omap_gpio_request(). OK, I see it now. Guess I grep'd for the wrong things. Thanks for pointing it out. 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