lkml.org 
[lkml]   [2018]   [Mar]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] soc: rockchip: power-domain: remove PM clocks
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@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
>
>
>


\
 
 \ /
  Last update: 2018-03-01 10:10    [W:0.099 / U:0.564 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site