[1/3] ARM: OMAP2+: clockdomain: Add mechanism for disabling HW_AUTO
diff mbox

Message ID 1435061394-20379-2-git-send-email-rogerq@ti.com
State New
Headers show

Commit Message

Roger Quadros June 23, 2015, 12:09 p.m. UTC
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).

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 arch/arm/mach-omap2/clockdomain.c | 71 +++++++++++++++++++++++++++++++++++++++
 arch/arm/mach-omap2/clockdomain.h |  5 +++
 2 files changed, 76 insertions(+)

Comments

Paul Walmsley July 16, 2015, 1:25 a.m. UTC | #1
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.

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@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.

> +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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Roger Quadros July 16, 2015, 1:56 p.m. UTC | #2
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?

> 
> 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@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Roger Quadros July 28, 2015, 11:34 a.m. UTC | #3
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?

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@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 majordomo@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Roger Quadros Sept. 3, 2015, 7:39 a.m. UTC | #4
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@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 majordomo@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 majordomo@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Roger Quadros Sept. 21, 2015, 7:42 a.m. UTC | #5
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@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 majordomo@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Roger Quadros Nov. 25, 2015, 10:51 a.m. UTC | #6
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@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 majordomo@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 majordomo@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 majordomo@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren Nov. 25, 2015, 6:14 p.m. UTC | #7
* Roger Quadros <rogerq@ti.com> [151125 02:52]:
> 
> 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.

Well in the long run we want to have a proper bus driver for each interconnect
instance and use genpd. I'm afraid that solution is not going to help
immediately though.

Regards,

Tony
--
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

Patch
diff mbox

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.
+ */
+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);