Re: [PATCH] soc: rockchip: power-domain: remove PM clocks

Re: [PATCH] soc: rockchip: power-domain: remove PM clocks

[Ulf Hansson]

+linux-pm

Geert, Jeffry, Tomasz,

Apologize for side-tracking the discussion. Just wanted to add a few
comments, whatever it's worth to you.

On 1 March 2018 at 09:33, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Jeffy,
>
> On Thu, Mar 1, 2018 at 4:40 AM, JeffyChen <jeffy.chen@rock-chips.com> wrote:
>> if i'm reading the code right, the PM clk means:
>> 1/ the clocks which would be enabled while power on
>> 2/ these clocks are optional, it's ok if anything wrong with them
>> 3/ controlled by pm_domain(or USE_PM_CLK_RUNTIME_OPS & pm_clk_add_notifier)
>>
>> and currently we're adding all clocks of the attached device as PM clk in
>> rockchip PM domain driver, which seems wrong. because we might have these
>> kinds of clocks:
>> 1/ critical, should block power on if anything wrong with it(failed to get/
>> prepare/ enable)
>> 2/ optional, could ignore it if anything wrong
>> 3/ only required in some special cases, for example register r/w, and
>> doesn't need to stay enabled while power on
>>
>> so maybe we can:
>> 1/ let the device(dts) or driver decide which clock is PM clk, and add it
>> using *pm_clk_add* APIs (even of_pm_clk_add_clks() if all clocks are pm clk)

We have already tried adding DT binding for this. Those got correctly
nacked, as this seems like a SW config and not HW config, at least in
my opinion.

>>
>> 2/ add support for critical PM clk, which would return error to the driver
>> if anything wrong
>>
>> 3/ make sure PM clk always be controlled(otherwise it might be unexpected
>> disabled by other clocks under the same clk parent?):
>>  a) make sure Runtime PM is always enabled. and as discussed, we can select
>> PM in ARCH_ROCKCHIP

I am fine enabling PM for ARCH_ROCKCHIP, if needed.

However, what I don't agree with in general, is to make a generic
driver to rely on having CONFIG_PM to be set to be functional. That's
to me, bad practice.

I understand, this approach exists in drivers today. I assume it
works, because those drivers are being used on SoCs which always has
CONFIG_PM set.

>
> On Renesas SoCs, we only add the device's module clock with pm_clk_add().
> Drivers that don't care about properties of the module clock just call
> pm_runtime_*(). That way the same driver works on different SoCs using
> the same device, with and without power and/or clock domains.

I understand your point and I accept your view.

However, I think this is more a mindset of which way one want
implement things. This has been discussed several times in the mailing
list as well.

Surely we can cope with SoC specific constraints in drivers as well,
we already do that.

In principle I think this boils done to comparing a centralized
method, where the PM domain deals with clocks vs a decentralized
method, where the driver deals with clocks. Both works, both have
positive and negative consequences.

In my experience for ARM SoCs, I have found that centralized method
doesn't work well, when one need flexibility. For example, if there
are strict constraints on the order of how to put device's PM
resources (clocks, pinctrl, etc) in low power states. For example, to
avoid clock glitches.

Another problem with the PM clk is, more exactly with
pm_clk_suspend|resume(), that those invokes only clk_enable|disable().
pm_clk_suspend|resume() can't call clk_prepare|unprepare(), because we
don't know if we running in atomic context when those are executed.
Potentially this means leaving the clocks ungated - all the time.

I have though about how to fix the above, several times, but I always
ends up with thinking that's it more easy, to let the driver deal with
the clocks, as then the problem goes away.

>
> Drivers that care about properties of the module clock (mainly frequency)
> can still use the clk_*() API for that. Other (optional) clocks must be
> handled by the device driver itself.

A comment on that;

Before we the PM clk was introduced, we didn't have the clk_bulk_*()
interface. To me, using clk_bulk_*() in drivers could help to simplify
the code in regards to manage clocks (including SoC specific clocks)
during runtime PM.

Perhaps this could be an option to using PM clk, as it provides both
flexibility and could manage SoC specific clocks.

Kind regards
Uffe

Re: [PATCH] soc: rockchip: power-domain: remove PM clocks

[Geert Uytterhoeven]

Hi Ulf,

On Thu, Mar 1, 2018 at 11:18 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 1 March 2018 at 09:33, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Thu, Mar 1, 2018 at 4:40 AM, JeffyChen <jeffy.chen@rock-chips.com> wrote:
>>> 3/ make sure PM clk always be controlled(otherwise it might be unexpected
>>> disabled by other clocks under the same clk parent?):
>>>  a) make sure Runtime PM is always enabled. and as discussed, we can select
>>> PM in ARCH_ROCKCHIP
>
> I am fine enabling PM for ARCH_ROCKCHIP, if needed.
>
> However, what I don't agree with in general, is to make a generic
> driver to rely on having CONFIG_PM to be set to be functional. That's
> to me, bad practice.
>
> I understand, this approach exists in drivers today. I assume it
> works, because those drivers are being used on SoCs which always has
> CONFIG_PM set.

I agree. This is mostly useful for drivers that are used on multiple SoCs,
where the driver doesn't care about the clock (doesn't care about its
properties), and where the clock is optional (i.e. either tied to an
always-running bus clock, or to a controllable module clock, depending on SoC).
The latter needs CONFIG_PM=y, but that's a platform property, controlled
by selecting support for that specific SoC.

>> On Renesas SoCs, we only add the device's module clock with pm_clk_add().
>> Drivers that don't care about properties of the module clock just call
>> pm_runtime_*(). That way the same driver works on different SoCs using
>> the same device, with and without power and/or clock domains.
>
> I understand your point and I accept your view.
>
> However, I think this is more a mindset of which way one want
> implement things. This has been discussed several times in the mailing
> list as well.
>
> Surely we can cope with SoC specific constraints in drivers as well,
> we already do that.
>
> In principle I think this boils done to comparing a centralized
> method, where the PM domain deals with clocks vs a decentralized
> method, where the driver deals with clocks. Both works, both have
> positive and negative consequences.

Is the clock (are the clocks) used purely for power management of the device,
or does it/do they serve another (independent) purpose?

Note that unlike clocks, power areas cannot be controlled explicitly by the
driver. That always has to be done through the Runtime PM API.

> In my experience for ARM SoCs, I have found that centralized method
> doesn't work well, when one need flexibility. For example, if there
> are strict constraints on the order of how to put device's PM
> resources (clocks, pinctrl, etc) in low power states. For example, to
> avoid clock glitches.

With multiple clocks used by a device, there's sometimes also a lack of
understanding of the real clock hierarchy. If these multiple clocks need
to be enabled/disabled in a specific order, perhaps they are not
independent, and modelling the hierarchy correctly, and describing in DT
the device as being connected to the leaf clock may solve the ordering issue.

> Another problem with the PM clk is, more exactly with
> pm_clk_suspend|resume(), that those invokes only clk_enable|disable().
> pm_clk_suspend|resume() can't call clk_prepare|unprepare(), because we
> don't know if we running in atomic context when those are executed.
> Potentially this means leaving the clocks ungated - all the time.
>
> I have though about how to fix the above, several times, but I always
> ends up with thinking that's it more easy, to let the driver deal with
> the clocks, as then the problem goes away.

There's a similar issue with powering on/off power areas.
How do device start/stop differ from power domain on/off?

>> Drivers that care about properties of the module clock (mainly frequency)
>> can still use the clk_*() API for that. Other (optional) clocks must be
>> handled by the device driver itself.
>
> A comment on that;
>
> Before we the PM clk was introduced, we didn't have the clk_bulk_*()
> interface. To me, using clk_bulk_*() in drivers could help to simplify
> the code in regards to manage clocks (including SoC specific clocks)
> during runtime PM.
>
> Perhaps this could be an option to using PM clk, as it provides both
> flexibility and could manage SoC specific clocks.

See my comment about multiple clocks above...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH] soc: rockchip: power-domain: remove PM clocks

[Ulf Hansson]

On 1 March 2018 at 11:37, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ulf,
>
> On Thu, Mar 1, 2018 at 11:18 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 1 March 2018 at 09:33, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>>> On Thu, Mar 1, 2018 at 4:40 AM, JeffyChen <jeffy.chen@rock-chips.com> wrote:
>>>> 3/ make sure PM clk always be controlled(otherwise it might be unexpected
>>>> disabled by other clocks under the same clk parent?):
>>>>  a) make sure Runtime PM is always enabled. and as discussed, we can select
>>>> PM in ARCH_ROCKCHIP
>>
>> I am fine enabling PM for ARCH_ROCKCHIP, if needed.
>>
>> However, what I don't agree with in general, is to make a generic
>> driver to rely on having CONFIG_PM to be set to be functional. That's
>> to me, bad practice.
>>
>> I understand, this approach exists in drivers today. I assume it
>> works, because those drivers are being used on SoCs which always has
>> CONFIG_PM set.
>
> I agree. This is mostly useful for drivers that are used on multiple SoCs,
> where the driver doesn't care about the clock (doesn't care about its
> properties), and where the clock is optional (i.e. either tied to an
> always-running bus clock, or to a controllable module clock, depending on SoC).
> The latter needs CONFIG_PM=y, but that's a platform property, controlled
> by selecting support for that specific SoC.
>
>>> On Renesas SoCs, we only add the device's module clock with pm_clk_add().
>>> Drivers that don't care about properties of the module clock just call
>>> pm_runtime_*(). That way the same driver works on different SoCs using
>>> the same device, with and without power and/or clock domains.
>>
>> I understand your point and I accept your view.
>>
>> However, I think this is more a mindset of which way one want
>> implement things. This has been discussed several times in the mailing
>> list as well.
>>
>> Surely we can cope with SoC specific constraints in drivers as well,
>> we already do that.
>>
>> In principle I think this boils done to comparing a centralized
>> method, where the PM domain deals with clocks vs a decentralized
>> method, where the driver deals with clocks. Both works, both have
>> positive and negative consequences.
>
> Is the clock (are the clocks) used purely for power management of the device,
> or does it/do they serve another (independent) purpose?

Well, I am not talking about one specific case.

I guess what you are saying is that, devices may need different kind
of clocks. Some can be power managed, some can't. That seems
reasonable.

In either way, the driver should be able to cope with both kinds of
clocks. Right!?

>
> Note that unlike clocks, power areas cannot be controlled explicitly by the
> driver. That always has to be done through the Runtime PM API.

Yes, agree!

At least until/if we get multiple power areas (PM domain) support for
devices. Then this may change. However, that's a different story. :-)

>
>> In my experience for ARM SoCs, I have found that centralized method
>> doesn't work well, when one need flexibility. For example, if there
>> are strict constraints on the order of how to put device's PM
>> resources (clocks, pinctrl, etc) in low power states. For example, to
>> avoid clock glitches.
>
> With multiple clocks used by a device, there's sometimes also a lack of
> understanding of the real clock hierarchy. If these multiple clocks need
> to be enabled/disabled in a specific order, perhaps they are not
> independent, and modelling the hierarchy correctly, and describing in DT
> the device as being connected to the leaf clock may solve the ordering issue.

Nope, this scenario I had in mind isn't about the clock hierarchy.
Instead this is about other resources that the driver deals with
during runtime PM. Pinctrl, regulators, device internals registers,
etc.

I have stumbled of cases, where the order dealing with these
resources, simply need to be strict, thus it is better managed from
the driver's runtime PM callbacks.

What I am saying is that, if you have these kind of constraints, then
having clocks being managed at the PM domain level via PM clk and
other resources in the driver, simply isn't a good mix.

>
>> Another problem with the PM clk is, more exactly with
>> pm_clk_suspend|resume(), that those invokes only clk_enable|disable().
>> pm_clk_suspend|resume() can't call clk_prepare|unprepare(), because we
>> don't know if we running in atomic context when those are executed.
>> Potentially this means leaving the clocks ungated - all the time.
>>
>> I have though about how to fix the above, several times, but I always
>> ends up with thinking that's it more easy, to let the driver deal with
>> the clocks, as then the problem goes away.
>
> There's a similar issue with powering on/off power areas.

I don't follow. Can you elaborate?

> How do device start/stop differ from power domain on/off?

I guess what you refer to is that, genpd's ->start|stop() callbacks
may be assigned to pm_clk_suspend|resume(), and those may be called
from genps's runtime PM callbacks and genpd's system sleep callbacks.

Yes, both cases suffers from the same problem as I describe above.
Clocks may stay ungated - all the time.

>
>>> Drivers that care about properties of the module clock (mainly frequency)
>>> can still use the clk_*() API for that. Other (optional) clocks must be
>>> handled by the device driver itself.
>>
>> A comment on that;
>>
>> Before we the PM clk was introduced, we didn't have the clk_bulk_*()
>> interface. To me, using clk_bulk_*() in drivers could help to simplify
>> the code in regards to manage clocks (including SoC specific clocks)
>> during runtime PM.
>>
>> Perhaps this could be an option to using PM clk, as it provides both
>> flexibility and could manage SoC specific clocks.
>
> See my comment about multiple clocks above...
>

Kind regards
Uffe

Re: [PATCH] soc: rockchip: power-domain: remove PM clocks

[Geert Uytterhoeven]

Hi Ulf,

On Thu, Mar 1, 2018 at 12:22 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 1 March 2018 at 11:37, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Thu, Mar 1, 2018 at 11:18 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>> Another problem with the PM clk is, more exactly with
>>> pm_clk_suspend|resume(), that those invokes only clk_enable|disable().
>>> pm_clk_suspend|resume() can't call clk_prepare|unprepare(), because we
>>> don't know if we running in atomic context when those are executed.
>>> Potentially this means leaving the clocks ungated - all the time.
>>>
>>> I have though about how to fix the above, several times, but I always
>>> ends up with thinking that's it more easy, to let the driver deal with
>>> the clocks, as then the problem goes away.
>>
>> There's a similar issue with powering on/off power areas.
>
> I don't follow. Can you elaborate?

I intended to comment on the atomic context (or not).
But I think I was wrong, and PM domain drivers do busy loops instead
of sleeps.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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