On 03/08/2013 03:16:52 PM, Wolfgang Denk wrote:
Dear Scott,
In message 1357696756-31079-1-git-send-email-scottwood@freescale.com 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