* [PATCHv5 1/3] OMAP: I2C: Reset support
2011-07-29 10:36 [PATCHv5 0/3] I2C driver updates Shubhrajyoti D
@ 2011-07-29 10:36 ` Shubhrajyoti D
2011-08-02 23:23 ` Kevin Hilman
2011-07-29 10:36 ` [PATCHv5 2/3] OMAP: I2C: Remove the reset in the init path Shubhrajyoti D
2011-07-29 10:36 ` [PATCHv5 3/3] OMAP: I2C: Remove the SYSC register definition Shubhrajyoti D
2 siblings, 1 reply; 9+ messages in thread
From: Shubhrajyoti D @ 2011-07-29 10:36 UTC (permalink / raw)
To: linux-arm-kernel
Under some error conditions the i2c driver may do a reset.
Adding a reset field and support in the platform.
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 2388b8e..be36cac 100644
--- a/arch/arm/plat-omap/i2c.c
+++ b/arch/arm/plat-omap/i2c.c
@@ -146,6 +146,22 @@ static struct omap_device_pm_latency omap_i2c_latency[] = {
.flags = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
},
};
+/**
+ * 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;
+}
static inline int omap2_i2c_add_bus(int bus_id)
{
@@ -187,6 +203,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;
od = omap_device_build(name, bus_id, oh, pdata,
sizeof(struct omap_i2c_bus_platform_data),
omap_i2c_latency, ARRAY_SIZE(omap_i2c_latency), 0);
diff --git a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h
index 98ae49b..8aa91b6 100644
--- a/include/linux/i2c-omap.h
+++ b/include/linux/i2c-omap.h
@@ -38,6 +38,7 @@ struct omap_i2c_bus_platform_data {
int (*device_enable) (struct platform_device *pdev);
int (*device_shutdown) (struct platform_device *pdev);
int (*device_idle) (struct platform_device *pdev);
+ int (*device_reset) (struct device *dev);
};
#endif
--
1.7.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCHv5 1/3] OMAP: I2C: Reset support
2011-07-29 10:36 ` [PATCHv5 1/3] OMAP: I2C: Reset support Shubhrajyoti D
@ 2011-08-02 23:23 ` Kevin Hilman
2011-08-04 6:25 ` Shubhrajyoti
0 siblings, 1 reply; 9+ messages in thread
From: Kevin Hilman @ 2011-08-02 23:23 UTC (permalink / raw)
To: linux-arm-kernel
Shubhrajyoti D <shubhrajyoti@ti.com> writes:
> Under some error conditions the i2c driver may do a reset.
^ ^
minor: extra whitespace
> Adding a reset field and support in the platform.
s/platform/device-specific code/
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
That being said, omap_device_shutdown() should already do a clean
shutdown + reset for you, and i2c-omap.h already has a pointer for
->device_shutdown() so a new device_reset() should not be needed.
IOW, simply adding pdata->device_shutdown = omap_device_shutdown()
should work.
Kevin
> ---
> 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 2388b8e..be36cac 100644
> --- a/arch/arm/plat-omap/i2c.c
> +++ b/arch/arm/plat-omap/i2c.c
> @@ -146,6 +146,22 @@ static struct omap_device_pm_latency omap_i2c_latency[] = {
> .flags = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
> },
> };
> +/**
> + * 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;
> +}
>
> static inline int omap2_i2c_add_bus(int bus_id)
> {
> @@ -187,6 +203,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;
> od = omap_device_build(name, bus_id, oh, pdata,
> sizeof(struct omap_i2c_bus_platform_data),
> omap_i2c_latency, ARRAY_SIZE(omap_i2c_latency), 0);
> diff --git a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h
> index 98ae49b..8aa91b6 100644
> --- a/include/linux/i2c-omap.h
> +++ b/include/linux/i2c-omap.h
> @@ -38,6 +38,7 @@ struct omap_i2c_bus_platform_data {
> int (*device_enable) (struct platform_device *pdev);
> int (*device_shutdown) (struct platform_device *pdev);
> int (*device_idle) (struct platform_device *pdev);
> + int (*device_reset) (struct device *dev);
> };
>
> #endif
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCHv5 1/3] OMAP: I2C: Reset support
2011-08-02 23:23 ` Kevin Hilman
@ 2011-08-04 6:25 ` Shubhrajyoti
2011-08-04 15:05 ` Kevin Hilman
0 siblings, 1 reply; 9+ messages in thread
From: Shubhrajyoti @ 2011-08-04 6:25 UTC (permalink / raw)
To: linux-arm-kernel
On Wednesday 03 August 2011 04:53 AM, Kevin Hilman wrote:
> Shubhrajyoti D<shubhrajyoti@ti.com> writes:
>
>> Under some error conditions the i2c driver may do a reset.
> ^ ^
> minor: extra whitespace
>
>> Adding a reset field and support in the platform.
> s/platform/device-specific code/
>
Yes will fix these.
>> Signed-off-by: Shubhrajyoti D<shubhrajyoti@ti.com>
> That being said, omap_device_shutdown() should already do a clean
> shutdown + reset for you, and i2c-omap.h already has a pointer for
> ->device_shutdown() so a new device_reset() should not be needed.
>
> IOW, simply adding pdata->device_shutdown = omap_device_shutdown()
> should work.
i2C has a special reset sequence ie Disable -> reset -> enable -> Poll
on reset done.
This function is implemented in omap_i2c_reset. The
omap_hwmod_reset calls -> _reset
which calls ocp_softrest or custom reset if it is present.The latter is
true for I2C.
However I see that
omap_device_shutdown -> _assert_hardreset However for I2c there doesnt seem to be a HW reset line.
Am I missing something?
> Kevin
>
>> ---
>> 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 2388b8e..be36cac 100644
>> --- a/arch/arm/plat-omap/i2c.c
>> +++ b/arch/arm/plat-omap/i2c.c
>> @@ -146,6 +146,22 @@ static struct omap_device_pm_latency omap_i2c_latency[] = {
>> .flags = OMAP_DEVICE_LATENCY_AUTO_ADJUST,
>> },
>> };
>> +/**
>> + * 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;
>> +}
>>
>> static inline int omap2_i2c_add_bus(int bus_id)
>> {
>> @@ -187,6 +203,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;
>> od = omap_device_build(name, bus_id, oh, pdata,
>> sizeof(struct omap_i2c_bus_platform_data),
>> omap_i2c_latency, ARRAY_SIZE(omap_i2c_latency), 0);
>> diff --git a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h
>> index 98ae49b..8aa91b6 100644
>> --- a/include/linux/i2c-omap.h
>> +++ b/include/linux/i2c-omap.h
>> @@ -38,6 +38,7 @@ struct omap_i2c_bus_platform_data {
>> int (*device_enable) (struct platform_device *pdev);
>> int (*device_shutdown) (struct platform_device *pdev);
>> int (*device_idle) (struct platform_device *pdev);
>> + int (*device_reset) (struct device *dev);
>> };
>>
>> #endif
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCHv5 1/3] OMAP: I2C: Reset support
2011-08-04 6:25 ` Shubhrajyoti
@ 2011-08-04 15:05 ` Kevin Hilman
0 siblings, 0 replies; 9+ messages in thread
From: Kevin Hilman @ 2011-08-04 15:05 UTC (permalink / raw)
To: linux-arm-kernel
Shubhrajyoti <shubhrajyoti@ti.com> writes:
> On Wednesday 03 August 2011 04:53 AM, Kevin Hilman wrote:
>> Shubhrajyoti D<shubhrajyoti@ti.com> writes:
>>
>>> Under some error conditions the i2c driver may do a reset.
>> ^ ^
>> minor: extra whitespace
>>
>>> Adding a reset field and support in the platform.
>> s/platform/device-specific code/
>>
> Yes will fix these.
>>> Signed-off-by: Shubhrajyoti D<shubhrajyoti@ti.com>
>> That being said, omap_device_shutdown() should already do a clean
>> shutdown + reset for you, and i2c-omap.h already has a pointer for
>> ->device_shutdown() so a new device_reset() should not be needed.
>>
>> IOW, simply adding pdata->device_shutdown = omap_device_shutdown()
>> should work.
> i2C has a special reset sequence ie Disable -> reset -> enable -> Poll
> on reset done.
> This function is implemented in omap_i2c_reset. The
>
> omap_hwmod_reset calls -> _reset
>
> which calls ocp_softrest or custom reset if it is present.The latter
> is true for I2C.
>
> However I see that
>
> omap_device_shutdown -> _assert_hardreset However for I2c there doesnt seem to be a HW reset line.
>
>
> Am I missing something?
>
No, my fault.
I thought omap_device_shutdown() also did soft reset, but I see that's
not the case. You can ignore my comments about using
omap_device_shutdown.
Kevin
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCHv5 2/3] OMAP: I2C: Remove the reset in the init path
2011-07-29 10:36 [PATCHv5 0/3] I2C driver updates Shubhrajyoti D
2011-07-29 10:36 ` [PATCHv5 1/3] OMAP: I2C: Reset support Shubhrajyoti D
@ 2011-07-29 10:36 ` Shubhrajyoti D
2011-08-02 23:27 ` Kevin Hilman
2011-07-29 10:36 ` [PATCHv5 3/3] OMAP: I2C: Remove the SYSC register definition Shubhrajyoti D
2 siblings, 1 reply; 9+ messages in thread
From: Shubhrajyoti D @ 2011-07-29 10:36 UTC (permalink / raw)
To: linux-arm-kernel
- 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 | 83 +++++++++++------------------------------
1 files changed, 22 insertions(+), 61 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 4e3256f..d8f4470 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,22 @@ 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;
- if (dev->rev < OMAP_I2C_REV_ON_3530_4430)
- 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;
+ if (dev->rev < OMAP_I2C_REV_ON_3530_4430)
+ omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
+ dev->westate);
}
omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
@@ -595,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 != NULL) {
+ r = dev->device_reset(dev->dev);
+ if (r < 0)
+ dev_err(dev->dev, "reset failed\n");
+ }
omap_i2c_init(dev);
return -ETIMEDOUT;
}
@@ -605,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 != NULL) {
+ r = dev->device_reset(dev->dev);
+ if (r < 0)
+ dev_err(dev->dev, "reset failed\n");
+ }
omap_i2c_init(dev);
return -EIO;
}
@@ -1005,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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCHv5 2/3] OMAP: I2C: Remove the reset in the init path
2011-07-29 10:36 ` [PATCHv5 2/3] OMAP: I2C: Remove the reset in the init path Shubhrajyoti D
@ 2011-08-02 23:27 ` Kevin Hilman
2011-08-04 16:41 ` Shubhrajyoti
0 siblings, 1 reply; 9+ messages in thread
From: Kevin Hilman @ 2011-08-02 23:27 UTC (permalink / raw)
To: linux-arm-kernel
Shubhrajyoti D <shubhrajyoti@ti.com> writes:
> - 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.
Not specifically related to this patch, as the reset behavior was
already there, but do you know why the reset is needed after timeout and
error handling?
IOW, was the reset truly required in those cases, or was just the
re-init necessary?
To me doing a full reset of the IP in those cases sounds like a hack for
a buggy driver.
> - 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 | 83 +++++++++++------------------------------
> 1 files changed, 22 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 4e3256f..d8f4470 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,22 @@ 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;
> - if (dev->rev < OMAP_I2C_REV_ON_3530_4430)
> - 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;
> + if (dev->rev < OMAP_I2C_REV_ON_3530_4430)
> + omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
> + dev->westate);
> }
> omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
>
> @@ -595,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 != NULL) {
No need for the '!= NULL'
if (dev->device_reset)
is more typical.
And based on my comments in 1/3, this would become device_shutdown.
> + r = dev->device_reset(dev->dev);
> + if (r < 0)
> + dev_err(dev->dev, "reset failed\n");
> + }
> omap_i2c_init(dev);
> return -ETIMEDOUT;
> }
> @@ -605,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 != NULL) {
> + r = dev->device_reset(dev->dev);
> + if (r < 0)
> + dev_err(dev->dev, "reset failed\n");
> + }
> omap_i2c_init(dev);
> return -EIO;
> }
> @@ -1005,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;
Kevin
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCHv5 2/3] OMAP: I2C: Remove the reset in the init path
2011-08-02 23:27 ` Kevin Hilman
@ 2011-08-04 16:41 ` Shubhrajyoti
0 siblings, 0 replies; 9+ messages in thread
From: Shubhrajyoti @ 2011-08-04 16:41 UTC (permalink / raw)
To: linux-arm-kernel
On Wednesday 03 August 2011 04:57 AM, Kevin Hilman wrote:
> Shubhrajyoti D<shubhrajyoti@ti.com> writes:
>
>> - 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.
> Not specifically related to this patch, as the reset behavior was
> already there, but do you know why the reset is needed after timeout and
> error handling?
>
> IOW, was the reset truly required in those cases, or was just the
> re-init necessary?
>
> To me doing a full reset of the IP in those cases sounds like a hack for
> a buggy driver.
My idea, there have been errata workaround that recommend reset in case
of a
lockup that may happen after a warmreset.
>> - 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 | 83 +++++++++++------------------------------
>> 1 files changed, 22 insertions(+), 61 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
>> index 4e3256f..d8f4470 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,22 @@ 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;
>> - if (dev->rev< OMAP_I2C_REV_ON_3530_4430)
>> - 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;
>> + if (dev->rev< OMAP_I2C_REV_ON_3530_4430)
>> + omap_i2c_write_reg(dev, OMAP_I2C_WE_REG,
>> + dev->westate);
>> }
>> omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
>>
>> @@ -595,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 != NULL) {
> No need for the '!= NULL'
>
> if (dev->device_reset)
>
> is more typical.
>
> And based on my comments in 1/3, this would become device_shutdown.
>
>> + r = dev->device_reset(dev->dev);
>> + if (r< 0)
>> + dev_err(dev->dev, "reset failed\n");
>> + }
>> omap_i2c_init(dev);
>> return -ETIMEDOUT;
>> }
>> @@ -605,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 != NULL) {
>> + r = dev->device_reset(dev->dev);
>> + if (r< 0)
>> + dev_err(dev->dev, "reset failed\n");
>> + }
>> omap_i2c_init(dev);
>> return -EIO;
>> }
>> @@ -1005,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;
> Kevin
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCHv5 3/3] OMAP: I2C: Remove the SYSC register definition
2011-07-29 10:36 [PATCHv5 0/3] I2C driver updates Shubhrajyoti D
2011-07-29 10:36 ` [PATCHv5 1/3] OMAP: I2C: Reset support Shubhrajyoti D
2011-07-29 10:36 ` [PATCHv5 2/3] OMAP: I2C: Remove the reset in the init path Shubhrajyoti D
@ 2011-07-29 10:36 ` Shubhrajyoti D
2 siblings, 0 replies; 9+ messages in thread
From: Shubhrajyoti D @ 2011-07-29 10:36 UTC (permalink / raw)
To: linux-arm-kernel
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@ti.com>
Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
---
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 d8f4470..d05efe7 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 @@ const static 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 @@ const static 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
^ permalink raw reply related [flat|nested] 9+ messages in thread