Hi,
On 03/09/15 10:39, Roger Quadros wrote:
> 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.
Not sure if Paul is receiving my e-mails but I'd like
others to give their opinion on how we can proceed with this. 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
>
--
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
