[PATCH 0/2] clk: Fix switching CPU rate from 300Mhz to 1.2GHz on Armada 3700

[PATCH 0/2] clk: Fix switching CPU rate from 300Mhz to 1.2GHz on Armada 3700

[Gregory CLEMENT]

Hello,

in this series the first patch is the main patch which fixes the case
when switching CPU rate from 300Mhz (or 200M MHz) to 1.2GH. See the
commit log for the details. As this one is a fix, it should be applied
on v4.18-rc.

While I was on this file, I also switch to SPDX license identifier,
this one is fine for 4.19*;

Thanks,

Gregory

Gregory CLEMENT (2):
  clk: mvebu: armada-37xx-periph: Fix switching CPU rate from 300Mhz to
    1.2GHz
  clk: mvebu: armada-37xx-periph: switch to SPDX license identifier

 drivers/clk/mvebu/armada-37xx-periph.c | 43 +++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 4 deletions(-)

-- 
2.17.1

[PATCH 1/2] clk: mvebu: armada-37xx-periph: Fix switching CPU rate from 300Mhz to 1.2GHz

[Gregory CLEMENT]

Switching the CPU from the L2 or L3 frequencies (300 and 200 Mhz
respectively) to L0 frequency (1.2 Ghz) requires a significant amount
of time to let VDD stabilize to the appropriate voltage. This amount of
time is large enough that it cannot be covered by the hardware
countdown register. Due to this, the CPU might start operating at L0
before the voltage is stabilized, leading to CPU stalls.

To work around this problem, we prevent switching directly from the
L2/L3 frequencies to the L0 frequency, and instead switch to the L1
frequency in-between. The sequence therefore becomes:

1. First switch from L2/L3(200/300MHz) to L1(600MHZ)
2. Sleep 20ms for stabling VDD voltage
3. Then switch from L1(600MHZ) to L0(1200Mhz).

It is based on the work done by Ken Ma <make@marvell.com>

Cc: stable at vger.kernel.org
Fixes: 2089dc33ea0e ("clk: mvebu: armada-37xx-periph: add DVFS support for cpu clocks")
Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---
 drivers/clk/mvebu/armada-37xx-periph.c | 38 ++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
index 6860bd5a37c5..44e4e27eddad 100644
--- a/drivers/clk/mvebu/armada-37xx-periph.c
+++ b/drivers/clk/mvebu/armada-37xx-periph.c
@@ -35,6 +35,7 @@
 #define CLK_SEL		0x10
 #define CLK_DIS		0x14
 
+#define  ARMADA_37XX_DVFS_LOAD_1 1
 #define LOAD_LEVEL_NR	4
 
 #define ARMADA_37XX_NB_L0L1	0x18
@@ -507,6 +508,40 @@ static long clk_pm_cpu_round_rate(struct clk_hw *hw, unsigned long rate,
 	return -EINVAL;
 }
 
+/*
+ * Switching the CPU from the L2 or L3 frequencies (300 and 200 Mhz
+ * respectively) to L0 frequency (1.2 Ghz) requires a significant
+ * amount of time to let VDD stabilize to the appropriate
+ * voltage. This amount of time is large enough that it cannot be
+ * covered by the hardware countdown register. Due to this, the CPU
+ * might start operating at L0 before the voltage is stabilized,
+ * leading to CPU stalls.
+ *
+ * To work around this problem, we prevent switching directly from the
+ * L2/L3 frequencies to the L0 frequency, and instead switch to the L1
+ * frequency in-between. The sequence therefore becomes:
+ * 1. First switch from L2/L3(200/300MHz) to L1(600MHZ)
+ * 2. Sleep 20ms for stabling VDD voltage
+ * 3. Then switch from L1(600MHZ) to L0(1200Mhz).
+ */
+static void clk_pm_cpu_set_rate_wa(unsigned long rate, struct regmap *base)
+{
+	unsigned int cur_level;
+
+	if (rate != 1200 * 1000 * 1000)
+		return;
+
+	regmap_read(base, ARMADA_37XX_NB_CPU_LOAD, &cur_level);
+	cur_level &= ARMADA_37XX_NB_CPU_LOAD_MASK;
+	if (cur_level <= ARMADA_37XX_DVFS_LOAD_1)
+		return;
+
+	regmap_update_bits(base, ARMADA_37XX_NB_CPU_LOAD,
+			   ARMADA_37XX_NB_CPU_LOAD_MASK,
+			   ARMADA_37XX_DVFS_LOAD_1);
+	msleep(20);
+}
+
 static int clk_pm_cpu_set_rate(struct clk_hw *hw, unsigned long rate,
 			       unsigned long parent_rate)
 {
@@ -537,6 +572,9 @@ static int clk_pm_cpu_set_rate(struct clk_hw *hw, unsigned long rate,
 			 */
 			reg = ARMADA_37XX_NB_CPU_LOAD;
 			mask = ARMADA_37XX_NB_CPU_LOAD_MASK;
+
+			clk_pm_cpu_set_rate_wa(rate, base);
+
 			regmap_update_bits(base, reg, mask, load_level);
 
 			return rate;
-- 
2.17.1

[PATCH 2/2] clk: mvebu: armada-37xx-periph: switch to SPDX license identifier

[Gregory CLEMENT]

Adopt the SPDX license identifier headers to ease license compliance
management.

Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---
 drivers/clk/mvebu/armada-37xx-periph.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
index 44e4e27eddad..a51edaab0c5c 100644
--- a/drivers/clk/mvebu/armada-37xx-periph.c
+++ b/drivers/clk/mvebu/armada-37xx-periph.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0+
 /*
  * Marvell Armada 37xx SoC Peripheral clocks
  *
@@ -5,10 +6,6 @@
  *
  * Gregory CLEMENT <gregory.clement@free-electrons.com>
  *
- * This file is licensed under the terms of the GNU General Public
- * License version 2 or later. This program is licensed "as is"
- * without any warranty of any kind, whether express or implied.
- *
  * Most of the peripheral clocks can be modelled like this:
  *             _____    _______    _______
  * TBG-A-P  --|     |  |       |  |       |   ______
-- 
2.17.1

[PATCH 1/2] clk: mvebu: armada-37xx-periph: Fix switching CPU rate from 300Mhz to 1.2GHz

[Gregory CLEMENT]

Hi,
 
 On mar., juin 19 2018, Gregory CLEMENT <gregory.clement@bootlin.com> wrote:

> Switching the CPU from the L2 or L3 frequencies (300 and 200 Mhz
> respectively) to L0 frequency (1.2 Ghz) requires a significant amount
> of time to let VDD stabilize to the appropriate voltage. This amount of
> time is large enough that it cannot be covered by the hardware
> countdown register. Due to this, the CPU might start operating at L0
> before the voltage is stabilized, leading to CPU stalls.
>
> To work around this problem, we prevent switching directly from the
> L2/L3 frequencies to the L0 frequency, and instead switch to the L1
> frequency in-between. The sequence therefore becomes:
>
> 1. First switch from L2/L3(200/300MHz) to L1(600MHZ)
> 2. Sleep 20ms for stabling VDD voltage
> 3. Then switch from L1(600MHZ) to L0(1200Mhz).

Do you have any comment on this fix?

Gregory

>
> It is based on the work done by Ken Ma <make@marvell.com>
>
> Cc: stable at vger.kernel.org
> Fixes: 2089dc33ea0e ("clk: mvebu: armada-37xx-periph: add DVFS support for cpu clocks")
> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> ---
>  drivers/clk/mvebu/armada-37xx-periph.c | 38 ++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>
> diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
> index 6860bd5a37c5..44e4e27eddad 100644
> --- a/drivers/clk/mvebu/armada-37xx-periph.c
> +++ b/drivers/clk/mvebu/armada-37xx-periph.c
> @@ -35,6 +35,7 @@
>  #define CLK_SEL		0x10
>  #define CLK_DIS		0x14
>  
> +#define  ARMADA_37XX_DVFS_LOAD_1 1
>  #define LOAD_LEVEL_NR	4
>  
>  #define ARMADA_37XX_NB_L0L1	0x18
> @@ -507,6 +508,40 @@ static long clk_pm_cpu_round_rate(struct clk_hw *hw, unsigned long rate,
>  	return -EINVAL;
>  }
>  
> +/*
> + * Switching the CPU from the L2 or L3 frequencies (300 and 200 Mhz
> + * respectively) to L0 frequency (1.2 Ghz) requires a significant
> + * amount of time to let VDD stabilize to the appropriate
> + * voltage. This amount of time is large enough that it cannot be
> + * covered by the hardware countdown register. Due to this, the CPU
> + * might start operating at L0 before the voltage is stabilized,
> + * leading to CPU stalls.
> + *
> + * To work around this problem, we prevent switching directly from the
> + * L2/L3 frequencies to the L0 frequency, and instead switch to the L1
> + * frequency in-between. The sequence therefore becomes:
> + * 1. First switch from L2/L3(200/300MHz) to L1(600MHZ)
> + * 2. Sleep 20ms for stabling VDD voltage
> + * 3. Then switch from L1(600MHZ) to L0(1200Mhz).
> + */
> +static void clk_pm_cpu_set_rate_wa(unsigned long rate, struct regmap *base)
> +{
> +	unsigned int cur_level;
> +
> +	if (rate != 1200 * 1000 * 1000)
> +		return;
> +
> +	regmap_read(base, ARMADA_37XX_NB_CPU_LOAD, &cur_level);
> +	cur_level &= ARMADA_37XX_NB_CPU_LOAD_MASK;
> +	if (cur_level <= ARMADA_37XX_DVFS_LOAD_1)
> +		return;
> +
> +	regmap_update_bits(base, ARMADA_37XX_NB_CPU_LOAD,
> +			   ARMADA_37XX_NB_CPU_LOAD_MASK,
> +			   ARMADA_37XX_DVFS_LOAD_1);
> +	msleep(20);
> +}
> +
>  static int clk_pm_cpu_set_rate(struct clk_hw *hw, unsigned long rate,
>  			       unsigned long parent_rate)
>  {
> @@ -537,6 +572,9 @@ static int clk_pm_cpu_set_rate(struct clk_hw *hw, unsigned long rate,
>  			 */
>  			reg = ARMADA_37XX_NB_CPU_LOAD;
>  			mask = ARMADA_37XX_NB_CPU_LOAD_MASK;
> +
> +			clk_pm_cpu_set_rate_wa(rate, base);
> +
>  			regmap_update_bits(base, reg, mask, load_level);
>  
>  			return rate;
> -- 
> 2.17.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Gregory Clement, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

[PATCH 1/2] clk: mvebu: armada-37xx-periph: Fix switching CPU rate from 300Mhz to 1.2GHz

[Stephen Boyd]

Quoting Gregory CLEMENT (2018-06-29 07:44:02)
> Hi,
>  
>  On mar., juin 19 2018, Gregory CLEMENT <gregory.clement@bootlin.com> wrote:
> 
> > Switching the CPU from the L2 or L3 frequencies (300 and 200 Mhz
> > respectively) to L0 frequency (1.2 Ghz) requires a significant amount
> > of time to let VDD stabilize to the appropriate voltage. This amount of
> > time is large enough that it cannot be covered by the hardware
> > countdown register. Due to this, the CPU might start operating at L0
> > before the voltage is stabilized, leading to CPU stalls.
> >
> > To work around this problem, we prevent switching directly from the
> > L2/L3 frequencies to the L0 frequency, and instead switch to the L1
> > frequency in-between. The sequence therefore becomes:
> >
> > 1. First switch from L2/L3(200/300MHz) to L1(600MHZ)
> > 2. Sleep 20ms for stabling VDD voltage
> > 3. Then switch from L1(600MHZ) to L0(1200Mhz).
> 
> Do you have any comment on this fix?
> 

Looks good. Is it crashing right now? I can throw it into clk-fixes if
it's fixing pain.

[PATCH 1/2] clk: mvebu: armada-37xx-periph: Fix switching CPU rate from 300Mhz to 1.2GHz

[Gregory CLEMENT]

Hi Stephen,
 
 On ven., juil. 06 2018, Stephen Boyd <sboyd@kernel.org> wrote:

> Quoting Gregory CLEMENT (2018-06-29 07:44:02)
>> Hi,
>>  
>>  On mar., juin 19 2018, Gregory CLEMENT <gregory.clement@bootlin.com> wrote:
>> 
>> > Switching the CPU from the L2 or L3 frequencies (300 and 200 Mhz
>> > respectively) to L0 frequency (1.2 Ghz) requires a significant amount
>> > of time to let VDD stabilize to the appropriate voltage. This amount of
>> > time is large enough that it cannot be covered by the hardware
>> > countdown register. Due to this, the CPU might start operating at L0
>> > before the voltage is stabilized, leading to CPU stalls.
>> >
>> > To work around this problem, we prevent switching directly from the
>> > L2/L3 frequencies to the L0 frequency, and instead switch to the L1
>> > frequency in-between. The sequence therefore becomes:
>> >
>> > 1. First switch from L2/L3(200/300MHz) to L1(600MHZ)
>> > 2. Sleep 20ms for stabling VDD voltage
>> > 3. Then switch from L1(600MHZ) to L0(1200Mhz).
>> 
>> Do you have any comment on this fix?
>> 
>
> Looks good. Is it crashing right now? I can throw it into clk-fixes if
> it's fixing pain.

Yes for the SoC capable to run at 1200Mhz it crashes. At the time the
initial support was provided, my SoC version only ran at 1000MHz, so the
problem was not noticed.

Thanks,

Gregory


-- 
Gregory Clement, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

[PATCH 1/2] clk: mvebu: armada-37xx-periph: Fix switching CPU rate from 300Mhz to 1.2GHz

[Stephen Boyd]

Quoting Gregory CLEMENT (2018-07-09 08:42:31)
> Hi Stephen,
>  
>  On ven., juil. 06 2018, Stephen Boyd <sboyd@kernel.org> wrote:
> 
> > Quoting Gregory CLEMENT (2018-06-29 07:44:02)
> >> Hi,
> >>  
> >>  On mar., juin 19 2018, Gregory CLEMENT <gregory.clement@bootlin.com> wrote:
> >> 
> >> > Switching the CPU from the L2 or L3 frequencies (300 and 200 Mhz
> >> > respectively) to L0 frequency (1.2 Ghz) requires a significant amount
> >> > of time to let VDD stabilize to the appropriate voltage. This amount of
> >> > time is large enough that it cannot be covered by the hardware
> >> > countdown register. Due to this, the CPU might start operating at L0
> >> > before the voltage is stabilized, leading to CPU stalls.
> >> >
> >> > To work around this problem, we prevent switching directly from the
> >> > L2/L3 frequencies to the L0 frequency, and instead switch to the L1
> >> > frequency in-between. The sequence therefore becomes:
> >> >
> >> > 1. First switch from L2/L3(200/300MHz) to L1(600MHZ)
> >> > 2. Sleep 20ms for stabling VDD voltage
> >> > 3. Then switch from L1(600MHZ) to L0(1200Mhz).
> >> 
> >> Do you have any comment on this fix?
> >> 
> >
> > Looks good. Is it crashing right now? I can throw it into clk-fixes if
> > it's fixing pain.
> 
> Yes for the SoC capable to run at 1200Mhz it crashes. At the time the
> initial support was provided, my SoC version only ran at 1000MHz, so the
> problem was not noticed.
> 

Ok. Applied to clk-fixes.

[PATCH 2/2] clk: mvebu: armada-37xx-periph: switch to SPDX license identifier

[Stephen Boyd]

Quoting Gregory CLEMENT (2018-06-19 05:34:46)
> Adopt the SPDX license identifier headers to ease license compliance
> management.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> ---

Applied to clk-next