[PATCH 0/2] i2c-s3c2410: Implementation bus arbitration support

[PATCH 0/2] i2c-s3c2410: Implementation bus arbitration support

[Naveen Krishna Chatradhi]

This patchset adds support for i2c bus arbitration between AP(exynos),
device(EC) in i2c-s3c2410 driver.

Simon Glass (2):
  i2c-s3c2410: Leave the bus disabled unless it is in use
  i2c-s3c2410: Add bus arbitration implementation

 .../devicetree/bindings/i2c/samsung-i2c.txt        |   46 +++++
 drivers/i2c/busses/i2c-s3c2410.c                   |  204 ++++++++++++++++++--
 2 files changed, 233 insertions(+), 17 deletions(-)

-- 
1.7.9.5

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

[Naveen Krishna Chatradhi]

From: Simon Glass <sjg@chromium.org>

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@chromium.org>
Cc: Grant Grundler <grundler@chromium.org>
Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
---
 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
+ * 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;
+
 	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));
 
 	return 0;
 }
-- 
1.7.9.5

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

[Kyungmin Park]

Acked-by: Kyungmin Park <kyungmin.park@samsung.com>

On Thu, Nov 29, 2012 at 2:05 PM, Naveen Krishna Chatradhi
<ch.naveen@samsung.com> wrote:
> From: Simon Glass <sjg@chromium.org>
>
> 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@chromium.org>
> Cc: Grant Grundler <grundler@chromium.org>
> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
> ---
>  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
> + * 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;
> +
>         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));
>
>         return 0;
>  }
> --
> 1.7.9.5
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Naveen Krishna Ch]

On 29 November 2012 20:14, Kyungmin Park <kmpark-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> wrote:
> Acked-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

I don't see this patch landed any where in linux-i2c tree, Though it was acked.
Was it missed or should i be doing something for this to be merged ??

>
> On Thu, Nov 29, 2012 at 2:05 PM, Naveen Krishna Chatradhi
> <ch.naveen-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
>> From: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>>
>> 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-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> Cc: Grant Grundler <grundler-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> ---
>>  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
>> + * 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;
>> +
>>         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));
>>
>>         return 0;
>>  }
>> --
>> 1.7.9.5
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Shine bright,
(: Nav :)

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 4881 bytes --]

On Thu, Nov 29, 2012 at 10:35:34AM +0530, Naveen Krishna Chatradhi wrote:
> From: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> 
> 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-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Cc: Grant Grundler <grundler-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Naveen Krishna Ch]

Hello Wolfram,

Sorry for a replying after really long time.

On 24 January 2013 17:50, Wolfram Sang <w.sang@pengutronix.de> wrote:
> On Thu, Nov 29, 2012 at 10:35:34AM +0530, Naveen Krishna Chatradhi wrote:
>> From: Simon Glass <sjg@chromium.org>
>>
>> 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@chromium.org>
>> Cc: Grant Grundler <grundler@chromium.org>
>> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
>
> So, I assume this patch is still needed despite the ongoing discussion
> about arbitration?
Yes, this is an i2c crontroller fix.
>
>> ---
>>  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?
As the interrupts are enabled in the beginning of this function.
and interrupts in STATE_IDLE needs handling in a different way.

This was added with the intention the irq routine will handle the cases
>
>>       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
Will re-base and post the patch..
>
> --
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |



-- 
Shine bright,
(: Nav :)

[PATCH] i2c-s3c2410: Leave the bus disabled unless it is in use

[Naveen Krishna Chatradhi]

From: Simon Glass <sjg@chromium.org>

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@chromium.org>
Cc: Grant Grundler <grundler@chromium.org>
Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 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 684d21e..9d39511 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -601,6 +601,31 @@ static irqreturn_t s3c24xx_i2c_irq(int irqno, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+/*
+ * 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
+ * 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_set_master
  *
@@ -735,7 +760,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;
+
 	return ret;
 }
 
@@ -1004,7 +1033,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;
 
@@ -1018,12 +1046,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;
 	}
@@ -1031,7 +1059,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));
 
 	return 0;
 }
-- 
1.7.9.5

Re: [PATCH] i2c-s3c2410: Leave the bus disabled unless it is in use

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1519 bytes --]

On Fri, Feb 07, 2014 at 02:24:09PM +0530, Naveen Krishna Chatradhi wrote:
> From: Simon Glass <sjg@chromium.org>
> 
> 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@chromium.org>
> Cc: Grant Grundler <grundler@chromium.org>
> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>

Applied to for-next, thanks!


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] i2c-s3c2410: Leave the bus disabled unless it is in use

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1695 bytes --]

On Wed, Mar 12, 2014 at 08:27:14PM +0100, Wolfram Sang wrote:
> On Fri, Feb 07, 2014 at 02:24:09PM +0530, Naveen Krishna Chatradhi wrote:
> > From: Simon Glass <sjg@chromium.org>
> > 
> > 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@chromium.org>
> > Cc: Grant Grundler <grundler@chromium.org>
> > Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
> > Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> 
> Applied to for-next, thanks!

Do you consider this relevant for stable?


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 2/2] i2c-s3c2410: Add bus arbitration implementation

[Naveen Krishna Chatradhi]

From: Simon Glass <sjg@chromium.org>

The arbitrator is a general purpose function which uses two GPIOs to
communicate with another device to claim/release a bus. We use it to
arbitrate an i2c port between the AP and the EC.

Signed-off-by: Simon Glass <sjg@chromium.org>
Cc: Grant Grundler <grundler@chromium.org>
Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
---
 .../devicetree/bindings/i2c/samsung-i2c.txt        |   46 ++++++
 drivers/i2c/busses/i2c-s3c2410.c                   |  167 ++++++++++++++++++--
 2 files changed, 200 insertions(+), 13 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/samsung-i2c.txt b/Documentation/devicetree/bindings/i2c/samsung-i2c.txt
index e9611ac..4bed49f 100644
--- a/Documentation/devicetree/bindings/i2c/samsung-i2c.txt
+++ b/Documentation/devicetree/bindings/i2c/samsung-i2c.txt
@@ -28,6 +28,11 @@ Optional properties:
     specified, default value is 0.
   - samsung,i2c-max-bus-freq: Desired frequency in Hz of the bus. If not
     specified, the default value in Hz is 100000.
+  - samsung,arbitration-gpios : Two GPIOs to use with the GPIO-based bus
+    arbitration protocol (see below). The first should be an output, and is
+    used to claim the I2C bus, the second should be an input, and signals that
+    the other side wants to claim the bus. This allows two masters to share
+    the same I2C bus.
 
 Example:
 
@@ -52,4 +57,45 @@ Example:
 			compatible = "wlf,wm8994";
 			reg = <0x1a>;
 		};
+
+		/* If you want GPIO-based bus arbitration */
+		samsung,arbitration-gpios = <&gpf0 3 1 0 0>,	/* AP_CLAIM */
+			<&gpe0 4 0 3 0>;			/* EC_CLAIM */
 	};
+
+
+GPIO-based Arbitration
+======================
+(documented here for want of a better place - an implementation is in the
+i2c-s3c2410 driver)
+
+This uses GPIO lines between the AP (Exynos) and an attached EC (embedded
+controller) which both want to talk on the same I2C bus as master.
+
+The AP and EC each have a 'bus claim' line, which is an output that the
+other can see. These are both active low, with pull-ups enabled.
+
+- AP_CLAIM: output from AP, signalling to the EC that the AP wants the bus
+- EC_CLAIM: output from EC, signalling to the AP that the EC wants the bus
+
+
+Algorithm
+---------
+The basic algorithm is to assert your line when you want the bus, then make
+sure that the other side doesn't want it also. A detailed explanation is best
+done with an example.
+
+Let's say the AP wants to claim the bus. It:
+1. Asserts AP_CLAIM
+2. Waits a little bit for the other side to notice (slew time, say 10
+microseconds)
+3. Checks EC_CLAIM. If this is not asserted, then the AP has the bus, and
+we are done
+4. Otherwise, wait for a few milliseconds and see if EC_CLAIM is released
+5. If not, back off, release the claim and wait for a few more milliseconds
+6. Go back to 1 (until retry time has expired)
+
+To release the bus, just de-assert the claim line. If the other wants the bus
+it will notice soon enough.
+
+The same algorithm applies on the EC side.
diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index 2fd346d..87a6928 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -62,6 +62,13 @@ enum s3c24xx_i2c_state {
 	STATE_STOP
 };
 
+enum {
+	I2C_ARB_GPIO_AP,		/* AP claims i2c bus */
+	I2C_ARB_GPIO_EC,		/* EC claims i2c bus */
+
+	I2C_ARB_GPIO_COUNT,
+};
+
 struct s3c24xx_i2c {
 	wait_queue_head_t	wait;
 	unsigned int            quirks;
@@ -85,10 +92,16 @@ struct s3c24xx_i2c {
 
 	struct s3c2410_platform_i2c	*pdata;
 	int			gpios[2];
+	int			arb_gpios[I2C_ARB_GPIO_COUNT];
 	struct pinctrl          *pctrl;
 #ifdef CONFIG_CPU_FREQ
 	struct notifier_block	freq_transition;
 #endif
+	/* Arbitration parameters */
+	bool			arbitrate;
+	unsigned int		slew_delay_us;
+	unsigned int		wait_retry_us;
+	unsigned int		wait_free_us;
 };
 
 static struct platform_device_id s3c24xx_driver_ids[] = {
@@ -116,6 +129,61 @@ static const struct of_device_id s3c24xx_i2c_match[] = {
 MODULE_DEVICE_TABLE(of, s3c24xx_i2c_match);
 #endif
 
+/*
+ * If we have enabled arbitration on this bus, claim the i2c bus, using
+ * the GPIO-based signalling protocol.
+ */
+int s3c24xx_i2c_claim(struct s3c24xx_i2c *i2c)
+{
+	unsigned long stop_retry, stop_time;
+
+	if (!i2c->arbitrate)
+		return 0;
+
+	/* Start a round of trying to claim the bus */
+	stop_time = jiffies + usecs_to_jiffies(i2c->wait_free_us) + 1;
+	do {
+		/* Indicate that we want to claim the bus */
+		gpio_set_value(i2c->arb_gpios[I2C_ARB_GPIO_AP], 0);
+		udelay(i2c->slew_delay_us);
+
+		/* Wait for the EC to release it */
+		stop_retry = jiffies + usecs_to_jiffies(i2c->wait_retry_us) + 1;
+		while (time_before(jiffies, stop_retry)) {
+			if (gpio_get_value(i2c->arb_gpios[I2C_ARB_GPIO_EC])) {
+				/* We got it, so return */
+				return 0;
+			}
+
+			usleep_range(50, 200);
+		}
+
+		/* It didn't release, so give up, wait, and try again */
+		gpio_set_value(i2c->arb_gpios[I2C_ARB_GPIO_AP], 1);
+
+		usleep_range(i2c->wait_retry_us, i2c->wait_retry_us * 2);
+	} while (time_before(jiffies, stop_time));
+
+	/* Give up, release our claim */
+	gpio_set_value(i2c->arb_gpios[I2C_ARB_GPIO_AP], 1);
+	udelay(i2c->slew_delay_us);
+	dev_err(i2c->dev, "I2C: Could not claim bus, timeout\n");
+	return -EBUSY;
+}
+
+/*
+ * If we have enabled arbitration on this bus, release the i2c bus, using
+ * the GPIO-based signalling protocol.
+ */
+void s3c24xx_i2c_release(struct s3c24xx_i2c *i2c)
+{
+	if (i2c->arbitrate) {
+		/* Release the bus and wait for the EC to notice */
+		gpio_set_value(i2c->arb_gpios[I2C_ARB_GPIO_AP], 1);
+		udelay(i2c->slew_delay_us);
+	}
+}
+
 /* s3c24xx_get_device_quirks
  *
  * Get controller type either from device tree or platform device variant.
@@ -637,6 +705,15 @@ static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c *i2c,
 	if (i2c->suspended)
 		return -EIO;
 
+	/*
+	 * Claim the bus if needed.
+	 *
+	 * Note, this needs a lock. How come s3c24xx_i2c_set_master() below
+	 * is outside the lock?
+	 */
+	if (s3c24xx_i2c_claim(i2c))
+		return -EBUSY;
+
 	ret = s3c24xx_i2c_set_master(i2c);
 	if (ret != 0) {
 		dev_err(i2c->dev, "cannot get bus (error %d)\n", ret);
@@ -676,6 +753,9 @@ static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c *i2c,
  out:
 	i2c->state = STATE_IDLE;
 
+	/* Release the bus if needed */
+	s3c24xx_i2c_release(i2c);
+
 	return ret;
 }
 
@@ -884,20 +964,42 @@ static inline void s3c24xx_i2c_deregister_cpufreq(struct s3c24xx_i2c *i2c)
 #endif
 
 #ifdef CONFIG_OF
-static int s3c24xx_i2c_parse_dt_gpio(struct s3c24xx_i2c *i2c)
+/*
+ * Parse a list of GPIOs from a node property and request each one
+ *
+ * This might be better as of_gpio_get_array() one day.
+ *
+ * @param i2c		i2c driver data
+ * @param name		name of property to read from
+ * @param gpios		returns an array of GPIOs
+ * @param count		number of GPIOs to read
+ * @param required	true if the property is required, false if it is
+ *			optional so no warning is printed (avoids a separate
+ *			check in caller)
+ * @return 0 on success, -ve on error, in which case no GPIOs remain
+ * requested
+ */
+static int s3c24xx_i2c_parse_gpio(struct s3c24xx_i2c *i2c, const char *name,
+		int gpios[], size_t count, bool required)
 {
-	int idx, gpio, ret;
-
-	if (i2c->quirks & QUIRK_NO_GPIO)
-		return 0;
+	struct device_node *dn = i2c->dev->of_node;
+	unsigned int idx;
+	int gpio, ret;
 
-	for (idx = 0; idx < 2; idx++) {
-		gpio = of_get_gpio(i2c->dev->of_node, idx);
+	/*
+	 * It would be nice if there were an of function to return a list
+	 * of GPIOs
+	 */
+	for (idx = 0; idx < count; idx++) {
+		gpio = of_get_named_gpio(dn, name, idx);
 		if (!gpio_is_valid(gpio)) {
-			dev_err(i2c->dev, "invalid gpio[%d]: %d\n", idx, gpio);
+			if (idx || required) {
+				dev_err(i2c->dev, "invalid gpio[%d]: %d\n",
+					idx, gpio);
+			}
 			goto free_gpio;
 		}
-		i2c->gpios[idx] = gpio;
+		gpios[idx] = gpio;
 
 		ret = gpio_request(gpio, "i2c-bus");
 		if (ret) {
@@ -909,19 +1011,47 @@ static int s3c24xx_i2c_parse_dt_gpio(struct s3c24xx_i2c *i2c)
 
 free_gpio:
 	while (--idx >= 0)
-		gpio_free(i2c->gpios[idx]);
+		gpio_free(gpios[idx]);
 	return -EINVAL;
 }
 
-static void s3c24xx_i2c_dt_gpio_free(struct s3c24xx_i2c *i2c)
+/* Free a list of GPIOs */
+static void s3c24xx_i2c_free_gpios(int gpios[], int count)
 {
 	unsigned int idx;
 
+	for (idx = 0; idx < count; idx++) {
+		if (gpio_is_valid(gpios[idx]))
+			gpio_free(gpios[idx]);
+	}
+}
+
+static int s3c24xx_i2c_parse_dt_gpio(struct s3c24xx_i2c *i2c)
+{
+	if (i2c->quirks & QUIRK_NO_GPIO)
+		return 0;
+
+	if (s3c24xx_i2c_parse_gpio(i2c, "gpios", i2c->gpios, 2, true))
+		return -EINVAL;
+
+	if (!s3c24xx_i2c_parse_gpio(i2c, "samsung,arbitration-gpios",
+			i2c->arb_gpios, I2C_ARB_GPIO_COUNT, false)) {
+		i2c->arbitrate = 1;
+		dev_warn(i2c->dev, "GPIO-based arbitration enabled");
+	}
+
+	return 0;
+
+}
+
+static void s3c24xx_i2c_dt_gpio_free(struct s3c24xx_i2c *i2c)
+{
 	if (i2c->quirks & QUIRK_NO_GPIO)
 		return;
 
-	for (idx = 0; idx < 2; idx++)
-		gpio_free(i2c->gpios[idx]);
+	s3c24xx_i2c_free_gpios(i2c->gpios, 2);
+	if (i2c->arbitrate)
+		s3c24xx_i2c_free_gpios(i2c->arb_gpios, I2C_ARB_GPIO_COUNT);
 }
 #else
 static int s3c24xx_i2c_parse_dt_gpio(struct s3c24xx_i2c *i2c)
@@ -992,6 +1122,17 @@ s3c24xx_i2c_parse_dt(struct device_node *np, struct s3c24xx_i2c *i2c)
 	of_property_read_u32(np, "samsung,i2c-slave-addr", &pdata->slave_addr);
 	of_property_read_u32(np, "samsung,i2c-max-bus-freq",
 				(u32 *)&pdata->frequency);
+
+	/* Arbitration parameters */
+	if (of_property_read_u32(np, "samsung,slew-delay-us",
+				 &i2c->slew_delay_us))
+		i2c->slew_delay_us = 10;
+	if (of_property_read_u32(np, "samsung,wait-retry-us",
+				 &i2c->wait_retry_us))
+		i2c->wait_retry_us = 2000;
+	if (of_property_read_u32(np, "samsung,wait-free-us",
+				 &i2c->wait_free_us))
+		i2c->wait_free_us = 50000;
 }
 #else
 static void
-- 
1.7.9.5

Re: [PATCH 2/2] i2c-s3c2410: Add bus arbitration implementation

[Mark Brown]

On Thu, Nov 29, 2012 at 10:35:35AM +0530, Naveen Krishna Chatradhi wrote:

> The arbitrator is a general purpose function which uses two GPIOs to
> communicate with another device to claim/release a bus. We use it to
> arbitrate an i2c port between the AP and the EC.

Should this not be layerd on top of the I2C controller rather than part
of the controller driver?  It doesn't seem terribly controller specific.

Re: [PATCH 2/2] i2c-s3c2410: Add bus arbitration implementation

[Simon Glass]

+Olof

On Thu, Nov 29, 2012 at 8:34 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Nov 29, 2012 at 10:35:35AM +0530, Naveen Krishna Chatradhi wrote:
>
>> The arbitrator is a general purpose function which uses two GPIOs to
>> communicate with another device to claim/release a bus. We use it to
>> arbitrate an i2c port between the AP and the EC.
>
> Should this not be layerd on top of the I2C controller rather than part
> of the controller driver?  It doesn't seem terribly controller specific.

It was originally done separately but I think it was felt that this
was overly complex. Olof can you please comment on this?

Regards,
Simon

Re: [PATCH 2/2] i2c-s3c2410: Add bus arbitration implementation

[Olof Johansson]

On Thu, Nov 29, 2012 at 6:13 PM, Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> +Olof
>
> On Thu, Nov 29, 2012 at 8:34 AM, Mark Brown
> <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
>> On Thu, Nov 29, 2012 at 10:35:35AM +0530, Naveen Krishna Chatradhi wrote:
>>
>>> The arbitrator is a general purpose function which uses two GPIOs to
>>> communicate with another device to claim/release a bus. We use it to
>>> arbitrate an i2c port between the AP and the EC.
>>
>> Should this not be layerd on top of the I2C controller rather than part
>> of the controller driver?  It doesn't seem terribly controller specific.
>
> It was originally done separately but I think it was felt that this
> was overly complex. Olof can you please comment on this?

it is indeed not controller specific per se, but we are unaware of any
other platform/driver using it. So, it seemed reasonable to implement
it in the driver as long as we have only one user; if another one
comes along it's of course better to move it to the common i2c code.

At least that was my opinion at the time. I could be convinced
otherwise if someone else has strong opinions on the matter.


-Olof

Re: [PATCH 2/2] i2c-s3c2410: Add bus arbitration implementation

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1021 bytes --]

On Thu, Nov 29, 2012 at 10:14:58PM -0800, Olof Johansson wrote:
> On Thu, Nov 29, 2012 at 6:13 PM, Simon Glass <sjg@chromium.org> wrote:

> > It was originally done separately but I think it was felt that this
> > was overly complex. Olof can you please comment on this?

> it is indeed not controller specific per se, but we are unaware of any
> other platform/driver using it. So, it seemed reasonable to implement
> it in the driver as long as we have only one user; if another one
> comes along it's of course better to move it to the common i2c code.

> At least that was my opinion at the time. I could be convinced
> otherwise if someone else has strong opinions on the matter.

This sort of approach is half the reason SPI ended up being so fun...  I
suspect if you look hard enough you'll find that this is just the first
time someone tried to upstream such a scheme.  This is all especially
true for the DT bindings, even if the implementation is driver local for
now it'd be better to define generic bindings.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] i2c-s3c2410: Add bus arbitration implementation

[Olof Johansson]

On Sat, Dec 1, 2012 at 5:26 AM, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> On Thu, Nov 29, 2012 at 10:14:58PM -0800, Olof Johansson wrote:
>> On Thu, Nov 29, 2012 at 6:13 PM, Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>
>> > It was originally done separately but I think it was felt that this
>> > was overly complex. Olof can you please comment on this?
>
>> it is indeed not controller specific per se, but we are unaware of any
>> other platform/driver using it. So, it seemed reasonable to implement
>> it in the driver as long as we have only one user; if another one
>> comes along it's of course better to move it to the common i2c code.
>
>> At least that was my opinion at the time. I could be convinced
>> otherwise if someone else has strong opinions on the matter.
>
> This sort of approach is half the reason SPI ended up being so fun...  I
> suspect if you look hard enough you'll find that this is just the first
> time someone tried to upstream such a scheme.  This is all especially
> true for the DT bindings, even if the implementation is driver local for
> now it'd be better to define generic bindings.

Ok, sounds like we might as well make it generic then. Naveen?


-Olof

Re: [PATCH 2/2] i2c-s3c2410: Add bus arbitration implementation

[Naveen Krishna Ch]

On 4 December 2012 05:41, Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org> wrote:
> On Sat, Dec 1, 2012 at 5:26 AM, Mark Brown
> <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
>> On Thu, Nov 29, 2012 at 10:14:58PM -0800, Olof Johansson wrote:
>>> On Thu, Nov 29, 2012 at 6:13 PM, Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>>
>>> > It was originally done separately but I think it was felt that this
>>> > was overly complex. Olof can you please comment on this?
>>
>>> it is indeed not controller specific per se, but we are unaware of any
>>> other platform/driver using it. So, it seemed reasonable to implement
>>> it in the driver as long as we have only one user; if another one
>>> comes along it's of course better to move it to the common i2c code.
>>
>>> At least that was my opinion at the time. I could be convinced
>>> otherwise if someone else has strong opinions on the matter.
>>
>> This sort of approach is half the reason SPI ended up being so fun...  I
>> suspect if you look hard enough you'll find that this is just the first
>> time someone tried to upstream such a scheme.  This is all especially
>> true for the DT bindings, even if the implementation is driver local for
>> now it'd be better to define generic bindings.
>
> Ok, sounds like we might as well make it generic then. Naveen?
Thanks for the comments.

Sure, Will send an RFC soon.
>
>
> -Olof
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Shine bright,
(: Nav :)