| Message ID | 1299245123-23444-1-git-send-email-rnayak@ti.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | Paul Walmsley |
| Headers | show |
On Fri, 1 Apr 2011, Rajendra Nayak wrote: > On 4/1/2011 8:21 PM, Rajendra Nayak wrote: > > > > In omap_hwmod.c:_enable(), what do you think about: > > > > > > > > 1. saving the current idle mode of the clockdomain, > > > > > > > > 2. forcing the clockdomain to software-supervised wakeup. > > > > > > > > 3. enabling clocks and waiting for the module as we currently do, then > > > > > > > > 4. switching the clockdomain's idle mode back to its original state? > > > > > > > > Seems like that would be a reasonable approach for the short term, at > > > > least for drivers that have been converted to PM runtime. > > > > > > Ok, I'll try and get some RFC patches in these lines soon. > > > > I tried some of what you were suggesting here and it seems to > > work well, like you said, for the drivers which are converted > > to PM runtime. > > > > Now the issue seems to be, how do we handle the ones which > > are *still* using clock framework to enable main clocks and > > are yet to be converted to PM runtime. > > > > One such, MMC, is showing me issues on OMAP4 even at boot > > and causes a crash. > > > > Its a different thing that some of these drivers which use > > direct clock calls are working by fluke on OMAP4 since the > > clock framework does not even wait for the modules to become > > accessible after the clock enable. > > > > I know the right way seems to be to get all these drivers > > converted to PM runtime, but that might take sometime. > > One way I am able to get this working (atleast MMC) > is by preventing the clock domain belonging to MMC module > from being programmed into HW_SUP mode. What programs the OMAP4 clockdomains into hwsup mode in the first place? There's no clkdms_setup() as there is in mach-omap2/pm34xx.c. I guess maybe this code in mach-omap2/pm.c might do it as a side effect: switch (sleep_switch) { case FORCEWAKEUP_SWITCH: if (pwrdm->pwrdm_clkdms[0]->flags & CLKDM_CAN_ENABLE_AUTO) clkdm_allow_idle(pwrdm->pwrdm_clkdms[0]); else clkdm_sleep(pwrdm->pwrdm_clkdms[0]); break; If I'm reading this code correctly, it will always force clockdomains that support hwsup mode into hwsup mode, even if they've been previously programmed to swsup mode. That's not right. This function should leave the clockdomain's autoidle setting where it was when the function was entered. So this needs to be fixed also. > Acceptable hack in the interim while we wait for the MMC driver to be > using PM runtime? Ideally this should be overridden in the code, not the data, since this hack is needed due to a software problem, not a hardware problem. The clockdomains data should just be a statement of what the hardware supports. But given the above bug and the lack of some clkdms_setup() function for OMAP4, I guess the clockdomains data is the least bad choice for right now, provided that the patch also: 1. puts a big comment in that data warning about the problem and explaining why 2. puts a pr_warn() in mach-omap2/pm44xx.c:omap4_pm_init() saying: "WARNING: OMAP4 power savings limited since MMC driver not converted to PM runtime" - Paul -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Paul, On 4/4/2011 12:17 PM, Paul Walmsley wrote: > On Fri, 1 Apr 2011, Rajendra Nayak wrote: > >> On 4/1/2011 8:21 PM, Rajendra Nayak wrote: >>>>> In omap_hwmod.c:_enable(), what do you think about: >>>>> >>>>> 1. saving the current idle mode of the clockdomain, >>>>> >>>>> 2. forcing the clockdomain to software-supervised wakeup. >>>>> >>>>> 3. enabling clocks and waiting for the module as we currently do, then >>>>> >>>>> 4. switching the clockdomain's idle mode back to its original state? >>>>> >>>>> Seems like that would be a reasonable approach for the short term, at >>>>> least for drivers that have been converted to PM runtime. >>>> >>>> Ok, I'll try and get some RFC patches in these lines soon. >>> >>> I tried some of what you were suggesting here and it seems to >>> work well, like you said, for the drivers which are converted >>> to PM runtime. >>> >>> Now the issue seems to be, how do we handle the ones which >>> are *still* using clock framework to enable main clocks and >>> are yet to be converted to PM runtime. >>> >>> One such, MMC, is showing me issues on OMAP4 even at boot >>> and causes a crash. >>> >>> Its a different thing that some of these drivers which use >>> direct clock calls are working by fluke on OMAP4 since the >>> clock framework does not even wait for the modules to become >>> accessible after the clock enable. >>> >>> I know the right way seems to be to get all these drivers >>> converted to PM runtime, but that might take sometime. >> >> One way I am able to get this working (atleast MMC) >> is by preventing the clock domain belonging to MMC module >> from being programmed into HW_SUP mode. > > What programs the OMAP4 clockdomains into hwsup mode in the first place? This is done as part of the OMAP4 PM series. Regards Santosh -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Paul, On 4/4/2011 12:17 PM, Paul Walmsley wrote: > On Fri, 1 Apr 2011, Rajendra Nayak wrote: > >> On 4/1/2011 8:21 PM, Rajendra Nayak wrote: >>>>> In omap_hwmod.c:_enable(), what do you think about: >>>>> >>>>> 1. saving the current idle mode of the clockdomain, >>>>> >>>>> 2. forcing the clockdomain to software-supervised wakeup. >>>>> >>>>> 3. enabling clocks and waiting for the module as we currently do, then >>>>> >>>>> 4. switching the clockdomain's idle mode back to its original state? >>>>> >>>>> Seems like that would be a reasonable approach for the short term, at >>>>> least for drivers that have been converted to PM runtime. >>>> >>>> Ok, I'll try and get some RFC patches in these lines soon. >>> >>> I tried some of what you were suggesting here and it seems to >>> work well, like you said, for the drivers which are converted >>> to PM runtime. >>> >>> Now the issue seems to be, how do we handle the ones which >>> are *still* using clock framework to enable main clocks and >>> are yet to be converted to PM runtime. >>> >>> One such, MMC, is showing me issues on OMAP4 even at boot >>> and causes a crash. >>> >>> Its a different thing that some of these drivers which use >>> direct clock calls are working by fluke on OMAP4 since the >>> clock framework does not even wait for the modules to become >>> accessible after the clock enable. >>> >>> I know the right way seems to be to get all these drivers >>> converted to PM runtime, but that might take sometime. >> >> One way I am able to get this working (atleast MMC) >> is by preventing the clock domain belonging to MMC module >> from being programmed into HW_SUP mode. > > What programs the OMAP4 clockdomains into hwsup mode in the first place? This is not yet in mainline, but being done by Santosh's series which adds OMAP4 low power support in idle and suspend. http://marc.info/?l=linux-omap&m=130130417927632&w=2 > There's no clkdms_setup() as there is in mach-omap2/pm34xx.c. I guess > maybe this code in mach-omap2/pm.c might do it as a side effect: > > switch (sleep_switch) { > case FORCEWAKEUP_SWITCH: > if (pwrdm->pwrdm_clkdms[0]->flags& CLKDM_CAN_ENABLE_AUTO) > clkdm_allow_idle(pwrdm->pwrdm_clkdms[0]); > else > clkdm_sleep(pwrdm->pwrdm_clkdms[0]); > break; > > If I'm reading this code correctly, it will always force clockdomains that > support hwsup mode into hwsup mode, even if they've been previously > programmed to swsup mode. That's not right. This function should leave > the clockdomain's autoidle setting where it was when the function was > entered. So this needs to be fixed also. You are right, the code is buggy, I just posted a short series which should fix this. http://marc.info/?l=linux-omap&m=130200752115352&w=2 > >> Acceptable hack in the interim while we wait for the MMC driver to be >> using PM runtime? > > Ideally this should be overridden in the code, not the data, since this > hack is needed due to a software problem, not a hardware problem. The > clockdomains data should just be a statement of what the hardware > supports. > > But given the above bug and the lack of some clkdms_setup() function for > OMAP4, I guess the clockdomains data is the least bad choice for right > now, provided that the patch also: With Santosh's series which adds the clkdms_setup and my series to fix the above bug, I should be able to handle this in the code, rather than hack the clockdomains data. I will post some patches for that as well. regards Rajendra > > 1. puts a big comment in that data warning about the problem and > explaining why > > 2. puts a pr_warn() in mach-omap2/pm44xx.c:omap4_pm_init() saying: > "WARNING: OMAP4 power savings limited since MMC driver not converted to > PM runtime" > > > - Paul -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/mach-omap2/clock.c b/arch/arm/mach-omap2/clock.c index 46d03cc..1cf6786 100644 --- a/arch/arm/mach-omap2/clock.c +++ b/arch/arm/mach-omap2/clock.c @@ -322,6 +322,10 @@ int omap2_clk_enable(struct clk *clk) } } + /* If clockdomain supports hardware control, enable it */ + if (clk->clkdm) + clkdm_allow_idle(clk->clkdm); + return 0; oce_err3: diff --git a/arch/arm/mach-omap2/clockdomain.c b/arch/arm/mach-omap2/clockdomain.c index a0341de..9e1164f 100644 --- a/arch/arm/mach-omap2/clockdomain.c +++ b/arch/arm/mach-omap2/clockdomain.c @@ -825,7 +825,12 @@ int clkdm_clk_enable(struct clockdomain *clkdm, struct clk *clk) if (!arch_clkdm || !arch_clkdm->clkdm_clk_enable) return -EINVAL; - if (atomic_inc_return(&clkdm->usecount) > 1) + /* + * For arch's with no autodeps, clkcm_clk_enable + * should be called for every clock instance that is + * enabled, so the clkdm can be force woken up. + */ + if ((atomic_inc_return(&clkdm->usecount) > 1) && autodeps) return 0; /* Clockdomain now has one enabled downstream clock */ diff --git a/arch/arm/mach-omap2/clockdomain44xx.c b/arch/arm/mach-omap2/clockdomain44xx.c index a1a4ecd..6f38d47 100644 --- a/arch/arm/mach-omap2/clockdomain44xx.c +++ b/arch/arm/mach-omap2/clockdomain44xx.c @@ -95,13 +95,8 @@ static void omap4_clkdm_deny_idle(struct clockdomain *clkdm) static int omap4_clkdm_clk_enable(struct clockdomain *clkdm) { - bool hwsup = false; - - hwsup = omap4_cminst_is_clkdm_in_hwsup(clkdm->prcm_partition, - clkdm->cm_inst, clkdm->clkdm_offs); - - if (!hwsup) - clkdm_wakeup(clkdm); + /* For every clock enable, force wakeup the clkdm */ + clkdm_wakeup(clkdm); return 0; }