Re: [U-Boot-Users] [PATCH 1/3] QE IO: Add initial data to pin configuration

On Mon, 31 Mar 2008 15:13:15 +0300 David Saada David.Saada@ecitele.com wrote:

On the MPC83xx & MPC85xx architectures that have QE, add initial data to the pin configuration table (qe_iop_conf_tab). This is relevant for GPIO pins defined as output. One can setup a value of -1 to leave the value unchanged. In addition, add I/O pin read & write functions.

Signed-off-by: David Saada david.saada@ecitele.com

cpu/mpc83xx/cpu_init.c | 7 ++---

where's the --- line?

cpu/mpc83xx/qe_io.c | 59 ++++++++++++++++++++++++++++++++++++++++++++-----

this patch is seriously line wrapped..and therefore unapplicable.

cpu/mpc85xx/cpu_init.c | 7 ++--- cpu/mpc85xx/qe_io.c | 58 ++++++++++++++++++++++++++++++++++++++++++------ include/ioports.h | 8 ++++++ 5 files changed, 118 insertions(+), 21 deletions(-)

--- a/include/ioports.h 2008-03-28 01:49:12.000000000 +0200 +++ b/include/ioports.h 2008-03-30 16:11:09.514274000 +0300 @@ -60,6 +60,14 @@ typedef struct { int dir; int open_drain; int assign;

• int data;

} qe_iop_conf_t;

#define QE_IOP_TAB_END (-1)

+void qe_config_iopin(u8 port, u8 pin, int dir, int open_drain, int assign,

• 			 int data);
  

+void qe_read_iopin(u8 port, u8 pin, int *data); +void qe_write_iopin(u8 port, u8 pin, int data);

--- a/cpu/mpc85xx/cpu_init.c 2008-03-28 01:49:12.000000000 +0200 +++ b/cpu/mpc85xx/cpu_init.c 2008-03-30 15:42:32.913152000 +0300 @@ -39,15 +39,13 @@ DECLARE_GLOBAL_DATA_PTR;

#ifdef CONFIG_QE extern qe_iop_conf_t qe_iop_conf_tab[]; -extern void qe_config_iopin(u8 port, u8 pin, int dir,

• 		int open_drain, int assign);
  

extern void qe_init(uint qe_base); extern void qe_reset(void);

static void config_qe_ioports(void) { u8 port, pin;

• int dir, open_drain, assign;

• int dir, open_drain, assign, data; int i;

  for (i = 0; qe_iop_conf_tab[i].assign != QE_IOP_TAB_END; i++) {

@@ -56,7 +54,8 @@ static void config_qe_ioports(void) dir = qe_iop_conf_tab[i].dir; open_drain = qe_iop_conf_tab[i].open_drain; assign = qe_iop_conf_tab[i].assign;

• qe_config_iopin(port, pin, dir, open_drain, assign);
  

• data		= qe_iop_conf_tab[i].data;
  

• qe_config_iopin(port, pin, dir, open_drain, assign,
  

data); } } #endif --- a/cpu/mpc85xx/qe_io.c 2008-03-28 01:49:12.000000000 +0200 +++ b/cpu/mpc85xx/qe_io.c 2008-03-30 15:44:56.961332000 +0300 @@ -27,16 +27,30 @@

#if defined(CONFIG_QE) #define NUM_OF_PINS 32 -void qe_config_iopin(u8 port, u8 pin, int dir, int open_drain, int assign) +void qe_config_iopin(u8 port, u8 pin, int dir, int open_drain, int assign,

• 			 int data)
  

{ u32 pin_2bit_mask; u32 pin_2bit_dir; u32 pin_2bit_assign; u32 pin_1bit_mask; u32 tmp_val;

• volatile ccsr_gur_t *gur = (void *)(CFG_MPC85xx_GUTS_ADDR);
• volatile par_io_t *par_io = (volatile par_io_t *)

• 				&(gur->qe_par_io);
  

• ccsr_gur_t *gur = (void *)(CFG_MPC85xx_GUTS_ADDR);
• par_io_t *par_io = (par_io_t *) &(gur->qe_par_io);
• /* Calculate pin location for 1bit mask */
• pin_1bit_mask = (u32)(1 << (NUM_OF_PINS - (pin+1)));

drop the innermost parens, add spaces around +. Is the cast really needed?

• /* Setup the data */
• if ((data != -1) && /* Don't leave unchanged */

• (assign == 0) && 	/* GPIO */
  

• (dir & 1)) { 		/* Has output */
  

alignment

• tmp_val = in_be32(&par_io[port].cpdat);
  

• if (data)
  

• 	out_be32(&par_io[port].cpdat, pin_1bit_mask |
  

tmp_val);

• else
  

• 	out_be32(&par_io[port].cpdat, ~pin_1bit_mask &
  

tmp_val);

• }

how about something like:

if (data) tmp_val |= pin_1bit_mask; else tmp_val &= ~pin_1bit_mask; out_be32(.., tmp_val);

/* Caculate pin location and 2bit mask and dir */ pin_2bit_mask = (u32)(0x3 << (NUM_OF_PINS-(pin%(NUM_OF_PINS/2)+1)*2)); @@ -55,9 +69,6 @@ void qe_config_iopin(u8 port, u8 pin, in out_be32(&par_io[port].cpdir1, pin_2bit_dir | tmp_val); }

• /* Calculate pin location for 1bit mask */
• pin_1bit_mask = (u32)(1 << (NUM_OF_PINS - (pin+1)));
• /* Setup the open drain */ tmp_val = in_be32(&par_io[port].cpodr); if (open_drain)

@@ -82,4 +93,37 @@ void qe_config_iopin(u8 port, u8 pin, in } }

+void qe_read_iopin(u8 port, u8 pin, int *data) +{

• u32 pin_1bit_mask;
• u32 tmp_val;
• ccsr_gur_t *gur = (void *)(CFG_MPC85xx_GUTS_ADDR);
• par_io_t *par_io = (par_io_t *) &(gur->qe_par_io);

alignment (unless it's your mailer messing with whitespace)

• /* Calculate pin location for 1bit mask */
• pin_1bit_mask = (u32)(1 << (NUM_OF_PINS - (pin+1)));
• /* Read the data */
• tmp_val = in_be32(&par_io[port].cpdat);
• *data = (tmp_val >> (NUM_OF_PINS - (pin+1))) & 0x1;

+}

+void qe_write_iopin(u8 port, u8 pin, int data) +{

• u32 pin_1bit_mask;
• u32 tmp_val;
• ccsr_gur_t *gur = (void *)(CFG_MPC85xx_GUTS_ADDR);
• par_io_t *par_io = (par_io_t *) &(gur->qe_par_io);
• /* Calculate pin location for 1bit mask */
• pin_1bit_mask = (u32)(1 << (NUM_OF_PINS - (pin+1)));
• /* Write the data */
• tmp_val = in_be32(&par_io[port].cpdat);
• if (data)

• out_be32(&par_io[port].cpdat, pin_1bit_mask | tmp_val);
  

• else

• out_be32(&par_io[port].cpdat, ~pin_1bit_mask & tmp_val);
  

+}

#endif /* CONFIG_QE */ --- a/cpu/mpc83xx/cpu_init.c 2008-03-28 01:49:12.000000000 +0200 +++ b/cpu/mpc83xx/cpu_init.c 2008-03-30 15:45:53.227362000 +0300 @@ -28,15 +28,13 @@ DECLARE_GLOBAL_DATA_PTR;

#ifdef CONFIG_QE extern qe_iop_conf_t qe_iop_conf_tab[]; -extern void qe_config_iopin(u8 port, u8 pin, int dir,

• 	 int open_drain, int assign);
  

extern void qe_init(uint qe_base); extern void qe_reset(void);

static void config_qe_ioports(void) { u8 port, pin;

• int dir, open_drain, assign;

• int dir, open_drain, assign, data; int i;

  for (i = 0; qe_iop_conf_tab[i].assign != QE_IOP_TAB_END; i++) {

@@ -45,7 +43,8 @@ static void config_qe_ioports(void) dir = qe_iop_conf_tab[i].dir; open_drain = qe_iop_conf_tab[i].open_drain; assign = qe_iop_conf_tab[i].assign;

• qe_config_iopin(port, pin, dir, open_drain, assign);
  

• data		= qe_iop_conf_tab[i].data;
  

• qe_config_iopin(port, pin, dir, open_drain, assign,
  

data); } } #endif --- a/cpu/mpc83xx/qe_io.c 2008-03-28 01:49:12.000000000 +0200 +++ b/cpu/mpc83xx/qe_io.c 2008-03-30 15:53:43.620970000 +0300 @@ -26,15 +26,30 @@ #include "asm/immap_83xx.h"

#define NUM_OF_PINS 32 -void qe_config_iopin(u8 port, u8 pin, int dir, int open_drain, int assign) +void qe_config_iopin(u8 port, u8 pin, int dir, int open_drain, int assign,

• 			 int data)
  

{ u32 pin_2bit_mask; u32 pin_2bit_dir; u32 pin_2bit_assign; u32 pin_1bit_mask; u32 tmp_val;

• volatile immap_t *im = (volatile immap_t *)CFG_IMMR;
• volatile qepio83xx_t *par_io = (volatile qepio83xx_t

*)&im->qepio;

• immap_t *im = (immap_t *)CFG_IMMR;
• qepio83xx_t *par_io = (qepio83xx_t *)&im->qepio;

alignment

• /* Calculate pin location for 1bit mask */
• pin_1bit_mask = (u32)(1 << (NUM_OF_PINS - (pin+1)));

cast necessary? pin + 1 doesn't need parens. also, spaces around the +.

• /* Setup the data */
• if ((data != -1) && /* Don't leave unchanged */

• (assign == 0) && 	/* GPIO */
  

• (dir & 1)) { 		/* Has output */
  

alignment

• tmp_val = in_be32(&par_io->ioport[port].pdat);
  

• if (data)
  

• 	out_be32(&par_io->ioport[port].pdat,
  

pin_1bit_mask | tmp_val);

• else
  

• 	out_be32(&par_io->ioport[port].pdat,
  

~pin_1bit_mask & tmp_val);

• }

how about something like:

tmp_val = in_be32(..);

if (data) tmp_val |= pin_1bit_mask; else tmp_val &= ~pin_1bit_mask;

out_be32(.., tmp_val);

so it's nice and easy to read.

/* Caculate pin location and 2bit mask and dir */ pin_2bit_mask = (u32)(0x3 << (NUM_OF_PINS-(pin%(NUM_OF_PINS/2)+1)*2)); @@ -53,9 +68,6 @@ void qe_config_iopin(u8 port, u8 pin, in out_be32(&par_io->ioport[port].dir1, pin_2bit_dir | tmp_val); }

• /* Calculate pin location for 1bit mask */
• pin_1bit_mask = (u32)(1 << (NUM_OF_PINS - (pin+1)));
• /* Setup the open drain */ tmp_val = in_be32(&par_io->ioport[port].podr); if (open_drain) {

@@ -80,3 +92,38 @@ void qe_config_iopin(u8 port, u8 pin, in out_be32(&par_io->ioport[port].ppar1, pin_2bit_assign | tmp_val); } }

+void qe_read_iopin(u8 port, u8 pin, int *data) +{

• u32 pin_1bit_mask;
• u32 tmp_val;
• immap_t *im = (immap_t *)CFG_IMMR;
• qepio83xx_t *par_io = (qepio83xx_t *)&im->qepio;

alignment

• /* Calculate pin location for 1bit mask */
• pin_1bit_mask = (u32)(1 << (NUM_OF_PINS - (pin+1)));

cast necessary? pin + 1 doesn't need parens. also, spaces around the +.

• /* Read the data */
• tmp_val = in_be32(&par_io->ioport[port].pdat);
• *data = (tmp_val >> (NUM_OF_PINS - (pin+1))) & 0x1;

pin + 1 doesn't need parens. also, spaces around the +.

+}

+void qe_write_iopin(u8 port, u8 pin, int data) +{

• u32 pin_1bit_mask;
• u32 tmp_val;
• immap_t *im = (immap_t *)CFG_IMMR;
• qepio83xx_t *par_io = (qepio83xx_t *)&im->qepio;

alignment

• /* Calculate pin location for 1bit mask */
• pin_1bit_mask = (u32)(1 << (NUM_OF_PINS - (pin+1)));

cast necessary? pin + 1 doesn't need parens. also, spaces around the +.

• /* Setup the data */
• tmp_val = in_be32(&par_io->ioport[port].pdat);
• if (data) {

• out_be32(&par_io->ioport[port].pdat, pin_1bit_mask |
  

tmp_val);

• } else {

• out_be32(&par_io->ioport[port].pdat, ~pin_1bit_mask &
  

tmp_val);

• }

+}

how about something like:

if (data) tmp_val |= pin_1bit_mask; else tmp_val &= ~pin_1bit_mask; out_be32(.., tmp_val);

I realize you may be duping the codingStyle of the existing stuff, but it doesn't mean that we should be adding less-than-readable code.

Also, and I've brought this up before, I don't see the diff in the 83xx vs. 85xx code, which is primarily evidenced by the repetition of my comments above. Can you explain again why they're not placed somewhere to be in common?

Kim