| Message ID | 1334235995-6727-1-git-send-email-shubhrajyoti@ti.com |
|---|---|
| State | New |
| Headers | show |
* Shubhrajyoti D <shubhrajyoti@ti.com> [120412 06:10]: > Currently i2c register restore is done always. > Adding conditional restore. > The i2c register restore is done only if the context is lost. > Also remove the definition of SYSS_RESETDONE_MASK and use the > one in omap_hwmod.h. > > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> > --- > arch/arm/plat-omap/i2c.c | 3 ++ > drivers/i2c/busses/i2c-omap.c | 52 ++++++++++++++++++++++++++-------------- > include/linux/i2c-omap.h | 1 + > 3 files changed, 38 insertions(+), 18 deletions(-) > > diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c > index db071bc..4ccab07 100644 > --- a/arch/arm/plat-omap/i2c.c > +++ b/arch/arm/plat-omap/i2c.c > @@ -179,6 +179,9 @@ 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->get_context_loss_count = omap_pm_get_dev_context_loss_count; > + > pdev = omap_device_build(name, bus_id, oh, pdata, > sizeof(struct omap_i2c_bus_platform_data), > NULL, 0, 0); This should be acked by Kevin, adding him to Cc. For the arch/arm/plat-omap/i2c.c part: Acked-by: Tony Lindgren <tony@atomide.com> > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index a882558..45389db 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -43,6 +43,7 @@ > #include <linux/slab.h> > #include <linux/i2c-omap.h> > #include <linux/pm_runtime.h> > +#include <plat/omap_device.h> > > /* I2C controller revisions */ > #define OMAP_I2C_OMAP1_REV_2 0x20 > @@ -157,9 +158,6 @@ enum { > #define OMAP_I2C_SYSTEST_SDA_I (1 << 1) /* SDA line sense in */ > #define OMAP_I2C_SYSTEST_SDA_O (1 << 0) /* SDA line drive out */ > > -/* 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) > @@ -184,6 +182,7 @@ struct omap_i2c_dev { > u32 latency; /* maximum mpu wkup latency */ > void (*set_mpu_wkup_lat)(struct device *dev, > long latency); > + int (*get_context_loss_count)(struct device *dev); > u32 speed; /* Speed of bus in kHz */ > u32 dtrev; /* extra revision from DT */ > u32 flags; > @@ -206,6 +205,7 @@ struct omap_i2c_dev { > u16 syscstate; > u16 westate; > u16 errata; > + int dev_lost_count; > }; > > static const u8 reg_map_ip_v1[] = { > @@ -1025,6 +1025,7 @@ omap_i2c_probe(struct platform_device *pdev) > dev->speed = pdata->clkrate; > dev->flags = pdata->flags; > dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat; > + dev->get_context_loss_count = pdata->get_context_loss_count; > dev->dtrev = pdata->rev; > } > > @@ -1151,12 +1152,32 @@ omap_i2c_remove(struct platform_device *pdev) > } > > #ifdef CONFIG_PM_RUNTIME > +static void omap_i2c_restore(struct omap_i2c_dev *dev) > +{ > + omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0); > + omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, dev->pscstate); > + 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_WE_REG, dev->westate); > + omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN); > + /* > + * Don't write to this register if the IE state is 0 as it can > + * cause deadlock. > + */ > + if (dev->iestate) > + omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate); > + > +} > static int omap_i2c_runtime_suspend(struct device *dev) > { > struct platform_device *pdev = to_platform_device(dev); > struct omap_i2c_dev *_dev = platform_get_drvdata(pdev); > u16 iv; > > + if (_dev->get_context_loss_count) > + _dev->dev_lost_count = _dev->get_context_loss_count(dev); > + > _dev->iestate = omap_i2c_read_reg(_dev, OMAP_I2C_IE_REG); > if (_dev->dtrev == OMAP_I2C_IP_VERSION_2) > omap_i2c_write_reg(_dev, OMAP_I2C_IP_V2_IRQENABLE_CLR, 1); > @@ -1179,24 +1200,19 @@ static int omap_i2c_runtime_resume(struct device *dev) > { > struct platform_device *pdev = to_platform_device(dev); > struct omap_i2c_dev *_dev = platform_get_drvdata(pdev); > + int loss_cnt; > > - if (_dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) { > - omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, 0); > - omap_i2c_write_reg(_dev, OMAP_I2C_PSC_REG, _dev->pscstate); > - 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); > + if (_dev->get_context_loss_count) { > + loss_cnt = _dev->get_context_loss_count(dev); > + if (loss_cnt < 0) > + return loss_cnt; > + > + if (_dev->dev_lost_count == loss_cnt && _dev->dev_lost_count) > + return 0; > } > > - /* > - * Don't write to this register if the IE state is 0 as it can > - * cause deadlock. > - */ > - if (_dev->iestate) > - omap_i2c_write_reg(_dev, OMAP_I2C_IE_REG, _dev->iestate); > + if (_dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) > + omap_i2c_restore(_dev); > > return 0; > } > diff --git a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h > index 92a0dc7..c76cbc0 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 (*get_context_loss_count)(struct device *dev); > }; > > #endif > -- > 1.7.4.1 >
On Monday 16 April 2012 11:43 PM, Tony Lindgren wrote: > * Shubhrajyoti D <shubhrajyoti@ti.com> [120412 06:10]: >> Currently i2c register restore is done always. >> Adding conditional restore. >> The i2c register restore is done only if the context is lost. >> Also remove the definition of SYSS_RESETDONE_MASK and use the >> one in omap_hwmod.h. >> >> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> >> --- >> arch/arm/plat-omap/i2c.c | 3 ++ >> drivers/i2c/busses/i2c-omap.c | 52 ++++++++++++++++++++++++++-------------- >> include/linux/i2c-omap.h | 1 + >> 3 files changed, 38 insertions(+), 18 deletions(-) >> >> diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c >> index db071bc..4ccab07 100644 >> --- a/arch/arm/plat-omap/i2c.c >> +++ b/arch/arm/plat-omap/i2c.c >> @@ -179,6 +179,9 @@ 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->get_context_loss_count = omap_pm_get_dev_context_loss_count; >> + >> pdev = omap_device_build(name, bus_id, oh, pdata, >> sizeof(struct omap_i2c_bus_platform_data), >> NULL, 0, 0); > This should be acked by Kevin, I should have cced Kevin. > adding him to Cc. Thanks. > For the arch/arm/plat-omap/i2c.c part: > > Acked-by: Tony Lindgren <tony@atomide.com> > Thanks.
Shubhrajyoti D <shubhrajyoti@ti.com> writes: > Currently i2c register restore is done always. > Adding conditional restore. > The i2c register restore is done only if the context is lost. OK, minor comment below. > Also remove the definition of SYSS_RESETDONE_MASK and use the > one in omap_hwmod.h. The definition is removed, but I don't see any of the users removed. If the users were cleaned up earlier in the series, then the removal of this should be done with that, or as a separate patch. I don't see why it belongs with this patch. > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> > --- > arch/arm/plat-omap/i2c.c | 3 ++ > drivers/i2c/busses/i2c-omap.c | 52 ++++++++++++++++++++++++++-------------- > include/linux/i2c-omap.h | 1 + > 3 files changed, 38 insertions(+), 18 deletions(-) > > diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c > index db071bc..4ccab07 100644 > --- a/arch/arm/plat-omap/i2c.c > +++ b/arch/arm/plat-omap/i2c.c > @@ -179,6 +179,9 @@ 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->get_context_loss_count = omap_pm_get_dev_context_loss_count; > + > pdev = omap_device_build(name, bus_id, oh, pdata, > sizeof(struct omap_i2c_bus_platform_data), > NULL, 0, 0); > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index a882558..45389db 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -43,6 +43,7 @@ > #include <linux/slab.h> > #include <linux/i2c-omap.h> > #include <linux/pm_runtime.h> > +#include <plat/omap_device.h> > > /* I2C controller revisions */ > #define OMAP_I2C_OMAP1_REV_2 0x20 > @@ -157,9 +158,6 @@ enum { > #define OMAP_I2C_SYSTEST_SDA_I (1 << 1) /* SDA line sense in */ > #define OMAP_I2C_SYSTEST_SDA_O (1 << 0) /* SDA line drive out */ > > -/* 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) > @@ -184,6 +182,7 @@ struct omap_i2c_dev { > u32 latency; /* maximum mpu wkup latency */ > void (*set_mpu_wkup_lat)(struct device *dev, > long latency); > + int (*get_context_loss_count)(struct device *dev); > u32 speed; /* Speed of bus in kHz */ > u32 dtrev; /* extra revision from DT */ > u32 flags; > @@ -206,6 +205,7 @@ struct omap_i2c_dev { > u16 syscstate; > u16 westate; > u16 errata; > + int dev_lost_count; > }; > > static const u8 reg_map_ip_v1[] = { > @@ -1025,6 +1025,7 @@ omap_i2c_probe(struct platform_device *pdev) > dev->speed = pdata->clkrate; > dev->flags = pdata->flags; > dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat; > + dev->get_context_loss_count = pdata->get_context_loss_count; > dev->dtrev = pdata->rev; > } > > @@ -1151,12 +1152,32 @@ omap_i2c_remove(struct platform_device *pdev) > } > > #ifdef CONFIG_PM_RUNTIME > +static void omap_i2c_restore(struct omap_i2c_dev *dev) > +{ > + omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0); > + omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, dev->pscstate); > + 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_WE_REG, dev->westate); > + omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN); > + /* > + * Don't write to this register if the IE state is 0 as it can > + * cause deadlock. > + */ > + if (dev->iestate) > + omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate); > + > +} > static int omap_i2c_runtime_suspend(struct device *dev) > { > struct platform_device *pdev = to_platform_device(dev); > struct omap_i2c_dev *_dev = platform_get_drvdata(pdev); > u16 iv; > > + if (_dev->get_context_loss_count) > + _dev->dev_lost_count = _dev->get_context_loss_count(dev); > + > _dev->iestate = omap_i2c_read_reg(_dev, OMAP_I2C_IE_REG); > if (_dev->dtrev == OMAP_I2C_IP_VERSION_2) > omap_i2c_write_reg(_dev, OMAP_I2C_IP_V2_IRQENABLE_CLR, 1); > @@ -1179,24 +1200,19 @@ static int omap_i2c_runtime_resume(struct device *dev) > { > struct platform_device *pdev = to_platform_device(dev); > struct omap_i2c_dev *_dev = platform_get_drvdata(pdev); > + int loss_cnt; > > - if (_dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) { > - omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, 0); > - omap_i2c_write_reg(_dev, OMAP_I2C_PSC_REG, _dev->pscstate); > - 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); > + if (_dev->get_context_loss_count) { > + loss_cnt = _dev->get_context_loss_count(dev); > + if (loss_cnt < 0) > + return loss_cnt; To avoid messing up driver state, upon error, you should probably restore context, not abort. > + if (_dev->dev_lost_count == loss_cnt && _dev->dev_lost_count) > + return 0; Why the non-zero check? Seems like all you need to check is if (_dev->dev_lost_count != loss_cnt). Kevin > } > > - /* > - * Don't write to this register if the IE state is 0 as it can > - * cause deadlock. > - */ > - if (_dev->iestate) > - omap_i2c_write_reg(_dev, OMAP_I2C_IE_REG, _dev->iestate); > + if (_dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) > + omap_i2c_restore(_dev); > > return 0; > } > diff --git a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h > index 92a0dc7..c76cbc0 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 (*get_context_loss_count)(struct device *dev); > }; > > #endif
Hi Kevin, Thanks for your review, On Tue, Apr 17, 2012 at 7:15 PM, Kevin Hilman <khilman@ti.com> wrote: > Shubhrajyoti D <shubhrajyoti@ti.com> writes: > >> Currently i2c register restore is done always. >> Adding conditional restore. >> The i2c register restore is done only if the context is lost. > > OK, minor comment below. > >> Also remove the definition of SYSS_RESETDONE_MASK and use the >> one in omap_hwmod.h. > > The definition is removed, but I don't see any of the users removed. > If the users were cleaned up earlier in the series, then the removal of > this should be done with that, or as a separate patch. I don't see why > it belongs with this patch. I have not removed the users actually the macro was redefined. I have instead used the definition in omap_hwmod.h which gets added when I include omap_device.h Let me know if you prefer a separate patch? > >> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> >> --- >> arch/arm/plat-omap/i2c.c | 3 ++ >> drivers/i2c/busses/i2c-omap.c | 52 ++++++++++++++++++++++++++-------------- >> include/linux/i2c-omap.h | 1 + >> 3 files changed, 38 insertions(+), 18 deletions(-) >> >> diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c >> index db071bc..4ccab07 100644 >> --- a/arch/arm/plat-omap/i2c.c >> +++ b/arch/arm/plat-omap/i2c.c >> @@ -179,6 +179,9 @@ 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->get_context_loss_count = omap_pm_get_dev_context_loss_count; >> + >> pdev = omap_device_build(name, bus_id, oh, pdata, >> sizeof(struct omap_i2c_bus_platform_data), >> NULL, 0, 0); >> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c >> index a882558..45389db 100644 >> --- a/drivers/i2c/busses/i2c-omap.c >> +++ b/drivers/i2c/busses/i2c-omap.c >> @@ -43,6 +43,7 @@ >> #include <linux/slab.h> >> #include <linux/i2c-omap.h> >> #include <linux/pm_runtime.h> >> +#include <plat/omap_device.h> >> >> /* I2C controller revisions */ >> #define OMAP_I2C_OMAP1_REV_2 0x20 >> @@ -157,9 +158,6 @@ enum { >> #define OMAP_I2C_SYSTEST_SDA_I (1 << 1) /* SDA line sense in */ >> #define OMAP_I2C_SYSTEST_SDA_O (1 << 0) /* SDA line drive out */ >> >> -/* 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) >> @@ -184,6 +182,7 @@ struct omap_i2c_dev { >> u32 latency; /* maximum mpu wkup latency */ >> void (*set_mpu_wkup_lat)(struct device *dev, >> long latency); >> + int (*get_context_loss_count)(struct device *dev); >> u32 speed; /* Speed of bus in kHz */ >> u32 dtrev; /* extra revision from DT */ >> u32 flags; >> @@ -206,6 +205,7 @@ struct omap_i2c_dev { >> u16 syscstate; >> u16 westate; >> u16 errata; >> + int dev_lost_count; >> }; >> >> static const u8 reg_map_ip_v1[] = { >> @@ -1025,6 +1025,7 @@ omap_i2c_probe(struct platform_device *pdev) >> dev->speed = pdata->clkrate; >> dev->flags = pdata->flags; >> dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat; >> + dev->get_context_loss_count = pdata->get_context_loss_count; >> dev->dtrev = pdata->rev; >> } >> >> @@ -1151,12 +1152,32 @@ omap_i2c_remove(struct platform_device *pdev) >> } >> >> #ifdef CONFIG_PM_RUNTIME >> +static void omap_i2c_restore(struct omap_i2c_dev *dev) >> +{ >> + omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0); >> + omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, dev->pscstate); >> + 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_WE_REG, dev->westate); >> + omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN); >> + /* >> + * Don't write to this register if the IE state is 0 as it can >> + * cause deadlock. >> + */ >> + if (dev->iestate) >> + omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate); >> + >> +} >> static int omap_i2c_runtime_suspend(struct device *dev) >> { >> struct platform_device *pdev = to_platform_device(dev); >> struct omap_i2c_dev *_dev = platform_get_drvdata(pdev); >> u16 iv; >> >> + if (_dev->get_context_loss_count) >> + _dev->dev_lost_count = _dev->get_context_loss_count(dev); >> + >> _dev->iestate = omap_i2c_read_reg(_dev, OMAP_I2C_IE_REG); >> if (_dev->dtrev == OMAP_I2C_IP_VERSION_2) >> omap_i2c_write_reg(_dev, OMAP_I2C_IP_V2_IRQENABLE_CLR, 1); >> @@ -1179,24 +1200,19 @@ static int omap_i2c_runtime_resume(struct device *dev) >> { >> struct platform_device *pdev = to_platform_device(dev); >> struct omap_i2c_dev *_dev = platform_get_drvdata(pdev); >> + int loss_cnt; >> >> - if (_dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) { >> - omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, 0); >> - omap_i2c_write_reg(_dev, OMAP_I2C_PSC_REG, _dev->pscstate); >> - 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); >> + if (_dev->get_context_loss_count) { >> + loss_cnt = _dev->get_context_loss_count(dev); >> + if (loss_cnt < 0) >> + return loss_cnt; > > To avoid messing up driver state, upon error, you should probably > restore context, not abort. Agreed , will fix this. > >> + if (_dev->dev_lost_count == loss_cnt && _dev->dev_lost_count) >> + return 0; > > Why the non-zero check? Actually the driver probe-->omap_i2c_init here <snip> dev->pscstate = psc; dev->scllstate = scll; dev->sclhstate = sclh; dev->bufstate = buf; <snip> variables are updated and the first write happens in the handler. > > Seems like all you need to check is if (_dev->dev_lost_count != loss_cnt). > > Kevin > >> } >> >> - /* >> - * Don't write to this register if the IE state is 0 as it can >> - * cause deadlock. >> - */ >> - if (_dev->iestate) >> - omap_i2c_write_reg(_dev, OMAP_I2C_IE_REG, _dev->iestate); >> + if (_dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) >> + omap_i2c_restore(_dev); >> >> return 0; >> } >> diff --git a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h >> index 92a0dc7..c76cbc0 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 (*get_context_loss_count)(struct device *dev); >> }; >> >> #endif > -- > 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
"Datta, Shubhrajyoti" <shubhrajyoti@ti.com> writes: > Hi Kevin, > >> Hi Kevin, >> Thanks for your review, >> >> On Tue, Apr 17, 2012 at 7:15 PM, Kevin Hilman <khilman@ti.com> wrote: >>> Shubhrajyoti D <shubhrajyoti@ti.com> writes: >>> >>>> Currently i2c register restore is done always. >>>> Adding conditional restore. >>>> The i2c register restore is done only if the context is lost. >>> >>> OK, minor comment below. > > Thanks for the suggestion of the error case restore. > Updated the patch. Also added Tony's ack for the plat part. > > > From 1ca222f6f50868e07d9a46575e73dd66fd2d542e Mon Sep 17 00:00:00 2001 > From: Shubhrajyoti D <shubhrajyoti@ti.com> > Date: Tue, 17 Jan 2012 16:13:03 +0530 > Subject: [PATCH] I2C: OMAP: I2C register restore only if context is lost > > Currently i2c register restore is done always. > Adding conditional restore. > The i2c register restore is done only if the context is lost > or in case of error to be on the safe side. > Also remove the definition of SYSS_RESETDONE_MASK and use the > one in omap_hwmod.h. > > Cc: Kevin Hilman <khilman@ti.com> > [For the arch/arm/plat-omap/i2c.c part] > Acked-by: Tony Lindgren <tony@atomide.com> > Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> > --- > arch/arm/plat-omap/i2c.c | 3 ++ > drivers/i2c/busses/i2c-omap.c | 53 +++++++++++++++++++++++++++-------------- > include/linux/i2c-omap.h | 1 + > 3 files changed, 39 insertions(+), 18 deletions(-) > > diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c > index db071bc..4ccab07 100644 > --- a/arch/arm/plat-omap/i2c.c > +++ b/arch/arm/plat-omap/i2c.c > @@ -179,6 +179,9 @@ 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->get_context_loss_count = omap_pm_get_dev_context_loss_count; > + > pdev = omap_device_build(name, bus_id, oh, pdata, > sizeof(struct omap_i2c_bus_platform_data), > NULL, 0, 0); > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c > index a882558..76cf066 100644 > --- a/drivers/i2c/busses/i2c-omap.c > +++ b/drivers/i2c/busses/i2c-omap.c > @@ -43,6 +43,7 @@ > #include <linux/slab.h> > #include <linux/i2c-omap.h> > #include <linux/pm_runtime.h> > +#include <plat/omap_device.h> > > /* I2C controller revisions */ > #define OMAP_I2C_OMAP1_REV_2 0x20 > @@ -157,9 +158,6 @@ enum { > #define OMAP_I2C_SYSTEST_SDA_I (1 << 1) /* SDA line sense in */ > #define OMAP_I2C_SYSTEST_SDA_O (1 << 0) /* SDA line drive out */ > > -/* OCP_SYSSTATUS bit definitions */ > -#define SYSS_RESETDONE_MASK (1 << 0) > - Unrelated to this patch. > /* OCP_SYSCONFIG bit definitions */ > #define SYSC_CLOCKACTIVITY_MASK (0x3 << 8) > #define SYSC_SIDLEMODE_MASK (0x3 << 3) > @@ -184,6 +182,7 @@ struct omap_i2c_dev { > u32 latency; /* maximum mpu wkup latency */ > void (*set_mpu_wkup_lat)(struct device *dev, > long latency); > + int (*get_context_loss_count)(struct device *dev); > u32 speed; /* Speed of bus in kHz */ > u32 dtrev; /* extra revision from DT */ > u32 flags; > @@ -206,6 +205,7 @@ struct omap_i2c_dev { > u16 syscstate; > u16 westate; > u16 errata; > + int dev_lost_count; > }; > > static const u8 reg_map_ip_v1[] = { > @@ -1025,6 +1025,7 @@ omap_i2c_probe(struct platform_device *pdev) > dev->speed = pdata->clkrate; > dev->flags = pdata->flags; > dev->set_mpu_wkup_lat = pdata->set_mpu_wkup_lat; > + dev->get_context_loss_count = pdata->get_context_loss_count; > dev->dtrev = pdata->rev; > } > > @@ -1151,12 +1152,32 @@ omap_i2c_remove(struct platform_device *pdev) > } > > #ifdef CONFIG_PM_RUNTIME > +static void omap_i2c_restore(struct omap_i2c_dev *dev) > +{ > + omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0); > + omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, dev->pscstate); > + 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_WE_REG, dev->westate); > + omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN); > + /* > + * Don't write to this register if the IE state is 0 as it can > + * cause deadlock. > + */ > + if (dev->iestate) > + omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate); > + > +} > static int omap_i2c_runtime_suspend(struct device *dev) > { > struct platform_device *pdev = to_platform_device(dev); > struct omap_i2c_dev *_dev = platform_get_drvdata(pdev); > u16 iv; > > + if (_dev->get_context_loss_count) > + _dev->dev_lost_count = _dev->get_context_loss_count(dev); > + > _dev->iestate = omap_i2c_read_reg(_dev, OMAP_I2C_IE_REG); > if (_dev->dtrev == OMAP_I2C_IP_VERSION_2) > omap_i2c_write_reg(_dev, OMAP_I2C_IP_V2_IRQENABLE_CLR, 1); > @@ -1179,24 +1200,20 @@ static int omap_i2c_runtime_resume(struct device *dev) > { > struct platform_device *pdev = to_platform_device(dev); > struct omap_i2c_dev *_dev = platform_get_drvdata(pdev); > + int loss_cnt; > > - if (_dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) { > - omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, 0); > - omap_i2c_write_reg(_dev, OMAP_I2C_PSC_REG, _dev->pscstate); > - 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); > + if (_dev->get_context_loss_count) { > + loss_cnt = _dev->get_context_loss_count(dev); > + if (loss_cnt < 0) { > + omap_i2c_restore(_dev); > + return 0; > + } > + if (_dev->dev_lost_count == loss_cnt && _dev->dev_lost_count) > + return 0; Again, why does it matter if _dev->dev_lost_count != 0? IMO, this is still more confusing than it needs to be. What is wrong with if (_dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) return 0; /* read current loss_cnt */ if (_dev->dev_lost_count != loss_cnt) omap_i2c_restore(_dev); return 0; Kevin > > - /* > - * Don't write to this register if the IE state is 0 as it can > - * cause deadlock. > - */ > - if (_dev->iestate) > - omap_i2c_write_reg(_dev, OMAP_I2C_IE_REG, _dev->iestate); > + if (_dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) > + omap_i2c_restore(_dev); > > return 0; > } > diff --git a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h > index 92a0dc7..c76cbc0 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 (*get_context_loss_count)(struct device *dev); > }; > > #endif
The patch series does the following - Warn fixes if CONFIG_PM_RUNTIME is not selected. - I2C register restore only if context if the context is lost - Bus busy recovery mechanism. - the reset is not done in init. - Adds a patch to use devm_* functions - Also checks the return type of the get_sync and in case of errors prevents register access, also print the cause of failure in case of errors. - In case of i2c remove register access was done without any get_sync fix the same. - Adds a pdata function pointer to do context save restore - Split the omap_i2c_isr to increase readability - Make the i2c use SET_RUNTIME_PM_OPS - Folds a patch from Tasslehoff to prevent any merge conflicts. - Prevents the XDUF flag to be set if the underflow condition is not met. - As per discussion in [1] .Adds a patch to rename the 1p153 errata and use the unique id instead as the section number in the recent errata docs has changed. - As discussed in [2], Paul has queued the flag for context restore patch, removing it from the series. v8 Fix Felipe's comments and use devm_request_and_ioremap. [1] http://www.spinics.net/lists/linux-i2c/msg07607.html [2] http://www.spinics.net/lists/linux-i2c/msg07685.html Tested on omap4sdp and omap3sdp. The following changes since commit 258f742635360175564e9470eb060ff4d4b984e7: modpost: Fix modpost license checking of vmlinux.o (2012-04-09 20:52:56 -0700) are available in the git repository at: git://gitorious.org/linus-tree/linus-tree.git i2c_omap-next Jon Hunter (1): I2C: OMAP: Correct I2C revision for OMAP3 Shubhrajyoti D (15): I2C: OMAP: make omap_i2c_unidle/idle functions depend on CONFIG_PM_RUNTIME I2C: OMAP: Remove reset at init I2C: OMAP: I2C register restore only if context is lost I2C: OMAP: Fix the interrupt clearing in OMAP4 I2C: OMAP: Fix the mismatch of pm_runtime enable and disable I2C: OMAP: Optimise the remove code I2C: OMAP: Fix the error handling I2C: OMAP: Don't check if wait_for_completion_timeout() returns less than zero I2C: OMAP: use devm_* functions I2C: OMAP: Fix the crash in i2c remove I2C: OMAP: Handle error check for pm runtime I2C: OMAP: Use SET_RUNTIME_PM_OPS I2C: OMAP: make the read ready processing a separate function I2C: OMAP: Do not set the XUDF if the underflow is not reached I2C: OMAP: Rename the 1p153 to the erratum id i462 Tasslehoff Kjappfot (1): I2C: OMAP: fix missing handling of errata I2C_OMAP3_1P153 Vikram Pandita (1): I2C: OMAP: Recover from Bus Busy condition arch/arm/plat-omap/i2c.c | 3 + drivers/i2c/busses/i2c-omap.c | 350 +++++++++++++++++++++++------------------ include/linux/i2c-omap.h | 1 + 3 files changed, 199 insertions(+), 155 deletions(-)