Re: [PATCH RESEND] i2c: omap: improve duty cycle on SCL

[Alexander Sverdlin <alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>]

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-l0cyMroinI0@public.gmane.org>

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;
> 

-- 
Best regards,
Alexander Sverdlin.