Hi Prafulla,
On Tue, Mar 29, 2011 at 9:27 PM, Prafulla Wadaskar <prafu...@marvell.com> wrote:
>
>
>> -----Original Message-----
>> From: Lei Wen [mailto:lei...@marvell.com]
>> Sent: Monday, March 28, 2011 12:24 PM
>> To: Heiko Schocher; Prafulla Wadaskar; Wolfgang Denk; u-
>> b...@lists.denx.de; Marek Vasut; Ashish Karkare; Prabhanjan Sarnaik; Yu
>> Tang; adrian.w...@gmail.com
>> Subject: [PATCH V5 3/6] mv_i2c: use structure to replace the direclty
>> define
>>
>> Add i2c_clk_enable in the cpu specific code, since previous platform it,
>> while new platform don't need. In the pantheon and armada100 platform,
>> this function is defined as NULL one.
>>
>> Signed-off-by: Lei Wen <lei...@marvell.com>
>> ---
>> Changelog:
>> V2:
>> NO CHANGE
>>
>> V3:
>> clean code sytle issue
>>
>> V4:
>> V5:
>> NO CHANGE
>>
>> 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 | 131 ++++++++++++++---------
>> -------
>> drivers/i2c/mv_i2c.h | 83 +++++++++++++++++++
>> include/configs/innokom.h | 1 +
>> include/configs/xm250.h | 1 +
>> 7 files changed, 159 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..734148b 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.schwe...@pengutronix.de>
>> *
>> + * (C) Copyright 2011 Marvell Inc.
>> + * Lei Wen <lei...@marvell.com>
>> + *
>> * See file CREDITS for list of people who contributed to this
>> * project.
>> *
>> @@ -34,24 +37,8 @@
>> #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
>> @@ -59,20 +46,6 @@
>> #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
>> -
>> /* All transfers are described by this data structure */
>> struct i2c_msg {
>> u8 condition;
>> @@ -81,27 +54,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;
>> +};
>
> (Optional to implement)
> It is better if you can push register structure definition to the SoC
> specific header file, so that if there are new SoC that has different
> register structures that can be addressed cleanly.
For current there is no different register arrage for this structure,
so I think it is
ok to just keep it current state. For the adding register doc
description, I have no
objection.
>
>> +
>> +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 */
>> + andl(~ICR_IUE, &base->icr); /* disable unit */
>> + orl(ICR_UR, &base->icr); /* reset the unit */
>
> Apart from discussion going on for patch - [PATCH V5.1 1/6] io: add and* and
> or* operation api to set and clear bit
>
> The original code looks more readable to me.
>
>> 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 */
>> + andl(~ICR_IUE, &base->icr); /* 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 */
>
> Why don't you do I2C_ICR_INIT | ICR_IUE here and avoid orl below?
>
This part of logic is the same which I brought from the pxa/i2c code.
I cannot make sure if I do this change whether it would make harm to
the original
platform or not... So my suggestion is keep it like it is. Is that ok for you?
Best regards,
Lei
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
