diff mbox

[U-Boot,V3+] I2C: mxc_i2c rework

Message ID 1310594283-19819-1-git-send-email-marek.vasut@gmail.com
State Superseded
Headers show

Commit Message

Marek Vasut July 13, 2011, 9:58 p.m. UTC
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 |  424 +++++++++++++++++++++++++++++++++----------------
 1 files changed, 290 insertions(+), 134 deletions(-)

V2: Convert register access to struct mxc_i2c_regs.

V3: Update licensing info

V3+: Add commit ID into commit message

Comments

Heiko Schocher July 14, 2011, 9:04 a.m. UTC | #1
Hello Marek,

Marek Vasut 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 ;-)
> 
> 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 |  424 +++++++++++++++++++++++++++++++++----------------
>  1 files changed, 290 insertions(+), 134 deletions(-)
> 
> V2: Convert register access to struct mxc_i2c_regs.
> 
> V3: Update licensing info
> 
> V3+: Add commit ID into commit message

checkpatch says:

ERROR: trailing statements should be on next line
#143: FILE: drivers/i2c/mxc_i2c.c:130:
+               for (i = 0; i2c_clk_div[i][0] < div; i++);

total: 1 errors, 0 warnings, 526 lines checked

Can you fix this?

> diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c
> index 89d1973..03e2448 100644
> --- a/drivers/i2c/mxc_i2c.c
> +++ b/drivers/i2c/mxc_i2c.c
[...]
> @@ -68,218 +78,364 @@
>  #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
> + *
> + * FIXME: remove #ifdefs !
> + */

As Stefano just posted a patch for this, see here:

http://patchwork.ozlabs.org/patch/104642/

Can you fix this please?

Thanks!

> +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;
>  
> +	/* Divider value calculation */
>  #if defined(CONFIG_MX31)
>  	struct clock_control_regs *sc_regs =
>  		(struct clock_control_regs *)CCM_BASE;
>  
> -	freq = mx31_get_ipg_clk();
> +	i2c_clk_rate = 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);
> +	i2c_clk_rate = mxc_get_clock(MXC_IPG_CLK);
>  #endif
[...]

bye,
Heiko
Albert ARIBAUD July 14, 2011, 1:55 p.m. UTC | #2
Hi Marek,

Le 13/07/2011 23:58, Marek Vasut a écrit :

> V3+: Add commit ID into commit message

What prevents a simple V4 here?

Amicalement,
Marek Vasut July 14, 2011, 2:35 p.m. UTC | #3
On Thursday, July 14, 2011 03:55:03 PM Albert ARIBAUD wrote:
> Hi Marek,
> 
> Le 13/07/2011 23:58, Marek Vasut a écrit :
> > V3+: Add commit ID into commit message
> 
> What prevents a simple V4 here?

No change in code ... but I suspect there'll be V4 anyway.
> 
> Amicalement,
Albert ARIBAUD July 14, 2011, 2:45 p.m. UTC | #4
Hi again Marek,

Le 14/07/2011 16:35, Marek Vasut a écrit :
> On Thursday, July 14, 2011 03:55:03 PM Albert ARIBAUD wrote:
>> Hi Marek,
>>
>> Le 13/07/2011 23:58, Marek Vasut a écrit :
>>> V3+: Add commit ID into commit message
>>
>> What prevents a simple V4 here?
>
> No change in code ... but I suspect there'll be V4 anyway.

Introducing variations in patch numbering, especially non-numeric 
variations of what is expected to be a number, is IMO useful only for 
testing how resilient patch processing tools can be. :)

Amicalement,
Wolfgang Denk July 28, 2011, 2:29 p.m. UTC | #5
Dear Marek,

In message <4E1EB127.3040505@denx.de> Heiko wrote:
> Hello Marek,
> 
> Marek Vasut 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 ;-)
> > 
> > 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 |  424 +++++++++++++++++++++++++++++++++----------------
> >  1 files changed, 290 insertions(+), 134 deletions(-)
> > 
> > V2: Convert register access to struct mxc_i2c_regs.
> > 
> > V3: Update licensing info
> > 
> > V3+: Add commit ID into commit message
> 
> checkpatch says:
> 
> ERROR: trailing statements should be on next line
> #143: FILE: drivers/i2c/mxc_i2c.c:130:
> +               for (i = 0; i2c_clk_div[i][0] < div; i++);
> 
> total: 1 errors, 0 warnings, 526 lines checked
> 
> Can you fix this?

Are you going to send a fixed version any time soon?


Best regards,

Wolfgang Denk
Jason Liu July 29, 2011, 6:55 a.m. UTC | #6
Hi, Marek,

On Thu, Jul 14, 2011 at 5:58 AM, 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 ;-)
>

why you change i2c clock from IPG_PERCLK to IPG_CLK?

[...]
> +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;
>
> +       /* Divider value calculation */
>  #if defined(CONFIG_MX31)
>        struct clock_control_regs *sc_regs =
>                (struct clock_control_regs *)CCM_BASE;
>
> -       freq = mx31_get_ipg_clk();
> +       i2c_clk_rate = 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);
> +       i2c_clk_rate = mxc_get_clock(MXC_IPG_CLK);
>  #endif

There are two clocks for i2c:

Peripheral clock (IPBus): source from  ipg_clk_root, which is for IP
bus register read/write.
Block clock: source from perclk_root, which is I2C function clock.

We need get perclk not ipg clock, right?

BTW, do you test this driver on mx53?

Jason
>
Marek Vasut July 29, 2011, 9:35 a.m. UTC | #7
On Friday, July 29, 2011 08:55:14 AM Jason Hui wrote:
> Hi, Marek,
> 
> On Thu, Jul 14, 2011 at 5:58 AM, 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 ;-)
> 
> why you change i2c clock from IPG_PERCLK to IPG_CLK?

On MX51, PERCLK are those fast (680MHz) clock, that's not source of clock for 
I2C. The IPG_CLK (they are 68.5MHz iirc) are source for the I2C. Also, I 
discussed this with Stefano and we agreed this is likely a bug.
> 
> [...]
> 
> > +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;
> > 
> > +       /* Divider value calculation */
> >  #if defined(CONFIG_MX31)
> >        struct clock_control_regs *sc_regs =
> >                (struct clock_control_regs *)CCM_BASE;
> > 
> > -       freq = mx31_get_ipg_clk();
> > +       i2c_clk_rate = 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);
> > +       i2c_clk_rate = mxc_get_clock(MXC_IPG_CLK);
> >  #endif
> 
> There are two clocks for i2c:
> 
> Peripheral clock (IPBus): source from  ipg_clk_root, which is for IP
> bus register read/write.
> Block clock: source from perclk_root, which is I2C function clock.
> 
> We need get perclk not ipg clock, right?

For divider, we need those slower ones, the IPG_CLK, not PERCLK.
> 
> BTW, do you test this driver on mx53?

No, I don't have one.

> 
> Jason
Stefano Babic July 29, 2011, 9:42 a.m. UTC | #8
On 07/29/2011 11:35 AM, Marek Vasut wrote:
>>
>> why you change i2c clock from IPG_PERCLK to IPG_CLK?
> 
> On MX51, PERCLK are those fast (680MHz) clock, that's not source of clock for 
> I2C. The IPG_CLK (they are 68.5MHz iirc) are source for the I2C. Also, I 
> discussed this with Stefano and we agreed this is likely a bug.

Well, this code must be suitable for all i.MX processor. If we have a
different source for i.MX51 and i.MX53, we can hide this adding an entry
to mxc_get_clock(), such as mxc_get_clock(MXC_I2C_CLK). the right clock
is then chosen in the specific mxc_get_clock function.

Best regards,
Stefano Babic
Stefano Babic Aug. 30, 2011, 10:48 a.m. UTC | #9
On 07/13/2011 11:58 PM, Marek Vasut 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 ;-)

Hi Marek,

as it seems we need a while to fix all issues with MX53, I would prefer
to fix the MX31, or some boards are broken (mx31phycore). I will send a
patch *only* to fix this issue, that means to get the clock via
mxc_get_clock() for MX31. This was also a comment in your patch. Please
then base your new version on the fix I will send.

Thanks,
Stefano
Marek Vasut Sept. 14, 2011, 7:39 p.m. UTC | #10
On Friday, July 29, 2011 08:55:14 AM Jason Hui wrote:
> Hi, Marek,
> 
> On Thu, Jul 14, 2011 at 5:58 AM, 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 ;-)
> 
> why you change i2c clock from IPG_PERCLK to IPG_CLK?
> 
> [...]

Ok, I investigated a bit deeper and I suspect the clock.c is the culprit.

Apparently, the PERCLK doesn't run at the frequency the clock.c reports it runs 
on. Therefore, the i2c miscalculates the divider etc -- falling crap model 
(waterfall model).

Anyway, Jason, can you look into that clock problem? I think there are more than 
just perclk miscalculated.

Cheers
Jason Liu Sept. 15, 2011, 1:43 a.m. UTC | #11
On Thu, Sep 15, 2011 at 3:39 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On Friday, July 29, 2011 08:55:14 AM Jason Hui wrote:
>> Hi, Marek,
>>
>> On Thu, Jul 14, 2011 at 5:58 AM, 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 ;-)
>>
>> why you change i2c clock from IPG_PERCLK to IPG_CLK?
>>
>> [...]
>
> Ok, I investigated a bit deeper and I suspect the clock.c is the culprit.
>
> Apparently, the PERCLK doesn't run at the frequency the clock.c reports it runs
> on. Therefore, the i2c miscalculates the divider etc -- falling crap model
> (waterfall model).

But apparently, the i2c function clock should be IPG_PERCLK not IPG
clock. And Linux also fix it already.

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>

>
> Anyway, Jason, can you look into that clock problem? I think there are more than
> just perclk miscalculated.

OK, I will check that part.

Jason

>
> Cheers
>
Marek Vasut Sept. 15, 2011, 2:07 a.m. UTC | #12
On Thursday, September 15, 2011 03:43:42 AM Jason Hui wrote:
> On Thu, Sep 15, 2011 at 3:39 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
> > On Friday, July 29, 2011 08:55:14 AM Jason Hui wrote:
> >> Hi, Marek,
> >> 
> >> On Thu, Jul 14, 2011 at 5:58 AM, 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 ;-)
> >> 
> >> why you change i2c clock from IPG_PERCLK to IPG_CLK?
> >> 
> >> [...]
> > 
> > Ok, I investigated a bit deeper and I suspect the clock.c is the culprit.
> > 
> > Apparently, the PERCLK doesn't run at the frequency the clock.c reports
> > it runs on. Therefore, the i2c miscalculates the divider etc -- falling
> > crap model (waterfall model).
> 
> But apparently, the i2c function clock should be IPG_PERCLK not IPG
> clock. And Linux also fix it already.

Then there's bulls**t in your mx51 and mx53 datasheet or what ? besides, PERCLK 
is faster than IPGCLK on MX51 so it makes even less sense! Can you please talk 
to the HW guys or whatever to clear this once and for all ? I smell noone really 
knows where the clock are sourced from and all this crap is just blind guessing.
Jason Liu Sept. 15, 2011, 2:26 a.m. UTC | #13
On Thu, Sep 15, 2011 at 10:07 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On Thursday, September 15, 2011 03:43:42 AM Jason Hui wrote:
>> On Thu, Sep 15, 2011 at 3:39 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
>> > On Friday, July 29, 2011 08:55:14 AM Jason Hui wrote:
>> >> Hi, Marek,
>> >>
>> >> On Thu, Jul 14, 2011 at 5:58 AM, 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 ;-)
>> >>
>> >> why you change i2c clock from IPG_PERCLK to IPG_CLK?
>> >>
>> >> [...]
>> >
>> > Ok, I investigated a bit deeper and I suspect the clock.c is the culprit.
>> >
>> > Apparently, the PERCLK doesn't run at the frequency the clock.c reports
>> > it runs on. Therefore, the i2c miscalculates the divider etc -- falling
>> > crap model (waterfall model).
>>
>> But apparently, the i2c function clock should be IPG_PERCLK not IPG
>> clock. And Linux also fix it already.
>
> Then there's bulls**t in your mx51 and mx53 datasheet or what ?

Please refer to MCIMX51RM.PDF, page 305,
Table 7-41. PERCLK-dependent Module Clock Sources
PERCLK-dependent Module Clocks Associated CCGR Register
uart1_perclk
CCGR1
uart2_perclk
uart3_perclk
i2c1 clocks
i2c2 clocks
epit1_highfreq
CCGR2
epit2_highfreq
pwm1_highfreq
pwm2_highfreq
gpt_highfreq
owire clocks

> besides, PERCLK
> is faster than IPGCLK on MX51 so it makes even less sense!

I don't think PERCLK is always faster than IPG clock, it's configurable.
please refer to MCIMX51RM.PDF, page 307.

>Can you please talk
> to the HW guys or whatever to clear this once and for all ? I smell noone really
> knows where the clock are sourced from and all this crap is just blind guessing.

I have asked the IC module owner again. It confirms that I2C function
clock is ipg_perclk.

Jason

>
Marek Vasut Sept. 15, 2011, 4:07 a.m. UTC | #14
On Thursday, September 15, 2011 04:26:17 AM Jason Hui wrote:
> On Thu, Sep 15, 2011 at 10:07 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
> > On Thursday, September 15, 2011 03:43:42 AM Jason Hui wrote:
> >> On Thu, Sep 15, 2011 at 3:39 AM, Marek Vasut <marek.vasut@gmail.com> wrote:
> >> > On Friday, July 29, 2011 08:55:14 AM Jason Hui wrote:
> >> >> Hi, Marek,
> >> >> 
> >> >> On Thu, Jul 14, 2011 at 5:58 AM, 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 ;-)
> >> >> 
> >> >> why you change i2c clock from IPG_PERCLK to IPG_CLK?
> >> >> 
> >> >> [...]
> >> > 
> >> > Ok, I investigated a bit deeper and I suspect the clock.c is the
> >> > culprit.
> >> > 
> >> > Apparently, the PERCLK doesn't run at the frequency the clock.c
> >> > reports it runs on. Therefore, the i2c miscalculates the divider etc
> >> > -- falling crap model (waterfall model).
> >> 
> >> But apparently, the i2c function clock should be IPG_PERCLK not IPG
> >> clock. And Linux also fix it already.
> > 
> > Then there's bulls**t in your mx51 and mx53 datasheet or what ?
> 
> Please refer to MCIMX51RM.PDF, page 305,
> Table 7-41. PERCLK-dependent Module Clock Sources
> PERCLK-dependent Module Clocks Associated CCGR Register
> uart1_perclk
> CCGR1
> uart2_perclk
> uart3_perclk
> i2c1 clocks
> i2c2 clocks
> epit1_highfreq
> CCGR2
> epit2_highfreq
> pwm1_highfreq
> pwm2_highfreq
> gpt_highfreq
> owire clocks

You see ... I'm starting to understand what is actually going wrong. The 
lowlevel_init.S is bloated with crap (why? why can't that be in cpu init C code 
?) and there is this one part, where CBCDR is overwritten with a configurable 
value instead of hardcoded value.

No documentation about that at all, but it's there ... and that's -- amongst 
other bugs -- my problem I assume. So I need to set this CONFIG_SYS_CLKTL_CBCDR 
to another magic value, now I get it.

> 
> > besides, PERCLK
> > is faster than IPGCLK on MX51 so it makes even less sense!
> 
> I don't think PERCLK is always faster than IPG clock, it's configurable.
> please refer to MCIMX51RM.PDF, page 307.

Yea ... it's configurable via some undocumented macro in assembler code. Damn.

Can you please comment on the other patches?

Thanks

> 
> >Can you please talk
> >
> > to the HW guys or whatever to clear this once and for all ? I smell noone
> > really knows where the clock are sourced from and all this crap is just
> > blind guessing.
> 
> I have asked the IC module owner again. It confirms that I2C function
> clock is ipg_perclk.
> 
> Jason
diff mbox

Patch

diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c
index 89d1973..03e2448 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,364 @@ 
 #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
+ *
+ * FIXME: remove #ifdefs !
+ */
+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;
 
+	/* Divider value calculation */
 #if defined(CONFIG_MX31)
 	struct clock_control_regs *sc_regs =
 		(struct clock_control_regs *)CCM_BASE;
 
-	freq = mx31_get_ipg_clk();
+	i2c_clk_rate = 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);
+	i2c_clk_rate = mxc_get_clock(MXC_IPG_CLK);
 #endif
+	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;
+}
 
-	for (i = 0; i < 0x1f; i++)
-		if (freq / div[i] <= speed)
-			break;
+/*
+ * Reset I2C Controller
+ */
+void i2c_reset(void)
+{
+	struct mxc_i2c_regs *i2c_regs = (struct mxc_i2c_regs *)I2C_BASE;
 
-	debug("%s: speed: %d\n", __func__, speed);
+	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 */