[RESEND] i2c: omap: improve duty cycle on SCL
diff mbox

Message ID 1434482445-1818-1-git-send-email-balbi@ti.com
State New
Headers show

Commit Message

Felipe Balbi June 16, 2015, 7:20 p.m. UTC
With this patch we try to be as close to 50%
duty cycle as possible. The reason for this
is that some devices present an erratic behavior
with certain duty cycles.

One such example is TPS65218 PMIC which fails
to change voltages when running @ 400kHz and
duty cycle is lower than 34%.

The idea of the patch is simple:

calculate desired scl_period from requested scl
and use 50% for tLow and 50% for tHigh.

tLow is calculated with a DIV_ROUND_UP() to make
sure it's slightly higher than tHigh and to make
sure that we end up within I2C specifications.

Kudos to Nishanth Menon and Dave Gerlach for helping
debugging the TPS65218 problem found on AM437x SK.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/i2c/busses/i2c-omap.c | 86 ++++++++++++++++++++++++++++---------------
 1 file changed, 56 insertions(+), 30 deletions(-)

Comments

Felipe Balbi June 16, 2015, 7:26 p.m. UTC | #1
On Tue, Jun 16, 2015 at 02:20:45PM -0500, Felipe Balbi wrote:
> With this patch we try to be as close to 50%
> duty cycle as possible. The reason for this
> is that some devices present an erratic behavior
> with certain duty cycles.
> 
> One such example is TPS65218 PMIC which fails
> to change voltages when running @ 400kHz and
> duty cycle is lower than 34%.
> 
> The idea of the patch is simple:
> 
> calculate desired scl_period from requested scl
> and use 50% for tLow and 50% for tHigh.
> 
> tLow is calculated with a DIV_ROUND_UP() to make
> sure it's slightly higher than tHigh and to make
> sure that we end up within I2C specifications.
> 
> Kudos to Nishanth Menon and Dave Gerlach for helping
> debugging the TPS65218 problem found on AM437x SK.
> 
> Signed-off-by: Felipe Balbi <balbi@ti.com>

Aaro, can you test this on N900 ?

> ---
>  drivers/i2c/busses/i2c-omap.c | 86 ++++++++++++++++++++++++++++---------------
>  1 file changed, 56 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 0e894193accf..034d2d1ff289 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -25,6 +25,7 @@
>   */
>  
>  #include <linux/module.h>
> +#include <linux/kernel.h>
>  #include <linux/delay.h>
>  #include <linux/i2c.h>
>  #include <linux/err.h>
> @@ -39,6 +40,8 @@
>  #include <linux/i2c-omap.h>
>  #include <linux/pm_runtime.h>
>  
> +#define NSECS_PER_SEC			1000000000
> +
>  /* I2C controller revisions */
>  #define OMAP_I2C_OMAP1_REV_2		0x20
>  
> @@ -359,6 +362,8 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
>  	u16 fsscll = 0, fssclh = 0, hsscll = 0, hssclh = 0;
>  	unsigned long fclk_rate = 12000000;
>  	unsigned long internal_clk = 0;
> +	unsigned long internal_clk_period = 0;
> +	unsigned long scl_period = 0;
>  	struct clk *fclk;
>  
>  	if (dev->rev >= OMAP_I2C_REV_ON_3430_3530) {
> @@ -395,52 +400,73 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
>  	}
>  
>  	if (!(dev->flags & OMAP_I2C_FLAG_SIMPLE_CLOCK)) {
> -
>  		/*
>  		 * HSI2C controller internal clk rate should be 19.2 Mhz for
> -		 * HS and for all modes on 2430. On 34xx we can use lower rate
> -		 * to get longer filter period for better noise suppression.
> -		 * The filter is iclk (fclk for HS) period.
> +		 * HS and for all modes on 2430. For all other devices and
> +		 * speeds we will use a 12MHz internal clock.
>  		 */
> -		if (dev->speed > 400 ||
> -			       dev->flags & OMAP_I2C_FLAG_FORCE_19200_INT_CLK)
> -			internal_clk = 19200;
> -		else if (dev->speed > 100)
> -			internal_clk = 9600;
> -		else
> -			internal_clk = 4000;
> +		if (dev->flags & OMAP_I2C_FLAG_FORCE_19200_INT_CLK ||
> +				dev->speed > 400) {
> +			internal_clk = 1920000;
> +			internal_clk_period = NSECS_PER_SEC /
> +				internal_clk; /* ns */
> +		} else {
> +			internal_clk = 12000000;
> +			internal_clk_period = NSECS_PER_SEC /
> +				internal_clk; /* ns */
> +		}
> +
>  		fclk = clk_get(dev->dev, "fck");
> -		fclk_rate = clk_get_rate(fclk) / 1000;
> +		fclk_rate = clk_get_rate(fclk);
>  		clk_put(fclk);
>  
>  		/* Compute prescaler divisor */
>  		psc = fclk_rate / internal_clk;
>  		psc = psc - 1;
>  
> +		/*
> +		 * Here's the tricky part, we want to make sure our duty cycle
> +		 * is as close to 50% as possible. In order to achieve that, we
> +		 * will first figure out what's the period on chosen scl is,
> +		 * then divide that by two and calculate SCLL and SCLH based on
> +		 * that.
> +		 *
> +		 * SCLL and SCLH equations are as folows:
> +		 *
> +		 * SCLL = (tLow / iclk_period) - 7;
> +		 * SCLH = (tHigh / iclk_period) - 5;
> +		 *
> +		 * Where iclk_period is period of Internal Clock.
> +		 *
> +		 * tLow and tHigh will be basically half of scl_period where
> +		 * possible as long as we can match I2C spec's minimum limits
> +		 * for them.
> +		 */
> +		scl_period = NSECS_PER_SEC / dev->speed;
> +
>  		/* If configured for High Speed */
>  		if (dev->speed > 400) {
> -			unsigned long scl;
> +			unsigned long fs_period;
> +
> +			/*
> +			 * first phase of HS mode is up to
> +			 * 400kHz so we will use that.
> +			 */
> +			fs_period = NSECS_PER_SEC / 400;
>  
>  			/* For first phase of HS mode */
> -			scl = internal_clk / 400;
> -			fsscll = scl - (scl / 3) - 7;
> -			fssclh = (scl / 3) - 5;
> +			fsscll = DIV_ROUND_UP(fs_period >> 1,
> +					internal_clk_period) - 7;
> +			fssclh = (fs_period >> 1) / internal_clk_period - 5;
>  
>  			/* For second phase of HS mode */
> -			scl = fclk_rate / dev->speed;
> -			hsscll = scl - (scl / 3) - 7;
> -			hssclh = (scl / 3) - 5;
> -		} else if (dev->speed > 100) {
> -			unsigned long scl;
> -
> -			/* Fast mode */
> -			scl = internal_clk / dev->speed;
> -			fsscll = scl - (scl / 3) - 7;
> -			fssclh = (scl / 3) - 5;
> -		} else {
> -			/* Standard mode */
> -			fsscll = internal_clk / (dev->speed * 2) - 7;
> -			fssclh = internal_clk / (dev->speed * 2) - 5;
> +			hsscll = DIV_ROUND_UP(scl_period >> 1,
> +					internal_clk_period) - 7;
> +			hssclh = (scl_period >> 1) / internal_clk_period - 5;
> +		} else  {
> +			fsscll = DIV_ROUND_UP(scl_period >> 1,
> +					internal_clk_period) - 7;
> +			fssclh = (scl_period >> 1) / internal_clk_period - 5;
>  		}
>  		scll = (hsscll << OMAP_I2C_SCLL_HSSCLL) | fsscll;
>  		sclh = (hssclh << OMAP_I2C_SCLH_HSSCLH) | fssclh;
> -- 
> 2.4.3
>
Alexander X Sverdlin June 17, 2015, 9:19 a.m. UTC | #2
Hello!

On 16/06/15 21:20, ext Felipe Balbi wrote:
> With this patch we try to be as close to 50%
> duty cycle as possible. The reason for this
> is that some devices present an erratic behavior
> with certain duty cycles.
> 
> One such example is TPS65218 PMIC which fails
> to change voltages when running @ 400kHz and
> duty cycle is lower than 34%.
> 
> The idea of the patch is simple:
> 
> calculate desired scl_period from requested scl
> and use 50% for tLow and 50% for tHigh.
> 
> tLow is calculated with a DIV_ROUND_UP() to make
> sure it's slightly higher than tHigh and to make
> sure that we end up within I2C specifications.
> 
> Kudos to Nishanth Menon and Dave Gerlach for helping
> debugging the TPS65218 problem found on AM437x SK.
> 
> Signed-off-by: Felipe Balbi <balbi@ti.com>

NAK.
This is a direct violation of PHILIPS I2C-bus Specification v.2.1, section 15.
Namely, you will have LOW period of SCL clock shorter than required 1.3uS.
Therefore, I'm trying to fix the same currently in davinci:
https://patchwork.ozlabs.org/patch/483024/

Probably your problem is in board design. I don't think this should be the reason
for the spec-violating code in the upstream Kernel. You have your TI SDK for this.

> ---
>  drivers/i2c/busses/i2c-omap.c | 86 ++++++++++++++++++++++++++++---------------
>  1 file changed, 56 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 0e894193accf..034d2d1ff289 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -25,6 +25,7 @@
>   */
>  
>  #include <linux/module.h>
> +#include <linux/kernel.h>
>  #include <linux/delay.h>
>  #include <linux/i2c.h>
>  #include <linux/err.h>
> @@ -39,6 +40,8 @@
>  #include <linux/i2c-omap.h>
>  #include <linux/pm_runtime.h>
>  
> +#define NSECS_PER_SEC			1000000000
> +
>  /* I2C controller revisions */
>  #define OMAP_I2C_OMAP1_REV_2		0x20
>  
> @@ -359,6 +362,8 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
>  	u16 fsscll = 0, fssclh = 0, hsscll = 0, hssclh = 0;
>  	unsigned long fclk_rate = 12000000;
>  	unsigned long internal_clk = 0;
> +	unsigned long internal_clk_period = 0;
> +	unsigned long scl_period = 0;
>  	struct clk *fclk;
>  
>  	if (dev->rev >= OMAP_I2C_REV_ON_3430_3530) {
> @@ -395,52 +400,73 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
>  	}
>  
>  	if (!(dev->flags & OMAP_I2C_FLAG_SIMPLE_CLOCK)) {
> -
>  		/*
>  		 * HSI2C controller internal clk rate should be 19.2 Mhz for
> -		 * HS and for all modes on 2430. On 34xx we can use lower rate
> -		 * to get longer filter period for better noise suppression.
> -		 * The filter is iclk (fclk for HS) period.
> +		 * HS and for all modes on 2430. For all other devices and
> +		 * speeds we will use a 12MHz internal clock.
>  		 */
> -		if (dev->speed > 400 ||
> -			       dev->flags & OMAP_I2C_FLAG_FORCE_19200_INT_CLK)
> -			internal_clk = 19200;
> -		else if (dev->speed > 100)
> -			internal_clk = 9600;
> -		else
> -			internal_clk = 4000;
> +		if (dev->flags & OMAP_I2C_FLAG_FORCE_19200_INT_CLK ||
> +				dev->speed > 400) {
> +			internal_clk = 1920000;
> +			internal_clk_period = NSECS_PER_SEC /
> +				internal_clk; /* ns */
> +		} else {
> +			internal_clk = 12000000;
> +			internal_clk_period = NSECS_PER_SEC /
> +				internal_clk; /* ns */
> +		}
> +
>  		fclk = clk_get(dev->dev, "fck");
> -		fclk_rate = clk_get_rate(fclk) / 1000;
> +		fclk_rate = clk_get_rate(fclk);
>  		clk_put(fclk);
>  
>  		/* Compute prescaler divisor */
>  		psc = fclk_rate / internal_clk;
>  		psc = psc - 1;
>  
> +		/*
> +		 * Here's the tricky part, we want to make sure our duty cycle
> +		 * is as close to 50% as possible. In order to achieve that, we
> +		 * will first figure out what's the period on chosen scl is,
> +		 * then divide that by two and calculate SCLL and SCLH based on
> +		 * that.
> +		 *
> +		 * SCLL and SCLH equations are as folows:
> +		 *
> +		 * SCLL = (tLow / iclk_period) - 7;
> +		 * SCLH = (tHigh / iclk_period) - 5;
> +		 *
> +		 * Where iclk_period is period of Internal Clock.
> +		 *
> +		 * tLow and tHigh will be basically half of scl_period where
> +		 * possible as long as we can match I2C spec's minimum limits
> +		 * for them.
> +		 */
> +		scl_period = NSECS_PER_SEC / dev->speed;
> +
>  		/* If configured for High Speed */
>  		if (dev->speed > 400) {
> -			unsigned long scl;
> +			unsigned long fs_period;
> +
> +			/*
> +			 * first phase of HS mode is up to
> +			 * 400kHz so we will use that.
> +			 */
> +			fs_period = NSECS_PER_SEC / 400;
>  
>  			/* For first phase of HS mode */
> -			scl = internal_clk / 400;
> -			fsscll = scl - (scl / 3) - 7;
> -			fssclh = (scl / 3) - 5;
> +			fsscll = DIV_ROUND_UP(fs_period >> 1,
> +					internal_clk_period) - 7;
> +			fssclh = (fs_period >> 1) / internal_clk_period - 5;
>  
>  			/* For second phase of HS mode */
> -			scl = fclk_rate / dev->speed;
> -			hsscll = scl - (scl / 3) - 7;
> -			hssclh = (scl / 3) - 5;
> -		} else if (dev->speed > 100) {
> -			unsigned long scl;
> -
> -			/* Fast mode */
> -			scl = internal_clk / dev->speed;
> -			fsscll = scl - (scl / 3) - 7;
> -			fssclh = (scl / 3) - 5;
> -		} else {
> -			/* Standard mode */
> -			fsscll = internal_clk / (dev->speed * 2) - 7;
> -			fssclh = internal_clk / (dev->speed * 2) - 5;
> +			hsscll = DIV_ROUND_UP(scl_period >> 1,
> +					internal_clk_period) - 7;
> +			hssclh = (scl_period >> 1) / internal_clk_period - 5;
> +		} else  {
> +			fsscll = DIV_ROUND_UP(scl_period >> 1,
> +					internal_clk_period) - 7;
> +			fssclh = (scl_period >> 1) / internal_clk_period - 5;
>  		}
>  		scll = (hsscll << OMAP_I2C_SCLL_HSSCLL) | fsscll;
>  		sclh = (hssclh << OMAP_I2C_SCLH_HSSCLH) | fssclh;
>
Felipe Balbi June 17, 2015, 6 p.m. UTC | #3
Hi,

On Wed, Jun 17, 2015 at 11:19:28AM +0200, Alexander Sverdlin wrote:
> > With this patch we try to be as close to 50%
> > duty cycle as possible. The reason for this
> > is that some devices present an erratic behavior
> > with certain duty cycles.
> > 
> > One such example is TPS65218 PMIC which fails
> > to change voltages when running @ 400kHz and
> > duty cycle is lower than 34%.
> > 
> > The idea of the patch is simple:
> > 
> > calculate desired scl_period from requested scl
> > and use 50% for tLow and 50% for tHigh.
> > 
> > tLow is calculated with a DIV_ROUND_UP() to make
> > sure it's slightly higher than tHigh and to make
> > sure that we end up within I2C specifications.
> > 
> > Kudos to Nishanth Menon and Dave Gerlach for helping
> > debugging the TPS65218 problem found on AM437x SK.
> > 
> > Signed-off-by: Felipe Balbi <balbi@ti.com>
> 
> NAK.
> This is a direct violation of PHILIPS I2C-bus Specification v.2.1,
> section 15.
> Namely, you will have LOW period of SCL clock shorter than required
> 1.3uS.

how is this out of spec ?

http://i.imgur.com/jEDlZT7.png

-Width = 1.4us, frequency 373.1kHz, duty cycle of 47.76%

In any case, I have to send v2 anyway (found a bug which would show up
on frequencies above 400kHz), so I'll resend this patch.
Felipe Balbi June 17, 2015, 6:04 p.m. UTC | #4
On Wed, Jun 17, 2015 at 01:00:52PM -0500, Felipe Balbi wrote:
> Hi,
> 
> On Wed, Jun 17, 2015 at 11:19:28AM +0200, Alexander Sverdlin wrote:
> > > With this patch we try to be as close to 50%
> > > duty cycle as possible. The reason for this
> > > is that some devices present an erratic behavior
> > > with certain duty cycles.
> > > 
> > > One such example is TPS65218 PMIC which fails
> > > to change voltages when running @ 400kHz and
> > > duty cycle is lower than 34%.
> > > 
> > > The idea of the patch is simple:
> > > 
> > > calculate desired scl_period from requested scl
> > > and use 50% for tLow and 50% for tHigh.
> > > 
> > > tLow is calculated with a DIV_ROUND_UP() to make
> > > sure it's slightly higher than tHigh and to make
> > > sure that we end up within I2C specifications.
> > > 
> > > Kudos to Nishanth Menon and Dave Gerlach for helping
> > > debugging the TPS65218 problem found on AM437x SK.
> > > 
> > > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > 
> > NAK.
> > This is a direct violation of PHILIPS I2C-bus Specification v.2.1,
> > section 15.
> > Namely, you will have LOW period of SCL clock shorter than required
> > 1.3uS.
> 
> how is this out of spec ?
> 
> http://i.imgur.com/jEDlZT7.png
> 
> -Width = 1.4us, frequency 373.1kHz, duty cycle of 47.76%
> 
> In any case, I have to send v2 anyway (found a bug which would show up
> on frequencies above 400kHz), so I'll resend this patch.

Also, let's assume we can generate exactly 400kHz with the same duty
cycle. 400kHz has a period of 2500ns, so:

2500 * (1 - 0.4773) = 1306us

How is that out of spec ?
Alexander Sverdlin June 17, 2015, 6:38 p.m. UTC | #5
Hello Felipe,

On 17/06/15 20:00, Felipe Balbi wrote:
>>> With this patch we try to be as close to 50%
>>> > > duty cycle as possible. The reason for this
>>> > > is that some devices present an erratic behavior
>>> > > with certain duty cycles.
>>> > > 
>>> > > One such example is TPS65218 PMIC which fails
>>> > > to change voltages when running @ 400kHz and
>>> > > duty cycle is lower than 34%.
>>> > > 
>>> > > The idea of the patch is simple:
>>> > > 
>>> > > calculate desired scl_period from requested scl
>>> > > and use 50% for tLow and 50% for tHigh.
>>> > > 
>>> > > tLow is calculated with a DIV_ROUND_UP() to make
>>> > > sure it's slightly higher than tHigh and to make
>>> > > sure that we end up within I2C specifications.
>>> > > 
>>> > > Kudos to Nishanth Menon and Dave Gerlach for helping
>>> > > debugging the TPS65218 problem found on AM437x SK.
>>> > > 
>>> > > Signed-off-by: Felipe Balbi <balbi@ti.com>
>> > 
>> > NAK.
>> > This is a direct violation of PHILIPS I2C-bus Specification v.2.1,
>> > section 15.
>> > Namely, you will have LOW period of SCL clock shorter than required
>> > 1.3uS.
> how is this out of spec ?
> 
> http://i.imgur.com/jEDlZT7.png
> 
> -Width = 1.4us, frequency 373.1kHz, duty cycle of 47.76%
> 
> In any case, I have to send v2 anyway (found a bug which would show up
> on frequencies above 400kHz), so I'll resend this patch.

If you really target 50% duty cycle and there will be no rounding/truncation
error, you will end up with 1.25uS at 400kHz. I understand why you want to
make HIGH phase longer, but 50% is a bad target at 400hHz. Probably more safe
value?

Alex.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Felipe Balbi June 17, 2015, 6:57 p.m. UTC | #6
Hi,

On Wed, Jun 17, 2015 at 08:38:42PM +0200, Alexander Sverdlin wrote:
> Hello Felipe,
> 
> On 17/06/15 20:00, Felipe Balbi wrote:
> >>> With this patch we try to be as close to 50%
> >>> > > duty cycle as possible. The reason for this
> >>> > > is that some devices present an erratic behavior
> >>> > > with certain duty cycles.
> >>> > > 
> >>> > > One such example is TPS65218 PMIC which fails
> >>> > > to change voltages when running @ 400kHz and
> >>> > > duty cycle is lower than 34%.
> >>> > > 
> >>> > > The idea of the patch is simple:
> >>> > > 
> >>> > > calculate desired scl_period from requested scl
> >>> > > and use 50% for tLow and 50% for tHigh.
> >>> > > 
> >>> > > tLow is calculated with a DIV_ROUND_UP() to make
> >>> > > sure it's slightly higher than tHigh and to make
> >>> > > sure that we end up within I2C specifications.
> >>> > > 
> >>> > > Kudos to Nishanth Menon and Dave Gerlach for helping
> >>> > > debugging the TPS65218 problem found on AM437x SK.
> >>> > > 
> >>> > > Signed-off-by: Felipe Balbi <balbi@ti.com>
> >> > 
> >> > NAK.
> >> > This is a direct violation of PHILIPS I2C-bus Specification v.2.1,
> >> > section 15.
> >> > Namely, you will have LOW period of SCL clock shorter than required
> >> > 1.3uS.
> > how is this out of spec ?
> > 
> > http://i.imgur.com/jEDlZT7.png
> > 
> > -Width = 1.4us, frequency 373.1kHz, duty cycle of 47.76%
> > 
> > In any case, I have to send v2 anyway (found a bug which would show up
> > on frequencies above 400kHz), so I'll resend this patch.
> 
> If you really target 50% duty cycle and there will be no
> rounding/truncation error, you will end up with 1.25uS at 400kHz. I
> understand why you want to make HIGH phase longer, but 50% is a bad
> target at 400hHz. Probably more safe value?

We can't generate exactly 400kHz anyway, and we're not getting exactly
50% duty cycle, it just brings it *closer* to that. I even mention the
reason for the DIV_ROUND_UP() there. Let's just go through the math
using TRM equation:

@400kHz with 12MHz internal clock:

tLow = (SCLL + 7) * iclk_period

1250 = (SCLL + 7) * 1/12MHz

SCLL = DIV_ROUND_UP(1250, 83) - 7
SCLL = (1250 + 82)/83 - 7
SCLL = 9

Now if we do the reverse to find actual tLow:

tLow = (9 + 7) * 83
tLow = 1328ns

Likewise if we do it for tHigh:

tHigh = (SCLH + 5) iclk_period

SCLH = 1250 / 83 - 5
SCLH = 10

tHigh = 15 * 83
tHigh = 1245

tHigh + tLow (SCL period) = 2573ns. That gives us SCL of 388.65kHz. An
error of mere 3% of what we really wanted to achieve. I'd say this is
pretty darn good.

Now you tell me, how is this *ever* going to be out of spec ?

DIV_ROUND_UP() and the truncation at internal clock period calculation
makes sure that tLow will be within spec. Instead of blindly NAKing the
patch, you could've gone through this exercise yourself.
Alexander Sverdlin June 17, 2015, 7:21 p.m. UTC | #7
Hello!

On 17/06/15 20:57, Felipe Balbi wrote:
>>>>>>> duty cycle as possible. The reason for this
>>>>>>> > >>> > > is that some devices present an erratic behavior
>>>>>>> > >>> > > with certain duty cycles.
>>>>>>> > >>> > > 
>>>>>>> > >>> > > One such example is TPS65218 PMIC which fails
>>>>>>> > >>> > > to change voltages when running @ 400kHz and
>>>>>>> > >>> > > duty cycle is lower than 34%.
>>>>>>> > >>> > > 
>>>>>>> > >>> > > The idea of the patch is simple:
>>>>>>> > >>> > > 
>>>>>>> > >>> > > calculate desired scl_period from requested scl
>>>>>>> > >>> > > and use 50% for tLow and 50% for tHigh.
>>>>>>> > >>> > > 
>>>>>>> > >>> > > tLow is calculated with a DIV_ROUND_UP() to make
>>>>>>> > >>> > > sure it's slightly higher than tHigh and to make
>>>>>>> > >>> > > sure that we end up within I2C specifications.
>>>>>>> > >>> > > 
>>>>>>> > >>> > > Kudos to Nishanth Menon and Dave Gerlach for helping
>>>>>>> > >>> > > debugging the TPS65218 problem found on AM437x SK.
>>>>>>> > >>> > > 
>>>>>>> > >>> > > Signed-off-by: Felipe Balbi <balbi@ti.com>
>>>>> > >> > 
>>>>> > >> > NAK.
>>>>> > >> > This is a direct violation of PHILIPS I2C-bus Specification v.2.1,
>>>>> > >> > section 15.
>>>>> > >> > Namely, you will have LOW period of SCL clock shorter than required
>>>>> > >> > 1.3uS.
>>> > > how is this out of spec ?
>>> > > 
>>> > > http://i.imgur.com/jEDlZT7.png
>>> > > 
>>> > > -Width = 1.4us, frequency 373.1kHz, duty cycle of 47.76%
>>> > > 
>>> > > In any case, I have to send v2 anyway (found a bug which would show up
>>> > > on frequencies above 400kHz), so I'll resend this patch.
>> > 
>> > If you really target 50% duty cycle and there will be no
>> > rounding/truncation error, you will end up with 1.25uS at 400kHz. I
>> > understand why you want to make HIGH phase longer, but 50% is a bad
>> > target at 400hHz. Probably more safe value?
> We can't generate exactly 400kHz anyway, and we're not getting exactly
> 50% duty cycle, it just brings it *closer* to that. I even mention the
> reason for the DIV_ROUND_UP() there. Let's just go through the math
> using TRM equation:
> 
> @400kHz with 12MHz internal clock:
> 
> tLow = (SCLL + 7) * iclk_period
> 
> 1250 = (SCLL + 7) * 1/12MHz
> 
> SCLL = DIV_ROUND_UP(1250, 83) - 7
> SCLL = (1250 + 82)/83 - 7
> SCLL = 9
> 
> Now if we do the reverse to find actual tLow:
> 
> tLow = (9 + 7) * 83
> tLow = 1328ns
> 
> Likewise if we do it for tHigh:
> 
> tHigh = (SCLH + 5) iclk_period
> 
> SCLH = 1250 / 83 - 5
> SCLH = 10
> 
> tHigh = 15 * 83
> tHigh = 1245
> 
> tHigh + tLow (SCL period) = 2573ns. That gives us SCL of 388.65kHz. An

The bus rate is actually 12MHz/(SCLH+5+SCLL+7), so it's ~387096Hz.
Anyway, it's still possible to have maximum bus rate if SCLH+SCLL=12000000/400000-5-7

Let say SCLH=9, SCLL=9, still within the spec and exactly 400kHz
But you need another equations and it's not about 50% duty cycle

> error of mere 3% of what we really wanted to achieve. I'd say this is
> pretty darn good.
> 
> Now you tell me, how is this *ever* going to be out of spec ?
> 
> DIV_ROUND_UP() and the truncation at internal clock period calculation
> makes sure that tLow will be within spec. Instead of blindly NAKing the
> patch, you could've gone through this exercise yourself.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 0e894193accf..034d2d1ff289 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -25,6 +25,7 @@ 
  */
 
 #include <linux/module.h>
+#include <linux/kernel.h>
 #include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/err.h>
@@ -39,6 +40,8 @@ 
 #include <linux/i2c-omap.h>
 #include <linux/pm_runtime.h>
 
+#define NSECS_PER_SEC			1000000000
+
 /* I2C controller revisions */
 #define OMAP_I2C_OMAP1_REV_2		0x20
 
@@ -359,6 +362,8 @@  static int omap_i2c_init(struct omap_i2c_dev *dev)
 	u16 fsscll = 0, fssclh = 0, hsscll = 0, hssclh = 0;
 	unsigned long fclk_rate = 12000000;
 	unsigned long internal_clk = 0;
+	unsigned long internal_clk_period = 0;
+	unsigned long scl_period = 0;
 	struct clk *fclk;
 
 	if (dev->rev >= OMAP_I2C_REV_ON_3430_3530) {
@@ -395,52 +400,73 @@  static int omap_i2c_init(struct omap_i2c_dev *dev)
 	}
 
 	if (!(dev->flags & OMAP_I2C_FLAG_SIMPLE_CLOCK)) {
-
 		/*
 		 * HSI2C controller internal clk rate should be 19.2 Mhz for
-		 * HS and for all modes on 2430. On 34xx we can use lower rate
-		 * to get longer filter period for better noise suppression.
-		 * The filter is iclk (fclk for HS) period.
+		 * HS and for all modes on 2430. For all other devices and
+		 * speeds we will use a 12MHz internal clock.
 		 */
-		if (dev->speed > 400 ||
-			       dev->flags & OMAP_I2C_FLAG_FORCE_19200_INT_CLK)
-			internal_clk = 19200;
-		else if (dev->speed > 100)
-			internal_clk = 9600;
-		else
-			internal_clk = 4000;
+		if (dev->flags & OMAP_I2C_FLAG_FORCE_19200_INT_CLK ||
+				dev->speed > 400) {
+			internal_clk = 1920000;
+			internal_clk_period = NSECS_PER_SEC /
+				internal_clk; /* ns */
+		} else {
+			internal_clk = 12000000;
+			internal_clk_period = NSECS_PER_SEC /
+				internal_clk; /* ns */
+		}
+
 		fclk = clk_get(dev->dev, "fck");
-		fclk_rate = clk_get_rate(fclk) / 1000;
+		fclk_rate = clk_get_rate(fclk);
 		clk_put(fclk);
 
 		/* Compute prescaler divisor */
 		psc = fclk_rate / internal_clk;
 		psc = psc - 1;
 
+		/*
+		 * Here's the tricky part, we want to make sure our duty cycle
+		 * is as close to 50% as possible. In order to achieve that, we
+		 * will first figure out what's the period on chosen scl is,
+		 * then divide that by two and calculate SCLL and SCLH based on
+		 * that.
+		 *
+		 * SCLL and SCLH equations are as folows:
+		 *
+		 * SCLL = (tLow / iclk_period) - 7;
+		 * SCLH = (tHigh / iclk_period) - 5;
+		 *
+		 * Where iclk_period is period of Internal Clock.
+		 *
+		 * tLow and tHigh will be basically half of scl_period where
+		 * possible as long as we can match I2C spec's minimum limits
+		 * for them.
+		 */
+		scl_period = NSECS_PER_SEC / dev->speed;
+
 		/* If configured for High Speed */
 		if (dev->speed > 400) {
-			unsigned long scl;
+			unsigned long fs_period;
+
+			/*
+			 * first phase of HS mode is up to
+			 * 400kHz so we will use that.
+			 */
+			fs_period = NSECS_PER_SEC / 400;
 
 			/* For first phase of HS mode */
-			scl = internal_clk / 400;
-			fsscll = scl - (scl / 3) - 7;
-			fssclh = (scl / 3) - 5;
+			fsscll = DIV_ROUND_UP(fs_period >> 1,
+					internal_clk_period) - 7;
+			fssclh = (fs_period >> 1) / internal_clk_period - 5;
 
 			/* For second phase of HS mode */
-			scl = fclk_rate / dev->speed;
-			hsscll = scl - (scl / 3) - 7;
-			hssclh = (scl / 3) - 5;
-		} else if (dev->speed > 100) {
-			unsigned long scl;
-
-			/* Fast mode */
-			scl = internal_clk / dev->speed;
-			fsscll = scl - (scl / 3) - 7;
-			fssclh = (scl / 3) - 5;
-		} else {
-			/* Standard mode */
-			fsscll = internal_clk / (dev->speed * 2) - 7;
-			fssclh = internal_clk / (dev->speed * 2) - 5;
+			hsscll = DIV_ROUND_UP(scl_period >> 1,
+					internal_clk_period) - 7;
+			hssclh = (scl_period >> 1) / internal_clk_period - 5;
+		} else  {
+			fsscll = DIV_ROUND_UP(scl_period >> 1,
+					internal_clk_period) - 7;
+			fssclh = (scl_period >> 1) / internal_clk_period - 5;
 		}
 		scll = (hsscll << OMAP_I2C_SCLL_HSSCLL) | fsscll;
 		sclh = (hssclh << OMAP_I2C_SCLH_HSSCLH) | fssclh;