Patchwork [U-Boot] arm: exynos: add missing readl

login
register
mail settings
Submitter Inha Song
Date Jan. 11, 2014, 9:02 a.m.
Message ID <20140111180220.644269f8@songinha-Samsung-DeskTop-System>
Download mbox | patch
Permalink /patch/309501/
State Changes Requested
Delegated to: Minkyu Kang
Headers show

Comments

Inha Song - Jan. 11, 2014, 9:02 a.m.
The readl function was missing in exynos/clock.c

Signed-off-by: Inha Song <ideal.song@samsung.com>
---
 arch/arm/cpu/armv7/exynos/clock.c |    3 +++
 1 file changed, 3 insertions(+)
Wolfgang Denk - Jan. 11, 2014, 4:51 p.m.
Dear Inha Song,

In message <20140111180220.644269f8@songinha-Samsung-DeskTop-System> you wrote:
> The readl function was missing in exynos/clock.c
> 
> Signed-off-by: Inha Song <ideal.song@samsung.com>
> ---
>  arch/arm/cpu/armv7/exynos/clock.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/arm/cpu/armv7/exynos/clock.c b/arch/arm/cpu/armv7/exynos/clock.c
> index 5bde9d1..08e7d50 100644
> --- a/arch/arm/cpu/armv7/exynos/clock.c
> +++ b/arch/arm/cpu/armv7/exynos/clock.c
> @@ -1114,6 +1114,7 @@ void exynos4_set_lcd_clk(void)
>  	 * MIPI0_PRE_RATIO	[23:20]
>  	 * set fimd ratio
>  	 */
> +	cfg = readl(&clk->div_lcd0);
>  	cfg &= ~(0xf);
>  	cfg |= 0x1;
>  	writel(cfg, &clk->div_lcd0);

Code like this should be rewritten to use better I/O accessors.  Here
these 4 statements could / should be rewritten as

	clrsetbits_le32(&clk->div_lcd0, 0xf, 0x1);

Actually you should probably rather write:

	clrsetbits_le32(&clk->div_lcd0, 0xe, 0x1);

because to me it seems silly to me to clear a bit that needs to be set?

> +	cfg = readl(&clk->div_disp1_0);
>  	cfg &= ~(0xf);
>  	cfg |= 0x0;
>  	writel(cfg, &clk->div_disp1_0);

Same here:

	clrbits_le32(&clk->div_disp1_0, 0xf);

Note: it makes zero sense to set zero bits.

Note 2: I wonder if these magic number (0xf, 0x1) could / should not
rather be replaced by some readable preprocessor macro def?

>  	 * MIPI0_PRE_RATIO	[23:20]
>  	 * set mipi ratio
>  	 */
> +	cfg = readl(&clk->div_lcd0);
>  	cfg &= ~(0xf << 16);
>  	cfg |= (0x1 << 16);
>  	writel(cfg, &clk->div_lcd0);

And again:

	clrsetbits_le32(&clk->div_lcd0, 0xf << 16, 0x1 << 16);

[again I wonder if the 0xf should not rathe rbe 0xe instead?]

Best regards,

Wolfgang Denk

Patch

diff --git a/arch/arm/cpu/armv7/exynos/clock.c b/arch/arm/cpu/armv7/exynos/clock.c
index 5bde9d1..08e7d50 100644
--- a/arch/arm/cpu/armv7/exynos/clock.c
+++ b/arch/arm/cpu/armv7/exynos/clock.c
@@ -1114,6 +1114,7 @@  void exynos4_set_lcd_clk(void)
 	 * MIPI0_PRE_RATIO	[23:20]
 	 * set fimd ratio
 	 */
+	cfg = readl(&clk->div_lcd0);
 	cfg &= ~(0xf);
 	cfg |= 0x1;
 	writel(cfg, &clk->div_lcd0);
@@ -1176,6 +1177,7 @@  void exynos5_set_lcd_clk(void)
 	 * MIPI0_PRE_RATIO	[23:20]
 	 * set fimd ratio
 	 */
+	cfg = readl(&clk->div_disp1_0);
 	cfg &= ~(0xf);
 	cfg |= 0x0;
 	writel(cfg, &clk->div_disp1_0);
@@ -1236,6 +1238,7 @@  void exynos4_set_mipi_clk(void)
 	 * MIPI0_PRE_RATIO	[23:20]
 	 * set mipi ratio
 	 */
+	cfg = readl(&clk->div_lcd0);
 	cfg &= ~(0xf << 16);
 	cfg |= (0x1 << 16);
 	writel(cfg, &clk->div_lcd0);