[4/9] clk: sunxi: PLL2 support for sun4i, sun5i and sun7i

This patch adds support for PLL2 and derivates on sun4i, sun5i and
sun7i SoCs. As this PLL is only used for audio and requires good
accuracy, we only support two known good rates.

Signed-off-by: Emilio López <emilio@elopez.com.ar>
---

Changes from RFC:
 * Add support for A10 rev. A
 * Document compatibles
 * Use fixed factors

 Documentation/devicetree/bindings/clock/sunxi.txt |   2 +
 drivers/clk/sunxi/Makefile                        |   1 +
 drivers/clk/sunxi/clk-a10-pll2.c                  | 243 ++++++++++++++++++++++
 3 files changed, 246 insertions(+)
 create mode 100644 drivers/clk/sunxi/clk-a10-pll2.c

On Thu, Jul 31, 2014 at 06:28:07PM -0300, Emilio López wrote:
> This patch adds support for PLL2 and derivates on sun4i, sun5i and
> sun7i SoCs. As this PLL is only used for audio and requires good
> accuracy, we only support two known good rates.
> 
> Signed-off-by: Emilio López <emilio@elopez.com.ar>
> ---
> 
> Changes from RFC:
>  * Add support for A10 rev. A
>  * Document compatibles
>  * Use fixed factors
> 
>  Documentation/devicetree/bindings/clock/sunxi.txt |   2 +
>  drivers/clk/sunxi/Makefile                        |   1 +
>  drivers/clk/sunxi/clk-a10-pll2.c                  | 243 ++++++++++++++++++++++
>  3 files changed, 246 insertions(+)
>  create mode 100644 drivers/clk/sunxi/clk-a10-pll2.c
> 
> diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt
> index d3a5c3c..41ada31 100644
> --- a/Documentation/devicetree/bindings/clock/sunxi.txt
> +++ b/Documentation/devicetree/bindings/clock/sunxi.txt
> @@ -10,6 +10,8 @@ Required properties:
>  	"allwinner,sun4i-a10-pll1-clk" - for the main PLL clock and PLL4
>  	"allwinner,sun6i-a31-pll1-clk" - for the main PLL clock on A31
>  	"allwinner,sun8i-a23-pll1-clk" - for the main PLL clock on A23
> +	"allwinner,sun4i-a10-a-pll2-clk" - for the PLL2 clock on A10 rev. A
> +	"allwinner,sun4i-a10-b-pll2-clk" - for the PLL2 clock on A10 rev. B
>  	"allwinner,sun4i-a10-pll5-clk" - for the PLL5 clock
>  	"allwinner,sun4i-a10-pll6-clk" - for the PLL6 clock
>  	"allwinner,sun6i-a31-pll6-clk" - for the PLL6 clock on A31
> diff --git a/drivers/clk/sunxi/Makefile b/drivers/clk/sunxi/Makefile
> index 6850cba..dcd5709 100644
> --- a/drivers/clk/sunxi/Makefile
> +++ b/drivers/clk/sunxi/Makefile
> @@ -4,6 +4,7 @@
>  
>  obj-y += clk-sunxi.o clk-factors.o
>  obj-y += clk-a10-hosc.o
> +obj-y += clk-a10-pll2.o
>  obj-y += clk-a20-gmac.o
>  
>  obj-$(CONFIG_MFD_SUN6I_PRCM) += \
> diff --git a/drivers/clk/sunxi/clk-a10-pll2.c b/drivers/clk/sunxi/clk-a10-pll2.c
> new file mode 100644
> index 0000000..bcf7d0b
> --- /dev/null
> +++ b/drivers/clk/sunxi/clk-a10-pll2.c
> @@ -0,0 +1,243 @@
> +/*
> + * Copyright 2014 Emilio López
> + *
> + * Emilio López <emilio@elopez.com.ar>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/slab.h>
> +
> +#define SUN4I_PLL2_ENABLE		31
> +#define SUN4I_PLL2_A_VCOBIAS		0
> +#define SUN4I_PLL2_A_VCOBIAS_MASK	0x1F
> +#define SUN4I_PLL2_A_N			7
> +#define SUN4I_PLL2_A_N_MASK		0x7F
> +#define SUN4I_PLL2_B_POST_DIV		26
> +#define SUN4I_PLL2_B_POST_DIV_MASK	0xF
> +#define SUN4I_PLL2_B_N			8
> +#define SUN4I_PLL2_B_N_MASK		0x7F
> +#define SUN4I_PLL2_B_PRE_DIV		0
> +#define SUN4I_PLL2_B_PRE_DIV_MASK	0x1F
> +
> +#define SUN4I_PLL2_OUTPUTS		4
> +
> +struct sun4i_pll2_clk {
> +	struct clk_hw hw;
> +	void __iomem *reg;
> +};
> +
> +static inline struct sun4i_pll2_clk *to_sun4i_pll2_clk(struct clk_hw *hw)
> +{
> +	return container_of(hw, struct sun4i_pll2_clk, hw);
> +}
> +
> +static unsigned long sun4i_pll2_recalc_rate_a(struct clk_hw *hw,
> +					      unsigned long parent_rate)
> +{
> +	struct sun4i_pll2_clk *clk = to_sun4i_pll2_clk(hw);
> +	int vcobias, n;
> +	u32 val;
> +
> +	val = readl(clk->reg);
> +	vcobias = (val >> SUN4I_PLL2_A_VCOBIAS) & SUN4I_PLL2_A_VCOBIAS_MASK;
> +	n = (val >> SUN4I_PLL2_A_N) & SUN4I_PLL2_A_N_MASK;
> +
> +	if (vcobias == 10 && n == 94)
> +		return 22579200;
> +	else if (vcobias == 9 && n == 83)
> +		return 24576000;
> +
> +	/*
> +	 * Unfortunately we don't really have much documentation on how
> +	 * these factors relate mathematically
> +	 */
> +	return 0;
> +}
> +
> +static unsigned long sun4i_pll2_recalc_rate_b(struct clk_hw *hw,
> +					      unsigned long parent_rate)
> +{
> +	struct sun4i_pll2_clk *clk = to_sun4i_pll2_clk(hw);
> +	int n, prediv, postdiv;
> +	u32 val;
> +
> +	val = readl(clk->reg);
> +	n = (val >> SUN4I_PLL2_B_N) & SUN4I_PLL2_B_N_MASK;
> +	prediv = (val >> SUN4I_PLL2_B_PRE_DIV) & SUN4I_PLL2_B_PRE_DIV_MASK;
> +	postdiv = (val >> SUN4I_PLL2_B_POST_DIV) & SUN4I_PLL2_B_POST_DIV_MASK;
> +
> +	/* 0 is a special case and means 1 */
> +	if (n == 0)
> +		n = 1;
> +	if (prediv == 0)
> +		prediv = 1;
> +	if (postdiv == 0)
> +		postdiv = 1;
> +
> +	return ((parent_rate * n) / prediv) / postdiv;
> +}
> +
> +static long sun4i_pll2_round_rate(struct clk_hw *hw, unsigned long rate,
> +				  unsigned long *parent_rate)
> +{
> +	/*
> +	 * There is only two interesting rates for the audio PLL, the
> +	 * rest isn't really usable due to accuracy concerns. Therefore,
> +	 * we specifically round to those rates here
> +	 */
> +	if (rate < 22579200)
> +		return -EINVAL;
> +
> +	if (rate >= 22579200 && rate < 24576000)
> +		return 22579200;
> +
> +	return 24576000;
> +}
> +
> +static int sun4i_pll2_set_rate_a(struct clk_hw *hw, unsigned long rate,
> +				 unsigned long parent_rate)
> +{
> +	struct sun4i_pll2_clk *clk = to_sun4i_pll2_clk(hw);
> +	u32 val = readl(clk->reg);
> +
> +	val &= ~(SUN4I_PLL2_A_VCOBIAS_MASK << SUN4I_PLL2_A_VCOBIAS);
> +	val &= ~(SUN4I_PLL2_A_N_MASK << SUN4I_PLL2_A_N);
> +
> +	if (rate == 22579200)
> +		val |= (10 << SUN4I_PLL2_A_VCOBIAS) | (94 << SUN4I_PLL2_A_N);
> +	else if (rate == 24576000)
> +		val |= (9 << SUN4I_PLL2_A_VCOBIAS) | (83 << SUN4I_PLL2_A_N);
> +	else
> +		return -EINVAL;
> +
> +	writel(val, clk->reg);
> +
> +	return 0;
> +}
> +
> +static int sun4i_pll2_set_rate_b(struct clk_hw *hw, unsigned long rate,
> +				 unsigned long parent_rate)
> +{
> +	struct sun4i_pll2_clk *clk = to_sun4i_pll2_clk(hw);
> +	u32 val = readl(clk->reg);
> +
> +	val &= ~(SUN4I_PLL2_B_N_MASK << SUN4I_PLL2_B_N);
> +	val &= ~(SUN4I_PLL2_B_PRE_DIV_MASK << SUN4I_PLL2_B_PRE_DIV);
> +	val &= ~(SUN4I_PLL2_B_POST_DIV_MASK << SUN4I_PLL2_B_POST_DIV);
> +
> +	val |= (21 << SUN4I_PLL2_B_PRE_DIV) | (4 << SUN4I_PLL2_B_POST_DIV);
> +
> +	if (rate == 22579200)
> +		val |= (79 << SUN4I_PLL2_B_N);
> +	else if (rate == 24576000)
> +		val |= (86 << SUN4I_PLL2_B_N);
> +	else
> +		return -EINVAL;
> +
> +	writel(val, clk->reg);
> +
> +	return 0;
> +}
> +
> +static const struct clk_ops sun4i_pll2_ops_a = {
> +	.recalc_rate = sun4i_pll2_recalc_rate_a,
> +	.round_rate = sun4i_pll2_round_rate,
> +	.set_rate = sun4i_pll2_set_rate_a,
> +};
> +
> +
> +static const struct clk_ops sun4i_pll2_ops_b = {
> +	.recalc_rate = sun4i_pll2_recalc_rate_b,
> +	.round_rate = sun4i_pll2_round_rate,
> +	.set_rate = sun4i_pll2_set_rate_b,
> +};
> +
> +static const struct of_device_id pll2_matches[] __initconst = {
> +	{ .compatible = "allwinner,sun4i-a10-a-pll2-clk", .data = &sun4i_pll2_ops_a },
> +	{ .compatible = "allwinner,sun4i-a10-b-pll2-clk", .data = &sun4i_pll2_ops_b },
> +	{ /* sentinel */ },
> +};
> +
> +static void __init sun4i_pll2_setup(struct device_node *np)
> +{
> +	const char *clk_name = np->name, *parent;
> +	const struct of_device_id *match;
> +	struct clk_onecell_data *clk_data;
> +	const struct clk_ops *pll2_ops;
> +	struct sun4i_pll2_clk *pll2;
> +	struct clk_gate *gate;
> +	struct clk **clks;
> +	void __iomem *reg;
> +
> +	/* Choose the correct ops for pll2 */
> +	match = of_match_node(pll2_matches, np);
> +	pll2_ops = match->data;
> +
> +	pll2 = kzalloc(sizeof(*pll2), GFP_KERNEL);
> +	gate = kzalloc(sizeof(*gate), GFP_KERNEL);
> +	clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
> +	clks = kcalloc(SUN4I_PLL2_OUTPUTS, sizeof(*clks), GFP_KERNEL);
> +	if (!pll2 || !gate || !clk_data || !clks)
> +		goto free_mem;
> +
> +	reg = of_iomap(np, 0);
> +	parent = of_clk_get_parent_name(np, 0);
> +	of_property_read_string_index(np, "clock-output-names", 0, &clk_name);
> +
> +	pll2->reg = reg;
> +	gate->reg = reg;
> +	gate->bit_idx = SUN4I_PLL2_ENABLE;
> +
> +	/* PLL2, also known as PLL2x1 */
> +	of_property_read_string_index(np, "clock-output-names", 0, &clk_name);
> +	clks[0] = clk_register_composite(NULL, clk_name, &parent, 1, NULL, NULL,
> +					 &pll2->hw, pll2_ops,
> +					 &gate->hw, &clk_gate_ops, 0);
> +	WARN_ON(IS_ERR(clks[0]));
> +	clk_set_rate(clks[0], 22579200);
> +	parent = clk_name;
> +
> +	/* PLL2x2, 1/4 the rate of PLL2x8 */
> +	of_property_read_string_index(np, "clock-output-names", 1, &clk_name);
> +	clks[1] = clk_register_fixed_factor(NULL, clk_name, parent,
> +					    CLK_SET_RATE_PARENT, 2, 1);
> +	WARN_ON(IS_ERR(clks[1]));
> +
> +	/* PLL2x4, 1/2 the rate of PLL2x8 */
> +	of_property_read_string_index(np, "clock-output-names", 2, &clk_name);
> +	clks[2] = clk_register_fixed_factor(NULL, clk_name, parent,
> +					    CLK_SET_RATE_PARENT, 4, 1);
> +	WARN_ON(IS_ERR(clks[2]));
> +
> +	/* PLL2x8, double of PLL2 without the post divisor */
> +	of_property_read_string_index(np, "clock-output-names", 3, &clk_name);
> +	clks[3] = clk_register_fixed_factor(NULL, clk_name, parent,
> +					    CLK_SET_RATE_PARENT, 2 * 4, 1);

Why have you declared them here, instead of using fixed factors in the
DT directly, like we have done in the past?

Maxime

On Sun, Aug 3, 2014 at 8:44 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Thu, Jul 31, 2014 at 06:28:07PM -0300, Emilio López wrote:
>> This patch adds support for PLL2 and derivates on sun4i, sun5i and
>> sun7i SoCs. As this PLL is only used for audio and requires good
>> accuracy, we only support two known good rates.
>>
>> Signed-off-by: Emilio López <emilio@elopez.com.ar>
>> ---
>>
>> Changes from RFC:
>>  * Add support for A10 rev. A
>>  * Document compatibles
>>  * Use fixed factors
>>
>>  Documentation/devicetree/bindings/clock/sunxi.txt |   2 +
>>  drivers/clk/sunxi/Makefile                        |   1 +
>>  drivers/clk/sunxi/clk-a10-pll2.c                  | 243 ++++++++++++++++++++++
>>  3 files changed, 246 insertions(+)
>>  create mode 100644 drivers/clk/sunxi/clk-a10-pll2.c
>>
>> diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt
>> index d3a5c3c..41ada31 100644
>> --- a/Documentation/devicetree/bindings/clock/sunxi.txt
>> +++ b/Documentation/devicetree/bindings/clock/sunxi.txt
>> @@ -10,6 +10,8 @@ Required properties:
>>       "allwinner,sun4i-a10-pll1-clk" - for the main PLL clock and PLL4
>>       "allwinner,sun6i-a31-pll1-clk" - for the main PLL clock on A31
>>       "allwinner,sun8i-a23-pll1-clk" - for the main PLL clock on A23
>> +     "allwinner,sun4i-a10-a-pll2-clk" - for the PLL2 clock on A10 rev. A
>> +     "allwinner,sun4i-a10-b-pll2-clk" - for the PLL2 clock on A10 rev. B
>>       "allwinner,sun4i-a10-pll5-clk" - for the PLL5 clock
>>       "allwinner,sun4i-a10-pll6-clk" - for the PLL6 clock
>>       "allwinner,sun6i-a31-pll6-clk" - for the PLL6 clock on A31
>> diff --git a/drivers/clk/sunxi/Makefile b/drivers/clk/sunxi/Makefile
>> index 6850cba..dcd5709 100644
>> --- a/drivers/clk/sunxi/Makefile
>> +++ b/drivers/clk/sunxi/Makefile
>> @@ -4,6 +4,7 @@
>>
>>  obj-y += clk-sunxi.o clk-factors.o
>>  obj-y += clk-a10-hosc.o
>> +obj-y += clk-a10-pll2.o
>>  obj-y += clk-a20-gmac.o
>>
>>  obj-$(CONFIG_MFD_SUN6I_PRCM) += \
>> diff --git a/drivers/clk/sunxi/clk-a10-pll2.c b/drivers/clk/sunxi/clk-a10-pll2.c
>> new file mode 100644
>> index 0000000..bcf7d0b
>> --- /dev/null
>> +++ b/drivers/clk/sunxi/clk-a10-pll2.c
>> @@ -0,0 +1,243 @@
>> +/*
>> + * Copyright 2014 Emilio López
>> + *
>> + * Emilio López <emilio@elopez.com.ar>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/clk-provider.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/slab.h>
>> +
>> +#define SUN4I_PLL2_ENABLE            31
>> +#define SUN4I_PLL2_A_VCOBIAS         0
>> +#define SUN4I_PLL2_A_VCOBIAS_MASK    0x1F
>> +#define SUN4I_PLL2_A_N                       7
>> +#define SUN4I_PLL2_A_N_MASK          0x7F
>> +#define SUN4I_PLL2_B_POST_DIV                26
>> +#define SUN4I_PLL2_B_POST_DIV_MASK   0xF
>> +#define SUN4I_PLL2_B_N                       8
>> +#define SUN4I_PLL2_B_N_MASK          0x7F
>> +#define SUN4I_PLL2_B_PRE_DIV         0
>> +#define SUN4I_PLL2_B_PRE_DIV_MASK    0x1F
>> +
>> +#define SUN4I_PLL2_OUTPUTS           4
>> +
>> +struct sun4i_pll2_clk {
>> +     struct clk_hw hw;
>> +     void __iomem *reg;
>> +};
>> +
>> +static inline struct sun4i_pll2_clk *to_sun4i_pll2_clk(struct clk_hw *hw)
>> +{
>> +     return container_of(hw, struct sun4i_pll2_clk, hw);
>> +}
>> +
>> +static unsigned long sun4i_pll2_recalc_rate_a(struct clk_hw *hw,
>> +                                           unsigned long parent_rate)
>> +{
>> +     struct sun4i_pll2_clk *clk = to_sun4i_pll2_clk(hw);
>> +     int vcobias, n;
>> +     u32 val;
>> +
>> +     val = readl(clk->reg);
>> +     vcobias = (val >> SUN4I_PLL2_A_VCOBIAS) & SUN4I_PLL2_A_VCOBIAS_MASK;
>> +     n = (val >> SUN4I_PLL2_A_N) & SUN4I_PLL2_A_N_MASK;
>> +
>> +     if (vcobias == 10 && n == 94)
>> +             return 22579200;
>> +     else if (vcobias == 9 && n == 83)
>> +             return 24576000;
>> +
>> +     /*
>> +      * Unfortunately we don't really have much documentation on how
>> +      * these factors relate mathematically
>> +      */
>> +     return 0;
>> +}
>> +
>> +static unsigned long sun4i_pll2_recalc_rate_b(struct clk_hw *hw,
>> +                                           unsigned long parent_rate)
>> +{
>> +     struct sun4i_pll2_clk *clk = to_sun4i_pll2_clk(hw);
>> +     int n, prediv, postdiv;
>> +     u32 val;
>> +
>> +     val = readl(clk->reg);
>> +     n = (val >> SUN4I_PLL2_B_N) & SUN4I_PLL2_B_N_MASK;
>> +     prediv = (val >> SUN4I_PLL2_B_PRE_DIV) & SUN4I_PLL2_B_PRE_DIV_MASK;
>> +     postdiv = (val >> SUN4I_PLL2_B_POST_DIV) & SUN4I_PLL2_B_POST_DIV_MASK;
>> +
>> +     /* 0 is a special case and means 1 */
>> +     if (n == 0)
>> +             n = 1;
>> +     if (prediv == 0)
>> +             prediv = 1;
>> +     if (postdiv == 0)
>> +             postdiv = 1;
>> +
>> +     return ((parent_rate * n) / prediv) / postdiv;
>> +}
>> +
>> +static long sun4i_pll2_round_rate(struct clk_hw *hw, unsigned long rate,
>> +                               unsigned long *parent_rate)
>> +{
>> +     /*
>> +      * There is only two interesting rates for the audio PLL, the
>> +      * rest isn't really usable due to accuracy concerns. Therefore,
>> +      * we specifically round to those rates here
>> +      */
>> +     if (rate < 22579200)
>> +             return -EINVAL;
>> +
>> +     if (rate >= 22579200 && rate < 24576000)
>> +             return 22579200;
>> +
>> +     return 24576000;
>> +}
>> +
>> +static int sun4i_pll2_set_rate_a(struct clk_hw *hw, unsigned long rate,
>> +                              unsigned long parent_rate)
>> +{
>> +     struct sun4i_pll2_clk *clk = to_sun4i_pll2_clk(hw);
>> +     u32 val = readl(clk->reg);
>> +
>> +     val &= ~(SUN4I_PLL2_A_VCOBIAS_MASK << SUN4I_PLL2_A_VCOBIAS);
>> +     val &= ~(SUN4I_PLL2_A_N_MASK << SUN4I_PLL2_A_N);
>> +
>> +     if (rate == 22579200)
>> +             val |= (10 << SUN4I_PLL2_A_VCOBIAS) | (94 << SUN4I_PLL2_A_N);
>> +     else if (rate == 24576000)
>> +             val |= (9 << SUN4I_PLL2_A_VCOBIAS) | (83 << SUN4I_PLL2_A_N);
>> +     else
>> +             return -EINVAL;
>> +
>> +     writel(val, clk->reg);
>> +
>> +     return 0;
>> +}
>> +
>> +static int sun4i_pll2_set_rate_b(struct clk_hw *hw, unsigned long rate,
>> +                              unsigned long parent_rate)
>> +{
>> +     struct sun4i_pll2_clk *clk = to_sun4i_pll2_clk(hw);
>> +     u32 val = readl(clk->reg);
>> +
>> +     val &= ~(SUN4I_PLL2_B_N_MASK << SUN4I_PLL2_B_N);
>> +     val &= ~(SUN4I_PLL2_B_PRE_DIV_MASK << SUN4I_PLL2_B_PRE_DIV);
>> +     val &= ~(SUN4I_PLL2_B_POST_DIV_MASK << SUN4I_PLL2_B_POST_DIV);
>> +
>> +     val |= (21 << SUN4I_PLL2_B_PRE_DIV) | (4 << SUN4I_PLL2_B_POST_DIV);
>> +
>> +     if (rate == 22579200)
>> +             val |= (79 << SUN4I_PLL2_B_N);
>> +     else if (rate == 24576000)
>> +             val |= (86 << SUN4I_PLL2_B_N);
>> +     else
>> +             return -EINVAL;
>> +
>> +     writel(val, clk->reg);
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct clk_ops sun4i_pll2_ops_a = {
>> +     .recalc_rate = sun4i_pll2_recalc_rate_a,
>> +     .round_rate = sun4i_pll2_round_rate,
>> +     .set_rate = sun4i_pll2_set_rate_a,
>> +};
>> +
>> +
>> +static const struct clk_ops sun4i_pll2_ops_b = {
>> +     .recalc_rate = sun4i_pll2_recalc_rate_b,
>> +     .round_rate = sun4i_pll2_round_rate,
>> +     .set_rate = sun4i_pll2_set_rate_b,
>> +};
>> +
>> +static const struct of_device_id pll2_matches[] __initconst = {
>> +     { .compatible = "allwinner,sun4i-a10-a-pll2-clk", .data = &sun4i_pll2_ops_a },
>> +     { .compatible = "allwinner,sun4i-a10-b-pll2-clk", .data = &sun4i_pll2_ops_b },
>> +     { /* sentinel */ },
>> +};
>> +
>> +static void __init sun4i_pll2_setup(struct device_node *np)
>> +{
>> +     const char *clk_name = np->name, *parent;
>> +     const struct of_device_id *match;
>> +     struct clk_onecell_data *clk_data;
>> +     const struct clk_ops *pll2_ops;
>> +     struct sun4i_pll2_clk *pll2;
>> +     struct clk_gate *gate;
>> +     struct clk **clks;
>> +     void __iomem *reg;
>> +
>> +     /* Choose the correct ops for pll2 */
>> +     match = of_match_node(pll2_matches, np);
>> +     pll2_ops = match->data;
>> +
>> +     pll2 = kzalloc(sizeof(*pll2), GFP_KERNEL);
>> +     gate = kzalloc(sizeof(*gate), GFP_KERNEL);
>> +     clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
>> +     clks = kcalloc(SUN4I_PLL2_OUTPUTS, sizeof(*clks), GFP_KERNEL);
>> +     if (!pll2 || !gate || !clk_data || !clks)
>> +             goto free_mem;
>> +
>> +     reg = of_iomap(np, 0);
>> +     parent = of_clk_get_parent_name(np, 0);
>> +     of_property_read_string_index(np, "clock-output-names", 0, &clk_name);
>> +
>> +     pll2->reg = reg;
>> +     gate->reg = reg;
>> +     gate->bit_idx = SUN4I_PLL2_ENABLE;
>> +
>> +     /* PLL2, also known as PLL2x1 */
>> +     of_property_read_string_index(np, "clock-output-names", 0, &clk_name);
>> +     clks[0] = clk_register_composite(NULL, clk_name, &parent, 1, NULL, NULL,
>> +                                      &pll2->hw, pll2_ops,
>> +                                      &gate->hw, &clk_gate_ops, 0);
>> +     WARN_ON(IS_ERR(clks[0]));
>> +     clk_set_rate(clks[0], 22579200);
>> +     parent = clk_name;
>> +
>> +     /* PLL2x2, 1/4 the rate of PLL2x8 */
>> +     of_property_read_string_index(np, "clock-output-names", 1, &clk_name);
>> +     clks[1] = clk_register_fixed_factor(NULL, clk_name, parent,
>> +                                         CLK_SET_RATE_PARENT, 2, 1);
>> +     WARN_ON(IS_ERR(clks[1]));
>> +
>> +     /* PLL2x4, 1/2 the rate of PLL2x8 */
>> +     of_property_read_string_index(np, "clock-output-names", 2, &clk_name);
>> +     clks[2] = clk_register_fixed_factor(NULL, clk_name, parent,
>> +                                         CLK_SET_RATE_PARENT, 4, 1);
>> +     WARN_ON(IS_ERR(clks[2]));
>> +
>> +     /* PLL2x8, double of PLL2 without the post divisor */
>> +     of_property_read_string_index(np, "clock-output-names", 3, &clk_name);
>> +     clks[3] = clk_register_fixed_factor(NULL, clk_name, parent,
>> +                                         CLK_SET_RATE_PARENT, 2 * 4, 1);
>
> Why have you declared them here, instead of using fixed factors in the
> DT directly, like we have done in the past?

IIRC, We have not done so for the PLL clocks. This is how it was, though
it was with the more complicated divs clocks.


ChenYu

On Sun, Aug 03, 2014 at 11:58:16PM +0800, Chen-Yu Tsai wrote:
> >> +     /* PLL2, also known as PLL2x1 */
> >> +     of_property_read_string_index(np, "clock-output-names", 0, &clk_name);
> >> +     clks[0] = clk_register_composite(NULL, clk_name, &parent, 1, NULL, NULL,
> >> +                                      &pll2->hw, pll2_ops,
> >> +                                      &gate->hw, &clk_gate_ops, 0);
> >> +     WARN_ON(IS_ERR(clks[0]));
> >> +     clk_set_rate(clks[0], 22579200);
> >> +     parent = clk_name;
> >> +
> >> +     /* PLL2x2, 1/4 the rate of PLL2x8 */
> >> +     of_property_read_string_index(np, "clock-output-names", 1, &clk_name);
> >> +     clks[1] = clk_register_fixed_factor(NULL, clk_name, parent,
> >> +                                         CLK_SET_RATE_PARENT, 2, 1);
> >> +     WARN_ON(IS_ERR(clks[1]));
> >> +
> >> +     /* PLL2x4, 1/2 the rate of PLL2x8 */
> >> +     of_property_read_string_index(np, "clock-output-names", 2, &clk_name);
> >> +     clks[2] = clk_register_fixed_factor(NULL, clk_name, parent,
> >> +                                         CLK_SET_RATE_PARENT, 4, 1);
> >> +     WARN_ON(IS_ERR(clks[2]));
> >> +
> >> +     /* PLL2x8, double of PLL2 without the post divisor */
> >> +     of_property_read_string_index(np, "clock-output-names", 3, &clk_name);
> >> +     clks[3] = clk_register_fixed_factor(NULL, clk_name, parent,
> >> +                                         CLK_SET_RATE_PARENT, 2 * 4, 1);
> >
> > Why have you declared them here, instead of using fixed factors in the
> > DT directly, like we have done in the past?
> 
> IIRC, We have not done so for the PLL clocks. This is how it was, though
> it was with the more complicated divs clocks.

I don't see any obvious multipliers in the clock-output-names.

Hi,

El 03/08/14 a las 09:44, Maxime Ripard escibió:
> On Thu, Jul 31, 2014 at 06:28:07PM -0300, Emilio López wrote:
>> This patch adds support for PLL2 and derivates on sun4i, sun5i and
>> sun7i SoCs. As this PLL is only used for audio and requires good
>> accuracy, we only support two known good rates.
>>
>> Signed-off-by: Emilio López <emilio@elopez.com.ar>
>> ---
>>
(...)
>> +	/* PLL2x8, double of PLL2 without the post divisor */
>> +	of_property_read_string_index(np, "clock-output-names", 3, &clk_name);
>> +	clks[3] = clk_register_fixed_factor(NULL, clk_name, parent,
>> +					    CLK_SET_RATE_PARENT, 2 * 4, 1);
>
> Why have you declared them here, instead of using fixed factors in the
> DT directly, like we have done in the past?

I'd prefer to see one clock with four outputs represented as one clock 
with four outputs rather than one clock with one output and other three 
weirdly named nodes floating around. I can only see four uses of 
fixed-factor-clock in our DTSI, three of which are "pass through" clocks 
for ahb and ar100 on sun6/8i, and a fourth one to implement the 32k that 
results from reducing the 24M osc on sun7i.

As a second reason, implementing it as fixed factors would be slightly 
incorrect, as it's only "x8" when you use the limited set of factors 
that Allwinner and us are using for audio. If we were ever to support 
the full range of the PLL. the DT binding would suddenly be off. By 
using a single binding, the meaning is self-contained in the driver.

As a third reason, of_fixed_factor_clk_setup() does not set 
CLK_SET_RATE_PARENT, which we'd need to propagate the rate change upstream.

Cheers,

Emilio

On Sun, Aug 03, 2014 at 07:02:41PM -0300, Emilio López wrote:
> Hi,
> 
> El 03/08/14 a las 09:44, Maxime Ripard escibió:
> >On Thu, Jul 31, 2014 at 06:28:07PM -0300, Emilio López wrote:
> >>This patch adds support for PLL2 and derivates on sun4i, sun5i and
> >>sun7i SoCs. As this PLL is only used for audio and requires good
> >>accuracy, we only support two known good rates.
> >>
> >>Signed-off-by: Emilio López <emilio@elopez.com.ar>
> >>---
> >>
> (...)
> >>+	/* PLL2x8, double of PLL2 without the post divisor */
> >>+	of_property_read_string_index(np, "clock-output-names", 3, &clk_name);
> >>+	clks[3] = clk_register_fixed_factor(NULL, clk_name, parent,
> >>+					    CLK_SET_RATE_PARENT, 2 * 4, 1);
> >
> >Why have you declared them here, instead of using fixed factors in the
> >DT directly, like we have done in the past?
> 
> I'd prefer to see one clock with four outputs represented as one
> clock with four outputs rather than one clock with one output and
> other three weirdly named nodes floating around.

Except that you have no control over these 3 other outputs, that
directly derive from the only one you can control.

Plus, their name wouldn't be weirded than the one you gave in
clock-output-names ;)

> I can only see four uses of fixed-factor-clock in our DTSI, three of
> which are "pass through" clocks for ahb and ar100 on sun6/8i, and a
> fourth one to implement the 32k that results from reducing the 24M
> osc on sun7i.

And PLLx2, PLL2x4, PLL2x8 are really just pass-through clocks
too. Once PLL2 is controlled, those will just be enabled.

> As a second reason, implementing it as fixed factors would be
> slightly incorrect, as it's only "x8" when you use the limited set
> of factors that Allwinner and us are using for audio.

Hu? What do you mean by that?

> If we were ever to support the full range of the PLL. the DT binding
> would suddenly be off. By using a single binding, the meaning is
> self-contained in the driver.
> 
> As a third reason, of_fixed_factor_clk_setup() does not set
> CLK_SET_RATE_PARENT, which we'd need to propagate the rate change
> upstream.

That would be easy to fix.

Maxime

El 04/08/14 a las 17:02, Maxime Ripard escibió:
> On Sun, Aug 03, 2014 at 07:02:41PM -0300, Emilio López wrote:
>> Hi,
>>
>> El 03/08/14 a las 09:44, Maxime Ripard escibió:
>>> On Thu, Jul 31, 2014 at 06:28:07PM -0300, Emilio López wrote:
>>>> This patch adds support for PLL2 and derivates on sun4i, sun5i and
>>>> sun7i SoCs. As this PLL is only used for audio and requires good
>>>> accuracy, we only support two known good rates.
>>>>
>>>> Signed-off-by: Emilio López <emilio@elopez.com.ar>
>>>> ---
>>>>
>> (...)
>>>> +	/* PLL2x8, double of PLL2 without the post divisor */
>>>> +	of_property_read_string_index(np, "clock-output-names", 3, &clk_name);
>>>> +	clks[3] = clk_register_fixed_factor(NULL, clk_name, parent,
>>>> +					    CLK_SET_RATE_PARENT, 2 * 4, 1);
>>>
>>> Why have you declared them here, instead of using fixed factors in the
>>> DT directly, like we have done in the past?
>>
>> I'd prefer to see one clock with four outputs represented as one
>> clock with four outputs rather than one clock with one output and
>> other three weirdly named nodes floating around.
>
> Except that you have no control over these 3 other outputs, that
> directly derive from the only one you can control.

You do have one degree of control, see below.

> Plus, their name wouldn't be weirded than the one you gave in
> clock-output-names ;)

Allwinner chose those names on the user manual, not me :)

>> I can only see four uses of fixed-factor-clock in our DTSI, three of
>> which are "pass through" clocks for ahb and ar100 on sun6/8i, and a
>> fourth one to implement the 32k that results from reducing the 24M
>> osc on sun7i.
>
> And PLLx2, PLL2x4, PLL2x8 are really just pass-through clocks
> too. Once PLL2 is controlled, those will just be enabled.

Yes and no.

>> As a second reason, implementing it as fixed factors would be
>> slightly incorrect, as it's only "x8" when you use the limited set
>> of factors that Allwinner and us are using for audio.
>
> Hu? What do you mean by that?

PLL2 (aka PLL2x1) is 24 * N / pre / post
PLL2x8 is 24 * 2 * N / pre

So PLL2 * 8 == PLL2x8 only if post == 4, which holds true on our 
software implementation as well as AW's, but may not always hold true 
hardware wise.

On Thu, Jul 31, 2014 at 5:28 PM, Emilio López <emilio@elopez.com.ar> wrote:
> +CLK_OF_DECLARE(sun4i_pll2_a, "allwinner,sun4i-a10-a-pll2-clk", sun4i_pll2_setup);
> +CLK_OF_DECLARE(sun4i_pll2_b, "allwinner,sun4i-a10-b-pll2-clk", sun4i_pll2_setup);


This second one should not encode the 'b'.
CLK_OF_DECLARE(sun4i_pll2_b, "allwinner,sun4i-a10-pll2-clk", sun4i_pll2_setup);

The generic DTDs should only contain compatibles that don't have a
chip revision in them.

Then there are two choices:
1) Identify your chip revision and load a DTD with the specific
revisions in the compatible strings.
2) Rely on the OS to identify the revision and modify the compatible at boot.

The strings in the generic DTD should never contain a specific
revision simply because it is unknown in a generic DTD.

> --
> 2.0.3

On Thu, Jul 31, 2014 at 5:28 PM, Emilio López <emilio@elopez.com.ar> wrote:
> This patch adds support for PLL2 and derivates on sun4i, sun5i and
> sun7i SoCs. As this PLL is only used for audio and requires good
> accuracy, we only support two known good rates.
>
> Signed-off-by: Emilio López <emilio@elopez.com.ar>
> ---
>
> Changes from RFC:
>  * Add support for A10 rev. A
>  * Document compatibles
>  * Use fixed factors
>
>  Documentation/devicetree/bindings/clock/sunxi.txt |   2 +
>  drivers/clk/sunxi/Makefile                        |   1 +
>  drivers/clk/sunxi/clk-a10-pll2.c                  | 243 ++++++++++++++++++++++
>  3 files changed, 246 insertions(+)
>  create mode 100644 drivers/clk/sunxi/clk-a10-pll2.c
>
> diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt
> index d3a5c3c..41ada31 100644
> --- a/Documentation/devicetree/bindings/clock/sunxi.txt
> +++ b/Documentation/devicetree/bindings/clock/sunxi.txt
> @@ -10,6 +10,8 @@ Required properties:
>         "allwinner,sun4i-a10-pll1-clk" - for the main PLL clock and PLL4
>         "allwinner,sun6i-a31-pll1-clk" - for the main PLL clock on A31
>         "allwinner,sun8i-a23-pll1-clk" - for the main PLL clock on A23
> +       "allwinner,sun4i-a10-a-pll2-clk" - for the PLL2 clock on A10 rev. A
> +       "allwinner,sun4i-a10-b-pll2-clk" - for the PLL2 clock on A10 rev. B
>         "allwinner,sun4i-a10-pll5-clk" - for the PLL5 clock
>         "allwinner,sun4i-a10-pll6-clk" - for the PLL6 clock
>         "allwinner,sun6i-a31-pll6-clk" - for the PLL6 clock on A31
> diff --git a/drivers/clk/sunxi/Makefile b/drivers/clk/sunxi/Makefile
> index 6850cba..dcd5709 100644
> --- a/drivers/clk/sunxi/Makefile
> +++ b/drivers/clk/sunxi/Makefile
> @@ -4,6 +4,7 @@
>
>  obj-y += clk-sunxi.o clk-factors.o
>  obj-y += clk-a10-hosc.o
> +obj-y += clk-a10-pll2.o
>  obj-y += clk-a20-gmac.o
>
>  obj-$(CONFIG_MFD_SUN6I_PRCM) += \
> diff --git a/drivers/clk/sunxi/clk-a10-pll2.c b/drivers/clk/sunxi/clk-a10-pll2.c
> new file mode 100644
> index 0000000..bcf7d0b
> --- /dev/null
> +++ b/drivers/clk/sunxi/clk-a10-pll2.c
> @@ -0,0 +1,243 @@
> +/*
> + * Copyright 2014 Emilio López
> + *
> + * Emilio López <emilio@elopez.com.ar>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/slab.h>
> +
> +#define SUN4I_PLL2_ENABLE              31
> +#define SUN4I_PLL2_A_VCOBIAS           0
> +#define SUN4I_PLL2_A_VCOBIAS_MASK      0x1F
> +#define SUN4I_PLL2_A_N                 7
> +#define SUN4I_PLL2_A_N_MASK            0x7F
> +#define SUN4I_PLL2_B_POST_DIV          26
> +#define SUN4I_PLL2_B_POST_DIV_MASK     0xF
> +#define SUN4I_PLL2_B_N                 8
> +#define SUN4I_PLL2_B_N_MASK            0x7F
> +#define SUN4I_PLL2_B_PRE_DIV           0
> +#define SUN4I_PLL2_B_PRE_DIV_MASK      0x1F
> +
> +#define SUN4I_PLL2_OUTPUTS             4
> +
> +struct sun4i_pll2_clk {
> +       struct clk_hw hw;
> +       void __iomem *reg;
> +};
> +
> +static inline struct sun4i_pll2_clk *to_sun4i_pll2_clk(struct clk_hw *hw)
> +{
> +       return container_of(hw, struct sun4i_pll2_clk, hw);
> +}
> +
> +static unsigned long sun4i_pll2_recalc_rate_a(struct clk_hw *hw,
> +                                             unsigned long parent_rate)
> +{
> +       struct sun4i_pll2_clk *clk = to_sun4i_pll2_clk(hw);
> +       int vcobias, n;
> +       u32 val;
> +
> +       val = readl(clk->reg);
> +       vcobias = (val >> SUN4I_PLL2_A_VCOBIAS) & SUN4I_PLL2_A_VCOBIAS_MASK;
> +       n = (val >> SUN4I_PLL2_A_N) & SUN4I_PLL2_A_N_MASK;
> +
> +       if (vcobias == 10 && n == 94)
> +               return 22579200;
> +       else if (vcobias == 9 && n == 83)
> +               return 24576000;
> +
> +       /*
> +        * Unfortunately we don't really have much documentation on how
> +        * these factors relate mathematically
> +        */
> +       return 0;
> +}
> +
> +static unsigned long sun4i_pll2_recalc_rate_b(struct clk_hw *hw,
> +                                             unsigned long parent_rate)
> +{
> +       struct sun4i_pll2_clk *clk = to_sun4i_pll2_clk(hw);
> +       int n, prediv, postdiv;
> +       u32 val;
> +
> +       val = readl(clk->reg);
> +       n = (val >> SUN4I_PLL2_B_N) & SUN4I_PLL2_B_N_MASK;
> +       prediv = (val >> SUN4I_PLL2_B_PRE_DIV) & SUN4I_PLL2_B_PRE_DIV_MASK;
> +       postdiv = (val >> SUN4I_PLL2_B_POST_DIV) & SUN4I_PLL2_B_POST_DIV_MASK;
> +
> +       /* 0 is a special case and means 1 */
> +       if (n == 0)
> +               n = 1;
> +       if (prediv == 0)
> +               prediv = 1;
> +       if (postdiv == 0)
> +               postdiv = 1;
> +
> +       return ((parent_rate * n) / prediv) / postdiv;
> +}
> +
> +static long sun4i_pll2_round_rate(struct clk_hw *hw, unsigned long rate,
> +                                 unsigned long *parent_rate)
> +{
> +       /*
> +        * There is only two interesting rates for the audio PLL, the
> +        * rest isn't really usable due to accuracy concerns. Therefore,
> +        * we specifically round to those rates here
> +        */
> +       if (rate < 22579200)
> +               return -EINVAL;
> +
> +       if (rate >= 22579200 && rate < 24576000)
> +               return 22579200;
> +
> +       return 24576000;
> +}

These should return the actual computed rate osc*n / (pre * post) so
that the error column is displayed correctly.

   pll2                                  1            1    22571428          0
       i2s0                               1            1    22571428          0
          mclk0                           0            0    22571428          0

At 22.5 the error is 0.03%, 24.5 error is 0.01%.

So when you ask for 24.576 it returns 24.571428 and 22.5792 returns 22.571428.




> +
> +static int sun4i_pll2_set_rate_a(struct clk_hw *hw, unsigned long rate,
> +                                unsigned long parent_rate)
> +{
> +       struct sun4i_pll2_clk *clk = to_sun4i_pll2_clk(hw);
> +       u32 val = readl(clk->reg);
> +
> +       val &= ~(SUN4I_PLL2_A_VCOBIAS_MASK << SUN4I_PLL2_A_VCOBIAS);
> +       val &= ~(SUN4I_PLL2_A_N_MASK << SUN4I_PLL2_A_N);
> +
> +       if (rate == 22579200)
> +               val |= (10 << SUN4I_PLL2_A_VCOBIAS) | (94 << SUN4I_PLL2_A_N);
> +       else if (rate == 24576000)
> +               val |= (9 << SUN4I_PLL2_A_VCOBIAS) | (83 << SUN4I_PLL2_A_N);
> +       else
> +               return -EINVAL;
> +
> +       writel(val, clk->reg);
> +
> +       return 0;
> +}
> +
> +static int sun4i_pll2_set_rate_b(struct clk_hw *hw, unsigned long rate,
> +                                unsigned long parent_rate)
> +{
> +       struct sun4i_pll2_clk *clk = to_sun4i_pll2_clk(hw);
> +       u32 val = readl(clk->reg);
> +
> +       val &= ~(SUN4I_PLL2_B_N_MASK << SUN4I_PLL2_B_N);
> +       val &= ~(SUN4I_PLL2_B_PRE_DIV_MASK << SUN4I_PLL2_B_PRE_DIV);
> +       val &= ~(SUN4I_PLL2_B_POST_DIV_MASK << SUN4I_PLL2_B_POST_DIV);
> +
> +       val |= (21 << SUN4I_PLL2_B_PRE_DIV) | (4 << SUN4I_PLL2_B_POST_DIV);
> +
> +       if (rate == 22579200)
> +               val |= (79 << SUN4I_PLL2_B_N);
> +       else if (rate == 24576000)
> +               val |= (86 << SUN4I_PLL2_B_N);
> +       else
> +               return -EINVAL;
> +
> +       writel(val, clk->reg);
> +
> +       return 0;
> +}
> +
> +static const struct clk_ops sun4i_pll2_ops_a = {
> +       .recalc_rate = sun4i_pll2_recalc_rate_a,
> +       .round_rate = sun4i_pll2_round_rate,
> +       .set_rate = sun4i_pll2_set_rate_a,
> +};
> +
> +
> +static const struct clk_ops sun4i_pll2_ops_b = {
> +       .recalc_rate = sun4i_pll2_recalc_rate_b,
> +       .round_rate = sun4i_pll2_round_rate,
> +       .set_rate = sun4i_pll2_set_rate_b,
> +};
> +
> +static const struct of_device_id pll2_matches[] __initconst = {
> +       { .compatible = "allwinner,sun4i-a10-a-pll2-clk", .data = &sun4i_pll2_ops_a },
> +       { .compatible = "allwinner,sun4i-a10-b-pll2-clk", .data = &sun4i_pll2_ops_b },
> +       { /* sentinel */ },
> +};
> +
> +static void __init sun4i_pll2_setup(struct device_node *np)
> +{
> +       const char *clk_name = np->name, *parent;
> +       const struct of_device_id *match;
> +       struct clk_onecell_data *clk_data;
> +       const struct clk_ops *pll2_ops;
> +       struct sun4i_pll2_clk *pll2;
> +       struct clk_gate *gate;
> +       struct clk **clks;
> +       void __iomem *reg;
> +
> +       /* Choose the correct ops for pll2 */
> +       match = of_match_node(pll2_matches, np);
> +       pll2_ops = match->data;
> +
> +       pll2 = kzalloc(sizeof(*pll2), GFP_KERNEL);
> +       gate = kzalloc(sizeof(*gate), GFP_KERNEL);
> +       clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
> +       clks = kcalloc(SUN4I_PLL2_OUTPUTS, sizeof(*clks), GFP_KERNEL);
> +       if (!pll2 || !gate || !clk_data || !clks)
> +               goto free_mem;
> +
> +       reg = of_iomap(np, 0);
> +       parent = of_clk_get_parent_name(np, 0);
> +       of_property_read_string_index(np, "clock-output-names", 0, &clk_name);
> +
> +       pll2->reg = reg;
> +       gate->reg = reg;
> +       gate->bit_idx = SUN4I_PLL2_ENABLE;
> +
> +       /* PLL2, also known as PLL2x1 */
> +       of_property_read_string_index(np, "clock-output-names", 0, &clk_name);
> +       clks[0] = clk_register_composite(NULL, clk_name, &parent, 1, NULL, NULL,
> +                                        &pll2->hw, pll2_ops,
> +                                        &gate->hw, &clk_gate_ops, 0);
> +       WARN_ON(IS_ERR(clks[0]));
> +       clk_set_rate(clks[0], 22579200);
> +       parent = clk_name;
> +
> +       /* PLL2x2, 1/4 the rate of PLL2x8 */
> +       of_property_read_string_index(np, "clock-output-names", 1, &clk_name);
> +       clks[1] = clk_register_fixed_factor(NULL, clk_name, parent,
> +                                           CLK_SET_RATE_PARENT, 2, 1);
> +       WARN_ON(IS_ERR(clks[1]));
> +
> +       /* PLL2x4, 1/2 the rate of PLL2x8 */
> +       of_property_read_string_index(np, "clock-output-names", 2, &clk_name);
> +       clks[2] = clk_register_fixed_factor(NULL, clk_name, parent,
> +                                           CLK_SET_RATE_PARENT, 4, 1);
> +       WARN_ON(IS_ERR(clks[2]));
> +
> +       /* PLL2x8, double of PLL2 without the post divisor */
> +       of_property_read_string_index(np, "clock-output-names", 3, &clk_name);
> +       clks[3] = clk_register_fixed_factor(NULL, clk_name, parent,
> +                                           CLK_SET_RATE_PARENT, 2 * 4, 1);
> +       WARN_ON(IS_ERR(clks[3]));
> +
> +       clk_data->clks = clks;
> +       clk_data->clk_num = SUN4I_PLL2_OUTPUTS;
> +       of_clk_add_provider(np, of_clk_src_onecell_get, clk_data);
> +
> +       return;
> +
> +free_mem:
> +       kfree(pll2);
> +       kfree(gate);
> +       kfree(clk_data);
> +       kfree(clks);
> +}
> +CLK_OF_DECLARE(sun4i_pll2_a, "allwinner,sun4i-a10-a-pll2-clk", sun4i_pll2_setup);
> +CLK_OF_DECLARE(sun4i_pll2_b, "allwinner,sun4i-a10-b-pll2-clk", sun4i_pll2_setup);
> --
> 2.0.3

On Mon, Aug 04, 2014 at 05:23:44PM -0300, Emilio López wrote:
> El 04/08/14 a las 17:02, Maxime Ripard escibió:
> >On Sun, Aug 03, 2014 at 07:02:41PM -0300, Emilio López wrote:
> >>Hi,
> >>
> >>El 03/08/14 a las 09:44, Maxime Ripard escibió:
> >>>On Thu, Jul 31, 2014 at 06:28:07PM -0300, Emilio López wrote:
> >>>>This patch adds support for PLL2 and derivates on sun4i, sun5i and
> >>>>sun7i SoCs. As this PLL is only used for audio and requires good
> >>>>accuracy, we only support two known good rates.
> >>>>
> >>>>Signed-off-by: Emilio López <emilio@elopez.com.ar>
> >>>>---
> >>>>
> >>(...)
> >>>>+	/* PLL2x8, double of PLL2 without the post divisor */
> >>>>+	of_property_read_string_index(np, "clock-output-names", 3, &clk_name);
> >>>>+	clks[3] = clk_register_fixed_factor(NULL, clk_name, parent,
> >>>>+					    CLK_SET_RATE_PARENT, 2 * 4, 1);
> >>>
> >>>Why have you declared them here, instead of using fixed factors in the
> >>>DT directly, like we have done in the past?
> >>
> >>I'd prefer to see one clock with four outputs represented as one
> >>clock with four outputs rather than one clock with one output and
> >>other three weirdly named nodes floating around.
> >
> >Except that you have no control over these 3 other outputs, that
> >directly derive from the only one you can control.
> 
> You do have one degree of control, see below.
> 
> >Plus, their name wouldn't be weirded than the one you gave in
> >clock-output-names ;)
> 
> Allwinner chose those names on the user manual, not me :)
> 
> >>I can only see four uses of fixed-factor-clock in our DTSI, three of
> >>which are "pass through" clocks for ahb and ar100 on sun6/8i, and a
> >>fourth one to implement the 32k that results from reducing the 24M
> >>osc on sun7i.
> >
> >And PLLx2, PLL2x4, PLL2x8 are really just pass-through clocks
> >too. Once PLL2 is controlled, those will just be enabled.
> 
> Yes and no.
> 
> >>As a second reason, implementing it as fixed factors would be
> >>slightly incorrect, as it's only "x8" when you use the limited set
> >>of factors that Allwinner and us are using for audio.
> >
> >Hu? What do you mean by that?
> 
> PLL2 (aka PLL2x1) is 24 * N / pre / post
> PLL2x8 is 24 * 2 * N / pre
> 
> So PLL2 * 8 == PLL2x8 only if post == 4, which holds true on our
> software implementation as well as AW's, but may not always hold
> true hardware wise.

Ah, right. I was looking at the wrong datasheet for this, my bad.

Then, at least document the bindings properly :)

Maxime

On Wed, Aug 6, 2014 at 11:20 AM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote:
> On Thu, Jul 31, 2014 at 5:28 PM, Emilio López <emilio@elopez.com.ar> wrote:
>> This patch adds support for PLL2 and derivates on sun4i, sun5i and
>> sun7i SoCs. As this PLL is only used for audio and requires good
>> accuracy, we only support two known good rates.
>>
>> Signed-off-by: Emilio López <emilio@elopez.com.ar>
>> ---
>>
>> Changes from RFC:
>>  * Add support for A10 rev. A
>>  * Document compatibles
>>  * Use fixed factors
>>
>>  Documentation/devicetree/bindings/clock/sunxi.txt |   2 +
>>  drivers/clk/sunxi/Makefile                        |   1 +
>>  drivers/clk/sunxi/clk-a10-pll2.c                  | 243 ++++++++++++++++++++++
>>  3 files changed, 246 insertions(+)
>>  create mode 100644 drivers/clk/sunxi/clk-a10-pll2.c
>>
>> diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt
>> index d3a5c3c..41ada31 100644
>> --- a/Documentation/devicetree/bindings/clock/sunxi.txt
>> +++ b/Documentation/devicetree/bindings/clock/sunxi.txt
>> @@ -10,6 +10,8 @@ Required properties:
>>         "allwinner,sun4i-a10-pll1-clk" - for the main PLL clock and PLL4
>>         "allwinner,sun6i-a31-pll1-clk" - for the main PLL clock on A31
>>         "allwinner,sun8i-a23-pll1-clk" - for the main PLL clock on A23
>> +       "allwinner,sun4i-a10-a-pll2-clk" - for the PLL2 clock on A10 rev. A
>> +       "allwinner,sun4i-a10-b-pll2-clk" - for the PLL2 clock on A10 rev. B
>>         "allwinner,sun4i-a10-pll5-clk" - for the PLL5 clock
>>         "allwinner,sun4i-a10-pll6-clk" - for the PLL6 clock
>>         "allwinner,sun6i-a31-pll6-clk" - for the PLL6 clock on A31
>> diff --git a/drivers/clk/sunxi/Makefile b/drivers/clk/sunxi/Makefile
>> index 6850cba..dcd5709 100644
>> --- a/drivers/clk/sunxi/Makefile
>> +++ b/drivers/clk/sunxi/Makefile
>> @@ -4,6 +4,7 @@
>>
>>  obj-y += clk-sunxi.o clk-factors.o
>>  obj-y += clk-a10-hosc.o
>> +obj-y += clk-a10-pll2.o
>>  obj-y += clk-a20-gmac.o
>>
>>  obj-$(CONFIG_MFD_SUN6I_PRCM) += \
>> diff --git a/drivers/clk/sunxi/clk-a10-pll2.c b/drivers/clk/sunxi/clk-a10-pll2.c
>> new file mode 100644
>> index 0000000..bcf7d0b
>> --- /dev/null
>> +++ b/drivers/clk/sunxi/clk-a10-pll2.c
>> @@ -0,0 +1,243 @@
>> +/*
>> + * Copyright 2014 Emilio López
>> + *
>> + * Emilio López <emilio@elopez.com.ar>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/clk-provider.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/slab.h>
>> +
>> +#define SUN4I_PLL2_ENABLE              31
>> +#define SUN4I_PLL2_A_VCOBIAS           0
>> +#define SUN4I_PLL2_A_VCOBIAS_MASK      0x1F
>> +#define SUN4I_PLL2_A_N                 7
>> +#define SUN4I_PLL2_A_N_MASK            0x7F
>> +#define SUN4I_PLL2_B_POST_DIV          26
>> +#define SUN4I_PLL2_B_POST_DIV_MASK     0xF
>> +#define SUN4I_PLL2_B_N                 8
>> +#define SUN4I_PLL2_B_N_MASK            0x7F
>> +#define SUN4I_PLL2_B_PRE_DIV           0
>> +#define SUN4I_PLL2_B_PRE_DIV_MASK      0x1F
>> +
>> +#define SUN4I_PLL2_OUTPUTS             4
>> +
>> +struct sun4i_pll2_clk {
>> +       struct clk_hw hw;
>> +       void __iomem *reg;
>> +};
>> +
>> +static inline struct sun4i_pll2_clk *to_sun4i_pll2_clk(struct clk_hw *hw)
>> +{
>> +       return container_of(hw, struct sun4i_pll2_clk, hw);
>> +}
>> +
>> +static unsigned long sun4i_pll2_recalc_rate_a(struct clk_hw *hw,
>> +                                             unsigned long parent_rate)
>> +{
>> +       struct sun4i_pll2_clk *clk = to_sun4i_pll2_clk(hw);
>> +       int vcobias, n;
>> +       u32 val;
>> +
>> +       val = readl(clk->reg);
>> +       vcobias = (val >> SUN4I_PLL2_A_VCOBIAS) & SUN4I_PLL2_A_VCOBIAS_MASK;
>> +       n = (val >> SUN4I_PLL2_A_N) & SUN4I_PLL2_A_N_MASK;
>> +
>> +       if (vcobias == 10 && n == 94)
>> +               return 22579200;
>> +       else if (vcobias == 9 && n == 83)
>> +               return 24576000;
>> +
>> +       /*
>> +        * Unfortunately we don't really have much documentation on how
>> +        * these factors relate mathematically
>> +        */
>> +       return 0;
>> +}
>> +
>> +static unsigned long sun4i_pll2_recalc_rate_b(struct clk_hw *hw,
>> +                                             unsigned long parent_rate)
>> +{
>> +       struct sun4i_pll2_clk *clk = to_sun4i_pll2_clk(hw);
>> +       int n, prediv, postdiv;
>> +       u32 val;
>> +
>> +       val = readl(clk->reg);
>> +       n = (val >> SUN4I_PLL2_B_N) & SUN4I_PLL2_B_N_MASK;
>> +       prediv = (val >> SUN4I_PLL2_B_PRE_DIV) & SUN4I_PLL2_B_PRE_DIV_MASK;
>> +       postdiv = (val >> SUN4I_PLL2_B_POST_DIV) & SUN4I_PLL2_B_POST_DIV_MASK;
>> +
>> +       /* 0 is a special case and means 1 */
>> +       if (n == 0)
>> +               n = 1;
>> +       if (prediv == 0)
>> +               prediv = 1;
>> +       if (postdiv == 0)
>> +               postdiv = 1;
>> +
>> +       return ((parent_rate * n) / prediv) / postdiv;
>> +}
>> +
>> +static long sun4i_pll2_round_rate(struct clk_hw *hw, unsigned long rate,
>> +                                 unsigned long *parent_rate)
>> +{
>> +       /*
>> +        * There is only two interesting rates for the audio PLL, the
>> +        * rest isn't really usable due to accuracy concerns. Therefore,
>> +        * we specifically round to those rates here
>> +        */
>> +       if (rate < 22579200)
>> +               return -EINVAL;
>> +
>> +       if (rate >= 22579200 && rate < 24576000)
>> +               return 22579200;
>> +
>> +       return 24576000;
>> +}
>
> These should return the actual computed rate osc*n / (pre * post) so
> that the error column is displayed correctly.
>
>    pll2                                  1            1    22571428          0
>        i2s0                               1            1    22571428          0
>           mclk0                           0            0    22571428          0
>
> At 22.5 the error is 0.03%, 24.5 error is 0.01%.
>
> So when you ask for 24.576 it returns 24.571428 and 22.5792 returns 22.571428.
>
>

This is more complicated that it looks. If you set 22.5792Mhz and the
clock doesn't return  22.5792Mhz it throws off the math inside of
ALSA. For example 22579200 / 44100 = 512fs. But if you use  22571428 /
44100 = 511.8 = 511 because of truncation. Now the checks looking for
512 don't trigger. I just hit this bug.

So I think the clock is doing the right thing when you set 22.5792Mhz
and it returns the actual number 22.571428Mhz.

It's the ALSA code that needs to be more tolerant and use some round()
functions to allow for a slight error in these frequencies.


>
>
>> +
>> +static int sun4i_pll2_set_rate_a(struct clk_hw *hw, unsigned long rate,
>> +                                unsigned long parent_rate)
>> +{
>> +       struct sun4i_pll2_clk *clk = to_sun4i_pll2_clk(hw);
>> +       u32 val = readl(clk->reg);
>> +
>> +       val &= ~(SUN4I_PLL2_A_VCOBIAS_MASK << SUN4I_PLL2_A_VCOBIAS);
>> +       val &= ~(SUN4I_PLL2_A_N_MASK << SUN4I_PLL2_A_N);
>> +
>> +       if (rate == 22579200)
>> +               val |= (10 << SUN4I_PLL2_A_VCOBIAS) | (94 << SUN4I_PLL2_A_N);
>> +       else if (rate == 24576000)
>> +               val |= (9 << SUN4I_PLL2_A_VCOBIAS) | (83 << SUN4I_PLL2_A_N);
>> +       else
>> +               return -EINVAL;
>> +
>> +       writel(val, clk->reg);
>> +
>> +       return 0;
>> +}
>> +
>> +static int sun4i_pll2_set_rate_b(struct clk_hw *hw, unsigned long rate,
>> +                                unsigned long parent_rate)
>> +{
>> +       struct sun4i_pll2_clk *clk = to_sun4i_pll2_clk(hw);
>> +       u32 val = readl(clk->reg);
>> +
>> +       val &= ~(SUN4I_PLL2_B_N_MASK << SUN4I_PLL2_B_N);
>> +       val &= ~(SUN4I_PLL2_B_PRE_DIV_MASK << SUN4I_PLL2_B_PRE_DIV);
>> +       val &= ~(SUN4I_PLL2_B_POST_DIV_MASK << SUN4I_PLL2_B_POST_DIV);
>> +
>> +       val |= (21 << SUN4I_PLL2_B_PRE_DIV) | (4 << SUN4I_PLL2_B_POST_DIV);
>> +
>> +       if (rate == 22579200)
>> +               val |= (79 << SUN4I_PLL2_B_N);
>> +       else if (rate == 24576000)
>> +               val |= (86 << SUN4I_PLL2_B_N);
>> +       else
>> +               return -EINVAL;
>> +
>> +       writel(val, clk->reg);
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct clk_ops sun4i_pll2_ops_a = {
>> +       .recalc_rate = sun4i_pll2_recalc_rate_a,
>> +       .round_rate = sun4i_pll2_round_rate,
>> +       .set_rate = sun4i_pll2_set_rate_a,
>> +};
>> +
>> +
>> +static const struct clk_ops sun4i_pll2_ops_b = {
>> +       .recalc_rate = sun4i_pll2_recalc_rate_b,
>> +       .round_rate = sun4i_pll2_round_rate,
>> +       .set_rate = sun4i_pll2_set_rate_b,
>> +};
>> +
>> +static const struct of_device_id pll2_matches[] __initconst = {
>> +       { .compatible = "allwinner,sun4i-a10-a-pll2-clk", .data = &sun4i_pll2_ops_a },
>> +       { .compatible = "allwinner,sun4i-a10-b-pll2-clk", .data = &sun4i_pll2_ops_b },
>> +       { /* sentinel */ },
>> +};
>> +
>> +static void __init sun4i_pll2_setup(struct device_node *np)
>> +{
>> +       const char *clk_name = np->name, *parent;
>> +       const struct of_device_id *match;
>> +       struct clk_onecell_data *clk_data;
>> +       const struct clk_ops *pll2_ops;
>> +       struct sun4i_pll2_clk *pll2;
>> +       struct clk_gate *gate;
>> +       struct clk **clks;
>> +       void __iomem *reg;
>> +
>> +       /* Choose the correct ops for pll2 */
>> +       match = of_match_node(pll2_matches, np);
>> +       pll2_ops = match->data;
>> +
>> +       pll2 = kzalloc(sizeof(*pll2), GFP_KERNEL);
>> +       gate = kzalloc(sizeof(*gate), GFP_KERNEL);
>> +       clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
>> +       clks = kcalloc(SUN4I_PLL2_OUTPUTS, sizeof(*clks), GFP_KERNEL);
>> +       if (!pll2 || !gate || !clk_data || !clks)
>> +               goto free_mem;
>> +
>> +       reg = of_iomap(np, 0);
>> +       parent = of_clk_get_parent_name(np, 0);
>> +       of_property_read_string_index(np, "clock-output-names", 0, &clk_name);
>> +
>> +       pll2->reg = reg;
>> +       gate->reg = reg;
>> +       gate->bit_idx = SUN4I_PLL2_ENABLE;
>> +
>> +       /* PLL2, also known as PLL2x1 */
>> +       of_property_read_string_index(np, "clock-output-names", 0, &clk_name);
>> +       clks[0] = clk_register_composite(NULL, clk_name, &parent, 1, NULL, NULL,
>> +                                        &pll2->hw, pll2_ops,
>> +                                        &gate->hw, &clk_gate_ops, 0);
>> +       WARN_ON(IS_ERR(clks[0]));
>> +       clk_set_rate(clks[0], 22579200);
>> +       parent = clk_name;
>> +
>> +       /* PLL2x2, 1/4 the rate of PLL2x8 */
>> +       of_property_read_string_index(np, "clock-output-names", 1, &clk_name);
>> +       clks[1] = clk_register_fixed_factor(NULL, clk_name, parent,
>> +                                           CLK_SET_RATE_PARENT, 2, 1);
>> +       WARN_ON(IS_ERR(clks[1]));
>> +
>> +       /* PLL2x4, 1/2 the rate of PLL2x8 */
>> +       of_property_read_string_index(np, "clock-output-names", 2, &clk_name);
>> +       clks[2] = clk_register_fixed_factor(NULL, clk_name, parent,
>> +                                           CLK_SET_RATE_PARENT, 4, 1);
>> +       WARN_ON(IS_ERR(clks[2]));
>> +
>> +       /* PLL2x8, double of PLL2 without the post divisor */
>> +       of_property_read_string_index(np, "clock-output-names", 3, &clk_name);
>> +       clks[3] = clk_register_fixed_factor(NULL, clk_name, parent,
>> +                                           CLK_SET_RATE_PARENT, 2 * 4, 1);
>> +       WARN_ON(IS_ERR(clks[3]));
>> +
>> +       clk_data->clks = clks;
>> +       clk_data->clk_num = SUN4I_PLL2_OUTPUTS;
>> +       of_clk_add_provider(np, of_clk_src_onecell_get, clk_data);
>> +
>> +       return;
>> +
>> +free_mem:
>> +       kfree(pll2);
>> +       kfree(gate);
>> +       kfree(clk_data);
>> +       kfree(clks);
>> +}
>> +CLK_OF_DECLARE(sun4i_pll2_a, "allwinner,sun4i-a10-a-pll2-clk", sun4i_pll2_setup);
>> +CLK_OF_DECLARE(sun4i_pll2_b, "allwinner,sun4i-a10-b-pll2-clk", sun4i_pll2_setup);
>> --
>> 2.0.3
>
>
>
> --
> Jon Smirl
> jonsmirl@gmail.com