[PATCH v2] clk: Properly update prepare/enable count on orphan clock reparent
Jerome Brunet
jbrunet at baylibre.com
Thu Feb 15 03:44:11 PST 2018
On Wed, 2018-02-14 at 14:37 +0100, Marek Szyprowski wrote:
> Hi Stephen,
>
> On 2018-01-30 10:01, Marek Szyprowski wrote:
> > Hi Stephen
> >
> > On 2018-01-27 01:49, Stephen Boyd wrote:
> > > On 01/26, Marek Szyprowski wrote:
> > > > If orphaned clock has been already prepared/enabled (for example if
> > > > it or
> > > > one of its children has CLK_IS_CRITICAL flag), then the prepare/enable
> > > > counters of the newly assigned parent are not updated correctly. This
> > > > might later cause warnings during changing clock parents.
> > >
> > > This doesn't feel right. Perhaps we should delay enabling a clk
> > > if it's CRITICAL until we adopt an orphaned clk. Good news is we
> > > have orphan status tracking now so this should be pretty simple.
> >
> > Not really. It won't be so simple, because we would need to track the
> > status of the whole clock sub-tree for enabling critical clocks. Even
> > then we would need to delay enabling them until the real top clock is
> > registered.
> >
> > > Otherwise migrating the count up is complicated and requires us
> > > to call the prepare/enable ops on a critical clk and then keep
> > > doing that each time it gets re-parented. Do you have this case,
> > > where some clk is marked as CRITICAL, and then we need to migrate
> > > that enable/prepare count to the parent?
> >
> > Yes, this is the case of Exynos5422-based boards, where we have a
> > bunch of critical clocks, which are reparented from internal PLL to
> > external oscillator when respective power domain is suspended.
> >
> > It took me a while to gather all the needed information from the
> > debug logs. Here is one example of such clock tree:
> >
> > fin_pll (clk1, top clock, external pll)
> > fout_dpll (clk2)
> > mout_sclk_dpll (clk3)
> > mout_aclk300_disp1 (clk4)
> > dout_aclk300_disp1 (clk5)
> > mout_sw_aclk300_disp1 (clk6)
> > mout_user_aclk300_disp1 (clk7)
> > aclk300_disp1 (clk8, critical)
> >
> > Before turning off power domains, aclk300_disp1 is reparented
> > directly to fin_pll (this is a hardware requirement). When power
> > domain is turned on again, aclk300_disp1 is reparented back to
> > mout_user_aclk300_disp1.
> >
> > Clocks are registered in the following order:
> > 1, 2, 7, 3, 6, 5, 8, 4.
> >
> > In this case when critical clock is registered, clk4 is not yet
> > available, so clocks 1-3 won't get proper prepare/enable count.
> > Then this causes a warning during reparenting clk8 to clk1:
> >
> > ------------[ cut here ]------------
> > WARNING: CPU: 0 PID: 59 at drivers/clk/clk.c:811
> > clk_core_disable_lock+0x18/0x24
> > Modules linked in:
> > CPU: 0 PID: 59 Comm: kworker/0:1 Not tainted
> > 4.15.0-next-20180130-dirty #131
> > Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> > Workqueue: pm genpd_power_off_work_fn
> > [<c01114e4>] (unwind_backtrace) from [<c010dbc0>] (show_stack+0x10/0x14)
> > [<c010dbc0>] (show_stack) from [<c09b7b94>] (dump_stack+0x90/0xc8)
> > [<c09b7b94>] (dump_stack) from [<c0124ff8>] (__warn+0xe4/0x110)
> > [<c0124ff8>] (__warn) from [<c0125064>] (warn_slowpath_null+0x40/0x48)
> > [<c0125064>] (warn_slowpath_null) from [<c048e650>]
> > (clk_core_disable_lock+0x18/0x24)
> > [<c048e650>] (clk_core_disable_lock) from [<c048f51c>]
> > (clk_core_disable_unprepare+0xc/0x20)
> > [<c048f51c>] (clk_core_disable_unprepare) from [<c048faec>]
> > (__clk_set_parent_after+0x48/0x4c)
> > [<c048faec>] (__clk_set_parent_after) from [<c048fd28>]
> > (clk_core_set_parent_nolock+0x238/0x5d0)
> > [<c048fd28>] (clk_core_set_parent_nolock) from [<c04902e4>]
> > (clk_set_parent+0x38/0x6c)
> > [<c04902e4>] (clk_set_parent) from [<c049d760>]
> > (exynos_pd_power+0x74/0x1cc)
> > [<c049d760>] (exynos_pd_power) from [<c0552064>]
> > (genpd_power_off+0x164/0x264)
> > [<c0552064>] (genpd_power_off) from [<c0552430>]
> > (genpd_power_off_work_fn+0x2c/0x40)
> > [<c0552430>] (genpd_power_off_work_fn) from [<c014355c>]
> > (process_one_work+0x1d0/0x7bc)
> > [<c014355c>] (process_one_work) from [<c0143bb4>]
> > (worker_thread+0x34/0x4dc)
> > [<c0143bb4>] (worker_thread) from [<c014a348>] (kthread+0x128/0x164)
> > [<c014a348>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20)
> > Exception stack(0xeeb01fb0 to 0xeeb01ff8)
> > 1fa0: 00000000 00000000 00000000
> > 00000000
> > 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> > 00000000
> > 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> > ---[ end trace 48eea511a44c78ef ]---
> > ------------[ cut here ]------------
> > WARNING: CPU: 0 PID: 59 at drivers/clk/clk.c:684
> > clk_core_disable_unprepare+0x18/0x20
> > Modules linked in:
> > CPU: 0 PID: 59 Comm: kworker/0:1 Tainted: G W
> > 4.15.0-next-20180130-dirty #131
> > Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> > Workqueue: pm genpd_power_off_work_fn
> > [<c01114e4>] (unwind_backtrace) from [<c010dbc0>] (show_stack+0x10/0x14)
> > [<c010dbc0>] (show_stack) from [<c09b7b94>] (dump_stack+0x90/0xc8)
> > [<c09b7b94>] (dump_stack) from [<c0124ff8>] (__warn+0xe4/0x110)
> > [<c0124ff8>] (__warn) from [<c0125064>] (warn_slowpath_null+0x40/0x48)
> > [<c0125064>] (warn_slowpath_null) from [<c048f528>]
> > (clk_core_disable_unprepare+0x18/0x20)
> > [<c048f528>] (clk_core_disable_unprepare) from [<c048faec>]
> > (__clk_set_parent_after+0x48/0x4c)
> > [<c048faec>] (__clk_set_parent_after) from [<c048fd28>]
> > (clk_core_set_parent_nolock+0x238/0x5d0)
> > [<c048fd28>] (clk_core_set_parent_nolock) from [<c04902e4>]
> > (clk_set_parent+0x38/0x6c)
> > [<c04902e4>] (clk_set_parent) from [<c049d760>]
> > (exynos_pd_power+0x74/0x1cc)
> > [<c049d760>] (exynos_pd_power) from [<c0552064>]
> > (genpd_power_off+0x164/0x264)
> > [<c0552064>] (genpd_power_off) from [<c0552430>]
> > (genpd_power_off_work_fn+0x2c/0x40)
> > [<c0552430>] (genpd_power_off_work_fn) from [<c014355c>]
> > (process_one_work+0x1d0/0x7bc)
> > [<c014355c>] (process_one_work) from [<c0143bb4>]
> > (worker_thread+0x34/0x4dc)
> > [<c0143bb4>] (worker_thread) from [<c014a348>] (kthread+0x128/0x164)
> > [<c014a348>] (kthread) from [<c01010b4>] (ret_from_fork+0x14/0x20)
> > Exception stack(0xeeb01fb0 to 0xeeb01ff8)
> > 1fa0: 00000000 00000000 00000000
> > 00000000
> > 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> > 00000000
> > 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
> > ---[ end trace 48eea511a44c78f0 ]---
> >
> > Please note that this pattern happens for other critical clocks too.
> >
> > > Hopefully it isn't the worser case, where the clk is handed out
> > > to some consumer but it's still orphaned at that point, and then
> > > we have little control over the migration of state to the parent.
> >
> > Well, in my opinion my patch is simplest fix for the regression
> > introduced by commit f8f8f1d04494 ("clk: Don't touch hardware when
> > reparenting during registration"). It also doesn't have any side-effects
> > as it affects only the situation when orphaned clock has been already
> > prepared/enabled, what in practice means it or one of its children has
> > critical flag. Reworking critical clock handling will be much more
> > complicated.
>
> Stephen, do you have any comments?
>
> The issue is there for almost 2 months... v4.16-rc1 is out and its
> a good time to get a fix in (this or any other if you propose such).
>
> Best regards
Hi Marek,
I've sent another patch addressing the same issue (sorry, I did not see your
thread earlier)
While I agree parent adopting orphan need to take care of any existing count,
your solution could be missing a few things, like dealing with
CLK_OPS_PARENT_ENABLE clock types
Fortunately, __clk_set_parent_before() and __clk_set_parent_after() already deal
with those annoying stuff and does count migration well.
Would you mind trying this change [0] and let me know if works for you as well ?
[0] : https://lkml.kernel.org/r/20180214134340.17242-5-jbrunet@baylibre.com
Cheers
Jerome
More information about the linux-arm-kernel
mailing list