Re: [PATCHv8 04/18] I2C: OMAP: I2C register restore only if context is lost

[Kevin Hilman <khilman-l0cyMroinI0@public.gmane.org>]

Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org> 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-l0cyMroinI0@public.gmane.org>
> ---
>  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