Hi Prafulla,
On Sat, Apr 2, 2011 at 2:29 AM, Prafulla Wadaskar <prafu...@marvell.com> wrote:
>
>
>> -----Original Message-----
>> From: Lei Wen [mailto:lei...@marvell.com]
>> Sent: Thursday, March 31, 2011 2:07 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 V6 2/5] 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:
>> V6:
>> 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
>>
>> 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); */
>
> This comment line looks like part of code, Can be rephrased in better way.
Yep.
>
>> + writel(readl(CKENB) | (CKENB_4_I2C), CKENB);
>> +#else /* CONFIG_CPU_MONAHANS */
>> + /* set the global I2C clock on */
>> + writel(readl(CKEN) | CKEN14_I2C, CKEN);
>> +#endif
>> +}
>
> ...snip...
>
>> @@ -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;
>> +};
>> +
>> +static struct pxa_i2c *base = (struct pxa_i2c *)CONFIG_PXA_I2C_REG;
>
> I think to sync with mc_i2c change at least the macro CONFIG_PXA_I2C_REG
> Need to be renamed as CONFIG_MV_I2C_REG, because the same will be referred by
> other SoC code.
Ok, I would do this change for next post.
>
>> +
>> /*
>> * i2c_pxa_reset: - reset the host controller
>> *
>> */
>
> ...snip...
>> @@ -114,13 +97,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 = 1000, isr;
>
> Is this done purposely? Or reducing timeout value from 10000 to 1000 has some
> meaning?
You may notice original comment above this function is timeout if (no
match within 10 ms).
So the 10ms is 1000*10us, not the 10000*10us. In this patch I correct
this to match its comments.
>
>>
>> - 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 +138,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);
>> + writel(readl(&base->icr) & ~ICR_START, &base->icr);
>> + writel(readl(&base->icr) & ~ICR_STOP, &base->icr);
>> + writel(msg->data, &base->idbr);
>> if (msg->condition == I2C_COND_START)
>> - writel(readl(ICR) | ICR_START, ICR);
>> + writel(readl(&base->icr) | ICR_START, &base->icr);
>> if (msg->condition == I2C_COND_STOP)
>> - writel(readl(ICR) | ICR_STOP, ICR);
>> + writel(readl(&base->icr) | ICR_STOP, &base->icr);
>> if (msg->acknack == I2C_ACKNAK_SENDNAK)
>> - writel(readl(ICR) | ICR_ACKNAK, ICR);
>> + writel(readl(&base->icr) | ICR_ACKNAK, &base->icr);
>> if (msg->acknack == I2C_ACKNAK_SENDACK)
>> - writel(readl(ICR) & ~ICR_ACKNAK, ICR);
>> - writel(readl(ICR) & ~ICR_ALDIE, ICR);
>> - writel(readl(ICR) | ICR_TB, ICR);
>> + writel(readl(&base->icr) & ~ICR_ACKNAK, &base->icr);
>> + writel(readl(&base->icr) & ~ICR_ALDIE, &base->icr);
>> + writel(readl(&base->icr) | ICR_TB, &base->icr);
>>
>> /* 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);
>> + writel(readl(&base->isr) | ISR_ITE, &base->isr);
>>
>> /* wait for ACK from slave */
>> if (msg->acknack == I2C_ACKNAK_WAITACK)
>> @@ -187,28 +172,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);
>> + writel(readl(&base->icr) & ~ICR_START, &base->icr);
>> + writel(readl(&base->icr) & ~ICR_STOP, &base->icr);
>> if (msg->condition == I2C_COND_START)
>> - writel(readl(ICR) | ICR_START, ICR);
>> + writel(readl(&base->icr) | ICR_START, &base->icr);
>> if (msg->condition == I2C_COND_STOP)
>> - writel(readl(ICR) | ICR_STOP, ICR);
>> + writel(readl(&base->icr) | ICR_STOP, &base->icr);
>> if (msg->acknack == I2C_ACKNAK_SENDNAK)
>> - writel(readl(ICR) | ICR_ACKNAK, ICR);
>> + writel(readl(&base->icr) | ICR_ACKNAK, &base->icr);
>> if (msg->acknack == I2C_ACKNAK_SENDACK)
>> - writel(readl(ICR) & ~ICR_ACKNAK, ICR);
>> - writel(readl(ICR) & ~ICR_ALDIE, ICR);
>> - writel(readl(ICR) | ICR_TB, ICR);
>> + writel(readl(&base->icr) & ~ICR_ACKNAK, &base->icr);
>> + writel(readl(&base->icr) & ~ICR_ALDIE, &base->icr);
>> + writel(readl(&base->icr) | ICR_TB, &base->icr);
>>
>> /* 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);
>> -
>> + writel(readl(&base->isr) | ISR_IRF, &base->isr);
>> break;
>> default:
>> goto transfer_error_illegal_param;
>> @@ -252,10 +236,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...
>> */
>
> Again, commenting style can be improved for overall block of comments.
Yep.
>
>> + icr = readl(&base->icr);
>> + writel(readl(&base->icr) & ~(ICR_SCLE | ICR_IUE), &base->icr);
>> +
>> 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
>
> This should be CONFIG_MV_I2C_REG
>
>> #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
>
> Ditto
>
>> #define CONFIG_HARD_I2C 1
>> #define CONFIG_SYS_I2C_SPEED 50000
>> #define CONFIG_SYS_I2C_SLAVE 0xfe
>> --
>
Best regards,
Lei
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
