i2c: omap: improve duty cycle on SCL

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 <[email protected]>

---
 drivers/i2c/busses/i2c-omap.c | 86 ++++++++++++++++++++++++++++---------------
 1 file changed, 56 insertions(+), 30 deletions(-)

drivers/i2c/busses/i2c-omap.c +56 -30

index 0e894193accf..034d2d1ff289 100644
@@ -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;

Signature · 1 line

2.4.3

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 <[email protected]>

---
 drivers/i2c/busses/i2c-omap.c | 86 ++++++++++++++++++++++++++++---------------
 1 file changed, 56 insertions(+), 30 deletions(-)

drivers/i2c/busses/i2c-omap.c +56 -30

index 0e894193accf..034d2d1ff289 100644
@@ -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;

Quoted 21 lines

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.

Quoted 1 line

Signed-off-by Felipe Balbi <[email protected]>

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.

Hi,

Quoted 21 lines · Alexander Sverdlin

Quoted 21 lines

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.

Quoted 7 lines

Quoted 1 line

Signed-off-by Felipe Balbi <[email protected]>

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.

Quoted 21 lines

Quoted 21 lines

With this patch we try to be as close to 50%

Quoted 20 lines

Quoted 20 lines

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.

Quoted 7 lines

Quoted 7 lines

Quoted 1 line

Quoted 1 line

Signed-off-by Felipe Balbi <[email protected]>

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.

Hello Felipe,

Quoted 34 lines · Felipe Balbi

Quoted 21 lines

Quoted 21 lines

With this patch we try to be as close to 50%

Quoted 20 lines

Quoted 20 lines

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.

Quoted 7 lines

Quoted 7 lines

Quoted 1 line

Quoted 1 line

Signed-off-by Felipe Balbi <[email protected]>

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%

Quoted 2 lines

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?

Quoted 33 lines

Quoted 33 lines

Quoted 33 lines

Quoted 33 lines

Quoted 20 lines

Quoted 20 lines

duty cycle as possible. The reason for this

Quoted 19 lines

Quoted 19 lines

Quoted 19 lines

Quoted 19 lines

Quoted 19 lines

Quoted 19 lines

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.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Signed-off-by: Felipe Balbi <[email protected]>

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%

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

Quoted 2 lines

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.