Re: [PATCH 1/3] ARM: OMAP2+: clockdomain: Add mechanism for disabling HW_AUTO

[Tony Lindgren]

* Roger Quadros  [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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] ARM: OMAP2+: clockdomain: Add mechanism for disabling HW_AUTO

[Roger Quadros]

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

Re: [PATCH 1/3] ARM: OMAP2+: clockdomain: Add mechanism for disabling HW_AUTO

[Roger Quadros]

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 
>>> ---
>>>  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)
>>> +   

Re: [PATCH 1/3] ARM: OMAP2+: clockdomain: Add mechanism for disabling HW_AUTO

[Roger Quadros]

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 
 ---
  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())
 +  

Re: [PATCH 1/3] ARM: OMAP2+: clockdomain: Add mechanism for disabling HW_AUTO

[Roger Quadros]

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

Re: [PATCH 1/3] ARM: OMAP2+: clockdomain: Add mechanism for disabling HW_AUTO

[Roger Quadros]

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

Re: [PATCH 1/3] ARM: OMAP2+: clockdomain: Add mechanism for disabling HW_AUTO

[Paul Walmsley]

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

 +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 

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

[Roger Quadros]

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

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