[U-Boot,2/7] powerpc, 8xx: Implement GLL2 ERRATA
diff mbox

Message ID 8947f895b86efe0396b1825998a64a3340fff597.1498751837.git.christophe.leroy@c-s.fr
State Superseded
Delegated to: Tom Rini
Headers show

Commit Message

Christophe Leroy June 29, 2017, 4:54 p.m. UTC
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 arch/powerpc/cpu/mpc8xx/cpu_init.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Wolfgang Denk June 30, 2017, 1:11 p.m. UTC | #1
Dear Christophe Leroy,

In message <8947f895b86efe0396b1825998a64a3340fff597.1498751837.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 | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/powerpc/cpu/mpc8xx/cpu_init.c b/arch/powerpc/cpu/mpc8xx/cpu_init.c
> index 0f935aff9e..e8045cc5c6 100644
> --- a/arch/powerpc/cpu/mpc8xx/cpu_init.c
> +++ b/arch/powerpc/cpu/mpc8xx/cpu_init.c
> @@ -55,6 +55,16 @@ void cpu_init_f (volatile immap_t * immr)
>  	reg |= CONFIG_SYS_SCCR;
>  	immr->im_clkrst.car_sccr = reg;
>  
> +	/* BUG MPC866 GLL2 consideration */
> +	reg = immr->im_clkrst.car_sccr;
> +	/* probably we use the mode 1:2:1 */
> +	if ((reg & 0x00060000) == 0x00020000) {
> +		reg &= ~0x00060000;
> +		immr->im_clkrst.car_sccr = reg;
> +		reg |= 0x00020000;
> +		immr->im_clkrst.car_sccr = reg;
> +	}
> +

This is an excellent example for future situations, so thanks a lot
for bringing it up early.

The code you are adding here violates basic (modern) programming
standards.  To access device registers, proper I/O accessors must be
used.  In this case the code should use the clrsetbits_be32() macro.

Strictly speaking, you should receive a full NAK on this code.

You may now argument that the surrounding code is full of similar
examples of obsolete, broken code, and you are just following this
(bad) example.

The standard reply to this would be: well, while you are modifying
this file, please clean up the other ocurrences of such bad code as
well.


Are you aware of this process?  How do you think to handle such
situations in the future?


What I really, really am scared of is that you might follow your
employer's requirements ("new version only introduces a reduced
amount of modifications in source code") instead of following your
duties as a custodian (cleaning up the code to meet current
programming standards).

Can you please make an explixit statement here how you are going to
handle this in the future?  [And again, schedule is important, too.]

Best regards,

Wolfgang Denk
Christophe Leroy July 4, 2017, 8:10 a.m. UTC | #2
Le 30/06/2017 à 15:11, Wolfgang Denk a écrit :
> Dear Christophe Leroy,
> 
> In message <8947f895b86efe0396b1825998a64a3340fff597.1498751837.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 | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/arch/powerpc/cpu/mpc8xx/cpu_init.c b/arch/powerpc/cpu/mpc8xx/cpu_init.c
>> index 0f935aff9e..e8045cc5c6 100644
>> --- a/arch/powerpc/cpu/mpc8xx/cpu_init.c
>> +++ b/arch/powerpc/cpu/mpc8xx/cpu_init.c
>> @@ -55,6 +55,16 @@ void cpu_init_f (volatile immap_t * immr)
>>   	reg |= CONFIG_SYS_SCCR;
>>   	immr->im_clkrst.car_sccr = reg;
>>   
>> +	/* BUG MPC866 GLL2 consideration */
>> +	reg = immr->im_clkrst.car_sccr;
>> +	/* probably we use the mode 1:2:1 */
>> +	if ((reg & 0x00060000) == 0x00020000) {
>> +		reg &= ~0x00060000;
>> +		immr->im_clkrst.car_sccr = reg;
>> +		reg |= 0x00020000;
>> +		immr->im_clkrst.car_sccr = reg;
>> +	}
>> +
> 
> This is an excellent example for future situations, so thanks a lot
> for bringing it up early.
> 
> The code you are adding here violates basic (modern) programming
> standards.  To access device registers, proper I/O accessors must be
> used.  In this case the code should use the clrsetbits_be32() macro.

Oh right, that's the way it is done in Linux Kernel, I used to be 
surprised it was not that way in U-Boot. If it is like this now, that's 
good.


> 
> Strictly speaking, you should receive a full NAK on this code.
> 
> You may now argument that the surrounding code is full of similar
> examples of obsolete, broken code, and you are just following this
> (bad) example.

No, I may argument that the code I'm submitting is not new code but code 
that was written in 2010, and maybe even before, when we were using ppcboot.

> 
> The standard reply to this would be: well, while you are modifying
> this file, please clean up the other ocurrences of such bad code as
> well.
> 
> 
> Are you aware of this process?  How do you think to handle such
> situations in the future?

I am now :)
I'll keep it in mind for the future.

> 
> 
> What I really, really am scared of is that you might follow your
> employer's requirements ("new version only introduces a reduced
> amount of modifications in source code") instead of following your
> duties as a custodian (cleaning up the code to meet current
> programming standards).

Do not worry. Any justified improvement is worth it. My only employer's 
requirement is "make sure it will be accepted by Air Trafic Control 
authorities". All is a matter of justification and I'm not worried about 
it as far as I manage to keep commits focussed on only one thing. U-boot 
General Patch Submission Rules states 'Changes that contain different, 
unrelated modifications shall be submitted as separate patches, one 
patch per changeset', so this is not in contradiction with my needs.

What the auditors really focus on is the deltas between the COTS (ie the 
official U-boot release) and the sources we use for them. So it is of 
our interest to have our changes/fixes properly merged into U-boot.

> 
> Can you please make an explixit statement here how you are going to
> handle this in the future?  [And again, schedule is important, too.]

My plan is to convert to the use of IO accessors during this week.

Best regards
Christophe

> 
> Best regards,
> 
> Wolfgang Denk
>
Wolfgang Denk July 4, 2017, 10:25 a.m. UTC | #3
Dear Christophe,

In message <556dc687-0331-40a6-4260-9f78f6131e47@c-s.fr> you wrote:
> 
> > The code you are adding here violates basic (modern) programming
> > standards.  To access device registers, proper I/O accessors must be
> > used.  In this case the code should use the clrsetbits_be32() macro.
> 
> Oh right, that's the way it is done in Linux Kernel, I used to be 
> surprised it was not that way in U-Boot. If it is like this now, that's 
> good.

It's been here for ages as well.  It's only really ancient (and not
well maintained) code that does not use it yet.

> > You may now argument that the surrounding code is full of similar
> > examples of obsolete, broken code, and you are just following this
> > (bad) example.
> 
> No, I may argument that the code I'm submitting is not new code but code 
> that was written in 2010, and maybe even before, when we were using ppcboot.

So what? You are submitting it today, to be included into curent
mainline.

> > Are you aware of this process?  How do you think to handle such
> > situations in the future?
> 
> I am now :)
> I'll keep it in mind for the future.

The Correct Thing (TM) to do here is to submit two patches: a first
one to convert the file in question to using I/O accessors, and a
second one to extend the code as needed.

> What the auditors really focus on is the deltas between the COTS (ie the 
> official U-boot release) and the sources we use for them. So it is of 
> our interest to have our changes/fixes properly merged into U-boot.

I'm perplexed.  If that was the case then why did you never care to
minimize such differences?

> > 
> > Can you please make an explixit statement here how you are going to
> > handle this in the future?  [And again, schedule is important, too.]
> 
> My plan is to convert to the use of IO accessors during this week.

Thanks.

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 0f935aff9e..e8045cc5c6 100644
--- a/arch/powerpc/cpu/mpc8xx/cpu_init.c
+++ b/arch/powerpc/cpu/mpc8xx/cpu_init.c
@@ -55,6 +55,16 @@  void cpu_init_f (volatile immap_t * immr)
 	reg |= CONFIG_SYS_SCCR;
 	immr->im_clkrst.car_sccr = reg;
 
+	/* BUG MPC866 GLL2 consideration */
+	reg = immr->im_clkrst.car_sccr;
+	/* probably we use the mode 1:2:1 */
+	if ((reg & 0x00060000) == 0x00020000) {
+		reg &= ~0x00060000;
+		immr->im_clkrst.car_sccr = reg;
+		reg |= 0x00020000;
+		immr->im_clkrst.car_sccr = reg;
+	}
+
 	/* PLL (CPU clock) settings (15-30) */
 
 	immr->im_clkrstk.cark_plprcrk = KAPWR_KEY;