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?
Need your view on this before I can re-spin this series. 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
