Kevin, On Tue, Mar 8, 2011 at 13:23, Kevin Hilman <khilman@xxxxxx> wrote: > "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. Yes. You are right. Will modify this. > >> 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 */ Okay. But I felt that adding more flags to manage this might look ugly. But yes, this is better in terms of readability. Will do the needful. > >> 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. Ok. I will modify the GPIO driver to access these APIs directly from runtime_suspend hook of GPIO to read the next_state of its powerdomain. > 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. Okay. > >> 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. Thanks, V Charulatha <<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
