Re: [PATCH] clks: imx7: fix initial clock setup with deep probe enabled

[Johannes Zink <j.zink@pengutronix.de>]

Hi Sascha,

On Tue, 2022-08-30 at 17:05 +0200, Sascha Hauer wrote:
> On Tue, Aug 30, 2022 at 10:39:37AM +0200, Johannes Zink wrote:
> > We register the i.MX7 clock controller driver at core_initcall
> > level and
> > then do some initial clock setup/reparenting at postcore_initcall
> > level.
> > This doesn't work as expected when deep probe is enabled, because
> > while
> > the driver is registered at core_initcall level, it's only probed
> > later on, currently at postcore_initcall level because it's a
> > dependency
> > of the timer for which of_ensure_device_probed is called.
> > 
> > As the initial clock setup is also at postcore_initcall level, it's
> > no
> > longer guaranteed that the code executes in the same order. Fix
> > this by
> > directly doing the setup at the end of the probe function.
> 
> Does this still work with deep probe disabled?
> 
> I am asking because this effectively reverts bce79428773 ("clk:
> i.MX7: do
> clock reparenting when all clocks are initialized"). Switching all
> i.MX7 boards to deep probe might be a solution as well.
> 
> Sascha
> 

Though I would not _expect_ any problems here, I would like to check
that more thoroughly. This will probably take a bit, please stand by.

Best regards
Johannes

> 
> > 
> > Co-developed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> > Signed-off-by: Johannes Zink <j.zink@pengutronix.de>
> > ---
> >  drivers/clk/imx/clk-imx7.c | 62 +++++++++++++++++-----------------
> > ----
> >  1 file changed, 27 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/clk/imx/clk-imx7.c b/drivers/clk/imx/clk-
> > imx7.c
> > index ffa39d17b0..67876a8404 100644
> > --- a/drivers/clk/imx/clk-imx7.c
> > +++ b/drivers/clk/imx/clk-imx7.c
> > @@ -358,7 +358,32 @@ static int const clks_init_on[] __initconst =
> > {
> >  
> >  static struct clk_onecell_data clk_data;
> >  
> > -static int imx7_clk_initialized;
> > +static void imx7_clk_setup(void)
> > +{
> > +       int i;
> > +
> > +       clks[IMX7D_OSC_24M_CLK] = clk_lookup("osc");
> > +
> > +       for (i = 0; i < ARRAY_SIZE(clks_init_on); i++)
> > +               clk_enable(clks[clks_init_on[i]]);
> > +
> > +       /* use old gpt clk setting, gpt1 root clk must be twice as
> > gpt counter freq */
> > +       clk_set_parent(clks[IMX7D_GPT1_ROOT_SRC],
> > clks[IMX7D_OSC_24M_CLK]);
> > +
> > +       /* set uart module clock's parent clock source that must be
> > great then 80MHz */
> > +       clk_set_parent(clks[IMX7D_UART1_ROOT_SRC],
> > clks[IMX7D_OSC_24M_CLK]);
> > +
> > +       clk_set_parent(clks[IMX7D_ENET1_REF_ROOT_SRC],
> > clks[IMX7D_PLL_ENET_MAIN_125M_CLK]);
> > +       clk_set_parent(clks[IMX7D_ENET1_TIME_ROOT_SRC],
> > clks[IMX7D_PLL_ENET_MAIN_100M_CLK]);
> > +       clk_set_parent(clks[IMX7D_ENET2_REF_ROOT_SRC],
> > clks[IMX7D_PLL_ENET_MAIN_125M_CLK]);
> > +       clk_set_parent(clks[IMX7D_ENET2_TIME_ROOT_SRC],
> > clks[IMX7D_PLL_ENET_MAIN_100M_CLK]);
> > +
> > +       clk_set_rate(clks[IMX7D_PLL_SYS_PFD4_CLK], 392000000);
> > +       clk_set_parent(clks[IMX7D_ENET_AXI_ROOT_SRC],
> > clks[IMX7D_PLL_SYS_PFD4_CLK]);
> > +       clk_set_rate(clks[IMX7D_ENET_AXI_ROOT_CLK], 197000000);
> > +       clk_set_rate(clks[IMX7D_ENET1_TIME_ROOT_CLK], 25000000);
> > +       clk_set_rate(clks[IMX7D_ENET2_TIME_ROOT_CLK], 25000000);
> > +}
> >  
> >  static int imx7_ccm_probe(struct device_d *dev)
> >  {
> > @@ -806,43 +831,10 @@ static int imx7_ccm_probe(struct device_d
> > *dev)
> >         clk_data.clk_num = ARRAY_SIZE(clks);
> >         of_clk_add_provider(dev->device_node,
> > of_clk_src_onecell_get, &clk_data);
> >  
> > -       imx7_clk_initialized = 1;
> > -
> > -       return 0;
> > -}
> > -
> > -static int imx7_clk_setup(void)
> > -{
> > -       int i;
> > -
> > -       if (!imx7_clk_initialized)
> > -               return 0;
> > -
> > -       clks[IMX7D_OSC_24M_CLK] = clk_lookup("osc");
> > -
> > -       for (i = 0; i < ARRAY_SIZE(clks_init_on); i++)
> > -               clk_enable(clks[clks_init_on[i]]);
> > -
> > -       /* use old gpt clk setting, gpt1 root clk must be twice as
> > gpt counter freq */
> > -       clk_set_parent(clks[IMX7D_GPT1_ROOT_SRC],
> > clks[IMX7D_OSC_24M_CLK]);
> > -
> > -       /* set uart module clock's parent clock source that must be
> > great then 80MHz */
> > -       clk_set_parent(clks[IMX7D_UART1_ROOT_SRC],
> > clks[IMX7D_OSC_24M_CLK]);
> > -
> > -       clk_set_parent(clks[IMX7D_ENET1_REF_ROOT_SRC],
> > clks[IMX7D_PLL_ENET_MAIN_125M_CLK]);
> > -       clk_set_parent(clks[IMX7D_ENET1_TIME_ROOT_SRC],
> > clks[IMX7D_PLL_ENET_MAIN_100M_CLK]);
> > -       clk_set_parent(clks[IMX7D_ENET2_REF_ROOT_SRC],
> > clks[IMX7D_PLL_ENET_MAIN_125M_CLK]);
> > -       clk_set_parent(clks[IMX7D_ENET2_TIME_ROOT_SRC],
> > clks[IMX7D_PLL_ENET_MAIN_100M_CLK]);
> > -
> > -       clk_set_rate(clks[IMX7D_PLL_SYS_PFD4_CLK], 392000000);
> > -       clk_set_parent(clks[IMX7D_ENET_AXI_ROOT_SRC],
> > clks[IMX7D_PLL_SYS_PFD4_CLK]);
> > -       clk_set_rate(clks[IMX7D_ENET_AXI_ROOT_CLK], 197000000);
> > -       clk_set_rate(clks[IMX7D_ENET1_TIME_ROOT_CLK], 25000000);
> > -       clk_set_rate(clks[IMX7D_ENET2_TIME_ROOT_CLK], 25000000);
> > +       imx7_clk_setup();
> >  
> >         return 0;
> >  }
> > -postcore_initcall(imx7_clk_setup);
> >  
> >  static __maybe_unused struct of_device_id imx7_ccm_dt_ids[] = {
> >         {
> > -- 
> > 2.30.2
> > 
> > 
> > 
> 

-- 
Pengutronix e.K.                | Johannes Zink                  |
Steuerwalder Str. 21            | https://www.pengutronix.de/    |
31137 Hildesheim, Germany       | Phone: +49-5121-206917-0       |
Amtsgericht Hildesheim, HRA 2686| Fax:   +49-5121-206917-5555    |