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

Prafulla Wadaskar prafulla at marvell.com
Wed Mar 30 20:54:16 CEST 2011



> -----Original Message-----
> From: Lei Wen [mailto:adrian.wenl at gmail.com]
> Sent: Wednesday, March 30, 2011 7:42 PM
> To: Prafulla Wadaskar
> Cc: Lei Wen; Heiko Schocher; Wolfgang Denk; u-boot at lists.denx.de; Marek
> Vasut; Ashish Karkare; Prabhanjan Sarnaik; Yu Tang
> Subject: Re: [PATCH V5 3/6] mv_i2c: use structure to replace the
> direclty define
> 
> 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.

That was just a suggestion, it up to you.

> >
> >> +
> >> +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?

That should not make a difference, but if we don't have h/w to test I respect your comments.

Regards..
Prafulla . .

> 
> Best regards,
> Lei


More information about the U-Boot mailing list