[PATCH 8/8] clk: ppc-corenet: Add support for the platform PLL

[Yuantian Tang]

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 at Freescale.com]
> Sent: Wednesday, January 21, 2015 5:03 PM
> To: Tang Yuantian-B29983; linuxppc-dev at lists.ozlabs.org; Wood Scott-B07421;
> mturquette at linaro.org; haokexin at 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 at Freescale.com]
> >> Sent: Wednesday, January 21, 2015 4:20 PM
> >> To: Tang Yuantian-B29983; linuxppc-dev at lists.ozlabs.org; Wood
> >> Scott-B07421; mturquette at linaro.org; haokexin at 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 at Freescale.com]
> >>>> Sent: Tuesday, January 20, 2015 6:10 PM
> >>>> To: linuxppc-dev at lists.ozlabs.org; Wood Scott-B07421;
> >>>> mturquette at linaro.org; haokexin at 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 at 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