From mboxrd@z Thu Jan  1 00:00:00 1970
From: Scott Wood <scottwood@freescale.com>
Date: Fri, 8 Mar 2013 18:27:51 -0600
Subject: [U-Boot] [PATCH] powerpc: fix 8xx and 82xx type-punning
 warnings with GCC 4.7
In-Reply-To: <20130308211652.A014F20025F@gemini.denx.de> (from wd@denx.de on
	Fri Mar  8 15:16:52 2013)
Message-ID: <1362788871.29198.8@snotra>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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