[PATCHv5 2/3] OMAP: I2C: Remove the reset in the init path
Shubhrajyoti
shubhrajyoti at ti.com
Thu Aug 4 12:41:18 EDT 2011
On Wednesday 03 August 2011 04:57 AM, Kevin Hilman wrote:
> Shubhrajyoti D<shubhrajyoti at 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 at 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
More information about the linux-arm-kernel
mailing list