In multi-master environment, this driver's master cannot know
exactly when a peer master sends data to this driver's slave so a
case can be happened that this master tries to send data through
the master_xfer function but slave data from peer master is still
being processed by this driver. To prevent state corruption in the
case, this patch adds checking code if any slave operation is
ongoing and it waits up to the bus timeout duration before starting
a master_xfer operation.
To support this change, it introduces changes on i2c-core-base to
make that able to read the bus timeout and master transfer retries
count values from device tree properties.
Please review this patch set.
Thanks,
-Jae
Changes since v8:
- Fixed a build break when CONFIG_I2C_SLAVE is not set.
Changes since v7:
- Simplified the bus idle waiting logic using jiffy based timer APIs.
Changes since v6:
- Changed the 'timeout-ms' property name to 'bus-timeout-ms'.
Changes since v5:
- Changed using of property reading API to device_property_read_u32.
Changes since v4:
- Moved the property reading code into i2c-base-core and changed the
property name to 'timeout-ms'. Also, added '#retries' property reading
code.
- Changed bus busy checking logic to make that check slave_state instead
of 'Transfer Mode State Machine' reg value.
Changes since v3:
- Changed the property name to 'timeout' and made it use the
default setting in i2c-core when not specified.
Changes since v2:
- Changed the property name to 'aspeed,timeout' and made it to
update the adapter's timeout configuration.
Changes since v1:
- Changed define names of timeout related.
Jae Hyun Yoo (5):
dt-bindings: i2c: Add 'bus-timeout-ms' and '#retries' properties as
common optional
i2c: core: Add support reading of 'bus-timeout-ms' and '#retries'
properties
dt-bindings: i2c: aspeed: Add 'bus-timeout-ms' property as an optional
property
i2c: aspeed: Remove hard-coded bus timeout value setting
i2c: aspeed: Add bus idle waiting logic for multi-master use cases
.../devicetree/bindings/i2c/i2c-aspeed.txt | 3 +
Documentation/devicetree/bindings/i2c/i2c.txt | 6 ++
drivers/i2c/busses/i2c-aspeed.c | 56 +++++++++++++------
drivers/i2c/i2c-core-base.c | 12 +++-
4 files changed, 59 insertions(+), 18 deletions(-)
--
2.19.1
This commit adds 'bus-timeout-ms' property as an optional property
which can be used for setting the bus timeout value of an adapter.
With this patch, the bus timeout value can be set through this
property at the probing time of this module. Still the bus timeout
value can be set by an I2C_TIMEOUT ioctl on cdev at runtime too.
Signed-off-by: Jae Hyun Yoo <[email protected]>
Reviewed-by: Brendan Higgins <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
Documentation/devicetree/bindings/i2c/i2c-aspeed.txt | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
index 8fbd8633a387..ce1f07620368 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-aspeed.txt
@@ -17,6 +17,9 @@ Optional Properties:
specified
- multi-master : states that there is another master active on this bus.
+- bus-timeout-ms: bus timeout in milliseconds defaults to 1 second when not
+ specified.
+
Example:
i2c {
--
2.19.1
This commit removes hard-coded bus timeout value setting so that
it can be set by i2c-core-base.
Signed-off-by: Jae Hyun Yoo <[email protected]>
Reviewed-by: Joel Stanley <[email protected]>
Reviewed-by: Brendan Higgins <[email protected]>
---
drivers/i2c/busses/i2c-aspeed.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 8dc9161ced38..833b6b6a4c7e 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -930,7 +930,6 @@ static int aspeed_i2c_probe_bus(struct platform_device *pdev)
init_completion(&bus->cmd_complete);
bus->adap.owner = THIS_MODULE;
bus->adap.retries = 0;
- bus->adap.timeout = 5 * HZ;
bus->adap.algo = &aspeed_i2c_algo;
bus->adap.dev.parent = &pdev->dev;
bus->adap.dev.of_node = pdev->dev.of_node;
--
2.19.1
In multi-master environment, this driver's master cannot know
exactly when a peer master sends data to this driver's slave so a
case can be happened that this master tries to send data through
the master_xfer function but slave data from peer master is still
being processed by this driver.
To prevent any state corruption in the case, this patch adds
checking code if any slave operation is ongoing and it waits up to
the bus timeout duration before starting a master_xfer operation.
Signed-off-by: Jae Hyun Yoo <[email protected]>
Reviewed-by: Brendan Higgins <[email protected]>
---
drivers/i2c/busses/i2c-aspeed.c | 55 ++++++++++++++++++++++++---------
1 file changed, 40 insertions(+), 15 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 833b6b6a4c7e..30c3ab3a4844 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -12,6 +12,7 @@
#include <linux/clk.h>
#include <linux/completion.h>
+#include <linux/delay.h>
#include <linux/err.h>
#include <linux/errno.h>
#include <linux/i2c.h>
@@ -115,6 +116,9 @@
/* 0x18 : I2CD Slave Device Address Register */
#define ASPEED_I2CD_DEV_ADDR_MASK GENMASK(6, 0)
+/* Busy checking */
+#define ASPEED_I2C_BUS_BUSY_CHECK_INTERVAL_US (10 * 1000)
+
enum aspeed_i2c_master_state {
ASPEED_I2C_MASTER_INACTIVE,
ASPEED_I2C_MASTER_START,
@@ -156,6 +160,8 @@ struct aspeed_i2c_bus {
int cmd_err;
/* Protected only by i2c_lock_bus */
int master_xfer_result;
+ /* Multi-master */
+ bool multi_master;
#if IS_ENABLED(CONFIG_I2C_SLAVE)
struct i2c_client *slave;
enum aspeed_i2c_slave_state slave_state;
@@ -596,27 +602,44 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
}
+static int aspeed_i2c_check_bus_busy(struct aspeed_i2c_bus *bus)
+{
+ unsigned long timeout;
+
+ if (bus->multi_master) {
+ might_sleep();
+ /* Initialize it only when multi_master is set */
+ timeout = jiffies + bus->adap.timeout;
+ }
+
+ for (;;) {
+ if (!(readl(bus->base + ASPEED_I2C_CMD_REG) &
+ ASPEED_I2CD_BUS_BUSY_STS))
+#if IS_ENABLED(CONFIG_I2C_SLAVE)
+ if (bus->slave_state == ASPEED_I2C_SLAVE_STOP)
+#endif
+ return 0;
+ if (!bus->multi_master)
+ break;
+ if (time_after(jiffies, timeout))
+ break;
+ usleep_range((ASPEED_I2C_BUS_BUSY_CHECK_INTERVAL_US >> 2) + 1,
+ ASPEED_I2C_BUS_BUSY_CHECK_INTERVAL_US);
+ }
+
+ return aspeed_i2c_recover_bus(bus);
+}
+
static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
struct i2c_msg *msgs, int num)
{
struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);
unsigned long time_left, flags;
- int ret = 0;
- spin_lock_irqsave(&bus->lock, flags);
- bus->cmd_err = 0;
-
- /* If bus is busy, attempt recovery. We assume a single master
- * environment.
- */
- if (readl(bus->base + ASPEED_I2C_CMD_REG) & ASPEED_I2CD_BUS_BUSY_STS) {
- spin_unlock_irqrestore(&bus->lock, flags);
- ret = aspeed_i2c_recover_bus(bus);
- if (ret)
- return ret;
- spin_lock_irqsave(&bus->lock, flags);
- }
+ if (aspeed_i2c_check_bus_busy(bus))
+ return -EAGAIN;
+ spin_lock_irqsave(&bus->lock, flags);
bus->cmd_err = 0;
bus->msgs = msgs;
bus->msgs_index = 0;
@@ -827,7 +850,9 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus *bus,
if (ret < 0)
return ret;
- if (!of_property_read_bool(pdev->dev.of_node, "multi-master"))
+ if (of_property_read_bool(pdev->dev.of_node, "multi-master"))
+ bus->multi_master = true;
+ else
fun_ctrl_reg |= ASPEED_I2CD_MULTI_MASTER_DIS;
/* Enable Master Mode */
--
2.19.1
This commit adds support for 'bus-timeout-ms' and '#retries'
properties to set 'timeout' and 'retries' values in
'struct i2c_adapter' in case an adapter node has the properties.
Still the values can be set by I2C_TIMEOUT and I2C_RETRIES ioctls
on cdev at runtime too.
These properties may not be supported by all drivers. However, if
a driver wants to support one of them, it should adapt the
bindings in the dt-bindings document.
Signed-off-by: Jae Hyun Yoo <[email protected]>
---
drivers/i2c/i2c-core-base.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index dc78aa7369de..abf8e78bdb82 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1214,6 +1214,7 @@ EXPORT_SYMBOL_GPL(i2c_handle_smbus_host_notify);
static int i2c_register_adapter(struct i2c_adapter *adap)
{
int res = -EINVAL;
+ u32 bus_timeout_ms = 0;
/* Can't register until after driver model init */
if (WARN_ON(!is_registered)) {
@@ -1239,8 +1240,15 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
INIT_LIST_HEAD(&adap->userspace_clients);
/* Set default timeout to 1 second if not already set */
- if (adap->timeout == 0)
- adap->timeout = HZ;
+ if (adap->timeout == 0) {
+ device_property_read_u32(&adap->dev, "bus-timeout-ms",
+ &bus_timeout_ms);
+ adap->timeout = bus_timeout_ms ?
+ msecs_to_jiffies(bus_timeout_ms) : HZ;
+ }
+
+ /* Set retries count if it has the property setting */
+ device_property_read_u32(&adap->dev, "#retries", &adap->retries);
/* register soft irqs for Host Notify */
res = i2c_setup_host_notify_irq_domain(adap);
--
2.19.1
This commit adds 'bus-timeout-ms' and '#retries' properties as
common optional properties that can be used for setting 'timeout'
and 'retries' values of 'struct i2c_adapter'. With this patch, the
bus timeout value and the master transfer retries count can be set
through these properties at the registration time of an adapter.
Still the values can be set by I2C_TIMEOUT and I2C_RETRIES ioctls
on cdev at runtime too.
These properties may not be supported by all drivers. However, if
a driver wants to support one of them, it should adapt the
bindings in this document.
Signed-off-by: Jae Hyun Yoo <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
Documentation/devicetree/bindings/i2c/i2c.txt | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt
index 11263982470e..bdead91f82a4 100644
--- a/Documentation/devicetree/bindings/i2c/i2c.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c.txt
@@ -80,6 +80,12 @@ wants to support one of the below features, it should adapt the bindings below.
Names of map programmable addresses.
It can contain any map needing another address than default one.
+- bus-timeout-ms
+ Bus timeout in milliseconds.
+
+- #retries
+ Number of retries for master transfer.
+
Binding may contain optional "interrupts" property, describing interrupts
used by the device. I2C core will assign "irq" interrupt (or the very first
interrupt if not using interrupt names) as primary interrupt for the slave.
--
2.19.1
On 10/30/2018 3:22 PM, kbuild test robot wrote:
> Hi Jae,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on wsa/i2c/for-next]
> [also build test WARNING on v4.19 next-20181030]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url: https://github.com/0day-ci/linux/commits/Jae-Hyun-Yoo/dt-bindings-i2c-Add-bus-timeout-ms-and-retries-properties-as-common-optional/20181031-051152
> base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
> config: xtensa-allyesconfig (attached as .config)
> compiler: xtensa-linux-gcc (GCC) 8.1.0
> reproduce:
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> GCC_VERSION=8.1.0 make.cross ARCH=xtensa
>
> Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
> http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
>
> All warnings (new ones prefixed by >>):
>
> In file included from include/linux/ktime.h:25,
> from include/linux/rcutiny.h:28,
> from include/linux/rcupdate.h:209,
> from include/linux/srcu.h:33,
> from include/linux/notifier.h:16,
> from include/linux/clk.h:17,
> from drivers/i2c/busses/i2c-aspeed.c:13:
> drivers/i2c/busses/i2c-aspeed.c: In function 'aspeed_i2c_master_xfer':
>>> include/linux/jiffies.h:108:15: warning: 'timeout' may be used uninitialized in this function [-Wmaybe-uninitialized]
> ((long)((b) - (a)) < 0))
> ^
> drivers/i2c/busses/i2c-aspeed.c:607:16: note: 'timeout' was declared here
> unsigned long timeout;
> ^~~~~~~
> --
> In file included from include/linux/ktime.h:25,
> from include/linux/rcutiny.h:28,
> from include/linux/rcupdate.h:209,
> from include/linux/srcu.h:33,
> from include/linux/notifier.h:16,
> from include/linux/clk.h:17,
> from drivers/i2c//busses/i2c-aspeed.c:13:
> drivers/i2c//busses/i2c-aspeed.c: In function 'aspeed_i2c_master_xfer':
>>> include/linux/jiffies.h:108:15: warning: 'timeout' may be used uninitialized in this function [-Wmaybe-uninitialized]
> ((long)((b) - (a)) < 0))
> ^
> drivers/i2c//busses/i2c-aspeed.c:607:16: note: 'timeout' was declared here
> unsigned long timeout;
> ^~~~~~~
>
> vim +/timeout +108 include/linux/jiffies.h
>
> ^1da177e Linus Torvalds 2005-04-16 91
> ^1da177e Linus Torvalds 2005-04-16 92 /*
> ^1da177e Linus Torvalds 2005-04-16 93 * These inlines deal with timer wrapping correctly. You are
> ^1da177e Linus Torvalds 2005-04-16 94 * strongly encouraged to use them
> ^1da177e Linus Torvalds 2005-04-16 95 * 1. Because people otherwise forget
> ^1da177e Linus Torvalds 2005-04-16 96 * 2. Because if the timer wrap changes in future you won't have to
> ^1da177e Linus Torvalds 2005-04-16 97 * alter your driver code.
> ^1da177e Linus Torvalds 2005-04-16 98 *
> ^1da177e Linus Torvalds 2005-04-16 99 * time_after(a,b) returns true if the time a is after time b.
> ^1da177e Linus Torvalds 2005-04-16 100 *
> ^1da177e Linus Torvalds 2005-04-16 101 * Do this with "<0" and ">=0" to only test the sign of the result. A
> ^1da177e Linus Torvalds 2005-04-16 102 * good compiler would generate better code (and a really good compiler
> ^1da177e Linus Torvalds 2005-04-16 103 * wouldn't care). Gcc is currently neither.
> ^1da177e Linus Torvalds 2005-04-16 104 */
> ^1da177e Linus Torvalds 2005-04-16 105 #define time_after(a,b) \
> ^1da177e Linus Torvalds 2005-04-16 106 (typecheck(unsigned long, a) && \
> ^1da177e Linus Torvalds 2005-04-16 107 typecheck(unsigned long, b) && \
> 5a581b36 Paul E. McKenney 2013-07-27 @108 ((long)((b) - (a)) < 0))
> ^1da177e Linus Torvalds 2005-04-16 109 #define time_before(a,b) time_after(b,a)
> ^1da177e Linus Torvalds 2005-04-16 110
>
> :::::: The code at line 108 was first introduced by commit
> :::::: 5a581b367b5df0531265311fc681c2abd377e5e6 jiffies: Avoid undefined behavior from signed overflow
>
> :::::: TO: Paul E. McKenney <[email protected]>
> :::::: CC: Paul E. McKenney <[email protected]>
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
>
This is a false warning.
The 'timeout' local variable will be initialized properly if it's gonna
be used when the multi-master property is set.
-Jae
Hi Jae,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on wsa/i2c/for-next]
[also build test WARNING on v4.19 next-20181101]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Jae-Hyun-Yoo/dt-bindings-i2c-Add-bus-timeout-ms-and-retries-properties-as-common-optional/20181031-051152
base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: i386-allyesconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
All warnings (new ones prefixed by >>):
drivers/i2c/busses/i2c-aspeed.c: In function 'aspeed_i2c_master_xfer':
>> drivers/i2c/busses/i2c-aspeed.c:624:210: warning: 'timeout' may be used uninitialized in this function [-Wmaybe-uninitialized]
if (time_after(jiffies, timeout))
^
drivers/i2c/busses/i2c-aspeed.c:607:16: note: 'timeout' was declared here
unsigned long timeout;
^~~~~~~
vim +/timeout +624 drivers/i2c/busses/i2c-aspeed.c
604
605 static int aspeed_i2c_check_bus_busy(struct aspeed_i2c_bus *bus)
606 {
607 unsigned long timeout;
608
609 if (bus->multi_master) {
610 might_sleep();
611 /* Initialize it only when multi_master is set */
612 timeout = jiffies + bus->adap.timeout;
613 }
614
615 for (;;) {
616 if (!(readl(bus->base + ASPEED_I2C_CMD_REG) &
617 ASPEED_I2CD_BUS_BUSY_STS))
618 #if IS_ENABLED(CONFIG_I2C_SLAVE)
619 if (bus->slave_state == ASPEED_I2C_SLAVE_STOP)
620 #endif
621 return 0;
622 if (!bus->multi_master)
623 break;
> 624 if (time_after(jiffies, timeout))
625 break;
626 usleep_range((ASPEED_I2C_BUS_BUSY_CHECK_INTERVAL_US >> 2) + 1,
627 ASPEED_I2C_BUS_BUSY_CHECK_INTERVAL_US);
628 }
629
630 return aspeed_i2c_recover_bus(bus);
631 }
632
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 11/1/2018 10:44 AM, kbuild test robot wrote:
> Hi Jae,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on wsa/i2c/for-next]
> [also build test WARNING on v4.19 next-20181101]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url: https://github.com/0day-ci/linux/commits/Jae-Hyun-Yoo/dt-bindings-i2c-Add-bus-timeout-ms-and-retries-properties-as-common-optional/20181031-051152
> base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
> config: i386-allyesconfig (attached as .config)
> compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=i386
>
> Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
> http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
>
> All warnings (new ones prefixed by >>):
>
> drivers/i2c/busses/i2c-aspeed.c: In function 'aspeed_i2c_master_xfer':
>>> drivers/i2c/busses/i2c-aspeed.c:624:210: warning: 'timeout' may be used uninitialized in this function [-Wmaybe-uninitialized]
> if (time_after(jiffies, timeout))
> ^
> drivers/i2c/busses/i2c-aspeed.c:607:16: note: 'timeout' was declared here
> unsigned long timeout;
> ^~~~~~~
>
> vim +/timeout +624 drivers/i2c/busses/i2c-aspeed.c
>
> 604
> 605 static int aspeed_i2c_check_bus_busy(struct aspeed_i2c_bus *bus)
> 606 {
> 607 unsigned long timeout;
> 608
> 609 if (bus->multi_master) {
> 610 might_sleep();
> 611 /* Initialize it only when multi_master is set */
> 612 timeout = jiffies + bus->adap.timeout;
The 'timeout' variable will be initialized only when bus->multi_master
is true to save some OPs in case this variable isn't used. The saved OPs
are not expensive but it makes a meaning because this function is called
very frequently on every master transfer.
> 613 }
> 614
> 615 for (;;) {
> 616 if (!(readl(bus->base + ASPEED_I2C_CMD_REG) &
> 617 ASPEED_I2CD_BUS_BUSY_STS))
> 618 #if IS_ENABLED(CONFIG_I2C_SLAVE)
> 619 if (bus->slave_state == ASPEED_I2C_SLAVE_STOP)
> 620 #endif
> 621 return 0;
> 622 if (!bus->multi_master)
> 623 break;
If bus->multi_master is false then it doesn't fall through so the
'timeout' variable in below will not be used when itself is
uninitialized. Also, bus->multi_master is fixed at probing time and it
doesn't change at runtime so the warning case never happens.
-Jae
> > 624 if (time_after(jiffies, timeout))
> 625 break;
> 626 usleep_range((ASPEED_I2C_BUS_BUSY_CHECK_INTERVAL_US >> 2) + 1,
> 627 ASPEED_I2C_BUS_BUSY_CHECK_INTERVAL_US);
> 628 }
> 629
> 630 return aspeed_i2c_recover_bus(bus);
> 631 }
> 632
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
>
On Tue, Oct 30, 2018 at 02:09:16PM -0700, Jae Hyun Yoo wrote:
> In multi-master environment, this driver's master cannot know
> exactly when a peer master sends data to this driver's slave so a
> case can be happened that this master tries to send data through
> the master_xfer function but slave data from peer master is still
> being processed by this driver.
So, that I get this correct: your IP core cannot detect a bus busy state
when its own slave address is acessed? Well, I know HW can have bugs,
but I still wonder because a bus is busy as soon as another START has
been detected, independent of which device is going to be accessed.
Even if so, why can't you use a mutex to prevent any master_xfer while
slave_xfer is ongoing?
Shouldn't that be enough according to the above description? Why do you
need the delay? I might be missing something...
> To prevent any state corruption in the case, this patch adds
> checking code if any slave operation is ongoing and it waits up to
> the bus timeout duration before starting a master_xfer operation.
>
> Signed-off-by: Jae Hyun Yoo <[email protected]>
> Reviewed-by: Brendan Higgins <[email protected]>
> ---
> drivers/i2c/busses/i2c-aspeed.c | 55 ++++++++++++++++++++++++---------
> 1 file changed, 40 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index 833b6b6a4c7e..30c3ab3a4844 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -12,6 +12,7 @@
>
> #include <linux/clk.h>
> #include <linux/completion.h>
> +#include <linux/delay.h>
> #include <linux/err.h>
> #include <linux/errno.h>
> #include <linux/i2c.h>
> @@ -115,6 +116,9 @@
> /* 0x18 : I2CD Slave Device Address Register */
> #define ASPEED_I2CD_DEV_ADDR_MASK GENMASK(6, 0)
>
> +/* Busy checking */
> +#define ASPEED_I2C_BUS_BUSY_CHECK_INTERVAL_US (10 * 1000)
> +
> enum aspeed_i2c_master_state {
> ASPEED_I2C_MASTER_INACTIVE,
> ASPEED_I2C_MASTER_START,
> @@ -156,6 +160,8 @@ struct aspeed_i2c_bus {
> int cmd_err;
> /* Protected only by i2c_lock_bus */
> int master_xfer_result;
> + /* Multi-master */
> + bool multi_master;
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> struct i2c_client *slave;
> enum aspeed_i2c_slave_state slave_state;
> @@ -596,27 +602,44 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
> }
>
> +static int aspeed_i2c_check_bus_busy(struct aspeed_i2c_bus *bus)
> +{
> + unsigned long timeout;
> +
> + if (bus->multi_master) {
> + might_sleep();
> + /* Initialize it only when multi_master is set */
> + timeout = jiffies + bus->adap.timeout;
> + }
> +
> + for (;;) {
> + if (!(readl(bus->base + ASPEED_I2C_CMD_REG) &
> + ASPEED_I2CD_BUS_BUSY_STS))
> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
> + if (bus->slave_state == ASPEED_I2C_SLAVE_STOP)
> +#endif
> + return 0;
> + if (!bus->multi_master)
> + break;
> + if (time_after(jiffies, timeout))
> + break;
> + usleep_range((ASPEED_I2C_BUS_BUSY_CHECK_INTERVAL_US >> 2) + 1,
> + ASPEED_I2C_BUS_BUSY_CHECK_INTERVAL_US);
> + }
> +
> + return aspeed_i2c_recover_bus(bus);
> +}
> +
> static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
> struct i2c_msg *msgs, int num)
> {
> struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);
> unsigned long time_left, flags;
> - int ret = 0;
>
> - spin_lock_irqsave(&bus->lock, flags);
> - bus->cmd_err = 0;
> -
> - /* If bus is busy, attempt recovery. We assume a single master
> - * environment.
> - */
> - if (readl(bus->base + ASPEED_I2C_CMD_REG) & ASPEED_I2CD_BUS_BUSY_STS) {
> - spin_unlock_irqrestore(&bus->lock, flags);
> - ret = aspeed_i2c_recover_bus(bus);
> - if (ret)
> - return ret;
> - spin_lock_irqsave(&bus->lock, flags);
> - }
> + if (aspeed_i2c_check_bus_busy(bus))
> + return -EAGAIN;
>
> + spin_lock_irqsave(&bus->lock, flags);
> bus->cmd_err = 0;
> bus->msgs = msgs;
> bus->msgs_index = 0;
> @@ -827,7 +850,9 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus *bus,
> if (ret < 0)
> return ret;
>
> - if (!of_property_read_bool(pdev->dev.of_node, "multi-master"))
> + if (of_property_read_bool(pdev->dev.of_node, "multi-master"))
> + bus->multi_master = true;
> + else
> fun_ctrl_reg |= ASPEED_I2CD_MULTI_MASTER_DIS;
>
> /* Enable Master Mode */
> --
> 2.19.1
>
On Tue, Oct 30, 2018 at 02:09:12PM -0700, Jae Hyun Yoo wrote:
> This commit adds 'bus-timeout-ms' and '#retries' properties as
> common optional properties that can be used for setting 'timeout'
> and 'retries' values of 'struct i2c_adapter'. With this patch, the
> bus timeout value and the master transfer retries count can be set
> through these properties at the registration time of an adapter.
> Still the values can be set by I2C_TIMEOUT and I2C_RETRIES ioctls
> on cdev at runtime too.
>
> These properties may not be supported by all drivers. However, if
> a driver wants to support one of them, it should adapt the
> bindings in this document.
>
> Signed-off-by: Jae Hyun Yoo <[email protected]>
> Reviewed-by: Rob Herring <[email protected]>
Definately NACK on the #retries part. This is purely a configuration
thing. Besides that it is kind of ancient cruft, too. I don't recommend
it, at all.
On the timeout thing, I am still not fully convinced that this is a HW
description. I said that before. But maybe I need to understand your
problem case better.
> ---
> Documentation/devicetree/bindings/i2c/i2c.txt | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt
> index 11263982470e..bdead91f82a4 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c.txt
> @@ -80,6 +80,12 @@ wants to support one of the below features, it should adapt the bindings below.
> Names of map programmable addresses.
> It can contain any map needing another address than default one.
>
> +- bus-timeout-ms
> + Bus timeout in milliseconds.
> +
> +- #retries
> + Number of retries for master transfer.
> +
> Binding may contain optional "interrupts" property, describing interrupts
> used by the device. I2C core will assign "irq" interrupt (or the very first
> interrupt if not using interrupt names) as primary interrupt for the slave.
> --
> 2.19.1
>
On 11/28/2018 3:54 PM, Wolfram Sang wrote:
> On Tue, Oct 30, 2018 at 02:09:16PM -0700, Jae Hyun Yoo wrote:
>> In multi-master environment, this driver's master cannot know
>> exactly when a peer master sends data to this driver's slave so a
>> case can be happened that this master tries to send data through
>> the master_xfer function but slave data from peer master is still
>> being processed by this driver.
>
> So, that I get this correct: your IP core cannot detect a bus busy state
> when its own slave address is acessed? Well, I know HW can have bugs,
> but I still wonder because a bus is busy as soon as another START has
> been detected, independent of which device is going to be accessed.
>
> Even if so, why can't you use a mutex to prevent any master_xfer while
> slave_xfer is ongoing?
>
> Shouldn't that be enough according to the above description? Why do you
> need the delay? I might be missing something...
>
This IP has a bus busy state bit which can cover master and slave
operations but in case of slave operation, the bit is cleared little
bit earlier than its actual completion so it's the reason why I made
this patch. I agree with you that it still has a weak point since
the bus can be changed to busy state immediately just after finishing
the busy waiting delay.
As you suggested, I'll try to implement it using a mutex instead of
using delay. Thanks a lot for your comment.
Jae
>> To prevent any state corruption in the case, this patch adds
>> checking code if any slave operation is ongoing and it waits up to
>> the bus timeout duration before starting a master_xfer operation.
>>
>> Signed-off-by: Jae Hyun Yoo <[email protected]>
>> Reviewed-by: Brendan Higgins <[email protected]>
>> ---
>> drivers/i2c/busses/i2c-aspeed.c | 55 ++++++++++++++++++++++++---------
>> 1 file changed, 40 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>> index 833b6b6a4c7e..30c3ab3a4844 100644
>> --- a/drivers/i2c/busses/i2c-aspeed.c
>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>> @@ -12,6 +12,7 @@
>>
>> #include <linux/clk.h>
>> #include <linux/completion.h>
>> +#include <linux/delay.h>
>> #include <linux/err.h>
>> #include <linux/errno.h>
>> #include <linux/i2c.h>
>> @@ -115,6 +116,9 @@
>> /* 0x18 : I2CD Slave Device Address Register */
>> #define ASPEED_I2CD_DEV_ADDR_MASK GENMASK(6, 0)
>>
>> +/* Busy checking */
>> +#define ASPEED_I2C_BUS_BUSY_CHECK_INTERVAL_US (10 * 1000)
>> +
>> enum aspeed_i2c_master_state {
>> ASPEED_I2C_MASTER_INACTIVE,
>> ASPEED_I2C_MASTER_START,
>> @@ -156,6 +160,8 @@ struct aspeed_i2c_bus {
>> int cmd_err;
>> /* Protected only by i2c_lock_bus */
>> int master_xfer_result;
>> + /* Multi-master */
>> + bool multi_master;
>> #if IS_ENABLED(CONFIG_I2C_SLAVE)
>> struct i2c_client *slave;
>> enum aspeed_i2c_slave_state slave_state;
>> @@ -596,27 +602,44 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>> return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>> }
>>
>> +static int aspeed_i2c_check_bus_busy(struct aspeed_i2c_bus *bus)
>> +{
>> + unsigned long timeout;
>> +
>> + if (bus->multi_master) {
>> + might_sleep();
>> + /* Initialize it only when multi_master is set */
>> + timeout = jiffies + bus->adap.timeout;
>> + }
>> +
>> + for (;;) {
>> + if (!(readl(bus->base + ASPEED_I2C_CMD_REG) &
>> + ASPEED_I2CD_BUS_BUSY_STS))
>> +#if IS_ENABLED(CONFIG_I2C_SLAVE)
>> + if (bus->slave_state == ASPEED_I2C_SLAVE_STOP)
>> +#endif
>> + return 0;
>> + if (!bus->multi_master)
>> + break;
>> + if (time_after(jiffies, timeout))
>> + break;
>> + usleep_range((ASPEED_I2C_BUS_BUSY_CHECK_INTERVAL_US >> 2) + 1,
>> + ASPEED_I2C_BUS_BUSY_CHECK_INTERVAL_US);
>> + }
>> +
>> + return aspeed_i2c_recover_bus(bus);
>> +}
>> +
>> static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
>> struct i2c_msg *msgs, int num)
>> {
>> struct aspeed_i2c_bus *bus = i2c_get_adapdata(adap);
>> unsigned long time_left, flags;
>> - int ret = 0;
>>
>> - spin_lock_irqsave(&bus->lock, flags);
>> - bus->cmd_err = 0;
>> -
>> - /* If bus is busy, attempt recovery. We assume a single master
>> - * environment.
>> - */
>> - if (readl(bus->base + ASPEED_I2C_CMD_REG) & ASPEED_I2CD_BUS_BUSY_STS) {
>> - spin_unlock_irqrestore(&bus->lock, flags);
>> - ret = aspeed_i2c_recover_bus(bus);
>> - if (ret)
>> - return ret;
>> - spin_lock_irqsave(&bus->lock, flags);
>> - }
>> + if (aspeed_i2c_check_bus_busy(bus))
>> + return -EAGAIN;
>>
>> + spin_lock_irqsave(&bus->lock, flags);
>> bus->cmd_err = 0;
>> bus->msgs = msgs;
>> bus->msgs_index = 0;
>> @@ -827,7 +850,9 @@ static int aspeed_i2c_init(struct aspeed_i2c_bus *bus,
>> if (ret < 0)
>> return ret;
>>
>> - if (!of_property_read_bool(pdev->dev.of_node, "multi-master"))
>> + if (of_property_read_bool(pdev->dev.of_node, "multi-master"))
>> + bus->multi_master = true;
>> + else
>> fun_ctrl_reg |= ASPEED_I2CD_MULTI_MASTER_DIS;
>>
>> /* Enable Master Mode */
>> --
>> 2.19.1
>>
On 11/28/2018 3:54 PM, Wolfram Sang wrote:
> On Tue, Oct 30, 2018 at 02:09:12PM -0700, Jae Hyun Yoo wrote:
>> This commit adds 'bus-timeout-ms' and '#retries' properties as
>> common optional properties that can be used for setting 'timeout'
>> and 'retries' values of 'struct i2c_adapter'. With this patch, the
>> bus timeout value and the master transfer retries count can be set
>> through these properties at the registration time of an adapter.
>> Still the values can be set by I2C_TIMEOUT and I2C_RETRIES ioctls
>> on cdev at runtime too.
>>
>> These properties may not be supported by all drivers. However, if
>> a driver wants to support one of them, it should adapt the
>> bindings in this document.
>>
>> Signed-off-by: Jae Hyun Yoo <[email protected]>
>> Reviewed-by: Rob Herring <[email protected]>
>
> Definately NACK on the #retries part. This is purely a configuration
> thing. Besides that it is kind of ancient cruft, too. I don't recommend
> it, at all.
>
> On the timeout thing, I am still not fully convinced that this is a HW
> description. I said that before. But maybe I need to understand your
> problem case better.
>
Okay, I got it. I'll leave those as configuration things. Will remove
this part in the next version.
Thanks for your comment.
Jae
On Tue, Oct 30, 2018 at 02:09:11PM -0700, Jae Hyun Yoo wrote:
> In multi-master environment, this driver's master cannot know
> exactly when a peer master sends data to this driver's slave so a
> case can be happened that this master tries to send data through
> the master_xfer function but slave data from peer master is still
> being processed by this driver. To prevent state corruption in the
> case, this patch adds checking code if any slave operation is
> ongoing and it waits up to the bus timeout duration before starting
> a master_xfer operation.
>
> To support this change, it introduces changes on i2c-core-base to
> make that able to read the bus timeout and master transfer retries
> count values from device tree properties.
>
> Please review this patch set.
Marking this as "Changes requested". I think if the mutex approach
works, these patches become obsolete then.
On 1/15/2019 2:47 PM, Wolfram Sang wrote:
> On Tue, Oct 30, 2018 at 02:09:11PM -0700, Jae Hyun Yoo wrote:
>> In multi-master environment, this driver's master cannot know
>> exactly when a peer master sends data to this driver's slave so a
>> case can be happened that this master tries to send data through
>> the master_xfer function but slave data from peer master is still
>> being processed by this driver. To prevent state corruption in the
>> case, this patch adds checking code if any slave operation is
>> ongoing and it waits up to the bus timeout duration before starting
>> a master_xfer operation.
>>
>> To support this change, it introduces changes on i2c-core-base to
>> make that able to read the bus timeout and master transfer retries
>> count values from device tree properties.
>>
>> Please review this patch set.
>
> Marking this as "Changes requested". I think if the mutex approach
> works, these patches become obsolete then.
>
Hi Wolfram,
Thanks for your touching base. I made and tested a new patch in my
local. Actually, I couldn't use a mutex because it should provide
locking between driver context and interrupt context. Instead, after
taking lots of experiments, I checked that idle waiting can be supported
by this H/W so I didn't need to use any locking method in the driver
code for multi-master handling. I'll submit a new patch tomorrow using a
different thread. Please drop this patch set.
Thanks,
Jae