Reinvent I2C ops

Do not use the global platform_i2c_transfer() function that can only be
implemented by a single driver. Instead, make a `struct device` aware
transfer() function the only interface function for I2C controller dri-
vers to implement.

To not force the slave device drivers to be implemented either above
generic I2C or specialized SMBus operations, we support SMBus control-
lers in the slave device interface too.

We start with four simple slave functions: i2c_readb(), i2c_writeb(),
i2c_readb_at() and i2c_writeb_at(). They are all compatible to respec-
tive SMBus functions. But we keep aliases because it would be weird to
force e.g. an I2C EEPROM driver to call smbus_read_byte().

Change-Id: I98386f91bf4799ba3df84ec8bc0f64edd4142818
Signed-off-by: Nico Huber <nico.huber@secunet.com>
Reviewed-on: https://review.coreboot.org/20846
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Aaron Durbin <adurbin@chromium.org>
Reviewed-by: Kyösti Mälkki <kyosti.malkki@gmail.com>

src/device/Makefile.inc

@@ -37,3 +37,4 @@ bootblock-y += i2c.c
 verstage-y += i2c.c
 romstage-y += i2c.c
 ramstage-y += i2c.c
+ramstage-y += i2c_bus.c

src/device/i2c.c

@@ -13,92 +13,9 @@
  * GNU General Public License for more details.
  */
 
-#include <device/device.h>
-#include <device/i2c_bus.h>
 #include <device/i2c_simple.h>
 #include <stdint.h>
 
-#if ENV_RAMSTAGE
-/* Return I2C operations for a bus */
-static inline const struct i2c_bus_operations *ops_i2c_bus(struct bus *bus)
-{
-	if (bus && bus->dev && bus->dev->ops)
-		return bus->dev->ops->ops_i2c_bus;
-	return NULL;
-}
-
-int i2c_dev_find_bus(struct device *dev)
-{
-	const struct i2c_bus_operations *ops;
-	struct bus *pbus;
-
-	if (!dev)
-		return -1;
-
-	/* Locate parent bus with I2C controller ops */
-	pbus = dev->bus;
-	while (pbus && pbus->dev && !ops_i2c_bus(pbus))
-		if (pbus->dev->bus != pbus)
-			pbus = pbus->dev->bus;
-
-	/* Check if this I2C controller ops implements dev_to_bus() */
-	ops = ops_i2c_bus(pbus);
-	if (!ops || !ops->dev_to_bus)
-		return -1;
-
-	/* Use controller ops to determine the bus number */
-	return ops->dev_to_bus(pbus->dev);
-}
-
-int i2c_dev_transfer(struct device *dev, struct i2c_msg *segments, int count)
-{
-	int bus = i2c_dev_find_bus(dev);
-	if (bus < 0)
-		return -1;
-	return i2c_transfer(bus, segments, count);
-}
-
-int i2c_dev_readb(struct device *dev, uint8_t reg, uint8_t *data)
-{
-	int bus = i2c_dev_find_bus(dev);
-	if (bus < 0)
-		return -1;
-	return i2c_readb(bus, dev->path.i2c.device, reg, data);
-}
-
-int i2c_dev_writeb(struct device *dev, uint8_t reg, uint8_t data)
-{
-	int bus = i2c_dev_find_bus(dev);
-	if (bus < 0)
-		return -1;
-	return i2c_writeb(bus, dev->path.i2c.device, reg, data);
-}
-
-int i2c_dev_read_bytes(struct device *dev, uint8_t reg, uint8_t *data, int len)
-{
-	int bus = i2c_dev_find_bus(dev);
-	if (bus < 0)
-		return -1;
-	return i2c_read_bytes(bus, dev->path.i2c.device, reg, data, len);
-}
-
-int i2c_dev_read_raw(struct device *dev, uint8_t *data, int len)
-{
-	int bus = i2c_dev_find_bus(dev);
-	if (bus < 0)
-		return -1;
-	return i2c_read_raw(bus, dev->path.i2c.device, data, len);
-}
-
-int i2c_dev_write_raw(struct device *dev, uint8_t *data, int len)
-{
-	int bus = i2c_dev_find_bus(dev);
-	if (bus < 0)
-		return -1;
-	return i2c_write_raw(bus, dev->path.i2c.device, data, len);
-}
-#endif
-
 int i2c_read_field(unsigned bus, uint8_t chip, uint8_t reg, uint8_t *data,
 		   uint8_t mask, uint8_t shift)
 {

src/device/i2c_bus.c

@@ -0,0 +1,161 @@
+/*
+ * This file is part of the coreboot project.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <stdlib.h>
+#include <stdint.h>
+#include <console/console.h>
+#include <device/smbus.h>
+#include <device/i2c_bus.h>
+
+struct bus *i2c_link(struct device *const dev)
+{
+	if (!dev || !dev->bus)
+		return NULL;
+
+	struct bus *link = dev->bus;
+	while (link) {
+		struct device *const parent = link->dev;
+
+		if (parent && parent->ops &&
+		    (parent->ops->ops_i2c_bus || parent->ops->ops_smbus_bus))
+			break;
+
+		if (parent && parent->bus)
+			link = parent->bus;
+		else
+			link = NULL;
+	}
+
+	if (!link) {
+		printk(BIOS_ALERT, "%s Cannot find I2C or SMBus bus operations",
+		       dev_path(dev));
+	}
+
+	return link;
+}
+
+int i2c_readb(struct device *const dev)
+{
+	struct device *const busdev = i2c_busdev(dev);
+	if (!busdev)
+		return -1;
+
+	if (busdev->ops->ops_i2c_bus) {
+		uint8_t val;
+		const struct i2c_msg msg = {
+			.flags	= I2C_M_RD,
+			.slave	= dev->path.i2c.device,
+			.buf	= &val,
+			.len	= sizeof(val),
+		};
+
+		const int ret = busdev->ops->ops_i2c_bus->
+			transfer(busdev, &msg, 1);
+		if (ret)
+			return ret;
+		else
+			return val;
+	} else if (busdev->ops->ops_smbus_bus->recv_byte) {
+		return busdev->ops->ops_smbus_bus->recv_byte(dev);
+	} else {
+		printk(BIOS_ERR, "%s Missing ops_smbus_bus->recv_byte",
+		       dev_path(busdev));
+		return -1;
+	}
+}
+
+int i2c_writeb(struct device *const dev, uint8_t val)
+{
+	struct device *const busdev = i2c_busdev(dev);
+	if (!busdev)
+		return -1;
+
+	if (busdev->ops->ops_i2c_bus) {
+		const struct i2c_msg msg = {
+			.flags	= 0,
+			.slave	= dev->path.i2c.device,
+			.buf	= &val,
+			.len	= sizeof(val),
+		};
+		return busdev->ops->ops_i2c_bus->transfer(busdev, &msg, 1);
+	} else if (busdev->ops->ops_smbus_bus->send_byte) {
+		return busdev->ops->ops_smbus_bus->send_byte(dev, val);
+	} else {
+		printk(BIOS_ERR, "%s Missing ops_smbus_bus->send_byte",
+		       dev_path(busdev));
+		return -1;
+	}
+}
+
+int i2c_readb_at(struct device *const dev, uint8_t off)
+{
+	struct device *const busdev = i2c_busdev(dev);
+	if (!busdev)
+		return -1;
+
+	if (busdev->ops->ops_i2c_bus) {
+		uint8_t val;
+		const struct i2c_msg msg[] = {
+			{
+				.flags	= 0,
+				.slave	= dev->path.i2c.device,
+				.buf	= &off,
+				.len	= sizeof(off),
+			},
+			{
+				.flags	= I2C_M_RD,
+				.slave	= dev->path.i2c.device,
+				.buf	= &val,
+				.len	= sizeof(val),
+			},
+		};
+
+		const int ret = busdev->ops->ops_i2c_bus->
+			transfer(busdev, msg, ARRAY_SIZE(msg));
+		if (ret)
+			return ret;
+		else
+			return val;
+	} else if (busdev->ops->ops_smbus_bus->read_byte) {
+		return busdev->ops->ops_smbus_bus->read_byte(dev, off);
+	} else {
+		printk(BIOS_ERR, "%s Missing ops_smbus_bus->read_byte",
+		       dev_path(busdev));
+		return -1;
+	}
+}
+
+int i2c_writeb_at(struct device *const dev,
+		  const uint8_t off, const uint8_t val)
+{
+	struct device *const busdev = i2c_busdev(dev);
+	if (!busdev)
+		return -1;
+
+	if (busdev->ops->ops_i2c_bus) {
+		uint8_t buf[] = { off, val };
+		const struct i2c_msg msg = {
+			.flags	= 0,
+			.slave	= dev->path.i2c.device,
+			.buf	= buf,
+			.len	= sizeof(buf),
+		};
+		return busdev->ops->ops_i2c_bus->transfer(busdev, &msg, 1);
+	} else if (busdev->ops->ops_smbus_bus->write_byte) {
+		return busdev->ops->ops_smbus_bus->write_byte(dev, off, val);
+	} else {
+		printk(BIOS_ERR, "%s Missing ops_smbus_bus->write_byte",
+		       dev_path(busdev));
+		return -1;
+	}
+}

src/include/device/i2c_bus.h

@@ -1,8 +1,6 @@
 /*
  * This file is part of the coreboot project.
  *
- * Copyright (C) 2014 Google, Inc.
- *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
  * the Free Software Foundation; version 2 of the License.
@@ -16,29 +14,79 @@
 #ifndef _DEVICE_I2C_BUS_H_
 #define _DEVICE_I2C_BUS_H_
 
+#include <stdlib.h>
 #include <stdint.h>
 #include <device/i2c.h>
 #include <device/device.h>
 
 /* I2C bus operation for ramstage drivers */
 struct i2c_bus_operations {
-	/*
+	int (*transfer)(struct device *, const struct i2c_msg *, size_t count);
-	 * This is an SOC specific method that can be provided to translate the
-	 * 'struct device' for an I2C controller into a unique I2C bus number.
-	 * Returns -1 if the bus number for this device cannot be determined.
-	 */
-	int (*dev_to_bus)(struct device *dev);
 };
 
-/* Return I2C bus number for provided device, -1 if not found */
+/*
-int i2c_dev_find_bus(struct device *dev);
+ * Returns the first upstream facing link whose bus implements either
+ * `i2c_bus_operations` *or* `smbus_bus_operations`.
+ *
+ * If not NULL, guarantees that `->dev`, `->dev->ops` and either
+ * `->dev->ops->ops_i2c_bus` or `->dev->ops->ops_smbus_bus` are
+ * not NULL.
+ */
+struct bus *i2c_link(struct device *);
 
-/* Variants of I2C helper functions that take a device instead of bus number */
+/*
-int i2c_dev_transfer(struct device *dev, struct i2c_msg *segments, int count);
+ * Shorthand for `i2c_link(dev)->dev`.
-int i2c_dev_readb(struct device *dev, uint8_t reg, uint8_t *data);
+ *
-int i2c_dev_writeb(struct device *dev, uint8_t reg, uint8_t data);
+ * Returns NULL if i2c_link(dev) returns NULL.
-int i2c_dev_read_bytes(struct device *dev, uint8_t reg, uint8_t *data, int len);
+ */
-int i2c_dev_read_raw(struct device *dev, uint8_t *data, int len);
+static inline struct device *i2c_busdev(struct device *dev)
-int i2c_dev_write_raw(struct device *dev, uint8_t *data, int len);
+{
+	struct bus *const link = i2c_link(dev);
+	return link ? link->dev : NULL;
+}
+
+/*
+ * Slave driver interface functions. These will look for the next
+ * `i2c_bus_operations` *or* `smbus_bus_operations` and perform the
+ * respective transfers.
+ *
+ * The interface is limited to what current slave drivers demand.
+ * Extend as required.
+ *
+ * All functions return a negative `enum cb_err` value on error.
+ * Either CB_ERR, CB_ERR_ARG or any CB_I2C_* (cf. include/types.h).
+ */
+
+/*
+ * Reads one byte.
+ * Compatible to smbus_recv_byte().
+ *
+ * Returns the read byte on success, negative `enum cb_err` value on error.
+ */
+int i2c_readb(struct device *);
+
+/*
+ * Writes the byte `val`.
+ * Compatible to smbus_send_byte().
+ *
+ * Returns 0 on success, negative `enum cb_err` value on error.
+ */
+int i2c_writeb(struct device *, uint8_t val);
+
+/*
+ * Sends the register offset `off` and reads one byte.
+ * Compatible to smbus_read_byte().
+ *
+ * Returns the read byte on success, negative `enum cb_err` value on error.
+ */
+int i2c_readb_at(struct device *, uint8_t off);
+
+/*
+ * Sends the register offset `off` followed by the byte `val`.
+ * Compatible to smbus_write_byte().
+ *
+ * Returns 0 on success, negative `enum cb_err` value on error.
+ */
+int i2c_writeb_at(struct device *, uint8_t off, uint8_t val);
 
 #endif	/* _DEVICE_I2C_BUS_H_ */

src/include/device/smbus_def.h

@@ -1,10 +1,12 @@
 #ifndef DEVICE_SMBUS_DEF_H
 #define DEVICE_SMBUS_DEF_H
 
+#include <types.h>
+
 /* Error results are negative success is >= 0 */
-#define SMBUS_ERROR -1
+#define SMBUS_ERROR			CB_ERR
-#define SMBUS_WAIT_UNTIL_READY_TIMEOUT -2
+#define SMBUS_WAIT_UNTIL_READY_TIMEOUT	CB_I2C_BUSY
-#define SMBUS_WAIT_UNTIL_DONE_TIMEOUT  -3
+#define SMBUS_WAIT_UNTIL_DONE_TIMEOUT	CB_I2C_TIMEOUT
-#define SMBUS_WAIT_UNTIL_ACTIVE_TIMEOUT -4
+#define SMBUS_WAIT_UNTIL_ACTIVE_TIMEOUT	CB_I2C_NO_DEVICE
 
 #endif /* DEVICE_SMBUS_DEF_H */

src/include/types.h

@@ -47,7 +47,14 @@ enum cb_err {
 
 	/* Keyboard test failures */
 	CB_KBD_CONTROLLER_FAILURE = -200,
-	CB_KBD_INTERFACE_FAILURE = -201
+	CB_KBD_INTERFACE_FAILURE = -201,
+
+	/* I2C controller failures */
+	CB_I2C_NO_DEVICE	= -300,	/**< Device is not responding */
+	CB_I2C_BUSY		= -301,	/**< Device tells it's busy */
+	CB_I2C_PROTOCOL_ERROR	= -302,	/**< Data lost or spurious slave
+					     device response, try again? */
+	CB_I2C_TIMEOUT		= -303, /**< Transmission timed out */
 };
 
 #endif /* __TYPES_H */

src/soc/intel/common/block/i2c/i2c.c

@@ -150,8 +150,14 @@ static void lpss_i2c_acpi_fill_ssdt(struct device *dev)
 	acpigen_pop_len();
 }
 
-static struct i2c_bus_operations i2c_bus_ops = {
+static int lpss_i2c_dev_transfer(struct device *dev,
-	.dev_to_bus			= &lpss_i2c_dev_to_bus,
+				 const struct i2c_msg *msg, size_t count)
+{
+	return lpss_i2c_transfer(lpss_i2c_dev_to_bus(dev), msg, count);
+}
+
+static const struct i2c_bus_operations i2c_bus_ops = {
+	.transfer = lpss_i2c_dev_transfer,
 };
 
 static struct device_operations i2c_dev_ops = {

src/soc/intel/common/block/i2c/lpss_i2c.c

@@ -258,7 +258,7 @@ static int lpss_i2c_wait_for_bus_idle(struct lpss_i2c_regs *regs)
 
 /* Transfer one byte of one segment, sending stop bit if requested */
 static int lpss_i2c_transfer_byte(struct lpss_i2c_regs *regs,
-				  struct i2c_msg *segment,
+				  const struct i2c_msg *segment,
 				  size_t byte, int send_stop)
 {
 	struct stopwatch sw;
@@ -297,16 +297,15 @@ static int lpss_i2c_transfer_byte(struct lpss_i2c_regs *regs,
 	return 0;
 }
 
-/* Global I2C bus handler, defined in include/i2c.h */
+int lpss_i2c_transfer(unsigned int bus,
-int platform_i2c_transfer(unsigned int bus, struct i2c_msg *segments,
+		      const struct i2c_msg *segments, size_t count)
-			  int count)
 {
 	struct stopwatch sw;
 	struct lpss_i2c_regs *regs;
 	size_t byte;
 	int ret = -1;
 
-	if (count <= 0 || !segments)
+	if (count == 0 || !segments)
 		return -1;
 
 	regs = (struct lpss_i2c_regs *)lpss_i2c_base_address(bus);
@@ -396,6 +395,12 @@ out:
 	return ret;
 }
 
+/* Global I2C bus handler, defined in include/device/i2c_simple.h */
+int platform_i2c_transfer(unsigned int bus, struct i2c_msg *msg, int count)
+{
+	return lpss_i2c_transfer(bus, msg, count < 0 ? 0 : count);
+}
+
 static int lpss_i2c_set_speed_config(unsigned int bus,
 				const struct lpss_i2c_speed_config *config)
 {

src/soc/intel/common/block/i2c/lpss_i2c.h

@@ -89,3 +89,11 @@ int lpss_i2c_gen_speed_config(struct lpss_i2c_regs *regs,
 					enum i2c_speed speed,
 					const struct lpss_i2c_bus_config *bcfg,
 					struct lpss_i2c_speed_config *config);
+
+/*
+ * Process given I2C segments in a single transfer
+ * Return value:
+ * -1 = failure
+ *  0 = success
+ */
+int lpss_i2c_transfer(unsigned int bus, const struct i2c_msg *, size_t count);