[PATCH 0/4] clk:mvebu: Improve clock drift

[PATCH 0/4] clk:mvebu: Improve clock drift

[Gregory CLEMENT]

Hi Mike, Jason, Andrew and Sebastian,

Few users reported a timer drift on the Armada 370 based board such as
the mirabox or the Netgear ReadyNAS 102.

The reason is that when the SSCG (Spread Spectrum Clock Generator) is
enabled, it shifts the frequency of the clock. The percentage is no
more than 1% but when the clock is used for a timer it leads to a
clock drift.

This series allows to correct the affected clock when the SSCG is
enabled. This drift can happen on all the mvebu SoC on the cpu clock
block (ie cpu, ddr and l2 cache). Currently the only notable effect is
for the Armada 370 because this SoC use the l2cache clock as source
for the timer. That's why even if the series allow any of the mvebu
SoC to benefit to this correction, Armada 370 is the only user of it.

The first 2 patches should go through the clk subsystem, whereas the
third one should go to the arm-soc through the mvebu tree.

The last one is just to fix a typo I found while I was reading the clk
code.

Thanks,


Gregory CLEMENT (4):
  clk: mvebu: Fix clk frequency value if SSCG is enabled
  clk: mvebu: armada-370: Fix timer drift caused by the SSCG deviation
  ARM: mvebu: add SSCG to Armada 370 Device Tree
  clk: mvebu: armada-375: Fix the description of the SAR in the comment

 arch/arm/boot/dts/armada-370.dtsi |  4 +++
 drivers/clk/mvebu/armada-370.c    |  7 ++++
 drivers/clk/mvebu/armada-375.c    |  4 +--
 drivers/clk/mvebu/common.c        | 74 +++++++++++++++++++++++++++++++++++++++
 drivers/clk/mvebu/common.h        |  1 +
 5 files changed, 88 insertions(+), 2 deletions(-)

-- 
1.9.1

[PATCH 1/4] clk: mvebu: Fix clk frequency value if SSCG is enabled

[Gregory CLEMENT]

When the SSCG (Spread Spectrum Clock Generator) is enabled, it shifts
the frequency of the clock. The percentage is no more than 1% but when
the clock is used for a timer it leads to a clock drift.

This patch allows to correct the affected clock when the SSCG is
enabled. The check is done in an new optional function related to each
SoC: is_sscg_enabled(). If this function is not present then no
correction is done on the clock frequency.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/clk/mvebu/common.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/clk/mvebu/common.h |  1 +
 2 files changed, 75 insertions(+)

diff --git a/drivers/clk/mvebu/common.c b/drivers/clk/mvebu/common.c
index 25ceccf939ad..834d36cf79b0 100644
--- a/drivers/clk/mvebu/common.c
+++ b/drivers/clk/mvebu/common.c
@@ -26,8 +26,78 @@
  * Core Clocks
  */
 
+#define SSCG_CONF_MODE(reg)	(((reg) >> 16) & 0x3)
+#define SSCG_SPREAD_DOWN	0x0
+#define SSCG_SPREAD_UP		0x1
+#define SSCG_SPREAD_CENTRAL	0x2
+#define SSCG_CONF_LOW(reg)	(((reg) >> 8) & 0xFF)
+#define SSCG_CONF_HIGH(reg)	((reg) & 0xFF)
+
 static struct clk_onecell_data clk_data;
 
+static u32 fix_sscg_deviation(struct device_node *np, u32 system_clk)
+{
+	struct device_node *sscg_np = NULL;
+	void __iomem *sscg_map;
+	u32 sscg_reg;
+	s32 low_bound, high_bound;
+	u64 freq_swing_half;
+
+	sscg_np = of_find_node_by_name(np, "sscg");
+	if (sscg_np == NULL) {
+		pr_err("cannot get SSCG register node\n");
+		return system_clk;
+	}
+
+	sscg_map = of_iomap(sscg_np, 0);
+	if (sscg_map == NULL) {
+		pr_err("cannot map SSCG register\n");
+		goto out;
+	}
+
+	sscg_reg = readl(sscg_map);
+	high_bound = SSCG_CONF_HIGH(sscg_reg);
+	low_bound = SSCG_CONF_LOW(sscg_reg);
+
+	if ((high_bound - low_bound) <= 0)
+		goto out;
+	/*
+	 * From the datasheet we got the following formula
+	 * Spread percentage = 1/96 * (H - L) / H
+	 * H = SSCG_High_Boundary
+	 * L = SSCG_Low_Boundary
+	 *
+	 * As the deviation is half of spread then it lead to the
+	 * following formula in the code.
+	 *
+	 * To avoid an overflow and not lose any significant digit in
+	 * the same time we have to use a 64 bit integer.
+	 */
+
+	freq_swing_half = (((u64)high_bound - (u64)low_bound)
+			* (u64)system_clk);
+	do_div(freq_swing_half, (2 * 96 * high_bound));
+
+	switch (SSCG_CONF_MODE(sscg_reg)) {
+	case SSCG_SPREAD_DOWN:
+		system_clk -= freq_swing_half;
+		break;
+	case SSCG_SPREAD_UP:
+		system_clk += freq_swing_half;
+		break;
+	case SSCG_SPREAD_CENTRAL:
+	default:
+		break;
+	}
+
+	iounmap(sscg_map);
+
+out:
+	of_node_put(sscg_np);
+
+	return system_clk;
+}
+
 void __init mvebu_coreclk_setup(struct device_node *np,
 				const struct coreclk_soc_desc *desc)
 {
@@ -62,6 +132,10 @@ void __init mvebu_coreclk_setup(struct device_node *np,
 	of_property_read_string_index(np, "clock-output-names", 1,
 				      &cpuclk_name);
 	rate = desc->get_cpu_freq(base);
+
+	if (desc->is_sscg_enabled && desc->is_sscg_enabled(base))
+		rate = fix_sscg_deviation(np, rate);
+
 	clk_data.clks[1] = clk_register_fixed_rate(NULL, cpuclk_name, NULL,
 						   CLK_IS_ROOT, rate);
 	WARN_ON(IS_ERR(clk_data.clks[1]));
diff --git a/drivers/clk/mvebu/common.h b/drivers/clk/mvebu/common.h
index f968b4d9df92..495f94ff4c90 100644
--- a/drivers/clk/mvebu/common.h
+++ b/drivers/clk/mvebu/common.h
@@ -28,6 +28,7 @@ struct coreclk_soc_desc {
 	u32 (*get_tclk_freq)(void __iomem *sar);
 	u32 (*get_cpu_freq)(void __iomem *sar);
 	void (*get_clk_ratio)(void __iomem *sar, int id, int *mult, int *div);
+	bool (*is_sscg_enabled)(void __iomem *sar);
 	const struct coreclk_ratio *ratios;
 	int num_ratios;
 };
-- 
1.9.1

[PATCH 2/4] clk: mvebu: armada-370: Fix timer drift caused by the SSCG deviation

[Gregory CLEMENT]

This commit activates the SSCG deviation correction for the Armada
370. It uses the optional function introduced by the commit "clk:
mvebu: Fix clk frequency value if SSCG is enabled".

Without this fix the deviation measured on a Mirabox was of a few
second each hour, whereas with this fix it was reduced at around
50ppm (around 4s per day).

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/clk/mvebu/armada-370.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/clk/mvebu/armada-370.c b/drivers/clk/mvebu/armada-370.c
index bef198a83863..b084a84d8fad 100644
--- a/drivers/clk/mvebu/armada-370.c
+++ b/drivers/clk/mvebu/armada-370.c
@@ -23,6 +23,7 @@
  */
 
 #define SARL				0	/* Low part [0:31] */
+#define	 SARL_A370_SSCG_ENABLE		BIT(10)
 #define	 SARL_A370_PCLK_FREQ_OPT	11
 #define	 SARL_A370_PCLK_FREQ_OPT_MASK	0xF
 #define	 SARL_A370_FAB_FREQ_OPT		15
@@ -133,10 +134,16 @@ static void __init a370_get_clk_ratio(
 	}
 }
 
+bool a370_is_sscg_enabled(void __iomem *sar)
+{
+	return !(readl(sar) & SARL_A370_SSCG_ENABLE);
+}
+
 static const struct coreclk_soc_desc a370_coreclks = {
 	.get_tclk_freq = a370_get_tclk_freq,
 	.get_cpu_freq = a370_get_cpu_freq,
 	.get_clk_ratio = a370_get_clk_ratio,
+	.is_sscg_enabled = a370_is_sscg_enabled,
 	.ratios = a370_coreclk_ratios,
 	.num_ratios = ARRAY_SIZE(a370_coreclk_ratios),
 };
-- 
1.9.1

[PATCH 3/4] ARM: mvebu: add SSCG to Armada 370 Device Tree

[Gregory CLEMENT]

The Armada 370 SoC has a Spread Spectrum Clock Generator. This commit
adds the description of this generator to the Device Tree describing
this SoC.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/boot/dts/armada-370.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/armada-370.dtsi b/arch/arm/boot/dts/armada-370.dtsi
index 21b588b6f6bd..dfbbb709f4fb 100644
--- a/arch/arm/boot/dts/armada-370.dtsi
+++ b/arch/arm/boot/dts/armada-370.dtsi
@@ -206,6 +206,10 @@
 				status = "okay";
 			};
 
+			sscg@18330 {
+				reg = <0x18330 0x4>;
+			};
+
 			interrupt-controller@20000 {
 				reg = <0x20a00 0x1d0>, <0x21870 0x58>;
 			};
-- 
1.9.1

[PATCH 4/4] clk: mvebu: armada-375: Fix the description of the SAR in the comment

[Gregory CLEMENT]

For dealing with the code we use the SAR1 and not the SAR0. The code
was correct, and now the comments too.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/clk/mvebu/armada-375.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/mvebu/armada-375.c b/drivers/clk/mvebu/armada-375.c
index c991a4d95e10..c7af2242b796 100644
--- a/drivers/clk/mvebu/armada-375.c
+++ b/drivers/clk/mvebu/armada-375.c
@@ -27,14 +27,14 @@
  * all modified at the same time, and not separately as for the Armada
  * 370 or the Armada XP SoCs.
  *
- * SAR0[21:17]   : CPU frequency    DDR frequency   L2 frequency
+ * SAR1[21:17]   : CPU frequency    DDR frequency   L2 frequency
  *		 6   =  400 MHz	    400 MHz	    200 MHz
  *		 15  =  600 MHz	    600 MHz	    300 MHz
  *		 21  =  800 MHz	    534 MHz	    400 MHz
  *		 25  = 1000 MHz	    500 MHz	    500 MHz
  *		 others reserved.
  *
- * SAR0[22]   : TCLK frequency
+ * SAR1[22]   : TCLK frequency
  *		 0 = 166 MHz
  *		 1 = 200 MHz
  */
-- 
1.9.1

Re: [PATCH 1/4] clk: mvebu: Fix clk frequency value if SSCG is enabled

[Sebastian Hesselbarth]

On 08/29/2014 01:43 PM, Gregory CLEMENT wrote:
> When the SSCG (Spread Spectrum Clock Generator) is enabled, it shifts
> the frequency of the clock. The percentage is no more than 1% but when
> the clock is used for a timer it leads to a clock drift.
>
> This patch allows to correct the affected clock when the SSCG is
> enabled. The check is done in an new optional function related to each
> SoC: is_sscg_enabled(). If this function is not present then no
> correction is done on the clock frequency.

Gregory,

I think computing the SSCG inside the common clk part is a no-go. I
flipped through KW, Dove, Armada 370, and Armada XP FS looking at
SSCG config register layout. Guess what: Dove is different.

How about we have a .get_sscg_deviation() callback instead and move
the one below to some common place where KW, Armada 370, and XP can
set it for their callback struct?

Sebastian

> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>   drivers/clk/mvebu/common.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++
>   drivers/clk/mvebu/common.h |  1 +
>   2 files changed, 75 insertions(+)
>
> diff --git a/drivers/clk/mvebu/common.c b/drivers/clk/mvebu/common.c
> index 25ceccf939ad..834d36cf79b0 100644
> --- a/drivers/clk/mvebu/common.c
> +++ b/drivers/clk/mvebu/common.c
> @@ -26,8 +26,78 @@
>    * Core Clocks
>    */
>
> +#define SSCG_CONF_MODE(reg)	(((reg) >> 16) & 0x3)
> +#define SSCG_SPREAD_DOWN	0x0
> +#define SSCG_SPREAD_UP		0x1
> +#define SSCG_SPREAD_CENTRAL	0x2
> +#define SSCG_CONF_LOW(reg)	(((reg) >> 8) & 0xFF)
> +#define SSCG_CONF_HIGH(reg)	((reg) & 0xFF)
> +
>   static struct clk_onecell_data clk_data;
>
> +static u32 fix_sscg_deviation(struct device_node *np, u32 system_clk)
> +{
> +	struct device_node *sscg_np = NULL;
> +	void __iomem *sscg_map;
> +	u32 sscg_reg;
> +	s32 low_bound, high_bound;
> +	u64 freq_swing_half;
> +
> +	sscg_np = of_find_node_by_name(np, "sscg");
> +	if (sscg_np == NULL) {
> +		pr_err("cannot get SSCG register node\n");
> +		return system_clk;
> +	}
> +
> +	sscg_map = of_iomap(sscg_np, 0);
> +	if (sscg_map == NULL) {
> +		pr_err("cannot map SSCG register\n");
> +		goto out;
> +	}
> +
> +	sscg_reg = readl(sscg_map);
> +	high_bound = SSCG_CONF_HIGH(sscg_reg);
> +	low_bound = SSCG_CONF_LOW(sscg_reg);
> +
> +	if ((high_bound - low_bound) <= 0)
> +		goto out;
> +	/*
> +	 * From the datasheet we got the following formula
> +	 * Spread percentage = 1/96 * (H - L) / H
> +	 * H = SSCG_High_Boundary
> +	 * L = SSCG_Low_Boundary
> +	 *
> +	 * As the deviation is half of spread then it lead to the
> +	 * following formula in the code.
> +	 *
> +	 * To avoid an overflow and not lose any significant digit in
> +	 * the same time we have to use a 64 bit integer.
> +	 */
> +
> +	freq_swing_half = (((u64)high_bound - (u64)low_bound)
> +			* (u64)system_clk);
> +	do_div(freq_swing_half, (2 * 96 * high_bound));
> +
> +	switch (SSCG_CONF_MODE(sscg_reg)) {
> +	case SSCG_SPREAD_DOWN:
> +		system_clk -= freq_swing_half;
> +		break;
> +	case SSCG_SPREAD_UP:
> +		system_clk += freq_swing_half;
> +		break;
> +	case SSCG_SPREAD_CENTRAL:
> +	default:
> +		break;
> +	}
> +
> +	iounmap(sscg_map);
> +
> +out:
> +	of_node_put(sscg_np);
> +
> +	return system_clk;
> +}
> +
>   void __init mvebu_coreclk_setup(struct device_node *np,
>   				const struct coreclk_soc_desc *desc)
>   {
> @@ -62,6 +132,10 @@ void __init mvebu_coreclk_setup(struct device_node *np,
>   	of_property_read_string_index(np, "clock-output-names", 1,
>   				      &cpuclk_name);
>   	rate = desc->get_cpu_freq(base);
> +
> +	if (desc->is_sscg_enabled && desc->is_sscg_enabled(base))
> +		rate = fix_sscg_deviation(np, rate);
> +
>   	clk_data.clks[1] = clk_register_fixed_rate(NULL, cpuclk_name, NULL,
>   						   CLK_IS_ROOT, rate);
>   	WARN_ON(IS_ERR(clk_data.clks[1]));
> diff --git a/drivers/clk/mvebu/common.h b/drivers/clk/mvebu/common.h
> index f968b4d9df92..495f94ff4c90 100644
> --- a/drivers/clk/mvebu/common.h
> +++ b/drivers/clk/mvebu/common.h
> @@ -28,6 +28,7 @@ struct coreclk_soc_desc {
>   	u32 (*get_tclk_freq)(void __iomem *sar);
>   	u32 (*get_cpu_freq)(void __iomem *sar);
>   	void (*get_clk_ratio)(void __iomem *sar, int id, int *mult, int *div);
> +	bool (*is_sscg_enabled)(void __iomem *sar);
>   	const struct coreclk_ratio *ratios;
>   	int num_ratios;
>   };
>

Re: [PATCH 2/4] clk: mvebu: armada-370: Fix timer drift caused by the SSCG deviation

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Fri, 29 Aug 2014 13:43:38 +0200, Gregory CLEMENT wrote:

> +bool a370_is_sscg_enabled(void __iomem *sar)

Minor nit: "static" is missing here.

> +{
> +	return !(readl(sar) & SARL_A370_SSCG_ENABLE);
> +}
> +
>  static const struct coreclk_soc_desc a370_coreclks = {
>  	.get_tclk_freq = a370_get_tclk_freq,
>  	.get_cpu_freq = a370_get_cpu_freq,
>  	.get_clk_ratio = a370_get_clk_ratio,
> +	.is_sscg_enabled = a370_is_sscg_enabled,
>  	.ratios = a370_coreclk_ratios,
>  	.num_ratios = ARRAY_SIZE(a370_coreclk_ratios),
>  };

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 1/4] clk: mvebu: Fix clk frequency value if SSCG is enabled

[Gregory CLEMENT]

Hi Sebastian,

On 29/08/2014 14:48, Sebastian Hesselbarth wrote:
> On 08/29/2014 01:43 PM, Gregory CLEMENT wrote:
>> When the SSCG (Spread Spectrum Clock Generator) is enabled, it shifts
>> the frequency of the clock. The percentage is no more than 1% but when
>> the clock is used for a timer it leads to a clock drift.
>>
>> This patch allows to correct the affected clock when the SSCG is
>> enabled. The check is done in an new optional function related to each
>> SoC: is_sscg_enabled(). If this function is not present then no
>> correction is done on the clock frequency.
> 
> Gregory,
> 
> I think computing the SSCG inside the common clk part is a no-go. I
> flipped through KW, Dove, Armada 370, and Armada XP FS looking at
> SSCG config register layout. Guess what: Dove is different.


argh I checked all the other SoC excepting Dove! Sometime I wonder if
the Dove really belongs to the mvebu family

> 
> How about we have a .get_sscg_deviation() callback instead and move
> the one below to some common place where KW, Armada 370, and XP can
> set it for their callback struct?

I agree. Given the name of thie file the common place will be here.

Thanks,

Gregory

> 
> Sebastian
> 
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> ---
>>   drivers/clk/mvebu/common.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++
>>   drivers/clk/mvebu/common.h |  1 +
>>   2 files changed, 75 insertions(+)
>>
>> diff --git a/drivers/clk/mvebu/common.c b/drivers/clk/mvebu/common.c
>> index 25ceccf939ad..834d36cf79b0 100644
>> --- a/drivers/clk/mvebu/common.c
>> +++ b/drivers/clk/mvebu/common.c
>> @@ -26,8 +26,78 @@
>>    * Core Clocks
>>    */
>>
>> +#define SSCG_CONF_MODE(reg)	(((reg) >> 16) & 0x3)
>> +#define SSCG_SPREAD_DOWN	0x0
>> +#define SSCG_SPREAD_UP		0x1
>> +#define SSCG_SPREAD_CENTRAL	0x2
>> +#define SSCG_CONF_LOW(reg)	(((reg) >> 8) & 0xFF)
>> +#define SSCG_CONF_HIGH(reg)	((reg) & 0xFF)
>> +
>>   static struct clk_onecell_data clk_data;
>>
>> +static u32 fix_sscg_deviation(struct device_node *np, u32 system_clk)
>> +{
>> +	struct device_node *sscg_np = NULL;
>> +	void __iomem *sscg_map;
>> +	u32 sscg_reg;
>> +	s32 low_bound, high_bound;
>> +	u64 freq_swing_half;
>> +
>> +	sscg_np = of_find_node_by_name(np, "sscg");
>> +	if (sscg_np == NULL) {
>> +		pr_err("cannot get SSCG register node\n");
>> +		return system_clk;
>> +	}
>> +
>> +	sscg_map = of_iomap(sscg_np, 0);
>> +	if (sscg_map == NULL) {
>> +		pr_err("cannot map SSCG register\n");
>> +		goto out;
>> +	}
>> +
>> +	sscg_reg = readl(sscg_map);
>> +	high_bound = SSCG_CONF_HIGH(sscg_reg);
>> +	low_bound = SSCG_CONF_LOW(sscg_reg);
>> +
>> +	if ((high_bound - low_bound) <= 0)
>> +		goto out;
>> +	/*
>> +	 * From the datasheet we got the following formula
>> +	 * Spread percentage = 1/96 * (H - L) / H
>> +	 * H = SSCG_High_Boundary
>> +	 * L = SSCG_Low_Boundary
>> +	 *
>> +	 * As the deviation is half of spread then it lead to the
>> +	 * following formula in the code.
>> +	 *
>> +	 * To avoid an overflow and not lose any significant digit in
>> +	 * the same time we have to use a 64 bit integer.
>> +	 */
>> +
>> +	freq_swing_half = (((u64)high_bound - (u64)low_bound)
>> +			* (u64)system_clk);
>> +	do_div(freq_swing_half, (2 * 96 * high_bound));
>> +
>> +	switch (SSCG_CONF_MODE(sscg_reg)) {
>> +	case SSCG_SPREAD_DOWN:
>> +		system_clk -= freq_swing_half;
>> +		break;
>> +	case SSCG_SPREAD_UP:
>> +		system_clk += freq_swing_half;
>> +		break;
>> +	case SSCG_SPREAD_CENTRAL:
>> +	default:
>> +		break;
>> +	}
>> +
>> +	iounmap(sscg_map);
>> +
>> +out:
>> +	of_node_put(sscg_np);
>> +
>> +	return system_clk;
>> +}
>> +
>>   void __init mvebu_coreclk_setup(struct device_node *np,
>>   				const struct coreclk_soc_desc *desc)
>>   {
>> @@ -62,6 +132,10 @@ void __init mvebu_coreclk_setup(struct device_node *np,
>>   	of_property_read_string_index(np, "clock-output-names", 1,
>>   				      &cpuclk_name);
>>   	rate = desc->get_cpu_freq(base);
>> +
>> +	if (desc->is_sscg_enabled && desc->is_sscg_enabled(base))
>> +		rate = fix_sscg_deviation(np, rate);
>> +
>>   	clk_data.clks[1] = clk_register_fixed_rate(NULL, cpuclk_name, NULL,
>>   						   CLK_IS_ROOT, rate);
>>   	WARN_ON(IS_ERR(clk_data.clks[1]));
>> diff --git a/drivers/clk/mvebu/common.h b/drivers/clk/mvebu/common.h
>> index f968b4d9df92..495f94ff4c90 100644
>> --- a/drivers/clk/mvebu/common.h
>> +++ b/drivers/clk/mvebu/common.h
>> @@ -28,6 +28,7 @@ struct coreclk_soc_desc {
>>   	u32 (*get_tclk_freq)(void __iomem *sar);
>>   	u32 (*get_cpu_freq)(void __iomem *sar);
>>   	void (*get_clk_ratio)(void __iomem *sar, int id, int *mult, int *div);
>> +	bool (*is_sscg_enabled)(void __iomem *sar);
>>   	const struct coreclk_ratio *ratios;
>>   	int num_ratios;
>>   };
>>
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 2/4] clk: mvebu: armada-370: Fix timer drift caused by the SSCG deviation

[Gregory CLEMENT]

Hi Thomas,

On 29/08/2014 15:08, Thomas Petazzoni wrote:
> Dear Gregory CLEMENT,
> 
> On Fri, 29 Aug 2014 13:43:38 +0200, Gregory CLEMENT wrote:
> 
>> +bool a370_is_sscg_enabled(void __iomem *sar)
> 
> Minor nit: "static" is missing here.

OK I will fix it in the next version


Thanks,

Gregory


> 
>> +{
>> +	return !(readl(sar) & SARL_A370_SSCG_ENABLE);
>> +}
>> +
>>  static const struct coreclk_soc_desc a370_coreclks = {
>>  	.get_tclk_freq = a370_get_tclk_freq,
>>  	.get_cpu_freq = a370_get_cpu_freq,
>>  	.get_clk_ratio = a370_get_clk_ratio,
>> +	.is_sscg_enabled = a370_is_sscg_enabled,
>>  	.ratios = a370_coreclk_ratios,
>>  	.num_ratios = ARRAY_SIZE(a370_coreclk_ratios),
>>  };
> 
> Thomas
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 1/4] clk: mvebu: Fix clk frequency value if SSCG is enabled

[Leigh Brown]

Hi Gregory,

On 2014-08-29 12:43, Gregory CLEMENT wrote:
> When the SSCG (Spread Spectrum Clock Generator) is enabled, it shifts
> the frequency of the clock. The percentage is no more than 1% but when
> the clock is used for a timer it leads to a clock drift.

Thank you so much for this series.  I'm running 3.16 and without these 
patches ntpd is all over the place, so it is a huge improvement  I do 
have a comment further down though..

> This patch allows to correct the affected clock when the SSCG is
> enabled. The check is done in an new optional function related to each
> SoC: is_sscg_enabled(). If this function is not present then no
> correction is done on the clock frequency.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> ---
>  drivers/clk/mvebu/common.c | 74 
> ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/clk/mvebu/common.h |  1 +
>  2 files changed, 75 insertions(+)
> 
> diff --git a/drivers/clk/mvebu/common.c b/drivers/clk/mvebu/common.c
> index 25ceccf939ad..834d36cf79b0 100644
> --- a/drivers/clk/mvebu/common.c
> +++ b/drivers/clk/mvebu/common.c
> @@ -26,8 +26,78 @@
>   * Core Clocks
>   */
> 
> +#define SSCG_CONF_MODE(reg)	(((reg) >> 16) & 0x3)
> +#define SSCG_SPREAD_DOWN	0x0
> +#define SSCG_SPREAD_UP		0x1
> +#define SSCG_SPREAD_CENTRAL	0x2
> +#define SSCG_CONF_LOW(reg)	(((reg) >> 8) & 0xFF)
> +#define SSCG_CONF_HIGH(reg)	((reg) & 0xFF)
> +
>  static struct clk_onecell_data clk_data;
> 
> +static u32 fix_sscg_deviation(struct device_node *np, u32 system_clk)
> +{
> +	struct device_node *sscg_np = NULL;
> +	void __iomem *sscg_map;
> +	u32 sscg_reg;
> +	s32 low_bound, high_bound;
> +	u64 freq_swing_half;
> +
> +	sscg_np = of_find_node_by_name(np, "sscg");
> +	if (sscg_np == NULL) {
> +		pr_err("cannot get SSCG register node\n");
> +		return system_clk;
> +	}
> +
> +	sscg_map = of_iomap(sscg_np, 0);
> +	if (sscg_map == NULL) {
> +		pr_err("cannot map SSCG register\n");
> +		goto out;
> +	}
> +
> +	sscg_reg = readl(sscg_map);
> +	high_bound = SSCG_CONF_HIGH(sscg_reg);
> +	low_bound = SSCG_CONF_LOW(sscg_reg);
> +
> +	if ((high_bound - low_bound) <= 0)
> +		goto out;
> +	/*
> +	 * From the datasheet we got the following formula
> +	 * Spread percentage = 1/96 * (H - L) / H

In the datasheet it says the percentage is 0.96 * (H - L) / H.  1/96 is 
close but not the same as 0.0096.

> +	 * H = SSCG_High_Boundary
> +	 * L = SSCG_Low_Boundary
> +	 *
> +	 * As the deviation is half of spread then it lead to the
> +	 * following formula in the code.
> +	 *
> +	 * To avoid an overflow and not lose any significant digit in
> +	 * the same time we have to use a 64 bit integer.
> +	 */
> +
> +	freq_swing_half = (((u64)high_bound - (u64)low_bound)
> +			* (u64)system_clk);
> +	do_div(freq_swing_half, (2 * 96 * high_bound));

So this would be become :-

	/* NB: 0.96% = 0.0048 or 3/625 */
	freq_swing_half = (((u64)high_bound - (u64)low_bound)
			* (u64)system_clk * 3);
	do_div(freq_swing_half, (625 * high_bound));

> +
> +	switch (SSCG_CONF_MODE(sscg_reg)) {
> +	case SSCG_SPREAD_DOWN:
> +		system_clk -= freq_swing_half;
> +		break;
> +	case SSCG_SPREAD_UP:
> +		system_clk += freq_swing_half;
> +		break;
> +	case SSCG_SPREAD_CENTRAL:
> +	default:
> +		break;
> +	}
> +
> +	iounmap(sscg_map);
> +
> +out:
> +	of_node_put(sscg_np);
> +
> +	return system_clk;
> +}
> +
>  void __init mvebu_coreclk_setup(struct device_node *np,
>  				const struct coreclk_soc_desc *desc)
>  {
> @@ -62,6 +132,10 @@ void __init mvebu_coreclk_setup(struct device_node 
> *np,
>  	of_property_read_string_index(np, "clock-output-names", 1,
>  				      &cpuclk_name);
>  	rate = desc->get_cpu_freq(base);
> +
> +	if (desc->is_sscg_enabled && desc->is_sscg_enabled(base))
> +		rate = fix_sscg_deviation(np, rate);
> +
>  	clk_data.clks[1] = clk_register_fixed_rate(NULL, cpuclk_name, NULL,
>  						   CLK_IS_ROOT, rate);
>  	WARN_ON(IS_ERR(clk_data.clks[1]));
> diff --git a/drivers/clk/mvebu/common.h b/drivers/clk/mvebu/common.h
> index f968b4d9df92..495f94ff4c90 100644
> --- a/drivers/clk/mvebu/common.h
> +++ b/drivers/clk/mvebu/common.h
> @@ -28,6 +28,7 @@ struct coreclk_soc_desc {
>  	u32 (*get_tclk_freq)(void __iomem *sar);
>  	u32 (*get_cpu_freq)(void __iomem *sar);
>  	void (*get_clk_ratio)(void __iomem *sar, int id, int *mult, int 
> *div);
> +	bool (*is_sscg_enabled)(void __iomem *sar);
>  	const struct coreclk_ratio *ratios;
>  	int num_ratios;
>  };

I don't have a good way to measure the difference but my small 
modification seems correct as per the datasheet.

Regards,

Leigh.

Re: [PATCH 1/4] clk: mvebu: Fix clk frequency value if SSCG is enabled

[Leigh Brown]

On 2014-08-31 23:25, Leigh Brown wrote:
[...]
> /* NB: 0.96% = 0.0048 or 3/625 */
> freq_swing_half = (((u64)high_bound - (u64)low_bound)
> 		* (u64)system_clk * 3);
> do_div(freq_swing_half, (625 * high_bound));

Sigh.  The comment should be:

/* NB: half of 0.96% is 0.0048 or 3/625 */

The code should be right :-)

Regards,

Leigh.

Re: [PATCH 1/4] clk: mvebu: Fix clk frequency value if SSCG is enabled

[Gregory CLEMENT]

Hi Leigh,

On 01/09/2014 00:25, Leigh Brown wrote:
> Hi Gregory,
> 
> On 2014-08-29 12:43, Gregory CLEMENT wrote:
>> When the SSCG (Spread Spectrum Clock Generator) is enabled, it shifts
>> the frequency of the clock. The percentage is no more than 1% but when
>> the clock is used for a timer it leads to a clock drift.
> 
> Thank you so much for this series.  I'm running 3.16 and without these 
> patches ntpd is all over the place, so it is a huge improvement  I do 
> have a comment further down though..
> 
>> This patch allows to correct the affected clock when the SSCG is
>> enabled. The check is done in an new optional function related to each
>> SoC: is_sscg_enabled(). If this function is not present then no
>> correction is done on the clock frequency.
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> ---
>>  drivers/clk/mvebu/common.c | 74 
>> ++++++++++++++++++++++++++++++++++++++++++++++
>>  drivers/clk/mvebu/common.h |  1 +
>>  2 files changed, 75 insertions(+)
>>
>> diff --git a/drivers/clk/mvebu/common.c b/drivers/clk/mvebu/common.c
>> index 25ceccf939ad..834d36cf79b0 100644
>> --- a/drivers/clk/mvebu/common.c
>> +++ b/drivers/clk/mvebu/common.c
>> @@ -26,8 +26,78 @@
>>   * Core Clocks
>>   */
>>
>> +#define SSCG_CONF_MODE(reg)	(((reg) >> 16) & 0x3)
>> +#define SSCG_SPREAD_DOWN	0x0
>> +#define SSCG_SPREAD_UP		0x1
>> +#define SSCG_SPREAD_CENTRAL	0x2
>> +#define SSCG_CONF_LOW(reg)	(((reg) >> 8) & 0xFF)
>> +#define SSCG_CONF_HIGH(reg)	((reg) & 0xFF)
>> +
>>  static struct clk_onecell_data clk_data;
>>
>> +static u32 fix_sscg_deviation(struct device_node *np, u32 system_clk)
>> +{
>> +	struct device_node *sscg_np = NULL;
>> +	void __iomem *sscg_map;
>> +	u32 sscg_reg;
>> +	s32 low_bound, high_bound;
>> +	u64 freq_swing_half;
>> +
>> +	sscg_np = of_find_node_by_name(np, "sscg");
>> +	if (sscg_np == NULL) {
>> +		pr_err("cannot get SSCG register node\n");
>> +		return system_clk;
>> +	}
>> +
>> +	sscg_map = of_iomap(sscg_np, 0);
>> +	if (sscg_map == NULL) {
>> +		pr_err("cannot map SSCG register\n");
>> +		goto out;
>> +	}
>> +
>> +	sscg_reg = readl(sscg_map);
>> +	high_bound = SSCG_CONF_HIGH(sscg_reg);
>> +	low_bound = SSCG_CONF_LOW(sscg_reg);
>> +
>> +	if ((high_bound - low_bound) <= 0)
>> +		goto out;
>> +	/*
>> +	 * From the datasheet we got the following formula
>> +	 * Spread percentage = 1/96 * (H - L) / H
> 
> In the datasheet it says the percentage is 0.96 * (H - L) / H.  1/96 is 
> close but not the same as 0.0096.

Actually the public datasheet is wrong (even the NDA datasheet).

> 
>> +	 * H = SSCG_High_Boundary
>> +	 * L = SSCG_Low_Boundary
>> +	 *
>> +	 * As the deviation is half of spread then it lead to the
>> +	 * following formula in the code.
>> +	 *
>> +	 * To avoid an overflow and not lose any significant digit in
>> +	 * the same time we have to use a 64 bit integer.
>> +	 */
>> +
>> +	freq_swing_half = (((u64)high_bound - (u64)low_bound)
>> +			* (u64)system_clk);
>> +	do_div(freq_swing_half, (2 * 96 * high_bound));
> 
> So this would be become :-
> 
> 	/* NB: 0.96% = 0.0048 or 3/625 */
> 	freq_swing_half = (((u64)high_bound - (u64)low_bound)
> 			* (u64)system_clk * 3);
> 	do_div(freq_swing_half, (625 * high_bound));

It was the first implementation I tried, but with this one
the drift was about 200ppm instead of 50ppm. After having
checked this with the Marvell engineers, they confirmed me
that the datasheet was erroneous.


Thanks for your feedback: I will add a comment in the code
about the fact that the datasheet is erroneous.



-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com