[PATCH 0/2] i2c: Implement generic gpio based bus arbitration

[PATCH 0/2] i2c: Implement generic gpio based bus arbitration

[Naveen Krishna Chatradhi]

This patchset adds
1. Support for generic gpio based i2c bus arbitration between 2 i2c Masters
	Ex: between AP(exynos), device(EC).
2. Documentation and sample implmentation in i2c-s3c2410 driver.

Naveen Krishna Chatradhi (2):
  i2c-core: Add gpio based bus arbitration implementation
  i2c-s3c2410: Add GPIO based bus arbitration functionality

 .../devicetree/bindings/i2c/arbitrator-i2c.txt     |   56 ++++++++++++++
 drivers/i2c/busses/i2c-s3c2410.c                   |   79 ++++++++++++--------
 drivers/i2c/i2c-core.c                             |   67 +++++++++++++++++
 drivers/of/of_i2c.c                                |   27 +++++++
 include/linux/i2c.h                                |   17 +++++
 include/linux/of_i2c.h                             |    2 +
 6 files changed, 216 insertions(+), 32 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/i2c/arbitrator-i2c.txt

-- 
1.7.9.5

[PATCH 1/2] i2c-core: Add gpio based bus arbitration implementation

[Naveen Krishna Chatradhi]

The arbitrator is a general purpose function which uses two GPIOs to
communicate with another device to claim/release a bus.

i2c_transfer()
	if adapter->gpio_arbit
		i2c_bus_claim();
			__i2c_transfer();
		i2c_bus_release();

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>
---
 .../devicetree/bindings/i2c/arbitrator-i2c.txt     |   56 ++++++++++++++++
 drivers/i2c/i2c-core.c                             |   67 ++++++++++++++++++++
 drivers/of/of_i2c.c                                |   27 ++++++++
 include/linux/i2c.h                                |   17 +++++
 include/linux/of_i2c.h                             |    2 +
 5 files changed, 169 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/arbitrator-i2c.txt

diff --git a/Documentation/devicetree/bindings/i2c/arbitrator-i2c.txt b/Documentation/devicetree/bindings/i2c/arbitrator-i2c.txt
new file mode 100644
index 0000000..cb91ea8
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/arbitrator-i2c.txt
@@ -0,0 +1,56 @@
+Device-Tree bindings for i2c gpio based bus arbitrator
+
+bus-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 (Client)
+wants to claim the bus. This allows two masters to share the same I2C bus.
+
+Required properties:
+	- bus-needs-gpio-arbitration;
+	- bus-arbitration-gpios: AP_CLAIM and Client_CLAIM gpios
+	- bus-arbitration-slew-delay-us:
+	- bus-arbitration-wait-retry-us:
+	- bus-arbitration-wait-free-us:
+
+Example nodes:
+
+i2c@0 {
+	/* If you want GPIO-based bus arbitration */
+	bus-needs-gpio-arbitration;
+	bus-arbitration-gpios = <&gpf0 3 1 0 0>, /* AP_CLAIM */
+		<&gpe0 4 0 3 0>; /* EC_CLAIM */
+
+	bus-arbitration-slew-delay-us = <10>;
+	bus-arbitration-wait-retry-us = <2000>;
+	bus-arbitration-wait-free-us = <50000>;
+};
+
+GPIO-based Arbitration
+======================
+This uses GPIO lines between the AP (SoC) 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)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index a7edf98..222a6da 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -32,6 +32,7 @@
 #include <linux/i2c.h>
 #include <linux/init.h>
 #include <linux/idr.h>
+#include <linux/delay.h>
 #include <linux/mutex.h>
 #include <linux/of_device.h>
 #include <linux/completion.h>
@@ -39,6 +40,7 @@
 #include <linux/irqflags.h>
 #include <linux/rwsem.h>
 #include <linux/pm_runtime.h>
+#include <linux/gpio.h>
 #include <asm/uaccess.h>
 
 #include "i2c-core.h"
@@ -1256,6 +1258,59 @@ void i2c_release_client(struct i2c_client *client)
 }
 EXPORT_SYMBOL(i2c_release_client);
 
+/**
+ * If we have enabled arbitration on this bus, claim the i2c bus, using
+ * the GPIO-based signalling protocol.
+ */
+static int i2c_bus_claim(struct i2c_gpio_arbit *i2c_arbit)
+{
+	unsigned long stop_retry, stop_time;
+	unsigned int gpio;
+
+	/* Start a round of trying to claim the bus */
+	stop_time = jiffies + usecs_to_jiffies(i2c_arbit->wait_free_us) + 1;
+	do {
+		/* Indicate that we want to claim the bus */
+		gpio_set_value(i2c_arbit->arb_gpios[I2C_ARB_GPIO_AP], 0);
+		udelay(i2c_arbit->slew_delay_us);
+
+		/* Wait for the EC to release it */
+		stop_retry = jiffies +
+			usecs_to_jiffies(i2c_arbit->wait_retry_us) + 1;
+		while (time_before(jiffies, stop_retry)) {
+			gpio = i2c_arbit->arb_gpios[I2C_ARB_GPIO_EC];
+			if (gpio_get_value(gpio)) {
+				/* 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_arbit->arb_gpios[I2C_ARB_GPIO_AP], 1);
+
+		usleep_range(i2c_arbit->wait_retry_us,
+					i2c_arbit->wait_retry_us * 2);
+	} while (time_before(jiffies, stop_time));
+
+	/* Give up, release our claim */
+	gpio_set_value(i2c_arbit->arb_gpios[I2C_ARB_GPIO_AP], 1);
+	udelay(i2c_arbit->slew_delay_us);
+
+	return -EBUSY;
+}
+
+/**
+ * If we have enabled arbitration on this bus, release the i2c bus.
+ */
+static void i2c_bus_release(struct i2c_gpio_arbit *i2c_arbit)
+{
+	/* Release the bus and wait for the EC to notice */
+	gpio_set_value(i2c_arbit->arb_gpios[I2C_ARB_GPIO_AP], 1);
+	udelay(i2c_arbit->slew_delay_us);
+}
+
 struct i2c_cmd_arg {
 	unsigned	cmd;
 	void		*arg;
@@ -1412,7 +1467,19 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 			i2c_lock_adapter(adap);
 		}
 
+		if (adap->gpio_arbit) {
+			if (i2c_bus_claim(adap->gpio_arbit)) {
+				dev_err(&adap->dev, "I2C: Could not claim bus, timeout\n");
+				return -EBUSY;
+			}
+		}
+
 		ret = __i2c_transfer(adap, msgs, num);
+
+		/* Release the bus if needed */
+		if (adap->gpio_arbit)
+			i2c_bus_release(adap->gpio_arbit);
+
 		i2c_unlock_adapter(adap);
 
 		return ret;
diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
index 3550f3b..4ec3f44 100644
--- a/drivers/of/of_i2c.c
+++ b/drivers/of/of_i2c.c
@@ -111,4 +111,31 @@ struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node *node)
 }
 EXPORT_SYMBOL(of_find_i2c_adapter_by_node);
 
+/*
+ * Property "bus-needs-gpio-arbitration" in DT node enables
+ * GPIO based bus arbitration.
+ * Populates the default timing values if not specified.
+ *
+ * Returns NULL, if "bus-needs-gpio-arbitration" is not specified.
+ */
+struct i2c_gpio_arbit *of_get_arbitrator_info(struct device_node *np,
+				struct i2c_gpio_arbit *i2c_arbit)
+{
+	if (of_get_property(np, "bus-needs-gpio-arbitration", NULL)) {
+		if (of_property_read_u32(np, "bus-arbitration-slew-delay-us",
+				&i2c_arbit->slew_delay_us))
+			i2c_arbit->slew_delay_us = 10;
+		if (of_property_read_u32(np, "bus-arbitration-wait-retry-us",
+				&i2c_arbit->wait_retry_us))
+			i2c_arbit->wait_retry_us = 2000;
+		if (of_property_read_u32(np, "bus-arbitration-wait-free-us",
+				&i2c_arbit->wait_free_us))
+			i2c_arbit->wait_free_us = 50000;
+	} else
+		i2c_arbit = NULL;
+
+	return i2c_arbit;
+}
+EXPORT_SYMBOL_GPL(of_get_arbitrator_info);
+
 MODULE_LICENSE("GPL");
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 800de22..d1ae491 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -295,6 +295,22 @@ struct i2c_board_info {
 #define I2C_BOARD_INFO(dev_type, dev_addr) \
 	.type = dev_type, .addr = (dev_addr)
 
+enum {
+	I2C_ARB_GPIO_AP,		/* AP claims i2c bus */
+	I2C_ARB_GPIO_EC,		/* EC claims i2c bus */
+
+	I2C_ARB_GPIO_COUNT,
+};
+
+/* I2C bus GPIO based arbitration information */
+struct i2c_gpio_arbit {
+	int			arb_gpios[I2C_ARB_GPIO_COUNT];
+
+	/* Arbitration parameters */
+	unsigned int		slew_delay_us;
+	unsigned int		wait_retry_us;
+	unsigned int		wait_free_us;
+};
 
 #if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE)
 /* Add-on boards should register/unregister their devices; e.g. a board
@@ -388,6 +404,7 @@ struct i2c_adapter {
 	int nr;
 	char name[48];
 	struct completion dev_released;
+	struct i2c_gpio_arbit *gpio_arbit;
 
 	struct mutex userspace_clients_lock;
 	struct list_head userspace_clients;
diff --git a/include/linux/of_i2c.h b/include/linux/of_i2c.h
index 1cb775f..c57ffba 100644
--- a/include/linux/of_i2c.h
+++ b/include/linux/of_i2c.h
@@ -24,6 +24,8 @@ extern struct i2c_client *of_find_i2c_device_by_node(struct device_node *node);
 extern struct i2c_adapter *of_find_i2c_adapter_by_node(
 						struct device_node *node);
 
+extern struct i2c_gpio_arbit *of_get_arbitrator_info(struct device_node *node,
+					struct i2c_gpio_arbit *i2c_arbit);
 #else
 static inline void of_i2c_register_devices(struct i2c_adapter *adap)
 {
-- 
1.7.9.5

[PATCH 2/2] i2c-s3c2410: Add GPIO based bus arbitration functionality

[Naveen Krishna Chatradhi]

Makes use of the generic fucntions in of_i2c.c to parse arbitration
timing information and GPIOs for arbitration.

Also uses devm_gpio_request() instead of gpio_request() and
removes the gpio_free() calls

Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
---
 drivers/i2c/busses/i2c-s3c2410.c |   79 +++++++++++++++++++++++---------------
 1 file changed, 47 insertions(+), 32 deletions(-)

diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index e93e7d6..d055cf8 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -855,54 +855,61 @@ 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)
+static int of_i2c_parse_gpio(struct device *dev, 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 = dev->of_node;
+	int idx, gpio;
 
-	for (idx = 0; idx < 2; idx++) {
-		gpio = of_get_gpio(i2c->dev->of_node, idx);
+	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);
-			goto free_gpio;
+			dev_dbg(dev, "invalid gpio[%d]: %d\n", idx, gpio);
+			if (idx || required) {
+				dev_err(dev, "invalid gpio[%d]: %d\n",
+					idx, gpio);
+			}
+			return -EINVAL;
 		}
-		i2c->gpios[idx] = gpio;
+		gpios[idx] = gpio;
 
-		ret = gpio_request(gpio, "i2c-bus");
-		if (ret) {
-			dev_err(i2c->dev, "gpio [%d] request failed\n", gpio);
-			goto free_gpio;
+		if (devm_gpio_request(dev, gpio, "i2c-bus")) {
+			dev_err(dev, "gpio [%d] request failed\n", gpio);
+			return -EINVAL;
 		}
 	}
 	return 0;
-
-free_gpio:
-	while (--idx >= 0)
-		gpio_free(i2c->gpios[idx]);
-	return -EINVAL;
 }
 
-static void s3c24xx_i2c_dt_gpio_free(struct s3c24xx_i2c *i2c)
+static int s3c24xx_i2c_parse_dt_gpio(struct s3c24xx_i2c *i2c)
 {
-	unsigned int idx;
+	int ret = 0;
 
 	if (i2c->quirks & QUIRK_NO_GPIO)
-		return;
+		goto out;
+
+	if (of_i2c_parse_gpio(i2c->dev, "gpios", i2c->gpios, 2, true)) {
+		ret = -EINVAL;
+		goto out;
+	}
 
-	for (idx = 0; idx < 2; idx++)
-		gpio_free(i2c->gpios[idx]);
+	if (i2c->adap.gpio_arbit) {
+		if (!of_i2c_parse_gpio(i2c->dev, "bus-arbitration-gpios",
+			i2c->adap.gpio_arbit->arb_gpios, I2C_ARB_GPIO_COUNT,
+			false)) {
+			dev_warn(i2c->dev, "GPIO-based arbitration enabled");
+		} else
+			ret = -EINVAL;
+	}
+
+ out:
+	return ret;
 }
 #else
 static int s3c24xx_i2c_parse_dt_gpio(struct s3c24xx_i2c *i2c)
 {
 	return 0;
 }
-
-static void s3c24xx_i2c_dt_gpio_free(struct s3c24xx_i2c *i2c)
-{
-}
 #endif
 
 /* s3c24xx_i2c_init
@@ -981,6 +988,7 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
 {
 	struct s3c24xx_i2c *i2c;
 	struct s3c2410_platform_i2c *pdata = NULL;
+	struct i2c_gpio_arbit *arbit = NULL;
 	struct resource *res;
 	int ret;
 
@@ -1004,11 +1012,21 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
 		goto err_noclk;
 	}
 
+	arbit = devm_kzalloc(&pdev->dev, sizeof(*arbit), GFP_KERNEL);
+	if (!arbit) {
+		ret = -ENOMEM;
+		goto err_noclk;
+	}
+
 	i2c->quirks = s3c24xx_get_device_quirks(pdev);
 	if (pdata)
 		memcpy(i2c->pdata, pdata, sizeof(*pdata));
-	else
+	else {
 		s3c24xx_i2c_parse_dt(pdev->dev.of_node, i2c);
+		/* Arbitration parameters */
+		i2c->adap.gpio_arbit = of_get_arbitrator_info(
+					pdev->dev.of_node, arbit);
+	}
 
 	strlcpy(i2c->adap.name, "s3c2410-i2c", sizeof(i2c->adap.name));
 	i2c->adap.owner   = THIS_MODULE;
@@ -1158,9 +1176,6 @@ static int s3c24xx_i2c_remove(struct platform_device *pdev)
 	clk_disable_unprepare(i2c->clk);
 	clk_put(i2c->clk);
 
-	if (pdev->dev.of_node && IS_ERR(i2c->pctrl))
-		s3c24xx_i2c_dt_gpio_free(i2c);
-
 	return 0;
 }
 
-- 
1.7.9.5

Re: [PATCH 1/2] i2c-core: Add gpio based bus arbitration implementation

[Stephen Warren]

On 12/13/2012 10:50 PM, 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.

> diff --git a/Documentation/devicetree/bindings/i2c/arbitrator-i2c.txt b/Documentation/devicetree/bindings/i2c/arbitrator-i2c.txt
> new file mode 100644
> index 0000000..cb91ea8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/arbitrator-i2c.txt
> @@ -0,0 +1,56 @@
> +Device-Tree bindings for i2c gpio based bus arbitrator
> +
> +bus-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 (Client)
> +wants to claim the bus. This allows two masters to share the same I2C bus.

I'm confused why this is even needed; the I2C protocol itself defines
how multi-master is supposed to work, just using the regular SCL/SDA lines.

Re: [PATCH 1/2] i2c-core: Add gpio based bus arbitration implementation

[Mark Brown]

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

On Fri, Dec 14, 2012 at 09:06:49AM -0700, Stephen Warren wrote:
> On 12/13/2012 10:50 PM, Naveen Krishna Chatradhi wrote:

> > +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 (Client)
> > +wants to claim the bus. This allows two masters to share the same I2C bus.

> I'm confused why this is even needed; the I2C protocol itself defines
> how multi-master is supposed to work, just using the regular SCL/SDA lines.

Practically speaking essentially no systems actually do that - mostly
Linux will treat failure to get the bus as an error for example.  You
also get things like read operations which appear as multiple
transactions on the I2C bus so require something higher level than what
multi-master provides.

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

Re: [PATCH 1/2] i2c-core: Add gpio based bus arbitration implementation

[Grant Likely]

On Fri, 14 Dec 2012 11:20:53 +0530, Naveen Krishna Chatradhi <ch.naveen-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> The arbitrator is a general purpose function which uses two GPIOs to
> communicate with another device to claim/release a bus.
> 
> i2c_transfer()
> 	if adapter->gpio_arbit
> 		i2c_bus_claim();
> 			__i2c_transfer();
> 		i2c_bus_release();
> 
> 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>

Hi Naveen,

I'm not convinced on the design of this protocol. It won't scale beyond
2 bus masters and it seems very specific to the design of a specific
piece of hardware. I don't think it is mature enough to bake into the
core i2c infrastructure or bindings. Nor do I think it will handle other
busses properly.

I can see two alternatives here.
1) build in hooks for doing i2c bus claim/release, but don't put this
specific implementation into the core infrastructure
2) Create an i2c bridge driver. This would kind of be like an i2c
multiplexer, but it would only have one bus and it would include the
knowledge of how to use the GPIO lines for bus arbitration.

I think option 2 would be the cleanest option. It would be straight
forward to design a binding for it by placing a node between the i2c
bus and all the i2c clients. For example:

i2c@60000000 {
	compatible = "acme,some-i2c-device";
	#address-cells = <1>;
	#size-cells = <0>;

	i2c-bridge {
		#address-cells = <1>;
		#size-cells = <0>;
		compatible = "samaung,i2c-gpio-arbitrate";

		bus-arbitration-gpios = <&gpf0 3 1 0 0>, /* AP_CLAIM */
			<&gpe0 4 0 3 0>; /* EC_CLAIM */
	
		bus-arbitration-slew-delay-us = <10>;
		bus-arbitration-wait-retry-us = <2000>;
		bus-arbitration-wait-free-us = <50000>;

		i2c@52 {
			// Normal i2c device
		};
	};
};

I don't know what the state of the i2c subsystem is with regard to
supporting i2c multiplexer devices, so there might be some changes
needed there, but I can't see it being particularly complex. It should
just be a device in the middle. Any i2c device that is a child of the
bridge would send transfers to the bridge, and the bridge would be
responsible to claim the bus and then pass the transfer through
unchanged.

That eliminates the problem of trying to design an arbitration scheme
that works for all.

g.

Re: [PATCH 1/2] i2c-core: Add gpio based bus arbitration implementation

[Mark Brown]

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

On Wed, Dec 19, 2012 at 12:32:01PM +0000, Grant Likely wrote:

> I'm not convinced on the design of this protocol. It won't scale beyond
> 2 bus masters and it seems very specific to the design of a specific
> piece of hardware. I don't think it is mature enough to bake into the

I ought to point out that the original patch would've baked this into
the Samsung I2C driver but I asked for it to be split out as it's
nothing to do with the controller really and I'm sure I've seen it
before.

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

Re: [PATCH 1/2] i2c-core: Add gpio based bus arbitration implementation

[Simon Glass]

On Wed, Dec 19, 2012 at 9:14 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Dec 19, 2012 at 12:32:01PM +0000, Grant Likely wrote:
>
>> I'm not convinced on the design of this protocol. It won't scale beyond
>> 2 bus masters and it seems very specific to the design of a specific
>> piece of hardware. I don't think it is mature enough to bake into the
>
> I ought to point out that the original patch would've baked this into
> the Samsung I2C driver but I asked for it to be split out as it's
> nothing to do with the controller really and I'm sure I've seen it
> before.

Grant your idea looks nice to me. I only worry if we should wait until
we have a second user before going so far with it. But it is certainly
a nice general solution.

The scaling beyond two bus masters would require the code to be
changed to check more input lines from other masters. Best avoided for
now I think until we have a use for it.

Regards,
Simon

Re: [PATCH 1/2] i2c-core: Add gpio based bus arbitration implementation

[Grant Likely]

On Wed, Dec 19, 2012 at 5:14 PM, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> On Wed, Dec 19, 2012 at 12:32:01PM +0000, Grant Likely wrote:
>
>> I'm not convinced on the design of this protocol. It won't scale beyond
>> 2 bus masters and it seems very specific to the design of a specific
>> piece of hardware. I don't think it is mature enough to bake into the
>
> I ought to point out that the original patch would've baked this into
> the Samsung I2C driver but I asked for it to be split out as it's
> nothing to do with the controller really and I'm sure I've seen it
> before.

Indeed, baking it into the driver is also the wrong abstraction.

g.

--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 1/2] i2c-core: Add gpio based bus arbitration implementation

[Grant Likely]

On Thu, Dec 20, 2012 at 12:17 AM, Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> On Wed, Dec 19, 2012 at 9:14 AM, Mark Brown
> <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
>> On Wed, Dec 19, 2012 at 12:32:01PM +0000, Grant Likely wrote:
>>
>>> I'm not convinced on the design of this protocol. It won't scale beyond
>>> 2 bus masters and it seems very specific to the design of a specific
>>> piece of hardware. I don't think it is mature enough to bake into the
>>
>> I ought to point out that the original patch would've baked this into
>> the Samsung I2C driver but I asked for it to be split out as it's
>> nothing to do with the controller really and I'm sure I've seen it
>> before.
>
> Grant your idea looks nice to me. I only worry if we should wait until
> we have a second user before going so far with it. But it is certainly
> a nice general solution.
>
> The scaling beyond two bus masters would require the code to be
> changed to check more input lines from other masters. Best avoided for
> now I think until we have a use for it.

There is two parts to this; the design of the binding, and the
implementation. Of the two, the binding is the most difficult to
change, so it is important to get something we can live with long term
sorted out now. The implementation can start somewhat naive, with a
plan for how to extend it.

Regarding the scaling to multiple devices, it is a much smaller issue
(if an issue at all) if the arbitration back end is pluggable. Then it
becomes easy to support hardware with a different arbitration
protocol. If it gets baked into the core i2c code, then we've got a
problem and a lot more scrutiny needs to be put into this specific
protocol.

g.

Re: [PATCH 1/2] i2c-core: Add gpio based bus arbitration implementation

[Wolfram Sang]

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

Hi,

On Sat, Dec 15, 2012 at 11:21:36PM +0900, Mark Brown wrote:
> On Fri, Dec 14, 2012 at 09:06:49AM -0700, Stephen Warren wrote:
> > On 12/13/2012 10:50 PM, Naveen Krishna Chatradhi wrote:
> 
> > > +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 (Client)
> > > +wants to claim the bus. This allows two masters to share the same I2C bus.
> 
> > I'm confused why this is even needed; the I2C protocol itself defines
> > how multi-master is supposed to work, just using the regular SCL/SDA lines.
> 
> Practically speaking essentially no systems actually do that - mostly
> Linux will treat failure to get the bus as an error for example.

It is true that Linux currently does not have proper multi-master
support. It is worth a look what is missing and how far we can get with
the I2C specified arbitration IMO.

> also get things like read operations which appear as multiple
> transactions on the I2C bus so require something higher level than what
> multi-master provides.

I don't get what you mean here. Can you elaborate?

That being said. Grant's design was the most promising one.

Thanks,

   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-core: Add gpio based bus arbitration implementation

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 609 bytes --]

On Thu, Jan 24, 2013 at 12:13:29PM +0100, Wolfram Sang wrote:
> On Sat, Dec 15, 2012 at 11:21:36PM +0900, Mark Brown wrote:

> > also get things like read operations which appear as multiple
> > transactions on the I2C bus so require something higher level than what
> > multi-master provides.

> I don't get what you mean here. Can you elaborate?

A read is typically implemented as a write of the register address
followed by a read of the value, usually with the ability to free the
bus in between.  If two devices attempt to access the register map
simultaneously this results in the address going wrong.

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

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 1/2] i2c-core: Add gpio based bus arbitration implementation

[Wolfram Sang]

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

On Thu, Jan 24, 2013 at 07:18:47PM +0800, Mark Brown wrote:
> On Thu, Jan 24, 2013 at 12:13:29PM +0100, Wolfram Sang wrote:
> > On Sat, Dec 15, 2012 at 11:21:36PM +0900, Mark Brown wrote:
> 
> > > also get things like read operations which appear as multiple
> > > transactions on the I2C bus so require something higher level than what
> > > multi-master provides.
> 
> > I don't get what you mean here. Can you elaborate?
> 
> A read is typically implemented as a write of the register address
> followed by a read of the value, usually with the ability to free the
> bus in between.  If two devices attempt to access the register map
> simultaneously this results in the address going wrong.

Could happen. But in what situations will one not use repeated start
here? Especially when designing a multi-master bus?

-- 
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-core: Add gpio based bus arbitration implementation

[Mark Brown]

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

On Thu, Jan 24, 2013 at 12:39:48PM +0100, Wolfram Sang wrote:
> On Thu, Jan 24, 2013 at 07:18:47PM +0800, Mark Brown wrote:

> > A read is typically implemented as a write of the register address
> > followed by a read of the value, usually with the ability to free the
> > bus in between.  If two devices attempt to access the register map
> > simultaneously this results in the address going wrong.

> Could happen. But in what situations will one not use repeated start
> here? Especially when designing a multi-master bus?

Well, you're depending on the specific drivers doing things that way and
it's actually quite rare for the controller drivers in Linux to support
I2C_M_NOSTART which discourages this.

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