diff mbox series

[v2,1/4] clk: imx: lpcg-scu: SW workaround for errata (e10858)

Message ID 20241018-imx-clk-v1-v2-1-92c0b66ca970@nxp.com (mailing list archive)
State New, archived
Headers show
Series clk: imx: scu and fracn pll update | expand

Commit Message

Peng Fan (OSS) Oct. 18, 2024, 10 a.m. UTC 
From: Peng Fan <peng.fan@nxp.com>

Back-to-back LPCG writes can be ignored by the LPCG register due to
a HW bug. The writes need to be separated by at least 4 cycles of
the gated clock. See https://www.nxp.com.cn/docs/en/errata/IMX8_1N94W.pdf

The workaround is implemented as follows:
1. For clocks running greater than or equal to 24MHz, a read
followed by the write will provide sufficient delay.
2. For clocks running below 24MHz, add a delay of 4 clock cylces
after the write to the LPCG register.

Fixes: 2f77296d3df9 ("clk: imx: add lpcg clock support")
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/clk/imx/clk-lpcg-scu.c | 35 +++++++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)

Comments

Abel Vesa Oct. 22, 2024, 2:29 p.m. UTC | #1 
On 24-10-18 18:00:55, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Back-to-back LPCG writes can be ignored by the LPCG register due to
> a HW bug. The writes need to be separated by at least 4 cycles of
> the gated clock. See https://www.nxp.com.cn/docs/en/errata/IMX8_1N94W.pdf
> 
> The workaround is implemented as follows:
> 1. For clocks running greater than or equal to 24MHz, a read
> followed by the write will provide sufficient delay.
> 2. For clocks running below 24MHz, add a delay of 4 clock cylces
> after the write to the LPCG register.
> 
> Fixes: 2f77296d3df9 ("clk: imx: add lpcg clock support")
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/clk/imx/clk-lpcg-scu.c | 35 +++++++++++++++++++++++++++++++----
>  1 file changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/imx/clk-lpcg-scu.c b/drivers/clk/imx/clk-lpcg-scu.c
> index dd5abd09f3e206a5073767561b517d5b3320b28c..cd42190233662c66f2c354f0a2eee3a2531eeb3a 100644
> --- a/drivers/clk/imx/clk-lpcg-scu.c
> +++ b/drivers/clk/imx/clk-lpcg-scu.c
> @@ -6,10 +6,12 @@
>  
>  #include <linux/bits.h>
>  #include <linux/clk-provider.h>
> +#include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/io.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
> +#include <linux/units.h>
>  
>  #include "clk-scu.h"
>  
> @@ -41,6 +43,31 @@ struct clk_lpcg_scu {
>  
>  #define to_clk_lpcg_scu(_hw) container_of(_hw, struct clk_lpcg_scu, hw)
>  
> +/* e10858 -LPCG clock gating register synchronization errata */
> +static void lpcg_e10858_writel(unsigned long rate, void __iomem *reg, u32 val)
> +{
> +	u32 reg1;
> +
> +	writel(val, reg);
> +
> +	if (rate >= 24 * HZ_PER_MHZ || rate == 0) {
> +		/*
> +		 * The time taken to access the LPCG registers from the AP core
> +		 * through the interconnect is longer than the minimum delay
> +		 * of 4 clock cycles required by the errata.
> +		 * Adding a readl will provide sufficient delay to prevent
> +		 * back-to-back writes.
> +		 */
> +		reg1 = readl(reg);
> +	} else {
> +		/*
> +		 * For clocks running below 24MHz, wait a minimum of
> +		 * 4 clock cycles.
> +		 */
> +		ndelay(4 * (DIV_ROUND_UP(1000 * HZ_PER_MHZ, rate)));
> +	}
> +}
> +
>  static int clk_lpcg_scu_enable(struct clk_hw *hw)
>  {
>  	struct clk_lpcg_scu *clk = to_clk_lpcg_scu(hw);
> @@ -57,7 +84,8 @@ static int clk_lpcg_scu_enable(struct clk_hw *hw)
>  		val |= CLK_GATE_SCU_LPCG_HW_SEL;
>  
>  	reg |= val << clk->bit_idx;
> -	writel(reg, clk->reg);
> +
> +	lpcg_e10858_writel(clk_hw_get_rate(hw), clk->reg, reg);
>  
>  	spin_unlock_irqrestore(&imx_lpcg_scu_lock, flags);
>  
> @@ -74,7 +102,7 @@ static void clk_lpcg_scu_disable(struct clk_hw *hw)
>  
>  	reg = readl_relaxed(clk->reg);
>  	reg &= ~(CLK_GATE_SCU_LPCG_MASK << clk->bit_idx);
> -	writel(reg, clk->reg);
> +	lpcg_e10858_writel(clk_hw_get_rate(hw), clk->reg, reg);
>  
>  	spin_unlock_irqrestore(&imx_lpcg_scu_lock, flags);
>  }
> @@ -149,9 +177,8 @@ static int __maybe_unused imx_clk_lpcg_scu_resume(struct device *dev)
>  	 * FIXME: Sometimes writes don't work unless the CPU issues
>  	 * them twice
>  	 */
> -
> -	writel(clk->state, clk->reg);

Now that you removed one of the writes, doesn't the comment above has to
be removed as well ?

>  	writel(clk->state, clk->reg);
> +	lpcg_e10858_writel(0, clk->reg, clk->state);
>  	dev_dbg(dev, "restore lpcg state 0x%x\n", clk->state);
>  
>  	return 0;
> 
> -- 
> 2.37.1
>
kernel test robot Oct. 22, 2024, 6:09 p.m. UTC | #2 
Hi Peng,

kernel test robot noticed the following build warnings:

[auto build test WARNING on d61a00525464bfc5fe92c6ad713350988e492b88]

url:    https://github.com/intel-lab-lkp/linux/commits/Peng-Fan-OSS/clk-imx-lpcg-scu-SW-workaround-for-errata-e10858/20241018-175440
base:   d61a00525464bfc5fe92c6ad713350988e492b88
patch link:    https://lore.kernel.org/r/20241018-imx-clk-v1-v2-1-92c0b66ca970%40nxp.com
patch subject: [PATCH v2 1/4] clk: imx: lpcg-scu: SW workaround for errata (e10858)
config: arm64-randconfig-r061-20241022 (https://download.01.org/0day-ci/archive/20241023/202410230141.3xLvkclt-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241023/202410230141.3xLvkclt-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410230141.3xLvkclt-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/clk/imx/clk-lpcg-scu.c: In function 'lpcg_e10858_writel':
>> drivers/clk/imx/clk-lpcg-scu.c:49:13: warning: variable 'reg1' set but not used [-Wunused-but-set-variable]
      49 |         u32 reg1;
         |             ^~~~


vim +/reg1 +49 drivers/clk/imx/clk-lpcg-scu.c

    45	
    46	/* e10858 -LPCG clock gating register synchronization errata */
    47	static void lpcg_e10858_writel(unsigned long rate, void __iomem *reg, u32 val)
    48	{
  > 49		u32 reg1;
    50	
    51		writel(val, reg);
    52	
    53		if (rate >= 24 * HZ_PER_MHZ || rate == 0) {
    54			/*
    55			 * The time taken to access the LPCG registers from the AP core
    56			 * through the interconnect is longer than the minimum delay
    57			 * of 4 clock cycles required by the errata.
    58			 * Adding a readl will provide sufficient delay to prevent
    59			 * back-to-back writes.
    60			 */
    61			reg1 = readl(reg);
    62		} else {
    63			/*
    64			 * For clocks running below 24MHz, wait a minimum of
    65			 * 4 clock cycles.
    66			 */
    67			ndelay(4 * (DIV_ROUND_UP(1000 * HZ_PER_MHZ, rate)));
    68		}
    69	}
    70
Peng Fan Oct. 23, 2024, 3:15 a.m. UTC | #3 
> Subject: Re: [PATCH v2 1/4] clk: imx: lpcg-scu: SW workaround for
> errata (e10858)
> 
> On 24-10-18 18:00:55, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Back-to-back LPCG writes can be ignored by the LPCG register due to
> a
> > HW bug. The writes need to be separated by at least 4 cycles of the
> > gated clock. See
> >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> www.
> >
> nxp.com.cn%2Fdocs%2Fen%2Ferrata%2FIMX8_1N94W.pdf&data=05%
> 7C02%7Cpeng.f
> >
> an%40nxp.com%7Cbe85b7f113724069f20408dcf2a5eb47%7C686ea1
> d3bc2b4c6fa92c
> >
> d99c5c301635%7C0%7C1%7C638652041660547699%7CUnknown%7
> CTWFpbGZsb3d8eyJW
> >
> IjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%
> 3D%7C0%7C%
> >
> 7C%7C&sdata=YYgvBob0hm1poDDzb%2FEo6uRo1G29a0jmhqzhwr%2B
> Hzcg%3D&reserve
> > d=0
> >
> > The workaround is implemented as follows:
> > 1. For clocks running greater than or equal to 24MHz, a read followed
> > by the write will provide sufficient delay.
> > 2. For clocks running below 24MHz, add a delay of 4 clock cylces
> after
> > the write to the LPCG register.
> >
> > Fixes: 2f77296d3df9 ("clk: imx: add lpcg clock support")
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/clk/imx/clk-lpcg-scu.c | 35
> > +++++++++++++++++++++++++++++++----
> >  1 file changed, 31 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/clk/imx/clk-lpcg-scu.c
> > b/drivers/clk/imx/clk-lpcg-scu.c index
> >
> dd5abd09f3e206a5073767561b517d5b3320b28c..cd42190233662c6
> 6f2c354f0a2ee
> > e3a2531eeb3a 100644
> > --- a/drivers/clk/imx/clk-lpcg-scu.c
> > +++ b/drivers/clk/imx/clk-lpcg-scu.c
> > @@ -6,10 +6,12 @@
> >
> >  #include <linux/bits.h>
> >  #include <linux/clk-provider.h>
> > +#include <linux/delay.h>
> >  #include <linux/err.h>
> >  #include <linux/io.h>
> >  #include <linux/slab.h>
> >  #include <linux/spinlock.h>
> > +#include <linux/units.h>
> >
> >  #include "clk-scu.h"
> >
> > @@ -41,6 +43,31 @@ struct clk_lpcg_scu {
> >
> >  #define to_clk_lpcg_scu(_hw) container_of(_hw, struct clk_lpcg_scu,
> > hw)
> >
> > +/* e10858 -LPCG clock gating register synchronization errata */
> > +static void lpcg_e10858_writel(unsigned long rate, void __iomem
> *reg,
> > +u32 val) {
> > +	u32 reg1;
> > +
> > +	writel(val, reg);
> > +
> > +	if (rate >= 24 * HZ_PER_MHZ || rate == 0) {
> > +		/*
> > +		 * The time taken to access the LPCG registers from
> the AP core
> > +		 * through the interconnect is longer than the
> minimum delay
> > +		 * of 4 clock cycles required by the errata.
> > +		 * Adding a readl will provide sufficient delay to
> prevent
> > +		 * back-to-back writes.
> > +		 */
> > +		reg1 = readl(reg);
> > +	} else {
> > +		/*
> > +		 * For clocks running below 24MHz, wait a minimum
> of
> > +		 * 4 clock cycles.
> > +		 */
> > +		ndelay(4 * (DIV_ROUND_UP(1000 * HZ_PER_MHZ,
> rate)));
> > +	}
> > +}
> > +
> >  static int clk_lpcg_scu_enable(struct clk_hw *hw)  {
> >  	struct clk_lpcg_scu *clk = to_clk_lpcg_scu(hw); @@ -57,7
> +84,8 @@
> > static int clk_lpcg_scu_enable(struct clk_hw *hw)
> >  		val |= CLK_GATE_SCU_LPCG_HW_SEL;
> >
> >  	reg |= val << clk->bit_idx;
> > -	writel(reg, clk->reg);
> > +
> > +	lpcg_e10858_writel(clk_hw_get_rate(hw), clk->reg, reg);
> >
> >  	spin_unlock_irqrestore(&imx_lpcg_scu_lock, flags);
> >
> > @@ -74,7 +102,7 @@ static void clk_lpcg_scu_disable(struct clk_hw
> *hw)
> >
> >  	reg = readl_relaxed(clk->reg);
> >  	reg &= ~(CLK_GATE_SCU_LPCG_MASK << clk->bit_idx);
> > -	writel(reg, clk->reg);
> > +	lpcg_e10858_writel(clk_hw_get_rate(hw), clk->reg, reg);
> >
> >  	spin_unlock_irqrestore(&imx_lpcg_scu_lock, flags);  } @@ -
> 149,9
> > +177,8 @@ static int __maybe_unused
> imx_clk_lpcg_scu_resume(struct device *dev)
> >  	 * FIXME: Sometimes writes don't work unless the CPU issues
> >  	 * them twice
> >  	 */
> > -
> > -	writel(clk->state, clk->reg);
> 
> Now that you removed one of the writes, doesn't the comment above
> has to be removed as well ?

Sure, let me remove it.

Thanks,
Peng
> 
> >  	writel(clk->state, clk->reg);
> > +	lpcg_e10858_writel(0, clk->reg, clk->state);
> >  	dev_dbg(dev, "restore lpcg state 0x%x\n", clk->state);
> >
> >  	return 0;
> >
> > --
> > 2.37.1
> >
diff mbox series

Patch

diff --git a/drivers/clk/imx/clk-lpcg-scu.c b/drivers/clk/imx/clk-lpcg-scu.c
index dd5abd09f3e206a5073767561b517d5b3320b28c..cd42190233662c66f2c354f0a2eee3a2531eeb3a 100644
--- a/drivers/clk/imx/clk-lpcg-scu.c
+++ b/drivers/clk/imx/clk-lpcg-scu.c
@@ -6,10 +6,12 @@ 
 
 #include <linux/bits.h>
 #include <linux/clk-provider.h>
+#include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
+#include <linux/units.h>
 
 #include "clk-scu.h"
 
@@ -41,6 +43,31 @@  struct clk_lpcg_scu {
 
 #define to_clk_lpcg_scu(_hw) container_of(_hw, struct clk_lpcg_scu, hw)
 
+/* e10858 -LPCG clock gating register synchronization errata */
+static void lpcg_e10858_writel(unsigned long rate, void __iomem *reg, u32 val)
+{
+	u32 reg1;
+
+	writel(val, reg);
+
+	if (rate >= 24 * HZ_PER_MHZ || rate == 0) {
+		/*
+		 * The time taken to access the LPCG registers from the AP core
+		 * through the interconnect is longer than the minimum delay
+		 * of 4 clock cycles required by the errata.
+		 * Adding a readl will provide sufficient delay to prevent
+		 * back-to-back writes.
+		 */
+		reg1 = readl(reg);
+	} else {
+		/*
+		 * For clocks running below 24MHz, wait a minimum of
+		 * 4 clock cycles.
+		 */
+		ndelay(4 * (DIV_ROUND_UP(1000 * HZ_PER_MHZ, rate)));
+	}
+}
+
 static int clk_lpcg_scu_enable(struct clk_hw *hw)
 {
 	struct clk_lpcg_scu *clk = to_clk_lpcg_scu(hw);
@@ -57,7 +84,8 @@  static int clk_lpcg_scu_enable(struct clk_hw *hw)
 		val |= CLK_GATE_SCU_LPCG_HW_SEL;
 
 	reg |= val << clk->bit_idx;
-	writel(reg, clk->reg);
+
+	lpcg_e10858_writel(clk_hw_get_rate(hw), clk->reg, reg);
 
 	spin_unlock_irqrestore(&imx_lpcg_scu_lock, flags);
 
@@ -74,7 +102,7 @@  static void clk_lpcg_scu_disable(struct clk_hw *hw)
 
 	reg = readl_relaxed(clk->reg);
 	reg &= ~(CLK_GATE_SCU_LPCG_MASK << clk->bit_idx);
-	writel(reg, clk->reg);
+	lpcg_e10858_writel(clk_hw_get_rate(hw), clk->reg, reg);
 
 	spin_unlock_irqrestore(&imx_lpcg_scu_lock, flags);
 }
@@ -149,9 +177,8 @@  static int __maybe_unused imx_clk_lpcg_scu_resume(struct device *dev)
 	 * FIXME: Sometimes writes don't work unless the CPU issues
 	 * them twice
 	 */
-
-	writel(clk->state, clk->reg);
 	writel(clk->state, clk->reg);
+	lpcg_e10858_writel(0, clk->reg, clk->state);
 	dev_dbg(dev, "restore lpcg state 0x%x\n", clk->state);
 
 	return 0;