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

[Wolfgang Denk]

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.

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

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Diplomacy is the art of saying "nice doggy" until you can find a rock.