[PATCH v3 0/3] mach-omap2: handle autoidle denial

[PATCH v3 0/3] 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.

To handle multiple users of a single ick, autoidle is disabled
when a user of that ick requires that (has the OCPIF_SWSUP_IDLE))

Changes v3:
- replace CLK_IS_BASIC

Changes v2:
- uses spinlocks instead of mutexes
- invert counter logic
- check whether clock type is basic

Depends on: clk: ti: get rid of CLK_IS_BASIC

Andreas Kemnade (3):
  clk: ti: add a usecount for autoidle
  clk: ti: check clock type before doing autoidle ops
  arm: omap_hwmod disable ick autoidling when a hwmod requires that

 arch/arm/mach-omap2/omap_hwmod.c | 16 +++++++++----
 drivers/clk/ti/autoidle.c        | 52 +++++++++++++++++++++++++++++++++-------
 include/linux/clk/ti.h           |  1 +
 3 files changed, 57 insertions(+), 12 deletions(-)

-- 
2.11.0

[PATCH v3 1/3] clk: ti: add a usecount for autoidle

[Andreas Kemnade]

Multiple users might deny autoidle on a clock. So we should have some
counting here, also according to the comment in  _setup_iclk_autoidle().
Also setting autoidle regs is not atomic, so there is another reason
for locking.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
Change  since v2:
- rebase on top of clk: ti: get rid of CLK_IS_BASIC
Changes since v1:
- use spinlocks instead of mutexes
- invert logic
 drivers/clk/ti/autoidle.c | 32 ++++++++++++++++++++++++++++----
 include/linux/clk/ti.h    |  1 +
 2 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/ti/autoidle.c b/drivers/clk/ti/autoidle.c
index a129b4b36ea3..964e97b5478a 100644
--- a/drivers/clk/ti/autoidle.c
+++ b/drivers/clk/ti/autoidle.c
@@ -36,17 +36,41 @@ struct clk_ti_autoidle {
 
 static LIST_HEAD(autoidle_clks);
 
+/*
+ * we have some non-atomic read/write
+ * operations behind it, so lets
+ * take one lock for handling autoidle
+ * of all clocks
+ */
+static DEFINE_SPINLOCK(autoidle_spinlock);
+
 static int _omap2_clk_deny_idle(struct clk_hw_omap *clk)
 {
-	if (clk->ops && clk->ops->deny_idle)
-		clk->ops->deny_idle(clk);
+	if (clk->ops && clk->ops->deny_idle) {
+		unsigned long irqflags;
+
+		spin_lock_irqsave(&autoidle_spinlock, irqflags);
+		clk->autoidle_count++;
+		if (clk->autoidle_count == 1)
+			clk->ops->deny_idle(clk);
+
+		spin_unlock_irqrestore(&autoidle_spinlock, irqflags);
+	}
 	return 0;
 }
 
 static int _omap2_clk_allow_idle(struct clk_hw_omap *clk)
 {
-	if (clk->ops && clk->ops->allow_idle)
-		clk->ops->allow_idle(clk);
+	if (clk->ops && clk->ops->allow_idle) {
+		unsigned long irqflags;
+
+		spin_lock_irqsave(&autoidle_spinlock, irqflags);
+		clk->autoidle_count--;
+		if (clk->autoidle_count == 0)
+			clk->ops->allow_idle(clk);
+
+		spin_unlock_irqrestore(&autoidle_spinlock, irqflags);
+	}
 	return 0;
 }
 
diff --git a/include/linux/clk/ti.h b/include/linux/clk/ti.h
index eacc5df57b99..78872efc7be0 100644
--- a/include/linux/clk/ti.h
+++ b/include/linux/clk/ti.h
@@ -160,6 +160,7 @@ struct clk_hw_omap {
 	struct clockdomain	*clkdm;
 	const struct clk_hw_omap_ops	*ops;
 	u32			context;
+	int			autoidle_count;
 };
 
 /*
-- 
2.11.0

[PATCH v3 2/3] clk: ti: check clock type before doing autoidle ops

[Andreas Kemnade]

Code might use autoidle api with clocks not being omap2 clocks,
so check if clock type is really omap2.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
v3: replace CLK_IS_BASIC check
New in v2
 drivers/clk/ti/autoidle.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/ti/autoidle.c b/drivers/clk/ti/autoidle.c
index 964e97b5478a..1cae226759dd 100644
--- a/drivers/clk/ti/autoidle.c
+++ b/drivers/clk/ti/autoidle.c
@@ -82,9 +82,15 @@ static int _omap2_clk_allow_idle(struct clk_hw_omap *clk)
  */
 int omap2_clk_deny_idle(struct clk *clk)
 {
-	struct clk_hw_omap *c = to_clk_hw_omap(__clk_get_hw(clk));
+	struct clk_hw *hw = __clk_get_hw(clk);
 
-	return _omap2_clk_deny_idle(c);
+	if (omap2_clk_is_hw_omap(hw)) {
+		struct clk_hw_omap *c = to_clk_hw_omap(hw);
+
+		return _omap2_clk_deny_idle(c);
+	}
+
+	return -EINVAL;
 }
 
 /**
@@ -95,9 +101,15 @@ int omap2_clk_deny_idle(struct clk *clk)
  */
 int omap2_clk_allow_idle(struct clk *clk)
 {
-	struct clk_hw_omap *c = to_clk_hw_omap(__clk_get_hw(clk));
+	struct clk_hw *hw = __clk_get_hw(clk);
+
+	if (omap2_clk_is_hw_omap(hw)) {
+		struct clk_hw_omap *c = to_clk_hw_omap(hw);
+
+		return _omap2_clk_allow_idle(c);
+	}
 
-	return _omap2_clk_allow_idle(c);
+	return -EINVAL;
 }
 
 static void _allow_autoidle(struct clk_ti_autoidle *clk)
-- 
2.11.0

[PATCH v3 3/3] arm: omap_hwmod disable ick autoidling when a hwmod requires that

[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.
Since an ick might have mulitple users, autoidle is disabled
when an individual user requires that rather than in
_setup_iclk_autoidle. dss_ick is an example for that.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 Comments to v1 to this patch were worked into a new 2/3
---
 arch/arm/mach-omap2/omap_hwmod.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index b5531dd3ae9c..3a04c73ac03c 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -1002,8 +1002,10 @@ static int _enable_clocks(struct omap_hwmod *oh)
 		clk_enable(oh->_clk);
 
 	list_for_each_entry(os, &oh->slave_ports, node) {
-		if (os->_clk && (os->flags & OCPIF_SWSUP_IDLE))
+		if (os->_clk && (os->flags & OCPIF_SWSUP_IDLE)) {
+			omap2_clk_deny_idle(os->_clk);
 			clk_enable(os->_clk);
+		}
 	}
 
 	/* The opt clocks are controlled by the device driver. */
@@ -1055,8 +1057,10 @@ static int _disable_clocks(struct omap_hwmod *oh)
 		clk_disable(oh->_clk);
 
 	list_for_each_entry(os, &oh->slave_ports, node) {
-		if (os->_clk && (os->flags & OCPIF_SWSUP_IDLE))
+		if (os->_clk && (os->flags & OCPIF_SWSUP_IDLE)) {
 			clk_disable(os->_clk);
+			omap2_clk_allow_idle(os->_clk);
+		}
 	}
 
 	if (oh->flags & HWMOD_OPT_CLKS_NEEDED)
@@ -2436,9 +2440,13 @@ static void _setup_iclk_autoidle(struct omap_hwmod *oh)
 			continue;
 
 		if (os->flags & OCPIF_SWSUP_IDLE) {
-			/* XXX omap_iclk_deny_idle(c); */
+			/*
+			 * we might have multiple users of one iclk with
+			 * different requirements, disable autoidle when
+			 * the module is enabled, e.g. dss iclk
+			 */
 		} else {
-			/* XXX omap_iclk_allow_idle(c); */
+			/* we are enabling autoidle afterwards anyways */
 			clk_enable(os->_clk);
 		}
 	}
-- 
2.11.0

Re: [PATCH v3 3/3] arm: omap_hwmod disable ick autoidling when a hwmod requires that

[Tony Lindgren]

Hi,

* Andreas Kemnade <andreas@kemnade.info> [190116 22:04]:
> Deny autoidle for hwmods with the OCPIF_SWSUP_IDLE flag,
> that makes hwmods working properly which cannot handle
> autoidle properly in lower power states.

Sorry if I'm still missing something :)

But doesn't this now block autoidle for all modules
with OCPIF_SWSUP_IDLE even if they work just fine with
autoidle?

I think what you want to do is keep clocks enabled
while in use?

If so, how about using HWMOD_CLKDM_NOAUTO:

"HWMOD_CLKDM_NOAUTO: Allows the hwmod's clockdomain to
 be prevented from entering HW_AUTO while hwmod is
 active."

> Affected is e. g. the omap_hdq.

Have you already tried what happens if you just tag
omap_hdq with HWMOD_CLKDM_NOAUTO?

Regards,

Tony

Re: [PATCH v3 3/3] arm: omap_hwmod disable ick autoidling when a hwmod requires that

[Andreas Kemnade]

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

Hi,

On Fri, 18 Jan 2019 07:48:07 -0800
Tony Lindgren <tony@atomide.com> wrote:

> Hi,
> 
> * Andreas Kemnade <andreas@kemnade.info> [190116 22:04]:
> > Deny autoidle for hwmods with the OCPIF_SWSUP_IDLE flag,
> > that makes hwmods working properly which cannot handle
> > autoidle properly in lower power states.  
> 
> Sorry if I'm still missing something :)
> 
> But doesn't this now block autoidle for all modules
> with OCPIF_SWSUP_IDLE even if they work just fine with
> autoidle?

According to the code comments, it was just meant for that.
             if (os->flags & OCPIF_SWSUP_IDLE) {
-                       /* XXX omap_iclk_deny_idle(c); */
+                       /*


I guess there are workarounds for the other modules in place,
or critical situations were not found yet. 
The other affected module is e.g. DSS. And we had some trouble
in initialisation order for omap3 in the past and did some
quirks. Probably we also fixed issues in reality caused by
having the autoidle bit set.
That flags also causes the iclk being enabled/disabled
manually.

Did you see any regressions? The patch is now 3 month old
and nobody complained. I checked module idlest bits and did
not see any changes.

> 
> I think what you want to do is keep clocks enabled
> while in use?
> 
> If so, how about using HWMOD_CLKDM_NOAUTO:
> 
> "HWMOD_CLKDM_NOAUTO: Allows the hwmod's clockdomain to
>  be prevented from entering HW_AUTO while hwmod is
>  active."

That is about iclk. I think we should have clear wording here
between all the idle things.
> 
> > Affected is e. g. the omap_hdq.  
> 
> Have you already tried what happens if you just tag
> omap_hdq with HWMOD_CLKDM_NOAUTO?
> 
Well, I am just happy with having that single bit cleared.
But having two flags for the same things makes no sense to me.

Regards,
Andreas

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

Re: [PATCH v3 3/3] arm: omap_hwmod disable ick autoidling when a hwmod requires that

[Tony Lindgren]

* Andreas Kemnade <andreas@kemnade.info> [190118 17:18]:
> On Fri, 18 Jan 2019 07:48:07 -0800
> Tony Lindgren <tony@atomide.com> wrote:
> > * Andreas Kemnade <andreas@kemnade.info> [190116 22:04]:
> > > Deny autoidle for hwmods with the OCPIF_SWSUP_IDLE flag,
> > > that makes hwmods working properly which cannot handle
> > > autoidle properly in lower power states.  
> > 
> > Sorry if I'm still missing something :)
> > 
> > But doesn't this now block autoidle for all modules
> > with OCPIF_SWSUP_IDLE even if they work just fine with
> > autoidle?
> 
> According to the code comments, it was just meant for that.
>              if (os->flags & OCPIF_SWSUP_IDLE) {
> -                       /* XXX omap_iclk_deny_idle(c); */
> +                       /*

Hmm..

> I guess there are workarounds for the other modules in place,
> or critical situations were not found yet. 
> The other affected module is e.g. DSS. And we had some trouble
> in initialisation order for omap3 in the past and did some
> quirks. Probably we also fixed issues in reality caused by
> having the autoidle bit set.

Yes this is rather confusing plus we can't do anything
from the interconnect or reset device drivers to block
autoidle for a clock currently.

So I'd like to have a generic interfaces for clk_deny_idle()
and clk_allow_idle() in place for a proper hardware based
solution instead of the hackish PM QoS DMA latency tinkering
and other workarounds. Anything else feels just like kicking
the can until the next workaround.

> That flags also causes the iclk being enabled/disabled
> manually.

Yes but SWSUP_IDLE for the interface clock to me currently
just means:

"manually enable and disable ocp interface clock"

and with your changes it becomes:

"manually enable and disable ocp interface clock and block
 autoidle while in use".

So aren't we now changing the way things behave in general
for SWSUP_IDLE?

> Did you see any regressions? The patch is now 3 month old
> and nobody complained. I checked module idlest bits and did
> not see any changes.

Sorry for all the delays. But I also need to consider how
this is going to make things easier for moving to use
drivers/reset for example. And it seems we're just now
piling up more dependencies and don't have a generic
interface that keeps preventing doing proper device
drivers. I don't see issue with your patches except for
the few open questions in this email.

> > I think what you want to do is keep clocks enabled
> > while in use?
> > 
> > If so, how about using HWMOD_CLKDM_NOAUTO:
> > 
> > "HWMOD_CLKDM_NOAUTO: Allows the hwmod's clockdomain to
> >  be prevented from entering HW_AUTO while hwmod is
> >  active."
> 
> That is about iclk. I think we should have clear wording here
> between all the idle things.

Do you mean HWMOD_CLKDM_NOAUTO is about the module clock
while your patches are about the ocp interface clock?
Yup that's confusing between ocp interface clock and the
module clock..

Then we also have SWSUP_IDLE vs SWSUP_SIDLE that can get
confused :)

BTW, is the ocp module interface clock also the module
clock in your case?

> > > Affected is e. g. the omap_hdq.  
> > 
> > Have you already tried what happens if you just tag
> > omap_hdq with HWMOD_CLKDM_NOAUTO?
> > 
> Well, I am just happy with having that single bit cleared.
> But having two flags for the same things makes no sense to me.

To me it seems there are at least the following case where we
need to block autoidle for clocks:

1. Modules that need to stay active and don't automatically
   block SoC idle states (mcbsp, hdq) where this should
   happen automatically when pm_runtime_get() is done.

2. Any drivers/reset driver while doing a reset

Regards,

Tony

Re: [PATCH v3 3/3] arm: omap_hwmod disable ick autoidling when a hwmod requires that

[Andreas Kemnade]

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

Hi,

On Fri, 18 Jan 2019 10:36:30 -0800
Tony Lindgren <tony@atomide.com> wrote:

[...]
> til the next workaround.
> 
> > That flags also causes the iclk being enabled/disabled
> > manually.  
> 
> Yes but SWSUP_IDLE for the interface clock to me currently
> just means:
> 
> "manually enable and disable ocp interface clock"
> 
well, if we want to manually disable it and not automatically,
we have to disable autoidle or it will be automatically disabled.

Disabling it manually when it is already auto-disabled (by autoidle) is
just practically a no-op towards the clock.

> and with your changes it becomes:
> 
> "manually enable and disable ocp interface clock and block
>  autoidle while in use".
> 
> So aren't we now changing the way things behave in general
> for SWSUP_IDLE?
> 
Yes, we are, so proper testing is needed. But If I read those comments
it was always intended this way but not fully implemented because it
appeared to be more work like needing a usecounter (which my patchset
also adds) for that autoidle flag.

Regards,
Andreas

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

Re: [Letux-kernel] [PATCH v3 3/3] arm: omap_hwmod disable ick autoidling when a hwmod requires that

[Andreas Kemnade]

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

On Fri, 18 Jan 2019 20:38:47 +0100
Andreas Kemnade <andreas@kemnade.info> wrote:

> Hi,
> 
> On Fri, 18 Jan 2019 10:36:30 -0800
> Tony Lindgren <tony@atomide.com> wrote:
> 
> [...]
> > til the next workaround.
> >   
> > > That flags also causes the iclk being enabled/disabled
> > > manually.    
> > 
> > Yes but SWSUP_IDLE for the interface clock to me currently
> > just means:
> > 
> > "manually enable and disable ocp interface clock"
> >   
> well, if we want to manually disable it and not automatically,
> we have to disable autoidle or it will be automatically disabled.
> 
> Disabling it manually when it is already auto-disabled (by autoidle) is
> just practically a no-op towards the clock.
> 
> > and with your changes it becomes:
> > 
> > "manually enable and disable ocp interface clock and block
> >  autoidle while in use".
> > 
> > So aren't we now changing the way things behave in general
> > for SWSUP_IDLE?
> >   
> Yes, we are, so proper testing is needed. But If I read those comments
> it was always intended this way but not fully implemented because it
> appeared to be more work like needing a usecounter (which my patchset
> also adds) for that autoidle flag.
> 
and there are quite few hwmods marked by this flag. 
And then there are those clocks marked by this flags (on am33xx) which
do not have that autoidle feature at all, so the risk is not too high.

Regards,
Andreas

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

Re: [PATCH v3 3/3] arm: omap_hwmod disable ick autoidling when a hwmod requires that

[Tony Lindgren]

* Andreas Kemnade <andreas@kemnade.info> [190118 19:39]:
> Hi,
> 
> On Fri, 18 Jan 2019 10:36:30 -0800
> Tony Lindgren <tony@atomide.com> wrote:
> 
> [...]
> > til the next workaround.
> > 
> > > That flags also causes the iclk being enabled/disabled
> > > manually.  
> > 
> > Yes but SWSUP_IDLE for the interface clock to me currently
> > just means:
> > 
> > "manually enable and disable ocp interface clock"
> > 
> well, if we want to manually disable it and not automatically,
> we have to disable autoidle or it will be automatically disabled.
> 
> Disabling it manually when it is already auto-disabled (by autoidle) is
> just practically a no-op towards the clock.

OK I buy that :) It should be probably added to the patch
description to make it clear what it changes.

Tero, any comments on this change?

> > and with your changes it becomes:
> > 
> > "manually enable and disable ocp interface clock and block
> >  autoidle while in use".
> > 
> > So aren't we now changing the way things behave in general
> > for SWSUP_IDLE?
> > 
> Yes, we are, so proper testing is needed. But If I read those comments
> it was always intended this way but not fully implemented because it
> appeared to be more work like needing a usecounter (which my patchset
> also adds) for that autoidle flag.

OK yeah the use count seems necessary. I'll test here
with my PM use cases.

Regards,

Tony

Re: [Letux-kernel] [PATCH v3 3/3] arm: omap_hwmod disable ick autoidling when a hwmod requires that

[Tony Lindgren]

* Andreas Kemnade <andreas@kemnade.info> [190118 19:42]:
> On Fri, 18 Jan 2019 20:38:47 +0100
> Andreas Kemnade <andreas@kemnade.info> wrote:
> 
> > Hi,
> > 
> > On Fri, 18 Jan 2019 10:36:30 -0800
> > Tony Lindgren <tony@atomide.com> wrote:
> > 
> > [...]
> > > til the next workaround.
> > >   
> > > > That flags also causes the iclk being enabled/disabled
> > > > manually.    
> > > 
> > > Yes but SWSUP_IDLE for the interface clock to me currently
> > > just means:
> > > 
> > > "manually enable and disable ocp interface clock"
> > >   
> > well, if we want to manually disable it and not automatically,
> > we have to disable autoidle or it will be automatically disabled.
> > 
> > Disabling it manually when it is already auto-disabled (by autoidle) is
> > just practically a no-op towards the clock.
> > 
> > > and with your changes it becomes:
> > > 
> > > "manually enable and disable ocp interface clock and block
> > >  autoidle while in use".
> > > 
> > > So aren't we now changing the way things behave in general
> > > for SWSUP_IDLE?
> > >   
> > Yes, we are, so proper testing is needed. But If I read those comments
> > it was always intended this way but not fully implemented because it
> > appeared to be more work like needing a usecounter (which my patchset
> > also adds) for that autoidle flag.
> > 
> and there are quite few hwmods marked by this flag. 
> And then there are those clocks marked by this flags (on am33xx) which
> do not have that autoidle feature at all, so the risk is not too high.

Keerthy, can you please test this series on top of the
related clock patches with your am335x PM test cases?

Regards,

Tony

Re: [Letux-kernel] [PATCH v3 3/3] arm: omap_hwmod disable ick autoidling when a hwmod requires that

[J, KEERTHY]

On 1/19/2019 1:18 AM, Tony Lindgren wrote:
> * Andreas Kemnade <andreas@kemnade.info> [190118 19:42]:
>> On Fri, 18 Jan 2019 20:38:47 +0100
>> Andreas Kemnade <andreas@kemnade.info> wrote:
>>
>>> Hi,
>>>
>>> On Fri, 18 Jan 2019 10:36:30 -0800
>>> Tony Lindgren <tony@atomide.com> wrote:
>>>
>>> [...]
>>>> til the next workaround.
>>>>    
>>>>> That flags also causes the iclk being enabled/disabled
>>>>> manually.
>>>>
>>>> Yes but SWSUP_IDLE for the interface clock to me currently
>>>> just means:
>>>>
>>>> "manually enable and disable ocp interface clock"
>>>>    
>>> well, if we want to manually disable it and not automatically,
>>> we have to disable autoidle or it will be automatically disabled.
>>>
>>> Disabling it manually when it is already auto-disabled (by autoidle) is
>>> just practically a no-op towards the clock.
>>>
>>>> and with your changes it becomes:
>>>>
>>>> "manually enable and disable ocp interface clock and block
>>>>   autoidle while in use".
>>>>
>>>> So aren't we now changing the way things behave in general
>>>> for SWSUP_IDLE?
>>>>    
>>> Yes, we are, so proper testing is needed. But If I read those comments
>>> it was always intended this way but not fully implemented because it
>>> appeared to be more work like needing a usecounter (which my patchset
>>> also adds) for that autoidle flag.
>>>
>> and there are quite few hwmods marked by this flag.
>> And then there are those clocks marked by this flags (on am33xx) which
>> do not have that autoidle feature at all, so the risk is not too high.
> 
> Keerthy, can you please test this series on top of the
> related clock patches with your am335x PM test cases?

Can you point me to the clock series that needs to be tested
along with this?

- Keerthy

> 
> Regards,
> 
> Tony
> 
> 

Re: [Letux-kernel] [PATCH v3 3/3] arm: omap_hwmod disable ick autoidling when a hwmod requires that

[Andreas Kemnade]

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

On Sat, 19 Jan 2019 12:09:48 +0530
"J, KEERTHY" <j-keerthy@ti.com> wrote:

> On 1/19/2019 1:18 AM, Tony Lindgren wrote:
> > * Andreas Kemnade <andreas@kemnade.info> [190118 19:42]:  
> >> On Fri, 18 Jan 2019 20:38:47 +0100
> >> Andreas Kemnade <andreas@kemnade.info> wrote:
> >>  
> >>> Hi,
> >>>
> >>> On Fri, 18 Jan 2019 10:36:30 -0800
> >>> Tony Lindgren <tony@atomide.com> wrote:
> >>>
> >>> [...]  
> >>>> til the next workaround.
> >>>>      
> >>>>> That flags also causes the iclk being enabled/disabled
> >>>>> manually.  
> >>>>
> >>>> Yes but SWSUP_IDLE for the interface clock to me currently
> >>>> just means:
> >>>>
> >>>> "manually enable and disable ocp interface clock"
> >>>>      
> >>> well, if we want to manually disable it and not automatically,
> >>> we have to disable autoidle or it will be automatically disabled.
> >>>
> >>> Disabling it manually when it is already auto-disabled (by autoidle) is
> >>> just practically a no-op towards the clock.
> >>>  
> >>>> and with your changes it becomes:
> >>>>
> >>>> "manually enable and disable ocp interface clock and block
> >>>>   autoidle while in use".
> >>>>
> >>>> So aren't we now changing the way things behave in general
> >>>> for SWSUP_IDLE?
> >>>>      
> >>> Yes, we are, so proper testing is needed. But If I read those comments
> >>> it was always intended this way but not fully implemented because it
> >>> appeared to be more work like needing a usecounter (which my patchset
> >>> also adds) for that autoidle flag.
> >>>  
> >> and there are quite few hwmods marked by this flag.
> >> And then there are those clocks marked by this flags (on am33xx) which
> >> do not have that autoidle feature at all, so the risk is not too high.  
> > 
> > Keerthy, can you please test this series on top of the
> > related clock patches with your am335x PM test cases?  
> 
> Can you point me to the clock series that needs to be tested
> along with this?
> 

https://patchwork.kernel.org/project/linux-clk/list/?series=66691

Regards,
Andreas


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

Re: [Letux-kernel] [PATCH v3 3/3] arm: omap_hwmod disable ick autoidling when a hwmod requires that

[J, KEERTHY]

On 1/19/2019 12:42 PM, Andreas Kemnade wrote:
> On Sat, 19 Jan 2019 12:09:48 +0530
> "J, KEERTHY" <j-keerthy@ti.com> wrote:
> 
>> On 1/19/2019 1:18 AM, Tony Lindgren wrote:
>>> * Andreas Kemnade <andreas@kemnade.info> [190118 19:42]:
>>>> On Fri, 18 Jan 2019 20:38:47 +0100
>>>> Andreas Kemnade <andreas@kemnade.info> wrote:
>>>>   
>>>>> Hi,
>>>>>
>>>>> On Fri, 18 Jan 2019 10:36:30 -0800
>>>>> Tony Lindgren <tony@atomide.com> wrote:
>>>>>
>>>>> [...]
>>>>>> til the next workaround.
>>>>>>       
>>>>>>> That flags also causes the iclk being enabled/disabled
>>>>>>> manually.
>>>>>>
>>>>>> Yes but SWSUP_IDLE for the interface clock to me currently
>>>>>> just means:
>>>>>>
>>>>>> "manually enable and disable ocp interface clock"
>>>>>>       
>>>>> well, if we want to manually disable it and not automatically,
>>>>> we have to disable autoidle or it will be automatically disabled.
>>>>>
>>>>> Disabling it manually when it is already auto-disabled (by autoidle) is
>>>>> just practically a no-op towards the clock.
>>>>>   
>>>>>> and with your changes it becomes:
>>>>>>
>>>>>> "manually enable and disable ocp interface clock and block
>>>>>>    autoidle while in use".
>>>>>>
>>>>>> So aren't we now changing the way things behave in general
>>>>>> for SWSUP_IDLE?
>>>>>>       
>>>>> Yes, we are, so proper testing is needed. But If I read those comments
>>>>> it was always intended this way but not fully implemented because it
>>>>> appeared to be more work like needing a usecounter (which my patchset
>>>>> also adds) for that autoidle flag.
>>>>>   
>>>> and there are quite few hwmods marked by this flag.
>>>> And then there are those clocks marked by this flags (on am33xx) which
>>>> do not have that autoidle feature at all, so the risk is not too high.
>>>
>>> Keerthy, can you please test this series on top of the
>>> related clock patches with your am335x PM test cases?
>>
>> Can you point me to the clock series that needs to be tested
>> along with this?
>>
> 
> https://patchwork.kernel.org/project/linux-clk/list/?series=66691

Thanks Andreas. I will test both series and get back.

> 
> Regards,
> Andreas
> 

Re: [PATCH v3 3/3] arm: omap_hwmod disable ick autoidling when a hwmod requires that

[Tero Kristo]

On 18/01/2019 21:45, Tony Lindgren wrote:
> * Andreas Kemnade <andreas@kemnade.info> [190118 19:39]:
>> Hi,
>>
>> On Fri, 18 Jan 2019 10:36:30 -0800
>> Tony Lindgren <tony@atomide.com> wrote:
>>
>> [...]
>>> til the next workaround.
>>>
>>>> That flags also causes the iclk being enabled/disabled
>>>> manually.
>>>
>>> Yes but SWSUP_IDLE for the interface clock to me currently
>>> just means:
>>>
>>> "manually enable and disable ocp interface clock"
>>>
>> well, if we want to manually disable it and not automatically,
>> we have to disable autoidle or it will be automatically disabled.
>>
>> Disabling it manually when it is already auto-disabled (by autoidle) is
>> just practically a no-op towards the clock.
> 
> OK I buy that :) It should be probably added to the patch
> description to make it clear what it changes.
> 
> Tero, any comments on this change?

Well, seeing the flag is pretty much only used for a handful of hwmods 
nowadays, it should be fine. OMAP3 PM should be tested with this change 
though, as there are couple of hwmods impacted on that platform. I 
wonder what happens to cpuidle when display is active...

-Tero

> 
>>> and with your changes it becomes:
>>>
>>> "manually enable and disable ocp interface clock and block
>>>   autoidle while in use".
>>>
>>> So aren't we now changing the way things behave in general
>>> for SWSUP_IDLE?
>>>
>> Yes, we are, so proper testing is needed. But If I read those comments
>> it was always intended this way but not fully implemented because it
>> appeared to be more work like needing a usecounter (which my patchset
>> also adds) for that autoidle flag.
> 
> OK yeah the use count seems necessary. I'll test here
> with my PM use cases.
> 
> Regards,
> 
> Tony
> 

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

Re: [PATCH v3 3/3] arm: omap_hwmod disable ick autoidling when a hwmod requires that

[Tony Lindgren]

* Tero Kristo <t-kristo@ti.com> [190121 07:13]:
> On 18/01/2019 21:45, Tony Lindgren wrote:
> > * Andreas Kemnade <andreas@kemnade.info> [190118 19:39]:
> > > Hi,
> > > 
> > > On Fri, 18 Jan 2019 10:36:30 -0800
> > > Tony Lindgren <tony@atomide.com> wrote:
> > > 
> > > [...]
> > > > til the next workaround.
> > > > 
> > > > > That flags also causes the iclk being enabled/disabled
> > > > > manually.
> > > > 
> > > > Yes but SWSUP_IDLE for the interface clock to me currently
> > > > just means:
> > > > 
> > > > "manually enable and disable ocp interface clock"
> > > > 
> > > well, if we want to manually disable it and not automatically,
> > > we have to disable autoidle or it will be automatically disabled.
> > > 
> > > Disabling it manually when it is already auto-disabled (by autoidle) is
> > > just practically a no-op towards the clock.
> > 
> > OK I buy that :) It should be probably added to the patch
> > description to make it clear what it changes.
> > 
> > Tero, any comments on this change?
> 
> Well, seeing the flag is pretty much only used for a handful of hwmods
> nowadays, it should be fine. OMAP3 PM should be tested with this change
> though, as there are couple of hwmods impacted on that platform. I wonder
> what happens to cpuidle when display is active...

OK so that's a good test case. AFAIK, we should have DSS idle
and have the SoC hit at least core retention with DSI command mode
displays. I don't know if this patch would block DSS autoidle then
or not.. I'm guessing 80% chance that we still need DSS hit runtime
suspend to enter SoC idle states meaning this patch would not
affect it :)

> > > > and with your changes it becomes:
> > > > 
> > > > "manually enable and disable ocp interface clock and block
> > > >   autoidle while in use".
> > > > 
> > > > So aren't we now changing the way things behave in general
> > > > for SWSUP_IDLE?
> > > > 
> > > Yes, we are, so proper testing is needed. But If I read those comments
> > > it was always intended this way but not fully implemented because it
> > > appeared to be more work like needing a usecounter (which my patchset
> > > also adds) for that autoidle flag.
> > 
> > OK yeah the use count seems necessary. I'll test here
> > with my PM use cases.

Regards,

Tony

Re: [PATCH v3 3/3] arm: omap_hwmod disable ick autoidling when a hwmod requires that

[Andreas Kemnade]

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

Hi,

On Mon, 21 Jan 2019 09:07:43 -0800
Tony Lindgren <tony@atomide.com> wrote:

> * Tero Kristo <t-kristo@ti.com> [190121 07:13]:
> > On 18/01/2019 21:45, Tony Lindgren wrote:  
> > > * Andreas Kemnade <andreas@kemnade.info> [190118 19:39]:  
> > > > Hi,
> > > > 
> > > > On Fri, 18 Jan 2019 10:36:30 -0800
> > > > Tony Lindgren <tony@atomide.com> wrote:
> > > > 
> > > > [...]  
> > > > > til the next workaround.
> > > > >   
> > > > > > That flags also causes the iclk being enabled/disabled
> > > > > > manually.  
> > > > > 
> > > > > Yes but SWSUP_IDLE for the interface clock to me currently
> > > > > just means:
> > > > > 
> > > > > "manually enable and disable ocp interface clock"
> > > > >   
> > > > well, if we want to manually disable it and not automatically,
> > > > we have to disable autoidle or it will be automatically disabled.
> > > > 
> > > > Disabling it manually when it is already auto-disabled (by autoidle) is
> > > > just practically a no-op towards the clock.  
> > > 
> > > OK I buy that :) It should be probably added to the patch
> > > description to make it clear what it changes.
> > > 
> > > Tero, any comments on this change?  
> > 
> > Well, seeing the flag is pretty much only used for a handful of hwmods
> > nowadays, it should be fine. OMAP3 PM should be tested with this change
> > though, as there are couple of hwmods impacted on that platform. I wonder
> > what happens to cpuidle when display is active...  
> 
> OK so that's a good test case. AFAIK, we should have DSS idle
> and have the SoC hit at least core retention with DSI command mode
> displays. I don't know if this patch would block DSS autoidle then
> or not.. I'm guessing 80% chance that we still need DSS hit runtime
> suspend to enter SoC idle states meaning this patch would not
> affect it :)
> 
So this is a call to Nokia N950 owners?
Unfortunately, I do not have one. I have seen on non-command-mode
displays that dss goes to idle when display is blanked.

Regards,
Andreas

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

Re: [PATCH v3 3/3] arm: omap_hwmod disable ick autoidling when a hwmod requires that

[Tony Lindgren]

* Andreas Kemnade <andreas@kemnade.info> [190121 17:54]:
> Hi,
> 
> On Mon, 21 Jan 2019 09:07:43 -0800
> Tony Lindgren <tony@atomide.com> wrote:
> 
> > * Tero Kristo <t-kristo@ti.com> [190121 07:13]:
> > > On 18/01/2019 21:45, Tony Lindgren wrote:  
> > > > * Andreas Kemnade <andreas@kemnade.info> [190118 19:39]:  
> > > > > Hi,
> > > > > 
> > > > > On Fri, 18 Jan 2019 10:36:30 -0800
> > > > > Tony Lindgren <tony@atomide.com> wrote:
> > > > > 
> > > > > [...]  
> > > > > > til the next workaround.
> > > > > >   
> > > > > > > That flags also causes the iclk being enabled/disabled
> > > > > > > manually.  
> > > > > > 
> > > > > > Yes but SWSUP_IDLE for the interface clock to me currently
> > > > > > just means:
> > > > > > 
> > > > > > "manually enable and disable ocp interface clock"
> > > > > >   
> > > > > well, if we want to manually disable it and not automatically,
> > > > > we have to disable autoidle or it will be automatically disabled.
> > > > > 
> > > > > Disabling it manually when it is already auto-disabled (by autoidle) is
> > > > > just practically a no-op towards the clock.  
> > > > 
> > > > OK I buy that :) It should be probably added to the patch
> > > > description to make it clear what it changes.
> > > > 
> > > > Tero, any comments on this change?  
> > > 
> > > Well, seeing the flag is pretty much only used for a handful of hwmods
> > > nowadays, it should be fine. OMAP3 PM should be tested with this change
> > > though, as there are couple of hwmods impacted on that platform. I wonder
> > > what happens to cpuidle when display is active...  
> > 
> > OK so that's a good test case. AFAIK, we should have DSS idle
> > and have the SoC hit at least core retention with DSI command mode
> > displays. I don't know if this patch would block DSS autoidle then
> > or not.. I'm guessing 80% chance that we still need DSS hit runtime
> > suspend to enter SoC idle states meaning this patch would not
> > affect it :)
> > 
> So this is a call to Nokia N950 owners?

Well sort of.. But the LCD command mode patches are still pending.
I've added Aaro and Sebastian to Cc in case they have it testable.

> Unfortunately, I do not have one. I have seen on non-command-mode
> displays that dss goes to idle when display is blanked.

OK. And I did a brief test on droid4 with the pending DSI
command mode patches by sprinkling some OCPIF_SWSUP_IDLE
to DSS and DSI with the hack below (that is not needed for
normal use). Things idle just fine for me when LCD is on
and I can see the SoC hit core retention the same way as
without the test patch below.

So as far as I'm concerned, these patches are good to go
and I'll go ack the patches.

Thanks,

Tony

8< --------------------
--- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
@@ -3424,6 +3424,7 @@ static struct omap_hwmod_ocp_if omap44xx_l3_main_2__dss = {
 	.slave		= &omap44xx_dss_hwmod,
 	.clk		= "l3_div_ck",
 	.user		= OCP_USER_SDMA,
+	.flags		= OCPIF_SWSUP_IDLE,
 };
 
 /* l4_per -> dss */
@@ -3472,6 +3473,7 @@ static struct omap_hwmod_ocp_if omap44xx_l3_main_2__dss_dsi2 = {
 	.slave		= &omap44xx_dss_dsi2_hwmod,
 	.clk		= "l3_div_ck",
 	.user		= OCP_USER_SDMA,
+	.flags		= OCPIF_SWSUP_IDLE,
 };
 
 /* l4_per -> dss_dsi2 */
@@ -3480,6 +3482,7 @@ static struct omap_hwmod_ocp_if omap44xx_l4_per__dss_dsi2 = {
 	.slave		= &omap44xx_dss_dsi2_hwmod,
 	.clk		= "l4_div_ck",
 	.user		= OCP_USER_MPU,
+	.flags		= OCPIF_SWSUP_IDLE,
 };
 
 /* l3_main_2 -> dss_hdmi */

Re: [PATCH v3 0/3] mach-omap2: handle autoidle denial

[Tony Lindgren]

* Andreas Kemnade <andreas@kemnade.info> [190116 14:04]:
> 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.
> 
> To handle multiple users of a single ick, autoidle is disabled
> when a user of that ick requires that (has the OCPIF_SWSUP_IDLE))
> 
> Changes v3:
> - replace CLK_IS_BASIC
> 
> Changes v2:
> - uses spinlocks instead of mutexes
> - invert counter logic
> - check whether clock type is basic

For this series it's best to merge it all via the
clock tree along with the related clock patches:

Acked-by: Tony Lindgren <tony@atomide.com>

Re: [Letux-kernel] [PATCH v3 3/3] arm: omap_hwmod disable ick autoidling when a hwmod requires that

[Keerthy]

On 19/01/19 1:28 PM, J, KEERTHY wrote:
> 
> 
> On 1/19/2019 12:42 PM, Andreas Kemnade wrote:
>> On Sat, 19 Jan 2019 12:09:48 +0530
>> "J, KEERTHY" <j-keerthy@ti.com> wrote:
>>
>>> On 1/19/2019 1:18 AM, Tony Lindgren wrote:
>>>> * Andreas Kemnade <andreas@kemnade.info> [190118 19:42]:
>>>>> On Fri, 18 Jan 2019 20:38:47 +0100
>>>>> Andreas Kemnade <andreas@kemnade.info> wrote:
>>>>>  
>>>>>> Hi,
>>>>>>
>>>>>> On Fri, 18 Jan 2019 10:36:30 -0800
>>>>>> Tony Lindgren <tony@atomide.com> wrote:
>>>>>>
>>>>>> [...]
>>>>>>> til the next workaround.
>>>>>>>      
>>>>>>>> That flags also causes the iclk being enabled/disabled
>>>>>>>> manually.
>>>>>>>
>>>>>>> Yes but SWSUP_IDLE for the interface clock to me currently
>>>>>>> just means:
>>>>>>>
>>>>>>> "manually enable and disable ocp interface clock"
>>>>>>>       
>>>>>> well, if we want to manually disable it and not automatically,
>>>>>> we have to disable autoidle or it will be automatically disabled.
>>>>>>
>>>>>> Disabling it manually when it is already auto-disabled (by
>>>>>> autoidle) is
>>>>>> just practically a no-op towards the clock.
>>>>>>  
>>>>>>> and with your changes it becomes:
>>>>>>>
>>>>>>> "manually enable and disable ocp interface clock and block
>>>>>>>    autoidle while in use".
>>>>>>>
>>>>>>> So aren't we now changing the way things behave in general
>>>>>>> for SWSUP_IDLE?
>>>>>>>       
>>>>>> Yes, we are, so proper testing is needed. But If I read those
>>>>>> comments
>>>>>> it was always intended this way but not fully implemented because it
>>>>>> appeared to be more work like needing a usecounter (which my patchset
>>>>>> also adds) for that autoidle flag.
>>>>>>   
>>>>> and there are quite few hwmods marked by this flag.
>>>>> And then there are those clocks marked by this flags (on am33xx) which
>>>>> do not have that autoidle feature at all, so the risk is not too high.
>>>>
>>>> Keerthy, can you please test this series on top of the
>>>> related clock patches with your am335x PM test cases?
>>>
>>> Can you point me to the clock series that needs to be tested
>>> along with this?
>>>
>>
>> https://patchwork.kernel.org/project/linux-clk/list/?series=66691
> 
> Thanks Andreas. I will test both series and get back.

Tested for DS0 (deeps sleep 0 on am33/am43 boards) No issues seen with
the current patch series on top of clock series.

Tested-by: Keerthy <j-keerthy@ti.com>

> 
>>
>> Regards,
>> Andreas
>>

Re: [PATCH v3 0/3] mach-omap2: handle autoidle denial

[Andreas Kemnade]

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

On Mon, 21 Jan 2019 11:58:03 -0800
Tony Lindgren <tony@atomide.com> wrote:

> * Andreas Kemnade <andreas@kemnade.info> [190116 14:04]:
> > 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.
> > 
> > To handle multiple users of a single ick, autoidle is disabled
> > when a user of that ick requires that (has the OCPIF_SWSUP_IDLE))
> > 
> > Changes v3:
> > - replace CLK_IS_BASIC
> > 
> > Changes v2:
> > - uses spinlocks instead of mutexes
> > - invert counter logic
> > - check whether clock type is basic  
> 
> For this series it's best to merge it all via the
> clock tree along with the related clock patches:
> 
> Acked-by: Tony Lindgren <tony@atomide.com>
> 
hmm, this is stalled. Have I missed any new objections?

Regards,
Andreas

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

Re: [PATCH v3 0/3] mach-omap2: handle autoidle denial

[Tero Kristo]

On 09/02/2019 20:53, Andreas Kemnade wrote:
> On Mon, 21 Jan 2019 11:58:03 -0800
> Tony Lindgren <tony@atomide.com> wrote:
> 
>> * Andreas Kemnade <andreas@kemnade.info> [190116 14:04]:
>>> 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.
>>>
>>> To handle multiple users of a single ick, autoidle is disabled
>>> when a user of that ick requires that (has the OCPIF_SWSUP_IDLE))
>>>
>>> Changes v3:
>>> - replace CLK_IS_BASIC
>>>
>>> Changes v2:
>>> - uses spinlocks instead of mutexes
>>> - invert counter logic
>>> - check whether clock type is basic
>>
>> For this series it's best to merge it all via the
>> clock tree along with the related clock patches:
>>
>> Acked-by: Tony Lindgren <tony@atomide.com>
>>
> hmm, this is stalled. Have I missed any new objections?

Sorry, I've just been pretty busy as usual, queued up for 5.1 now, thanks.

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