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

[Marek Vasut <marex@denx.de>]

On Sunday, December 14, 2014 at 04:28:53 PM, 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.
> 
> The changes has been tested only with a i.MX28 board, because i don't
> have access to an i.MX23 board.

The clock block is the same, so this _should_ be fine.

> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
>  drivers/clk/mxs/clk-imx23.c |    8 +++++---
>  drivers/clk/mxs/clk-imx28.c |   14 ++++++++------
>  drivers/clk/mxs/clk-ref.c   |   19 ++++++++++---------
>  3 files changed, 23 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/clk/mxs/clk-imx23.c b/drivers/clk/mxs/clk-imx23.c
> index 9fc9359..371ba03 100644
> --- a/drivers/clk/mxs/clk-imx23.c
> +++ b/drivers/clk/mxs/clk-imx23.c
> @@ -46,7 +46,8 @@ 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)
>  {
> @@ -72,9 +73,10 @@ 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);
> +	writeb_relaxed(0x3f, FRAC + FRAC_IO + CLR);
> +	writeb_relaxed(30, FRAC + FRAC_IO + SET);
>  }
> 
>  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..3eae119 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
> @@ -118,11 +119,12 @@ 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);
> +	writeb_relaxed(0x3f, FRAC0 + FRAC0_IO0 + CLR);
> +	writeb_relaxed(30, FRAC0 + FRAC0_IO0 + SET);
> +	writeb_relaxed(0x3f, FRAC0 + FRAC0_IO1 + CLR);
> +	writeb_relaxed(30, FRAC0 + FRAC0_IO1 + SET);

This used to be a R-M-W sequence, but now it's changed to multiple writes. This
changes the behavior and seeing you use the CLR register, I am worried this 
might be prone to clock glitches. What do you think please ?

[...]

Also, it might be a good idea to zap the 0x3f mask and use HEX and DEC numbers 
consistently, but this is an idea for another patch.

Best regards,