Re: [PATCH] clk: rockchip: register pll mux before pll itself

[Stephen Boyd <sboyd@codeaurora.org>]

On 08/24, Doug Anderson wrote:
> Heiko
> 
> On Wed, Aug 19, 2015 at 6:06 AM, Heiko Stuebner <heiko@sntech.de> 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
> > [<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 <heiko@sntech.de>
> > ---
> > 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 <dianders@chromium.org>

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