| Message ID | 1421748570-14282-9-git-send-email-Emilian.Medve@Freescale.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Scott Wood |
| Headers | show |
Which platform are you trying to use this on? Can this be initialized by core pll function core_pll_init()? I just saw most of this function is silimar to the core_pll_init(). Thanks, Yuantian > -----Original Message----- > From: Emil Medve [mailto:Emilian.Medve@Freescale.com] > Sent: Tuesday, January 20, 2015 6:10 PM > To: linuxppc-dev@lists.ozlabs.org; Wood Scott-B07421; mturquette@linaro.org; > haokexin@gmail.com; Tang Yuantian-B29983 > Cc: Medve Emilian-EMMEDVE1 > Subject: [PATCH 8/8] clk: ppc-corenet: Add support for the platform PLL > > Change-Id: Iac11ed95f274485a86d2c11f32a3dc502bcd020f > Signed-off-by: Emil Medve <Emilian.Medve@Freescale.com> > --- > drivers/clk/clk-ppc-corenet.c | 85 > +++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 85 insertions(+) > > diff --git a/drivers/clk/clk-ppc-corenet.c b/drivers/clk/clk-ppc-corenet.c index > 91816b1..ff425e1 100644 > --- a/drivers/clk/clk-ppc-corenet.c > +++ b/drivers/clk/clk-ppc-corenet.c > @@ -7,6 +7,9 @@ > * > * clock driver for Freescale PowerPC corenet SoCs. > */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > #include <linux/clk-provider.h> > #include <linux/io.h> > #include <linux/kernel.h> > @@ -261,9 +264,91 @@ static void __init sysclk_init(struct device_node *node) > of_clk_add_provider(np, of_clk_src_simple_get, clk); } > > +static void __init pltfrm_pll_init(struct device_node *np) { > + void __iomem *base; > + uint32_t mult; > + const char *parent_name, *clk_name; > + int i, _errno; > + struct clk_onecell_data *cod; > + > + base = of_iomap(np, 0); > + if (!base) { > + pr_err("%s(): %s: of_iomap() failed\n", __func__, np->name); > + return; > + } > + > + /* Get the multiple of PLL */ > + mult = ioread32be(base); > + > + iounmap(base); > + > + /* Check if this PLL is disabled */ > + if (mult & PLL_KILL) { > + pr_debug("%s(): %s: Disabled\n", __func__, np->name); > + return; > + } > + mult = (mult & GENMASK(6, 1)) >> 1; > + > + parent_name = of_clk_get_parent_name(np, 0); > + if (!parent_name) { > + pr_err("%s(): %s: of_clk_get_parent_name() failed\n", > + __func__, np->name); > + return; > + } > + > + i = of_property_count_strings(np, "clock-output-names"); > + if (i < 0) { > + pr_err("%s(): %s: of_property_count_strings(clock-output-names) > = %d\n", > + __func__, np->name, i); > + return; > + } > + > + cod = kmalloc(sizeof(*cod) + i * sizeof(struct clk *), GFP_KERNEL); > + if (!cod) > + return; > + cod->clks = (struct clk **)(cod + 1); > + cod->clk_num = i; > + > + for (i = 0; i < cod->clk_num; i++) { > + _errno = of_property_read_string_index(np, "clock-output-names", > + i, &clk_name); > + if (_errno < 0) { > + pr_err("%s(): %s: > of_property_read_string_index(clock-output-names) = %d\n", > + __func__, np->name, _errno); > + goto return_clk_unregister; > + } > + > + cod->clks[i] = clk_register_fixed_factor(NULL, clk_name, > + parent_name, 0, mult, 1 + i); > + if (IS_ERR(cod->clks[i])) { > + pr_err("%s(): %s: clk_register_fixed_factor(%s) = %ld\n", > + __func__, np->name, > + clk_name, PTR_ERR(cod->clks[i])); > + goto return_clk_unregister; > + } > + } > + > + _errno = of_clk_add_provider(np, of_clk_src_onecell_get, cod); > + if (_errno < 0) { > + pr_err("%s(): %s: of_clk_add_provider() = %d\n", > + __func__, np->name, _errno); > + goto return_clk_unregister; > + } > + > + return; > + > +return_clk_unregister: > + while (--i >= 0) > + clk_unregister(cod->clks[i]); > + kfree(cod); > +} > + > CLK_OF_DECLARE(qoriq_sysclk_1, "fsl,qoriq-sysclk-1.0", sysclk_init); > CLK_OF_DECLARE(qoriq_sysclk_2, "fsl,qoriq-sysclk-2.0", sysclk_init); > CLK_OF_DECLARE(qoriq_core_pll_1, "fsl,qoriq-core-pll-1.0", core_pll_init); > CLK_OF_DECLARE(qoriq_core_pll_2, "fsl,qoriq-core-pll-2.0", core_pll_init); > CLK_OF_DECLARE(qoriq_core_mux_1, "fsl,qoriq-core-mux-1.0", core_mux_init); > CLK_OF_DECLARE(qoriq_core_mux_2, "fsl,qoriq-core-mux-2.0", core_mux_init); > +CLK_OF_DECLARE(qoriq_pltfrm_pll_1, "fsl,qoriq-platform-pll-1.0", > +pltfrm_pll_init); CLK_OF_DECLARE(qoriq_pltfrm_pll_2, > +"fsl,qoriq-platform-pll-2.0", pltfrm_pll_init); > -- > 2.2.2
Hello Yuan-Tian, On 01/20/2015 11:42 PM, Tang Yuantian-B29983 wrote: > Which platform are you trying to use this on? CoreNet chassis v1 and v2 SoC(s) > Can this be initialized by core pll function core_pll_init()? > I just saw most of this function is silimar to the core_pll_init(). Yes, the flow is similar, but core_pll_init() makes assumptions that it shouldn't or are not relevant to the platform PLL Cheers, > Thanks, > Yuantian > >> -----Original Message----- >> From: Emil Medve [mailto:Emilian.Medve@Freescale.com] >> Sent: Tuesday, January 20, 2015 6:10 PM >> To: linuxppc-dev@lists.ozlabs.org; Wood Scott-B07421; mturquette@linaro.org; >> haokexin@gmail.com; Tang Yuantian-B29983 >> Cc: Medve Emilian-EMMEDVE1 >> Subject: [PATCH 8/8] clk: ppc-corenet: Add support for the platform PLL >> >> Change-Id: Iac11ed95f274485a86d2c11f32a3dc502bcd020f >> Signed-off-by: Emil Medve <Emilian.Medve@Freescale.com> >> --- >> drivers/clk/clk-ppc-corenet.c | 85 >> +++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 85 insertions(+) >> >> diff --git a/drivers/clk/clk-ppc-corenet.c b/drivers/clk/clk-ppc-corenet.c index >> 91816b1..ff425e1 100644 >> --- a/drivers/clk/clk-ppc-corenet.c >> +++ b/drivers/clk/clk-ppc-corenet.c >> @@ -7,6 +7,9 @@ >> * >> * clock driver for Freescale PowerPC corenet SoCs. >> */ >> + >> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt >> + >> #include <linux/clk-provider.h> >> #include <linux/io.h> >> #include <linux/kernel.h> >> @@ -261,9 +264,91 @@ static void __init sysclk_init(struct device_node *node) >> of_clk_add_provider(np, of_clk_src_simple_get, clk); } >> >> +static void __init pltfrm_pll_init(struct device_node *np) { >> + void __iomem *base; >> + uint32_t mult; >> + const char *parent_name, *clk_name; >> + int i, _errno; >> + struct clk_onecell_data *cod; >> + >> + base = of_iomap(np, 0); >> + if (!base) { >> + pr_err("%s(): %s: of_iomap() failed\n", __func__, np->name); >> + return; >> + } >> + >> + /* Get the multiple of PLL */ >> + mult = ioread32be(base); >> + >> + iounmap(base); >> + >> + /* Check if this PLL is disabled */ >> + if (mult & PLL_KILL) { >> + pr_debug("%s(): %s: Disabled\n", __func__, np->name); >> + return; >> + } >> + mult = (mult & GENMASK(6, 1)) >> 1; >> + >> + parent_name = of_clk_get_parent_name(np, 0); >> + if (!parent_name) { >> + pr_err("%s(): %s: of_clk_get_parent_name() failed\n", >> + __func__, np->name); >> + return; >> + } >> + >> + i = of_property_count_strings(np, "clock-output-names"); >> + if (i < 0) { >> + pr_err("%s(): %s: of_property_count_strings(clock-output-names) >> = %d\n", >> + __func__, np->name, i); >> + return; >> + } >> + >> + cod = kmalloc(sizeof(*cod) + i * sizeof(struct clk *), GFP_KERNEL); >> + if (!cod) >> + return; >> + cod->clks = (struct clk **)(cod + 1); >> + cod->clk_num = i; >> + >> + for (i = 0; i < cod->clk_num; i++) { >> + _errno = of_property_read_string_index(np, "clock-output-names", >> + i, &clk_name); >> + if (_errno < 0) { >> + pr_err("%s(): %s: >> of_property_read_string_index(clock-output-names) = %d\n", >> + __func__, np->name, _errno); >> + goto return_clk_unregister; >> + } >> + >> + cod->clks[i] = clk_register_fixed_factor(NULL, clk_name, >> + parent_name, 0, mult, 1 + i); >> + if (IS_ERR(cod->clks[i])) { >> + pr_err("%s(): %s: clk_register_fixed_factor(%s) = %ld\n", >> + __func__, np->name, >> + clk_name, PTR_ERR(cod->clks[i])); >> + goto return_clk_unregister; >> + } >> + } >> + >> + _errno = of_clk_add_provider(np, of_clk_src_onecell_get, cod); >> + if (_errno < 0) { >> + pr_err("%s(): %s: of_clk_add_provider() = %d\n", >> + __func__, np->name, _errno); >> + goto return_clk_unregister; >> + } >> + >> + return; >> + >> +return_clk_unregister: >> + while (--i >= 0) >> + clk_unregister(cod->clks[i]); >> + kfree(cod); >> +} >> + >> CLK_OF_DECLARE(qoriq_sysclk_1, "fsl,qoriq-sysclk-1.0", sysclk_init); >> CLK_OF_DECLARE(qoriq_sysclk_2, "fsl,qoriq-sysclk-2.0", sysclk_init); >> CLK_OF_DECLARE(qoriq_core_pll_1, "fsl,qoriq-core-pll-1.0", core_pll_init); >> CLK_OF_DECLARE(qoriq_core_pll_2, "fsl,qoriq-core-pll-2.0", core_pll_init); >> CLK_OF_DECLARE(qoriq_core_mux_1, "fsl,qoriq-core-mux-1.0", core_mux_init); >> CLK_OF_DECLARE(qoriq_core_mux_2, "fsl,qoriq-core-mux-2.0", core_mux_init); >> +CLK_OF_DECLARE(qoriq_pltfrm_pll_1, "fsl,qoriq-platform-pll-1.0", >> +pltfrm_pll_init); CLK_OF_DECLARE(qoriq_pltfrm_pll_2, >> +"fsl,qoriq-platform-pll-2.0", pltfrm_pll_init); >> -- >> 2.2.2
Hi Emil, I don't think it is the best to add a function that is very similar to existing one. If you think the function name is not appropriate, rename it. Thanks, Yuantian > -----Original Message----- > From: Emil Medve [mailto:Emilian.Medve@Freescale.com] > Sent: Wednesday, January 21, 2015 4:20 PM > To: Tang Yuantian-B29983; linuxppc-dev@lists.ozlabs.org; Wood Scott-B07421; > mturquette@linaro.org; haokexin@gmail.com > Subject: Re: [PATCH 8/8] clk: ppc-corenet: Add support for the platform PLL > > Hello Yuan-Tian, > > > On 01/20/2015 11:42 PM, Tang Yuantian-B29983 wrote: > > Which platform are you trying to use this on? > > CoreNet chassis v1 and v2 SoC(s) > > > Can this be initialized by core pll function core_pll_init()? > > I just saw most of this function is silimar to the core_pll_init(). > > Yes, the flow is similar, but core_pll_init() makes assumptions that it shouldn't or > are not relevant to the platform PLL > > > Cheers, > > > > Thanks, > > Yuantian > > > >> -----Original Message----- > >> From: Emil Medve [mailto:Emilian.Medve@Freescale.com] > >> Sent: Tuesday, January 20, 2015 6:10 PM > >> To: linuxppc-dev@lists.ozlabs.org; Wood Scott-B07421; > >> mturquette@linaro.org; haokexin@gmail.com; Tang Yuantian-B29983 > >> Cc: Medve Emilian-EMMEDVE1 > >> Subject: [PATCH 8/8] clk: ppc-corenet: Add support for the platform > >> PLL > >> > >> Change-Id: Iac11ed95f274485a86d2c11f32a3dc502bcd020f > >> Signed-off-by: Emil Medve <Emilian.Medve@Freescale.com> > >> --- > >> drivers/clk/clk-ppc-corenet.c | 85 > >> +++++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 85 insertions(+) > >> > >> diff --git a/drivers/clk/clk-ppc-corenet.c > >> b/drivers/clk/clk-ppc-corenet.c index > >> 91816b1..ff425e1 100644 > >> --- a/drivers/clk/clk-ppc-corenet.c > >> +++ b/drivers/clk/clk-ppc-corenet.c > >> @@ -7,6 +7,9 @@ > >> * > >> * clock driver for Freescale PowerPC corenet SoCs. > >> */ > >> + > >> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > >> + > >> #include <linux/clk-provider.h> > >> #include <linux/io.h> > >> #include <linux/kernel.h> > >> @@ -261,9 +264,91 @@ static void __init sysclk_init(struct device_node > *node) > >> of_clk_add_provider(np, of_clk_src_simple_get, clk); } > >> > >> +static void __init pltfrm_pll_init(struct device_node *np) { > >> + void __iomem *base; > >> + uint32_t mult; > >> + const char *parent_name, *clk_name; > >> + int i, _errno; > >> + struct clk_onecell_data *cod; > >> + > >> + base = of_iomap(np, 0); > >> + if (!base) { > >> + pr_err("%s(): %s: of_iomap() failed\n", __func__, np->name); > >> + return; > >> + } > >> + > >> + /* Get the multiple of PLL */ > >> + mult = ioread32be(base); > >> + > >> + iounmap(base); > >> + > >> + /* Check if this PLL is disabled */ > >> + if (mult & PLL_KILL) { > >> + pr_debug("%s(): %s: Disabled\n", __func__, np->name); > >> + return; > >> + } > >> + mult = (mult & GENMASK(6, 1)) >> 1; > >> + > >> + parent_name = of_clk_get_parent_name(np, 0); > >> + if (!parent_name) { > >> + pr_err("%s(): %s: of_clk_get_parent_name() failed\n", > >> + __func__, np->name); > >> + return; > >> + } > >> + > >> + i = of_property_count_strings(np, "clock-output-names"); > >> + if (i < 0) { > >> + pr_err("%s(): %s: of_property_count_strings(clock-output-names) > >> = %d\n", > >> + __func__, np->name, i); > >> + return; > >> + } > >> + > >> + cod = kmalloc(sizeof(*cod) + i * sizeof(struct clk *), GFP_KERNEL); > >> + if (!cod) > >> + return; > >> + cod->clks = (struct clk **)(cod + 1); > >> + cod->clk_num = i; > >> + > >> + for (i = 0; i < cod->clk_num; i++) { > >> + _errno = of_property_read_string_index(np, "clock-output-names", > >> + i, &clk_name); > >> + if (_errno < 0) { > >> + pr_err("%s(): %s: > >> of_property_read_string_index(clock-output-names) = %d\n", > >> + __func__, np->name, _errno); > >> + goto return_clk_unregister; > >> + } > >> + > >> + cod->clks[i] = clk_register_fixed_factor(NULL, clk_name, > >> + parent_name, 0, mult, 1 + i); > >> + if (IS_ERR(cod->clks[i])) { > >> + pr_err("%s(): %s: clk_register_fixed_factor(%s) = %ld\n", > >> + __func__, np->name, > >> + clk_name, PTR_ERR(cod->clks[i])); > >> + goto return_clk_unregister; > >> + } > >> + } > >> + > >> + _errno = of_clk_add_provider(np, of_clk_src_onecell_get, cod); > >> + if (_errno < 0) { > >> + pr_err("%s(): %s: of_clk_add_provider() = %d\n", > >> + __func__, np->name, _errno); > >> + goto return_clk_unregister; > >> + } > >> + > >> + return; > >> + > >> +return_clk_unregister: > >> + while (--i >= 0) > >> + clk_unregister(cod->clks[i]); > >> + kfree(cod); > >> +} > >> + > >> CLK_OF_DECLARE(qoriq_sysclk_1, "fsl,qoriq-sysclk-1.0", sysclk_init); > >> CLK_OF_DECLARE(qoriq_sysclk_2, "fsl,qoriq-sysclk-2.0", sysclk_init); > >> CLK_OF_DECLARE(qoriq_core_pll_1, "fsl,qoriq-core-pll-1.0", > >> core_pll_init); CLK_OF_DECLARE(qoriq_core_pll_2, > >> "fsl,qoriq-core-pll-2.0", core_pll_init); > >> CLK_OF_DECLARE(qoriq_core_mux_1, "fsl,qoriq-core-mux-1.0", > >> core_mux_init); CLK_OF_DECLARE(qoriq_core_mux_2, > >> "fsl,qoriq-core-mux-2.0", core_mux_init); > >> +CLK_OF_DECLARE(qoriq_pltfrm_pll_1, "fsl,qoriq-platform-pll-1.0", > >> +pltfrm_pll_init); CLK_OF_DECLARE(qoriq_pltfrm_pll_2, > >> +"fsl,qoriq-platform-pll-2.0", pltfrm_pll_init); > >> -- > >> 2.2.2
Hello Yuan-Tian, On 01/21/2015 02:35 AM, Tang Yuantian-B29983 wrote: > Hi Emil, > > I don't think it is the best to add a function that is very similar to existing one. > If you think the function name is not appropriate, rename it. It's not a naming matter. As I said, core_pll_init() assumptions and decisions based on the number of clocks in the DT. My hunch is some of these assumptions are not necessary and/or should be explicit based on the node/device compatible. Having a standalone platform PLL initialization function wasn't a unilateral lack of foresight Cheers, > Thanks, > Yuantian > >> -----Original Message----- >> From: Emil Medve [mailto:Emilian.Medve@Freescale.com] >> Sent: Wednesday, January 21, 2015 4:20 PM >> To: Tang Yuantian-B29983; linuxppc-dev@lists.ozlabs.org; Wood Scott-B07421; >> mturquette@linaro.org; haokexin@gmail.com >> Subject: Re: [PATCH 8/8] clk: ppc-corenet: Add support for the platform PLL >> >> Hello Yuan-Tian, >> >> >> On 01/20/2015 11:42 PM, Tang Yuantian-B29983 wrote: >>> Which platform are you trying to use this on? >> >> CoreNet chassis v1 and v2 SoC(s) >> >>> Can this be initialized by core pll function core_pll_init()? >>> I just saw most of this function is silimar to the core_pll_init(). >> >> Yes, the flow is similar, but core_pll_init() makes assumptions that it shouldn't or >> are not relevant to the platform PLL >> >> >> Cheers, >> >> >>> Thanks, >>> Yuantian >>> >>>> -----Original Message----- >>>> From: Emil Medve [mailto:Emilian.Medve@Freescale.com] >>>> Sent: Tuesday, January 20, 2015 6:10 PM >>>> To: linuxppc-dev@lists.ozlabs.org; Wood Scott-B07421; >>>> mturquette@linaro.org; haokexin@gmail.com; Tang Yuantian-B29983 >>>> Cc: Medve Emilian-EMMEDVE1 >>>> Subject: [PATCH 8/8] clk: ppc-corenet: Add support for the platform >>>> PLL >>>> >>>> Change-Id: Iac11ed95f274485a86d2c11f32a3dc502bcd020f >>>> Signed-off-by: Emil Medve <Emilian.Medve@Freescale.com> >>>> --- >>>> drivers/clk/clk-ppc-corenet.c | 85 >>>> +++++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 85 insertions(+) >>>> >>>> diff --git a/drivers/clk/clk-ppc-corenet.c >>>> b/drivers/clk/clk-ppc-corenet.c index >>>> 91816b1..ff425e1 100644 >>>> --- a/drivers/clk/clk-ppc-corenet.c >>>> +++ b/drivers/clk/clk-ppc-corenet.c >>>> @@ -7,6 +7,9 @@ >>>> * >>>> * clock driver for Freescale PowerPC corenet SoCs. >>>> */ >>>> + >>>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt >>>> + >>>> #include <linux/clk-provider.h> >>>> #include <linux/io.h> >>>> #include <linux/kernel.h> >>>> @@ -261,9 +264,91 @@ static void __init sysclk_init(struct device_node >> *node) >>>> of_clk_add_provider(np, of_clk_src_simple_get, clk); } >>>> >>>> +static void __init pltfrm_pll_init(struct device_node *np) { >>>> + void __iomem *base; >>>> + uint32_t mult; >>>> + const char *parent_name, *clk_name; >>>> + int i, _errno; >>>> + struct clk_onecell_data *cod; >>>> + >>>> + base = of_iomap(np, 0); >>>> + if (!base) { >>>> + pr_err("%s(): %s: of_iomap() failed\n", __func__, np->name); >>>> + return; >>>> + } >>>> + >>>> + /* Get the multiple of PLL */ >>>> + mult = ioread32be(base); >>>> + >>>> + iounmap(base); >>>> + >>>> + /* Check if this PLL is disabled */ >>>> + if (mult & PLL_KILL) { >>>> + pr_debug("%s(): %s: Disabled\n", __func__, np->name); >>>> + return; >>>> + } >>>> + mult = (mult & GENMASK(6, 1)) >> 1; >>>> + >>>> + parent_name = of_clk_get_parent_name(np, 0); >>>> + if (!parent_name) { >>>> + pr_err("%s(): %s: of_clk_get_parent_name() failed\n", >>>> + __func__, np->name); >>>> + return; >>>> + } >>>> + >>>> + i = of_property_count_strings(np, "clock-output-names"); >>>> + if (i < 0) { >>>> + pr_err("%s(): %s: of_property_count_strings(clock-output-names) >>>> = %d\n", >>>> + __func__, np->name, i); >>>> + return; >>>> + } >>>> + >>>> + cod = kmalloc(sizeof(*cod) + i * sizeof(struct clk *), GFP_KERNEL); >>>> + if (!cod) >>>> + return; >>>> + cod->clks = (struct clk **)(cod + 1); >>>> + cod->clk_num = i; >>>> + >>>> + for (i = 0; i < cod->clk_num; i++) { >>>> + _errno = of_property_read_string_index(np, "clock-output-names", >>>> + i, &clk_name); >>>> + if (_errno < 0) { >>>> + pr_err("%s(): %s: >>>> of_property_read_string_index(clock-output-names) = %d\n", >>>> + __func__, np->name, _errno); >>>> + goto return_clk_unregister; >>>> + } >>>> + >>>> + cod->clks[i] = clk_register_fixed_factor(NULL, clk_name, >>>> + parent_name, 0, mult, 1 + i); >>>> + if (IS_ERR(cod->clks[i])) { >>>> + pr_err("%s(): %s: clk_register_fixed_factor(%s) = %ld\n", >>>> + __func__, np->name, >>>> + clk_name, PTR_ERR(cod->clks[i])); >>>> + goto return_clk_unregister; >>>> + } >>>> + } >>>> + >>>> + _errno = of_clk_add_provider(np, of_clk_src_onecell_get, cod); >>>> + if (_errno < 0) { >>>> + pr_err("%s(): %s: of_clk_add_provider() = %d\n", >>>> + __func__, np->name, _errno); >>>> + goto return_clk_unregister; >>>> + } >>>> + >>>> + return; >>>> + >>>> +return_clk_unregister: >>>> + while (--i >= 0) >>>> + clk_unregister(cod->clks[i]); >>>> + kfree(cod); >>>> +} >>>> + >>>> CLK_OF_DECLARE(qoriq_sysclk_1, "fsl,qoriq-sysclk-1.0", sysclk_init); >>>> CLK_OF_DECLARE(qoriq_sysclk_2, "fsl,qoriq-sysclk-2.0", sysclk_init); >>>> CLK_OF_DECLARE(qoriq_core_pll_1, "fsl,qoriq-core-pll-1.0", >>>> core_pll_init); CLK_OF_DECLARE(qoriq_core_pll_2, >>>> "fsl,qoriq-core-pll-2.0", core_pll_init); >>>> CLK_OF_DECLARE(qoriq_core_mux_1, "fsl,qoriq-core-mux-1.0", >>>> core_mux_init); CLK_OF_DECLARE(qoriq_core_mux_2, >>>> "fsl,qoriq-core-mux-2.0", core_mux_init); >>>> +CLK_OF_DECLARE(qoriq_pltfrm_pll_1, "fsl,qoriq-platform-pll-1.0", >>>> +pltfrm_pll_init); CLK_OF_DECLARE(qoriq_pltfrm_pll_2, >>>> +"fsl,qoriq-platform-pll-2.0", pltfrm_pll_init); >>>> -- >>>> 2.2.2
Hello Emil, From the RM and your code, I didn't see any difference between the two type of PLL. Could you provide some use cases or feature that prove this is necessary? If there did have some features that current function didn't contain, can we expend the current one to include it? BTW: if you did need this, please update the binding as well, if any: Documentation/devicetree/bindings/clock/qoriq-clock.txt Thanks, Yuantian > -----Original Message----- > From: Emil Medve [mailto:Emilian.Medve@Freescale.com] > Sent: Wednesday, January 21, 2015 5:03 PM > To: Tang Yuantian-B29983; linuxppc-dev@lists.ozlabs.org; Wood Scott-B07421; > mturquette@linaro.org; haokexin@gmail.com > Subject: Re: [PATCH 8/8] clk: ppc-corenet: Add support for the platform PLL > > Hello Yuan-Tian, > > > On 01/21/2015 02:35 AM, Tang Yuantian-B29983 wrote: > > Hi Emil, > > > > I don't think it is the best to add a function that is very similar to existing one. > > If you think the function name is not appropriate, rename it. > > It's not a naming matter. As I said, core_pll_init() assumptions and decisions > based on the number of clocks in the DT. My hunch is some of these assumptions > are not necessary and/or should be explicit based on the node/device compatible. > Having a standalone platform PLL initialization function wasn't a unilateral lack of > foresight > > > Cheers, > > > > Thanks, > > Yuantian > > > >> -----Original Message----- > >> From: Emil Medve [mailto:Emilian.Medve@Freescale.com] > >> Sent: Wednesday, January 21, 2015 4:20 PM > >> To: Tang Yuantian-B29983; linuxppc-dev@lists.ozlabs.org; Wood > >> Scott-B07421; mturquette@linaro.org; haokexin@gmail.com > >> Subject: Re: [PATCH 8/8] clk: ppc-corenet: Add support for the > >> platform PLL > >> > >> Hello Yuan-Tian, > >> > >> > >> On 01/20/2015 11:42 PM, Tang Yuantian-B29983 wrote: > >>> Which platform are you trying to use this on? > >> > >> CoreNet chassis v1 and v2 SoC(s) > >> > >>> Can this be initialized by core pll function core_pll_init()? > >>> I just saw most of this function is silimar to the core_pll_init(). > >> > >> Yes, the flow is similar, but core_pll_init() makes assumptions that > >> it shouldn't or are not relevant to the platform PLL > >> > >> > >> Cheers, > >> > >> > >>> Thanks, > >>> Yuantian > >>> > >>>> -----Original Message----- > >>>> From: Emil Medve [mailto:Emilian.Medve@Freescale.com] > >>>> Sent: Tuesday, January 20, 2015 6:10 PM > >>>> To: linuxppc-dev@lists.ozlabs.org; Wood Scott-B07421; > >>>> mturquette@linaro.org; haokexin@gmail.com; Tang Yuantian-B29983 > >>>> Cc: Medve Emilian-EMMEDVE1 > >>>> Subject: [PATCH 8/8] clk: ppc-corenet: Add support for the platform > >>>> PLL > >>>> > >>>> Change-Id: Iac11ed95f274485a86d2c11f32a3dc502bcd020f > >>>> Signed-off-by: Emil Medve <Emilian.Medve@Freescale.com> > >>>> --- > >>>> drivers/clk/clk-ppc-corenet.c | 85 > >>>> +++++++++++++++++++++++++++++++++++++++++++ > >>>> 1 file changed, 85 insertions(+) > >>>> > >>>> diff --git a/drivers/clk/clk-ppc-corenet.c > >>>> b/drivers/clk/clk-ppc-corenet.c index > >>>> 91816b1..ff425e1 100644 > >>>> --- a/drivers/clk/clk-ppc-corenet.c > >>>> +++ b/drivers/clk/clk-ppc-corenet.c > >>>> @@ -7,6 +7,9 @@ > >>>> * > >>>> * clock driver for Freescale PowerPC corenet SoCs. > >>>> */ > >>>> + > >>>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > >>>> + > >>>> #include <linux/clk-provider.h> > >>>> #include <linux/io.h> > >>>> #include <linux/kernel.h> > >>>> @@ -261,9 +264,91 @@ static void __init sysclk_init(struct > >>>> device_node > >> *node) > >>>> of_clk_add_provider(np, of_clk_src_simple_get, clk); } > >>>> > >>>> +static void __init pltfrm_pll_init(struct device_node *np) { > >>>> + void __iomem *base; > >>>> + uint32_t mult; > >>>> + const char *parent_name, *clk_name; > >>>> + int i, _errno; > >>>> + struct clk_onecell_data *cod; > >>>> + > >>>> + base = of_iomap(np, 0); > >>>> + if (!base) { > >>>> + pr_err("%s(): %s: of_iomap() failed\n", __func__, np->name); > >>>> + return; > >>>> + } > >>>> + > >>>> + /* Get the multiple of PLL */ > >>>> + mult = ioread32be(base); > >>>> + > >>>> + iounmap(base); > >>>> + > >>>> + /* Check if this PLL is disabled */ > >>>> + if (mult & PLL_KILL) { > >>>> + pr_debug("%s(): %s: Disabled\n", __func__, np->name); > >>>> + return; > >>>> + } > >>>> + mult = (mult & GENMASK(6, 1)) >> 1; > >>>> + > >>>> + parent_name = of_clk_get_parent_name(np, 0); > >>>> + if (!parent_name) { > >>>> + pr_err("%s(): %s: of_clk_get_parent_name() failed\n", > >>>> + __func__, np->name); > >>>> + return; > >>>> + } > >>>> + > >>>> + i = of_property_count_strings(np, "clock-output-names"); > >>>> + if (i < 0) { > >>>> + pr_err("%s(): %s: of_property_count_strings(clock-output-names) > >>>> = %d\n", > >>>> + __func__, np->name, i); > >>>> + return; > >>>> + } > >>>> + > >>>> + cod = kmalloc(sizeof(*cod) + i * sizeof(struct clk *), GFP_KERNEL); > >>>> + if (!cod) > >>>> + return; > >>>> + cod->clks = (struct clk **)(cod + 1); > >>>> + cod->clk_num = i; > >>>> + > >>>> + for (i = 0; i < cod->clk_num; i++) { > >>>> + _errno = of_property_read_string_index(np, > "clock-output-names", > >>>> + i, &clk_name); > >>>> + if (_errno < 0) { > >>>> + pr_err("%s(): %s: > >>>> of_property_read_string_index(clock-output-names) = %d\n", > >>>> + __func__, np->name, _errno); > >>>> + goto return_clk_unregister; > >>>> + } > >>>> + > >>>> + cod->clks[i] = clk_register_fixed_factor(NULL, clk_name, > >>>> + parent_name, 0, mult, 1 + i); > >>>> + if (IS_ERR(cod->clks[i])) { > >>>> + pr_err("%s(): %s: clk_register_fixed_factor(%s) = %ld\n", > >>>> + __func__, np->name, > >>>> + clk_name, PTR_ERR(cod->clks[i])); > >>>> + goto return_clk_unregister; > >>>> + } > >>>> + } > >>>> + > >>>> + _errno = of_clk_add_provider(np, of_clk_src_onecell_get, cod); > >>>> + if (_errno < 0) { > >>>> + pr_err("%s(): %s: of_clk_add_provider() = %d\n", > >>>> + __func__, np->name, _errno); > >>>> + goto return_clk_unregister; > >>>> + } > >>>> + > >>>> + return; > >>>> + > >>>> +return_clk_unregister: > >>>> + while (--i >= 0) > >>>> + clk_unregister(cod->clks[i]); > >>>> + kfree(cod); > >>>> +} > >>>> + > >>>> CLK_OF_DECLARE(qoriq_sysclk_1, "fsl,qoriq-sysclk-1.0", > >>>> sysclk_init); CLK_OF_DECLARE(qoriq_sysclk_2, > >>>> "fsl,qoriq-sysclk-2.0", sysclk_init); > >>>> CLK_OF_DECLARE(qoriq_core_pll_1, "fsl,qoriq-core-pll-1.0", > >>>> core_pll_init); CLK_OF_DECLARE(qoriq_core_pll_2, > >>>> "fsl,qoriq-core-pll-2.0", core_pll_init); > >>>> CLK_OF_DECLARE(qoriq_core_mux_1, "fsl,qoriq-core-mux-1.0", > >>>> core_mux_init); CLK_OF_DECLARE(qoriq_core_mux_2, > >>>> "fsl,qoriq-core-mux-2.0", core_mux_init); > >>>> +CLK_OF_DECLARE(qoriq_pltfrm_pll_1, "fsl,qoriq-platform-pll-1.0", > >>>> +pltfrm_pll_init); CLK_OF_DECLARE(qoriq_pltfrm_pll_2, > >>>> +"fsl,qoriq-platform-pll-2.0", pltfrm_pll_init); > >>>> -- > >>>> 2.2.2
Hello Yuan-Tian, On 01/21/2015 03:37 AM, Tang Yuantian-B29983 wrote: > Hello Emil, > >>From the RM and your code, I didn't see any difference between the two type of PLL. > Could you provide some use cases or feature that prove this is necessary? I said the DT makes the core_pll_init() code stand out. Yes, the register layout is identical, but that's not the entire story about the place of these PLL(s) in the SoC clocking hierarchy/tree > If there did have some features that current function didn't > contain, can we expend the current one to include it? Once core_pll_init() gets fixed/cleaned up somebody should look into unifying *_pll_init() > BTW: if you did need this, please update the binding as well, if any: > Documentation/devicetree/bindings/clock/qoriq-clock.txt The platform PLL binding is already there Cheers, > Thanks, > Yuantian > >> -----Original Message----- >> From: Emil Medve [mailto:Emilian.Medve@Freescale.com] >> Sent: Wednesday, January 21, 2015 5:03 PM >> To: Tang Yuantian-B29983; linuxppc-dev@lists.ozlabs.org; Wood Scott-B07421; >> mturquette@linaro.org; haokexin@gmail.com >> Subject: Re: [PATCH 8/8] clk: ppc-corenet: Add support for the platform PLL >> >> Hello Yuan-Tian, >> >> >> On 01/21/2015 02:35 AM, Tang Yuantian-B29983 wrote: >>> Hi Emil, >>> >>> I don't think it is the best to add a function that is very similar to existing one. >>> If you think the function name is not appropriate, rename it. >> >> It's not a naming matter. As I said, core_pll_init() assumptions and decisions >> based on the number of clocks in the DT. My hunch is some of these assumptions >> are not necessary and/or should be explicit based on the node/device compatible. >> Having a standalone platform PLL initialization function wasn't a unilateral lack of >> foresight >> >> >> Cheers, >> >> >>> Thanks, >>> Yuantian >>> >>>> -----Original Message----- >>>> From: Emil Medve [mailto:Emilian.Medve@Freescale.com] >>>> Sent: Wednesday, January 21, 2015 4:20 PM >>>> To: Tang Yuantian-B29983; linuxppc-dev@lists.ozlabs.org; Wood >>>> Scott-B07421; mturquette@linaro.org; haokexin@gmail.com >>>> Subject: Re: [PATCH 8/8] clk: ppc-corenet: Add support for the >>>> platform PLL >>>> >>>> Hello Yuan-Tian, >>>> >>>> >>>> On 01/20/2015 11:42 PM, Tang Yuantian-B29983 wrote: >>>>> Which platform are you trying to use this on? >>>> >>>> CoreNet chassis v1 and v2 SoC(s) >>>> >>>>> Can this be initialized by core pll function core_pll_init()? >>>>> I just saw most of this function is silimar to the core_pll_init(). >>>> >>>> Yes, the flow is similar, but core_pll_init() makes assumptions that >>>> it shouldn't or are not relevant to the platform PLL >>>> >>>> >>>> Cheers, >>>> >>>> >>>>> Thanks, >>>>> Yuantian >>>>> >>>>>> -----Original Message----- >>>>>> From: Emil Medve [mailto:Emilian.Medve@Freescale.com] >>>>>> Sent: Tuesday, January 20, 2015 6:10 PM >>>>>> To: linuxppc-dev@lists.ozlabs.org; Wood Scott-B07421; >>>>>> mturquette@linaro.org; haokexin@gmail.com; Tang Yuantian-B29983 >>>>>> Cc: Medve Emilian-EMMEDVE1 >>>>>> Subject: [PATCH 8/8] clk: ppc-corenet: Add support for the platform >>>>>> PLL >>>>>> >>>>>> Change-Id: Iac11ed95f274485a86d2c11f32a3dc502bcd020f >>>>>> Signed-off-by: Emil Medve <Emilian.Medve@Freescale.com> >>>>>> --- >>>>>> drivers/clk/clk-ppc-corenet.c | 85 >>>>>> +++++++++++++++++++++++++++++++++++++++++++ >>>>>> 1 file changed, 85 insertions(+) >>>>>> >>>>>> diff --git a/drivers/clk/clk-ppc-corenet.c >>>>>> b/drivers/clk/clk-ppc-corenet.c index >>>>>> 91816b1..ff425e1 100644 >>>>>> --- a/drivers/clk/clk-ppc-corenet.c >>>>>> +++ b/drivers/clk/clk-ppc-corenet.c >>>>>> @@ -7,6 +7,9 @@ >>>>>> * >>>>>> * clock driver for Freescale PowerPC corenet SoCs. >>>>>> */ >>>>>> + >>>>>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt >>>>>> + >>>>>> #include <linux/clk-provider.h> >>>>>> #include <linux/io.h> >>>>>> #include <linux/kernel.h> >>>>>> @@ -261,9 +264,91 @@ static void __init sysclk_init(struct >>>>>> device_node >>>> *node) >>>>>> of_clk_add_provider(np, of_clk_src_simple_get, clk); } >>>>>> >>>>>> +static void __init pltfrm_pll_init(struct device_node *np) { >>>>>> + void __iomem *base; >>>>>> + uint32_t mult; >>>>>> + const char *parent_name, *clk_name; >>>>>> + int i, _errno; >>>>>> + struct clk_onecell_data *cod; >>>>>> + >>>>>> + base = of_iomap(np, 0); >>>>>> + if (!base) { >>>>>> + pr_err("%s(): %s: of_iomap() failed\n", __func__, np->name); >>>>>> + return; >>>>>> + } >>>>>> + >>>>>> + /* Get the multiple of PLL */ >>>>>> + mult = ioread32be(base); >>>>>> + >>>>>> + iounmap(base); >>>>>> + >>>>>> + /* Check if this PLL is disabled */ >>>>>> + if (mult & PLL_KILL) { >>>>>> + pr_debug("%s(): %s: Disabled\n", __func__, np->name); >>>>>> + return; >>>>>> + } >>>>>> + mult = (mult & GENMASK(6, 1)) >> 1; >>>>>> + >>>>>> + parent_name = of_clk_get_parent_name(np, 0); >>>>>> + if (!parent_name) { >>>>>> + pr_err("%s(): %s: of_clk_get_parent_name() failed\n", >>>>>> + __func__, np->name); >>>>>> + return; >>>>>> + } >>>>>> + >>>>>> + i = of_property_count_strings(np, "clock-output-names"); >>>>>> + if (i < 0) { >>>>>> + pr_err("%s(): %s: of_property_count_strings(clock-output-names) >>>>>> = %d\n", >>>>>> + __func__, np->name, i); >>>>>> + return; >>>>>> + } >>>>>> + >>>>>> + cod = kmalloc(sizeof(*cod) + i * sizeof(struct clk *), GFP_KERNEL); >>>>>> + if (!cod) >>>>>> + return; >>>>>> + cod->clks = (struct clk **)(cod + 1); >>>>>> + cod->clk_num = i; >>>>>> + >>>>>> + for (i = 0; i < cod->clk_num; i++) { >>>>>> + _errno = of_property_read_string_index(np, >> "clock-output-names", >>>>>> + i, &clk_name); >>>>>> + if (_errno < 0) { >>>>>> + pr_err("%s(): %s: >>>>>> of_property_read_string_index(clock-output-names) = %d\n", >>>>>> + __func__, np->name, _errno); >>>>>> + goto return_clk_unregister; >>>>>> + } >>>>>> + >>>>>> + cod->clks[i] = clk_register_fixed_factor(NULL, clk_name, >>>>>> + parent_name, 0, mult, 1 + i); >>>>>> + if (IS_ERR(cod->clks[i])) { >>>>>> + pr_err("%s(): %s: clk_register_fixed_factor(%s) = %ld\n", >>>>>> + __func__, np->name, >>>>>> + clk_name, PTR_ERR(cod->clks[i])); >>>>>> + goto return_clk_unregister; >>>>>> + } >>>>>> + } >>>>>> + >>>>>> + _errno = of_clk_add_provider(np, of_clk_src_onecell_get, cod); >>>>>> + if (_errno < 0) { >>>>>> + pr_err("%s(): %s: of_clk_add_provider() = %d\n", >>>>>> + __func__, np->name, _errno); >>>>>> + goto return_clk_unregister; >>>>>> + } >>>>>> + >>>>>> + return; >>>>>> + >>>>>> +return_clk_unregister: >>>>>> + while (--i >= 0) >>>>>> + clk_unregister(cod->clks[i]); >>>>>> + kfree(cod); >>>>>> +} >>>>>> + >>>>>> CLK_OF_DECLARE(qoriq_sysclk_1, "fsl,qoriq-sysclk-1.0", >>>>>> sysclk_init); CLK_OF_DECLARE(qoriq_sysclk_2, >>>>>> "fsl,qoriq-sysclk-2.0", sysclk_init); >>>>>> CLK_OF_DECLARE(qoriq_core_pll_1, "fsl,qoriq-core-pll-1.0", >>>>>> core_pll_init); CLK_OF_DECLARE(qoriq_core_pll_2, >>>>>> "fsl,qoriq-core-pll-2.0", core_pll_init); >>>>>> CLK_OF_DECLARE(qoriq_core_mux_1, "fsl,qoriq-core-mux-1.0", >>>>>> core_mux_init); CLK_OF_DECLARE(qoriq_core_mux_2, >>>>>> "fsl,qoriq-core-mux-2.0", core_mux_init); >>>>>> +CLK_OF_DECLARE(qoriq_pltfrm_pll_1, "fsl,qoriq-platform-pll-1.0", >>>>>> +pltfrm_pll_init); CLK_OF_DECLARE(qoriq_pltfrm_pll_2, >>>>>> +"fsl,qoriq-platform-pll-2.0", pltfrm_pll_init); >>>>>> -- >>>>>> 2.2.2
diff --git a/drivers/clk/clk-ppc-corenet.c b/drivers/clk/clk-ppc-corenet.c index 91816b1..ff425e1 100644 --- a/drivers/clk/clk-ppc-corenet.c +++ b/drivers/clk/clk-ppc-corenet.c @@ -7,6 +7,9 @@ * * clock driver for Freescale PowerPC corenet SoCs. */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + #include <linux/clk-provider.h> #include <linux/io.h> #include <linux/kernel.h> @@ -261,9 +264,91 @@ static void __init sysclk_init(struct device_node *node) of_clk_add_provider(np, of_clk_src_simple_get, clk); } +static void __init pltfrm_pll_init(struct device_node *np) +{ + void __iomem *base; + uint32_t mult; + const char *parent_name, *clk_name; + int i, _errno; + struct clk_onecell_data *cod; + + base = of_iomap(np, 0); + if (!base) { + pr_err("%s(): %s: of_iomap() failed\n", __func__, np->name); + return; + } + + /* Get the multiple of PLL */ + mult = ioread32be(base); + + iounmap(base); + + /* Check if this PLL is disabled */ + if (mult & PLL_KILL) { + pr_debug("%s(): %s: Disabled\n", __func__, np->name); + return; + } + mult = (mult & GENMASK(6, 1)) >> 1; + + parent_name = of_clk_get_parent_name(np, 0); + if (!parent_name) { + pr_err("%s(): %s: of_clk_get_parent_name() failed\n", + __func__, np->name); + return; + } + + i = of_property_count_strings(np, "clock-output-names"); + if (i < 0) { + pr_err("%s(): %s: of_property_count_strings(clock-output-names) = %d\n", + __func__, np->name, i); + return; + } + + cod = kmalloc(sizeof(*cod) + i * sizeof(struct clk *), GFP_KERNEL); + if (!cod) + return; + cod->clks = (struct clk **)(cod + 1); + cod->clk_num = i; + + for (i = 0; i < cod->clk_num; i++) { + _errno = of_property_read_string_index(np, "clock-output-names", + i, &clk_name); + if (_errno < 0) { + pr_err("%s(): %s: of_property_read_string_index(clock-output-names) = %d\n", + __func__, np->name, _errno); + goto return_clk_unregister; + } + + cod->clks[i] = clk_register_fixed_factor(NULL, clk_name, + parent_name, 0, mult, 1 + i); + if (IS_ERR(cod->clks[i])) { + pr_err("%s(): %s: clk_register_fixed_factor(%s) = %ld\n", + __func__, np->name, + clk_name, PTR_ERR(cod->clks[i])); + goto return_clk_unregister; + } + } + + _errno = of_clk_add_provider(np, of_clk_src_onecell_get, cod); + if (_errno < 0) { + pr_err("%s(): %s: of_clk_add_provider() = %d\n", + __func__, np->name, _errno); + goto return_clk_unregister; + } + + return; + +return_clk_unregister: + while (--i >= 0) + clk_unregister(cod->clks[i]); + kfree(cod); +} + CLK_OF_DECLARE(qoriq_sysclk_1, "fsl,qoriq-sysclk-1.0", sysclk_init); CLK_OF_DECLARE(qoriq_sysclk_2, "fsl,qoriq-sysclk-2.0", sysclk_init); CLK_OF_DECLARE(qoriq_core_pll_1, "fsl,qoriq-core-pll-1.0", core_pll_init); CLK_OF_DECLARE(qoriq_core_pll_2, "fsl,qoriq-core-pll-2.0", core_pll_init); CLK_OF_DECLARE(qoriq_core_mux_1, "fsl,qoriq-core-mux-1.0", core_mux_init); CLK_OF_DECLARE(qoriq_core_mux_2, "fsl,qoriq-core-mux-2.0", core_mux_init); +CLK_OF_DECLARE(qoriq_pltfrm_pll_1, "fsl,qoriq-platform-pll-1.0", pltfrm_pll_init); +CLK_OF_DECLARE(qoriq_pltfrm_pll_2, "fsl,qoriq-platform-pll-2.0", pltfrm_pll_init);
Change-Id: Iac11ed95f274485a86d2c11f32a3dc502bcd020f Signed-off-by: Emil Medve <Emilian.Medve@Freescale.com> --- drivers/clk/clk-ppc-corenet.c | 85 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+)