Hi, Inha.

Plz, add Samsung SoC custodian.

Refer to http://www.denx.de/wiki/U-Boot/Custodians

On 01/14/2014 01:01 PM, Inha Song wrote:

Use setbits/clrbits macro instead of readl/writel function

Signed-off-by: Inha Song ideal.song@samsung.com

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

diff --git a/arch/arm/cpu/armv7/exynos/clock.c b/arch/arm/cpu/armv7/exynos/clock.c index 5bde9d1..eaabfb9 100644 --- a/arch/arm/cpu/armv7/exynos/clock.c +++ b/arch/arm/cpu/armv7/exynos/clock.c @@ -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));
  

Can use only two line..like this.

clrestbits_le31(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));
  

Ditto

}

/* 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));
  

Ditto.

}

/* 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);
  

Can use the one line..? why do you use 3-line?

}

/* 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);

}

/*

On Tue, Jan 14, 2014 at 13:01 +0900, Inha Song wrote:

Use setbits/clrbits macro instead of readl/writel function

Signed-off-by: Inha Song ideal.song@samsung.com

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.

@@ -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);

}

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.)

virtually yours Gerhard Sittig