Patchwork [U-Boot,V4] I2C: mxc_i2c rework

login
register
mail settings
Submitter Marek Vasut
Date July 29, 2011, 9:32 a.m.
Message ID <1311931978-18506-1-git-send-email-marek.vasut@gmail.com>
Download mbox | patch
Permalink /patch/107361/
State Superseded
Headers show

Comments

Marek Vasut - July 29, 2011, 9:32 a.m.
Rewrite the mxc_i2c driver.
 * This version is much closer to Linux implementation.
 * Fixes IPG_PERCLK being incorrectly used as clock source
 * Fixes behaviour of the driver on iMX51
 * Clean up coding style a bit ;-)

Based on commit: e39428d53d080ad2615b772d7f99d2a70c2aaab2
Date:   Mon Jun 21 09:27:05 2010 +0200
i2c-imx: do not allow interruptions when waiting for I2C to complete

Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
---
 drivers/i2c/mxc_i2c.c |  425 +++++++++++++++++++++++++++++++++----------------
 1 files changed, 290 insertions(+), 135 deletions(-)

V2: Convert register access to struct mxc_i2c_regs.
V3: Update licensing info
V3+: Add commit ID into commit message
V4: Fix coding style error and fix ifdef issue with clock
Jason Liu - July 29, 2011, 11:24 a.m.
Hi, Marek,

On Fri, Jul 29, 2011 at 5:32 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> Rewrite the mxc_i2c driver.
>  * This version is much closer to Linux implementation.
>  * Fixes IPG_PERCLK being incorrectly used as clock source
>  * Fixes behaviour of the driver on iMX51
>  * Clean up coding style a bit ;-)

I don't think you did the right thing by chaning IPG_PERCLK to IPG_CLK
As I said, the IPG_CLK is for IP register clock, while IPG_PERCLK is for
i2c function clock.

if you run clock command from mx51evk, you will get:
...
ipg clock     : 66500000Hz
ipg per clock : 665000000Hz
MX51EVK U-Boot >

It will give you that ipg per clock is 665M, which seems too big. It
is due to we configure
the pre-divider/pos-devider for perclk to zero, which leads to ipg_per
clock to be same as pll2 clock.
But I don't think this will have some issue.

BTW, I have applied your patch and test on mx53evk board, it seems the
i2c does not work correctly.
After apply your patch:
MX53EVK U-Boot > pmic dump  30
PMIC ID: 0x0000ffff [Rev: unknown]

0x00: 0001ffff 00ffffff 0039ffff 0000ffff 00ffffff 0000ffff 0000ffff 0000ffff
0x08: 0000ffff 0000ffff 0000ffff 0000ffff 0000ffff 0000ffff 0000ffff 0040ffff
0x10: 0000ffff 0000ffff 0000ffff 0000ffff 0000ffff 0001ffff 0000ffff 0000ffff
0x18: 0045ffff 0045ffff 0000ffff 0080ffff 0021ffff 0000ffff 0002ffff 0000ffff
0x20: 0004ffff 0000ffff 0021ffff 0000ffff 0000ffff 0000ffff 0000ffff 0000ffff
0x28: 0000ffff 0000ffff 0000ffff 0000ffff 0000ffff 0004ffff 0000ffff 00aeffff

The old:
MX53EVK U-Boot > pmic dump 30
PMIC ID: 0x000045d0 [Rev: 2.0]

0x00: 00015088 00ffffff 00395208 00000081 00fff7ff 0000401c 00000418 000045d0
0x08: 00000000 00000000 00000001 00000000 00000000 00000040 00000000 00400000
0x10: 00000000 00000000 00000000 00000000 00000011 0001ffff 00000000 00007fff
0x18: 00454a52 00456739 0000631a 0080739c 0021284a 00000a0a 00024fd0 000001d8
0x20: 00049208 00000000 00218000 00000000 00000000 00000000 00000000 00000000
0x28: 00000000 00000000 00000000 00008000 00000000 00046046 000001c0 00aeeaee

Jason
Marek Vasut - July 29, 2011, 5:09 p.m.
On Friday, July 29, 2011 01:24:49 PM Jason Hui wrote:
> Hi, Marek,
> 
> On Fri, Jul 29, 2011 at 5:32 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> > Rewrite the mxc_i2c driver.
> >  * This version is much closer to Linux implementation.
> >  * Fixes IPG_PERCLK being incorrectly used as clock source
> >  * Fixes behaviour of the driver on iMX51
> >  * Clean up coding style a bit ;-)
> 
> I don't think you did the right thing by chaning IPG_PERCLK to IPG_CLK
> As I said, the IPG_CLK is for IP register clock, while IPG_PERCLK is for
> i2c function clock.
> 
> if you run clock command from mx51evk, you will get:
> ...
> ipg clock     : 66500000Hz
> ipg per clock : 665000000Hz
> MX51EVK U-Boot >
> 
> It will give you that ipg per clock is 665M, which seems too big. It
> is due to we configure
> the pre-divider/pos-devider for perclk to zero, which leads to ipg_per
> clock to be same as pll2 clock.
> But I don't think this will have some issue.

Yes it will, the divider will be computed to be maximum in all cases ... you 
won't be able it divide 665MHz by anything from the table 40-7 in MX51RM to 
achieve any reasonable frequency.

On the contrary, 66.5MHz does give fine results.

> 
> BTW, I have applied your patch and test on mx53evk board, it seems the
> i2c does not work correctly.

Great. The clock used by the I2C module for this task are likely the 
module_clock, which are -- like on MX51 -- 66.5MHz. What do you get when you run 
the "clock" command on MX53EVK ?

Thanks

> After apply your patch:
> MX53EVK U-Boot > pmic dump  30
> PMIC ID: 0x0000ffff [Rev: unknown]
> 
> 0x00: 0001ffff 00ffffff 0039ffff 0000ffff 00ffffff 0000ffff 0000ffff
> 0000ffff 0x08: 0000ffff 0000ffff 0000ffff 0000ffff 0000ffff 0000ffff
> 0000ffff 0040ffff 0x10: 0000ffff 0000ffff 0000ffff 0000ffff 0000ffff
> 0001ffff 0000ffff 0000ffff 0x18: 0045ffff 0045ffff 0000ffff 0080ffff
> 0021ffff 0000ffff 0002ffff 0000ffff 0x20: 0004ffff 0000ffff 0021ffff
> 0000ffff 0000ffff 0000ffff 0000ffff 0000ffff 0x28: 0000ffff 0000ffff
> 0000ffff 0000ffff 0000ffff 0004ffff 0000ffff 00aeffff
> 
> The old:
> MX53EVK U-Boot > pmic dump 30
> PMIC ID: 0x000045d0 [Rev: 2.0]
> 
> 0x00: 00015088 00ffffff 00395208 00000081 00fff7ff 0000401c 00000418
> 000045d0 0x08: 00000000 00000000 00000001 00000000 00000000 00000040
> 00000000 00400000 0x10: 00000000 00000000 00000000 00000000 00000011
> 0001ffff 00000000 00007fff 0x18: 00454a52 00456739 0000631a 0080739c
> 0021284a 00000a0a 00024fd0 000001d8 0x20: 00049208 00000000 00218000
> 00000000 00000000 00000000 00000000 00000000 0x28: 00000000 00000000
> 00000000 00008000 00000000 00046046 000001c0 00aeeaee
> 
> Jason
Jason Liu - July 30, 2011, 6:42 a.m.
Hi, Marek,

On Sat, Jul 30, 2011 at 1:09 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On Friday, July 29, 2011 01:24:49 PM Jason Hui wrote:
>> Hi, Marek,
>>
>> On Fri, Jul 29, 2011 at 5:32 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> > Rewrite the mxc_i2c driver.
>> >  * This version is much closer to Linux implementation.
>> >  * Fixes IPG_PERCLK being incorrectly used as clock source
>> >  * Fixes behaviour of the driver on iMX51
>> >  * Clean up coding style a bit ;-)
>>
>> I don't think you did the right thing by chaning IPG_PERCLK to IPG_CLK
>> As I said, the IPG_CLK is for IP register clock, while IPG_PERCLK is for
>> i2c function clock.
>>
>> if you run clock command from mx51evk, you will get:
>> ...
>> ipg clock     : 66500000Hz
>> ipg per clock : 665000000Hz
>> MX51EVK U-Boot >
>>
>> It will give you that ipg per clock is 665M, which seems too big. It
>> is due to we configure
>> the pre-divider/pos-devider for perclk to zero, which leads to ipg_per
>> clock to be same as pll2 clock.
>> But I don't think this will have some issue.
>
> Yes it will, the divider will be computed to be maximum in all cases ... you
> won't be able it divide 665MHz by anything from the table 40-7 in MX51RM to
> achieve any reasonable frequency.
>
> On the contrary, 66.5MHz does give fine results.

it that, we can change the ipg_perclk to low freq, but we should not change the
i2c clock to ipg_clk, this is not correct.

>
>>
>> BTW, I have applied your patch and test on mx53evk board, it seems the
>> i2c does not work correctly.
>
> Great. The clock used by the I2C module for this task are likely the
> module_clock, which are -- like on MX51 -- 66.5MHz. What do you get when you run
> the "clock" command on MX53EVK ?

I'm not in the office for the whole next week, thus, I can't give you
the clock output for
mx53evk, but I'm sure the ipg_perclk on mx53evk is not set at 66.5Mhz.

Jason

>
> Thanks
>
>> After apply your patch:
>> MX53EVK U-Boot > pmic dump  30
>> PMIC ID: 0x0000ffff [Rev: unknown]
>>
>> 0x00: 0001ffff 00ffffff 0039ffff 0000ffff 00ffffff 0000ffff 0000ffff
>> 0000ffff 0x08: 0000ffff 0000ffff 0000ffff 0000ffff 0000ffff 0000ffff
>> 0000ffff 0040ffff 0x10: 0000ffff 0000ffff 0000ffff 0000ffff 0000ffff
>> 0001ffff 0000ffff 0000ffff 0x18: 0045ffff 0045ffff 0000ffff 0080ffff
>> 0021ffff 0000ffff 0002ffff 0000ffff 0x20: 0004ffff 0000ffff 0021ffff
>> 0000ffff 0000ffff 0000ffff 0000ffff 0000ffff 0x28: 0000ffff 0000ffff
>> 0000ffff 0000ffff 0000ffff 0004ffff 0000ffff 00aeffff
>>
>> The old:
>> MX53EVK U-Boot > pmic dump 30
>> PMIC ID: 0x000045d0 [Rev: 2.0]
>>
>> 0x00: 00015088 00ffffff 00395208 00000081 00fff7ff 0000401c 00000418
>> 000045d0 0x08: 00000000 00000000 00000001 00000000 00000000 00000040
>> 00000000 00400000 0x10: 00000000 00000000 00000000 00000000 00000011
>> 0001ffff 00000000 00007fff 0x18: 00454a52 00456739 0000631a 0080739c
>> 0021284a 00000a0a 00024fd0 000001d8 0x20: 00049208 00000000 00218000
>> 00000000 00000000 00000000 00000000 00000000 0x28: 00000000 00000000
>> 00000000 00008000 00000000 00046046 000001c0 00aeeaee
>>
>> Jason
>
Marek Vasut - July 30, 2011, 10:52 a.m.
On Saturday, July 30, 2011 08:42:19 AM Jason Hui wrote:
> Hi, Marek,
> 
> On Sat, Jul 30, 2011 at 1:09 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
> > On Friday, July 29, 2011 01:24:49 PM Jason Hui wrote:
> >> Hi, Marek,
> >> 
> >> On Fri, Jul 29, 2011 at 5:32 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> >> > Rewrite the mxc_i2c driver.
> >> >  * This version is much closer to Linux implementation.
> >> >  * Fixes IPG_PERCLK being incorrectly used as clock source
> >> >  * Fixes behaviour of the driver on iMX51
> >> >  * Clean up coding style a bit ;-)
> >> 
> >> I don't think you did the right thing by chaning IPG_PERCLK to IPG_CLK
> >> As I said, the IPG_CLK is for IP register clock, while IPG_PERCLK is for
> >> i2c function clock.
> >> 
> >> if you run clock command from mx51evk, you will get:
> >> ...
> >> ipg clock     : 66500000Hz
> >> ipg per clock : 665000000Hz
> >> MX51EVK U-Boot >
> >> 
> >> It will give you that ipg per clock is 665M, which seems too big. It
> >> is due to we configure
> >> the pre-divider/pos-devider for perclk to zero, which leads to ipg_per
> >> clock to be same as pll2 clock.
> >> But I don't think this will have some issue.
> > 
> > Yes it will, the divider will be computed to be maximum in all cases ...
> > you won't be able it divide 665MHz by anything from the table 40-7 in
> > MX51RM to achieve any reasonable frequency.
> > 
> > On the contrary, 66.5MHz does give fine results.
> 
> it that, we can change the ipg_perclk to low freq, but we should not change
> the i2c clock to ipg_clk, this is not correct.
> 
> >> BTW, I have applied your patch and test on mx53evk board, it seems the
> >> i2c does not work correctly.
> > 
> > Great. The clock used by the I2C module for this task are likely the
> > module_clock, which are -- like on MX51 -- 66.5MHz. What do you get when
> > you run the "clock" command on MX53EVK ?
> 
> I'm not in the office for the whole next week, thus, I can't give you
> the clock output for
> mx53evk, but I'm sure the ipg_perclk on mx53evk is not set at 66.5Mhz.

Yes, it's 33.3MHz on iMX53. I got a (remote) hand of a MX53 board. The IPG 
clock, on the other hand, are 66.5MHz on both MX51 and MX53. Maybe the issue 
with PMIC you're seeing is something else?

btw. how did you obtain these results ? Can you check the FDR divider value with 
and without this patch?

Thanks
> 
> Jason
> 
> > Thanks
> > 
> >> After apply your patch:
> >> MX53EVK U-Boot > pmic dump  30
> >> PMIC ID: 0x0000ffff [Rev: unknown]
> >> 
> >> 0x00: 0001ffff 00ffffff 0039ffff 0000ffff 00ffffff 0000ffff 0000ffff
> >> 0000ffff 0x08: 0000ffff 0000ffff 0000ffff 0000ffff 0000ffff 0000ffff
> >> 0000ffff 0040ffff 0x10: 0000ffff 0000ffff 0000ffff 0000ffff 0000ffff
> >> 0001ffff 0000ffff 0000ffff 0x18: 0045ffff 0045ffff 0000ffff 0080ffff
> >> 0021ffff 0000ffff 0002ffff 0000ffff 0x20: 0004ffff 0000ffff 0021ffff
> >> 0000ffff 0000ffff 0000ffff 0000ffff 0000ffff 0x28: 0000ffff 0000ffff
> >> 0000ffff 0000ffff 0000ffff 0004ffff 0000ffff 00aeffff
> >> 
> >> The old:
> >> MX53EVK U-Boot > pmic dump 30
> >> PMIC ID: 0x000045d0 [Rev: 2.0]
> >> 
> >> 0x00: 00015088 00ffffff 00395208 00000081 00fff7ff 0000401c 00000418
> >> 000045d0 0x08: 00000000 00000000 00000001 00000000 00000000 00000040
> >> 00000000 00400000 0x10: 00000000 00000000 00000000 00000000 00000011
> >> 0001ffff 00000000 00007fff 0x18: 00454a52 00456739 0000631a 0080739c
> >> 0021284a 00000a0a 00024fd0 000001d8 0x20: 00049208 00000000 00218000
> >> 00000000 00000000 00000000 00000000 00000000 0x28: 00000000 00000000
> >> 00000000 00008000 00000000 00046046 000001c0 00aeeaee
> >> 
> >> Jason
Marek Vasut - Aug. 10, 2011, 5:59 a.m.
On Saturday, July 30, 2011 12:52:53 PM Marek Vasut wrote:
> On Saturday, July 30, 2011 08:42:19 AM Jason Hui wrote:
> > Hi, Marek,
> > 
> > On Sat, Jul 30, 2011 at 1:09 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
> > > On Friday, July 29, 2011 01:24:49 PM Jason Hui wrote:
> > >> Hi, Marek,
> > >> 
> > >> On Fri, Jul 29, 2011 at 5:32 PM, Marek Vasut <marek.vasut@gmail.com> 
wrote:
> > >> > Rewrite the mxc_i2c driver.
> > >> > 
> > >> >  * This version is much closer to Linux implementation.
> > >> >  * Fixes IPG_PERCLK being incorrectly used as clock source
> > >> >  * Fixes behaviour of the driver on iMX51
> > >> >  * Clean up coding style a bit ;-)
> > >> 
> > >> I don't think you did the right thing by chaning IPG_PERCLK to IPG_CLK
> > >> As I said, the IPG_CLK is for IP register clock, while IPG_PERCLK is
> > >> for i2c function clock.
> > >> 
> > >> if you run clock command from mx51evk, you will get:
> > >> ...
> > >> ipg clock     : 66500000Hz
> > >> ipg per clock : 665000000Hz
> > >> MX51EVK U-Boot >
> > >> 
> > >> It will give you that ipg per clock is 665M, which seems too big. It
> > >> is due to we configure
> > >> the pre-divider/pos-devider for perclk to zero, which leads to ipg_per
> > >> clock to be same as pll2 clock.
> > >> But I don't think this will have some issue.
> > > 
> > > Yes it will, the divider will be computed to be maximum in all cases
> > > ... you won't be able it divide 665MHz by anything from the table 40-7
> > > in MX51RM to achieve any reasonable frequency.
> > > 
> > > On the contrary, 66.5MHz does give fine results.
> > 
> > it that, we can change the ipg_perclk to low freq, but we should not
> > change the i2c clock to ipg_clk, this is not correct.
> > 
> > >> BTW, I have applied your patch and test on mx53evk board, it seems the
> > >> i2c does not work correctly.
> > > 
> > > Great. The clock used by the I2C module for this task are likely the
> > > module_clock, which are -- like on MX51 -- 66.5MHz. What do you get
> > > when you run the "clock" command on MX53EVK ?
> > 
> > I'm not in the office for the whole next week, thus, I can't give you
> > the clock output for
> > mx53evk, but I'm sure the ipg_perclk on mx53evk is not set at 66.5Mhz.
> 
> Yes, it's 33.3MHz on iMX53. I got a (remote) hand of a MX53 board. The IPG
> clock, on the other hand, are 66.5MHz on both MX51 and MX53. Maybe the
> issue with PMIC you're seeing is something else?
> 
> btw. how did you obtain these results ? Can you check the FDR divider value
> with and without this patch?
> 
> Thanks

BUMP

> 
> > Jason
> > 
> > > Thanks
> > > 
> > >> After apply your patch:
> > >> MX53EVK U-Boot > pmic dump  30
> > >> PMIC ID: 0x0000ffff [Rev: unknown]
> > >> 
> > >> 0x00: 0001ffff 00ffffff 0039ffff 0000ffff 00ffffff 0000ffff 0000ffff
> > >> 0000ffff 0x08: 0000ffff 0000ffff 0000ffff 0000ffff 0000ffff 0000ffff
> > >> 0000ffff 0040ffff 0x10: 0000ffff 0000ffff 0000ffff 0000ffff 0000ffff
> > >> 0001ffff 0000ffff 0000ffff 0x18: 0045ffff 0045ffff 0000ffff 0080ffff
> > >> 0021ffff 0000ffff 0002ffff 0000ffff 0x20: 0004ffff 0000ffff 0021ffff
> > >> 0000ffff 0000ffff 0000ffff 0000ffff 0000ffff 0x28: 0000ffff 0000ffff
> > >> 0000ffff 0000ffff 0000ffff 0004ffff 0000ffff 00aeffff
> > >> 
> > >> The old:
> > >> MX53EVK U-Boot > pmic dump 30
> > >> PMIC ID: 0x000045d0 [Rev: 2.0]
> > >> 
> > >> 0x00: 00015088 00ffffff 00395208 00000081 00fff7ff 0000401c 00000418
> > >> 000045d0 0x08: 00000000 00000000 00000001 00000000 00000000 00000040
> > >> 00000000 00400000 0x10: 00000000 00000000 00000000 00000000 00000011
> > >> 0001ffff 00000000 00007fff 0x18: 00454a52 00456739 0000631a 0080739c
> > >> 0021284a 00000a0a 00024fd0 000001d8 0x20: 00049208 00000000 00218000
> > >> 00000000 00000000 00000000 00000000 00000000 0x28: 00000000 00000000
> > >> 00000000 00008000 00000000 00046046 000001c0 00aeeaee
> > >> 
> > >> Jason
Jason Liu - Aug. 10, 2011, 6:26 a.m.
On Wed, Aug 10, 2011 at 1:59 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On Saturday, July 30, 2011 12:52:53 PM Marek Vasut wrote:
>> On Saturday, July 30, 2011 08:42:19 AM Jason Hui wrote:
>> > Hi, Marek,
>> >
>> > On Sat, Jul 30, 2011 at 1:09 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> > > On Friday, July 29, 2011 01:24:49 PM Jason Hui wrote:
>> > >> Hi, Marek,
>> > >>
>> > >> On Fri, Jul 29, 2011 at 5:32 PM, Marek Vasut <marek.vasut@gmail.com>
> wrote:
>> > >> > Rewrite the mxc_i2c driver.
>> > >> >
>> > >> >  * This version is much closer to Linux implementation.
>> > >> >  * Fixes IPG_PERCLK being incorrectly used as clock source
>> > >> >  * Fixes behaviour of the driver on iMX51
>> > >> >  * Clean up coding style a bit ;-)
>> > >>
>> > >> I don't think you did the right thing by chaning IPG_PERCLK to IPG_CLK
>> > >> As I said, the IPG_CLK is for IP register clock, while IPG_PERCLK is
>> > >> for i2c function clock.
>> > >>
>> > >> if you run clock command from mx51evk, you will get:
>> > >> ...
>> > >> ipg clock     : 66500000Hz
>> > >> ipg per clock : 665000000Hz
>> > >> MX51EVK U-Boot >
>> > >>
>> > >> It will give you that ipg per clock is 665M, which seems too big. It
>> > >> is due to we configure
>> > >> the pre-divider/pos-devider for perclk to zero, which leads to ipg_per
>> > >> clock to be same as pll2 clock.
>> > >> But I don't think this will have some issue.
>> > >
>> > > Yes it will, the divider will be computed to be maximum in all cases
>> > > ... you won't be able it divide 665MHz by anything from the table 40-7
>> > > in MX51RM to achieve any reasonable frequency.
>> > >
>> > > On the contrary, 66.5MHz does give fine results.
>> >
>> > it that, we can change the ipg_perclk to low freq, but we should not
>> > change the i2c clock to ipg_clk, this is not correct.
>> >
>> > >> BTW, I have applied your patch and test on mx53evk board, it seems the
>> > >> i2c does not work correctly.
>> > >
>> > > Great. The clock used by the I2C module for this task are likely the
>> > > module_clock, which are -- like on MX51 -- 66.5MHz. What do you get
>> > > when you run the "clock" command on MX53EVK ?
>> >
>> > I'm not in the office for the whole next week, thus, I can't give you
>> > the clock output for
>> > mx53evk, but I'm sure the ipg_perclk on mx53evk is not set at 66.5Mhz.
>>
>> Yes, it's 33.3MHz on iMX53. I got a (remote) hand of a MX53 board. The IPG
>> clock, on the other hand, are 66.5MHz on both MX51 and MX53. Maybe the
>> issue with PMIC you're seeing is something else?
>>
>> btw. how did you obtain these results ? Can you check the FDR divider value
>> with and without this patch?

You can use the clock command to get it. As for the FDR, you want me to check it
on i.mx53evk board right?

Jason

>>
>> Thanks
>
>
>>
>> > Jason
>> >
>> > > Thanks
>> > >
>> > >> After apply your patch:
>> > >> MX53EVK U-Boot > pmic dump  30
>> > >> PMIC ID: 0x0000ffff [Rev: unknown]
>> > >>
>> > >> 0x00: 0001ffff 00ffffff 0039ffff 0000ffff 00ffffff 0000ffff 0000ffff
>> > >> 0000ffff 0x08: 0000ffff 0000ffff 0000ffff 0000ffff 0000ffff 0000ffff
>> > >> 0000ffff 0040ffff 0x10: 0000ffff 0000ffff 0000ffff 0000ffff 0000ffff
>> > >> 0001ffff 0000ffff 0000ffff 0x18: 0045ffff 0045ffff 0000ffff 0080ffff
>> > >> 0021ffff 0000ffff 0002ffff 0000ffff 0x20: 0004ffff 0000ffff 0021ffff
>> > >> 0000ffff 0000ffff 0000ffff 0000ffff 0000ffff 0x28: 0000ffff 0000ffff
>> > >> 0000ffff 0000ffff 0000ffff 0004ffff 0000ffff 00aeffff
>> > >>
>> > >> The old:
>> > >> MX53EVK U-Boot > pmic dump 30
>> > >> PMIC ID: 0x000045d0 [Rev: 2.0]
>> > >>
>> > >> 0x00: 00015088 00ffffff 00395208 00000081 00fff7ff 0000401c 00000418
>> > >> 000045d0 0x08: 00000000 00000000 00000001 00000000 00000000 00000040
>> > >> 00000000 00400000 0x10: 00000000 00000000 00000000 00000000 00000011
>> > >> 0001ffff 00000000 00007fff 0x18: 00454a52 00456739 0000631a 0080739c
>> > >> 0021284a 00000a0a 00024fd0 000001d8 0x20: 00049208 00000000 00218000
>> > >> 00000000 00000000 00000000 00000000 00000000 0x28: 00000000 00000000
>> > >> 00000000 00008000 00000000 00046046 000001c0 00aeeaee
>> > >>
>> > >> Jason
>
Marek Vasut - Aug. 10, 2011, 7:11 a.m.
On Wednesday, August 10, 2011 08:26:05 AM Jason Hui wrote:
> On Wed, Aug 10, 2011 at 1:59 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> > On Saturday, July 30, 2011 12:52:53 PM Marek Vasut wrote:
> >> On Saturday, July 30, 2011 08:42:19 AM Jason Hui wrote:
> >> > Hi, Marek,
> >> > 
> >> > On Sat, Jul 30, 2011 at 1:09 AM, Marek Vasut <marek.vasut@gmail.com> 
wrote:
> >> > > On Friday, July 29, 2011 01:24:49 PM Jason Hui wrote:
> >> > >> Hi, Marek,
> >> > >> 
> >> > >> On Fri, Jul 29, 2011 at 5:32 PM, Marek Vasut
> >> > >> <marek.vasut@gmail.com>
> > 
> > wrote:
> >> > >> > Rewrite the mxc_i2c driver.
> >> > >> > 
> >> > >> >  * This version is much closer to Linux implementation.
> >> > >> >  * Fixes IPG_PERCLK being incorrectly used as clock source
> >> > >> >  * Fixes behaviour of the driver on iMX51
> >> > >> >  * Clean up coding style a bit ;-)
> >> > >> 
> >> > >> I don't think you did the right thing by chaning IPG_PERCLK to
> >> > >> IPG_CLK As I said, the IPG_CLK is for IP register clock, while
> >> > >> IPG_PERCLK is for i2c function clock.
> >> > >> 
> >> > >> if you run clock command from mx51evk, you will get:
> >> > >> ...
> >> > >> ipg clock     : 66500000Hz
> >> > >> ipg per clock : 665000000Hz
> >> > >> MX51EVK U-Boot >
> >> > >> 
> >> > >> It will give you that ipg per clock is 665M, which seems too big.
> >> > >> It is due to we configure
> >> > >> the pre-divider/pos-devider for perclk to zero, which leads to
> >> > >> ipg_per clock to be same as pll2 clock.
> >> > >> But I don't think this will have some issue.
> >> > > 
> >> > > Yes it will, the divider will be computed to be maximum in all cases
> >> > > ... you won't be able it divide 665MHz by anything from the table
> >> > > 40-7 in MX51RM to achieve any reasonable frequency.
> >> > > 
> >> > > On the contrary, 66.5MHz does give fine results.
> >> > 
> >> > it that, we can change the ipg_perclk to low freq, but we should not
> >> > change the i2c clock to ipg_clk, this is not correct.
> >> > 
> >> > >> BTW, I have applied your patch and test on mx53evk board, it seems
> >> > >> the i2c does not work correctly.
> >> > > 
> >> > > Great. The clock used by the I2C module for this task are likely the
> >> > > module_clock, which are -- like on MX51 -- 66.5MHz. What do you get
> >> > > when you run the "clock" command on MX53EVK ?
> >> > 
> >> > I'm not in the office for the whole next week, thus, I can't give you
> >> > the clock output for
> >> > mx53evk, but I'm sure the ipg_perclk on mx53evk is not set at 66.5Mhz.
> >> 
> >> Yes, it's 33.3MHz on iMX53. I got a (remote) hand of a MX53 board. The
> >> IPG clock, on the other hand, are 66.5MHz on both MX51 and MX53. Maybe
> >> the issue with PMIC you're seeing is something else?
> >> 
> >> btw. how did you obtain these results ? Can you check the FDR divider
> >> value with and without this patch?
> 
> You can use the clock command to get it. As for the FDR, you want me to
> check it on i.mx53evk board right?

Yes please. I hope to get my hands on mx53 system in the evening probably, but 
I'd still be grateful if you could test it.

> 
> Jason
> 
> >> Thanks
> >> 
> >> > Jason
> >> > 
> >> > > Thanks
> >> > > 
> >> > >> After apply your patch:
> >> > >> MX53EVK U-Boot > pmic dump  30
> >> > >> PMIC ID: 0x0000ffff [Rev: unknown]
> >> > >> 
> >> > >> 0x00: 0001ffff 00ffffff 0039ffff 0000ffff 00ffffff 0000ffff
> >> > >> 0000ffff 0000ffff 0x08: 0000ffff 0000ffff 0000ffff 0000ffff
> >> > >> 0000ffff 0000ffff 0000ffff 0040ffff 0x10: 0000ffff 0000ffff
> >> > >> 0000ffff 0000ffff 0000ffff 0001ffff 0000ffff 0000ffff 0x18:
> >> > >> 0045ffff 0045ffff 0000ffff 0080ffff 0021ffff 0000ffff 0002ffff
> >> > >> 0000ffff 0x20: 0004ffff 0000ffff 0021ffff 0000ffff 0000ffff
> >> > >> 0000ffff 0000ffff 0000ffff 0x28: 0000ffff 0000ffff 0000ffff
> >> > >> 0000ffff 0000ffff 0004ffff 0000ffff 00aeffff
> >> > >> 
> >> > >> The old:
> >> > >> MX53EVK U-Boot > pmic dump 30
> >> > >> PMIC ID: 0x000045d0 [Rev: 2.0]
> >> > >> 
> >> > >> 0x00: 00015088 00ffffff 00395208 00000081 00fff7ff 0000401c
> >> > >> 00000418 000045d0 0x08: 00000000 00000000 00000001 00000000
> >> > >> 00000000 00000040 00000000 00400000 0x10: 00000000 00000000
> >> > >> 00000000 00000000 00000011 0001ffff 00000000 00007fff 0x18:
> >> > >> 00454a52 00456739 0000631a 0080739c 0021284a 00000a0a 00024fd0
> >> > >> 000001d8 0x20: 00049208 00000000 00218000 00000000 00000000
> >> > >> 00000000 00000000 00000000 0x28: 00000000 00000000 00000000
> >> > >> 00008000 00000000 00046046 000001c0 00aeeaee
> >> > >> 
> >> > >> Jason
Jason Liu - Aug. 11, 2011, 5:26 a.m.
Hi, Marek,

On Wed, Aug 10, 2011 at 3:11 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On Wednesday, August 10, 2011 08:26:05 AM Jason Hui wrote:
>> On Wed, Aug 10, 2011 at 1:59 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> > On Saturday, July 30, 2011 12:52:53 PM Marek Vasut wrote:
>> >> On Saturday, July 30, 2011 08:42:19 AM Jason Hui wrote:
>> >> > Hi, Marek,
>> >> >
>> >> > On Sat, Jul 30, 2011 at 1:09 AM, Marek Vasut <marek.vasut@gmail.com>
> wrote:
>> >> > > On Friday, July 29, 2011 01:24:49 PM Jason Hui wrote:
>> >> > >> Hi, Marek,
>> >> > >>
>> >> > >> On Fri, Jul 29, 2011 at 5:32 PM, Marek Vasut
>> >> > >> <marek.vasut@gmail.com>
>> >
>> > wrote:
>> >> > >> > Rewrite the mxc_i2c driver.
>> >> > >> >
>> >> > >> >  * This version is much closer to Linux implementation.
>> >> > >> >  * Fixes IPG_PERCLK being incorrectly used as clock source
>> >> > >> >  * Fixes behaviour of the driver on iMX51
>> >> > >> >  * Clean up coding style a bit ;-)
>> >> > >>
>> >> > >> I don't think you did the right thing by chaning IPG_PERCLK to
>> >> > >> IPG_CLK As I said, the IPG_CLK is for IP register clock, while
>> >> > >> IPG_PERCLK is for i2c function clock.
>> >> > >>
>> >> > >> if you run clock command from mx51evk, you will get:
>> >> > >> ...
>> >> > >> ipg clock     : 66500000Hz
>> >> > >> ipg per clock : 665000000Hz
>> >> > >> MX51EVK U-Boot >
>> >> > >>
>> >> > >> It will give you that ipg per clock is 665M, which seems too big.
>> >> > >> It is due to we configure
>> >> > >> the pre-divider/pos-devider for perclk to zero, which leads to
>> >> > >> ipg_per clock to be same as pll2 clock.
>> >> > >> But I don't think this will have some issue.
>> >> > >
>> >> > > Yes it will, the divider will be computed to be maximum in all cases
>> >> > > ... you won't be able it divide 665MHz by anything from the table
>> >> > > 40-7 in MX51RM to achieve any reasonable frequency.
>> >> > >
>> >> > > On the contrary, 66.5MHz does give fine results.
>> >> >
>> >> > it that, we can change the ipg_perclk to low freq, but we should not
>> >> > change the i2c clock to ipg_clk, this is not correct.
>> >> >
>> >> > >> BTW, I have applied your patch and test on mx53evk board, it seems
>> >> > >> the i2c does not work correctly.
>> >> > >
>> >> > > Great. The clock used by the I2C module for this task are likely the
>> >> > > module_clock, which are -- like on MX51 -- 66.5MHz. What do you get
>> >> > > when you run the "clock" command on MX53EVK ?
>> >> >
>> >> > I'm not in the office for the whole next week, thus, I can't give you
>> >> > the clock output for
>> >> > mx53evk, but I'm sure the ipg_perclk on mx53evk is not set at 66.5Mhz.
>> >>
>> >> Yes, it's 33.3MHz on iMX53. I got a (remote) hand of a MX53 board. The
>> >> IPG clock, on the other hand, are 66.5MHz on both MX51 and MX53. Maybe
>> >> the issue with PMIC you're seeing is something else?
>> >>
>> >> btw. how did you obtain these results ? Can you check the FDR divider
>> >> value with and without this patch?
>>
>> You can use the clock command to get it. As for the FDR, you want me to
>> check it on i.mx53evk board right?
>
> Yes please. I hope to get my hands on mx53 system in the evening probably, but
> I'd still be grateful if you could test it.

The clock for i2c should be ipg_perclk, the linux kernel code has been
fixed by the following
commit:

commit 9d73242458d9a2fe26e2e240488063d414eacb1c
Author: Lothar Waßmann <LW@KARO-electronics.de>
Date:   Mon Jul 4 15:52:17 2011 +0200

    mach-mx5: fix the I2C clock parents

    The clock from which the I2C timing is derived is the ipg_perclk
not ipg_clk.

    I2C bus frequency was lower by a factor of ~8 due to the clock divider
    calculation being based on 66.5MHz IPG clock while the bus actually
    uses 8MHz ipg_perclk.

    Kernel version: 3.0.0-rc2 branch 'imx-for-next' of
git://git.pengutronix.de/git/imx/linux-2.6

    Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
    Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Jason

>
>>
>> Jason
>>
>> >> Thanks
>> >>
>> >> > Jason
>> >> >
>> >> > > Thanks
>> >> > >
>> >> > >> After apply your patch:
>> >> > >> MX53EVK U-Boot > pmic dump  30
>> >> > >> PMIC ID: 0x0000ffff [Rev: unknown]
>> >> > >>
>> >> > >> 0x00: 0001ffff 00ffffff 0039ffff 0000ffff 00ffffff 0000ffff
>> >> > >> 0000ffff 0000ffff 0x08: 0000ffff 0000ffff 0000ffff 0000ffff
>> >> > >> 0000ffff 0000ffff 0000ffff 0040ffff 0x10: 0000ffff 0000ffff
>> >> > >> 0000ffff 0000ffff 0000ffff 0001ffff 0000ffff 0000ffff 0x18:
>> >> > >> 0045ffff 0045ffff 0000ffff 0080ffff 0021ffff 0000ffff 0002ffff
>> >> > >> 0000ffff 0x20: 0004ffff 0000ffff 0021ffff 0000ffff 0000ffff
>> >> > >> 0000ffff 0000ffff 0000ffff 0x28: 0000ffff 0000ffff 0000ffff
>> >> > >> 0000ffff 0000ffff 0004ffff 0000ffff 00aeffff
>> >> > >>
>> >> > >> The old:
>> >> > >> MX53EVK U-Boot > pmic dump 30
>> >> > >> PMIC ID: 0x000045d0 [Rev: 2.0]
>> >> > >>
>> >> > >> 0x00: 00015088 00ffffff 00395208 00000081 00fff7ff 0000401c
>> >> > >> 00000418 000045d0 0x08: 00000000 00000000 00000001 00000000
>> >> > >> 00000000 00000040 00000000 00400000 0x10: 00000000 00000000
>> >> > >> 00000000 00000000 00000011 0001ffff 00000000 00007fff 0x18:
>> >> > >> 00454a52 00456739 0000631a 0080739c 0021284a 00000a0a 00024fd0
>> >> > >> 000001d8 0x20: 00049208 00000000 00218000 00000000 00000000
>> >> > >> 00000000 00000000 00000000 0x28: 00000000 00000000 00000000
>> >> > >> 00008000 00000000 00046046 000001c0 00aeeaee
>> >> > >>
>> >> > >> Jason
>

Patch

diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c
index 89d1973..8049c93 100644
--- a/drivers/i2c/mxc_i2c.c
+++ b/drivers/i2c/mxc_i2c.c
@@ -1,7 +1,15 @@ 
 /*
- * i2c driver for Freescale mx31
+ * i2c driver for Freescale i.MX series
  *
  * (c) 2007 Pengutronix, Sascha Hauer <s.hauer@pengutronix.de>
+ * (c) 2011 Marek Vasut <marek.vasut@gmail.com>
+ *
+ * Based on i2c-imx.c from linux kernel:
+ *  Copyright (C) 2005 Torsten Koschorrek <koschorrek at synertronixx.de>
+ *  Copyright (C) 2005 Matthias Blaschke <blaschke at synertronixx.de>
+ *  Copyright (C) 2007 RightHand Technologies, Inc.
+ *  Copyright (C) 2008 Darius Augulis <darius.augulis at teltonika.lt>
+ *
  *
  * See file CREDITS for list of people who contributed to this
  * project.
@@ -30,11 +38,13 @@ 
 #include <asm/arch/clock.h>
 #include <asm/arch/imx-regs.h>
 
-#define IADR	0x00
-#define IFDR	0x04
-#define I2CR	0x08
-#define I2SR	0x0c
-#define I2DR	0x10
+struct mxc_i2c_regs {
+	uint32_t	iadr;
+	uint32_t	ifdr;
+	uint32_t	i2cr;
+	uint32_t	i2sr;
+	uint32_t	i2dr;
+};
 
 #define I2CR_IEN	(1 << 7)
 #define I2CR_IIEN	(1 << 6)
@@ -68,218 +78,363 @@ 
 #endif
 
 #define I2C_MAX_TIMEOUT		10000
-#define I2C_MAX_RETRIES		3
 
-static u16 div[] = { 30, 32, 36, 42, 48, 52, 60, 72, 80, 88, 104, 128, 144,
-	             160, 192, 240, 288, 320, 384, 480, 576, 640, 768, 960,
-	             1152, 1280, 1536, 1920, 2304, 2560, 3072, 3840};
+static u16 i2c_clk_div[50][2] = {
+	{ 22,	0x20 }, { 24,	0x21 }, { 26,	0x22 }, { 28,	0x23 },
+	{ 30,	0x00 }, { 32,	0x24 }, { 36,	0x25 }, { 40,	0x26 },
+	{ 42,	0x03 }, { 44,	0x27 }, { 48,	0x28 }, { 52,	0x05 },
+	{ 56,	0x29 }, { 60,	0x06 }, { 64,	0x2A }, { 72,	0x2B },
+	{ 80,	0x2C }, { 88,	0x09 }, { 96,	0x2D }, { 104,	0x0A },
+	{ 112,	0x2E }, { 128,	0x2F }, { 144,	0x0C }, { 160,	0x30 },
+	{ 192,	0x31 }, { 224,	0x32 }, { 240,	0x0F }, { 256,	0x33 },
+	{ 288,	0x10 }, { 320,	0x34 }, { 384,	0x35 }, { 448,	0x36 },
+	{ 480,	0x13 }, { 512,	0x37 }, { 576,	0x14 }, { 640,	0x38 },
+	{ 768,	0x39 }, { 896,	0x3A }, { 960,	0x17 }, { 1024,	0x3B },
+	{ 1152,	0x18 }, { 1280,	0x3C }, { 1536,	0x3D }, { 1792,	0x3E },
+	{ 1920,	0x1B }, { 2048,	0x3F }, { 2304,	0x1C }, { 2560,	0x1D },
+	{ 3072,	0x1E }, { 3840,	0x1F }
+};
+
+static u8 clk_idx;
 
-static inline void i2c_reset(void)
-{
-	writew(0, I2C_BASE + I2CR);	/* Reset module */
-	writew(0, I2C_BASE + I2SR);
-	writew(I2CR_IEN, I2C_BASE + I2CR);
-}
-
-void i2c_init(int speed, int unused)
+/*
+ * Calculate and set proper clock divider
+ */
+static void i2c_imx_set_clk(unsigned int rate)
 {
-	int freq;
+	struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;
+	unsigned int i2c_clk_rate;
+	unsigned int div;
 	int i;
 
 #if defined(CONFIG_MX31)
 	struct clock_control_regs *sc_regs =
 		(struct clock_control_regs *)CCM_BASE;
 
-	freq = mx31_get_ipg_clk();
 	/* start the required I2C clock */
 	writel(readl(&sc_regs->cgr0) | (3 << I2C_CLK_OFFSET),
 		&sc_regs->cgr0);
-#else
-	freq = mxc_get_clock(MXC_IPG_PERCLK);
 #endif
 
-	for (i = 0; i < 0x1f; i++)
-		if (freq / div[i] <= speed)
-			break;
+	/* Divider value calculation */
+	i2c_clk_rate = mxc_get_clock(MXC_IPG_CLK);
+
+	div = (i2c_clk_rate + rate - 1) / rate;
+	if (div < i2c_clk_div[0][0])
+		i = 0;
+	else if (div > i2c_clk_div[ARRAY_SIZE(i2c_clk_div) - 1][0])
+		i = ARRAY_SIZE(i2c_clk_div) - 1;
+	else
+		for (i = 0; i2c_clk_div[i][0] < div; i++)
+			;
+
+	/* Store divider value */
+	writeb(div, &i2c_regs->ifdr);
+	clk_idx = div;
+}
 
-	debug("%s: speed: %d\n", __func__, speed);
+/*
+ * Reset I2C Controller
+ */
+void i2c_reset(void)
+{
+	struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;
+
+	writeb(0, &i2c_regs->i2cr);	/* Reset module */
+	writeb(0, &i2c_regs->i2sr);
+}
 
-	writew(i, I2C_BASE + IFDR);
+/*
+ * Init I2C Bus
+ */
+void i2c_init(int speed, int unused)
+{
+	i2c_imx_set_clk(speed);
 	i2c_reset();
 }
 
-static int wait_idle(void)
+/*
+ * Wait for bus to be busy (or free if for_busy = 0)
+ *
+ * for_busy = 1: Wait for IBB to be asserted
+ * for_busy = 0: Wait for IBB to be de-asserted
+ */
+int i2c_imx_bus_busy(int for_busy)
 {
+	struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;
+	unsigned int temp;
+
 	int timeout = I2C_MAX_TIMEOUT;
 
-	while ((readw(I2C_BASE + I2SR) & I2SR_IBB) && --timeout) {
-		writew(0, I2C_BASE + I2SR);
+	while (timeout--) {
+		temp = readb(&i2c_regs->i2sr);
+
+		if (for_busy && (temp & I2SR_IBB))
+			return 0;
+		if (!for_busy && !(temp & I2SR_IBB))
+			return 0;
+
 		udelay(1);
 	}
-	return timeout ? timeout : (!(readw(I2C_BASE + I2SR) & I2SR_IBB));
+
+	return 1;
 }
 
-static int wait_busy(void)
+/*
+ * Wait for transaction to complete
+ */
+int i2c_imx_trx_complete(void)
 {
+	struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;
 	int timeout = I2C_MAX_TIMEOUT;
 
-	while (!(readw(I2C_BASE + I2SR) & I2SR_IBB) && --timeout)
+	while (timeout--) {
+		if (readb(&i2c_regs->i2sr) & I2SR_IIF) {
+			writeb(0, &i2c_regs->i2sr);
+			return 0;
+		}
+
 		udelay(1);
-	writew(0, I2C_BASE + I2SR); /* clear interrupt */
+	}
 
-	return timeout;
+	return 1;
 }
 
-static int wait_complete(void)
+/*
+ * Check if the transaction was ACKed
+ */
+int i2c_imx_acked(void)
 {
-	int timeout = I2C_MAX_TIMEOUT;
+	struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;
 
-	while ((!(readw(I2C_BASE + I2SR) & I2SR_ICF)) && (--timeout)) {
-		writew(0, I2C_BASE + I2SR);
-		udelay(1);
-	}
-	udelay(200);
+	return readb(&i2c_regs->i2sr) & I2SR_RX_NO_AK;
+}
 
-	writew(0, I2C_BASE + I2SR);	/* clear interrupt */
+/*
+ * Start the controller
+ */
+int i2c_imx_start(void)
+{
+	struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;
+	unsigned int temp = 0;
+	int result;
 
-	return timeout;
-}
+	writeb(clk_idx, &i2c_regs->ifdr);
 
+	/* Enable I2C controller */
+	writeb(0, &i2c_regs->i2sr);
+	writeb(I2CR_IEN, &i2c_regs->i2cr);
 
-static int tx_byte(u8 byte)
-{
-	writew(byte, I2C_BASE + I2DR);
+	/* Wait controller to be stable */
+	udelay(50);
+
+	/* Start I2C transaction */
+	temp = readb(&i2c_regs->i2cr);
+	temp |= I2CR_MSTA;
+	writeb(temp, &i2c_regs->i2cr);
+
+	result = i2c_imx_bus_busy(1);
+	if (result)
+		return result;
+
+	temp |= I2CR_MTX | I2CR_TX_NO_AK;
+	writeb(temp, &i2c_regs->i2cr);
 
-	if (!wait_complete() || readw(I2C_BASE + I2SR) & I2SR_RX_NO_AK)
-		return -1;
 	return 0;
 }
 
-static int rx_byte(int last)
+/*
+ * Stop the controller
+ */
+void i2c_imx_stop()
 {
-	if (!wait_complete())
-		return -1;
+	struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;
+	unsigned int temp = 0;
+
+	/* Stop I2C transaction */
+	temp = readb(&i2c_regs->i2cr);
+	temp |= ~(I2CR_MSTA | I2CR_MTX);
+	writeb(temp, &i2c_regs->i2cr);
 
-	if (last)
-		writew(I2CR_IEN, I2C_BASE + I2CR);
+	i2c_imx_bus_busy(0);
 
-	return readw(I2C_BASE + I2DR);
+	/* Disable I2C controller */
+	writeb(0, &i2c_regs->i2cr);
 }
 
-int i2c_probe(uchar chip)
+/*
+ * Set chip address and access mode
+ *
+ * read = 1: READ access
+ * read = 0: WRITE access
+ */
+int i2c_imx_set_chip_addr(uchar chip, int read)
 {
+	struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;
 	int ret;
 
-	writew(0, I2C_BASE + I2CR); /* Reset module */
-	writew(I2CR_IEN, I2C_BASE + I2CR);
+	writeb((chip << 1) | read, &i2c_regs->i2dr);
+
+	ret = i2c_imx_trx_complete();
+	if (ret)
+		return ret;
 
-	writew(I2CR_IEN |  I2CR_MSTA | I2CR_MTX, I2C_BASE + I2CR);
-	ret = tx_byte(chip << 1);
-	writew(I2CR_IEN | I2CR_MTX, I2C_BASE + I2CR);
+	ret = i2c_imx_acked();
+	if (ret)
+		return ret;
 
 	return ret;
 }
 
-static int i2c_addr(uchar chip, uint addr, int alen)
+/*
+ * Write register address
+ */
+int i2c_imx_set_reg_addr(uint addr, int alen)
 {
-	int i, retry = 0;
-	for (retry = 0; retry < 3; retry++) {
-		if (wait_idle())
+	struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;
+	int ret;
+	int i;
+
+	for (i = 0; i < (8 * alen); i += 8) {
+		writeb((addr >> i) & 0xff, &i2c_regs->i2dr);
+
+		ret = i2c_imx_trx_complete();
+		if (ret)
 			break;
-		i2c_reset();
-		for (i = 0; i < I2C_MAX_TIMEOUT; i++)
-			udelay(1);
-	}
-	if (retry >= I2C_MAX_RETRIES) {
-		debug("%s:bus is busy(%x)\n",
-		       __func__, readw(I2C_BASE + I2SR));
-		return -1;
-	}
-	writew(I2CR_IEN | I2CR_MSTA | I2CR_MTX, I2C_BASE + I2CR);
 
-	if (!wait_busy()) {
-		debug("%s:trigger start fail(%x)\n",
-		       __func__, readw(I2C_BASE + I2SR));
-		return -1;
+		ret = i2c_imx_acked();
+		if (ret)
+			break;
 	}
 
-	if (tx_byte(chip << 1) || (readw(I2C_BASE + I2SR) & I2SR_RX_NO_AK)) {
-		debug("%s:chip address cycle fail(%x)\n",
-		       __func__, readw(I2C_BASE + I2SR));
-		return -1;
-	}
-	while (alen--)
-		if (tx_byte((addr >> (alen * 8)) & 0xff) ||
-		    (readw(I2C_BASE + I2SR) & I2SR_RX_NO_AK)) {
-			debug("%s:device address cycle fail(%x)\n",
-			       __func__, readw(I2C_BASE + I2SR));
-			return -1;
-		}
-	return 0;
+	return ret;
 }
 
-int i2c_read(uchar chip, uint addr, int alen, uchar *buf, int len)
+/*
+ * Try if a chip add given address responds (probe the chip)
+ */
+int i2c_probe(uchar chip)
 {
-	int timeout = I2C_MAX_TIMEOUT;
 	int ret;
 
-	debug("%s chip: 0x%02x addr: 0x%04x alen: %d len: %d\n",
-		__func__, chip, addr, alen, len);
+	ret = i2c_imx_start();
+	if (ret)
+		return ret;
 
-	if (i2c_addr(chip, addr, alen)) {
-		printf("i2c_addr failed\n");
-		return -1;
-	}
+	ret = i2c_imx_set_chip_addr(chip, 0);
+	if (ret)
+		return ret;
 
-	writew(I2CR_IEN | I2CR_MSTA | I2CR_MTX | I2CR_RSTA, I2C_BASE + I2CR);
+	i2c_imx_stop();
 
-	if (tx_byte(chip << 1 | 1))
-		return -1;
+	return ret;
+}
 
-	writew(I2CR_IEN | I2CR_MSTA |
-		((len == 1) ? I2CR_TX_NO_AK : 0),
-		I2C_BASE + I2CR);
+/*
+ * Read data from I2C device
+ */
+int i2c_read(uchar chip, uint addr, int alen, uchar *buf, int len)
+{
+	struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;
+	int ret;
+	unsigned int temp;
+	int i;
 
-	ret = readw(I2C_BASE + I2DR);
+	ret = i2c_imx_start();
+	if (ret)
+		return ret;
+
+	/* write slave address */
+	ret = i2c_imx_set_chip_addr(chip, 0);
+	if (ret)
+		return ret;
+
+	ret = i2c_imx_set_reg_addr(addr, alen);
+	if (ret)
+		return ret;
+
+	temp = readb(&i2c_regs->i2cr);
+	temp |= I2CR_RSTA;
+	writeb(temp, &i2c_regs->i2cr);
+
+	ret = i2c_imx_set_chip_addr(chip, 1);
+	if (ret)
+		return ret;
+
+	/* setup bus to read data */
+	temp = readb(&i2c_regs->i2cr);
+	temp &= ~I2CR_MTX;
+	if (len == 1)
+		temp &= ~I2CR_TX_NO_AK;
+	writeb(temp, &i2c_regs->i2cr);
+	readb(&i2c_regs->i2dr);
+
+	/* read data */
+	for (i = 0; i < len; i++) {
+		ret = i2c_imx_trx_complete();
+		if (ret)
+			return ret;
+
+		/*
+		 * It must generate STOP before read I2DR to prevent
+		 * controller from generating another clock cycle
+		 */
+		if (i == (len - 1)) {
+			temp = readb(&i2c_regs->i2cr);
+			temp &= ~(I2CR_MSTA | I2CR_MTX);
+			writeb(temp, &i2c_regs->i2cr);
+			i2c_imx_bus_busy(0);
+		} else if (i == (len - 2)) {
+			temp = readb(&i2c_regs->i2cr);
+			temp |= I2CR_TX_NO_AK;
+			writeb(temp, &i2c_regs->i2cr);
+		}
 
-	while (len--) {
-		ret = rx_byte(len == 0);
-		if (ret  < 0)
-			return -1;
-		*buf++ = ret;
-		if (len <= 1)
-			writew(I2CR_IEN | I2CR_MSTA |
-				I2CR_TX_NO_AK,
-				I2C_BASE + I2CR);
+		buf[i] = readb(&i2c_regs->i2dr);
 	}
 
-	writew(I2CR_IEN, I2C_BASE + I2CR);
-
-	while (readw(I2C_BASE + I2SR) & I2SR_IBB && --timeout)
-		udelay(1);
+	i2c_imx_stop();
 
-	return 0;
+	return ret;
 }
 
+/*
+ * Write data to I2C device
+ */
 int i2c_write(uchar chip, uint addr, int alen, uchar *buf, int len)
 {
-	int timeout = I2C_MAX_TIMEOUT;
-	debug("%s chip: 0x%02x addr: 0x%04x alen: %d len: %d\n",
-		__func__, chip, addr, alen, len);
+	struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;
+	int ret;
+	unsigned int temp;
+	int i;
 
-	if (i2c_addr(chip, addr, alen))
-		return -1;
+	ret = i2c_imx_start();
+	if (ret)
+		return ret;
 
-	while (len--)
-		if (tx_byte(*buf++))
-			return -1;
+	/* write slave address */
+	ret = i2c_imx_set_chip_addr(chip, 0);
+	if (ret)
+		return ret;
 
-	writew(I2CR_IEN, I2C_BASE + I2CR);
+	ret = i2c_imx_set_reg_addr(addr, alen);
+	if (ret)
+		return ret;
 
-	while (readw(I2C_BASE + I2SR) & I2SR_IBB && --timeout)
-		udelay(1);
+	for (i = 0; i < len; i++) {
+		writeb(buf[i], &i2c_regs->i2dr);
 
-	return 0;
-}
+		ret = i2c_imx_trx_complete();
+		if (ret)
+			return ret;
+
+		ret = i2c_imx_acked();
+		if (ret)
+			return ret;
+	}
 
+	i2c_imx_stop();
+
+	return ret;
+}
 #endif /* CONFIG_HARD_I2C */