clk: mxs: Fix invalid 32-bit access to frac registers

According to i.MX23 and i.MX28 reference manual the fractional
clock control registers must be addressed by byte instructions.

This patch fixes the erroneous 32-bit access to these registers
and extends the comment in the init functions.

Btw the imx23 init now uses a R-M-W sequence just like imx28 init
to avoid any clock glitches.

The changes has been tested only with a i.MX28 board, because i don't
have access to an i.MX23 board.

Signed-off-by: Stefan Wahren <redacted>
---
Changes in V2:
- use relaxed access operations in clk-ref

 drivers/clk/mxs/clk-imx23.c |   11 ++++++++---
 drivers/clk/mxs/clk-imx28.c |   19 +++++++++++++------
 drivers/clk/mxs/clk-ref.c   |   19 ++++++++++---------
 3 files changed, 31 insertions(+), 18 deletions(-)

diff --git a/drivers/clk/mxs/clk-imx23.c b/drivers/clk/mxs/clk-imx23.c
index 9fc9359..a084566 100644
--- a/drivers/clk/mxs/clk-imx23.c
+++ b/drivers/clk/mxs/clk-imx23.c

@@ -46,11 +46,13 @@ static void __iomem *digctrl;
 #define BP_CLKSEQ_BYPASS_SAIF	0
 #define BP_CLKSEQ_BYPASS_SSP	5
 #define BP_SAIF_DIV_FRAC_EN	16
-#define BP_FRAC_IOFRAC		24
+
+#define FRAC_IO	3
 
 static void __init clk_misc_init(void)
 {
 	u32 val;
+	u8 frac;
 
 	/* Gate off cpu clock in WFI for power saving */
 	writel_relaxed(1 << BP_CPU_INTERRUPT_WAIT, CPU + SET);

@@ -72,9 +74,12 @@ static void __init clk_misc_init(void)
 	/*
 	 * 480 MHz seems too high to be ssp clock source directly,
 	 * so set frac to get a 288 MHz ref_io.
+	 * According to reference manual we must access frac bytewise.
 	 */
-	writel_relaxed(0x3f << BP_FRAC_IOFRAC, FRAC + CLR);
-	writel_relaxed(30 << BP_FRAC_IOFRAC, FRAC + SET);
+	frac = readb_relaxed(FRAC + FRAC_IO);
+	frac &= ~0x3f;
+	frac |= 30;
+	writeb_relaxed(frac, FRAC + FRAC_IO);
 }
 
 static const char *sel_pll[]  __initconst = { "pll", "ref_xtal", };

diff --git a/drivers/clk/mxs/clk-imx28.c b/drivers/clk/mxs/clk-imx28.c
index a6c3501..c541377 100644
--- a/drivers/clk/mxs/clk-imx28.c
+++ b/drivers/clk/mxs/clk-imx28.c

@@ -53,8 +53,9 @@ static void __iomem *clkctrl;
 #define BP_ENET_SLEEP		31
 #define BP_CLKSEQ_BYPASS_SAIF0	0
 #define BP_CLKSEQ_BYPASS_SSP0	3
-#define BP_FRAC0_IO1FRAC	16
-#define BP_FRAC0_IO0FRAC	24
+
+#define FRAC0_IO1	2
+#define FRAC0_IO0	3
 
 static void __iomem *digctrl;
 #define DIGCTRL digctrl

@@ -85,6 +86,7 @@ int mxs_saif_clkmux_select(unsigned int clkmux)
 static void __init clk_misc_init(void)
 {
 	u32 val;
+	u8 frac;
 
 	/* Gate off cpu clock in WFI for power saving */
 	writel_relaxed(1 << BP_CPU_INTERRUPT_WAIT, CPU + SET);

@@ -118,11 +120,16 @@ static void __init clk_misc_init(void)
 	/*
 	 * 480 MHz seems too high to be ssp clock source directly,
 	 * so set frac0 to get a 288 MHz ref_io0 and ref_io1.
+	 * According to reference manual we must access frac0 bytewise.
 	 */
-	val = readl_relaxed(FRAC0);
-	val &= ~((0x3f << BP_FRAC0_IO0FRAC) | (0x3f << BP_FRAC0_IO1FRAC));
-	val |= (30 << BP_FRAC0_IO0FRAC) | (30 << BP_FRAC0_IO1FRAC);
-	writel_relaxed(val, FRAC0);
+	frac = readb_relaxed(FRAC0 + FRAC0_IO0);
+	frac &= ~0x3f;
+	frac |= 30;
+	writeb_relaxed(frac, FRAC0 + FRAC0_IO0);
+	frac = readb_relaxed(FRAC0 + FRAC0_IO1);
+	frac &= ~0x3f;
+	frac |= 30;
+	writeb_relaxed(frac, FRAC0 + FRAC0_IO1);
 }
 
 static const char *sel_cpu[]  __initconst = { "ref_cpu", "ref_xtal", };

diff --git a/drivers/clk/mxs/clk-ref.c b/drivers/clk/mxs/clk-ref.c
index 4adeed6..ad3851c 100644
--- a/drivers/clk/mxs/clk-ref.c
+++ b/drivers/clk/mxs/clk-ref.c

@@ -16,6 +16,8 @@
 #include <linux/slab.h>
 #include "clk.h"
 
+#define BF_CLKGATE	BIT(7)
+
 /**
  * struct clk_ref - mxs reference clock
  * @hw: clk_hw for the reference clock

@@ -39,7 +41,7 @@ static int clk_ref_enable(struct clk_hw *hw)
 {
 	struct clk_ref *ref = to_clk_ref(hw);
 
-	writel_relaxed(1 << ((ref->idx + 1) * 8 - 1), ref->reg + CLR);
+	writeb_relaxed(BF_CLKGATE, ref->reg + ref->idx + CLR);
 
 	return 0;
 }

@@ -48,7 +50,7 @@ static void clk_ref_disable(struct clk_hw *hw)
 {
 	struct clk_ref *ref = to_clk_ref(hw);
 
-	writel_relaxed(1 << ((ref->idx + 1) * 8 - 1), ref->reg + SET);
+	writeb_relaxed(BF_CLKGATE, ref->reg + ref->idx + SET);
 }
 
 static unsigned long clk_ref_recalc_rate(struct clk_hw *hw,

@@ -56,7 +58,7 @@ static unsigned long clk_ref_recalc_rate(struct clk_hw *hw,
 {
 	struct clk_ref *ref = to_clk_ref(hw);
 	u64 tmp = parent_rate;
-	u8 frac = (readl_relaxed(ref->reg) >> (ref->idx * 8)) & 0x3f;
+	u8 frac = readb_relaxed(ref->reg + ref->idx) & 0x3f;
 
 	tmp *= 18;
 	do_div(tmp, frac);

@@ -93,8 +95,7 @@ static int clk_ref_set_rate(struct clk_hw *hw, unsigned long rate,
 	struct clk_ref *ref = to_clk_ref(hw);
 	unsigned long flags;
 	u64 tmp = parent_rate;
-	u32 val;
-	u8 frac, shift = ref->idx * 8;
+	u8 frac, val;
 
 	tmp = tmp * 18 + rate / 2;
 	do_div(tmp, rate);

@@ -107,10 +108,10 @@ static int clk_ref_set_rate(struct clk_hw *hw, unsigned long rate,
 
 	spin_lock_irqsave(&mxs_lock, flags);
 
-	val = readl_relaxed(ref->reg);
-	val &= ~(0x3f << shift);
-	val |= frac << shift;
-	writel_relaxed(val, ref->reg);
+	val = readb_relaxed(ref->reg + ref->idx);
+	val &= ~0x3f;
+	val |= frac;
+	writeb_relaxed(val, ref->reg + ref->idx);
 
 	spin_unlock_irqrestore(&mxs_lock, flags);
 

-- 
1.7.9.5

On Sunday, December 28, 2014 at 11:26:42 AM, Stefan Wahren wrote:

According to i.MX23 and i.MX28 reference manual the fractional
clock control registers must be addressed by byte instructions.

This patch fixes the erroneous 32-bit access to these registers
and extends the comment in the init functions.

Btw the imx23 init now uses a R-M-W sequence just like imx28 init
to avoid any clock glitches.

The changes has been tested only with a i.MX28 board, because i don't
have access to an i.MX23 board.

Signed-off-by: Stefan Wahren <redacted>

I don't see a problem,

Reviewed-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

Hi,

Marek Vasut [off-list ref] hat am 28. Dezember 2014 um 19:30 geschrieben:


On Sunday, December 28, 2014 at 11:26:42 AM, Stefan Wahren wrote:

quoted

According to i.MX23 and i.MX28 reference manual the fractional
clock control registers must be addressed by byte instructions.

This patch fixes the erroneous 32-bit access to these registers
and extends the comment in the init functions.

Btw the imx23 init now uses a R-M-W sequence just like imx28 init
to avoid any clock glitches.

The changes has been tested only with a i.MX28 board, because i don't
have access to an i.MX23 board.

since i've also a i.MX23 board, i tested this patch successful.

Any conserns about it?

quoted

Signed-off-by: Stefan Wahren <redacted>

I don't see a problem,

Reviewed-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Sun, Dec 28, 2014 at 4:26 AM, Stefan Wahren [off-list ref] wrote:

According to i.MX23 and i.MX28 reference manual the fractional
clock control registers must be addressed by byte instructions.

I don't think mx23 and mx28 have such limitation. I will double check
with IC team about this.
RTL is generated from a xml file. All registers implement is unified.
I don't think only clock control register have such limitation and
other registers not.

best regards
Frank Li (Frank.Li at freecale.com).

131 more quoted lines

On Wednesday, January 21, 2015 at 05:16:03 PM, Zhi Li wrote:

On Sun, Dec 28, 2014 at 4:26 AM, Stefan Wahren [off-list ref] wrote:

quoted

According to i.MX23 and i.MX28 reference manual the fractional
clock control registers must be addressed by byte instructions.

I don't think mx23 and mx28 have such limitation. I will double check
with IC team about this.
RTL is generated from a xml file. All registers implement is unified.
I don't think only clock control register have such limitation and
other registers not.

Hi,

Section 10.8.24 in the MX28 datasheet (Fractional Clock Control Register 0) 
states otherwise, but maybe the documentation is simply not matching the
silicon.

Here's a quote:
"
This register controls the 9-phase fractional clock dividers. The fractional 
clock frequencies are a product of the values in these registers. NOTE: This 
register can only be addressed by byte instructions. Addressing word or half-
word are not allowed.
"

I also recall seeing weird behavior when these registers were accessed by word
access in U-Boot, so I believe the datasheet is correct.

Best regards,
Marek Vasut

Quoting Marek Vasut (2015-01-21 15:39:01)

On Wednesday, January 21, 2015 at 05:16:03 PM, Zhi Li wrote:

quoted

On Sun, Dec 28, 2014 at 4:26 AM, Stefan Wahren [off-list ref] wrote:

quoted

According to i.MX23 and i.MX28 reference manual the fractional
clock control registers must be addressed by byte instructions.

I don't think mx23 and mx28 have such limitation. I will double check
with IC team about this.
RTL is generated from a xml file. All registers implement is unified.
I don't think only clock control register have such limitation and
other registers not.

Hi,

Section 10.8.24 in the MX28 datasheet (Fractional Clock Control Register 0) 
states otherwise, but maybe the documentation is simply not matching the
silicon.

Here's a quote:
"
This register controls the 9-phase fractional clock dividers. The fractional 
clock frequencies are a product of the values in these registers. NOTE: This 
register can only be addressed by byte instructions. Addressing word or half-
word are not allowed.
"

I also recall seeing weird behavior when these registers were accessed by word
access in U-Boot, so I believe the datasheet is correct.

Hi Frank,

Are you satisfied with this patch?

Regards,
Mike

Best regards,
Marek Vasut

On Tue, Jan 27, 2015 at 7:51 PM, Mike Turquette [off-list ref] wrote:

Quoting Marek Vasut (2015-01-21 15:39:01)

quoted

On Wednesday, January 21, 2015 at 05:16:03 PM, Zhi Li wrote:

quoted

On Sun, Dec 28, 2014 at 4:26 AM, Stefan Wahren [off-list ref] wrote:

quoted

According to i.MX23 and i.MX28 reference manual the fractional
clock control registers must be addressed by byte instructions.

I don't think mx23 and mx28 have such limitation. I will double check
with IC team about this.
RTL is generated from a xml file. All registers implement is unified.
I don't think only clock control register have such limitation and
other registers not.

Hi,

Section 10.8.24 in the MX28 datasheet (Fractional Clock Control Register 0)
states otherwise, but maybe the documentation is simply not matching the
silicon.

Here's a quote:
"
This register controls the 9-phase fractional clock dividers. The fractional
clock frequencies are a product of the values in these registers. NOTE: This
register can only be addressed by byte instructions. Addressing word or half-
word are not allowed.
"

I also recall seeing weird behavior when these registers were accessed by word
access in U-Boot, so I believe the datasheet is correct.

Hi Frank,

Are you satisfied with this patch?

I asked IC designer about this.
They will check RTL code.
I will check their status again.
Our released BSP code used 32bit WORD access.

best regards
Frank Li

Regards,
Mike

quoted

Best regards,
Marek Vasut

Hi,

Am 28.01.2015 um 04:36 schrieb Zhi Li:

On Tue, Jan 27, 2015 at 7:51 PM, Mike Turquette [off-list ref] wrote:

quoted

Quoting Marek Vasut (2015-01-21 15:39:01)

quoted

On Wednesday, January 21, 2015 at 05:16:03 PM, Zhi Li wrote:

quoted

On Sun, Dec 28, 2014 at 4:26 AM, Stefan Wahren [off-list ref] wrote:

quoted

According to i.MX23 and i.MX28 reference manual the fractional
clock control registers must be addressed by byte instructions.

I don't think mx23 and mx28 have such limitation. I will double check
with IC team about this.
RTL is generated from a xml file. All registers implement is unified.
I don't think only clock control register have such limitation and
other registers not.

Hi,

Section 10.8.24 in the MX28 datasheet (Fractional Clock Control Register 0)
states otherwise, but maybe the documentation is simply not matching the
silicon.

Here's a quote:
"
This register controls the 9-phase fractional clock dividers. The fractional
clock frequencies are a product of the values in these registers. NOTE: This
register can only be addressed by byte instructions. Addressing word or half-
word are not allowed.
"

I also recall seeing weird behavior when these registers were accessed by word
access in U-Boot, so I believe the datasheet is correct.

Hi Frank,

Are you satisfied with this patch?

I asked IC designer about this.
They will check RTL code.
I will check their status again.
Our released BSP code used 32bit WORD access.

i want to point out that the 32bit WORD is divided in 4 parts (IO0FRAC,
IO1FRAC, EMIFRAC, CPUFRAC). Yes, it's true that BSP code access the
register as 32bit, but it's never modify the complete 32bit at once just
only 1 part (8bit) at a time.

So here is my theory about Fractional Clock Control Register:

Reading as 32bit WORD => safe
Modify only 1 part (8bit) of the 32bit WORD => safe
Modify more than 1 part of the 32bit WORD => unsafe !!!

Best regards
Stefan

best regards
Frank Li

quoted

Regards,
Mike

quoted

Best regards,
Marek Vasut

On Wed, Jan 28, 2015 at 9:52 AM, Stefan Wahren [off-list ref] wrote:

Hi,

Am 28.01.2015 um 04:36 schrieb Zhi Li:

quoted

On Tue, Jan 27, 2015 at 7:51 PM, Mike Turquette [off-list ref] wrote:

quoted

Quoting Marek Vasut (2015-01-21 15:39:01)

quoted

On Wednesday, January 21, 2015 at 05:16:03 PM, Zhi Li wrote:

quoted

On Sun, Dec 28, 2014 at 4:26 AM, Stefan Wahren [off-list ref] wrote:

quoted

According to i.MX23 and i.MX28 reference manual the fractional
clock control registers must be addressed by byte instructions.

I don't think mx23 and mx28 have such limitation. I will double check
with IC team about this.
RTL is generated from a xml file. All registers implement is unified.
I don't think only clock control register have such limitation and
other registers not.

Hi,

Section 10.8.24 in the MX28 datasheet (Fractional Clock Control Register 0)
states otherwise, but maybe the documentation is simply not matching the
silicon.

Here's a quote:
"
This register controls the 9-phase fractional clock dividers. The fractional
clock frequencies are a product of the values in these registers. NOTE: This
register can only be addressed by byte instructions. Addressing word or half-
word are not allowed.
"

I also recall seeing weird behavior when these registers were accessed by word
access in U-Boot, so I believe the datasheet is correct.

Hi Frank,

Are you satisfied with this patch?

I asked IC designer about this.
They will check RTL code.
I will check their status again.
Our released BSP code used 32bit WORD access.

i want to point out that the 32bit WORD is divided in 4 parts (IO0FRAC,
IO1FRAC, EMIFRAC, CPUFRAC). Yes, it's true that BSP code access the

8 more quoted lines

Yes, it is align with what I get from IC designer.

best regards
Frank Li

Best regards
Stefan

quoted

best regards
Frank Li

quoted

Regards,
Mike

quoted

Best regards,
Marek Vasut

On Wed, Jan 28, 2015 at 10:40 AM, Stefan Wahren [off-list ref] wrote:

The clk init for the i.MX28 in the kernel tries to the set
IO0FRAC and IO1FRAC at once. So this patch fix this problem.

Okay, I am  fine. I suggest add comment about only change one FRAC once.

Hi Stefan,

On Sun, Dec 28, 2014 at 8:26 AM, Stefan Wahren [off-list ref] wrote:

According to i.MX23 and i.MX28 reference manual the fractional
clock control registers must be addressed by byte instructions.

This patch fixes the erroneous 32-bit access to these registers
and extends the comment in the init functions.

Btw the imx23 init now uses a R-M-W sequence just like imx28 init
to avoid any clock glitches.

The changes has been tested only with a i.MX28 board, because i don't
have access to an i.MX23 board.

As you recently had a chance to test it on mx23, maybe you could
resend it and change this last parapgraph.

Regards,

Fabio Estevam

Hi,

Am 28.01.2015 um 17:10 schrieb Zhi Li:

On Wed, Jan 28, 2015 at 9:52 AM, Stefan Wahren [off-list ref] wrote:

quoted

Hi,

Am 28.01.2015 um 04:36 schrieb Zhi Li:

quoted

On Tue, Jan 27, 2015 at 7:51 PM, Mike Turquette [off-list ref] wrote:

quoted

Quoting Marek Vasut (2015-01-21 15:39:01)

quoted

On Wednesday, January 21, 2015 at 05:16:03 PM, Zhi Li wrote:

quoted

On Sun, Dec 28, 2014 at 4:26 AM, Stefan Wahren [off-list ref] wrote:

quoted

According to i.MX23 and i.MX28 reference manual the fractional
clock control registers must be addressed by byte instructions.

I don't think mx23 and mx28 have such limitation. I will double check
with IC team about this.
RTL is generated from a xml file. All registers implement is unified.
I don't think only clock control register have such limitation and
other registers not.

Hi,

Section 10.8.24 in the MX28 datasheet (Fractional Clock Control Register 0)
states otherwise, but maybe the documentation is simply not matching the
silicon.

Here's a quote:
"
This register controls the 9-phase fractional clock dividers. The fractional
clock frequencies are a product of the values in these registers. NOTE: This
register can only be addressed by byte instructions. Addressing word or half-
word are not allowed.
"

I also recall seeing weird behavior when these registers were accessed by word
access in U-Boot, so I believe the datasheet is correct.

Hi Frank,

Are you satisfied with this patch?

I asked IC designer about this.
They will check RTL code.
I will check their status again.
Our released BSP code used 32bit WORD access.

i want to point out that the 32bit WORD is divided in 4 parts (IO0FRAC,
IO1FRAC, EMIFRAC, CPUFRAC). Yes, it's true that BSP code access the

12 more quoted lines

okay, fine. The clk init for the i.MX28 in the kernel tries to the set
IO0FRAC and IO1FRAC at once. So this patch fix this problem.

And since i was on that, i changed all accesses on that register to 8bit
to avoid this problem in the future.

So what's your opinion about this patch?

Best regards
Stefan

quoted

Best regards
Stefan

quoted

best regards
Frank Li

quoted

Regards,
Mike

quoted

Best regards,
Marek Vasut

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel