Add cros_ec changes for newer boards

---
Changes in v2: None

 drivers/mfd/cros_ec_spi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/mfd/cros_ec_spi.c +1 -1

index 84af8d7..c185eb6 100644
@@ -219,7 +219,7 @@ static int cros_ec_command_spi_xfer(struct cros_ec_device *ec_dev,
 		ktime_get_ts(&ts);
 		delay = timespec_to_ns(&ts) - ec_spi->last_transfer_ns;
 		if (delay < EC_SPI_RECOVERY_TIME_NS)
-			ndelay(delay);
+			ndelay(EC_SPI_RECOVERY_TIME_NS - delay);
 	}
 
 	/* Transmit phase - send our message */

Signature · 1 line

1.9.1.423.g4596e3a

Lee Jones

Acked-by on v2 1/7

2014-04-23 12:33 UTC #2 lore raw

Re: [PATCH v2 1/7] mfd: cros_ec: spi: calculate delay between transfers correctly

Quoted 15 lines

From: David Hendricks <[email protected]>
To avoid spamming the EC we calculate the time between the previous transfer and the current transfer and force a delay if the time delta is too small.
However, a small miscalculation causes the delay period to be far too short. Most noticably this impacts commands with a long turnaround time such as EC firmware reads and writes.
Signed-off-by David Hendricks <[email protected]>
Signed-off-by Doug Anderson <[email protected]>
Reviewed-by Simon Glass <[email protected]>
Tested-by Andrew Bresticker <[email protected]>
Tested-by Stephen Warren <[email protected]>

Quoted 5 lines

---
Changes in v2: None

 drivers/mfd/cros_ec_spi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Waiting for other subsystems to Ack, so for now:

Signature · 4 lines

Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Doug Anderson

author

2014-04-22 16:45 UTC #3 lore raw

PATCH v2 2/7 mfd: cros_ec: spi: Add mutex to cros_ec_spi

The main transfer function for cros_ec_spi can be called by more than one client at a time. Make sure that those clients don't stomp on each other by locking the bus for the duration of the transfer function.

---
Changes in v2: None

 drivers/mfd/cros_ec_spi.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

drivers/mfd/cros_ec_spi.c +21 -5

index c185eb6..a2a605d 100644
@@ -65,11 +65,13 @@
  *	if no record
  * @end_of_msg_delay: used to set the delay_usecs on the spi_transfer that
  *      is sent when we want to turn off CS at the end of a transaction.
+ * @lock: mutex to ensure only one user of cros_ec_command_spi_xfer at a time
  */
 struct cros_ec_spi {
 	struct spi_device *spi;
 	s64 last_transfer_ns;
 	unsigned int end_of_msg_delay;
+	struct mutex lock;
 };
 
 static void debug_packet(struct device *dev, const char *name, u8 *ptr,

@@ -208,6 +210,13 @@ static int cros_ec_command_spi_xfer(struct cros_ec_device *ec_dev,
 	int ret = 0, final_ret;
 	struct timespec ts;
 
+	/*
+	 * We have the shared ec_dev buffer plus we do lots of separate spi_sync
+	 * calls, so we need to make sure only one person is using this at a
+	 * time.
+	 */
+	mutex_lock(&ec_spi->lock);
+
 	len = cros_ec_prepare_tx(ec_dev, ec_msg);
 	dev_dbg(ec_dev->dev, "prepared, len=%d\n", len);

@@ -260,7 +269,7 @@ static int cros_ec_command_spi_xfer(struct cros_ec_device *ec_dev,
 		ret = final_ret;
 	if (ret < 0) {
 		dev_err(ec_dev->dev, "spi transfer failed: %d\n", ret);
-		return ret;
+		goto exit;
 	}
 
 	/* check response error code */

@@ -269,14 +278,16 @@ static int cros_ec_command_spi_xfer(struct cros_ec_device *ec_dev,
 		dev_warn(ec_dev->dev, "command 0x%02x returned an error %d\n",
 			 ec_msg->cmd, ptr[0]);
 		debug_packet(ec_dev->dev, "in_err", ptr, len);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto exit;
 	}
 	len = ptr[1];
 	sum = ptr[0] + ptr[1];
 	if (len > ec_msg->in_len) {
 		dev_err(ec_dev->dev, "packet too long (%d bytes, expected %d)",
 			len, ec_msg->in_len);
-		return -ENOSPC;
+		ret = -ENOSPC;
+		goto exit;
 	}
 
 	/* copy response packet payload and compute checksum */

@@ -293,10 +304,14 @@ static int cros_ec_command_spi_xfer(struct cros_ec_device *ec_dev,
 		dev_err(ec_dev->dev,
 			"bad packet checksum, expected %02x, got %02x\n",
 			sum, ptr[len + 2]);
-		return -EBADMSG;
+		ret = -EBADMSG;
+		goto exit;
 	}
 
-	return 0;
+	ret = 0;
+exit:
+	mutex_unlock(&ec_spi->lock);
+	return ret;
 }
 
 static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev)

@@ -327,6 +342,7 @@ static int cros_ec_spi_probe(struct spi_device *spi)
 	if (ec_spi == NULL)
 		return -ENOMEM;
 	ec_spi->spi = spi;
+	mutex_init(&ec_spi->lock);
 	ec_dev = devm_kzalloc(dev, sizeof(*ec_dev), GFP_KERNEL);
 	if (!ec_dev)
 		return -ENOMEM;

Signature · 1 line

1.9.1.423.g4596e3a

Lee Jones

Acked-by on v2 2/7

2014-04-23 12:34 UTC #4 lore raw

Re: [PATCH v2 2/7] mfd: cros_ec: spi: Add mutex to cros_ec_spi

Quoted 9 lines · Doug Anderson

The main transfer function for cros_ec_spi can be called by more than one client at a time. Make sure that those clients don't stomp on each other by locking the bus for the duration of the transfer function.
Signed-off-by Doug Anderson <[email protected]>
Reviewed-by Simon Glass <[email protected]>
Tested-by Andrew Bresticker <[email protected]>
Tested-by Stephen Warren <[email protected]>

Quoted 5 lines

---
Changes in v2: None

 drivers/mfd/cros_ec_spi.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

Waiting for other subsystems to Ack, so for now:

Signature · 4 lines

Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Doug Anderson

author

2014-04-22 16:45 UTC #5 lore raw

PATCH v2 3/7 mfd: cros_ec: spi: Make the cros_ec_spi timeout more reliable

The cros_ec_spi transfer had two problems with its timeout code:

1. It looked at the timeout even in the case that it found valid data.
2. If the cros_ec_spi code got switched out for a while, it's possible it could get a timeout after a single loop. Let's be paranoid and make sure we do one last transfer after the timeout expires.

---
Changes in v2: None

 drivers/mfd/cros_ec_spi.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

drivers/mfd/cros_ec_spi.c +12 -3

index a2a605d..4f863c3 100644
@@ -113,7 +113,9 @@ static int cros_ec_spi_receive_response(struct cros_ec_device *ec_dev,
 
 	/* Receive data until we see the header byte */
 	deadline = jiffies + msecs_to_jiffies(EC_MSG_DEADLINE_MS);
-	do {
+	while (true) {
+		unsigned long start_jiffies = jiffies;
+
 		memset(&trans, 0, sizeof(trans));
 		trans.cs_change = 1;
 		trans.rx_buf = ptr = ec_dev->din;

@@ -134,12 +136,19 @@ static int cros_ec_spi_receive_response(struct cros_ec_device *ec_dev,
 				break;
 			}
 		}
+		if (ptr != end)
+			break;
 
-		if (time_after(jiffies, deadline)) {
+		/*
+		 * Use the time at the start of the loop as a timeout.  This
+		 * gives us one last shot at getting the transfer and is useful
+		 * in case we got context switched out for a while.
+		 */
+		if (time_after(start_jiffies, deadline)) {
 			dev_warn(ec_dev->dev, "EC failed to respond in time\n");
 			return -ETIMEDOUT;
 		}
-	} while (ptr == end);
+	}
 
 	/*
 	 * ptr now points to the header byte. Copy any valid data to the

Signature · 1 line

1.9.1.423.g4596e3a

Lee Jones

Acked-by on v2 3/7

2014-04-23 12:35 UTC #6 lore raw

Re: [PATCH v2 3/7] mfd: cros_ec: spi: Make the cros_ec_spi timeout more reliable

Quoted 11 lines · Doug Anderson

The cros_ec_spi transfer had two problems with its timeout code:
1. It looked at the timeout even in the case that it found valid data.
2. If the cros_ec_spi code got switched out for a while, it's possible it could get a timeout after a single loop. Let's be paranoid and make sure we do one last transfer after the timeout expires.
Signed-off-by Doug Anderson <[email protected]>
Reviewed-by Simon Glass <[email protected]>
Tested-by Andrew Bresticker <[email protected]>
Tested-by Stephen Warren <[email protected]>

Quoted 5 lines

---
Changes in v2: None

 drivers/mfd/cros_ec_spi.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Waiting for other subsystems to Ack, so for now:

Signature · 4 lines

Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Doug Anderson

author

2014-04-22 16:45 UTC #7 lore raw

PATCH v2 4/7 mfd: cros_ec: spi: Increase cros_ec_spi deadline from 5ms to 100ms

We're adding i2c tunneling to the list of things that goes over cros_ec. i2c tunneling can be slooooooow, so increase our deadline to 100ms to account for that.

---
Changes in v2: None

 drivers/mfd/cros_ec_spi.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

drivers/mfd/cros_ec_spi.c +16 -8

index 4f863c3..0b8d328 100644
@@ -39,14 +39,22 @@
 #define EC_MSG_PREAMBLE_COUNT		32
 
 /*
-  * We must get a response from the EC in 5ms. This is a very long
-  * time, but the flash write command can take 2-3ms. The EC command
-  * processing is currently not very fast (about 500us). We could
-  * look at speeding this up and making the flash write command a
-  * 'slow' command, requiring a GET_STATUS wait loop, like flash
-  * erase.
-  */
-#define EC_MSG_DEADLINE_MS		5
+ * Allow for a long time for the EC to respond.  We support i2c
+ * tunneling and support fairly long messages for the tunnel (249
+ * bytes long at the moment).  If we're talking to a 100 kHz device
+ * on the other end and need to transfer ~256 bytes, then we need:
+ *  10 us/bit * ~10 bits/byte * ~256 bytes = ~25ms
+ *
+ * We'll wait 4 times that to handle clock stretching and other
+ * paranoia.
+ *
+ * It's pretty unlikely that we'll really see a 249 byte tunnel in
+ * anything other than testing.  If this was more common we might
+ * consider having slow commands like this require a GET_STATUS
+ * wait loop.  The 'flash write' command would be another candidate
+ * for this, clocking in at 2-3ms.
+ */
+#define EC_MSG_DEADLINE_MS		100
 
 /*
   * Time between raising the SPI chip select (for the end of a

Signature · 1 line

1.9.1.423.g4596e3a

Lee Jones

Acked-by on v2 4/7

2014-04-23 12:36 UTC #8 lore raw

Re: [PATCH v2 4/7] mfd: cros_ec: spi: Increase cros_ec_spi deadline from 5ms to 100ms

Quoted 8 lines · Doug Anderson

We're adding i2c tunneling to the list of things that goes over cros_ec. i2c tunneling can be slooooooow, so increase our deadline to 100ms to account for that.
Signed-off-by Doug Anderson <[email protected]>
Reviewed-by Simon Glass <[email protected]>
Tested-by Andrew Bresticker <[email protected]>
Tested-by Stephen Warren <[email protected]>

Quoted 5 lines

---
Changes in v2: None

 drivers/mfd/cros_ec_spi.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

Waiting for other subsystems to Ack, so for now:

Signature · 4 lines

Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Doug Anderson

author

2014-04-22 16:45 UTC #9 lore raw

PATCH v2 5/7 mfd: cros_ec: Sync to the latest cros_ec_commands.h from EC sources

From: Bill Richardson <[email protected]>

This just updates include/linux/mfd/cros_ec_commands.h to match the latest EC version (which is the One True Source for such things). See
<https://chromium.googlesource.com/chromiumos/platform/ec>

[dianders: took today's ToT version from the Chromium OS EC; deleted references to cros_ec_dev and cros_ec_lpc since those aren't upstream yet]

Signed-off-by Bill Richardson <[email protected]>

---
Changes in v2: None

 drivers/mfd/cros_ec.c                |    2 +-
 include/linux/mfd/cros_ec.h          |    4 +-
 include/linux/mfd/cros_ec_commands.h | 1128 +++++++++++++++++++++++++++++++---
 3 files changed, 1059 insertions(+), 75 deletions(-)

drivers/mfd/cros_ec.c +1 -1

index 783fe2e..c58ab96 100644
@@ -30,7 +30,7 @@ int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
 	uint8_t *out;
 	int csum, i;
 
-	BUG_ON(msg->out_len > EC_HOST_PARAM_SIZE);
+	BUG_ON(msg->out_len > EC_PROTO2_MAX_PARAM_SIZE);
 	out = ec_dev->dout;
 	out[0] = EC_CMD_VERSION0 + msg->version;
 	out[1] = msg->cmd;

include/linux/mfd/cros_ec.h +2 -2

index 032af7f..887ef4f 100644
@@ -29,8 +29,8 @@ enum {
 	EC_MSG_RX_PROTO_BYTES	= 3,
 
 	/* Max length of messages */
-	EC_MSG_BYTES		= EC_HOST_PARAM_SIZE + EC_MSG_TX_PROTO_BYTES,
-
+	EC_MSG_BYTES		= EC_PROTO2_MAX_PARAM_SIZE +
+					EC_MSG_TX_PROTO_BYTES,
 };
 
 /**

include/linux/mfd/cros_ec_commands.h +1056 ~~-72~~

1,391 lines not on this page · raw message

Signature · 1 line

1.9.1.423.g4596e3a

Lee Jones

Acked-by on v2 5/7

2014-04-23 12:36 UTC #10 lore raw

Re: [PATCH v2 5/7] mfd: cros_ec: Sync to the latest cros_ec_commands.h from EC sources

Quoted 15 lines · Doug Anderson

From: Bill Richardson <[email protected]>
This just updates include/linux/mfd/cros_ec_commands.h to match the latest EC version (which is the One True Source for such things). See
<https://chromium.googlesource.com/chromiumos/platform/ec>
[dianders: took today's ToT version from the Chromium OS EC; deleted references to cros_ec_dev and cros_ec_lpc since those aren't upstream yet]
Signed-off-by Bill Richardson <[email protected]>
Signed-off-by Doug Anderson <[email protected]>
Reviewed-by Simon Glass <[email protected]>
Tested-by Andrew Bresticker <[email protected]>
Tested-by Stephen Warren <[email protected]>

Quoted 7 lines

---
Changes in v2: None

 drivers/mfd/cros_ec.c                |    2 +-
 include/linux/mfd/cros_ec.h          |    4 +-
 include/linux/mfd/cros_ec_commands.h | 1128 +++++++++++++++++++++++++++++++---
 3 files changed, 1059 insertions(+), 75 deletions(-)

Waiting for other subsystems to Ack, so for now:

Signature · 4 lines

Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Doug Anderson

author

2014-04-22 16:45 UTC #11 lore raw

PATCH v2 6/7 i2c: ChromeOS EC tunnel driver

On ARM Chromebooks we have a few devices that are accessed by both the AP (the main "Application Processor") and the EC (the Embedded Controller). These are:
* The battery (sbs-battery).
* The power management unit tps65090.

On the original Samsung ARM Chromebook these devices were on an I2C bus that was shared between the AP and the EC and arbitrated using some extranal GPIOs (see i2c-arb-gpio-challenge).

The original arbitration scheme worked well enough but had some downsides:
* It was nonstandard (not using standard I2C multimaster)
* It only worked if the EC-AP communication was I2C
* It was relatively hard to debug problems (hard to tell if i2c issues were caused by the EC, the AP, or some device on the bus).

On the HP Chromebook 11 the design was changed to:
* The AP/EC comms were still i2c, but the battery/tps65090 were no longer on the bus used for AP/EC communication. The battery was exposed to the AP through a limited i2c tunnel and tps65090 was exposed to the AP through a custom Linux driver.

On the Samsung ARM Chromebook 2 the scheme is changed yet again, now:
* The AP/EC comms are now using SPI for faster speeds.
* The EC's i2c bus is exposed to the AP through a full i2c tunnel.

The upstream "tegra124-venice2" uses the same scheme as the Samsung ARM Chromebook 2, though it has a different set of components on the other side of the bus.

This driver supports the scheme used by the Samsung ARM Chromebook 2. Future patches to this driver could add support for the battery tunnel on the HP Chromebook 11 (and perhaps could even be used to access tps65090 on the HP Chromebook 11 instead of using a special driver, but I haven't researched that enough).

Signed-off-by Vincent Palatin <[email protected]>

Signed-off-by Simon Glass <[email protected]>

---
Changes in v2:
- Update tunnel binding as per swarren

 .../devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt |  39 +++
 drivers/i2c/busses/Kconfig                         |   9 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-cros-ec-tunnel.c            | 304 +++++++++++++++++++++
 drivers/mfd/cros_ec.c                              |   5 +
 5 files changed, 358 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt
 create mode 100644 drivers/i2c/busses/i2c-cros-ec-tunnel.c

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt new file +39 -0

new file mode 100644
index 0000000..898f030
@@ -0,0 +1,39 @@
+I2C bus that tunnels through the ChromeOS EC (cros-ec)
+======================================================
+On some ChromeOS board designs we've got a connection to the EC (embedded
+controller) but no direct connection to some devices on the other side of
+the EC (like a battery and PMIC).  To get access to those devices we need
+to tunnel our i2c commands through the EC.
+
+The node for this device should be under a cros-ec node like google,cros-ec-spi
+or google,cros-ec-i2c.
+
+
+Required properties:
+- compatible: google,cros-ec-i2c-tunnel
+- google,remote-bus: The EC bus we'd like to talk to.
+
+Optional child nodes:
+- One node per I2C device connected to the tunnelled I2C bus.
+
+
+Example:
+	cros-ec@0 {
+		compatible = "google,cros-ec-spi";
+
+		...
+
+		i2c-tunnel {
+			compatible = "google,cros-ec-i2c-tunnel";
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			google,remote-bus = <0>;
+
+			battery: sbs-battery@b {
+				compatible = "sbs,sbs-battery";
+				reg = <0xb>;
+				sbs,poll-retry-count = <1>;
+			};
+		};
+	}

drivers/i2c/busses/Kconfig +9 -0

index c94db1c..9a0a6cc 100644
@@ -993,6 +993,15 @@ config I2C_SIBYTE
 	help
 	  Supports the SiByte SOC on-chip I2C interfaces (2 channels).
 
+config I2C_CROS_EC_TUNNEL
+	tristate "ChromeOS EC tunnel I2C bus"
+	depends on MFD_CROS_EC
+	help
+	  If you say yes here you get an I2C bus that will tunnel i2c commands
+	  through to the other side of the ChromeOS EC to the i2c bus
+	  connected there. This will work whatever the interface used to
+	  talk to the EC (SPI, I2C or LPC).
+
 config SCx200_I2C
 	tristate "NatSemi SCx200 I2C using GPIO pins (DEPRECATED)"
 	depends on SCx200_GPIO

drivers/i2c/busses/Makefile +1 -0

index 18d18ff..e110ca9 100644
@@ -95,6 +95,7 @@ obj-$(CONFIG_I2C_VIPERBOARD)	+= i2c-viperboard.o
 # Other I2C/SMBus bus drivers
 obj-$(CONFIG_I2C_ACORN)		+= i2c-acorn.o
 obj-$(CONFIG_I2C_BCM_KONA)	+= i2c-bcm-kona.o
+obj-$(CONFIG_I2C_CROS_EC_TUNNEL)	+= i2c-cros-ec-tunnel.o
 obj-$(CONFIG_I2C_ELEKTOR)	+= i2c-elektor.o
 obj-$(CONFIG_I2C_PCA_ISA)	+= i2c-pca-isa.o
 obj-$(CONFIG_I2C_SIBYTE)	+= i2c-sibyte.o

drivers/i2c/busses/i2c-cros-ec-tunnel.c new file +304 -0

304 lines not on this page · raw message

drivers/mfd/cros_ec.c +5 -0

index c58ab96..61bc909 100644
@@ -90,6 +90,11 @@ static const struct mfd_cell cros_devs[] = {
 		.id = 1,
 		.of_compatible = "google,cros-ec-keyb",
 	},
+	{
+		.name = "cros-ec-i2c-tunnel",
+		.id = 2,
+		.of_compatible = "google,cros-ec-i2c-tunnel",
+	},
 };
 
 int cros_ec_register(struct cros_ec_device *ec_dev)

Signature · 1 line

1.9.1.423.g4596e3a

Lee Jones

Acked-by on v2 6/7

2014-04-23 12:37 UTC #12 lore raw

Re: [PATCH v2 6/7] i2c: ChromeOS EC tunnel driver

Quoted 46 lines · Doug Anderson

On ARM Chromebooks we have a few devices that are accessed by both the AP (the main "Application Processor") and the EC (the Embedded Controller). These are:
* The battery (sbs-battery).
* The power management unit tps65090.
On the original Samsung ARM Chromebook these devices were on an I2C bus that was shared between the AP and the EC and arbitrated using some extranal GPIOs (see i2c-arb-gpio-challenge).
The original arbitration scheme worked well enough but had some downsides:
* It was nonstandard (not using standard I2C multimaster)
* It only worked if the EC-AP communication was I2C
* It was relatively hard to debug problems (hard to tell if i2c issues were caused by the EC, the AP, or some device on the bus).
On the HP Chromebook 11 the design was changed to:
* The AP/EC comms were still i2c, but the battery/tps65090 were no longer on the bus used for AP/EC communication. The battery was exposed to the AP through a limited i2c tunnel and tps65090 was exposed to the AP through a custom Linux driver.
On the Samsung ARM Chromebook 2 the scheme is changed yet again, now:
* The AP/EC comms are now using SPI for faster speeds.
* The EC's i2c bus is exposed to the AP through a full i2c tunnel.
The upstream "tegra124-venice2" uses the same scheme as the Samsung ARM Chromebook 2, though it has a different set of components on the other side of the bus.
This driver supports the scheme used by the Samsung ARM Chromebook 2. Future patches to this driver could add support for the battery tunnel on the HP Chromebook 11 (and perhaps could even be used to access tps65090 on the HP Chromebook 11 instead of using a special driver, but I haven't researched that enough).
Signed-off-by Vincent Palatin <[email protected]>
Signed-off-by Simon Glass <[email protected]>
Signed-off-by Doug Anderson <[email protected]>
Tested-by Andrew Bresticker <[email protected]>
Tested-by Stephen Warren <[email protected]>
---
Changes in v2:
- Update tunnel binding as per swarren

Quoted 5 lines

 .../devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt |  39 +++
 drivers/i2c/busses/Kconfig                         |   9 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-cros-ec-tunnel.c            | 304 +++++++++++++++++++++
 drivers/mfd/cros_ec.c                              |   5 +

For the MFD changes:

Signature · 4 lines

Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Doug Anderson

author

2014-04-23 16:37 UTC #13 lore raw

Re: [PATCH v2 6/7] i2c: ChromeOS EC tunnel driver

Wolfram,

Quoted 46 lines · Doug Anderson

On ARM Chromebooks we have a few devices that are accessed by both the AP (the main "Application Processor") and the EC (the Embedded Controller). These are:
* The battery (sbs-battery).
* The power management unit tps65090.
On the original Samsung ARM Chromebook these devices were on an I2C bus that was shared between the AP and the EC and arbitrated using some extranal GPIOs (see i2c-arb-gpio-challenge).
The original arbitration scheme worked well enough but had some downsides:
* It was nonstandard (not using standard I2C multimaster)
* It only worked if the EC-AP communication was I2C
* It was relatively hard to debug problems (hard to tell if i2c issues were caused by the EC, the AP, or some device on the bus).
On the HP Chromebook 11 the design was changed to:
* The AP/EC comms were still i2c, but the battery/tps65090 were no longer on the bus used for AP/EC communication. The battery was exposed to the AP through a limited i2c tunnel and tps65090 was exposed to the AP through a custom Linux driver.
On the Samsung ARM Chromebook 2 the scheme is changed yet again, now:
* The AP/EC comms are now using SPI for faster speeds.
* The EC's i2c bus is exposed to the AP through a full i2c tunnel.
The upstream "tegra124-venice2" uses the same scheme as the Samsung ARM Chromebook 2, though it has a different set of components on the other side of the bus.
This driver supports the scheme used by the Samsung ARM Chromebook 2. Future patches to this driver could add support for the battery tunnel on the HP Chromebook 11 (and perhaps could even be used to access tps65090 on the HP Chromebook 11 instead of using a special driver, but I haven't researched that enough).
Signed-off-by Vincent Palatin <[email protected]>
Signed-off-by Simon Glass <[email protected]>
Signed-off-by Doug Anderson <[email protected]>
Tested-by Andrew Bresticker <[email protected]>
Tested-by Stephen Warren <[email protected]>
---
Changes in v2:
- Update tunnel binding as per swarren

Quoted 6 lines

 .../devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt |  39 +++
 drivers/i2c/busses/Kconfig                         |   9 +
 drivers/i2c/busses/Makefile                        |   1 +
 drivers/i2c/busses/i2c-cros-ec-tunnel.c            | 304 +++++++++++++++++++++
 drivers/mfd/cros_ec.c                              |   5 +
 5 files changed, 358 insertions(+)

Did you have any thoughts on this i2c driver? Please let me know and I'd be happy to spin with changes.

-Doug

Wolfram Sang

2014-04-29 12:33 UTC #14 lore raw

Re: [PATCH v2 6/7] i2c: ChromeOS EC tunnel driver

Hi,

just a basic review to keep things rolling...

Quoted 4 lines

On the original Samsung ARM Chromebook these devices were on an I2C bus that was shared between the AP and the EC and arbitrated using some extranal GPIOs (see i2c-arb-gpio-challenge).

Quoted 8 lines

The original arbitration scheme worked well enough but had some downsides:
* It was nonstandard (not using standard I2C multimaster)
* It only worked if the EC-AP communication was I2C
* It was relatively hard to debug problems (hard to tell if i2c issues were caused by the EC, the AP, or some device on the bus).
On the HP Chromebook 11 the design was changed to:

This paragraph would be a nice update for the gpio-arbitration docs.

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt

Quoted 1 line

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt +0 -0

The bindings should independently be sent to the devicetree list.

Quoted 34 lines

new file mode 100644
index 0000000..898f030

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt new file +29 -0

@@ -0,0 +1,39 @@
+I2C bus that tunnels through the ChromeOS EC (cros-ec)
+======================================================
+On some ChromeOS board designs we've got a connection to the EC (embedded
+controller) but no direct connection to some devices on the other side of
+the EC (like a battery and PMIC).  To get access to those devices we need
+to tunnel our i2c commands through the EC.
+
+The node for this device should be under a cros-ec node like google,cros-ec-spi
+or google,cros-ec-i2c.
+
+
+Required properties:
+- compatible: google,cros-ec-i2c-tunnel
+- google,remote-bus: The EC bus we'd like to talk to.
+
+Optional child nodes:
+- One node per I2C device connected to the tunnelled I2C bus.
+
+
+Example:
+	cros-ec@0 {
+		compatible = "google,cros-ec-spi";
+
+		...
+
+		i2c-tunnel {
+			compatible = "google,cros-ec-i2c-tunnel";
+			#address-cells = <1>;
+			#size-cells = <0>;

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt line 39

Quoted 10 lines

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt new file +10 -0

+
+			google,remote-bus = <0>;
+
+			battery: sbs-battery@b {
+				compatible = "sbs,sbs-battery";
+				reg = <0xb>;
+				sbs,poll-retry-count = <1>;
+			};
+		};
+	}

Can the tunnel have n busses? How to represent them then? I think the remote-bus property should go in favor of proper sub-nodes? Wouldn't it also be more generic to have the tunnel node seperate and reference the tunnel-mechanism (spi here) as a phandle?

Quoted 8 lines

+/**
+ * ec_i2c_construct_message - construct a message to go to the EC
+ *
+ * This function effectively stuffs the standard i2c_msg format of Linux into
+ * a format that the EC understands.
+ *
+ * @buf: The buffer to fill. Can pass NULL to count how many bytes the message
+ * would be.

I wonder about this NULL thing. That means the size is calculated twice. Why not make two functions instead, one fir size calc, one for setting up?

Quoted 8 lines

+/**
+ * ec_i2c_parse_response - Parse a response from the EC
+ *
+ * We'll take the EC's response and copy it back into msgs.
+ *
+ * @buf: The buffer to parse. Can pass NULL to count how many bytes we expect
+ * the response to be. Otherwise we assume that the right number of
+ * bytes are available.

Ditto.

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt

Quoted 3 lines

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt new file +3 -0
+	result = bus->ec->command_sendrecv(bus->ec, EC_CMD_I2C_PASSTHRU,
+					   request, request_len,
+					   response, response_len);

This function pointer should be checked against NULL in probe, I think.

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt

Quoted 10 lines

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt new file +10 -0

+static int ec_i2c_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
+	struct device *dev = &pdev->dev;
+	struct ec_i2c_device *bus = NULL;
+	u32 remote_bus;
+	int err;
+
+	dev_dbg(dev, "Probing\n");

Drop. Device core has it already.

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt

Quoted 5 lines

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt new file +5 -0
+
+	if (!np) {
+		dev_err(dev, "no device node\n");
+		return -ENOENT;
+	}

Can this happen?

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt

Quoted 4 lines

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt new file +4 -0
+
+	bus = devm_kzalloc(dev, sizeof(*bus), GFP_KERNEL);
+	if (bus == NULL) {
+		dev_err(dev, "cannot allocate bus device\n");

No need for error strings when allocating.

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt

Quoted 4 lines

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt new file +4 -0
+		return -ENOMEM;
+	}
+
+	dev_dbg(dev, "ChromeOS EC I2C tunnel adapter\n");

Drop. Device core debug has it, too.

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt

Quoted 9 lines

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt new file +9 -0

+
+	return err;
+}
+
+static int ec_i2c_remove(struct platform_device *dev)
+{
+	struct ec_i2c_device *bus = platform_get_drvdata(dev);
+
+	platform_set_drvdata(dev, NULL);

Not needed.

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt

Quoted 6 lines

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt new file +6 -0
+
+	i2c_del_adapter(&bus->adap);
+
+	return 0;
+}
+

Regards,

Wolfram

Doug Anderson

author

2014-04-30 17:44 UTC #15 lore raw

Re: [PATCH v2 6/7] i2c: ChromeOS EC tunnel driver

Wolfram,

Quoted 3 lines · Wolfram Sang

Hi,
just a basic review to keep things rolling...

Thanks for the review! It sounds like other changes are blocked on this one landing, so hopefully we can make some good progress! :)

Quoted 11 lines

Quoted 11 lines
On the original Samsung ARM Chromebook these devices were on an I2C bus that was shared between the AP and the EC and arbitrated using some extranal GPIOs (see i2c-arb-gpio-challenge).
The original arbitration scheme worked well enough but had some downsides:
* It was nonstandard (not using standard I2C multimaster)
* It only worked if the EC-AP communication was I2C
* It was relatively hard to debug problems (hard to tell if i2c issues were caused by the EC, the AP, or some device on the bus).

Quoted 3 lines

Quoted 1 line
On the HP Chromebook 11 the design was changed to:
This paragraph would be a nice update for the gpio-arbitration docs.

Done. <https://patchwork.kernel.org/patch/4088551/>

Quoted 43 lines

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt

Quoted 1 line

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt +0 -0

The bindings should independently be sent to the devicetree list.

Quoted 39 lines

new file mode 100644
index 0000000..898f030

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt new file +34 -0

@@ -0,0 +1,39 @@
+I2C bus that tunnels through the ChromeOS EC (cros-ec)
+======================================================
+On some ChromeOS board designs we've got a connection to the EC (embedded
+controller) but no direct connection to some devices on the other side of
+the EC (like a battery and PMIC).  To get access to those devices we need
+to tunnel our i2c commands through the EC.
+
+The node for this device should be under a cros-ec node like google,cros-ec-spi
+or google,cros-ec-i2c.
+
+
+Required properties:
+- compatible: google,cros-ec-i2c-tunnel
+- google,remote-bus: The EC bus we'd like to talk to.
+
+Optional child nodes:
+- One node per I2C device connected to the tunnelled I2C bus.
+
+
+Example:
+     cros-ec@0 {
+             compatible = "google,cros-ec-spi";
+
+             ...
+
+             i2c-tunnel {
+                     compatible = "google,cros-ec-i2c-tunnel";
+                     #address-cells = <1>;
+                     #size-cells = <0>;
+
+                     google,remote-bus = <0>;
+
+                     battery: sbs-battery@b {
+                             compatible = "sbs,sbs-battery";

Quoted 8 lines

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt line 39
Quoted 5 lines
Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt new file +5 -0
+                             reg = <0xb>;
+                             sbs,poll-retry-count = <1>;
+                     };
+             };
+     }
Can the tunnel have n busses? How to represent them then? I think the remote-bus property should go in favor of proper sub-nodes?

I suppose it could have n busses, though today's ECs don't.

I think that the binding could later be extended to support this with:

        cros-ec@0 {
                compatible = "google,cros-ec-spi";

                ...

                i2c-tunnel0 {
                        compatible = "google,cros-ec-i2c-tunnel";
                        #address-cells = <1>;
                        #size-cells = <0>;

                        google,remote-bus = <0>;

                        battery: sbs-battery@b {
                                compatible = "sbs,sbs-battery";
                                reg = <0xb>;
                                sbs,poll-retry-count = <1>;
                        };
                };

                i2c-tunnel1 {
                        compatible = "google,cros-ec-i2c-tunnel";
                        #address-cells = <1>;
                        #size-cells = <0>;

                        google,remote-bus = <1>;

                        battery: sbs-battery@b {
                                compatible = "sbs,sbs-battery";
                                reg = <0xb>;
                                sbs,poll-retry-count = <1>;
                        };
                };
       }

...but since there's no need now, it seems like we could just leave it with supporting 1 for now. If we extend it like above then old device trees will still work on newer kernels.

Note about "google,remote-bus" instead of "reg". The main
"google,cros-ec-spi" generally has non-addressed sub-nodes. It has no "#address-cells" or "#size-cells". The keyboard component, for instance, doesn't have an "address". You talk to it using the keyboard commands.

...I don't think it's generally possible to have some sub nodes that have addresses and other sub nodes that don't. I'm happy to be corrected, though.

Note: according to the device tree stable bindings theorem, I don't think I'm allowed to retroactively go back and say that the keyboard node needs a dummy "reg = <0>;" nor can I require that "google,cros-ec-spi" add #address-cells and #size-cells. Both device tree bindings have been in kernel since ~3.9.

Can you give me an example of what you mean by "proper sub nodes"?

Quoted 3 lines

Wouldn't it
also be more generic to have the tunnel node seperate and reference the tunnel-mechanism (spi here) as a phandle?

What makes it more generic? It's still a MFD and still only works atop the cros_ec, so it should be under the cros_ec device, shouldn't it?

I know use a phandle for mux, but from
<https://lkml.org/lkml/2013/2/13/517> it sounds like we were forced into it.

Quoted 9 lines

Quoted 8 lines
+/**
+ * ec_i2c_construct_message - construct a message to go to the EC
+ *
+ * This function effectively stuffs the standard i2c_msg format of Linux into
+ * a format that the EC understands.
+ *
+ * @buf: The buffer to fill. Can pass NULL to count how many bytes the message
+ * would be.

Quoted 3 lines

I wonder about this NULL thing. That means the size is calculated twice. Why not make two functions instead, one fir size calc, one for setting up?

Done.

Quoted 10 lines

Quoted 8 lines
+/**
+ * ec_i2c_parse_response - Parse a response from the EC
+ *
+ * We'll take the EC's response and copy it back into msgs.
+ *
+ * @buf: The buffer to parse. Can pass NULL to count how many bytes we expect
+ * the response to be. Otherwise we assume that the right number of
+ * bytes are available.
Ditto.

Done.

Quoted 6 lines

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt
Quoted 3 lines
Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt new file +3 -0
+     result = bus->ec->command_sendrecv(bus->ec, EC_CMD_I2C_PASSTHRU,
+                                        request, request_len,
+                                        response, response_len);
This function pointer should be checked against NULL in probe, I think.

Done.

Quoted 4 lines

Quoted 4 lines
Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt new file +4 -0
+static int ec_i2c_probe(struct platform_device *pdev)
+{
+     struct device_node *np = pdev->dev.of_node;
+     struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent);

Quoted 8 lines

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt
Quoted 6 lines
Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt new file +6 -0
+     struct device *dev = &pdev->dev;
+     struct ec_i2c_device *bus = NULL;
+     u32 remote_bus;
+     int err;
+
+     dev_dbg(dev, "Probing\n");
Drop. Device core has it already.

Done.

Quoted 7 lines

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt
Quoted 5 lines
Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt new file +5 -0
+
+     if (!np) {
+             dev_err(dev, "no device node\n");
+             return -ENOENT;
+     }
Can this happen?

Not that I know of. I assume you want me to remove this, so done.

Quoted 5 lines

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt
Quoted 3 lines
Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt new file +3 -0
+     bus = devm_kzalloc(dev, sizeof(*bus), GFP_KERNEL);
+     if (bus == NULL) {
+             dev_err(dev, "cannot allocate bus device\n");
No need for error strings when allocating.

Done.

Quoted 6 lines

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt
Quoted 4 lines
Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt new file +4 -0
+             return -ENOMEM;
+     }
+
+     dev_dbg(dev, "ChromeOS EC I2C tunnel adapter\n");
Drop. Device core debug has it, too.

Done.

Quoted 11 lines

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt
Quoted 9 lines
Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt new file +9 -0
+
+     return err;
+}
+
+static int ec_i2c_remove(struct platform_device *dev)
+{
+     struct ec_i2c_device *bus = platform_get_drvdata(dev);
+
+     platform_set_drvdata(dev, NULL);
Not needed.

Duh, thanks. Done.

Quoted 5 lines

Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt
Quoted 5 lines
Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt new file +5 -0
+     i2c_del_adapter(&bus->adap);
+
+     return 0;
+}
+

Sending up a v3 with fixes...

-Doug

Doug Anderson

author

2014-04-22 16:45 UTC #16 lore raw

PATCH v2 7/7 ARM: tegra: Add the EC i2c tunnel to tegra124-venice2

This adds the EC i2c tunnel (and devices under it) to the tegra124-venice2 device tree.

---
Changes in v2:
- Removed i2c20 alias for i2c tunnel

 arch/arm/boot/dts/tegra124-venice2.dts | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

arch/arm/boot/dts/tegra124-venice2.dts +26 -0

index c17283c..89cf776 100644
@@ -813,6 +813,32 @@
 
 			google,cros-ec-spi-msg-delay = <2000>;
 
+			i2c-tunnel {
+				compatible = "google,cros-ec-i2c-tunnel";
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				google,remote-bus = <0>;
+
+				charger: bq24735 at 9 {
+					compatible = "ti,bq24735";
+					reg = <0x9>;
+					interrupt-parent = <&gpio>;
+					interrupts = <TEGRA_GPIO(J, 0)
+							GPIO_ACTIVE_HIGH>;
+					ti,ac-detect-gpios = <&gpio
+							TEGRA_GPIO(J, 0)
+							GPIO_ACTIVE_HIGH>;
+				};
+
+				battery: sbs-battery at b {
+					compatible = "sbs,sbs-battery";
+					reg = <0xb>;
+					sbs,i2c-retry-count = <2>;
+					sbs,poll-retry-count = <1>;
+				};
+			};
+
 			cros-ec-keyb {
 				compatible = "google,cros-ec-keyb";
 				keypad,num-rows = <8>;

Signature · 1 line

1.9.1.423.g4596e3a

Lee Jones

2014-04-23 12:32 UTC #17 lore raw