Message ID | 1310594283-19819-1-git-send-email-marek.vasut@gmail.com |
---|---|
State | Superseded |
Headers | show |
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
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,
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,
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,
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
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 >
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
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
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
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
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 >
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.
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 >
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 --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 */
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