[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