Re: I2C: Fix unhandled fault in i2c-omap controller

[Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>]

On Thu, 13 Mar 2008 17:51:24 +0200, Tony Lindgren wrote:
> Hi Jean,
> 
> Here's an omap I2C fix that would be nice to get into 2.6.25 if still
> possible.

Review:

> commit 8e4f19286e68d38d589451ad2a10545f2c40032d
> Author: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> Date:   Mon Feb 18 20:16:00 2008 -0800
> 
>     I2C: Fix unhandled fault in i2c-omap controller
>     
>     If an I2C interrupt happens between disabling interface clock
>     and functional clock, the interrupt handler will produce an
>     external abort on non-linefetch error when trying to access
>     driver registers while interface clock is disabled.
>     
>     This patch fixes the problem by saving and disabling i2c-omap
>     interrupt before turning off the clocks. Also disable functional
>     clock before the interface clock as suggested by Paul Walmsley.
>     
>     Patch also renames enable/disable_clocks functions to unidle/idle
>     functions. Note that the driver is currently not taking advantage
>     of the idle interrupts. To use the idle interrupts, driver would
>     have to enable interface clock based on the idle interrupt
>     and dev->idle flag.
>     
>     Cc: Paul Walmsley <paul-DWxLp4Yu+b8AvxtiuMwx3w@public.gmane.org>
>     Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index da66397..862dd69 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -128,6 +128,8 @@ struct omap_i2c_dev {
>  	size_t			buf_len;
>  	struct i2c_adapter	adapter;
>  	unsigned		rev1:1;
> +	unsigned		idle:1;
> +	u16			iestate;	/* Saved interrupt register */
>  };
>  
>  static inline void omap_i2c_write_reg(struct omap_i2c_dev *i2c_dev,
> @@ -174,18 +176,30 @@ static void omap_i2c_put_clocks(struct omap_i2c_dev *dev)
>  	}
>  }
>  
> -static void omap_i2c_enable_clocks(struct omap_i2c_dev *dev)
> +static void omap_i2c_unidle(struct omap_i2c_dev *dev)
>  {
>  	if (dev->iclk != NULL)
>  		clk_enable(dev->iclk);
>  	clk_enable(dev->fclk);
> +	if (dev->iestate)
> +		omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
> +	dev->idle = 0;
>  }
>  
> -static void omap_i2c_disable_clocks(struct omap_i2c_dev *dev)
> +static void omap_i2c_idle(struct omap_i2c_dev *dev)
>  {
> +	u16 iv;
> +
> +	dev->idle = 1;
> +	dev->iestate = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
> +	omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, 0);
> +	if (dev->rev1)
> +		iv = omap_i2c_read_reg(dev, OMAP_I2C_IV_REG);

Reading OMAP_I2C_IV_REG but not doing anything with the value? This
deserves at least a comment if this is done on purpose (but then I
don't think you need the iv variable at all.)

> +	else
> +		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, dev->iestate);
> +	clk_disable(dev->fclk);
>  	if (dev->iclk != NULL)
>  		clk_disable(dev->iclk);
> -	clk_disable(dev->fclk);
>  }
>  
>  static int omap_i2c_init(struct omap_i2c_dev *dev)
> @@ -360,7 +374,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  	int i;
>  	int r;
>  
> -	omap_i2c_enable_clocks(dev);
> +	omap_i2c_unidle(dev);
>  
>  	if ((r = omap_i2c_wait_for_bb(dev)) < 0)
>  		goto out;
> @@ -374,7 +388,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  	if (r == 0)
>  		r = num;
>  out:
> -	omap_i2c_disable_clocks(dev);
> +	omap_i2c_idle(dev);
>  	return r;
>  }
>  
> @@ -403,6 +417,9 @@ omap_i2c_rev1_isr(int this_irq, void *dev_id)
>  	struct omap_i2c_dev *dev = dev_id;
>  	u16 iv, w;
>  
> +	if (dev->idle)
> +		return IRQ_NONE;
> +
>  	iv = omap_i2c_read_reg(dev, OMAP_I2C_IV_REG);
>  	switch (iv) {
>  	case 0x00:	/* None */
> @@ -457,6 +474,9 @@ omap_i2c_isr(int this_irq, void *dev_id)
>  	u16 stat, w;
>  	int count = 0;
>  
> +	if (dev->idle)
> +		return IRQ_NONE;
> +
>  	bits = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
>  	while ((stat = (omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG))) & bits) {
>  		dev_dbg(dev->dev, "IRQ (ISR = 0x%04x)\n", stat);
> @@ -575,7 +595,7 @@ omap_i2c_probe(struct platform_device *pdev)
>  	if ((r = omap_i2c_get_clocks(dev)) != 0)
>  		goto err_free_mem;
>  
> -	omap_i2c_enable_clocks(dev);
> +	omap_i2c_unidle(dev);
>  
>  	if (cpu_is_omap15xx())
>  		dev->rev1 = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) < 0x20;
> @@ -610,7 +630,7 @@ omap_i2c_probe(struct platform_device *pdev)
>  		goto err_free_irq;
>  	}
>  
> -	omap_i2c_disable_clocks(dev);
> +	omap_i2c_idle(dev);
>  
>  	return 0;
>  
> @@ -618,7 +638,7 @@ err_free_irq:
>  	free_irq(dev->irq, dev);
>  err_unuse_clocks:
>  	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
> -	omap_i2c_disable_clocks(dev);
> +	omap_i2c_idle(dev);
>  	omap_i2c_put_clocks(dev);
>  err_free_mem:
>  	platform_set_drvdata(pdev, NULL);

The rest of the patch looks OK to me.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c