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

[JeffyChen]

Hi Geert,

Thanks for your reply.

On 03/01/2018 04:33 PM, Geert Uytterhoeven wrote:
> > >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)
> > >
> > >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
> 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.

well, i think we may need to check:
1/ so the driver might not know is the clock really enabled(currently):

static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce)
{
    if (!ce->clk)
        ce->clk = clk_get(dev, ce->con_id);
    if (IS_ERR(ce->clk)) {
        ce->status = PCE_STATUS_ERROR;

static inline void __pm_clk_enable(struct device *dev, struct 
pm_clock_entry *ce)
{
    int ret;

    if (ce->status < PCE_STATUS_ERROR) {
        ret = clk_enable(ce->clk);
        if (!ret)
            ce->status = PCE_STATUS_ENABLED;

it seems the status is private.

2/ the pm_clk_resume/suspend seems only be called in the domain's 
suspend/resume ops(or USE_PM_CLK_RUNTIME_OPS, or called directly in the 
drivers for example tegra-aconnect):

# cgrep pm_clk_resume -w
./include/linux/pm_clock.h:50:extern int pm_clk_resume(struct device *dev);
./include/linux/pm_clock.h:83:#define pm_clk_resume     NULL
./drivers/bus/tegra-aconnect.c:62:      return pm_clk_resume(dev);
./drivers/dma/tegra210-adma.c:649:      ret = pm_clk_resume(dev);
./drivers/base/power/clock_ops.c:421: * pm_clk_resume - Enable clocks in 
a device's PM clock list.
./drivers/base/power/clock_ops.c:424:int pm_clk_resume(struct device *dev)
./drivers/base/power/clock_ops.c:444:EXPORT_SYMBOL_GPL(pm_clk_resume);
./drivers/base/power/clock_ops.c:533:   ret = pm_clk_resume(dev);
./drivers/base/power/domain.c:1685:             genpd->dev_ops.start = 
pm_clk_resume;
./drivers/irqchip/irq-gic-pm.c:36:      ret = pm_clk_resume(dev);

so if the device doesn't have a power domain, the PM clks could be 
unmanaged right?

by the way, the tegra drivers(tegra-aconnect/tegra210-adma) seems like 
good examples of using current PM clks...


but anyway, force adding all clocks as PM clks in the rockchip power 
domain driver seems wrong...

> 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.
>
> 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