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
From: Simon Glass <[email protected]>
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 <[email protected]>
Cc: Grant Grundler <[email protected]>
Signed-off-by: Naveen Krishna Chatradhi <[email protected]>
---
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
From: Simon Glass <[email protected]>
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 <[email protected]>
Cc: Grant Grundler <[email protected]>
Signed-off-by: Naveen Krishna Chatradhi <[email protected]>
---
.../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
Acked-by: Kyungmin Park <[email protected]>
On Thu, Nov 29, 2012 at 2:05 PM, Naveen Krishna Chatradhi
<[email protected]> wrote:
> From: Simon Glass <[email protected]>
>
> 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 <[email protected]>
> Cc: Grant Grundler <[email protected]>
> Signed-off-by: Naveen Krishna Chatradhi <[email protected]>
> ---
> 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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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.
+Olof
On Thu, Nov 29, 2012 at 8:34 AM, Mark Brown
<[email protected]> 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
On Thu, Nov 29, 2012 at 6:13 PM, Simon Glass <[email protected]> wrote:
> +Olof
>
> On Thu, Nov 29, 2012 at 8:34 AM, Mark Brown
> <[email protected]> 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
Hello Wolfram,
Sorry for a replying after really long time.
On 24 January 2013 17:50, Wolfram Sang <[email protected]> wrote:
> On Thu, Nov 29, 2012 at 10:35:34AM +0530, Naveen Krishna Chatradhi wrote:
>> From: Simon Glass <[email protected]>
>>
>> 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 <[email protected]>
>> Cc: Grant Grundler <[email protected]>
>> Signed-off-by: Naveen Krishna Chatradhi <[email protected]>
>
> 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 :)