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

[Gregory CLEMENT <gregory.clement@bootlin.com>]

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@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@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