mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH] clks: imx7: fix initial clock setup with deep probe enabled
@ 2022-08-30  8:39 Johannes Zink
  2022-08-30 15:05 ` Sascha Hauer
  0 siblings, 1 reply; 5+ messages in thread
From: Johannes Zink @ 2022-08-30  8:39 UTC (permalink / raw)
  To: barebox; +Cc: Ahmad Fatoum, Johannes Zink

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.

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




^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] clks: imx7: fix initial clock setup with deep probe enabled
  2022-08-30  8:39 [PATCH] clks: imx7: fix initial clock setup with deep probe enabled Johannes Zink
@ 2022-08-30 15:05 ` Sascha Hauer
  2022-08-31  8:21   ` Johannes Zink
  0 siblings, 1 reply; 5+ messages in thread
From: Sascha Hauer @ 2022-08-30 15:05 UTC (permalink / raw)
  To: Johannes Zink; +Cc: barebox, Ahmad Fatoum

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


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



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] clks: imx7: fix initial clock setup with deep probe enabled
  2022-08-30 15:05 ` Sascha Hauer
@ 2022-08-31  8:21   ` Johannes Zink
  2022-09-02 14:02     ` Ahmad Fatoum
  0 siblings, 1 reply; 5+ messages in thread
From: Johannes Zink @ 2022-08-31  8:21 UTC (permalink / raw)
  To: Sascha Hauer; +Cc: barebox, Ahmad Fatoum

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    |




^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] clks: imx7: fix initial clock setup with deep probe enabled
  2022-08-31  8:21   ` Johannes Zink
@ 2022-09-02 14:02     ` Ahmad Fatoum
  2022-09-02 18:38       ` Sascha Hauer
  0 siblings, 1 reply; 5+ messages in thread
From: Ahmad Fatoum @ 2022-09-02 14:02 UTC (permalink / raw)
  To: Johannes Zink, Sascha Hauer; +Cc: barebox

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 |



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] clks: imx7: fix initial clock setup with deep probe enabled
  2022-09-02 14:02     ` Ahmad Fatoum
@ 2022-09-02 18:38       ` Sascha Hauer
  0 siblings, 0 replies; 5+ messages in thread
From: Sascha Hauer @ 2022-09-02 18:38 UTC (permalink / raw)
  To: Ahmad Fatoum; +Cc: Johannes Zink, barebox

On Fri, Sep 02, 2022 at 04:02:32PM +0200, Ahmad Fatoum wrote:
> 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.

So this problem is already fixed, great.

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

Have a look at the tree at bce79428773. Back then the i.MX7 clock driver
was registered at core_initcall level, the same level that of_probe()
was running which first called of_platform_populate() and then
of_clk_init().

Sascha

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



^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-09-02 18:40 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-30  8:39 [PATCH] clks: imx7: fix initial clock setup with deep probe enabled Johannes Zink
2022-08-30 15:05 ` Sascha Hauer
2022-08-31  8:21   ` Johannes Zink
2022-09-02 14:02     ` Ahmad Fatoum
2022-09-02 18:38       ` Sascha Hauer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox