On 03/08/2013 03:16:52 PM, Wolfgang Denk wrote:
Dear Scott,
In message
<[email protected]> you wrote:
> C99's strict aliasing rules are insane to use in low-level code
such as a
> bootloader, but as Wolfgang has rejected -fno-strict-aliasing in the
> past, add a union so that 16-bit accesses can be performed.
Sorry, I saw this patch only after re-inventing the fix for 8xx.
> #ifdef CONFIG_HARD_I2C
> - *((unsigned short*)(&immr->im_dprambase[PROFF_I2C_BASE])) = 0;
> + immr->im_dprambase16[PROFF_I2C_BASE / 2] = 0;
I have to admit that I dislike this approach pretty much.
I think we agree that, if we attempted to play strictly by the rules,
this code should probably rewritten using C structs and proper I/O
accessors. But then, both 8xx and 82xx are essentially dead horses,
and I guess you have no more enthusiasm in cleaning up that old code
than me. So let's ignore that for now.
Yeah. Especially since I don't have easy access to hardware to test
this stuff, I wanted to be as conservative as possible with the
changes, to just address the build breakage.
But this "...[OFFSET / 2]" is basicly unreadable. Can we please at
least make this "...[OFFSET / sizeof(u16)]" so the reader gets a hint
of where this is coming from?
OK.
> --- a/arch/powerpc/cpu/mpc8xx/cpu.c
> +++ b/arch/powerpc/cpu/mpc8xx/cpu.c
> @@ -79,7 +79,7 @@ static int check_CPU (long clock, uint pvr, uint
immr)
> if ((pvr >> 16) != 0x0050)
> return -1;
>
> - k = (immr << 16) | *((ushort *) &
immap->im_cpm.cp_dparam[0xB0]);
> + k = (immr << 16) | immap->im_cpm.cp_dparam16[0xB0 / 2];
> m = 0;
> suf = "";
>
> @@ -195,7 +195,7 @@ static int check_CPU (long clock, uint pvr,
uint immr)
> if ((pvr >> 16) != 0x0050)
> return -1;
>
> - k = (immr << 16) | *((ushort *) &
immap->im_cpm.cp_dparam[0xB0]);
> + k = (immr << 16) | in_be16((ushort
*)&immap->im_cpm.cp_dparam[0xB0]);
Now this is very inconsistent - you convert the very same code line in
very different ways here. Please don't.
Sorry -- I started to use the accessor approach, and then changed my
mind, and some of that accidentally leaked through.
Is there any specific reason you did not use a similar approach of
using in_be16() in the other locations? Actually I feel this is still
the most readable version - I prefer this, even though it clashes
with the style of the rest of the code.
Besides the issue of so much else not using accessors -- I certainly
didn't want to get asked to convert the entire thing :-) -- switching
to an I/O accessor would change the generated code slightly, and I
wanted to avoid that since I can't test it.
It also doesn't really address the problem -- it's still type-punning,
just not noticed by the compiler due to how in_be16() is implemented.
I'm not sure why this is acceptable but -fno-strict-aliasing isn't.
Oh, and can we please get rid of this magic number 0xB0 here, and
introduce PROFF_REVNUM like we do everywhere else?
I suppose, though again I'd rather not get into doing random cleanups
on this code.
-Scott
_______________________________________________
U-Boot mailing list
[email protected]
http://lists.denx.de/mailman/listinfo/u-boot
