Le 30/06/2017 à 15:11, Wolfgang Denk a écrit :
Dear Christophe Leroy,
In message
<8947f895b86efe0396b1825998a64a3340fff597.1498751837.git.christophe.le...@c-s.fr>
you wrote:
Signed-off-by: Christophe Leroy <christophe.le...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
