diff options
author | Jonathan Brandmeyer <jbrandmeyer@chromium.org> | 2018-08-09 12:58:03 -0600 |
---|---|---|
committer | chrome-bot <chrome-bot@chromium.org> | 2018-08-16 13:14:49 -0700 |
commit | 39ab7a039676ef3738171ed00dc3c7f61802e7b3 (patch) | |
tree | a32fdd410d4d41f2092dbaf283c44b528d4b03c4 | |
parent | 94d06cd5f92e163fca391529509f85909ed65ac6 (diff) | |
download | chrome-ec-39ab7a039676ef3738171ed00dc3c7f61802e7b3.tar.gz |
i2c: Split i2c_xfer into locked/unlocked versions.
Rename i2c_xfer to i2c_xfer_unlocked. Audit all users of i2c_xfer to
see if they can use simple locking semantics or require their own
locking. Since locked accesses are only safe for I2C_XFER_SINGLE
transactions, remove the flags parameter from i2c_xfer. This also makes
the audit a bit easier.
Some remaining applications hold the bus locked across several
transactions that would otherwise be atomic, and a few others implement
complex I2C transactions in multiple commands.
Add a (nondeterministic) verification on the I2C port locking
correctness. This will catch all statically incorrect locking patterns,
although dynamically incorrect locking patterns may not be caught.
Related: Revise the i2c_port_active_count to be a bitmap of the active
ports instead of a count of the active ports. The EC's mutex does not
provide an is_locked() primitive, and we would prefer not to have one.
- board/glados: Custom locking for battery reset
- board/mchpevb1: Custom locking for battery reset
- board/oak: Custom locking for battery reset
- board/samus: Custom locking for lightbar reset
- board/sweetberry: simple locking
- board/servo_micro: Custom locking for funky i2c transfers
- capsense: simple locking
- host_command_master: multi-command transactions
- lb_common: manual locking to support samus power sequence
- smbus: multi-command transactions
- usb_i2c: simple locking
- driver/tcpm/fusb302: simple locking and multi-command transactions
- driver/tcpm/tcpi: Forward _unlocked and implicitly locked interface to
fusb302
- driver/touchpad_elan: simple locking
BUG=chromium:871851
BRANCH=none
TEST=buildall; very careful audit
TEST=grunt clamshell and Coral clamshell, test boot, battery charging,
PD communication, and TCPC port low-power mode.
Signed-off-by: Jonathan Brandmeyer <jbrandmeyer@chromium.org>
Change-Id: Ieabf22bcab42780bdb994fca3ced5d8c62519d56
Reviewed-on: https://chromium-review.googlesource.com/1169913
Commit-Ready: Jonathan Brandmeyer <jbrandmeyer@chromium.org>
Tested-by: Jonathan Brandmeyer <jbrandmeyer@chromium.org>
Reviewed-by: Randall Spangler <rspangler@chromium.org>
Reviewed-by: Jett Rink <jettrink@chromium.org>
-rw-r--r-- | board/glados/battery.c | 4 | ||||
-rw-r--r-- | board/mchpevb1/battery.c | 4 | ||||
-rw-r--r-- | board/oak/battery.c | 4 | ||||
-rw-r--r-- | board/samus/battery.c | 4 | ||||
-rw-r--r-- | board/servo_micro/board.c | 7 | ||||
-rw-r--r-- | board/sweetberry/board.c | 4 | ||||
-rw-r--r-- | chip/stm32/usb_power.c | 5 | ||||
-rw-r--r-- | common/capsense.c | 4 | ||||
-rw-r--r-- | common/host_command_master.c | 7 | ||||
-rw-r--r-- | common/i2c_master.c | 127 | ||||
-rw-r--r-- | common/lb_common.c | 6 | ||||
-rw-r--r-- | common/smbus.c | 29 | ||||
-rw-r--r-- | common/usb_i2c.c | 9 | ||||
-rw-r--r-- | driver/tcpm/fusb302.c | 10 | ||||
-rw-r--r-- | driver/tcpm/tcpci.c | 16 | ||||
-rw-r--r-- | driver/tcpm/tcpm.h | 12 | ||||
-rw-r--r-- | driver/touchpad_elan.c | 36 | ||||
-rw-r--r-- | include/i2c.h | 21 |
18 files changed, 156 insertions, 153 deletions
diff --git a/board/glados/battery.c b/board/glados/battery.c index 2372affb64..016d225950 100644 --- a/board/glados/battery.c +++ b/board/glados/battery.c @@ -47,9 +47,9 @@ int board_cut_off_battery(void) buf[2] = PARAM_CUT_OFF_HIGH; i2c_lock(I2C_PORT_BATTERY, 1); - rv = i2c_xfer(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0, + rv = i2c_xfer_unlocked(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0, I2C_XFER_SINGLE); - rv |= i2c_xfer(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0, + rv |= i2c_xfer_unlocked(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0, I2C_XFER_SINGLE); i2c_lock(I2C_PORT_BATTERY, 0); diff --git a/board/mchpevb1/battery.c b/board/mchpevb1/battery.c index 2372affb64..016d225950 100644 --- a/board/mchpevb1/battery.c +++ b/board/mchpevb1/battery.c @@ -47,9 +47,9 @@ int board_cut_off_battery(void) buf[2] = PARAM_CUT_OFF_HIGH; i2c_lock(I2C_PORT_BATTERY, 1); - rv = i2c_xfer(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0, + rv = i2c_xfer_unlocked(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0, I2C_XFER_SINGLE); - rv |= i2c_xfer(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0, + rv |= i2c_xfer_unlocked(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0, I2C_XFER_SINGLE); i2c_lock(I2C_PORT_BATTERY, 0); diff --git a/board/oak/battery.c b/board/oak/battery.c index a3fe0cd452..a868e20bc0 100644 --- a/board/oak/battery.c +++ b/board/oak/battery.c @@ -57,9 +57,9 @@ static int cutoff(void) buf[2] = PARAM_CUT_OFF_HIGH; i2c_lock(I2C_PORT_BATTERY, 1); - rv = i2c_xfer(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0, + rv = i2c_xfer_unlocked(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0, I2C_XFER_SINGLE); - rv |= i2c_xfer(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0, + rv |= i2c_xfer_unlocked(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0, I2C_XFER_SINGLE); i2c_lock(I2C_PORT_BATTERY, 0); diff --git a/board/samus/battery.c b/board/samus/battery.c index 64cf417ecc..f3c78e9d96 100644 --- a/board/samus/battery.c +++ b/board/samus/battery.c @@ -290,9 +290,9 @@ int board_cut_off_battery(void) buf[2] = PARAM_CUT_OFF_HIGH; i2c_lock(I2C_PORT_BATTERY, 1); - rv = i2c_xfer(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0, + rv = i2c_xfer_unlocked(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0, I2C_XFER_SINGLE); - rv |= i2c_xfer(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0, + rv |= i2c_xfer_unlocked(I2C_PORT_BATTERY, BATTERY_ADDR, buf, 3, NULL, 0, I2C_XFER_SINGLE); i2c_lock(I2C_PORT_BATTERY, 0); diff --git a/board/servo_micro/board.c b/board/servo_micro/board.c index 5887d559ae..355c091a77 100644 --- a/board/servo_micro/board.c +++ b/board/servo_micro/board.c @@ -231,12 +231,13 @@ static int ite_i2c_read_register(uint8_t register_offset, uint8_t *output) */ int ret; /* Tell the ITE EC which register we want to read. */ - ret = i2c_xfer(I2C_PORT_MASTER, ITE_DFU_I2C_CMD_ADDR, ®ister_offset, - sizeof(register_offset), NULL, 0, I2C_XFER_SINGLE); + ret = i2c_xfer_unlocked(I2C_PORT_MASTER, ITE_DFU_I2C_CMD_ADDR, + ®ister_offset, sizeof(register_offset), + NULL, 0, I2C_XFER_SINGLE); if (ret != EC_SUCCESS) return ret; /* Read in the 1 byte register value. */ - ret = i2c_xfer(I2C_PORT_MASTER, ITE_DFU_I2C_DATA_ADDR, NULL, 0, + ret = i2c_xfer_unlocked(I2C_PORT_MASTER, ITE_DFU_I2C_DATA_ADDR, NULL, 0, output, sizeof(*output), I2C_XFER_SINGLE); return ret; } diff --git a/board/sweetberry/board.c b/board/sweetberry/board.c index 8640cfd87f..2d94f06f76 100644 --- a/board/sweetberry/board.c +++ b/board/sweetberry/board.c @@ -121,8 +121,6 @@ static void board_init(void) uint8_t tmp; /* i2c 0 has a tendancy to get wedged. TODO(nsanders): why? */ - i2c_lock(0, 1); - i2c_xfer(0, 0, NULL, 0, &tmp, 1, I2C_XFER_SINGLE); - i2c_lock(0, 0); + i2c_xfer(0, 0, NULL, 0, &tmp, 1); } DECLARE_HOOK(HOOK_INIT, board_init, HOOK_PRIO_DEFAULT); diff --git a/chip/stm32/usb_power.c b/chip/stm32/usb_power.c index cc4f723322..7e4206e83b 100644 --- a/chip/stm32/usb_power.c +++ b/chip/stm32/usb_power.c @@ -411,10 +411,7 @@ uint16_t ina2xx_readagain(uint8_t port, uint8_t addr) int res; uint16_t val; - i2c_lock(port, 1); - res = i2c_xfer(port, addr, NULL, 0, (uint8_t *)&val, sizeof(uint16_t), - I2C_XFER_SINGLE); - i2c_lock(port, 0); + res = i2c_xfer(port, addr, NULL, 0, (uint8_t *)&val, sizeof(uint16_t)); if (res) { CPRINTS("INA2XX I2C readagain failed p:%d a:%02x", diff --git a/common/capsense.c b/common/capsense.c index bc54d5aae3..2e34651573 100644 --- a/common/capsense.c +++ b/common/capsense.c @@ -24,10 +24,8 @@ static int capsense_read_bitmask(void) int rv; uint8_t val = 0; - i2c_lock(I2C_PORT_CAPSENSE, 1); rv = i2c_xfer(I2C_PORT_CAPSENSE, CAPSENSE_I2C_ADDR, - 0, 0, &val, 1, I2C_XFER_SINGLE); - i2c_lock(I2C_PORT_CAPSENSE, 0); + 0, 0, &val, 1); if (rv) CPRINTS("%s failed: error %d", __func__, rv); diff --git a/common/host_command_master.c b/common/host_command_master.c index c567271688..9ba8ba64ab 100644 --- a/common/host_command_master.c +++ b/common/host_command_master.c @@ -80,7 +80,7 @@ static int pd_host_command_internal(int command, int version, */ i2c_lock(I2C_PORT_PD_MCU, 1); i2c_set_timeout(I2C_PORT_PD_MCU, PD_HOST_COMMAND_TIMEOUT_US); - ret = i2c_xfer(I2C_PORT_PD_MCU, CONFIG_USB_PD_I2C_SLAVE_ADDR, + ret = i2c_xfer_unlocked(I2C_PORT_PD_MCU, CONFIG_USB_PD_I2C_SLAVE_ADDR, &req_buf[0], outsize + sizeof(rq) + 1, &resp_buf[0], 2, I2C_XFER_START); i2c_set_timeout(I2C_PORT_PD_MCU, 0); @@ -94,7 +94,7 @@ static int pd_host_command_internal(int command, int version, if (resp_len > (insize + sizeof(rs))) { /* Do a dummy read to generate stop condition */ - i2c_xfer(I2C_PORT_PD_MCU, CONFIG_USB_PD_I2C_SLAVE_ADDR, + i2c_xfer_unlocked(I2C_PORT_PD_MCU, CONFIG_USB_PD_I2C_SLAVE_ADDR, 0, 0, &resp_buf[2], 1, I2C_XFER_STOP); i2c_lock(I2C_PORT_PD_MCU, 0); CPRINTF("[%T response size is too large %d > %d]\n", @@ -103,7 +103,8 @@ static int pd_host_command_internal(int command, int version, } /* Receive remaining data */ - ret = i2c_xfer(I2C_PORT_PD_MCU, CONFIG_USB_PD_I2C_SLAVE_ADDR, 0, 0, + ret = i2c_xfer_unlocked(I2C_PORT_PD_MCU, CONFIG_USB_PD_I2C_SLAVE_ADDR, + 0, 0, &resp_buf[2], resp_len, I2C_XFER_STOP); i2c_lock(I2C_PORT_PD_MCU, 0); if (ret) { diff --git a/common/i2c_master.c b/common/i2c_master.c index aa6340610c..e02a479aeb 100644 --- a/common/i2c_master.c +++ b/common/i2c_master.c @@ -34,9 +34,27 @@ #endif static struct mutex port_mutex[I2C_CONTROLLER_COUNT]; -static uint32_t i2c_port_active_count; +/* A bitmap of the controllers which are currently servicing a request. */ +static uint32_t i2c_port_active_list; +BUILD_ASSERT(I2C_CONTROLLER_COUNT < 32); static uint8_t port_protected[I2C_CONTROLLER_COUNT]; +/** + * Non-deterministically test the lock status of the port. If another task + * has locked the port and the caller is accessing it illegally, then this test + * will incorrectly return true. However, callers which failed to statically + * lock the port will fail quickly. + */ +static int i2c_port_is_locked(int port) +{ +#ifdef CONFIG_I2C_MULTI_PORT_CONTROLLER + /* Test the controller, not the port */ + port = i2c_port_to_controller(port); +#endif + return (i2c_port_active_list >> port) & 1; +} + + const struct i2c_port_t *get_i2c_port(int port) { int i; @@ -98,12 +116,18 @@ static int i2c_xfer_no_retry(int port, int slave_addr, const uint8_t *out, } #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) +int i2c_xfer_unlocked(int port, int slave_addr, + const uint8_t *out, int out_size, + uint8_t *in, int in_size, int flags) { int i; int ret = EC_SUCCESS; + if (!i2c_port_is_locked(port)) { + CPUTS("Access I2C without lock!"); + return EC_ERROR_INVAL; + } + 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, @@ -118,6 +142,19 @@ int i2c_xfer(int port, int slave_addr, const uint8_t *out, int out_size, return ret; } +int i2c_xfer(int port, int slave_addr, const uint8_t *out, int out_size, + uint8_t *in, int in_size) +{ + int rv; + + i2c_lock(port, 1); + rv = i2c_xfer_unlocked(port, slave_addr, out, out_size, in, in_size, + I2C_XFER_SINGLE); + i2c_lock(port, 0); + + return rv; +} + void i2c_lock(int port, int lock) { #ifdef CONFIG_I2C_MULTI_PORT_CONTROLLER @@ -133,7 +170,7 @@ void i2c_lock(int port, int lock) /* Disable interrupt during changing counter for preemption. */ interrupt_disable(); - i2c_port_active_count++; + i2c_port_active_list |= 1 << port; /* Ec cannot enter sleep if there's any i2c port active. */ disable_sleep(SLEEP_MASK_I2C_MASTER); @@ -141,9 +178,9 @@ void i2c_lock(int port, int lock) } else { interrupt_disable(); - i2c_port_active_count--; + i2c_port_active_list &= ~(1 << port); /* Once there is no i2c port active, enable sleep bit of i2c. */ - if (!i2c_port_active_count) + if (!i2c_port_active_list) enable_sleep(SLEEP_MASK_I2C_MASTER); interrupt_enable(); @@ -168,10 +205,7 @@ int i2c_read32(int port, int slave_addr, int offset, int *data) reg = offset & 0xff; /* I2C read 32-bit word: transmit 8-bit offset, and read 32bits */ - i2c_lock(port, 1); - rv = i2c_xfer(port, slave_addr, ®, 1, buf, sizeof(uint32_t), - I2C_XFER_SINGLE); - i2c_lock(port, 0); + rv = i2c_xfer(port, slave_addr, ®, 1, buf, sizeof(uint32_t)); if (rv) return rv; @@ -188,7 +222,6 @@ int i2c_read32(int port, int slave_addr, int offset, int *data) int i2c_write32(int port, int slave_addr, int offset, int data) { - int rv; uint8_t buf[1 + sizeof(uint32_t)]; buf[0] = offset & 0xff; @@ -205,12 +238,8 @@ int i2c_write32(int port, int slave_addr, int offset, int data) buf[4] = (data >> 24) & 0xff; } - i2c_lock(port, 1); - rv = i2c_xfer(port, slave_addr, buf, sizeof(uint32_t) + 1, NULL, 0, - I2C_XFER_SINGLE); - i2c_lock(port, 0); - - return rv; + return i2c_xfer(port, slave_addr, buf, sizeof(uint32_t) + 1, + NULL, 0); } int i2c_read16(int port, int slave_addr, int offset, int *data) @@ -220,10 +249,7 @@ int i2c_read16(int port, int slave_addr, int offset, int *data) reg = offset & 0xff; /* I2C read 16-bit word: transmit 8-bit offset, and read 16bits */ - i2c_lock(port, 1); - rv = i2c_xfer(port, slave_addr, ®, 1, buf, sizeof(uint16_t), - I2C_XFER_SINGLE); - i2c_lock(port, 0); + rv = i2c_xfer(port, slave_addr, ®, 1, buf, sizeof(uint16_t)); if (rv) return rv; @@ -238,7 +264,6 @@ int i2c_read16(int port, int slave_addr, int offset, int *data) int i2c_write16(int port, int slave_addr, int offset, int data) { - int rv; uint8_t buf[1 + sizeof(uint16_t)]; buf[0] = offset & 0xff; @@ -251,45 +276,32 @@ int i2c_write16(int port, int slave_addr, int offset, int data) buf[2] = (data >> 8) & 0xff; } - i2c_lock(port, 1); - rv = i2c_xfer(port, slave_addr, buf, 1 + sizeof(uint16_t), NULL, 0, - I2C_XFER_SINGLE); - i2c_lock(port, 0); - - return rv; + return i2c_xfer(port, slave_addr, buf, 1 + sizeof(uint16_t), NULL, 0); } int i2c_read8(int port, int slave_addr, int offset, int *data) { int rv; - /* We use buf[1] here so it's aligned for DMA on STM32 */ - uint8_t reg, buf[1]; + uint8_t reg = offset; + uint8_t buf; reg = offset; - i2c_lock(port, 1); - rv = i2c_xfer(port, slave_addr, ®, 1, buf, 1, I2C_XFER_SINGLE); - i2c_lock(port, 0); - + rv = i2c_xfer(port, slave_addr, ®, 1, &buf, 1); if (!rv) - *data = buf[0]; + *data = buf; return rv; } int i2c_write8(int port, int slave_addr, int offset, int data) { - int rv; uint8_t buf[2]; buf[0] = offset; buf[1] = data; - i2c_lock(port, 1); - rv = i2c_xfer(port, slave_addr, buf, 2, 0, 0, I2C_XFER_SINGLE); - i2c_lock(port, 0); - - return rv; + return i2c_xfer(port, slave_addr, buf, 2, 0, 0); } int i2c_read_string(int port, int slave_addr, int offset, uint8_t *data, @@ -305,15 +317,19 @@ int i2c_read_string(int port, int slave_addr, int offset, uint8_t *data, * Send device reg space offset, and read back block length. Keep this * session open without a stop. */ - rv = i2c_xfer(port, slave_addr, ®, 1, &block_length, 1, + rv = i2c_xfer_unlocked(port, slave_addr, ®, 1, &block_length, 1, I2C_XFER_START); - if (rv) + if (rv) { + /* Dummy read for the stop bit */ + i2c_xfer_unlocked(port, slave_addr, 0, 0, ®, 1, + I2C_XFER_STOP); goto exit; + } if (len && block_length > (len - 1)) block_length = len - 1; - rv = i2c_xfer(port, slave_addr, 0, 0, data, block_length, + rv = i2c_xfer_unlocked(port, slave_addr, 0, 0, data, block_length, I2C_XFER_STOP); data[block_length] = 0; @@ -328,10 +344,7 @@ int i2c_read_block(int port, int slave_addr, int offset, uint8_t *data, int rv; uint8_t reg_address = offset; - i2c_lock(port, 1); - rv = i2c_xfer(port, slave_addr, ®_address, 1, data, len, - I2C_XFER_SINGLE); - i2c_lock(port, 0); + rv = i2c_xfer(port, slave_addr, ®_address, 1, data, len); return rv; } @@ -348,9 +361,9 @@ int i2c_write_block(int port, int slave_addr, int offset, const uint8_t *data, * order to have a better chance at sending out the stop bit. */ i2c_lock(port, 1); - rv0 = i2c_xfer(port, slave_addr, ®_address, 1, NULL, 0, + rv0 = i2c_xfer_unlocked(port, slave_addr, ®_address, 1, NULL, 0, I2C_XFER_START); - rv1 = i2c_xfer(port, slave_addr, data, len, NULL, 0, + rv1 = i2c_xfer_unlocked(port, slave_addr, data, len, NULL, 0, I2C_XFER_STOP); i2c_lock(port, 0); @@ -711,8 +724,10 @@ static int i2c_command_passthru(struct host_cmd_handler_args *args) #endif if (!port_is_locked) i2c_lock(params->port, (port_is_locked = 1)); - rv = i2c_xfer(params->port, addr, out, write_len, - &resp->data[in_len], read_len, xferflags); + rv = i2c_xfer_unlocked(params->port, addr, + out, write_len, + &resp->data[in_len], read_len, + xferflags); } if (rv) { @@ -844,7 +859,8 @@ static void scan_bus(int port, const char *desc) ccputs("."); /* Do a single read */ - if (!i2c_xfer(port, a, NULL, 0, &tmp, 1, I2C_XFER_SINGLE)) + if (!i2c_xfer_unlocked(port, a, NULL, 0, &tmp, 1, + I2C_XFER_SINGLE)) ccprintf("\n 0x%02x", a); } @@ -925,10 +941,7 @@ static int command_i2cxfer(int argc, char **argv) if (argc < 6 || v < 0 || v > sizeof(data)) return EC_ERROR_PARAM5; - i2c_lock(port, 1); - rv = i2c_xfer(port, slave_addr, - &offset, 1, data, v, I2C_XFER_SINGLE); - i2c_lock(port, 0); + rv = i2c_xfer(port, slave_addr, &offset, 1, data, v); if (!rv) ccprintf("Data: %.*h\n", v, data); diff --git a/common/lb_common.c b/common/lb_common.c index 2ffba93b2f..f800a6a65a 100644 --- a/common/lb_common.c +++ b/common/lb_common.c @@ -120,7 +120,7 @@ static inline void controller_write(int ctrl_num, uint8_t reg, uint8_t val) buf[0] = reg; buf[1] = val; ctrl_num = ctrl_num % ARRAY_SIZE(i2c_addr); - i2c_xfer(I2C_PORT_LIGHTBAR, i2c_addr[ctrl_num], buf, 2, 0, 0, + i2c_xfer_unlocked(I2C_PORT_LIGHTBAR, i2c_addr[ctrl_num], buf, 2, 0, 0, I2C_XFER_SINGLE); } @@ -130,8 +130,8 @@ static inline uint8_t controller_read(int ctrl_num, uint8_t reg) int rv; ctrl_num = ctrl_num % ARRAY_SIZE(i2c_addr); - rv = i2c_xfer(I2C_PORT_LIGHTBAR, i2c_addr[ctrl_num], ®, 1, buf, 1, - I2C_XFER_SINGLE); + rv = i2c_xfer_unlocked(I2C_PORT_LIGHTBAR, i2c_addr[ctrl_num], + ®, 1, buf, 1, I2C_XFER_SINGLE); return rv ? 0 : buf[0]; } diff --git a/common/smbus.c b/common/smbus.c index 1c612983d0..2694eba08a 100644 --- a/common/smbus.c +++ b/common/smbus.c @@ -20,9 +20,6 @@ int smbus_write_word(uint8_t i2c_port, uint8_t slave_addr, uint8_t smbus_cmd, uint16_t d16) { uint8_t buf[5]; - int rv; - - i2c_lock(i2c_port, 1); /* Command sequence for CRC calculation */ buf[0] = slave_addr; @@ -30,11 +27,7 @@ int smbus_write_word(uint8_t i2c_port, uint8_t slave_addr, buf[2] = d16 & 0xff; buf[3] = (d16 >> 8) & 0xff; buf[4] = crc8(buf, 4); - rv = i2c_xfer(i2c_port, slave_addr, - buf + 1, 4, NULL, 0, I2C_XFER_SINGLE); - - i2c_lock(i2c_port, 0); - return rv; + return i2c_xfer(i2c_port, slave_addr, buf + 1, 4, NULL, 0); } /* Write up to SMBUS_MAX_BLOCK_SIZE bytes using smbus block access protocol */ @@ -52,20 +45,21 @@ int smbus_write_block(uint8_t i2c_port, uint8_t slave_addr, i2c_lock(i2c_port, 1); /* Send command + length */ - rv = i2c_xfer(i2c_port, slave_addr, + rv = i2c_xfer_unlocked(i2c_port, slave_addr, buf + 1, 2, NULL, 0, I2C_XFER_START); if (rv != EC_SUCCESS) goto smbus_write_block_done; /* Send data */ - rv = i2c_xfer(i2c_port, slave_addr, data, len, NULL, 0, 0); + rv = i2c_xfer_unlocked(i2c_port, slave_addr, data, len, NULL, 0, 0); if (rv != EC_SUCCESS) goto smbus_write_block_done; /* Send CRC */ buf[0] = crc8(buf, 3); buf[0] = crc8_arg(data, len, buf[0]); - rv = i2c_xfer(i2c_port, slave_addr, buf, 1, NULL, 0, I2C_XFER_STOP); + rv = i2c_xfer_unlocked(i2c_port, slave_addr, buf, 1, NULL, 0, + I2C_XFER_STOP); smbus_write_block_done: i2c_lock(i2c_port, 0); @@ -86,11 +80,8 @@ int smbus_read_word(uint8_t i2c_port, uint8_t slave_addr, buf[2] = slave_addr | 0x1; crc = crc8(buf, 3); - i2c_lock(i2c_port, 1); - /* Read data bytes + CRC byte */ - rv = i2c_xfer(i2c_port, slave_addr, - &smbus_cmd, 1, buf, 3, I2C_XFER_SINGLE); + rv = i2c_xfer(i2c_port, slave_addr, &smbus_cmd, 1, buf, 3); /* Verify CRC */ if (crc8_arg(buf, 2, crc) != buf[2]) @@ -101,7 +92,6 @@ int smbus_read_word(uint8_t i2c_port, uint8_t slave_addr, else *p16 = 0; - i2c_lock(i2c_port, 0); return rv; } @@ -122,7 +112,7 @@ int smbus_read_block(uint8_t i2c_port, uint8_t slave_addr, i2c_lock(i2c_port, 1); /* First read size from slave */ - rv = i2c_xfer(i2c_port, slave_addr, + rv = i2c_xfer_unlocked(i2c_port, slave_addr, &smbus_cmd, 1, buf + 3, 1, I2C_XFER_START); if (rv != EC_SUCCESS) goto smbus_read_block_done; @@ -140,12 +130,13 @@ int smbus_read_block(uint8_t i2c_port, uint8_t slave_addr, } /* Now read back all bytes */ - rv = i2c_xfer(i2c_port, slave_addr, NULL, 0, data, read_len, 0); + rv = i2c_xfer_unlocked(i2c_port, slave_addr, + NULL, 0, data, read_len, 0); if (rv) goto smbus_read_block_done; /* Read CRC + verify */ - rv = i2c_xfer(i2c_port, slave_addr, + rv = i2c_xfer_unlocked(i2c_port, slave_addr, NULL, 0, buf, 1, I2C_XFER_STOP); if (do_crc && crc8_arg(data, read_len, crc) != buf[0]) rv = EC_ERROR_CRC; diff --git a/common/usb_i2c.c b/common/usb_i2c.c index 4ceb4fe031..a92c411f1b 100644 --- a/common/usb_i2c.c +++ b/common/usb_i2c.c @@ -85,7 +85,6 @@ void usb_i2c_execute(struct usb_i2c_config const *config) int write_count = (config->buffer[1] >> 0) & 0xff; int read_count = (config->buffer[1] >> 8) & 0xff; int offset = 0; /* Offset for extended reading header. */ - int port; config->buffer[0] = 0; config->buffer[1] = 0; @@ -117,14 +116,12 @@ void usb_i2c_execute(struct usb_i2c_config const *config) * knows about. It should behave closer to * EC_CMD_I2C_PASSTHRU, which can protect ports and ranges. */ - port = i2c_ports[portindex].port; - i2c_lock(port, 1); - ret = i2c_xfer(port, slave_addr, + ret = i2c_xfer(i2c_ports[portindex].port, + slave_addr, (uint8_t *)(config->buffer + 2) + offset, write_count, (uint8_t *)(config->buffer + 2), - read_count, I2C_XFER_SINGLE); - i2c_lock(port, 0); + read_count); config->buffer[0] = usb_i2c_map_error(ret); } usb_i2c_write_packet(config, read_count + 4); diff --git a/driver/tcpm/fusb302.c b/driver/tcpm/fusb302.c index f89f47896e..da5d32e206 100644 --- a/driver/tcpm/fusb302.c +++ b/driver/tcpm/fusb302.c @@ -322,9 +322,7 @@ static int fusb302_send_message(int port, uint16_t header, const uint32_t *data, buf[buf_pos++] = FUSB302_TKN_TXON; /* burst write for speed! */ - tcpc_lock(port, 1); - rv = tcpc_xfer(port, buf, buf_pos, 0, 0, I2C_XFER_SINGLE); - tcpc_lock(port, 0); + rv = tcpc_xfer(port, buf, buf_pos, 0, 0); return rv; } @@ -727,7 +725,7 @@ static int fusb302_tcpm_get_message(int port, uint32_t *payload, int *head) * PART 1 OF BURST READ: Write in register address. * Issue a START, no STOP. */ - rv = tcpc_xfer(port, buf, 1, 0, 0, I2C_XFER_START); + rv = tcpc_xfer_unlocked(port, buf, 1, 0, 0, I2C_XFER_START); /* * PART 2 OF BURST READ: Read up to the header. @@ -736,7 +734,7 @@ static int fusb302_tcpm_get_message(int port, uint32_t *payload, int *head) * and determine how many more bytes we need to read. * TODO: Check token to ensure valid packet. */ - rv |= tcpc_xfer(port, 0, 0, buf, 3, I2C_XFER_START); + rv |= tcpc_xfer_unlocked(port, 0, 0, buf, 3, I2C_XFER_START); /* Grab the header */ *head = (buf[1] & 0xFF); @@ -750,7 +748,7 @@ static int fusb302_tcpm_get_message(int port, uint32_t *payload, int *head) * No START, but do issue a STOP at the end. * add 4 to len to read CRC out */ - rv |= tcpc_xfer(port, 0, 0, buf, len+4, I2C_XFER_STOP); + rv |= tcpc_xfer_unlocked(port, 0, 0, buf, len+4, I2C_XFER_STOP); tcpc_lock(port, 0); } while (!rv && PACKET_IS_GOOD_CRC(*head) && diff --git a/driver/tcpm/tcpci.c b/driver/tcpm/tcpci.c index f9760a0aed..aec5c8a329 100644 --- a/driver/tcpm/tcpci.c +++ b/driver/tcpm/tcpci.c @@ -106,14 +106,26 @@ int tcpc_write_block(int port, int reg, const uint8_t *out, int size) } int tcpc_xfer(int port, const uint8_t *out, int out_size, + uint8_t *in, int in_size) +{ + int rv; + /* Dispatching to tcpc_xfer_unlocked reduces code size growth. */ + tcpc_lock(port, 1); + rv = tcpc_xfer_unlocked(port, out, out_size, in, in_size, + I2C_XFER_SINGLE); + tcpc_lock(port, 0); + return rv; +} + +int tcpc_xfer_unlocked(int port, const uint8_t *out, int out_size, uint8_t *in, int in_size, int flags) { - int rv = i2c_xfer(tcpc_config[port].i2c_host_port, + int rv = i2c_xfer_unlocked(tcpc_config[port].i2c_host_port, tcpc_config[port].i2c_slave_addr, out, out_size, in, in_size, flags); if (rv && pd_device_in_low_power(port)) { pd_wait_for_wakeup(port); - rv = i2c_xfer(tcpc_config[port].i2c_host_port, + rv = i2c_xfer_unlocked(tcpc_config[port].i2c_host_port, tcpc_config[port].i2c_slave_addr, out, out_size, in, in_size, flags); } diff --git a/driver/tcpm/tcpm.h b/driver/tcpm/tcpm.h index cc360ba75d..0ea0bf47a7 100644 --- a/driver/tcpm/tcpm.h +++ b/driver/tcpm/tcpm.h @@ -51,10 +51,18 @@ static inline int tcpc_read16(int port, int reg, int *val) } static inline int tcpc_xfer(int port, const uint8_t *out, int out_size, - uint8_t *in, int in_size, int flags) + uint8_t *in, int in_size) { return i2c_xfer(tcpc_config[port].i2c_host_port, tcpc_config[port].i2c_slave_addr, out, out_size, in, + in_size); +} + +static inline int tcpc_xfer_unlocked(int port, const uint8_t *out, int out_size, + uint8_t *in, int in_size, int flags) +{ + return i2c_xfer_unlocked(tcpc_config[port].i2c_host_port, + tcpc_config[port].i2c_slave_addr, out, out_size, in, in_size, flags); } @@ -79,6 +87,8 @@ int tcpc_read16(int port, int reg, int *val); int tcpc_read_block(int port, int reg, uint8_t *in, int size); int tcpc_write_block(int port, int reg, const uint8_t *out, int size); int tcpc_xfer(int port, const uint8_t *out, int out_size, + uint8_t *in, int in_size); +int tcpc_xfer_unlocked(int port, const uint8_t *out, int out_size, uint8_t *in, int in_size, int flags); #endif /* CONFIG_USB_PD_TCPC_LOW_POWER */ diff --git a/driver/touchpad_elan.c b/driver/touchpad_elan.c index e1f95f3949..b39ef4fecc 100644 --- a/driver/touchpad_elan.c +++ b/driver/touchpad_elan.c @@ -112,36 +112,25 @@ const int pressure_div = 1024; static int elan_tp_read_cmd(uint16_t reg, uint16_t *val) { uint8_t buf[2]; - int rv; buf[0] = reg; buf[1] = reg >> 8; - i2c_lock(CONFIG_TOUCHPAD_I2C_PORT, 1); - rv = i2c_xfer(CONFIG_TOUCHPAD_I2C_PORT, CONFIG_TOUCHPAD_I2C_ADDR, - buf, sizeof(buf), (uint8_t *)val, sizeof(*val), - I2C_XFER_SINGLE); - i2c_lock(CONFIG_TOUCHPAD_I2C_PORT, 0); - - return rv; + return i2c_xfer(CONFIG_TOUCHPAD_I2C_PORT, CONFIG_TOUCHPAD_I2C_ADDR, + buf, sizeof(buf), (uint8_t *)val, sizeof(*val)); } static int elan_tp_write_cmd(uint16_t reg, uint16_t val) { uint8_t buf[4]; - int rv; buf[0] = reg; buf[1] = reg >> 8; buf[2] = val; buf[3] = val >> 8; - i2c_lock(CONFIG_TOUCHPAD_I2C_PORT, 1); - rv = i2c_xfer(CONFIG_TOUCHPAD_I2C_PORT, CONFIG_TOUCHPAD_I2C_ADDR, - buf, sizeof(buf), NULL, 0, I2C_XFER_SINGLE); - i2c_lock(CONFIG_TOUCHPAD_I2C_PORT, 0); - - return rv; + return i2c_xfer(CONFIG_TOUCHPAD_I2C_PORT, CONFIG_TOUCHPAD_I2C_ADDR, + buf, sizeof(buf), NULL, 0); } /* Power is on by default. */ @@ -201,10 +190,8 @@ static int elan_tp_read_report(void) /* Compute and save timestamp early in case another interrupt comes. */ timestamp = irq_ts / USB_HID_TOUCHPAD_TIMESTAMP_UNIT; - i2c_lock(CONFIG_TOUCHPAD_I2C_PORT, 1); rv = i2c_xfer(CONFIG_TOUCHPAD_I2C_PORT, CONFIG_TOUCHPAD_I2C_ADDR, - NULL, 0, tp_buf, ETP_I2C_REPORT_LEN, I2C_XFER_SINGLE); - i2c_lock(CONFIG_TOUCHPAD_I2C_PORT, 0); + NULL, 0, tp_buf, ETP_I2C_REPORT_LEN); if (rv) { CPRINTS("read report error (%d)", rv); @@ -288,10 +275,8 @@ static void elan_tp_init(void) elan_tp_write_cmd(ETP_I2C_STAND_CMD, ETP_I2C_RESET); msleep(100); - i2c_lock(CONFIG_TOUCHPAD_I2C_PORT, 1); rv = i2c_xfer(CONFIG_TOUCHPAD_I2C_PORT, CONFIG_TOUCHPAD_I2C_ADDR, - NULL, 0, val, sizeof(val), I2C_XFER_SINGLE); - i2c_lock(CONFIG_TOUCHPAD_I2C_PORT, 0); + NULL, 0, val, sizeof(val)); CPRINTS("reset rv %d buf=%04x", rv, *((uint16_t *)val)); if (rv) @@ -480,11 +465,8 @@ static int touchpad_update_page(const uint8_t *data) page_store[FW_PAGE_SIZE + 2 + 0] = checksum & 0xff; page_store[FW_PAGE_SIZE + 2 + 1] = (checksum >> 8) & 0xff; - i2c_lock(CONFIG_TOUCHPAD_I2C_PORT, 1); rv = i2c_xfer(CONFIG_TOUCHPAD_I2C_PORT, CONFIG_TOUCHPAD_I2C_ADDR, - page_store, sizeof(page_store), NULL, 0, - I2C_XFER_SINGLE); - i2c_lock(CONFIG_TOUCHPAD_I2C_PORT, 0); + page_store, sizeof(page_store), NULL, 0); if (rv) return rv; msleep(20); @@ -647,12 +629,10 @@ int touchpad_debug(const uint8_t *param, unsigned int param_size, memset(buffer, 0, buffer_size); } - i2c_lock(CONFIG_TOUCHPAD_I2C_PORT, 1); rv = i2c_xfer(CONFIG_TOUCHPAD_I2C_PORT, CONFIG_TOUCHPAD_I2C_ADDR, ¶m[offset], write_length, - buffer, read_length, I2C_XFER_SINGLE); - i2c_lock(CONFIG_TOUCHPAD_I2C_PORT, 0); + buffer, read_length); if (rv) return EC_RES_BUS_ERROR; diff --git a/include/i2c.h b/include/i2c.h index 0bd47fd24f..8ae21566cb 100644 --- a/include/i2c.h +++ b/include/i2c.h @@ -83,7 +83,7 @@ extern struct i2c_stress_test i2c_stress_tests[]; extern const int i2c_test_dev_used; #endif -/* Flags for i2c_xfer() */ +/* Flags for i2c_xfer_unlocked() */ #define I2C_XFER_START (1 << 0) /* Start smbus session from idle state */ #define I2C_XFER_STOP (1 << 1) /* Terminate smbus session with stop bit */ #define I2C_XFER_SINGLE (I2C_XFER_START | I2C_XFER_STOP) /* One transaction */ @@ -91,10 +91,8 @@ extern const int i2c_test_dev_used; /** * 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). + * CONFIG_I2C_XFER_LARGE_READ is not defined. The transfer is strictly atomic, + * by locking the I2C port and performing an I2C_XFER_SINGLE transfer. * * @param port Port to access * @param slave_addr Slave device address @@ -102,11 +100,20 @@ extern const int i2c_test_dev_used; * @param out_size Number of bytes to send * @param in Destination buffer for received data * @param in_size Number of bytes to receive - * @param flags Flags (see I2C_XFER_* above) * @return EC_SUCCESS, or non-zero if error. */ int i2c_xfer(int port, int slave_addr, const uint8_t *out, int out_size, - uint8_t *in, int in_size, int flags); + uint8_t *in, int in_size); + +/** + * Same as i2c_xfer, but the bus is not implicitly locked. It must be called + * between i2c_lock(port, 1) and i2c_lock(port, 0). + * + * @param flags Flags (see I2C_XFER_* above) + */ +int i2c_xfer_unlocked(int port, int slave_addr, + const uint8_t *out, int out_size, + uint8_t *in, int in_size, int flags); #define I2C_LINE_SCL_HIGH (1 << 0) #define I2C_LINE_SDA_HIGH (1 << 1) |