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