[U-Boot,v2,04/10] powerpc, 8xx: Implement GLL2 ERRATA
diff mbox

Message ID 466a431b5430548a018c21222080ed4040596147.1499329461.git.christophe.leroy@c-s.fr
State Superseded
Delegated to: Tom Rini
Headers show

Commit Message

Christophe Leroy July 6, 2017, 8:33 a.m. UTC
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 arch/powerpc/cpu/mpc8xx/cpu_init.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Heiko Schocher July 6, 2017, 9:43 a.m. UTC | #1
Hello Christophe,

Am 06.07.2017 um 10:33 schrieb Christophe Leroy:
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
>   arch/powerpc/cpu/mpc8xx/cpu_init.c | 8 ++++++++
>   1 file changed, 8 insertions(+)

Reviewed-by: Heiko Schocher <hs@denx.de>

nitpick only

> diff --git a/arch/powerpc/cpu/mpc8xx/cpu_init.c b/arch/powerpc/cpu/mpc8xx/cpu_init.c
> index cf1280983a..52406e8483 100644
> --- a/arch/powerpc/cpu/mpc8xx/cpu_init.c
> +++ b/arch/powerpc/cpu/mpc8xx/cpu_init.c
> @@ -51,6 +51,14 @@ void cpu_init_f(immap_t __iomem *immr)
>   	clrsetbits_be32(&immr->im_clkrst.car_sccr, ~SCCR_MASK,
>   			CONFIG_SYS_SCCR);
>
> +	/* BUG MPC866 GLL2 consideration */
> +	reg = in_be32(&immr->im_clkrst.car_sccr);
> +	/* probably we use the mode 1:2:1 */
> +	if ((reg & 0x00060000) == 0x00020000) {
> +		clrbits_be32(&immr->im_clkrst.car_sccr, 0x00060000);
> +		setbits_be32(&immr->im_clkrst.car_sccr, 0x00020000);

You may can introduce defines for the magic values here.

> +	}
> +
>   	/* PLL (CPU clock) settings (15-30) */
>
>   	out_be32(&immr->im_clkrstk.cark_plprcrk, KAPWR_KEY);
>

bye,
Heiko
Wolfgang Denk July 6, 2017, 10:56 a.m. UTC | #2
Dear Christophe,

In message <466a431b5430548a018c21222080ed4040596147.1499329461.git.christophe.leroy@c-s.fr> you wrote:
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
>  arch/powerpc/cpu/mpc8xx/cpu_init.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/powerpc/cpu/mpc8xx/cpu_init.c b/arch/powerpc/cpu/mpc8xx/cpu_init.c
> index cf1280983a..52406e8483 100644
> --- a/arch/powerpc/cpu/mpc8xx/cpu_init.c
> +++ b/arch/powerpc/cpu/mpc8xx/cpu_init.c
> @@ -51,6 +51,14 @@ void cpu_init_f(immap_t __iomem *immr)
>  	clrsetbits_be32(&immr->im_clkrst.car_sccr, ~SCCR_MASK,
>  			CONFIG_SYS_SCCR);
>  
> +	/* BUG MPC866 GLL2 consideration */
> +	reg = in_be32(&immr->im_clkrst.car_sccr);
> +	/* probably we use the mode 1:2:1 */
> +	if ((reg & 0x00060000) == 0x00020000) {
> +		clrbits_be32(&immr->im_clkrst.car_sccr, 0x00060000);
> +		setbits_be32(&immr->im_clkrst.car_sccr, 0x00020000);
> +	}

Like a few lines above, you could/should use a single call to
clrsetbits_be32() here.  And as Heiko already commented, please use
readable names istead of the magic numbers.

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


Best regards,

Wolfgang Denk
Christophe Leroy July 6, 2017, 11:12 a.m. UTC | #3
Le 06/07/2017 à 12:56, Wolfgang Denk a écrit :
> Dear Christophe,
> 
> In message <466a431b5430548a018c21222080ed4040596147.1499329461.git.christophe.leroy@c-s.fr> you wrote:
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>>   arch/powerpc/cpu/mpc8xx/cpu_init.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/arch/powerpc/cpu/mpc8xx/cpu_init.c b/arch/powerpc/cpu/mpc8xx/cpu_init.c
>> index cf1280983a..52406e8483 100644
>> --- a/arch/powerpc/cpu/mpc8xx/cpu_init.c
>> +++ b/arch/powerpc/cpu/mpc8xx/cpu_init.c
>> @@ -51,6 +51,14 @@ void cpu_init_f(immap_t __iomem *immr)
>>   	clrsetbits_be32(&immr->im_clkrst.car_sccr, ~SCCR_MASK,
>>   			CONFIG_SYS_SCCR);
>>   
>> +	/* BUG MPC866 GLL2 consideration */
>> +	reg = in_be32(&immr->im_clkrst.car_sccr);
>> +	/* probably we use the mode 1:2:1 */
>> +	if ((reg & 0x00060000) == 0x00020000) {
>> +		clrbits_be32(&immr->im_clkrst.car_sccr, 0x00060000);
>> +		setbits_be32(&immr->im_clkrst.car_sccr, 0x00020000);
>> +	}
> 
> Like a few lines above, you could/should use a single call to
> clrsetbits_be32() here.  And as Heiko already commented, please use
> readable names istead of the magic numbers.

I shall not use clrsetbits_be32(), because the ERRATA says:

Program the PLPRCR such that the PLL clock will change, then reprogram 
the PLPRCR value back to the desired value

Christophe

> 
> Reviewed-by: Wolfgang Denk <wd@denx.de>
> 
> 
> Best regards,
> 
> Wolfgang Denk
>
Christophe Leroy July 6, 2017, 11:18 a.m. UTC | #4
Oops, I copied wrong alternative of the ERRATA. Correct one this time.

Le 06/07/2017 à 13:12, Christophe LEROY a écrit :
>
>
> Le 06/07/2017 à 12:56, Wolfgang Denk a écrit :
>> Dear Christophe,
>>
>> In message 
>> <466a431b5430548a018c21222080ed4040596147.1499329461.git.christophe.leroy@c-s.fr> 
>> you wrote:
>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>> ---
>>>   arch/powerpc/cpu/mpc8xx/cpu_init.c | 8 ++++++++
>>>   1 file changed, 8 insertions(+)
>>>
>>> diff --git a/arch/powerpc/cpu/mpc8xx/cpu_init.c 
>>> b/arch/powerpc/cpu/mpc8xx/cpu_init.c
>>> index cf1280983a..52406e8483 100644
>>> --- a/arch/powerpc/cpu/mpc8xx/cpu_init.c
>>> +++ b/arch/powerpc/cpu/mpc8xx/cpu_init.c
>>> @@ -51,6 +51,14 @@ void cpu_init_f(immap_t __iomem *immr)
>>>       clrsetbits_be32(&immr->im_clkrst.car_sccr, ~SCCR_MASK,
>>>               CONFIG_SYS_SCCR);
>>>   +    /* BUG MPC866 GLL2 consideration */
>>> +    reg = in_be32(&immr->im_clkrst.car_sccr);
>>> +    /* probably we use the mode 1:2:1 */
>>> +    if ((reg & 0x00060000) == 0x00020000) {
>>> +        clrbits_be32(&immr->im_clkrst.car_sccr, 0x00060000);
>>> +        setbits_be32(&immr->im_clkrst.car_sccr, 0x00020000);
>>> +    }
>>
>> Like a few lines above, you could/should use a single call to
>> clrsetbits_be32() here.  And as Heiko already commented, please use
>> readable names istead of the magic numbers.
>
> I shall not use clrsetbits_be32(), because the ERRATA says:

The ERRATA says:
Reprogram the SCCR:
1.   Write 1'b00 to SCCR[EBDF].
2.   Write 1'b01 to SCCR[EBDF].
3.   Rewrite the desired value to the PLPRCR register

Christophe

>
> Program the PLPRCR such that the PLL clock will change, then reprogram 
> the PLPRCR value back to the desired value
>
> Christophe
>
>>
>> Reviewed-by: Wolfgang Denk <wd@denx.de>
>>
>>
>> Best regards,
>>
>> Wolfgang Denk
>>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
Wolfgang Denk July 6, 2017, 11:20 a.m. UTC | #5
Dear Christophe,

In message <1e6c1b5c-2e49-6784-6d6d-f4532aa20434@c-s.fr> you wrote:
> 
> > Like a few lines above, you could/should use a single call to
> > clrsetbits_be32() here.  And as Heiko already commented, please use
> > readable names istead of the magic numbers.
> 
> I shall not use clrsetbits_be32(), because the ERRATA says:
> 
> Program the PLPRCR such that the PLL clock will change, then reprogram 
> the PLPRCR value back to the desired value

Ah! This is critical information, so please add a comment to explain
this.  [Otherwise there is the risk some later "optimization" intro-
duces a bug.]

Best regards,

Wolfgang Denk

Patch
diff mbox

diff --git a/arch/powerpc/cpu/mpc8xx/cpu_init.c b/arch/powerpc/cpu/mpc8xx/cpu_init.c
index cf1280983a..52406e8483 100644
--- a/arch/powerpc/cpu/mpc8xx/cpu_init.c
+++ b/arch/powerpc/cpu/mpc8xx/cpu_init.c
@@ -51,6 +51,14 @@  void cpu_init_f(immap_t __iomem *immr)
 	clrsetbits_be32(&immr->im_clkrst.car_sccr, ~SCCR_MASK,
 			CONFIG_SYS_SCCR);
 
+	/* BUG MPC866 GLL2 consideration */
+	reg = in_be32(&immr->im_clkrst.car_sccr);
+	/* probably we use the mode 1:2:1 */
+	if ((reg & 0x00060000) == 0x00020000) {
+		clrbits_be32(&immr->im_clkrst.car_sccr, 0x00060000);
+		setbits_be32(&immr->im_clkrst.car_sccr, 0x00020000);
+	}
+
 	/* PLL (CPU clock) settings (15-30) */
 
 	out_be32(&immr->im_clkrstk.cark_plprcrk, KAPWR_KEY);