Reland "smart_battery: add smbus error checking support"

This is a reland of daccb3adea9394116d7ab2c807e4a360cb5a93a1

Original change's description:
> smart_battery: add smbus error checking support
>
> Jacuzzi/Kodama has a unstable software controlled i2c bus, its data
> transmission may be interrupted by other higher priority tasks and
> causes device timeout.
>
> If timeout happens when ec is reading data, it has no knowledge about
> what's happening on slave, and keep receiving bad data (0xFF's) until
> end. The standard i2c/smbus error handling mechanism can not handle this
> case, so we need the error checking feature from smbus 1.1 to ensure our
> received data is correct.
>
> This CL adds the error checking (PEC) functions to i2c and smart battery
> module.
>
> BUG=b:138415463
> TEST=On kodama, enable CONFIG_CMD_I2C_STRESS_TEST,
>      no failure after 100k read/writes.
>      test code at CL:1865054
> BRANCH=master
>
> Change-Id: Ibb9ad3aa03d7690a08f59c617c2cd9c1b9cb0ff3
> Signed-off-by: Ting Shen <phoenixshen@google.com>
> Reviewed-on: http://crrev.com/c/1827138
> Reviewed-by: Denis Brockus <dbrockus@chromium.org>
> Tested-by: Ting Shen <phoenixshen@chromium.org>
> Commit-Queue: Ting Shen <phoenixshen@chromium.org>

BUG=b:138415463
TEST=in addition to the TESTs above, verified this CL boots on
     hatch(npcx chips), and reef_it8320(it83xx chips).
BRANCH=master

Change-Id: I67975eee677cfd6e383742d48103662372cac061
Signed-off-by: Ting Shen <phoenixshen@google.com>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/1913940
Commit-Queue: Ting Shen <phoenixshen@chromium.org>
Tested-by: Ting Shen <phoenixshen@chromium.org>
Reviewed-by: Denis Brockus <dbrockus@chromium.org>

5 files changed, 278 insertions, 44 deletions

diff --git a/common/i2c_master.c b/common/i2c_master.c
index c84bf02f9e..d89e20b920 100644
--- a/common/i2c_master.c
+++ b/common/i2c_master.c
@@ -9,6 +9,7 @@
 #include "clock.h"
 #include "charge_state.h"
 #include "console.h"
+#include "crc8.h"
 #include "host_command.h"
 #include "gpio.h"
 #include "i2c.h"
@@ -81,6 +82,7 @@ static int chip_i2c_xfer_with_notify(const int port,
 				     uint8_t *in, int in_size, int flags)
 {
 	int ret;
+	uint16_t addr_flags = slave_addr_flags;
 
 	if (IS_ENABLED(CONFIG_I2C_DEBUG))
 		i2c_trace_notify(port, slave_addr_flags, 0, out, out_size);
@@ -88,8 +90,14 @@ static int chip_i2c_xfer_with_notify(const int port,
 	if (IS_ENABLED(CONFIG_I2C_XFER_BOARD_CALLBACK))
 		i2c_start_xfer_notify(port, slave_addr_flags);
 
-	ret = chip_i2c_xfer(port, slave_addr_flags,
-			    out, out_size, in, in_size, flags);
+	if (IS_ENABLED(CONFIG_SMBUS_PEC))
+		/*
+		 * Since we've done PEC processing here,
+		 * remove the flag so it won't confuse chip driver.
+		 */
+		addr_flags &= ~I2C_FLAG_PEC;
+	ret = chip_i2c_xfer(port, addr_flags, out, out_size, in, in_size,
+			    flags);
 
 	if (IS_ENABLED(CONFIG_I2C_XFER_BOARD_CALLBACK))
 		i2c_end_xfer_notify(port, slave_addr_flags);
@@ -140,6 +148,8 @@ int i2c_xfer_unlocked(const int port,
 	int i;
 	int ret = EC_SUCCESS;
 
+	uint16_t addr_flags = slave_addr_flags & ~I2C_FLAG_PEC;
+
 	if (!i2c_port_is_locked(port)) {
 		CPUTS("Access I2C without lock!");
 		return EC_ERROR_INVAL;
@@ -147,11 +157,11 @@ int i2c_xfer_unlocked(const int port,
 
 	for (i = 0; i <= CONFIG_I2C_NACK_RETRY_COUNT; i++) {
 #ifdef CONFIG_I2C_XFER_LARGE_READ
-		ret = i2c_xfer_no_retry(port, slave_addr_flags,
+		ret = i2c_xfer_no_retry(port, addr_flags,
 					    out, out_size, in,
 					    in_size, flags);
 #else
-		ret = chip_i2c_xfer_with_notify(port, slave_addr_flags,
+		ret = chip_i2c_xfer_with_notify(port, addr_flags,
 						    out, out_size,
 						    in, in_size, flags);
 #endif /* CONFIG_I2C_XFER_LARGE_READ */
@@ -220,6 +230,84 @@ void i2c_prepare_sysjump(void)
 		mutex_lock(port_mutex + i);
 }
 
+/* i2c_readN with optional error checking */
+static int i2c_read(const int port, const uint16_t slave_addr_flags,
+			uint8_t reg, uint8_t *in, int in_size)
+{
+	if (!IS_ENABLED(CONFIG_SMBUS_PEC) && I2C_USE_PEC(slave_addr_flags))
+		return EC_ERROR_UNIMPLEMENTED;
+
+	if (IS_ENABLED(CONFIG_SMBUS_PEC) && I2C_USE_PEC(slave_addr_flags)) {
+		int i, rv;
+		/* addr_8bit = 7 bit addr_flags + 1 bit r/w */
+		uint8_t addr_8bit = I2C_GET_ADDR(slave_addr_flags) << 1;
+		uint8_t out[3] = {addr_8bit, reg, addr_8bit | 1};
+		uint8_t pec_local = 0, pec_remote;
+
+		i2c_lock(port, 1);
+		for (i = 0; i <= CONFIG_I2C_NACK_RETRY_COUNT; i++) {
+			rv = i2c_xfer_unlocked(port, slave_addr_flags, &reg, 1,
+					       in, in_size, I2C_XFER_START);
+			if (rv)
+				continue;
+
+			rv = i2c_xfer_unlocked(port, slave_addr_flags, NULL, 0,
+					       &pec_remote, 1, I2C_XFER_STOP);
+			if (rv)
+				continue;
+
+			pec_local = crc8(out, ARRAY_SIZE(out));
+			pec_local = crc8_arg(in, in_size, pec_local);
+			if (pec_local == pec_remote)
+				break;
+
+			rv = EC_ERROR_CRC;
+		}
+		i2c_lock(port, 0);
+
+		return rv;
+	}
+
+	return i2c_xfer(port, slave_addr_flags, &reg, 1, in, in_size);
+}
+
+/* i2c_writeN with optional error checking */
+static int i2c_write(const int port, const uint16_t slave_addr_flags,
+			 const uint8_t *out, int out_size)
+{
+	if (!IS_ENABLED(CONFIG_SMBUS_PEC) && I2C_USE_PEC(slave_addr_flags))
+		return EC_ERROR_UNIMPLEMENTED;
+
+	if (IS_ENABLED(CONFIG_SMBUS_PEC) && I2C_USE_PEC(slave_addr_flags)) {
+		int i, rv;
+		uint8_t addr_8bit = I2C_GET_ADDR(slave_addr_flags) << 1;
+		uint8_t pec;
+
+		pec = crc8(&addr_8bit, 1);
+		pec = crc8_arg(out, out_size, pec);
+
+		i2c_lock(port, 1);
+		for (i = 0; i <= CONFIG_I2C_NACK_RETRY_COUNT; i++) {
+			rv = i2c_xfer_unlocked(port, slave_addr_flags,
+					       out, out_size, NULL, 0,
+					       I2C_XFER_START);
+			if (rv)
+				continue;
+
+			rv = i2c_xfer_unlocked(port, slave_addr_flags,
+					       &pec, 1, NULL, 0,
+					       I2C_XFER_STOP);
+			if (!rv)
+				break;
+		}
+		i2c_lock(port, 0);
+
+		return rv;
+	}
+
+	return i2c_xfer(port, slave_addr_flags, out, out_size, NULL, 0);
+}
+
 int i2c_read32(const int port,
 	       const uint16_t slave_addr_flags,
 	       int offset, int *data)
@@ -229,8 +317,7 @@ int i2c_read32(const int port,
 
 	reg = offset & 0xff;
 	/* I2C read 32-bit word: transmit 8-bit offset, and read 32bits */
-	rv = i2c_xfer(port, slave_addr_flags,
-		      &reg, 1, buf, sizeof(uint32_t));
+	rv = i2c_read(port, slave_addr_flags, reg, buf, sizeof(uint32_t));
 
 	if (rv)
 		return rv;
@@ -265,8 +352,7 @@ int i2c_write32(const int port,
 		buf[4] = (data >> 24) & 0xff;
 	}
 
-	return i2c_xfer(port, slave_addr_flags,
-			buf, sizeof(uint32_t) + 1, NULL, 0);
+	return i2c_write(port, slave_addr_flags, buf, sizeof(uint32_t) + 1);
 }
 
 int i2c_read16(const int port,
@@ -278,8 +364,7 @@ int i2c_read16(const int port,
 
 	reg = offset & 0xff;
 	/* I2C read 16-bit word: transmit 8-bit offset, and read 16bits */
-	rv = i2c_xfer(port, slave_addr_flags,
-		      &reg, 1, buf, sizeof(uint16_t));
+	rv = i2c_read(port, slave_addr_flags, reg, buf, sizeof(uint16_t));
 
 	if (rv)
 		return rv;
@@ -308,8 +393,7 @@ int i2c_write16(const int port,
 		buf[2] = (data >> 8) & 0xff;
 	}
 
-	return i2c_xfer(port, slave_addr_flags,
-			buf, 1 + sizeof(uint16_t), NULL, 0);
+	return i2c_write(port, slave_addr_flags, buf, 1 + sizeof(uint16_t));
 }
 
 int i2c_read8(const int port,
@@ -322,7 +406,7 @@ int i2c_read8(const int port,
 
 	reg = offset;
 
-	rv = i2c_xfer(port, slave_addr_flags, &reg, 1, &buf, 1);
+	rv = i2c_read(port, slave_addr_flags, reg, &buf, sizeof(uint8_t));
 	if (!rv)
 		*data = buf;
 
@@ -338,7 +422,7 @@ int i2c_write8(const int port,
 	buf[0] = offset;
 	buf[1] = data;
 
-	return i2c_xfer(port, slave_addr_flags, buf, 2, 0, 0);
+	return i2c_write(port, slave_addr_flags, buf, sizeof(buf));
 }
 
 int i2c_read_offset16(const int port,
@@ -440,29 +524,84 @@ int i2c_read_string(const int port,
 		    const uint16_t slave_addr_flags,
 		    int offset, uint8_t *data, int len)
 {
-	int rv;
+	int i, rv;
 	uint8_t reg, block_length;
 
-	i2c_lock(port, 1);
+	if (!IS_ENABLED(CONFIG_SMBUS_PEC) && I2C_USE_PEC(slave_addr_flags))
+		return EC_ERROR_UNIMPLEMENTED;
 
 	reg = offset;
-	/*
-	 * Send device reg space offset, and read back block length.  Keep this
-	 * session open without a stop.
-	 */
-	rv = i2c_xfer_unlocked(port, slave_addr_flags,
-			       &reg, 1, &block_length, 1, I2C_XFER_START);
-	if (rv)
-		goto exit;
+	i2c_lock(port, 1);
 
-	if (len && block_length > (len - 1))
-		block_length = len - 1;
+	for (i = 0; i <= CONFIG_I2C_NACK_RETRY_COUNT; i++) {
+		int data_length;
 
-	rv = i2c_xfer_unlocked(port, slave_addr_flags,
-			       0, 0, data, block_length, I2C_XFER_STOP);
-	data[block_length] = 0;
+		/*
+		 * Send device reg space offset, and read back block length.
+		 * Keep this session open without a stop.
+		 */
+		rv = i2c_xfer_unlocked(port, slave_addr_flags,
+				       &reg, 1, &block_length, 1,
+				       I2C_XFER_START);
+		if (rv)
+			continue;
+
+		if (len && block_length > (len - 1))
+			data_length = len - 1;
+		else
+			data_length = block_length;
+
+		if (IS_ENABLED(CONFIG_SMBUS_PEC) &&
+				I2C_USE_PEC(slave_addr_flags)) {
+			uint8_t addr_8bit = I2C_GET_ADDR(slave_addr_flags) << 1;
+			uint8_t out[3] = {addr_8bit, reg, addr_8bit | 1};
+			uint8_t pec, pec_remote;
+
+			rv = i2c_xfer_unlocked(port, slave_addr_flags,
+					       0, 0, data, data_length, 0);
+			data[data_length] = 0;
+			if (rv)
+				continue;
+
+			pec = crc8(out, sizeof(out));
+			pec = crc8_arg(&block_length, 1, pec);
+			pec = crc8_arg(data, data_length, pec);
+
+			/* read all remaining bytes */
+			block_length -= data_length;
+			while (block_length) {
+				uint8_t byte;
+
+				rv = i2c_xfer_unlocked(port, slave_addr_flags,
+						       NULL, 0, &byte, 1, 0);
+				if (rv)
+					break;
+				pec = crc8_arg(&byte, 1, pec);
+				--block_length;
+			}
+			if (rv)
+				continue;
+
+			rv = i2c_xfer_unlocked(port, slave_addr_flags, NULL, 0,
+					       &pec_remote, 1, I2C_XFER_STOP);
+			if (rv)
+				continue;
+
+			if (pec != pec_remote)
+				rv = EC_ERROR_CRC;
+		} else {
+			rv = i2c_xfer_unlocked(port, slave_addr_flags,
+					       0, 0, data, data_length,
+					       I2C_XFER_STOP);
+			data[data_length] = 0;
+			if (rv)
+				continue;
+		}
+
+		/* execution reaches here implies rv=0, so we can exit now */
+		break;
+	}
 
-exit:
 	i2c_lock(port, 0);
 	return rv;
 }
@@ -482,19 +621,52 @@ int i2c_write_block(const int port,
 		    const uint16_t slave_addr_flags,
 		    int offset, const uint8_t *data, int len)
 {
-	int rv;
-	uint8_t reg_address = offset;
+	int i, rv;
+	uint8_t reg_address = offset, pec = 0;
+
+	if (!IS_ENABLED(CONFIG_SMBUS_PEC) && I2C_USE_PEC(slave_addr_flags))
+		return EC_ERROR_UNIMPLEMENTED;
+
+	if (IS_ENABLED(CONFIG_SMBUS_PEC) && I2C_USE_PEC(slave_addr_flags)) {
+		uint8_t addr_8bit = I2C_GET_ADDR(slave_addr_flags) << 1;
+
+		pec = crc8(&addr_8bit, sizeof(uint8_t));
+		pec = crc8_arg(data, len, pec);
+	}
 
 	/*
 	 * Split into two transactions to avoid the stack space consumption of
 	 * appending the destination address with the data array.
 	 */
 	i2c_lock(port, 1);
-	rv = i2c_xfer_unlocked(port, slave_addr_flags,
-			       &reg_address, 1, NULL, 0, I2C_XFER_START);
-	if (!rv) {
+	for (i = 0; i <= CONFIG_I2C_NACK_RETRY_COUNT; i++) {
 		rv = i2c_xfer_unlocked(port, slave_addr_flags,
-				       data, len, NULL, 0, I2C_XFER_STOP);
+				       &reg_address, 1, NULL, 0,
+				       I2C_XFER_START);
+		if (rv)
+			continue;
+
+		if (I2C_USE_PEC(slave_addr_flags)) {
+			rv = i2c_xfer_unlocked(port, slave_addr_flags,
+					       data, len, NULL, 0, 0);
+			if (rv)
+				continue;
+
+			rv = i2c_xfer_unlocked(port, slave_addr_flags,
+					       &pec, sizeof(uint8_t), NULL, 0,
+					       I2C_XFER_STOP);
+			if (rv)
+				continue;
+		} else {
+			rv = i2c_xfer_unlocked(port, slave_addr_flags,
+					       data, len, NULL, 0,
+					       I2C_XFER_STOP);
+			if (rv)
+				continue;
+		}
+
+		/* execution reaches here implies rv=0, so we can exit now */
+		break;
 	}
 	i2c_lock(port, 0);
 
diff --git a/driver/battery/smart.c b/driver/battery/smart.c
index a2527db42f..14106664cb 100644
--- a/driver/battery/smart.c
+++ b/driver/battery/smart.c
@@ -22,8 +22,32 @@
 static int fake_state_of_charge = -1;
 static int fake_temperature = -1;
 
+static int battery_supports_pec(void)
+{
+	static int supports_pec = -1;
+
+	if (!IS_ENABLED(CONFIG_SMBUS_PEC))
+		return 0;
+
+	if (supports_pec < 0) {
+		int spec_info;
+		int rv = i2c_read16(I2C_PORT_BATTERY, BATTERY_ADDR_FLAGS,
+				    SB_SPECIFICATION_INFO, &spec_info);
+		/* failed, assuming not support and try again later */
+		if (rv)
+			return 0;
+
+		supports_pec = (BATTERY_SPEC_VERSION(spec_info) ==
+				BATTERY_SPEC_VER_1_1_WITH_PEC);
+		CPRINTS("battery supports pec: %d", supports_pec);
+	}
+	return supports_pec;
+}
+
 test_mockable int sb_read(int cmd, int *param)
 {
+	uint16_t addr_flags = BATTERY_ADDR_FLAGS;
+
 #ifdef CONFIG_BATTERY_CUT_OFF
 	/*
 	 * Some batteries would wake up after cut-off if we talk to it.
@@ -31,13 +55,16 @@ test_mockable int sb_read(int cmd, int *param)
 	if (battery_is_cut_off())
 		return EC_RES_ACCESS_DENIED;
 #endif
+	if (battery_supports_pec())
+		addr_flags |= I2C_FLAG_PEC;
 
-	return i2c_read16(I2C_PORT_BATTERY, BATTERY_ADDR_FLAGS,
-			  cmd, param);
+	return i2c_read16(I2C_PORT_BATTERY, addr_flags, cmd, param);
 }
 
 test_mockable int sb_write(int cmd, int param)
 {
+	uint16_t addr_flags = BATTERY_ADDR_FLAGS;
+
 #ifdef CONFIG_BATTERY_CUT_OFF
 	/*
 	 * Some batteries would wake up after cut-off if we talk to it.
@@ -45,13 +72,16 @@ test_mockable int sb_write(int cmd, int param)
 	if (battery_is_cut_off())
 		return EC_RES_ACCESS_DENIED;
 #endif
+	if (battery_supports_pec())
+		addr_flags |= I2C_FLAG_PEC;
 
-	return i2c_write16(I2C_PORT_BATTERY, BATTERY_ADDR_FLAGS,
-			   cmd, param);
+	return i2c_write16(I2C_PORT_BATTERY, addr_flags, cmd, param);
 }
 
 int sb_read_string(int offset, uint8_t *data, int len)
 {
+	uint16_t addr_flags = BATTERY_ADDR_FLAGS;
+
 #ifdef CONFIG_BATTERY_CUT_OFF
 	/*
 	 * Some batteries would wake up after cut-off if we talk to it.
@@ -59,9 +89,10 @@ int sb_read_string(int offset, uint8_t *data, int len)
 	if (battery_is_cut_off())
 		return EC_RES_ACCESS_DENIED;
 #endif
+	if (battery_supports_pec())
+		addr_flags |= I2C_FLAG_PEC;
 
-	return i2c_read_string(I2C_PORT_BATTERY, BATTERY_ADDR_FLAGS,
-			       offset, data, len);
+	return i2c_read_string(I2C_PORT_BATTERY, addr_flags, offset, data, len);
 }
 
 int sb_read_mfgacc(int cmd, int block, uint8_t *data, int len)
@@ -96,6 +127,8 @@ int sb_read_mfgacc(int cmd, int block, uint8_t *data, int len)
 
 int sb_write_block(int reg, const uint8_t *val, int len)
 {
+	uint16_t addr_flags = BATTERY_ADDR_FLAGS;
+
 #ifdef CONFIG_BATTERY_CUT_OFF
 	/*
 	 * Some batteries would wake up after cut-off if we talk to it.
@@ -104,9 +137,11 @@ int sb_write_block(int reg, const uint8_t *val, int len)
 		return EC_RES_ACCESS_DENIED;
 #endif
 
+	if (battery_supports_pec())
+		addr_flags |= I2C_FLAG_PEC;
+
 	/* TODO: implement smbus_write_block. */
-	return i2c_write_block(I2C_PORT_BATTERY, BATTERY_ADDR_FLAGS,
-			       reg, val, len);
+	return i2c_write_block(I2C_PORT_BATTERY, addr_flags, reg, val, len);
 }
 
 int battery_get_mode(int *mode)
diff --git a/include/battery_smart.h b/include/battery_smart.h
index 1c088a5e27..9295cb884a 100644
--- a/include/battery_smart.h
+++ b/include/battery_smart.h
@@ -91,6 +91,13 @@
 #define STATUS_TERMINATE_CHARGE_ALARM   BIT(14)
 #define STATUS_OVERCHARGED_ALARM        BIT(15)
 
+/* Battery Spec Info */
+#define BATTERY_SPEC_VERSION(INFO)      ((INFO >> 4) & 0xF)
+/* Smart battery version info */
+#define BATTERY_SPEC_VER_1_0            1
+#define BATTERY_SPEC_VER_1_1            2
+#define BATTERY_SPEC_VER_1_1_WITH_PEC   3
+
 /* Charger alarm warning */
 #define ALARM_OVER_CHARGED              0x8000
 #define ALARM_TERMINATE_CHARGE          0x4000
diff --git a/include/config.h b/include/config.h
index d95d938ce4..d9b8afcd8b 100644
--- a/include/config.h
+++ b/include/config.h
@@ -2310,6 +2310,20 @@
  */
 #undef CONFIG_I2C_MULTI_PORT_CONTROLLER
 
+/*
+ * Packet error checking support for SMBus.
+ *
+ * If defined, adds error checking support for i2c_readN, i2c_writeN,
+ * i2c_read_string and i2c_write_block. Where
+ * - write operation appends an error checking byte at end of transfer, and
+ * - read operatoin verifies the correctness of error checking byte from the
+ * slave.
+ * Set I2C_FLAG on addr_flags parameter to use this feature.
+ *
+ * This option also enables error checking function on smart batteries.
+ */
+#undef CONFIG_SMBUS_PEC
+
 /*****************************************************************************/
 /* IPI configuration.  Support mt_scp only for now. */
 
@@ -5077,4 +5091,8 @@
 #define CONFIG_SHA256
 #endif
 
+#ifdef CONFIG_SMBUS_PEC
+#define CONFIG_CRC8
+#endif
+
 #endif  /* __CROS_EC_CONFIG_H */
diff --git a/include/i2c.h b/include/i2c.h
index 0d68b5bc90..4be5472a11 100644
--- a/include/i2c.h
+++ b/include/i2c.h
@@ -35,11 +35,13 @@
  * address that is pertinent to its use.
  */
 #define I2C_ADDR_MASK		0x03FF
+#define I2C_FLAG_PEC		BIT(13)
 #define I2C_FLAG_BIG_ENDIAN	BIT(14)
 /* BIT(15) SPI_FLAG - used in motion_sense to overload address */
 #define I2C_FLAG_ADDR_IS_SPI	BIT(15)
 
 #define I2C_GET_ADDR(addr_flags)	((addr_flags) & I2C_ADDR_MASK)
+#define I2C_USE_PEC(addr_flags)		((addr_flags) & I2C_FLAG_PEC)
 #define I2C_IS_BIG_ENDIAN(addr_flags)	((addr_flags) & I2C_FLAG_BIG_ENDIAN)
 
 /*