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 <rog...@ti.com>
>>>> ---
>>>> 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 majord...@vger.kernel.org
>> 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 majord...@vger.kernel.org
> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
