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@xxxxxxxxxx> wrote:
> On 1 March 2018 at 09:33, Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>> On Thu, Mar 1, 2018 at 4:40 AM, JeffyChen <jeffy.chen@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx

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