[PATCHv2 0/5] clk: samsung: exynos5410: Implementation of the PLL clocks

[PATCHv2 0/5] clk: samsung: exynos5410: Implementation of the PLL clocks

[Humberto Silva Naves]

Hi,

This patch series slightly improves the exynos5410 clock driver. Below is
a list of changes introduced by the patch:
 - Basic validation in the clock initialization routine
 - Added resume/suspend handler
 - Implemented some fixed rate clocks and changed the way "fin_pll" is defined
 - Added the remaining PLL clocks

Changelog since v1:
 * Split the reordering and addition of new constants  into two different
commits, as suggested by Thomas Abraham. For details, see:
http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg34950.html
 * The rate tables are only installed if the clock rate of the parent matches
a specific frequency of 24MHz. For details, please refer to:
http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg34953.html
 * Added some fixed rate clocks.

Humberto Silva Naves (5):
  clk: samsung: exynos5410: Add NULL pointer checks in clock init
  clk: samsung: exynos5410: Organize register offset constants
  clk: samsung: exynos5410: Add suspend/resume handling
  clk: samsung: exynos5410: Add fixed rate clocks
  clk: samsung: exynos5410: Added clocks DPLL, EPLL, IPLL, and VPLL

 .../devicetree/bindings/clock/exynos5410-clock.txt |   17 +-
 drivers/clk/samsung/clk-exynos5410.c               |  437 ++++++++++++++++++--
 include/dt-bindings/clock/exynos5410.h             |    5 +
 3 files changed, 430 insertions(+), 29 deletions(-)

-- 
1.7.10.4

[PATCHv2 1/5] clk: samsung: exynos5410: Add NULL pointer checks in clock init

[Humberto Silva Naves]

Added NULL pointer checks for device_node input parameter and
for the samsung_clk_provider context returned by samsung_clk_init.
Even though the *current* samsung_clk_init function never returns
a NULL pointer, it is good to keep this check in place to avoid
possible problems in the future due to changes in implementation.
That way, we also improve the consistency of the code that performs
clock initialization across the different SoCs.

Signed-off-by: Humberto Silva Naves <hsnaves@gmail.com>
---
 drivers/clk/samsung/clk-exynos5410.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos5410.c b/drivers/clk/samsung/clk-exynos5410.c
index 231475b..bf57c80 100644
--- a/drivers/clk/samsung/clk-exynos5410.c
+++ b/drivers/clk/samsung/clk-exynos5410.c
@@ -188,11 +188,17 @@ static void __init exynos5410_clk_init(struct device_node *np)
 	struct samsung_clk_provider *ctx;
 	void __iomem *reg_base;
 
-	reg_base = of_iomap(np, 0);
-	if (!reg_base)
-		panic("%s: failed to map registers\n", __func__);
+	if (np) {
+		reg_base = of_iomap(np, 0);
+		if (!reg_base)
+			panic("%s: failed to map registers\n", __func__);
+	} else {
+		panic("%s: unable to determine soc\n", __func__);
+	}
 
 	ctx = samsung_clk_init(np, reg_base, CLK_NR_CLKS);
+	if (!ctx)
+		panic("%s: unable to allocate context.\n", __func__);
 
 	samsung_clk_register_pll(ctx, exynos5410_plls,
 			ARRAY_SIZE(exynos5410_plls), reg_base);
-- 
1.7.10.4

Re: [PATCHv2 1/5] clk: samsung: exynos5410: Add NULL pointer checks in clock init

[Tomasz Figa]

Hi Humberto,

Please see my comments inline.

On 31.07.2014 13:22, Humberto Silva Naves wrote:
> Added NULL pointer checks for device_node input parameter and
> for the samsung_clk_provider context returned by samsung_clk_init.
> Even though the *current* samsung_clk_init function never returns
> a NULL pointer, it is good to keep this check in place to avoid
> possible problems in the future due to changes in implementation.
> That way, we also improve the consistency of the code that performs
> clock initialization across the different SoCs.
> 
> Signed-off-by: Humberto Silva Naves <hsnaves@gmail.com>
> ---
>  drivers/clk/samsung/clk-exynos5410.c |   12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/samsung/clk-exynos5410.c b/drivers/clk/samsung/clk-exynos5410.c
> index 231475b..bf57c80 100644
> --- a/drivers/clk/samsung/clk-exynos5410.c
> +++ b/drivers/clk/samsung/clk-exynos5410.c
> @@ -188,11 +188,17 @@ static void __init exynos5410_clk_init(struct device_node *np)
>  	struct samsung_clk_provider *ctx;
>  	void __iomem *reg_base;
>  
> -	reg_base = of_iomap(np, 0);
> -	if (!reg_base)
> -		panic("%s: failed to map registers\n", __func__);
> +	if (np) {

Since all Exynos-based boards are always booted using DT, this function
will never be called if there is no node for the clock controller and so
there is no way this pointer can end up being NULL. I don't see a point
in complicating this code with useless checks.

> +		reg_base = of_iomap(np, 0);
> +		if (!reg_base)
> +			panic("%s: failed to map registers\n", __func__);
> +	} else {
> +		panic("%s: unable to determine soc\n", __func__);
> +	}

As a side note, since panic() does not return, the code above could be
changed to follow rest of checks in this function:

	if (!np)
		panic("%s: unable to determine soc\n", __func__);

	reg_base = of_iomap(np, 0);
	...

leading to more readable code with less indentation and less changes to
existing code.

>  
>  	ctx = samsung_clk_init(np, reg_base, CLK_NR_CLKS);
> +	if (!ctx)
> +		panic("%s: unable to allocate context.\n", __func__);

samsung_clk_init() already panics on any error, although now as I think
of it, it probably should be changed with a patch to just error out and
let the caller handle the error. However callers don't need to be
changed before this is done.

Best regards,
Tomasz

Re: [PATCHv2 1/5] clk: samsung: exynos5410: Add NULL pointer checks in clock init

[Humberto Naves]

Hi,

I am bit confused by your response: first you mentioned that I should
remove the NULL check for variable np, but later on you suggested that
I should rearrange the conditional statement to avoid adding more
indentation. My guess is that I should remove that if statement
altogether?

Regarding the ctx variable, should I still remove the NULL check? As
you said, in the near future samsung_clk_init() won't panic anymore,
and keeping the check in place won't hurt anybody.

Best,
Humberto

On Thu, Jul 31, 2014 at 2:34 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi Humberto,
>
> Please see my comments inline.
>
> On 31.07.2014 13:22, Humberto Silva Naves wrote:
>> Added NULL pointer checks for device_node input parameter and
>> for the samsung_clk_provider context returned by samsung_clk_init.
>> Even though the *current* samsung_clk_init function never returns
>> a NULL pointer, it is good to keep this check in place to avoid
>> possible problems in the future due to changes in implementation.
>> That way, we also improve the consistency of the code that performs
>> clock initialization across the different SoCs.
>>
>> Signed-off-by: Humberto Silva Naves <hsnaves@gmail.com>
>> ---
>>  drivers/clk/samsung/clk-exynos5410.c |   12 +++++++++---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/clk/samsung/clk-exynos5410.c b/drivers/clk/samsung/clk-exynos5410.c
>> index 231475b..bf57c80 100644
>> --- a/drivers/clk/samsung/clk-exynos5410.c
>> +++ b/drivers/clk/samsung/clk-exynos5410.c
>> @@ -188,11 +188,17 @@ static void __init exynos5410_clk_init(struct device_node *np)
>>       struct samsung_clk_provider *ctx;
>>       void __iomem *reg_base;
>>
>> -     reg_base = of_iomap(np, 0);
>> -     if (!reg_base)
>> -             panic("%s: failed to map registers\n", __func__);
>> +     if (np) {
>
> Since all Exynos-based boards are always booted using DT, this function
> will never be called if there is no node for the clock controller and so
> there is no way this pointer can end up being NULL. I don't see a point
> in complicating this code with useless checks.
>
>> +             reg_base = of_iomap(np, 0);
>> +             if (!reg_base)
>> +                     panic("%s: failed to map registers\n", __func__);
>> +     } else {
>> +             panic("%s: unable to determine soc\n", __func__);
>> +     }
>
> As a side note, since panic() does not return, the code above could be
> changed to follow rest of checks in this function:
>
>         if (!np)
>                 panic("%s: unable to determine soc\n", __func__);
>
>         reg_base = of_iomap(np, 0);
>         ...
>
> leading to more readable code with less indentation and less changes to
> existing code.
>
>>
>>       ctx = samsung_clk_init(np, reg_base, CLK_NR_CLKS);
>> +     if (!ctx)
>> +             panic("%s: unable to allocate context.\n", __func__);
>
> samsung_clk_init() already panics on any error, although now as I think
> of it, it probably should be changed with a patch to just error out and
> let the caller handle the error. However callers don't need to be
> changed before this is done.
>
> Best regards,
> Tomasz

Re: [PATCHv2 1/5] clk: samsung: exynos5410: Add NULL pointer checks in clock init

[Tomasz Figa]

On 31.07.2014 15:13, Humberto Naves wrote:
> Hi,
> 
> I am bit confused by your response: first you mentioned that I should
> remove the NULL check for variable np, but later on you suggested that
> I should rearrange the conditional statement to avoid adding more
> indentation.

That was just a side note.

> My guess is that I should remove that if statement
> altogether?

Yes, that was my intention.

> 
> Regarding the ctx variable, should I still remove the NULL check? As
> you said, in the near future samsung_clk_init() won't panic anymore,
> and keeping the check in place won't hurt anybody.

The rule of thumb for kernel patches is that we want a patch if we know
that it is something we need. We don't know yet when and how (which
error returning convention, NULL or ERR_PTR() or maybe something else?)
samsung_clk_init() gets changed, so right now we shouldn't change its
callers.

Of course a patch changing samsung_clk_init() and all its callers in one
go will be welcome.

By the way, please avoid top posting. Here's a good read on Linux
mailing lists netiquette: http://www.tux.org/lkml/#s3 .

Best regards,
Tomasz

> 
> Best,
> Humberto
> 
> On Thu, Jul 31, 2014 at 2:34 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> Hi Humberto,
>>
>> Please see my comments inline.
>>
>> On 31.07.2014 13:22, Humberto Silva Naves wrote:
>>> Added NULL pointer checks for device_node input parameter and
>>> for the samsung_clk_provider context returned by samsung_clk_init.
>>> Even though the *current* samsung_clk_init function never returns
>>> a NULL pointer, it is good to keep this check in place to avoid
>>> possible problems in the future due to changes in implementation.
>>> That way, we also improve the consistency of the code that performs
>>> clock initialization across the different SoCs.
>>>
>>> Signed-off-by: Humberto Silva Naves <hsnaves@gmail.com>
>>> ---
>>>  drivers/clk/samsung/clk-exynos5410.c |   12 +++++++++---
>>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/clk/samsung/clk-exynos5410.c b/drivers/clk/samsung/clk-exynos5410.c
>>> index 231475b..bf57c80 100644
>>> --- a/drivers/clk/samsung/clk-exynos5410.c
>>> +++ b/drivers/clk/samsung/clk-exynos5410.c
>>> @@ -188,11 +188,17 @@ static void __init exynos5410_clk_init(struct device_node *np)
>>>       struct samsung_clk_provider *ctx;
>>>       void __iomem *reg_base;
>>>
>>> -     reg_base = of_iomap(np, 0);
>>> -     if (!reg_base)
>>> -             panic("%s: failed to map registers\n", __func__);
>>> +     if (np) {
>>
>> Since all Exynos-based boards are always booted using DT, this function
>> will never be called if there is no node for the clock controller and so
>> there is no way this pointer can end up being NULL. I don't see a point
>> in complicating this code with useless checks.
>>
>>> +             reg_base = of_iomap(np, 0);
>>> +             if (!reg_base)
>>> +                     panic("%s: failed to map registers\n", __func__);
>>> +     } else {
>>> +             panic("%s: unable to determine soc\n", __func__);
>>> +     }
>>
>> As a side note, since panic() does not return, the code above could be
>> changed to follow rest of checks in this function:
>>
>>         if (!np)
>>                 panic("%s: unable to determine soc\n", __func__);
>>
>>         reg_base = of_iomap(np, 0);
>>         ...
>>
>> leading to more readable code with less indentation and less changes to
>> existing code.
>>
>>>
>>>       ctx = samsung_clk_init(np, reg_base, CLK_NR_CLKS);
>>> +     if (!ctx)
>>> +             panic("%s: unable to allocate context.\n", __func__);
>>
>> samsung_clk_init() already panics on any error, although now as I think
>> of it, it probably should be changed with a patch to just error out and
>> let the caller handle the error. However callers don't need to be
>> changed before this is done.
>>
>> Best regards,
>> Tomasz

[PATCHv2 2/5] clk: samsung: exynos5410: Organize register offset constants

[Humberto Silva Naves]

The different register groups (SRC, DIV, PLL, GATE, etc) are
now separated by a blank line, and within the same group, the
definitions are ordered by address. This is done to reduce the
chances of potential conflicts when adding new entries, and
to improve the readability of code. While at it, replaced some
spaces with tabs to keep consistency.

Signed-off-by: Humberto Silva Naves <hsnaves@gmail.com>
---
 drivers/clk/samsung/clk-exynos5410.c |   42 +++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos5410.c b/drivers/clk/samsung/clk-exynos5410.c
index bf57c80..92c56b7 100644
--- a/drivers/clk/samsung/clk-exynos5410.c
+++ b/drivers/clk/samsung/clk-exynos5410.c
@@ -19,39 +19,43 @@
 
 #include "clk.h"
 
-#define APLL_LOCK               0x0
-#define APLL_CON0               0x100
-#define CPLL_LOCK               0x10020
-#define CPLL_CON0               0x10120
-#define MPLL_LOCK               0x4000
-#define MPLL_CON0               0x4100
-#define BPLL_LOCK               0x20010
-#define BPLL_CON0               0x20110
-#define KPLL_LOCK               0x28000
-#define KPLL_CON0               0x28100
+#define APLL_LOCK		0x0
+#define APLL_CON0		0x100
+#define MPLL_LOCK		0x4000
+#define MPLL_CON0		0x4100
+#define CPLL_LOCK		0x10020
+#define CPLL_CON0		0x10120
+#define BPLL_LOCK		0x20010
+#define BPLL_CON0		0x20110
+#define KPLL_LOCK		0x28000
+#define KPLL_CON0		0x28100
 
 #define SRC_CPU			0x200
-#define DIV_CPU0		0x500
 #define SRC_CPERI1		0x4204
-#define DIV_TOP0		0x10510
-#define DIV_TOP1		0x10514
-#define DIV_FSYS1		0x1054c
-#define DIV_FSYS2		0x10550
-#define DIV_PERIC0		0x10558
 #define SRC_TOP0		0x10210
 #define SRC_TOP1		0x10214
 #define SRC_TOP2		0x10218
 #define SRC_FSYS		0x10244
 #define SRC_PERIC0		0x10250
+#define SRC_CDREX		0x20200
+#define SRC_KFC			0x28200
+
 #define SRC_MASK_FSYS		0x10340
 #define SRC_MASK_PERIC0		0x10350
+
+#define DIV_CPU0		0x500
+#define DIV_TOP0		0x10510
+#define DIV_TOP1		0x10514
+#define DIV_FSYS1		0x1054c
+#define DIV_FSYS2		0x10550
+#define DIV_PERIC0		0x10558
+#define DIV_KFC0		0x28500
+
 #define GATE_BUS_FSYS0		0x10740
+
 #define GATE_IP_FSYS		0x10944
 #define GATE_IP_PERIC		0x10950
 #define GATE_IP_PERIS		0x10960
-#define SRC_CDREX		0x20200
-#define SRC_KFC			0x28200
-#define DIV_KFC0		0x28500
 
 /* list of PLLs */
 enum exynos5410_plls {
-- 
1.7.10.4

Re: [PATCHv2 2/5] clk: samsung: exynos5410: Organize register offset constants

[Tomasz Figa]

Hi Humberto,

Please see my comments inline.

On 31.07.2014 13:22, Humberto Silva Naves wrote:
> The different register groups (SRC, DIV, PLL, GATE, etc) are
> now separated by a blank line, and within the same group, the
> definitions are ordered by address. This is done to reduce the
> chances of potential conflicts when adding new entries, and
> to improve the readability of code. While at it, replaced some
> spaces with tabs to keep consistency.

I'm not sure whether this change really improves anything.

It might seem plausible to have the registers grouped by their purpose,
however remaining drivers have them directly listed in order of their
addresses to match the order they are mentioned in documentation. For
consistency, I'd prefer only one convention to be used across all
Samsung clock drivers, so they would have to be changed as well. But
IMHO this is a material for a separate clean-up series, while this one
should be limited to functional improvements.

In fact, this driver is kind of exception as it has PLL register
definitions separated, which I probably missed in review.

Best regards,
Tomasz

[PATCHv2 3/5] clk: samsung: exynos5410: Add suspend/resume handling

[Humberto Silva Naves]

This patch implements all the necessary code that handles register
saving and restoring during a suspend/resume cycle. To make this
possible, the local variable reg_base from the function
exynos5410_clk_init was changed to global. In addition, new
clock register definitions were added for the majority of the relevant
clocks inside the SoC.

Signed-off-by: Humberto Silva Naves <hsnaves@gmail.com>
---
 drivers/clk/samsung/clk-exynos5410.c |  231 +++++++++++++++++++++++++++++++++-
 1 file changed, 228 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos5410.c b/drivers/clk/samsung/clk-exynos5410.c
index 92c56b7..a9c261c 100644
--- a/drivers/clk/samsung/clk-exynos5410.c
+++ b/drivers/clk/samsung/clk-exynos5410.c
@@ -16,6 +16,7 @@
 #include <linux/clk-provider.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/syscore_ops.h>
 
 #include "clk.h"
 
@@ -24,38 +25,134 @@
 #define MPLL_LOCK		0x4000
 #define MPLL_CON0		0x4100
 #define CPLL_LOCK		0x10020
+#define DPLL_LOCK		0x10030
+#define EPLL_LOCK		0x10040
+#define VPLL_LOCK		0x10050
+#define IPLL_LOCK		0x10060
 #define CPLL_CON0		0x10120
+#define CPLL_CON1		0x10124
+#define DPLL_CON0		0x10128
+#define DPLL_CON1		0x1012C
+#define EPLL_CON0		0x10130
+#define EPLL_CON1		0x10134
+#define EPLL_CON2		0x10138
+#define VPLL_CON0		0x10140
+#define VPLL_CON1		0x10144
+#define VPLL_CON2		0x10148
+#define IPLL_CON0		0x10150
+#define IPLL_CON1		0x10154
 #define BPLL_LOCK		0x20010
 #define BPLL_CON0		0x20110
 #define KPLL_LOCK		0x28000
 #define KPLL_CON0		0x28100
 
 #define SRC_CPU			0x200
+#define SRC_CPERI0		0x4200
 #define SRC_CPERI1		0x4204
 #define SRC_TOP0		0x10210
 #define SRC_TOP1		0x10214
 #define SRC_TOP2		0x10218
+#define SRC_TOP3		0x1021C
+#define SRC_GSCL		0x10220
+#define SRC_DISP0_0		0x10224
+#define SRC_DISP0_1		0x10228
+#define SRC_DISP1_0		0x1022C
+#define SRC_DISP1_1		0x10230
+#define SRC_MAU			0x10240
 #define SRC_FSYS		0x10244
 #define SRC_PERIC0		0x10250
+#define SRC_PERIC1		0x10254
 #define SRC_CDREX		0x20200
 #define SRC_KFC			0x28200
 
+#define SRC_MASK_TOP		0x10310
+#define SRC_MASK_GSCL		0x10320
+#define SRC_MASK_DISP0_0	0x10324
+#define SRC_MASK_DISP0_1	0x10328
+#define SRC_MASK_DISP1_0	0x1032C
+#define SRC_MASK_DISP1_1	0x10330
+#define SRC_MASK_MAU		0x10334
 #define SRC_MASK_FSYS		0x10340
+#define SRC_MASK_GEN		0x10344
 #define SRC_MASK_PERIC0		0x10350
+#define SRC_MASK_PERIC1		0x10354
 
 #define DIV_CPU0		0x500
+#define DIV_CPU1		0x504
+#define DIV_CPERI0		0x4500
+#define DIV_CPERI1		0x4504
+#define DIV_G2D			0x8500
+#define DIV_ISP0		0xC300
+#define DIV_ISP1		0xC304
+#define DIV_ISP2		0xC308
 #define DIV_TOP0		0x10510
 #define DIV_TOP1		0x10514
-#define DIV_FSYS1		0x1054c
+#define DIV_TOP2		0x10518
+#define DIV_TOP3		0x1051C
+#define DIV_GSCL		0x10520
+#define DIV_DISP0_0		0x10524
+#define DIV_DISP0_1		0x10528
+#define DIV_DISP1_0		0x1052C
+#define DIV_DISP1_1		0x10530
+#define DIV_GEN			0x1053C
+#define DIV_MAU			0x10544
+#define DIV_FSYS0		0x10548
+#define DIV_FSYS1		0x1054C
 #define DIV_FSYS2		0x10550
+#define DIV_FSYS3		0x10554
 #define DIV_PERIC0		0x10558
+#define DIV_PERIC1		0x1055C
+#define DIV_PERIC2		0x10560
+#define DIV_PERIC3		0x10564
+#define DIV_PERIC4		0x10568
+#define DIV_PERIC5		0x1056C
+#define DIV2_RATIO0		0x10590
+#define DIV2_RATIO1		0x10594
+#define DIV_CDREX		0x20500
+#define DIV_CDREX2		0x20504
 #define DIV_KFC0		0x28500
 
+#define GATE_BUS_CPU		0x700
+#define GATE_BUS_GSCL0		0x10710
+#define GATE_BUS_GSCL1		0x10720
+#define GATE_BUS_DISP0		0x10724
+#define GATE_BUS_DISP1		0x10728
+#define GATE_BUS_MFC		0x10734
+#define GATE_BUS_G3D		0x10738
+#define GATE_BUS_GEN		0x1073C
 #define GATE_BUS_FSYS0		0x10740
-
+#define GATE_BUS_FSYS1		0x10744
+#define GATE_BUS_CDREX		0x20700
+
+#define GATE_IP_CORE		0x4900
+#define GATE_IP_G2D		0x8800
+#define GATE_IP_ISP0		0xC800
+#define GATE_IP_ISP1		0xC804
+#define GATE_IP_GSCL0		0x10910
+#define GATE_IP_GSCL1		0x10920
+#define GATE_IP_DISP0		0x10924
+#define GATE_IP_DISP1		0x10928
+#define GATE_IP_MFC		0x1092C
+#define GATE_IP_G3D		0x10930
+#define GATE_IP_GEN		0x10934
 #define GATE_IP_FSYS		0x10944
 #define GATE_IP_PERIC		0x10950
 #define GATE_IP_PERIS		0x10960
+#define GATE_IP_CDREX		0x20900
+
+#define GATE_TOP_SCLK_GSCL	0x10820
+#define GATE_TOP_SCLK_DISP0	0x10824
+#define GATE_TOP_SCLK_DISP1	0x10828
+#define GATE_TOP_SCLK_GEN	0x1082C
+#define GATE_TOP_SCLK_MAU	0x1083C
+#define GATE_TOP_SCLK_FSYS	0x10840
+#define GATE_TOP_SCLK_PERIC	0x10850
+
+#define GATE_SCLK_CPU		0x800
+#define SCLK_SRC_ISP		0x10270
+#define SCLK_DIV_ISP		0x10580
+#define SCLK_DIV_ISP1		0x10584
+
 
 /* list of PLLs */
 enum exynos5410_plls {
@@ -64,6 +161,134 @@ enum exynos5410_plls {
 	nr_plls                 /* number of PLLs */
 };
 
+static void __iomem *reg_base;
+
+#ifdef CONFIG_PM_SLEEP
+static struct samsung_clk_reg_dump *exynos5410_save;
+
+/*
+ * list of controller registers to be saved and restored during a
+ * suspend/resume cycle.
+ */
+static unsigned long exynos5410_clk_regs[] __initdata = {
+	SRC_CDREX,
+	SRC_CPERI0,
+	SRC_CPERI1,
+	SRC_CPU,
+	SRC_DISP0_0,
+	SRC_DISP0_1,
+	SRC_DISP1_0,
+	SRC_DISP1_1,
+	SRC_FSYS,
+	SRC_GSCL,
+	SRC_KFC,
+	SRC_MAU,
+	SRC_PERIC0,
+	SRC_PERIC1,
+	SRC_TOP0,
+	SRC_TOP1,
+	SRC_TOP2,
+	SRC_TOP3,
+
+	DIV_CDREX,
+	DIV_CDREX2,
+	DIV_CPU0,
+	DIV_CPERI1,
+	DIV_DISP0_0,
+	DIV_DISP0_1,
+	DIV_DISP1_0,
+	DIV_DISP1_1,
+	DIV_FSYS0,
+	DIV_FSYS1,
+	DIV_FSYS2,
+	DIV_GEN,
+	DIV_GSCL,
+	DIV_G2D,
+	DIV_KFC0,
+	DIV_MAU,
+	DIV_PERIC0,
+	DIV_PERIC1,
+	DIV_PERIC2,
+	DIV_PERIC3,
+	DIV_PERIC4,
+	DIV_PERIC5,
+	DIV_TOP0,
+	DIV_TOP1,
+	DIV_TOP2,
+	DIV_TOP3,
+
+	GATE_BUS_DISP1,
+	GATE_BUS_FSYS0,
+
+	GATE_IP_CDREX,
+	GATE_IP_CORE,
+	GATE_IP_DISP0,
+	GATE_IP_DISP1,
+	GATE_IP_FSYS,
+	GATE_IP_GEN,
+	GATE_IP_GSCL0,
+	GATE_IP_GSCL1,
+	GATE_IP_G2D,
+	GATE_IP_G3D,
+	GATE_IP_MFC,
+	GATE_IP_PERIC,
+	GATE_IP_PERIS,
+
+	GATE_TOP_SCLK_DISP1,
+	GATE_TOP_SCLK_FSYS,
+	GATE_TOP_SCLK_GSCL,
+	GATE_TOP_SCLK_MAU,
+	GATE_TOP_SCLK_PERIC,
+
+	GATE_BUS_DISP1,
+	GATE_BUS_FSYS0,
+
+	GATE_SCLK_CPU,
+
+	SRC_MASK_DISP0_0,
+	SRC_MASK_DISP1_0,
+	SRC_MASK_FSYS,
+	SRC_MASK_MAU,
+	SRC_MASK_PERIC0,
+	SRC_MASK_PERIC1,
+};
+
+static int exynos5410_clk_suspend(void)
+{
+	samsung_clk_save(reg_base, exynos5410_save,
+				ARRAY_SIZE(exynos5410_clk_regs));
+
+	return 0;
+}
+
+static void exynos5410_clk_resume(void)
+{
+	samsung_clk_restore(reg_base, exynos5410_save,
+				ARRAY_SIZE(exynos5410_clk_regs));
+}
+
+static struct syscore_ops exynos5410_clk_syscore_ops = {
+	.suspend = exynos5410_clk_suspend,
+	.resume = exynos5410_clk_resume,
+};
+
+static void exynos5410_clk_sleep_init(void)
+{
+	exynos5410_save = samsung_clk_alloc_reg_dump(exynos5410_clk_regs,
+					ARRAY_SIZE(exynos5410_clk_regs));
+	if (!exynos5410_save) {
+		pr_warn("%s: failed to allocate sleep save data, no sleep support!\n",
+			__func__);
+		return;
+	}
+
+	register_syscore_ops(&exynos5410_clk_syscore_ops);
+}
+#else
+static void exynos5410_clk_sleep_init(void) {}
+#endif
+
+
 /* list of all parent clocks */
 PNAME(apll_p)		= { "fin_pll", "fout_apll", };
 PNAME(bpll_p)		= { "fin_pll", "fout_bpll", };
@@ -190,7 +415,6 @@ static struct samsung_pll_clock exynos5410_plls[nr_plls] __initdata = {
 static void __init exynos5410_clk_init(struct device_node *np)
 {
 	struct samsung_clk_provider *ctx;
-	void __iomem *reg_base;
 
 	if (np) {
 		reg_base = of_iomap(np, 0);
@@ -214,6 +438,7 @@ static void __init exynos5410_clk_init(struct device_node *np)
 	samsung_clk_register_gate(ctx, exynos5410_gate_clks,
 			ARRAY_SIZE(exynos5410_gate_clks));
 
+	exynos5410_clk_sleep_init();
 	samsung_clk_of_add_provider(np, ctx);
 
 	pr_debug("Exynos5410: clock setup completed.\n");
-- 
1.7.10.4

Re: [PATCHv2 3/5] clk: samsung: exynos5410: Add suspend/resume handling

[Tomasz Figa]

Hi Humberto,

On 31.07.2014 13:22, Humberto Silva Naves wrote:
> This patch implements all the necessary code that handles register
> saving and restoring during a suspend/resume cycle. To make this
> possible, the local variable reg_base from the function
> exynos5410_clk_init was changed to global. In addition, new
> clock register definitions were added for the majority of the relevant
> clocks inside the SoC.
> 
> Signed-off-by: Humberto Silva Naves <hsnaves@gmail.com>
> ---
>  drivers/clk/samsung/clk-exynos5410.c |  231 +++++++++++++++++++++++++++++++++-
>  1 file changed, 228 insertions(+), 3 deletions(-)

Looks good, thanks. Since this patch depends on previous ones from this
series, I'll wait for next revision (+some time to let people see the
patches) before applying.

Best regards,
Tomasz

[PATCHv2 4/5] clk: samsung: exynos5410: Add fixed rate clocks

[Humberto Silva Naves]

This implements the fixed rate clocks generated either inside or
outside the SoC. It also adds a dt-binding constant for the
sclk_hdmiphy clock, which shall be later used by other drivers,
such as the DRM.

Since the external fixed rate clock fin_pll is now registered by
the clk-exynos5410 file, the bindings with the device tree file have
changed. It is no longer needed to define fin_pll as a fixed clock,
such as in:

	fin_pll: xxti {
  		compatible = "fixed-clock";
	  	clock-frequency = <24000000>;
	  	clock-output-names = "fin_pll";
	  	#clock-cells = <0>;
	};

The above lines should be replaced by the following lines:

	fixed-rate-clocks {
		oscclk {
			compatible = "samsung,exynos5410-oscclk";
			clock-frequency = <24000000>;
		};
	};

This new form of binding was properly documented in the relevant
documentation file.

Signed-off-by: Humberto Silva Naves <hsnaves@gmail.com>
---
 .../devicetree/bindings/clock/exynos5410-clock.txt |   17 ++++++++++---
 drivers/clk/samsung/clk-exynos5410.c               |   26 +++++++++++++++++++-
 include/dt-bindings/clock/exynos5410.h             |    1 +
 3 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/exynos5410-clock.txt b/Documentation/devicetree/bindings/clock/exynos5410-clock.txt
index aeab635..9f4a286 100644
--- a/Documentation/devicetree/bindings/clock/exynos5410-clock.txt
+++ b/Documentation/devicetree/bindings/clock/exynos5410-clock.txt
@@ -18,12 +18,21 @@ tree sources.
 
 External clock:
 
-There is clock that is generated outside the SoC. It
-is expected that it is defined using standard clock bindings
-with following clock-output-name:
-
+There is a clock that is generated outside the SoC, namely
  - "fin_pll" - PLL input clock from XXTI
 
+It is expected that a binding compatible with "samsung,exynos5410-oscclk"
+having a populated clock-frequency field such as follows to be used in
+order to define this external clock:
+
+	fixed-rate-clocks {
+		oscclk {
+			compatible = "samsung,exynos5410-oscclk";
+			clock-frequency = <24000000>;
+		};
+	};
+
+
 Example 1: An example of a clock controller node is listed below.
 
 	clock: clock-controller@0x10010000 {
diff --git a/drivers/clk/samsung/clk-exynos5410.c b/drivers/clk/samsung/clk-exynos5410.c
index a9c261c..efbe734 100644
--- a/drivers/clk/samsung/clk-exynos5410.c
+++ b/drivers/clk/samsung/clk-exynos5410.c
@@ -307,6 +307,20 @@ PNAME(group2_p)		= { "fin_pll", "fin_pll", "none", "none",
 			"none", "none", "sclk_mpll_bpll",
 			 "none", "none", "sclk_cpll" };
 
+/* fixed rate clocks generated outside the soc */
+static struct samsung_fixed_rate_clock exynos5410_fixed_rate_ext_clks[] __initdata = {
+	FRATE(CLK_FIN_PLL, "fin_pll", NULL, CLK_IS_ROOT, 0),
+};
+
+/* fixed rate clocks generated inside the soc */
+static struct samsung_fixed_rate_clock exynos5410_fixed_rate_clks[] __initdata = {
+	FRATE(CLK_SCLK_HDMIPHY, "sclk_hdmiphy", NULL, CLK_IS_ROOT, 24000000),
+	FRATE(0, "sclk_hdmi24m", NULL, CLK_IS_ROOT, 24000000),
+	FRATE(0, "sclk_hdmi27m", NULL, CLK_IS_ROOT, 27000000),
+	FRATE(0, "sclk_dptxphy", NULL, CLK_IS_ROOT, 24000000),
+	FRATE(0, "sclk_uhostphy", NULL, CLK_IS_ROOT, 48000000),
+};
+
 static struct samsung_mux_clock exynos5410_mux_clks[] __initdata = {
 	MUX(0, "mout_apll", apll_p, SRC_CPU, 0, 1),
 	MUX(0, "mout_cpu", mout_cpu_p, SRC_CPU, 16, 1),
@@ -411,6 +425,11 @@ static struct samsung_pll_clock exynos5410_plls[nr_plls] __initdata = {
 		KPLL_CON0, NULL),
 };
 
+static const struct of_device_id ext_clk_match[] __initconst = {
+	{ .compatible = "samsung,exynos5410-oscclk", .data = (void *)0, },
+	{ },
+};
+
 /* register exynos5410 clocks */
 static void __init exynos5410_clk_init(struct device_node *np)
 {
@@ -428,9 +447,14 @@ static void __init exynos5410_clk_init(struct device_node *np)
 	if (!ctx)
 		panic("%s: unable to allocate context.\n", __func__);
 
+	samsung_clk_of_register_fixed_ext(ctx, exynos5410_fixed_rate_ext_clks,
+			ARRAY_SIZE(exynos5410_fixed_rate_ext_clks),
+			ext_clk_match);
+
 	samsung_clk_register_pll(ctx, exynos5410_plls,
 			ARRAY_SIZE(exynos5410_plls), reg_base);
-
+	samsung_clk_register_fixed_rate(ctx, exynos5410_fixed_rate_clks,
+			ARRAY_SIZE(exynos5410_fixed_rate_clks));
 	samsung_clk_register_mux(ctx, exynos5410_mux_clks,
 			ARRAY_SIZE(exynos5410_mux_clks));
 	samsung_clk_register_div(ctx, exynos5410_div_clks,
diff --git a/include/dt-bindings/clock/exynos5410.h b/include/dt-bindings/clock/exynos5410.h
index 9b180f0..3a8da3c 100644
--- a/include/dt-bindings/clock/exynos5410.h
+++ b/include/dt-bindings/clock/exynos5410.h
@@ -17,6 +17,7 @@
 #define CLK_SCLK_MMC0 132
 #define CLK_SCLK_MMC1 133
 #define CLK_SCLK_MMC2 134
+#define CLK_SCLK_HDMIPHY 135
 
 /* gate clocks */
 #define CLK_UART0 257
-- 
1.7.10.4

Re: [PATCHv2 4/5] clk: samsung: exynos5410: Add fixed rate clocks

[Sylwester Nawrocki]

(dropping linux-doc ML and Randy from Cc)

On 31/07/14 13:22, Humberto Silva Naves wrote:
> This implements the fixed rate clocks generated either inside or
> outside the SoC. It also adds a dt-binding constant for the
> sclk_hdmiphy clock, which shall be later used by other drivers,
> such as the DRM.
> 
> Since the external fixed rate clock fin_pll is now registered by
> the clk-exynos5410 file, the bindings with the device tree file have
> changed. It is no longer needed to define fin_pll as a fixed clock,
> such as in:
> 
> 	fin_pll: xxti {
>   		compatible = "fixed-clock";
> 	  	clock-frequency = <24000000>;
> 	  	clock-output-names = "fin_pll";
> 	  	#clock-cells = <0>;
> 	};
> 
> The above lines should be replaced by the following lines:
> 
> 	fixed-rate-clocks {
> 		oscclk {
> 			compatible = "samsung,exynos5410-oscclk";
> 			clock-frequency = <24000000>;
> 		};
> 	};
> 
> This new form of binding was properly documented in the relevant
> documentation file.

Can you explain what is rationale behind this change ? Is it related to
suspend/resume ordering ?
Obviously it breaks the kernel/dtb compatibility. We should be moving
in opposite direction, i.e. completely remove the custom samsung fixed
rate clocks. I've tried to address this with patches [1], [2] but Tomasz
wasn't happy with them IIRC and I postponed work on that.

[1] http://www.spinics.net/lists/arm-kernel/msg333211.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/258490.html

--
Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 4/5] clk: samsung: exynos5410: Add fixed rate clocks

[Humberto Naves]

Hi,

On Thu, Jul 31, 2014 at 1:45 PM, Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
> Can you explain what is rationale behind this change ? Is it related to
> suspend/resume ordering ?

I had forgotten, but now remember the reason why I did this. If you
see the current implementation of clk-exynos5410, you will notice it
heavily depends on the clock "fin_pll". On the other hand, this clock
exists because in the current dtb (exynos5410-smdk5410.dts), there is
a node fin_pll such as

       fin_pll: xxti {
               compatible = "fixed-clock";
               clock-frequency = <24000000>;
               clock-output-names = "fin_pll";
               #clock-cells = <0>;
       };

So far so good. But the real problem comes in when I check the rate of
fin_pll to determine if I should install the rate table or not (and I
really need this for my patch). More specifically

       if (_get_rate("fin_pll") == 24 * MHZ) {
               exynos5410_plls[apll].rate_table = apll_24mhz_tbl;
               exynos5410_plls[cpll].rate_table = cpll_24mhz_tbl;
               exynos5410_plls[kpll].rate_table = kpll_24mhz_tbl;
               exynos5410_plls[dpll].rate_table = dpll_24mhz_tbl;
               exynos5410_plls[epll].rate_table = epll_24mhz_tbl;
               exynos5410_plls[ipll].rate_table = ipll_24mhz_tbl;
       }

I *have* to determine if the rate of fin_pll is 24MHz, and this is
impossible to do if fin_pll is not available. Moreover, there is no
way I can ensure that the fixed clock provider for fin_pll was
initialized before mine, so there is chance that _get_rate won't work.
The only way I fix that is to set the dependency explicitly in the
dtb, by adding the fin_pll clock as required resource.

              clock: clock-controller@10010000 {
                      compatible = "samsung,exynos5410-clock";
                      reg = <0x10010000 0x30000>;
                      #clock-cells = <1>;
                      /* Add the parent clock */
                      clocks = <&fin_pll>;
                      clock-names = "fin_pll";
              };

But in any case, the bindings with the DTB must be changed one way or
another, because I *really* need to use fin_pll on my driver
registration. If you agree with this alternative solution I previously
described, I can change that in the next version of the patch series.

Best regards,
Humberto

> Obviously it breaks the kernel/dtb compatibility. We should be moving
> in opposite direction, i.e. completely remove the custom samsung fixed
> rate clocks.

Re: [PATCHv2 4/5] clk: samsung: exynos5410: Add fixed rate clocks

[Tomasz Figa]

Humberto,

On 31.07.2014 23:01, Humberto Naves wrote:
> Hi,
> 
> On Thu, Jul 31, 2014 at 1:45 PM, Sylwester Nawrocki
> <s.nawrocki@samsung.com> wrote:
>> Can you explain what is rationale behind this change ? Is it related to
>> suspend/resume ordering ?
> 
> I had forgotten, but now remember the reason why I did this. If you
> see the current implementation of clk-exynos5410, you will notice it
> heavily depends on the clock "fin_pll". On the other hand, this clock
> exists because in the current dtb (exynos5410-smdk5410.dts), there is
> a node fin_pll such as
> 
>        fin_pll: xxti {
>                compatible = "fixed-clock";
>                clock-frequency = <24000000>;
>                clock-output-names = "fin_pll";
>                #clock-cells = <0>;
>        };
> 
> So far so good. But the real problem comes in when I check the rate of
> fin_pll to determine if I should install the rate table or not (and I
> really need this for my patch). More specifically
> 
>        if (_get_rate("fin_pll") == 24 * MHZ) {
>                exynos5410_plls[apll].rate_table = apll_24mhz_tbl;
>                exynos5410_plls[cpll].rate_table = cpll_24mhz_tbl;
>                exynos5410_plls[kpll].rate_table = kpll_24mhz_tbl;
>                exynos5410_plls[dpll].rate_table = dpll_24mhz_tbl;
>                exynos5410_plls[epll].rate_table = epll_24mhz_tbl;
>                exynos5410_plls[ipll].rate_table = ipll_24mhz_tbl;
>        }
> 
> I *have* to determine if the rate of fin_pll is 24MHz, and this is
> impossible to do if fin_pll is not available. Moreover, there is no
> way I can ensure that the fixed clock provider for fin_pll was
> initialized before mine, so there is chance that _get_rate won't work.
> The only way I fix that is to set the dependency explicitly in the
> dtb, by adding the fin_pll clock as required resource.
> 
>               clock: clock-controller@10010000 {
>                       compatible = "samsung,exynos5410-clock";
>                       reg = <0x10010000 0x30000>;
>                       #clock-cells = <1>;
>                       /* Add the parent clock */
>                       clocks = <&fin_pll>;
>                       clock-names = "fin_pll";
>               };

This is the correct solution to your problem. The clocks and clock-names
properties should have been there from the beginning but apparently this
has been missed in review. Also see below.

> 
> But in any case, the bindings with the DTB must be changed one way or
> another, because I *really* need to use fin_pll on my driver
> registration.

This is a backwards compatible change. On DTBs without clocks and
clock-names properties the PLL tables simply won't be registered which
is exactly the same behavior we have now without any tables in the
driver at all.

> If you agree with this alternative solution I previously
> described, I can change that in the next version of the patch series.

Please update the dts instead, in the way you pointed above.

Best regards,
Tomasz

Re: [PATCHv2 4/5] clk: samsung: exynos5410: Add fixed rate clocks

[Tomasz Figa]

Hi Humberto,

You can find my comments inline.

On 31.07.2014 13:22, Humberto Silva Naves wrote:
> This implements the fixed rate clocks generated either inside or
> outside the SoC. It also adds a dt-binding constant for the
> sclk_hdmiphy clock, which shall be later used by other drivers,
> such as the DRM.
> 
> Since the external fixed rate clock fin_pll is now registered by
> the clk-exynos5410 file, the bindings with the device tree file have
> changed. It is no longer needed to define fin_pll as a fixed clock,
> such as in:
> 
> 	fin_pll: xxti {
>   		compatible = "fixed-clock";
> 	  	clock-frequency = <24000000>;
> 	  	clock-output-names = "fin_pll";
> 	  	#clock-cells = <0>;
> 	};
> 
> The above lines should be replaced by the following lines:
> 
> 	fixed-rate-clocks {
> 		oscclk {
> 			compatible = "samsung,exynos5410-oscclk";
> 			clock-frequency = <24000000>;
> 		};
> 	};
> 
> This new form of binding was properly documented in the relevant
> documentation file.

In general this is backwards. This Exynos-specific clock binding was
invented before generic fixed rate clock binding showed up and so few
drivers still use it to maintain DT ABI compatibility. However new
drivers are required to use the new generic binding and so does the one
for Exynos5410.

Best regards,
Tomasz

Re: [PATCHv2 4/5] clk: samsung: exynos5410: Add fixed rate clocks

[Humberto Naves]

Hi Tomasz,

I perfectly see your point.
However my question was why you did you decide to postpone
Sylwester's? Was there any specific reason?
I suppose it would break all the dtb compatibility, but besides that,
was there any other reason?

Best,
Humberto

On Thu, Jul 31, 2014 at 2:53 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi Humberto,
>
> You can find my comments inline.
>
> On 31.07.2014 13:22, Humberto Silva Naves wrote:
>> This implements the fixed rate clocks generated either inside or
>> outside the SoC. It also adds a dt-binding constant for the
>> sclk_hdmiphy clock, which shall be later used by other drivers,
>> such as the DRM.
>>
>> Since the external fixed rate clock fin_pll is now registered by
>> the clk-exynos5410 file, the bindings with the device tree file have
>> changed. It is no longer needed to define fin_pll as a fixed clock,
>> such as in:
>>
>>       fin_pll: xxti {
>>               compatible = "fixed-clock";
>>               clock-frequency = <24000000>;
>>               clock-output-names = "fin_pll";
>>               #clock-cells = <0>;
>>       };
>>
>> The above lines should be replaced by the following lines:
>>
>>       fixed-rate-clocks {
>>               oscclk {
>>                       compatible = "samsung,exynos5410-oscclk";
>>                       clock-frequency = <24000000>;
>>               };
>>       };
>>
>> This new form of binding was properly documented in the relevant
>> documentation file.
>
> In general this is backwards. This Exynos-specific clock binding was
> invented before generic fixed rate clock binding showed up and so few
> drivers still use it to maintain DT ABI compatibility. However new
> drivers are required to use the new generic binding and so does the one
> for Exynos5410.
>
> Best regards,
> Tomasz

Re: [PATCHv2 4/5] clk: samsung: exynos5410: Add fixed rate clocks

[Tomasz Figa]

On 31.07.2014 15:23, Humberto Naves wrote:
> Hi Tomasz,
> 
> I perfectly see your point.
> However my question was why you did you decide to postpone
> Sylwester's? Was there any specific reason?
> I suppose it would break all the dtb compatibility, but besides that,
> was there any other reason?

We discussed this in private (we work in the same office) and I pointed
out certain issues with Sylwester's proposed implementation and we
agreed that one more revision of the patch is needed, but as it happens,
higher priority tasks showed up and this one got lost in action.

The first version of the patch [1] changed the original behavior
breaking DT ABI compatibility and relied on improper assumption that
those clocks are always in "fixed-rate-clocks" node. The thing is that
no code should rely on DT node naming.

Second version [2] was much better in this aspect, but it had some minor
implementation issues - custom clk_ops used instead of generic mux clock
and chipid block being constantly mapped and unmapped in every call to
__fin_pll_mux_get_parent().

I should have reviewed them both on the mailing lists, but at that time
there was no major activity related to Exynos4 outside of our office, so
it was more convenient to just talk together directly.

[1] http://www.spinics.net/lists/arm-kernel/msg333211.html
[2]
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-May/258490.html

Anyway, I don't think this is all relevant to Exynos5410, which just
uses the generic fixed rate binding and has the thing done right from
the start.

Best regards,
Tomasz

> 
> Best,
> Humberto
> 
> On Thu, Jul 31, 2014 at 2:53 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> Hi Humberto,
>>
>> You can find my comments inline.
>>
>> On 31.07.2014 13:22, Humberto Silva Naves wrote:
>>> This implements the fixed rate clocks generated either inside or
>>> outside the SoC. It also adds a dt-binding constant for the
>>> sclk_hdmiphy clock, which shall be later used by other drivers,
>>> such as the DRM.
>>>
>>> Since the external fixed rate clock fin_pll is now registered by
>>> the clk-exynos5410 file, the bindings with the device tree file have
>>> changed. It is no longer needed to define fin_pll as a fixed clock,
>>> such as in:
>>>
>>>       fin_pll: xxti {
>>>               compatible = "fixed-clock";
>>>               clock-frequency = <24000000>;
>>>               clock-output-names = "fin_pll";
>>>               #clock-cells = <0>;
>>>       };
>>>
>>> The above lines should be replaced by the following lines:
>>>
>>>       fixed-rate-clocks {
>>>               oscclk {
>>>                       compatible = "samsung,exynos5410-oscclk";
>>>                       clock-frequency = <24000000>;
>>>               };
>>>       };
>>>
>>> This new form of binding was properly documented in the relevant
>>> documentation file.
>>
>> In general this is backwards. This Exynos-specific clock binding was
>> invented before generic fixed rate clock binding showed up and so few
>> drivers still use it to maintain DT ABI compatibility. However new
>> drivers are required to use the new generic binding and so does the one
>> for Exynos5410.
>>
>> Best regards,
>> Tomasz

[PATCHv2 5/5] clk: samsung: exynos5410: Added clocks DPLL, EPLL, IPLL, and VPLL

[Humberto Silva Naves]

Added the remaining PLL clocks, and also added the configuration
tables with the PLL coefficients for the supported frequencies.
These frequency tables are only installed when a 24MHz clock is
supplied as the input clock source. To reflect these changes, new
constants were added to the dt-bindings file.

Furthermore, the definition of the clock "mout_vpllsrc" was added,
as it is required for the VPLL.

Signed-off-by: Humberto Silva Naves <hsnaves@gmail.com>
---
 drivers/clk/samsung/clk-exynos5410.c   |  130 +++++++++++++++++++++++++++++++-
 include/dt-bindings/clock/exynos5410.h |    4 +
 2 files changed, 133 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/samsung/clk-exynos5410.c b/drivers/clk/samsung/clk-exynos5410.c
index efbe734..9a6a371 100644
--- a/drivers/clk/samsung/clk-exynos5410.c
+++ b/drivers/clk/samsung/clk-exynos5410.c
@@ -157,7 +157,8 @@
 /* list of PLLs */
 enum exynos5410_plls {
 	apll, cpll, mpll,
-	bpll, kpll,
+	bpll, kpll, dpll,
+	epll, ipll, vpll,
 	nr_plls                 /* number of PLLs */
 };
 
@@ -302,6 +303,7 @@ PNAME(mout_kfc_p)	= { "mout_kpll", "sclk_mpll", };
 PNAME(mpll_user_p)	= { "fin_pll", "sclk_mpll", };
 PNAME(bpll_user_p)	= { "fin_pll", "sclk_bpll", };
 PNAME(mpll_bpll_p)	= { "sclk_mpll_muxed", "sclk_bpll_muxed", };
+PNAME(mout_vpllsrc_p)	= { "fin_pll", "sclk_hdmi27m" };
 
 PNAME(group2_p)		= { "fin_pll", "fin_pll", "none", "none",
 			"none", "none", "sclk_mpll_bpll",
@@ -321,6 +323,10 @@ static struct samsung_fixed_rate_clock exynos5410_fixed_rate_clks[] __initdata =
 	FRATE(0, "sclk_uhostphy", NULL, CLK_IS_ROOT, 48000000),
 };
 
+static struct samsung_mux_clock exynos5410_pll_pmux_clks[] __initdata = {
+	MUX(0, "mout_vpllsrc", mout_vpllsrc_p, SRC_TOP2, 0, 1),
+};
+
 static struct samsung_mux_clock exynos5410_mux_clks[] __initdata = {
 	MUX(0, "mout_apll", apll_p, SRC_CPU, 0, 1),
 	MUX(0, "mout_cpu", mout_cpu_p, SRC_CPU, 16, 1),
@@ -412,6 +418,107 @@ static struct samsung_gate_clock exynos5410_gate_clks[] __initdata = {
 			SRC_MASK_PERIC0, 8, CLK_SET_RATE_PARENT, 0),
 };
 
+static struct samsung_pll_rate_table apll_24mhz_tbl[] __initdata = {
+	/* sorted in descending order */
+	/* PLL_35XX_RATE(rate, m, p, s) */
+	PLL_35XX_RATE(2100000000, 175, 2, 0),
+	PLL_35XX_RATE(2000000000, 250, 3, 0),
+	PLL_35XX_RATE(1900000000, 475, 6, 0),
+	PLL_35XX_RATE(1800000000, 225, 3, 0),
+	PLL_35XX_RATE(1700000000, 425, 6, 0),
+	PLL_35XX_RATE(1600000000, 200, 3, 0),
+	PLL_35XX_RATE(1500000000, 250, 4, 0),
+	PLL_35XX_RATE(1400000000, 175, 3, 0),
+	PLL_35XX_RATE(1300000000, 325, 6, 0),
+	PLL_35XX_RATE(1200000000, 100, 2, 0),
+	PLL_35XX_RATE(1100000000, 275, 3, 1),
+	PLL_35XX_RATE(1000000000, 250, 3, 1),
+	PLL_35XX_RATE(900000000, 150, 2, 1),
+	PLL_35XX_RATE(800000000, 200, 3, 1),
+	PLL_35XX_RATE(700000000, 175, 3, 1),
+	PLL_35XX_RATE(600000000, 100, 2, 1),
+	PLL_35XX_RATE(500000000, 250, 3, 2),
+	PLL_35XX_RATE(400000000, 200, 3, 2),
+	PLL_35XX_RATE(300000000, 100, 2, 2),
+	PLL_35XX_RATE(200000000, 200, 3, 3),
+	{ },
+};
+
+static struct samsung_pll_rate_table cpll_24mhz_tbl[] __initdata = {
+	/* sorted in descending order */
+	/* PLL_35XX_RATE(rate, m, p, s) */
+	PLL_35XX_RATE(666000000, 222, 4, 1),
+	PLL_35XX_RATE(640000000, 160, 3, 1),
+	PLL_35XX_RATE(320000000, 160, 3, 2),
+	{ },
+};
+
+static struct samsung_pll_rate_table dpll_24mhz_tbl[] __initdata = {
+	/* sorted in descending order */
+	/* PLL_35XX_RATE(rate, m, p, s) */
+	PLL_35XX_RATE(600000000, 200, 4, 1),
+	{ },
+};
+
+static struct samsung_pll_rate_table epll_24mhz_tbl[] __initdata = {
+	/* sorted in descending order */
+	/* PLL_36XX_RATE(rate, m, p, s, k) */
+	PLL_36XX_RATE(600000000, 100, 2, 1,      0),
+	PLL_36XX_RATE(400000000, 200, 3, 2,      0),
+	PLL_36XX_RATE(200000000, 200, 3, 3,      0),
+	PLL_36XX_RATE(180633600, 301, 5, 3,  -3670),
+	PLL_36XX_RATE( 67737600, 452, 5, 5, -27263),
+	PLL_36XX_RATE( 49152000, 197, 3, 5, -25690),
+	PLL_36XX_RATE( 45158401, 181, 3, 5, -24012),
+	{ },
+};
+
+static struct samsung_pll_rate_table ipll_24mhz_tbl[] __initdata = {
+	/* sorted in descending order */
+	/* PLL_35XX_RATE(rate, m, p, s, k) */
+	PLL_35XX_RATE(864000000, 288, 4, 1),
+	PLL_35XX_RATE(666000000, 222, 4, 1),
+	PLL_35XX_RATE(432000000, 288, 4, 2),
+	{ },
+};
+
+static struct samsung_pll_rate_table kpll_24mhz_tbl[] __initdata = {
+	/* sorted in descending order */
+	/* PLL_35XX_RATE(rate, m, p, s) */
+	PLL_35XX_RATE(1500000000, 250, 4, 0),
+	PLL_35XX_RATE(1400000000, 175, 3, 0),
+	PLL_35XX_RATE(1300000000, 325, 6, 0),
+	PLL_35XX_RATE(1200000000, 100, 2, 0),
+	PLL_35XX_RATE(1100000000, 275, 3, 1),
+	PLL_35XX_RATE(1000000000, 250, 3, 1),
+	PLL_35XX_RATE(900000000, 150, 2, 1),
+	PLL_35XX_RATE(800000000, 200, 3, 1),
+	PLL_35XX_RATE(700000000, 175, 3, 1),
+	PLL_35XX_RATE(600000000, 100, 2, 1),
+	PLL_35XX_RATE(500000000, 250, 3, 2),
+	PLL_35XX_RATE(400000000, 200, 3, 2),
+	PLL_35XX_RATE(300000000, 100, 2, 2),
+	PLL_35XX_RATE(200000000, 200, 3, 3),
+	{ },
+};
+
+static struct samsung_pll_rate_table vpll_24mhz_tbl[] __initdata = {
+	/* sorted in descending order */
+	/* PLL_36XX_RATE(rate, m, p, s, k) */
+	PLL_36XX_RATE(880000000, 220, 3, 1, 0),
+	PLL_36XX_RATE(640000000, 160, 3, 1, 0),
+	PLL_36XX_RATE(532000000, 133, 3, 1, 0),
+	PLL_36XX_RATE(480000000, 240, 3, 2, 0),
+	PLL_36XX_RATE(440000000, 220, 3, 2, 0),
+	PLL_36XX_RATE(350000000, 175, 3, 2, 0),
+	PLL_36XX_RATE(333000000, 111, 2, 2, 0),
+	PLL_36XX_RATE(266000000, 133, 3, 2, 0),
+	PLL_36XX_RATE(177000000, 118, 2, 3, 0),
+	PLL_36XX_RATE(123500000, 330, 4, 4, 0),
+	PLL_36XX_RATE( 89000000, 178, 3, 4, 0),
+	{ },
+};
+
 static struct samsung_pll_clock exynos5410_plls[nr_plls] __initdata = {
 	[apll] = PLL(pll_35xx, CLK_FOUT_APLL, "fout_apll", "fin_pll", APLL_LOCK,
 		APLL_CON0, NULL),
@@ -423,6 +530,14 @@ static struct samsung_pll_clock exynos5410_plls[nr_plls] __initdata = {
 		BPLL_CON0, NULL),
 	[kpll] = PLL(pll_35xx, CLK_FOUT_KPLL, "fout_kpll", "fin_pll", KPLL_LOCK,
 		KPLL_CON0, NULL),
+	[dpll] = PLL(pll_35xx, CLK_FOUT_DPLL, "fout_dpll", "fin_pll", DPLL_LOCK,
+		DPLL_CON0, NULL),
+	[epll] = PLL(pll_36xx, CLK_FOUT_EPLL, "fout_epll", "fin_pll", EPLL_LOCK,
+		EPLL_CON0, NULL),
+	[ipll] = PLL(pll_35xx, CLK_FOUT_IPLL, "fout_ipll", "fin_pll", IPLL_LOCK,
+		IPLL_CON0, NULL),
+	[vpll] = PLL(pll_36xx, CLK_FOUT_VPLL, "fout_vpll", "mout_vpllsrc",
+		VPLL_LOCK, VPLL_CON0, NULL),
 };
 
 static const struct of_device_id ext_clk_match[] __initconst = {
@@ -450,6 +565,19 @@ static void __init exynos5410_clk_init(struct device_node *np)
 	samsung_clk_of_register_fixed_ext(ctx, exynos5410_fixed_rate_ext_clks,
 			ARRAY_SIZE(exynos5410_fixed_rate_ext_clks),
 			ext_clk_match);
+	samsung_clk_register_mux(ctx, exynos5410_pll_pmux_clks,
+				ARRAY_SIZE(exynos5410_pll_pmux_clks));
+
+	if (_get_rate("fin_pll") == 24 * MHZ) {
+		exynos5410_plls[apll].rate_table = apll_24mhz_tbl;
+		exynos5410_plls[cpll].rate_table = cpll_24mhz_tbl;
+		exynos5410_plls[kpll].rate_table = kpll_24mhz_tbl;
+		exynos5410_plls[dpll].rate_table = dpll_24mhz_tbl;
+		exynos5410_plls[epll].rate_table = epll_24mhz_tbl;
+		exynos5410_plls[ipll].rate_table = ipll_24mhz_tbl;
+	}
+	if (_get_rate("mout_vpllsrc") == 24 * MHZ)
+		exynos5410_plls[vpll].rate_table = vpll_24mhz_tbl;
 
 	samsung_clk_register_pll(ctx, exynos5410_plls,
 			ARRAY_SIZE(exynos5410_plls), reg_base);
diff --git a/include/dt-bindings/clock/exynos5410.h b/include/dt-bindings/clock/exynos5410.h
index 3a8da3c..7da296c 100644
--- a/include/dt-bindings/clock/exynos5410.h
+++ b/include/dt-bindings/clock/exynos5410.h
@@ -8,6 +8,10 @@
 #define CLK_FOUT_MPLL 4
 #define CLK_FOUT_BPLL 5
 #define CLK_FOUT_KPLL 6
+#define CLK_FOUT_DPLL 7
+#define CLK_FOUT_EPLL 8
+#define CLK_FOUT_IPLL 9
+#define CLK_FOUT_VPLL 10
 
 /* gate for special clocks (sclk) */
 #define CLK_SCLK_UART0 128
-- 
1.7.10.4

Re: [PATCHv2 5/5] clk: samsung: exynos5410: Added clocks DPLL, EPLL, IPLL, and VPLL

[Tomasz Figa]

Hi Humberto,

You can find my comments inline.

On 31.07.2014 13:22, Humberto Silva Naves wrote:
> Added the remaining PLL clocks, and also added the configuration
> tables with the PLL coefficients for the supported frequencies.
> These frequency tables are only installed when a 24MHz clock is
> supplied as the input clock source. To reflect these changes, new
> constants were added to the dt-bindings file.

[snip]

> +static struct samsung_pll_rate_table apll_24mhz_tbl[] __initdata = {
> +	/* sorted in descending order */
> +	/* PLL_35XX_RATE(rate, m, p, s) */
> +	PLL_35XX_RATE(2100000000, 175, 2, 0),
> +	PLL_35XX_RATE(2000000000, 250, 3, 0),
> +	PLL_35XX_RATE(1900000000, 475, 6, 0),
> +	PLL_35XX_RATE(1800000000, 225, 3, 0),
> +	PLL_35XX_RATE(1700000000, 425, 6, 0),
> +	PLL_35XX_RATE(1600000000, 200, 3, 0),
> +	PLL_35XX_RATE(1500000000, 250, 4, 0),
> +	PLL_35XX_RATE(1400000000, 175, 3, 0),
> +	PLL_35XX_RATE(1300000000, 325, 6, 0),
> +	PLL_35XX_RATE(1200000000, 100, 2, 0),
> +	PLL_35XX_RATE(1100000000, 275, 3, 1),
> +	PLL_35XX_RATE(1000000000, 250, 3, 1),
> +	PLL_35XX_RATE(900000000, 150, 2, 1),
> +	PLL_35XX_RATE(800000000, 200, 3, 1),
> +	PLL_35XX_RATE(700000000, 175, 3, 1),
> +	PLL_35XX_RATE(600000000, 100, 2, 1),
> +	PLL_35XX_RATE(500000000, 250, 3, 2),
> +	PLL_35XX_RATE(400000000, 200, 3, 2),
> +	PLL_35XX_RATE(300000000, 100, 2, 2),
> +	PLL_35XX_RATE(200000000, 200, 3, 3),

nit: The numbers could be aligned to the right using spaces (see exynos4.c).

> +	{ },
> +};
> +
> +static struct samsung_pll_rate_table cpll_24mhz_tbl[] __initdata = {
> +	/* sorted in descending order */
> +	/* PLL_35XX_RATE(rate, m, p, s) */
> +	PLL_35XX_RATE(666000000, 222, 4, 1),
> +	PLL_35XX_RATE(640000000, 160, 3, 1),
> +	PLL_35XX_RATE(320000000, 160, 3, 2),
> +	{ },
> +};
> +
> +static struct samsung_pll_rate_table dpll_24mhz_tbl[] __initdata = {
> +	/* sorted in descending order */
> +	/* PLL_35XX_RATE(rate, m, p, s) */
> +	PLL_35XX_RATE(600000000, 200, 4, 1),
> +	{ },
> +};
> +
> +static struct samsung_pll_rate_table epll_24mhz_tbl[] __initdata = {
> +	/* sorted in descending order */
> +	/* PLL_36XX_RATE(rate, m, p, s, k) */
> +	PLL_36XX_RATE(600000000, 100, 2, 1,      0),
> +	PLL_36XX_RATE(400000000, 200, 3, 2,      0),
> +	PLL_36XX_RATE(200000000, 200, 3, 3,      0),
> +	PLL_36XX_RATE(180633600, 301, 5, 3,  -3670),
> +	PLL_36XX_RATE( 67737600, 452, 5, 5, -27263),
> +	PLL_36XX_RATE( 49152000, 197, 3, 5, -25690),
> +	PLL_36XX_RATE( 45158401, 181, 3, 5, -24012),

Have you ensured that the rates specified match the rates calculated
using PLL equation? You can find how it is calculated in recalc_rate
callback of this particular PLL type in clk-pll.c.

As a side note, the PLL registration code should be made a bit more
robust and just calculate the rates itself and printing warnings if they
don't match the entered ones. I definitely need more hours in a day, so
much to do. ;)

> +	{ },
> +};
> +
> +static struct samsung_pll_rate_table ipll_24mhz_tbl[] __initdata = {
> +	/* sorted in descending order */
> +	/* PLL_35XX_RATE(rate, m, p, s, k) */
> +	PLL_35XX_RATE(864000000, 288, 4, 1),
> +	PLL_35XX_RATE(666000000, 222, 4, 1),
> +	PLL_35XX_RATE(432000000, 288, 4, 2),
> +	{ },
> +};
> +
> +static struct samsung_pll_rate_table kpll_24mhz_tbl[] __initdata = {
> +	/* sorted in descending order */
> +	/* PLL_35XX_RATE(rate, m, p, s) */
> +	PLL_35XX_RATE(1500000000, 250, 4, 0),
> +	PLL_35XX_RATE(1400000000, 175, 3, 0),
> +	PLL_35XX_RATE(1300000000, 325, 6, 0),
> +	PLL_35XX_RATE(1200000000, 100, 2, 0),
> +	PLL_35XX_RATE(1100000000, 275, 3, 1),
> +	PLL_35XX_RATE(1000000000, 250, 3, 1),
> +	PLL_35XX_RATE(900000000, 150, 2, 1),
> +	PLL_35XX_RATE(800000000, 200, 3, 1),
> +	PLL_35XX_RATE(700000000, 175, 3, 1),
> +	PLL_35XX_RATE(600000000, 100, 2, 1),
> +	PLL_35XX_RATE(500000000, 250, 3, 2),
> +	PLL_35XX_RATE(400000000, 200, 3, 2),
> +	PLL_35XX_RATE(300000000, 100, 2, 2),
> +	PLL_35XX_RATE(200000000, 200, 3, 3),

nit: Alignment.

Otherwise looks good, thanks.

Best regards,
Tomasz

Re: [PATCHv2 5/5] clk: samsung: exynos5410: Added clocks DPLL, EPLL, IPLL, and VPLL

[Humberto Naves]

Hi Tomasz,

I remember checking these rates on my calculator. You might notice the
odd frequency of 45158401Hz (no pun intended) in the EPLL clock. This
particular clock frequency was giving me a big headache in a previous
project, since it was wrongly listed as 45158400. At first it seems
innocuous, but whenever I would ask for one of the child clocks to
change the rate, the driver would miscalculate the correct frequencies
and errors would propagate up and down the clock tree.

Anyway, I would double check these tables. And if you want, I can
write a separate patch for the rate table validation. I presume that
you would like to add a new field, such as default_base_freq, to the
structure samsung_pll_clock, and if that field is non-zero, you
perform the validation of the table in _samsung_clk_register_pll?

Best,
Humberto

On Thu, Jul 31, 2014 at 3:07 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi Humberto,
>
> You can find my comments inline.
>
> On 31.07.2014 13:22, Humberto Silva Naves wrote:
>> Added the remaining PLL clocks, and also added the configuration
>> tables with the PLL coefficients for the supported frequencies.
>> These frequency tables are only installed when a 24MHz clock is
>> supplied as the input clock source. To reflect these changes, new
>> constants were added to the dt-bindings file.
>
> [snip]
>
>> +static struct samsung_pll_rate_table apll_24mhz_tbl[] __initdata = {
>> +     /* sorted in descending order */
>> +     /* PLL_35XX_RATE(rate, m, p, s) */
>> +     PLL_35XX_RATE(2100000000, 175, 2, 0),
>> +     PLL_35XX_RATE(2000000000, 250, 3, 0),
>> +     PLL_35XX_RATE(1900000000, 475, 6, 0),
>> +     PLL_35XX_RATE(1800000000, 225, 3, 0),
>> +     PLL_35XX_RATE(1700000000, 425, 6, 0),
>> +     PLL_35XX_RATE(1600000000, 200, 3, 0),
>> +     PLL_35XX_RATE(1500000000, 250, 4, 0),
>> +     PLL_35XX_RATE(1400000000, 175, 3, 0),
>> +     PLL_35XX_RATE(1300000000, 325, 6, 0),
>> +     PLL_35XX_RATE(1200000000, 100, 2, 0),
>> +     PLL_35XX_RATE(1100000000, 275, 3, 1),
>> +     PLL_35XX_RATE(1000000000, 250, 3, 1),
>> +     PLL_35XX_RATE(900000000, 150, 2, 1),
>> +     PLL_35XX_RATE(800000000, 200, 3, 1),
>> +     PLL_35XX_RATE(700000000, 175, 3, 1),
>> +     PLL_35XX_RATE(600000000, 100, 2, 1),
>> +     PLL_35XX_RATE(500000000, 250, 3, 2),
>> +     PLL_35XX_RATE(400000000, 200, 3, 2),
>> +     PLL_35XX_RATE(300000000, 100, 2, 2),
>> +     PLL_35XX_RATE(200000000, 200, 3, 3),
>
> nit: The numbers could be aligned to the right using spaces (see exynos4.c).
>
>> +     { },
>> +};
>> +
>> +static struct samsung_pll_rate_table cpll_24mhz_tbl[] __initdata = {
>> +     /* sorted in descending order */
>> +     /* PLL_35XX_RATE(rate, m, p, s) */
>> +     PLL_35XX_RATE(666000000, 222, 4, 1),
>> +     PLL_35XX_RATE(640000000, 160, 3, 1),
>> +     PLL_35XX_RATE(320000000, 160, 3, 2),
>> +     { },
>> +};
>> +
>> +static struct samsung_pll_rate_table dpll_24mhz_tbl[] __initdata = {
>> +     /* sorted in descending order */
>> +     /* PLL_35XX_RATE(rate, m, p, s) */
>> +     PLL_35XX_RATE(600000000, 200, 4, 1),
>> +     { },
>> +};
>> +
>> +static struct samsung_pll_rate_table epll_24mhz_tbl[] __initdata = {
>> +     /* sorted in descending order */
>> +     /* PLL_36XX_RATE(rate, m, p, s, k) */
>> +     PLL_36XX_RATE(600000000, 100, 2, 1,      0),
>> +     PLL_36XX_RATE(400000000, 200, 3, 2,      0),
>> +     PLL_36XX_RATE(200000000, 200, 3, 3,      0),
>> +     PLL_36XX_RATE(180633600, 301, 5, 3,  -3670),
>> +     PLL_36XX_RATE( 67737600, 452, 5, 5, -27263),
>> +     PLL_36XX_RATE( 49152000, 197, 3, 5, -25690),
>> +     PLL_36XX_RATE( 45158401, 181, 3, 5, -24012),
>
> Have you ensured that the rates specified match the rates calculated
> using PLL equation? You can find how it is calculated in recalc_rate
> callback of this particular PLL type in clk-pll.c.
>
> As a side note, the PLL registration code should be made a bit more
> robust and just calculate the rates itself and printing warnings if they
> don't match the entered ones. I definitely need more hours in a day, so
> much to do. ;)
>
>> +     { },
>> +};
>> +
>> +static struct samsung_pll_rate_table ipll_24mhz_tbl[] __initdata = {
>> +     /* sorted in descending order */
>> +     /* PLL_35XX_RATE(rate, m, p, s, k) */
>> +     PLL_35XX_RATE(864000000, 288, 4, 1),
>> +     PLL_35XX_RATE(666000000, 222, 4, 1),
>> +     PLL_35XX_RATE(432000000, 288, 4, 2),
>> +     { },
>> +};
>> +
>> +static struct samsung_pll_rate_table kpll_24mhz_tbl[] __initdata = {
>> +     /* sorted in descending order */
>> +     /* PLL_35XX_RATE(rate, m, p, s) */
>> +     PLL_35XX_RATE(1500000000, 250, 4, 0),
>> +     PLL_35XX_RATE(1400000000, 175, 3, 0),
>> +     PLL_35XX_RATE(1300000000, 325, 6, 0),
>> +     PLL_35XX_RATE(1200000000, 100, 2, 0),
>> +     PLL_35XX_RATE(1100000000, 275, 3, 1),
>> +     PLL_35XX_RATE(1000000000, 250, 3, 1),
>> +     PLL_35XX_RATE(900000000, 150, 2, 1),
>> +     PLL_35XX_RATE(800000000, 200, 3, 1),
>> +     PLL_35XX_RATE(700000000, 175, 3, 1),
>> +     PLL_35XX_RATE(600000000, 100, 2, 1),
>> +     PLL_35XX_RATE(500000000, 250, 3, 2),
>> +     PLL_35XX_RATE(400000000, 200, 3, 2),
>> +     PLL_35XX_RATE(300000000, 100, 2, 2),
>> +     PLL_35XX_RATE(200000000, 200, 3, 3),
>
> nit: Alignment.
>
> Otherwise looks good, thanks.
>
> Best regards,
> Tomasz

Re: [PATCHv2 5/5] clk: samsung: exynos5410: Added clocks DPLL, EPLL, IPLL, and VPLL

[Tomasz Figa]

On 31.07.2014 15:37, Humberto Naves wrote:
> Hi Tomasz,
> 
> I remember checking these rates on my calculator. You might notice the
> odd frequency of 45158401Hz (no pun intended) in the EPLL clock. This
> particular clock frequency was giving me a big headache in a previous
> project, since it was wrongly listed as 45158400. At first it seems
> innocuous, but whenever I would ask for one of the child clocks to
> change the rate, the driver would miscalculate the correct frequencies
> and errors would propagate up and down the clock tree.
> 
> Anyway, I would double check these tables. And if you want, I can
> write a separate patch for the rate table validation. I presume that
> you would like to add a new field, such as default_base_freq, to the
> structure samsung_pll_clock, and if that field is non-zero, you
> perform the validation of the table in _samsung_clk_register_pll?

I'm not sure I get the idea of the field you're suggesting. If I
understand correctly, your intention would be to provide a default
frequency if there is no table provided. I don't think there is a need
for it, because current code can read back current settings from
registers and calculate current rate.

As for the validation itself, one more thing that needs to be considered
is that the rate table must be sorted.

We once decided to rely on the fact that tables in SoC drivers have
rates explicitly specified and are correctly sorted, but now I'm
inclined to reconsider this, based on the fact that those rates often
are already incorrectly calculated in vendor code or even datasheets,
which are main information sources for patch authors.

Before mainlining PLL drivers (which was quite some time ago), we had a
bit different implementation in our internal tree, which did not use
explicitly specified rates at all (they could have been considered just
comments to improve table readability) and the _register_pll() function
simply calculated rates for all entries creating new table for internal
use of the PLL driver that was in addition explicitly sorted to make
sure that the order is correct. This kind of implementation is what I
would lean toward today.

Best regards,
Tomasz

Re: [PATCHv2 5/5] clk: samsung: exynos5410: Added clocks DPLL, EPLL, IPLL, and VPLL

[Humberto Naves]

Hi,

On Thu, Jul 31, 2014 at 5:19 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>
> I'm not sure I get the idea of the field you're suggesting. If I
> understand correctly, your intention would be to provide a default
> frequency if there is no table provided. I don't think there is a need
> for it, because current code can read back current settings from
> registers and calculate current rate.
>
I think I was not clear enough. I am not trying to provide a default
frequency for the clock, but I do want to specify the base frequency
on which the rate table was based upon. Let me give you an example
that will hopefully clarify the matter. Suppose I want to register my
PLL, such as in the 5410. The *current* solution would be like this:

static struct samsung_pll_rate_table ipll_24mhz_tbl[] __initdata = {
       /* PLL_35XX_RATE(rate, m, p, s, k) */
       PLL_35XX_RATE(864000000, 288, 4, 1),
       { },
};
static struct samsung_pll_clock exynos5410_plls[nr_plls] __initdata = {
       [ipll] = PLL(pll_35xx, CLK_FOUT_IPLL, "fout_ipll", "fin_pll", IPLL_LOCK,
               IPLL_CON0, NULL),
};

And in the driver initialization function, I would add the rate table
if the input clock source matches what I expected, in this case 24Mhz:

if (_get_rate("fin_pll") == 24 * MHZ) {
         exynos5410_plls[ipll].rate_table = ipll_24mhz_tbl;
}

An alternative approach would be as follows, we add a new field (say
"base_rate") to the structure samsung_pll_clock, and to the macro PLL,
and describe the pll table in a simpler way, such as follows:

static struct samsung_pll_clock exynos5410_plls[nr_plls] __initdata = {
       [ipll] = PLL(pll_35xx, CLK_FOUT_IPLL, "fout_ipll", "fin_pll", IPLL_LOCK,
               IPLL_CON0, &ipll_24mhz_tbl, 24 * MHZ),
};

and in the _samsung_clk_register_pll function, all the validation
would be performed. Here I am talking about checking if the parent
rate is what is specified on the samsung_pll_clock structure, and if
so, to check if the rates on the rate table actually match what the
coefficients are telling.

> As for the validation itself, one more thing that needs to be considered
> is that the rate table must be sorted.

The _samsung_clk_register_pll could do that in theory.

>
> We once decided to rely on the fact that tables in SoC drivers have
> rates explicitly specified and are correctly sorted, but now I'm
> inclined to reconsider this, based on the fact that those rates often
> are already incorrectly calculated in vendor code or even datasheets,
> which are main information sources for patch authors.
>
> Before mainlining PLL drivers (which was quite some time ago), we had a
> bit different implementation in our internal tree, which did not use
> explicitly specified rates at all (they could have been considered just
> comments to improve table readability) and the _register_pll() function
> simply calculated rates for all entries creating new table for internal
> use of the PLL driver that was in addition explicitly sorted to make
> sure that the order is correct. This kind of implementation is what I
> would lean toward today.

I would strongly object to such as solution. I think that in the
table, the frequency *must* be specified. As you said previously, the
coefficients should be carefully chosen. We cannot know for sure that
the same coefficients that work for a base frequency of 24 Mhz will
also work for a different base frequency. So the driver cannot simply
compute the frequency from the coefficients, and it must also check
that the input rate is correct. This is another reason why I want to
add the base_frequency field to that structure.

I believe that the the _samsung_clk_register_pll must double-check if
the frequencies match what the formula tells, and must drop the
entries (or the whole frequency table) that are faulty. And then
finally, it should sort the entries in descending order.

I hope I made myself clear now.
Best regards,
Humberto


>
> Best regards,
> Tomasz