Re: [PATCH v13 4/6] clk: Add rate constraints to clocks

Re: [PATCH v13 4/6] clk: Add rate constraints to clocks

[Geert Uytterhoeven]

Hi Tomeu, Mike,

On Fri, Jan 23, 2015 at 12:03 PM, Tomeu Vizoso
<tomeu.vizoso@collabora.com> wrote:
> Adds a way for clock consumers to set maximum and minimum rates. This
> can be used for thermal drivers to set minimum rates, or by misc.
> drivers to set maximum rates to assure a minimum performance level.
>
> Changes the signature of the determine_rate callback by adding the
> parameters min_rate and max_rate.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

This is now in clk-next, and causes division by zeroes on all shmobile
platforms that use renesas,cpg-div6-clock (verified on r8a73a4, r8a7740,
r8a7791, sh73a0):

Division by zero in kernel.
CPU: 0 PID: 0 Comm: swapper/0 Not tainted
3.19.0-rc6-koelsch-04360-g48d797d57c5932c8-dirty #792
Hardware name: Generic R8A7791 (Flattened Device Tree)
Backtrace:
[<c001216c>] (dump_backtrace) from [<c001238c>] (show_stack+0x18/0x1c)
 r6:c051b124 r5:00000000 r4:00000000 r3:00200000
[<c0012374>] (show_stack) from [<c03955d0>] (dump_stack+0x78/0x94)
[<c0395558>] (dump_stack) from [<c00122f4>] (__div0+0x18/0x20)
 r4:2e7ddb00 r3:c05093c8
[<c00122dc>] (__div0) from [<c01bdc9c>] (Ldiv0+0x8/0x10)
[<c02d8efc>] (cpg_div6_clock_round_rate) from [<c02d56a0>]
(clk_calc_new_rates+0xc8/0x1d4)
 r4:eec14e00 r3:c03cb52c
[<c02d55d8>] (clk_calc_new_rates) from [<c02d57f4>]
(clk_core_set_rate_nolock+0x48/0x90)
 r9:eec02f40 r8:00000001 r7:c051b0b8 r6:c051b124 r5:00000000 r4:eec14e00
[<c02d57ac>] (clk_core_set_rate_nolock) from [<c02d6848>] (__clk_put+0x78/0xdc)
 r7:c051b0b8 r6:c051b124 r5:eec08100 r4:eec029c0
[<c02d67d0>] (__clk_put) from [<c02d3238>] (clk_put+0x10/0x14)
 r5:eec08100 r4:00000000
[<c02d3228>] (clk_put) from [<c04db4d0>] (of_clk_init+0x144/0x178)
[<c04db38c>] (of_clk_init) from [<c04dbd34>] (rcar_gen2_clocks_init+0x1c/0x24)
 r10:c04ed098 r9:c05023c0 r8:ffffffff r7:19432124 r6:c0502404 r5:00989680
 r4:f0006000
[<c04dbd18>] (rcar_gen2_clocks_init) from [<c04c6a3c>]
(rcar_gen2_timer_init+0x130/0x14c)
[<c04c690c>] (rcar_gen2_timer_init) from [<c04c15b4>] (time_init+0x24/0x38)
 r7:00000000 r6:c0520c80 r5:00000000 r4:ef7fcbc0
[<c04c1590>] (time_init) from [<c04bdb84>] (start_kernel+0x248/0x3bc)
[<c04bd93c>] (start_kernel) from [<40008084>] (0x40008084)
 r10:00000000 r9:413fc0f2 r8:40007000 r7:c0505870 r6:c04ed094 r5:c0502440
 r4:c0521054


> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c

> @@ -2391,25 +2543,24 @@ int __clk_get(struct clk *clk)
>         return 1;
>  }
>
> -static void clk_core_put(struct clk_core *core)
> +void __clk_put(struct clk *clk)
>  {
>         struct module *owner;
>
> -       owner = core->owner;
> +       if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
> +               return;
>
>         clk_prepare_lock();
> -       kref_put(&core->ref, __clk_release);
> +
> +       hlist_del(&clk->child_node);
> +       clk_core_set_rate_nolock(clk->core, clk->core->req_rate);

At this point, clk->core->req_rate is still zero, causing
cpg_div6_clock_round_rate() to be called with a zero "rate" parameter,
e.g. on r8a7791:

cpg_div6_clock_round_rate: clock sd2 rate 0 parent_rate 780000000 div 1
cpg_div6_clock_round_rate: clock sd1 rate 0 parent_rate 780000000 div 1
cpg_div6_clock_round_rate: clock mmc0 rate 0 parent_rate 780000000 div 1
cpg_div6_clock_round_rate: clock sd1 rate 0 parent_rate 780000000 div 1
cpg_div6_clock_round_rate: clock sd1 rate 0 parent_rate 780000000 div 1
cpg_div6_clock_round_rate: clock sd2 rate 0 parent_rate 780000000 div 1
cpg_div6_clock_round_rate: clock sd2 rate 0 parent_rate 780000000 div 1

and cpg_div6_clock_calc_div() is called to calculate

        div = DIV_ROUND_CLOSEST(parent_rate, rate);

Why was this call to clk_core_set_rate_nolock() in __clk_put() added?
Before, there was no rate setting done at this point, and
cpg_div6_clock_round_rate() was not called.

Have the semantics changed? Should .round_rate() be ready to
accept a "zero" rate, and use e.g. the current rate instead?

> +       owner = clk->core->owner;
> +       kref_put(&clk->core->ref, __clk_release);
> +
>         clk_prepare_unlock();
>
>         module_put(owner);
> -}
> -
> -void __clk_put(struct clk *clk)
> -{
> -       if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
> -               return;
>
> -       clk_core_put(clk->core);
>         kfree(clk);
>  }

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

Re: [PATCH v13 4/6] clk: Add rate constraints to clocks

[Stephen Boyd]

On 01/29/15 05:31, Geert Uytterhoeven wrote:
> Hi Tomeu, Mike,
>
> On Fri, Jan 23, 2015 at 12:03 PM, Tomeu Vizoso
> <tomeu.vizoso@collabora.com> wrote:
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -2391,25 +2543,24 @@ int __clk_get(struct clk *clk)
>>         return 1;
>>  }
>>
>> -static void clk_core_put(struct clk_core *core)
>> +void __clk_put(struct clk *clk)
>>  {
>>         struct module *owner;
>>
>> -       owner = core->owner;
>> +       if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
>> +               return;
>>
>>         clk_prepare_lock();
>> -       kref_put(&core->ref, __clk_release);
>> +
>> +       hlist_del(&clk->child_node);
>> +       clk_core_set_rate_nolock(clk->core, clk->core->req_rate);
> At this point, clk->core->req_rate is still zero, causing
> cpg_div6_clock_round_rate() to be called with a zero "rate" parameter,
> e.g. on r8a7791:

Hmm.. I wonder if we should assign core->req_rate to be the same as
core->rate during __clk_init()? That would make this call to
clk_core_set_rate_nolock() a nop in this case.

>
> cpg_div6_clock_round_rate: clock sd2 rate 0 parent_rate 780000000 div 1
> cpg_div6_clock_round_rate: clock sd1 rate 0 parent_rate 780000000 div 1
> cpg_div6_clock_round_rate: clock mmc0 rate 0 parent_rate 780000000 div 1
> cpg_div6_clock_round_rate: clock sd1 rate 0 parent_rate 780000000 div 1
> cpg_div6_clock_round_rate: clock sd1 rate 0 parent_rate 780000000 div 1
> cpg_div6_clock_round_rate: clock sd2 rate 0 parent_rate 780000000 div 1
> cpg_div6_clock_round_rate: clock sd2 rate 0 parent_rate 780000000 div 1
>
> and cpg_div6_clock_calc_div() is called to calculate
>
>         div = DIV_ROUND_CLOSEST(parent_rate, rate);
>
> Why was this call to clk_core_set_rate_nolock() in __clk_put() added?
> Before, there was no rate setting done at this point, and
> cpg_div6_clock_round_rate() was not called.

We need to call clk_core_set_rate_nolock() here to drop any min/max rate
request that this consumer has.

>
> Have the semantics changed? Should .round_rate() be ready to
> accept a "zero" rate, and use e.g. the current rate instead?

It seems like we've also exposed a bug in cpg_div6_clock_calc_div().
Technically any driver could have called clk_round_rate() with a zero
rate before this change and that would have caused the same division by
zero.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v13 4/6] clk: Add rate constraints to clocks

[Stephen Boyd]

On 01/29, Stephen Boyd wrote:
> On 01/29/15 05:31, Geert Uytterhoeven wrote:
> > Hi Tomeu, Mike,
> >
> > On Fri, Jan 23, 2015 at 12:03 PM, Tomeu Vizoso
> > <tomeu.vizoso@collabora.com> wrote:
> >> --- a/drivers/clk/clk.c
> >> +++ b/drivers/clk/clk.c
> >> @@ -2391,25 +2543,24 @@ int __clk_get(struct clk *clk)
> >>         return 1;
> >>  }
> >>
> >> -static void clk_core_put(struct clk_core *core)
> >> +void __clk_put(struct clk *clk)
> >>  {
> >>         struct module *owner;
> >>
> >> -       owner = core->owner;
> >> +       if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
> >> +               return;
> >>
> >>         clk_prepare_lock();
> >> -       kref_put(&core->ref, __clk_release);
> >> +
> >> +       hlist_del(&clk->child_node);
> >> +       clk_core_set_rate_nolock(clk->core, clk->core->req_rate);
> > At this point, clk->core->req_rate is still zero, causing
> > cpg_div6_clock_round_rate() to be called with a zero "rate" parameter,
> > e.g. on r8a7791:
> 
> Hmm.. I wonder if we should assign core->req_rate to be the same as
> core->rate during __clk_init()? That would make this call to
> clk_core_set_rate_nolock() a nop in this case.
> 

Here's a patch to do this

---8<----
From: Stephen Boyd <sboyd@codeaurora.org>
Subject: [PATCH] clk: Assign a requested rate by default

We need to assign a requested rate here so that we avoid
requesting a rate of 0 on clocks when we remove clock consumers.

Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
 drivers/clk/clk.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index a29daf9edea4..8416ed1c40be 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2142,6 +2142,7 @@ int __clk_init(struct device *dev, struct clk *clk_user)
 	struct clk_core *orphan;
 	struct hlist_node *tmp2;
 	struct clk_core *clk;
+	unsigned long rate;
 
 	if (!clk_user)
 		return -EINVAL;
@@ -2266,12 +2267,13 @@ int __clk_init(struct device *dev, struct clk *clk_user)
 	 * then rate is set to zero.
 	 */
 	if (clk->ops->recalc_rate)
-		clk->rate = clk->ops->recalc_rate(clk->hw,
+		rate = clk->ops->recalc_rate(clk->hw,
 				clk_core_get_rate_nolock(clk->parent));
 	else if (clk->parent)
-		clk->rate = clk->parent->rate;
+		rate = clk->parent->rate;
 	else
-		clk->rate = 0;
+		rate = 0;
+	clk->rate = clk->req_rate = rate;
 
 	/*
 	 * walk the list of orphan clocks and reparent any that are children of
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v13 4/6] clk: Add rate constraints to clocks

[Tomeu Vizoso]

On 31 January 2015 at 02:31, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 01/29, Stephen Boyd wrote:
>> On 01/29/15 05:31, Geert Uytterhoeven wrote:
>> > Hi Tomeu, Mike,
>> >
>> > On Fri, Jan 23, 2015 at 12:03 PM, Tomeu Vizoso
>> > <tomeu.vizoso@collabora.com> wrote:
>> >> --- a/drivers/clk/clk.c
>> >> +++ b/drivers/clk/clk.c
>> >> @@ -2391,25 +2543,24 @@ int __clk_get(struct clk *clk)
>> >>         return 1;
>> >>  }
>> >>
>> >> -static void clk_core_put(struct clk_core *core)
>> >> +void __clk_put(struct clk *clk)
>> >>  {
>> >>         struct module *owner;
>> >>
>> >> -       owner = core->owner;
>> >> +       if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
>> >> +               return;
>> >>
>> >>         clk_prepare_lock();
>> >> -       kref_put(&core->ref, __clk_release);
>> >> +
>> >> +       hlist_del(&clk->child_node);
>> >> +       clk_core_set_rate_nolock(clk->core, clk->core->req_rate);
>> > At this point, clk->core->req_rate is still zero, causing
>> > cpg_div6_clock_round_rate() to be called with a zero "rate" parameter,
>> > e.g. on r8a7791:
>>
>> Hmm.. I wonder if we should assign core->req_rate to be the same as
>> core->rate during __clk_init()? That would make this call to
>> clk_core_set_rate_nolock() a nop in this case.
>>
>
> Here's a patch to do this
>
> ---8<----
> From: Stephen Boyd <sboyd@codeaurora.org>
> Subject: [PATCH] clk: Assign a requested rate by default
>
> We need to assign a requested rate here so that we avoid
> requesting a rate of 0 on clocks when we remove clock consumers.

Hi, this looks good to me.

Reviewed-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>

Thanks,

Tomeu

> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
>  drivers/clk/clk.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index a29daf9edea4..8416ed1c40be 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2142,6 +2142,7 @@ int __clk_init(struct device *dev, struct clk *clk_user)
>         struct clk_core *orphan;
>         struct hlist_node *tmp2;
>         struct clk_core *clk;
> +       unsigned long rate;
>
>         if (!clk_user)
>                 return -EINVAL;
> @@ -2266,12 +2267,13 @@ int __clk_init(struct device *dev, struct clk *clk_user)
>          * then rate is set to zero.
>          */
>         if (clk->ops->recalc_rate)
> -               clk->rate = clk->ops->recalc_rate(clk->hw,
> +               rate = clk->ops->recalc_rate(clk->hw,
>                                 clk_core_get_rate_nolock(clk->parent));
>         else if (clk->parent)
> -               clk->rate = clk->parent->rate;
> +               rate = clk->parent->rate;
>         else
> -               clk->rate = 0;
> +               rate = 0;
> +       clk->rate = clk->req_rate = rate;
>
>         /*
>          * walk the list of orphan clocks and reparent any that are children of
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v13 4/6] clk: Add rate constraints to clocks

[Mike Turquette]

Quoting Tomeu Vizoso (2015-01-31 10:36:22)
> On 31 January 2015 at 02:31, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > On 01/29, Stephen Boyd wrote:
> >> On 01/29/15 05:31, Geert Uytterhoeven wrote:
> >> > Hi Tomeu, Mike,
> >> >
> >> > On Fri, Jan 23, 2015 at 12:03 PM, Tomeu Vizoso
> >> > <tomeu.vizoso@collabora.com> wrote:
> >> >> --- a/drivers/clk/clk.c
> >> >> +++ b/drivers/clk/clk.c
> >> >> @@ -2391,25 +2543,24 @@ int __clk_get(struct clk *clk)
> >> >>         return 1;
> >> >>  }
> >> >>
> >> >> -static void clk_core_put(struct clk_core *core)
> >> >> +void __clk_put(struct clk *clk)
> >> >>  {
> >> >>         struct module *owner;
> >> >>
> >> >> -       owner = core->owner;
> >> >> +       if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
> >> >> +               return;
> >> >>
> >> >>         clk_prepare_lock();
> >> >> -       kref_put(&core->ref, __clk_release);
> >> >> +
> >> >> +       hlist_del(&clk->child_node);
> >> >> +       clk_core_set_rate_nolock(clk->core, clk->core->req_rate);
> >> > At this point, clk->core->req_rate is still zero, causing
> >> > cpg_div6_clock_round_rate() to be called with a zero "rate" parameter,
> >> > e.g. on r8a7791:
> >>
> >> Hmm.. I wonder if we should assign core->req_rate to be the same as
> >> core->rate during __clk_init()? That would make this call to
> >> clk_core_set_rate_nolock() a nop in this case.
> >>
> >
> > Here's a patch to do this
> >
> > ---8<----
> > From: Stephen Boyd <sboyd@codeaurora.org>
> > Subject: [PATCH] clk: Assign a requested rate by default
> >
> > We need to assign a requested rate here so that we avoid
> > requesting a rate of 0 on clocks when we remove clock consumers.
> 
> Hi, this looks good to me.
> 
> Reviewed-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>

It seems to fix the total boot failure on OMAPs, and hopefully does the
same for SH Mobile and others. I've squashed this into Tomeu's rate
constraints patch to maintain bisect.

Regards,
Mike

> 
> Thanks,
> 
> Tomeu
> 
> > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> > ---
> >  drivers/clk/clk.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index a29daf9edea4..8416ed1c40be 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -2142,6 +2142,7 @@ int __clk_init(struct device *dev, struct clk *clk_user)
> >         struct clk_core *orphan;
> >         struct hlist_node *tmp2;
> >         struct clk_core *clk;
> > +       unsigned long rate;
> >
> >         if (!clk_user)
> >                 return -EINVAL;
> > @@ -2266,12 +2267,13 @@ int __clk_init(struct device *dev, struct clk *clk_user)
> >          * then rate is set to zero.
> >          */
> >         if (clk->ops->recalc_rate)
> > -               clk->rate = clk->ops->recalc_rate(clk->hw,
> > +               rate = clk->ops->recalc_rate(clk->hw,
> >                                 clk_core_get_rate_nolock(clk->parent));
> >         else if (clk->parent)
> > -               clk->rate = clk->parent->rate;
> > +               rate = clk->parent->rate;
> >         else
> > -               clk->rate = 0;
> > +               rate = 0;
> > +       clk->rate = clk->req_rate = rate;
> >
> >         /*
> >          * walk the list of orphan clocks and reparent any that are children of
> > --
> > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> > a Linux Foundation Collaborative Project
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v13 4/6] clk: Add rate constraints to clocks

[Geert Uytterhoeven]

On Sun, Feb 1, 2015 at 11:18 PM, Mike Turquette <mturquette@linaro.org> wrote:
> Quoting Tomeu Vizoso (2015-01-31 10:36:22)
>> On 31 January 2015 at 02:31, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> > On 01/29, Stephen Boyd wrote:
>> >> On 01/29/15 05:31, Geert Uytterhoeven wrote:
>> >> > Hi Tomeu, Mike,
>> >> >
>> >> > On Fri, Jan 23, 2015 at 12:03 PM, Tomeu Vizoso
>> >> > <tomeu.vizoso@collabora.com> wrote:
>> >> >> --- a/drivers/clk/clk.c
>> >> >> +++ b/drivers/clk/clk.c
>> >> >> @@ -2391,25 +2543,24 @@ int __clk_get(struct clk *clk)
>> >> >>         return 1;
>> >> >>  }
>> >> >>
>> >> >> -static void clk_core_put(struct clk_core *core)
>> >> >> +void __clk_put(struct clk *clk)
>> >> >>  {
>> >> >>         struct module *owner;
>> >> >>
>> >> >> -       owner = core->owner;
>> >> >> +       if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
>> >> >> +               return;
>> >> >>
>> >> >>         clk_prepare_lock();
>> >> >> -       kref_put(&core->ref, __clk_release);
>> >> >> +
>> >> >> +       hlist_del(&clk->child_node);
>> >> >> +       clk_core_set_rate_nolock(clk->core, clk->core->req_rate);
>> >> > At this point, clk->core->req_rate is still zero, causing
>> >> > cpg_div6_clock_round_rate() to be called with a zero "rate" parameter,
>> >> > e.g. on r8a7791:
>> >>
>> >> Hmm.. I wonder if we should assign core->req_rate to be the same as
>> >> core->rate during __clk_init()? That would make this call to
>> >> clk_core_set_rate_nolock() a nop in this case.
>> >>
>> >
>> > Here's a patch to do this
>> >
>> > ---8<----
>> > From: Stephen Boyd <sboyd@codeaurora.org>
>> > Subject: [PATCH] clk: Assign a requested rate by default
>> >
>> > We need to assign a requested rate here so that we avoid
>> > requesting a rate of 0 on clocks when we remove clock consumers.
>>
>> Hi, this looks good to me.
>>
>> Reviewed-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>
> It seems to fix the total boot failure on OMAPs, and hopefully does the
> same for SH Mobile and others. I've squashed this into Tomeu's rate
> constraints patch to maintain bisect.

Yes, it fixes shmobile. .round_rate() is now called with a sane value of rate.

Thanks!

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

Re: [PATCH v13 4/6] clk: Add rate constraints to clocks

[Tony Lindgren]

* Geert Uytterhoeven <geert@linux-m68k.org> [150202 00:03]:
> On Sun, Feb 1, 2015 at 11:18 PM, Mike Turquette <mturquette@linaro.org> wrote:
> > Quoting Tomeu Vizoso (2015-01-31 10:36:22)
> >> On 31 January 2015 at 02:31, Stephen Boyd <sboyd@codeaurora.org> wrote:
> >> > On 01/29, Stephen Boyd wrote:
> >> >> On 01/29/15 05:31, Geert Uytterhoeven wrote:
> >> >> > Hi Tomeu, Mike,
> >> >> >
> >> >> > On Fri, Jan 23, 2015 at 12:03 PM, Tomeu Vizoso
> >> >> > <tomeu.vizoso@collabora.com> wrote:
> >> >> >> --- a/drivers/clk/clk.c
> >> >> >> +++ b/drivers/clk/clk.c
> >> >> >> @@ -2391,25 +2543,24 @@ int __clk_get(struct clk *clk)
> >> >> >>         return 1;
> >> >> >>  }
> >> >> >>
> >> >> >> -static void clk_core_put(struct clk_core *core)
> >> >> >> +void __clk_put(struct clk *clk)
> >> >> >>  {
> >> >> >>         struct module *owner;
> >> >> >>
> >> >> >> -       owner = core->owner;
> >> >> >> +       if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
> >> >> >> +               return;
> >> >> >>
> >> >> >>         clk_prepare_lock();
> >> >> >> -       kref_put(&core->ref, __clk_release);
> >> >> >> +
> >> >> >> +       hlist_del(&clk->child_node);
> >> >> >> +       clk_core_set_rate_nolock(clk->core, clk->core->req_rate);
> >> >> > At this point, clk->core->req_rate is still zero, causing
> >> >> > cpg_div6_clock_round_rate() to be called with a zero "rate" parameter,
> >> >> > e.g. on r8a7791:
> >> >>
> >> >> Hmm.. I wonder if we should assign core->req_rate to be the same as
> >> >> core->rate during __clk_init()? That would make this call to
> >> >> clk_core_set_rate_nolock() a nop in this case.
> >> >>
> >> >
> >> > Here's a patch to do this
> >> >
> >> > ---8<----
> >> > From: Stephen Boyd <sboyd@codeaurora.org>
> >> > Subject: [PATCH] clk: Assign a requested rate by default
> >> >
> >> > We need to assign a requested rate here so that we avoid
> >> > requesting a rate of 0 on clocks when we remove clock consumers.
> >>
> >> Hi, this looks good to me.
> >>
> >> Reviewed-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> >
> > It seems to fix the total boot failure on OMAPs, and hopefully does the
> > same for SH Mobile and others. I've squashed this into Tomeu's rate
> > constraints patch to maintain bisect.
> 
> Yes, it fixes shmobile. .round_rate() is now called with a sane value of rate.

Looks like next-20150202 now produces tons of the following errors,
these from omap4:

[   10.568206] ------------[ cut here ]------------
[   10.568206] WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:925 clk_disable+0x28/0x34()
[   10.568237] Modules linked in:
[   10.568237] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W       3.19.0-rc6-next-20150202 #2037
[   10.568237] Hardware name: Generic OMAP4 (Flattened Device Tree)
[   10.568267] [<c0015bdc>] (unwind_backtrace) from [<c001222c>] (show_stack+0x10/0x14)
[   10.568267] [<c001222c>] (show_stack) from [<c05d2014>] (dump_stack+0x84/0x9c)
[   10.568267] [<c05d2014>] (dump_stack) from [<c003ea90>] (warn_slowpath_common+0x7c/0xb8)
[   10.568298] [<c003ea90>] (warn_slowpath_common) from [<c003eb68>] (warn_slowpath_null+0x1c/0x24)
[   10.568298] [<c003eb68>] (warn_slowpath_null) from [<c04c1ffc>] (clk_disable+0x28/0x34)
[   10.568328] [<c04c1ffc>] (clk_disable) from [<c0025b3c>] (_disable_clocks+0x18/0x68)
[   10.568328] [<c0025b3c>] (_disable_clocks) from [<c0026f14>] (_idle+0x10c/0x214)
[   10.568328] [<c0026f14>] (_idle) from [<c0855fac>] (_setup+0x338/0x410)
[   10.568359] [<c0855fac>] (_setup) from [<c0027360>] (omap_hwmod_for_each+0x34/0x60)
[   10.568359] [<c0027360>] (omap_hwmod_for_each) from [<c08563c4>] (__omap_hwmod_setup_all+0x30/0x40)
[   10.568389] [<c08563c4>] (__omap_hwmod_setup_all) from [<c0008a04>] (do_one_initcall+0x80/0x1dc)
[   10.568389] [<c0008a04>] (do_one_initcall) from [<c0848ea0>] (kernel_init_freeable+0x204/0x2d0)
[   10.568420] [<c0848ea0>] (kernel_init_freeable) from [<c05cdab8>] (kernel_init+0x8/0xec)
[   10.568420] [<c05cdab8>] (kernel_init) from [<c000e790>] (ret_from_fork+0x14/0x24)
[   10.568420] ---[ end trace cb88537fdc8fa211 ]---


[   10.568450] ------------[ cut here ]------------
[   10.568450] WARNING: CPU: 0 PID: 1 at arch/arm/mach-omap2/dpll3xxx.c:436 omap3_noncore_dpll_enable+0xdc/0
x10c()
[   10.568450] Modules linked in:
[   10.568481] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W       3.19.0-rc6-next-20150202 #2037
[   10.568481] Hardware name: Generic OMAP4 (Flattened Device Tree)
[   10.568481] [<c0015bdc>] (unwind_backtrace) from [<c001222c>] (show_stack+0x10/0x14)
[   10.568511] [<c001222c>] (show_stack) from [<c05d2014>] (dump_stack+0x84/0x9c)
[   10.568511] [<c05d2014>] (dump_stack) from [<c003ea90>] (warn_slowpath_common+0x7c/0xb8)
[   10.568511] [<c003ea90>] (warn_slowpath_common) from [<c003eb68>] (warn_slowpath_null+0x1c/0x24)
[   10.568542] [<c003eb68>] (warn_slowpath_null) from [<c0035800>] (omap3_noncore_dpll_enable+0xdc/0x10c)
[   10.568542] [<c0035800>] (omap3_noncore_dpll_enable) from [<c04c0a10>] (clk_core_enable+0x60/0x9c)
[   10.568572] [<c04c0a10>] (clk_core_enable) from [<c04c09f0>] (clk_core_enable+0x40/0x9c)
[   10.568572] ---[ end trace cb88537fdc8fa212 ]---
...

Regards,

Tony

Re: [PATCH v13 4/6] clk: Add rate constraints to clocks

[Mike Turquette]

Quoting Tony Lindgren (2015-02-02 08:12:37)
> * Geert Uytterhoeven <geert@linux-m68k.org> [150202 00:03]:
> > On Sun, Feb 1, 2015 at 11:18 PM, Mike Turquette <mturquette@linaro.org> wrote:
> > > Quoting Tomeu Vizoso (2015-01-31 10:36:22)
> > >> On 31 January 2015 at 02:31, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > >> > On 01/29, Stephen Boyd wrote:
> > >> >> On 01/29/15 05:31, Geert Uytterhoeven wrote:
> > >> >> > Hi Tomeu, Mike,
> > >> >> >
> > >> >> > On Fri, Jan 23, 2015 at 12:03 PM, Tomeu Vizoso
> > >> >> > <tomeu.vizoso@collabora.com> wrote:
> > >> >> >> --- a/drivers/clk/clk.c
> > >> >> >> +++ b/drivers/clk/clk.c
> > >> >> >> @@ -2391,25 +2543,24 @@ int __clk_get(struct clk *clk)
> > >> >> >>         return 1;
> > >> >> >>  }
> > >> >> >>
> > >> >> >> -static void clk_core_put(struct clk_core *core)
> > >> >> >> +void __clk_put(struct clk *clk)
> > >> >> >>  {
> > >> >> >>         struct module *owner;
> > >> >> >>
> > >> >> >> -       owner = core->owner;
> > >> >> >> +       if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
> > >> >> >> +               return;
> > >> >> >>
> > >> >> >>         clk_prepare_lock();
> > >> >> >> -       kref_put(&core->ref, __clk_release);
> > >> >> >> +
> > >> >> >> +       hlist_del(&clk->child_node);
> > >> >> >> +       clk_core_set_rate_nolock(clk->core, clk->core->req_rate);
> > >> >> > At this point, clk->core->req_rate is still zero, causing
> > >> >> > cpg_div6_clock_round_rate() to be called with a zero "rate" parameter,
> > >> >> > e.g. on r8a7791:
> > >> >>
> > >> >> Hmm.. I wonder if we should assign core->req_rate to be the same as
> > >> >> core->rate during __clk_init()? That would make this call to
> > >> >> clk_core_set_rate_nolock() a nop in this case.
> > >> >>
> > >> >
> > >> > Here's a patch to do this
> > >> >
> > >> > ---8<----
> > >> > From: Stephen Boyd <sboyd@codeaurora.org>
> > >> > Subject: [PATCH] clk: Assign a requested rate by default
> > >> >
> > >> > We need to assign a requested rate here so that we avoid
> > >> > requesting a rate of 0 on clocks when we remove clock consumers.
> > >>
> > >> Hi, this looks good to me.
> > >>
> > >> Reviewed-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> > >
> > > It seems to fix the total boot failure on OMAPs, and hopefully does the
> > > same for SH Mobile and others. I've squashed this into Tomeu's rate
> > > constraints patch to maintain bisect.
> > 
> > Yes, it fixes shmobile. .round_rate() is now called with a sane value of rate.
> 
> Looks like next-20150202 now produces tons of the following errors,
> these from omap4:

next-20150202 is the rolled-back changes from last Friday. I removed the
clock constraints patch and in doing so also rolled back the TI clock
driver migration and clk-private.h removal patches. Those are all back
in clk-next as of last night and it looks as though they missed being
pulled into todays linux-next by a matter of minutes :-/

> 
> [   10.568206] ------------[ cut here ]------------
> [   10.568206] WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:925 clk_disable+0x28/0x34()
> [   10.568237] Modules linked in:
> [   10.568237] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W       3.19.0-rc6-next-20150202 #2037
> [   10.568237] Hardware name: Generic OMAP4 (Flattened Device Tree)
> [   10.568267] [<c0015bdc>] (unwind_backtrace) from [<c001222c>] (show_stack+0x10/0x14)
> [   10.568267] [<c001222c>] (show_stack) from [<c05d2014>] (dump_stack+0x84/0x9c)
> [   10.568267] [<c05d2014>] (dump_stack) from [<c003ea90>] (warn_slowpath_common+0x7c/0xb8)
> [   10.568298] [<c003ea90>] (warn_slowpath_common) from [<c003eb68>] (warn_slowpath_null+0x1c/0x24)
> [   10.568298] [<c003eb68>] (warn_slowpath_null) from [<c04c1ffc>] (clk_disable+0x28/0x34)
> [   10.568328] [<c04c1ffc>] (clk_disable) from [<c0025b3c>] (_disable_clocks+0x18/0x68)
> [   10.568328] [<c0025b3c>] (_disable_clocks) from [<c0026f14>] (_idle+0x10c/0x214)
> [   10.568328] [<c0026f14>] (_idle) from [<c0855fac>] (_setup+0x338/0x410)
> [   10.568359] [<c0855fac>] (_setup) from [<c0027360>] (omap_hwmod_for_each+0x34/0x60)
> [   10.568359] [<c0027360>] (omap_hwmod_for_each) from [<c08563c4>] (__omap_hwmod_setup_all+0x30/0x40)
> [   10.568389] [<c08563c4>] (__omap_hwmod_setup_all) from [<c0008a04>] (do_one_initcall+0x80/0x1dc)
> [   10.568389] [<c0008a04>] (do_one_initcall) from [<c0848ea0>] (kernel_init_freeable+0x204/0x2d0)
> [   10.568420] [<c0848ea0>] (kernel_init_freeable) from [<c05cdab8>] (kernel_init+0x8/0xec)
> [   10.568420] [<c05cdab8>] (kernel_init) from [<c000e790>] (ret_from_fork+0x14/0x24)
> [   10.568420] ---[ end trace cb88537fdc8fa211 ]---

This looks like mis-matched enable/disable calls. We now have unique
struct clk pointers for every call to clk_get. I haven't yet looked
through the hwmod code but I have a feeling that we're doing something
like this:

	/* enable clock */
	my_clk = clk_get(...);
	clk_prepare_enable(my_clk);
	clk_put(my_clk);

	/* do some work */
	do_work();

	/* disable clock */
	my_clk = clk_get(...);
	clk_disable_unprepare(my_clk);
	clk_put(my_clk);

The above pattern no longer works since my_clk will be two different
unique pointers, but it really should be one stable pointer across the
whole usage of the clk. E.g:

	/* enable clock */
	my_clk = clk_get(...);
	clk_prepare_enable(my_clk);

	/* do some work */
	do_work();

	/* disable clock */
	clk_disable_unprepare(my_clk);
	clk_put(my_clk);

Again, I haven't looked through the code, so the above is just an
educated guess.

Anyways I am testing with an OMAP4460 Panda ES and I didn't see the
above. Is there a test you are running to get this?

> 
> 
> [   10.568450] ------------[ cut here ]------------
> [   10.568450] WARNING: CPU: 0 PID: 1 at arch/arm/mach-omap2/dpll3xxx.c:436 omap3_noncore_dpll_enable+0xdc/0
> x10c()
> [   10.568450] Modules linked in:
> [   10.568481] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W       3.19.0-rc6-next-20150202 #2037
> [   10.568481] Hardware name: Generic OMAP4 (Flattened Device Tree)
> [   10.568481] [<c0015bdc>] (unwind_backtrace) from [<c001222c>] (show_stack+0x10/0x14)
> [   10.568511] [<c001222c>] (show_stack) from [<c05d2014>] (dump_stack+0x84/0x9c)
> [   10.568511] [<c05d2014>] (dump_stack) from [<c003ea90>] (warn_slowpath_common+0x7c/0xb8)
> [   10.568511] [<c003ea90>] (warn_slowpath_common) from [<c003eb68>] (warn_slowpath_null+0x1c/0x24)
> [   10.568542] [<c003eb68>] (warn_slowpath_null) from [<c0035800>] (omap3_noncore_dpll_enable+0xdc/0x10c)
> [   10.568542] [<c0035800>] (omap3_noncore_dpll_enable) from [<c04c0a10>] (clk_core_enable+0x60/0x9c)
> [   10.568572] [<c04c0a10>] (clk_core_enable) from [<c04c09f0>] (clk_core_enable+0x40/0x9c)
> [   10.568572] ---[ end trace cb88537fdc8fa212 ]---
> ...

This is the same issue discussed already in this thread[0]. Feedback
from Tero & Paul on how to handle it would be nice.

Please let me know if anything else breaks for you.

Regards,
Mike

> 
> Regards,
> 
> Tony

Re: [PATCH v13 4/6] clk: Add rate constraints to clocks

[Russell King - ARM Linux]

On Mon, Feb 02, 2015 at 09:46:46AM -0800, Mike Turquette wrote:
> This looks like mis-matched enable/disable calls. We now have unique
> struct clk pointers for every call to clk_get. I haven't yet looked
> through the hwmod code but I have a feeling that we're doing something
> like this:
> 
> 	/* enable clock */
> 	my_clk = clk_get(...);
> 	clk_prepare_enable(my_clk);
> 	clk_put(my_clk);
> 
> 	/* do some work */
> 	do_work();
> 
> 	/* disable clock */
> 	my_clk = clk_get(...);
> 	clk_disable_unprepare(my_clk);
> 	clk_put(my_clk);
> 
> The above pattern no longer works since my_clk will be two different
> unique pointers, but it really should be one stable pointer across the
> whole usage of the clk. E.g:

Yes, it has always been documented that shall be the case.  Anyone doing
the above is basically broken.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v13 4/6] clk: Add rate constraints to clocks

[Tony Lindgren]

* Mike Turquette <mturquette@linaro.org> [150202 09:50]:
> Quoting Tony Lindgren (2015-02-02 08:12:37)
> > 
> > [   10.568206] ------------[ cut here ]------------
> > [   10.568206] WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:925 clk_disable+0x28/0x34()
> > [   10.568237] Modules linked in:
> > [   10.568237] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W       3.19.0-rc6-next-20150202 #2037
> > [   10.568237] Hardware name: Generic OMAP4 (Flattened Device Tree)
> > [   10.568267] [<c0015bdc>] (unwind_backtrace) from [<c001222c>] (show_stack+0x10/0x14)
> > [   10.568267] [<c001222c>] (show_stack) from [<c05d2014>] (dump_stack+0x84/0x9c)
> > [   10.568267] [<c05d2014>] (dump_stack) from [<c003ea90>] (warn_slowpath_common+0x7c/0xb8)
> > [   10.568298] [<c003ea90>] (warn_slowpath_common) from [<c003eb68>] (warn_slowpath_null+0x1c/0x24)
> > [   10.568298] [<c003eb68>] (warn_slowpath_null) from [<c04c1ffc>] (clk_disable+0x28/0x34)
> > [   10.568328] [<c04c1ffc>] (clk_disable) from [<c0025b3c>] (_disable_clocks+0x18/0x68)
> > [   10.568328] [<c0025b3c>] (_disable_clocks) from [<c0026f14>] (_idle+0x10c/0x214)
> > [   10.568328] [<c0026f14>] (_idle) from [<c0855fac>] (_setup+0x338/0x410)
> > [   10.568359] [<c0855fac>] (_setup) from [<c0027360>] (omap_hwmod_for_each+0x34/0x60)
> > [   10.568359] [<c0027360>] (omap_hwmod_for_each) from [<c08563c4>] (__omap_hwmod_setup_all+0x30/0x40)
> > [   10.568389] [<c08563c4>] (__omap_hwmod_setup_all) from [<c0008a04>] (do_one_initcall+0x80/0x1dc)
> > [   10.568389] [<c0008a04>] (do_one_initcall) from [<c0848ea0>] (kernel_init_freeable+0x204/0x2d0)
> > [   10.568420] [<c0848ea0>] (kernel_init_freeable) from [<c05cdab8>] (kernel_init+0x8/0xec)
> > [   10.568420] [<c05cdab8>] (kernel_init) from [<c000e790>] (ret_from_fork+0x14/0x24)
> > [   10.568420] ---[ end trace cb88537fdc8fa211 ]---

For reference, the above is line 992 in clk-next.

> This looks like mis-matched enable/disable calls. We now have unique
> struct clk pointers for every call to clk_get. I haven't yet looked
> through the hwmod code but I have a feeling that we're doing something
> like this:
> 
> 	/* enable clock */
> 	my_clk = clk_get(...);
> 	clk_prepare_enable(my_clk);
> 	clk_put(my_clk);
> 
> 	/* do some work */
> 	do_work();
> 
> 	/* disable clock */
> 	my_clk = clk_get(...);
> 	clk_disable_unprepare(my_clk);
> 	clk_put(my_clk);
> 
> The above pattern no longer works since my_clk will be two different
> unique pointers, but it really should be one stable pointer across the
> whole usage of the clk. E.g:
> 
> 	/* enable clock */
> 	my_clk = clk_get(...);
> 	clk_prepare_enable(my_clk);
> 
> 	/* do some work */
> 	do_work();
> 
> 	/* disable clock */
> 	clk_disable_unprepare(my_clk);
> 	clk_put(my_clk);
> 
> Again, I haven't looked through the code, so the above is just an
> educated guess.
> 
> Anyways I am testing with an OMAP4460 Panda ES and I didn't see the
> above. Is there a test you are running to get this?

Just booting 4430-sdp with omap2plus_defconfig. And git bisect
points to 59cf3fcf9baf ("clk: Make clk API return per-user struct
clk instances") as you already guessed.
 
> > [   10.568450] ------------[ cut here ]------------
> > [   10.568450] WARNING: CPU: 0 PID: 1 at arch/arm/mach-omap2/dpll3xxx.c:436 omap3_noncore_dpll_enable+0xdc/0
> > x10c()
> > [   10.568450] Modules linked in:
> > [   10.568481] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W       3.19.0-rc6-next-20150202 #2037
> > [   10.568481] Hardware name: Generic OMAP4 (Flattened Device Tree)
> > [   10.568481] [<c0015bdc>] (unwind_backtrace) from [<c001222c>] (show_stack+0x10/0x14)
> > [   10.568511] [<c001222c>] (show_stack) from [<c05d2014>] (dump_stack+0x84/0x9c)
> > [   10.568511] [<c05d2014>] (dump_stack) from [<c003ea90>] (warn_slowpath_common+0x7c/0xb8)
> > [   10.568511] [<c003ea90>] (warn_slowpath_common) from [<c003eb68>] (warn_slowpath_null+0x1c/0x24)
> > [   10.568542] [<c003eb68>] (warn_slowpath_null) from [<c0035800>] (omap3_noncore_dpll_enable+0xdc/0x10c)
> > [   10.568542] [<c0035800>] (omap3_noncore_dpll_enable) from [<c04c0a10>] (clk_core_enable+0x60/0x9c)
> > [   10.568572] [<c04c0a10>] (clk_core_enable) from [<c04c09f0>] (clk_core_enable+0x40/0x9c)
> > [   10.568572] ---[ end trace cb88537fdc8fa212 ]---
> > ...
> 
> This is the same issue discussed already in this thread[0]. Feedback
> from Tero & Paul on how to handle it would be nice.

Yes that one is a separate issue.
 
> Please let me know if anything else breaks for you.

Also off-idle on omap3 seems to be broken by commit 59cf3fcf9baf.

Regards,

Tony

Re: [PATCH v13 4/6] clk: Add rate constraints to clocks

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [150202 11:28]:
> * Mike Turquette <mturquette@linaro.org> [150202 09:50]:
> > Quoting Tony Lindgren (2015-02-02 08:12:37)
> > > 
> > > [   10.568206] ------------[ cut here ]------------
> > > [   10.568206] WARNING: CPU: 0 PID: 1 at drivers/clk/clk.c:925 clk_disable+0x28/0x34()
> > > [   10.568237] Modules linked in:
> > > [   10.568237] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W       3.19.0-rc6-next-20150202 #2037
> > > [   10.568237] Hardware name: Generic OMAP4 (Flattened Device Tree)
> > > [   10.568267] [<c0015bdc>] (unwind_backtrace) from [<c001222c>] (show_stack+0x10/0x14)
> > > [   10.568267] [<c001222c>] (show_stack) from [<c05d2014>] (dump_stack+0x84/0x9c)
> > > [   10.568267] [<c05d2014>] (dump_stack) from [<c003ea90>] (warn_slowpath_common+0x7c/0xb8)
> > > [   10.568298] [<c003ea90>] (warn_slowpath_common) from [<c003eb68>] (warn_slowpath_null+0x1c/0x24)
> > > [   10.568298] [<c003eb68>] (warn_slowpath_null) from [<c04c1ffc>] (clk_disable+0x28/0x34)
> > > [   10.568328] [<c04c1ffc>] (clk_disable) from [<c0025b3c>] (_disable_clocks+0x18/0x68)
> > > [   10.568328] [<c0025b3c>] (_disable_clocks) from [<c0026f14>] (_idle+0x10c/0x214)
> > > [   10.568328] [<c0026f14>] (_idle) from [<c0855fac>] (_setup+0x338/0x410)
> > > [   10.568359] [<c0855fac>] (_setup) from [<c0027360>] (omap_hwmod_for_each+0x34/0x60)
> > > [   10.568359] [<c0027360>] (omap_hwmod_for_each) from [<c08563c4>] (__omap_hwmod_setup_all+0x30/0x40)
> > > [   10.568389] [<c08563c4>] (__omap_hwmod_setup_all) from [<c0008a04>] (do_one_initcall+0x80/0x1dc)
> > > [   10.568389] [<c0008a04>] (do_one_initcall) from [<c0848ea0>] (kernel_init_freeable+0x204/0x2d0)
> > > [   10.568420] [<c0848ea0>] (kernel_init_freeable) from [<c05cdab8>] (kernel_init+0x8/0xec)
> > > [   10.568420] [<c05cdab8>] (kernel_init) from [<c000e790>] (ret_from_fork+0x14/0x24)
> > > [   10.568420] ---[ end trace cb88537fdc8fa211 ]---
> 
> For reference, the above is line 992 in clk-next.
...
 
> Just booting 4430-sdp with omap2plus_defconfig. And git bisect
> points to 59cf3fcf9baf ("clk: Make clk API return per-user struct
> clk instances") as you already guessed.
>  
> > > [   10.568450] ------------[ cut here ]------------
> > > [   10.568450] WARNING: CPU: 0 PID: 1 at arch/arm/mach-omap2/dpll3xxx.c:436 omap3_noncore_dpll_enable+0xdc/0
> > > x10c()
> > > [   10.568450] Modules linked in:
> > > [   10.568481] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W       3.19.0-rc6-next-20150202 #2037
> > > [   10.568481] Hardware name: Generic OMAP4 (Flattened Device Tree)
> > > [   10.568481] [<c0015bdc>] (unwind_backtrace) from [<c001222c>] (show_stack+0x10/0x14)
> > > [   10.568511] [<c001222c>] (show_stack) from [<c05d2014>] (dump_stack+0x84/0x9c)
> > > [   10.568511] [<c05d2014>] (dump_stack) from [<c003ea90>] (warn_slowpath_common+0x7c/0xb8)
> > > [   10.568511] [<c003ea90>] (warn_slowpath_common) from [<c003eb68>] (warn_slowpath_null+0x1c/0x24)
> > > [   10.568542] [<c003eb68>] (warn_slowpath_null) from [<c0035800>] (omap3_noncore_dpll_enable+0xdc/0x10c)
> > > [   10.568542] [<c0035800>] (omap3_noncore_dpll_enable) from [<c04c0a10>] (clk_core_enable+0x60/0x9c)
> > > [   10.568572] [<c04c0a10>] (clk_core_enable) from [<c04c09f0>] (clk_core_enable+0x40/0x9c)
> > > [   10.568572] ---[ end trace cb88537fdc8fa212 ]---
> > > ...
> > 
> > This is the same issue discussed already in this thread[0]. Feedback
> > from Tero & Paul on how to handle it would be nice.
> 
> Yes that one is a separate issue.
>  
> > Please let me know if anything else breaks for you.
> 
> Also off-idle on omap3 seems to be broken by commit 59cf3fcf9baf.

Actually the two warnigns above are all caused by the same issue.
And the patch Tero just posted fixes both of the issue. It's the thread 
"[PATCH v13 3/6] clk: Make clk API return per-user struct clk instances"
for reference.

Regards,

Tony