From ffc59c496bf8498657321c59433f55bbcf2d9c38 Mon Sep 17 00:00:00 2001 From: Wolfram Sang Date: Tue, 10 Jul 2018 23:42:16 +0200 Subject: i2c: recovery: require either get_sda or set_sda For bus recovery, we either need to bail out early if we can read SDA or we need to send STOP after every pulse. Otherwise recovery might be misinterpreted as an unwanted write. So, require one of those SDA handling functions to avoid this problem. Signed-off-by: Wolfram Sang Acked-by: Peter Rosin Signed-off-by: Wolfram Sang --- drivers/i2c/i2c-core-base.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) (limited to 'drivers/i2c/i2c-core-base.c') diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 301285c54603..871a9731894f 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -202,7 +202,8 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap) /* * If we can set SDA, we will always create STOP here to ensure * the additional pulses will do no harm. This is achieved by - * letting SDA follow SCL half a cycle later. + * letting SDA follow SCL half a cycle later. Check the + * 'incomplete_write_byte' fault injector for details. */ ndelay(RECOVERY_NDELAY / 2); if (bri->set_sda) @@ -274,6 +275,10 @@ static void i2c_init_recovery(struct i2c_adapter *adap) err_str = "no {get|set}_scl() found"; goto err; } + if (!bri->set_sda && !bri->get_sda) { + err_str = "either get_sda() or set_sda() needed"; + goto err; + } } return; -- cgit v1.2.3 From 0b71026c69caa10261218528326721828d29a481 Mon Sep 17 00:00:00 2001 From: Wolfram Sang Date: Tue, 10 Jul 2018 23:42:17 +0200 Subject: i2c: recovery: refactor recovery function After exiting the while loop, we checked if recovery was successful and sent a STOP to the clients. Meanwhile however, we send a STOP after every pulse, so it is not needed after the loop. If we move the check for a free bus to the end of the while loop, we can shorten and simplify the logic. It is still ensured that at least one STOP will be sent to the wire even if SDA was not stuck low. Signed-off-by: Wolfram Sang Reviewed-by: Peter Rosin Signed-off-by: Wolfram Sang --- drivers/i2c/i2c-core-base.c | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-) (limited to 'drivers/i2c/i2c-core-base.c') diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 871a9731894f..c7995efd58ea 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -191,9 +191,6 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap) ret = -EBUSY; break; } - /* Break if SDA is high */ - if (bri->get_sda && bri->get_sda(adap)) - break; } val = !val; @@ -209,22 +206,13 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap) if (bri->set_sda) bri->set_sda(adap, val); ndelay(RECOVERY_NDELAY / 2); - } - - /* check if recovery actually succeeded */ - if (bri->get_sda && !bri->get_sda(adap)) - ret = -EBUSY; - /* If all went well, send STOP for a sane bus state. */ - if (ret == 0 && bri->set_sda) { - bri->set_scl(adap, 0); - ndelay(RECOVERY_NDELAY / 2); - bri->set_sda(adap, 0); - ndelay(RECOVERY_NDELAY / 2); - bri->set_scl(adap, 1); - ndelay(RECOVERY_NDELAY / 2); - bri->set_sda(adap, 1); - ndelay(RECOVERY_NDELAY / 2); + /* Break if SDA is high */ + if (val && bri->get_sda) { + ret = bri->get_sda(adap) ? 0 : -EBUSY; + if (ret == 0) + break; + } } if (bri->unprepare_recovery) -- cgit v1.2.3 From 7ca5f6be7900ca753ed01c0202dc5f998a41f4ee Mon Sep 17 00:00:00 2001 From: Wolfram Sang Date: Wed, 11 Jul 2018 00:24:22 +0200 Subject: i2c: recovery: add get_bus_free callback Some IP cores have an internal 'bus free' logic which may be more advanced than just checking if SDA is high. Add a separate callback to get this status. Filling it is optional. Signed-off-by: Wolfram Sang Signed-off-by: Wolfram Sang --- drivers/i2c/i2c-core-base.c | 27 +++++++++++++++++++++++---- include/linux/i2c.h | 3 +++ 2 files changed, 26 insertions(+), 4 deletions(-) (limited to 'drivers/i2c/i2c-core-base.c') diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index c7995efd58ea..59f8dfc5be36 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -158,6 +158,22 @@ static void set_sda_gpio_value(struct i2c_adapter *adap, int val) gpiod_set_value_cansleep(adap->bus_recovery_info->sda_gpiod, val); } +static int i2c_generic_bus_free(struct i2c_adapter *adap) +{ + struct i2c_bus_recovery_info *bri = adap->bus_recovery_info; + int ret = -EOPNOTSUPP; + + if (bri->get_bus_free) + ret = bri->get_bus_free(adap); + else if (bri->get_sda) + ret = bri->get_sda(adap); + + if (ret < 0) + return ret; + + return ret ? 0 : -EBUSY; +} + /* * We are generating clock pulses. ndelay() determines durating of clk pulses. * We will generate clock with rate 100 KHz and so duration of both clock levels @@ -169,7 +185,7 @@ static void set_sda_gpio_value(struct i2c_adapter *adap, int val) int i2c_generic_scl_recovery(struct i2c_adapter *adap) { struct i2c_bus_recovery_info *bri = adap->bus_recovery_info; - int i = 0, val = 1, ret = 0; + int i = 0, val = 1, ret; if (bri->prepare_recovery) bri->prepare_recovery(adap); @@ -207,14 +223,17 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap) bri->set_sda(adap, val); ndelay(RECOVERY_NDELAY / 2); - /* Break if SDA is high */ - if (val && bri->get_sda) { - ret = bri->get_sda(adap) ? 0 : -EBUSY; + if (val) { + ret = i2c_generic_bus_free(adap); if (ret == 0) break; } } + /* If we can't check bus status, assume recovery worked */ + if (ret == -EOPNOTSUPP) + ret = 0; + if (bri->unprepare_recovery) bri->unprepare_recovery(adap); diff --git a/include/linux/i2c.h b/include/linux/i2c.h index 4c4360f94786..bc8d42f8544f 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -587,6 +587,8 @@ struct i2c_timings { * @set_sda: This sets/clears the SDA line. This or get_sda() is mandatory for * generic SCL recovery. Populated internally, if sda_gpio is a valid GPIO, * for generic GPIO recovery. + * @get_bus_free: Returns the bus free state as seen from the IP core in case it + * has a more complex internal logic than just reading SDA. Optional. * @prepare_recovery: This will be called before starting recovery. Platform may * configure padmux here for SDA/SCL line or something else they want. * @unprepare_recovery: This will be called after completing recovery. Platform @@ -601,6 +603,7 @@ struct i2c_bus_recovery_info { void (*set_scl)(struct i2c_adapter *adap, int val); int (*get_sda)(struct i2c_adapter *adap); void (*set_sda)(struct i2c_adapter *adap, int val); + int (*get_bus_free)(struct i2c_adapter *adap); void (*prepare_recovery)(struct i2c_adapter *adap); void (*unprepare_recovery)(struct i2c_adapter *adap); -- cgit v1.2.3 From f7ff75e2a88f9246dace2195e9dedd98df41d416 Mon Sep 17 00:00:00 2001 From: Wolfram Sang Date: Wed, 11 Jul 2018 00:27:22 +0200 Subject: i2c: recovery: rename variable for easier understanding While refactoring the routine before, it occurred to me that this will make the code much easier to understand. Signed-off-by: Wolfram Sang Acked-by: Peter Rosin Signed-off-by: Wolfram Sang --- drivers/i2c/i2c-core-base.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'drivers/i2c/i2c-core-base.c') diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 59f8dfc5be36..57538d72f2e5 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -185,12 +185,12 @@ static int i2c_generic_bus_free(struct i2c_adapter *adap) int i2c_generic_scl_recovery(struct i2c_adapter *adap) { struct i2c_bus_recovery_info *bri = adap->bus_recovery_info; - int i = 0, val = 1, ret; + int i = 0, scl = 1, ret; if (bri->prepare_recovery) bri->prepare_recovery(adap); - bri->set_scl(adap, val); + bri->set_scl(adap, scl); if (bri->set_sda) bri->set_sda(adap, 1); ndelay(RECOVERY_NDELAY); @@ -199,7 +199,7 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap) * By this time SCL is high, as we need to give 9 falling-rising edges */ while (i++ < RECOVERY_CLK_CNT * 2) { - if (val) { + if (scl) { /* SCL shouldn't be low here */ if (!bri->get_scl(adap)) { dev_err(&adap->dev, @@ -209,8 +209,8 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap) } } - val = !val; - bri->set_scl(adap, val); + scl = !scl; + bri->set_scl(adap, scl); /* * If we can set SDA, we will always create STOP here to ensure @@ -220,10 +220,10 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap) */ ndelay(RECOVERY_NDELAY / 2); if (bri->set_sda) - bri->set_sda(adap, val); + bri->set_sda(adap, scl); ndelay(RECOVERY_NDELAY / 2); - if (val) { + if (scl) { ret = i2c_generic_bus_free(adap); if (ret == 0) break; -- cgit v1.2.3 From c4ae05b976b2a67fb24f35d21731b4da2c235bbf Mon Sep 17 00:00:00 2001 From: Wolfram Sang Date: Tue, 17 Jul 2018 11:00:05 +0200 Subject: i2c: recovery: make pin init look like STOP When we initialize the pins, make sure it looks like STOP by dividing the delay into halves. It shouldn't matter because SDA is expected to be held low by a device, but for super-safety, let's do it. Signed-off-by: Wolfram Sang Reviewed-by: Ulrich Hecht Reviewed-by: Peter Rosin Signed-off-by: Wolfram Sang --- drivers/i2c/i2c-core-base.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) (limited to 'drivers/i2c/i2c-core-base.c') diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 57538d72f2e5..02d6f27b19e4 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -190,10 +190,17 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap) if (bri->prepare_recovery) bri->prepare_recovery(adap); + /* + * If we can set SDA, we will always create a STOP to ensure additional + * pulses will do no harm. This is achieved by letting SDA follow SCL + * half a cycle later. Check the 'incomplete_write_byte' fault injector + * for details. + */ bri->set_scl(adap, scl); + ndelay(RECOVERY_NDELAY / 2); if (bri->set_sda) - bri->set_sda(adap, 1); - ndelay(RECOVERY_NDELAY); + bri->set_sda(adap, scl); + ndelay(RECOVERY_NDELAY / 2); /* * By this time SCL is high, as we need to give 9 falling-rising edges @@ -211,13 +218,7 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap) scl = !scl; bri->set_scl(adap, scl); - - /* - * If we can set SDA, we will always create STOP here to ensure - * the additional pulses will do no harm. This is achieved by - * letting SDA follow SCL half a cycle later. Check the - * 'incomplete_write_byte' fault injector for details. - */ + /* Creating STOP again, see above */ ndelay(RECOVERY_NDELAY / 2); if (bri->set_sda) bri->set_sda(adap, scl); -- cgit v1.2.3 From d9cfe2ce246845b9cca0ec1b881e826965893c58 Mon Sep 17 00:00:00 2001 From: Wolfram Sang Date: Mon, 23 Jul 2018 22:26:05 +0200 Subject: i2c: quirks: add zero length checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some adapters do not support a message length of 0. Add this as a quirk so drivers don't have to open code it. Signed-off-by: Wolfram Sang Reviewed-by: Niklas Söderlund Reviewed-by: Andy Shevchenko Tested-by: Jarkko Nikula Acked-by: Jarkko Nikula Signed-off-by: Wolfram Sang --- drivers/i2c/i2c-core-base.c | 6 ++++++ include/linux/i2c.h | 4 ++++ 2 files changed, 10 insertions(+) (limited to 'drivers/i2c/i2c-core-base.c') diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 02d6f27b19e4..a26b3e9cc441 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -1839,9 +1839,15 @@ static int i2c_check_for_quirks(struct i2c_adapter *adap, struct i2c_msg *msgs, if (msgs[i].flags & I2C_M_RD) { if (do_len_check && i2c_quirk_exceeded(len, q->max_read_len)) return i2c_quirk_error(adap, &msgs[i], "msg too long"); + + if (q->flags & I2C_AQ_NO_ZERO_LEN_READ && len == 0) + return i2c_quirk_error(adap, &msgs[i], "no zero length"); } else { if (do_len_check && i2c_quirk_exceeded(len, q->max_write_len)) return i2c_quirk_error(adap, &msgs[i], "msg too long"); + + if (q->flags & I2C_AQ_NO_ZERO_LEN_WRITE && len == 0) + return i2c_quirk_error(adap, &msgs[i], "no zero length"); } } diff --git a/include/linux/i2c.h b/include/linux/i2c.h index bc8d42f8544f..2a98d0886d2e 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -661,6 +661,10 @@ struct i2c_adapter_quirks { I2C_AQ_COMB_READ_SECOND | I2C_AQ_COMB_SAME_ADDR) /* clock stretching is not supported */ #define I2C_AQ_NO_CLK_STRETCH BIT(4) +/* message cannot have length of 0 */ +#define I2C_AQ_NO_ZERO_LEN_READ BIT(5) +#define I2C_AQ_NO_ZERO_LEN_WRITE BIT(6) +#define I2C_AQ_NO_ZERO_LEN (I2C_AQ_NO_ZERO_LEN_READ | I2C_AQ_NO_ZERO_LEN_WRITE) /* * i2c_adapter is the structure used to identify a physical i2c bus along -- cgit v1.2.3 From 4717be73c2843a3d6d8546872177a19358f6b7b5 Mon Sep 17 00:00:00 2001 From: Andy Shevchenko Date: Wed, 25 Jul 2018 17:39:25 +0300 Subject: i2c: core: Parse SDA hold time from firmware There are two drivers already using the SDA hold time setting. It might be more in the future, thus, make I2C core to parse the setting for us if provided by firmware. Signed-off-by: Andy Shevchenko Tested-by: Alexandre Belloni Reviewed-by: Alexandre Belloni Acked-by: Ludovic Desroches Signed-off-by: Wolfram Sang --- drivers/i2c/i2c-core-base.c | 2 ++ include/linux/i2c.h | 2 ++ 2 files changed, 4 insertions(+) (limited to 'drivers/i2c/i2c-core-base.c') diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index a26b3e9cc441..043c4aadaa44 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -1576,6 +1576,8 @@ void i2c_parse_fw_timings(struct device *dev, struct i2c_timings *t, bool use_de ret = device_property_read_u32(dev, "i2c-sda-falling-time-ns", &t->sda_fall_ns); if (ret && use_defaults) t->sda_fall_ns = t->scl_fall_ns; + + device_property_read_u32(dev, "i2c-sda-hold-time-ns", &t->sda_hold_ns); } EXPORT_SYMBOL_GPL(i2c_parse_fw_timings); diff --git a/include/linux/i2c.h b/include/linux/i2c.h index 2a98d0886d2e..36f357ecdf67 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -564,6 +564,7 @@ struct i2c_lock_operations { * @scl_fall_ns: time SCL signal takes to fall in ns; t(f) in the I2C specification * @scl_int_delay_ns: time IP core additionally needs to setup SCL in ns * @sda_fall_ns: time SDA signal takes to fall in ns; t(f) in the I2C specification + * @sda_hold_ns: time IP core additionally needs to hold SDA in ns */ struct i2c_timings { u32 bus_freq_hz; @@ -571,6 +572,7 @@ struct i2c_timings { u32 scl_fall_ns; u32 scl_int_delay_ns; u32 sda_fall_ns; + u32 sda_hold_ns; }; /** -- cgit v1.2.3