> Hi,
>
> Few comments inlined.
>
> Regards,
> Chandra
> ----- Original Message -----
> From: ""Högander" Jouni" <jouni.hogander@xxxxxxxxx>
> To: "ext Rajendra Nayak" <rnayak@xxxxxx>
> Cc: <linux-omap@xxxxxxxxxxxxxxx>
> Sent: Thursday, September 11, 2008 3:34 PM
> Subject: Re: [PATCH 04/12] I2C re-init for every cmd
>
>
> "ext Rajendra Nayak" <rnayak@xxxxxx> writes:
>
>> This patch does i2c init/re-init for every transfer
>>
>> Signed-off-by: Rajendra Nayak <rnayak@xxxxxx>
>> ---
>> drivers/i2c/busses/i2c-omap.c | 2 ++
>> 1 files changed, 2 insertions(+)
>>
>> Index: linux-omap-2.6/drivers/i2c/busses/i2c-omap.c
>> ===================================================================
>> --- linux-omap-2.6.orig/drivers/i2c/busses/i2c-omap.c 2008-09-01
>> 18:11:28.000000000 +0530
>> +++ linux-omap-2.6/drivers/i2c/busses/i2c-omap.c 2008-09-01 18:11:52.000000000
>> +0530
>> @@ -496,6 +496,8 @@ omap_i2c_xfer(struct i2c_adapter *adap,
>>
>> omap_i2c_unidle(dev);
>>
>> + omap_i2c_init(dev);
>> +
>> if ((r = omap_i2c_wait_for_bb(dev)) < 0)
>> goto out;
>
> This is causing unacceptable delays on i2c transfers. This is because
> occasionally reset loop in init function enters msleep. Is it
> necessary to reset i2c controller after off-mode?
>
> Any opinions on this:
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 3778735..4a27035 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -126,6 +126,14 @@
> /* I2C System Configuration Register (OMAP_I2C_SYSC): */
> #define OMAP_I2C_SYSC_SRST (1 << 1) /* Soft Reset */
>
> +struct omap3_i2c_regs {
> + u16 sysc;
> + u16 psc;
> + u16 scll;
> + u16 sclh;
> + u16 buf;
> +};
> +
>
>
>
> We can add this as a part of controller structure itself instaed of
> creating a new structure. we will store this
> psc, scll and others as a part of controller and just restore it. That
> way we can do away with omap3_i2c_save_context every time in clk
> disable.
>My patch is neither saving ctx on every clk disable. Just after init.
>
>
>
>
>
> struct omap_i2c_dev {
> struct device *dev;
> void __iomem *base; /* virtual */
> @@ -147,6 +155,9 @@ struct omap_i2c_dev {
> unsigned b_hw:1; /* bad h/w fixes */
> unsigned idle:1;
> u16 iestate; /* Saved interrupt register */
> +#ifdef CONFIG_ARCH_OMAP34XX
> + struct omap3_i2c_regs context;
> +#endif
>
>
>
> I guess this should work for all the platforms. at least for omap2/3
> it should work w/o any hiccups.
> I2C_BUF is the only register which will require cpu check..so while
> restoring we can put that check for 2430/34xx.
>Can we use off mode with omap2? Generally, should we build in
>save/restore code for other than omap3?
agreed..but i guess it will not compile for 2430 and prev. platforms.
omap3_i2c_save_context and restore definition is under macro
while call is not. let me know if i am missing something.
also is SYSC register restore needed??
>
>
> };
>
>
>
> static inline void omap_i2c_write_reg(struct omap_i2c_dev *i2c_dev,
> @@ -160,6 +171,26 @@ static inline u16 omap_i2c_read_reg(struct
> omap_i2c_dev *i2c_dev, int reg)
> return __raw_readw(i2c_dev->base + reg);
> }
>
> +#ifdef CONFIG_ARCH_OMAP34XX
> +void omap3_i2c_save_context(struct omap_i2c_dev *dev)
> +{
> + dev->context.sysc = omap_i2c_read_reg(dev, OMAP_I2C_SYSC_REG);
> + dev->context.psc = omap_i2c_read_reg(dev, OMAP_I2C_PSC_REG);
> + dev->context.scll = omap_i2c_read_reg(dev, OMAP_I2C_SCLL_REG);
> + dev->context.sclh = omap_i2c_read_reg(dev, OMAP_I2C_SCLH_REG);
> + dev->context.buf = omap_i2c_read_reg(dev, OMAP_I2C_BUF_REG);
> +}
> +
>
>
> +void omap3_i2c_restore_context(struct omap_i2c_dev *dev)
> +{
> + omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, dev->context.sysc);
> + omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, dev->context.psc);
> + omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG, dev->context.scll);
> + omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG, dev->context.sclh);
> + omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG, dev->context.buf);
> +}
> +#endif
> +
>
> static int __init omap_i2c_get_clocks(struct omap_i2c_dev *dev)
> {
> if (cpu_is_omap16xx() || cpu_class_is_omap2()) {
> @@ -210,6 +241,10 @@ static void omap_i2c_unidle(struct omap_i2c_dev *dev)
> clk_enable(dev->iclk);
> clk_enable(dev->fclk);
> dev->idle = 0;
> +
> + if (cpu_is_omap34xx())
> + omap3_i2c_restore_context(dev);
> +
> if (dev->iestate)
> omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
> }
> @@ -344,6 +379,10 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
> OMAP_I2C_IE_ARDY | OMAP_I2C_IE_NACK |
> OMAP_I2C_IE_AL) | ((dev->fifo_size) ?
> (OMAP_I2C_IE_RDR | OMAP_I2C_IE_XDR) : 0));
> +
> + if (cpu_is_omap34xx())
> + omap3_i2c_save_context(dev);
> +
>
>
>
> this will become redundant as data will be part of controller
> structure.
>
>
> return 0;
> }
>
> @@ -496,8 +535,6 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct
> i2c_msg msgs[], int num)
>
> omap_i2c_unidle(dev);
>
> - omap_i2c_init(dev);
> -
>
>
> instead of calling restore from clk_enable we can replace
> omap_i2c_init by omap3_i2c_restore_context. that way clock enable code
> will be clean.
Well, I though unidle could be ok as ie reg is already restored there?
>
>
>
> if ((r = omap_i2c_wait_for_bb(dev)) < 0)
> goto out;
>
> --
> Jouni Högander
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
