[U-Boot] [PATCH] powerpc: fix 8xx and 82xx type-punning warnings with GCC 4.7
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(-)
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 */
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
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
participants (2)
-
Scott Wood -
Wolfgang Denk