clk: samsung: add set_rate and round_rate callbacks for pll45xx
Subsystems (2)
PATCH 1/1 clk: samsung: add set_rate and round_rate callbacks for pll45xx
Add support for set_rate and round_rate callbacks for pll45xx pll. This allows configuring pll45xx to generate a desired clock output.
---
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(-)
drivers/clk/samsung/clk-pll.c +88 -0
@@ -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, };
drivers/clk/samsung/clk-pll.h +15 -0
@@ -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,
Signature · 1 line
1.7.5.4
PATCH 1/1 clk: samsung: add set_rate and round_rate callbacks for pll45xx
Hi Thomas,
Quoted 11 lines · On Thursday 11 of July 2013 14:57:49 Thomas Abraham wrote:
Add support for
set_rateandround_ratecallbacks for pll45xx pll. This allows configuring pll45xx to generate a desired clock output.Signed-off-by Thomas Abraham <[email protected]>---
The
set_rateandround_ratecallbacks 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.
Quoted 6 lines
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.
Quoted 4 lines
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.
Quoted 35 lines
drivers/clk/samsung/clk-pll.c | 88 +++++++++++++++++++++++++++++++++++++++++ drivers/clk/samsung/clk-pll.h | 15 +++++++ 2 files changed, 103 insertions(+), 0 deletions(-)drivers/clk/samsung/clk-pll.c +0
-0@@ -172,6 +172,8 @@ struct clk * __init samsung_clk_register_pll36xx(constchar *name, * PLL45xx Clock Type
*/drivers/clk/samsung/clk-pll.c +14
-0+#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) +{
drivers/clk/samsung/clk-pll.c struct samsung_clk_pll45xx { · line 214
Quoted 10 lines
drivers/clk/samsung/clk-pll.c +10
-0+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.
drivers/clk/samsung/clk-pll.c struct samsung_clk_pll45xx { · line 223
Quoted 10 lines
drivers/clk/samsung/clk-pll.c +10
-0+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.
Quoted 31 lines
drivers/clk/samsung/clk-pll.c +30
-0+ +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; +
drivers/clk/samsung/clk-pll.c struct samsung_clk_pll45xx { · line 264
Quoted 10 lines
drivers/clk/samsung/clk-pll.c +10
-0+/* 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.
Quoted 38 lines
drivers/clk/samsung/clk-pll.c +12
-0+/* 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, };drivers/clk/samsung/clk-pll.h +8
-0@@ -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) \
drivers/clk/samsung/clk-pll.h enum pll45xx_type { · line 38
Quoted 10 lines
drivers/clk/samsung/clk-pll.h +7
-0+ { \ + .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 1/1 clk: samsung: add set_rate and round_rate callbacks for pll45xx
Hi Tomasz,
Thanks for reviewing the patch. I have few comments inline below.
Quoted 15 lines · Tomasz Figa
Hi Thomas,
Quoted 11 lines · On Thursday 11 of July 2013 14:57:49 Thomas Abraham wrote:
Add support for
set_rateandround_ratecallbacks for pll45xx pll. This allows configuring pll45xx to generate a desired clock output.Signed-off-by Thomas Abraham <[email protected]>---
The
set_rateandround_ratecallbacks 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 )
Quoted 3 lines
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.
Quoted 10 lines
Quoted 7 lines
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.
Quoted 8 lines
Quoted 5 lines
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.
Quoted 42 lines
Quoted 42 lines
drivers/clk/samsung/clk-pll.c | 88 +++++++++++++++++++++++++++++++++++++++++ drivers/clk/samsung/clk-pll.h | 15 +++++++ 2 files changed, 103 insertions(+), 0 deletions(-)drivers/clk/samsung/clk-pll.c +0
-0@@ -172,6 +172,8 @@ struct clk * __init samsung_clk_register_pll36xx(constchar *name, * PLL45xx Clock Type
*/drivers/clk/samsung/clk-pll.c +20
-0+#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) {
Quoted 8 lines
drivers/clk/samsung/clk-pll.c struct samsung_clk_pll45xx { · line 213
Quoted 4 lines
drivers/clk/samsung/clk-pll.c +4
-0+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.
Quoted 12 lines
drivers/clk/samsung/clk-pll.c struct samsung_clk_pll45xx { · line 223
Quoted 11 lines
drivers/clk/samsung/clk-pll.c +10
-0+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 ;
Quoted 3 lines
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.
Quoted 36 lines
Quoted 36 lines
drivers/clk/samsung/clk-pll.c +34
-0+ +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;
Quoted 8 lines
drivers/clk/samsung/clk-pll.c struct samsung_clk_pll45xx { · line 264
Quoted 6 lines
drivers/clk/samsung/clk-pll.c +6
-0+ 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.
Quoted 46 lines
Quoted 46 lines
drivers/clk/samsung/clk-pll.c +12
-0+/* 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, };drivers/clk/samsung/clk-pll.h +15
-0@@ -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, \ + } +
Quoted 8 lines
drivers/clk/samsung/clk-pll.h enum pll45xx_type { · line 38
Quoted 3 lines
drivers/clk/samsung/clk-pll.h +0
-0enum 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.
Quoted 4 lines
Best regards,
Tomasz
Thanks,
Thomas.
PATCH 1/1 clk: samsung: add set_rate and round_rate callbacks for pll45xx
Hi Thomas,
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
drivers/clk/samsung/clk-pll.c +10
-0+/* 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; + }
Quoted 8 lines
Quoted 1 line
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 1/1 clk: samsung: add set_rate and round_rate callbacks for pll45xx
In reply to Thomas AbrahamSorry I missed to state a point in earlier reply..,
Quoted 17 lines
Quoted 8 lines
Quoted 8 lines
drivers/clk/samsung/clk-pll.h +8
-0+#define PLL45XX_PMS (f, p, m, s) \ + { \ + .target_freq = f, \ + .pdiv = p, \ + .mdiv = m, \ + .sdiv = s, \ + } +Quoted 8 lines
Quoted 3 lines
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.
Quoted 4 lines
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 1/1 clk: samsung: add set_rate and round_rate callbacks for pll45xx
Quoting Yadwinder Singh Brar (2013-07-11 22:48:55)
Quoted 25 lines
Sorry I missed to state a point in earlier reply..,
Quoted 17 lines
Quoted 8 lines
Quoted 8 lines
drivers/clk/samsung/clk-pll.h +8
-0+#define PLL45XX_PMS (f, p, m, s) \ + { \ + .target_freq = f, \ + .pdiv = p, \ + .mdiv = m, \ + .sdiv = s, \ + } +Quoted 8 lines
Quoted 3 lines
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.
Quoted 4 lines
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.
Quoted 5 lines
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
Quoted 3 lines
Regards,
Yadwinder
PATCH 1/1 clk: samsung: add set_rate and round_rate callbacks for pll45xx
In reply to Thomas AbrahamHi Thomas,
Quoted 11 lines · Thomas Abraham
Add support for
set_rateandround_ratecallbacks for pll45xx pll. This allows configuring pll45xx to generate a desired clock output.Signed-off-by Thomas Abraham <[email protected]>---
The
set_rateandround_ratecallbacks 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)
Quoted 5 lines
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.
Quoted 3 lines
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
Quoted 147 lines
drivers/clk/samsung/clk-pll.c | 88 +++++++++++++++++++++++++++++++++++++++++ drivers/clk/samsung/clk-pll.h | 15 +++++++ 2 files changed, 103 insertions(+), 0 deletions(-)drivers/clk/samsung/clk-pll.c +88
-0@@ -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, };drivers/clk/samsung/clk-pll.h +15
-1@@ -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 1/1 clk: samsung: add set_rate and round_rate callbacks for pll45xx
Hi Yadwinder,
Thanks for your comments.
Quoted 21 lines · Yadwinder Singh Brar
Hi Thomas,
Quoted 11 lines · Thomas Abraham
Add support for
set_rateandround_ratecallbacks for pll45xx pll. This allows configuring pll45xx to generate a desired clock output.Signed-off-by Thomas Abraham <[email protected]>---
The
set_rateandround_ratecallbacks 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)Quoted 5 lines
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
Quoted 6 lines
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.
Quoted 10 lines
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.Quoted 3 lines
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.
Quoted 5 lines
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.
Quoted 4 lines
Regards,
Yadwinder
Thanks,
Thomas.
[...]
PATCH 1/1 clk: samsung: add set_rate and round_rate callbacks for pll45xx
Quoted 33 lines · Thomas Abraham
Hi Yadwinder,
Thanks for your comments.
Quoted 21 lines · Yadwinder Singh Brar
Hi Thomas,
Quoted 11 lines · Thomas Abraham
Add support for
set_rateandround_ratecallbacks for pll45xx pll. This allows configuring pll45xx to generate a desired clock output.Signed-off-by Thomas Abraham <[email protected]>---
The
set_rateandround_ratecallbacks 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)Quoted 5 lines
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
Quoted 6 lines
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 ?
Quoted 2 lines
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.
Quoted 10 lines
values should not matter since the CPUFreq driver would have a smaller subset of frequency which can be selected on a particular SoC.
Quoted 6 lines
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