diff mbox

[U-Boot,V3,2/5] mv_i2c: use structure to replace the direclty define

Message ID 1300344306-26608-3-git-send-email-leiwen@marvell.com
State Superseded
Headers show

Commit Message

Lei Wen March 17, 2011, 6:45 a.m. UTC
Signed-off-by: Lei Wen <leiwen@marvell.com>
---
Changelog:
V3:
clean code sytle issue

 arch/arm/cpu/pxa/cpu.c                   |   11 +++
 arch/arm/include/asm/arch-pxa/pxa-regs.h |   56 -------------
 board/innokom/innokom.c                  |    9 +--
 drivers/i2c/mv_i2c.c                     |  133 ++++++++++++++---------------
 drivers/i2c/mv_i2c.h                     |   83 +++++++++++++++++++
 include/configs/innokom.h                |    1 +
 include/configs/xm250.h                  |    1 +
 7 files changed, 161 insertions(+), 133 deletions(-)
 create mode 100644 drivers/i2c/mv_i2c.h

Comments

Prafulla Wadaskar March 22, 2011, 11:17 a.m. UTC | #1
> -----Original Message-----
> From: Lei Wen [mailto:leiwen@marvell.com]
> Sent: Thursday, March 17, 2011 12:15 PM
> To: Heiko Schocher; Wolfgang Denk; Prafulla Wadaskar; u-
> boot@lists.denx.de; Marek Vasut; Ashish Karkare; Prabhanjan Sarnaik;
> adrian.wenl@gmail.com
> Subject: [PATCH V3 2/5] mv_i2c: use structure to replace the direclty
> define
> 
> Signed-off-by: Lei Wen <leiwen@marvell.com>
> ---
> Changelog:
> V3:
> clean code sytle issue
> 
>  arch/arm/cpu/pxa/cpu.c                   |   11 +++
>  arch/arm/include/asm/arch-pxa/pxa-regs.h |   56 -------------
>  board/innokom/innokom.c                  |    9 +--
>  drivers/i2c/mv_i2c.c                     |  133 ++++++++++++++---------
> ------
>  drivers/i2c/mv_i2c.h                     |   83 +++++++++++++++++++
>  include/configs/innokom.h                |    1 +
>  include/configs/xm250.h                  |    1 +
>  7 files changed, 161 insertions(+), 133 deletions(-)
>  create mode 100644 drivers/i2c/mv_i2c.h
> 
...snip...
> diff --git a/drivers/i2c/mv_i2c.c b/drivers/i2c/mv_i2c.c
> index 09756a4..152ea43 100644
> --- a/drivers/i2c/mv_i2c.c
> +++ b/drivers/i2c/mv_i2c.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.
>   *
> @@ -34,44 +37,16 @@
>  #include <asm/io.h>
> 
>  #ifdef CONFIG_HARD_I2C
> -
> -/*
> - *	- CONFIG_SYS_I2C_SPEED
> - *	- I2C_PXA_SLAVE_ADDR
> - */
> -
> -#include <asm/arch/hardware.h>
> -#include <asm/arch/pxa-regs.h>
>  #include <i2c.h>
> -
> -#if (CONFIG_SYS_I2C_SPEED == 400000)
> -#define I2C_ICR_INIT	(ICR_FM | ICR_BEIE | ICR_IRFIE | ICR_ITEIE |
> ICR_GCD \
> -			| ICR_SCLE)
> -#else
> -#define I2C_ICR_INIT	(ICR_BEIE | ICR_IRFIE | ICR_ITEIE | ICR_GCD |
> ICR_SCLE)
> -#endif
> -
> -#define I2C_ISR_INIT		0x7FF
> +#include "mv_i2c.h"
> 
>  #ifdef DEBUG_I2C
>  #define PRINTD(x) printf x
>  #else
>  #define PRINTD(x)
>  #endif
> -
> -/* Shall the current transfer have a start/stop condition? */
> -#define I2C_COND_NORMAL		0
> -#define I2C_COND_START		1
> -#define I2C_COND_STOP		2
> -
> -/* Shall the current transfer be ack/nacked or being waited for it? */
> -#define I2C_ACKNAK_WAITACK	1
> -#define I2C_ACKNAK_SENDACK	2
> -#define I2C_ACKNAK_SENDNAK	4
> -
> -/* Specify who shall transfer the data (master or slave) */
> -#define I2C_READ		0
> -#define I2C_WRITE		1
> +#define PXAI2C_AND(reg, val)	writel(readl(reg) & val, reg)
> +#define PXAI2C_OR(reg, val)	writel(readl(reg) | val, reg)

With reference to the file name MVI2C_ is better macro name to be used here.
Applicable for s/pxa/mv/g in the entire file too
 
> 
>  /* All transfers are described by this data structure */
>  struct i2c_msg {
> @@ -81,27 +56,37 @@ 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;
> +};

Please document the long names of these register with their offsets.

> +
> +static struct pxa_i2c *base = (struct pxa_i2c *)CONFIG_PXA_I2C_REG;
> +
...snip...
>  		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);

What are benefits of those macros?
To me this looks ugly and looses readability.

...snip...
> diff --git a/drivers/i2c/mv_i2c.h b/drivers/i2c/mv_i2c.h
> new file mode 100644
> index 0000000..41af0d9
> --- /dev/null
> +++ b/drivers/i2c/mv_i2c.h
> @@ -0,0 +1,83 @@
> +/*
> + * (C) Copyright 2011
> + * Marvell Inc, <www.marvell.com>
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#ifndef _MV_I2C_H_
> +#define _MV_I2C_H_
> +extern void i2c_clk_enable(void);
> +
> +/* Shall the current transfer have a start/stop condition? */
> +#define I2C_COND_NORMAL		0
> +#define I2C_COND_START		1
> +#define I2C_COND_STOP		2

How about prefixing the macros in this file as MVI2C instead of I2C?

...snip...
> +#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

Please define i2c base address in armada100.h and in pantheon.h and use it in driver, avoid this definition here.

Regards..
Prafulla . .
Lei Wen March 22, 2011, 12:34 p.m. UTC | #2
Hi Prafulla,

On Tue, Mar 22, 2011 at 7:17 PM, Prafulla Wadaskar <prafulla@marvell.com> wrote:
>
>
>> -----Original Message-----
>> From: Lei Wen [mailto:leiwen@marvell.com]
>> Sent: Thursday, March 17, 2011 12:15 PM
>> To: Heiko Schocher; Wolfgang Denk; Prafulla Wadaskar; u-
>> boot@lists.denx.de; Marek Vasut; Ashish Karkare; Prabhanjan Sarnaik;
>> adrian.wenl@gmail.com
>> Subject: [PATCH V3 2/5] mv_i2c: use structure to replace the direclty
>> define
>>
>> Signed-off-by: Lei Wen <leiwen@marvell.com>
>> ---
>> Changelog:
>> V3:
>> clean code sytle issue
>>
>>  arch/arm/cpu/pxa/cpu.c                   |   11 +++
>>  arch/arm/include/asm/arch-pxa/pxa-regs.h |   56 -------------
>>  board/innokom/innokom.c                  |    9 +--
>>  drivers/i2c/mv_i2c.c                     |  133 ++++++++++++++---------
>> ------
>>  drivers/i2c/mv_i2c.h                     |   83 +++++++++++++++++++
>>  include/configs/innokom.h                |    1 +
>>  include/configs/xm250.h                  |    1 +
>>  7 files changed, 161 insertions(+), 133 deletions(-)
>>  create mode 100644 drivers/i2c/mv_i2c.h
>>
> ...snip...
>> diff --git a/drivers/i2c/mv_i2c.c b/drivers/i2c/mv_i2c.c
>> index 09756a4..152ea43 100644
>> --- a/drivers/i2c/mv_i2c.c
>> +++ b/drivers/i2c/mv_i2c.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.
>>   *
>> @@ -34,44 +37,16 @@
>>  #include <asm/io.h>
>>
>>  #ifdef CONFIG_HARD_I2C
>> -
>> -/*
>> - *   - CONFIG_SYS_I2C_SPEED
>> - *   - I2C_PXA_SLAVE_ADDR
>> - */
>> -
>> -#include <asm/arch/hardware.h>
>> -#include <asm/arch/pxa-regs.h>
>>  #include <i2c.h>
>> -
>> -#if (CONFIG_SYS_I2C_SPEED == 400000)
>> -#define I2C_ICR_INIT (ICR_FM | ICR_BEIE | ICR_IRFIE | ICR_ITEIE |
>> ICR_GCD \
>> -                     | ICR_SCLE)
>> -#else
>> -#define I2C_ICR_INIT (ICR_BEIE | ICR_IRFIE | ICR_ITEIE | ICR_GCD |
>> ICR_SCLE)
>> -#endif
>> -
>> -#define I2C_ISR_INIT         0x7FF
>> +#include "mv_i2c.h"
>>
>>  #ifdef DEBUG_I2C
>>  #define PRINTD(x) printf x
>>  #else
>>  #define PRINTD(x)
>>  #endif
>> -
>> -/* Shall the current transfer have a start/stop condition? */
>> -#define I2C_COND_NORMAL              0
>> -#define I2C_COND_START               1
>> -#define I2C_COND_STOP                2
>> -
>> -/* Shall the current transfer be ack/nacked or being waited for it? */
>> -#define I2C_ACKNAK_WAITACK   1
>> -#define I2C_ACKNAK_SENDACK   2
>> -#define I2C_ACKNAK_SENDNAK   4
>> -
>> -/* Specify who shall transfer the data (master or slave) */
>> -#define I2C_READ             0
>> -#define I2C_WRITE            1
>> +#define PXAI2C_AND(reg, val) writel(readl(reg) & val, reg)
>> +#define PXAI2C_OR(reg, val)  writel(readl(reg) | val, reg)
>
> With reference to the file name MVI2C_ is better macro name to be used here.
> Applicable for s/pxa/mv/g in the entire file too
>
>>
>>  /* All transfers are described by this data structure */
>>  struct i2c_msg {
>> @@ -81,27 +56,37 @@ 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;
>> +};
>
> Please document the long names of these register with their offsets.

Good point, I would do it in next post.

>
>> +
>> +static struct pxa_i2c *base = (struct pxa_i2c *)CONFIG_PXA_I2C_REG;
>> +
> ...snip...
>>               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);
>
> What are benefits of those macros?
> To me this looks ugly and looses readability.

This intend for short the original too long code, but I don't reject to return
to the readl, write one.

>
> ...snip...
>> diff --git a/drivers/i2c/mv_i2c.h b/drivers/i2c/mv_i2c.h
>> new file mode 100644
>> index 0000000..41af0d9
>> --- /dev/null
>> +++ b/drivers/i2c/mv_i2c.h
>> @@ -0,0 +1,83 @@
>> +/*
>> + * (C) Copyright 2011
>> + * Marvell Inc, <www.marvell.com>
>> + *
>> + * See file CREDITS for list of people who contributed to this
>> + * project.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 of
>> + * the License, or (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>> + * MA 02111-1307 USA
>> + */
>> +
>> +#ifndef _MV_I2C_H_
>> +#define _MV_I2C_H_
>> +extern void i2c_clk_enable(void);
>> +
>> +/* Shall the current transfer have a start/stop condition? */
>> +#define I2C_COND_NORMAL              0
>> +#define I2C_COND_START               1
>> +#define I2C_COND_STOP                2
>
> How about prefixing the macros in this file as MVI2C instead of I2C?

I am ok with this change proposal.

>
> ...snip...
>> +#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
>
> Please define i2c base address in armada100.h and in pantheon.h and use it in driver, avoid this definition here.

This is not armada100 nor pantheon board, but innokom board...
This driver intend to be used by all Marvell pxa series.
So for innokom board where should it define itself?

Best regards,
Lei
Wolfgang Denk March 22, 2011, 3:16 p.m. UTC | #3
Dear Lei Wen,

In message <AANLkTikMYpVVJDRnGRVZ3XZsYTn_yprpfMUUp_3DdHbJ@mail.gmail.com> you wrote:
> 
...
> >> -                     writel(readl(ICR) & ~ICR_ACKNA> K, ICR);
> >> -             writel(readl(ICR) & ~ICR_ALDIE, ICR);
> >> -             writel(readl(ICR) | ICR_TB, ICR);
> >> +                     PXAI2C_AND(&base->icr, ~ICR_AC> KNAK);
> >> +             PXAI2C_AND(&base->icr, ~ICR_ALDIE);
> >> +             PXAI2C_OR(&base->icr, ICR_TB);
> >
> > What are benefits of those macros?
> > To me this looks ugly and looses readability.
>
> This intend for short the original too long code, but I don't reject to ret> urn
> to the readl, write one.

Please either use plain writel() / readl() accessors, or,
alternatively, implement standard accessors that could be used by
everybody else for the same purpose, too.

See for example the misc clrbits*(), setbits*() or clrsetbits*()
macros as dfined for example in "arch/powerpc/include/asm/io.h"

Best regards,

Wolfgang Denk
Lei Wen March 23, 2011, 8:48 a.m. UTC | #4
Hi Wolfgang,

On Tue, Mar 22, 2011 at 11:16 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Lei Wen,
>
> In message <AANLkTikMYpVVJDRnGRVZ3XZsYTn_yprpfMUUp_3DdHbJ@mail.gmail.com> you wrote:
>>
> ...
>> >> -                     writel(readl(ICR) & ~ICR_ACKNA> K, ICR);
>> >> -             writel(readl(ICR) & ~ICR_ALDIE, ICR);
>> >> -             writel(readl(ICR) | ICR_TB, ICR);
>> >> +                     PXAI2C_AND(&base->icr, ~ICR_AC> KNAK);
>> >> +             PXAI2C_AND(&base->icr, ~ICR_ALDIE);
>> >> +             PXAI2C_OR(&base->icr, ICR_TB);
>> >
>> > What are benefits of those macros?
>> > To me this looks ugly and looses readability.
>>
>> This intend for short the original too long code, but I don't reject to ret> urn
>> to the readl, write one.
>
> Please either use plain writel() / readl() accessors, or,
> alternatively, implement standard accessors that could be used by
> everybody else for the same purpose, too.
>
> See for example the misc clrbits*(), setbits*() or clrsetbits*()
> macros as dfined for example in "arch/powerpc/include/asm/io.h"

Yep, that is what I want. :)
I would post next patch set include this suggestion.

Thanks,
Lei
diff mbox

Patch

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/mv_i2c.c b/drivers/i2c/mv_i2c.c
index 09756a4..152ea43 100644
--- a/drivers/i2c/mv_i2c.c
+++ b/drivers/i2c/mv_i2c.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.
  *
@@ -34,44 +37,16 @@ 
 #include <asm/io.h>
 
 #ifdef CONFIG_HARD_I2C
-
-/*
- *	- CONFIG_SYS_I2C_SPEED
- *	- I2C_PXA_SLAVE_ADDR
- */
-
-#include <asm/arch/hardware.h>
-#include <asm/arch/pxa-regs.h>
 #include <i2c.h>
-
-#if (CONFIG_SYS_I2C_SPEED == 400000)
-#define I2C_ICR_INIT	(ICR_FM | ICR_BEIE | ICR_IRFIE | ICR_ITEIE | ICR_GCD \
-			| ICR_SCLE)
-#else
-#define I2C_ICR_INIT	(ICR_BEIE | ICR_IRFIE | ICR_ITEIE | ICR_GCD | ICR_SCLE)
-#endif
-
-#define I2C_ISR_INIT		0x7FF
+#include "mv_i2c.h"
 
 #ifdef DEBUG_I2C
 #define PRINTD(x) printf x
 #else
 #define PRINTD(x)
 #endif
-
-/* Shall the current transfer have a start/stop condition? */
-#define I2C_COND_NORMAL		0
-#define I2C_COND_START		1
-#define I2C_COND_STOP		2
-
-/* Shall the current transfer be ack/nacked or being waited for it? */
-#define I2C_ACKNAK_WAITACK	1
-#define I2C_ACKNAK_SENDACK	2
-#define I2C_ACKNAK_SENDNAK	4
-
-/* Specify who shall transfer the data (master or slave) */
-#define I2C_READ		0
-#define I2C_WRITE		1
+#define PXAI2C_AND(reg, val)	writel(readl(reg) & val, reg)
+#define PXAI2C_OR(reg, val)	writel(readl(reg) | val, reg)
 
 /* All transfers are described by this data structure */
 struct i2c_msg {
@@ -81,27 +56,37 @@  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;
+
 /*
  * i2c_pxa_reset: - reset the host controller
  *
  */
 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(CONFIG_SYS_I2C_SLAVE, &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);
 }
 
@@ -114,13 +99,15 @@  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)) {
+	do {
+		isr = readl(&base->isr);
 		udelay(10);
 		if (timeout-- < 0)
 			return 0;
-	}
+	} while (((isr & set_mask) != set_mask)
+		|| ((isr & cleared_mask) != 0));
 
 	return 1;
 }
@@ -153,26 +140,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)
@@ -187,28 +174,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;
 	default:
 		goto transfer_error_illegal_param;
@@ -252,10 +238,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...                                        */
+	icr = readl(&base->icr);
+	PXAI2C_AND(&base->icr, ~(ICR_SCLE | ICR_IUE));
+
 	i2c_init_board();
+
+	writel(icr, &base->icr);
 #endif
 }
 
diff --git a/drivers/i2c/mv_i2c.h b/drivers/i2c/mv_i2c.h
new file mode 100644
index 0000000..41af0d9
--- /dev/null
+++ b/drivers/i2c/mv_i2c.h
@@ -0,0 +1,83 @@ 
+/*
+ * (C) Copyright 2011
+ * Marvell Inc, <www.marvell.com>
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#ifndef _MV_I2C_H_
+#define _MV_I2C_H_
+extern void i2c_clk_enable(void);
+
+/* Shall the current transfer have a start/stop condition? */
+#define I2C_COND_NORMAL		0
+#define I2C_COND_START		1
+#define I2C_COND_STOP		2
+
+/* Shall the current transfer be ack/nacked or being waited for it? */
+#define I2C_ACKNAK_WAITACK	1
+#define I2C_ACKNAK_SENDACK	2
+#define I2C_ACKNAK_SENDNAK	4
+
+/* Specify who shall transfer the data (master or slave) */
+#define I2C_READ		0
+#define I2C_WRITE		1
+
+#if (CONFIG_SYS_I2C_SPEED == 400000)
+#define I2C_ICR_INIT	(ICR_FM | ICR_BEIE | ICR_IRFIE | ICR_ITEIE | ICR_GCD \
+		| ICR_SCLE)
+#else
+#define I2C_ICR_INIT	(ICR_BEIE | ICR_IRFIE | ICR_ITEIE | ICR_GCD | ICR_SCLE)
+#endif
+
+#define I2C_ISR_INIT		0x7FF
+/* ----- 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 */
+
+#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
 #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