From ab238235f8f45cb1ba3e26893199c01f4792a8ff Mon Sep 17 00:00:00 2001 From: Chun-Ta Lin Date: Tue, 28 Nov 2017 15:16:22 +0800 Subject: [PATCH] i2c: support large reading in i2c_xfer() There might be more than one place that will use very similar codes in CL:542716 (usb_i2c.c). To avoid unnecessary duplication of code, we fold the logic into i2c_xfer(). With config enabled (illustrated in this CL), i2c_xfer() will support large reading. An early prototype of potential usage is demostrated in CL:781300. BRANCH=none TEST=For CONFIG_I2C_XFER_LARGE_READ: With proprietary software on slave: ./touchpad_updater -d TEST=For usb_i2c regression: ./touchpad_updater still works (where the reading is less than 255) TEST=For generic EC regression test: On poppy EC (With CONFIG_I2C_XFER_LARGE_READ defined) (1) Prevent override of update /usr/share/vboot/bin/set_gbb_flags.sh 0xa39 (2) flashrom -p ec -w ec_binary_contains_this_CL.bin (3) ectool reboot_ec cold (4) "ectool version" verified that both RO/RW/Build info is local. (5) Test with a type-C DP dongle on DELL 2408WFP. (6) Plug-in type-C charger and external display still works. (7) Confirmed that battery is charging. (7) "ectool battery" reads same data (for fixed field) as before. (8) "ectool usbpdpower", confirmed charger info displayed as before. (9) "ectool usbpd 0", confirmed content exactly as before. (10) For Gyro. Verify under Arc++ Using App: com.gamma.bubblelevel, works the same as before. (11) For Accelerometers. Verify under Arc++ Using App: com.innoventions.sensorkinetics, small movement's charts looks as expected. TEST=For generic EC regression test: On Caroline EC (Without CONFIG_I2C_XFER_LARGE_READ defined). Details in CL:810332 BUG=b:63993891 Change-Id: I654868945fa535e784800177d54eb2d9803f5249 Signed-off-by: Chun-Ta Lin Reviewed-on: https://chromium-review.googlesource.com/788479 Commit-Ready: Chun-ta Lin Tested-by: Chun-ta Lin Reviewed-by: Vincent Palatin --- board/hammer/board.h | 2 ++ common/i2c_master.c | 33 ++++++++++++++++++++++ common/usb_i2c.c | 34 +++++------------------ extra/touchpad_updater/touchpad_updater.c | 15 ++++++---- include/config.h | 12 ++++++++ include/i2c.h | 4 ++- 6 files changed, 66 insertions(+), 34 deletions(-) diff --git a/board/hammer/board.h b/board/hammer/board.h index c150ac40bd..7febe92ddb 100644 --- a/board/hammer/board.h +++ b/board/hammer/board.h @@ -142,6 +142,8 @@ #undef CONFIG_USB_I2C_MAX_READ_COUNT #define CONFIG_USB_I2C_MAX_READ_COUNT (1024 - 6) /* 6 is maximum header size */ +#define CONFIG_I2C_XFER_LARGE_READ + /* No lid switch */ #undef CONFIG_LID_SWITCH diff --git a/common/i2c_master.c b/common/i2c_master.c index b2f9ec285f..c14ed03d73 100644 --- a/common/i2c_master.c +++ b/common/i2c_master.c @@ -50,6 +50,34 @@ const struct i2c_port_t *get_i2c_port(int port) return NULL; } +#ifdef CONFIG_I2C_XFER_LARGE_READ +/* + * Internal function that splits reading into multiple chip_i2c_xfer() calls + * if in_size exceeds CONFIG_I2C_CHIP_MAX_READ_SIZE. + */ +static int i2c_xfer_no_retry(int port, int slave_addr, const uint8_t *out, + int out_size, uint8_t *in, int in_size, int flags) +{ + int ret; + int out_flags = flags & I2C_XFER_START; + int in_chunk_size = MIN(in_size, CONFIG_I2C_CHIP_MAX_READ_SIZE); + + in_size -= in_chunk_size; + out_flags |= !in_size ? (flags & I2C_XFER_STOP) : 0; + ret = chip_i2c_xfer(port, slave_addr, out, out_size, in, in_chunk_size, + out_flags); + in += in_chunk_size; + while (in_size && ret == EC_SUCCESS) { + in_chunk_size = MIN(in_size, CONFIG_I2C_CHIP_MAX_READ_SIZE); + in_size -= in_chunk_size; + ret = chip_i2c_xfer(port, slave_addr, NULL, 0, in, + in_chunk_size, !in_size ? (flags & I2C_XFER_STOP) : 0); + in += in_chunk_size; + } + return ret; +} +#endif /* CONFIG_I2C_XFER_LARGE_READ */ + int i2c_xfer(int port, int slave_addr, const uint8_t *out, int out_size, uint8_t *in, int in_size, int flags) { @@ -57,8 +85,13 @@ int i2c_xfer(int port, int slave_addr, const uint8_t *out, int out_size, int ret = EC_SUCCESS; for (i = 0; i <= CONFIG_I2C_NACK_RETRY_COUNT; i++) { +#ifdef CONFIG_I2C_XFER_LARGE_READ + ret = i2c_xfer_no_retry(port, slave_addr, out, out_size, in, + in_size, flags); +#else ret = chip_i2c_xfer(port, slave_addr, out, out_size, in, in_size, flags); +#endif /* CONFIG_I2C_XFER_LARGE_READ */ if (ret != EC_ERROR_BUSY) break; } diff --git a/common/usb_i2c.c b/common/usb_i2c.c index de8314f8fa..5fa04e0ba1 100644 --- a/common/usb_i2c.c +++ b/common/usb_i2c.c @@ -84,12 +84,8 @@ void usb_i2c_execute(struct usb_i2c_config const *config) uint8_t slave_addr = (config->buffer[0] >> 7) & 0xfe; int write_count = (config->buffer[1] >> 0) & 0xff; int read_count = (config->buffer[1] >> 8) & 0xff; - uint8_t *payload = (uint8_t *)(config->buffer + 2); - int xfer_flag = I2C_XFER_START; int offset = 0; /* Offset for extended reading header. */ - int rv = 0; - int complete_bytes = 0; - int bytes_to_read, port; + int port; config->buffer[0] = 0; config->buffer[1] = 0; @@ -120,28 +116,12 @@ void usb_i2c_execute(struct usb_i2c_config const *config) * EC_CMD_I2C_PASSTHRU, which can protect ports and ranges. */ port = i2c_ports[portindex].port; - /* - * Because The I2C_XFER_SINGLE might limit the reading to be - * less than 255 bytes (For example, register design of - * STM32_I2C_CR2 in i2c-stm32f0.c), we hold the STOP bits for - * reading request larger than 255 bytes. - */ - do { - bytes_to_read = read_count - complete_bytes; - if (bytes_to_read > MAX_BYTES_IN_ONE_READING) - bytes_to_read = MAX_BYTES_IN_ONE_READING; - else - xfer_flag |= I2C_XFER_STOP; - rv = i2c_xfer( - port, slave_addr, - complete_bytes ? NULL : payload + offset, - complete_bytes ? 0 : write_count, - payload + complete_bytes, - bytes_to_read, xfer_flag); - complete_bytes += bytes_to_read; - xfer_flag = 0; - } while (complete_bytes < read_count && !rv); - config->buffer[0] = usb_i2c_map_error(rv); + config->buffer[0] = usb_i2c_map_error( + i2c_xfer(port, slave_addr, + (uint8_t *)(config->buffer + 2) + offset, + write_count, + (uint8_t *)(config->buffer + 2), + read_count, I2C_XFER_SINGLE)); } usb_i2c_write_packet(config, read_count + 4); } diff --git a/extra/touchpad_updater/touchpad_updater.c b/extra/touchpad_updater/touchpad_updater.c index 059ddb0933..d09b899a8d 100644 --- a/extra/touchpad_updater/touchpad_updater.c +++ b/extra/touchpad_updater/touchpad_updater.c @@ -588,13 +588,16 @@ int main(int argc, char *argv[]) */ elan_get_fw_info(); + /* Trigger an I2C transaction of expecting reading of 633 bytes. */ if (extended_i2c_exercise) { - /* - * Trigger an I2C transaction of expecting reading > 60 bytes. - * source: https://goo.gl/pSxESS - */ - elan_write_and_read(0x0002, rx_buf, 118, 0, 0); - pretty_print_buffer(rx_buf, 118); + tx_buf[0] = 0x05; + tx_buf[1] = 0x00; + tx_buf[2] = 0x3C; + tx_buf[3] = 0x02; + tx_buf[4] = 0x06; + tx_buf[5] = 0x00; + libusb_single_write_and_read(tx_buf, 6, rx_buf, 633); + pretty_print_buffer(rx_buf, 637); } /* Get the trackpad ready for receiving update */ diff --git a/include/config.h b/include/config.h index 713bb87bb5..f5c26ffa55 100644 --- a/include/config.h +++ b/include/config.h @@ -1576,6 +1576,18 @@ #undef CONFIG_I2C_PASSTHRU_RESTRICTED #undef CONFIG_I2C_VIRTUAL_BATTERY +/* + * Conservative I2C reading size per single transaction. For example, register + * of stm32f0 and stm32l4 are limited to be 8 bits for this field. + */ +#define CONFIG_I2C_CHIP_MAX_READ_SIZE 255 + +/* + * Enable i2c_xfer() for receiving request larger than + * CONFIG_I2C_CHIP_MAX_READ_SIZE. + */ +#undef CONFIG_I2C_XFER_LARGE_READ + /* EC uses an I2C master interface */ #undef CONFIG_I2C_MASTER diff --git a/include/i2c.h b/include/i2c.h index 73716cf705..63169ec055 100644 --- a/include/i2c.h +++ b/include/i2c.h @@ -89,7 +89,9 @@ extern const int i2c_test_dev_used; #define I2C_XFER_SINGLE (I2C_XFER_START | I2C_XFER_STOP) /* One transaction */ /** - * Transmit one block of raw data, then receive one block of raw data. + * Transmit one block of raw data, then receive one block of raw data. However, + * received data might be capped at CONFIG_I2C_CHIP_MAX_READ_SIZE if + * CONFIG_I2C_XFER_LARGE_READ is not defined. * * This is a wrapper function for chip_i2c_xfer(), a low-level chip-dependent * function. It must be called between i2c_lock(port, 1) and i2c_lock(port, 0).