On 28/07/15 14:34, Roger Quadros wrote: > Paul, > > On 16/07/15 16:56, Roger Quadros wrote: >> On 16/07/15 04:25, Paul Walmsley wrote: >>> Hi >>> >>> On Tue, 23 Jun 2015, Roger Quadros wrote: >>> >>>> For some hwmods (e.g. DCAN on DRA7) we need the possibility to >>>> disable HW_AUTO for the clockdomain while the module is active. >>>> >>>> To achieve this there needs to be a refcounting mechanism to >>>> indicate whether any module in the clockdomain has requested >>>> to disable HW_AUTO. We keep track of this in 'noidlecount'. >>>> >>>> Hwmod code must use clkdm_hwmod_prevent_hwauto() to prevent >>>> HW_AUTO of the clockdomain in the future clkdm_hwmod_hwauto() calls. >>>> >>>> It must use clkdm_hwmod_allow_hwauto() to allow HW_AUTO in >>>> the future clkdm_hwmod_hwauto() calls. >>>> >>>> Hwmod code must use clkdm_hwmod_allow_hwauto() whenever it needs >>>> to request HW_AUTO of any clockdomain. (Typically after it has >>>> enabled the module). >>> >>> How about just modifying clkdm_{allow,deny}_idle*() to do the >>> idle-block-counting? The default state would be to allow idle, assuming >>> that the clockdomain flags support that state, and then clkdm_deny_idle*() >>> would increment the idle-blocking count, clkdm_allow_idle*() would >>> decrement it. Then on the 0 -> 1 or 1 -> 0 transitions, the hardware >>> would be reprogrammed appropiately. >> >> That is not possible with current hwmod code as clkdm_allow_idle() and clkdm_deny_idle() >> are not symmetrically placed. >> >> The usual flow is >> clkdm_enable_hwmod(); >> if (hwsup) >> clkdm_allow_idle(); >> >> This is mainly because it is redundant to disable auto idle before enableing >> (SW_WKUP) the clockdomain. >> >> If we take your proposal above then we have to modify all users like so. >> >> if (hwsup) >> clkdm_deny_idle(); >> clkdm_enable_hwmod(); >> if (hwsup) >> clkdm_allow_idle(); >> >> Is this really what we want? > > Any comments on this? Paul. I'm waiting on your input to rework this series if needed. Early input would be much appreciated. Thanks. cheers, -roger > >> >>> >>> Anyway, seems like that would avoid races with any >>> clkdm_{allow,deny}_idle*() users. >>> >>> A few other comments below: >>> >>> >>>> >>>> Signed-off-by: Roger Quadros <rogerq@xxxxxx> >>>> --- >>>> arch/arm/mach-omap2/clockdomain.c | 71 +++++++++++++++++++++++++++++++++++++++ >>>> arch/arm/mach-omap2/clockdomain.h | 5 +++ >>>> 2 files changed, 76 insertions(+) >>>> >>>> diff --git a/arch/arm/mach-omap2/clockdomain.c b/arch/arm/mach-omap2/clockdomain.c >>>> index 2da3b5e..a7190d2 100644 >>>> --- a/arch/arm/mach-omap2/clockdomain.c >>>> +++ b/arch/arm/mach-omap2/clockdomain.c >>>> @@ -1212,6 +1212,77 @@ ccd_exit: >>>> return 0; >>>> } >>>> >>>> +/* >>>> + * prevent future hwauto for this clkdm. If clkdm->usecount becomes hwauto isn't prevented. >>>> + * It will only prevnt future hwauto but not bring it out of hwauto. >>>> + */ >>> >>> If you modify clkdm_{allow,deny}_idle*(), this shouldn't be a major issue >>> - but all of the function comments should be fixed so that they are >>> understandable and follow kernel-doc-nano specs. >> >> OK for updating the comments. >> >> >> cheers, >> -roger >> >>> >>>> +int clkdm_hwmod_prevent_hwauto(struct clockdomain *clkdm, struct omap_hwmod *oh) >>>> +{ >>>> + /* The clkdm attribute does not exist yet prior OMAP4 */ >>>> + if (cpu_is_omap24xx() || cpu_is_omap34xx()) >>>> + return 0; >>>> + >>>> + if (!clkdm || !oh || !arch_clkdm || !arch_clkdm->clkdm_clk_disable) >>>> + return -EINVAL; >>>> + >>>> + >>>> + pwrdm_lock(clkdm->pwrdm.ptr); >>>> + clkdm->noidlecount++; >>>> + pwrdm_unlock(clkdm->pwrdm.ptr); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +/* >>>> + * allow future hwauto for this clkdm >>>> + * It won't put clkdm into hwauto. use clkdm_hwmod_hwauto() for that. >>>> + */ >>>> +int clkdm_hwmod_allow_hwauto(struct clockdomain *clkdm, struct omap_hwmod *oh) >>>> +{ >>>> + /* The clkdm attribute does not exist yet prior OMAP4 */ >>>> + if (cpu_is_omap24xx() || cpu_is_omap34xx()) >>>> + return 0; >>>> + >>>> + if (!clkdm || !oh || !arch_clkdm || !arch_clkdm->clkdm_clk_disable) >>>> + return -EINVAL; >>>> + >>>> + >>>> + pwrdm_lock(clkdm->pwrdm.ptr); >>>> + >>>> + if (clkdm->noidlecount == 0) { >>>> + pwrdm_unlock(clkdm->pwrdm.ptr); >>>> + WARN_ON(1); /* underflow */ >>>> + return -ERANGE; >>>> + } >>>> + >>>> + clkdm->noidlecount--; >>>> + pwrdm_unlock(clkdm->pwrdm.ptr); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +/* >>>> + * put clkdm in hwauto if we can. checks noidlecount to see if we can. >>>> + */ >>>> +int clkdm_hwmod_hwauto(struct clockdomain *clkdm, struct omap_hwmod *oh) >>>> +{ >>>> + /* The clkdm attribute does not exist yet prior OMAP4 */ >>>> + if (cpu_is_omap24xx() || cpu_is_omap34xx()) >>>> + return 0; >>>> + >>>> + if (!clkdm || !oh || !arch_clkdm || !arch_clkdm->clkdm_clk_disable) >>>> + return -EINVAL; >>>> + >>>> + >>>> + pwrdm_lock(clkdm->pwrdm.ptr); >>>> + if (clkdm->noidlecount == 0) >>>> + clkdm_allow_idle_nolock(clkdm); >>>> + >>>> + pwrdm_unlock(clkdm->pwrdm.ptr); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> /** >>>> * clkdm_hwmod_enable - add an enabled downstream hwmod to this clkdm >>>> * @clkdm: struct clockdomain * >>>> diff --git a/arch/arm/mach-omap2/clockdomain.h b/arch/arm/mach-omap2/clockdomain.h >>>> index 77bab5f..8c491be 100644 >>>> --- a/arch/arm/mach-omap2/clockdomain.h >>>> +++ b/arch/arm/mach-omap2/clockdomain.h >>>> @@ -114,6 +114,7 @@ struct omap_hwmod; >>>> * @wkdep_srcs: Clockdomains that can be told to wake this powerdomain up >>>> * @sleepdep_srcs: Clockdomains that can be told to keep this clkdm from inact >>>> * @usecount: Usecount tracking >>>> + * @noidlecount: Noidle count tracking. Domain won't be auto idled this is > 0. >>>> * @node: list_head to link all clockdomains together >>>> * >>>> * @prcm_partition should be a macro from mach-omap2/prcm44xx.h (OMAP4 only) >>>> @@ -138,6 +139,7 @@ struct clockdomain { >>>> struct clkdm_dep *wkdep_srcs; >>>> struct clkdm_dep *sleepdep_srcs; >>>> int usecount; >>>> + int noidlecount; >>>> struct list_head node; >>>> }; >>>> >>>> @@ -211,6 +213,9 @@ int clkdm_clk_enable(struct clockdomain *clkdm, struct clk *clk); >>>> int clkdm_clk_disable(struct clockdomain *clkdm, struct clk *clk); >>>> int clkdm_hwmod_enable(struct clockdomain *clkdm, struct omap_hwmod *oh); >>>> int clkdm_hwmod_disable(struct clockdomain *clkdm, struct omap_hwmod *oh); >>>> +int clkdm_hwmod_prevent_hwauto(struct clockdomain *clkdm, struct omap_hwmod *oh); >>>> +int clkdm_hwmod_allow_hwauto(struct clockdomain *clkdm, struct omap_hwmod *oh); >>>> +int clkdm_hwmod_hwauto(struct clockdomain *clkdm, struct omap_hwmod *oh); >>>> >>>> extern void __init omap242x_clockdomains_init(void); >>>> extern void __init omap243x_clockdomains_init(void); >>>> -- >>>> 2.1.4 >>>> >>> >>> >>> - Paul >>> >> -- >> 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 >> > -- > 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 > -- 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