[PATCHv7 0/3] I2C driver updates

[PATCHv7 0/3] I2C driver updates

[Shubhrajyoti D]

The series attempts to do the following

- The reset should not be done in the driver 
  have support for the same.
- Remove the sysc register access in the driver.


Incorporated review comments:

 - Fix the indentation.
 - Restore in the error path is not needed as we are
   doing a init.
 - Combine the patch 1 and 2 as i2c-omap.h isn't a generic header
 - Making the changelogs more descriptive.
 
This is rebased on Kevin's tree  for_3.2/fixes/i2c branch.


Acknowledge Balaji ,Santosh and Kevin for the review comments.


Shubhrajyoti D (3):
  OMAP: I2C: Reset support
  OMAP: I2C: Remove the reset in the init path
  OMAP: I2C: Remove the SYSC register definition

 arch/arm/plat-omap/i2c.c      |   18 ++++++++
 drivers/i2c/busses/i2c-omap.c |   87 ++++++++++------------------------------
 include/linux/i2c-omap.h      |    1 +
 3 files changed, 41 insertions(+), 65 deletions(-)

[PATCHv7 1/3] OMAP: I2C: Reset support

[Shubhrajyoti D]

Under some error conditions the i2c driver may do a reset.
Adding a reset field and support in the device-specific code.

Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
---
 arch/arm/plat-omap/i2c.c |   18 ++++++++++++++++++
 include/linux/i2c-omap.h |    1 +
 2 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c
index db071bc..883229c 100644
--- a/arch/arm/plat-omap/i2c.c
+++ b/arch/arm/plat-omap/i2c.c
@@ -74,6 +74,22 @@ static struct omap_i2c_bus_platform_data i2c_pdata[OMAP_I2C_MAX_CONTROLLERS];
 static struct platform_device omap_i2c_devices[] = {
 	I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_pdata[0]),
 };
+/**
+ * omap2_i2c_reset - reset the omap i2c module.
+ * @dev: struct device*
+ */
+
+static int omap2_i2c_reset(struct device *dev)
+{
+	int r = 0;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct omap_device *odev = to_omap_device(pdev);
+	struct omap_hwmod *oh;
+
+	oh = odev->hwmods[0];
+	r = omap_hwmod_reset(oh);
+	return r;
+}
 
 #define OMAP_I2C_CMDLINE_SETUP	(BIT(31))
 
@@ -179,6 +195,8 @@ static inline int omap2_i2c_add_bus(int bus_id)
 	 */
 	if (cpu_is_omap34xx())
 		pdata->set_mpu_wkup_lat = omap_pm_set_max_mpu_wakeup_lat_compat;
+
+	pdata->device_reset = omap2_i2c_reset;
 	pdev = omap_device_build(name, bus_id, oh, pdata,
 			sizeof(struct omap_i2c_bus_platform_data),
 			NULL, 0, 0);
diff --git a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h
index 92a0dc7..fd38249 100644
--- a/include/linux/i2c-omap.h
+++ b/include/linux/i2c-omap.h
@@ -35,6 +35,7 @@ struct omap_i2c_bus_platform_data {
 	u32		rev;
 	u32		flags;
 	void		(*set_mpu_wkup_lat)(struct device *dev, long set);
+	int		(*device_reset) (struct device *dev);
 };
 
 #endif
-- 
1.7.1

Re: [PATCHv7 1/3] OMAP: I2C: Reset support

[Paul Walmsley]

Hi

some comments

On Fri, 2 Dec 2011, Shubhrajyoti D wrote:

> Under some error conditions the i2c driver may do a reset.
> Adding a reset field and support in the device-specific code.
> 
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
>  arch/arm/plat-omap/i2c.c |   18 ++++++++++++++++++
>  include/linux/i2c-omap.h |    1 +
>  2 files changed, 19 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c
> index db071bc..883229c 100644
> --- a/arch/arm/plat-omap/i2c.c
> +++ b/arch/arm/plat-omap/i2c.c
> @@ -74,6 +74,22 @@ static struct omap_i2c_bus_platform_data i2c_pdata[OMAP_I2C_MAX_CONTROLLERS];
>  static struct platform_device omap_i2c_devices[] = {
>  	I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_pdata[0]),
>  };
> +/**
> + * omap2_i2c_reset - reset the omap i2c module.
> + * @dev: struct device*
> + */
> +
> +static int omap2_i2c_reset(struct device *dev)
> +{
> +	int r = 0;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct omap_device *odev = to_omap_device(pdev);
> +	struct omap_hwmod *oh;
> +
> +	oh = odev->hwmods[0];
> +	r = omap_hwmod_reset(oh);
> +	return r;
> +}

This function should go into arch/arm/plat-omap/omap_device.c, and it 
should be renamed to omap_device_reset(), since it's not I2C specific.  
Also it shouldn't assume that there's only one hwmod - it should iterate 
over all of the hwmods in the omap_device and call omap_hwmod_reset() on 
each.

>  
>  #define OMAP_I2C_CMDLINE_SETUP	(BIT(31))
>  
> @@ -179,6 +195,8 @@ static inline int omap2_i2c_add_bus(int bus_id)
>  	 */
>  	if (cpu_is_omap34xx())
>  		pdata->set_mpu_wkup_lat = omap_pm_set_max_mpu_wakeup_lat_compat;
> +
> +	pdata->device_reset = omap2_i2c_reset;

omap_device_reset

>  	pdev = omap_device_build(name, bus_id, oh, pdata,
>  			sizeof(struct omap_i2c_bus_platform_data),
>  			NULL, 0, 0);
> diff --git a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h
> index 92a0dc7..fd38249 100644
> --- a/include/linux/i2c-omap.h
> +++ b/include/linux/i2c-omap.h
> @@ -35,6 +35,7 @@ struct omap_i2c_bus_platform_data {
>  	u32		rev;
>  	u32		flags;
>  	void		(*set_mpu_wkup_lat)(struct device *dev, long set);
> +	int		(*device_reset) (struct device *dev);
>  };
>  
>  #endif
> -- 
> 1.7.1
> 
> --
> 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
> 


- Paul

Re: [PATCHv7 1/3] OMAP: I2C: Reset support

[Shubhrajyoti]

On Friday 09 December 2011 06:45 AM, Paul Walmsley wrote:
> Hi
>
> some comments
>
> On Fri, 2 Dec 2011, Shubhrajyoti D wrote:
>
>> Under some error conditions the i2c driver may do a reset.
>> Adding a reset field and support in the device-specific code.
>>
>> Signed-off-by: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>
>> ---
>>  arch/arm/plat-omap/i2c.c |   18 ++++++++++++++++++
>>  include/linux/i2c-omap.h |    1 +
>>  2 files changed, 19 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c
>> index db071bc..883229c 100644
>> --- a/arch/arm/plat-omap/i2c.c
>> +++ b/arch/arm/plat-omap/i2c.c
>> @@ -74,6 +74,22 @@ static struct omap_i2c_bus_platform_data i2c_pdata[OMAP_I2C_MAX_CONTROLLERS];
>>  static struct platform_device omap_i2c_devices[] = {
>>  	I2C_DEV_BUILDER(1, i2c_resources[0], &i2c_pdata[0]),
>>  };
>> +/**
>> + * omap2_i2c_reset - reset the omap i2c module.
>> + * @dev: struct device*
>> + */
>> +
>> +static int omap2_i2c_reset(struct device *dev)
>> +{
>> +	int r = 0;
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct omap_device *odev = to_omap_device(pdev);
>> +	struct omap_hwmod *oh;
>> +
>> +	oh = odev->hwmods[0];
>> +	r = omap_hwmod_reset(oh);
>> +	return r;
>> +}
> This function should go into arch/arm/plat-omap/omap_device.c, and it 
agree
> should be renamed to omap_device_reset(), since it's not I2C specific.  
> Also it shouldn't assume that there's only one hwmod - it should iterate 
> over all of the hwmods in the omap_device and call omap_hwmod_reset() on 
> each.
Yes agree will fix in the next version.

>>  
>>  #define OMAP_I2C_CMDLINE_SETUP	(BIT(31))
>>  
>> @@ -179,6 +195,8 @@ static inline int omap2_i2c_add_bus(int bus_id)
>>  	 */
>>  	if (cpu_is_omap34xx())
>>  		pdata->set_mpu_wkup_lat = omap_pm_set_max_mpu_wakeup_lat_compat;
>> +
>> +	pdata->device_reset = omap2_i2c_reset;
> omap_device_reset
>
>>  	pdev = omap_device_build(name, bus_id, oh, pdata,
>>  			sizeof(struct omap_i2c_bus_platform_data),
>>  			NULL, 0, 0);
>> diff --git a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h
>> index 92a0dc7..fd38249 100644
>> --- a/include/linux/i2c-omap.h
>> +++ b/include/linux/i2c-omap.h
>> @@ -35,6 +35,7 @@ struct omap_i2c_bus_platform_data {
>>  	u32		rev;
>>  	u32		flags;
>>  	void		(*set_mpu_wkup_lat)(struct device *dev, long set);
>> +	int		(*device_reset) (struct device *dev);
>>  };
>>  
>>  #endif
>> -- 
>> 1.7.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
> - Paul

[PATCHv7 2/3] OMAP: I2C: Remove the reset in the init path

[Shubhrajyoti D]

-  The reset in the driver at init is not needed anymore as the
   hwmod framework takes care of reseting it.
-  Reset is removed from omap_i2c_init, which was called
   not only during probe, but also after time out and error handling.
   device_reset were added in those places to effect the reset.
-  Earlier the hwmod SYSC settings were over-written in the driver.
   Removing the same and letting the hwmod take care of the settings.
-  Clean up the SYSS_RESETDONE_MASK macro as it is no longer needed.
-  Clean up the SYSCONFIG SYSC bit defination macros.
-  Fix the typos in wakeup.

Signed-off-by: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>
---
 drivers/i2c/busses/i2c-omap.c |   82 +++++++++++------------------------------
 1 files changed, 22 insertions(+), 60 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index fa23faa..beff9f3 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -155,19 +155,6 @@ enum {
 #define OMAP_I2C_SYSTEST_SDA_O		(1 << 0)	/* SDA line drive out */
 #endif
 
-/* OCP_SYSSTATUS bit definitions */
-#define SYSS_RESETDONE_MASK		(1 << 0)
-
-/* OCP_SYSCONFIG bit definitions */
-#define SYSC_CLOCKACTIVITY_MASK		(0x3 << 8)
-#define SYSC_SIDLEMODE_MASK		(0x3 << 3)
-#define SYSC_ENAWAKEUP_MASK		(1 << 2)
-#define SYSC_SOFTRESET_MASK		(1 << 1)
-#define SYSC_AUTOIDLE_MASK		(1 << 0)
-
-#define SYSC_IDLEMODE_SMART		0x2
-#define SYSC_CLOCKACTIVITY_FCLK		0x2
-
 /* Errata definitions */
 #define I2C_OMAP_ERRATA_I207		(1 << 0)
 #define I2C_OMAP3_1P153			(1 << 1)
@@ -182,6 +169,7 @@ struct omap_i2c_dev {
 	u32			latency;	/* maximum mpu wkup latency */
 	void			(*set_mpu_wkup_lat)(struct device *dev,
 						    long latency);
+	int			(*device_reset)(struct device *dev);
 	u32			speed;		/* Speed of bus in Khz */
 	u16			cmd_err;
 	u8			*buf;
@@ -317,60 +305,23 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
 	u16 psc = 0, scll = 0, sclh = 0, buf = 0;
 	u16 fsscll = 0, fssclh = 0, hsscll = 0, hssclh = 0;
 	unsigned long fclk_rate = 12000000;
-	unsigned long timeout;
 	unsigned long internal_clk = 0;
 	struct clk *fclk;
 	struct omap_i2c_bus_platform_data *pdata;
 
 	pdata = dev->dev->platform_data;
 
-	if (dev->rev >= OMAP_I2C_OMAP1_REV_2) {
-		/* Disable I2C controller before soft reset */
-		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
-			omap_i2c_read_reg(dev, OMAP_I2C_CON_REG) &
-				~(OMAP_I2C_CON_EN));
-
-		omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, SYSC_SOFTRESET_MASK);
-		/* For some reason we need to set the EN bit before the
-		 * reset done bit gets set. */
-		timeout = jiffies + OMAP_I2C_TIMEOUT;
-		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
-		while (!(omap_i2c_read_reg(dev, OMAP_I2C_SYSS_REG) &
-			 SYSS_RESETDONE_MASK)) {
-			if (time_after(jiffies, timeout)) {
-				dev_warn(dev->dev, "timeout waiting "
-						"for controller reset\n");
-				return -ETIMEDOUT;
-			}
-			msleep(1);
-		}
-
-		/* SYSC register is cleared by the reset; rewrite it */
-		if (dev->rev == OMAP_I2C_REV_ON_2430) {
-
-			omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG,
-					   SYSC_AUTOIDLE_MASK);
-
-		} else if (dev->rev >= OMAP_I2C_REV_ON_3430) {
-			dev->syscstate = SYSC_AUTOIDLE_MASK;
-			dev->syscstate |= SYSC_ENAWAKEUP_MASK;
-			dev->syscstate |= (SYSC_IDLEMODE_SMART <<
-			      __ffs(SYSC_SIDLEMODE_MASK));
-			dev->syscstate |= (SYSC_CLOCKACTIVITY_FCLK <<
-			      __ffs(SYSC_CLOCKACTIVITY_MASK));
-
-			omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG,
-							dev->syscstate);
-			/*
-			 * Enabling all wakup sources to stop I2C freezing on
-			 * WFI instruction.
-			 * REVISIT: Some wkup sources might not be needed.
-			 */
-			dev->westate = OMAP_I2C_WE_ALL;
-			omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
-							dev->westate);
-		}
+	if (dev->rev >= OMAP_I2C_REV_ON_3430) {
+		/*
+		 * Enabling all wakeup sources to stop I2C freezing on
+		 * WFI instruction.
+		 * REVISIT: Some wakeup sources might not be needed.
+		 */
+		dev->westate = OMAP_I2C_WE_ALL;
+		omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
+						dev->westate);
 	}
+
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
 
 	if (pdata->flags & OMAP_I2C_FLAG_ALWAYS_ARMXOR_CLK) {
@@ -594,6 +545,11 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 		return r;
 	if (r == 0) {
 		dev_err(dev->dev, "controller timed out\n");
+		if (dev->device_reset) {
+			r = dev->device_reset(dev->dev);
+			if (r < 0)
+				dev_err(dev->dev, "reset failed\n");
+		}
 		omap_i2c_init(dev);
 		return -ETIMEDOUT;
 	}
@@ -604,6 +560,11 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 	/* We have an error */
 	if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
 			    OMAP_I2C_STAT_XUDF)) {
+		if (dev->device_reset) {
+			r = dev->device_reset(dev->dev);
+			if (r < 0)
+				dev_err(dev->dev, "reset failed\n");
+		}
 		omap_i2c_init(dev);
 		return -EIO;
 	}
@@ -1004,6 +965,7 @@ omap_i2c_probe(struct platform_device *pdev)
 	if (pdata != NULL) {
 		speed = pdata->clkrate;
 		dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
+		dev->device_reset = pdata->device_reset;
 	} else {
 		speed = 100;	/* Default speed */
 		dev->set_mpu_wkup_lat = NULL;
-- 
1.7.1

Re: [PATCHv7 2/3] OMAP: I2C: Remove the reset in the init path

[Jon Hunter]

Hi Shubhrajyoti,

On 12/2/2011 3:21, Shubhrajyoti D wrote:
> -  The reset in the driver at init is not needed anymore as the
>     hwmod framework takes care of reseting it.
> -  Reset is removed from omap_i2c_init, which was called
>     not only during probe, but also after time out and error handling.
>     device_reset were added in those places to effect the reset.
> -  Earlier the hwmod SYSC settings were over-written in the driver.
>     Removing the same and letting the hwmod take care of the settings.
> -  Clean up the SYSS_RESETDONE_MASK macro as it is no longer needed.
> -  Clean up the SYSCONFIG SYSC bit defination macros.
> -  Fix the typos in wakeup.
>
> Signed-off-by: Shubhrajyoti D<shubhrajyoti@ti.com>
> ---
>   drivers/i2c/busses/i2c-omap.c |   82 +++++++++++------------------------------
>   1 files changed, 22 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index fa23faa..beff9f3 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -155,19 +155,6 @@ enum {
>   #define OMAP_I2C_SYSTEST_SDA_O		(1<<  0)	/* SDA line drive out */
>   #endif
>
> -/* OCP_SYSSTATUS bit definitions */
> -#define SYSS_RESETDONE_MASK		(1<<  0)
> -
> -/* OCP_SYSCONFIG bit definitions */
> -#define SYSC_CLOCKACTIVITY_MASK		(0x3<<  8)
> -#define SYSC_SIDLEMODE_MASK		(0x3<<  3)
> -#define SYSC_ENAWAKEUP_MASK		(1<<  2)
> -#define SYSC_SOFTRESET_MASK		(1<<  1)
> -#define SYSC_AUTOIDLE_MASK		(1<<  0)
> -
> -#define SYSC_IDLEMODE_SMART		0x2
> -#define SYSC_CLOCKACTIVITY_FCLK		0x2
> -
>   /* Errata definitions */
>   #define I2C_OMAP_ERRATA_I207		(1<<  0)
>   #define I2C_OMAP3_1P153			(1<<  1)
> @@ -182,6 +169,7 @@ struct omap_i2c_dev {
>   	u32			latency;	/* maximum mpu wkup latency */
>   	void			(*set_mpu_wkup_lat)(struct device *dev,
>   						    long latency);
> +	int			(*device_reset)(struct device *dev);
>   	u32			speed;		/* Speed of bus in Khz */
>   	u16			cmd_err;
>   	u8			*buf;
> @@ -317,60 +305,23 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
>   	u16 psc = 0, scll = 0, sclh = 0, buf = 0;
>   	u16 fsscll = 0, fssclh = 0, hsscll = 0, hssclh = 0;
>   	unsigned long fclk_rate = 12000000;
> -	unsigned long timeout;
>   	unsigned long internal_clk = 0;
>   	struct clk *fclk;
>   	struct omap_i2c_bus_platform_data *pdata;
>
>   	pdata = dev->dev->platform_data;
>
> -	if (dev->rev>= OMAP_I2C_OMAP1_REV_2) {
> -		/* Disable I2C controller before soft reset */
> -		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
> -			omap_i2c_read_reg(dev, OMAP_I2C_CON_REG)&
> -				~(OMAP_I2C_CON_EN));
> -
> -		omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, SYSC_SOFTRESET_MASK);
> -		/* For some reason we need to set the EN bit before the
> -		 * reset done bit gets set. */
> -		timeout = jiffies + OMAP_I2C_TIMEOUT;
> -		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
> -		while (!(omap_i2c_read_reg(dev, OMAP_I2C_SYSS_REG)&
> -			 SYSS_RESETDONE_MASK)) {
> -			if (time_after(jiffies, timeout)) {
> -				dev_warn(dev->dev, "timeout waiting "
> -						"for controller reset\n");
> -				return -ETIMEDOUT;
> -			}
> -			msleep(1);
> -		}
> -
> -		/* SYSC register is cleared by the reset; rewrite it */
> -		if (dev->rev == OMAP_I2C_REV_ON_2430) {
> -
> -			omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG,
> -					   SYSC_AUTOIDLE_MASK);
> -
> -		} else if (dev->rev>= OMAP_I2C_REV_ON_3430) {
> -			dev->syscstate = SYSC_AUTOIDLE_MASK;
> -			dev->syscstate |= SYSC_ENAWAKEUP_MASK;
> -			dev->syscstate |= (SYSC_IDLEMODE_SMART<<
> -			      __ffs(SYSC_SIDLEMODE_MASK));
> -			dev->syscstate |= (SYSC_CLOCKACTIVITY_FCLK<<
> -			      __ffs(SYSC_CLOCKACTIVITY_MASK));

The issue I see with this change is that you are removing the above code 
to set the CLKACTIVITY field. Today, AFAIK, hwmod does not set this. 
Ideally it should. However, from discussing this with Richard W, this 
can cause timeouts to occur for i2c transactions. So before removing 
this we need to make sure that this is handled by hwmod or else where.

> -			omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG,
> -							dev->syscstate);
> -			/*
> -			 * Enabling all wakup sources to stop I2C freezing on
> -			 * WFI instruction.
> -			 * REVISIT: Some wkup sources might not be needed.
> -			 */
> -			dev->westate = OMAP_I2C_WE_ALL;
> -			omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
> -							dev->westate);
> -		}
> +	if (dev->rev>= OMAP_I2C_REV_ON_3430) {
> +		/*
> +		 * Enabling all wakeup sources to stop I2C freezing on
> +		 * WFI instruction.
> +		 * REVISIT: Some wakeup sources might not be needed.
> +		 */
> +		dev->westate = OMAP_I2C_WE_ALL;
> +		omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
> +						dev->westate);
>   	}
> +
>   	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
>
>   	if (pdata->flags&  OMAP_I2C_FLAG_ALWAYS_ARMXOR_CLK) {
> @@ -594,6 +545,11 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
>   		return r;
>   	if (r == 0) {
>   		dev_err(dev->dev, "controller timed out\n");
> +		if (dev->device_reset) {
> +			r = dev->device_reset(dev->dev);
> +			if (r<  0)
> +				dev_err(dev->dev, "reset failed\n");
> +		}
>   		omap_i2c_init(dev);

Why put the reset here? The function omap_i2c_init is going to perform a 
soft reset. So why not replace the reset in that function?

Furthermore does this work for omap1 devices? I think that you would 
need to remove the existing soft-reset from omap_i2c_init() into an omap1.

>   		return -ETIMEDOUT;
>   	}
> @@ -604,6 +560,11 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
>   	/* We have an error */
>   	if (dev->cmd_err&  (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
>   			    OMAP_I2C_STAT_XUDF)) {
> +		if (dev->device_reset) {
> +			r = dev->device_reset(dev->dev);
> +			if (r<  0)
> +				dev_err(dev->dev, "reset failed\n");
> +		}
>   		omap_i2c_init(dev);

Same as above.

>   		return -EIO;
>   	}
> @@ -1004,6 +965,7 @@ omap_i2c_probe(struct platform_device *pdev)
>   	if (pdata != NULL) {
>   		speed = pdata->clkrate;
>   		dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
> +		dev->device_reset = pdata->device_reset;
>   	} else {
>   		speed = 100;	/* Default speed */
>   		dev->set_mpu_wkup_lat = NULL;

Cheers
Jon

Re: [PATCHv7 2/3] OMAP: I2C: Remove the reset in the init path

[Jon Hunter]

Hi Shubhrajyoti,

On 12/2/2011 15:37, Jon Hunter wrote:

[snip]
>> @@ -594,6 +545,11 @@ static int omap_i2c_xfer_msg(struct i2c_adapter
>> *adap,
>> return r;
>> if (r == 0) {
>> dev_err(dev->dev, "controller timed out\n");
>> + if (dev->device_reset) {
>> + r = dev->device_reset(dev->dev);
>> + if (r< 0)
>> + dev_err(dev->dev, "reset failed\n");
>> + }
>> omap_i2c_init(dev);
>
> Why put the reset here? The function omap_i2c_init is going to perform a
> soft reset. So why not replace the reset in that function?
>
> Furthermore does this work for omap1 devices? I think that you would
> need to remove the existing soft-reset from omap_i2c_init() into an omap1.

Sorry, I see you did remove the soft-reset in the omap_i2c_init(). 
However, why not just replace the reset in the omap_i2c_init() instead 
of moving it?

As for omap1, I see we don't perform a soft-reset so that is a don't care.

Cheers
Jon

Re: [PATCHv7 2/3] OMAP: I2C: Remove the reset in the init path

[Shubhrajyoti]

On Saturday 03 December 2011 03:07 AM, Jon Hunter wrote:
> Hi Shubhrajyoti,
>
> On 12/2/2011 3:21, Shubhrajyoti D wrote:
>> -  The reset in the driver at init is not needed anymore as the
>>     hwmod framework takes care of reseting it.
>> -  Reset is removed from omap_i2c_init, which was called
>>     not only during probe, but also after time out and error handling.
>>     device_reset were added in those places to effect the reset.
>> -  Earlier the hwmod SYSC settings were over-written in the driver.
>>     Removing the same and letting the hwmod take care of the settings.
>> -  Clean up the SYSS_RESETDONE_MASK macro as it is no longer needed.
>> -  Clean up the SYSCONFIG SYSC bit defination macros.
>> -  Fix the typos in wakeup.
>>
>> Signed-off-by: Shubhrajyoti D<shubhrajyoti-l0cyMroinI0@public.gmane.org>
>> ---
>>   drivers/i2c/busses/i2c-omap.c |   82
>> +++++++++++------------------------------
>>   1 files changed, 22 insertions(+), 60 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-omap.c
>> b/drivers/i2c/busses/i2c-omap.c
>> index fa23faa..beff9f3 100644
>> --- a/drivers/i2c/busses/i2c-omap.c
>> +++ b/drivers/i2c/busses/i2c-omap.c
>> @@ -155,19 +155,6 @@ enum {
>>   #define OMAP_I2C_SYSTEST_SDA_O        (1<<  0)    /* SDA line drive
>> out */
>>   #endif
>>
>> -/* OCP_SYSSTATUS bit definitions */
>> -#define SYSS_RESETDONE_MASK        (1<<  0)
>> -
>> -/* OCP_SYSCONFIG bit definitions */
>> -#define SYSC_CLOCKACTIVITY_MASK        (0x3<<  8)
>> -#define SYSC_SIDLEMODE_MASK        (0x3<<  3)
>> -#define SYSC_ENAWAKEUP_MASK        (1<<  2)
>> -#define SYSC_SOFTRESET_MASK        (1<<  1)
>> -#define SYSC_AUTOIDLE_MASK        (1<<  0)
>> -
>> -#define SYSC_IDLEMODE_SMART        0x2
>> -#define SYSC_CLOCKACTIVITY_FCLK        0x2
>> -
>>   /* Errata definitions */
>>   #define I2C_OMAP_ERRATA_I207        (1<<  0)
>>   #define I2C_OMAP3_1P153            (1<<  1)
>> @@ -182,6 +169,7 @@ struct omap_i2c_dev {
>>       u32            latency;    /* maximum mpu wkup latency */
>>       void            (*set_mpu_wkup_lat)(struct device *dev,
>>                               long latency);
>> +    int            (*device_reset)(struct device *dev);
>>       u32            speed;        /* Speed of bus in Khz */
>>       u16            cmd_err;
>>       u8            *buf;
>> @@ -317,60 +305,23 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
>>       u16 psc = 0, scll = 0, sclh = 0, buf = 0;
>>       u16 fsscll = 0, fssclh = 0, hsscll = 0, hssclh = 0;
>>       unsigned long fclk_rate = 12000000;
>> -    unsigned long timeout;
>>       unsigned long internal_clk = 0;
>>       struct clk *fclk;
>>       struct omap_i2c_bus_platform_data *pdata;
>>
>>       pdata = dev->dev->platform_data;
>>
>> -    if (dev->rev>= OMAP_I2C_OMAP1_REV_2) {
>> -        /* Disable I2C controller before soft reset */
>> -        omap_i2c_write_reg(dev, OMAP_I2C_CON_REG,
>> -            omap_i2c_read_reg(dev, OMAP_I2C_CON_REG)&
>> -                ~(OMAP_I2C_CON_EN));
>> -
>> -        omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG,
>> SYSC_SOFTRESET_MASK);
>> -        /* For some reason we need to set the EN bit before the
>> -         * reset done bit gets set. */
>> -        timeout = jiffies + OMAP_I2C_TIMEOUT;
>> -        omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
>> -        while (!(omap_i2c_read_reg(dev, OMAP_I2C_SYSS_REG)&
>> -             SYSS_RESETDONE_MASK)) {
>> -            if (time_after(jiffies, timeout)) {
>> -                dev_warn(dev->dev, "timeout waiting "
>> -                        "for controller reset\n");
>> -                return -ETIMEDOUT;
>> -            }
>> -            msleep(1);
>> -        }
>> -
>> -        /* SYSC register is cleared by the reset; rewrite it */
>> -        if (dev->rev == OMAP_I2C_REV_ON_2430) {
>> -
>> -            omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG,
>> -                       SYSC_AUTOIDLE_MASK);
>> -
>> -        } else if (dev->rev>= OMAP_I2C_REV_ON_3430) {
>> -            dev->syscstate = SYSC_AUTOIDLE_MASK;
>> -            dev->syscstate |= SYSC_ENAWAKEUP_MASK;
>> -            dev->syscstate |= (SYSC_IDLEMODE_SMART<<
>> -                  __ffs(SYSC_SIDLEMODE_MASK));
>> -            dev->syscstate |= (SYSC_CLOCKACTIVITY_FCLK<<
>> -                  __ffs(SYSC_CLOCKACTIVITY_MASK));
>
> The issue I see with this change is that you are removing the above
> code to set the CLKACTIVITY field. Today, AFAIK, hwmod does not set
> this. Ideally it should. However, from discussing this with Richard W,
> this can cause timeouts to occur for i2c transactions. So before
> removing this we need to make sure that this is handled by hwmod or
> else where.
Agree.
Thanks  will try to fix in the next version.
>
>> -            omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG,
>> -                            dev->syscstate);
>> -            /*
>> -             * Enabling all wakup sources to stop I2C freezing on
>> -             * WFI instruction.
>> -             * REVISIT: Some wkup sources might not be needed.
>> -             */
>> -            dev->westate = OMAP_I2C_WE_ALL;
>> -            omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
>> -                            dev->westate);
>> -        }
>> +    if (dev->rev>= OMAP_I2C_REV_ON_3430) {
>> +        /*
>> +         * Enabling all wakeup sources to stop I2C freezing on
>> +         * WFI instruction.
>> +         * REVISIT: Some wakeup sources might not be needed.
>> +         */
>> +        dev->westate = OMAP_I2C_WE_ALL;
>> +        omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
>> +                        dev->westate);
>>       }
>> +
>>       omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
>>
>>       if (pdata->flags&  OMAP_I2C_FLAG_ALWAYS_ARMXOR_CLK) {
>> @@ -594,6 +545,11 @@ static int omap_i2c_xfer_msg(struct i2c_adapter
>> *adap,
>>           return r;
>>       if (r == 0) {
>>           dev_err(dev->dev, "controller timed out\n");
>> +        if (dev->device_reset) {
>> +            r = dev->device_reset(dev->dev);
>> +            if (r<  0)
>> +                dev_err(dev->dev, "reset failed\n");
>> +        }
>>           omap_i2c_init(dev);
>
> Why put the reset here? The function omap_i2c_init is going to perform
> a soft reset. So why not replace the reset in that function?
>
> Furthermore does this work for omap1 devices? I think that you would
> need to remove the existing soft-reset from omap_i2c_init() into an
> omap1.
>
>>           return -ETIMEDOUT;
>>       }
>> @@ -604,6 +560,11 @@ static int omap_i2c_xfer_msg(struct i2c_adapter
>> *adap,
>>       /* We have an error */
>>       if (dev->cmd_err&  (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
>>                   OMAP_I2C_STAT_XUDF)) {
>> +        if (dev->device_reset) {
>> +            r = dev->device_reset(dev->dev);
>> +            if (r<  0)
>> +                dev_err(dev->dev, "reset failed\n");
>> +        }
>>           omap_i2c_init(dev);
>
> Same as above.
>
>>           return -EIO;
>>       }
>> @@ -1004,6 +965,7 @@ omap_i2c_probe(struct platform_device *pdev)
>>       if (pdata != NULL) {
>>           speed = pdata->clkrate;
>>           dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat;
>> +        dev->device_reset = pdata->device_reset;
>>       } else {
>>           speed = 100;    /* Default speed */
>>           dev->set_mpu_wkup_lat = NULL;
>
> Cheers
> Jon

[PATCHv7 3/3] OMAP: I2C: Remove the SYSC register definition

[Shubhrajyoti D]

The SYSC register should not accessed in the driver removing the
define from the driver.
Also clean up the syscstate from the omap_i2c_dev struct.

Acked-by: Santosh Shilimkar <santosh.shilimkar-l0cyMroinI0@public.gmane.org>
Signed-off-by: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>
---
 drivers/i2c/busses/i2c-omap.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index beff9f3..8a60b95 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -63,7 +63,6 @@ enum {
 	OMAP_I2C_BUF_REG,
 	OMAP_I2C_CNT_REG,
 	OMAP_I2C_DATA_REG,
-	OMAP_I2C_SYSC_REG,
 	OMAP_I2C_CON_REG,
 	OMAP_I2C_OA_REG,
 	OMAP_I2C_SA_REG,
@@ -187,7 +186,6 @@ struct omap_i2c_dev {
 	u16			scllstate;
 	u16			sclhstate;
 	u16			bufstate;
-	u16			syscstate;
 	u16			westate;
 	u16			errata;
 };
@@ -202,7 +200,6 @@ static const u8 reg_map_ip_v1[] = {
 	[OMAP_I2C_BUF_REG] = 0x05,
 	[OMAP_I2C_CNT_REG] = 0x06,
 	[OMAP_I2C_DATA_REG] = 0x07,
-	[OMAP_I2C_SYSC_REG] = 0x08,
 	[OMAP_I2C_CON_REG] = 0x09,
 	[OMAP_I2C_OA_REG] = 0x0a,
 	[OMAP_I2C_SA_REG] = 0x0b,
@@ -223,7 +220,6 @@ static const u8 reg_map_ip_v2[] = {
 	[OMAP_I2C_BUF_REG] = 0x94,
 	[OMAP_I2C_CNT_REG] = 0x98,
 	[OMAP_I2C_DATA_REG] = 0x9c,
-	[OMAP_I2C_SYSC_REG] = 0x20,
 	[OMAP_I2C_CON_REG] = 0xa4,
 	[OMAP_I2C_OA_REG] = 0xa8,
 	[OMAP_I2C_SA_REG] = 0xac,
@@ -264,7 +260,6 @@ static void omap_i2c_unidle(struct omap_i2c_dev *dev)
 		omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG, dev->scllstate);
 		omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG, dev->sclhstate);
 		omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG, dev->bufstate);
-		omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, dev->syscstate);
 		omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev->westate);
 		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
 	}
-- 
1.7.1