Re: [U-Boot] [PATCH] arm: exynos: change to use clrbits macro instead of readl/writel function
On 16/01/14 01:09, Gerhard Sittig wrote: > 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 > > 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. Indeed.. there are many magic numbers in this file. At past, we were agreed to use "magic numbers with comment" instead of defines, because of much changes. For now, of course it can be modified to using defines. But I think it is not a scope of this patch. :) > > 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.) Thanks, Minkyu Kang. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] arm: exynos: change to use clrbits macro instead of readl/writel function
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 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 -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] arm: exynos: change to use clrbits macro instead of readl/writel function
Hello Inha,
On 01/14/2014 05:01 AM, Inha Song wrote:
Use setbits/clrbits macro instead of readl/writel function
Signed-off-by: Inha Song
---
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));
}
/* 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);
+ clrset
Re: [U-Boot] [PATCH] arm: exynos: change to use clrbits macro instead of readl/writel function
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
> ---
> 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
>
