[U-Boot-Users] [COLDFIRE]

[U-Boot-Users] [COLDFIRE]

[Luigi 'Comio' Mantellini]

The coldfire cpus use a little different i2c device that is not perfectly compatible with the i2c fsl driver present on mpc cpus. consequently the fsl_i2c driver needs some little modification in order to work on these cpus (m547x/m548x and m5445x cpus).

luigi

[U-Boot-Users] [PATCH 1/1] Fix I2C for m547x/548x and m5445x processors.

[Luigi 'Comio' Mantellini]

---
 drivers/i2c/fsl_i2c.c |   68 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 68 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c
index 0690340..8fd73c0 100644
--- a/drivers/i2c/fsl_i2c.c
+++ b/drivers/i2c/fsl_i2c.c
@@ -78,6 +78,73 @@ static const struct fsl_i2c *i2c_dev[2] = {
  * register.  See the application note AN2919 "Determining the I2C Frequency
  * Divider Ratio for SCL"
  */
+#if defined(CONFIG_MCF547x_8x) || defined(CONFIG_MCF5445x)
+static const struct {
+	u8 fdr;
+	unsigned short divider;
+} fsl_i2c_speed_map[] = {
+	  {.fdr=0x20, .divider=  20},   {.fdr=0x21, .divider=  22},   {.fdr=0x22, .divider=  24},
+	  {.fdr=0x23, .divider=  26},   {.fdr=0x00, .divider=  28},   {.fdr=0x24, .divider=  28},
+	  {.fdr=0x01, .divider=  30},   {.fdr=0x25, .divider=  32},   {.fdr=0x02, .divider=  34},
+	  {.fdr=0x26, .divider=  36},   {.fdr=0x03, .divider=  40},   {.fdr=0x27, .divider=  40},
+	  {.fdr=0x04, .divider=  44},   {.fdr=0x05, .divider=  48}, /*{.fdr=0x28, .divider=  48},*/
+	  {.fdr=0x06, .divider=  56}, /*{.fdr=0x29, .divider=  56},*/ {.fdr=0x2A, .divider=  64},
+	  {.fdr=0x07, .divider=  68},   {.fdr=0x2B, .divider=  72},   {.fdr=0x08, .divider=  80},
+	/*{.fdr=0x2C, .divider=  80},*/ {.fdr=0x09, .divider=  88},   {.fdr=0x2D, .divider=  96},
+	  {.fdr=0x0A, .divider= 104},   {.fdr=0x2E, .divider= 112},   {.fdr=0x0B, .divider= 128},
+	/*{.fdr=0x2F, .divider= 128},*/ {.fdr=0x0C, .divider= 144},   {.fdr=0x0D, .divider= 160},
+	/*{.fdr=0x30, .divider= 160},*/ {.fdr=0x0E, .divider= 192}, /*{.fdr=0x31, .divider= 192},*/
+	  {.fdr=0x32, .divider= 224},   {.fdr=0x0F, .divider= 240},   {.fdr=0x33, .divider= 256},
+	  {.fdr=0x10, .divider= 288},   {.fdr=0x11, .divider= 320}, /*{.fdr=0x34, .divider= 320},*/
+	  {.fdr=0x12, .divider= 384}, /*{.fdr=0x35, .divider= 384},*/ {.fdr=0x36, .divider= 448},
+	/*{.fdr=0x13, .divider= 480},*/ {.fdr=0x37, .divider= 512},   {.fdr=0x14, .divider= 576},
+	  {.fdr=0x15, .divider= 640}, /*{.fdr=0x38, .divider= 640},*/ {.fdr=0x16, .divider= 768},
+	/*{.fdr=0x39, .divider= 768},*/ {.fdr=0x3A, .divider= 896},   {.fdr=0x17, .divider= 960},
+	  {.fdr=0x3B, .divider=1024},   {.fdr=0x18, .divider=1152},   {.fdr=0x19, .divider=1280},
+	/*{.fdr=0x3C, .divider=1280},*/ {.fdr=0x1A, .divider=1536}, /*{.fdr=0x3D, .divider=1536},*/
+	  {.fdr=0x3E, .divider=1792},   {.fdr=0x1B, .divider=1920},   {.fdr=0x3F, .divider=2048},
+	  {.fdr=0x1C, .divider=2304},   {.fdr=0x1D, .divider=2560},   {.fdr=0x1E, .divider=3072},
+	  {.fdr=0x1F, .divider=3840},   {.fdr=0x00, .divider=-1}
+};
+
+/**
+ * Set the I2C bus speed for a given I2C device
+ *
+ * @param dev: the I2C device
+ * @i2c_clk: I2C bus clock frequency
+ * @speed: the desired speed of the bus
+ *
+ * The I2C device must be stopped before calling this function.
+ *
+ * The return value is the actual bus speed that is set.
+ */
+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;
+	u8 fdr = 0;
+
+	/*
+	 * We want to choose an FDR/DFSR that generates an I2C bus speed that
+	 * is equal to or lower than the requested speed.  That means that we
+	 * 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) {
+			fdr = fsl_i2c_speed_map[i].fdr;
+			speed = i2c_clk / fsl_i2c_speed_map[i].divider;
+			break;
+		}
+
+	writeb(fdr, &dev->fdr);			/* set bus speed */
+
+	return speed;
+}
+#else /* ! (defined(CONFIG_MCF547x_8x) || defined(CONFIG_MCF5445x) */
+
 static const struct {
 	unsigned short divider;
 	u8 dfsr;
@@ -140,6 +207,7 @@ static unsigned int set_i2c_bus_speed(const struct fsl_i2c *dev,
 
 	return speed;
 }
+#endif /* if (defined(CONFIG_MCF547x_8x) || defined(CONFIG_MCF5445x) */
 
 void
 i2c_init(int speed, int slaveadd)
-- 
1.5.4.3

[U-Boot-Users] [PATCH 1/1] Fix I2C for m547x/548x and m5445x processors.

[Luigi 'Comio' Mantellini]

---
 drivers/i2c/fsl_i2c.c |   68 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 68 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c
index 0690340..8fd73c0 100644
--- a/drivers/i2c/fsl_i2c.c
+++ b/drivers/i2c/fsl_i2c.c
@@ -78,6 +78,73 @@ static const struct fsl_i2c *i2c_dev[2] = {
  * register.  See the application note AN2919 "Determining the I2C Frequency
  * Divider Ratio for SCL"
  */
+#if defined(CONFIG_MCF547x_8x) || defined(CONFIG_MCF5445x)
+static const struct {
+	u8 fdr;
+	unsigned short divider;
+} fsl_i2c_speed_map[] = {
+	  {.fdr=0x20, .divider=  20},   {.fdr=0x21, .divider=  22},   {.fdr=0x22, .divider=  24},
+	  {.fdr=0x23, .divider=  26},   {.fdr=0x00, .divider=  28},   {.fdr=0x24, .divider=  28},
+	  {.fdr=0x01, .divider=  30},   {.fdr=0x25, .divider=  32},   {.fdr=0x02, .divider=  34},
+	  {.fdr=0x26, .divider=  36},   {.fdr=0x03, .divider=  40},   {.fdr=0x27, .divider=  40},
+	  {.fdr=0x04, .divider=  44},   {.fdr=0x05, .divider=  48}, /*{.fdr=0x28, .divider=  48},*/
+	  {.fdr=0x06, .divider=  56}, /*{.fdr=0x29, .divider=  56},*/ {.fdr=0x2A, .divider=  64},
+	  {.fdr=0x07, .divider=  68},   {.fdr=0x2B, .divider=  72},   {.fdr=0x08, .divider=  80},
+	/*{.fdr=0x2C, .divider=  80},*/ {.fdr=0x09, .divider=  88},   {.fdr=0x2D, .divider=  96},
+	  {.fdr=0x0A, .divider= 104},   {.fdr=0x2E, .divider= 112},   {.fdr=0x0B, .divider= 128},
+	/*{.fdr=0x2F, .divider= 128},*/ {.fdr=0x0C, .divider= 144},   {.fdr=0x0D, .divider= 160},
+	/*{.fdr=0x30, .divider= 160},*/ {.fdr=0x0E, .divider= 192}, /*{.fdr=0x31, .divider= 192},*/
+	  {.fdr=0x32, .divider= 224},   {.fdr=0x0F, .divider= 240},   {.fdr=0x33, .divider= 256},
+	  {.fdr=0x10, .divider= 288},   {.fdr=0x11, .divider= 320}, /*{.fdr=0x34, .divider= 320},*/
+	  {.fdr=0x12, .divider= 384}, /*{.fdr=0x35, .divider= 384},*/ {.fdr=0x36, .divider= 448},
+	/*{.fdr=0x13, .divider= 480},*/ {.fdr=0x37, .divider= 512},   {.fdr=0x14, .divider= 576},
+	  {.fdr=0x15, .divider= 640}, /*{.fdr=0x38, .divider= 640},*/ {.fdr=0x16, .divider= 768},
+	/*{.fdr=0x39, .divider= 768},*/ {.fdr=0x3A, .divider= 896},   {.fdr=0x17, .divider= 960},
+	  {.fdr=0x3B, .divider=1024},   {.fdr=0x18, .divider=1152},   {.fdr=0x19, .divider=1280},
+	/*{.fdr=0x3C, .divider=1280},*/ {.fdr=0x1A, .divider=1536}, /*{.fdr=0x3D, .divider=1536},*/
+	  {.fdr=0x3E, .divider=1792},   {.fdr=0x1B, .divider=1920},   {.fdr=0x3F, .divider=2048},
+	  {.fdr=0x1C, .divider=2304},   {.fdr=0x1D, .divider=2560},   {.fdr=0x1E, .divider=3072},
+	  {.fdr=0x1F, .divider=3840},   {.fdr=0x00, .divider=-1}
+};
+
+/**
+ * Set the I2C bus speed for a given I2C device
+ *
+ * @param dev: the I2C device
+ * @i2c_clk: I2C bus clock frequency
+ * @speed: the desired speed of the bus
+ *
+ * The I2C device must be stopped before calling this function.
+ *
+ * The return value is the actual bus speed that is set.
+ */
+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;
+	u8 fdr = 0;
+
+	/*
+	 * We want to choose an FDR/DFSR that generates an I2C bus speed that
+	 * is equal to or lower than the requested speed.  That means that we
+	 * 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) {
+			fdr = fsl_i2c_speed_map[i].fdr;
+			speed = i2c_clk / fsl_i2c_speed_map[i].divider;
+			break;
+		}
+
+	writeb(fdr, &dev->fdr);			/* set bus speed */
+
+	return speed;
+}
+#else /* ! (defined(CONFIG_MCF547x_8x) || defined(CONFIG_MCF5445x) */
+
 static const struct {
 	unsigned short divider;
 	u8 dfsr;
@@ -140,6 +207,7 @@ static unsigned int set_i2c_bus_speed(const struct fsl_i2c *dev,
 
 	return speed;
 }
+#endif /* if (defined(CONFIG_MCF547x_8x) || defined(CONFIG_MCF5445x) */
 
 void
 i2c_init(int speed, int slaveadd)
-- 
1.5.4.3

[U-Boot-Users] [PATCH 1/1] Fix I2C for m547x/548x and m5445x processors.

[Luigi 'Comio' Mantellini]

---
 drivers/i2c/fsl_i2c.c |   68 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 68 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c
index 0690340..8fd73c0 100644
--- a/drivers/i2c/fsl_i2c.c
+++ b/drivers/i2c/fsl_i2c.c
@@ -78,6 +78,73 @@ static const struct fsl_i2c *i2c_dev[2] = {
  * register.  See the application note AN2919 "Determining the I2C Frequency
  * Divider Ratio for SCL"
  */
+#if defined(CONFIG_MCF547x_8x) || defined(CONFIG_MCF5445x)
+static const struct {
+	u8 fdr;
+	unsigned short divider;
+} fsl_i2c_speed_map[] = {
+	  {.fdr=0x20, .divider=  20},   {.fdr=0x21, .divider=  22},   {.fdr=0x22, .divider=  24},
+	  {.fdr=0x23, .divider=  26},   {.fdr=0x00, .divider=  28},   {.fdr=0x24, .divider=  28},
+	  {.fdr=0x01, .divider=  30},   {.fdr=0x25, .divider=  32},   {.fdr=0x02, .divider=  34},
+	  {.fdr=0x26, .divider=  36},   {.fdr=0x03, .divider=  40},   {.fdr=0x27, .divider=  40},
+	  {.fdr=0x04, .divider=  44},   {.fdr=0x05, .divider=  48}, /*{.fdr=0x28, .divider=  48},*/
+	  {.fdr=0x06, .divider=  56}, /*{.fdr=0x29, .divider=  56},*/ {.fdr=0x2A, .divider=  64},
+	  {.fdr=0x07, .divider=  68},   {.fdr=0x2B, .divider=  72},   {.fdr=0x08, .divider=  80},
+	/*{.fdr=0x2C, .divider=  80},*/ {.fdr=0x09, .divider=  88},   {.fdr=0x2D, .divider=  96},
+	  {.fdr=0x0A, .divider= 104},   {.fdr=0x2E, .divider= 112},   {.fdr=0x0B, .divider= 128},
+	/*{.fdr=0x2F, .divider= 128},*/ {.fdr=0x0C, .divider= 144},   {.fdr=0x0D, .divider= 160},
+	/*{.fdr=0x30, .divider= 160},*/ {.fdr=0x0E, .divider= 192}, /*{.fdr=0x31, .divider= 192},*/
+	  {.fdr=0x32, .divider= 224},   {.fdr=0x0F, .divider= 240},   {.fdr=0x33, .divider= 256},
+	  {.fdr=0x10, .divider= 288},   {.fdr=0x11, .divider= 320}, /*{.fdr=0x34, .divider= 320},*/
+	  {.fdr=0x12, .divider= 384}, /*{.fdr=0x35, .divider= 384},*/ {.fdr=0x36, .divider= 448},
+	/*{.fdr=0x13, .divider= 480},*/ {.fdr=0x37, .divider= 512},   {.fdr=0x14, .divider= 576},
+	  {.fdr=0x15, .divider= 640}, /*{.fdr=0x38, .divider= 640},*/ {.fdr=0x16, .divider= 768},
+	/*{.fdr=0x39, .divider= 768},*/ {.fdr=0x3A, .divider= 896},   {.fdr=0x17, .divider= 960},
+	  {.fdr=0x3B, .divider=1024},   {.fdr=0x18, .divider=1152},   {.fdr=0x19, .divider=1280},
+	/*{.fdr=0x3C, .divider=1280},*/ {.fdr=0x1A, .divider=1536}, /*{.fdr=0x3D, .divider=1536},*/
+	  {.fdr=0x3E, .divider=1792},   {.fdr=0x1B, .divider=1920},   {.fdr=0x3F, .divider=2048},
+	  {.fdr=0x1C, .divider=2304},   {.fdr=0x1D, .divider=2560},   {.fdr=0x1E, .divider=3072},
+	  {.fdr=0x1F, .divider=3840},   {.fdr=0x00, .divider=-1}
+};
+
+/**
+ * Set the I2C bus speed for a given I2C device
+ *
+ * @param dev: the I2C device
+ * @i2c_clk: I2C bus clock frequency
+ * @speed: the desired speed of the bus
+ *
+ * The I2C device must be stopped before calling this function.
+ *
+ * The return value is the actual bus speed that is set.
+ */
+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;
+	u8 fdr = 0;
+
+	/*
+	 * We want to choose an FDR/DFSR that generates an I2C bus speed that
+	 * is equal to or lower than the requested speed.  That means that we
+	 * 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) {
+			fdr = fsl_i2c_speed_map[i].fdr;
+			speed = i2c_clk / fsl_i2c_speed_map[i].divider;
+			break;
+		}
+
+	writeb(fdr, &dev->fdr);			/* set bus speed */
+
+	return speed;
+}
+#else /* ! (defined(CONFIG_MCF547x_8x) || defined(CONFIG_MCF5445x) */
+
 static const struct {
 	unsigned short divider;
 	u8 dfsr;
@@ -140,6 +207,7 @@ static unsigned int set_i2c_bus_speed(const struct fsl_i2c *dev,
 
 	return speed;
 }
+#endif /* if (defined(CONFIG_MCF547x_8x) || defined(CONFIG_MCF5445x) */
 
 void
 i2c_init(int speed, int slaveadd)
-- 
1.5.4.3

[U-Boot-Users] [PATCH 1/1] Fix I2C for m547x/548x and m5445x processors.

[Wolfgang Denk]

In message <1215439715-28742-2-git-send-email-luigi.mantellini@idf-hit.com> you wrote:
> ---
>  drivers/i2c/fsl_i2c.c |   68 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 68 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c
> index 0690340..8fd73c0 100644
> --- a/drivers/i2c/fsl_i2c.c
> +++ b/drivers/i2c/fsl_i2c.c
> @@ -78,6 +78,73 @@ static const struct fsl_i2c *i2c_dev[2] = {
>   * register.  See the application note AN2919 "Determining the I2C Frequency
>   * Divider Ratio for SCL"
>   */
> +#if defined(CONFIG_MCF547x_8x) || defined(CONFIG_MCF5445x)
> +static const struct {
> +	u8 fdr;
> +	unsigned short divider;
> +} fsl_i2c_speed_map[] = {
> +	  {.fdr=0x20, .divider=  20},   {.fdr=0x21, .divider=  22},   {.fdr=0x22, .divider=  24},
> +	  {.fdr=0x23, .divider=  26},   {.fdr=0x00, .divider=  28},   {.fdr=0x24, .divider=  28},
> +	  {.fdr=0x01, .divider=  30},   {.fdr=0x25, .divider=  32},   {.fdr=0x02, .divider=  34},
> +	  {.fdr=0x26, .divider=  36},   {.fdr=0x03, .divider=  40},   {.fdr=0x27, .divider=  40},
> +	  {.fdr=0x04, .divider=  44},   {.fdr=0x05, .divider=  48}, /*{.fdr=0x28, .divider=  48},*/
> +	  {.fdr=0x06, .divider=  56}, /*{.fdr=0x29, .divider=  56},*/ {.fdr=0x2A, .divider=  64},
> +	  {.fdr=0x07, .divider=  68},   {.fdr=0x2B, .divider=  72},   {.fdr=0x08, .divider=  80},
> +	/*{.fdr=0x2C, .divider=  80},*/ {.fdr=0x09, .divider=  88},   {.fdr=0x2D, .divider=  96},
> +	  {.fdr=0x0A, .divider= 104},   {.fdr=0x2E, .divider= 112},   {.fdr=0x0B, .divider= 128},
> +	/*{.fdr=0x2F, .divider= 128},*/ {.fdr=0x0C, .divider= 144},   {.fdr=0x0D, .divider= 160},
> +	/*{.fdr=0x30, .divider= 160},*/ {.fdr=0x0E, .divider= 192}, /*{.fdr=0x31, .divider= 192},*/
> +	  {.fdr=0x32, .divider= 224},   {.fdr=0x0F, .divider= 240},   {.fdr=0x33, .divider= 256},
> +	  {.fdr=0x10, .divider= 288},   {.fdr=0x11, .divider= 320}, /*{.fdr=0x34, .divider= 320},*/
> +	  {.fdr=0x12, .divider= 384}, /*{.fdr=0x35, .divider= 384},*/ {.fdr=0x36, .divider= 448},
> +	/*{.fdr=0x13, .divider= 480},*/ {.fdr=0x37, .divider= 512},   {.fdr=0x14, .divider= 576},
> +	  {.fdr=0x15, .divider= 640}, /*{.fdr=0x38, .divider= 640},*/ {.fdr=0x16, .divider= 768},
> +	/*{.fdr=0x39, .divider= 768},*/ {.fdr=0x3A, .divider= 896},   {.fdr=0x17, .divider= 960},
> +	  {.fdr=0x3B, .divider=1024},   {.fdr=0x18, .divider=1152},   {.fdr=0x19, .divider=1280},
> +	/*{.fdr=0x3C, .divider=1280},*/ {.fdr=0x1A, .divider=1536}, /*{.fdr=0x3D, .divider=1536},*/
> +	  {.fdr=0x3E, .divider=1792},   {.fdr=0x1B, .divider=1920},   {.fdr=0x3F, .divider=2048},
> +	  {.fdr=0x1C, .divider=2304},   {.fdr=0x1D, .divider=2560},   {.fdr=0x1E, .divider=3072},
> +	  {.fdr=0x1F, .divider=3840},   {.fdr=0x00, .divider=-1}
> +};
> +
> +/**
> + * Set the I2C bus speed for a given I2C device
> + *
> + * @param dev: the I2C device
> + * @i2c_clk: I2C bus clock frequency
> + * @speed: the desired speed of the bus
> + *
> + * The I2C device must be stopped before calling this function.
> + *
> + * The return value is the actual bus speed that is set.
> + */
> +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;
> +	u8 fdr = 0;
> +
> +	/*
> +	 * We want to choose an FDR/DFSR that generates an I2C bus speed that
> +	 * is equal to or lower than the requested speed.  That means that we
> +	 * 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) {
> +			fdr = fsl_i2c_speed_map[i].fdr;
> +			speed = i2c_clk / fsl_i2c_speed_map[i].divider;
> +			break;
> +		}
> +
> +	writeb(fdr, &dev->fdr);			/* set bus speed */
> +
> +	return speed;
> +}
> +#else /* ! (defined(CONFIG_MCF547x_8x) || defined(CONFIG_MCF5445x) */
> +

What a nightmare of data and code. So many magic numbers, so much
potential for errors. And for what?  Just for I2C support in U-Boot?

Are you really sure you don't simply want to use the bit-banging
driver?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Committee, n.:  A group of men who individually can do nothing but as
a group decide that nothing can be done.                 - Fred Allen

[U-Boot-Users] [PATCH 1/1] Fix I2C for m547x/548x and m5445x processors.

[Luigi 'Comio' Mantellini]

On lun, 2008-07-07 at 16:59 +0200, Wolfgang Denk wrote:
> In message <1215439715-28742-2-git-send-email-luigi.mantellini@idf-hit.com> you wrote:
....
> > +
> 
> What a nightmare of data and code. So many magic numbers, so much
> potential for errors. And for what?  Just for I2C support in U-Boot?
> 

From your PoV  you are right. 

> Are you really sure you don't simply want to use the bit-banging
> driver?
> 

I just extended the fsl_i2c.c driver that already uses a precomputed
table to setup the fsl i2c. I haven't invented these values but just
copied from the 547x and 5445x manuals. Furthermore, the h/w i2c is more
accurate that the simple bitbanging.

my 2cents.

luigi

> Best regards,
> 
> Wolfgang Denk

-- 
     ______       Luigi Mantellini
   .'______'.     R&D - Software
  (.'      '.)    Industrie Dial Face S.p.A.
  ( :=----=: )    Via Canzo, 4
  ('.______.')    20068 Peschiera Borromeo (MI), Italy
   '.______.'     Tel.: +39 02 5167 2813
                  Fax:  +39 02 5167 2459
Ind.  Dial Face   Email: luigi.mantellini at idf-hit.com
www.idf-hit.com   GPG fingerprint: 3DD1 7B71 FBDF 6376 1B4A
                                   B003 175F E979 907E 1650

[U-Boot-Users] [PATCH 1/1] Fix I2C for m547x/548x and m5445x processors.

[Wolfgang Denk]

In message <1215447465.12657.15.camel@localhost> you wrote:
>
> I just extended the fsl_i2c.c driver that already uses a precomputed

Yes, I know. I never understood what such a  complicated  driver  for
sich  a  simple  protocol  was  good  for,  especially as we don't do
multimegabytepersecond transfers over such a bus.

> table to setup the fsl i2c. I haven't invented these values but just
> copied from the 547x and 5445x manuals. Furthermore, the h/w i2c is more
> accurate that the simple bitbanging.

Accurate? There is no requirement to be "accurate"  anywhere  in  the
I2C  protocol.  It  is  a simple, brain-dead protokol where timing is
highly uncritical. And given that we need it to read a few bytes from
EEPROM or RTC or similar I really see no  benefit  in  using  such  a
complicated driver.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
How many seconds are there in a year? If I tell you there are 3.155 x
10^7, you won't even try to remember it. On the other hand, who could
forget that, to within half a percent, pi seconds is a nanocentury.
                                                - Tom Duff, Bell Labs

[U-Boot-Users] [PATCH 1/1] Fix I2C for m547x/548x and m5445x processors.

[Luigi 'Comio' Mantellini]

Il giorno lun, 07/07/2008 alle 20.02 +0200, Wolfgang Denk ha scritto:
> In message <1215447465.12657.15.camel@localhost> you wrote:
> >
> > I just extended the fsl_i2c.c driver that already uses a precomputed
> 
> Yes, I know. I never understood what such a  complicated  driver  for
> sich  a  simple  protocol  was  good  for,  especially as we don't do
> multimegabytepersecond transfers over such a bus.
> 

I agree with you. But I prefer to have a (too) complex working driver
instead an buggy driver. In other hands if m547x cpus have to use the
fsl_driver... this should be fixed in order to work fine. Otherwise the
HW i2c driver should be dropped from m57x/m5445x boards in order to
avoid to confuse the u-boot users/developers.

Today my HW-man ask me why the i2c bus ran at 1Mhz... The cause was that
I used the driver with confidence... too confidence...

> > table to setup the fsl i2c. I haven't invented these values but just
> > copied from the 547x and 5445x manuals. Furthermore, the h/w i2c is more
> > accurate that the simple bitbanging.
> 
> Accurate? There is no requirement to be "accurate"  anywhere  in  the
> I2C  protocol.  It  is  a simple, brain-dead protokol where timing is
> highly uncritical. And given that we need it to read a few bytes from
> EEPROM or RTC or similar I really see no  benefit  in  using  such  a
> complicated driver.
> 

I know :) but I like see 100kHz +/-0.01% when I configure 100kHz... I'm
kidding, of course. eheheh...
In the last time I preferred to disable i2c driver on all my
applications... I ask me why all embedded controller on the SoC that I'm
using are bugged... are the masks too expensive to work fine?

Ciao ciao

luigi

> Best regards,
> 
> Wolfgang Denk
> 

Ing. Luigi Mantellini
Industrie Dial Face S.p.A.
Via Canzo, 4 
20068 Peschiera Borromeo (MI)
Tel.: +39 02 5167 2813
Fax: +39 02 5167 2459
E-mail: luigi.mantellini at idf-hit.com

[U-Boot-Users] [PATCH 1/1] Fix I2C for m547x/548x and m5445x processors.

[Timur Tabi]

On Jul 7, 2008, at 2:02 PM, Wolfgang Denk wrote:

> In message <1215447465.12657.15.camel@localhost> you wrote:
>>
>> I just extended the fsl_i2c.c driver that already uses a precomputed
>
> Yes, I know. I never understood what such a  complicated  driver  for
> sich  a  simple  protocol  was  good  for,  especially as we don't do
> multimegabytepersecond transfers over such a bus.

The driver is no more complicated than the hardware itself.  The  
Freescale I2C controller has a complicated, screwball method for  
setting the I2C bus speed, and the driver tries to simplify that as  
much as possible.  Setting the I2C bus speed *may* be important on  
some boards with flaky I2C devices.

> Accurate? There is no requirement to be "accurate"  anywhere  in  the
> I2C  protocol.

Some devices cannot handle certain bus speeds, and some devices may  
work better at higher bus speeds, so it may be important to control  
the bus speed accurately.

However, Luigi's patch is bad because it modifies a common file for a  
specific board.  I'm going to NAK it.

[U-Boot-Users] [PATCH 1/1] Fix I2C for m547x/548x and m5445x processors.

[Timur Tabi]

On Jul 7, 2008, at 10:17 AM, Luigi 'Comio' Mantellini wrote:

> ---
> drivers/i2c/fsl_i2c.c |   68 ++++++++++++++++++++++++++++++++++++++++ 
> +++++++++
> 1 files changed, 68 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c
> index 0690340..8fd73c0 100644
> --- a/drivers/i2c/fsl_i2c.c
> +++ b/drivers/i2c/fsl_i2c.c
> @@ -78,6 +78,73 @@ static const struct fsl_i2c *i2c_dev[2] = {
>  * register.  See the application note AN2919 "Determining the I2C  
> Frequency
>  * Divider Ratio for SCL"
>  */
> +#if defined(CONFIG_MCF547x_8x) || defined(CONFIG_MCF5445x)
> +static const struct {
> +	u8 fdr;
> +	unsigned short divider;
> +} fsl_i2c_speed_map[] = {
> +	  {.fdr=0x20, .divider=  20},   {.fdr=0x21, .divider=  22},    
> {.fdr=0x22, .divider=  24},
> +	  {.fdr=0x23, .divider=  26},   {.fdr=0x00, .divider=  28},    
> {.fdr=0x24, .divider=  28},
> +	  {.fdr=0x01, .divider=  30},   {.fdr=0x25, .divider=  32},    
> {.fdr=0x02, .divider=  34},
> +	  {.fdr=0x26, .divider=  36},   {.fdr=0x03, .divider=  40},    
> {.fdr=0x27, .divider=  40},
> +	  {.fdr=0x04, .divider=  44},   {.fdr=0x05, .divider=  48}, / 
> *{.fdr=0x28, .divider=  48},*/
> +	  {.fdr=0x06, .divider=  56}, /*{.fdr=0x29, .divider=  56},*/  
> {.fdr=0x2A, .divider=  64},
> +	  {.fdr=0x07, .divider=  68},   {.fdr=0x2B, .divider=  72},    
> {.fdr=0x08, .divider=  80},
> +	/*{.fdr=0x2C, .divider=  80},*/ {.fdr=0x09, .divider=  88},    
> {.fdr=0x2D, .divider=  96},
> +	  {.fdr=0x0A, .divider= 104},   {.fdr=0x2E, .divider= 112},    
> {.fdr=0x0B, .divider= 128},
> +	/*{.fdr=0x2F, .divider= 128},*/ {.fdr=0x0C, .divider= 144},    
> {.fdr=0x0D, .divider= 160},
> +	/*{.fdr=0x30, .divider= 160},*/ {.fdr=0x0E, .divider= 192}, / 
> *{.fdr=0x31, .divider= 192},*/
> +	  {.fdr=0x32, .divider= 224},   {.fdr=0x0F, .divider= 240},    
> {.fdr=0x33, .divider= 256},
> +	  {.fdr=0x10, .divider= 288},   {.fdr=0x11, .divider= 320}, / 
> *{.fdr=0x34, .divider= 320},*/
> +	  {.fdr=0x12, .divider= 384}, /*{.fdr=0x35, .divider= 384},*/  
> {.fdr=0x36, .divider= 448},
> +	/*{.fdr=0x13, .divider= 480},*/ {.fdr=0x37, .divider= 512},    
> {.fdr=0x14, .divider= 576},
> +	  {.fdr=0x15, .divider= 640}, /*{.fdr=0x38, .divider= 640},*/  
> {.fdr=0x16, .divider= 768},
> +	/*{.fdr=0x39, .divider= 768},*/ {.fdr=0x3A, .divider= 896},    
> {.fdr=0x17, .divider= 960},
> +	  {.fdr=0x3B, .divider=1024},   {.fdr=0x18, .divider=1152},    
> {.fdr=0x19, .divider=1280},
> +	/*{.fdr=0x3C, .divider=1280},*/ {.fdr=0x1A, .divider=1536}, / 
> *{.fdr=0x3D, .divider=1536},*/
> +	  {.fdr=0x3E, .divider=1792},   {.fdr=0x1B, .divider=1920},    
> {.fdr=0x3F, .divider=2048},
> +	  {.fdr=0x1C, .divider=2304},   {.fdr=0x1D, .divider=2560},    
> {.fdr=0x1E, .divider=3072},
> +	  {.fdr=0x1F, .divider=3840},   {.fdr=0x00, .divider=-1}
> +};

NAK.  There's a better way to do this.  Don't just copy/paste my data  
structures and comment-out specific fields like this.

I'm on vacation this week.  Next week, I'll take a closer look at your  
patch and suggest something better.

What is the reason you are commenting-out those specific entries in  
fsl_i2c_speed_map[]?

[U-Boot-Users] [PATCH 1/1] Fix I2C for m547x/548x and m5445x processors.

[Luigi 'Comio' Mantellini]

On mer, 2008-07-09 at 09:25 -0400, Timur Tabi wrote:

> On Jul 7, 2008, at 10:17 AM, Luigi 'Comio' Mantellini wrote:
> 
> > ---
> > drivers/i2c/fsl_i2c.c |   68 ++++++++++++++++++++++++++++++++++++++++ 
> > +++++++++
> > 1 files changed, 68 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c
> > index 0690340..8fd73c0 100644
> > --- a/drivers/i2c/fsl_i2c.c
> > +++ b/drivers/i2c/fsl_i2c.c
> > @@ -78,6 +78,73 @@ static const struct fsl_i2c *i2c_dev[2] = {

...

> > +	  {.fdr=0x3B, .divider=1024},   {.fdr=0x18, .divider=1152},    
> > {.fdr=0x19, .divider=1280},
> > +	/*{.fdr=0x3C, .divider=1280},*/ {.fdr=0x1A, .divider=1536}, / 
> > *{.fdr=0x3D, .divider=1536},*/

....

> NAK.  There's a better way to do this.  Don't just copy/paste my data  
> structures and comment-out specific fields like this.
> 


The values are from the 547x manual because the fsl_i2c on coldfire is a
little different (e.g. there isn't the default filter register).
The entries commented are just synonymous of others. For example the
divider 1280 can be obtained using both the fdr values 0x19 and 0x3C
(always on coldfire cpus). 



> I'm on vacation this week.  Next week, I'll take a closer look at your  
> patch and suggest something better.
> 


Thanks, I just reported a problem (i think)... the patches is just the
quick solution that I have adopted on my application.

Best regards and have a good vacation!


luigi



> What is the reason you are commenting-out those specific entries in  
> fsl_i2c_speed_map[]?





Industrie Dial Face S.p.A.
Luigi Mantellini
R&D - Software
Industrie Dial Face S.p.A.
Via Canzo, 4 
20068 Peschiera Borromeo (MI), Italy
Tel.:   +39 02 5167 2813
Fax:    +39 02 5167 2459
E-mail: luigi.mantellini at idf-hit.com
GPG fingerprint: 3DD1 7B71 FBDF 6376
1B4A
                 B003 175F E979 907E
1650
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.denx.de/pipermail/u-boot/attachments/20080709/d7ad416b/attachment.htm 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: idf_logo.gif
Type: image/gif
Size: 4122 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20080709/d7ad416b/attachment.gif