[PATCH V2] clk: mxs: Fix invalid 32-bit access to frac registers

[PATCH V2] clk: mxs: Fix invalid 32-bit access to frac registers

[Stefan Wahren]

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 <stefan.wahren@i2se.com>
---
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

Re: [PATCH V2] clk: mxs: Fix invalid 32-bit access to frac registers

[Marek Vasut]

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 <stefan.wahren@i2se.com>

I don't see a problem,

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

Best regards,
Marek Vasut

Re: [PATCH V2] clk: mxs: Fix invalid 32-bit access to frac registers

[Stefan Wahren]

Hi,

> Marek Vasut <marex@denx.de> hat am 28. Dezember 2014 um 19:30 geschrieben:
>
>
> 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.

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

Any conserns about it?

> >
> > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>
> 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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2] clk: mxs: Fix invalid 32-bit access to frac registers

[Zhi Li]

On Sun, Dec 28, 2014 at 4:26 AM, Stefan Wahren <stefan.wahren@i2se.com> 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@freecale.com).

> 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 <stefan.wahren@i2se.com>
> ---
> 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
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2] clk: mxs: Fix invalid 32-bit access to frac registers

[Marek Vasut]

On Wednesday, January 21, 2015 at 05:16:03 PM, Zhi Li wrote:
> On Sun, Dec 28, 2014 at 4:26 AM, Stefan Wahren <stefan.wahren@i2se.com> 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.

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

Re: [PATCH V2] clk: mxs: Fix invalid 32-bit access to frac registers

[Mike Turquette]

Quoting Marek Vasut (2015-01-21 15:39:01)
> On Wednesday, January 21, 2015 at 05:16:03 PM, Zhi Li wrote:
> > On Sun, Dec 28, 2014 at 4:26 AM, Stefan Wahren <stefan.wahren@i2se.com> 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.
> 
> 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

Re: [PATCH V2] clk: mxs: Fix invalid 32-bit access to frac registers

[Zhi Li]

On Tue, Jan 27, 2015 at 7:51 PM, Mike Turquette <mturquette@linaro.org> wrote:
> Quoting Marek Vasut (2015-01-21 15:39:01)
>> On Wednesday, January 21, 2015 at 05:16:03 PM, Zhi Li wrote:
>> > On Sun, Dec 28, 2014 at 4:26 AM, Stefan Wahren <stefan.wahren@i2se.com> 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.
>>
>> 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
>
>>
>> Best regards,
>> Marek Vasut

Re: [PATCH V2] clk: mxs: Fix invalid 32-bit access to frac registers

[Stefan Wahren]

Hi,

Am 28.01.2015 um 04:36 schrieb Zhi Li:
> On Tue, Jan 27, 2015 at 7:51 PM, Mike Turquette <mturquette@linaro.org> wrote:
>> Quoting Marek Vasut (2015-01-21 15:39:01)
>>> On Wednesday, January 21, 2015 at 05:16:03 PM, Zhi Li wrote:
>>>> On Sun, Dec 28, 2014 at 4:26 AM, Stefan Wahren <stefan.wahren@i2se.com> 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.
>>> 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
>> Regards,
>> Mike
>>
>>> Best regards,
>>> Marek Vasut

Re: [PATCH V2] clk: mxs: Fix invalid 32-bit access to frac registers

[Zhi Li]

On Wed, Jan 28, 2015 at 9:52 AM, Stefan Wahren <stefan.wahren@i2se.com> wrote:
> Hi,
>
> Am 28.01.2015 um 04:36 schrieb Zhi Li:
>> On Tue, Jan 27, 2015 at 7:51 PM, Mike Turquette <mturquette@linaro.org> wrote:
>>> Quoting Marek Vasut (2015-01-21 15:39:01)
>>>> On Wednesday, January 21, 2015 at 05:16:03 PM, Zhi Li wrote:
>>>>> On Sun, Dec 28, 2014 at 4:26 AM, Stefan Wahren <stefan.wahren@i2se.com> 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.
>>>> 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 !!!

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

best regards
Frank Li

>
> Best regards
> Stefan
>
>>
>> best regards
>> Frank Li
>>> Regards,
>>> Mike
>>>
>>>> Best regards,
>>>> Marek Vasut
>
>

Re: [PATCH V2] clk: mxs: Fix invalid 32-bit access to frac registers

[Stefan Wahren]

Hi,

Am 28.01.2015 um 17:10 schrieb Zhi Li:
> On Wed, Jan 28, 2015 at 9:52 AM, Stefan Wahren <stefan.wahren@i2se.com> wrote:
>> Hi,
>>
>> Am 28.01.2015 um 04:36 schrieb Zhi Li:
>>> On Tue, Jan 27, 2015 at 7:51 PM, Mike Turquette <mturquette@linaro.org> wrote:
>>>> Quoting Marek Vasut (2015-01-21 15:39:01)
>>>>> On Wednesday, January 21, 2015 at 05:16:03 PM, Zhi Li wrote:
>>>>>> On Sun, Dec 28, 2014 at 4:26 AM, Stefan Wahren <stefan.wahren@i2se.com> 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.
>>>>> 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 !!!
> Yes, it is align with what I get from IC designer.
>
> best regards
> Frank Li

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

>
>> Best regards
>> Stefan
>>
>>> best regards
>>> Frank Li
>>>> Regards,
>>>> Mike
>>>>
>>>>> Best regards,
>>>>> Marek Vasut
>>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2] clk: mxs: Fix invalid 32-bit access to frac registers

[Zhi Li]

On Wed, Jan 28, 2015 at 10:40 AM, Stefan Wahren <stefan.wahren@i2se.com> 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.

Re: [PATCH V2] clk: mxs: Fix invalid 32-bit access to frac registers

[Fabio Estevam]

Hi Stefan,

On Sun, Dec 28, 2014 at 8:26 AM, Stefan Wahren <stefan.wahren@i2se.com> 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