| Message ID | 20230810072155.3726352-1-zhangjian.3032@bytedance.com |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | i2c: aspeed: Fix i2c bus hang in slave read | expand |
On Thu, 10 Aug 2023 at 07:22, Jian Zhang <zhangjian.3032@bytedance.com> wrote: > > When the `CONFIG_I2C_SLAVE` option is enabled and the device operates > as a slave, a situation arises where the master sends a START signal > without the accompanying STOP signal. This action results in a > persistent I2C bus timeout. The core issue stems from the fact that > the i2c controller remains in a slave read state without a timeout > mechanism. As a consequence, the bus perpetually experiences timeouts. > > This proposed patch addresses this problem by introducing a status > check during i2c transmit timeouts. In the event that the controller > is in a slave read state, the i2c controller will be reset to restore > proper functionality and allow the I2C bus to resume normal operation. > > Signed-off-by: Jian Zhang <zhangjian.3032@bytedance.com> Tommy has submitted a similar fix: https://lore.kernel.org/linux-i2c/20230906004910.4157305-1-tommy_huang@aspeedtech.com/ His change is very heavy handed; it reinitialises the bus including re-parsing the device tree (!). Should we have merged this fix instead? If not, are you able to confirm that his change fixes your issue? Cheers, Joel > --- > drivers/i2c/busses/i2c-aspeed.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c > index e76befe3f60f..1a95205fc946 100644 > --- a/drivers/i2c/busses/i2c-aspeed.c > +++ b/drivers/i2c/busses/i2c-aspeed.c > @@ -113,6 +113,7 @@ > ASPEED_I2CD_M_RX_CMD | \ > ASPEED_I2CD_M_TX_CMD | \ > ASPEED_I2CD_M_START_CMD) > +#define ASPEED_I2CD_SLAVE_CMDS_MASK GENMASK(31, 29) > > /* 0x18 : I2CD Slave Device Address Register */ > #define ASPEED_I2CD_DEV_ADDR_MASK GENMASK(6, 0) > @@ -706,6 +707,22 @@ static int aspeed_i2c_master_xfer(struct i2c_adapter *adap, > ASPEED_I2CD_BUS_BUSY_STS)) > aspeed_i2c_recover_bus(bus); > > +#if IS_ENABLED(CONFIG_I2C_SLAVE) > + /* > + * If master timed out and bus is in slave mode. > + * reset the slave mode. > + */ > + if (readl(bus->base + ASPEED_I2C_CMD_REG) & ASPEED_I2CD_SLAVE_CMDS_MASK) { > + spin_lock_irqsave(&bus->lock, flags); > + u32 func_ctrl_reg_val = readl(bus->base + ASPEED_I2C_FUN_CTRL_REG); > + > + writel(0, bus->base + ASPEED_I2C_FUN_CTRL_REG); > + writel(func_ctrl_reg_val, bus->base + ASPEED_I2C_FUN_CTRL_REG); > + bus->slave_state = ASPEED_I2C_SLAVE_INACTIVE; > + spin_unlock_irqrestore(&bus->lock, flags); > + } > +#endif > + > /* > * If timed out and the state is still pending, drop the pending > * master command. > -- > 2.30.2 >
> From: "Joel Stanley"<joel@jms.id.au> > Date: Fri, Sep 22, 2023, 14:48 > Subject: [External] Re: [PATCH] i2c: aspeed: Fix i2c bus hang in slave read > To: "Jian Zhang"<zhangjian.3032@bytedance.com>, "Tommy Huang"<tommy_huang@aspeedtech.com> > Cc: <brendan.higgins@linux.dev>, <benh@kernel.crashing.org>, <andrew@aj.id.au>, <zhangjian3032@gmail.com>, <yulei.sh@bytedance.com>, <xiexinnan@bytedance.com>, "open list:ARM/ASPEED I2C DRIVER"<linux-i2c@vger.kernel.org>, "moderated list:ARM/ASPEED I2C DRIVER"<openbmc@lists.ozlabs.org>, "moderated list:ARM/ASPEED MACHINE SUPPORT"<linux-arm-kernel@lists.infradead.org>, "moderated list:ARM/ASPEED MACHINE SUPPORT"<linux-aspeed@lists.ozlabs.org>, "open list"<linux-kernel@vger.kernel.org> > On Thu, 10 Aug 2023 at 07:22, Jian Zhang <zhangjian.3032@bytedance.com> wrote: > > > > When the `CONFIG_I2C_SLAVE` option is enabled and the device operates > > as a slave, a situation arises where the master sends a START signal > > without the accompanying STOP signal. This action results in a > > persistent I2C bus timeout. The core issue stems from the fact that > > the i2c controller remains in a slave read state without a timeout > > mechanism. As a consequence, the bus perpetually experiences timeouts. > > > > This proposed patch addresses this problem by introducing a status > > check during i2c transmit timeouts. In the event that the controller > > is in a slave read state, the i2c controller will be reset to restore > > proper functionality and allow the I2C bus to resume normal operation. > > > > Signed-off-by: Jian Zhang <zhangjian.3032@bytedance.com> > > Tommy has submitted a similar fix: > > https://lore.kernel.org/linux-i2c/20230906004910.4157305-1-tommy_huang@aspeedtech.com/ > > His change is very heavy handed; it reinitialises the bus including > re-parsing the device tree (!). > > Should we have merged this fix instead? If not, are you able to > confirm that his change fixes your issue? I feel it's for solving the same issue, but I think this patch is missing the action `bus->slave_state = ASPEED_I2C_SLAVE_INACTIVE;`, which means it can't resolve my problem. @Tommy, can you help confirm this? Thanks, Jian > > Cheers, > > Joel > > > --- > > drivers/i2c/busses/i2c-aspeed.c | 17 +++++++++++++++++ > > 1 file changed, 17 insertions(+) > > > > diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c > > index e76befe3f60f..1a95205fc946 100644 > > --- a/drivers/i2c/busses/i2c-aspeed.c > > +++ b/drivers/i2c/busses/i2c-aspeed.c > > @@ -113,6 +113,7 @@ > > ASPEED_I2CD_M_RX_CMD | \ > > ASPEED_I2CD_M_TX_CMD | \ > > ASPEED_I2CD_M_START_CMD) > > +#define ASPEED_I2CD_SLAVE_CMDS_MASK GENMASK(31, 29) > > > > /* 0x18 : I2CD Slave Device Address Register */ > > #define ASPEED_I2CD_DEV_ADDR_MASK GENMASK(6, 0) > > @@ -706,6 +707,22 @@ static int aspeed_i2c_master_xfer(struct i2c_adapter *adap, > > ASPEED_I2CD_BUS_BUSY_STS)) > > aspeed_i2c_recover_bus(bus); > > > > +#if IS_ENABLED(CONFIG_I2C_SLAVE) > > + /* > > + * If master timed out and bus is in slave mode. > > + * reset the slave mode. > > + */ > > + if (readl(bus->base + ASPEED_I2C_CMD_REG) & ASPEED_I2CD_SLAVE_CMDS_MASK) { > > + spin_lock_irqsave(&bus->lock, flags); > > + u32 func_ctrl_reg_val = readl(bus->base + ASPEED_I2C_FUN_CTRL_REG); > > + > > + writel(0, bus->base + ASPEED_I2C_FUN_CTRL_REG); > > + writel(func_ctrl_reg_val, bus->base + ASPEED_I2C_FUN_CTRL_REG); > > + bus->slave_state = ASPEED_I2C_SLAVE_INACTIVE; > > + spin_unlock_irqrestore(&bus->lock, flags); > > + } > > +#endif > > + > > /* > > * If timed out and the state is still pending, drop the pending > > * master command. > > -- > > 2.30.2 > >
On Fri, 22 Sept 2023 at 14:39, Jian Zhang <zhangjian.3032@bytedance.com> wrote: > > > > Tommy has submitted a similar fix: > > > > https://lore.kernel.org/linux-i2c/20230906004910.4157305-1-tommy_huang@aspeedtech.com/ > > > > His change is very heavy handed; it reinitialises the bus including > > re-parsing the device tree (!). > > > > Should we have merged this fix instead? If not, are you able to > > confirm that his change fixes your issue? > > I feel it's for solving the same issue, but I think this patch is > missing the action > `bus->slave_state = ASPEED_I2C_SLAVE_INACTIVE;`, > which means it can't resolve my problem. @Tommy, can you help confirm this? You're right, it doesn't change the slave_state at all. Unfortunately, despite no acks from the maintainers, this patch has now been merged and backported to stable. We should complete the fix, or revert it asap.
Hi Joel / Jian, Sorry ~ I didn't observe the last mail from Jian. My last patch was used to avoid the situation like below link. https://lists.ozlabs.org/pipermail/openbmc/2023-August/033756.html Because I have applied controller reset in my patch. Therefore, I think you just need to reset the slave state by "bus->slave_state = ASPEED_I2C_SLAVE_INACTIVE" in your case. BR, by Tommy > -----Original Message----- > From: Joel Stanley <joel@jms.id.au> > Sent: Wednesday, September 27, 2023 10:43 AM > To: Jian Zhang <zhangjian.3032@bytedance.com> > Cc: Tommy Huang <tommy_huang@aspeedtech.com>; > brendan.higgins@linux.dev; benh@kernel.crashing.org; andrew@aj.id.au; > zhangjian3032@gmail.com; yulei.sh@bytedance.com; > xiexinnan@bytedance.com; open list:ARM/ASPEED I2C DRIVER > <linux-i2c@vger.kernel.org>; moderated list:ARM/ASPEED I2C DRIVER > <openbmc@lists.ozlabs.org>; moderated list:ARM/ASPEED MACHINE SUPPORT > <linux-arm-kernel@lists.infradead.org>; moderated list:ARM/ASPEED > MACHINE SUPPORT <linux-aspeed@lists.ozlabs.org>; open list > <linux-kernel@vger.kernel.org> > Subject: Re: [External] Re: [PATCH] i2c: aspeed: Fix i2c bus hang in slave read > > On Fri, 22 Sept 2023 at 14:39, Jian Zhang <zhangjian.3032@bytedance.com> > wrote: > > > > > > Tommy has submitted a similar fix: > > > > > > > > > https://lore.kernel.org/linux-i2c/20230906004910.4157305-1-tommy_hua > > > ng@aspeedtech.com/ > > > > > > His change is very heavy handed; it reinitialises the bus including > > > re-parsing the device tree (!). > > > > > > Should we have merged this fix instead? If not, are you able to > > > confirm that his change fixes your issue? > > > > I feel it's for solving the same issue, but I think this patch is > > missing the action `bus->slave_state = ASPEED_I2C_SLAVE_INACTIVE;`, > > which means it can't resolve my problem. @Tommy, can you help confirm > this? > > You're right, it doesn't change the slave_state at all. > > Unfortunately, despite no acks from the maintainers, this patch has now been > merged and backported to stable. We should complete the fix, or revert it > asap.
Hi Joel! > Unfortunately, despite no acks from the maintainers, this patch has > now been merged and backported to stable. To explain my reasoning: given the list of unreviewed patches for aspeed[1], I thought that the driver maintainers were busy or similar. This happens all the time, so it would be no surprise. Andi is the driver-comaintainer meanwhile, so an ack from him counts for me as well. [1] http://patchwork.ozlabs.org/project/linux-i2c/list/?series=&submitter=&state=&q=aspeed&archive=&delegate= > We should complete the fix, or revert it asap. Agreed. We can always revert if you guys need more time fixing the issue properly. Thanks, Wolfram
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c index e76befe3f60f..1a95205fc946 100644 --- a/drivers/i2c/busses/i2c-aspeed.c +++ b/drivers/i2c/busses/i2c-aspeed.c @@ -113,6 +113,7 @@ ASPEED_I2CD_M_RX_CMD | \ ASPEED_I2CD_M_TX_CMD | \ ASPEED_I2CD_M_START_CMD) +#define ASPEED_I2CD_SLAVE_CMDS_MASK GENMASK(31, 29) /* 0x18 : I2CD Slave Device Address Register */ #define ASPEED_I2CD_DEV_ADDR_MASK GENMASK(6, 0) @@ -706,6 +707,22 @@ static int aspeed_i2c_master_xfer(struct i2c_adapter *adap, ASPEED_I2CD_BUS_BUSY_STS)) aspeed_i2c_recover_bus(bus); +#if IS_ENABLED(CONFIG_I2C_SLAVE) + /* + * If master timed out and bus is in slave mode. + * reset the slave mode. + */ + if (readl(bus->base + ASPEED_I2C_CMD_REG) & ASPEED_I2CD_SLAVE_CMDS_MASK) { + spin_lock_irqsave(&bus->lock, flags); + u32 func_ctrl_reg_val = readl(bus->base + ASPEED_I2C_FUN_CTRL_REG); + + writel(0, bus->base + ASPEED_I2C_FUN_CTRL_REG); + writel(func_ctrl_reg_val, bus->base + ASPEED_I2C_FUN_CTRL_REG); + bus->slave_state = ASPEED_I2C_SLAVE_INACTIVE; + spin_unlock_irqrestore(&bus->lock, flags); + } +#endif + /* * If timed out and the state is still pending, drop the pending * master command.
When the `CONFIG_I2C_SLAVE` option is enabled and the device operates as a slave, a situation arises where the master sends a START signal without the accompanying STOP signal. This action results in a persistent I2C bus timeout. The core issue stems from the fact that the i2c controller remains in a slave read state without a timeout mechanism. As a consequence, the bus perpetually experiences timeouts. This proposed patch addresses this problem by introducing a status check during i2c transmit timeouts. In the event that the controller is in a slave read state, the i2c controller will be reset to restore proper functionality and allow the I2C bus to resume normal operation. Signed-off-by: Jian Zhang <zhangjian.3032@bytedance.com> --- drivers/i2c/busses/i2c-aspeed.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)