clk: rockchip: register pll mux before pll itself
Subsystems (2)
Heiko Stuebner
maintainerPATCH 1/1 clk: rockchip: register pll mux before pll itself
The structure is xin24m -> pll -> pll-mux (xin24m,pll,xin32k). The pll does have an init callback to make sure the boot-selected frequency is using the expected pll settings and resets the same frequency using the values provided in the driver if necessary.
The setting itself also involves remuxing the pll-mux temporarily to the xin24m source to let the new pll rate settle. Until now this worked flawlessly, even when it had the flaw of accessing the mux settings before the mux actually got registered.
With the recent clock-core conversions this flaw became apparent in
null pointer dereference in
[<c03fc400>] (clk_hw_get_num_parents) from [<c0400df0>] (clk_mux_get_parent+0x14/0xc8) [<c0400ddc>] (clk_mux_get_parent) from [<c040246c>] (rockchip_rk3066_pll_set_rate+0xd8/0x320)
So to fix that, simply register the pll-mux before the pll, so that it will be fully initialized when the pll clock executes its init- callback and possibly touches the pll-mux clock.
---
This only surfaced with the clk_core changes for 4.3, so should probably just go on top.
drivers/clk/rockchip/clk-pll.c | 63 +++++++++++++++++++++--------------------- 1 file changed, 32 insertions(+), 31 deletions(-)
drivers/clk/rockchip/clk-pll.c +32 -31
@@ -353,6 +353,35 @@ struct clk *rockchip_clk_register_pll(enum rockchip_pll_type pll_type,if (!pll)return ERR_PTR (-ENOMEM ); +/* create the mux on top of the real pll */ + pll->pll_mux_ops = &clk_mux_ops; + pll_mux = &pll->pll_mux; + pll_mux->reg = base + mode_offset; + pll_mux->shift = mode_shift; + pll_mux->mask =PLL_MODE_MASK ; + pll_mux->flags =0 ; + pll_mux->lock = lock; + pll_mux->hw.init = &init; + +if (pll_type == pll_rk3066) + pll_mux->flags |=CLK_MUX_HIWORD_MASK ; + +/* the actual muxing is xin24m, pll-output, xin32k */ + pll_parents[0 ] = parent_names[0 ]; + pll_parents[1 ] = pll_name; + pll_parents[2 ] = parent_names[1 ]; + + init.name = name; + init.flags =CLK_SET_RATE_PARENT ; + init.ops = pll->pll_mux_ops; + init.parent_names = pll_parents; + init.num_parents =ARRAY_SIZE (pll_parents); + + mux_clk =clk_register (NULL , &pll_mux->hw); +if (IS_ERR (mux_clk)) + goto err_mux; + +/* now create the actual pll */ init.name = pll_name;/* keep all plls untouched for now */
@@ -398,47 +427,19 @@ struct clk *rockchip_clk_register_pll(enum rockchip_pll_type pll_type, pll->flags = clk_pll_flags; pll->lock = lock; -/* create the mux on top of the real pll */ - pll->pll_mux_ops = &clk_mux_ops; - pll_mux = &pll->pll_mux; - pll_mux->reg = base + mode_offset; - pll_mux->shift = mode_shift; - pll_mux->mask =PLL_MODE_MASK ; - pll_mux->flags =0 ; - pll_mux->lock = lock; - pll_mux->hw.init = &init; - -if (pll_type == pll_rk3066) - pll_mux->flags |=CLK_MUX_HIWORD_MASK ; - pll_clk =clk_register (NULL , &pll->hw);if (IS_ERR (pll_clk)) {pr_err ("%s: failed to register pll clock %s : %ld\n" , __func__, name,PTR_ERR (pll_clk)); - mux_clk = pll_clk; goto err_pll; } -/* the actual muxing is xin24m, pll-output, xin32k */ - pll_parents[0 ] = parent_names[0 ]; - pll_parents[1 ] = pll_name; - pll_parents[2 ] = parent_names[1 ]; - - init.name = name; - init.flags =CLK_SET_RATE_PARENT ; - init.ops = pll->pll_mux_ops; - init.parent_names = pll_parents; - init.num_parents =ARRAY_SIZE (pll_parents); - - mux_clk =clk_register (NULL , &pll_mux->hw); -if (IS_ERR (mux_clk)) - goto err_mux; -return mux_clk; -err_mux: -clk_unregister (pll_clk); err_pll: +clk_unregister (mux_clk); + mux_clk = pll_clk; +err_mux:kfree (pll);return mux_clk; }
Signature · 1 line
2.1.4
PATCH 1/1 clk: rockchip: register pll mux before pll itself
Heiko
Quoted 19 lines · Heiko Stuebner
The structure is xin24m -> pll -> pll-mux (xin24m,pll,xin32k). The pll does have an init callback to make sure the boot-selected frequency is using the expected pll settings and resets the same frequency using the values provided in the driver if necessary.
The setting itself also involves remuxing the pll-mux temporarily to the xin24m source to let the new pll rate settle. Until now this worked flawlessly, even when it had the flaw of accessing the mux settings before the mux actually got registered.
With the recent clock-core conversions this flaw became apparent in
null pointer dereference in
[<c03fc400>] (clk_hw_get_num_parents) from [<c0400df0>] (clk_mux_get_parent+0x14/0xc8) [<c0400ddc>] (clk_mux_get_parent) from [<c040246c>] (rockchip_rk3066_pll_set_rate+0xd8/0x320)So to fix that, simply register the pll-mux before the pll, so that it will be fully initialized when the pll clock executes its init- callback and possibly touches the pll-mux clock.
Quoted 7 lines
Signed-off-by Heiko Stuebner <[email protected]>---
This only surfaced with theclk_corechanges for 4.3, so should probably just go on top.drivers/clk/rockchip/clk-pll.c | 63 +++++++++++++++++++++--------------------- 1 file changed, 32 insertions(+), 31 deletions(-)
Fixes boot crash on rk3288-veyron-jerry on next-20150824. It'd be super great to get this landed somewhere so that we can boot linuxnext again. :)
PATCH 1/1 clk: rockchip: register pll mux before pll itself
Quoted 27 lines · Doug Anderson
Heiko
Quoted 24 lines · Heiko Stuebner
The structure is xin24m -> pll -> pll-mux (xin24m,pll,xin32k). The pll does have an init callback to make sure the boot-selected frequency is using the expected pll settings and resets the same frequency using the values provided in the driver if necessary.
The setting itself also involves remuxing the pll-mux temporarily to the xin24m source to let the new pll rate settle. Until now this worked flawlessly, even when it had the flaw of accessing the mux settings before the mux actually got registered.
With the recent clock-core conversions this flaw became apparent in
null pointer dereference in
[<c03fc400>] (clk_hw_get_num_parents) from [<c0400df0>] (clk_mux_get_parent+0x14/0xc8) [<c0400ddc>] (clk_mux_get_parent) from [<c040246c>] (rockchip_rk3066_pll_set_rate+0xd8/0x320)So to fix that, simply register the pll-mux before the pll, so that it will be fully initialized when the pll clock executes its init- callback and possibly touches the pll-mux clock.
Signed-off-by Heiko Stuebner <[email protected]>---
This only surfaced with theclk_corechanges for 4.3, so should probably just go on top.
Quoted 8 lines
Quoted 2 lines
drivers/clk/rockchip/clk-pll.c | 63 +++++++++++++++++++++--------------------- 1 file changed, 32 insertions(+), 31 deletions(-)Fixes boot crash on rk3288-veyron-jerry on next-20150824. It'd be super great to get this landed somewhere so that we can boot linuxnext again. :)
Tested-by Douglas Anderson <[email protected]>
So I understand the fix, but how could it have ever possibly worked flawlessly? clk_mux_get_parent() should have returned -EINVAL through that u8 which would have meant that the check in rockchip_rk3066_pll_set_rate() for cur_parent == PLL_MODE_NORM would never have been true, and we would never have switched the PLL mux over. I guess we've been getting away with this because we don't need to actually switch the mux at this time?
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Heiko Stuebner
maintainerPATCH 1/1 clk: rockchip: register pll mux before pll itself
Am Montag, 24. August 2015, 16:43:19 schrieb Stephen Boyd:
Quoted 39 lines · Doug Anderson
Quoted 32 lines
Heiko
Quoted 20 lines · Heiko Stuebner
The structure is xin24m -> pll -> pll-mux (xin24m,pll,xin32k). The pll does have an init callback to make sure the boot-selected frequency is using the expected pll settings and resets the same frequency using the values provided in the driver if necessary.
The setting itself also involves remuxing the pll-mux temporarily to the xin24m source to let the new pll rate settle. Until now this worked flawlessly, even when it had the flaw of accessing the mux settings before the mux actually got registered.
With the recent clock-core conversions this flaw became apparent in null pointer dereference in
[<c03fc400>] (clk_hw_get_num_parents) from [<c0400df0>] (clk_mux_get_parent+0x14/0xc8) [<c0400ddc>] (clk_mux_get_parent) from [<c040246c>] (rockchip_rk3066_pll_set_rate+0xd8/0x320)So to fix that, simply register the pll-mux before the pll, so that it will be fully initialized when the pll clock executes its init- callback and possibly touches the pll-mux clock.
Quoted 8 lines
Signed-off-by Heiko Stuebner <[email protected]>---
This only surfaced with theclk_corechanges for 4.3, so should probably just go on top.drivers/clk/rockchip/clk-pll.c | 63+++++++++++++++++++++--------------------- 1 file changed, 32 insertions(+), 31 deletions(-)
Quoted 5 lines
Fixes boot crash on rk3288-veyron-jerry on next-20150824. It'd be super great to get this landed somewhere so that we can boot linuxnext again. :)
Tested-by Douglas Anderson <[email protected]>
Quoted 7 lines
So I understand the fix, but how could it have ever possibly worked flawlessly?
clk_mux_get_parent()should have returned-EINVALthrough that u8 which would have meant that the check inrockchip_rk3066_pll_set_rate()forcur_parent==PLL_MODE_NORMwould never have been true, and we would never have switched the PLL mux over. I guess we've been getting away with this because we don't need to actually switch the mux at this time?
The manual mandates the switch away from the pll source before touching the pll settings, aka to the slow mode from the 24MHz source. I guess we were lucky it simply still worked nevertheless.
In normal rate changes both clocks will have been registered already, so only the init callback is affected and also only runs on plls that generate the rate from a different settings-touple that what we have in the rate-table, so not even all plls normally.
PATCH 1/1 clk: rockchip: register pll mux before pll itself
Quoted 48 lines · Heiko Stuebner
Am Montag, 24. August 2015, 16:43:19 schrieb Stephen Boyd:
Quoted 39 lines · Doug Anderson
Quoted 32 lines
Heiko
Quoted 20 lines · Heiko Stuebner
The structure is xin24m -> pll -> pll-mux (xin24m,pll,xin32k). The pll does have an init callback to make sure the boot-selected frequency is using the expected pll settings and resets the same frequency using the values provided in the driver if necessary.
The setting itself also involves remuxing the pll-mux temporarily to the xin24m source to let the new pll rate settle. Until now this worked flawlessly, even when it had the flaw of accessing the mux settings before the mux actually got registered.
With the recent clock-core conversions this flaw became apparent in null pointer dereference in
[<c03fc400>] (clk_hw_get_num_parents) from [<c0400df0>] (clk_mux_get_parent+0x14/0xc8) [<c0400ddc>] (clk_mux_get_parent) from [<c040246c>] (rockchip_rk3066_pll_set_rate+0xd8/0x320)So to fix that, simply register the pll-mux before the pll, so that it will be fully initialized when the pll clock executes its init- callback and possibly touches the pll-mux clock.
Quoted 8 lines
Signed-off-by Heiko Stuebner <[email protected]>---
This only surfaced with theclk_corechanges for 4.3, so should probably just go on top.drivers/clk/rockchip/clk-pll.c | 63+++++++++++++++++++++--------------------- 1 file changed, 32 insertions(+), 31 deletions(-)
Quoted 5 lines
Fixes boot crash on rk3288-veyron-jerry on next-20150824. It'd be super great to get this landed somewhere so that we can boot linuxnext again. :)
Tested-by Douglas Anderson <[email protected]>Quoted 7 lines
So I understand the fix, but how could it have ever possibly worked flawlessly?
clk_mux_get_parent()should have returned-EINVALthrough that u8 which would have meant that the check inrockchip_rk3066_pll_set_rate()forcur_parent==PLL_MODE_NORMwould never have been true, and we would never have switched the PLL mux over. I guess we've been getting away with this because we don't need to actually switch the mux at this time?
Quoted 8 lines
The manual mandates the switch away from the pll source before touching the pll settings, aka to the slow mode from the 24MHz source. I guess we were lucky it simply still worked nevertheless.
In normal rate changes both clocks will have been registered already, so only the init callback is affected and also only runs on plls that generate the rate from a different settings-touple that what we have in the rate-table, so not even all plls normally.
FYI... I also confirm this patch is needed to boot linux-next on the rk3288-veyron-jerry.
Kevin