* [PATCH] i2c: aspeed: Fix i2c bus hang in slave read @ 2023-08-10 7:21 Jian Zhang 2023-09-22 6:47 ` Joel Stanley 0 siblings, 1 reply; 6+ messages in thread From: Jian Zhang @ 2023-08-10 7:21 UTC (permalink / raw) To: brendan.higgins, benh, joel, andrew Cc: zhangjian3032, yulei.sh, xiexinnan, open list:ARM/ASPEED I2C DRIVER, moderated list:ARM/ASPEED I2C DRIVER, moderated list:ARM/ASPEED MACHINE SUPPORT, moderated list:ARM/ASPEED MACHINE SUPPORT, open list 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(+) 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] i2c: aspeed: Fix i2c bus hang in slave read 2023-08-10 7:21 [PATCH] i2c: aspeed: Fix i2c bus hang in slave read Jian Zhang @ 2023-09-22 6:47 ` Joel Stanley 2023-09-22 14:39 ` [External] " Jian Zhang 0 siblings, 1 reply; 6+ messages in thread From: Joel Stanley @ 2023-09-22 6:47 UTC (permalink / raw) To: Jian Zhang, Tommy Huang Cc: brendan.higgins, benh, andrew, zhangjian3032, yulei.sh, xiexinnan, open list:ARM/ASPEED I2C DRIVER, moderated list:ARM/ASPEED I2C DRIVER, moderated list:ARM/ASPEED MACHINE SUPPORT, moderated list:ARM/ASPEED MACHINE SUPPORT, open list 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 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [External] Re: [PATCH] i2c: aspeed: Fix i2c bus hang in slave read 2023-09-22 6:47 ` Joel Stanley @ 2023-09-22 14:39 ` Jian Zhang 2023-09-27 2:42 ` Joel Stanley 0 siblings, 1 reply; 6+ messages in thread From: Jian Zhang @ 2023-09-22 14:39 UTC (permalink / raw) To: Joel Stanley Cc: Tommy Huang, brendan.higgins, benh, andrew, zhangjian3032, yulei.sh, xiexinnan, open list:ARM/ASPEED I2C DRIVER, moderated list:ARM/ASPEED I2C DRIVER, moderated list:ARM/ASPEED MACHINE SUPPORT, moderated list:ARM/ASPEED MACHINE SUPPORT, open list > 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 > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [External] Re: [PATCH] i2c: aspeed: Fix i2c bus hang in slave read 2023-09-22 14:39 ` [External] " Jian Zhang @ 2023-09-27 2:42 ` Joel Stanley 2023-09-27 3:24 ` Tommy Huang 2023-09-27 6:18 ` Wolfram Sang 0 siblings, 2 replies; 6+ messages in thread From: Joel Stanley @ 2023-09-27 2:42 UTC (permalink / raw) To: Jian Zhang Cc: Tommy Huang, brendan.higgins, benh, andrew, zhangjian3032, yulei.sh, xiexinnan, open list:ARM/ASPEED I2C DRIVER, moderated list:ARM/ASPEED I2C DRIVER, moderated list:ARM/ASPEED MACHINE SUPPORT, moderated list:ARM/ASPEED MACHINE SUPPORT, open list 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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [External] Re: [PATCH] i2c: aspeed: Fix i2c bus hang in slave read 2023-09-27 2:42 ` Joel Stanley @ 2023-09-27 3:24 ` Tommy Huang 2023-09-27 6:18 ` Wolfram Sang 1 sibling, 0 replies; 6+ messages in thread From: Tommy Huang @ 2023-09-27 3:24 UTC (permalink / raw) To: Joel Stanley, Jian Zhang 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, moderated list:ARM/ASPEED I2C DRIVER, moderated list:ARM/ASPEED MACHINE SUPPORT, moderated list:ARM/ASPEED MACHINE SUPPORT, open list 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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [External] Re: [PATCH] i2c: aspeed: Fix i2c bus hang in slave read 2023-09-27 2:42 ` Joel Stanley 2023-09-27 3:24 ` Tommy Huang @ 2023-09-27 6:18 ` Wolfram Sang 1 sibling, 0 replies; 6+ messages in thread From: Wolfram Sang @ 2023-09-27 6:18 UTC (permalink / raw) To: Joel Stanley Cc: Jian Zhang, Tommy Huang, brendan.higgins, benh, andrew, zhangjian3032, yulei.sh, xiexinnan, open list:ARM/ASPEED I2C DRIVER, moderated list:ARM/ASPEED I2C DRIVER, moderated list:ARM/ASPEED MACHINE SUPPORT, moderated list:ARM/ASPEED MACHINE SUPPORT, open list [-- Attachment #1.1: Type: text/plain, Size: 669 bytes --] 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 [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-09-27 6:18 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-08-10 7:21 [PATCH] i2c: aspeed: Fix i2c bus hang in slave read Jian Zhang 2023-09-22 6:47 ` Joel Stanley 2023-09-22 14:39 ` [External] " Jian Zhang 2023-09-27 2:42 ` Joel Stanley 2023-09-27 3:24 ` Tommy Huang 2023-09-27 6:18 ` Wolfram Sang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox