[v4,1/2] ARM: keystone: pm: switch to use generic pm domains

Commit Message

Grygorii Strashko Nov. 10, 2014, 2:59 p.m. UTC

This patch switches Keystone 2 PM code to use Generic PM domains
instead of PM clock domains because of the lack of DT support
for the last.

CC: Ulf Hansson <[e-mail]>
CC: Geert Uytterhoeven <[e-mail]>
CC: Dmitry Torokhov <[e-mail]>
Reviewed-by: Santosh Shilimkar <[e-mail]>
Reviewed-by: Kevin Hilman <[e-mail]>
Signed-off-by: Grygorii Strashko <[e-mail]>
---
 .../bindings/power/ti,keystone-powerdomain.txt     |  31 ++++++
 arch/arm/mach-keystone/Kconfig                     |   1 +
 arch/arm/mach-keystone/pm_domain.c                 | 106 ++++++++++++++-------
 3 files changed, 101 insertions(+), 37 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt

Comments

Arnd Bergmann Nov. 10, 2014, 3:06 p.m. UTC | #1

On Monday 10 November 2014 16:59:16 Grygorii Strashko wrote:
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt
> @@ -0,0 +1,31 @@
> +* TI Keystone 2 Generic PM Controller
> +
> +The TI Keystone 2 Generic PM Controller is responsible for Clock gating
> +for each controlled IP module.
> +
> +Required properties:
> +- compatible: Should be "ti,keystone-powerdomain"
> +- #power-domain-cells: Should be 0, see below:
> +
> +The PM Controller node is a PM domain as documented in
> +Documentation/devicetree/bindings/power/power_domain.txt.
> +
> +Example:
> +
> +       pm_controller: pm-controller {
> +               compatible = "ti,keystone-powerdomain";
> +               #power-domain-cells = <0>;
> +       };
> +
> +       netcp: netcp@2090000 {
> +               reg = <0x2620110 0x8>;
> +               reg-names = "efuse";
> +               ...
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +               ranges;
> +               power-domains = <&pm_controller>;
> +
> +               clocks = <&clkpa>, <&clkcpgmac>, <&chipclk12>;
> +               dma-coherent;
> +       }

I don't get it. What keystone specific about a "ti,keystone-powerdomain"
device? It seems that the device has no registers whatsoever and the
driver doesn't really do anything that relates to the platform.

	Arnd

Grygorii Strashko Nov. 10, 2014, 5:38 p.m. UTC | #2

Hi Arnd,

On 11/10/2014 05:06 PM, Arnd Bergmann wrote:
> On Monday 10 November 2014 16:59:16 Grygorii Strashko wrote:
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt
>> @@ -0,0 +1,31 @@
>> +* TI Keystone 2 Generic PM Controller
>> +
>> +The TI Keystone 2 Generic PM Controller is responsible for Clock gating
>> +for each controlled IP module.
>> +
>> +Required properties:
>> +- compatible: Should be "ti,keystone-powerdomain"
>> +- #power-domain-cells: Should be 0, see below:
>> +
>> +The PM Controller node is a PM domain as documented in
>> +Documentation/devicetree/bindings/power/power_domain.txt.
>> +
>> +Example:
>> +
>> +       pm_controller: pm-controller {
>> +               compatible = "ti,keystone-powerdomain";
>> +               #power-domain-cells = <0>;
>> +       };
>> +
>> +       netcp: netcp@2090000 {
>> +               reg = <0x2620110 0x8>;
>> +               reg-names = "efuse";
>> +               ...
>> +               #address-cells = <1>;
>> +               #size-cells = <1>;
>> +               ranges;
>> +               power-domains = <&pm_controller>;
>> +
>> +               clocks = <&clkpa>, <&clkcpgmac>, <&chipclk12>;
>> +               dma-coherent;
>> +       }
> 
> I don't get it. What keystone specific about a "ti,keystone-powerdomain"
> device? It seems that the device has no registers whatsoever and the
> driver doesn't really do anything that relates to the platform.

That's true. but it was the only one acceptable way  to enable
Generic clock manipulation PM callbacks for the DT-boot case.
After several unsuccessful attempts the idea to use GPD
was introduced by Kevin there:
  https://lkml.org/lkml/2014/9/8/643

So, The Keystone 2 Generic PM Controller is just a proxy PM layer here between
device and Generic clock manipulation PM callbacks.
It fills per-device clock list when device is attached to GPD and
ensures that all clocks from that list enabled/disabled when device is
started/stopped.

Regards,
-grygorii

Arnd Bergmann Nov. 10, 2014, 8:36 p.m. UTC | #3

On Monday 10 November 2014 19:38:14 Grygorii Strashko wrote:
> Hi Arnd,
> 
> On 11/10/2014 05:06 PM, Arnd Bergmann wrote:
> > On Monday 10 November 2014 16:59:16 Grygorii Strashko wrote:
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt
> >> @@ -0,0 +1,31 @@
> >> +* TI Keystone 2 Generic PM Controller
> >> +
> >> +The TI Keystone 2 Generic PM Controller is responsible for Clock gating
> >> +for each controlled IP module.
> >> +
> >> +Required properties:
> >> +- compatible: Should be "ti,keystone-powerdomain"
> >> +- #power-domain-cells: Should be 0, see below:
> >> +
> >> +The PM Controller node is a PM domain as documented in
> >> +Documentation/devicetree/bindings/power/power_domain.txt.
> >> +
> >> +Example:
> >> +
> >> +       pm_controller: pm-controller {
> >> +               compatible = "ti,keystone-powerdomain";
> >> +               #power-domain-cells = <0>;
> >> +       };
> >> +
> >> +       netcp: netcp@2090000 {
> >> +               reg = <0x2620110 0x8>;
> >> +               reg-names = "efuse";
> >> +               ...
> >> +               #address-cells = <1>;
> >> +               #size-cells = <1>;
> >> +               ranges;
> >> +               power-domains = <&pm_controller>;
> >> +
> >> +               clocks = <&clkpa>, <&clkcpgmac>, <&chipclk12>;
> >> +               dma-coherent;
> >> +       }
> > 
> > I don't get it. What keystone specific about a "ti,keystone-powerdomain"
> > device? It seems that the device has no registers whatsoever and the
> > driver doesn't really do anything that relates to the platform.
> 
> That's true. but it was the only one acceptable way  to enable
> Generic clock manipulation PM callbacks for the DT-boot case.
> After several unsuccessful attempts the idea to use GPD
> was introduced by Kevin there:
>   https://lkml.org/lkml/2014/9/8/643
> 
> So, The Keystone 2 Generic PM Controller is just a proxy PM layer here between
> device and Generic clock manipulation PM callbacks.
> It fills per-device clock list when device is attached to GPD and
> ensures that all clocks from that list enabled/disabled when device is
> started/stopped.

The idea of such a generic power domain implementation sounds useful, but
it has absolutely no business in platform specific code.

I suggest you either remove the power domain proxy from your drivers
and use the clocks directly, or come up with an implementation that can
be used across other platforms and CPU architectures.

	Arnd

Kevin Hilman Nov. 17, 2014, 7:14 p.m. UTC | #4

Arnd Bergmann <[e-mail]> writes:

> On Monday 10 November 2014 19:38:14 Grygorii Strashko wrote:
>> Hi Arnd,
>> 
>> On 11/10/2014 05:06 PM, Arnd Bergmann wrote:
>> > On Monday 10 November 2014 16:59:16 Grygorii Strashko wrote:
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt
>> >> @@ -0,0 +1,31 @@
>> >> +* TI Keystone 2 Generic PM Controller
>> >> +
>> >> +The TI Keystone 2 Generic PM Controller is responsible for Clock gating
>> >> +for each controlled IP module.
>> >> +
>> >> +Required properties:
>> >> +- compatible: Should be "ti,keystone-powerdomain"
>> >> +- #power-domain-cells: Should be 0, see below:
>> >> +
>> >> +The PM Controller node is a PM domain as documented in
>> >> +Documentation/devicetree/bindings/power/power_domain.txt.
>> >> +
>> >> +Example:
>> >> +
>> >> +       pm_controller: pm-controller {
>> >> +               compatible = "ti,keystone-powerdomain";
>> >> +               #power-domain-cells = <0>;
>> >> +       };
>> >> +
>> >> +       netcp: netcp@2090000 {
>> >> +               reg = <0x2620110 0x8>;
>> >> +               reg-names = "efuse";
>> >> +               ...
>> >> +               #address-cells = <1>;
>> >> +               #size-cells = <1>;
>> >> +               ranges;
>> >> +               power-domains = <&pm_controller>;
>> >> +
>> >> +               clocks = <&clkpa>, <&clkcpgmac>, <&chipclk12>;
>> >> +               dma-coherent;
>> >> +       }
>> > 
>> > I don't get it. What keystone specific about a "ti,keystone-powerdomain"
>> > device? It seems that the device has no registers whatsoever and the
>> > driver doesn't really do anything that relates to the platform.
>> 
>> That's true. but it was the only one acceptable way  to enable
>> Generic clock manipulation PM callbacks for the DT-boot case.
>> After several unsuccessful attempts the idea to use GPD
>> was introduced by Kevin there:
>>   https://lkml.org/lkml/2014/9/8/643
>> 
>> So, The Keystone 2 Generic PM Controller is just a proxy PM layer here between
>> device and Generic clock manipulation PM callbacks.
>> It fills per-device clock list when device is attached to GPD and
>> ensures that all clocks from that list enabled/disabled when device is
>> started/stopped.
>
> The idea of such a generic power domain implementation sounds useful, but
> it has absolutely no business in platform specific code.

Yes it does.  This isn't a generic power domain implementation, but
rather just the platform-specific glue that hooks up the clocks to the
right devices and power-domains so that the generic power-domain and
generic pm_clocks code does the right thing.

> I suggest you either remove the power domain proxy from your drivers
> and use the clocks directly, 

No.  That's a step in the wrong direction.  This change isn't affecting
drivers directly.  It's the runtime PM and generic power domain layers
that handle this, and runtime PM adapted drivers don't need any changes.
 
> or come up with an implementation that can be used across other
> platforms and CPU architectures.

We already have those in the generic power domain and the pm_clock
layers.  This series is just hooking those up for Keystone.

Kevin

Arnd Bergmann Nov. 17, 2014, 8:37 p.m. UTC | #5

On Monday 17 November 2014 11:14:16 Kevin Hilman wrote:
> >> 
> >> So, The Keystone 2 Generic PM Controller is just a proxy PM layer here between
> >> device and Generic clock manipulation PM callbacks.
> >> It fills per-device clock list when device is attached to GPD and
> >> ensures that all clocks from that list enabled/disabled when device is
> >> started/stopped.
> >
> > The idea of such a generic power domain implementation sounds useful, but
> > it has absolutely no business in platform specific code.
> 
> Yes it does.  This isn't a generic power domain implementation, but
> rather just the platform-specific glue that hooks up the clocks to the
> right devices and power-domains so that the generic power-domain and
> generic pm_clocks code does the right thing.

How would you do this on an arm64 version of keystone then? With
the current approach, you'd need to add a machine specific directory,
and that seems completely pointless since this is not even about
a hardware requirement.

> > I suggest you either remove the power domain proxy from your drivers
> > and use the clocks directly, 
> 
> No.  That's a step in the wrong direction.  This change isn't affecting
> drivers directly.  It's the runtime PM and generic power domain layers
> that handle this, and runtime PM adapted drivers don't need any changes.
>  
> > or come up with an implementation that can be used across other
> > platforms and CPU architectures.
> 
> We already have those in the generic power domain and the pm_clock
> layers.  This series is just hooking those up for Keystone.

Then why not add the missing piece to the generic power domain
code to avoid having to add infrastructure to the platform
for it?

	Arnd

Kevin Hilman Nov. 17, 2014, 9:50 p.m. UTC | #6

Arnd Bergmann <[e-mail]> writes:

> On Monday 17 November 2014 11:14:16 Kevin Hilman wrote:
>> >> 
>> >> So, The Keystone 2 Generic PM Controller is just a proxy PM layer here between
>> >> device and Generic clock manipulation PM callbacks.
>> >> It fills per-device clock list when device is attached to GPD and
>> >> ensures that all clocks from that list enabled/disabled when device is
>> >> started/stopped.
>> >
>> > The idea of such a generic power domain implementation sounds useful, but
>> > it has absolutely no business in platform specific code.
>> 
>> Yes it does.  This isn't a generic power domain implementation, but
>> rather just the platform-specific glue that hooks up the clocks to the
>> right devices and power-domains so that the generic power-domain and
>> generic pm_clocks code does the right thing.
>
> How would you do this on an arm64 version of keystone then? With
> the current approach, you'd need to add a machine specific directory,
> and that seems completely pointless since this is not even about
> a hardware requirement.

Yeah, you're right.  I misunderstood you're original comment.

>> > I suggest you either remove the power domain proxy from your drivers
>> > and use the clocks directly, 
>> 
>> No.  That's a step in the wrong direction.  This change isn't affecting
>> drivers directly.  It's the runtime PM and generic power domain layers
>> that handle this, and runtime PM adapted drivers don't need any changes.
>>  
>> > or come up with an implementation that can be used across other
>> > platforms and CPU architectures.
>> 
>> We already have those in the generic power domain and the pm_clock
>> layers.  This series is just hooking those up for Keystone.
>
> Then why not add the missing piece to the generic power domain
> code to avoid having to add infrastructure to the platform
> for it?

Yes, good point.  There is nothing keystone-specific in this glue.

Grygorii, what about adding a feature to the generic domain parsing so
that it can get clocks from device nodes that are part of the domain,
and so it sets up pm_clk accordingly.

I've recently seen other SoCs doing very similar, so this really should
be generalized.

I've been looking at this primarily as a right incremental improvement
from what is there for Keystone today, but Arnd is right.  This should
be moved out of platform code.

Kevin

Santosh Shilimkar Nov. 18, 2014, 2:18 a.m. UTC | #7

On 11/17/14 12:37 PM, Arnd Bergmann wrote:
> On Monday 17 November 2014 11:14:16 Kevin Hilman wrote:
>>>>
>>>> So, The Keystone 2 Generic PM Controller is just a proxy PM layer here between
>>>> device and Generic clock manipulation PM callbacks.
>>>> It fills per-device clock list when device is attached to GPD and
>>>> ensures that all clocks from that list enabled/disabled when device is
>>>> started/stopped.
>>>
>>> The idea of such a generic power domain implementation sounds useful, but
>>> it has absolutely no business in platform specific code.
>>
>> Yes it does.  This isn't a generic power domain implementation, but
>> rather just the platform-specific glue that hooks up the clocks to the
>> right devices and power-domains so that the generic power-domain and
>> generic pm_clocks code does the right thing.
>
> How would you do this on an arm64 version of keystone then? With
> the current approach, you'd need to add a machine specific directory,
> and that seems completely pointless since this is not even about
> a hardware requirement.
>
The Keystone PM domain code actually doesn't have to be under machine
code. Infact my first patch I added that under drivers/bus/ and later
based on Kevin's suggestion moved under machine. That time the Keystone
64 bit arch point I couldn't bring up for other reasons.

But the code itself can be easily moved to drivers/power/ without
any issue if thats what is the concern here.

Regards,
Santosh

Grygorii Strashko Nov. 18, 2014, 6:54 p.m. UTC | #8

Hi All,

Thank you for your comments.

On 11/17/2014 11:50 PM, Kevin Hilman wrote:
> Arnd Bergmann <[e-mail]> writes:
> 
>> On Monday 17 November 2014 11:14:16 Kevin Hilman wrote:
>>>>>
>>>>> So, The Keystone 2 Generic PM Controller is just a proxy PM layer here between
>>>>> device and Generic clock manipulation PM callbacks.
>>>>> It fills per-device clock list when device is attached to GPD and
>>>>> ensures that all clocks from that list enabled/disabled when device is
>>>>> started/stopped.
>>>>
>>>> The idea of such a generic power domain implementation sounds useful, but
>>>> it has absolutely no business in platform specific code.
>>>
>>> Yes it does.  This isn't a generic power domain implementation, but
>>> rather just the platform-specific glue that hooks up the clocks to the
>>> right devices and power-domains so that the generic power-domain and
>>> generic pm_clocks code does the right thing.
>>
>> How would you do this on an arm64 version of keystone then? With
>> the current approach, you'd need to add a machine specific directory,
>> and that seems completely pointless since this is not even about
>> a hardware requirement.
> 
> Yeah, you're right.  I misunderstood you're original comment.
> 
>>>> I suggest you either remove the power domain proxy from your drivers
>>>> and use the clocks directly,

Hm. I've been thinking about this, but the problem is that Keystone 2
reuses a lot of IPs from Davinci and PM for Davinci is based on Generic clock
manipulation PM callbacks framework, but for non-DT case. So, I can't simply
use clocks directly.

>>>
>>> No.  That's a step in the wrong direction.  This change isn't affecting
>>> drivers directly.  It's the runtime PM and generic power domain layers
>>> that handle this, and runtime PM adapted drivers don't need any changes.
>>>   
>>>> or come up with an implementation that can be used across other
>>>> platforms and CPU architectures.
>>>
>>> We already have those in the generic power domain and the pm_clock
>>> layers.  This series is just hooking those up for Keystone.
>>
>> Then why not add the missing piece to the generic power domain
>> code to avoid having to add infrastructure to the platform
>> for it?
> 
> Yes, good point.  There is nothing keystone-specific in this glue.
> 
> Grygorii, what about adding a feature to the generic domain parsing so
> that it can get clocks from device nodes that are part of the domain,
> and so it sets up pm_clk accordingly.

I'd like to mention few points here:
1)  not all platforms may need this

2)  not all platforms may allow to add ALL clocks from "clocks" property
   to pm_clk as some of them can be optional or have to be controlled by drivers only
   (for example, initially, it was the case for SH-mobile https://lkml.org/lkml/2014/4/24/197
    also now, last implementation for shmobile add only first clock from "clocks" property
    to pm_clk https://lkml.org/lkml/2014/11/17/272).

3)  such functionality have to be enabled for devices selectively, for example
   now we are going to enable it for devices which a ready for runtime PM.

Current implementation cover 1 & 3, but also it allows to cover 2 too, because
it's platform specific implementation and .attach_dev() can be updated to skip some 
clocks or devices if needed.

> 
> I've recently seen other SoCs doing very similar, so this really should
> be generalized.
> 
> I've been looking at this primarily as a right incremental improvement
> from what is there for Keystone today, but Arnd is right.  This should
> be moved out of platform code.

I'm ready to do what ever you want, but I don't fully understand what exactly to do :(
Should I create some generic_pm_clk_domain.c?
- or - Do you mean to integrate it in domain.c (see no way to do it:()?
- or - smth. else

What about introduced DT bindings? For example, How will devices be selected for attachment 
to Generic pm_clk domain if I'll introduce generic_pm_clk_domain.c?

Regards,
-grygorii

Arnd Bergmann Nov. 18, 2014, 7:32 p.m. UTC | #9

On Tuesday 18 November 2014 20:54:36 Grygorii Strashko wrote:
> Hi All,
> 
> Thank you for your comments.
> 
> On 11/17/2014 11:50 PM, Kevin Hilman wrote:
> > Arnd Bergmann <[e-mail]> writes:
> > 
> >> On Monday 17 November 2014 11:14:16 Kevin Hilman wrote:
> >>>>>
> >>>>> So, The Keystone 2 Generic PM Controller is just a proxy PM layer here between
> >>>>> device and Generic clock manipulation PM callbacks.
> >>>>> It fills per-device clock list when device is attached to GPD and
> >>>>> ensures that all clocks from that list enabled/disabled when device is
> >>>>> started/stopped.
> >>>>
> >>>> The idea of such a generic power domain implementation sounds useful, but
> >>>> it has absolutely no business in platform specific code.
> >>>
> >>> Yes it does.  This isn't a generic power domain implementation, but
> >>> rather just the platform-specific glue that hooks up the clocks to the
> >>> right devices and power-domains so that the generic power-domain and
> >>> generic pm_clocks code does the right thing.
> >>
> >> How would you do this on an arm64 version of keystone then? With
> >> the current approach, you'd need to add a machine specific directory,
> >> and that seems completely pointless since this is not even about
> >> a hardware requirement.
> > 
> > Yeah, you're right.  I misunderstood you're original comment.
> > 
> >>>> I suggest you either remove the power domain proxy from your drivers
> >>>> and use the clocks directly,
> 
> Hm. I've been thinking about this, but the problem is that Keystone 2
> reuses a lot of IPs from Davinci and PM for Davinci is based on Generic clock
> manipulation PM callbacks framework, but for non-DT case. So, I can't simply
> use clocks directly.

I think you could get that to work without too much trouble, but as Kevin
comments, the generic pmdomain code is helpful here, and we should find
a way to make it work better.

> >>> No.  That's a step in the wrong direction.  This change isn't affecting
> >>> drivers directly.  It's the runtime PM and generic power domain layers
> >>> that handle this, and runtime PM adapted drivers don't need any changes.
> >>>   
> >>>> or come up with an implementation that can be used across other
> >>>> platforms and CPU architectures.
> >>>
> >>> We already have those in the generic power domain and the pm_clock
> >>> layers.  This series is just hooking those up for Keystone.
> >>
> >> Then why not add the missing piece to the generic power domain
> >> code to avoid having to add infrastructure to the platform
> >> for it?
> > 
> > Yes, good point.  There is nothing keystone-specific in this glue.
> > 
> > Grygorii, what about adding a feature to the generic domain parsing so
> > that it can get clocks from device nodes that are part of the domain,
> > and so it sets up pm_clk accordingly.
> 
> I'd like to mention few points here:
> 1)  not all platforms may need this
> 
> 2)  not all platforms may allow to add ALL clocks from "clocks" property
>    to pm_clk as some of them can be optional or have to be controlled by drivers only
>    (for example, initially, it was the case for SH-mobile https://lkml.org/lkml/2014/4/24/197
>     also now, last implementation for shmobile add only first clock from "clocks" property
>     to pm_clk https://lkml.org/lkml/2014/11/17/272).
> 
> 3)  such functionality have to be enabled for devices selectively, for example
>    now we are going to enable it for devices which a ready for runtime PM.
> 
> Current implementation cover 1 & 3, but also it allows to cover 2 too, because
> it's platform specific implementation and .attach_dev() can be updated to skip some 
> clocks or devices if needed.

Well, not all drivers and not all platforms have to use it, I think it would
just be good to make the case you are interested in really easy, and definitely
work without platform specific code.

> > I've recently seen other SoCs doing very similar, so this really should
> > be generalized.
> > 
> > I've been looking at this primarily as a right incremental improvement
> > from what is there for Keystone today, but Arnd is right.  This should
> > be moved out of platform code.
> 
> I'm ready to do what ever you want, but I don't fully understand what exactly to do :(
> Should I create some generic_pm_clk_domain.c?
> - or - Do you mean to integrate it in domain.c (see no way to do it:()?
> - or - smth. else
> 
> What about introduced DT bindings? For example, How will devices be
> selected for attachment to Generic pm_clk domain if I'll introduce
> generic_pm_clk_domain.c?

I am not really familiar with the pmdomain code at all, but here is
what I had thought the simplest generic pmdomain code would do:

Have one pmdomain driver in the generic code that knows about clocks,
possibly also regulators and pins and just turns them on when needed.
You can have a "simple-pmdomain" or "generic-pmdomain" compatible
string.

I'm a bit surprised that your pmdomain code looks up the clocks from the
respective device, rather than know about the clocks itself. There is
probably a good reason for this, but I don't see it yet.

Another option would be to have a special case for an empty
"power-domains" property if this is the most common case: if that
property exists but is empty, the pmdomain core could interpret
it as an indication to control all the clocks/regulators/pins
of a device.

	Arnd

Grygorii Strashko Nov. 19, 2014, 11:32 a.m. UTC | #10

Hi Arnd,

On 11/18/2014 09:32 PM, Arnd Bergmann wrote:
> On Tuesday 18 November 2014 20:54:36 Grygorii Strashko wrote:
>> Hi All,
>>
>> Thank you for your comments.
>>
>> On 11/17/2014 11:50 PM, Kevin Hilman wrote:
>>> Arnd Bergmann <[e-mail]> writes:
>>>
>>>> On Monday 17 November 2014 11:14:16 Kevin Hilman wrote:
>>>>>>>
>>>>>>> So, The Keystone 2 Generic PM Controller is just a proxy PM layer here between
>>>>>>> device and Generic clock manipulation PM callbacks.
>>>>>>> It fills per-device clock list when device is attached to GPD and
>>>>>>> ensures that all clocks from that list enabled/disabled when device is
>>>>>>> started/stopped.
>>>>>>
>>>>>> The idea of such a generic power domain implementation sounds useful, but
>>>>>> it has absolutely no business in platform specific code.
>>>>>
>>>>> Yes it does.  This isn't a generic power domain implementation, but
>>>>> rather just the platform-specific glue that hooks up the clocks to the
>>>>> right devices and power-domains so that the generic power-domain and
>>>>> generic pm_clocks code does the right thing.
>>>>
>>>> How would you do this on an arm64 version of keystone then? With
>>>> the current approach, you'd need to add a machine specific directory,
>>>> and that seems completely pointless since this is not even about
>>>> a hardware requirement.
>>>
>>> Yeah, you're right.  I misunderstood you're original comment.
>>>
>>>>>> I suggest you either remove the power domain proxy from your drivers
>>>>>> and use the clocks directly,
>>
>> Hm. I've been thinking about this, but the problem is that Keystone 2
>> reuses a lot of IPs from Davinci and PM for Davinci is based on Generic clock
>> manipulation PM callbacks framework, but for non-DT case. So, I can't simply
>> use clocks directly.
> 
> I think you could get that to work without too much trouble, but as Kevin
> comments, the generic pmdomain code is helpful here, and we should find
> a way to make it work better.
> 
>>>>> No.  That's a step in the wrong direction.  This change isn't affecting
>>>>> drivers directly.  It's the runtime PM and generic power domain layers
>>>>> that handle this, and runtime PM adapted drivers don't need any changes.
>>>>>    
>>>>>> or come up with an implementation that can be used across other
>>>>>> platforms and CPU architectures.
>>>>>
>>>>> We already have those in the generic power domain and the pm_clock
>>>>> layers.  This series is just hooking those up for Keystone.
>>>>
>>>> Then why not add the missing piece to the generic power domain
>>>> code to avoid having to add infrastructure to the platform
>>>> for it?
>>>
>>> Yes, good point.  There is nothing keystone-specific in this glue.
>>>
>>> Grygorii, what about adding a feature to the generic domain parsing so
>>> that it can get clocks from device nodes that are part of the domain,
>>> and so it sets up pm_clk accordingly.
>>
>> I'd like to mention few points here:
>> 1)  not all platforms may need this
>>
>> 2)  not all platforms may allow to add ALL clocks from "clocks" property
>>     to pm_clk as some of them can be optional or have to be controlled by drivers only
>>     (for example, initially, it was the case for SH-mobile https://lkml.org/lkml/2014/4/24/197
>>      also now, last implementation for shmobile add only first clock from "clocks" property
>>      to pm_clk https://lkml.org/lkml/2014/11/17/272).
>>
>> 3)  such functionality have to be enabled for devices selectively, for example
>>     now we are going to enable it for devices which a ready for runtime PM.
>>
>> Current implementation cover 1 & 3, but also it allows to cover 2 too, because
>> it's platform specific implementation and .attach_dev() can be updated to skip some
>> clocks or devices if needed.
> 
> Well, not all drivers and not all platforms have to use it, I think it would
> just be good to make the case you are interested in really easy, and definitely
> work without platform specific code.
> 
>>> I've recently seen other SoCs doing very similar, so this really should
>>> be generalized.
>>>
>>> I've been looking at this primarily as a right incremental improvement
>>> from what is there for Keystone today, but Arnd is right.  This should
>>> be moved out of platform code.
>>
>> I'm ready to do what ever you want, but I don't fully understand what exactly to do :(
>> Should I create some generic_pm_clk_domain.c?
>> - or - Do you mean to integrate it in domain.c (see no way to do it:()?
>> - or - smth. else
>>
>> What about introduced DT bindings? For example, How will devices be
>> selected for attachment to Generic pm_clk domain if I'll introduce
>> generic_pm_clk_domain.c?
> 
> I am not really familiar with the pmdomain code at all, but here is
> what I had thought the simplest generic pmdomain code would do:
> 
> Have one pmdomain driver in the generic code that knows about clocks,
> possibly also regulators and pins and just turns them on when needed.
> You can have a "simple-pmdomain" or "generic-pmdomain" compatible
> string.
> 
> I'm a bit surprised that your pmdomain code looks up the clocks from the
> respective device, rather than know about the clocks itself. There is
> probably a good reason for this, but I don't see it yet.

The keystone 2 uses simple PM schema based on clocks only:
- clocks enabled -> dev is active
- clocks disabled -> dev is suspended

To achieve explained above the Generic clock manipulation PM callbacks framework (pm_clk) is used.
- list of managed clocks is filled for each device (for non-DT case the con_id list
  is specified by platform code like:
	.con_ids = { "fck", "master", "slave", NULL }, 
        - or - 
	.con_ids = { }, <-- in this case only first clock will be added to pm_clk
  )
- dev.pm_domain is assigned to pm_clk_domain
- pm_clk_domain has .runtime_resume/runtime_suspend callbacks set and implemented like:
  static int keystone_pm_runtime_suspend(struct device *dev)
  {
	ret = pm_generic_runtime_suspend(dev);
	if (ret)
		return ret;

	ret = pm_clk_suspend(dev);
	if (ret) {
		pm_generic_runtime_resume(dev);
		return ret;
	}

	return 0;
  }
  static int keystone_pm_runtime_resume(struct device *dev)
  {
	pm_clk_resume(dev);

	return pm_generic_runtime_resume(dev);
  }
Now this configuration can be done from Platform Bus notifier (clock_ops.c) only and It'll
be done for ALL Platform devices and, once above steps have been done, the Runtime PM
starts working for device.

As was described early in this thread, Keystone 2 PM domain is glue layer which
allows:
 - configure pm_clk for devices from DT and get rid of .con_ids
 - configure only selected devices
 - get rid of using Platform Bus notifier
 - enable suspend/resume for devices as side effect :)
and which manages state of its devices through dev_ops->start()/stop() callbacks. 

> 
> Another option would be to have a special case for an empty
> "power-domains" property if this is the most common case: if that
> property exists but is empty, the pmdomain core could interpret
> it as an indication to control all the clocks/regulators/pins
> of a device.

I can try to do the following:
- move Keystone PM domain code in drivers/base/power/simple_domain.c
- add DT bindings like:
+	clk_pmdomain: simple-clk-pmdomain {
+		compatible = "simple-clk-pmdomain", "simple-pmdomain";
+		#power-domain-cells = <0>;
+	};
- in case if compatible == "simple-clk-pmdomain" it will do the same sings as in this patch

In the future, additional support for regulators/clocks/gpios can be added and these resources
can be managed in .power_on/power_off.

Is it ok for you?

Regards,
- grygorii

Arnd Bergmann Nov. 19, 2014, 1:47 p.m. UTC | #11

On Wednesday 19 November 2014 13:32:45 Grygorii Strashko wrote:
> On 11/18/2014 09:32 PM, Arnd Bergmann wrote:
> > On Tuesday 18 November 2014 20:54:36 Grygorii Strashko wrote:
> > 
> > Have one pmdomain driver in the generic code that knows about clocks,
> > possibly also regulators and pins and just turns them on when needed.
> > You can have a "simple-pmdomain" or "generic-pmdomain" compatible
> > string.
> > 
> > I'm a bit surprised that your pmdomain code looks up the clocks from the
> > respective device, rather than know about the clocks itself. There is
> > probably a good reason for this, but I don't see it yet.
> 
> The keystone 2 uses simple PM schema based on clocks only:
> - clocks enabled -> dev is active
> - clocks disabled -> dev is suspended
> 
> To achieve explained above the Generic clock manipulation PM callbacks framework (pm_clk) is used.
> - list of managed clocks is filled for each device (for non-DT case the con_id list
>   is specified by platform code like:
> 	.con_ids = { "fck", "master", "slave", NULL }, 
>         - or - 
> 	.con_ids = { }, <-- in this case only first clock will be added to pm_clk
>   )
> - dev.pm_domain is assigned to pm_clk_domain
> - pm_clk_domain has .runtime_resume/runtime_suspend callbacks set and implemented like:
>   static int keystone_pm_runtime_suspend(struct device *dev)
>   {
> 	ret = pm_generic_runtime_suspend(dev);
> 	if (ret)
> 		return ret;
> 
> 	ret = pm_clk_suspend(dev);
> 	if (ret) {
> 		pm_generic_runtime_resume(dev);
> 		return ret;
> 	}
> 
> 	return 0;
>   }
>   static int keystone_pm_runtime_resume(struct device *dev)
>   {
> 	pm_clk_resume(dev);
> 
> 	return pm_generic_runtime_resume(dev);
>   }
> Now this configuration can be done from Platform Bus notifier (clock_ops.c) only and It'll
> be done for ALL Platform devices and, once above steps have been done, the Runtime PM
> starts working for device.
> 
> As was described early in this thread, Keystone 2 PM domain is glue layer which
> allows:
>  - configure pm_clk for devices from DT and get rid of .con_ids
>  - configure only selected devices
>  - get rid of using Platform Bus notifier
>  - enable suspend/resume for devices as side effect :)
> and which manages state of its devices through dev_ops->start()/stop() callbacks. 

Right, that makes a lot more sense now, thanks for the detailed explanation!

> > Another option would be to have a special case for an empty
> > "power-domains" property if this is the most common case: if that
> > property exists but is empty, the pmdomain core could interpret
> > it as an indication to control all the clocks/regulators/pins
> > of a device.
> 
> I can try to do the following:
> - move Keystone PM domain code in drivers/base/power/simple_domain.c
> - add DT bindings like:
> +	clk_pmdomain: simple-clk-pmdomain {
> +		compatible = "simple-clk-pmdomain", "simple-pmdomain";
> +		#power-domain-cells = <0>;
> +	};
> - in case if compatible == "simple-clk-pmdomain" it will do the same sings as in this patch
> 
> In the future, additional support for regulators/clocks/gpios can be added and these resources
> can be managed in .power_on/power_off.
> 
> Is it ok for you?

Yes, it would definitely solve the problem that I see with the infrastructure
code that the current version adds into the platform directory.

The exact binding of course should be reviewed by the pmdomain and
DT maintainers, to ensure that it is done the best possible way, because
I assume we will end up using it a lot, and it would be a shame to get
it slightly wrong.

One possible variation I can think of would be to just use "simple-pmdomain"
as the compatible string, and use properties in the node itself to decide
what the domain should control, e.g.

	clk_pmdomain: pmdomain {
		compatible = "simple-pmdomain";
		pmdomain-enable-clocks;
		#power-domain-cells = <0>;
	};
	clk_regulator_pmdomain: pmdomain {
		compatible = "simple-pmdomain";
		pmdomain-enable-clocks;
		pmdomain-enable-regulators;
		#power-domain-cells = <0>;
	};

and then have each device link to one of the nodes as the pmdomain.

	Arnd

Ulf Hansson Nov. 20, 2014, 11:34 a.m. UTC | #12

On 19 November 2014 14:47, Arnd Bergmann <[e-mail]> wrote:
> On Wednesday 19 November 2014 13:32:45 Grygorii Strashko wrote:
>> On 11/18/2014 09:32 PM, Arnd Bergmann wrote:
>> > On Tuesday 18 November 2014 20:54:36 Grygorii Strashko wrote:
>> >
>> > Have one pmdomain driver in the generic code that knows about clocks,
>> > possibly also regulators and pins and just turns them on when needed.
>> > You can have a "simple-pmdomain" or "generic-pmdomain" compatible
>> > string.
>> >
>> > I'm a bit surprised that your pmdomain code looks up the clocks from the
>> > respective device, rather than know about the clocks itself. There is
>> > probably a good reason for this, but I don't see it yet.
>>
>> The keystone 2 uses simple PM schema based on clocks only:
>> - clocks enabled -> dev is active
>> - clocks disabled -> dev is suspended
>>
>> To achieve explained above the Generic clock manipulation PM callbacks framework (pm_clk) is used.
>> - list of managed clocks is filled for each device (for non-DT case the con_id list
>>   is specified by platform code like:
>>       .con_ids = { "fck", "master", "slave", NULL },
>>         - or -
>>       .con_ids = { }, <-- in this case only first clock will be added to pm_clk
>>   )

According to earlier comments in this thread, device's clocks are
split into "functional" and "PM" clocks.

If I understand correctly, a typical platform driver will enable it's
"functional" clocks during ->probe() and you want the PM domain to
take care of the "PM" clocks, when the device changes runtime PM
status.

How will you describe these different set of device clocks in DT?

[...]

>
> Yes, it would definitely solve the problem that I see with the infrastructure
> code that the current version adds into the platform directory.
>
> The exact binding of course should be reviewed by the pmdomain and
> DT maintainers, to ensure that it is done the best possible way, because
> I assume we will end up using it a lot, and it would be a shame to get
> it slightly wrong.
>
> One possible variation I can think of would be to just use "simple-pmdomain"
> as the compatible string, and use properties in the node itself to decide
> what the domain should control, e.g.
>
>         clk_pmdomain: pmdomain {
>                 compatible = "simple-pmdomain";
>                 pmdomain-enable-clocks;
>                 #power-domain-cells = <0>;
>         };
>         clk_regulator_pmdomain: pmdomain {
>                 compatible = "simple-pmdomain";
>                 pmdomain-enable-clocks;
>                 pmdomain-enable-regulators;
>                 #power-domain-cells = <0>;
>         };
>
> and then have each device link to one of the nodes as the pmdomain.
>

That's seems like a good approach to me.

Kind regards
Uffe

Grygorii Strashko Nov. 20, 2014, 12:03 p.m. UTC | #13

On 11/20/2014 01:34 PM, Ulf Hansson wrote:
> On 19 November 2014 14:47, Arnd Bergmann <[e-mail]> wrote:
>> On Wednesday 19 November 2014 13:32:45 Grygorii Strashko wrote:
>>> On 11/18/2014 09:32 PM, Arnd Bergmann wrote:
>>>> On Tuesday 18 November 2014 20:54:36 Grygorii Strashko wrote:
>>>>
>>>> Have one pmdomain driver in the generic code that knows about clocks,
>>>> possibly also regulators and pins and just turns them on when needed.
>>>> You can have a "simple-pmdomain" or "generic-pmdomain" compatible
>>>> string.
>>>>
>>>> I'm a bit surprised that your pmdomain code looks up the clocks from the
>>>> respective device, rather than know about the clocks itself. There is
>>>> probably a good reason for this, but I don't see it yet.
>>>
>>> The keystone 2 uses simple PM schema based on clocks only:
>>> - clocks enabled -> dev is active
>>> - clocks disabled -> dev is suspended
>>>
>>> To achieve explained above the Generic clock manipulation PM callbacks framework (pm_clk) is used.
>>> - list of managed clocks is filled for each device (for non-DT case the con_id list
>>>    is specified by platform code like:
>>>        .con_ids = { "fck", "master", "slave", NULL },
>>>          - or -
>>>        .con_ids = { }, <-- in this case only first clock will be added to pm_clk
>>>    )
> 
> According to earlier comments in this thread, device's clocks are
> split into "functional" and "PM" clocks.
> 
> If I understand correctly, a typical platform driver will enable it's
> "functional" clocks during ->probe() and you want the PM domain to
> take care of the "PM" clocks, when the device changes runtime PM
> status.
> 
> How will you describe these different set of device clocks in DT?

True :(  You can dig deeper in the history of this series if you wish.
- first Geert Uytterhoeven proposed to use CLK_RUNTIME_PM there 
  https://lkml.org/lkml/2014/11/6/319
- second I proposed to introduce smth. like "clkops-clocks", "pm-clocks" there
  https://lkml.org/lkml/2014/6/12/436
 or "fck-clocks"/"opt-clocks" later. 

^failed.

So, this implementation picks up all clocks for each device, which is ok for
Keystone 2 and, because it's platform specific.

>>
>> Yes, it would definitely solve the problem that I see with the infrastructure
>> code that the current version adds into the platform directory.
>>
>> The exact binding of course should be reviewed by the pmdomain and
>> DT maintainers, to ensure that it is done the best possible way, because
>> I assume we will end up using it a lot, and it would be a shame to get
>> it slightly wrong.
>>
>> One possible variation I can think of would be to just use "simple-pmdomain"
>> as the compatible string, and use properties in the node itself to decide
>> what the domain should control, e.g.
>>
>>          clk_pmdomain: pmdomain {
>>                  compatible = "simple-pmdomain";
>>                  pmdomain-enable-clocks;
>>                  #power-domain-cells = <0>;
>>          };
>>          clk_regulator_pmdomain: pmdomain {
>>                  compatible = "simple-pmdomain";
>>                  pmdomain-enable-clocks;
>>                  pmdomain-enable-regulators;
>>                  #power-domain-cells = <0>;
>>          };
>>
>> and then have each device link to one of the nodes as the pmdomain.
>>
> 
> That's seems like a good approach to me.
 
Yes, but your previous comment is still actual :(

Regards,
-grygorii

Ulf Hansson Nov. 20, 2014, 1:12 p.m. UTC | #14

On 20 November 2014 13:03, Grygorii Strashko <[e-mail]> wrote:
> On 11/20/2014 01:34 PM, Ulf Hansson wrote:
>> On 19 November 2014 14:47, Arnd Bergmann <[e-mail]> wrote:
>>> On Wednesday 19 November 2014 13:32:45 Grygorii Strashko wrote:
>>>> On 11/18/2014 09:32 PM, Arnd Bergmann wrote:
>>>>> On Tuesday 18 November 2014 20:54:36 Grygorii Strashko wrote:
>>>>>
>>>>> Have one pmdomain driver in the generic code that knows about clocks,
>>>>> possibly also regulators and pins and just turns them on when needed.
>>>>> You can have a "simple-pmdomain" or "generic-pmdomain" compatible
>>>>> string.
>>>>>
>>>>> I'm a bit surprised that your pmdomain code looks up the clocks from the
>>>>> respective device, rather than know about the clocks itself. There is
>>>>> probably a good reason for this, but I don't see it yet.
>>>>
>>>> The keystone 2 uses simple PM schema based on clocks only:
>>>> - clocks enabled -> dev is active
>>>> - clocks disabled -> dev is suspended
>>>>
>>>> To achieve explained above the Generic clock manipulation PM callbacks framework (pm_clk) is used.
>>>> - list of managed clocks is filled for each device (for non-DT case the con_id list
>>>>    is specified by platform code like:
>>>>        .con_ids = { "fck", "master", "slave", NULL },
>>>>          - or -
>>>>        .con_ids = { }, <-- in this case only first clock will be added to pm_clk
>>>>    )
>>
>> According to earlier comments in this thread, device's clocks are
>> split into "functional" and "PM" clocks.
>>
>> If I understand correctly, a typical platform driver will enable it's
>> "functional" clocks during ->probe() and you want the PM domain to
>> take care of the "PM" clocks, when the device changes runtime PM
>> status.
>>
>> How will you describe these different set of device clocks in DT?
>
> True :(  You can dig deeper in the history of this series if you wish.
> - first Geert Uytterhoeven proposed to use CLK_RUNTIME_PM there
>   https://lkml.org/lkml/2014/11/6/319
> - second I proposed to introduce smth. like "clkops-clocks", "pm-clocks" there
>   https://lkml.org/lkml/2014/6/12/436
>  or "fck-clocks"/"opt-clocks" later.
>
> ^failed.
>
> So, this implementation picks up all clocks for each device, which is ok for
> Keystone 2 and, because it's platform specific.
>
>>>
>>> Yes, it would definitely solve the problem that I see with the infrastructure
>>> code that the current version adds into the platform directory.
>>>
>>> The exact binding of course should be reviewed by the pmdomain and
>>> DT maintainers, to ensure that it is done the best possible way, because
>>> I assume we will end up using it a lot, and it would be a shame to get
>>> it slightly wrong.
>>>
>>> One possible variation I can think of would be to just use "simple-pmdomain"
>>> as the compatible string, and use properties in the node itself to decide
>>> what the domain should control, e.g.
>>>
>>>          clk_pmdomain: pmdomain {
>>>                  compatible = "simple-pmdomain";
>>>                  pmdomain-enable-clocks;
>>>                  #power-domain-cells = <0>;
>>>          };
>>>          clk_regulator_pmdomain: pmdomain {
>>>                  compatible = "simple-pmdomain";
>>>                  pmdomain-enable-clocks;
>>>                  pmdomain-enable-regulators;
>>>                  #power-domain-cells = <0>;
>>>          };
>>>
>>> and then have each device link to one of the nodes as the pmdomain.
>>>
>>
>> That's seems like a good approach to me.
>
> Yes, but your previous comment is still actual :(

Agree!

So I really think we need to decide on how to address the split of the
device clocks. Before that's done, I don't think it make sense to add
a "simple-pmdomain" compatible, since it will likely not be that many
SoC that can use it.

So, does anyone have a suggestion on how to deal with the split of the
device clocks into "functional" clocks and into "PM" clocks?

Kind regards
Uffe

Geert Uytterhoeven Nov. 20, 2014, 1:32 p.m. UTC | #15

On Thu, Nov 20, 2014 at 2:12 PM, Ulf Hansson <[e-mail]> wrote:
>>> According to earlier comments in this thread, device's clocks are
>>> split into "functional" and "PM" clocks.
>>>
>>> If I understand correctly, a typical platform driver will enable it's
>>> "functional" clocks during ->probe() and you want the PM domain to
>>> take care of the "PM" clocks, when the device changes runtime PM
>>> status.
>>>
>>> How will you describe these different set of device clocks in DT?
>>
>> True :(  You can dig deeper in the history of this series if you wish.
>> - first Geert Uytterhoeven proposed to use CLK_RUNTIME_PM there
>>   https://lkml.org/lkml/2014/11/6/319
>> - second I proposed to introduce smth. like "clkops-clocks", "pm-clocks" there
>>   https://lkml.org/lkml/2014/6/12/436
>>  or "fck-clocks"/"opt-clocks" later.
>>
>> ^failed.
>>
>> So, this implementation picks up all clocks for each device, which is ok for
>> Keystone 2 and, because it's platform specific.
>>
>>>>
>>>> Yes, it would definitely solve the problem that I see with the infrastructure
>>>> code that the current version adds into the platform directory.
>>>>
>>>> The exact binding of course should be reviewed by the pmdomain and
>>>> DT maintainers, to ensure that it is done the best possible way, because
>>>> I assume we will end up using it a lot, and it would be a shame to get
>>>> it slightly wrong.
>>>>
>>>> One possible variation I can think of would be to just use "simple-pmdomain"
>>>> as the compatible string, and use properties in the node itself to decide
>>>> what the domain should control, e.g.
>>>>
>>>>          clk_pmdomain: pmdomain {
>>>>                  compatible = "simple-pmdomain";
>>>>                  pmdomain-enable-clocks;
>>>>                  #power-domain-cells = <0>;
>>>>          };
>>>>          clk_regulator_pmdomain: pmdomain {
>>>>                  compatible = "simple-pmdomain";
>>>>                  pmdomain-enable-clocks;
>>>>                  pmdomain-enable-regulators;
>>>>                  #power-domain-cells = <0>;
>>>>          };
>>>>
>>>> and then have each device link to one of the nodes as the pmdomain.
>>>>
>>>
>>> That's seems like a good approach to me.
>>
>> Yes, but your previous comment is still actual :(
>
> Agree!
>
> So I really think we need to decide on how to address the split of the
> device clocks. Before that's done, I don't think it make sense to add
> a "simple-pmdomain" compatible, since it will likely not be that many
> SoC that can use it.
>
> So, does anyone have a suggestion on how to deal with the split of the
> device clocks into "functional" clocks and into "PM" clocks?

That's indeed the tricky part.

On shmobile, we just use the "NULL" clock, i.e. the first clock, for historical
reasons.

Using clock-names (e.g. "fck" and "pm") will conflict with existing bindings
for devices that already mandate specific clock names.

As the clock being a "PM" clock is a property of the clock provider
(at last on shmobile), I originally came up with not handling it in DT,
but advertising it in the clock provider driver:

> > - first Geert Uytterhoeven proposed to use CLK_RUNTIME_PM there
> >   https://lkml.org/lkml/2014/11/6/319

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Grygorii Strashko Nov. 20, 2014, 3:32 p.m. UTC | #16

On 11/20/2014 03:32 PM, Geert Uytterhoeven wrote:
> On Thu, Nov 20, 2014 at 2:12 PM, Ulf Hansson <[e-mail]> wrote:
>>>> According to earlier comments in this thread, device's clocks are
>>>> split into "functional" and "PM" clocks.
>>>>
>>>> If I understand correctly, a typical platform driver will enable it's
>>>> "functional" clocks during ->probe() and you want the PM domain to
>>>> take care of the "PM" clocks, when the device changes runtime PM
>>>> status.
>>>>
>>>> How will you describe these different set of device clocks in DT?
>>>
>>> True :(  You can dig deeper in the history of this series if you wish.
>>> - first Geert Uytterhoeven proposed to use CLK_RUNTIME_PM there
>>>    https://lkml.org/lkml/2014/11/6/319
>>> - second I proposed to introduce smth. like "clkops-clocks", "pm-clocks" there
>>>    https://lkml.org/lkml/2014/6/12/436
>>>   or "fck-clocks"/"opt-clocks" later.
>>>
>>> ^failed.
>>>
>>> So, this implementation picks up all clocks for each device, which is ok for
>>> Keystone 2 and, because it's platform specific.
>>>
>>>>>
>>>>> Yes, it would definitely solve the problem that I see with the infrastructure
>>>>> code that the current version adds into the platform directory.
>>>>>
>>>>> The exact binding of course should be reviewed by the pmdomain and
>>>>> DT maintainers, to ensure that it is done the best possible way, because
>>>>> I assume we will end up using it a lot, and it would be a shame to get
>>>>> it slightly wrong.
>>>>>
>>>>> One possible variation I can think of would be to just use "simple-pmdomain"
>>>>> as the compatible string, and use properties in the node itself to decide
>>>>> what the domain should control, e.g.
>>>>>
>>>>>           clk_pmdomain: pmdomain {
>>>>>                   compatible = "simple-pmdomain";
>>>>>                   pmdomain-enable-clocks;
>>>>>                   #power-domain-cells = <0>;
>>>>>           };
>>>>>           clk_regulator_pmdomain: pmdomain {
>>>>>                   compatible = "simple-pmdomain";
>>>>>                   pmdomain-enable-clocks;
>>>>>                   pmdomain-enable-regulators;
>>>>>                   #power-domain-cells = <0>;
>>>>>           };
>>>>>
>>>>> and then have each device link to one of the nodes as the pmdomain.
>>>>>
>>>>
>>>> That's seems like a good approach to me.
>>>
>>> Yes, but your previous comment is still actual :(
>>
>> Agree!
>>
>> So I really think we need to decide on how to address the split of the
>> device clocks. Before that's done, I don't think it make sense to add
>> a "simple-pmdomain" compatible, since it will likely not be that many
>> SoC that can use it.
>>
>> So, does anyone have a suggestion on how to deal with the split of the
>> device clocks into "functional" clocks and into "PM" clocks?

Would it be better to say "functional" and "optional"? In my opinion
"PM" == "functional". Also, such clock's separation is used in TRM/DM/UMs on HW.

> 
> That's indeed the tricky part.
> 
> On shmobile, we just use the "NULL" clock, i.e. the first clock, for historical
> reasons.
> 
> Using clock-names (e.g. "fck" and "pm") will conflict with existing bindings
> for devices that already mandate specific clock names.
> 
> As the clock being a "PM" clock is a property of the clock provider
> (at last on shmobile), I originally came up with not handling it in DT,
> but advertising it in the clock provider driver:
> 
>>> - first Geert Uytterhoeven proposed to use CLK_RUNTIME_PM there
>>>    https://lkml.org/lkml/2014/11/6/319

As I mentioned previously I don't like it, because in many cases one driver is used for
all/set_of clocks which support gating function. As result, some sort of tables will
need to be created and maintained in code by these drivers per each SoC
(or even per each SoC revision) for identification that clock is "PM clock".

Additional points are : both parent and child clock can be gated; one clock
can be used by two devices and be "functional" and "optional" at once :)

regards,
-grygorii

Kevin Hilman Nov. 20, 2014, 8:22 p.m. UTC | #17

Grygorii Strashko <[e-mail]> writes:

> On 11/20/2014 03:32 PM, Geert Uytterhoeven wrote:
>> On Thu, Nov 20, 2014 at 2:12 PM, Ulf Hansson <[e-mail]> wrote:

[...]

>>> So I really think we need to decide on how to address the split of the
>>> device clocks. Before that's done, I don't think it make sense to add
>>> a "simple-pmdomain" compatible, since it will likely not be that many
>>> SoC that can use it.
>>>
>>> So, does anyone have a suggestion on how to deal with the split of the
>>> device clocks into "functional" clocks and into "PM" clocks?
>
> Would it be better to say "functional" and "optional"? In my opinion
> "PM" == "functional". Also, such clock's separation is used in TRM/DM/UMs on HW.

Yes!  I really don't like the name "PM" clock, since it's not at all
obvious what that means.  To me, "PM" == "functional" as well.

So what exactly are we talking about with "PM" clocks, and why are they
"special" when it comes to PM domains?  IOW, why are the clocks to be
managed during PM domain transitions for a given device any different
than the clocks that need to be managed for a runtime suspend/resume (or
system suspend/resume) sequence for the same device?

Kevin

Geert Uytterhoeven Nov. 20, 2014, 8:26 p.m. UTC | #18

Hi Kevin,

On Thu, Nov 20, 2014 at 9:22 PM, Kevin Hilman <[e-mail]> wrote:
> Grygorii Strashko <[e-mail]> writes:
>> On 11/20/2014 03:32 PM, Geert Uytterhoeven wrote:
>>> On Thu, Nov 20, 2014 at 2:12 PM, Ulf Hansson <[e-mail]> wrote:
>
> [...]
>
>>>> So I really think we need to decide on how to address the split of the
>>>> device clocks. Before that's done, I don't think it make sense to add
>>>> a "simple-pmdomain" compatible, since it will likely not be that many
>>>> SoC that can use it.
>>>>
>>>> So, does anyone have a suggestion on how to deal with the split of the
>>>> device clocks into "functional" clocks and into "PM" clocks?
>>
>> Would it be better to say "functional" and "optional"? In my opinion
>> "PM" == "functional". Also, such clock's separation is used in TRM/DM/UMs on HW.
>
> Yes!  I really don't like the name "PM" clock, since it's not at all
> obvious what that means.  To me, "PM" == "functional" as well.
>
> So what exactly are we talking about with "PM" clocks, and why are they
> "special" when it comes to PM domains?  IOW, why are the clocks to be
> managed during PM domain transitions for a given device any different
> than the clocks that need to be managed for a runtime suspend/resume (or
> system suspend/resume) sequence for the same device?

(Speaking for my case, shmobile)

They're not. The clocks to be managed during PM domain transitions are the
same as the clocks that need to be managed for a runtime suspend/resume
(or system suspend/resume) sequence.

The special thing is that this is more a platform than a driver thing: the same
module may have a "PM/functional" clock (that is documented to enable/disable
the module) on one Soc, but not on another.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Kevin Hilman Nov. 20, 2014, 9:48 p.m. UTC | #19

Geert Uytterhoeven <[e-mail]> writes:

> Hi Kevin,
>
> On Thu, Nov 20, 2014 at 9:22 PM, Kevin Hilman <[e-mail]> wrote:
>> Grygorii Strashko <[e-mail]> writes:
>>> On 11/20/2014 03:32 PM, Geert Uytterhoeven wrote:
>>>> On Thu, Nov 20, 2014 at 2:12 PM, Ulf Hansson <[e-mail]> wrote:
>>
>> [...]
>>
>>>>> So I really think we need to decide on how to address the split of the
>>>>> device clocks. Before that's done, I don't think it make sense to add
>>>>> a "simple-pmdomain" compatible, since it will likely not be that many
>>>>> SoC that can use it.
>>>>>
>>>>> So, does anyone have a suggestion on how to deal with the split of the
>>>>> device clocks into "functional" clocks and into "PM" clocks?
>>>
>>> Would it be better to say "functional" and "optional"? In my opinion
>>> "PM" == "functional". Also, such clock's separation is used in TRM/DM/UMs on HW.
>>
>> Yes!  I really don't like the name "PM" clock, since it's not at all
>> obvious what that means.  To me, "PM" == "functional" as well.
>>
>> So what exactly are we talking about with "PM" clocks, and why are they
>> "special" when it comes to PM domains?  IOW, why are the clocks to be
>> managed during PM domain transitions for a given device any different
>> than the clocks that need to be managed for a runtime suspend/resume (or
>> system suspend/resume) sequence for the same device?
>
> (Speaking for my case, shmobile)
>
> They're not. The clocks to be managed during PM domain transitions are the
> same as the clocks that need to be managed for a runtime suspend/resume
> (or system suspend/resume) sequence.
>
> The special thing is that this is more a platform than a driver thing: the same
> module may have a "PM/functional" clock (that is documented to enable/disable
> the module) on one Soc, but not on another.

So why isn't the presence or absence of the clock described in the .dtsi
for the SoC instead of being handled by special PM domain logic?

Kevin

Geert Uytterhoeven Nov. 20, 2014, 9:54 p.m. UTC | #20

On Thu, Nov 20, 2014 at 10:48 PM, Kevin Hilman <[e-mail]> wrote:
>>> So what exactly are we talking about with "PM" clocks, and why are they
>>> "special" when it comes to PM domains?  IOW, why are the clocks to be
>>> managed during PM domain transitions for a given device any different
>>> than the clocks that need to be managed for a runtime suspend/resume (or
>>> system suspend/resume) sequence for the same device?
>>
>> (Speaking for my case, shmobile)
>>
>> They're not. The clocks to be managed during PM domain transitions are the
>> same as the clocks that need to be managed for a runtime suspend/resume
>> (or system suspend/resume) sequence.
>>
>> The special thing is that this is more a platform than a driver thing: the same
>> module may have a "PM/functional" clock (that is documented to enable/disable
>> the module) on one Soc, but noet on another.
>
> So why isn't the presence or absence of the clock described in the .dtsi
> for the SoC instead of being handled by special PM domain logic?

It is. Cfr. the presence/absence of clocks for renesas,rcar-gpio nodes.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Kevin Hilman Nov. 21, 2014, 1:30 a.m. UTC | #21

Geert Uytterhoeven <[e-mail]> writes:

> On Thu, Nov 20, 2014 at 10:48 PM, Kevin Hilman <[e-mail]> wrote:
>>>> So what exactly are we talking about with "PM" clocks, and why are they
>>>> "special" when it comes to PM domains?  IOW, why are the clocks to be
>>>> managed during PM domain transitions for a given device any different
>>>> than the clocks that need to be managed for a runtime suspend/resume (or
>>>> system suspend/resume) sequence for the same device?
>>>
>>> (Speaking for my case, shmobile)
>>>
>>> They're not. The clocks to be managed during PM domain transitions are the
>>> same as the clocks that need to be managed for a runtime suspend/resume
>>> (or system suspend/resume) sequence.
>>>
>>> The special thing is that this is more a platform than a driver thing: the same
>>> module may have a "PM/functional" clock (that is documented to enable/disable
>>> the module) on one Soc, but noet on another.
>>
>> So why isn't the presence or absence of the clock described in the .dtsi
>> for the SoC instead of being handled by special PM domain logic?
>
> It is. Cfr. the presence/absence of clocks for renesas,rcar-gpio nodes.

Hmm, OK, Good.  

So now I'm confused about why the PM domain has to do anything special
if the presence/absence of the clocks is already handled by the DT.

Kevin

Geert Uytterhoeven Nov. 21, 2014, 8:06 a.m. UTC | #22

Hi Kevin,

On Fri, Nov 21, 2014 at 2:30 AM, Kevin Hilman <[e-mail]> wrote:
> Geert Uytterhoeven <[e-mail]> writes:
>> On Thu, Nov 20, 2014 at 10:48 PM, Kevin Hilman <[e-mail]> wrote:
>>>>> So what exactly are we talking about with "PM" clocks, and why are they
>>>>> "special" when it comes to PM domains?  IOW, why are the clocks to be
>>>>> managed during PM domain transitions for a given device any different
>>>>> than the clocks that need to be managed for a runtime suspend/resume (or
>>>>> system suspend/resume) sequence for the same device?
>>>>
>>>> (Speaking for my case, shmobile)
>>>>
>>>> They're not. The clocks to be managed during PM domain transitions are the
>>>> same as the clocks that need to be managed for a runtime suspend/resume
>>>> (or system suspend/resume) sequence.
>>>>
>>>> The special thing is that this is more a platform than a driver thing: the same
>>>> module may have a "PM/functional" clock (that is documented to enable/disable
>>>> the module) on one Soc, but noet on another.
>>>
>>> So why isn't the presence or absence of the clock described in the .dtsi
>>> for the SoC instead of being handled by special PM domain logic?
>>
>> It is. Cfr. the presence/absence of clocks for renesas,rcar-gpio nodes.
>
> Hmm, OK, Good.
>
> So now I'm confused about why the PM domain has to do anything special
> if the presence/absence of the clocks is already handled by the DT.

Just adding a clock property to a device node in DT doesn't enable the clock
automatically, nor make it runtime-managed automatically.
Compare this to e.g. pinctrl, where adding pinctrl properties to DT does enable
them automatically, without the driver for the device having to care about it.

Drivers interfacing external hardware typically do care about clocks, as they
have to program clock generators for the external hardware interface (e.g.
driving spi or i2c buses at specific frequencies).

Other random drivers don't care about clocks, so they don't handle them.
But as long as they make basic pm_runtime_{enable,get_sync,put}() calls,
the (optional) clocks (and hardware PM domains) will "work" fine, if handled
by the PM (clock) domain.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Geert Uytterhoeven Nov. 21, 2014, 9:04 a.m. UTC | #23

On Thu, Nov 20, 2014 at 4:32 PM, Grygorii Strashko
<[e-mail]> wrote:
>> That's indeed the tricky part.
>>
>> On shmobile, we just use the "NULL" clock, i.e. the first clock, for historical
>> reasons.
>>
>> Using clock-names (e.g. "fck" and "pm") will conflict with existing bindings
>> for devices that already mandate specific clock names.
>>
>> As the clock being a "PM" clock is a property of the clock provider
>> (at last on shmobile), I originally came up with not handling it in DT,
>> but advertising it in the clock provider driver:
>>
>>>> - first Geert Uytterhoeven proposed to use CLK_RUNTIME_PM there
>>>>    https://lkml.org/lkml/2014/11/6/319
>
> As I mentioned previously I don't like it, because in many cases one driver is used for
> all/set_of clocks which support gating function. As result, some sort of tables will
> need to be created and maintained in code by these drivers per each SoC
> (or even per each SoC revision) for identification that clock is "PM clock".

That depends. On shmobile, there's a separate hardware module for those
clocks, hence all clocks provided by drivers/clk/shmobile/clk-mstp.c are "PM"
clocks.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [e-mail]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Grygorii Strashko Nov. 21, 2014, 6:58 p.m. UTC | #24

Hi Kevin,
On 11/21/2014 10:06 AM, Geert Uytterhoeven wrote:
> On Fri, Nov 21, 2014 at 2:30 AM, Kevin Hilman <[e-mail]> wrote:
>> Geert Uytterhoeven <[e-mail]> writes:
>>> On Thu, Nov 20, 2014 at 10:48 PM, Kevin Hilman <[e-mail]> wrote:
>>>>>> So what exactly are we talking about with "PM" clocks, and why are they
>>>>>> "special" when it comes to PM domains?  IOW, why are the clocks to be
>>>>>> managed during PM domain transitions for a given device any different
>>>>>> than the clocks that need to be managed for a runtime suspend/resume (or
>>>>>> system suspend/resume) sequence for the same device?
>>>>>
>>>>> (Speaking for my case, shmobile)
>>>>>
>>>>> They're not. The clocks to be managed during PM domain transitions are the
>>>>> same as the clocks that need to be managed for a runtime suspend/resume
>>>>> (or system suspend/resume) sequence.
>>>>>
>>>>> The special thing is that this is more a platform than a driver thing: the same
>>>>> module may have a "PM/functional" clock (that is documented to enable/disable
>>>>> the module) on one Soc, but noet on another.
>>>>
>>>> So why isn't the presence or absence of the clock described in the .dtsi
>>>> for the SoC instead of being handled by special PM domain logic?
>>>
>>> It is. Cfr. the presence/absence of clocks for renesas,rcar-gpio nodes.
>>
>> Hmm, OK, Good.
>>
>> So now I'm confused about why the PM domain has to do anything special
>> if the presence/absence of the clocks is already handled by the DT.
> 
> Just adding a clock property to a device node in DT doesn't enable the clock
> automatically, nor make it runtime-managed automatically.
> Compare this to e.g. pinctrl, where adding pinctrl properties to DT does enable
> them automatically, without the driver for the device having to care about it.
> 
> Drivers interfacing external hardware typically do care about clocks, as they
> have to program clock generators for the external hardware interface (e.g.
> driving spi or i2c buses at specific frequencies).
> 
> Other random drivers don't care about clocks, so they don't handle them.
> But as long as they make basic pm_runtime_{enable,get_sync,put}() calls,
> the (optional) clocks (and hardware PM domains) will "work" fine, if handled
> by the PM (clock) domain.

Personally, I don't see problems with "functional" clocks.
The problem, in my opinion, is that we can't specify in DT that some clock is
"optional", so no one should touch such clock except driver.
For example:
Keystone 2 NETCP module has 3 clocks:
	clocks = <&papllclk>, <&clkcpgmac>, <&chipclk12>;
	clock-names = "clk_pa", "clk_cpgmac", "cpsw_cpts_rft_clk";
 and CPTS functionality is optional (in general) and can be enabled/disabled.
Also, usual example is MMC hosts 
- OMAP hsmmc has "functional" clock "hsmmc1_fclk", and may have
"optional" clock "mmchsdb_fck". As you may remember, PM runtime
for OMAP2+ implemented by OMAP PM domain which performs many things,
but one thing which is done always is enabling/disabling of "fck"
when get/put() are called.

Actually, pm_clk is very simple case of OMAP PM domain which should only
handle clocks.

In non-DT case, we have possibility to divide clocks on "fck" and "opt"
(The way it can be done is not convenient, but it is - .con_id).

For DT-case - no way now. Also, PM domains are not physically present on
Keystone 2 and GPD was selected as glue layer to integrate DT, pm_clk and
PM runtime all together (one big-fat-global PM domain :).

So, I was able to find only following way to define "fck" clocks in DT:
	clocks = <&papllclk>, <&clkcpgmac>, <&chipclk12>;
	clock-names = "clk_pa", "clk_cpgmac", "cpsw_cpts_rft_clk";
	fck-clocks = <&papllclk>, <&clkcpgmac>;
As you can see - this will lead to data duplication in DT (

Any propositions are welcome?

Unfortunately, It seems that if we would not able to find DT solution
then there will be following ways to move forward:
- "remove the power domain proxy from your drivers and use the clocks directly" 
((c) Arnd Bergmann).
[As possibility - It can be allowed to use clk_pm APIs by drivers]
- continue using platform specific implementations.

regards,
-grygorii

Kevin Hilman Nov. 21, 2014, 7:20 p.m. UTC | #25

Geert Uytterhoeven <[e-mail]> writes:

> Hi Kevin,
>
> On Fri, Nov 21, 2014 at 2:30 AM, Kevin Hilman <[e-mail]> wrote:
>> Geert Uytterhoeven <[e-mail]> writes:
>>> On Thu, Nov 20, 2014 at 10:48 PM, Kevin Hilman <[e-mail]> wrote:
>>>>>> So what exactly are we talking about with "PM" clocks, and why are they
>>>>>> "special" when it comes to PM domains?  IOW, why are the clocks to be
>>>>>> managed during PM domain transitions for a given device any different
>>>>>> than the clocks that need to be managed for a runtime suspend/resume (or
>>>>>> system suspend/resume) sequence for the same device?
>>>>>
>>>>> (Speaking for my case, shmobile)
>>>>>
>>>>> They're not. The clocks to be managed during PM domain transitions are the
>>>>> same as the clocks that need to be managed for a runtime suspend/resume
>>>>> (or system suspend/resume) sequence.
>>>>>
>>>>> The special thing is that this is more a platform than a driver thing: the same
>>>>> module may have a "PM/functional" clock (that is documented to enable/disable
>>>>> the module) on one Soc, but noet on another.
>>>>
>>>> So why isn't the presence or absence of the clock described in the .dtsi
>>>> for the SoC instead of being handled by special PM domain logic?
>>>
>>> It is. Cfr. the presence/absence of clocks for renesas,rcar-gpio nodes.
>>
>> Hmm, OK, Good.
>>
>> So now I'm confused about why the PM domain has to do anything special
>> if the presence/absence of the clocks is already handled by the DT.
>
> Just adding a clock property to a device node in DT doesn't enable the clock
> automatically, nor make it runtime-managed automatically.

In general, that's true.  But I thought you're PM domain was written to
look for clock properties, and if present would manage them.  The
proposed genpd support for TI Keystone2 would make it so these clocks
would definitely be automatically managed by the PM domain.

> Compare this to e.g. pinctrl, where adding pinctrl properties to DT does enable
> them automatically, without the driver for the device having to care about it.

Well, we're headed down the same path with genpd (if given the right
properties in genpd.)

> Drivers interfacing external hardware typically do care about clocks, as they
> have to program clock generators for the external hardware interface (e.g.
> driving spi or i2c buses at specific frequencies).

Yes, but IMO, these should be handled by the driver, not by the PM
domain.  More specifically, if a device is generating a clock for
external hardware, presumably it cannot be runtime suspended, so the
enclosing PM domain can be powered off.  If it's not generating a clock,
then it can be runtime suspended and presumably would gate it's
externally facing clocks when it runtime suspends.

> Other random drivers don't care about clocks, so they don't handle them.
> But as long as they make basic pm_runtime_{enable,get_sync,put}() calls,
> the (optional) clocks (and hardware PM domains) will "work" fine, if handled
> by the PM (clock) domain.

Yes, I understand that.  But this still isn't helping me understand why
your PM domain has to distinguish between different types of clocks
(e.g. why it only manages the first clock.) 

Did you set up the properties so that the first clock was the functional
clock and any additional ones were for devices that generate external
clocks?

Kevin

Kevin Hilman Nov. 21, 2014, 7:29 p.m. UTC | #26

Grygorii Strashko <[e-mail]> writes:

> Hi Kevin,
> On 11/21/2014 10:06 AM, Geert Uytterhoeven wrote:
>> On Fri, Nov 21, 2014 at 2:30 AM, Kevin Hilman <[e-mail]> wrote:
>>> Geert Uytterhoeven <[e-mail]> writes:
>>>> On Thu, Nov 20, 2014 at 10:48 PM, Kevin Hilman <[e-mail]> wrote:
>>>>>>> So what exactly are we talking about with "PM" clocks, and why are they
>>>>>>> "special" when it comes to PM domains?  IOW, why are the clocks to be
>>>>>>> managed during PM domain transitions for a given device any different
>>>>>>> than the clocks that need to be managed for a runtime suspend/resume (or
>>>>>>> system suspend/resume) sequence for the same device?
>>>>>>
>>>>>> (Speaking for my case, shmobile)
>>>>>>
>>>>>> They're not. The clocks to be managed during PM domain transitions are the
>>>>>> same as the clocks that need to be managed for a runtime suspend/resume
>>>>>> (or system suspend/resume) sequence.
>>>>>>
>>>>>> The special thing is that this is more a platform than a driver thing: the same
>>>>>> module may have a "PM/functional" clock (that is documented to enable/disable
>>>>>> the module) on one Soc, but noet on another.
>>>>>
>>>>> So why isn't the presence or absence of the clock described in the .dtsi
>>>>> for the SoC instead of being handled by special PM domain logic?
>>>>
>>>> It is. Cfr. the presence/absence of clocks for renesas,rcar-gpio nodes.
>>>
>>> Hmm, OK, Good.
>>>
>>> So now I'm confused about why the PM domain has to do anything special
>>> if the presence/absence of the clocks is already handled by the DT.
>> 
>> Just adding a clock property to a device node in DT doesn't enable the clock
>> automatically, nor make it runtime-managed automatically.
>> Compare this to e.g. pinctrl, where adding pinctrl properties to DT does enable
>> them automatically, without the driver for the device having to care about it.
>> 
>> Drivers interfacing external hardware typically do care about clocks, as they
>> have to program clock generators for the external hardware interface (e.g.
>> driving spi or i2c buses at specific frequencies).
>> 
>> Other random drivers don't care about clocks, so they don't handle them.
>> But as long as they make basic pm_runtime_{enable,get_sync,put}() calls,
>> the (optional) clocks (and hardware PM domains) will "work" fine, if handled
>> by the PM (clock) domain.
>
> Personally, I don't see problems with "functional" clocks.
> The problem, in my opinion, is that we can't specify in DT that some clock is
> "optional", so no one should touch such clock except driver.
> For example:
> Keystone 2 NETCP module has 3 clocks:
> 	clocks = <&papllclk>, <&clkcpgmac>, <&chipclk12>;
> 	clock-names = "clk_pa", "clk_cpgmac", "cpsw_cpts_rft_clk";
>  and CPTS functionality is optional (in general) and can be enabled/disabled.
> Also, usual example is MMC hosts 
> - OMAP hsmmc has "functional" clock "hsmmc1_fclk", and may have
> "optional" clock "mmchsdb_fck". As you may remember, PM runtime
> for OMAP2+ implemented by OMAP PM domain which performs many things,
> but one thing which is done always is enabling/disabling of "fck"
> when get/put() are called.
>
> Actually, pm_clk is very simple case of OMAP PM domain which should only
> handle clocks.
>
> In non-DT case, we have possibility to divide clocks on "fck" and "opt"
> (The way it can be done is not convenient, but it is - .con_id).
>
> For DT-case - no way now. Also, PM domains are not physically present on
> Keystone 2 and GPD was selected as glue layer to integrate DT, pm_clk and
> PM runtime all together (one big-fat-global PM domain :).
>
> So, I was able to find only following way to define "fck" clocks in DT:
> 	clocks = <&papllclk>, <&clkcpgmac>, <&chipclk12>;
> 	clock-names = "clk_pa", "clk_cpgmac", "cpsw_cpts_rft_clk";
> 	fck-clocks = <&papllclk>, <&clkcpgmac>;
> As you can see - this will lead to data duplication in DT (
>
> Any propositions are welcome?

How about you proposed a new (optional) property for the clock bindings
called 'clocks-optional' or something like that.  The clock maintainer
(Mike Turquette, Cc'd) is also very familiar with OMAP and the need for
these different kinds of clocks.

Kevin

Grygorii Strashko Nov. 21, 2014, 8:14 p.m. UTC | #27

Hi Kevin,

On 11/21/2014 09:29 PM, Kevin Hilman wrote:
> Grygorii Strashko <[e-mail]> writes:
> 
>> Hi Kevin,
>> On 11/21/2014 10:06 AM, Geert Uytterhoeven wrote:
>>> On Fri, Nov 21, 2014 at 2:30 AM, Kevin Hilman <[e-mail]> wrote:
>>>> Geert Uytterhoeven <[e-mail]> writes:
>>>>> On Thu, Nov 20, 2014 at 10:48 PM, Kevin Hilman <[e-mail]> wrote:
>>>>>>>> So what exactly are we talking about with "PM" clocks, and why are they
>>>>>>>> "special" when it comes to PM domains?  IOW, why are the clocks to be
>>>>>>>> managed during PM domain transitions for a given device any different
>>>>>>>> than the clocks that need to be managed for a runtime suspend/resume (or
>>>>>>>> system suspend/resume) sequence for the same device?
>>>>>>>
>>>>>>> (Speaking for my case, shmobile)
>>>>>>>
>>>>>>> They're not. The clocks to be managed during PM domain transitions are the
>>>>>>> same as the clocks that need to be managed for a runtime suspend/resume
>>>>>>> (or system suspend/resume) sequence.
>>>>>>>
>>>>>>> The special thing is that this is more a platform than a driver thing: the same
>>>>>>> module may have a "PM/functional" clock (that is documented to enable/disable
>>>>>>> the module) on one Soc, but noet on another.
>>>>>>
>>>>>> So why isn't the presence or absence of the clock described in the .dtsi
>>>>>> for the SoC instead of being handled by special PM domain logic?
>>>>>
>>>>> It is. Cfr. the presence/absence of clocks for renesas,rcar-gpio nodes.
>>>>
>>>> Hmm, OK, Good.
>>>>
>>>> So now I'm confused about why the PM domain has to do anything special
>>>> if the presence/absence of the clocks is already handled by the DT.
>>>
>>> Just adding a clock property to a device node in DT doesn't enable the clock
>>> automatically, nor make it runtime-managed automatically.
>>> Compare this to e.g. pinctrl, where adding pinctrl properties to DT does enable
>>> them automatically, without the driver for the device having to care about it.
>>>
>>> Drivers interfacing external hardware typically do care about clocks, as they
>>> have to program clock generators for the external hardware interface (e.g.
>>> driving spi or i2c buses at specific frequencies).
>>>
>>> Other random drivers don't care about clocks, so they don't handle them.
>>> But as long as they make basic pm_runtime_{enable,get_sync,put}() calls,
>>> the (optional) clocks (and hardware PM domains) will "work" fine, if handled
>>> by the PM (clock) domain.
>>
>> Personally, I don't see problems with "functional" clocks.
>> The problem, in my opinion, is that we can't specify in DT that some clock is
>> "optional", so no one should touch such clock except driver.
>> For example:
>> Keystone 2 NETCP module has 3 clocks:
>> 	clocks = <&papllclk>, <&clkcpgmac>, <&chipclk12>;
>> 	clock-names = "clk_pa", "clk_cpgmac", "cpsw_cpts_rft_clk";
>>   and CPTS functionality is optional (in general) and can be enabled/disabled.
>> Also, usual example is MMC hosts
>> - OMAP hsmmc has "functional" clock "hsmmc1_fclk", and may have
>> "optional" clock "mmchsdb_fck". As you may remember, PM runtime
>> for OMAP2+ implemented by OMAP PM domain which performs many things,
>> but one thing which is done always is enabling/disabling of "fck"
>> when get/put() are called.
>>
>> Actually, pm_clk is very simple case of OMAP PM domain which should only
>> handle clocks.
>>
>> In non-DT case, we have possibility to divide clocks on "fck" and "opt"
>> (The way it can be done is not convenient, but it is - .con_id).
>>
>> For DT-case - no way now. Also, PM domains are not physically present on
>> Keystone 2 and GPD was selected as glue layer to integrate DT, pm_clk and
>> PM runtime all together (one big-fat-global PM domain :).
>>
>> So, I was able to find only following way to define "fck" clocks in DT:
>> 	clocks = <&papllclk>, <&clkcpgmac>, <&chipclk12>;
>> 	clock-names = "clk_pa", "clk_cpgmac", "cpsw_cpts_rft_clk";
>> 	fck-clocks = <&papllclk>, <&clkcpgmac>;
>> As you can see - this will lead to data duplication in DT (
>>
>> Any propositions are welcome?
> 
> How about you proposed a new (optional) property for the clock bindings
> called 'clocks-optional' or something like that.  The clock maintainer
> (Mike Turquette, Cc'd) is also very familiar with OMAP and the need for
> these different kinds of clocks.

'clocks-optional' - is not good, as for me. DT ABI:( Many drivers have to get "fck" 
even in case they don't need to manage them - usual case is to get
clock rate (SPI, I2C, MDIO,.. as mentioned by Geert).
Also, CLK core is using clock's names when it is looking for requested clock in DT.
Also, CLK core uses sequential indexing of clock.

Or, may be i don't fully understand, how 'clocks-optional' can be handled :( ?

'clocks-functional'- is better. 
"Optional. List of phandle and functional clock specifier pairs, one pair
 for each clock input to the device. 
 Note: Functional clock is mandatory clock which supplies the functional part
 of a module or a subsystem and which must be operational always when 
 a module or a subsystem is enabled and must be to complete I/O operations as needed.
 Note: it can be empty - then all clocks specified in 'clocks' property will be treated as
 functional"

What do you think?

regards,
-grygorii

Arnd Bergmann Nov. 24, 2014, 10:50 a.m. UTC | #28

On Friday 21 November 2014 20:58:01 Grygorii Strashko wrote:
> Hi Kevin,
> On 11/21/2014 10:06 AM, Geert Uytterhoeven wrote:
> > On Fri, Nov 21, 2014 at 2:30 AM, Kevin Hilman <[e-mail]> wrote:
> >> Geert Uytterhoeven <[e-mail]> writes:
> >>
> >> So now I'm confused about why the PM domain has to do anything special
> >> if the presence/absence of the clocks is already handled by the DT.
> > 
> > Just adding a clock property to a device node in DT doesn't enable the clock
> > automatically, nor make it runtime-managed automatically.
> > Compare this to e.g. pinctrl, where adding pinctrl properties to DT does enable
> > them automatically, without the driver for the device having to care about it.
> >
> > Drivers interfacing external hardware typically do care about clocks, as they
> > have to program clock generators for the external hardware interface (e.g.
> > driving spi or i2c buses at specific frequencies).

But is this a property of the driver or of the device? If this is true
independent of the driver implementation, I don't see a problem with 
the approach of linking to a power-domain that automatically manages
all clocks for the devices that need this, and requires the driver to
manage them itself when there are any clocks that can't be handled
with the generic clk-power-domain implementation.
 
> 
> In non-DT case, we have possibility to divide clocks on "fck" and "opt"
> (The way it can be done is not convenient, but it is - .con_id).
> 
> For DT-case - no way now. Also, PM domains are not physically present on
> Keystone 2 and GPD was selected as glue layer to integrate DT, pm_clk and
> PM runtime all together (one big-fat-global PM domain :).
> 
> So, I was able to find only following way to define "fck" clocks in DT:
> 	clocks = <&papllclk>, <&clkcpgmac>, <&chipclk12>;
> 	clock-names = "clk_pa", "clk_cpgmac", "cpsw_cpts_rft_clk";
> 	fck-clocks = <&papllclk>, <&clkcpgmac>;
> As you can see - this will lead to data duplication in DT (
> 
> Any propositions are welcome?
> 
> Unfortunately, It seems that if we would not able to find DT solution
> then there will be following ways to move forward:
> - "remove the power domain proxy from your drivers and use the clocks directly" 
> ((c) Arnd Bergmann).
> [As possibility - It can be allowed to use clk_pm APIs by drivers]
> - continue using platform specific implementations.

Could the driver maybe identify the clocks that it wants to manage itself
to the pm-domain code? If it's safe for a device to have the clock turned
on at the default rate before loading the driver, any device that is connected
to the simple clk-pm-domain code could have all its clocks start out as
owned by the pm-domain, but then claim the clocks it needs to reprogram for
itself and take them out of the pmdomain.

	Arnd

Mike Turquette Nov. 25, 2014, 6:44 a.m. UTC | #29

Quoting Arnd Bergmann (2014-11-24 02:50:28)
> On Friday 21 November 2014 20:58:01 Grygorii Strashko wrote:
> > Hi Kevin,
> > On 11/21/2014 10:06 AM, Geert Uytterhoeven wrote:
> > > On Fri, Nov 21, 2014 at 2:30 AM, Kevin Hilman <[e-mail]> wrote:
> > >> Geert Uytterhoeven <[e-mail]> writes:
> > >>
> > >> So now I'm confused about why the PM domain has to do anything special
> > >> if the presence/absence of the clocks is already handled by the DT.
> > > 
> > > Just adding a clock property to a device node in DT doesn't enable the clock
> > > automatically, nor make it runtime-managed automatically.
> > > Compare this to e.g. pinctrl, where adding pinctrl properties to DT does enable
> > > them automatically, without the driver for the device having to care about it.
> > >
> > > Drivers interfacing external hardware typically do care about clocks, as they
> > > have to program clock generators for the external hardware interface (e.g.
> > > driving spi or i2c buses at specific frequencies).
> 
> But is this a property of the driver or of the device? If this is true
> independent of the driver implementation, I don't see a problem with 
> the approach of linking to a power-domain that automatically manages
> all clocks for the devices that need this, and requires the driver to
> manage them itself when there are any clocks that can't be handled
> with the generic clk-power-domain implementation.
>  
> > 
> > In non-DT case, we have possibility to divide clocks on "fck" and "opt"
> > (The way it can be done is not convenient, but it is - .con_id).
> > 
> > For DT-case - no way now. Also, PM domains are not physically present on
> > Keystone 2 and GPD was selected as glue layer to integrate DT, pm_clk and
> > PM runtime all together (one big-fat-global PM domain :).
> > 
> > So, I was able to find only following way to define "fck" clocks in DT:
> >       clocks = <&papllclk>, <&clkcpgmac>, <&chipclk12>;
> >       clock-names = "clk_pa", "clk_cpgmac", "cpsw_cpts_rft_clk";
> >       fck-clocks = <&papllclk>, <&clkcpgmac>;
> > As you can see - this will lead to data duplication in DT (
> > 
> > Any propositions are welcome?
> > 
> > Unfortunately, It seems that if we would not able to find DT solution
> > then there will be following ways to move forward:
> > - "remove the power domain proxy from your drivers and use the clocks directly" 
> > ((c) Arnd Bergmann).
> > [As possibility - It can be allowed to use clk_pm APIs by drivers]
> > - continue using platform specific implementations.
> 
> Could the driver maybe identify the clocks that it wants to manage itself
> to the pm-domain code? If it's safe for a device to have the clock turned
> on at the default rate before loading the driver, any device that is connected
> to the simple clk-pm-domain code could have all its clocks start out as
> owned by the pm-domain, but then claim the clocks it needs to reprogram for
> itself and take them out of the pmdomain.

I was thinking along similar lines. The functional versus optional stuff
is really a property of the consuming device, not the clock signal
itself.

Instead of adding a new property to the clock binding (e.g. fck-clocks
or optional-clocks), could we simply wrap those lists of clocks in
another node? E.g:

mandatory-clocks {
       clocks = <&papllclk>, <&clkcpgmac>;
       clock-names = "clk_pa", "clk_cpgmac";
}

clocks = <&papllclk>, <&clkcpgmac>, <&chipclk12>;
       clocks = <&clkcpgmac>;
       clock-names = "cpsw_cpts_rft_clk";
}

I'm showing my DT ignorance on this one. I haven't really thought
through how these sub-nodes would work with of_clk_* handlers in
drivers/clk/clkdev.c.

Regards,
Mike

> 
>         Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Arnd Bergmann Nov. 25, 2014, 10:33 a.m. UTC | #30

On Monday 24 November 2014 22:44:06 Mike Turquette wrote:
> Quoting Arnd Bergmann (2014-11-24 02:50:28)
> > 
> > Could the driver maybe identify the clocks that it wants to manage itself
> > to the pm-domain code? If it's safe for a device to have the clock turned
> > on at the default rate before loading the driver, any device that is connected
> > to the simple clk-pm-domain code could have all its clocks start out as
> > owned by the pm-domain, but then claim the clocks it needs to reprogram for
> > itself and take them out of the pmdomain.
> 
> I was thinking along similar lines. The functional versus optional stuff
> is really a property of the consuming device, not the clock signal
> itself.
> 
> Instead of adding a new property to the clock binding (e.g. fck-clocks
> or optional-clocks), could we simply wrap those lists of clocks in
> another node? E.g:
> 
> mandatory-clocks {
>        clocks = <&papllclk>, <&clkcpgmac>;
>        clock-names = "clk_pa", "clk_cpgmac";
> }
> 
> clocks = <&papllclk>, <&clkcpgmac>, <&chipclk12>;
>        clocks = <&clkcpgmac>;
>        clock-names = "cpsw_cpts_rft_clk";
> }
> 
> I'm showing my DT ignorance on this one. I haven't really thought
> through how these sub-nodes would work with of_clk_* handlers in
> drivers/clk/clkdev.c.

I'm not sure I even understand what you intended the example to look
like, it does't parse ;-)

My point above was completely different, the suggestion I made was
to not classify the clocks in DT at all, but to leave it all in
the client driver.

	Arnd

Grygorii Strashko Nov. 25, 2014, 11:08 a.m. UTC | #31

Hi Arnd,

On 11/25/2014 12:33 PM, Arnd Bergmann wrote:
> On Monday 24 November 2014 22:44:06 Mike Turquette wrote:
>> Quoting Arnd Bergmann (2014-11-24 02:50:28)
>>>
>>> Could the driver maybe identify the clocks that it wants to manage itself
>>> to the pm-domain code? If it's safe for a device to have the clock turned
>>> on at the default rate before loading the driver, any device that is connected
>>> to the simple clk-pm-domain code could have all its clocks start out as
>>> owned by the pm-domain, but then claim the clocks it needs to reprogram for
>>> itself and take them out of the pmdomain.
>>
>> I was thinking along similar lines. The functional versus optional stuff
>> is really a property of the consuming device, not the clock signal
>> itself.
>>
>> Instead of adding a new property to the clock binding (e.g. fck-clocks
>> or optional-clocks), could we simply wrap those lists of clocks in
>> another node? E.g:
>>
>> mandatory-clocks {
>>         clocks = <&papllclk>, <&clkcpgmac>;
>>         clock-names = "clk_pa", "clk_cpgmac";
>> }
>>
>> clocks = <&papllclk>, <&clkcpgmac>, <&chipclk12>;

optional-clocks { ?
>>         clocks = <&clkcpgmac>;         

	   clocks = <&chipclk12>; ?

>>         clock-names = "cpsw_cpts_rft_clk";
>> }
>>
>> I'm showing my DT ignorance on this one. I haven't really thought
>> through how these sub-nodes would work with of_clk_* handlers in
>> drivers/clk/clkdev.c.

As I remember, there was nack for sub-nodes approach from Olof :(
"[RFC 0/2] pwrseq: Add subsystem for power sequences"
http://thread.gmane.org/gmane.linux.power-management.general/46635

> 
> I'm not sure I even understand what you intended the example to look
> like, it does't parse ;-)
> 
> My point above was completely different, the suggestion I made was
> to not classify the clocks in DT at all, but to leave it all in
> the client driver.

I slept with this idea :) From one side it sounds good. Pls, Correct me if I'm wrong:
- there still will be "simple-pmdomain" and all devices will be attached to it by
   default (or as specified in DT power-domains = <&simple_pmdomain>;);
- drivers will use smth. like pm_clk_remove() to remove optional clocks from pm_clk;

From another side:
- drivers will get dependency from pm_clk;
- HW limitations can't be taken into account - it's possible that some clocks should
  not be enabled until it's allowed. And only driver know when it's allowed.
  Otherwise, HW state may become unspecified or wrong output can be generated.

regards,
-grygorii

Arnd Bergmann Nov. 25, 2014, 12:09 p.m. UTC | #32

On Tuesday 25 November 2014 13:08:57 Grygorii Strashko wrote:
> On 11/25/2014 12:33 PM, Arnd Bergmann wrote:
> > On Monday 24 November 2014 22:44:06 Mike Turquette wrote:
> >> Quoting Arnd Bergmann (2014-11-24 02:50:28)
> >>>
> > 
> > I'm not sure I even understand what you intended the example to look
> > like, it does't parse 
> > 
> > My point above was completely different, the suggestion I made was
> > to not classify the clocks in DT at all, but to leave it all in
> > the client driver.
> 
> I slept with this idea  From one side it sounds good. Pls, Correct me if I'm wrong:
> - there still will be "simple-pmdomain" and all devices will be attached to it by
>    default (or as specified in DT power-domains = <&simple_pmdomain>;);

I would assume only devices that set "power-domains = <&simple_pmdomain>"

> - drivers will use smth. like pm_clk_remove() to remove optional clocks from pm_clk;

Right. Regarding the naming of the function, I would pick something other
than remove, since the main purpose is not to have that clock abandoned
by the pm-domain code (this is still a side-effect), but to have the
clock put under control of the driver itself.

It might be possible to do this implicitly if the driver calls clk_get(),
basically doing clk_get() (or another call if necessary) would prevent the
simple pmdomain from turning it off during suspend.

> From another side:
> - drivers will get dependency from pm_clk;

There are three cases here:

- A device that is always used with a pm-domain, the driver doesn't
  have to worry about it but do need the dependency on having the
  simple-pmdomain code enabled.

- A device that may or may not have clocks, but if it has them, they
  are managed through a pm-domain. In this case, it's platform dependent
  whether we have the dependency. We may want to prevent the device from
  being probed if a power-domain property is present but no driver
  for the domain.

- A device that uses the pm-domain on some machines but not on others:
  this is a bit tricky, because the driver will still have to know
  about all the clocks, although we could choose not to turn off the
  clocks during suspend if the power-domain is not set.

> - HW limitations can't be taken into account - it's possible that some clocks should
>   not be enabled until it's allowed. And only driver know when it's allowed.
>   Otherwise, HW state may become unspecified or wrong output can be generated.

Correct: if you have a device that you don't want to be handled by a simple
pm-domain, then you have to connect it to a different pm-domain, e.g. one that
manages a fixed set of clocks itself.

	Arnd

Grygorii Strashko Nov. 25, 2014, 1:30 p.m. UTC | #33

On 11/25/2014 02:09 PM, Arnd Bergmann wrote:
> On Tuesday 25 November 2014 13:08:57 Grygorii Strashko wrote:
>> On 11/25/2014 12:33 PM, Arnd Bergmann wrote:
>>> On Monday 24 November 2014 22:44:06 Mike Turquette wrote:
>>>> Quoting Arnd Bergmann (2014-11-24 02:50:28)
>>>>>
>>>
>>> I'm not sure I even understand what you intended the example to look
>>> like, it does't parse
>>>
>>> My point above was completely different, the suggestion I made was
>>> to not classify the clocks in DT at all, but to leave it all in
>>> the client driver.
>>
>> I slept with this idea  From one side it sounds good. Pls, Correct me if I'm wrong:
>> - there still will be "simple-pmdomain" and all devices will be attached to it by
>>     default (or as specified in DT power-domains = <&simple_pmdomain>;);
> 
> I would assume only devices that set "power-domains = <&simple_pmdomain>"
> 
>> - drivers will use smth. like pm_clk_remove() to remove optional clocks from pm_clk;
> 
> Right. Regarding the naming of the function, I would pick something other
> than remove, since the main purpose is not to have that clock abandoned
> by the pm-domain code (this is still a side-effect), but to have the
> clock put under control of the driver itself.

pm_clk_remove() is implemented already.

> 
> It might be possible to do this implicitly if the driver calls clk_get(),
> basically doing clk_get() (or another call if necessary) would prevent the
> simple pmdomain from turning it off during suspend.

Unfortunately, clk_get() will not work, because drivers still need to use it
to get functional clocks even if they are not going to control them explicitly.
For example, if it need to know clock's rate.

> 
>>  From another side:
>> - drivers will get dependency from pm_clk;
> 
> There are three cases here:
> 
> - A device that is always used with a pm-domain, the driver doesn't
>    have to worry about it but do need the dependency on having the
>    simple-pmdomain code enabled.
> 
> - A device that may or may not have clocks, but if it has them, they
>    are managed through a pm-domain. In this case, it's platform dependent
>    whether we have the dependency. We may want to prevent the device from
>    being probed if a power-domain property is present but no driver
>    for the domain.

Seems, solved by PM core - .probe() will be deferred forever if pm-domain is
not ready.

> 
> - A device that uses the pm-domain on some machines but not on others:
>    this is a bit tricky, because the driver will still have to know
>    about all the clocks, although we could choose not to turn off the
>    clocks during suspend if the power-domain is not set.
> 
>> - HW limitations can't be taken into account - it's possible that some clocks should
>>    not be enabled until it's allowed. And only driver know when it's allowed.
>>    Otherwise, HW state may become unspecified or wrong output can be generated.
> 
> Correct: if you have a device that you don't want to be handled by a simple
> pm-domain, then you have to connect it to a different pm-domain, e.g. one that
> manages a fixed set of clocks itself.

regards,
-grygorii

Russell King - ARM Linux Nov. 25, 2014, 2:04 p.m. UTC | #34

On Tue, Nov 25, 2014 at 03:30:20PM +0200, Grygorii Strashko wrote:
> On 11/25/2014 02:09 PM, Arnd Bergmann wrote:
> > It might be possible to do this implicitly if the driver calls clk_get(),
> > basically doing clk_get() (or another call if necessary) would prevent the
> > simple pmdomain from turning it off during suspend.
> 
> Unfortunately, clk_get() will not work, because drivers still need to use it
> to get functional clocks even if they are not going to control them explicitly.
> For example, if it need to know clock's rate.

If you don't want a clock to be turned off, then clk_get() it, then
clk_prepare() it, and finally clk_enable() it.

Even if someone else gets it, the only time that a clock is turned off
is when _all_ users of it mutually agree that it can be turned off - by
every user disabling (and possibly unpreparing) it.

So, if the PM domain code gets a clock, prepares and enables it, then
a driver gets the same clock, prepares and enables it also, it won't
be disabled until _both_ the PM domain code _and_ the driver disable
and unprepare the clock.

Grygorii Strashko Nov. 25, 2014, 2:53 p.m. UTC | #35

Hi Russell,

On 11/25/2014 04:04 PM, Russell King - ARM Linux wrote:
> On Tue, Nov 25, 2014 at 03:30:20PM +0200, Grygorii Strashko wrote:
>> On 11/25/2014 02:09 PM, Arnd Bergmann wrote:
>>> It might be possible to do this implicitly if the driver calls clk_get(),
>>> basically doing clk_get() (or another call if necessary) would prevent the
>>> simple pmdomain from turning it off during suspend.
>>
>> Unfortunately, clk_get() will not work, because drivers still need to use it
>> to get functional clocks even if they are not going to control them explicitly.
>> For example, if it need to know clock's rate.
> 
> If you don't want a clock to be turned off, then clk_get() it, then
> clk_prepare() it, and finally clk_enable() it.
> 
> Even if someone else gets it, the only time that a clock is turned off
> is when _all_ users of it mutually agree that it can be turned off - by
> every user disabling (and possibly unpreparing) it.
> 
> So, if the PM domain code gets a clock, prepares and enables it, then
> a driver gets the same clock, prepares and enables it also, it won't
> be disabled until _both_ the PM domain code _and_ the driver disable
> and unprepare the clock.
> 

All 100% true :)

But the question here is how prevent pm_clk domain (clock_ops.c) from
getting the control on clock if this particular clock is optional from driver's
perspective. So, only driver should control it. As opposite, all other clocks
should be controlled by pm-domain (in case of GPD from .start/stop callbacks).

You can find more detailed description of problem which this patch was
created to solve here:
https://lkml.org/lkml/2014/11/19/225

Thanks for your time.

regards,
-grygorii

Santosh Shilimkar Nov. 25, 2014, 4:28 p.m. UTC | #36

Grygorii,

On 11/25/2014 6:53 AM, Grygorii Strashko wrote:
> Hi Russell,
>
> On 11/25/2014 04:04 PM, Russell King - ARM Linux wrote:
>> On Tue, Nov 25, 2014 at 03:30:20PM +0200, Grygorii Strashko wrote:
>>> On 11/25/2014 02:09 PM, Arnd Bergmann wrote:
>>>> It might be possible to do this implicitly if the driver calls clk_get(),
>>>> basically doing clk_get() (or another call if necessary) would prevent the
>>>> simple pmdomain from turning it off during suspend.
>>>
>>> Unfortunately, clk_get() will not work, because drivers still need to use it
>>> to get functional clocks even if they are not going to control them explicitly.
>>> For example, if it need to know clock's rate.
>>
>> If you don't want a clock to be turned off, then clk_get() it, then
>> clk_prepare() it, and finally clk_enable() it.
>>
>> Even if someone else gets it, the only time that a clock is turned off
>> is when _all_ users of it mutually agree that it can be turned off - by
>> every user disabling (and possibly unpreparing) it.
>>
>> So, if the PM domain code gets a clock, prepares and enables it, then
>> a driver gets the same clock, prepares and enables it also, it won't
>> be disabled until _both_ the PM domain code _and_ the driver disable
>> and unprepare the clock.
>>
>
> All 100% true :)
>
> But the question here is how prevent pm_clk domain (clock_ops.c) from
> getting the control on clock if this particular clock is optional from driver's
> perspective. So, only driver should control it. As opposite, all other clocks
> should be controlled by pm-domain (in case of GPD from .start/stop callbacks).
>
I know there has been lot of back and forth, we have done on getting
multiple clock handling and from the thread looks like we are converging
with simple power domain approach. Since we have already made several
different attempt to solve the problem and some how stumbled, I suggest
you to do a new RFC with new $subject so that we can get rest of the
folks eyes on it.

The reason I say that because may be just because of $subject, some
folks might ignore the thread ;-) and later raise objections.

Thanks for persisting with it.

Regards,
Santosh

Patch

diff --git a/Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt b/Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt
new file mode 100644
index 0000000..d4e24d5
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/ti,keystone-powerdomain.txt
@@ -0,0 +1,31 @@ 
+* TI Keystone 2 Generic PM Controller
+
+The TI Keystone 2 Generic PM Controller is responsible for Clock gating
+for each controlled IP module.
+
+Required properties:
+- compatible: Should be "ti,keystone-powerdomain"
+- #power-domain-cells: Should be 0, see below:
+
+The PM Controller node is a PM domain as documented in
+Documentation/devicetree/bindings/power/power_domain.txt.
+
+Example:
+
+	pm_controller: pm-controller {
+		compatible = "ti,keystone-powerdomain";
+		#power-domain-cells = <0>;
+	};
+
+	netcp: netcp@2090000 {
+		reg = <0x2620110 0x8>;
+		reg-names = "efuse";
+		...
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+		power-domains = <&pm_controller>;
+
+		clocks = <&clkpa>, <&clkcpgmac>, <&chipclk12>;
+		dma-coherent;
+	}
diff --git a/arch/arm/mach-keystone/Kconfig b/arch/arm/mach-keystone/Kconfig
index 98a156a..de43107 100644
--- a/arch/arm/mach-keystone/Kconfig
+++ b/arch/arm/mach-keystone/Kconfig
@@ -9,6 +9,7 @@  config ARCH_KEYSTONE
 	select COMMON_CLK_KEYSTONE
 	select ARCH_SUPPORTS_BIG_ENDIAN
 	select ZONE_DMA if ARM_LPAE
+	select PM_GENERIC_DOMAINS if PM
 	help
 	  Support for boards based on the Texas Instruments Keystone family of
 	  SoCs.
diff --git a/arch/arm/mach-keystone/pm_domain.c b/arch/arm/mach-keystone/pm_domain.c
index ca79dda..8f243c2 100644
--- a/arch/arm/mach-keystone/pm_domain.c
+++ b/arch/arm/mach-keystone/pm_domain.c
@@ -12,69 +12,101 @@ 
  * version 2, as published by the Free Software Foundation.
  */
 
+#include <linux/clk.h>
 #include <linux/init.h>
-#include <linux/pm_runtime.h>
 #include <linux/pm_clock.h>
+#include <linux/pm_domain.h>
 #include <linux/platform_device.h>
-#include <linux/clk-provider.h>
 #include <linux/of.h>
 
-#ifdef CONFIG_PM_RUNTIME
-static int keystone_pm_runtime_suspend(struct device *dev)
+#ifdef CONFIG_PM_GENERIC_DOMAINS
+
+struct keystone_domain {
+	struct generic_pm_domain genpd;
+	struct device	*dev;
+};
+
+void keystone_pm_domain_attach_dev(struct device *dev)
 {
+	struct clk *clk;
 	int ret;
+	int i = 0;
 
 	dev_dbg(dev, "%s\n", __func__);
 
-	ret = pm_generic_runtime_suspend(dev);
-	if (ret)
-		return ret;
-
-	ret = pm_clk_suspend(dev);
+	ret = pm_clk_create(dev);
 	if (ret) {
-		pm_generic_runtime_resume(dev);
-		return ret;
+		dev_err(dev, "pm_clk_create failed %d\n", ret);
+		return;
+	};
+
+	while ((clk = of_clk_get(dev->of_node, i++)) && !IS_ERR(clk)) {
+		ret = pm_clk_add_clk(dev, clk);
+		clk_put(clk);
+		if (ret) {
+			dev_err(dev, "pm_clk_add_clk failed %d\n", ret);
+			goto clk_err;
+		};
 	}
 
-	return 0;
+	return;
+
+clk_err:
+	pm_clk_destroy(dev);
 }
 
-static int keystone_pm_runtime_resume(struct device *dev)
+void keystone_pm_domain_detach_dev(struct device *dev)
 {
 	dev_dbg(dev, "%s\n", __func__);
-
-	pm_clk_resume(dev);
-
-	return pm_generic_runtime_resume(dev);
+	pm_clk_destroy(dev);
 }
-#endif
 
-static struct dev_pm_domain keystone_pm_domain = {
-	.ops = {
-		SET_RUNTIME_PM_OPS(keystone_pm_runtime_suspend,
-				   keystone_pm_runtime_resume, NULL)
-		USE_PLATFORM_PM_SLEEP_OPS
+static const struct keystone_domain keystone_domain = {
+	.genpd = {
+		.name = "keystone",
+		.attach_dev = keystone_pm_domain_attach_dev,
+		.detach_dev = keystone_pm_domain_detach_dev,
+		.dev_ops = {
+			.stop = pm_clk_suspend,
+			.start = pm_clk_resume,
+		},
 	},
 };
 
-static struct pm_clk_notifier_block platform_domain_notifier = {
-	.pm_domain = &keystone_pm_domain,
+static int keystone_pm_domain_probe(struct platform_device *pdev)
+{
+	struct keystone_domain *domain;
+
+	domain = devm_kzalloc(&pdev->dev,
+			      sizeof(struct keystone_domain), GFP_KERNEL);
+	if (!domain)
+		return -ENOMEM;
+
+	domain->genpd = keystone_domain.genpd;
+	domain->dev = &pdev->dev;
+
+	pm_genpd_init(&domain->genpd, NULL, false);
+	return of_genpd_add_provider_simple(pdev->dev.of_node, &domain->genpd);
+}
+
+static struct of_device_id keystone_pm_domain_dt_ids[] = {
+	{ .compatible = "ti,keystone-powerdomain" },
+	{ }
 };
 
-static struct of_device_id of_keystone_table[] = {
-	{.compatible = "ti,keystone"},
-	{ /* end of list */ },
+static struct platform_driver keystone_pm_domain_driver = {
+	.driver = {
+		.name = "ti,keystone-powerdomain",
+		.owner = THIS_MODULE,
+		.of_match_table = keystone_pm_domain_dt_ids,
+	},
+	.probe = keystone_pm_domain_probe,
 };
 
 int __init keystone_pm_runtime_init(void)
 {
-	struct device_node *np;
-
-	np = of_find_matching_node(NULL, of_keystone_table);
-	if (!np)
-		return 0;
-
-	pm_clk_add_notifier(&platform_bus_type, &platform_domain_notifier);
-
-	return 0;
+	return platform_driver_register(&keystone_pm_domain_driver);
 }
+#else
+int __init keystone_pm_runtime_init(void) { return 0; }
+#endif /* CONFIG_PM_GENERIC_DOMAINS */