[PATCH v8 1/5] clk: Add support for runtime PM

Fri Aug 11 09:28:35 PDT 2017

Quoting Marek Szyprowski (2017-08-10 23:18:42)
> Hi Michael,
> 
> Thanks for the comments!
> 
> On 2017-08-10 20:28, Michael Turquette wrote:
> > Hi Marek,
> >
> > Quoting Marek Szyprowski (2017-08-09 03:35:03)
> >> Registers for some clocks might be located in the SOC area, which are under the
> >> power domain. To enable access to those registers respective domain has to be
> >> turned on. Additionally, registers for such clocks will usually loose its
> >> contents when power domain is turned off, so additional saving and restoring of
> >> them might be needed in the clock controller driver.
> >>
> >> This patch adds basic infrastructure in the clocks core to allow implementing
> >> driver for such clocks under power domains. Clock provider can supply a
> >> struct device pointer, which is the used by clock core for tracking and managing
> >> clock's controller runtime pm state. Each clk_prepare() operation
> >> will first call pm_runtime_get_sync() on the supplied device, while
> >> clk_unprepare() will do pm_runtime_put_sync() at the end.
> >>
> >> Additional calls to pm_runtime_get/put functions are required to ensure that any
> >> register access (like calculating/changing clock rates and unpreparing/disabling
> >> unused clocks on boot) will be done with clock controller in runtime resumend
> >> state.
> >>
> >> When one wants to register clock controller, which make use of this feature, he
> >> has to:
> >> 1. Provide a struct device to the core when registering the provider.
> >> 2. Ensure to enable runtime PM for that device before registering clocks.
> >> 3. Make sure that the runtime PM status of the controller device reflects
> >>     the HW state.
> >>
> >> Signed-off-by: Marek Szyprowski <m.szyprowski at samsung.com>
> >> Reviewed-by: Ulf Hansson <ulf.hansson at linaro.org>
> >> Acked-by: Krzysztof Kozlowski <krzk at kernel.org>
> > Overall the idea and the patch look good to me.
> >
> > Calling pm_runtime_get for prepare and pm_runtime_put for unprepared
> > makes perfect sense to me. It means that the power domain that powers
> > that clock hardware must be on for that clock to function properly.
> >
> > It’s worth pointing out that this patch leaves that power domain enabled
> > for for as long as prepare_count is greater than zero.
> >
> > However the rate-change operations only use the pm_runtime functions to
> > program the hardware and modify the registers. At the end of each
> > rate-change call there is a pm_runtime_put. This makes sense to me, but
> > I’m trying to wrap my head around the fact that this patch introduces
> > two behaviors:
> >
> > 1) protect access to the clk hardware by wrapping writes across the bus
> > with pm_runtime calls
> > 2) power the clock logic when the clock is in use (e.g. clk_prepare())
> >
> > Everything about this patch *seems* correct to me, but I’m trying to
> > figure out what it means to for a clock consumer driver to do something
> > like this:
> >
> > clk_prepare_enable()
> > clk_disable_unprepare()
> > clk_set_rate()
> >
> > Maybe patches #2-5 will help me understand, but I wanted to jot this
> > down in my review before I forget.
> 
> Well, the question is what will the above sequence do without my patches?
> 
> IMO the only result of above calls is (optionally) changed rate of parent
> clocks and appropriate values written to given clock's registers, so next
> time one calls clk_(prepare)_enable(), the given clock will be enabled
> with the configured rate.
> 
> With runtime pm patches the overall result will be the same. However, if
> the given clock is the only/last enabled clock from the provider, there
> might be a runtime pm resume/suspend transition (2 times: one from
> prepare/unprepare calls, the second from set_rate calls). This means that
> the rate of parent clocks (especially if they comes from the other
> providers) might or might not change between those transitions. This
> depends how the runtime pm is implemented and what operations has to be
> done for putting provider into suspend mode. The final result will be
> however the same as without runtime pm patches.
> 
> Clock provider's runtime pm suspend callback should save internal
> configuration and resume callback must restore it completely, so the
> provider's internal state is exactly the same as before suspend. If we
> assume that clock provider might be in a power domain, the internal
> clock provider's hardware context might be lost after suspend callback.
> 
> >> @@ -2583,6 +2673,12 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
> >>                  goto fail_name;
> >>          }
> >>          core->ops = hw->init->ops;
> >> +       if (dev && pm_runtime_enabled(dev)) {
> >> +               core->dev = dev;
> >> +               ret = clk_pm_runtime_get(core);
> >> +               if (ret)
> >> +                       goto fail_pm;
> >> +       }
> >>          if (dev && dev->driver)
> >>                  core->owner = dev->driver->owner;
> >>          core->hw = hw;
> >> @@ -2629,12 +2725,13 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw)
> >>          }
> >>   
> >>          ret = __clk_core_init(core);
> >> -       if (!ret)
> >> +       if (!ret) {
> >> +               clk_pm_runtime_put(core);
> >>                  return hw->clk;
> >> +       }
> > I wonder if the above can be moved inside of __clk_core_init?
> > clk_register shouldn't manage any clk_ops directly, only __clk_core_init
> > does this.
> 
> If you prefer that, I will move runtime pm related bits to 
> __clk_core_init then.

Please do. After that I can pull v9 into clk-next.

Thanks,
Mike

> 
> Best regards
> -- 
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-clk" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html