I2C troubles with Au1550

I2C troubles with Au1550

[Clem Taylor]

We've been having troubles with the reliability of the I2C interface
on the Au1550.  Basically 1% of the I2C transactions would timeout.
Sometimes the failures would cluster in long runs which was causing
pain.

Last night I got annoyed with the problem enough that I applied a
portion of a Au1200 I2C patch posted by Jordan Crouse on 2005.12.02.
This patch was not applied to the the linux-mips tree (as of
2.6.16.16) but it seems to have fixed our timeout problems. I ran a
I2C test for 14 hours doing constant I2C transactions from user space
and did not see an error.

Maybe Jordan could try again with a fresh patch because it really does
seem to help...

This is the subset of the patch I used:
--- drivers/i2c/busses/i2c-au1550.c	(revision 2271)
+++ drivers/i2c/busses/i2c-au1550.c	(working copy)
@@ -118,13 +118,19 @@

  	/* Reset the FIFOs, clear events.
 	*/
-	sp->psc_smbpcr = PSC_SMBPCR_DC;
+	stat = sp->psc_smbstat;
  	sp->psc_smbevnt = PSC_SMBEVNT_ALLCLR;
 	au_sync();
-	do {
-		stat = sp->psc_smbpcr;
+
+	if (!(stat & PSC_SMBSTAT_TE) || !(stat & PSC_SMBSTAT_RE)) {
+		sp->psc_smbpcr = PSC_SMBPCR_DC;
 		au_sync();
-	} while ((stat & PSC_SMBPCR_DC) != 0);
+		do {
+			stat = sp->psc_smbpcr;
+			au_sync();
+		} while ((stat & PSC_SMBPCR_DC) != 0);
+		udelay(50);
+	}

 	/* Write out the i2c chip address and specify operation
 	*/

Re: I2C troubles with Au1550

[Jordan Crouse]

[-- Attachment #1: Type: text/plain, Size: 303 bytes --]

On 18/05/06 17:54 -0400, Clem Taylor wrote:
> Maybe Jordan could try again with a fresh patch because it really does
> seem to help...
 
Here you go, fresh out of the oven.. :)

Jordan

---
Jordan Crouse
Senior Linux Engineer
AMD - Personal Connectivity Solutions Group
<www.amd.com/embeddedprocessors>

[-- Attachment #2: i2c.patch --]
[-- Type: text/plain, Size: 4898 bytes --]

ALCHEMY:  AU1200 I2C modifications

From: Jordan Crouse <jordan.crouse@amd.com>

Modifications to the existing AU1XXX I2C controller for the Au1200.

Signed-off-by: Jordan Crouse <jordan.crouse@amd.com>
---

 arch/mips/au1000/db1x00/board_setup.c     |   37 +++++++++++++++++++++++++++++
 drivers/i2c/busses/Kconfig                |    2 +-
 drivers/i2c/busses/i2c-au1550.c           |   29 ++++++++++++++++++-----
 include/asm-mips/mach-au1x00/au1xxx_psc.h |    7 +++++
 4 files changed, 67 insertions(+), 8 deletions(-)

diff --git a/arch/mips/au1000/db1x00/board_setup.c b/arch/mips/au1000/db1x00/board_setup.c
index f00ec3b..a2638c8 100644
--- a/arch/mips/au1000/db1x00/board_setup.c
+++ b/arch/mips/au1000/db1x00/board_setup.c
@@ -76,6 +76,43 @@ #if defined(CONFIG_IRDA) && (defined(CON
 #endif
 	bcsr->pcmcia = 0x0000; /* turn off PCMCIA power */
 
+#if defined(CONFIG_I2C_AU1550) && defined(CONFIG_MIPS_DB1200)
+	{
+	u32 freq0, clksrc;
+
+	/* Select SMBUS in CPLD */
+	bcsr->resets &= ~(BCSR_RESETS_PCS0MUX);
+
+	pin_func = au_readl(SYS_PINFUNC);
+	au_sync();
+	pin_func &= ~(3<<17 | 1<<4);
+	/* Set GPIOs correctly */
+	pin_func |= 2<<17;
+	au_writel(pin_func, SYS_PINFUNC);
+	au_sync();
+
+	/* The i2c driver depends on 50Mhz clock */
+	freq0 = au_readl(SYS_FREQCTRL0);
+	au_sync();
+	freq0 &= ~(SYS_FC_FRDIV1_MASK | SYS_FC_FS1 | SYS_FC_FE1);
+	freq0 |= (3<<SYS_FC_FRDIV1_BIT);
+	/* 396Mhz / (3+1)*2 == 49.5Mhz */
+	au_writel(freq0, SYS_FREQCTRL0);
+	au_sync();
+	freq0 |= SYS_FC_FE1;
+	au_writel(freq0, SYS_FREQCTRL0);
+	au_sync();
+
+	clksrc = au_readl(SYS_CLKSRC);
+	au_sync();
+	clksrc &= ~0x01f00000;
+	/* bit 22 is EXTCLK0 for PSC0 */
+	clksrc |= (0x3 << 22);
+	au_writel(clksrc, SYS_CLKSRC);
+	au_sync();
+	}
+#endif
+
 #ifdef CONFIG_MIPS_MIRAGE
 	/* enable GPIO[31:0] inputs */
 	au_writel(0, SYS_PININPUTEN);
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index d6d4494..e023563 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -76,7 +76,7 @@ config I2C_AMD8111
 
 config I2C_AU1550
 	tristate "Au1550 SMBus interface"
-	depends on I2C && SOC_AU1550
+	depends on I2C && (SOC_AU1550 || SOC_AU1200)
 	help
 	  If you say yes to this option, support will be included for the
 	  Au1550 SMBus interface.
diff --git a/drivers/i2c/busses/i2c-au1550.c b/drivers/i2c/busses/i2c-au1550.c
index d06edce..4508629 100644
--- a/drivers/i2c/busses/i2c-au1550.c
+++ b/drivers/i2c/busses/i2c-au1550.c
@@ -35,7 +35,15 @@ #include <linux/errno.h>
 #include <linux/i2c.h>
 
 #include <asm/mach-au1x00/au1000.h>
-#include <asm/mach-pb1x00/pb1550.h>
+#if defined(CONFIG_MIPS_PB1550)
+ #include <asm/mach-pb1x00/pb1550.h>
+#endif
+#if defined(CONFIG_MIPS_PB1200)
+ #include <asm/mach-pb1x00/pb1200.h>
+#endif
+#if defined(CONFIG_MIPS_DB1200)
+ #include <asm/mach-db1x00/db1200.h>
+#endif
 #include <asm/mach-au1x00/au1xxx_psc.h>
 
 #include "i2c-au1550.h"
@@ -118,13 +126,20 @@ do_address(struct i2c_au1550_data *adap,
 
 	/* Reset the FIFOs, clear events.
 	*/
-	sp->psc_smbpcr = PSC_SMBPCR_DC;
+	stat = sp->psc_smbstat;
 	sp->psc_smbevnt = PSC_SMBEVNT_ALLCLR;
 	au_sync();
-	do {
-		stat = sp->psc_smbpcr;
+
+	if (!(stat & PSC_SMBSTAT_TE) || !(stat & PSC_SMBSTAT_RE))
+	{
+		sp->psc_smbpcr = PSC_SMBPCR_DC;
 		au_sync();
-	} while ((stat & PSC_SMBPCR_DC) != 0);
+		do {
+			stat = sp->psc_smbpcr;
+			au_sync();
+		} while ((stat & PSC_SMBPCR_DC) != 0);
+		udelay(50);
+	}
 
 	/* Write out the i2c chip address and specify operation
 	*/
@@ -367,7 +382,7 @@ static struct i2c_au1550_data pb1550_i2c
 	SMBUS_PSC_BASE, 200, 200
 };
 
-static struct i2c_adapter pb1550_board_adapter = {
+struct i2c_adapter pb1550_board_adapter = {
 	name:              "pb1550 adapter",
 	id:                I2C_HW_AU1550_PSC,
 	algo:              NULL,
@@ -376,6 +391,8 @@ static struct i2c_adapter pb1550_board_a
 	client_unregister: pb1550_unreg,
 };
 
+EXPORT_SYMBOL(pb1550_board_adapter);
+
 /* BIG hack to support the control interface on the Wolfson WM8731
  * audio codec on the Pb1550 board.  We get an address and two data
  * bytes to write, create an i2c message, and send it across the
diff --git a/include/asm-mips/mach-au1x00/au1xxx_psc.h b/include/asm-mips/mach-au1x00/au1xxx_psc.h
index faa5ffe..a523079 100644
--- a/include/asm-mips/mach-au1x00/au1xxx_psc.h
+++ b/include/asm-mips/mach-au1x00/au1xxx_psc.h
@@ -48,6 +48,11 @@ #define PSC0_BASE_ADDR		0xb1a00000
 #define PSC1_BASE_ADDR		0xb1b00000
 #endif
 
+#ifdef CONFIG_SOC_AU1200
+#define PSC0_BASE_ADDR         0xb1a00000
+#define PSC1_BASE_ADDR         0xb1b00000
+#endif
+
 /* The PSC select and control registers are common to
  * all protocols.
  */
@@ -513,7 +518,7 @@ #define PSC_SMBEVNT_ALLCLR	(PSC_SMBEVNT_
 
 /* Transmit register control.
 */
-#define PSC_SMBTXRX_RSR		(1 << 30)
+#define PSC_SMBTXRX_RSR		(1 << 28)
 #define PSC_SMBTXRX_STP		(1 << 29)
 #define PSC_SMBTXRX_DATAMASK	(0xff)
 

Re: I2C troubles with Au1550

[Sergei Shtylyov]

Hello.

Jordan Crouse wrote:
>>Maybe Jordan could try again with a fresh patch because it really does
>>seem to help...

> Here you go, fresh out of the oven.. :)

> ------------------------------------------------------------------------
> 
> ALCHEMY:  AU1200 I2C modifications
> 
> From: Jordan Crouse <jordan.crouse@amd.com>
> 
> Modifications to the existing AU1XXX I2C controller for the Au1200.
> 
> Signed-off-by: Jordan Crouse <jordan.crouse@amd.com>
> ---
> 
>  arch/mips/au1000/db1x00/board_setup.c     |   37 +++++++++++++++++++++++++++++
>  drivers/i2c/busses/Kconfig                |    2 +-
>  drivers/i2c/busses/i2c-au1550.c           |   29 ++++++++++++++++++-----
>  include/asm-mips/mach-au1x00/au1xxx_psc.h |    7 +++++
>  4 files changed, 67 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/mips/au1000/db1x00/board_setup.c b/arch/mips/au1000/db1x00/board_setup.c
> index f00ec3b..a2638c8 100644
> --- a/arch/mips/au1000/db1x00/board_setup.c
> +++ b/arch/mips/au1000/db1x00/board_setup.c
> @@ -76,6 +76,43 @@ #if defined(CONFIG_IRDA) && (defined(CON
>  #endif
>  	bcsr->pcmcia = 0x0000; /* turn off PCMCIA power */
>  
> +#if defined(CONFIG_I2C_AU1550) && defined(CONFIG_MIPS_DB1200)
> +	{
> +	u32 freq0, clksrc;
> +
> +	/* Select SMBUS in CPLD */
> +	bcsr->resets &= ~(BCSR_RESETS_PCS0MUX);
> +
> +	pin_func = au_readl(SYS_PINFUNC);
> +	au_sync();
> +	pin_func &= ~(3<<17 | 1<<4);
> +	/* Set GPIOs correctly */
> +	pin_func |= 2<<17;
> +	au_writel(pin_func, SYS_PINFUNC);
> +	au_sync();
> +
> +	/* The i2c driver depends on 50Mhz clock */
> +	freq0 = au_readl(SYS_FREQCTRL0);
> +	au_sync();
> +	freq0 &= ~(SYS_FC_FRDIV1_MASK | SYS_FC_FS1 | SYS_FC_FE1);
> +	freq0 |= (3<<SYS_FC_FRDIV1_BIT);
> +	/* 396Mhz / (3+1)*2 == 49.5Mhz */
> +	au_writel(freq0, SYS_FREQCTRL0);
> +	au_sync();
> +	freq0 |= SYS_FC_FE1;
> +	au_writel(freq0, SYS_FREQCTRL0);
> +	au_sync();
> +
> +	clksrc = au_readl(SYS_CLKSRC);
> +	au_sync();
> +	clksrc &= ~0x01f00000;
> +	/* bit 22 is EXTCLK0 for PSC0 */
> +	clksrc |= (0x3 << 22);
> +	au_writel(clksrc, SYS_CLKSRC);
> +	au_sync();
> +	}
> +#endif
> +
>  #ifdef CONFIG_MIPS_MIRAGE
>  	/* enable GPIO[31:0] inputs */
>  	au_writel(0, SYS_PININPUTEN);

    Alas, I have to NAK this. DBAu1200 code should be in 
arch/mips/au1000/pb1200/...

> diff --git a/include/asm-mips/mach-au1x00/au1xxx_psc.h b/include/asm-mips/mach-au1x00/au1xxx_psc.h
> index faa5ffe..a523079 100644
> --- a/include/asm-mips/mach-au1x00/au1xxx_psc.h
> +++ b/include/asm-mips/mach-au1x00/au1xxx_psc.h
> @@ -48,6 +48,11 @@ #define PSC0_BASE_ADDR		0xb1a00000
>  #define PSC1_BASE_ADDR		0xb1b00000
>  #endif
>  
> +#ifdef CONFIG_SOC_AU1200
> +#define PSC0_BASE_ADDR         0xb1a00000
> +#define PSC1_BASE_ADDR         0xb1b00000
> +#endif
> +
>  /* The PSC select and control registers are common to
>   * all protocols.
>   */

    Well, I have already submitted this change this March, here's the patch:

http://www.linux-mips.org/archives/linux-mips/2006-03/msg00245.html

  . As usual, Ralf is not eager to commit my patches... :-/
    Thou wait... that hunk won't even aplly to the current git tree... Looks 
like this patch is trying to redeclare PSC base addresses for Au1200 on top of 
my patch... If it's so, NAK.

WBR, Sergei

Re: I2C troubles with Au1550

[Sergei Shtylyov]

Hello.

Jordan Crouse wrote:
> On 18/05/06 17:54 -0400, Clem Taylor wrote:
> 
>>Maybe Jordan could try again with a fresh patch because it really does
>>seem to help...
> 
>  
> Here you go, fresh out of the oven.. :)

[...]

> --- a/drivers/i2c/busses/i2c-au1550.c
> +++ b/drivers/i2c/busses/i2c-au1550.c
> @@ -35,7 +35,15 @@ #include <linux/errno.h>
>  #include <linux/i2c.h>
>  
>  #include <asm/mach-au1x00/au1000.h>
> -#include <asm/mach-pb1x00/pb1550.h>
> +#if defined(CONFIG_MIPS_PB1550)
> + #include <asm/mach-pb1x00/pb1550.h>
> +#endif
> +#if defined(CONFIG_MIPS_PB1200)
> + #include <asm/mach-pb1x00/pb1200.h>
> +#endif
> +#if defined(CONFIG_MIPS_DB1200)
> + #include <asm/mach-db1x00/db1200.h>
> +#endif

    Instead of all this, just #include <asm/mach-au1x00/au1xxx.h>

WBR, Sergei

Re: I2C troubles with Au1550

[Jordan Crouse]

On 19/05/06 18:48 +0400, Sergei Shtylyov wrote:
 
>    Alas, I have to NAK this. DBAu1200 code should be in 
> arch/mips/au1000/pb1200/...

if this was DB1200 code only, I would be inclined to agree, but its
not - so this code is well placed.

>    Thou wait... that hunk won't even aplly to the current git tree... 

Well, it does apply - latest GIT tree, right from l-m.o

> Looks like this patch is trying to redeclare PSC base addresses for Au1200 

Yeah, it does a redundant declaration - I'll pull that part of it.  The
rest of the patch is still valid though - I see no reason why you should NAK
it, especially when this was posted by popular request.

Jordan

Re: I2C troubles with Au1550

[Ralf Baechle]

On Fri, May 19, 2006 at 06:48:30PM +0400, Sergei Shtylyov wrote:

> http://www.linux-mips.org/archives/linux-mips/2006-03/msg00245.html
> 
>  . As usual, Ralf is not eager to commit my patches... :-/

It has not always be obvious which of the patches to apply ...

  Ralf

Re: I2C troubles with Au1550

[Sergei Shtylyov]

Jordan Crouse wrote:
> On 19/05/06 18:48 +0400, Sergei Shtylyov wrote:
>  
> 
>>   Alas, I have to NAK this. DBAu1200 code should be in 
>>arch/mips/au1000/pb1200/...

> if this was DB1200 code only, I would be inclined to agree, but its
> not - so this code is well placed.

    It's under #ifdef CONFIG_MIPS_DB1200, so is *completely* misplaced. This 
file is not even compiled for DBAu1200. Therefore, this code will never execute.

>>   Thou wait... that hunk won't even aplly to the current git tree... 

> Well, it does apply - latest GIT tree, right from l-m.o

    Hmm, indeed it applies with fuzz (because of PSC redefinitions)... So, I'm 
taking this back. :-)

>>Looks like this patch is trying to redeclare PSC base addresses for Au1200 

> Yeah, it does a redundant declaration - I'll pull that part of it.  The
> rest of the patch is still valid though - I see no reason why you should NAK
> it, especially when this was posted by popular request.

    Because I caught a defect in it with naked eye. Maybe NAKing it was indeed 
too much. :-)

> Jordan

MBR, Sergei