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

Commit Message

Andreas Kemnade Jan. 16, 2019, 10:04 p.m. UTC

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 <[e-mail]>
---
 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(-)

Comments

Tony Lindgren Jan. 18, 2019, 3:48 p.m. UTC | #1

Hi,

* Andreas Kemnade <[e-mail]> [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

Andreas Kemnade Jan. 18, 2019, 5:18 p.m. UTC | #2

Hi,

On Fri, 18 Jan 2019 07:48:07 -0800
Tony Lindgren <[e-mail]> wrote:

> Hi,
> 
> * Andreas Kemnade <[e-mail]> [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

Tony Lindgren Jan. 18, 2019, 6:36 p.m. UTC | #3

* Andreas Kemnade <[e-mail]> [190118 17:18]:
> On Fri, 18 Jan 2019 07:48:07 -0800
> Tony Lindgren <[e-mail]> wrote:
> > * Andreas Kemnade <[e-mail]> [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

Andreas Kemnade Jan. 18, 2019, 7:38 p.m. UTC | #4

Hi,

On Fri, 18 Jan 2019 10:36:30 -0800
Tony Lindgren <[e-mail]> 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

Andreas Kemnade Jan. 18, 2019, 7:42 p.m. UTC | #5

On Fri, 18 Jan 2019 20:38:47 +0100
Andreas Kemnade <[e-mail]> wrote:

> Hi,
> 
> On Fri, 18 Jan 2019 10:36:30 -0800
> Tony Lindgren <[e-mail]> 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

Tony Lindgren Jan. 18, 2019, 7:45 p.m. UTC | #6

* Andreas Kemnade <[e-mail]> [190118 19:39]:
> Hi,
> 
> On Fri, 18 Jan 2019 10:36:30 -0800
> Tony Lindgren <[e-mail]> 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

Tony Lindgren Jan. 18, 2019, 7:48 p.m. UTC | #7

* Andreas Kemnade <[e-mail]> [190118 19:42]:
> On Fri, 18 Jan 2019 20:38:47 +0100
> Andreas Kemnade <[e-mail]> wrote:
> 
> > Hi,
> > 
> > On Fri, 18 Jan 2019 10:36:30 -0800
> > Tony Lindgren <[e-mail]> 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

J, KEERTHY Jan. 19, 2019, 6:39 a.m. UTC | #8

On 1/19/2019 1:18 AM, Tony Lindgren wrote:
> * Andreas Kemnade <[e-mail]> [190118 19:42]:
>> On Fri, 18 Jan 2019 20:38:47 +0100
>> Andreas Kemnade <[e-mail]> wrote:
>>
>>> Hi,
>>>
>>> On Fri, 18 Jan 2019 10:36:30 -0800
>>> Tony Lindgren <[e-mail]> 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
> 
>

Andreas Kemnade Jan. 19, 2019, 7:12 a.m. UTC | #9

On Sat, 19 Jan 2019 12:09:48 +0530
"J, KEERTHY" <[e-mail]> wrote:

> On 1/19/2019 1:18 AM, Tony Lindgren wrote:
> > * Andreas Kemnade <[e-mail]> [190118 19:42]:  
> >> On Fri, 18 Jan 2019 20:38:47 +0100
> >> Andreas Kemnade <[e-mail]> wrote:
> >>  
> >>> Hi,
> >>>
> >>> On Fri, 18 Jan 2019 10:36:30 -0800
> >>> Tony Lindgren <[e-mail]> 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

J, KEERTHY Jan. 19, 2019, 7:58 a.m. UTC | #10

On 1/19/2019 12:42 PM, Andreas Kemnade wrote:
> On Sat, 19 Jan 2019 12:09:48 +0530
> "J, KEERTHY" <[e-mail]> wrote:
> 
>> On 1/19/2019 1:18 AM, Tony Lindgren wrote:
>>> * Andreas Kemnade <[e-mail]> [190118 19:42]:
>>>> On Fri, 18 Jan 2019 20:38:47 +0100
>>>> Andreas Kemnade <[e-mail]> wrote:
>>>>   
>>>>> Hi,
>>>>>
>>>>> On Fri, 18 Jan 2019 10:36:30 -0800
>>>>> Tony Lindgren <[e-mail]> 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
>

Tero Kristo Jan. 21, 2019, 7:12 a.m. UTC | #11

On 18/01/2019 21:45, Tony Lindgren wrote:
> * Andreas Kemnade <[e-mail]> [190118 19:39]:
>> Hi,
>>
>> On Fri, 18 Jan 2019 10:36:30 -0800
>> Tony Lindgren <[e-mail]> 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

Tony Lindgren Jan. 21, 2019, 5:07 p.m. UTC | #12

* Tero Kristo <[e-mail]> [190121 07:13]:
> On 18/01/2019 21:45, Tony Lindgren wrote:
> > * Andreas Kemnade <[e-mail]> [190118 19:39]:
> > > Hi,
> > > 
> > > On Fri, 18 Jan 2019 10:36:30 -0800
> > > Tony Lindgren <[e-mail]> 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

Andreas Kemnade Jan. 21, 2019, 5:53 p.m. UTC | #13

Hi,

On Mon, 21 Jan 2019 09:07:43 -0800
Tony Lindgren <[e-mail]> wrote:

> * Tero Kristo <[e-mail]> [190121 07:13]:
> > On 18/01/2019 21:45, Tony Lindgren wrote:  
> > > * Andreas Kemnade <[e-mail]> [190118 19:39]:  
> > > > Hi,
> > > > 
> > > > On Fri, 18 Jan 2019 10:36:30 -0800
> > > > Tony Lindgren <[e-mail]> 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

Tony Lindgren Jan. 21, 2019, 7:56 p.m. UTC | #14

* Andreas Kemnade <[e-mail]> [190121 17:54]:
> Hi,
> 
> On Mon, 21 Jan 2019 09:07:43 -0800
> Tony Lindgren <[e-mail]> wrote:
> 
> > * Tero Kristo <[e-mail]> [190121 07:13]:
> > > On 18/01/2019 21:45, Tony Lindgren wrote:  
> > > > * Andreas Kemnade <[e-mail]> [190118 19:39]:  
> > > > > Hi,
> > > > > 
> > > > > On Fri, 18 Jan 2019 10:36:30 -0800
> > > > > Tony Lindgren <[e-mail]> 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 */

J, KEERTHY Jan. 22, 2019, 6:26 a.m. UTC | #15

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" <[e-mail]> wrote:
>>
>>> On 1/19/2019 1:18 AM, Tony Lindgren wrote:
>>>> * Andreas Kemnade <[e-mail]> [190118 19:42]:
>>>>> On Fri, 18 Jan 2019 20:38:47 +0100
>>>>> Andreas Kemnade <[e-mail]> wrote:
>>>>>  
>>>>>> Hi,
>>>>>>
>>>>>> On Fri, 18 Jan 2019 10:36:30 -0800
>>>>>> Tony Lindgren <[e-mail]> 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 <[e-mail]>

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

Patch

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);
 		}
 	}