[U-Boot] [PATCH V5 3/6] mv_i2c: use structure to replace the direclty define

[Lei Wen]

Hi Prafulla,

On Tue, Mar 29, 2011 at 9:27 PM, Prafulla Wadaskar <prafulla at marvell.com> wrote:
>
>
>> -----Original Message-----
>> From: Lei Wen [mailto:leiwen at marvell.com]
>> Sent: Monday, March 28, 2011 12:24 PM
>> To: Heiko Schocher; Prafulla Wadaskar; Wolfgang Denk; u-
>> boot at lists.denx.de; Marek Vasut; Ashish Karkare; Prabhanjan Sarnaik; Yu
>> Tang; adrian.wenl at 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 <leiwen at 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.schwebel at pengutronix.de>
>>   *
>> + * (C) Copyright 2011 Marvell Inc.
>> + * Lei Wen <leiwen at 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