diff mbox

[U-Boot] powerpc: fix 8xx and 82xx type-punning warnings with GCC 4.7

Message ID 1357696756-31079-1-git-send-email-scottwood@freescale.com
State Changes Requested
Delegated to: Wolfgang Denk
Headers show

Commit Message

Scott Wood Jan. 9, 2013, 1:59 a.m. UTC 
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.

Compile-tested only.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 arch/powerpc/cpu/mpc8260/commproc.c        |    2 +-
 arch/powerpc/cpu/mpc8260/cpu.c             |    2 +-
 arch/powerpc/cpu/mpc8260/i2c.c             |    8 ++++----
 arch/powerpc/cpu/mpc8260/serial_smc.c      |    4 ++--
 arch/powerpc/cpu/mpc8260/spi.c             |    2 +-
 arch/powerpc/cpu/mpc8xx/cpu.c              |    6 +++---
 arch/powerpc/include/asm/8xx_immap.h       |    7 ++++++-
 arch/powerpc/include/asm/immap_8260.h      |   19 ++++++++++++-------
 common/cmd_immap.c                         |    2 +-
 examples/standalone/mem_to_mem_idma2intr.c |    3 ++-
 10 files changed, 33 insertions(+), 22 deletions(-)

Comments

Wolfgang Denk March 8, 2013, 9:16 p.m. UTC | #1 
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.

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?

> --- 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.

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.

Oh, and can we please get rid of this magic number 0xB0 here, and
introduce PROFF_REVNUM like we do everywhere else?

See http://patchwork.ozlabs.org/patch/226185/ for the needed addition
to arch/powerpc/include/asm/8xx_immap.h

Best regards,

Wolfgang Denk
diff mbox

Patch

diff --git a/arch/powerpc/cpu/mpc8260/commproc.c b/arch/powerpc/cpu/mpc8260/commproc.c
index 082957e..70af613 100644
--- a/arch/powerpc/cpu/mpc8260/commproc.c
+++ b/arch/powerpc/cpu/mpc8260/commproc.c
@@ -43,7 +43,7 @@  m8260_cpm_reset(void)
 	} while ((immr->im_cpm.cp_cpcr & CPM_CR_FLG) && ++count < 1000000);
 
 #ifdef CONFIG_HARD_I2C
-	*((unsigned short*)(&immr->im_dprambase[PROFF_I2C_BASE])) = 0;
+	immr->im_dprambase16[PROFF_I2C_BASE / 2] = 0;
 #endif
 }
 
diff --git a/arch/powerpc/cpu/mpc8260/cpu.c b/arch/powerpc/cpu/mpc8260/cpu.c
index 220c1e2..85d209e 100644
--- a/arch/powerpc/cpu/mpc8260/cpu.c
+++ b/arch/powerpc/cpu/mpc8260/cpu.c
@@ -107,7 +107,7 @@  int checkcpu (void)
 	 * in the mask.
 	 */
 	m = immr & (IMMR_PARTNUM_MSK | IMMR_MASKNUM_MSK);
-	k = *((ushort *) & immap->im_dprambase[PROFF_REVNUM]);
+	k = immap->im_dprambase16[PROFF_REVNUM / 2];
 
 	switch (m) {
 	case 0x0000:
diff --git a/arch/powerpc/cpu/mpc8260/i2c.c b/arch/powerpc/cpu/mpc8260/i2c.c
index 7382cba..70c1b6f 100644
--- a/arch/powerpc/cpu/mpc8260/i2c.c
+++ b/arch/powerpc/cpu/mpc8260/i2c.c
@@ -221,14 +221,14 @@  void i2c_init(int speed, int slaveadd)
 	i2c_init_board();
 #endif
 
-	dpaddr = *((unsigned short *) (&immap->im_dprambase[PROFF_I2C_BASE]));
+	dpaddr = immap->im_dprambase16[PROFF_I2C_BASE / 2];
 	if (dpaddr == 0) {
 		/* need to allocate dual port ram */
 		dpaddr = m8260_cpm_dpalloc(64 +
 					(NUM_RX_BDS * sizeof(I2C_BD)) +
 					(NUM_TX_BDS * sizeof(I2C_BD)) +
 					MAX_TX_SPACE, 64);
-		*((unsigned short *)(&immap->im_dprambase[PROFF_I2C_BASE])) =
+		immap->im_dprambase16[PROFF_I2C_BASE / 2] =
 			dpaddr;
 	}
 
@@ -305,7 +305,7 @@  void i2c_newio(i2c_state_t *state)
 
 	debug("[I2C] i2c_newio\n");
 
-	dpaddr = *((unsigned short *)(&immap->im_dprambase[PROFF_I2C_BASE]));
+	dpaddr = immap->im_dprambase16[PROFF_I2C_BASE / 2];
 	iip = (iic_t *)&immap->im_dprambase[dpaddr];
 	state->rx_idx = 0;
 	state->tx_idx = 0;
@@ -480,7 +480,7 @@  int i2c_doio(i2c_state_t *state)
 		return I2CERR_QUEUE_EMPTY;
 	}
 
-	dpaddr = *((unsigned short *)(&immap->im_dprambase[PROFF_I2C_BASE]));
+	dpaddr = immap->im_dprambase16[PROFF_I2C_BASE / 2];
 	iip = (iic_t *)&immap->im_dprambase[dpaddr];
 	iip->iic_rbptr = iip->iic_rbase;
 	iip->iic_tbptr = iip->iic_tbase;
diff --git a/arch/powerpc/cpu/mpc8260/serial_smc.c b/arch/powerpc/cpu/mpc8260/serial_smc.c
index feba1f6..50c81ed 100644
--- a/arch/powerpc/cpu/mpc8260/serial_smc.c
+++ b/arch/powerpc/cpu/mpc8260/serial_smc.c
@@ -105,7 +105,7 @@  static int mpc8260_smc_serial_init(void)
 	/* initialize pointers to SMC */
 
 	sp = (smc_t *) &(im->im_smc[SMC_INDEX]);
-	*(ushort *)(&im->im_dprambase[PROFF_SMC_BASE]) = PROFF_SMC;
+	im->im_dprambase16[PROFF_SMC_BASE / 2] = PROFF_SMC;
 	up = (smc_uart_t *)&im->im_dprambase[PROFF_SMC];
 
 	/* Disable transmitter/receiver. */
@@ -331,7 +331,7 @@  kgdb_serial_init (void)
 	/* initialize pointers to SMC */
 
 	sp = (smc_t *) &(im->im_smc[KGDB_SMC_INDEX]);
-	*(ushort *)(&im->im_dprambase[KGDB_PROFF_SMC_BASE]) = KGDB_PROFF_SMC;
+	im->im_dprambase16[KGDB_PROFF_SMC_BASE / 2] = KGDB_PROFF_SMC;
 	up = (smc_uart_t *)&im->im_dprambase[KGDB_PROFF_SMC];
 
 	/* Disable transmitter/receiver. */
diff --git a/arch/powerpc/cpu/mpc8260/spi.c b/arch/powerpc/cpu/mpc8260/spi.c
index dc98ea7..24386a5 100644
--- a/arch/powerpc/cpu/mpc8260/spi.c
+++ b/arch/powerpc/cpu/mpc8260/spi.c
@@ -146,7 +146,7 @@  void spi_init_f (void)
 	immr = (immap_t *)  CONFIG_SYS_IMMR;
 	cp   = (cpm8260_t *) &immr->im_cpm;
 
-	*(ushort *)(&immr->im_dprambase[PROFF_SPI_BASE]) = PROFF_SPI;
+	immr->im_dprambase16[PROFF_SPI_BASE / 2] = PROFF_SPI;
 	spi  = (spi_t *)&immr->im_dprambase[PROFF_SPI];
 
 /* 1 */
diff --git a/arch/powerpc/cpu/mpc8xx/cpu.c b/arch/powerpc/cpu/mpc8xx/cpu.c
index b3fcfe5..4a11d10 100644
--- 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]);
 	m = 0;
 
 	switch (k) {
@@ -313,7 +313,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]);
 	m = 0;
 
 	switch (k) {
diff --git a/arch/powerpc/include/asm/8xx_immap.h b/arch/powerpc/include/asm/8xx_immap.h
index 40679cb..01129ed 100644
--- a/arch/powerpc/include/asm/8xx_immap.h
+++ b/arch/powerpc/include/asm/8xx_immap.h
@@ -485,7 +485,12 @@  typedef struct comm_proc {
 	 * Some processors don't have all of it populated.
 	 */
 	u_char	cp_dpmem[0x1C00];	/* BD / Data / ucode */
-	u_char	cp_dparam[0x400];	/* Parameter RAM */
+
+	/* Parameter RAM */
+	union {
+		u_char	cp_dparam[0x400];
+		u16	cp_dparam16[0x200];
+	};
 } cpm8xx_t;
 
 /* Internal memory map.
diff --git a/arch/powerpc/include/asm/immap_8260.h b/arch/powerpc/include/asm/immap_8260.h
index 4974ae5..c7021a7 100644
--- a/arch/powerpc/include/asm/immap_8260.h
+++ b/arch/powerpc/include/asm/immap_8260.h
@@ -526,13 +526,18 @@  typedef struct immap {
 	/* Some references are into the unique and known dpram spaces,
 	 * others are from the generic base.
 	 */
-#define im_dprambase	im_dpram1
-	u_char		im_dpram1[16*1024];
-	char		res1[16*1024];
-	u_char		im_dpram2[4*1024];
-	char		res2[8*1024];
-	u_char		im_dpram3[4*1024];
-	char		res3[16*1024];
+	union {
+		struct {
+			u_char		im_dpram1[16 * 1024];
+			char		res1[16 * 1024];
+			u_char		im_dpram2[4 * 1024];
+			char		res2[8 * 1024];
+			u_char		im_dpram3[4 * 1024];
+			char		res3[16 * 1024];
+		};
+		u8	im_dprambase[64 * 1024];
+		u16	im_dprambase16[32 * 1024];
+	};
 
 	sysconf8260_t	im_siu_conf;	/* SIU Configuration */
 	memctl8260_t	im_memctl;	/* Memory Controller */
diff --git a/common/cmd_immap.c b/common/cmd_immap.c
index 1f59c1e..1fcc13d 100644
--- a/common/cmd_immap.c
+++ b/common/cmd_immap.c
@@ -535,7 +535,7 @@  do_i2cinfo (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	volatile iic_t *iip;
 	uint dpaddr;
 
-	dpaddr = *((unsigned short *) (&immap->im_dprambase[PROFF_I2C_BASE]));
+	dpaddr = immap->im_dprambase16[PROFF_I2C_BASE / 2];
 	if (dpaddr == 0)
 		iip = NULL;
 	else
diff --git a/examples/standalone/mem_to_mem_idma2intr.c b/examples/standalone/mem_to_mem_idma2intr.c
index d0a75ea..1b83876 100644
--- a/examples/standalone/mem_to_mem_idma2intr.c
+++ b/examples/standalone/mem_to_mem_idma2intr.c
@@ -309,7 +309,8 @@  int idma_init (void)
 
 	memaddr = dpalloc (sizeof (pram_idma_t), 64);
 
-	*(volatile ushort *) &immap->im_dprambase[PROFF_IDMA2_BASE] = memaddr;
+	*(volatile u16 *)&immap->im_dprambase16[PROFF_IDMA2_BASE / 2] =
+		memaddr;
 	piptr = (volatile pram_idma_t *) ((uint) (immap) + memaddr);
 
 	piptr->pi_resv1 = 0;		/* manual says: clear it */