# [U-Boot,01/14] powerpc, 8xx: Simplify brgclk calculation and remove get_brgclk()

Message ID 59ac182501e89d3b9ee1dc7c31ce358ff33c0877.1499629706.git.christophe.leroy@c-s.fr Superseded Tom Rini show

## Commit Message

Christophe Leroy July 12, 2017, 9:43 a.m. UTC
```divider is calculated based on SCCR_DFBRG, with:
SCCR_DFBRG 00 => divider 1  = 1 << 0
SCCR_DFBRG 01 => divider 4  = 1 << 2
SCCR_DFBRG 10 => divider 16 = 1 << 4
SCCR_DFBRG 11 => divider 64 = 1 << 6

This can be easily converted to a single shift operation:
divider = 1 << (SCCR_DFBRG * 2)

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/cpu/mpc8xx/speed.c | 25 +++----------------------
1 file changed, 3 insertions(+), 22 deletions(-)
```

Wolfgang Denk July 12, 2017, 12:52 p.m. UTC | #1
```Dear Christophe,

In message <59ac182501e89d3b9ee1dc7c31ce358ff33c0877.1499629706.git.christophe.leroy@c-s.fr> you wrote:
> divider is calculated based on SCCR_DFBRG, with:
> SCCR_DFBRG 00 => divider 1  = 1 << 0
> SCCR_DFBRG 01 => divider 4  = 1 << 2
> SCCR_DFBRG 10 => divider 16 = 1 << 4
> SCCR_DFBRG 11 => divider 64 = 1 << 6
>
> This can be easily converted to a single shift operation:
> divider = 1 << (SCCR_DFBRG * 2)

Agreed, but...

> -	switch ((sccr & SCCR_DFBRG11) >> 11) {
...
> +	uint divider = 1 << ((sccr & SCCR_DFBRG11) >> 10);

The code would be easier to read / understand if you made the
calculation obvious, i. e.

uint divider = 1 << (((sccr & SCCR_DFBRG11) >> 11) * 2);

The compiler generates the same code, so there is no size effect.

Reviewed-by: Wolfgang Denk <wd@denx.de>

Best regards,

Wolfgang Denk
```
Christophe Leroy July 13, 2017, 1:12 p.m. UTC | #2
```Le 12/07/2017 à 14:52, Wolfgang Denk a écrit :
> Dear Christophe,
>
> In message <59ac182501e89d3b9ee1dc7c31ce358ff33c0877.1499629706.git.christophe.leroy@c-s.fr> you wrote:
>> divider is calculated based on SCCR_DFBRG, with:
>> SCCR_DFBRG 00 => divider 1  = 1 << 0
>> SCCR_DFBRG 01 => divider 4  = 1 << 2
>> SCCR_DFBRG 10 => divider 16 = 1 << 4
>> SCCR_DFBRG 11 => divider 64 = 1 << 6
>>
>> This can be easily converted to a single shift operation:
>> divider = 1 << (SCCR_DFBRG * 2)
>
> Agreed, but...
>
>> -	switch ((sccr & SCCR_DFBRG11) >> 11) {
> ...
>> +	uint divider = 1 << ((sccr & SCCR_DFBRG11) >> 10);
>
> The code would be easier to read / understand if you made the
> calculation obvious, i. e.
>
> 	uint divider = 1 << (((sccr & SCCR_DFBRG11) >> 11) * 2);
>
> The compiler generates the same code, so there is no size effect.

Ok, done in v2

Christophe
>
> Reviewed-by: Wolfgang Denk <wd@denx.de>
>
>
> Best regards,
>
> Wolfgang Denk
>
```

## Patch

```diff --git a/arch/powerpc/cpu/mpc8xx/speed.c b/arch/powerpc/cpu/mpc8xx/speed.c
index 8d43efff6c..21a3de317c 100644
--- a/arch/powerpc/cpu/mpc8xx/speed.c
+++ b/arch/powerpc/cpu/mpc8xx/speed.c
@@ -12,27 +12,6 @@

DECLARE_GLOBAL_DATA_PTR;

-void get_brgclk(uint sccr)
-{
-	uint divider = 0;
-
-	switch ((sccr & SCCR_DFBRG11) >> 11) {
-	case 0:
-		divider = 1;
-		break;
-	case 1:
-		divider = 4;
-		break;
-	case 2:
-		divider = 16;
-		break;
-	case 3:
-		divider = 64;
-		break;
-	}
-	gd->arch.brg_clk = gd->cpu_clk / divider;
-}
-
/*
* get_clocks() fills in gd->cpu_clock depending on CONFIG_8xx_GCLK_FREQ
*/
@@ -41,6 +20,8 @@  int get_clocks(void)
uint immr = get_immr(0);	/* Return full IMMR contents */
immap_t __iomem *immap = (immap_t __iomem *)(immr & 0xFFFF0000);
uint sccr = in_be32(&immap->im_clkrst.car_sccr);
+	uint divider = 1 << ((sccr & SCCR_DFBRG11) >> 10);
+
/*
* If for some reason measuring the gclk frequency won't
* work, we return the hardwired value.
@@ -57,7 +38,7 @@  int get_clocks(void)
gd->bus_clk = gd->cpu_clk / 2;
}

-	get_brgclk(sccr);
+	gd->arch.brg_clk = gd->cpu_clk / divider;

return 0;
}

```