RE: [PATCH V3 2/2]OMAP: Disable internal I2C pull-ups in OMAP3630

RE: [PATCH V3 2/2]OMAP: Disable internal I2C pull-ups in OMAP3630

[Sonasath, Moiz]

Tony,

> -----Original Message-----
> From: Sonasath, Moiz
> Sent: Friday, February 26, 2010 1:15 PM
> To: linux-omap@vger.kernel.org
> Cc: tony@atomide.com; Sonasath, Moiz; Pais, Allen; Pandita, Vikram
> Subject: [PATCH V3 2/2]OMAP: Disable internal I2C pull-ups in OMAP3630
> 
> From: Moiz Sonasath <m-sonasath@ti.com>
> 
> This patch disables the newly introduced internal pull-up feature in
> OMAP3630, to use only the external HW resitor >= 470 Ohm for the
> assured functionality in HS mode.
> 
> While testing the I2C in High Speed mode, it was discovered that
> without a proper pull-up resitor, there is data corruption during
> multi-byte transfers. RTC(time_set) test case was used for testing.
> 
> From the analysis done, it was concluded that ideally we need a pull-up
> of 1.6 K Ohm (recomended) or atleast 470 Ohm or greater for assured
> performance in HS mode.
> 
> Signed-off-by: Moiz Sonasath <m-sonasath@ti.com>
> Signed-off-by: Allen Pais <allen.pais@ti.com>
> Signed-off-by: Vikram Pandita <vikram.pandita@ti.com>

I did resend this patch as per our last conversation, Can you please take this patch in?

> ---
>  arch/arm/mach-omap2/i2c.c                 |   24 ++++++++++++++++++++++++
>  arch/arm/plat-omap/include/plat/control.h |   14 ++++++++++++++
>  2 files changed, 38 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/i2c.c b/arch/arm/mach-omap2/i2c.c
> index 789ca8c..2e6eb28 100644
> --- a/arch/arm/mach-omap2/i2c.c
> +++ b/arch/arm/mach-omap2/i2c.c
> @@ -22,6 +22,7 @@
>  #include <plat/cpu.h>
>  #include <plat/i2c.h>
>  #include <plat/mux.h>
> +#include <plat/control.h>
> 
>  #include "mux.h"
> 
> @@ -52,5 +53,28 @@ int __init omap_register_i2c_bus(int bus_id, u32
> clkrate,
>  		omap_mux_init_signal(mux_name, OMAP_PIN_INPUT);
>  	}
> 
> +	/* Disable OMAP 3630 internal pull-ups for all I2Ci */
> +	if (cpu_is_omap3630() &&
> !(omap_ctrl_readl(OMAP343X_CONTROL_PROG_IO1) &
> OMAP3630_PRG_I2C1_PULLUPRESX)) {
> +
> +		u32 prog_io;
> +
> +		prog_io = omap_ctrl_readl(OMAP343X_CONTROL_PROG_IO1);
> +		/* Program (bit 19)=1 to disable internal pull-up on I2C1 */
> +		prog_io |= OMAP3630_PRG_I2C1_PULLUPRESX;
> +		/* Program (bit 0)=1 to disable internal pull-up on I2C2 */
> +		prog_io |= OMAP3630_PRG_I2C2_PULLUPRESX;
> +		omap_ctrl_writel(prog_io, OMAP343X_CONTROL_PROG_IO1);
> +
> +		prog_io = omap_ctrl_readl(OMAP36XX_CONTROL_PROG_IO2);
> +		/* Program (bit 7)=1 to disable internal pull-up on I2C3 */
> +		prog_io |= OMAP3630_PRG_I2C3_PULLUPRESX;
> +		omap_ctrl_writel(prog_io, OMAP36XX_CONTROL_PROG_IO2);
> +
> +		prog_io = omap_ctrl_readl(OMAP36XX_CONTROL_PROG_IO_WKUP1);
> +		/* Program (bit 5)=1 to disable internall pull-up on I2C4(SR)
> */
> +		prog_io |= OMAP3630_PRG_SR_PULLUPRESX;
> +		omap_ctrl_writel(prog_io, OMAP36XX_CONTROL_PROG_IO_WKUP1);
> +	}
> +
>  	return omap_plat_register_i2c_bus(bus_id, clkrate, info, len);
>  }
> diff --git a/arch/arm/plat-omap/include/plat/control.h b/arch/arm/plat-
> omap/include/plat/control.h
> index 2074473..9e58d8e 100644
> --- a/arch/arm/plat-omap/include/plat/control.h
> +++ b/arch/arm/plat-omap/include/plat/control.h
> @@ -169,6 +169,9 @@
>  #define AM35XX_CONTROL_IP_SW_RESET      (OMAP2_CONTROL_GENERAL + 0x0328)
>  #define AM35XX_CONTROL_IPSS_CLK_CTRL    (OMAP2_CONTROL_GENERAL + 0x032C)
> 
> +/* 36xx-only CONTROL_GENERAL register offsets */
> +#define OMAP36XX_CONTROL_PROG_IO2       (OMAP2_CONTROL_GENERAL + 0x0198)
> +
>  /* 34xx PADCONF register offsets */
>  #define OMAP343X_PADCONF_ETK(i)		(OMAP2_CONTROL_PADCONFS + 0x5a8
> + \
>  						(i)*2)
> @@ -200,6 +203,9 @@
>  #define OMAP343X_CONTROL_WKUP_DEBOBS3 (OMAP343X_CONTROL_GENERAL_WKUP +
> 0x014)
>  #define OMAP343X_CONTROL_WKUP_DEBOBS4 (OMAP343X_CONTROL_GENERAL_WKUP +
> 0x018)
> 
> +/* 36xx-only GENERAL_WKUP register offsets */
> +#define OMAP36XX_CONTROL_PROG_IO_WKUP1 (OMAP343X_CONTROL_GENERAL_WKUP +
> 0x020)
> +
>  /* 34xx D2D idle-related pins, handled by PM core */
>  #define OMAP3_PADCONF_SAD2D_MSTANDBY   0x250
>  #define OMAP3_PADCONF_SAD2D_IDLEACK    0x254
> @@ -250,6 +256,8 @@
>  #define OMAP2_PBIASLITEVMODE0		(1 << 0)
> 
>  /* CONTROL_PROG_IO1 bits */
> +#define OMAP3630_PRG_I2C2_PULLUPRESX    (1 << 0)
> +#define OMAP3630_PRG_I2C1_PULLUPRESX	(1 << 19)
>  #define OMAP3630_PRG_SDMMC1_SPEEDCTRL	(1 << 20)
> 
>  /* CONTROL_IVA2_BOOTMOD bits */
> @@ -257,6 +265,12 @@
>  #define OMAP3_IVA2_BOOTMOD_MASK		(0xf << 0)
>  #define OMAP3_IVA2_BOOTMOD_IDLE		(0x1 << 0)
> 
> +/* CONTROL_PROG_IO2 bits on omap3630 */
> +#define OMAP3630_PRG_I2C3_PULLUPRESX    (1 << 7)
> +
> +/* CONTROL_PROG_IO_WKUP1 bits on omap3630 */
> +#define OMAP3630_PRG_SR_PULLUPRESX    (1 << 5)
> +
>  /* CONTROL_PADCONF_X bits */
>  #define OMAP3_PADCONF_WAKEUPEVENT0	(1 << 15)
>  #define OMAP3_PADCONF_WAKEUPENABLE0	(1 << 14)
> --
> 1.5.6.3

Re: [PATCH V3 2/2]OMAP: Disable internal I2C pull-ups in OMAP3630

[Tony Lindgren]

Hi,

* Sonasath, Moiz <m-sonasath@ti.com> [100323 00:40]:
> Tony,
> 
> > -----Original Message-----
> > From: Sonasath, Moiz
> > Sent: Friday, February 26, 2010 1:15 PM
> > To: linux-omap@vger.kernel.org
> > Cc: tony@atomide.com; Sonasath, Moiz; Pais, Allen; Pandita, Vikram
> > Subject: [PATCH V3 2/2]OMAP: Disable internal I2C pull-ups in OMAP3630
> > 
> > From: Moiz Sonasath <m-sonasath@ti.com>
> > 
> > This patch disables the newly introduced internal pull-up feature in
> > OMAP3630, to use only the external HW resitor >= 470 Ohm for the
> > assured functionality in HS mode.
> > 
> > While testing the I2C in High Speed mode, it was discovered that
> > without a proper pull-up resitor, there is data corruption during
> > multi-byte transfers. RTC(time_set) test case was used for testing.
> > 
> > From the analysis done, it was concluded that ideally we need a pull-up
> > of 1.6 K Ohm (recomended) or atleast 470 Ohm or greater for assured
> > performance in HS mode.
> > 
> > Signed-off-by: Moiz Sonasath <m-sonasath@ti.com>
> > Signed-off-by: Allen Pais <allen.pais@ti.com>
> > Signed-off-by: Vikram Pandita <vikram.pandita@ti.com>
> 
> I did resend this patch as per our last conversation, Can you please take this patch in?

Sorry for the delay, here's some more info on this issue. So it looks
like starting with 3630 there are dedicated pull-up for all the I2C buses.
And the pull values are configurable with software.

Because of this, 3630 boards should have the mux register pull-ups disabled,
and only use the dedicated I2C pull-ups. In theory external pull-ups should
not be needed. The value configured for the dedicated I2C pull-ups depends
on the connected I2C device, and the number of devices.

So it looks like we should do the following changes:

- Disable mux register pull-ups on 3630 and later

- Allow setting the dedicated I2C pull-up values from board-*.c files
  for 3630 and later

- Warn if the dedicated pull-up values are not configured on 3630 and
  later

- Allow disabling the dedicated I2C pull-up values on 3630 and later
  in case external pull-up resistors are being used.

Comments?

Regards,

Tony

 
> > ---
> >  arch/arm/mach-omap2/i2c.c                 |   24 ++++++++++++++++++++++++
> >  arch/arm/plat-omap/include/plat/control.h |   14 ++++++++++++++
> >  2 files changed, 38 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/arm/mach-omap2/i2c.c b/arch/arm/mach-omap2/i2c.c
> > index 789ca8c..2e6eb28 100644
> > --- a/arch/arm/mach-omap2/i2c.c
> > +++ b/arch/arm/mach-omap2/i2c.c
> > @@ -22,6 +22,7 @@
> >  #include <plat/cpu.h>
> >  #include <plat/i2c.h>
> >  #include <plat/mux.h>
> > +#include <plat/control.h>
> > 
> >  #include "mux.h"
> > 
> > @@ -52,5 +53,28 @@ int __init omap_register_i2c_bus(int bus_id, u32
> > clkrate,
> >  		omap_mux_init_signal(mux_name, OMAP_PIN_INPUT);
> >  	}
> > 
> > +	/* Disable OMAP 3630 internal pull-ups for all I2Ci */
> > +	if (cpu_is_omap3630() &&
> > !(omap_ctrl_readl(OMAP343X_CONTROL_PROG_IO1) &
> > OMAP3630_PRG_I2C1_PULLUPRESX)) {
> > +
> > +		u32 prog_io;
> > +
> > +		prog_io = omap_ctrl_readl(OMAP343X_CONTROL_PROG_IO1);
> > +		/* Program (bit 19)=1 to disable internal pull-up on I2C1 */
> > +		prog_io |= OMAP3630_PRG_I2C1_PULLUPRESX;
> > +		/* Program (bit 0)=1 to disable internal pull-up on I2C2 */
> > +		prog_io |= OMAP3630_PRG_I2C2_PULLUPRESX;
> > +		omap_ctrl_writel(prog_io, OMAP343X_CONTROL_PROG_IO1);
> > +
> > +		prog_io = omap_ctrl_readl(OMAP36XX_CONTROL_PROG_IO2);
> > +		/* Program (bit 7)=1 to disable internal pull-up on I2C3 */
> > +		prog_io |= OMAP3630_PRG_I2C3_PULLUPRESX;
> > +		omap_ctrl_writel(prog_io, OMAP36XX_CONTROL_PROG_IO2);
> > +
> > +		prog_io = omap_ctrl_readl(OMAP36XX_CONTROL_PROG_IO_WKUP1);
> > +		/* Program (bit 5)=1 to disable internall pull-up on I2C4(SR)
> > */
> > +		prog_io |= OMAP3630_PRG_SR_PULLUPRESX;
> > +		omap_ctrl_writel(prog_io, OMAP36XX_CONTROL_PROG_IO_WKUP1);
> > +	}
> > +
> >  	return omap_plat_register_i2c_bus(bus_id, clkrate, info, len);
> >  }
> > diff --git a/arch/arm/plat-omap/include/plat/control.h b/arch/arm/plat-
> > omap/include/plat/control.h
> > index 2074473..9e58d8e 100644
> > --- a/arch/arm/plat-omap/include/plat/control.h
> > +++ b/arch/arm/plat-omap/include/plat/control.h
> > @@ -169,6 +169,9 @@
> >  #define AM35XX_CONTROL_IP_SW_RESET      (OMAP2_CONTROL_GENERAL + 0x0328)
> >  #define AM35XX_CONTROL_IPSS_CLK_CTRL    (OMAP2_CONTROL_GENERAL + 0x032C)
> > 
> > +/* 36xx-only CONTROL_GENERAL register offsets */
> > +#define OMAP36XX_CONTROL_PROG_IO2       (OMAP2_CONTROL_GENERAL + 0x0198)
> > +
> >  /* 34xx PADCONF register offsets */
> >  #define OMAP343X_PADCONF_ETK(i)		(OMAP2_CONTROL_PADCONFS + 0x5a8
> > + \
> >  						(i)*2)
> > @@ -200,6 +203,9 @@
> >  #define OMAP343X_CONTROL_WKUP_DEBOBS3 (OMAP343X_CONTROL_GENERAL_WKUP +
> > 0x014)
> >  #define OMAP343X_CONTROL_WKUP_DEBOBS4 (OMAP343X_CONTROL_GENERAL_WKUP +
> > 0x018)
> > 
> > +/* 36xx-only GENERAL_WKUP register offsets */
> > +#define OMAP36XX_CONTROL_PROG_IO_WKUP1 (OMAP343X_CONTROL_GENERAL_WKUP +
> > 0x020)
> > +
> >  /* 34xx D2D idle-related pins, handled by PM core */
> >  #define OMAP3_PADCONF_SAD2D_MSTANDBY   0x250
> >  #define OMAP3_PADCONF_SAD2D_IDLEACK    0x254
> > @@ -250,6 +256,8 @@
> >  #define OMAP2_PBIASLITEVMODE0		(1 << 0)
> > 
> >  /* CONTROL_PROG_IO1 bits */
> > +#define OMAP3630_PRG_I2C2_PULLUPRESX    (1 << 0)
> > +#define OMAP3630_PRG_I2C1_PULLUPRESX	(1 << 19)
> >  #define OMAP3630_PRG_SDMMC1_SPEEDCTRL	(1 << 20)
> > 
> >  /* CONTROL_IVA2_BOOTMOD bits */
> > @@ -257,6 +265,12 @@
> >  #define OMAP3_IVA2_BOOTMOD_MASK		(0xf << 0)
> >  #define OMAP3_IVA2_BOOTMOD_IDLE		(0x1 << 0)
> > 
> > +/* CONTROL_PROG_IO2 bits on omap3630 */
> > +#define OMAP3630_PRG_I2C3_PULLUPRESX    (1 << 7)
> > +
> > +/* CONTROL_PROG_IO_WKUP1 bits on omap3630 */
> > +#define OMAP3630_PRG_SR_PULLUPRESX    (1 << 5)
> > +
> >  /* CONTROL_PADCONF_X bits */
> >  #define OMAP3_PADCONF_WAKEUPEVENT0	(1 << 15)
> >  #define OMAP3_PADCONF_WAKEUPENABLE0	(1 << 14)
> > --
> > 1.5.6.3
> 

RE: [PATCH V3 2/2]OMAP: Disable internal I2C pull-ups in OMAP3630

[Woodruff, Richard]

> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Tony Lindgren
> Sent: Tuesday, June 08, 2010 5:05 AM

> Sorry for the delay, here's some more info on this issue. So it looks
> like starting with 3630 there are dedicated pull-up for all the I2C buses.
> And the pull values are configurable with software.

Even 3430 claimed to have this controllable in CONGROL.CONTROL_DEVCONF1[12,13,14]

Last year I had read some of this for an omap34xx issue.  A read of TRM + schematic might imply you have 4 possible resistors to watch out for on a typical I2C channel.
        1 weak pull through standard padconf
        2 stronger pull accessible in CONTROL block of SOC
        3 pulls available on T2 (if your i2c target was twl5030)
        4 possible pull on trace for board schematic

Depending on the TRM/DM you look at guidance of value is given in terms of capacitive loading and speed of operation.

A very strong pull might be good to overcome capacitive loading but it will burn more power per bit transmitted.  I saw one customer stick with full speed and use a weaker pull just based on power savings.

If you want to play around you can use pulls in parallel and attempt to make a stronger pull up.  What you end up doing is really determined by what the signal looks like for that design.

> - Disable mux register pull-ups on 3630 and later
>
> - Allow setting the dedicated I2C pull-up values from board-*.c files
>   for 3630 and later
>
> - Warn if the dedicated pull-up values are not configured on 3630 and
>   later

3430 claims to have some dedicated ones. I have heard that seconded guessed.  IIRC 2430 might even have had something. I don't think this is really so new for 36xx just no one used it.

> - Allow disabling the dedicated I2C pull-up values on 3630 and later
>   in case external pull-up resistors are being used.

The settings required are board specific and depend on speed in use.  Having more than available is not necessary an error and might even be deliberate if someone was trying to be cleaver.

Regards,
Richard W.

Re: [PATCH V3 2/2]OMAP: Disable internal I2C pull-ups in OMAP3630

[Tony Lindgren]

* Woodruff, Richard <r-woodruff2@ti.com> [100609 02:24]:
> 
> > Sorry for the delay, here's some more info on this issue. So it looks
> > like starting with 3630 there are dedicated pull-up for all the I2C buses.
> > And the pull values are configurable with software.
> 
> Even 3430 claimed to have this controllable in CONGROL.CONTROL_DEVCONF1[12,13,14]

Hmm OK, that needs to be verified then.

Regards,

Tony

Re: [PATCH V3 2/2]OMAP: Disable internal I2C pull-ups in OMAP3630

[Leon Woestenberg]

Hello Tony,

On Tue, Jun 8, 2010 at 12:04 PM, Tony Lindgren <tony@atomide.com> wrote:
> Because of this, 3630 boards should have the mux register pull-ups disabled,
> and only use the dedicated I2C pull-ups. In theory external pull-ups should
> not be needed. The value configured for the dedicated I2C pull-ups depends
> on the connected I2C device, and the number of devices.
>
If the internal pull-ups are disabled, why are external pull-ups not needed?

Regards,
-- 
Leon

Re: [PATCH V3 2/2]OMAP: Disable internal I2C pull-ups in OMAP3630

[Tony Lindgren]

* Leon Woestenberg <leon.woestenberg@gmail.com> [100609 22:20]:
> Hello Tony,
> 
> On Tue, Jun 8, 2010 at 12:04 PM, Tony Lindgren <tony@atomide.com> wrote:
> > Because of this, 3630 boards should have the mux register pull-ups disabled,
> > and only use the dedicated I2C pull-ups. In theory external pull-ups should
> > not be needed. The value configured for the dedicated I2C pull-ups depends
> > on the connected I2C device, and the number of devices.
> >
> If the internal pull-ups are disabled, why are external pull-ups not needed?

There are two omap internal pull-ups available: The iopad mux pull-ups and the
I2C specific pull-ups. Only the I2C specific ones should be used.

Regards,

Tony