arm: exynos: change to use clrbits macro instead of readl/writel function

Use setbits/clrbits macro instead of readl/writel function

Signed-off-by Inha Song <[email protected]>

---
 arch/arm/cpu/armv7/exynos/clock.c |   87 +++++++++++--------------------------
 1 file changed, 25 insertions(+), 62 deletions(-)

arch/arm/cpu/armv7/exynos/clock.c +0 -1

index 5bde9d1..eaabfb9 100644
@@ -870,7 +870,6 @@ static void exynos4_set_mmc_clk(int dev_index, unsigned int div)
 	struct exynos4_clock *clk =
 		(struct exynos4_clock *)samsung_get_base_clock();
 	unsigned int addr;
-	unsigned int val;
 
 	/*
 	 * CLK_DIV_FSYS1

arch/arm/cpu/armv7/exynos/clock.c +0 -1

 }
 
 /* exynos4x12: set the mmc clock */

@@ -902,7 +900,6 @@ static void exynos4x12_set_mmc_clk(int dev_index, unsigned int div)
 	struct exynos4x12_clock *clk =
 		(struct exynos4x12_clock *)samsung_get_base_clock();
 	unsigned int addr;
-	unsigned int val;
 
 	/*
 	 * CLK_DIV_FSYS1

arch/arm/cpu/armv7/exynos/clock.c +0 -1

 }
 
 /* exynos5: set the mmc clock */

@@ -929,7 +925,6 @@ static void exynos5_set_mmc_clk(int dev_index, unsigned int div)
 	struct exynos5_clock *clk =
 		(struct exynos5_clock *)samsung_get_base_clock();
 	unsigned int addr;
-	unsigned int val;
 
 	/*
 	 * CLK_DIV_FSYS1

arch/arm/cpu/armv7/exynos/clock.c +1 -1

 }
 
 /* exynos5: set the mmc clock */

@@ -956,7 +950,7 @@ static void exynos5420_set_mmc_clk(int dev_index, unsigned int div)
 	struct exynos5420_clock *clk =
 		(struct exynos5420_clock *)samsung_get_base_clock();
 	unsigned int addr;
-	unsigned int val, shift;
+	unsigned int shift;
 
 	/*
 	 * CLK_DIV_FSYS1

arch/arm/cpu/armv7/exynos/clock.c +12 -42

 }
 
 /* get_lcd_clk: return lcd clock frequency */

@@ -1061,7 +1054,6 @@ void exynos4_set_lcd_clk(void)
 {
 	struct exynos4_clock *clk =
 	    (struct exynos4_clock *)samsung_get_base_clock();
-	unsigned int cfg = 0;
 
 	/*
 	 * CLK_GATE_BLOCK

@@ -1073,9 +1065,7 @@ void exynos4_set_lcd_clk(void)
 	 * CLK_LCD1	[5]
 	 * CLK_GPS	[7]
 	 */
-	cfg = readl(&clk->gate_block);
-	cfg |= 1 << 4;
-	writel(cfg, &clk->gate_block);
+	setbits_le32(&clk->gate_block, 1 << 4);
 
 	/*
 	 * CLK_SRC_LCD0

@@ -1085,10 +1075,7 @@ void exynos4_set_lcd_clk(void)
 	 * MIPI0_SEL		[12:15]
 	 * set lcd0 src clock 0x6: SCLK_MPLL
 	 */
-	cfg = readl(&clk->src_lcd0);
-	cfg &= ~(0xf);
-	cfg |= 0x6;
-	writel(cfg, &clk->src_lcd0);
+	clrsetbits_le32(&clk->src_lcd0, 0x9, 0x6);
 
 	/*
 	 * CLK_GATE_IP_LCD0

@@ -1100,9 +1087,7 @@ void exynos4_set_lcd_clk(void)
 	 * CLK_PPMULCD0		[5]
 	 * Gating all clocks for FIMD0
 	 */
-	cfg = readl(&clk->gate_ip_lcd0);
-	cfg |= 1 << 0;
-	writel(cfg, &clk->gate_ip_lcd0);
+	setbits_le32(&clk->gate_ip_lcd0, 1 << 0);
 
 	/*
 	 * CLK_DIV_LCD0

@@ -1114,16 +1099,13 @@ void exynos4_set_lcd_clk(void)
 	 * MIPI0_PRE_RATIO	[23:20]
 	 * set fimd ratio
 	 */
-	cfg &= ~(0xf);
-	cfg |= 0x1;
-	writel(cfg, &clk->div_lcd0);
+	clrsetbits_le32(&clk->div_lcd0, 0xe, 0x1);
 }
 
 void exynos5_set_lcd_clk(void)
 {
 	struct exynos5_clock *clk =
 	    (struct exynos5_clock *)samsung_get_base_clock();
-	unsigned int cfg = 0;
 
 	/*
 	 * CLK_GATE_BLOCK

@@ -1135,9 +1117,7 @@ void exynos5_set_lcd_clk(void)
 	 * CLK_LCD1	[5]
 	 * CLK_GPS	[7]
 	 */
-	cfg = readl(&clk->gate_block);
-	cfg |= 1 << 4;
-	writel(cfg, &clk->gate_block);
+	setbits_le32(&clk->gate_block, 1 << 4);
 
 	/*
 	 * CLK_SRC_LCD0

@@ -1147,10 +1127,7 @@ void exynos5_set_lcd_clk(void)
 	 * MIPI0_SEL		[12:15]
 	 * set lcd0 src clock 0x6: SCLK_MPLL
 	 */
-	cfg = readl(&clk->src_disp1_0);
-	cfg &= ~(0xf);
-	cfg |= 0x6;
-	writel(cfg, &clk->src_disp1_0);
+	clrsetbits_le32(&clk->src_disp1_0, 0x9, 0x6);
 
 	/*
 	 * CLK_GATE_IP_LCD0

@@ -1162,9 +1139,7 @@ void exynos5_set_lcd_clk(void)
 	 * CLK_PPMULCD0		[5]
 	 * Gating all clocks for FIMD0
 	 */
-	cfg = readl(&clk->gate_ip_disp1);
-	cfg |= 1 << 0;
-	writel(cfg, &clk->gate_ip_disp1);
+	setbits_le32(&clk->gate_ip_disp1, 1 << 0);
 
 	/*
 	 * CLK_DIV_LCD0

@@ -1176,16 +1151,13 @@ void exynos5_set_lcd_clk(void)
 	 * MIPI0_PRE_RATIO	[23:20]
 	 * set fimd ratio
 	 */
-	cfg &= ~(0xf);
-	cfg |= 0x0;
-	writel(cfg, &clk->div_disp1_0);
+	clrbits_le32(&clk->div_disp1_0, 0xf);
 }
 
 void exynos4_set_mipi_clk(void)
 {
 	struct exynos4_clock *clk =
 	    (struct exynos4_clock *)samsung_get_base_clock();
-	unsigned int cfg = 0;
 
 	/*
 	 * CLK_SRC_LCD0

@@ -1195,10 +1167,7 @@ void exynos4_set_mipi_clk(void)
 	 * MIPI0_SEL		[12:15]
 	 * set mipi0 src clock 0x6: SCLK_MPLL
 	 */
-	cfg = readl(&clk->src_lcd0);
-	cfg &= ~(0xf << 12);
-	cfg |= (0x6 << 12);
-	writel(cfg, &clk->src_lcd0);
+	clrsetbits_le32(&clk->src_lcd0, 0x9 << 12, 0x6 << 12);
 
 	/*
 	 * CLK_SRC_MASK_LCD0

@@ -1208,9 +1177,7 @@ void exynos4_set_mipi_clk(void)
 	 * MIPI0_MASK		[12]
 	 * set src mask mipi0 0x1: Unmask
 	 */
-	cfg = readl(&clk->src_mask_lcd0);
-	cfg |= (0x1 << 12);
-	writel(cfg, &clk->src_mask_lcd0);
+	setbits_le32(&clk->src_mask_lcd0, 0x1 << 12);
 
 	/*
 	 * CLK_GATE_IP_LCD0

@@ -1222,9 +1189,7 @@ void exynos4_set_mipi_clk(void)
 	 * CLK_PPMULCD0		[5]
 	 * Gating all clocks for MIPI0
 	 */
-	cfg = readl(&clk->gate_ip_lcd0);
-	cfg |= 1 << 3;
-	writel(cfg, &clk->gate_ip_lcd0);
+	setbits_le32(&clk->gate_ip_lcd0, 1 << 3);
 
 	/*
 	 * CLK_DIV_LCD0

@@ -1236,9 +1201,7 @@ void exynos4_set_mipi_clk(void)
 	 * MIPI0_PRE_RATIO	[23:20]
 	 * set mipi ratio
 	 */
-	cfg &= ~(0xf << 16);
-	cfg |= (0x1 << 16);
-	writel(cfg, &clk->div_lcd0);
+	clrsetbits_le32(&clk->div_lcd0, 0xe << 16, 0x1 << 16);
 }
 
 /*

Use setbits/clrbits macro instead of readl/writel function

Signed-off-by Inha Song <[email protected]>

---
  arch/arm/cpu/armv7/exynos/clock.c |   87 +++++++++++--------------------------
  1 file changed, 25 insertions(+), 62 deletions(-)

arch/arm/cpu/armv7/exynos/clock.c +24 -59

index 5bde9d1..eaabfb9 100644
@@ -870,7 +870,6 @@ static void exynos4_set_mmc_clk(int dev_index, unsigned int div)
  	struct exynos4_clock *clk =
  		(struct exynos4_clock *)samsung_get_base_clock();
  	unsigned int addr;
-	unsigned int val;
 
  	/*
  	 * CLK_DIV_FSYS1

@@ -890,10 +889,9 @@ static void exynos4_set_mmc_clk(int dev_index, unsigned int div)
  		dev_index -= 2;
  	}
 
-	val = readl(addr);
-	val &= ~(0xff << ((dev_index << 4) + 8));
-	val |= (div & 0xff) << ((dev_index << 4) + 8);
-	writel(val, addr);
+	clrsetbits_le32(addr,
+			0xff << ((dev_index << 4) + 8),
+			(div & 0xff) << ((dev_index << 4) + 8));
  }
 
  /* exynos4x12: set the mmc clock */

@@ -902,7 +900,6 @@ static void exynos4x12_set_mmc_clk(int dev_index, unsigned int div)
  	struct exynos4x12_clock *clk =
  		(struct exynos4x12_clock *)samsung_get_base_clock();
  	unsigned int addr;
-	unsigned int val;
 
  	/*
  	 * CLK_DIV_FSYS1

@@ -917,10 +914,9 @@ static void exynos4x12_set_mmc_clk(int dev_index, unsigned int div)
  		dev_index -= 2;
  	}
 
-	val = readl(addr);
-	val &= ~(0xff << ((dev_index << 4) + 8));
-	val |= (div & 0xff) << ((dev_index << 4) + 8);
-	writel(val, addr);
+	clrsetbits_le32(addr,
+			0xff << ((dev_index << 4) + 8),
+			(div & 0xff) << ((dev_index << 4) + 8));
  }
 
  /* exynos5: set the mmc clock */

@@ -929,7 +925,6 @@ static void exynos5_set_mmc_clk(int dev_index, unsigned int div)
  	struct exynos5_clock *clk =
  		(struct exynos5_clock *)samsung_get_base_clock();
  	unsigned int addr;
-	unsigned int val;
 
  	/*
  	 * CLK_DIV_FSYS1

@@ -944,10 +939,9 @@ static void exynos5_set_mmc_clk(int dev_index, unsigned int div)
  		dev_index -= 2;
  	}
 
-	val = readl(addr);
-	val &= ~(0xff << ((dev_index << 4) + 8));
-	val |= (div & 0xff) << ((dev_index << 4) + 8);
-	writel(val, addr);
+	clrsetbits_le32(addr,
+			0xff << ((dev_index << 4) + 8),
+			(div & 0xff) << ((dev_index << 4) + 8));
  }
 
  /* exynos5: set the mmc clock */

@@ -956,7 +950,7 @@ static void exynos5420_set_mmc_clk(int dev_index, unsigned int div)
  	struct exynos5420_clock *clk =
  		(struct exynos5420_clock *)samsung_get_base_clock();
  	unsigned int addr;
-	unsigned int val, shift;
+	unsigned int shift;
 
  	/*
  	 * CLK_DIV_FSYS1

@@ -967,10 +961,9 @@ static void exynos5420_set_mmc_clk(int dev_index, unsigned int div)
  	addr = (unsigned int)&clk->div_fsys1;
  	shift = dev_index * 10;
 
-	val = readl(addr);
-	val &= ~(0x3ff << shift);
-	val |= (div & 0x3ff) << shift;
-	writel(val, addr);
+	clrsetbits_le32(addr,
+			0x3ff << shift,
+			(div & 0x3ff) << shift);
  }
 
  /* get_lcd_clk: return lcd clock frequency */

@@ -1061,7 +1054,6 @@ void exynos4_set_lcd_clk(void)
  {
  	struct exynos4_clock *clk =
  	    (struct exynos4_clock *)samsung_get_base_clock();
-	unsigned int cfg = 0;
 
  	/*
  	 * CLK_GATE_BLOCK

@@ -1073,9 +1065,7 @@ void exynos4_set_lcd_clk(void)
  	 * CLK_LCD1	[5]
  	 * CLK_GPS	[7]
  	 */
-	cfg = readl(&clk->gate_block);
-	cfg |= 1 << 4;
-	writel(cfg, &clk->gate_block);
+	setbits_le32(&clk->gate_block, 1 << 4);
 
  	/*
  	 * CLK_SRC_LCD0

@@ -1085,10 +1075,7 @@ void exynos4_set_lcd_clk(void)
  	 * MIPI0_SEL		[12:15]
  	 * set lcd0 src clock 0x6: SCLK_MPLL
  	 */
-	cfg = readl(&clk->src_lcd0);
-	cfg &= ~(0xf);
-	cfg |= 0x6;
-	writel(cfg, &clk->src_lcd0);
+	clrsetbits_le32(&clk->src_lcd0, 0x9, 0x6);
 
  	/*
  	 * CLK_GATE_IP_LCD0

@@ -1100,9 +1087,7 @@ void exynos4_set_lcd_clk(void)
  	 * CLK_PPMULCD0		[5]
  	 * Gating all clocks for FIMD0
  	 */
-	cfg = readl(&clk->gate_ip_lcd0);
-	cfg |= 1 << 0;
-	writel(cfg, &clk->gate_ip_lcd0);
+	setbits_le32(&clk->gate_ip_lcd0, 1 << 0);
 
  	/*
  	 * CLK_DIV_LCD0

@@ -1114,16 +1099,13 @@ void exynos4_set_lcd_clk(void)
  	 * MIPI0_PRE_RATIO	[23:20]
  	 * set fimd ratio
  	 */
-	cfg &= ~(0xf);
-	cfg |= 0x1;
-	writel(cfg, &clk->div_lcd0);
+	clrsetbits_le32(&clk->div_lcd0, 0xe, 0x1);
  }
 
  void exynos5_set_lcd_clk(void)
  {
  	struct exynos5_clock *clk =
  	    (struct exynos5_clock *)samsung_get_base_clock();
-	unsigned int cfg = 0;
 
  	/*
  	 * CLK_GATE_BLOCK

@@ -1135,9 +1117,7 @@ void exynos5_set_lcd_clk(void)
  	 * CLK_LCD1	[5]
  	 * CLK_GPS	[7]
  	 */
-	cfg = readl(&clk->gate_block);
-	cfg |= 1 << 4;
-	writel(cfg, &clk->gate_block);
+	setbits_le32(&clk->gate_block, 1 << 4);
 
  	/*
  	 * CLK_SRC_LCD0

@@ -1147,10 +1127,7 @@ void exynos5_set_lcd_clk(void)
  	 * MIPI0_SEL		[12:15]
  	 * set lcd0 src clock 0x6: SCLK_MPLL
  	 */
-	cfg = readl(&clk->src_disp1_0);
-	cfg &= ~(0xf);
-	cfg |= 0x6;
-	writel(cfg, &clk->src_disp1_0);
+	clrsetbits_le32(&clk->src_disp1_0, 0x9, 0x6);
 
  	/*
  	 * CLK_GATE_IP_LCD0

@@ -1162,9 +1139,7 @@ void exynos5_set_lcd_clk(void)
  	 * CLK_PPMULCD0		[5]
  	 * Gating all clocks for FIMD0
  	 */
-	cfg = readl(&clk->gate_ip_disp1);
-	cfg |= 1 << 0;
-	writel(cfg, &clk->gate_ip_disp1);
+	setbits_le32(&clk->gate_ip_disp1, 1 << 0);
 
  	/*
  	 * CLK_DIV_LCD0

@@ -1176,16 +1151,13 @@ void exynos5_set_lcd_clk(void)
  	 * MIPI0_PRE_RATIO	[23:20]
  	 * set fimd ratio
  	 */
-	cfg &= ~(0xf);
-	cfg |= 0x0;
-	writel(cfg, &clk->div_disp1_0);
+	clrbits_le32(&clk->div_disp1_0, 0xf);
  }
 
  void exynos4_set_mipi_clk(void)
  {
  	struct exynos4_clock *clk =
  	    (struct exynos4_clock *)samsung_get_base_clock();
-	unsigned int cfg = 0;
 
  	/*
  	 * CLK_SRC_LCD0

@@ -1195,10 +1167,7 @@ void exynos4_set_mipi_clk(void)
  	 * MIPI0_SEL		[12:15]
  	 * set mipi0 src clock 0x6: SCLK_MPLL
  	 */
-	cfg = readl(&clk->src_lcd0);
-	cfg &= ~(0xf << 12);
-	cfg |= (0x6 << 12);
-	writel(cfg, &clk->src_lcd0);
+	clrsetbits_le32(&clk->src_lcd0, 0x9 << 12, 0x6 << 12);
 
  	/*
  	 * CLK_SRC_MASK_LCD0

@@ -1208,9 +1177,7 @@ void exynos4_set_mipi_clk(void)
  	 * MIPI0_MASK		[12]
  	 * set src mask mipi0 0x1: Unmask
  	 */
-	cfg = readl(&clk->src_mask_lcd0);
-	cfg |= (0x1 << 12);
-	writel(cfg, &clk->src_mask_lcd0);
+	setbits_le32(&clk->src_mask_lcd0, 0x1 << 12);
 
  	/*
  	 * CLK_GATE_IP_LCD0

@@ -1222,9 +1189,7 @@ void exynos4_set_mipi_clk(void)
  	 * CLK_PPMULCD0		[5]
  	 * Gating all clocks for MIPI0
  	 */
-	cfg = readl(&clk->gate_ip_lcd0);
-	cfg |= 1 << 3;
-	writel(cfg, &clk->gate_ip_lcd0);
+	setbits_le32(&clk->gate_ip_lcd0, 1 << 3);
 
  	/*
  	 * CLK_DIV_LCD0

Use setbits/clrbits macro instead of readl/writel function

Signed-off-by Inha Song <[email protected]>

Quoted 4 lines · Inha Song

Use setbits/clrbits macro instead of readl/writel function

Signed-off-by Inha Song <[email protected]>

You can probably trim down the subject line's length by omitting the "change to" words. It's obvious that a patch changes something.

It's true that your replacing open-coded read-modify-write sequences with bit manipulation macros much better reflects what actually is happening from the application's POV. But your patch does more than that (supported by the "now works for me" reply), and I feel that your commit message should reflect this so others can be aware (both of the fix, and of the former bug).

"In bypassing" you fix a few bugs (I noticed those around lines 1114 and 1176, and only because I remembered your other submission which you don't reference) where random data was written to clock control registers (either uninitialized data, or potentially stale data that was set from unrelated sources).

So you may want to followup to your former submission, stating that it has become obsolete or superseeded. Or send out a v2 series (or any other appropriate version number) with a reference to the former patch, when you consider this new patch the improvement to your earlier submission. This helps reviewers and maintainers.

Quoted 9 lines

arch/arm/cpu/armv7/exynos/clock.c +1 -3

@@ -1114,16 +1099,13 @@ void exynos4_set_lcd_clk(void)
 	 * MIPI0_PRE_RATIO	[23:20]
 	 * set fimd ratio
 	 */
-	cfg &= ~(0xf);
-	cfg |= 0x1;
-	writel(cfg, &clk->div_lcd0);
+	clrsetbits_le32(&clk->div_lcd0, 0xe, 0x1);
 }

Quoted 1 line

There still is the question whether you happen to set individual bits (and where clearing and setting the very same bit is unexpected), or whether you are writing an integer value into a bitfield that only spans part of a register. And whether using symbolic identifiers helps readers and fellow developers since magic numbers obfuscate the motivation.

Please check for those style issues. I've seen e.g. both "and 0xf, or 0x1" as well as "and 0x9, or 0x6" which is inconsistent. Unless this is the "bits vs numbers" thing, where comments may help. (Ignore this latter concern if there already are those comments, which just are not in the patch's context. I haven't checked the source but only looked at your patch.)