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

[Arnd Bergmann <arnd@arndb.de>]

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 <arnd@arndb.de> 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