Re: [U-Boot] [PATCH v2 2/5] mvi2c: use structure to replace the direclty define

Hello Lei,

Lei Wen wrote:

Signed-off-by: Lei Wen leiwen@marvell.com

arch/arm/cpu/pxa/cpu.c | 11 +++ arch/arm/include/asm/arch-pxa/pxa-regs.h | 56 ------------ board/innokom/innokom.c | 9 +-- drivers/i2c/mvi2c.c | 139 +++++++++++++++++++++--------- include/configs/innokom.h | 1 + include/configs/xm250.h | 1 + 6 files changed, 111 insertions(+), 106 deletions(-)

diff --git a/arch/arm/cpu/pxa/cpu.c b/arch/arm/cpu/pxa/cpu.c index 7d49cbb..24b59e7 100644 --- a/arch/arm/cpu/pxa/cpu.c +++ b/arch/arm/cpu/pxa/cpu.c @@ -318,3 +318,14 @@ int arch_cpu_init(void) pxa_clock_setup(); return 0; }

+void i2c_clk_enable(void) +{ +#ifdef CONFIG_CPU_MONAHANS

• /* | CKENB_1_PWM1 | CKENB_0_PWM0); */
• writel(readl(CKENB) | (CKENB_4_I2C), CKENB);

+#else /* CONFIG_CPU_MONAHANS */

• /* set the global I2C clock on */
• writel(readl(CKEN) | CKEN14_I2C, CKEN);

+#endif +} diff --git a/arch/arm/include/asm/arch-pxa/pxa-regs.h b/arch/arm/include/asm/arch-pxa/pxa-regs.h index 65a387f..109fdc0 100644 --- a/arch/arm/include/asm/arch-pxa/pxa-regs.h +++ b/arch/arm/include/asm/arch-pxa/pxa-regs.h @@ -456,62 +456,6 @@ typedef void (*ExcpHndlr) (void) ; IrSR_XMITIR_IR_MODE)

/*

• • I2C registers
• */

-#define IBMR 0x40301680 /* I2C Bus Monitor Register - IBMR */ -#define IDBR 0x40301688 /* I2C Data Buffer Register - IDBR */ -#define ICR 0x40301690 /* I2C Control Register - ICR */ -#define ISR 0x40301698 /* I2C Status Register - ISR */ -#define ISAR 0x403016A0 /* I2C Slave Address Register - ISAR */

-#ifdef CONFIG_CPU_MONAHANS -#define PWRIBMR 0x40f500C0 /* Power I2C Bus Monitor Register-IBMR */ -#define PWRIDBR 0x40f500C4 /* Power I2C Data Buffer Register-IDBR */ -#define PWRICR 0x40f500C8 /* Power I2C Control Register - ICR */ -#define PWRISR 0x40f500CC /* Power I2C Status Register - ISR */ -#define PWRISAR 0x40f500D0 /* Power I2C Slave Address Register-ISAR */ -#else -#define PWRIBMR 0x40f00180 /* Power I2C Bus Monitor Register-IBMR */ -#define PWRIDBR 0x40f00188 /* Power I2C Data Buffer Register-IDBR */ -#define PWRICR 0x40f00190 /* Power I2C Control Register - ICR */ -#define PWRISR 0x40f00198 /* Power I2C Status Register - ISR */ -#define PWRISAR 0x40f001A0 /* Power I2C Slave Address Register-ISAR */ -#endif

-/* ----- Control register bits ---------------------------------------- */

-#define ICR_START 0x1 /* start bit */ -#define ICR_STOP 0x2 /* stop bit */ -#define ICR_ACKNAK 0x4 /* send ACK(0) or NAK(1) */ -#define ICR_TB 0x8 /* transfer byte bit */ -#define ICR_MA 0x10 /* master abort */ -#define ICR_SCLE 0x20 /* master clock enable, mona SCLEA */ -#define ICR_IUE 0x40 /* unit enable */ -#define ICR_GCD 0x80 /* general call disable */ -#define ICR_ITEIE 0x100 /* enable tx interrupts */ -#define ICR_IRFIE 0x200 /* enable rx interrupts, mona: DRFIE */ -#define ICR_BEIE 0x400 /* enable bus error ints */ -#define ICR_SSDIE 0x800 /* slave STOP detected int enable */ -#define ICR_ALDIE 0x1000 /* enable arbitration interrupt */ -#define ICR_SADIE 0x2000 /* slave address detected int enable */ -#define ICR_UR 0x4000 /* unit reset */ -#define ICR_FM 0x8000 /* Fast Mode */

-/* ----- Status register bits ----------------------------------------- */

-#define ISR_RWM 0x1 /* read/write mode */ -#define ISR_ACKNAK 0x2 /* ack/nak status */ -#define ISR_UB 0x4 /* unit busy */ -#define ISR_IBB 0x8 /* bus busy */ -#define ISR_SSD 0x10 /* slave stop detected */ -#define ISR_ALD 0x20 /* arbitration loss detected */ -#define ISR_ITE 0x40 /* tx buffer empty */ -#define ISR_IRF 0x80 /* rx buffer full */ -#define ISR_GCAD 0x100 /* general call address detected */ -#define ISR_SAD 0x200 /* slave address detected */ -#define ISR_BED 0x400 /* bus error no ACK/NAK */

-/*

• Serial Audio Controller

*/ /* FIXME the audio defines collide w/ the SA1111 defines. I don't like these diff --git a/board/innokom/innokom.c b/board/innokom/innokom.c index e658c35..22de7e3 100644 --- a/board/innokom/innokom.c +++ b/board/innokom/innokom.c @@ -45,12 +45,7 @@ DECLARE_GLOBAL_DATA_PTR; */ int i2c_init_board(void) {

• int i, icr;
• /* disable I2C controller first, otherwhise it thinks we want to */
• /* talk to the slave port... */
• icr = readl(ICR);
• writel(readl(ICR) & ~(ICR_SCLE | ICR_IUE), ICR);

• int i;

  /* set gpio pin low _before_ we change direction to output */ writel(GPIO_bit(70), GPCR(70));

@@ -63,8 +58,6 @@ int i2c_init_board(void) udelay(10); }

• writel(icr, ICR);
• return 0;

}

diff --git a/drivers/i2c/mvi2c.c b/drivers/i2c/mvi2c.c index 7aa49ae..0e37417 100644 --- a/drivers/i2c/mvi2c.c +++ b/drivers/i2c/mvi2c.c @@ -8,6 +8,9 @@

• (C) Copyright 2003 Pengutronix e.K.
• Robert Schwebel r.schwebel@pengutronix.de

• • (C) Copyright 2011 Marvell Inc.
• • Lei Wen leiwen@marvell.com
• • See file CREDITS for list of people who contributed to this
  • project.

@@ -30,8 +33,6 @@

• Murray.Jensen@cmst.csiro.au, 27-Jan-01.

*/

-/* FIXME: this file is PXA255 specific! What about other XScales? */

Ah, you remove this comment here, Ok!

#include <common.h> #include <asm/io.h>

@@ -42,9 +43,41 @@

• • I2C_PXA_SLAVE_ADDR

*/

-#include <asm/arch/hardware.h> -#include <asm/arch/pxa-regs.h> #include <i2c.h> +extern void i2c_clk_enable(void);

Hmm... as you define this function in arch/arm/cpu/pxa/cpu.c there should be somewhere an appropriate header file, where it fits in, so we can avoid this extern.

+/* ----- Control register bits ---------------------------------------- */

+#define ICR_START 0x1 /* start bit */ +#define ICR_STOP 0x2 /* stop bit */ +#define ICR_ACKNAK 0x4 /* send ACK(0) or NAK(1) */ +#define ICR_TB 0x8 /* transfer byte bit */ +#define ICR_MA 0x10 /* master abort */ +#define ICR_SCLE 0x20 /* master clock enable, mona SCLEA */ +#define ICR_IUE 0x40 /* unit enable */ +#define ICR_GCD 0x80 /* general call disable */ +#define ICR_ITEIE 0x100 /* enable tx interrupts */ +#define ICR_IRFIE 0x200 /* enable rx interrupts, mona: DRFIE */ +#define ICR_BEIE 0x400 /* enable bus error ints */ +#define ICR_SSDIE 0x800 /* slave STOP detected int enable */ +#define ICR_ALDIE 0x1000 /* enable arbitration interrupt */ +#define ICR_SADIE 0x2000 /* slave address detected int enable */ +#define ICR_UR 0x4000 /* unit reset */ +#define ICR_FM 0x8000 /* Fast Mode */

+/* ----- Status register bits ----------------------------------------- */

+#define ISR_RWM 0x1 /* read/write mode */ +#define ISR_ACKNAK 0x2 /* ack/nak status */ +#define ISR_UB 0x4 /* unit busy */ +#define ISR_IBB 0x8 /* bus busy */ +#define ISR_SSD 0x10 /* slave stop detected */ +#define ISR_ALD 0x20 /* arbitration loss detected */ +#define ISR_ITE 0x40 /* tx buffer empty */ +#define ISR_IRF 0x80 /* rx buffer full */ +#define ISR_GCAD 0x100 /* general call address detected */ +#define ISR_SAD 0x200 /* slave address detected */ +#define ISR_BED 0x400 /* bus error no ACK/NAK */

/*#define DEBUG_I2C 1 /###* activate local debugging output */ #define I2C_PXA_SLAVE_ADDR 0x1 /* slave pxa unit address */ @@ -86,6 +119,21 @@ struct i2c_msg { u8 data; };

+struct pxa_i2c {

• u32 ibmr;
• u32 pad0;
• u32 idbr;
• u32 pad1;
• u32 icr;
• u32 pad2;
• u32 isr;
• u32 pad3;
• u32 isar;

+};

+static struct pxa_i2c *base = (struct pxa_i2c *)CONFIG_PXA_I2C_REG; +#define PXAI2C_AND(reg, val) writel(readl(reg) & val, reg) +#define PXAI2C_OR(reg, val) writel(readl(reg) | val, reg)

/**

• i2c_pxa_reset: - reset the host controller

@@ -94,21 +142,17 @@ struct i2c_msg {

static void i2c_reset( void ) {

• writel(readl(ICR) & ~ICR_IUE, ICR); /* disable unit */
• writel(readl(ICR) | ICR_UR, ICR); /* reset the unit */

• PXAI2C_AND(&base->icr, ~ICR_IUE); /* disable unit */
• PXAI2C_OR(&base->icr, ICR_UR); /* reset the unit */ udelay(100);

• writel(readl(ICR) & ~ICR_IUE, ICR); /* disable unit */

-#ifdef CONFIG_CPU_MONAHANS

• /* | CKENB_1_PWM1 | CKENB_0_PWM0); */
• writel(readl(CKENB) | (CKENB_4_I2C), CKENB);

-#else /* CONFIG_CPU_MONAHANS */

• /* set the global I2C clock on */
• writel(readl(CKEN) | CKEN14_I2C, CKEN);

-#endif

• writel(I2C_PXA_SLAVE_ADDR, ISAR); /* set our slave address */
• writel(I2C_ICR_INIT, ICR); /* set control reg values */
• writel(I2C_ISR_INIT, ISR); /* set clear interrupt bits */
• writel(readl(ICR) | ICR_IUE, ICR); /* enable unit */

• PXAI2C_AND(&base->icr, ~ICR_IUE); /* disable unit */
• i2c_clk_enable();
• writel(I2C_PXA_SLAVE_ADDR, &base->isar);/* set our slave address */
• writel(I2C_ICR_INIT, &base->icr); /* set control reg values */
• writel(I2C_ISR_INIT, &base->isr); /* set clear interrupt bits */
• PXAI2C_OR(&base->icr, ICR_IUE); /* enable unit */ udelay(100);

}

@@ -121,12 +165,14 @@ static void i2c_reset( void ) */ static int i2c_isr_set_cleared( unsigned long set_mask, unsigned long cleared_mask ) {

• int timeout = 10000;

• int timeout = 10000, isr;

• while( ((ISR & set_mask)!=set_mask) || ((ISR & cleared_mask)!=0) ){

• udelay( 10 );
  

• do {

• isr = readl(&base->isr);
  

• udelay(10);
  

  if( timeout-- < 0 ) return 0;

• }

• } while (((isr & set_mask)!=set_mask)

^ Spaces around "!=" Please fix globally, thanks!

•  || ((isr & cleared_mask)!=0));
  
  

  return 1;

} @@ -162,26 +208,26 @@ int i2c_transfer(struct i2c_msg *msg) goto transfer_error_bus_busy;

/* start transmission */

• writel(readl(ICR) & ~ICR_START, ICR);
  

• writel(readl(ICR) & ~ICR_STOP, ICR);
  

• writel(msg->data, IDBR);
  

• PXAI2C_AND(&base->icr, ~ICR_START);
  

• PXAI2C_AND(&base->icr, ~ICR_STOP);
  

• writel(msg->data, &base->idbr);
  

  if (msg->condition == I2C_COND_START)

• 	writel(readl(ICR) | ICR_START, ICR);
  

• 	PXAI2C_OR(&base->icr, ICR_START);
  

  if (msg->condition == I2C_COND_STOP)

• 	writel(readl(ICR) | ICR_STOP, ICR);
  

• 	PXAI2C_OR(&base->icr, ICR_STOP);
  

  if (msg->acknack == I2C_ACKNAK_SENDNAK)

• 	writel(readl(ICR) | ICR_ACKNAK, ICR);
  

• 	PXAI2C_OR(&base->icr, ICR_ACKNAK);
  

  if (msg->acknack == I2C_ACKNAK_SENDACK)

• 	writel(readl(ICR) & ~ICR_ACKNAK, ICR);
  

• writel(readl(ICR) & ~ICR_ALDIE, ICR);
  

• writel(readl(ICR) | ICR_TB, ICR);
  

• 	PXAI2C_AND(&base->icr, ~ICR_ACKNAK);
  

• PXAI2C_AND(&base->icr, ~ICR_ALDIE);
  

• PXAI2C_OR(&base->icr, ICR_TB);
  
  

  /* transmit register empty? */ if (!i2c_isr_set_cleared(ISR_ITE,0)) goto transfer_error_transmit_timeout;

  /* clear 'transmit empty' state */

• writel(readl(ISR) | ISR_ITE, ISR);
  

• PXAI2C_OR(&base->isr, ISR_ITE);
  
  

  /* wait for ACK from slave */ if (msg->acknack == I2C_ACKNAK_WAITACK)

@@ -196,27 +242,27 @@ int i2c_transfer(struct i2c_msg *msg) goto transfer_error_bus_busy;

/* start receive */

• writel(readl(ICR) & ~ICR_START, ICR);
  

• writel(readl(ICR) & ~ICR_STOP, ICR);
  

• PXAI2C_AND(&base->icr, ~ICR_START);
  

• PXAI2C_AND(&base->icr, ~ICR_STOP);
  

  if (msg->condition == I2C_COND_START)

• 	writel(readl(ICR) | ICR_START, ICR);
  

• 	PXAI2C_OR(&base->icr, ICR_START);
  

  if (msg->condition == I2C_COND_STOP)

• 	writel(readl(ICR) | ICR_STOP, ICR);
  

• 	PXAI2C_OR(&base->icr, ICR_STOP);
  

  if (msg->acknack == I2C_ACKNAK_SENDNAK)

• 	writel(readl(ICR) | ICR_ACKNAK, ICR);
  

• 	PXAI2C_OR(&base->icr, ICR_ACKNAK);
  

  if (msg->acknack == I2C_ACKNAK_SENDACK)

• 	writel(readl(ICR) & ~ICR_ACKNAK, ICR);
  

• writel(readl(ICR) & ~ICR_ALDIE, ICR);
  

• writel(readl(ICR) | ICR_TB, ICR);
  

• 	PXAI2C_AND(&base->icr, ~ICR_ACKNAK);
  

• PXAI2C_AND(&base->icr, ~ICR_ALDIE);
  

• PXAI2C_OR(&base->icr, ICR_TB);
  
  

  /* receive register full? */ if (!i2c_isr_set_cleared(ISR_IRF,0)) goto transfer_error_receive_timeout;

• msg->data = readl(IDBR);
  

• msg->data = readl(&base->idbr);
  
  

  /* clear 'receive empty' state */

• writel(readl(ISR) | ISR_IRF, ISR);
  

• PXAI2C_OR(&base->isr, ISR_IRF);
  
  

  break;

@@ -266,10 +312,19 @@ i2c_transfer_finish: void i2c_init(int speed, int slaveaddr) { #ifdef CONFIG_SYS_I2C_INIT_BOARD

• u32 icr; /* call board specific i2c bus reset routine before accessing the */ /* environment, which might be in a chip on that bus. For details */ /* about this problem see doc/I2C_Edge_Conditions. */
• /* disable I2C controller first, otherwhise it thinks we want to */
• /* talk to the slave port... */

Wrong comment style, please fix

• icr = readl(&base->icr);
• PXAI2C_AND(&base->icr, ~(ICR_SCLE | ICR_IUE));
• i2c_init_board();
• writel(icr, &base->icr);

#endif }

diff --git a/include/configs/innokom.h b/include/configs/innokom.h index 0ea73c9..1ddee03 100644 --- a/include/configs/innokom.h +++ b/include/configs/innokom.h @@ -141,6 +141,7 @@

• I2C bus

*/ #define CONFIG_I2C_MV 1 +#define CONFIG_PXA_I2C_REG 0x40301680

Hmm.. is there no define for this magic value?

#define CONFIG_HARD_I2C 1 #define CONFIG_SYS_I2C_SPEED 50000 #define CONFIG_SYS_I2C_SLAVE 0xfe diff --git a/include/configs/xm250.h b/include/configs/xm250.h index b4b940a..682d1ed 100644 --- a/include/configs/xm250.h +++ b/include/configs/xm250.h @@ -62,6 +62,7 @@

• I2C bus

*/ #define CONFIG_I2C_MV 1 +#define CONFIG_PXA_I2C_REG 0x40301680 #define CONFIG_HARD_I2C 1 #define CONFIG_SYS_I2C_SPEED 50000 #define CONFIG_SYS_I2C_SLAVE 0xfe

Apart from this few comments patch looks fine, thanks!

bye, Heiko