Re: [PATCH 1/2] i2c-s3c2410: Leave the bus disabled unless it is inuse

[Wolfram Sang]

On Thu, Nov 29, 2012 at 10:35:34AM +0530, Naveen Krishna Chatradhi wrote:
> From: Simon Glass <sjg@xxxxxxxxxxxx>
> 
> There is a rather odd feature of the exynos i2c controller that if it
> is left enabled, it can lock itself up with the clk line held low.
> This makes the bus unusable.
> 
> Unfortunately, the s3c24xx_i2c_set_master() function does not notice
> this, and reports a timeout. From then on the bus cannot be used until
> the AP is rebooted.
> 
> The problem happens when any sort of interrupt occurs (e.g. due to a
> bus transition) when we are not in the middle of a transaction. We
> have seen many instances of this when U-Boot leaves the bus apparently
> happy, but Linux cannot access it.
> 
> The current code is therefore pretty fragile.
> 
> This fixes things by leaving the bus disabled unless we are actually
> in a transaction. We enable the bus at the start of the transaction and
> disable it at the end. That way we won't get interrupts and will not
> lock up the bus.
> 
> It might be possible to clear pending interrupts on start-up, but this
> seems to be a more robust solution. We can't service interrupts when
> we are not in a transaction, and anyway would rather not lock up the
> bus while we try.
> 
> Signed-off-by: Simon Glass <sjg@xxxxxxxxxxxx>
> Cc: Grant Grundler <grundler@xxxxxxxxxxxx>
> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@xxxxxxxxxxx>

So, I assume this patch is still needed despite the ongoing discussion
about arbitration?

> ---
>  drivers/i2c/busses/i2c-s3c2410.c |   37 +++++++++++++++++++++++++++++++++----
>  1 file changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
> index e93e7d6..2fd346d 100644
> --- a/drivers/i2c/busses/i2c-s3c2410.c
> +++ b/drivers/i2c/busses/i2c-s3c2410.c
> @@ -186,6 +186,31 @@ static inline void s3c24xx_i2c_enable_irq(struct s3c24xx_i2c *i2c)
>  	writel(tmp | S3C2410_IICCON_IRQEN, i2c->regs + S3C2410_IICCON);
>  }
>  
> +/*
> + * Disable the bus so that we won't get any interrupts from now on, or try
> + * to drive any lines. This is the default state when we don't have
> + * anything to send/receive.
> + *
> + * If there is an event on the bus, or we have a pre-existing event at

Otherwise, if...

> + * kernel boot time, we may not notice the event and the I2C controller
> + * will lock the bus with the I2C clock line low indefinitely.
> + */
> +static inline void s3c24xx_i2c_disable_bus(struct s3c24xx_i2c *i2c)
> +{
> +	unsigned long tmp;
> +
> +	/* Stop driving the I2C pins */
> +	tmp = readl(i2c->regs + S3C2410_IICSTAT);
> +	tmp &= ~S3C2410_IICSTAT_TXRXEN;
> +	writel(tmp, i2c->regs + S3C2410_IICSTAT);
> +
> +	/* We don't expect any interrupts now, and don't want send acks */
> +	tmp = readl(i2c->regs + S3C2410_IICCON);
> +	tmp &= ~(S3C2410_IICCON_IRQEN | S3C2410_IICCON_IRQPEND |
> +		S3C2410_IICCON_ACKEN);
> +	writel(tmp, i2c->regs + S3C2410_IICCON);
> +}
> +
>  
>  /* s3c24xx_i2c_message_start
>   *
> @@ -646,7 +671,11 @@ static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c *i2c,
>  
>  	s3c24xx_i2c_wait_idle(i2c);
>  
> +	s3c24xx_i2c_disable_bus(i2c);
> +
>   out:
> +	i2c->state = STATE_IDLE;
> +

Why is the state change after the label?

>  	return ret;
>  }
>  
> @@ -912,7 +941,6 @@ static void s3c24xx_i2c_dt_gpio_free(struct s3c24xx_i2c *i2c)
>  
>  static int s3c24xx_i2c_init(struct s3c24xx_i2c *i2c)
>  {
> -	unsigned long iicon = S3C2410_IICCON_IRQEN | S3C2410_IICCON_ACKEN;
>  	struct s3c2410_platform_i2c *pdata;
>  	unsigned int freq;
>  
> @@ -926,12 +954,12 @@ static int s3c24xx_i2c_init(struct s3c24xx_i2c *i2c)
>  
>  	dev_info(i2c->dev, "slave address 0x%02x\n", pdata->slave_addr);
>  
> -	writel(iicon, i2c->regs + S3C2410_IICCON);
> +	writel(0, i2c->regs + S3C2410_IICCON);
> +	writel(0, i2c->regs + S3C2410_IICSTAT);
>  
>  	/* we need to work out the divisors for the clock... */
>  
>  	if (s3c24xx_i2c_clockrate(i2c, &freq) != 0) {
> -		writel(0, i2c->regs + S3C2410_IICCON);
>  		dev_err(i2c->dev, "cannot meet bus frequency required\n");
>  		return -EINVAL;
>  	}
> @@ -939,7 +967,8 @@ static int s3c24xx_i2c_init(struct s3c24xx_i2c *i2c)
>  	/* todo - check that the i2c lines aren't being dragged anywhere */
>  
>  	dev_info(i2c->dev, "bus frequency set to %d KHz\n", freq);
> -	dev_dbg(i2c->dev, "S3C2410_IICCON=0x%02lx\n", iicon);
> +	dev_dbg(i2c->dev, "S3C2410_IICCON=0x%02x\n",
> +		readl(i2c->regs + S3C2410_IICCON));
>  

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Attachment: signature.asc
Description: Digital signature