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 <[email protected]>
---
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 Thu, 10 Aug 2023 at 07:22, Jian Zhang <[email protected]> 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 <[email protected]>
Tommy has submitted a similar fix:
https://lore.kernel.org/linux-i2c/[email protected]/
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"<[email protected]>
> Date: Fri, Sep 22, 2023, 14:48
> Subject: [External] Re: [PATCH] i2c: aspeed: Fix i2c bus hang in slave read
> To: "Jian Zhang"<[email protected]>, "Tommy Huang"<[email protected]>
> Cc: <[email protected]>, <[email protected]>, <[email protected]>, <[email protected]>, <[email protected]>, <[email protected]>, "open list:ARM/ASPEED I2C DRIVER"<[email protected]>, "moderated list:ARM/ASPEED I2C DRIVER"<[email protected]>, "moderated list:ARM/ASPEED MACHINE SUPPORT"<[email protected]>, "moderated list:ARM/ASPEED MACHINE SUPPORT"<[email protected]>, "open list"<[email protected]>
> On Thu, 10 Aug 2023 at 07:22, Jian Zhang <[email protected]> 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 <[email protected]>
>
> Tommy has submitted a similar fix:
>
> https://lore.kernel.org/linux-i2c/[email protected]/
>
> 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 <[email protected]> wrote:
> >
> > Tommy has submitted a similar fix:
> >
> > https://lore.kernel.org/linux-i2c/[email protected]/
> >
> > 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 <[email protected]>
> Sent: Wednesday, September 27, 2023 10:43 AM
> To: Jian Zhang <[email protected]>
> Cc: Tommy Huang <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; open list:ARM/ASPEED I2C DRIVER
> <[email protected]>; moderated list:ARM/ASPEED I2C DRIVER
> <[email protected]>; moderated list:ARM/ASPEED MACHINE SUPPORT
> <[email protected]>; moderated list:ARM/ASPEED
> MACHINE SUPPORT <[email protected]>; open list
> <[email protected]>
> Subject: Re: [External] Re: [PATCH] i2c: aspeed: Fix i2c bus hang in slave read
>
> On Fri, 22 Sept 2023 at 14:39, Jian Zhang <[email protected]>
> wrote:
> > >
> > > Tommy has submitted a similar fix:
> > >
> > >
> > > https://lore.kernel.org/linux-i2c/20230906004910.4157305-1-tommy_hua
> > > [email protected]/
> > >
> > > 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