| Message ID | 20151021155057.20687.14055@quantum (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Mike, On Wed, Oct 21, 2015 at 5:50 PM, Michael Turquette <mturquette@baylibre.com> wrote: > Quoting Russell King - ARM Linux (2015-10-21 03:59:32) >> On Wed, Oct 21, 2015 at 11:50:07AM +0200, Geert Uytterhoeven wrote: >> > On Tue, Oct 20, 2015 at 2:40 PM, Michael Turquette >> > <mturquette@baylibre.com> wrote: >> > > Why not keep the reference to the struct clk after get'ing it the first >> > > time? >> > >> > And store it where? >> >> Not my problem :) >> >> Users are supposed to hold on to the reference obtained via clk_get() >> while they're making use of the clock: in some implementations, this >> increments the module use count if the clock driver is a module, and >> may have other effects too. >> >> Dropping that while you're still requiring the clock to be enabled is >> unsafe: if it is provided by a module, then removing and reinserting >> the module may very well change the enabled state of the clock, it >> most certainly will disrupt the enable count. >> >> It's always been this way, right from the outset, and when I've seen >> people doing this bollocks, I've always pointed out that it's wrong. >> Generally, people will fix it once they become aware of it, so it's >> really that people just don't like reading and conforming to published >> API requirements. >> >> I think the root cause is that people just don't like reading what >> other people write in terms of documentation, and they prefer to go >> off and do their own thing, provided it works for them. > > Right, so in other words this problem must be solved by the caller of > clk_get, as it should be. I have never much looked at the pm clk code in > question, but I gave it a quick look and came up with some example code > that does not compile, in an effort to be as helpful as possible. > > Basically I added a flex array to struct pm_clk_notifier_block, so that > now there are two flex arrays which is stupid. But I am too lazy to > replace the nbclk->clks thing with a malloc after walking all of the > clk_id's to figure out the number of clocks. Or we could just add > .num_clk to the struct, fix up all 4 users of it and drop the NULL > sentinel used the .clk_id's... Hmm that would have been better. Thanks for trying. > index 25266c6..45e58fe 100644 > --- a/include/linux/pm_clock.h > +++ b/include/linux/pm_clock.h > @@ -16,6 +16,7 @@ struct pm_clk_notifier_block { > struct notifier_block nb; > struct dev_pm_domain *pm_domain; > char *con_ids[]; > + struct clk *clks[]; > }; > > struct clk; Unfortunately that won't work: while the .con_ids[] array is per-platform, the .clks[] array should be per-device. I.e. it should be tied to the struct device, not to the struct pm_clk_notifier_block. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/mach-davinci/pm_domain.c b/arch/arm/mach-davinci/pm_domain.c index 78eac2c..b46e5ce 100644 --- a/arch/arm/mach-davinci/pm_domain.c +++ b/arch/arm/mach-davinci/pm_domain.c @@ -24,6 +24,7 @@ static struct dev_pm_domain davinci_pm_domain = { static struct pm_clk_notifier_block platform_bus_notifier = { .pm_domain = &davinci_pm_domain, .con_ids = { "fck", "master", "slave", NULL }, + .clks = { ERR_PTR(-EAGAIN), ERR_PTR(-EAGAIN), ERR_PTR(-EAGAIN) }, }; static int __init davinci_pm_runtime_init(void) diff --git a/arch/arm/mach-keystone/pm_domain.c b/arch/arm/mach-keystone/pm_domain.c index e283939..d21c18b 100644 --- a/arch/arm/mach-keystone/pm_domain.c +++ b/arch/arm/mach-keystone/pm_domain.c @@ -27,6 +27,7 @@ static struct dev_pm_domain keystone_pm_domain = { static struct pm_clk_notifier_block platform_domain_notifier = { .pm_domain = &keystone_pm_domain, + .clks = { ERR_PTR(-EAGAIN) }, }; static const struct of_device_id of_keystone_table[] = { diff --git a/arch/arm/mach-omap1/pm_bus.c b/arch/arm/mach-omap1/pm_bus.c index 667c163..5506453 100644 --- a/arch/arm/mach-omap1/pm_bus.c +++ b/arch/arm/mach-omap1/pm_bus.c @@ -31,6 +31,7 @@ static struct dev_pm_domain default_pm_domain = { static struct pm_clk_notifier_block platform_bus_notifier = { .pm_domain = &default_pm_domain, .con_ids = { "ick", "fck", NULL, }, + .clks = { ERR_PTR(-EAGAIN), ERR_PTR(-EAGAIN) }, }; static int __init omap1_pm_runtime_init(void) diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c index 652b5a3..26f0dcf 100644 --- a/drivers/base/power/clock_ops.c +++ b/drivers/base/power/clock_ops.c @@ -407,40 +407,6 @@ int pm_clk_runtime_resume(struct device *dev) #else /* !CONFIG_PM */ /** - * enable_clock - Enable a device clock. - * @dev: Device whose clock is to be enabled. - * @con_id: Connection ID of the clock. - */ -static void enable_clock(struct device *dev, const char *con_id) -{ - struct clk *clk; - - clk = clk_get(dev, con_id); - if (!IS_ERR(clk)) { - clk_prepare_enable(clk); - clk_put(clk); - dev_info(dev, "Runtime PM disabled, clock forced on.\n"); - } -} - -/** - * disable_clock - Disable a device clock. - * @dev: Device whose clock is to be disabled. - * @con_id: Connection ID of the clock. - */ -static void disable_clock(struct device *dev, const char *con_id) -{ - struct clk *clk; - - clk = clk_get(dev, con_id); - if (!IS_ERR(clk)) { - clk_disable_unprepare(clk); - clk_put(clk); - dev_info(dev, "Runtime PM disabled, clock forced off.\n"); - } -} - -/** * pm_clk_notify - Notify routine for device addition and removal. * @nb: Notifier block object this function is a member of. * @action: Operation being carried out by the caller. @@ -465,18 +431,45 @@ static int pm_clk_notify(struct notifier_block *nb, switch (action) { case BUS_NOTIFY_BIND_DRIVER: if (clknb->con_ids[0]) { - for (con_id = clknb->con_ids; *con_id; con_id++) - enable_clock(dev, *con_id); + int i; + for (con_id = clknb->con_ids, i = 0; *con_id; + con_id++, i++) { + if (clknb->clks[i] == ERR_PTR(-EAGAIN)) + clks[i] = clk_get(dev, *con_id); + if (!IS_ERR(clknb->clks[i])) { + clk_prepare_enable(clk); + dev_info(dev, "Runtime PM disabled, clock forced on.\n"); + } } else { - enable_clock(dev, NULL); + if (clknb->clks[0] == ERR_PTR(-EAGAIN)) + clks[0] = clk_get(dev, NULL); + if (!IS_ERR(clknb->clks[0])) { + clk_prepare_enable(clk); + dev_info(dev, "Runtime PM disabled, clock forced on.\n"); + } } break; case BUS_NOTIFY_UNBOUND_DRIVER: + /* + * FIXME + * We never call clk_put. This should be done with + * pm_clk_remove_notifier, which doesn't exist but probably + * should + */ if (clknb->con_ids[0]) { - for (con_id = clknb->con_ids; *con_id; con_id++) - disable_clock(dev, *con_id); + int i; + for (con_id = clknb->con_ids, i = 0; *con_id; + con_id++, i++) { + if (!IS_ERR(clknb->clks[i])) { + clk_disable_unprepare(clknb->clks[i]); + dev_info(dev, "Runtime PM disabled, clock forced off.\n"); + } + } } else { - disable_clock(dev, NULL); + if (!IS_ERR(clknb->clks[0])) { + clk_disable_unprepare(clknb->clks[i]); + dev_info(dev, "Runtime PM disabled, clock forced off.\n"); + } } break; } diff --git a/drivers/sh/pm_runtime.c b/drivers/sh/pm_runtime.c index 25abd4e..08754a4 100644 --- a/drivers/sh/pm_runtime.c +++ b/drivers/sh/pm_runtime.c @@ -30,6 +30,7 @@ static struct dev_pm_domain default_pm_domain = { static struct pm_clk_notifier_block platform_bus_notifier = { .pm_domain = &default_pm_domain, .con_ids = { NULL, }, + .clks = { ERR_PTR(-EAGAIN) }, }; static int __init sh_pm_runtime_init(void) diff --git a/include/linux/pm_clock.h b/include/linux/pm_clock.h index 25266c6..45e58fe 100644 --- a/include/linux/pm_clock.h +++ b/include/linux/pm_clock.h @@ -16,6 +16,7 @@ struct pm_clk_notifier_block { struct notifier_block nb; struct dev_pm_domain *pm_domain; char *con_ids[]; + struct clk *clks[]; }; struct clk;