[U-Boot] [PATCH 3/3] fsl_i2c: Impl. AN2819, rev 5 to calculate FDR/DFSR

[Heiko Schocher <hs@denx.de>]

Hello Joakim,

Joakim Tjernlund wrote:
> The latest AN2819 has changed the way FDR/DFSR should be calculated.
> Update the driver according to spec. However, Condition 2
> is not accounted for as it is not clear how to do so.

Thanks for your work, just some minor Codingstyle comments:

> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> ---
>  drivers/i2c/fsl_i2c.c |   88 +++++++++++++++++++++++++++++-------------------
>  1 files changed, 53 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c
> index 0c5f6be..0491a69 100644
> --- a/drivers/i2c/fsl_i2c.c
> +++ b/drivers/i2c/fsl_i2c.c
> @@ -100,29 +100,9 @@ static const struct fsl_i2c *i2c_dev[2] = {
>   */
>  static const struct {
>  	unsigned short divider;
> -#ifdef __PPC__
> -	u8 dfsr;
> -#endif
>  	u8 fdr;
>  } fsl_i2c_speed_map[] = {
> -#ifdef __PPC__
> -	{160, 1, 32}, {192, 1, 33}, {224, 1, 34}, {256, 1, 35},
> -	{288, 1, 0}, {320, 1, 1}, {352, 6, 1}, {384, 1, 2}, {416, 6, 2},
> -	{448, 1, 38}, {480, 1, 3}, {512, 1, 39}, {544, 11, 3}, {576, 1, 4},
> -	{608, 22, 3}, {640, 1, 5}, {672, 32, 3}, {704, 11, 5}, {736, 43, 3},
> -	{768, 1, 6}, {800, 54, 3}, {832, 11, 6}, {896, 1, 42}, {960, 1, 7},
> -	{1024, 1, 43}, {1088, 22, 7}, {1152, 1, 8}, {1216, 43, 7}, {1280, 1, 9},
> -	{1408, 22, 9}, {1536, 1, 10}, {1664, 22, 10}, {1792, 1, 46},
> -	{1920, 1, 11}, {2048, 1, 47}, {2176, 43, 11}, {2304, 1, 12},
> -	{2560, 1, 13}, {2816, 43, 13}, {3072, 1, 14}, {3328, 43, 14},
> -	{3584, 1, 50}, {3840, 1, 15}, {4096, 1, 51}, {4608, 1, 16},
> -	{5120, 1, 17}, {6144, 1, 18}, {7168, 1, 54}, {7680, 1, 19},
> -	{8192, 1, 55}, {9216, 1, 20}, {10240, 1, 21}, {12288, 1, 22},
> -	{14336, 1, 58}, {15360, 1, 23}, {16384, 1, 59}, {18432, 1, 24},
> -	{20480, 1, 25}, {24576, 1, 26}, {28672, 1, 62}, {30720, 1, 27},
> -	{32768, 1, 63}, {36864, 1, 28}, {40960, 1, 29}, {49152, 1, 30},
> -	{61440, 1, 31}, {-1, 1, 31}
> -#elif defined(__M68K__)
> +#ifdef __M68K__
>  	{20, 32}, {22, 33}, {24, 34}, {26, 35},
>  	{28, 0}, {28, 36}, {30, 1}, {32, 37},
>  	{34, 2}, {36, 38}, {40, 3}, {40, 39},
> @@ -158,7 +138,6 @@ static unsigned int set_i2c_bus_speed(const struct fsl_i2c *dev,
>  	unsigned int i2c_clk, unsigned int speed)
>  {
>  	unsigned short divider = min(i2c_clk / speed, (unsigned short) -1);
> -	unsigned int i;
>  
>  	/*
>  	 * We want to choose an FDR/DFSR that generates an I2C bus speed that
> @@ -166,31 +145,70 @@ static unsigned int set_i2c_bus_speed(const struct fsl_i2c *dev,
>  	 * want the first divider that is equal to or greater than the
>  	 * calculated divider.
>  	 */
> -
> -	for (i = 0; i < ARRAY_SIZE(fsl_i2c_speed_map); i++)
> -		if (fsl_i2c_speed_map[i].divider >= divider) {
> -			u8 fdr;
>  #ifdef __PPC__
> -			u8 dfsr;
> +	u8 dfsr, fdr = 0x31; /* Default if no FDR found */
> +	unsigned short A, B, Ga, Gb;

Please do not use mixed-case variables, thanks.

> +	unsigned long c_div, est_div;
> +
>  #ifdef CONFIG_FSL_I2C_CUSTOM_DFSR
> -			dfsr = CONFIG_FSL_I2C_CUSTOM_DFSR;
> +	dfsr = CONFIG_FSL_I2C_CUSTOM_DFSR;
>  #else
> -			dfsr = fsl_i2c_speed_map[i].dfsr;
> -#endif
> -			writeb(dfsr, &dev->dfsrr);	/* set default filter */
> +	/* Condition 1: dfsr <= 50/T */
> +	dfsr = (5*(i2c_clk/1000))/(100000);

Please use one space around (on each side of) most binary
and ternary operators.

>  #endif
>  #ifdef CONFIG_FSL_I2C_CUSTOM_FDR
> -			fdr = CONFIG_FSL_I2C_CUSTOM_FDR;
> -			speed = i2c_clk / divider; /* Fake something */
> +	fdr = CONFIG_FSL_I2C_CUSTOM_FDR;
> +	speed = i2c_clk / divider; /* Fake something */
>  #else
> +	debug("Requested speed:%d, i2c_clk:%d\n", speed, i2c_clk);
> +	if (!dfsr)
> +		dfsr = 1;
> +
> +	est_div = ~0;
> +	for(Ga=0x4, A=10; A<=30; Ga++, A+=2) {

spaces her too.

> +		for (Gb=0; Gb<8; Gb++) {

and here too. Please check the whole patch.

> +			B = 16 << Gb;
> +			c_div = B * (A + ((3*dfsr)/B)*2);
> +			if (c_div > divider && c_div < est_div) {

Can we make

if ((c_div > divider) && (c_div < est_div)) {


> +				unsigned short bin_Gb, bin_Ga;

here too, please no mixed-case vars.

> +
> +				est_div = c_div;
> +				bin_Gb = Gb << 2;
> +				bin_Ga = (Ga & 0x3) | ((Ga & 0x4) << 3);
> +				fdr = bin_Gb | bin_Ga;
> +				speed = i2c_clk/est_div;
> +				debug("FDR:0x%.2x, div:%ld, Ga:0x%x, Gb:0x%x, "
> +				      "A:%d, B:%d, speed:%d\n",
> +				      fdr, est_div, Ga, Gb, A, B, speed);
> +				/* Condition 2 not accounted for */
> +				debug("Tr <= %d ns\n", 
> +				      (B-3*dfsr)* 1000000/(i2c_clk/1000));
> +			}
> +		}
> +		if (A == 20)
> +			A+=2;
> +		if (A == 24)
> +			A+=4;
> +	}
> +	debug("divider:%d, est_div:%ld, DFSR:%d\n", divider, est_div, dfsr);
> +	debug("FDR:0x%.2x, speed:%d\n", fdr, speed);
> +#endif
> +	writeb(dfsr, &dev->dfsrr);	/* set default filter */
> +	writeb(fdr, &dev->fdr);		/* set bus speed */
> +#else
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(fsl_i2c_speed_map); i++)
> +		if (fsl_i2c_speed_map[i].divider >= divider) {
> +			u8 fdr;
> +
>  			fdr = fsl_i2c_speed_map[i].fdr;
>  			speed = i2c_clk / fsl_i2c_speed_map[i].divider;
> -#endif
>  			writeb(fdr, &dev->fdr);		/* set bus speed */
>  
>  			break;
>  		}
> -
> +#endif
>  	return speed;
>  }

bye
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany