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

[Christophe LEROY]

Le 30/06/2017 à 15:11, Wolfgang Denk a écrit :
> Dear Christophe Leroy,
> 
> In message <8947f895b86efe0396b1825998a64a3340fff597.1498751837.git.christophe.leroy at c-s.fr> you wrote:
>> Signed-off-by: Christophe Leroy <christophe.leroy at 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
>