| Message ID | 1389445540-21426-1-git-send-email-ben.dooks@codethink.co.uk (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi Ben, Thank you for the patch. On Saturday 11 January 2014 13:05:38 Ben Dooks wrote: > The drivers/base/power/clock_ops.c file is causing warnings from > the clock driver (as shown below) due to failing to do a clk_prepare() > call before enabling a clock. It also fails to check the balance of > prepare/unprepare as __pm_clk_remove() do clk_disable_unprepare() call. > > This bug has probably been in since commit b2476490e ("clk: introduce > the common clock framework") as the warning was part of the original > commit. It is strange that it has not been noticed (although this has > also been coupled with a failure for certain SH builds to not build the > necessary glue to use this method of controlling the clocks). > > In summary, this is probably needed in several stable branches but need > advice on which ones. > > On the Renesas Lager board, this causes numerous warnings of the following > and even worse the clock system will not enable clocks, causing drivers > that are in development to fail to work: > > WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:883 __clk_enable+0x2c/0xa0() I've never noticed this on Lager, probably because Lager multiplatform doesn't make use of clock_ops.c as drivers/sh/pm_runtime.c (which you addressed in another patch that I've also replied to). I'm thus not sure we need to apply this as a fix and backport it to stable branches. > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Cc: Pavel Machek <pavel@ucw.cz> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: linux-pm@vger.kernel.org > Cc: linux-sh@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> > Reviewed-by: Ian Molton <ian.molton@codethink.co.uk> > --- > drivers/base/power/clock_ops.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c > index 9d8fde7..b9dd8fa 100644 > --- a/drivers/base/power/clock_ops.c > +++ b/drivers/base/power/clock_ops.c > @@ -43,6 +43,7 @@ static void pm_clk_acquire(struct device *dev, struct > pm_clock_entry *ce) if (IS_ERR(ce->clk)) { > ce->status = PCE_STATUS_ERROR; > } else { > + clk_prepare(ce->clk); > ce->status = PCE_STATUS_ACQUIRED; > dev_dbg(dev, "Clock %s managed by runtime PM.\n", ce->con_id); > } > @@ -99,10 +100,12 @@ static void __pm_clk_remove(struct pm_clock_entry *ce) > > if (ce->status < PCE_STATUS_ERROR) { > if (ce->status == PCE_STATUS_ENABLED) > - clk_disable_unprepare(ce->clk); > + clk_disable(ce->clk); > > - if (ce->status >= PCE_STATUS_ACQUIRED) > + if (ce->status >= PCE_STATUS_ACQUIRED) { > + clk_unprepare(ce->clk); > clk_put(ce->clk); > + } > } > > kfree(ce->con_id);
On 12/01/14 22:04, Laurent Pinchart wrote: > Hi Ben, > > Thank you for the patch. > > On Saturday 11 January 2014 13:05:38 Ben Dooks wrote: >> The drivers/base/power/clock_ops.c file is causing warnings from >> the clock driver (as shown below) due to failing to do a clk_prepare() >> call before enabling a clock. It also fails to check the balance of >> prepare/unprepare as __pm_clk_remove() do clk_disable_unprepare() call. >> >> This bug has probably been in since commit b2476490e ("clk: introduce >> the common clock framework") as the warning was part of the original >> commit. It is strange that it has not been noticed (although this has >> also been coupled with a failure for certain SH builds to not build the >> necessary glue to use this method of controlling the clocks). >> >> In summary, this is probably needed in several stable branches but need >> advice on which ones. >> >> On the Renesas Lager board, this causes numerous warnings of the following >> and even worse the clock system will not enable clocks, causing drivers >> that are in development to fail to work: >> >> WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:883 __clk_enable+0x2c/0xa0() > > I've never noticed this on Lager, probably because Lager multiplatform doesn't > make use of clock_ops.c as drivers/sh/pm_runtime.c (which you addressed in > another patch that I've also replied to). I'm thus not sure we need to apply > this as a fix and backport it to stable branches. Yes, it seems that a lot of the lager drivers assume that this support is there, and the work seems to have been sponsored by Renesas. I think there must be people still building the old style of kernel as there is still work going on for board-lager.c which leads me to being a bit surprised that no-one had noticed this (as it isn't as if it produces a pile of warnings on the console output).
On Mon, Jan 13, 2014 at 7:28 AM, Ben Dooks <ben.dooks@codethink.co.uk> wrote: >> I've never noticed this on Lager, probably because Lager multiplatform >> doesn't >> make use of clock_ops.c as drivers/sh/pm_runtime.c (which you addressed in >> another patch that I've also replied to). I'm thus not sure we need to >> apply >> this as a fix and backport it to stable branches. > > > Yes, it seems that a lot of the lager drivers assume that this support > is there, and the work seems to have been sponsored by Renesas. I can confirm that applying both this and "[PATCH] ARM: shmobile: compile drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI" makes rcar_thermal e61f0000.thermal: thermal sensor was broken go away on Koelsch. 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-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 13/01/14 08:50, Geert Uytterhoeven wrote: > On Mon, Jan 13, 2014 at 7:28 AM, Ben Dooks <ben.dooks@codethink.co.uk> wrote: >>> I've never noticed this on Lager, probably because Lager multiplatform >>> doesn't >>> make use of clock_ops.c as drivers/sh/pm_runtime.c (which you addressed in >>> another patch that I've also replied to). I'm thus not sure we need to >>> apply >>> this as a fix and backport it to stable branches. >> >> >> Yes, it seems that a lot of the lager drivers assume that this support >> is there, and the work seems to have been sponsored by Renesas. > > I can confirm that applying both this and "[PATCH] ARM: shmobile: compile > drivers/sh for CONFIG_ARCH_SHMOBILE_MULTI" makes Thanks. Wonder how long this has been broken for?
diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c index 9d8fde7..b9dd8fa 100644 --- a/drivers/base/power/clock_ops.c +++ b/drivers/base/power/clock_ops.c @@ -43,6 +43,7 @@ static void pm_clk_acquire(struct device *dev, struct pm_clock_entry *ce) if (IS_ERR(ce->clk)) { ce->status = PCE_STATUS_ERROR; } else { + clk_prepare(ce->clk); ce->status = PCE_STATUS_ACQUIRED; dev_dbg(dev, "Clock %s managed by runtime PM.\n", ce->con_id); } @@ -99,10 +100,12 @@ static void __pm_clk_remove(struct pm_clock_entry *ce) if (ce->status < PCE_STATUS_ERROR) { if (ce->status == PCE_STATUS_ENABLED) - clk_disable_unprepare(ce->clk); + clk_disable(ce->clk); - if (ce->status >= PCE_STATUS_ACQUIRED) + if (ce->status >= PCE_STATUS_ACQUIRED) { + clk_unprepare(ce->clk); clk_put(ce->clk); + } } kfree(ce->con_id);
The drivers/base/power/clock_ops.c file is causing warnings from the clock driver (as shown below) due to failing to do a clk_prepare() call before enabling a clock. It also fails to check the balance of prepare/unprepare as __pm_clk_remove() do clk_disable_unprepare() call. This bug has probably been in since commit b2476490e ("clk: introduce the common clock framework") as the warning was part of the original commit. It is strange that it has not been noticed (although this has also been coupled with a failure for certain SH builds to not build the necessary glue to use this method of controlling the clocks). In summary, this is probably needed in several stable branches but need advice on which ones. On the Renesas Lager board, this causes numerous warnings of the following and even worse the clock system will not enable clocks, causing drivers that are in development to fail to work: WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:883 __clk_enable+0x2c/0xa0() Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> Cc: Pavel Machek <pavel@ucw.cz> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: linux-pm@vger.kernel.org Cc: linux-sh@vger.kernel.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> Reviewed-by: Ian Molton <ian.molton@codethink.co.uk> --- drivers/base/power/clock_ops.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)