| Message ID | 1435633127-31952-2-git-send-email-jamesjj.liao@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi James, To be precise, it is the CLK_TOP clocks that have CLK_APMIXED PLLs as their parents, so we cannot enable the CLK_TOP critical clocks until the CLK_APMIXED clocks have all been registered. Please add something like the above to the commit message. More comments inline... On Tue, Jun 30, 2015 at 10:58 AM, James Liao <jamesjj.liao@mediatek.com> wrote: > From: Sascha Hauer <s.hauer@pengutronix.de> > > On the MT8173 the clocks are provided by different units. To enable > the critical clocks we must be sure that all parent clocks are already > registered, otherwise the parents of the critical clocks end up being > unused and get disabled later. To find a place where all parents are > registered we try each time after we've registered some clocks if > all known providers are present now and only then we enable the critical > clocks > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > Signed-off-by: James Liao <jamesjj.liao@mediatek.com> > --- > drivers/clk/mediatek/clk-mt8173.c | 26 +++++++++++++++++++++----- > 1 file changed, 21 insertions(+), 5 deletions(-) > > diff --git a/drivers/clk/mediatek/clk-mt8173.c b/drivers/clk/mediatek/clk-mt8173.c > index 4b9e04c..c483336 100644 > --- a/drivers/clk/mediatek/clk-mt8173.c > +++ b/drivers/clk/mediatek/clk-mt8173.c > @@ -700,6 +700,22 @@ static const struct mtk_composite peri_clks[] __initconst = { > MUX(CLK_PERI_UART3_SEL, "uart3_ck_sel", uart_ck_sel_parents, 0x40c, 3, 1), > }; > > +static struct clk_onecell_data *mt8173_top_clk_data; > +static struct clk_onecell_data *mt8173_pll_clk_data; These globals can be: __initdata > + > +static void mtk_clk_enable_critical(void) And this function is: static void __init Other than the above, this one is: Reviewed-by: Daniel Kurtz <djkurtz@chromium.org> -Dan > +{ > + if (!mt8173_top_clk_data || !mt8173_pll_clk_data) > + return; > + > + clk_prepare_enable(mt8173_pll_clk_data->clks[CLK_APMIXED_ARMCA15PLL]); > + clk_prepare_enable(mt8173_pll_clk_data->clks[CLK_APMIXED_ARMCA7PLL]); > + clk_prepare_enable(mt8173_top_clk_data->clks[CLK_TOP_MEM_SEL]); > + clk_prepare_enable(mt8173_top_clk_data->clks[CLK_TOP_DDRPHYCFG_SEL]); > + clk_prepare_enable(mt8173_top_clk_data->clks[CLK_TOP_CCI400_SEL]); > + clk_prepare_enable(mt8173_top_clk_data->clks[CLK_TOP_RTC_SEL]); > +} > + > static void __init mtk_topckgen_init(struct device_node *node) > { > struct clk_onecell_data *clk_data; > @@ -712,19 +728,19 @@ static void __init mtk_topckgen_init(struct device_node *node) > return; > } > > - clk_data = mtk_alloc_clk_data(CLK_TOP_NR_CLK); > + mt8173_top_clk_data = clk_data = mtk_alloc_clk_data(CLK_TOP_NR_CLK); > > mtk_clk_register_factors(root_clk_alias, ARRAY_SIZE(root_clk_alias), clk_data); > mtk_clk_register_factors(top_divs, ARRAY_SIZE(top_divs), clk_data); > mtk_clk_register_composites(top_muxes, ARRAY_SIZE(top_muxes), base, > &mt8173_clk_lock, clk_data); > > - clk_prepare_enable(clk_data->clks[CLK_TOP_CCI400_SEL]); > - > r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data); > if (r) > pr_err("%s(): could not register clock provider: %d\n", > __func__, r); > + > + mtk_clk_enable_critical(); > } > CLK_OF_DECLARE(mtk_topckgen, "mediatek,mt8173-topckgen", mtk_topckgen_init); > > @@ -818,13 +834,13 @@ static void __init mtk_apmixedsys_init(struct device_node *node) > { > struct clk_onecell_data *clk_data; > > - clk_data = mtk_alloc_clk_data(CLK_APMIXED_NR_CLK); > + mt8173_pll_clk_data = clk_data = mtk_alloc_clk_data(CLK_APMIXED_NR_CLK); > if (!clk_data) > return; > > mtk_clk_register_plls(node, plls, ARRAY_SIZE(plls), clk_data); > > - clk_prepare_enable(clk_data->clks[CLK_APMIXED_ARMCA15PLL]); > + mtk_clk_enable_critical(); > } > CLK_OF_DECLARE(mtk_apmixedsys, "mediatek,mt8173-apmixedsys", > mtk_apmixedsys_init); > -- > 1.8.1.1.dirty >
On Wed, Jul 1, 2015 at 10:21 PM, Daniel Kurtz <djkurtz@chromium.org> wrote: > Hi James, > > To be precise, it is the CLK_TOP clocks that have CLK_APMIXED PLLs as > their parents, so we cannot enable the CLK_TOP critical clocks until > the CLK_APMIXED clocks have all been registered. > > Please add something like the above to the commit message. > > More comments inline... > > On Tue, Jun 30, 2015 at 10:58 AM, James Liao <jamesjj.liao@mediatek.com> wrote: >> From: Sascha Hauer <s.hauer@pengutronix.de> >> >> On the MT8173 the clocks are provided by different units. To enable >> the critical clocks we must be sure that all parent clocks are already >> registered, otherwise the parents of the critical clocks end up being >> unused and get disabled later. To find a place where all parents are >> registered we try each time after we've registered some clocks if >> all known providers are present now and only then we enable the critical >> clocks >> >> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> >> Signed-off-by: James Liao <jamesjj.liao@mediatek.com> >> --- >> drivers/clk/mediatek/clk-mt8173.c | 26 +++++++++++++++++++++----- >> 1 file changed, 21 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/clk/mediatek/clk-mt8173.c b/drivers/clk/mediatek/clk-mt8173.c >> index 4b9e04c..c483336 100644 >> --- a/drivers/clk/mediatek/clk-mt8173.c >> +++ b/drivers/clk/mediatek/clk-mt8173.c >> @@ -700,6 +700,22 @@ static const struct mtk_composite peri_clks[] __initconst = { >> MUX(CLK_PERI_UART3_SEL, "uart3_ck_sel", uart_ck_sel_parents, 0x40c, 3, 1), >> }; >> >> +static struct clk_onecell_data *mt8173_top_clk_data; >> +static struct clk_onecell_data *mt8173_pll_clk_data; > > These globals can be: > __initdata Oops, never mind about this comment... Using "__initdata" for uninitialized variables doesn't make sense - "__initdata" is for initialization data, not variables themselves. Sorry for the noise, -Dan > >> + >> +static void mtk_clk_enable_critical(void) > > And this function is: > > static void __init > > Other than the above, this one is: > > Reviewed-by: Daniel Kurtz <djkurtz@chromium.org> > > -Dan > >> +{ >> + if (!mt8173_top_clk_data || !mt8173_pll_clk_data) >> + return; >> + >> + clk_prepare_enable(mt8173_pll_clk_data->clks[CLK_APMIXED_ARMCA15PLL]); >> + clk_prepare_enable(mt8173_pll_clk_data->clks[CLK_APMIXED_ARMCA7PLL]); >> + clk_prepare_enable(mt8173_top_clk_data->clks[CLK_TOP_MEM_SEL]); >> + clk_prepare_enable(mt8173_top_clk_data->clks[CLK_TOP_DDRPHYCFG_SEL]); >> + clk_prepare_enable(mt8173_top_clk_data->clks[CLK_TOP_CCI400_SEL]); >> + clk_prepare_enable(mt8173_top_clk_data->clks[CLK_TOP_RTC_SEL]); >> +} >> + >> static void __init mtk_topckgen_init(struct device_node *node) >> { >> struct clk_onecell_data *clk_data; >> @@ -712,19 +728,19 @@ static void __init mtk_topckgen_init(struct device_node *node) >> return; >> } >> >> - clk_data = mtk_alloc_clk_data(CLK_TOP_NR_CLK); >> + mt8173_top_clk_data = clk_data = mtk_alloc_clk_data(CLK_TOP_NR_CLK); >> >> mtk_clk_register_factors(root_clk_alias, ARRAY_SIZE(root_clk_alias), clk_data); >> mtk_clk_register_factors(top_divs, ARRAY_SIZE(top_divs), clk_data); >> mtk_clk_register_composites(top_muxes, ARRAY_SIZE(top_muxes), base, >> &mt8173_clk_lock, clk_data); >> >> - clk_prepare_enable(clk_data->clks[CLK_TOP_CCI400_SEL]); >> - >> r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data); >> if (r) >> pr_err("%s(): could not register clock provider: %d\n", >> __func__, r); >> + >> + mtk_clk_enable_critical(); >> } >> CLK_OF_DECLARE(mtk_topckgen, "mediatek,mt8173-topckgen", mtk_topckgen_init); >> >> @@ -818,13 +834,13 @@ static void __init mtk_apmixedsys_init(struct device_node *node) >> { >> struct clk_onecell_data *clk_data; >> >> - clk_data = mtk_alloc_clk_data(CLK_APMIXED_NR_CLK); >> + mt8173_pll_clk_data = clk_data = mtk_alloc_clk_data(CLK_APMIXED_NR_CLK); >> if (!clk_data) >> return; >> >> mtk_clk_register_plls(node, plls, ARRAY_SIZE(plls), clk_data); >> >> - clk_prepare_enable(clk_data->clks[CLK_APMIXED_ARMCA15PLL]); >> + mtk_clk_enable_critical(); >> } >> CLK_OF_DECLARE(mtk_apmixedsys, "mediatek,mt8173-apmixedsys", >> mtk_apmixedsys_init); >> -- >> 1.8.1.1.dirty >>
Hi Daniel, On Wed, 2015-07-01 at 22:21 +0800, Daniel Kurtz wrote: > To be precise, it is the CLK_TOP clocks that have CLK_APMIXED PLLs as > their parents, so we cannot enable the CLK_TOP critical clocks until > the CLK_APMIXED clocks have all been registered. > > Please add something like the above to the commit message. OK. I'll add them. > > > > > +static struct clk_onecell_data *mt8173_top_clk_data; > > +static struct clk_onecell_data *mt8173_pll_clk_data; > > These globals can be: > __initdata > > > + > > +static void mtk_clk_enable_critical(void) > > And this function is: > > static void __init I'll add them in next patch. Best regards, James
On 06/30, James Liao wrote: > From: Sascha Hauer <s.hauer@pengutronix.de> > > On the MT8173 the clocks are provided by different units. To enable > the critical clocks we must be sure that all parent clocks are already > registered, otherwise the parents of the critical clocks end up being > unused and get disabled later. To find a place where all parents are > registered we try each time after we've registered some clocks if > all known providers are present now and only then we enable the critical > clocks > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > Signed-off-by: James Liao <jamesjj.liao@mediatek.com> > --- Applied to clk-fixes
Hi Stephen, On Fri, Jul 3, 2015 at 7:03 AM, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 06/30, James Liao wrote: >> From: Sascha Hauer <s.hauer@pengutronix.de> >> >> On the MT8173 the clocks are provided by different units. To enable >> the critical clocks we must be sure that all parent clocks are already >> registered, otherwise the parents of the critical clocks end up being >> unused and get disabled later. To find a place where all parents are >> registered we try each time after we've registered some clocks if >> all known providers are present now and only then we enable the critical >> clocks >> >> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> >> Signed-off-by: James Liao <jamesjj.liao@mediatek.com> >> --- > > Applied to clk-fixes I think James plans to send an updated version of this patch. > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project
On 07/02/2015 11:29 PM, Daniel Kurtz wrote: > Hi Stephen, > > On Fri, Jul 3, 2015 at 7:03 AM, Stephen Boyd <sboyd@codeaurora.org> wrote: >> On 06/30, James Liao wrote: >>> From: Sascha Hauer <s.hauer@pengutronix.de> >>> >>> On the MT8173 the clocks are provided by different units. To enable >>> the critical clocks we must be sure that all parent clocks are already >>> registered, otherwise the parents of the critical clocks end up being >>> unused and get disabled later. To find a place where all parents are >>> registered we try each time after we've registered some clocks if >>> all known providers are present now and only then we enable the critical >>> clocks >>> >>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> >>> Signed-off-by: James Liao <jamesjj.liao@mediatek.com> >>> --- >> Applied to clk-fixes > I think James plans to send an updated version of this patch. > > Oh right. Well those comments are minor so I'll just fold them in given that I'm rewinding clk-fixes today. I see that comment in init.h about uninitialized variables and __initdata too. I found this 10 year old thread about that [1]. At least with my gcc-4.9 arm compiler I don't see uninitialized __initdata moved to bss. A workaround seems to be to initialize it to NULL in this case, but maybe that doesn't matter because compilers don't have that problem anymore. [1] http://lkml.iu.edu/hypermail/linux/kernel/0512.0/1366.html
diff --git a/drivers/clk/mediatek/clk-mt8173.c b/drivers/clk/mediatek/clk-mt8173.c index 4b9e04c..c483336 100644 --- a/drivers/clk/mediatek/clk-mt8173.c +++ b/drivers/clk/mediatek/clk-mt8173.c @@ -700,6 +700,22 @@ static const struct mtk_composite peri_clks[] __initconst = { MUX(CLK_PERI_UART3_SEL, "uart3_ck_sel", uart_ck_sel_parents, 0x40c, 3, 1), }; +static struct clk_onecell_data *mt8173_top_clk_data; +static struct clk_onecell_data *mt8173_pll_clk_data; + +static void mtk_clk_enable_critical(void) +{ + if (!mt8173_top_clk_data || !mt8173_pll_clk_data) + return; + + clk_prepare_enable(mt8173_pll_clk_data->clks[CLK_APMIXED_ARMCA15PLL]); + clk_prepare_enable(mt8173_pll_clk_data->clks[CLK_APMIXED_ARMCA7PLL]); + clk_prepare_enable(mt8173_top_clk_data->clks[CLK_TOP_MEM_SEL]); + clk_prepare_enable(mt8173_top_clk_data->clks[CLK_TOP_DDRPHYCFG_SEL]); + clk_prepare_enable(mt8173_top_clk_data->clks[CLK_TOP_CCI400_SEL]); + clk_prepare_enable(mt8173_top_clk_data->clks[CLK_TOP_RTC_SEL]); +} + static void __init mtk_topckgen_init(struct device_node *node) { struct clk_onecell_data *clk_data; @@ -712,19 +728,19 @@ static void __init mtk_topckgen_init(struct device_node *node) return; } - clk_data = mtk_alloc_clk_data(CLK_TOP_NR_CLK); + mt8173_top_clk_data = clk_data = mtk_alloc_clk_data(CLK_TOP_NR_CLK); mtk_clk_register_factors(root_clk_alias, ARRAY_SIZE(root_clk_alias), clk_data); mtk_clk_register_factors(top_divs, ARRAY_SIZE(top_divs), clk_data); mtk_clk_register_composites(top_muxes, ARRAY_SIZE(top_muxes), base, &mt8173_clk_lock, clk_data); - clk_prepare_enable(clk_data->clks[CLK_TOP_CCI400_SEL]); - r = of_clk_add_provider(node, of_clk_src_onecell_get, clk_data); if (r) pr_err("%s(): could not register clock provider: %d\n", __func__, r); + + mtk_clk_enable_critical(); } CLK_OF_DECLARE(mtk_topckgen, "mediatek,mt8173-topckgen", mtk_topckgen_init); @@ -818,13 +834,13 @@ static void __init mtk_apmixedsys_init(struct device_node *node) { struct clk_onecell_data *clk_data; - clk_data = mtk_alloc_clk_data(CLK_APMIXED_NR_CLK); + mt8173_pll_clk_data = clk_data = mtk_alloc_clk_data(CLK_APMIXED_NR_CLK); if (!clk_data) return; mtk_clk_register_plls(node, plls, ARRAY_SIZE(plls), clk_data); - clk_prepare_enable(clk_data->clks[CLK_APMIXED_ARMCA15PLL]); + mtk_clk_enable_critical(); } CLK_OF_DECLARE(mtk_apmixedsys, "mediatek,mt8173-apmixedsys", mtk_apmixedsys_init);