[PATCH] clk: samsung: add set_rate and round_rate callbacks for pll45xx

[PATCH] clk: samsung: add set_rate and round_rate callbacks for pll45xx

[Thomas Abraham]

Add support for set_rate and round_rate callbacks for pll45xx pll. This allows
configuring pll45xx to generate a desired clock output.

Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
---

The set_rate and round_rate callbacks in this patch for pll45xx are handled
slightly differently from the way it is done in the (not yet merged) patch
series from Yadwinder
(http://www.mail-archive.com/linux-samsung-soc at vger.kernel.org/msg19540.html)

In this patch, the pll lookup table is kept as part of the pll configuration
code itself, since the pll is just a hardware block which takes an input
frequency and configuration values to genertate a output clock frequency.
Given a set of inputs, a pll of a given type will generate the same output
regardless of the SoC it is used in. So instead of supplying the pll lookup
table from per-soc clock driver code (as done in the Yadwinder's patch series),
the pll lookup table can be coupled with the pll code itself, saving duplication
of pll lookup table for every SoC the pll is used with.

 drivers/clk/samsung/clk-pll.c |   88 +++++++++++++++++++++++++++++++++++++++++
 drivers/clk/samsung/clk-pll.h |   15 +++++++
 2 files changed, 103 insertions(+), 0 deletions(-)

diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
index 362f12d..4940936 100644
--- a/drivers/clk/samsung/clk-pll.c
+++ b/drivers/clk/samsung/clk-pll.c
@@ -172,6 +172,8 @@ struct clk * __init samsung_clk_register_pll36xx(const char *name,
  * PLL45xx Clock Type
  */
 
+#define PLL45XX_EN_MASK		(1 << 31)
+#define PLL45XX_LOCK_MASK	(1 << 29)
 #define PLL45XX_MDIV_MASK	(0x3FF)
 #define PLL45XX_PDIV_MASK	(0x3F)
 #define PLL45XX_SDIV_MASK	(0x7)
@@ -187,6 +189,90 @@ struct samsung_clk_pll45xx {
 
 #define to_clk_pll45xx(_hw) container_of(_hw, struct samsung_clk_pll45xx, hw)
 
+/* a sorted table of freq supported by pll45xx with 24mhz parent clock */
+static struct pll45xx_freq_lookup pll45xx_freq_lookup_24mhz[] = {
+	PLL45XX_PMS(1000000000, 6, 250, 1),
+	PLL45XX_PMS(800000000, 6, 200, 1),
+	PLL45XX_PMS(500000000, 6, 250, 2),
+	PLL45XX_PMS(400000000, 6, 200, 2),
+	PLL45XX_PMS(200000000, 6, 200, 3),
+};
+
+static int samsung_pll45xx_set_rate(struct clk_hw *hw, unsigned long rate,
+					unsigned long prate)
+{
+	struct samsung_clk_pll45xx *pll = to_clk_pll45xx(hw);
+	struct pll45xx_freq_lookup *f;
+	unsigned long timeout, pll_con, cnt, idx;
+
+	/* select a lookup table based on parent clock frequency */
+	switch (prate) {
+	case 24000000:
+		f = pll45xx_freq_lookup_24mhz;
+		cnt = ARRAY_SIZE(pll45xx_freq_lookup_24mhz);
+		break;
+	default:
+		pr_err("%s: unsupported parent clock rate, failed to set rate",
+					__func__);
+		return -EINVAL;
+	}
+
+	/* check if the requested freq is in the list of supported freq */
+	for (idx = 0; idx < cnt; idx++, f++)
+		if (f->target_freq == rate)
+			break;
+
+	if (idx == cnt) {
+		pr_err("%s: unspported clock speed %ld requested\n",
+				__func__, rate);
+		return -EINVAL;
+	}
+
+	/* first, disable the output of the pll */
+	writel(readl(pll->con_reg) & ~PLL45XX_EN_MASK, (void *)pll->con_reg);
+
+	/* write the new pll configuration values */
+	pll_con = (f->pdiv << PLL45XX_PDIV_SHIFT) |
+			(f->mdiv << PLL45XX_MDIV_SHIFT) |
+			(f->sdiv << PLL45XX_SDIV_SHIFT);
+	writel(pll_con, (void *)pll->con_reg);
+
+	/* enable the pll and wait for it to stabilize */
+	writel(pll_con | PLL45XX_EN_MASK, (void *)pll->con_reg);
+	timeout = jiffies + msecs_to_jiffies(20);
+	while (time_before(jiffies, timeout))
+		if (readl(pll->con_reg) & PLL45XX_LOCK_MASK)
+			return 0;
+	return -EBUSY;
+}
+
+static long samsung_pll45xx_round_rate(struct clk_hw *hw, unsigned long rate,
+						unsigned long *prate)
+{
+	struct pll45xx_freq_lookup *f;
+	unsigned long cnt;
+
+	/* select a lookup table based on parent clock frequency */
+	switch (*prate) {
+	case 24000000:
+		f = pll45xx_freq_lookup_24mhz;
+		cnt = ARRAY_SIZE(pll45xx_freq_lookup_24mhz);
+		break;
+	default:
+		pr_err("%s: unsupported parent clock rate", __func__);
+		return *prate;
+	}
+
+	/* find the nearest possible clock output that can be supported */
+	while (cnt-- > 0) {
+		if (rate >= f->target_freq)
+			return f->target_freq;
+		f++;
+	}
+
+	return (--f)->target_freq;
+}
+
 static unsigned long samsung_pll45xx_recalc_rate(struct clk_hw *hw,
 				unsigned long parent_rate)
 {
@@ -209,6 +295,8 @@ static unsigned long samsung_pll45xx_recalc_rate(struct clk_hw *hw,
 }
 
 static const struct clk_ops samsung_pll45xx_clk_ops = {
+	.set_rate = samsung_pll45xx_set_rate,
+	.round_rate = samsung_pll45xx_round_rate,
 	.recalc_rate = samsung_pll45xx_recalc_rate,
 };
 
diff --git a/drivers/clk/samsung/clk-pll.h b/drivers/clk/samsung/clk-pll.h
index f33786e..fb687ec 100644
--- a/drivers/clk/samsung/clk-pll.h
+++ b/drivers/clk/samsung/clk-pll.h
@@ -18,6 +18,21 @@ enum pll45xx_type {
 	pll_4508
 };
 
+struct pll45xx_freq_lookup {
+	unsigned long	target_freq;
+	unsigned long	pdiv;
+	unsigned long	mdiv;
+	unsigned long	sdiv;
+};
+
+#define PLL45XX_PMS(f, p, m, s)		\
+	{					\
+		.target_freq	= f,		\
+		.pdiv		= p,		\
+		.mdiv		= m,		\
+		.sdiv		= s,		\
+	}
+
 enum pll46xx_type {
 	pll_4600,
 	pll_4650,
-- 
1.7.5.4

[PATCH] clk: samsung: add set_rate and round_rate callbacks for pll45xx

[Tomasz Figa]

Hi Thomas,

On Thursday 11 of July 2013 14:57:49 Thomas Abraham wrote:
> Add support for set_rate and round_rate callbacks for pll45xx pll. This
> allows configuring pll45xx to generate a desired clock output.
> 
> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> ---
> 
> The set_rate and round_rate callbacks in this patch for pll45xx are handled
> slightly differently from the way it is done in the (not yet merged) patch
> series from Yadwinder
> (http://www.mail-archive.com/linux-samsung-soc at vger.kernel.org/msg19540.html
> )

I think this series should be considered as already merged. All the patches 
have been already acked and are just waiting to be picked up after 3.11-rc1 
gets released.

> 
> In this patch, the pll lookup table is kept as part of the pll configuration
> code itself, since the pll is just a hardware block which takes an input
> frequency and configuration values to genertate a output clock frequency.
> Given a set of inputs, a pll of a given type will generate the same output
> regardless of the SoC it is used in.

Are you sure about this? I've seen different sets of PMS(K) settings across 
different SoCs with the same PLL blocks.

> So instead of supplying the pll lookup
> table from per-soc clock driver code (as done in the Yadwinder's patch
> series), the pll lookup table can be coupled with the pll code itself,
> saving duplication of pll lookup table for every SoC the pll is used with.

Yes, that would be nice to have, but we already discussed this problem a lot 
and found out that this has to be specified on per-SoC basis.

>  drivers/clk/samsung/clk-pll.c |   88
> +++++++++++++++++++++++++++++++++++++++++ drivers/clk/samsung/clk-pll.h |  
> 15 +++++++
>  2 files changed, 103 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
> index 362f12d..4940936 100644
> --- a/drivers/clk/samsung/clk-pll.c
> +++ b/drivers/clk/samsung/clk-pll.c
> @@ -172,6 +172,8 @@ struct clk * __init samsung_clk_register_pll36xx(const
> char *name, * PLL45xx Clock Type
>   */
> 
> +#define PLL45XX_EN_MASK		(1 << 31)
> +#define PLL45XX_LOCK_MASK	(1 << 29)
>  #define PLL45XX_MDIV_MASK	(0x3FF)
>  #define PLL45XX_PDIV_MASK	(0x3F)
>  #define PLL45XX_SDIV_MASK	(0x7)
> @@ -187,6 +189,90 @@ struct samsung_clk_pll45xx {
> 
>  #define to_clk_pll45xx(_hw) container_of(_hw, struct samsung_clk_pll45xx,
> hw)
> 
> +/* a sorted table of freq supported by pll45xx with 24mhz parent clock */
> +static struct pll45xx_freq_lookup pll45xx_freq_lookup_24mhz[] = {
> +	PLL45XX_PMS(1000000000, 6, 250, 1),
> +	PLL45XX_PMS(800000000, 6, 200, 1),
> +	PLL45XX_PMS(500000000, 6, 250, 2),
> +	PLL45XX_PMS(400000000, 6, 200, 2),
> +	PLL45XX_PMS(200000000, 6, 200, 3),
> +};
> +
> +static int samsung_pll45xx_set_rate(struct clk_hw *hw, unsigned long rate,
> +					unsigned long prate)
> +{
> +	struct samsung_clk_pll45xx *pll = to_clk_pll45xx(hw);
> +	struct pll45xx_freq_lookup *f;
> +	unsigned long timeout, pll_con, cnt, idx;
> +
> +	/* select a lookup table based on parent clock frequency */
> +	switch (prate) {
> +	case 24000000:
> +		f = pll45xx_freq_lookup_24mhz;
> +		cnt = ARRAY_SIZE(pll45xx_freq_lookup_24mhz);
> +		break;

Do you really need to check the input frequency every time? I think you could 
just set up appropriate rate table in register function, like it is done in 
Yadwinder's patches.

> +	default:
> +		pr_err("%s: unsupported parent clock rate, failed to set rate",
> +					__func__);
> +		return -EINVAL;
> +	}
> +
> +	/* check if the requested freq is in the list of supported freq */
> +	for (idx = 0; idx < cnt; idx++, f++)
> +		if (f->target_freq == rate)
> +			break;

You don't have to check all the entries if you keep the PMS table sorted. You 
just have to look for first entry that has frequency less or equal to requested 
one.

> +
> +	if (idx == cnt) {
> +		pr_err("%s: unspported clock speed %ld requested\n",
> +				__func__, rate);
> +		return -EINVAL;
> +	}
> +
> +	/* first, disable the output of the pll */
> +	writel(readl(pll->con_reg) & ~PLL45XX_EN_MASK, (void *)pll->con_reg);
> +
> +	/* write the new pll configuration values */
> +	pll_con = (f->pdiv << PLL45XX_PDIV_SHIFT) |
> +			(f->mdiv << PLL45XX_MDIV_SHIFT) |
> +			(f->sdiv << PLL45XX_SDIV_SHIFT);
> +	writel(pll_con, (void *)pll->con_reg);
> +
> +	/* enable the pll and wait for it to stabilize */
> +	writel(pll_con | PLL45XX_EN_MASK, (void *)pll->con_reg);
> +	timeout = jiffies + msecs_to_jiffies(20);
> +	while (time_before(jiffies, timeout))
> +		if (readl(pll->con_reg) & PLL45XX_LOCK_MASK)
> +			return 0;
> +	return -EBUSY;
> +}
> +
> +static long samsung_pll45xx_round_rate(struct clk_hw *hw, unsigned long
> rate, +						unsigned long *prate)
> +{
> +	struct pll45xx_freq_lookup *f;
> +	unsigned long cnt;
> +
> +	/* select a lookup table based on parent clock frequency */
> +	switch (*prate) {
> +	case 24000000:
> +		f = pll45xx_freq_lookup_24mhz;
> +		cnt = ARRAY_SIZE(pll45xx_freq_lookup_24mhz);
> +		break;
> +	default:
> +		pr_err("%s: unsupported parent clock rate", __func__);
> +		return *prate;
> +	}

Again, PMS table can be set in register function and just used here.

> +	/* find the nearest possible clock output that can be supported */
> +	while (cnt-- > 0) {
> +		if (rate >= f->target_freq)
> +			return f->target_freq;
> +		f++;
> +	}
> +
> +	return (--f)->target_freq;
> +}
> +
>  static unsigned long samsung_pll45xx_recalc_rate(struct clk_hw *hw,
>  				unsigned long parent_rate)
>  {
> @@ -209,6 +295,8 @@ static unsigned long samsung_pll45xx_recalc_rate(struct
> clk_hw *hw, }
> 
>  static const struct clk_ops samsung_pll45xx_clk_ops = {
> +	.set_rate = samsung_pll45xx_set_rate,
> +	.round_rate = samsung_pll45xx_round_rate,
>  	.recalc_rate = samsung_pll45xx_recalc_rate,
>  };
> 
> diff --git a/drivers/clk/samsung/clk-pll.h b/drivers/clk/samsung/clk-pll.h
> index f33786e..fb687ec 100644
> --- a/drivers/clk/samsung/clk-pll.h
> +++ b/drivers/clk/samsung/clk-pll.h
> @@ -18,6 +18,21 @@ enum pll45xx_type {
>  	pll_4508
>  };
> 
> +struct pll45xx_freq_lookup {
> +	unsigned long	target_freq;
> +	unsigned long	pdiv;
> +	unsigned long	mdiv;
> +	unsigned long	sdiv;
> +};
> +
> +#define PLL45XX_PMS(f, p, m, s)		\
> +	{					\
> +		.target_freq	= f,		\
> +		.pdiv		= p,		\
> +		.mdiv		= m,		\
> +		.sdiv		= s,		\
> +	}
> +
>  enum pll46xx_type {
>  	pll_4600,
>  	pll_4650,

Basically, I would prefer the way introduced by Yadwinder's and Vikas' patches 
to be used. We already discussed all the aspects during all the 7 versions of 
those patches and decided to go with that solution, so for the case of 
consistency, same should be used for remaining PLLs.

Best regards,
Tomasz

[PATCH] clk: samsung: add set_rate and round_rate callbacks for pll45xx

[Yadwinder Singh Brar]

Hi Thomas,

On Thu, Jul 11, 2013 at 2:57 PM, Thomas Abraham
<thomas.abraham@linaro.org> wrote:
> Add support for set_rate and round_rate callbacks for pll45xx pll. This allows
> configuring pll45xx to generate a desired clock output.
>
> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
> ---
>
> The set_rate and round_rate callbacks in this patch for pll45xx are handled
> slightly differently from the way it is done in the (not yet merged) patch
> series from Yadwinder
> (http://www.mail-archive.com/linux-samsung-soc at vger.kernel.org/msg19540.html)
>
> In this patch, the pll lookup table is kept as part of the pll configuration
> code itself, since the pll is just a hardware block which takes an input
> frequency and configuration values to genertate a output clock frequency.
> Given a set of inputs, a pll of a given type will generate the same output
> regardless of the SoC it is used in. So instead of supplying the pll lookup

Tomasz also raised similar point earlier in discussion on that series.
That time a question which remained unanswered for which we were waiting
(as requested from hardware guys also) was that :
Whether we need to stick to recommended values only or we can drive a
particular output frequency with a given input frequency with any possible
set of (P, M, S) values ?

Theoretically it seems possible but from manual its seems to prefer
recommended values.
Also I am not sure whether same values are always recommended in user manuals
for different SoCs using same PLL type.

> table from per-soc clock driver code (as done in the Yadwinder's patch series),
> the pll lookup table can be coupled with the pll code itself, saving duplication
> of pll lookup table for every SoC the pll is used with.

If we don't need to stick to recommended table, definitely it will
save lot of duplication.
In-fact then we can use same pll lookup table for other PLLs also
which have same calculation
equation for output frequency.


Regards,
Yadwinder

>
>  drivers/clk/samsung/clk-pll.c |   88 +++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/samsung/clk-pll.h |   15 +++++++
>  2 files changed, 103 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
> index 362f12d..4940936 100644
> --- a/drivers/clk/samsung/clk-pll.c
> +++ b/drivers/clk/samsung/clk-pll.c
> @@ -172,6 +172,8 @@ struct clk * __init samsung_clk_register_pll36xx(const char *name,
>   * PLL45xx Clock Type
>   */
>
> +#define PLL45XX_EN_MASK                (1 << 31)
> +#define PLL45XX_LOCK_MASK      (1 << 29)
>  #define PLL45XX_MDIV_MASK      (0x3FF)
>  #define PLL45XX_PDIV_MASK      (0x3F)
>  #define PLL45XX_SDIV_MASK      (0x7)
> @@ -187,6 +189,90 @@ struct samsung_clk_pll45xx {
>
>  #define to_clk_pll45xx(_hw) container_of(_hw, struct samsung_clk_pll45xx, hw)
>
> +/* a sorted table of freq supported by pll45xx with 24mhz parent clock */
> +static struct pll45xx_freq_lookup pll45xx_freq_lookup_24mhz[] = {
> +       PLL45XX_PMS(1000000000, 6, 250, 1),
> +       PLL45XX_PMS(800000000, 6, 200, 1),
> +       PLL45XX_PMS(500000000, 6, 250, 2),
> +       PLL45XX_PMS(400000000, 6, 200, 2),
> +       PLL45XX_PMS(200000000, 6, 200, 3),
> +};
> +
> +static int samsung_pll45xx_set_rate(struct clk_hw *hw, unsigned long rate,
> +                                       unsigned long prate)
> +{
> +       struct samsung_clk_pll45xx *pll = to_clk_pll45xx(hw);
> +       struct pll45xx_freq_lookup *f;
> +       unsigned long timeout, pll_con, cnt, idx;
> +
> +       /* select a lookup table based on parent clock frequency */
> +       switch (prate) {
> +       case 24000000:
> +               f = pll45xx_freq_lookup_24mhz;
> +               cnt = ARRAY_SIZE(pll45xx_freq_lookup_24mhz);
> +               break;
> +       default:
> +               pr_err("%s: unsupported parent clock rate, failed to set rate",
> +                                       __func__);
> +               return -EINVAL;
> +       }
> +
> +       /* check if the requested freq is in the list of supported freq */
> +       for (idx = 0; idx < cnt; idx++, f++)
> +               if (f->target_freq == rate)
> +                       break;
> +
> +       if (idx == cnt) {
> +               pr_err("%s: unspported clock speed %ld requested\n",
> +                               __func__, rate);
> +               return -EINVAL;
> +       }
> +
> +       /* first, disable the output of the pll */
> +       writel(readl(pll->con_reg) & ~PLL45XX_EN_MASK, (void *)pll->con_reg);
> +
> +       /* write the new pll configuration values */
> +       pll_con = (f->pdiv << PLL45XX_PDIV_SHIFT) |
> +                       (f->mdiv << PLL45XX_MDIV_SHIFT) |
> +                       (f->sdiv << PLL45XX_SDIV_SHIFT);
> +       writel(pll_con, (void *)pll->con_reg);
> +
> +       /* enable the pll and wait for it to stabilize */
> +       writel(pll_con | PLL45XX_EN_MASK, (void *)pll->con_reg);
> +       timeout = jiffies + msecs_to_jiffies(20);
> +       while (time_before(jiffies, timeout))
> +               if (readl(pll->con_reg) & PLL45XX_LOCK_MASK)
> +                       return 0;
> +       return -EBUSY;
> +}
> +
> +static long samsung_pll45xx_round_rate(struct clk_hw *hw, unsigned long rate,
> +                                               unsigned long *prate)
> +{
> +       struct pll45xx_freq_lookup *f;
> +       unsigned long cnt;
> +
> +       /* select a lookup table based on parent clock frequency */
> +       switch (*prate) {
> +       case 24000000:
> +               f = pll45xx_freq_lookup_24mhz;
> +               cnt = ARRAY_SIZE(pll45xx_freq_lookup_24mhz);
> +               break;
> +       default:
> +               pr_err("%s: unsupported parent clock rate", __func__);
> +               return *prate;
> +       }
> +
> +       /* find the nearest possible clock output that can be supported */
> +       while (cnt-- > 0) {
> +               if (rate >= f->target_freq)
> +                       return f->target_freq;
> +               f++;
> +       }
> +
> +       return (--f)->target_freq;
> +}
> +
>  static unsigned long samsung_pll45xx_recalc_rate(struct clk_hw *hw,
>                                 unsigned long parent_rate)
>  {
> @@ -209,6 +295,8 @@ static unsigned long samsung_pll45xx_recalc_rate(struct clk_hw *hw,
>  }
>
>  static const struct clk_ops samsung_pll45xx_clk_ops = {
> +       .set_rate = samsung_pll45xx_set_rate,
> +       .round_rate = samsung_pll45xx_round_rate,
>         .recalc_rate = samsung_pll45xx_recalc_rate,
>  };
>
> diff --git a/drivers/clk/samsung/clk-pll.h b/drivers/clk/samsung/clk-pll.h
> index f33786e..fb687ec 100644
> --- a/drivers/clk/samsung/clk-pll.h
> +++ b/drivers/clk/samsung/clk-pll.h
> @@ -18,6 +18,21 @@ enum pll45xx_type {
>         pll_4508
>  };
>
> +struct pll45xx_freq_lookup {
> +       unsigned long   target_freq;
> +       unsigned long   pdiv;
> +       unsigned long   mdiv;
> +       unsigned long   sdiv;
> +};
> +
> +#define PLL45XX_PMS(f, p, m, s)                \
> +       {                                       \
> +               .target_freq    = f,            \
> +               .pdiv           = p,            \
> +               .mdiv           = m,            \
> +               .sdiv           = s,            \
> +       }
> +
>  enum pll46xx_type {
>         pll_4600,
>         pll_4650,
> --
> 1.7.5.4
>

[PATCH] clk: samsung: add set_rate and round_rate callbacks for pll45xx

[Thomas Abraham]

Hi Tomasz,

Thanks for reviewing the patch. I have few comments inline below.

On 11 July 2013 15:49, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi Thomas,
>
> On Thursday 11 of July 2013 14:57:49 Thomas Abraham wrote:
>> Add support for set_rate and round_rate callbacks for pll45xx pll. This
>> allows configuring pll45xx to generate a desired clock output.
>>
>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>> ---
>>
>> The set_rate and round_rate callbacks in this patch for pll45xx are handled
>> slightly differently from the way it is done in the (not yet merged) patch
>> series from Yadwinder
>> (http://www.mail-archive.com/linux-samsung-soc at vger.kernel.org/msg19540.html
>> )
>
> I think this series should be considered as already merged. All the patches
> have been already acked and are just waiting to be picked up after 3.11-rc1
> gets released.

Ok.

>
>>
>> In this patch, the pll lookup table is kept as part of the pll configuration
>> code itself, since the pll is just a hardware block which takes an input
>> frequency and configuration values to genertate a output clock frequency.
>> Given a set of inputs, a pll of a given type will generate the same output
>> regardless of the SoC it is used in.
>
> Are you sure about this? I've seen different sets of PMS(K) settings across
> different SoCs with the same PLL blocks.

Looking at Exynos4412 and Exynos5250 user manuals which uses PLL35xx,
there is only one entry of 1.2GHz which have differing values for
recommended PMS value. I am not sure if there is any issue in using a
common PMS value for 1.2Ghz for both Exynos4412 and Exynos5250.

>
>> So instead of supplying the pll lookup
>> table from per-soc clock driver code (as done in the Yadwinder's patch
>> series), the pll lookup table can be coupled with the pll code itself,
>> saving duplication of pll lookup table for every SoC the pll is used with.
>
> Yes, that would be nice to have, but we already discussed this problem a lot
> and found out that this has to be specified on per-SoC basis.

Ok.

>
>>  drivers/clk/samsung/clk-pll.c |   88
>> +++++++++++++++++++++++++++++++++++++++++ drivers/clk/samsung/clk-pll.h |
>> 15 +++++++
>>  2 files changed, 103 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
>> index 362f12d..4940936 100644
>> --- a/drivers/clk/samsung/clk-pll.c
>> +++ b/drivers/clk/samsung/clk-pll.c
>> @@ -172,6 +172,8 @@ struct clk * __init samsung_clk_register_pll36xx(const
>> char *name, * PLL45xx Clock Type
>>   */
>>
>> +#define PLL45XX_EN_MASK              (1 << 31)
>> +#define PLL45XX_LOCK_MASK    (1 << 29)
>>  #define PLL45XX_MDIV_MASK    (0x3FF)
>>  #define PLL45XX_PDIV_MASK    (0x3F)
>>  #define PLL45XX_SDIV_MASK    (0x7)
>> @@ -187,6 +189,90 @@ struct samsung_clk_pll45xx {
>>
>>  #define to_clk_pll45xx(_hw) container_of(_hw, struct samsung_clk_pll45xx,
>> hw)
>>
>> +/* a sorted table of freq supported by pll45xx with 24mhz parent clock */
>> +static struct pll45xx_freq_lookup pll45xx_freq_lookup_24mhz[] = {
>> +     PLL45XX_PMS(1000000000, 6, 250, 1),
>> +     PLL45XX_PMS(800000000, 6, 200, 1),
>> +     PLL45XX_PMS(500000000, 6, 250, 2),
>> +     PLL45XX_PMS(400000000, 6, 200, 2),
>> +     PLL45XX_PMS(200000000, 6, 200, 3),
>> +};
>> +
>> +static int samsung_pll45xx_set_rate(struct clk_hw *hw, unsigned long rate,
>> +                                     unsigned long prate)
>> +{
>> +     struct samsung_clk_pll45xx *pll = to_clk_pll45xx(hw);
>> +     struct pll45xx_freq_lookup *f;
>> +     unsigned long timeout, pll_con, cnt, idx;
>> +
>> +     /* select a lookup table based on parent clock frequency */
>> +     switch (prate) {
>> +     case 24000000:
>> +             f = pll45xx_freq_lookup_24mhz;
>> +             cnt = ARRAY_SIZE(pll45xx_freq_lookup_24mhz);
>> +             break;
>
> Do you really need to check the input frequency every time? I think you could
> just set up appropriate rate table in register function, like it is done in
> Yadwinder's patches.

There is no assumption made about the parent clock speed of the PLL.
The PLL might have been re-parented in which case the parent's clock
frequency would be different.

>
>> +     default:
>> +             pr_err("%s: unsupported parent clock rate, failed to set rate",
>> +                                     __func__);
>> +             return -EINVAL;
>> +     }
>> +
>> +     /* check if the requested freq is in the list of supported freq */
>> +     for (idx = 0; idx < cnt; idx++, f++)
>> +             if (f->target_freq == rate)
>> +                     break;
>
> You don't have to check all the entries if you keep the PMS table sorted. You
> just have to look for first entry that has frequency less or equal to requested
> one.

Yes, the lookup table above is sorted in descending order and this
lookup code is aware of the order and works as you described.

>
>> +
>> +     if (idx == cnt) {
>> +             pr_err("%s: unspported clock speed %ld requested\n",
>> +                             __func__, rate);
>> +             return -EINVAL;
>> +     }
>> +
>> +     /* first, disable the output of the pll */
>> +     writel(readl(pll->con_reg) & ~PLL45XX_EN_MASK, (void *)pll->con_reg);
>> +
>> +     /* write the new pll configuration values */
>> +     pll_con = (f->pdiv << PLL45XX_PDIV_SHIFT) |
>> +                     (f->mdiv << PLL45XX_MDIV_SHIFT) |
>> +                     (f->sdiv << PLL45XX_SDIV_SHIFT);
>> +     writel(pll_con, (void *)pll->con_reg);
>> +
>> +     /* enable the pll and wait for it to stabilize */
>> +     writel(pll_con | PLL45XX_EN_MASK, (void *)pll->con_reg);
>> +     timeout = jiffies + msecs_to_jiffies(20);
>> +     while (time_before(jiffies, timeout))
>> +             if (readl(pll->con_reg) & PLL45XX_LOCK_MASK)
>> +                     return 0;
>> +     return -EBUSY;
>> +}
>> +
>> +static long samsung_pll45xx_round_rate(struct clk_hw *hw, unsigned long
>> rate, +                                               unsigned long *prate)
>> +{
>> +     struct pll45xx_freq_lookup *f;
>> +     unsigned long cnt;
>> +
>> +     /* select a lookup table based on parent clock frequency */
>> +     switch (*prate) {
>> +     case 24000000:
>> +             f = pll45xx_freq_lookup_24mhz;
>> +             cnt = ARRAY_SIZE(pll45xx_freq_lookup_24mhz);
>> +             break;
>> +     default:
>> +             pr_err("%s: unsupported parent clock rate", __func__);
>> +             return *prate;
>> +     }
>
> Again, PMS table can be set in register function and just used here.

Ok, but it would be good to give another thought about having the
lookup table tied to the pll configuration code. If required, we could
have both options implemented. A pll will have a default PMS lookup
table here and if a SoC specific table is needed, it can be supplied
at the time of PLL registration.

>
>> +     /* find the nearest possible clock output that can be supported */
>> +     while (cnt-- > 0) {
>> +             if (rate >= f->target_freq)
>> +                     return f->target_freq;
>> +             f++;
>> +     }
>> +
>> +     return (--f)->target_freq;
>> +}
>> +
>>  static unsigned long samsung_pll45xx_recalc_rate(struct clk_hw *hw,
>>                               unsigned long parent_rate)
>>  {
>> @@ -209,6 +295,8 @@ static unsigned long samsung_pll45xx_recalc_rate(struct
>> clk_hw *hw, }
>>
>>  static const struct clk_ops samsung_pll45xx_clk_ops = {
>> +     .set_rate = samsung_pll45xx_set_rate,
>> +     .round_rate = samsung_pll45xx_round_rate,
>>       .recalc_rate = samsung_pll45xx_recalc_rate,
>>  };
>>
>> diff --git a/drivers/clk/samsung/clk-pll.h b/drivers/clk/samsung/clk-pll.h
>> index f33786e..fb687ec 100644
>> --- a/drivers/clk/samsung/clk-pll.h
>> +++ b/drivers/clk/samsung/clk-pll.h
>> @@ -18,6 +18,21 @@ enum pll45xx_type {
>>       pll_4508
>>  };
>>
>> +struct pll45xx_freq_lookup {
>> +     unsigned long   target_freq;
>> +     unsigned long   pdiv;
>> +     unsigned long   mdiv;
>> +     unsigned long   sdiv;
>> +};
>> +
>> +#define PLL45XX_PMS(f, p, m, s)              \
>> +     {                                       \
>> +             .target_freq    = f,            \
>> +             .pdiv           = p,            \
>> +             .mdiv           = m,            \
>> +             .sdiv           = s,            \
>> +     }
>> +
>>  enum pll46xx_type {
>>       pll_4600,
>>       pll_4650,
>
> Basically, I would prefer the way introduced by Yadwinder's and Vikas' patches
> to be used. We already discussed all the aspects during all the 7 versions of
> those patches and decided to go with that solution, so for the case of
> consistency, same should be used for remaining PLLs.

Ok, but could we look at this once more. We could avoid duplication of
PLL tables if the lookup tables are kept generic. It would be nice to
get opinions from others as well on this.

>
> Best regards,
> Tomasz
>

Thanks,
Thomas.

[PATCH] clk: samsung: add set_rate and round_rate callbacks for pll45xx

[Thomas Abraham]

Hi Yadwinder,

Thanks for your comments.

On 11 July 2013 16:14, Yadwinder Singh Brar <yadi.brar01@gmail.com> wrote:
> Hi Thomas,
>
> On Thu, Jul 11, 2013 at 2:57 PM, Thomas Abraham
> <thomas.abraham@linaro.org> wrote:
>> Add support for set_rate and round_rate callbacks for pll45xx pll. This allows
>> configuring pll45xx to generate a desired clock output.
>>
>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>> ---
>>
>> The set_rate and round_rate callbacks in this patch for pll45xx are handled
>> slightly differently from the way it is done in the (not yet merged) patch
>> series from Yadwinder
>> (http://www.mail-archive.com/linux-samsung-soc at vger.kernel.org/msg19540.html)
>>
>> In this patch, the pll lookup table is kept as part of the pll configuration
>> code itself, since the pll is just a hardware block which takes an input
>> frequency and configuration values to genertate a output clock frequency.
>> Given a set of inputs, a pll of a given type will generate the same output
>> regardless of the SoC it is used in. So instead of supplying the pll lookup
>
> Tomasz also raised similar point earlier in discussion on that series.
> That time a question which remained unanswered for which we were waiting
> (as requested from hardware guys also) was that :
> Whether we need to stick to recommended values only or we can drive a
> particular output frequency with a given input frequency with any possible
> set of (P, M, S) values ?

The default lookup table could list all the possible target frequency
and its associated PMS values. Having a bigger list of PMS lookup
values should not matter since the CPUFreq driver would have a smaller
subset of frequency which can be selected on a particular SoC.

>
> Theoretically it seems possible but from manual its seems to prefer
> recommended values.
> Also I am not sure whether same values are always recommended in user manuals
> for different SoCs using same PLL type.
>
>> table from per-soc clock driver code (as done in the Yadwinder's patch series),
>> the pll lookup table can be coupled with the pll code itself, saving duplication
>> of pll lookup table for every SoC the pll is used with.
>
> If we don't need to stick to recommended table, definitely it will
> save lot of duplication.
> In-fact then we can use same pll lookup table for other PLLs also
> which have same calculation
> equation for output frequency.

Ok.

>
>
> Regards,
> Yadwinder

Thanks,
Thomas.

[...]

[PATCH] clk: samsung: add set_rate and round_rate callbacks for pll45xx

[Yadwinder Singh Brar]

On Thu, Jul 11, 2013 at 8:54 PM, Thomas Abraham
<thomas.abraham@linaro.org> wrote:
> Hi Yadwinder,
>
> Thanks for your comments.
>
> On 11 July 2013 16:14, Yadwinder Singh Brar <yadi.brar01@gmail.com> wrote:
>> Hi Thomas,
>>
>> On Thu, Jul 11, 2013 at 2:57 PM, Thomas Abraham
>> <thomas.abraham@linaro.org> wrote:
>>> Add support for set_rate and round_rate callbacks for pll45xx pll. This allows
>>> configuring pll45xx to generate a desired clock output.
>>>
>>> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org>
>>> ---
>>>
>>> The set_rate and round_rate callbacks in this patch for pll45xx are handled
>>> slightly differently from the way it is done in the (not yet merged) patch
>>> series from Yadwinder
>>> (http://www.mail-archive.com/linux-samsung-soc at vger.kernel.org/msg19540.html)
>>>
>>> In this patch, the pll lookup table is kept as part of the pll configuration
>>> code itself, since the pll is just a hardware block which takes an input
>>> frequency and configuration values to genertate a output clock frequency.
>>> Given a set of inputs, a pll of a given type will generate the same output
>>> regardless of the SoC it is used in. So instead of supplying the pll lookup
>>
>> Tomasz also raised similar point earlier in discussion on that series.
>> That time a question which remained unanswered for which we were waiting
>> (as requested from hardware guys also) was that :
>> Whether we need to stick to recommended values only or we can drive a
>> particular output frequency with a given input frequency with any possible
>> set of (P, M, S) values ?
>
> The default lookup table could list all the possible target frequency
> and its associated PMS values. Having a bigger list of PMS lookup

same target frequency can have different associated PMS values recommended
in manual. So the question was in that case whether to stick to
recommended values.

> values should not matter since the CPUFreq driver would have a smaller
> subset of frequency which can be selected on a particular SoC.
>
>>
>> Theoretically it seems possible but from manual its seems to prefer
>> recommended values.
>> Also I am not sure whether same values are always recommended in user manuals
>> for different SoCs using same PLL type.
>>
> [...]

Regards,
Yadwinder

[PATCH] clk: samsung: add set_rate and round_rate callbacks for pll45xx

[Yadwinder Singh Brar]

Hi Thomas,

>>> +     /* select a lookup table based on parent clock frequency */
>>> +     switch (*prate) {
>>> +     case 24000000:
>>> +             f = pll45xx_freq_lookup_24mhz;
>>> +             cnt = ARRAY_SIZE(pll45xx_freq_lookup_24mhz);
>>> +             break;
>>> +     default:
>>> +             pr_err("%s: unsupported parent clock rate", __func__);
>>> +             return *prate;
>>> +     }
>>
>> Again, PMS table can be set in register function and just used here.
>
> Ok, but it would be good to give another thought about having the
> lookup table tied to the pll configuration code. If required, we could
> have both options implemented. A pll will have a default PMS lookup
> table here and if a SoC specific table is needed, it can be supplied
> at the time of PLL registration.
>

I agree with this approach.
But I am not sure actually how many SoC can have common  table.

Regards,
Yadwinder

[PATCH] clk: samsung: add set_rate and round_rate callbacks for pll45xx

[Yadwinder Singh Brar]

Sorry I missed to state a point in earlier reply..,


>>> +#define PLL45XX_PMS(f, p, m, s)              \
>>> +     {                                       \
>>> +             .target_freq    = f,            \
>>> +             .pdiv           = p,            \
>>> +             .mdiv           = m,            \
>>> +             .sdiv           = s,            \
>>> +     }
>>> +
>>>  enum pll46xx_type {
>>>       pll_4600,
>>>       pll_4650,
>>
>> Basically, I would prefer the way introduced by Yadwinder's and Vikas' patches
>> to be used. We already discussed all the aspects during all the 7 versions of
>> those patches and decided to go with that solution, so for the case of
>> consistency, same should be used for remaining PLLs.
>
> Ok, but could we look at this once more. We could avoid duplication of
> PLL tables if the lookup tables are kept generic. It would be nice to
> get opinions from others as well on this.
>

The rationale behind using common enum type was to use common(generic) set
of clk ops where ever it is possible, to avoid duplication of most of
the code(which
looks mostly common with many PLLs).  For example its done for 35xx & 25xx
and 36xx & 26xx PLLs in that series.

Regards,
Yadwinder

[PATCH] clk: samsung: add set_rate and round_rate callbacks for pll45xx

[Mike Turquette]

Quoting Yadwinder Singh Brar (2013-07-11 22:48:55)
> Sorry I missed to state a point in earlier reply..,
> 
> 
> >>> +#define PLL45XX_PMS(f, p, m, s)              \
> >>> +     {                                       \
> >>> +             .target_freq    = f,            \
> >>> +             .pdiv           = p,            \
> >>> +             .mdiv           = m,            \
> >>> +             .sdiv           = s,            \
> >>> +     }
> >>> +
> >>>  enum pll46xx_type {
> >>>       pll_4600,
> >>>       pll_4650,
> >>
> >> Basically, I would prefer the way introduced by Yadwinder's and Vikas' patches
> >> to be used. We already discussed all the aspects during all the 7 versions of
> >> those patches and decided to go with that solution, so for the case of
> >> consistency, same should be used for remaining PLLs.
> >
> > Ok, but could we look at this once more. We could avoid duplication of
> > PLL tables if the lookup tables are kept generic. It would be nice to
> > get opinions from others as well on this.
> >
> 
> The rationale behind using common enum type was to use common(generic) set
> of clk ops where ever it is possible, to avoid duplication of most of
> the code(which
> looks mostly common with many PLLs).  For example its done for 35xx & 25xx
> and 36xx & 26xx PLLs in that series.

Was there ever a consensus reached on the pll45xx round_rate & set_rate
implementation?

Regards,
Mike

> 
> Regards,
> Yadwinder