[PATCH RFC 0/2] mach-omap2: handle autoidle denial

[PATCH RFC 0/2] mach-omap2: handle autoidle denial

[Andreas Kemnade]

On the gta04 with a dm3730 omap_hdq does not work properly when the
device enters lower power states. Idling uart1 and 2 is enough
to show up that problem, if there are no other things enabled.
Further research reveals that hdq iclk must not be turned off during
transfers, also according to the TRM. That fact is also correctly described
in the flags but the code to handle that is incomplete.

Since the order is first disable all autoidles, then disable selected
and then enable all, we need to either change that order or add
a usecount. Since it is done only in init, we could think about changing
order.

Andreas Kemnade (2):
  clk: ti: add a usecount for autoidle
  arm: mach-omap2: setup iclk autoidle according to flags

 arch/arm/mach-omap2/omap_hwmod.c |  8 ++++++--
 drivers/clk/ti/autoidle.c        | 20 ++++++++++++--------
 include/linux/clk/ti.h           |  1 +
 3 files changed, 19 insertions(+), 10 deletions(-)

-- 
2.11.0

[PATCH RFC 1/2] clk: ti: add a usecount for autoidle

[Andreas Kemnade]

We have the scenario that first autoidle is disabled for all clocks,
then it is disabled for selected ones and then enabled for all. So
we should have some counting here, also according to the
comment in  _setup_iclk_autoidle()

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/clk/ti/autoidle.c | 20 ++++++++++++--------
 include/linux/clk/ti.h    |  1 +
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/ti/autoidle.c b/drivers/clk/ti/autoidle.c
index 7bb9afbe4058..be2ce42e4f33 100644
--- a/drivers/clk/ti/autoidle.c
+++ b/drivers/clk/ti/autoidle.c
@@ -48,8 +48,11 @@ int omap2_clk_deny_idle(struct clk *clk)
 	struct clk_hw_omap *c;
 
 	c = to_clk_hw_omap(__clk_get_hw(clk));
-	if (c->ops && c->ops->deny_idle)
-		c->ops->deny_idle(c);
+	if (c->ops && c->ops->deny_idle) {
+		c->autoidle_count--;
+		if (c->autoidle_count == -1)
+			c->ops->deny_idle(c);
+	}
 	return 0;
 }
 
@@ -64,8 +67,11 @@ int omap2_clk_allow_idle(struct clk *clk)
 	struct clk_hw_omap *c;
 
 	c = to_clk_hw_omap(__clk_get_hw(clk));
-	if (c->ops && c->ops->allow_idle)
-		c->ops->allow_idle(c);
+	if (c->ops && c->ops->allow_idle) {
+		c->autoidle_count++;
+		if (c->autoidle_count == 0)
+			c->ops->allow_idle(c);
+	}
 	return 0;
 }
 
@@ -201,8 +207,7 @@ int omap2_clk_enable_autoidle_all(void)
 	struct clk_hw_omap *c;
 
 	list_for_each_entry(c, &clk_hw_omap_clocks, node)
-		if (c->ops && c->ops->allow_idle)
-			c->ops->allow_idle(c);
+		omap2_clk_allow_idle(c->hw.clk);
 
 	_clk_generic_allow_autoidle_all();
 
@@ -223,8 +228,7 @@ int omap2_clk_disable_autoidle_all(void)
 	struct clk_hw_omap *c;
 
 	list_for_each_entry(c, &clk_hw_omap_clocks, node)
-		if (c->ops && c->ops->deny_idle)
-			c->ops->deny_idle(c);
+		omap2_clk_deny_idle(c->hw.clk);
 
 	_clk_generic_deny_autoidle_all();
 
diff --git a/include/linux/clk/ti.h b/include/linux/clk/ti.h
index a8faa38b1ed6..c460355419c0 100644
--- a/include/linux/clk/ti.h
+++ b/include/linux/clk/ti.h
@@ -159,6 +159,7 @@ struct clk_hw_omap {
 	const char		*clkdm_name;
 	struct clockdomain	*clkdm;
 	const struct clk_hw_omap_ops	*ops;
+	int autoidle_count;
 };
 
 /*
-- 
2.11.0

[PATCH RFC 2/2] arm: mach-omap2: setup iclk autoidle according to flags

[Andreas Kemnade]

Deny autoidle for hwmods with the OCPIF_SWSUP_IDLE flag,
that makes hwmods working properly which cannot handle
autoidle properly in lower power states.
Affected is e. g. the omap_hdq.
It also disables CM_AUTOIDLE_DSS. Need to check if
that is wanted or not.
Note: Autoidle is not enabled explicitly because there
might be clocks which do not support that operation and
it is enabled afterwards in arm/mach-omap2/pm.c anyways.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 arch/arm/mach-omap2/omap_hwmod.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index bb641e6c93d0..6f9687a4f421 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -131,6 +131,7 @@
 #include <linux/errno.h>
 #include <linux/io.h>
 #include <linux/clk.h>
+#include <linux/clk/ti.h>
 #include <linux/clk-provider.h>
 #include <linux/delay.h>
 #include <linux/err.h>
@@ -2410,9 +2411,12 @@ static void __init _setup_iclk_autoidle(struct omap_hwmod *oh)
 			continue;
 
 		if (os->flags & OCPIF_SWSUP_IDLE) {
-			/* XXX omap_iclk_deny_idle(c); */
+			omap2_clk_deny_idle(os->_clk);
 		} else {
-			/* XXX omap_iclk_allow_idle(c); */
+			/*
+			 * no allow_idle here since there is
+			 * later an allow idle in pm.c
+			 */
 			clk_enable(os->_clk);
 		}
 	}
-- 
2.11.0

Re: [PATCH RFC 0/2] mach-omap2: handle autoidle denial

[Tony Lindgren]

* Andreas Kemnade <andreas@kemnade.info> [181004 05:56]:
> On the gta04 with a dm3730 omap_hdq does not work properly when the
> device enters lower power states. Idling uart1 and 2 is enough
> to show up that problem, if there are no other things enabled.
> Further research reveals that hdq iclk must not be turned off during
> transfers, also according to the TRM. That fact is also correctly described
> in the flags but the code to handle that is incomplete.
> 
> Since the order is first disable all autoidles, then disable selected
> and then enable all, we need to either change that order or add
> a usecount. Since it is done only in init, we could think about changing
> order.

These patches look OK to me, assuming Tero will review them more
closely.

It seems we should just provide a generic interface for
clk_allow_autoidle() and clk_deny_autoidle()? Otherwise we'll
be forever stuck with pdata callbacks it seems.

Regards,

Tony

Re: [PATCH RFC 1/2] clk: ti: add a usecount for autoidle

[Tero Kristo]

On 04/10/18 08:51, Andreas Kemnade wrote:
> We have the scenario that first autoidle is disabled for all clocks,
> then it is disabled for selected ones and then enabled for all. So
> we should have some counting here, also according to the
> comment in  _setup_iclk_autoidle()
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
>   drivers/clk/ti/autoidle.c | 20 ++++++++++++--------
>   include/linux/clk/ti.h    |  1 +
>   2 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clk/ti/autoidle.c b/drivers/clk/ti/autoidle.c
> index 7bb9afbe4058..be2ce42e4f33 100644
> --- a/drivers/clk/ti/autoidle.c
> +++ b/drivers/clk/ti/autoidle.c
> @@ -48,8 +48,11 @@ int omap2_clk_deny_idle(struct clk *clk)
>   	struct clk_hw_omap *c;
>   
>   	c = to_clk_hw_omap(__clk_get_hw(clk));
> -	if (c->ops && c->ops->deny_idle)
> -		c->ops->deny_idle(c);
> +	if (c->ops && c->ops->deny_idle) {
> +		c->autoidle_count--;
> +		if (c->autoidle_count == -1)
> +			c->ops->deny_idle(c);

This code is racy as you have no locking in place. Please fix.

Also, this should be verified that it doesn't cause any PM regressions, 
I hope you have done that?

-Tero

> +	}
>   	return 0;
>   }
>   
> @@ -64,8 +67,11 @@ int omap2_clk_allow_idle(struct clk *clk)
>   	struct clk_hw_omap *c;
>   
>   	c = to_clk_hw_omap(__clk_get_hw(clk));
> -	if (c->ops && c->ops->allow_idle)
> -		c->ops->allow_idle(c);
> +	if (c->ops && c->ops->allow_idle) {
> +		c->autoidle_count++;
> +		if (c->autoidle_count == 0)
> +			c->ops->allow_idle(c);
> +	}
>   	return 0;
>   }
>   
> @@ -201,8 +207,7 @@ int omap2_clk_enable_autoidle_all(void)
>   	struct clk_hw_omap *c;
>   
>   	list_for_each_entry(c, &clk_hw_omap_clocks, node)
> -		if (c->ops && c->ops->allow_idle)
> -			c->ops->allow_idle(c);
> +		omap2_clk_allow_idle(c->hw.clk);
>   
>   	_clk_generic_allow_autoidle_all();
>   
> @@ -223,8 +228,7 @@ int omap2_clk_disable_autoidle_all(void)
>   	struct clk_hw_omap *c;
>   
>   	list_for_each_entry(c, &clk_hw_omap_clocks, node)
> -		if (c->ops && c->ops->deny_idle)
> -			c->ops->deny_idle(c);
> +		omap2_clk_deny_idle(c->hw.clk);
>   
>   	_clk_generic_deny_autoidle_all();
>   
> diff --git a/include/linux/clk/ti.h b/include/linux/clk/ti.h
> index a8faa38b1ed6..c460355419c0 100644
> --- a/include/linux/clk/ti.h
> +++ b/include/linux/clk/ti.h
> @@ -159,6 +159,7 @@ struct clk_hw_omap {
>   	const char		*clkdm_name;
>   	struct clockdomain	*clkdm;
>   	const struct clk_hw_omap_ops	*ops;
> +	int autoidle_count;
>   };
>   
>   /*
> 

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH RFC 0/2] mach-omap2: handle autoidle denial

[Tero Kristo]

On 04/10/18 17:25, Tony Lindgren wrote:
> * Andreas Kemnade <andreas@kemnade.info> [181004 05:56]:
>> On the gta04 with a dm3730 omap_hdq does not work properly when the
>> device enters lower power states. Idling uart1 and 2 is enough
>> to show up that problem, if there are no other things enabled.
>> Further research reveals that hdq iclk must not be turned off during
>> transfers, also according to the TRM. That fact is also correctly described
>> in the flags but the code to handle that is incomplete.
>>
>> Since the order is first disable all autoidles, then disable selected
>> and then enable all, we need to either change that order or add
>> a usecount. Since it is done only in init, we could think about changing
>> order.
> 
> These patches look OK to me, assuming Tero will review them more
> closely.

There is no locking whatsoever in the autoidle counting atm, that must 
be fixed otherwise you get races.

> It seems we should just provide a generic interface for
> clk_allow_autoidle() and clk_deny_autoidle()? Otherwise we'll
> be forever stuck with pdata callbacks it seems.

The TI clock driver is actually providing these APIs, so that should be 
fine. I don't think there is any use / need for pdata callbacks atm, it 
just happens hwmod core is calling these at the moment which might have 
confused you.

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH RFC 0/2] mach-omap2: handle autoidle denial

[Tony Lindgren]

* Tero Kristo <t-kristo@ti.com> [181004 14:47]:
> On 04/10/18 17:25, Tony Lindgren wrote:
> > It seems we should just provide a generic interface for
> > clk_allow_autoidle() and clk_deny_autoidle()? Otherwise we'll
> > be forever stuck with pdata callbacks it seems.
> 
> The TI clock driver is actually providing these APIs, so that should be
> fine. I don't think there is any use / need for pdata callbacks atm, it just
> happens hwmod core is calling these at the moment which might have confused
> you.

Hmm OK. So do we already have some way to deny autoidle for a
clock from ti-sysc.c driver without pdata callbacks?

Suman pointed out few days ago that for a reset driver to work
we must do clkdm_deny_idle() and clkdm_allow_idle() as the hwmod
code does. I gues that really just boils down to doing clk deny
idle and allow idle on the clockdomain clkctrl clock?

Regards,

Tony

Re: [PATCH RFC 0/2] mach-omap2: handle autoidle denial

[Tero Kristo]

On 04/10/18 18:07, Tony Lindgren wrote:
> * Tero Kristo <t-kristo@ti.com> [181004 14:47]:
>> On 04/10/18 17:25, Tony Lindgren wrote:
>>> It seems we should just provide a generic interface for
>>> clk_allow_autoidle() and clk_deny_autoidle()? Otherwise we'll
>>> be forever stuck with pdata callbacks it seems.
>>
>> The TI clock driver is actually providing these APIs, so that should be
>> fine. I don't think there is any use / need for pdata callbacks atm, it just
>> happens hwmod core is calling these at the moment which might have confused
>> you.
> 
> Hmm OK. So do we already have some way to deny autoidle for a
> clock from ti-sysc.c driver without pdata callbacks?
> 
> Suman pointed out few days ago that for a reset driver to work
> we must do clkdm_deny_idle() and clkdm_allow_idle() as the hwmod
> code does. I gues that really just boils down to doing clk deny
> idle and allow idle on the clockdomain clkctrl clock?

Clkdm handling is done via pdata callbacks, that is a separate topic 
from iclk autoidle. Iclk:s are effectively only for omap3, clkdm 
autoidle / deny_idle etc. are a generic mechanism that must be used on 
omap4+ if you want to prevent autoidle of certain domains/IPs.

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH RFC 0/2] mach-omap2: handle autoidle denial

[Tony Lindgren]

* Tero Kristo <t-kristo@ti.com> [181004 15:53]:
> On 04/10/18 18:07, Tony Lindgren wrote:
> > * Tero Kristo <t-kristo@ti.com> [181004 14:47]:
> > > On 04/10/18 17:25, Tony Lindgren wrote:
> > > > It seems we should just provide a generic interface for
> > > > clk_allow_autoidle() and clk_deny_autoidle()? Otherwise we'll
> > > > be forever stuck with pdata callbacks it seems.
> > > 
> > > The TI clock driver is actually providing these APIs, so that should be
> > > fine. I don't think there is any use / need for pdata callbacks atm, it just
> > > happens hwmod core is calling these at the moment which might have confused
> > > you.
> > 
> > Hmm OK. So do we already have some way to deny autoidle for a
> > clock from ti-sysc.c driver without pdata callbacks?
> > 
> > Suman pointed out few days ago that for a reset driver to work
> > we must do clkdm_deny_idle() and clkdm_allow_idle() as the hwmod
> > code does. I gues that really just boils down to doing clk deny
> > idle and allow idle on the clockdomain clkctrl clock?
> 
> Clkdm handling is done via pdata callbacks, that is a separate topic from
> iclk autoidle. Iclk:s are effectively only for omap3, clkdm autoidle /
> deny_idle etc. are a generic mechanism that must be used on omap4+ if you
> want to prevent autoidle of certain domains/IPs.

OK thanks.

Tony

Re: [PATCH RFC 1/2] clk: ti: add a usecount for autoidle

[Andreas Kemnade]

[-- Attachment #1: Type: text/plain, Size: 2383 bytes --]

Hi,

On Thu, 4 Oct 2018 17:40:06 +0300
Tero Kristo <t-kristo@ti.com> wrote:

> On 04/10/18 08:51, Andreas Kemnade wrote:
> > We have the scenario that first autoidle is disabled for all clocks,
> > then it is disabled for selected ones and then enabled for all. So
> > we should have some counting here, also according to the
> > comment in  _setup_iclk_autoidle()
> > 
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > ---
> >   drivers/clk/ti/autoidle.c | 20 ++++++++++++--------
> >   include/linux/clk/ti.h    |  1 +
> >   2 files changed, 13 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/clk/ti/autoidle.c b/drivers/clk/ti/autoidle.c
> > index 7bb9afbe4058..be2ce42e4f33 100644
> > --- a/drivers/clk/ti/autoidle.c
> > +++ b/drivers/clk/ti/autoidle.c
> > @@ -48,8 +48,11 @@ int omap2_clk_deny_idle(struct clk *clk)
> >   	struct clk_hw_omap *c;
> >   
> >   	c = to_clk_hw_omap(__clk_get_hw(clk));
> > -	if (c->ops && c->ops->deny_idle)
> > -		c->ops->deny_idle(c);
> > +	if (c->ops && c->ops->deny_idle) {
> > +		c->autoidle_count--;
> > +		if (c->autoidle_count == -1)
> > +			c->ops->deny_idle(c);  
> 
> This code is racy as you have no locking in place. Please fix.
> 

hmm, yes, it is. Due to low risk (things are only called from init
before the drivers are initialized, if I understand that correctly). I
kept that for final polishing and not for a rfc patch.
The deny_idle/allow_idle are also racy themselves. Multiple clocks with
bits in the same register changed at the same time would also create a
mess.

> Also, this should be verified that it doesn't cause any PM regressions, 
> I hope you have done that?
> 
Tested things: I checked various autoidle registers for changes.
I checked registers by reading out /dev/mem
Seen changes: hdq iclk no autoidle (that is the goal of all this)
dss iclk no autoidle. This one is really interesting: There are
multiple users of dss_ick, so that is a special case. I will check
whether I can handle that (I have already an idea) or just delete that
flag there for sorting out that later, we could somehow live with not
disabled autoidle flag there but needed some strange fixes in the past.
Now dss_ick does unecessarily enabled, so yes, a little regression.

CORE/PER idlest seems to behave as well as before that change.

Regards,
Andreas

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]