From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Mon, 24 Aug 2015 16:43:19 -0700 From: Stephen Boyd To: Doug Anderson Cc: Heiko Stuebner , Michael Turquette , "open list:ARM/Rockchip SoC..." , linux-clk@vger.kernel.org, "linux-arm-kernel@lists.infradead.org" Subject: Re: [PATCH] clk: rockchip: register pll mux before pll itself Message-ID: <20150824234319.GG14330@codeaurora.org> References: <7060610.CC2Q1RMG2L@phil> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: List-ID: On 08/24, Doug Anderson wrote: > Heiko > > On Wed, Aug 19, 2015 at 6:06 AM, Heiko Stuebner wrote: > > 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 > > [] (clk_hw_get_num_parents) from [] (clk_mux_get_parent+0x14/0xc8) > > [] (clk_mux_get_parent) from [] (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 > > --- > > 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(-) > > 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 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? -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Boyd Subject: Re: [PATCH] clk: rockchip: register pll mux before pll itself Date: Mon, 24 Aug 2015 16:43:19 -0700 Message-ID: <20150824234319.GG14330@codeaurora.org> References: <7060610.CC2Q1RMG2L@phil> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+glpar-linux-rockchip=m.gmane.org-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org To: Doug Anderson Cc: "open list:ARM/Rockchip SoC..." , Michael Turquette , Heiko Stuebner , "linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" , linux-clk-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-rockchip.vger.kernel.org On 08/24, Doug Anderson wrote: > Heiko > > On Wed, Aug 19, 2015 at 6:06 AM, Heiko Stuebner wrote: > > 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 > > [] (clk_hw_get_num_parents) from [] (clk_mux_get_parent+0x14/0xc8) > > [] (clk_mux_get_parent) from [] (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 > > --- > > 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(-) > > 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 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? -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project From mboxrd@z Thu Jan 1 00:00:00 1970 From: sboyd@codeaurora.org (Stephen Boyd) Date: Mon, 24 Aug 2015 16:43:19 -0700 Subject: [PATCH] clk: rockchip: register pll mux before pll itself In-Reply-To: References: <7060610.CC2Q1RMG2L@phil> Message-ID: <20150824234319.GG14330@codeaurora.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 08/24, Doug Anderson wrote: > Heiko > > On Wed, Aug 19, 2015 at 6:06 AM, Heiko Stuebner wrote: > > 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 > > [] (clk_hw_get_num_parents) from [] (clk_mux_get_parent+0x14/0xc8) > > [] (clk_mux_get_parent) from [] (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 > > --- > > 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(-) > > 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 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? -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project