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

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello,

On 31.08.22 10:21, Johannes Zink wrote:
> 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.

fixed clocks are registered via CLK_OF_DECLARE, so by the time the
imx7 clock probe runs, they will always be available, independent of
whether deep probe is enabled.

> 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.

I looked at this with Johannes and I am puzzled how the problem
that bce79428773 aimed to fix occurred. Could you elaborate?

Thanks,
Ahmad

> 
> 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.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |