LinuxLists.cc - [PATCH v2] i2c: aspeed: Update the stop sw state when the bus recovery occurs

2024-06-08 04:42:28

Subject: [PATCH v2] i2c: aspeed: Update the stop sw state when the bus recovery occurs

When the i2c bus recovery occurs, driver will send i2c stop command
in the scl low condition. In this case the sw state will still keep
original situation. Under multi-master usage, i2c bus recovery will
be called when i2c transfer timeout occurs. Update the stop command
calling with aspeed_i2c_do_stop function to update master_state.

Fixes: f327c686d3ba ("i2c: aspeed: added driver for Aspeed I2C")

Cc: <[email protected]> # v4.13+
Signed-off-by: Tommy Huang <[email protected]>
---
drivers/i2c/busses/i2c-aspeed.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index ce8c4846b7fa..be64e419adf0 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -25,6 +25,8 @@
#include <linux/reset.h>
#include <linux/slab.h>

+static void aspeed_i2c_do_stop(struct aspeed_i2c_bus *bus);
+
/* I2C Register */
#define ASPEED_I2C_FUN_CTRL_REG 0x00
#define ASPEED_I2C_AC_TIMING_REG1 0x04
@@ -187,7 +189,7 @@ static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus)
command);

reinit_completion(&bus->cmd_complete);
- writel(ASPEED_I2CD_M_STOP_CMD, bus->base + ASPEED_I2C_CMD_REG);
+ aspeed_i2c_do_stop(bus);
spin_unlock_irqrestore(&bus->lock, flags);

time_left = wait_for_completion_timeout(
--
2.25.1

2024-06-08 16:20:23

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v2] i2c: aspeed: Update the stop sw state when the bus recovery occurs

2024-06-08 17:04:26

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v2] i2c: aspeed: Update the stop sw state when the bus recovery occurs

Hi Tommy,

kernel test robot noticed the following build errors:

[auto build test ERROR on andi-shyti/i2c/i2c-host]
[also build test ERROR on linus/master v6.10-rc2 next-20240607]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Tommy-Huang/i2c-aspeed-Update-the-stop-sw-state-when-the-bus-recovery-occurs/20240608-124429
base: git://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
patch link: https://lore.kernel.org/r/20240608043653.4086647-1-tommy_huang%40aspeedtech.com
patch subject: [PATCH v2] i2c: aspeed: Update the stop sw state when the bus recovery occurs
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240609/[email protected]/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project d7d2d4f53fc79b4b58e8d8d08151b577c3699d4a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240609/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All error/warnings (new ones prefixed by >>):

In file included from drivers/i2c/busses/i2c-aspeed.c:14:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
548 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
561 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
| ^
In file included from drivers/i2c/busses/i2c-aspeed.c:14:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
574 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
| ^
In file included from drivers/i2c/busses/i2c-aspeed.c:14:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
585 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
595 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
605 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
In file included from drivers/i2c/busses/i2c-aspeed.c:14:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:21:
In file included from include/linux/mm.h:2253:
include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
514 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
>> drivers/i2c/busses/i2c-aspeed.c:28:39: warning: declaration of 'struct aspeed_i2c_bus' will not be visible outside of this function [-Wvisibility]
28 | static void aspeed_i2c_do_stop(struct aspeed_i2c_bus *bus);
| ^
>> drivers/i2c/busses/i2c-aspeed.c:192:22: error: incompatible pointer types passing 'struct aspeed_i2c_bus *' to parameter of type 'struct aspeed_i2c_bus *' [-Werror,-Wincompatible-pointer-types]
192 | aspeed_i2c_do_stop(bus);
| ^~~
drivers/i2c/busses/i2c-aspeed.c:28:55: note: passing argument to parameter 'bus' here
28 | static void aspeed_i2c_do_stop(struct aspeed_i2c_bus *bus);
| ^
>> drivers/i2c/busses/i2c-aspeed.c:396:13: error: conflicting types for 'aspeed_i2c_do_stop'
396 | static void aspeed_i2c_do_stop(struct aspeed_i2c_bus *bus)
| ^
drivers/i2c/busses/i2c-aspeed.c:28:13: note: previous declaration is here
28 | static void aspeed_i2c_do_stop(struct aspeed_i2c_bus *bus);
| ^
drivers/i2c/busses/i2c-aspeed.c:409:22: error: incompatible pointer types passing 'struct aspeed_i2c_bus *' to parameter of type 'struct aspeed_i2c_bus *' [-Werror,-Wincompatible-pointer-types]
409 | aspeed_i2c_do_stop(bus);
| ^~~
drivers/i2c/busses/i2c-aspeed.c:28:55: note: passing argument to parameter 'bus' here
28 | static void aspeed_i2c_do_stop(struct aspeed_i2c_bus *bus);
| ^
drivers/i2c/busses/i2c-aspeed.c:469:23: error: incompatible pointer types passing 'struct aspeed_i2c_bus *' to parameter of type 'struct aspeed_i2c_bus *' [-Werror,-Wincompatible-pointer-types]
469 | aspeed_i2c_do_stop(bus);
| ^~~
drivers/i2c/busses/i2c-aspeed.c:28:55: note: passing argument to parameter 'bus' here
28 | static void aspeed_i2c_do_stop(struct aspeed_i2c_bus *bus);
| ^
drivers/i2c/busses/i2c-aspeed.c:507:23: error: incompatible pointer types passing 'struct aspeed_i2c_bus *' to parameter of type 'struct aspeed_i2c_bus *' [-Werror,-Wincompatible-pointer-types]
507 | aspeed_i2c_do_stop(bus);
| ^~~
drivers/i2c/busses/i2c-aspeed.c:28:55: note: passing argument to parameter 'bus' here
28 | static void aspeed_i2c_do_stop(struct aspeed_i2c_bus *bus);
| ^
drivers/i2c/busses/i2c-aspeed.c:512:23: error: incompatible pointer types passing 'struct aspeed_i2c_bus *' to parameter of type 'struct aspeed_i2c_bus *' [-Werror,-Wincompatible-pointer-types]
512 | aspeed_i2c_do_stop(bus);
| ^~~
drivers/i2c/busses/i2c-aspeed.c:28:55: note: passing argument to parameter 'bus' here
28 | static void aspeed_i2c_do_stop(struct aspeed_i2c_bus *bus);
| ^
drivers/i2c/busses/i2c-aspeed.c:562:24: error: incompatible pointer types passing 'struct aspeed_i2c_bus *' to parameter of type 'struct aspeed_i2c_bus *' [-Werror,-Wincompatible-pointer-types]
562 | aspeed_i2c_do_stop(bus);
| ^~~
drivers/i2c/busses/i2c-aspeed.c:28:55: note: passing argument to parameter 'bus' here
28 | static void aspeed_i2c_do_stop(struct aspeed_i2c_bus *bus);
| ^
drivers/i2c/busses/i2c-aspeed.c:608:21: error: incompatible pointer types passing 'struct aspeed_i2c_bus *' to parameter of type 'struct aspeed_i2c_bus *' [-Werror,-Wincompatible-pointer-types]
608 | aspeed_i2c_do_stop(bus);
| ^~~
drivers/i2c/busses/i2c-aspeed.c:28:55: note: passing argument to parameter 'bus' here
28 | static void aspeed_i2c_do_stop(struct aspeed_i2c_bus *bus);
| ^
8 warnings and 8 errors generated.

vim +192 drivers/i2c/busses/i2c-aspeed.c

27
> 28 static void aspeed_i2c_do_stop(struct aspeed_i2c_bus *bus);
29
30 /* I2C Register */
31 #define ASPEED_I2C_FUN_CTRL_REG 0x00
32 #define ASPEED_I2C_AC_TIMING_REG1 0x04
33 #define ASPEED_I2C_AC_TIMING_REG2 0x08
34 #define ASPEED_I2C_INTR_CTRL_REG 0x0c
35 #define ASPEED_I2C_INTR_STS_REG 0x10
36 #define ASPEED_I2C_CMD_REG 0x14
37 #define ASPEED_I2C_DEV_ADDR_REG 0x18
38 #define ASPEED_I2C_BYTE_BUF_REG 0x20
39
40 /* Global Register Definition */
41 /* 0x00 : I2C Interrupt Status Register */
42 /* 0x08 : I2C Interrupt Target Assignment */
43
44 /* Device Register Definition */
45 /* 0x00 : I2CD Function Control Register */
46 #define ASPEED_I2CD_MULTI_MASTER_DIS BIT(15)
47 #define ASPEED_I2CD_SDA_DRIVE_1T_EN BIT(8)
48 #define ASPEED_I2CD_M_SDA_DRIVE_1T_EN BIT(7)
49 #define ASPEED_I2CD_M_HIGH_SPEED_EN BIT(6)
50 #define ASPEED_I2CD_SLAVE_EN BIT(1)
51 #define ASPEED_I2CD_MASTER_EN BIT(0)
52
53 /* 0x04 : I2CD Clock and AC Timing Control Register #1 */
54 #define ASPEED_I2CD_TIME_TBUF_MASK GENMASK(31, 28)
55 #define ASPEED_I2CD_TIME_THDSTA_MASK GENMASK(27, 24)
56 #define ASPEED_I2CD_TIME_TACST_MASK GENMASK(23, 20)
57 #define ASPEED_I2CD_TIME_SCL_HIGH_SHIFT 16
58 #define ASPEED_I2CD_TIME_SCL_HIGH_MASK GENMASK(19, 16)
59 #define ASPEED_I2CD_TIME_SCL_LOW_SHIFT 12
60 #define ASPEED_I2CD_TIME_SCL_LOW_MASK GENMASK(15, 12)
61 #define ASPEED_I2CD_TIME_BASE_DIVISOR_MASK GENMASK(3, 0)
62 #define ASPEED_I2CD_TIME_SCL_REG_MAX GENMASK(3, 0)
63 /* 0x08 : I2CD Clock and AC Timing Control Register #2 */
64 #define ASPEED_NO_TIMEOUT_CTRL 0
65
66 /* 0x0c : I2CD Interrupt Control Register &
67 * 0x10 : I2CD Interrupt Status Register
68 *
69 * These share bit definitions, so use the same values for the enable &
70 * status bits.
71 */
72 #define ASPEED_I2CD_INTR_RECV_MASK 0xf000ffff
73 #define ASPEED_I2CD_INTR_SDA_DL_TIMEOUT BIT(14)
74 #define ASPEED_I2CD_INTR_BUS_RECOVER_DONE BIT(13)
75 #define ASPEED_I2CD_INTR_SLAVE_MATCH BIT(7)
76 #define ASPEED_I2CD_INTR_SCL_TIMEOUT BIT(6)
77 #define ASPEED_I2CD_INTR_ABNORMAL BIT(5)
78 #define ASPEED_I2CD_INTR_NORMAL_STOP BIT(4)
79 #define ASPEED_I2CD_INTR_ARBIT_LOSS BIT(3)
80 #define ASPEED_I2CD_INTR_RX_DONE BIT(2)
81 #define ASPEED_I2CD_INTR_TX_NAK BIT(1)
82 #define ASPEED_I2CD_INTR_TX_ACK BIT(0)
83 #define ASPEED_I2CD_INTR_MASTER_ERRORS \
84 (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT | \
85 ASPEED_I2CD_INTR_SCL_TIMEOUT | \
86 ASPEED_I2CD_INTR_ABNORMAL | \
87 ASPEED_I2CD_INTR_ARBIT_LOSS)
88 #define ASPEED_I2CD_INTR_ALL \
89 (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT | \
90 ASPEED_I2CD_INTR_BUS_RECOVER_DONE | \
91 ASPEED_I2CD_INTR_SCL_TIMEOUT | \
92 ASPEED_I2CD_INTR_ABNORMAL | \
93 ASPEED_I2CD_INTR_NORMAL_STOP | \
94 ASPEED_I2CD_INTR_ARBIT_LOSS | \
95 ASPEED_I2CD_INTR_RX_DONE | \
96 ASPEED_I2CD_INTR_TX_NAK | \
97 ASPEED_I2CD_INTR_TX_ACK)
98
99 /* 0x14 : I2CD Command/Status Register */
100 #define ASPEED_I2CD_SCL_LINE_STS BIT(18)
101 #define ASPEED_I2CD_SDA_LINE_STS BIT(17)
102 #define ASPEED_I2CD_BUS_BUSY_STS BIT(16)
103 #define ASPEED_I2CD_BUS_RECOVER_CMD BIT(11)
104
105 /* Command Bit */
106 #define ASPEED_I2CD_M_STOP_CMD BIT(5)
107 #define ASPEED_I2CD_M_S_RX_CMD_LAST BIT(4)
108 #define ASPEED_I2CD_M_RX_CMD BIT(3)
109 #define ASPEED_I2CD_S_TX_CMD BIT(2)
110 #define ASPEED_I2CD_M_TX_CMD BIT(1)
111 #define ASPEED_I2CD_M_START_CMD BIT(0)
112 #define ASPEED_I2CD_MASTER_CMDS_MASK \
113 (ASPEED_I2CD_M_STOP_CMD | \
114 ASPEED_I2CD_M_S_RX_CMD_LAST | \
115 ASPEED_I2CD_M_RX_CMD | \
116 ASPEED_I2CD_M_TX_CMD | \
117 ASPEED_I2CD_M_START_CMD)
118
119 /* 0x18 : I2CD Slave Device Address Register */
120 #define ASPEED_I2CD_DEV_ADDR_MASK GENMASK(6, 0)
121
122 enum aspeed_i2c_master_state {
123 ASPEED_I2C_MASTER_INACTIVE,
124 ASPEED_I2C_MASTER_PENDING,
125 ASPEED_I2C_MASTER_START,
126 ASPEED_I2C_MASTER_TX_FIRST,
127 ASPEED_I2C_MASTER_TX,
128 ASPEED_I2C_MASTER_RX_FIRST,
129 ASPEED_I2C_MASTER_RX,
130 ASPEED_I2C_MASTER_STOP,
131 };
132
133 enum aspeed_i2c_slave_state {
134 ASPEED_I2C_SLAVE_INACTIVE,
135 ASPEED_I2C_SLAVE_START,
136 ASPEED_I2C_SLAVE_READ_REQUESTED,
137 ASPEED_I2C_SLAVE_READ_PROCESSED,
138 ASPEED_I2C_SLAVE_WRITE_REQUESTED,
139 ASPEED_I2C_SLAVE_WRITE_RECEIVED,
140 ASPEED_I2C_SLAVE_STOP,
141 };
142
143 struct aspeed_i2c_bus {
144 struct i2c_adapter adap;
145 struct device *dev;
146 void __iomem *base;
147 struct reset_control *rst;
148 /* Synchronizes I/O mem access to base. */
149 spinlock_t lock;
150 struct completion cmd_complete;
151 u32 (*get_clk_reg_val)(struct device *dev,
152 u32 divisor);
153 unsigned long parent_clk_frequency;
154 u32 bus_frequency;
155 /* Transaction state. */
156 enum aspeed_i2c_master_state master_state;
157 struct i2c_msg *msgs;
158 size_t buf_index;
159 size_t msgs_index;
160 size_t msgs_count;
161 bool send_stop;
162 int cmd_err;
163 /* Protected only by i2c_lock_bus */
164 int master_xfer_result;
165 /* Multi-master */
166 bool multi_master;
167 #if IS_ENABLED(CONFIG_I2C_SLAVE)
168 struct i2c_client *slave;
169 enum aspeed_i2c_slave_state slave_state;
170 #endif /* CONFIG_I2C_SLAVE */
171 };
172
173 static int aspeed_i2c_reset(struct aspeed_i2c_bus *bus);
174
175 static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus)
176 {
177 unsigned long time_left, flags;
178 int ret = 0;
179 u32 command;
180
181 spin_lock_irqsave(&bus->lock, flags);
182 command = readl(bus->base + ASPEED_I2C_CMD_REG);
183
184 if (command & ASPEED_I2CD_SDA_LINE_STS) {
185 /* Bus is idle: no recovery needed. */
186 if (command & ASPEED_I2CD_SCL_LINE_STS)
187 goto out;
188 dev_dbg(bus->dev, "SCL hung (state %x), attempting recovery\n",
189 command);
190
191 reinit_completion(&bus->cmd_complete);
> 192 aspeed_i2c_do_stop(bus);
193 spin_unlock_irqrestore(&bus->lock, flags);
194
195 time_left = wait_for_completion_timeout(
196 &bus->cmd_complete, bus->adap.timeout);
197
198 spin_lock_irqsave(&bus->lock, flags);
199 if (time_left == 0)
200 goto reset_out;
201 else if (bus->cmd_err)
202 goto reset_out;
203 /* Recovery failed. */
204 else if (!(readl(bus->base + ASPEED_I2C_CMD_REG) &
205 ASPEED_I2CD_SCL_LINE_STS))
206 goto reset_out;
207 /* Bus error. */
208 } else {
209 dev_dbg(bus->dev, "SDA hung (state %x), attempting recovery\n",
210 command);
211
212 reinit_completion(&bus->cmd_complete);
213 /* Writes 1 to 8 SCL clock cycles until SDA is released. */
214 writel(ASPEED_I2CD_BUS_RECOVER_CMD,
215 bus->base + ASPEED_I2C_CMD_REG);
216 spin_unlock_irqrestore(&bus->lock, flags);
217
218 time_left = wait_for_completion_timeout(
219 &bus->cmd_complete, bus->adap.timeout);
220
221 spin_lock_irqsave(&bus->lock, flags);
222 if (time_left == 0)
223 goto reset_out;
224 else if (bus->cmd_err)
225 goto reset_out;
226 /* Recovery failed. */
227 else if (!(readl(bus->base + ASPEED_I2C_CMD_REG) &
228 ASPEED_I2CD_SDA_LINE_STS))
229 goto reset_out;
230 }
231
232 out:
233 spin_unlock_irqrestore(&bus->lock, flags);
234
235 return ret;
236
237 reset_out:
238 spin_unlock_irqrestore(&bus->lock, flags);
239
240 return aspeed_i2c_reset(bus);
241 }
242

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki