[i2c-next,v6] i2c: aspeed: Handle master/slave combined irq events properly

Commit Message

Jae Hyun Yoo Aug. 23, 2018, 10:57 p.m. UTC

In most of cases, interrupt bits are set one by one but there are
also a lot of other cases that Aspeed I2C IP sends multiple
interrupt bits with combining master and slave events using a
single interrupt call. It happens much more in multi-master
environment than single-master. For an example, when master is
waiting for a NORMAL_STOP interrupt in its MASTER_STOP state,
SLAVE_MATCH and RX_DONE interrupts could come along with the
NORMAL_STOP in case of an another master immediately sends data
just after acquiring the bus. In this case, the NORMAL_STOP
interrupt should be handled by master_irq and the SLAVE_MATCH and
RX_DONE interrupts should be handled by slave_irq. This commit
modifies irq hadling logic to handle the master/slave combined
events properly.

Signed-off-by: Jae Hyun Yoo <[e-mail]>
---
Changes since v5:
- Changed variable names in irq hanlders to represent proper meaning.
- Fixed an error printing message again to make it use the irq_handled variable.

Changes since v4:
- Fixed an error printing message that handlers didn't handle all interrupts.

Changes since v3:
- Fixed typos in a comment.

Changes since v2:
- Changed the name of ASPEED_I2CD_INTR_ERRORS to ASPEED_I2CD_INTR_MASTER_ERRORS
- Removed a member irq_status from the struct aspeed_i2c_bus and changed
  master_irq and slave_irq handlers to make them return status_ack.
- Added a comment to explain why it needs to try both irq handlers.

Changes since v1:
- Fixed a grammar issue in commit message.
- Added a missing line feed character into a message printing.

 drivers/i2c/busses/i2c-aspeed.c | 131 ++++++++++++++++++--------------
 1 file changed, 76 insertions(+), 55 deletions(-)

Comments

Brendan Higgins Sept. 6, 2018, 5:26 p.m. UTC | #1

On Thu, Aug 23, 2018 at 3:58 PM Jae Hyun Yoo
<[e-mail]> wrote:
>
> In most of cases, interrupt bits are set one by one but there are
> also a lot of other cases that Aspeed I2C IP sends multiple
> interrupt bits with combining master and slave events using a
> single interrupt call. It happens much more in multi-master
> environment than single-master. For an example, when master is
> waiting for a NORMAL_STOP interrupt in its MASTER_STOP state,
> SLAVE_MATCH and RX_DONE interrupts could come along with the
> NORMAL_STOP in case of an another master immediately sends data
> just after acquiring the bus. In this case, the NORMAL_STOP
> interrupt should be handled by master_irq and the SLAVE_MATCH and
> RX_DONE interrupts should be handled by slave_irq. This commit
> modifies irq hadling logic to handle the master/slave combined
> events properly.
>
> Signed-off-by: Jae Hyun Yoo <[e-mail]>
> ---
> Changes since v5:
> - Changed variable names in irq hanlders to represent proper meaning.
> - Fixed an error printing message again to make it use the irq_handled variable.
>
> Changes since v4:
> - Fixed an error printing message that handlers didn't handle all interrupts.
>
> Changes since v3:
> - Fixed typos in a comment.
>
> Changes since v2:
> - Changed the name of ASPEED_I2CD_INTR_ERRORS to ASPEED_I2CD_INTR_MASTER_ERRORS
> - Removed a member irq_status from the struct aspeed_i2c_bus and changed
>   master_irq and slave_irq handlers to make them return status_ack.
> - Added a comment to explain why it needs to try both irq handlers.
>
> Changes since v1:
> - Fixed a grammar issue in commit message.
> - Added a missing line feed character into a message printing.
>
>  drivers/i2c/busses/i2c-aspeed.c | 131 ++++++++++++++++++--------------
>  1 file changed, 76 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index a4f956c6d567..c258c4d9a4c0 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -82,6 +82,11 @@
>  #define ASPEED_I2CD_INTR_RX_DONE                       BIT(2)
>  #define ASPEED_I2CD_INTR_TX_NAK                                BIT(1)
>  #define ASPEED_I2CD_INTR_TX_ACK                                BIT(0)
> +#define ASPEED_I2CD_INTR_MASTER_ERRORS                                        \
> +               (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT |                             \
> +                ASPEED_I2CD_INTR_SCL_TIMEOUT |                                \
> +                ASPEED_I2CD_INTR_ABNORMAL |                                   \
> +                ASPEED_I2CD_INTR_ARBIT_LOSS)
>  #define ASPEED_I2CD_INTR_ALL                                                  \
>                 (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT |                             \
>                  ASPEED_I2CD_INTR_BUS_RECOVER_DONE |                           \
> @@ -227,32 +232,26 @@ static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus)
>  }
>
>  #if IS_ENABLED(CONFIG_I2C_SLAVE)
> -static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
> +static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>  {
> -       u32 command, irq_status, status_ack = 0;
> +       u32 command, irq_handled = 0;
>         struct i2c_client *slave = bus->slave;
> -       bool irq_handled = true;
>         u8 value;
>
> -       if (!slave) {
> -               irq_handled = false;
> -               goto out;
> -       }
> +       if (!slave)
> +               return 0;
>
>         command = readl(bus->base + ASPEED_I2C_CMD_REG);
> -       irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>
>         /* Slave was requested, restart state machine. */
>         if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
> -               status_ack |= ASPEED_I2CD_INTR_SLAVE_MATCH;
> +               irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH;
>                 bus->slave_state = ASPEED_I2C_SLAVE_START;
>         }
>
>         /* Slave is not currently active, irq was for someone else. */
> -       if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
> -               irq_handled = false;
> -               goto out;
> -       }
> +       if (bus->slave_state == ASPEED_I2C_SLAVE_STOP)
> +               return irq_handled;
>
>         dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
>                 irq_status, command);
> @@ -269,31 +268,31 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
>                                 bus->slave_state =
>                                                 ASPEED_I2C_SLAVE_WRITE_REQUESTED;
>                 }
> -               status_ack |= ASPEED_I2CD_INTR_RX_DONE;
> +               irq_handled |= ASPEED_I2CD_INTR_RX_DONE;
>         }
>
>         /* Slave was asked to stop. */
>         if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
> -               status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
> +               irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
>                 bus->slave_state = ASPEED_I2C_SLAVE_STOP;
>         }
>         if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
> -               status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> +               irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
>                 bus->slave_state = ASPEED_I2C_SLAVE_STOP;
>         }
> +       if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
> +               irq_handled |= ASPEED_I2CD_INTR_TX_ACK;
>
>         switch (bus->slave_state) {
>         case ASPEED_I2C_SLAVE_READ_REQUESTED:
>                 if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
>                         dev_err(bus->dev, "Unexpected ACK on read request.\n");
>                 bus->slave_state = ASPEED_I2C_SLAVE_READ_PROCESSED;
> -
>                 i2c_slave_event(slave, I2C_SLAVE_READ_REQUESTED, &value);
>                 writel(value, bus->base + ASPEED_I2C_BYTE_BUF_REG);
>                 writel(ASPEED_I2CD_S_TX_CMD, bus->base + ASPEED_I2C_CMD_REG);
>                 break;
>         case ASPEED_I2C_SLAVE_READ_PROCESSED:
> -               status_ack |= ASPEED_I2CD_INTR_TX_ACK;
>                 if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK))
>                         dev_err(bus->dev,
>                                 "Expected ACK after processed read.\n");
> @@ -317,13 +316,6 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
>                 break;
>         }
>
> -       if (status_ack != irq_status)
> -               dev_err(bus->dev,
> -                       "irq handled != irq. expected %x, but was %x\n",
> -                       irq_status, status_ack);
> -       writel(status_ack, bus->base + ASPEED_I2C_INTR_STS_REG);
> -
> -out:
>         return irq_handled;
>  }
>  #endif /* CONFIG_I2C_SLAVE */
> @@ -380,21 +372,21 @@ static int aspeed_i2c_is_irq_error(u32 irq_status)
>         return 0;
>  }
>
> -static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
> +static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>  {
> -       u32 irq_status, status_ack = 0, command = 0;
> +       u32 irq_handled = 0, command = 0;
>         struct i2c_msg *msg;
>         u8 recv_byte;
>         int ret;
>
> -       irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> -       /* Ack all interrupt bits. */
> -       writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
> -
>         if (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
>                 bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> -               status_ack |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
> +               irq_handled |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
>                 goto out_complete;
> +       } else {
> +               /* Master is not currently active, irq was for someone else. */
> +               if (bus->master_state == ASPEED_I2C_MASTER_INACTIVE)
> +                       goto out_no_complete;
>         }
>
>         /*
> @@ -403,19 +395,22 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>          * INACTIVE state.
>          */
>         ret = aspeed_i2c_is_irq_error(irq_status);
> -       if (ret < 0) {
> +       if (ret) {
>                 dev_dbg(bus->dev, "received error interrupt: 0x%08x\n",
>                         irq_status);
>                 bus->cmd_err = ret;
>                 bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> +               irq_handled |= (irq_status & ASPEED_I2CD_INTR_MASTER_ERRORS);
>                 goto out_complete;
>         }
>
>         /* We are in an invalid state; reset bus to a known state. */
>         if (!bus->msgs) {
> -               dev_err(bus->dev, "bus in unknown state\n");
> +               dev_err(bus->dev, "bus in unknown state. irq_status: 0x%x\n",
> +                       irq_status);
>                 bus->cmd_err = -EIO;
> -               if (bus->master_state != ASPEED_I2C_MASTER_STOP)
> +               if (bus->master_state != ASPEED_I2C_MASTER_STOP &&
> +                   bus->master_state != ASPEED_I2C_MASTER_INACTIVE)
>                         aspeed_i2c_do_stop(bus);
>                 goto out_no_complete;
>         }
> @@ -428,13 +423,18 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>          */
>         if (bus->master_state == ASPEED_I2C_MASTER_START) {
>                 if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
> +                       if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_NAK))) {
> +                               bus->cmd_err = -ENXIO;
> +                               bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> +                               goto out_complete;
> +                       }
>                         pr_devel("no slave present at %02x\n", msg->addr);
> -                       status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> +                       irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
>                         bus->cmd_err = -ENXIO;
>                         aspeed_i2c_do_stop(bus);
>                         goto out_no_complete;
>                 }
> -               status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> +               irq_handled |= ASPEED_I2CD_INTR_TX_ACK;
>                 if (msg->len == 0) { /* SMBUS_QUICK */
>                         aspeed_i2c_do_stop(bus);
>                         goto out_no_complete;
> @@ -449,13 +449,13 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>         case ASPEED_I2C_MASTER_TX:
>                 if (unlikely(irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
>                         dev_dbg(bus->dev, "slave NACKed TX\n");
> -                       status_ack |= ASPEED_I2CD_INTR_TX_NAK;
> +                       irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
>                         goto error_and_stop;
>                 } else if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
>                         dev_err(bus->dev, "slave failed to ACK TX\n");
>                         goto error_and_stop;
>                 }
> -               status_ack |= ASPEED_I2CD_INTR_TX_ACK;
> +               irq_handled |= ASPEED_I2CD_INTR_TX_ACK;
>                 /* fallthrough intended */
>         case ASPEED_I2C_MASTER_TX_FIRST:
>                 if (bus->buf_index < msg->len) {
> @@ -478,7 +478,7 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>                         dev_err(bus->dev, "master failed to RX\n");
>                         goto error_and_stop;
>                 }
> -               status_ack |= ASPEED_I2CD_INTR_RX_DONE;
> +               irq_handled |= ASPEED_I2CD_INTR_RX_DONE;
>
>                 recv_byte = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
>                 msg->buf[bus->buf_index++] = recv_byte;
> @@ -506,11 +506,13 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>                 goto out_no_complete;
>         case ASPEED_I2C_MASTER_STOP:
>                 if (unlikely(!(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP))) {
> -                       dev_err(bus->dev, "master failed to STOP\n");
> +                       dev_err(bus->dev,
> +                               "master failed to STOP. irq_status:0x%x\n",
> +                               irq_status);
>                         bus->cmd_err = -EIO;
>                         /* Do not STOP as we have already tried. */
>                 } else {
> -                       status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
> +                       irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
>                 }
>
>                 bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
> @@ -540,33 +542,52 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>                 bus->master_xfer_result = bus->msgs_index + 1;
>         complete(&bus->cmd_complete);
>  out_no_complete:
> -       if (irq_status != status_ack)
> -               dev_err(bus->dev,
> -                       "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> -                       irq_status, status_ack);
> -       return !!irq_status;
> +       return irq_handled;
>  }
>
>  static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>  {
>         struct aspeed_i2c_bus *bus = dev_id;
> -       bool ret;
> +       u32 irq_received, irq_remaining, irq_handled;
>
>         spin_lock(&bus->lock);
> +       irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> +       irq_remaining = irq_received;
>
>  #if IS_ENABLED(CONFIG_I2C_SLAVE)
> -       if (aspeed_i2c_slave_irq(bus)) {
> -               dev_dbg(bus->dev, "irq handled by slave.\n");
> -               ret = true;
> -               goto out;
> +       /*
> +        * In most cases, interrupt bits will be set one by one, although
> +        * multiple interrupt bits could be set at the same time. It's also
> +        * possible that master interrupt bits could be set along with slave
> +        * interrupt bits. Each case needs to be handled using corresponding
> +        * handlers depending on the current state.
> +        */
> +       if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
> +               irq_handled = aspeed_i2c_master_irq(bus, irq_remaining);
> +               irq_remaining &= ~irq_handled;
> +               if (irq_remaining)
> +                       irq_handled |= aspeed_i2c_slave_irq(bus, irq_remaining);
> +       } else {
> +               irq_handled = aspeed_i2c_slave_irq(bus, irq_remaining);
> +               irq_remaining &= ~irq_handled;
> +               if (irq_remaining)
> +                       irq_handled |= aspeed_i2c_master_irq(bus,
> +                                                            irq_remaining);
>         }
> +#else
> +       irq_handled = aspeed_i2c_master_irq(bus, irq_remaining);
>  #endif /* CONFIG_I2C_SLAVE */
>
> -       ret = aspeed_i2c_master_irq(bus);
> +       irq_remaining &= ~irq_handled;
> +       if (irq_remaining)
> +               dev_err(bus->dev,
> +                       "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> +                       irq_received, irq_handled);
>
> -out:
> +       /* Ack all interrupt bits. */
> +       writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>         spin_unlock(&bus->lock);
> -       return ret ? IRQ_HANDLED : IRQ_NONE;
> +       return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>  }
>
>  static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
> --
> 2.18.0
>

Looks awesome! Thanks!

Reviewed-by: Brendan Higgins <[e-mail]>

Jae Hyun Yoo Sept. 6, 2018, 5:32 p.m. UTC | #2

On 9/6/2018 10:26 AM, Brendan Higgins wrote:
> On Thu, Aug 23, 2018 at 3:58 PM Jae Hyun Yoo
> <[e-mail]> wrote:
>>
>> In most of cases, interrupt bits are set one by one but there are
>> also a lot of other cases that Aspeed I2C IP sends multiple
>> interrupt bits with combining master and slave events using a
>> single interrupt call. It happens much more in multi-master
>> environment than single-master. For an example, when master is
>> waiting for a NORMAL_STOP interrupt in its MASTER_STOP state,
>> SLAVE_MATCH and RX_DONE interrupts could come along with the
>> NORMAL_STOP in case of an another master immediately sends data
>> just after acquiring the bus. In this case, the NORMAL_STOP
>> interrupt should be handled by master_irq and the SLAVE_MATCH and
>> RX_DONE interrupts should be handled by slave_irq. This commit
>> modifies irq hadling logic to handle the master/slave combined
>> events properly.
>>
>> Signed-off-by: Jae Hyun Yoo <[e-mail]>
>> ---
>> Changes since v5:
>> - Changed variable names in irq hanlders to represent proper meaning.
>> - Fixed an error printing message again to make it use the irq_handled variable.
>>
>> Changes since v4:
>> - Fixed an error printing message that handlers didn't handle all interrupts.
>>
>> Changes since v3:
>> - Fixed typos in a comment.
>>
>> Changes since v2:
>> - Changed the name of ASPEED_I2CD_INTR_ERRORS to ASPEED_I2CD_INTR_MASTER_ERRORS
>> - Removed a member irq_status from the struct aspeed_i2c_bus and changed
>>    master_irq and slave_irq handlers to make them return status_ack.
>> - Added a comment to explain why it needs to try both irq handlers.
>>
>> Changes since v1:
>> - Fixed a grammar issue in commit message.
>> - Added a missing line feed character into a message printing.
>>
>>   drivers/i2c/busses/i2c-aspeed.c | 131 ++++++++++++++++++--------------
>>   1 file changed, 76 insertions(+), 55 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>> index a4f956c6d567..c258c4d9a4c0 100644
>> --- a/drivers/i2c/busses/i2c-aspeed.c
>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>> @@ -82,6 +82,11 @@
>>   #define ASPEED_I2CD_INTR_RX_DONE                       BIT(2)
>>   #define ASPEED_I2CD_INTR_TX_NAK                                BIT(1)
>>   #define ASPEED_I2CD_INTR_TX_ACK                                BIT(0)
>> +#define ASPEED_I2CD_INTR_MASTER_ERRORS                                        \
>> +               (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT |                             \
>> +                ASPEED_I2CD_INTR_SCL_TIMEOUT |                                \
>> +                ASPEED_I2CD_INTR_ABNORMAL |                                   \
>> +                ASPEED_I2CD_INTR_ARBIT_LOSS)
>>   #define ASPEED_I2CD_INTR_ALL                                                  \
>>                  (ASPEED_I2CD_INTR_SDA_DL_TIMEOUT |                             \
>>                   ASPEED_I2CD_INTR_BUS_RECOVER_DONE |                           \
>> @@ -227,32 +232,26 @@ static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus)
>>   }
>>
>>   #if IS_ENABLED(CONFIG_I2C_SLAVE)
>> -static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
>> +static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>>   {
>> -       u32 command, irq_status, status_ack = 0;
>> +       u32 command, irq_handled = 0;
>>          struct i2c_client *slave = bus->slave;
>> -       bool irq_handled = true;
>>          u8 value;
>>
>> -       if (!slave) {
>> -               irq_handled = false;
>> -               goto out;
>> -       }
>> +       if (!slave)
>> +               return 0;
>>
>>          command = readl(bus->base + ASPEED_I2C_CMD_REG);
>> -       irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>>
>>          /* Slave was requested, restart state machine. */
>>          if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
>> -               status_ack |= ASPEED_I2CD_INTR_SLAVE_MATCH;
>> +               irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH;
>>                  bus->slave_state = ASPEED_I2C_SLAVE_START;
>>          }
>>
>>          /* Slave is not currently active, irq was for someone else. */
>> -       if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
>> -               irq_handled = false;
>> -               goto out;
>> -       }
>> +       if (bus->slave_state == ASPEED_I2C_SLAVE_STOP)
>> +               return irq_handled;
>>
>>          dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
>>                  irq_status, command);
>> @@ -269,31 +268,31 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
>>                                  bus->slave_state =
>>                                                  ASPEED_I2C_SLAVE_WRITE_REQUESTED;
>>                  }
>> -               status_ack |= ASPEED_I2CD_INTR_RX_DONE;
>> +               irq_handled |= ASPEED_I2CD_INTR_RX_DONE;
>>          }
>>
>>          /* Slave was asked to stop. */
>>          if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
>> -               status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
>> +               irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
>>                  bus->slave_state = ASPEED_I2C_SLAVE_STOP;
>>          }
>>          if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
>> -               status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> +               irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
>>                  bus->slave_state = ASPEED_I2C_SLAVE_STOP;
>>          }
>> +       if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
>> +               irq_handled |= ASPEED_I2CD_INTR_TX_ACK;
>>
>>          switch (bus->slave_state) {
>>          case ASPEED_I2C_SLAVE_READ_REQUESTED:
>>                  if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
>>                          dev_err(bus->dev, "Unexpected ACK on read request.\n");
>>                  bus->slave_state = ASPEED_I2C_SLAVE_READ_PROCESSED;
>> -
>>                  i2c_slave_event(slave, I2C_SLAVE_READ_REQUESTED, &value);
>>                  writel(value, bus->base + ASPEED_I2C_BYTE_BUF_REG);
>>                  writel(ASPEED_I2CD_S_TX_CMD, bus->base + ASPEED_I2C_CMD_REG);
>>                  break;
>>          case ASPEED_I2C_SLAVE_READ_PROCESSED:
>> -               status_ack |= ASPEED_I2CD_INTR_TX_ACK;
>>                  if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK))
>>                          dev_err(bus->dev,
>>                                  "Expected ACK after processed read.\n");
>> @@ -317,13 +316,6 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
>>                  break;
>>          }
>>
>> -       if (status_ack != irq_status)
>> -               dev_err(bus->dev,
>> -                       "irq handled != irq. expected %x, but was %x\n",
>> -                       irq_status, status_ack);
>> -       writel(status_ack, bus->base + ASPEED_I2C_INTR_STS_REG);
>> -
>> -out:
>>          return irq_handled;
>>   }
>>   #endif /* CONFIG_I2C_SLAVE */
>> @@ -380,21 +372,21 @@ static int aspeed_i2c_is_irq_error(u32 irq_status)
>>          return 0;
>>   }
>>
>> -static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>> +static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
>>   {
>> -       u32 irq_status, status_ack = 0, command = 0;
>> +       u32 irq_handled = 0, command = 0;
>>          struct i2c_msg *msg;
>>          u8 recv_byte;
>>          int ret;
>>
>> -       irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>> -       /* Ack all interrupt bits. */
>> -       writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
>> -
>>          if (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
>>                  bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
>> -               status_ack |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
>> +               irq_handled |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
>>                  goto out_complete;
>> +       } else {
>> +               /* Master is not currently active, irq was for someone else. */
>> +               if (bus->master_state == ASPEED_I2C_MASTER_INACTIVE)
>> +                       goto out_no_complete;
>>          }
>>
>>          /*
>> @@ -403,19 +395,22 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>>           * INACTIVE state.
>>           */
>>          ret = aspeed_i2c_is_irq_error(irq_status);
>> -       if (ret < 0) {
>> +       if (ret) {
>>                  dev_dbg(bus->dev, "received error interrupt: 0x%08x\n",
>>                          irq_status);
>>                  bus->cmd_err = ret;
>>                  bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
>> +               irq_handled |= (irq_status & ASPEED_I2CD_INTR_MASTER_ERRORS);
>>                  goto out_complete;
>>          }
>>
>>          /* We are in an invalid state; reset bus to a known state. */
>>          if (!bus->msgs) {
>> -               dev_err(bus->dev, "bus in unknown state\n");
>> +               dev_err(bus->dev, "bus in unknown state. irq_status: 0x%x\n",
>> +                       irq_status);
>>                  bus->cmd_err = -EIO;
>> -               if (bus->master_state != ASPEED_I2C_MASTER_STOP)
>> +               if (bus->master_state != ASPEED_I2C_MASTER_STOP &&
>> +                   bus->master_state != ASPEED_I2C_MASTER_INACTIVE)
>>                          aspeed_i2c_do_stop(bus);
>>                  goto out_no_complete;
>>          }
>> @@ -428,13 +423,18 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>>           */
>>          if (bus->master_state == ASPEED_I2C_MASTER_START) {
>>                  if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
>> +                       if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_NAK))) {
>> +                               bus->cmd_err = -ENXIO;
>> +                               bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
>> +                               goto out_complete;
>> +                       }
>>                          pr_devel("no slave present at %02x\n", msg->addr);
>> -                       status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> +                       irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
>>                          bus->cmd_err = -ENXIO;
>>                          aspeed_i2c_do_stop(bus);
>>                          goto out_no_complete;
>>                  }
>> -               status_ack |= ASPEED_I2CD_INTR_TX_ACK;
>> +               irq_handled |= ASPEED_I2CD_INTR_TX_ACK;
>>                  if (msg->len == 0) { /* SMBUS_QUICK */
>>                          aspeed_i2c_do_stop(bus);
>>                          goto out_no_complete;
>> @@ -449,13 +449,13 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>>          case ASPEED_I2C_MASTER_TX:
>>                  if (unlikely(irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
>>                          dev_dbg(bus->dev, "slave NACKed TX\n");
>> -                       status_ack |= ASPEED_I2CD_INTR_TX_NAK;
>> +                       irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
>>                          goto error_and_stop;
>>                  } else if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
>>                          dev_err(bus->dev, "slave failed to ACK TX\n");
>>                          goto error_and_stop;
>>                  }
>> -               status_ack |= ASPEED_I2CD_INTR_TX_ACK;
>> +               irq_handled |= ASPEED_I2CD_INTR_TX_ACK;
>>                  /* fallthrough intended */
>>          case ASPEED_I2C_MASTER_TX_FIRST:
>>                  if (bus->buf_index < msg->len) {
>> @@ -478,7 +478,7 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>>                          dev_err(bus->dev, "master failed to RX\n");
>>                          goto error_and_stop;
>>                  }
>> -               status_ack |= ASPEED_I2CD_INTR_RX_DONE;
>> +               irq_handled |= ASPEED_I2CD_INTR_RX_DONE;
>>
>>                  recv_byte = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
>>                  msg->buf[bus->buf_index++] = recv_byte;
>> @@ -506,11 +506,13 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>>                  goto out_no_complete;
>>          case ASPEED_I2C_MASTER_STOP:
>>                  if (unlikely(!(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP))) {
>> -                       dev_err(bus->dev, "master failed to STOP\n");
>> +                       dev_err(bus->dev,
>> +                               "master failed to STOP. irq_status:0x%x\n",
>> +                               irq_status);
>>                          bus->cmd_err = -EIO;
>>                          /* Do not STOP as we have already tried. */
>>                  } else {
>> -                       status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
>> +                       irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
>>                  }
>>
>>                  bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
>> @@ -540,33 +542,52 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
>>                  bus->master_xfer_result = bus->msgs_index + 1;
>>          complete(&bus->cmd_complete);
>>   out_no_complete:
>> -       if (irq_status != status_ack)
>> -               dev_err(bus->dev,
>> -                       "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>> -                       irq_status, status_ack);
>> -       return !!irq_status;
>> +       return irq_handled;
>>   }
>>
>>   static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>   {
>>          struct aspeed_i2c_bus *bus = dev_id;
>> -       bool ret;
>> +       u32 irq_received, irq_remaining, irq_handled;
>>
>>          spin_lock(&bus->lock);
>> +       irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>> +       irq_remaining = irq_received;
>>
>>   #if IS_ENABLED(CONFIG_I2C_SLAVE)
>> -       if (aspeed_i2c_slave_irq(bus)) {
>> -               dev_dbg(bus->dev, "irq handled by slave.\n");
>> -               ret = true;
>> -               goto out;
>> +       /*
>> +        * In most cases, interrupt bits will be set one by one, although
>> +        * multiple interrupt bits could be set at the same time. It's also
>> +        * possible that master interrupt bits could be set along with slave
>> +        * interrupt bits. Each case needs to be handled using corresponding
>> +        * handlers depending on the current state.
>> +        */
>> +       if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
>> +               irq_handled = aspeed_i2c_master_irq(bus, irq_remaining);
>> +               irq_remaining &= ~irq_handled;
>> +               if (irq_remaining)
>> +                       irq_handled |= aspeed_i2c_slave_irq(bus, irq_remaining);
>> +       } else {
>> +               irq_handled = aspeed_i2c_slave_irq(bus, irq_remaining);
>> +               irq_remaining &= ~irq_handled;
>> +               if (irq_remaining)
>> +                       irq_handled |= aspeed_i2c_master_irq(bus,
>> +                                                            irq_remaining);
>>          }
>> +#else
>> +       irq_handled = aspeed_i2c_master_irq(bus, irq_remaining);
>>   #endif /* CONFIG_I2C_SLAVE */
>>
>> -       ret = aspeed_i2c_master_irq(bus);
>> +       irq_remaining &= ~irq_handled;
>> +       if (irq_remaining)
>> +               dev_err(bus->dev,
>> +                       "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>> +                       irq_received, irq_handled);
>>
>> -out:
>> +       /* Ack all interrupt bits. */
>> +       writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>          spin_unlock(&bus->lock);
>> -       return ret ? IRQ_HANDLED : IRQ_NONE;
>> +       return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>>   }
>>
>>   static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
>> --
>> 2.18.0
>>
> 
> Looks awesome! Thanks!
> 
> Reviewed-by: Brendan Higgins <[e-mail]>
> 

Thanks a lot for your review Brendan!

-Jae

Wolfram Sang Sept. 6, 2018, 6:08 p.m. UTC | #3

> > Looks awesome! Thanks!
> > 
> > Reviewed-by: Brendan Higgins <[e-mail]>
> > 
> 
> Thanks a lot for your review Brendan!

Please don't quote the whole message. Everyone else has to scroll down
a lot to find the relevant information. Thanks!

Jae Hyun Yoo Sept. 6, 2018, 6:33 p.m. UTC | #4

> 
> Please don't quote the whole message. Everyone else has to scroll down
> a lot to find the relevant information. Thanks!
> 

Will keep that in mind. Thanks Wolfram!

Wolfram Sang Sept. 6, 2018, 6:40 p.m. UTC | #5

On Thu, Aug 23, 2018 at 03:57:31PM -0700, Jae Hyun Yoo wrote:
> In most of cases, interrupt bits are set one by one but there are
> also a lot of other cases that Aspeed I2C IP sends multiple
> interrupt bits with combining master and slave events using a
> single interrupt call. It happens much more in multi-master
> environment than single-master. For an example, when master is
> waiting for a NORMAL_STOP interrupt in its MASTER_STOP state,
> SLAVE_MATCH and RX_DONE interrupts could come along with the
> NORMAL_STOP in case of an another master immediately sends data
> just after acquiring the bus. In this case, the NORMAL_STOP
> interrupt should be handled by master_irq and the SLAVE_MATCH and
> RX_DONE interrupts should be handled by slave_irq. This commit
> modifies irq hadling logic to handle the master/slave combined
> events properly.
> 
> Signed-off-by: Jae Hyun Yoo <[e-mail]>

Applied to for-next, thanks!

Guenter Roeck Sept. 11, 2018, 6:37 p.m. UTC | #6

Hi,

On Thu, Aug 23, 2018 at 03:57:31PM -0700, Jae Hyun Yoo wrote:
> In most of cases, interrupt bits are set one by one but there are
> also a lot of other cases that Aspeed I2C IP sends multiple
> interrupt bits with combining master and slave events using a
> single interrupt call. It happens much more in multi-master
> environment than single-master. For an example, when master is
> waiting for a NORMAL_STOP interrupt in its MASTER_STOP state,
> SLAVE_MATCH and RX_DONE interrupts could come along with the
> NORMAL_STOP in case of an another master immediately sends data
> just after acquiring the bus. In this case, the NORMAL_STOP
> interrupt should be handled by master_irq and the SLAVE_MATCH and
> RX_DONE interrupts should be handled by slave_irq. This commit
> modifies irq hadling logic to handle the master/slave combined
> events properly.
> 
> Signed-off-by: Jae Hyun Yoo <[e-mail]>
> Reviewed-by: Brendan Higgins <[e-mail]>

This patch causes a boot stall when booting witherspoon-bmc with
qemu v3.0, and all i2c device probes fail with error -110 (timeout).
Bisect log is attached for reference.

With the same kernel configuration (aspeed_g5_defconfig),
ast2500-evb and romulus-bmc are still able to boot.
palmetto-bmc with aspeed_g4_defconfig also appears to work.

Is this a problem with qemu ? Should I drop the qemu test
for witherspoon-bmc starting with the next kernel release ?

Thanks,
Guenter

---
# bad: [09c0888767529cdb382f34452819e42d1a66a114] Add linux-next specific files for 20180911
# good: [11da3a7f84f19c26da6f86af878298694ede0804] Linux 4.19-rc3
git bisect start 'HEAD' 'v4.19-rc3'
# bad: [a2ebc71cf97bed9b453318418e4a281434565e8b] Merge remote-tracking branch 'nfc-next/master'
git bisect bad a2ebc71cf97bed9b453318418e4a281434565e8b
# good: [6fde463b32bf4105c28c0a297a5b66aca5d6ecd4] Merge remote-tracking branch 's390/features'
git bisect good 6fde463b32bf4105c28c0a297a5b66aca5d6ecd4
# bad: [136fd6d530a3ae0dd003984f683345cfe88c01f3] Merge remote-tracking branch 'v4l-dvb/master'
git bisect bad 136fd6d530a3ae0dd003984f683345cfe88c01f3
# good: [c7ae95368af43c08f5f615b00f2f7bf2e9c45788] Merge remote-tracking branch 'v9fs/9p-next'
git bisect good c7ae95368af43c08f5f615b00f2f7bf2e9c45788
# good: [4c640c41381e47b328c6507bcf534812761256cd] Merge branch 'for-4.19/fixes' into for-next
git bisect good 4c640c41381e47b328c6507bcf534812761256cd
# good: [5bc91f70c5ecc2bc5967b98ce7fa4e55ad230d99] Merge remote-tracking branch 'hid/for-next'
git bisect good 5bc91f70c5ecc2bc5967b98ce7fa4e55ad230d99
# bad: [657b9d37406ed1625d469db0fd356e364dc75dd8] Merge remote-tracking branch 'hwmon-staging/hwmon-next'
git bisect bad 657b9d37406ed1625d469db0fd356e364dc75dd8
# bad: [fc9f90ddace238716cfcbd00d51428ee8baa12c7] Merge branch 'i2c/for-current' into i2c/for-next
git bisect bad fc9f90ddace238716cfcbd00d51428ee8baa12c7
# good: [34b7be301d4c5d85d1d093d2faf856f3d727416f] Merge branch 'i2c/for-current' into i2c/for-next
git bisect good 34b7be301d4c5d85d1d093d2faf856f3d727416f
# bad: [3e9efc3299dd78a0fa96515f0a453fab1ed4a1bd] i2c: aspeed: Handle master/slave combined irq events properly
git bisect bad 3e9efc3299dd78a0fa96515f0a453fab1ed4a1bd
# good: [fc66b39fe36acfd06f716e338de7cd8f9550fad2] i2c: mediatek: Use DMA safe buffers for i2c transactions
git bisect good fc66b39fe36acfd06f716e338de7cd8f9550fad2
# first bad commit: [3e9efc3299dd78a0fa96515f0a453fab1ed4a1bd] i2c: aspeed: Handle master/slave combined irq events properly

Cédric Le Goater Sept. 11, 2018, 6:45 p.m. UTC | #7

On 09/11/2018 08:37 PM, Guenter Roeck wrote:
> Hi,
> 
> On Thu, Aug 23, 2018 at 03:57:31PM -0700, Jae Hyun Yoo wrote:
>> In most of cases, interrupt bits are set one by one but there are
>> also a lot of other cases that Aspeed I2C IP sends multiple
>> interrupt bits with combining master and slave events using a
>> single interrupt call. It happens much more in multi-master
>> environment than single-master. For an example, when master is
>> waiting for a NORMAL_STOP interrupt in its MASTER_STOP state,
>> SLAVE_MATCH and RX_DONE interrupts could come along with the
>> NORMAL_STOP in case of an another master immediately sends data
>> just after acquiring the bus. In this case, the NORMAL_STOP
>> interrupt should be handled by master_irq and the SLAVE_MATCH and
>> RX_DONE interrupts should be handled by slave_irq. This commit
>> modifies irq hadling logic to handle the master/slave combined
>> events properly.
>>
>> Signed-off-by: Jae Hyun Yoo <[e-mail]>
>> Reviewed-by: Brendan Higgins <[e-mail]>
> 
> This patch causes a boot stall when booting witherspoon-bmc with
> qemu v3.0, and all i2c device probes fail with error -110 (timeout).
> Bisect log is attached for reference.
> 
> With the same kernel configuration (aspeed_g5_defconfig),
> ast2500-evb and romulus-bmc are still able to boot.
> palmetto-bmc with aspeed_g4_defconfig also appears to work.
> 
> Is this a problem with qemu ? Should I drop the qemu test
> for witherspoon-bmc starting with the next kernel release ?

The QEMU model for the Aspeed I2C controller does not support 
slave mode or DMA registers. That might be the issue. 

I will check what this patch does when I have sometime. 

Thanks, 


C.

Jae Hyun Yoo Sept. 11, 2018, 8:30 p.m. UTC | #8

On 9/11/2018 11:37 AM, Guenter Roeck wrote:
> Hi,
> 
> On Thu, Aug 23, 2018 at 03:57:31PM -0700, Jae Hyun Yoo wrote:
>> In most of cases, interrupt bits are set one by one but there are
>> also a lot of other cases that Aspeed I2C IP sends multiple
>> interrupt bits with combining master and slave events using a
>> single interrupt call. It happens much more in multi-master
>> environment than single-master. For an example, when master is
>> waiting for a NORMAL_STOP interrupt in its MASTER_STOP state,
>> SLAVE_MATCH and RX_DONE interrupts could come along with the
>> NORMAL_STOP in case of an another master immediately sends data
>> just after acquiring the bus. In this case, the NORMAL_STOP
>> interrupt should be handled by master_irq and the SLAVE_MATCH and
>> RX_DONE interrupts should be handled by slave_irq. This commit
>> modifies irq hadling logic to handle the master/slave combined
>> events properly.
>>
>> Signed-off-by: Jae Hyun Yoo <[e-mail]>
>> Reviewed-by: Brendan Higgins <[e-mail]>
> 
> This patch causes a boot stall when booting witherspoon-bmc with
> qemu v3.0, and all i2c device probes fail with error -110 (timeout).
> Bisect log is attached for reference.
> 
> With the same kernel configuration (aspeed_g5_defconfig),
> ast2500-evb and romulus-bmc are still able to boot.
> palmetto-bmc with aspeed_g4_defconfig also appears to work.
> 
> Is this a problem with qemu ? Should I drop the qemu test
> for witherspoon-bmc starting with the next kernel release ?
> 
> Thanks,
> Guenter
> 

Hi Guenter,

Thanks for your report.

I checked this patch again but it doesn't have any change that could
affect to the probing flow. I'll debug the issue on qemu 3.0 environment
and will share if I find something.

Thanks,
Jae

Guenter Roeck Sept. 11, 2018, 8:41 p.m. UTC | #9

On Tue, Sep 11, 2018 at 01:30:41PM -0700, Jae Hyun Yoo wrote:
> On 9/11/2018 11:37 AM, Guenter Roeck wrote:
> >Hi,
> >
> >On Thu, Aug 23, 2018 at 03:57:31PM -0700, Jae Hyun Yoo wrote:
> >>In most of cases, interrupt bits are set one by one but there are
> >>also a lot of other cases that Aspeed I2C IP sends multiple
> >>interrupt bits with combining master and slave events using a
> >>single interrupt call. It happens much more in multi-master
> >>environment than single-master. For an example, when master is
> >>waiting for a NORMAL_STOP interrupt in its MASTER_STOP state,
> >>SLAVE_MATCH and RX_DONE interrupts could come along with the
> >>NORMAL_STOP in case of an another master immediately sends data
> >>just after acquiring the bus. In this case, the NORMAL_STOP
> >>interrupt should be handled by master_irq and the SLAVE_MATCH and
> >>RX_DONE interrupts should be handled by slave_irq. This commit
> >>modifies irq hadling logic to handle the master/slave combined
> >>events properly.
> >>
> >>Signed-off-by: Jae Hyun Yoo <[e-mail]>
> >>Reviewed-by: Brendan Higgins <[e-mail]>
> >
> >This patch causes a boot stall when booting witherspoon-bmc with
> >qemu v3.0, and all i2c device probes fail with error -110 (timeout).
> >Bisect log is attached for reference.
> >
> >With the same kernel configuration (aspeed_g5_defconfig),
> >ast2500-evb and romulus-bmc are still able to boot.
> >palmetto-bmc with aspeed_g4_defconfig also appears to work.
> >
> >Is this a problem with qemu ? Should I drop the qemu test
> >for witherspoon-bmc starting with the next kernel release ?
> >
> >Thanks,
> >Guenter
> >
> 
> Hi Guenter,
> 
> Thanks for your report.
> 
> I checked this patch again but it doesn't have any change that could
> affect to the probing flow. I'll debug the issue on qemu 3.0 environment
> and will share if I find something.
> 
The problem may be that qemu and the new code disagree how interrupts
should be generated and handled, and the new code does not handle the
interrupts it receives from the simulated hardware. This will result
in i2c device probe failure, which in turn can cause all kinds of
problems.

Thanks,
Guenter

Jae Hyun Yoo Sept. 11, 2018, 10:18 p.m. UTC | #10

On 9/11/2018 1:41 PM, Guenter Roeck wrote:
> On Tue, Sep 11, 2018 at 01:30:41PM -0700, Jae Hyun Yoo wrote:
>> On 9/11/2018 11:37 AM, Guenter Roeck wrote:
>>> Hi,
>>>
>>> On Thu, Aug 23, 2018 at 03:57:31PM -0700, Jae Hyun Yoo wrote:
>>>> In most of cases, interrupt bits are set one by one but there are
>>>> also a lot of other cases that Aspeed I2C IP sends multiple
>>>> interrupt bits with combining master and slave events using a
>>>> single interrupt call. It happens much more in multi-master
>>>> environment than single-master. For an example, when master is
>>>> waiting for a NORMAL_STOP interrupt in its MASTER_STOP state,
>>>> SLAVE_MATCH and RX_DONE interrupts could come along with the
>>>> NORMAL_STOP in case of an another master immediately sends data
>>>> just after acquiring the bus. In this case, the NORMAL_STOP
>>>> interrupt should be handled by master_irq and the SLAVE_MATCH and
>>>> RX_DONE interrupts should be handled by slave_irq. This commit
>>>> modifies irq hadling logic to handle the master/slave combined
>>>> events properly.
>>>>
>>>> Signed-off-by: Jae Hyun Yoo <[e-mail]>
>>>> Reviewed-by: Brendan Higgins <[e-mail]>
>>>
>>> This patch causes a boot stall when booting witherspoon-bmc with
>>> qemu v3.0, and all i2c device probes fail with error -110 (timeout).
>>> Bisect log is attached for reference.
>>>
>>> With the same kernel configuration (aspeed_g5_defconfig),
>>> ast2500-evb and romulus-bmc are still able to boot.
>>> palmetto-bmc with aspeed_g4_defconfig also appears to work.
>>>
>>> Is this a problem with qemu ? Should I drop the qemu test
>>> for witherspoon-bmc starting with the next kernel release ?
>>>
>>> Thanks,
>>> Guenter
>>>
>>
>> Hi Guenter,
>>
>> Thanks for your report.
>>
>> I checked this patch again but it doesn't have any change that could
>> affect to the probing flow. I'll debug the issue on qemu 3.0 environment
>> and will share if I find something.
>>
> The problem may be that qemu and the new code disagree how interrupts
> should be generated and handled, and the new code does not handle the
> interrupts it receives from the simulated hardware. This will result
> in i2c device probe failure, which in turn can cause all kinds of
> problems.
> 

Yes, that makes sense. Looks like it should be reverted until the issue
is fixed. Will submit a patch to revert it.

Thanks,
Jae

Joel Stanley Sept. 11, 2018, 10:53 p.m. UTC | #11

On Wed, 12 Sep 2018 at 07:48, Jae Hyun Yoo <[e-mail]> wrote:
>
> On 9/11/2018 1:41 PM, Guenter Roeck wrote:
> > On Tue, Sep 11, 2018 at 01:30:41PM -0700, Jae Hyun Yoo wrote:

> >> I checked this patch again but it doesn't have any change that could
> >> affect to the probing flow. I'll debug the issue on qemu 3.0 environment
> >> and will share if I find something.
> >>
> > The problem may be that qemu and the new code disagree how interrupts
> > should be generated and handled, and the new code does not handle the
> > interrupts it receives from the simulated hardware. This will result
> > in i2c device probe failure, which in turn can cause all kinds of
> > problems.
> >
>
> Yes, that makes sense. Looks like it should be reverted until the issue
> is fixed. Will submit a patch to revert it.

Let's not rush. The qemu model was written in order to allow us to
test the kernel code, and was validated by the kernel driver we have.
We've had situations in the past (with the i2c driver in fact) where a
change in the driver required an update of the model to be more
accurate.

I suggest we wait until Cedric has a chance to look at the issue
before reverting the patch.

Cheers,

Joel

Guenter Roeck Sept. 11, 2018, 11:33 p.m. UTC | #12

On Wed, Sep 12, 2018 at 08:23:29AM +0930, Joel Stanley wrote:
> On Wed, 12 Sep 2018 at 07:48, Jae Hyun Yoo <[e-mail]> wrote:
> >
> > On 9/11/2018 1:41 PM, Guenter Roeck wrote:
> > > On Tue, Sep 11, 2018 at 01:30:41PM -0700, Jae Hyun Yoo wrote:
> 
> > >> I checked this patch again but it doesn't have any change that could
> > >> affect to the probing flow. I'll debug the issue on qemu 3.0 environment
> > >> and will share if I find something.
> > >>
> > > The problem may be that qemu and the new code disagree how interrupts
> > > should be generated and handled, and the new code does not handle the
> > > interrupts it receives from the simulated hardware. This will result
> > > in i2c device probe failure, which in turn can cause all kinds of
> > > problems.
> > >
> >
> > Yes, that makes sense. Looks like it should be reverted until the issue
> > is fixed. Will submit a patch to revert it.
> 
> Let's not rush. The qemu model was written in order to allow us to
> test the kernel code, and was validated by the kernel driver we have.
> We've had situations in the past (with the i2c driver in fact) where a
> change in the driver required an update of the model to be more
> accurate.
> 
> I suggest we wait until Cedric has a chance to look at the issue
> before reverting the patch.
> 

Looking into the patch, clearing the interrupt status at the end of an
interrupt handler is always suspicious and tends to result in race
conditions (because additional interrupts may have arrived while handling
the existing interrupts, or because interrupt handling itself may trigger
another interrupt). With that in mind, the following patch fixes the
problem for me.

Guenter

---

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index c258c4d9a4c0..c488e6950b7c 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
 
 	spin_lock(&bus->lock);
 	irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
+	/* Ack all interrupt bits. */
+	writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
 	irq_remaining = irq_received;
 
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
@@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
 			"irq handled != irq. expected 0x%08x, but was 0x%08x\n",
 			irq_received, irq_handled);
 
-	/* Ack all interrupt bits. */
-	writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
 	spin_unlock(&bus->lock);
 	return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
 }

Jae Hyun Yoo Sept. 11, 2018, 11:58 p.m. UTC | #13

On 9/11/2018 4:33 PM, Guenter Roeck wrote:
> Looking into the patch, clearing the interrupt status at the end of an
> interrupt handler is always suspicious and tends to result in race
> conditions (because additional interrupts may have arrived while handling
> the existing interrupts, or because interrupt handling itself may trigger
> another interrupt). With that in mind, the following patch fixes the
> problem for me.
> 
> Guenter
> 
> ---
> 
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index c258c4d9a4c0..c488e6950b7c 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>   
>   	spin_lock(&bus->lock);
>   	irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> +	/* Ack all interrupt bits. */
> +	writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>   	irq_remaining = irq_received;
>   
>   #if IS_ENABLED(CONFIG_I2C_SLAVE)
> @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>   			"irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>   			irq_received, irq_handled);
>   
> -	/* Ack all interrupt bits. */
> -	writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>   	spin_unlock(&bus->lock);
>   	return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>   }
> 

My intention of putting the code at the end of interrupt handler was,
to reduce possibility of combined irq calls which is explained in this
patch. But YES, I agree with you. It could make a potential race
condition as you pointed out. I tested your code change and checked that
it works well. Let me take more sufficient test on real H/W. Will share
the test result.

Thanks a lot!

Jae

Guenter Roeck Sept. 12, 2018, 1:34 a.m. UTC | #14

On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote:
> On 9/11/2018 4:33 PM, Guenter Roeck wrote:
> >Looking into the patch, clearing the interrupt status at the end of an
> >interrupt handler is always suspicious and tends to result in race
> >conditions (because additional interrupts may have arrived while handling
> >the existing interrupts, or because interrupt handling itself may trigger
> >another interrupt). With that in mind, the following patch fixes the
> >problem for me.
> >
> >Guenter
> >
> >---
> >
> >diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> >index c258c4d9a4c0..c488e6950b7c 100644
> >--- a/drivers/i2c/busses/i2c-aspeed.c
> >+++ b/drivers/i2c/busses/i2c-aspeed.c
> >@@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> >  	spin_lock(&bus->lock);
> >  	irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> >+	/* Ack all interrupt bits. */
> >+	writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> >  	irq_remaining = irq_received;
> >  #if IS_ENABLED(CONFIG_I2C_SLAVE)
> >@@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> >  			"irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> >  			irq_received, irq_handled);
> >-	/* Ack all interrupt bits. */
> >-	writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> >  	spin_unlock(&bus->lock);
> >  	return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
> >  }
> >
> 
> My intention of putting the code at the end of interrupt handler was,
> to reduce possibility of combined irq calls which is explained in this
> patch. But YES, I agree with you. It could make a potential race

Hmm, yes, but that doesn't explain why it would make sense to acknowledge
the interrupt late. The interrupt ack only means "I am going to handle these
interrupts". If additional interrupts arrive while the interrupt handler
is active, those will have to be acknowledged separately.

Sure, there is a risk that an interrupt arrives while the handler is
running, and that it is handled but not acknowledged. That can happen
with pretty much all interrupt handlers, and there are mitigations to
limit the impact (for example, read the interrupt status register in
a loop until no more interrupts are pending). But acknowledging
an interrupt that was possibly not handled is always bad idea.

Thanks,
Guenter

Cédric Le Goater Sept. 12, 2018, 5:57 a.m. UTC | #15

On 09/12/2018 01:33 AM, Guenter Roeck wrote:
> On Wed, Sep 12, 2018 at 08:23:29AM +0930, Joel Stanley wrote:
>> On Wed, 12 Sep 2018 at 07:48, Jae Hyun Yoo <[e-mail]> wrote:
>>>
>>> On 9/11/2018 1:41 PM, Guenter Roeck wrote:
>>>> On Tue, Sep 11, 2018 at 01:30:41PM -0700, Jae Hyun Yoo wrote:
>>
>>>>> I checked this patch again but it doesn't have any change that could
>>>>> affect to the probing flow. I'll debug the issue on qemu 3.0 environment
>>>>> and will share if I find something.
>>>>>
>>>> The problem may be that qemu and the new code disagree how interrupts
>>>> should be generated and handled, and the new code does not handle the
>>>> interrupts it receives from the simulated hardware. This will result
>>>> in i2c device probe failure, which in turn can cause all kinds of
>>>> problems.
>>>>
>>>
>>> Yes, that makes sense. Looks like it should be reverted until the issue
>>> is fixed. Will submit a patch to revert it.
>>
>> Let's not rush. The qemu model was written in order to allow us to
>> test the kernel code, and was validated by the kernel driver we have.
>> We've had situations in the past (with the i2c driver in fact) where a
>> change in the driver required an update of the model to be more
>> accurate.
>>
>> I suggest we wait until Cedric has a chance to look at the issue
>> before reverting the patch.
>>
> 
> Looking into the patch, clearing the interrupt status at the end of an
> interrupt handler is always suspicious and tends to result in race

yes. That happened in the past with the I2C aspeed driver. I can not find
the thread anymore but we had to move up the ack of the interrupts. 

QEMU tends to be much faster to fire interrupts than real HW.


> conditions (because additional interrupts may have arrived while handling
> the existing interrupts, or because interrupt handling itself may trigger
> another interrupt). With that in mind, the following patch fixes the
> problem for me.

Acked-by: Cédric Le Goater <[e-mail]>

Thanks,

C.

> Guenter
> 
> ---
> 
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index c258c4d9a4c0..c488e6950b7c 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>  
>  	spin_lock(&bus->lock);
>  	irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> +	/* Ack all interrupt bits. */
> +	writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>  	irq_remaining = irq_received;
>  
>  #if IS_ENABLED(CONFIG_I2C_SLAVE)
> @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>  			"irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>  			irq_received, irq_handled);
>  
> -	/* Ack all interrupt bits. */
> -	writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>  	spin_unlock(&bus->lock);
>  	return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>  }
>

Jae Hyun Yoo Sept. 12, 2018, 4:54 p.m. UTC | #16

On 9/11/2018 6:34 PM, Guenter Roeck wrote:
> On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote:
>> On 9/11/2018 4:33 PM, Guenter Roeck wrote:
>>> Looking into the patch, clearing the interrupt status at the end of an
>>> interrupt handler is always suspicious and tends to result in race
>>> conditions (because additional interrupts may have arrived while handling
>>> the existing interrupts, or because interrupt handling itself may trigger
>>> another interrupt). With that in mind, the following patch fixes the
>>> problem for me.
>>>
>>> Guenter
>>>
>>> ---
>>>
>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>>> index c258c4d9a4c0..c488e6950b7c 100644
>>> --- a/drivers/i2c/busses/i2c-aspeed.c
>>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>>> @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>>   	spin_lock(&bus->lock);
>>>   	irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>>> +	/* Ack all interrupt bits. */
>>> +	writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>>   	irq_remaining = irq_received;
>>>   #if IS_ENABLED(CONFIG_I2C_SLAVE)
>>> @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>>   			"irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>>>   			irq_received, irq_handled);
>>> -	/* Ack all interrupt bits. */
>>> -	writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>>   	spin_unlock(&bus->lock);
>>>   	return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>>>   }
>>>
>>
>> My intention of putting the code at the end of interrupt handler was,
>> to reduce possibility of combined irq calls which is explained in this
>> patch. But YES, I agree with you. It could make a potential race
> 
> Hmm, yes, but that doesn't explain why it would make sense to acknowledge
> the interrupt late. The interrupt ack only means "I am going to handle these
> interrupts". If additional interrupts arrive while the interrupt handler
> is active, those will have to be acknowledged separately.
> 
> Sure, there is a risk that an interrupt arrives while the handler is
> running, and that it is handled but not acknowledged. That can happen
> with pretty much all interrupt handlers, and there are mitigations to
> limit the impact (for example, read the interrupt status register in
> a loop until no more interrupts are pending). But acknowledging
> an interrupt that was possibly not handled is always bad idea.

Well, that's generally right but not always. Sometimes that depends on
hardware and Aspeed I2C is the case.

This is a description from Aspeed AST2500 datasheet:
   I2CD10 Interrupt Status Register
   bit 2 Receive Done Interrupt status
         S/W needs to clear this status bit to allow next data receiving.

It means, driver should hold this bit to prevent transition of hardware
state machine until the driver handles received data, so the bit should
be cleared at the end of interrupt handler.

Let me share my test result. Your code change works on 100KHz bus speed
but doesn't work well on 1MHz bus speed. Investigated that interrupt
handling is fast enough in 100KHz test but in 1MHz, most of data is
corrupted because the bit is cleared at the beginning of interrupt
handler so it allows receiving of the next data but the interrupt
handler isn't fast enough to read the data buffer on time. I checked
this problem on BMC-ME channel which ME sends lots of IPMB packets to
BMC at 1MHz speed. You could simply check the data corruption problem on
the BMC-ME channel.

My thought is, the current code is right for real Aspeed I2C hardware.
It seems that QEMU 3.0 model for witherspoon-bmc doesn't simulate the
actual Aspeed I2C hardware correctly.

Thanks,
Jae

Guenter Roeck Sept. 12, 2018, 7:58 p.m. UTC | #17

On Wed, Sep 12, 2018 at 09:54:51AM -0700, Jae Hyun Yoo wrote:
> On 9/11/2018 6:34 PM, Guenter Roeck wrote:
> >On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote:
> >>On 9/11/2018 4:33 PM, Guenter Roeck wrote:
> >>>Looking into the patch, clearing the interrupt status at the end of an
> >>>interrupt handler is always suspicious and tends to result in race
> >>>conditions (because additional interrupts may have arrived while handling
> >>>the existing interrupts, or because interrupt handling itself may trigger
> >>>another interrupt). With that in mind, the following patch fixes the
> >>>problem for me.
> >>>
> >>>Guenter
> >>>
> >>>---
> >>>
> >>>diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> >>>index c258c4d9a4c0..c488e6950b7c 100644
> >>>--- a/drivers/i2c/busses/i2c-aspeed.c
> >>>+++ b/drivers/i2c/busses/i2c-aspeed.c
> >>>@@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> >>>  	spin_lock(&bus->lock);
> >>>  	irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> >>>+	/* Ack all interrupt bits. */
> >>>+	writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> >>>  	irq_remaining = irq_received;
> >>>  #if IS_ENABLED(CONFIG_I2C_SLAVE)
> >>>@@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> >>>  			"irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> >>>  			irq_received, irq_handled);
> >>>-	/* Ack all interrupt bits. */
> >>>-	writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> >>>  	spin_unlock(&bus->lock);
> >>>  	return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
> >>>  }
> >>>
> >>
> >>My intention of putting the code at the end of interrupt handler was,
> >>to reduce possibility of combined irq calls which is explained in this
> >>patch. But YES, I agree with you. It could make a potential race
> >
> >Hmm, yes, but that doesn't explain why it would make sense to acknowledge
> >the interrupt late. The interrupt ack only means "I am going to handle these
> >interrupts". If additional interrupts arrive while the interrupt handler
> >is active, those will have to be acknowledged separately.
> >
> >Sure, there is a risk that an interrupt arrives while the handler is
> >running, and that it is handled but not acknowledged. That can happen
> >with pretty much all interrupt handlers, and there are mitigations to
> >limit the impact (for example, read the interrupt status register in
> >a loop until no more interrupts are pending). But acknowledging
> >an interrupt that was possibly not handled is always bad idea.
> 
> Well, that's generally right but not always. Sometimes that depends on
> hardware and Aspeed I2C is the case.
> 
> This is a description from Aspeed AST2500 datasheet:
>   I2CD10 Interrupt Status Register
>   bit 2 Receive Done Interrupt status
>         S/W needs to clear this status bit to allow next data receiving.
> 
> It means, driver should hold this bit to prevent transition of hardware
> state machine until the driver handles received data, so the bit should
> be cleared at the end of interrupt handler.
> 
That makes sense. Does that apply to the other status bits as well ?
Reason for asking is that the current code actually gets stuck
in transmit, not receive.

Thanks,
Guenter

Jae Hyun Yoo Sept. 12, 2018, 8:10 p.m. UTC | #18

On 9/12/2018 12:58 PM, Guenter Roeck wrote:
> On Wed, Sep 12, 2018 at 09:54:51AM -0700, Jae Hyun Yoo wrote:
>> On 9/11/2018 6:34 PM, Guenter Roeck wrote:
>>> On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote:
>>>> On 9/11/2018 4:33 PM, Guenter Roeck wrote:
>>>>> Looking into the patch, clearing the interrupt status at the end of an
>>>>> interrupt handler is always suspicious and tends to result in race
>>>>> conditions (because additional interrupts may have arrived while handling
>>>>> the existing interrupts, or because interrupt handling itself may trigger
>>>>> another interrupt). With that in mind, the following patch fixes the
>>>>> problem for me.
>>>>>
>>>>> Guenter
>>>>>
>>>>> ---
>>>>>
>>>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>>>>> index c258c4d9a4c0..c488e6950b7c 100644
>>>>> --- a/drivers/i2c/busses/i2c-aspeed.c
>>>>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>>>>> @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>>>>   	spin_lock(&bus->lock);
>>>>>   	irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>> +	/* Ack all interrupt bits. */
>>>>> +	writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>>   	irq_remaining = irq_received;
>>>>>   #if IS_ENABLED(CONFIG_I2C_SLAVE)
>>>>> @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>>>>   			"irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>>>>>   			irq_received, irq_handled);
>>>>> -	/* Ack all interrupt bits. */
>>>>> -	writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>>   	spin_unlock(&bus->lock);
>>>>>   	return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>>>>>   }
>>>>>
>>>>
>>>> My intention of putting the code at the end of interrupt handler was,
>>>> to reduce possibility of combined irq calls which is explained in this
>>>> patch. But YES, I agree with you. It could make a potential race
>>>
>>> Hmm, yes, but that doesn't explain why it would make sense to acknowledge
>>> the interrupt late. The interrupt ack only means "I am going to handle these
>>> interrupts". If additional interrupts arrive while the interrupt handler
>>> is active, those will have to be acknowledged separately.
>>>
>>> Sure, there is a risk that an interrupt arrives while the handler is
>>> running, and that it is handled but not acknowledged. That can happen
>>> with pretty much all interrupt handlers, and there are mitigations to
>>> limit the impact (for example, read the interrupt status register in
>>> a loop until no more interrupts are pending). But acknowledging
>>> an interrupt that was possibly not handled is always bad idea.
>>
>> Well, that's generally right but not always. Sometimes that depends on
>> hardware and Aspeed I2C is the case.
>>
>> This is a description from Aspeed AST2500 datasheet:
>>    I2CD10 Interrupt Status Register
>>    bit 2 Receive Done Interrupt status
>>          S/W needs to clear this status bit to allow next data receiving.
>>
>> It means, driver should hold this bit to prevent transition of hardware
>> state machine until the driver handles received data, so the bit should
>> be cleared at the end of interrupt handler.
>>
> That makes sense. Does that apply to the other status bits as well ?
> Reason for asking is that the current code actually gets stuck
> in transmit, not receive.
> 
Only bit 2 has that description in datasheet. Is slave config enabled
for QEMU build? Does that get stuck in master sending or slave
receiving?

Thanks,
Jae

Guenter Roeck Sept. 12, 2018, 8:30 p.m. UTC | #19

On Wed, Sep 12, 2018 at 01:10:45PM -0700, Jae Hyun Yoo wrote:
> On 9/12/2018 12:58 PM, Guenter Roeck wrote:
> >On Wed, Sep 12, 2018 at 09:54:51AM -0700, Jae Hyun Yoo wrote:
> >>On 9/11/2018 6:34 PM, Guenter Roeck wrote:
> >>>On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote:
> >>>>On 9/11/2018 4:33 PM, Guenter Roeck wrote:
> >>>>>Looking into the patch, clearing the interrupt status at the end of an
> >>>>>interrupt handler is always suspicious and tends to result in race
> >>>>>conditions (because additional interrupts may have arrived while handling
> >>>>>the existing interrupts, or because interrupt handling itself may trigger
> >>>>>another interrupt). With that in mind, the following patch fixes the
> >>>>>problem for me.
> >>>>>
> >>>>>Guenter
> >>>>>
> >>>>>---
> >>>>>
> >>>>>diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> >>>>>index c258c4d9a4c0..c488e6950b7c 100644
> >>>>>--- a/drivers/i2c/busses/i2c-aspeed.c
> >>>>>+++ b/drivers/i2c/busses/i2c-aspeed.c
> >>>>>@@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> >>>>>  	spin_lock(&bus->lock);
> >>>>>  	irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> >>>>>+	/* Ack all interrupt bits. */
> >>>>>+	writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> >>>>>  	irq_remaining = irq_received;
> >>>>>  #if IS_ENABLED(CONFIG_I2C_SLAVE)
> >>>>>@@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> >>>>>  			"irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> >>>>>  			irq_received, irq_handled);
> >>>>>-	/* Ack all interrupt bits. */
> >>>>>-	writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> >>>>>  	spin_unlock(&bus->lock);
> >>>>>  	return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
> >>>>>  }
> >>>>>
> >>>>
> >>>>My intention of putting the code at the end of interrupt handler was,
> >>>>to reduce possibility of combined irq calls which is explained in this
> >>>>patch. But YES, I agree with you. It could make a potential race
> >>>
> >>>Hmm, yes, but that doesn't explain why it would make sense to acknowledge
> >>>the interrupt late. The interrupt ack only means "I am going to handle these
> >>>interrupts". If additional interrupts arrive while the interrupt handler
> >>>is active, those will have to be acknowledged separately.
> >>>
> >>>Sure, there is a risk that an interrupt arrives while the handler is
> >>>running, and that it is handled but not acknowledged. That can happen
> >>>with pretty much all interrupt handlers, and there are mitigations to
> >>>limit the impact (for example, read the interrupt status register in
> >>>a loop until no more interrupts are pending). But acknowledging
> >>>an interrupt that was possibly not handled is always bad idea.
> >>
> >>Well, that's generally right but not always. Sometimes that depends on
> >>hardware and Aspeed I2C is the case.
> >>
> >>This is a description from Aspeed AST2500 datasheet:
> >>   I2CD10 Interrupt Status Register
> >>   bit 2 Receive Done Interrupt status
> >>         S/W needs to clear this status bit to allow next data receiving.
> >>
> >>It means, driver should hold this bit to prevent transition of hardware
> >>state machine until the driver handles received data, so the bit should
> >>be cleared at the end of interrupt handler.
> >>
> >That makes sense. Does that apply to the other status bits as well ?
> >Reason for asking is that the current code actually gets stuck
> >in transmit, not receive.
> >
> Only bit 2 has that description in datasheet. Is slave config enabled
> for QEMU build? Does that get stuck in master sending or slave
> receiving?
> 
qemu does not support slave mode. Linux gets stuck in master tx.

I played with the code on both sides. I had to make changes in both
the linux kernel and in qemu to get the code to work again.
See attached.

Guenter

---
Linux:

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index c258c4d9a4c0..3d518e09369f 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -552,6 +552,9 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
 
 	spin_lock(&bus->lock);
 	irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
+	/* Ack all interrupts except for Rx done */
+	writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
+	       bus->base + ASPEED_I2C_INTR_STS_REG);
 	irq_remaining = irq_received;
 
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
@@ -584,8 +587,10 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
 			"irq handled != irq. expected 0x%08x, but was 0x%08x\n",
 			irq_received, irq_handled);
 
-	/* Ack all interrupt bits. */
-	writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
+	/* Ack Rx done */
+	if (irq_received & ASPEED_I2CD_INTR_RX_DONE)
+		writel(ASPEED_I2CD_INTR_RX_DONE,
+		       bus->base + ASPEED_I2C_INTR_STS_REG);
 	spin_unlock(&bus->lock);
 	return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
 }

---
qemu:

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index c762c73..0d4aa08 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -180,6 +180,33 @@ static uint8_t aspeed_i2c_get_state(AspeedI2CBus *bus)
     return (bus->cmd >> I2CD_TX_STATE_SHIFT) & I2CD_TX_STATE_MASK;
 }
 
+static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus)
+{
+    int ret;
+
+    if (!(bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST))) {
+        return;
+    }
+    if (bus->intr_status & I2CD_INTR_RX_DONE) {
+        return;
+    }
+
+    aspeed_i2c_set_state(bus, I2CD_MRXD);
+    ret = i2c_recv(bus->bus);
+    if (ret < 0) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
+        ret = 0xff;
+    } else {
+        bus->intr_status |= I2CD_INTR_RX_DONE;
+    }
+    bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
+    if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
+        i2c_nack(bus->bus);
+    }
+    bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
+    aspeed_i2c_set_state(bus, I2CD_MACTIVE);
+}
+
 /*
  * The state machine needs some refinement. It is only used to track
  * invalid STOP commands for the moment.
@@ -188,7 +215,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
 {
     bus->cmd &= ~0xFFFF;
     bus->cmd |= value & 0xFFFF;
-    bus->intr_status = 0;
+    bus->intr_status &= I2CD_INTR_RX_DONE;
 
     if (bus->cmd & I2CD_M_START_CMD) {
         uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
@@ -227,22 +254,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
     }
 
     if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) {
-        int ret;
-
-        aspeed_i2c_set_state(bus, I2CD_MRXD);
-        ret = i2c_recv(bus->bus);
-        if (ret < 0) {
-            qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
-            ret = 0xff;
-        } else {
-            bus->intr_status |= I2CD_INTR_RX_DONE;
-        }
-        bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
-        if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
-            i2c_nack(bus->bus);
-        }
-        bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
-        aspeed_i2c_set_state(bus, I2CD_MACTIVE);
+        aspeed_i2c_handle_rx_cmd(bus);
     }
 
     if (bus->cmd & I2CD_M_STOP_CMD) {
@@ -263,6 +275,7 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
                                  uint64_t value, unsigned size)
 {
     AspeedI2CBus *bus = opaque;
+    int status;
 
     switch (offset) {
     case I2CD_FUN_CTRL_REG:
@@ -283,9 +296,16 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
         bus->intr_ctrl = value & 0x7FFF;
         break;
     case I2CD_INTR_STS_REG:
+        status = bus->intr_status;
         bus->intr_status &= ~(value & 0x7FFF);
-        bus->controller->intr_status &= ~(1 << bus->id);
-        qemu_irq_lower(bus->controller->irq);
+        if (!bus->intr_status) {
+            bus->controller->intr_status &= ~(1 << bus->id);
+            qemu_irq_lower(bus->controller->irq);
+        }
+        if ((status & I2CD_INTR_RX_DONE) && !(bus->intr_status & I2CD_INTR_RX_DONE)) {
+            aspeed_i2c_handle_rx_cmd(bus);
+            aspeed_i2c_bus_raise_interrupt(bus);
+        }
         break;
     case I2CD_DEV_ADDR_REG:
         qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",

Jae Hyun Yoo Sept. 12, 2018, 10:31 p.m. UTC | #20

On 9/12/2018 1:30 PM, Guenter Roeck wrote:
> On Wed, Sep 12, 2018 at 01:10:45PM -0700, Jae Hyun Yoo wrote:
>> On 9/12/2018 12:58 PM, Guenter Roeck wrote:
>>> On Wed, Sep 12, 2018 at 09:54:51AM -0700, Jae Hyun Yoo wrote:
>>>> On 9/11/2018 6:34 PM, Guenter Roeck wrote:
>>>>> On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote:
>>>>>> On 9/11/2018 4:33 PM, Guenter Roeck wrote:
>>>>>>> Looking into the patch, clearing the interrupt status at the end of an
>>>>>>> interrupt handler is always suspicious and tends to result in race
>>>>>>> conditions (because additional interrupts may have arrived while handling
>>>>>>> the existing interrupts, or because interrupt handling itself may trigger
>>>>>>> another interrupt). With that in mind, the following patch fixes the
>>>>>>> problem for me.
>>>>>>>
>>>>>>> Guenter
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>>>>>>> index c258c4d9a4c0..c488e6950b7c 100644
>>>>>>> --- a/drivers/i2c/busses/i2c-aspeed.c
>>>>>>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>>>>>>> @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>>>>>>   	spin_lock(&bus->lock);
>>>>>>>   	irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>>>> +	/* Ack all interrupt bits. */
>>>>>>> +	writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>>>>   	irq_remaining = irq_received;
>>>>>>>   #if IS_ENABLED(CONFIG_I2C_SLAVE)
>>>>>>> @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>>>>>>   			"irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>>>>>>>   			irq_received, irq_handled);
>>>>>>> -	/* Ack all interrupt bits. */
>>>>>>> -	writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>>>>   	spin_unlock(&bus->lock);
>>>>>>>   	return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>>>>>>>   }
>>>>>>>
>>>>>>
>>>>>> My intention of putting the code at the end of interrupt handler was,
>>>>>> to reduce possibility of combined irq calls which is explained in this
>>>>>> patch. But YES, I agree with you. It could make a potential race
>>>>>
>>>>> Hmm, yes, but that doesn't explain why it would make sense to acknowledge
>>>>> the interrupt late. The interrupt ack only means "I am going to handle these
>>>>> interrupts". If additional interrupts arrive while the interrupt handler
>>>>> is active, those will have to be acknowledged separately.
>>>>>
>>>>> Sure, there is a risk that an interrupt arrives while the handler is
>>>>> running, and that it is handled but not acknowledged. That can happen
>>>>> with pretty much all interrupt handlers, and there are mitigations to
>>>>> limit the impact (for example, read the interrupt status register in
>>>>> a loop until no more interrupts are pending). But acknowledging
>>>>> an interrupt that was possibly not handled is always bad idea.
>>>>
>>>> Well, that's generally right but not always. Sometimes that depends on
>>>> hardware and Aspeed I2C is the case.
>>>>
>>>> This is a description from Aspeed AST2500 datasheet:
>>>>    I2CD10 Interrupt Status Register
>>>>    bit 2 Receive Done Interrupt status
>>>>          S/W needs to clear this status bit to allow next data receiving.
>>>>
>>>> It means, driver should hold this bit to prevent transition of hardware
>>>> state machine until the driver handles received data, so the bit should
>>>> be cleared at the end of interrupt handler.
>>>>
>>> That makes sense. Does that apply to the other status bits as well ?
>>> Reason for asking is that the current code actually gets stuck
>>> in transmit, not receive.
>>>
>> Only bit 2 has that description in datasheet. Is slave config enabled
>> for QEMU build? Does that get stuck in master sending or slave
>> receiving?
>>
> qemu does not support slave mode. Linux gets stuck in master tx.
> 
> I played with the code on both sides. I had to make changes in both
> the linux kernel and in qemu to get the code to work again.
> See attached.
> 
> Guenter
> 
> ---
> Linux:
> 
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index c258c4d9a4c0..3d518e09369f 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -552,6 +552,9 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>   
>   	spin_lock(&bus->lock);
>   	irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> +	/* Ack all interrupts except for Rx done */
> +	writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
> +	       bus->base + ASPEED_I2C_INTR_STS_REG);
>   	irq_remaining = irq_received;
>   
>   #if IS_ENABLED(CONFIG_I2C_SLAVE)
> @@ -584,8 +587,10 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>   			"irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>   			irq_received, irq_handled);
>   
> -	/* Ack all interrupt bits. */
> -	writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> +	/* Ack Rx done */
> +	if (irq_received & ASPEED_I2CD_INTR_RX_DONE)
> +		writel(ASPEED_I2CD_INTR_RX_DONE,
> +		       bus->base + ASPEED_I2C_INTR_STS_REG);
>   	spin_unlock(&bus->lock);
>   	return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>   }
> 
> ---
> qemu:
> 
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index c762c73..0d4aa08 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -180,6 +180,33 @@ static uint8_t aspeed_i2c_get_state(AspeedI2CBus *bus)
>       return (bus->cmd >> I2CD_TX_STATE_SHIFT) & I2CD_TX_STATE_MASK;
>   }
>   
> +static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus)
> +{
> +    int ret;
> +
> +    if (!(bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST))) {
> +        return;
> +    }
> +    if (bus->intr_status & I2CD_INTR_RX_DONE) {
> +        return;
> +    }
> +
> +    aspeed_i2c_set_state(bus, I2CD_MRXD);
> +    ret = i2c_recv(bus->bus);
> +    if (ret < 0) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
> +        ret = 0xff;
> +    } else {
> +        bus->intr_status |= I2CD_INTR_RX_DONE;
> +    }
> +    bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
> +    if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
> +        i2c_nack(bus->bus);
> +    }
> +    bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
> +    aspeed_i2c_set_state(bus, I2CD_MACTIVE);
> +}
> +
>   /*
>    * The state machine needs some refinement. It is only used to track
>    * invalid STOP commands for the moment.
> @@ -188,7 +215,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>   {
>       bus->cmd &= ~0xFFFF;
>       bus->cmd |= value & 0xFFFF;
> -    bus->intr_status = 0;
> +    bus->intr_status &= I2CD_INTR_RX_DONE;
>   
>       if (bus->cmd & I2CD_M_START_CMD) {
>           uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
> @@ -227,22 +254,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>       }
>   
>       if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) {
> -        int ret;
> -
> -        aspeed_i2c_set_state(bus, I2CD_MRXD);
> -        ret = i2c_recv(bus->bus);
> -        if (ret < 0) {
> -            qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
> -            ret = 0xff;
> -        } else {
> -            bus->intr_status |= I2CD_INTR_RX_DONE;
> -        }
> -        bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
> -        if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
> -            i2c_nack(bus->bus);
> -        }
> -        bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
> -        aspeed_i2c_set_state(bus, I2CD_MACTIVE);
> +        aspeed_i2c_handle_rx_cmd(bus);
>       }
>   
>       if (bus->cmd & I2CD_M_STOP_CMD) {
> @@ -263,6 +275,7 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
>                                    uint64_t value, unsigned size)
>   {
>       AspeedI2CBus *bus = opaque;
> +    int status;
>   
>       switch (offset) {
>       case I2CD_FUN_CTRL_REG:
> @@ -283,9 +296,16 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
>           bus->intr_ctrl = value & 0x7FFF;
>           break;
>       case I2CD_INTR_STS_REG:
> +        status = bus->intr_status;
>           bus->intr_status &= ~(value & 0x7FFF);
> -        bus->controller->intr_status &= ~(1 << bus->id);
> -        qemu_irq_lower(bus->controller->irq);
> +        if (!bus->intr_status) {
> +            bus->controller->intr_status &= ~(1 << bus->id);
> +            qemu_irq_lower(bus->controller->irq);
> +        }
> +        if ((status & I2CD_INTR_RX_DONE) && !(bus->intr_status & I2CD_INTR_RX_DONE)) {
> +            aspeed_i2c_handle_rx_cmd(bus);
> +            aspeed_i2c_bus_raise_interrupt(bus);
> +        }
>           break;
>       case I2CD_DEV_ADDR_REG:
>           qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
> 

Nice fix! LGTM. I've tested the new patch and checked that it works well
on both low and high bus speed environments. Thanks a lot!

Can you please submit this patch?

Thanks,
Jae

Guenter Roeck Sept. 12, 2018, 11:30 p.m. UTC | #21

On Wed, Sep 12, 2018 at 03:31:06PM -0700, Jae Hyun Yoo wrote:
> >
> >I played with the code on both sides. I had to make changes in both
> >the linux kernel and in qemu to get the code to work again.
> >See attached.
> >
> >Guenter
> >
> >---
> >Linux:
> >
> >diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> >index c258c4d9a4c0..3d518e09369f 100644
> >--- a/drivers/i2c/busses/i2c-aspeed.c
> >+++ b/drivers/i2c/busses/i2c-aspeed.c
> >@@ -552,6 +552,9 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> >  	spin_lock(&bus->lock);
> >  	irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> >+	/* Ack all interrupts except for Rx done */
> >+	writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
> >+	       bus->base + ASPEED_I2C_INTR_STS_REG);
> >  	irq_remaining = irq_received;
> >  #if IS_ENABLED(CONFIG_I2C_SLAVE)
> >@@ -584,8 +587,10 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
> >  			"irq handled != irq. expected 0x%08x, but was 0x%08x\n",
> >  			irq_received, irq_handled);
> >-	/* Ack all interrupt bits. */
> >-	writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> >+	/* Ack Rx done */
> >+	if (irq_received & ASPEED_I2CD_INTR_RX_DONE)
> >+		writel(ASPEED_I2CD_INTR_RX_DONE,
> >+		       bus->base + ASPEED_I2C_INTR_STS_REG);
> >  	spin_unlock(&bus->lock);
> >  	return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
> >  }
> >
> >---
> >qemu:
> >
> >diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> >index c762c73..0d4aa08 100644
> >--- a/hw/i2c/aspeed_i2c.c
> >+++ b/hw/i2c/aspeed_i2c.c
> >@@ -180,6 +180,33 @@ static uint8_t aspeed_i2c_get_state(AspeedI2CBus *bus)
> >      return (bus->cmd >> I2CD_TX_STATE_SHIFT) & I2CD_TX_STATE_MASK;
> >  }
> >+static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus)
> >+{
> >+    int ret;
> >+
> >+    if (!(bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST))) {
> >+        return;
> >+    }
> >+    if (bus->intr_status & I2CD_INTR_RX_DONE) {
> >+        return;
> >+    }
> >+
> >+    aspeed_i2c_set_state(bus, I2CD_MRXD);
> >+    ret = i2c_recv(bus->bus);
> >+    if (ret < 0) {
> >+        qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
> >+        ret = 0xff;
> >+    } else {
> >+        bus->intr_status |= I2CD_INTR_RX_DONE;
> >+    }
> >+    bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
> >+    if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
> >+        i2c_nack(bus->bus);
> >+    }
> >+    bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
> >+    aspeed_i2c_set_state(bus, I2CD_MACTIVE);
> >+}
> >+
> >  /*
> >   * The state machine needs some refinement. It is only used to track
> >   * invalid STOP commands for the moment.
> >@@ -188,7 +215,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
> >  {
> >      bus->cmd &= ~0xFFFF;
> >      bus->cmd |= value & 0xFFFF;
> >-    bus->intr_status = 0;
> >+    bus->intr_status &= I2CD_INTR_RX_DONE;
> >      if (bus->cmd & I2CD_M_START_CMD) {
> >          uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
> >@@ -227,22 +254,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
> >      }
> >      if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) {
> >-        int ret;
> >-
> >-        aspeed_i2c_set_state(bus, I2CD_MRXD);
> >-        ret = i2c_recv(bus->bus);
> >-        if (ret < 0) {
> >-            qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
> >-            ret = 0xff;
> >-        } else {
> >-            bus->intr_status |= I2CD_INTR_RX_DONE;
> >-        }
> >-        bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
> >-        if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
> >-            i2c_nack(bus->bus);
> >-        }
> >-        bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
> >-        aspeed_i2c_set_state(bus, I2CD_MACTIVE);
> >+        aspeed_i2c_handle_rx_cmd(bus);
> >      }
> >      if (bus->cmd & I2CD_M_STOP_CMD) {
> >@@ -263,6 +275,7 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
> >                                   uint64_t value, unsigned size)
> >  {
> >      AspeedI2CBus *bus = opaque;
> >+    int status;
> >      switch (offset) {
> >      case I2CD_FUN_CTRL_REG:
> >@@ -283,9 +296,16 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
> >          bus->intr_ctrl = value & 0x7FFF;
> >          break;
> >      case I2CD_INTR_STS_REG:
> >+        status = bus->intr_status;
> >          bus->intr_status &= ~(value & 0x7FFF);
> >-        bus->controller->intr_status &= ~(1 << bus->id);
> >-        qemu_irq_lower(bus->controller->irq);
> >+        if (!bus->intr_status) {
> >+            bus->controller->intr_status &= ~(1 << bus->id);
> >+            qemu_irq_lower(bus->controller->irq);
> >+        }
> >+        if ((status & I2CD_INTR_RX_DONE) && !(bus->intr_status & I2CD_INTR_RX_DONE)) {
> >+            aspeed_i2c_handle_rx_cmd(bus);
> >+            aspeed_i2c_bus_raise_interrupt(bus);
> >+        }
> >          break;
> >      case I2CD_DEV_ADDR_REG:
> >          qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
> >
> 
> Nice fix! LGTM. I've tested the new patch and checked that it works well
> on both low and high bus speed environments. Thanks a lot!
> 
> Can you please submit this patch?
> 
Assuming you mean both patches, sure, can do. I'll need to clean up
the qemu patch a bit, though; it looks too messy for my liking.

Guenter

Cédric Le Goater Sept. 13, 2018, 5:45 a.m. UTC | #22

On 09/12/2018 10:30 PM, Guenter Roeck wrote:
> On Wed, Sep 12, 2018 at 01:10:45PM -0700, Jae Hyun Yoo wrote:
>> On 9/12/2018 12:58 PM, Guenter Roeck wrote:
>>> On Wed, Sep 12, 2018 at 09:54:51AM -0700, Jae Hyun Yoo wrote:
>>>> On 9/11/2018 6:34 PM, Guenter Roeck wrote:
>>>>> On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote:
>>>>>> On 9/11/2018 4:33 PM, Guenter Roeck wrote:
>>>>>>> Looking into the patch, clearing the interrupt status at the end of an
>>>>>>> interrupt handler is always suspicious and tends to result in race
>>>>>>> conditions (because additional interrupts may have arrived while handling
>>>>>>> the existing interrupts, or because interrupt handling itself may trigger
>>>>>>> another interrupt). With that in mind, the following patch fixes the
>>>>>>> problem for me.
>>>>>>>
>>>>>>> Guenter
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>>>>>>> index c258c4d9a4c0..c488e6950b7c 100644
>>>>>>> --- a/drivers/i2c/busses/i2c-aspeed.c
>>>>>>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>>>>>>> @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>>>>>>  	spin_lock(&bus->lock);
>>>>>>>  	irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>>>> +	/* Ack all interrupt bits. */
>>>>>>> +	writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>>>>  	irq_remaining = irq_received;
>>>>>>>  #if IS_ENABLED(CONFIG_I2C_SLAVE)
>>>>>>> @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>>>>>>  			"irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>>>>>>>  			irq_received, irq_handled);
>>>>>>> -	/* Ack all interrupt bits. */
>>>>>>> -	writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>>>>  	spin_unlock(&bus->lock);
>>>>>>>  	return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>>>>>>>  }
>>>>>>>
>>>>>>
>>>>>> My intention of putting the code at the end of interrupt handler was,
>>>>>> to reduce possibility of combined irq calls which is explained in this
>>>>>> patch. But YES, I agree with you. It could make a potential race
>>>>>
>>>>> Hmm, yes, but that doesn't explain why it would make sense to acknowledge
>>>>> the interrupt late. The interrupt ack only means "I am going to handle these
>>>>> interrupts". If additional interrupts arrive while the interrupt handler
>>>>> is active, those will have to be acknowledged separately.
>>>>>
>>>>> Sure, there is a risk that an interrupt arrives while the handler is
>>>>> running, and that it is handled but not acknowledged. That can happen
>>>>> with pretty much all interrupt handlers, and there are mitigations to
>>>>> limit the impact (for example, read the interrupt status register in
>>>>> a loop until no more interrupts are pending). But acknowledging
>>>>> an interrupt that was possibly not handled is always bad idea.
>>>>
>>>> Well, that's generally right but not always. Sometimes that depends on
>>>> hardware and Aspeed I2C is the case.
>>>>
>>>> This is a description from Aspeed AST2500 datasheet:
>>>>   I2CD10 Interrupt Status Register
>>>>   bit 2 Receive Done Interrupt status
>>>>         S/W needs to clear this status bit to allow next data receiving.
>>>>
>>>> It means, driver should hold this bit to prevent transition of hardware
>>>> state machine until the driver handles received data, so the bit should
>>>> be cleared at the end of interrupt handler.
>>>>
>>> That makes sense. Does that apply to the other status bits as well ?
>>> Reason for asking is that the current code actually gets stuck
>>> in transmit, not receive.
>>>
>> Only bit 2 has that description in datasheet. Is slave config enabled
>> for QEMU build? Does that get stuck in master sending or slave
>> receiving?
>>
> qemu does not support slave mode. Linux gets stuck in master tx.
> 
> I played with the code on both sides. I had to make changes in both
> the linux kernel and in qemu to get the code to work again.
> See attached.
> 
> Guenter
> 
> ---
> Linux:
> 
> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
> index c258c4d9a4c0..3d518e09369f 100644
> --- a/drivers/i2c/busses/i2c-aspeed.c
> +++ b/drivers/i2c/busses/i2c-aspeed.c
> @@ -552,6 +552,9 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>  
>  	spin_lock(&bus->lock);
>  	irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
> +	/* Ack all interrupts except for Rx done */
> +	writel(irq_received & ~ASPEED_I2CD_INTR_RX_DONE,
> +	       bus->base + ASPEED_I2C_INTR_STS_REG);
>  	irq_remaining = irq_received;
>  
>  #if IS_ENABLED(CONFIG_I2C_SLAVE)
> @@ -584,8 +587,10 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>  			"irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>  			irq_received, irq_handled);
>  
> -	/* Ack all interrupt bits. */
> -	writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
> +	/* Ack Rx done */
> +	if (irq_received & ASPEED_I2CD_INTR_RX_DONE)
> +		writel(ASPEED_I2CD_INTR_RX_DONE,
> +		       bus->base + ASPEED_I2C_INTR_STS_REG);
>  	spin_unlock(&bus->lock);
>  	return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>  }
> 
> ---
> qemu:
> 
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index c762c73..0d4aa08 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -180,6 +180,33 @@ static uint8_t aspeed_i2c_get_state(AspeedI2CBus *bus)
>      return (bus->cmd >> I2CD_TX_STATE_SHIFT) & I2CD_TX_STATE_MASK;
>  }
>  
> +static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus)
> +{
> +    int ret;
> +
> +    if (!(bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST))) {
> +        return;
> +    }

it deserves a comment to understand which scenario we are trying to handle.

> +    if (bus->intr_status & I2CD_INTR_RX_DONE) {
> +        return;
> +    }

should be handled in aspeed_i2c_bus_handle_cmd() I think

> +    aspeed_i2c_set_state(bus, I2CD_MRXD);
> +    ret = i2c_recv(bus->bus);
> +    if (ret < 0) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
> +        ret = 0xff;
> +    } else {
> +        bus->intr_status |= I2CD_INTR_RX_DONE;
> +    }
> +    bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
> +    if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
> +        i2c_nack(bus->bus);
> +    }
> +    bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
> +    aspeed_i2c_set_state(bus, I2CD_MACTIVE);
> +}
> +
>  /*
>   * The state machine needs some refinement. It is only used to track
>   * invalid STOP commands for the moment.
> @@ -188,7 +215,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>  {
>      bus->cmd &= ~0xFFFF;
>      bus->cmd |= value & 0xFFFF;
> -    bus->intr_status = 0;> +    bus->intr_status &= I2CD_INTR_RX_DONE;

it deserves a comment to understand which scenario we are trying to handle.
  
>      if (bus->cmd & I2CD_M_START_CMD) {
>          uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
> @@ -227,22 +254,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>      }
>  
>      if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) {
> -        int ret;
> -
> -        aspeed_i2c_set_state(bus, I2CD_MRXD);
> -        ret = i2c_recv(bus->bus);
> -        if (ret < 0) {
> -            qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
> -            ret = 0xff;
> -        } else {
> -            bus->intr_status |= I2CD_INTR_RX_DONE;
> -        }
> -        bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
> -        if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
> -            i2c_nack(bus->bus);
> -        }
> -        bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
> -        aspeed_i2c_set_state(bus, I2CD_MACTIVE);
> +        aspeed_i2c_handle_rx_cmd(bus);
>      }
>  
>      if (bus->cmd & I2CD_M_STOP_CMD) {
> @@ -263,6 +275,7 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
>                                   uint64_t value, unsigned size)
>  {
>      AspeedI2CBus *bus = opaque;
> +    int status;
>  
>      switch (offset) {
>      case I2CD_FUN_CTRL_REG:
> @@ -283,9 +296,16 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
>          bus->intr_ctrl = value & 0x7FFF;
>          break;
>      case I2CD_INTR_STS_REG:
> +        status = bus->intr_status;
>          bus->intr_status &= ~(value & 0x7FFF);
> -        bus->controller->intr_status &= ~(1 << bus->id);
> -        qemu_irq_lower(bus->controller->irq);
> +        if (!bus->intr_status) {
> +            bus->controller->intr_status &= ~(1 << bus->id);
> +            qemu_irq_lower(bus->controller->irq);
> +        }

That part below is indeed something to fix. I had a similar patch.


> +        if ((status & I2CD_INTR_RX_DONE) && !(bus->intr_status & I2CD_INTR_RX_DONE)) {
> +            aspeed_i2c_handle_rx_cmd(bus);
> +            aspeed_i2c_bus_raise_interrupt(bus);
> +        }

ok.

Thanks for looking into this.

C.

>          break;
>      case I2CD_DEV_ADDR_REG:
>          qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
>

Cédric Le Goater Sept. 13, 2018, 5:47 a.m. UTC | #23

On 09/12/2018 06:54 PM, Jae Hyun Yoo wrote:
> On 9/11/2018 6:34 PM, Guenter Roeck wrote:
>> On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote:
>>> On 9/11/2018 4:33 PM, Guenter Roeck wrote:
>>>> Looking into the patch, clearing the interrupt status at the end of an
>>>> interrupt handler is always suspicious and tends to result in race
>>>> conditions (because additional interrupts may have arrived while handling
>>>> the existing interrupts, or because interrupt handling itself may trigger
>>>> another interrupt). With that in mind, the following patch fixes the
>>>> problem for me.
>>>>
>>>> Guenter
>>>>
>>>> ---
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>>>> index c258c4d9a4c0..c488e6950b7c 100644
>>>> --- a/drivers/i2c/busses/i2c-aspeed.c
>>>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>>>> @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>>>       spin_lock(&bus->lock);
>>>>       irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>>>> +    /* Ack all interrupt bits. */
>>>> +    writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>       irq_remaining = irq_received;
>>>>   #if IS_ENABLED(CONFIG_I2C_SLAVE)
>>>> @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>>>               "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>>>>               irq_received, irq_handled);
>>>> -    /* Ack all interrupt bits. */
>>>> -    writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>       spin_unlock(&bus->lock);
>>>>       return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>>>>   }
>>>>
>>>
>>> My intention of putting the code at the end of interrupt handler was,
>>> to reduce possibility of combined irq calls which is explained in this
>>> patch. But YES, I agree with you. It could make a potential race
>>
>> Hmm, yes, but that doesn't explain why it would make sense to acknowledge
>> the interrupt late. The interrupt ack only means "I am going to handle these
>> interrupts". If additional interrupts arrive while the interrupt handler
>> is active, those will have to be acknowledged separately.
>>
>> Sure, there is a risk that an interrupt arrives while the handler is
>> running, and that it is handled but not acknowledged. That can happen
>> with pretty much all interrupt handlers, and there are mitigations to
>> limit the impact (for example, read the interrupt status register in
>> a loop until no more interrupts are pending). But acknowledging
>> an interrupt that was possibly not handled is always bad idea.
> 
> Well, that's generally right but not always. Sometimes that depends on
> hardware and Aspeed I2C is the case.
> 
> This is a description from Aspeed AST2500 datasheet:
>   I2CD10 Interrupt Status Register
>   bit 2 Receive Done Interrupt status
>         S/W needs to clear this status bit to allow next data receiving.
> 
> It means, driver should hold this bit to prevent transition of hardware
> state machine until the driver handles received data, so the bit should
> be cleared at the end of interrupt handler.
> 
> Let me share my test result. Your code change works on 100KHz bus speed
> but doesn't work well on 1MHz bus speed. Investigated that interrupt
> handling is fast enough in 100KHz test but in 1MHz, most of data is
> corrupted because the bit is cleared at the beginning of interrupt
> handler so it allows receiving of the next data but the interrupt
> handler isn't fast enough to read the data buffer on time. I checked
> this problem on BMC-ME channel which ME sends lots of IPMB packets to
> BMC at 1MHz speed. You could simply check the data corruption problem on
> the BMC-ME channel.

OK.
 
> My thought is, the current code is right for real Aspeed I2C hardware.
> It seems that QEMU 3.0 model for witherspoon-bmc doesn't simulate the
> actual Aspeed I2C hardware correctly.

That might be very well possible yes. it also misses support for the slave 
mode and the DMA registers.

Thanks for the info,

C.

Guenter Roeck Sept. 13, 2018, 1:33 p.m. UTC | #24

On 09/12/2018 10:45 PM, Cédric Le Goater wrote

[ ... ]

>> ---
>> qemu:
>>
>> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
>> index c762c73..0d4aa08 100644
>> --- a/hw/i2c/aspeed_i2c.c
>> +++ b/hw/i2c/aspeed_i2c.c
>> @@ -180,6 +180,33 @@ static uint8_t aspeed_i2c_get_state(AspeedI2CBus *bus)
>>       return (bus->cmd >> I2CD_TX_STATE_SHIFT) & I2CD_TX_STATE_MASK;
>>   }
>>   
>> +static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus)
>> +{
>> +    int ret;
>> +
>> +    if (!(bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST))) {
>> +        return;
>> +    }
> 
> it deserves a comment to understand which scenario we are trying to handle.
> 
>> +    if (bus->intr_status & I2CD_INTR_RX_DONE) {
>> +        return;
>> +    }
> 
> should be handled in aspeed_i2c_bus_handle_cmd() I think
> 

I moved those two checks into the calling code.


>> +    aspeed_i2c_set_state(bus, I2CD_MRXD);
>> +    ret = i2c_recv(bus->bus);
>> +    if (ret < 0) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
>> +        ret = 0xff;
>> +    } else {
>> +        bus->intr_status |= I2CD_INTR_RX_DONE;
>> +    }
>> +    bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
>> +    if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
>> +        i2c_nack(bus->bus);
>> +    }
>> +    bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
>> +    aspeed_i2c_set_state(bus, I2CD_MACTIVE);
>> +}
>> +
>>   /*
>>    * The state machine needs some refinement. It is only used to track
>>    * invalid STOP commands for the moment.
>> @@ -188,7 +215,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>>   {
>>       bus->cmd &= ~0xFFFF;
>>       bus->cmd |= value & 0xFFFF;
>> -    bus->intr_status = 0;> +    bus->intr_status &= I2CD_INTR_RX_DONE;
> 
> it deserves a comment to understand which scenario we are trying to handle.
>    

Ok. FWIW, I wonder if intr_status should be touched here in the first place,
but I neither have the hardware nor a datasheet, so I don't know if any bits
are auto-cleared.

>>       if (bus->cmd & I2CD_M_START_CMD) {
>>           uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
>> @@ -227,22 +254,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>>       }
>>   
>>       if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) {
>> -        int ret;
>> -
>> -        aspeed_i2c_set_state(bus, I2CD_MRXD);
>> -        ret = i2c_recv(bus->bus);
>> -        if (ret < 0) {
>> -            qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
>> -            ret = 0xff;
>> -        } else {
>> -            bus->intr_status |= I2CD_INTR_RX_DONE;
>> -        }
>> -        bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
>> -        if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
>> -            i2c_nack(bus->bus);
>> -        }
>> -        bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
>> -        aspeed_i2c_set_state(bus, I2CD_MACTIVE);
>> +        aspeed_i2c_handle_rx_cmd(bus);
>>       }
>>   
>>       if (bus->cmd & I2CD_M_STOP_CMD) {
>> @@ -263,6 +275,7 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
>>                                    uint64_t value, unsigned size)
>>   {
>>       AspeedI2CBus *bus = opaque;
>> +    int status;
>>   
>>       switch (offset) {
>>       case I2CD_FUN_CTRL_REG:
>> @@ -283,9 +296,16 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
>>           bus->intr_ctrl = value & 0x7FFF;
>>           break;
>>       case I2CD_INTR_STS_REG:
>> +        status = bus->intr_status;
>>           bus->intr_status &= ~(value & 0x7FFF);
>> -        bus->controller->intr_status &= ~(1 << bus->id);
>> -        qemu_irq_lower(bus->controller->irq);
>> +        if (!bus->intr_status) {
>> +            bus->controller->intr_status &= ~(1 << bus->id);
>> +            qemu_irq_lower(bus->controller->irq);
>> +        }
> 
> That part below is indeed something to fix. I had a similar patch.
> 

Should I split it out as separate patch ? Alternatively, if you submitted
your patch already, I'll be happy to use it as base for mine.

Thanks,
Guenter

> 
>> +        if ((status & I2CD_INTR_RX_DONE) && !(bus->intr_status & I2CD_INTR_RX_DONE)) {
>> +            aspeed_i2c_handle_rx_cmd(bus);
>> +            aspeed_i2c_bus_raise_interrupt(bus);
>> +        }
> 
> ok.
> 
> Thanks for looking into this.
> 
> C.
> 
>>           break;
>>       case I2CD_DEV_ADDR_REG:
>>           qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
>>
> 
>

Cédric Le Goater Sept. 13, 2018, 3:48 p.m. UTC | #25

On 09/13/2018 03:33 PM, Guenter Roeck wrote:
> On 09/12/2018 10:45 PM, Cédric Le Goater wrote
> 
> [ ... ]
> 
>>> ---
>>> qemu:
>>>
>>> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
>>> index c762c73..0d4aa08 100644
>>> --- a/hw/i2c/aspeed_i2c.c
>>> +++ b/hw/i2c/aspeed_i2c.c
>>> @@ -180,6 +180,33 @@ static uint8_t aspeed_i2c_get_state(AspeedI2CBus *bus)
>>>       return (bus->cmd >> I2CD_TX_STATE_SHIFT) & I2CD_TX_STATE_MASK;
>>>   }
>>>   +static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus)
>>> +{
>>> +    int ret;
>>> +
>>> +    if (!(bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST))) {
>>> +        return;
>>> +    }
>>
>> it deserves a comment to understand which scenario we are trying to handle.
>>
>>> +    if (bus->intr_status & I2CD_INTR_RX_DONE) {
>>> +        return;
>>> +    }
>>
>> should be handled in aspeed_i2c_bus_handle_cmd() I think
>>
> 
> I moved those two checks into the calling code.

ok

 
>>> +    aspeed_i2c_set_state(bus, I2CD_MRXD);
>>> +    ret = i2c_recv(bus->bus);
>>> +    if (ret < 0) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
>>> +        ret = 0xff;
>>> +    } else {
>>> +        bus->intr_status |= I2CD_INTR_RX_DONE;
>>> +    }
>>> +    bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
>>> +    if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
>>> +        i2c_nack(bus->bus);
>>> +    }
>>> +    bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
>>> +    aspeed_i2c_set_state(bus, I2CD_MACTIVE);
>>> +}
>>> +
>>>   /*
>>>    * The state machine needs some refinement. It is only used to track
>>>    * invalid STOP commands for the moment.
>>> @@ -188,7 +215,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>>>   {
>>>       bus->cmd &= ~0xFFFF;
>>>       bus->cmd |= value & 0xFFFF;
>>> -    bus->intr_status = 0;> +    bus->intr_status &= I2CD_INTR_RX_DONE;
>>
>> it deserves a comment to understand which scenario we are trying to handle.
>>    
> 
> Ok. FWIW, I wonder if intr_status should be touched here in the first place,
> but I neither have the hardware nor a datasheet, so I don't know if any bits
> are auto-cleared.

I just pushed a patch on my branch with some more explanation : 

	https://github.com/legoater/qemu/commits/aspeed-3.1

> 
>>>       if (bus->cmd & I2CD_M_START_CMD) {
>>>           uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
>>> @@ -227,22 +254,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>>>       }
>>>         if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) {
>>> -        int ret;
>>> -
>>> -        aspeed_i2c_set_state(bus, I2CD_MRXD);
>>> -        ret = i2c_recv(bus->bus);
>>> -        if (ret < 0) {
>>> -            qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
>>> -            ret = 0xff;
>>> -        } else {
>>> -            bus->intr_status |= I2CD_INTR_RX_DONE;
>>> -        }
>>> -        bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
>>> -        if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
>>> -            i2c_nack(bus->bus);
>>> -        }
>>> -        bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
>>> -        aspeed_i2c_set_state(bus, I2CD_MACTIVE);
>>> +        aspeed_i2c_handle_rx_cmd(bus);
>>>       }
>>>         if (bus->cmd & I2CD_M_STOP_CMD) {
>>> @@ -263,6 +275,7 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
>>>                                    uint64_t value, unsigned size)
>>>   {
>>>       AspeedI2CBus *bus = opaque;
>>> +    int status;
>>>         switch (offset) {
>>>       case I2CD_FUN_CTRL_REG:
>>> @@ -283,9 +296,16 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
>>>           bus->intr_ctrl = value & 0x7FFF;
>>>           break;
>>>       case I2CD_INTR_STS_REG:
>>> +        status = bus->intr_status;
>>>           bus->intr_status &= ~(value & 0x7FFF);
>>> -        bus->controller->intr_status &= ~(1 << bus->id);
>>> -        qemu_irq_lower(bus->controller->irq);
>>> +        if (!bus->intr_status) {
>>> +            bus->controller->intr_status &= ~(1 << bus->id);
>>> +            qemu_irq_lower(bus->controller->irq);
>>> +        }
>>
>> That part below is indeed something to fix. I had a similar patch.
>>
> 
> Should I split it out as separate patch ? Alternatively, if you submitted
> your patch already, I'll be happy to use it as base for mine.

See below. 

Thanks a lot,

C. 

From dea796e8d35ba7a70e4b14fbc30ff970a347b195 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?C=C3=A9dric=20Le=20Goater?= <[e-mail]>
Date: Thu, 13 Sep 2018 17:44:32 +0200
Subject: [PATCH] aspeed/i2c: lower bus interrupt when all interrupts have been
 cleared
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Also include some documentation on the interrupt status bits and how
they should be cleared. Also, the model does not implement correctly
the RX_DONE bit behavior which should be cleared to allow more data to
be received. Yet to be fixed.

Signed-off-by: Cédric Le Goater <[e-mail]>
---
 hw/i2c/aspeed_i2c.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index c762c7366ad9..275377c2ab38 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -52,6 +52,13 @@
 #define I2CD_AC_TIMING_REG2     0x08       /* Clock and AC Timing Control #1 */
 #define I2CD_INTR_CTRL_REG      0x0c       /* I2CD Interrupt Control */
 #define I2CD_INTR_STS_REG       0x10       /* I2CD Interrupt Status */
+
+#define   I2CD_INTR_SLAVE_ADDR_MATCH       (0x1 << 31) /* 0: addr1 1: addr2 */
+#define   I2CD_INTR_SLAVE_ADDR_RX_PENDING  (0x1 << 30)
+/* bits[19-16] Reserved */
+
+/* All bits below are cleared by writing 1 */
+#define   I2CD_INTR_SLAVE_INACTIVE_TIMEOUT (0x1 << 15)
 #define   I2CD_INTR_SDA_DL_TIMEOUT         (0x1 << 14)
 #define   I2CD_INTR_BUS_RECOVER_DONE       (0x1 << 13)
 #define   I2CD_INTR_SMBUS_ALERT            (0x1 << 12) /* Bus [0-3] only */
@@ -59,11 +66,16 @@
 #define   I2CD_INTR_SMBUS_DEV_ALERT_ADDR   (0x1 << 10) /* Removed */
 #define   I2CD_INTR_SMBUS_DEF_ADDR         (0x1 << 9)  /* Removed */
 #define   I2CD_INTR_GCALL_ADDR             (0x1 << 8)  /* Removed */
-#define   I2CD_INTR_SLAVE_MATCH            (0x1 << 7)  /* use RX_DONE */
+#define   I2CD_INTR_SLAVE_ADDR_RX_MATCH    (0x1 << 7)  /* use RX_DONE */
 #define   I2CD_INTR_SCL_TIMEOUT            (0x1 << 6)
 #define   I2CD_INTR_ABNORMAL               (0x1 << 5)
 #define   I2CD_INTR_NORMAL_STOP            (0x1 << 4)
 #define   I2CD_INTR_ARBIT_LOSS             (0x1 << 3)
+
+/*
+ * TODO: handle correctly I2CD_INTR_RX_DONE which needs to be cleared
+ * to allow next data to be received.
+ */
 #define   I2CD_INTR_RX_DONE                (0x1 << 2)
 #define   I2CD_INTR_TX_NAK                 (0x1 << 1)
 #define   I2CD_INTR_TX_ACK                 (0x1 << 0)
@@ -284,8 +296,10 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
         break;
     case I2CD_INTR_STS_REG:
         bus->intr_status &= ~(value & 0x7FFF);
-        bus->controller->intr_status &= ~(1 << bus->id);
-        qemu_irq_lower(bus->controller->irq);
+        if (!bus->intr_status) {
+            bus->controller->intr_status &= ~(1 << bus->id);
+            qemu_irq_lower(bus->controller->irq);
+        }
         break;
     case I2CD_DEV_ADDR_REG:
         qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",

Guenter Roeck Sept. 13, 2018, 3:57 p.m. UTC | #26

On Thu, Sep 13, 2018 at 05:48:59PM +0200, Cédric Le Goater wrote:
> On 09/13/2018 03:33 PM, Guenter Roeck wrote:
[ ... ]
> >>>   /*
> >>>    * The state machine needs some refinement. It is only used to track
> >>>    * invalid STOP commands for the moment.
> >>> @@ -188,7 +215,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
> >>>   {
> >>>       bus->cmd &= ~0xFFFF;
> >>>       bus->cmd |= value & 0xFFFF;
> >>> -    bus->intr_status = 0;> +    bus->intr_status &= I2CD_INTR_RX_DONE;
> >>
> >> it deserves a comment to understand which scenario we are trying to handle.
> >>    
> > 
> > Ok. FWIW, I wonder if intr_status should be touched here in the first place,
> > but I neither have the hardware nor a datasheet, so I don't know if any bits
> > are auto-cleared.
> 
> I just pushed a patch on my branch with some more explanation : 
> 
> 	https://github.com/legoater/qemu/commits/aspeed-3.1
> 
That seems to suggest that none of the status bits auto-clears, and that
the above code clearing intr_status should be removed entirely.
Am I missing something ?

Thanks,
Guenter

Jae Hyun Yoo Sept. 13, 2018, 4:31 p.m. UTC | #27

Hi Cédric,

On 9/12/2018 10:47 PM, Cédric Le Goater wrote:
> On 09/12/2018 06:54 PM, Jae Hyun Yoo wrote:
>> On 9/11/2018 6:34 PM, Guenter Roeck wrote:
>>> On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote:
>>>> On 9/11/2018 4:33 PM, Guenter Roeck wrote:
>>>>> Looking into the patch, clearing the interrupt status at the end of an
>>>>> interrupt handler is always suspicious and tends to result in race
>>>>> conditions (because additional interrupts may have arrived while handling
>>>>> the existing interrupts, or because interrupt handling itself may trigger
>>>>> another interrupt). With that in mind, the following patch fixes the
>>>>> problem for me.
>>>>>
>>>>> Guenter
>>>>>
>>>>> ---
>>>>>
>>>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>>>>> index c258c4d9a4c0..c488e6950b7c 100644
>>>>> --- a/drivers/i2c/busses/i2c-aspeed.c
>>>>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>>>>> @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>>>>        spin_lock(&bus->lock);
>>>>>        irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>> +    /* Ack all interrupt bits. */
>>>>> +    writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>>        irq_remaining = irq_received;
>>>>>    #if IS_ENABLED(CONFIG_I2C_SLAVE)
>>>>> @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>>>>                "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>>>>>                irq_received, irq_handled);
>>>>> -    /* Ack all interrupt bits. */
>>>>> -    writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>>        spin_unlock(&bus->lock);
>>>>>        return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>>>>>    }
>>>>>
>>>>
>>>> My intention of putting the code at the end of interrupt handler was,
>>>> to reduce possibility of combined irq calls which is explained in this
>>>> patch. But YES, I agree with you. It could make a potential race
>>>
>>> Hmm, yes, but that doesn't explain why it would make sense to acknowledge
>>> the interrupt late. The interrupt ack only means "I am going to handle these
>>> interrupts". If additional interrupts arrive while the interrupt handler
>>> is active, those will have to be acknowledged separately.
>>>
>>> Sure, there is a risk that an interrupt arrives while the handler is
>>> running, and that it is handled but not acknowledged. That can happen
>>> with pretty much all interrupt handlers, and there are mitigations to
>>> limit the impact (for example, read the interrupt status register in
>>> a loop until no more interrupts are pending). But acknowledging
>>> an interrupt that was possibly not handled is always bad idea.
>>
>> Well, that's generally right but not always. Sometimes that depends on
>> hardware and Aspeed I2C is the case.
>>
>> This is a description from Aspeed AST2500 datasheet:
>>    I2CD10 Interrupt Status Register
>>    bit 2 Receive Done Interrupt status
>>          S/W needs to clear this status bit to allow next data receiving.
>>
>> It means, driver should hold this bit to prevent transition of hardware
>> state machine until the driver handles received data, so the bit should
>> be cleared at the end of interrupt handler.
>>
>> Let me share my test result. Your code change works on 100KHz bus speed
>> but doesn't work well on 1MHz bus speed. Investigated that interrupt
>> handling is fast enough in 100KHz test but in 1MHz, most of data is
>> corrupted because the bit is cleared at the beginning of interrupt
>> handler so it allows receiving of the next data but the interrupt
>> handler isn't fast enough to read the data buffer on time. I checked
>> this problem on BMC-ME channel which ME sends lots of IPMB packets to
>> BMC at 1MHz speed. You could simply check the data corruption problem on
>> the BMC-ME channel.
> 
> OK.
>   
>> My thought is, the current code is right for real Aspeed I2C hardware.
>> It seems that QEMU 3.0 model for witherspoon-bmc doesn't simulate the
>> actual Aspeed I2C hardware correctly.
> 
> That might be very well possible yes. it also misses support for the slave
> mode and the DMA registers.
> 

Yes, it would be good if qemu's Aspeed I2C model supports slave mode.
Since the current linux Aspeed I2C driver supports byte transfer mode
only, so DMA transfer mode support in qemu could be considered later.
Implementing pool mode and DMA mode for linux Aspeed I2C are in my
backlog at this moment.

Thanks,
Jae

> Thanks for the info,
> 
> C.
>

Cédric Le Goater Sept. 13, 2018, 4:35 p.m. UTC | #28

On 09/13/2018 05:57 PM, Guenter Roeck wrote:
> On Thu, Sep 13, 2018 at 05:48:59PM +0200, Cédric Le Goater wrote:
>> On 09/13/2018 03:33 PM, Guenter Roeck wrote:
> [ ... ]
>>>>>   /*
>>>>>    * The state machine needs some refinement. It is only used to track
>>>>>    * invalid STOP commands for the moment.
>>>>> @@ -188,7 +215,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>>>>>   {
>>>>>       bus->cmd &= ~0xFFFF;
>>>>>       bus->cmd |= value & 0xFFFF;
>>>>> -    bus->intr_status = 0;> +    bus->intr_status &= I2CD_INTR_RX_DONE;
>>>>
>>>> it deserves a comment to understand which scenario we are trying to handle.
>>>>    
>>>
>>> Ok. FWIW, I wonder if intr_status should be touched here in the first place,
>>> but I neither have the hardware nor a datasheet, so I don't know if any bits
>>> are auto-cleared.
>>
>> I just pushed a patch on my branch with some more explanation : 
>>
>> 	https://github.com/legoater/qemu/commits/aspeed-3.1
>>
> That seems to suggest that none of the status bits auto-clears, and that
> the above code clearing intr_status should be removed entirely.
> Am I missing something ?

You are right. I just pushed another version of the previous patch with this 
new hunk :

@@ -188,7 +200,6 @@ static void aspeed_i2c_bus_handle_cmd(As
 {
     bus->cmd &= ~0xFFFF;
     bus->cmd |= value & 0xFFFF;
-    bus->intr_status = 0;
 
     if (bus->cmd & I2CD_M_START_CMD) {
         uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?


The QEMU palmetto and witherspoon machines seem to behave fine. Can you give 
it a try ?

Thanks,

C.

Cédric Le Goater Sept. 13, 2018, 4:51 p.m. UTC | #29

Hello ! 

On 09/13/2018 06:31 PM, Jae Hyun Yoo wrote:
> Hi Cédric,
> 
> On 9/12/2018 10:47 PM, Cédric Le Goater wrote:
>> On 09/12/2018 06:54 PM, Jae Hyun Yoo wrote:
>>> On 9/11/2018 6:34 PM, Guenter Roeck wrote:
>>>> On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote:
>>>>> On 9/11/2018 4:33 PM, Guenter Roeck wrote:
>>>>>> Looking into the patch, clearing the interrupt status at the end of an
>>>>>> interrupt handler is always suspicious and tends to result in race
>>>>>> conditions (because additional interrupts may have arrived while handling
>>>>>> the existing interrupts, or because interrupt handling itself may trigger
>>>>>> another interrupt). With that in mind, the following patch fixes the
>>>>>> problem for me.
>>>>>>
>>>>>> Guenter
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>>>>>> index c258c4d9a4c0..c488e6950b7c 100644
>>>>>> --- a/drivers/i2c/busses/i2c-aspeed.c
>>>>>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>>>>>> @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>>>>>        spin_lock(&bus->lock);
>>>>>>        irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>>> +    /* Ack all interrupt bits. */
>>>>>> +    writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>>>        irq_remaining = irq_received;
>>>>>>    #if IS_ENABLED(CONFIG_I2C_SLAVE)
>>>>>> @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>>>>>                "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>>>>>>                irq_received, irq_handled);
>>>>>> -    /* Ack all interrupt bits. */
>>>>>> -    writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>>>        spin_unlock(&bus->lock);
>>>>>>        return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>>>>>>    }
>>>>>>
>>>>>
>>>>> My intention of putting the code at the end of interrupt handler was,
>>>>> to reduce possibility of combined irq calls which is explained in this
>>>>> patch. But YES, I agree with you. It could make a potential race
>>>>
>>>> Hmm, yes, but that doesn't explain why it would make sense to acknowledge
>>>> the interrupt late. The interrupt ack only means "I am going to handle these
>>>> interrupts". If additional interrupts arrive while the interrupt handler
>>>> is active, those will have to be acknowledged separately.
>>>>
>>>> Sure, there is a risk that an interrupt arrives while the handler is
>>>> running, and that it is handled but not acknowledged. That can happen
>>>> with pretty much all interrupt handlers, and there are mitigations to
>>>> limit the impact (for example, read the interrupt status register in
>>>> a loop until no more interrupts are pending). But acknowledging
>>>> an interrupt that was possibly not handled is always bad idea.
>>>
>>> Well, that's generally right but not always. Sometimes that depends on
>>> hardware and Aspeed I2C is the case.
>>>
>>> This is a description from Aspeed AST2500 datasheet:
>>>    I2CD10 Interrupt Status Register
>>>    bit 2 Receive Done Interrupt status
>>>          S/W needs to clear this status bit to allow next data receiving.
>>>
>>> It means, driver should hold this bit to prevent transition of hardware
>>> state machine until the driver handles received data, so the bit should
>>> be cleared at the end of interrupt handler.
>>>
>>> Let me share my test result. Your code change works on 100KHz bus speed
>>> but doesn't work well on 1MHz bus speed. Investigated that interrupt
>>> handling is fast enough in 100KHz test but in 1MHz, most of data is
>>> corrupted because the bit is cleared at the beginning of interrupt
>>> handler so it allows receiving of the next data but the interrupt
>>> handler isn't fast enough to read the data buffer on time. I checked
>>> this problem on BMC-ME channel which ME sends lots of IPMB packets to
>>> BMC at 1MHz speed. You could simply check the data corruption problem on
>>> the BMC-ME channel.
>>
>> OK.
>>  
>>> My thought is, the current code is right for real Aspeed I2C hardware.
>>> It seems that QEMU 3.0 model for witherspoon-bmc doesn't simulate the
>>> actual Aspeed I2C hardware correctly.
>>
>> That might be very well possible yes. it also misses support for the slave
>> mode and the DMA registers.
>>
> 
> Yes, it would be good if qemu's Aspeed I2C model supports slave mode.
> Since the current linux Aspeed I2C driver supports byte transfer mode
> only, so DMA transfer mode support in qemu could be considered later.

The Aspeed SDK already does, so yes, we will need to consider it.

> Implementing pool mode and DMA mode for linux Aspeed I2C are in my
> backlog at this moment.

Is there a QEMU model for an I2C/IPMB device ? 

There is already a large IPMI framework in QEMU, that would be interesting.
to extend.

Thanks,

C.

Jae Hyun Yoo Sept. 13, 2018, 5:01 p.m. UTC | #30

On 9/13/2018 9:51 AM, Cédric Le Goater wrote:
> Hello !
> 
> On 09/13/2018 06:31 PM, Jae Hyun Yoo wrote:
>> Hi Cédric,
>>
>> On 9/12/2018 10:47 PM, Cédric Le Goater wrote:
>>> On 09/12/2018 06:54 PM, Jae Hyun Yoo wrote:
>>>> On 9/11/2018 6:34 PM, Guenter Roeck wrote:
>>>>> On Tue, Sep 11, 2018 at 04:58:44PM -0700, Jae Hyun Yoo wrote:
>>>>>> On 9/11/2018 4:33 PM, Guenter Roeck wrote:
>>>>>>> Looking into the patch, clearing the interrupt status at the end of an
>>>>>>> interrupt handler is always suspicious and tends to result in race
>>>>>>> conditions (because additional interrupts may have arrived while handling
>>>>>>> the existing interrupts, or because interrupt handling itself may trigger
>>>>>>> another interrupt). With that in mind, the following patch fixes the
>>>>>>> problem for me.
>>>>>>>
>>>>>>> Guenter
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
>>>>>>> index c258c4d9a4c0..c488e6950b7c 100644
>>>>>>> --- a/drivers/i2c/busses/i2c-aspeed.c
>>>>>>> +++ b/drivers/i2c/busses/i2c-aspeed.c
>>>>>>> @@ -552,6 +552,8 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>>>>>>         spin_lock(&bus->lock);
>>>>>>>         irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>>>> +    /* Ack all interrupt bits. */
>>>>>>> +    writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>>>>         irq_remaining = irq_received;
>>>>>>>     #if IS_ENABLED(CONFIG_I2C_SLAVE)
>>>>>>> @@ -584,8 +586,6 @@ static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
>>>>>>>                 "irq handled != irq. expected 0x%08x, but was 0x%08x\n",
>>>>>>>                 irq_received, irq_handled);
>>>>>>> -    /* Ack all interrupt bits. */
>>>>>>> -    writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
>>>>>>>         spin_unlock(&bus->lock);
>>>>>>>         return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
>>>>>>>     }
>>>>>>>
>>>>>>
>>>>>> My intention of putting the code at the end of interrupt handler was,
>>>>>> to reduce possibility of combined irq calls which is explained in this
>>>>>> patch. But YES, I agree with you. It could make a potential race
>>>>>
>>>>> Hmm, yes, but that doesn't explain why it would make sense to acknowledge
>>>>> the interrupt late. The interrupt ack only means "I am going to handle these
>>>>> interrupts". If additional interrupts arrive while the interrupt handler
>>>>> is active, those will have to be acknowledged separately.
>>>>>
>>>>> Sure, there is a risk that an interrupt arrives while the handler is
>>>>> running, and that it is handled but not acknowledged. That can happen
>>>>> with pretty much all interrupt handlers, and there are mitigations to
>>>>> limit the impact (for example, read the interrupt status register in
>>>>> a loop until no more interrupts are pending). But acknowledging
>>>>> an interrupt that was possibly not handled is always bad idea.
>>>>
>>>> Well, that's generally right but not always. Sometimes that depends on
>>>> hardware and Aspeed I2C is the case.
>>>>
>>>> This is a description from Aspeed AST2500 datasheet:
>>>>     I2CD10 Interrupt Status Register
>>>>     bit 2 Receive Done Interrupt status
>>>>           S/W needs to clear this status bit to allow next data receiving.
>>>>
>>>> It means, driver should hold this bit to prevent transition of hardware
>>>> state machine until the driver handles received data, so the bit should
>>>> be cleared at the end of interrupt handler.
>>>>
>>>> Let me share my test result. Your code change works on 100KHz bus speed
>>>> but doesn't work well on 1MHz bus speed. Investigated that interrupt
>>>> handling is fast enough in 100KHz test but in 1MHz, most of data is
>>>> corrupted because the bit is cleared at the beginning of interrupt
>>>> handler so it allows receiving of the next data but the interrupt
>>>> handler isn't fast enough to read the data buffer on time. I checked
>>>> this problem on BMC-ME channel which ME sends lots of IPMB packets to
>>>> BMC at 1MHz speed. You could simply check the data corruption problem on
>>>> the BMC-ME channel.
>>>
>>> OK.
>>>   
>>>> My thought is, the current code is right for real Aspeed I2C hardware.
>>>> It seems that QEMU 3.0 model for witherspoon-bmc doesn't simulate the
>>>> actual Aspeed I2C hardware correctly.
>>>
>>> That might be very well possible yes. it also misses support for the slave
>>> mode and the DMA registers.
>>>
>>
>> Yes, it would be good if qemu's Aspeed I2C model supports slave mode.
>> Since the current linux Aspeed I2C driver supports byte transfer mode
>> only, so DMA transfer mode support in qemu could be considered later.
> 
> The Aspeed SDK already does, so yes, we will need to consider it.
> 

Yes, Aspeed SDK does but the driver in upstream doesn't at this time.
So we will need to consider it because byte mode makes too many
interrupt calls which is not good for performance.

>> Implementing pool mode and DMA mode for linux Aspeed I2C are in my
>> backlog at this moment.
> 
> Is there a QEMU model for an I2C/IPMB device ?
> 

Not sure because I'm not familiar with QEMU. Need to check.

Thanks,
Jae

> There is already a large IPMI framework in QEMU, that would be interesting.
> to extend.
> 
> Thanks,
> 
> C.
> 
>

Guenter Roeck Sept. 14, 2018, 3:48 a.m. UTC | #31

On 09/13/2018 09:35 AM, Cédric Le Goater wrote:
> On 09/13/2018 05:57 PM, Guenter Roeck wrote:
>> On Thu, Sep 13, 2018 at 05:48:59PM +0200, Cédric Le Goater wrote:
>>> On 09/13/2018 03:33 PM, Guenter Roeck wrote:
>> [ ... ]
>>>>>>    /*
>>>>>>     * The state machine needs some refinement. It is only used to track
>>>>>>     * invalid STOP commands for the moment.
>>>>>> @@ -188,7 +215,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>>>>>>    {
>>>>>>        bus->cmd &= ~0xFFFF;
>>>>>>        bus->cmd |= value & 0xFFFF;
>>>>>> -    bus->intr_status = 0;> +    bus->intr_status &= I2CD_INTR_RX_DONE;
>>>>>
>>>>> it deserves a comment to understand which scenario we are trying to handle.
>>>>>     
>>>>
>>>> Ok. FWIW, I wonder if intr_status should be touched here in the first place,
>>>> but I neither have the hardware nor a datasheet, so I don't know if any bits
>>>> are auto-cleared.
>>>
>>> I just pushed a patch on my branch with some more explanation :
>>>
>>> 	https://github.com/legoater/qemu/commits/aspeed-3.1
>>>
>> That seems to suggest that none of the status bits auto-clears, and that
>> the above code clearing intr_status should be removed entirely.
>> Am I missing something ?
> 
> You are right. I just pushed another version of the previous patch with this
> new hunk :
> 
> @@ -188,7 +200,6 @@ static void aspeed_i2c_bus_handle_cmd(As
>   {
>       bus->cmd &= ~0xFFFF;
>       bus->cmd |= value & 0xFFFF;
> -    bus->intr_status = 0;
>   
>       if (bus->cmd & I2CD_M_START_CMD) {
>           uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
> 
> 
> The QEMU palmetto and witherspoon machines seem to behave fine. Can you give
> it a try ?
> 

Works fine for me for all affected qemu platforms.

How do you want to proceed with the qemu patches ? I attached my patches
for reference. Maybe you can add them to your tree if they are ok and submit
the entire series together to the qemu mailing list ?

Thanks,
Guenter
From 9eb05b7f6d7ceb2919fa8b865772ab9207d1afed Mon Sep 17 00:00:00 2001
From: Guenter Roeck <[e-mail]>
Date: Wed, 12 Sep 2018 16:35:20 -0700
Subject: [PATCH 1/2] aspeed/i2c: Handle receive command in separate function

Receive command handling may have to be deferred if a previous receive
done interrupt was not yet acknowledged. Move receive command handling
into a separate function to prepare for the necessary changes.

Signed-off-by: Guenter Roeck <[e-mail]>
---
 hw/i2c/aspeed_i2c.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index de6b083..d81f865 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -192,6 +192,26 @@ static uint8_t aspeed_i2c_get_state(AspeedI2CBus *bus)
     return (bus->cmd >> I2CD_TX_STATE_SHIFT) & I2CD_TX_STATE_MASK;
 }
 
+static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus)
+{
+    int ret;
+
+    aspeed_i2c_set_state(bus, I2CD_MRXD);
+    ret = i2c_recv(bus->bus);
+    if (ret < 0) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
+        ret = 0xff;
+    } else {
+        bus->intr_status |= I2CD_INTR_RX_DONE;
+    }
+    bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
+    if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
+        i2c_nack(bus->bus);
+    }
+    bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
+    aspeed_i2c_set_state(bus, I2CD_MACTIVE);
+}
+
 /*
  * The state machine needs some refinement. It is only used to track
  * invalid STOP commands for the moment.
@@ -238,22 +258,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
     }
 
     if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) {
-        int ret;
-
-        aspeed_i2c_set_state(bus, I2CD_MRXD);
-        ret = i2c_recv(bus->bus);
-        if (ret < 0) {
-            qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
-            ret = 0xff;
-        } else {
-            bus->intr_status |= I2CD_INTR_RX_DONE;
-        }
-        bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
-        if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
-            i2c_nack(bus->bus);
-        }
-        bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
-        aspeed_i2c_set_state(bus, I2CD_MACTIVE);
+        aspeed_i2c_handle_rx_cmd(bus);
     }
 
     if (bus->cmd & I2CD_M_STOP_CMD) {

Cédric Le Goater Sept. 14, 2018, 5:38 a.m. UTC | #32

>>> That seems to suggest that none of the status bits auto-clears, and that
>>> the above code clearing intr_status should be removed entirely.
>>> Am I missing something ?
>>
>> You are right. I just pushed another version of the previous patch with this
>> new hunk :
>>
>> @@ -188,7 +200,6 @@ static void aspeed_i2c_bus_handle_cmd(As
>>   {
>>       bus->cmd &= ~0xFFFF;
>>       bus->cmd |= value & 0xFFFF;
>> -    bus->intr_status = 0;
>>         if (bus->cmd & I2CD_M_START_CMD) {
>>           uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
>>
>>
>> The QEMU palmetto and witherspoon machines seem to behave fine. Can you give
>> it a try ?
>>
> 
> Works fine for me for all affected qemu platforms.
> 
> How do you want to proceed with the qemu patches ? I attached my patches
> for reference. Maybe you can add them to your tree if they are ok and submit
> the entire series together to the qemu mailing list ?

yes. They are pushed in my aspeed-3.1 branch. I will send the series 
on the list.

Thanks,

C.

Guenter Roeck Sept. 14, 2018, 1:23 p.m. UTC | #33

On 09/13/2018 10:38 PM, Cédric Le Goater wrote:
>>>> That seems to suggest that none of the status bits auto-clears, and that
>>>> the above code clearing intr_status should be removed entirely.
>>>> Am I missing something ?
>>>
>>> You are right. I just pushed another version of the previous patch with this
>>> new hunk :
>>>
>>> @@ -188,7 +200,6 @@ static void aspeed_i2c_bus_handle_cmd(As
>>>    {
>>>        bus->cmd &= ~0xFFFF;
>>>        bus->cmd |= value & 0xFFFF;
>>> -    bus->intr_status = 0;
>>>          if (bus->cmd & I2CD_M_START_CMD) {
>>>            uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
>>>
>>>
>>> The QEMU palmetto and witherspoon machines seem to behave fine. Can you give
>>> it a try ?
>>>
>>
>> Works fine for me for all affected qemu platforms.
>>
>> How do you want to proceed with the qemu patches ? I attached my patches
>> for reference. Maybe you can add them to your tree if they are ok and submit
>> the entire series together to the qemu mailing list ?
> 
> yes. They are pushed in my aspeed-3.1 branch. I will send the series
> on the list.
> 

Excellent. Thanks a lot!

Guenter

Jae Hyun Yoo Sept. 14, 2018, 4:52 p.m. UTC | #34

On 9/14/2018 6:23 AM, Guenter Roeck wrote:
> On 09/13/2018 10:38 PM, Cédric Le Goater wrote:
>>>>> That seems to suggest that none of the status bits auto-clears, and 
>>>>> that
>>>>> the above code clearing intr_status should be removed entirely.
>>>>> Am I missing something ?
>>>>
>>>> You are right. I just pushed another version of the previous patch 
>>>> with this
>>>> new hunk :
>>>>
>>>> @@ -188,7 +200,6 @@ static void aspeed_i2c_bus_handle_cmd(As
>>>>    {
>>>>        bus->cmd &= ~0xFFFF;
>>>>        bus->cmd |= value & 0xFFFF;
>>>> -    bus->intr_status = 0;
>>>>          if (bus->cmd & I2CD_M_START_CMD) {
>>>>            uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
>>>>
>>>>
>>>> The QEMU palmetto and witherspoon machines seem to behave fine. Can 
>>>> you give
>>>> it a try ?
>>>>
>>>
>>> Works fine for me for all affected qemu platforms.
>>>
>>> How do you want to proceed with the qemu patches ? I attached my patches
>>> for reference. Maybe you can add them to your tree if they are ok and 
>>> submit
>>> the entire series together to the qemu mailing list ?
>>
>> yes. They are pushed in my aspeed-3.1 branch. I will send the series
>> on the list.
>>
> 
> Excellent. Thanks a lot!
> 
> Guenter
> 

Awesome! Many thanks to Guenter, Cédric and Joel. I really appreciate
it.

Jae

Patch

diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index a4f956c6d567..c258c4d9a4c0 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -82,6 +82,11 @@ 
 #define ASPEED_I2CD_INTR_RX_DONE			BIT(2)
 #define ASPEED_I2CD_INTR_TX_NAK				BIT(1)
 #define ASPEED_I2CD_INTR_TX_ACK				BIT(0)
+#define ASPEED_I2CD_INTR_MASTER_ERRORS					       \
+		(ASPEED_I2CD_INTR_SDA_DL_TIMEOUT |			       \
+		 ASPEED_I2CD_INTR_SCL_TIMEOUT |				       \
+		 ASPEED_I2CD_INTR_ABNORMAL |				       \
+		 ASPEED_I2CD_INTR_ARBIT_LOSS)
 #define ASPEED_I2CD_INTR_ALL						       \
 		(ASPEED_I2CD_INTR_SDA_DL_TIMEOUT |			       \
 		 ASPEED_I2CD_INTR_BUS_RECOVER_DONE |			       \
@@ -227,32 +232,26 @@  static int aspeed_i2c_recover_bus(struct aspeed_i2c_bus *bus)
 }
 
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
-static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
+static u32 aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
 {
-	u32 command, irq_status, status_ack = 0;
+	u32 command, irq_handled = 0;
 	struct i2c_client *slave = bus->slave;
-	bool irq_handled = true;
 	u8 value;
 
-	if (!slave) {
-		irq_handled = false;
-		goto out;
-	}
+	if (!slave)
+		return 0;
 
 	command = readl(bus->base + ASPEED_I2C_CMD_REG);
-	irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
 
 	/* Slave was requested, restart state machine. */
 	if (irq_status & ASPEED_I2CD_INTR_SLAVE_MATCH) {
-		status_ack |= ASPEED_I2CD_INTR_SLAVE_MATCH;
+		irq_handled |= ASPEED_I2CD_INTR_SLAVE_MATCH;
 		bus->slave_state = ASPEED_I2C_SLAVE_START;
 	}
 
 	/* Slave is not currently active, irq was for someone else. */
-	if (bus->slave_state == ASPEED_I2C_SLAVE_STOP) {
-		irq_handled = false;
-		goto out;
-	}
+	if (bus->slave_state == ASPEED_I2C_SLAVE_STOP)
+		return irq_handled;
 
 	dev_dbg(bus->dev, "slave irq status 0x%08x, cmd 0x%08x\n",
 		irq_status, command);
@@ -269,31 +268,31 @@  static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
 				bus->slave_state =
 						ASPEED_I2C_SLAVE_WRITE_REQUESTED;
 		}
-		status_ack |= ASPEED_I2CD_INTR_RX_DONE;
+		irq_handled |= ASPEED_I2CD_INTR_RX_DONE;
 	}
 
 	/* Slave was asked to stop. */
 	if (irq_status & ASPEED_I2CD_INTR_NORMAL_STOP) {
-		status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
+		irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
 		bus->slave_state = ASPEED_I2C_SLAVE_STOP;
 	}
 	if (irq_status & ASPEED_I2CD_INTR_TX_NAK) {
-		status_ack |= ASPEED_I2CD_INTR_TX_NAK;
+		irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
 		bus->slave_state = ASPEED_I2C_SLAVE_STOP;
 	}
+	if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
+		irq_handled |= ASPEED_I2CD_INTR_TX_ACK;
 
 	switch (bus->slave_state) {
 	case ASPEED_I2C_SLAVE_READ_REQUESTED:
 		if (irq_status & ASPEED_I2CD_INTR_TX_ACK)
 			dev_err(bus->dev, "Unexpected ACK on read request.\n");
 		bus->slave_state = ASPEED_I2C_SLAVE_READ_PROCESSED;
-
 		i2c_slave_event(slave, I2C_SLAVE_READ_REQUESTED, &value);
 		writel(value, bus->base + ASPEED_I2C_BYTE_BUF_REG);
 		writel(ASPEED_I2CD_S_TX_CMD, bus->base + ASPEED_I2C_CMD_REG);
 		break;
 	case ASPEED_I2C_SLAVE_READ_PROCESSED:
-		status_ack |= ASPEED_I2CD_INTR_TX_ACK;
 		if (!(irq_status & ASPEED_I2CD_INTR_TX_ACK))
 			dev_err(bus->dev,
 				"Expected ACK after processed read.\n");
@@ -317,13 +316,6 @@  static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
 		break;
 	}
 
-	if (status_ack != irq_status)
-		dev_err(bus->dev,
-			"irq handled != irq. expected %x, but was %x\n",
-			irq_status, status_ack);
-	writel(status_ack, bus->base + ASPEED_I2C_INTR_STS_REG);
-
-out:
 	return irq_handled;
 }
 #endif /* CONFIG_I2C_SLAVE */
@@ -380,21 +372,21 @@  static int aspeed_i2c_is_irq_error(u32 irq_status)
 	return 0;
 }
 
-static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
+static u32 aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus, u32 irq_status)
 {
-	u32 irq_status, status_ack = 0, command = 0;
+	u32 irq_handled = 0, command = 0;
 	struct i2c_msg *msg;
 	u8 recv_byte;
 	int ret;
 
-	irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
-	/* Ack all interrupt bits. */
-	writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
-
 	if (irq_status & ASPEED_I2CD_INTR_BUS_RECOVER_DONE) {
 		bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
-		status_ack |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
+		irq_handled |= ASPEED_I2CD_INTR_BUS_RECOVER_DONE;
 		goto out_complete;
+	} else {
+		/* Master is not currently active, irq was for someone else. */
+		if (bus->master_state == ASPEED_I2C_MASTER_INACTIVE)
+			goto out_no_complete;
 	}
 
 	/*
@@ -403,19 +395,22 @@  static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
 	 * INACTIVE state.
 	 */
 	ret = aspeed_i2c_is_irq_error(irq_status);
-	if (ret < 0) {
+	if (ret) {
 		dev_dbg(bus->dev, "received error interrupt: 0x%08x\n",
 			irq_status);
 		bus->cmd_err = ret;
 		bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
+		irq_handled |= (irq_status & ASPEED_I2CD_INTR_MASTER_ERRORS);
 		goto out_complete;
 	}
 
 	/* We are in an invalid state; reset bus to a known state. */
 	if (!bus->msgs) {
-		dev_err(bus->dev, "bus in unknown state\n");
+		dev_err(bus->dev, "bus in unknown state. irq_status: 0x%x\n",
+			irq_status);
 		bus->cmd_err = -EIO;
-		if (bus->master_state != ASPEED_I2C_MASTER_STOP)
+		if (bus->master_state != ASPEED_I2C_MASTER_STOP &&
+		    bus->master_state != ASPEED_I2C_MASTER_INACTIVE)
 			aspeed_i2c_do_stop(bus);
 		goto out_no_complete;
 	}
@@ -428,13 +423,18 @@  static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
 	 */
 	if (bus->master_state == ASPEED_I2C_MASTER_START) {
 		if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
+			if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_NAK))) {
+				bus->cmd_err = -ENXIO;
+				bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
+				goto out_complete;
+			}
 			pr_devel("no slave present at %02x\n", msg->addr);
-			status_ack |= ASPEED_I2CD_INTR_TX_NAK;
+			irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
 			bus->cmd_err = -ENXIO;
 			aspeed_i2c_do_stop(bus);
 			goto out_no_complete;
 		}
-		status_ack |= ASPEED_I2CD_INTR_TX_ACK;
+		irq_handled |= ASPEED_I2CD_INTR_TX_ACK;
 		if (msg->len == 0) { /* SMBUS_QUICK */
 			aspeed_i2c_do_stop(bus);
 			goto out_no_complete;
@@ -449,13 +449,13 @@  static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
 	case ASPEED_I2C_MASTER_TX:
 		if (unlikely(irq_status & ASPEED_I2CD_INTR_TX_NAK)) {
 			dev_dbg(bus->dev, "slave NACKed TX\n");
-			status_ack |= ASPEED_I2CD_INTR_TX_NAK;
+			irq_handled |= ASPEED_I2CD_INTR_TX_NAK;
 			goto error_and_stop;
 		} else if (unlikely(!(irq_status & ASPEED_I2CD_INTR_TX_ACK))) {
 			dev_err(bus->dev, "slave failed to ACK TX\n");
 			goto error_and_stop;
 		}
-		status_ack |= ASPEED_I2CD_INTR_TX_ACK;
+		irq_handled |= ASPEED_I2CD_INTR_TX_ACK;
 		/* fallthrough intended */
 	case ASPEED_I2C_MASTER_TX_FIRST:
 		if (bus->buf_index < msg->len) {
@@ -478,7 +478,7 @@  static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
 			dev_err(bus->dev, "master failed to RX\n");
 			goto error_and_stop;
 		}
-		status_ack |= ASPEED_I2CD_INTR_RX_DONE;
+		irq_handled |= ASPEED_I2CD_INTR_RX_DONE;
 
 		recv_byte = readl(bus->base + ASPEED_I2C_BYTE_BUF_REG) >> 8;
 		msg->buf[bus->buf_index++] = recv_byte;
@@ -506,11 +506,13 @@  static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
 		goto out_no_complete;
 	case ASPEED_I2C_MASTER_STOP:
 		if (unlikely(!(irq_status & ASPEED_I2CD_INTR_NORMAL_STOP))) {
-			dev_err(bus->dev, "master failed to STOP\n");
+			dev_err(bus->dev,
+				"master failed to STOP. irq_status:0x%x\n",
+				irq_status);
 			bus->cmd_err = -EIO;
 			/* Do not STOP as we have already tried. */
 		} else {
-			status_ack |= ASPEED_I2CD_INTR_NORMAL_STOP;
+			irq_handled |= ASPEED_I2CD_INTR_NORMAL_STOP;
 		}
 
 		bus->master_state = ASPEED_I2C_MASTER_INACTIVE;
@@ -540,33 +542,52 @@  static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus *bus)
 		bus->master_xfer_result = bus->msgs_index + 1;
 	complete(&bus->cmd_complete);
 out_no_complete:
-	if (irq_status != status_ack)
-		dev_err(bus->dev,
-			"irq handled != irq. expected 0x%08x, but was 0x%08x\n",
-			irq_status, status_ack);
-	return !!irq_status;
+	return irq_handled;
 }
 
 static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
 {
 	struct aspeed_i2c_bus *bus = dev_id;
-	bool ret;
+	u32 irq_received, irq_remaining, irq_handled;
 
 	spin_lock(&bus->lock);
+	irq_received = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
+	irq_remaining = irq_received;
 
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
-	if (aspeed_i2c_slave_irq(bus)) {
-		dev_dbg(bus->dev, "irq handled by slave.\n");
-		ret = true;
-		goto out;
+	/*
+	 * In most cases, interrupt bits will be set one by one, although
+	 * multiple interrupt bits could be set at the same time. It's also
+	 * possible that master interrupt bits could be set along with slave
+	 * interrupt bits. Each case needs to be handled using corresponding
+	 * handlers depending on the current state.
+	 */
+	if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
+		irq_handled = aspeed_i2c_master_irq(bus, irq_remaining);
+		irq_remaining &= ~irq_handled;
+		if (irq_remaining)
+			irq_handled |= aspeed_i2c_slave_irq(bus, irq_remaining);
+	} else {
+		irq_handled = aspeed_i2c_slave_irq(bus, irq_remaining);
+		irq_remaining &= ~irq_handled;
+		if (irq_remaining)
+			irq_handled |= aspeed_i2c_master_irq(bus,
+							     irq_remaining);
 	}
+#else
+	irq_handled = aspeed_i2c_master_irq(bus, irq_remaining);
 #endif /* CONFIG_I2C_SLAVE */
 
-	ret = aspeed_i2c_master_irq(bus);
+	irq_remaining &= ~irq_handled;
+	if (irq_remaining)
+		dev_err(bus->dev,
+			"irq handled != irq. expected 0x%08x, but was 0x%08x\n",
+			irq_received, irq_handled);
 
-out:
+	/* Ack all interrupt bits. */
+	writel(irq_received, bus->base + ASPEED_I2C_INTR_STS_REG);
 	spin_unlock(&bus->lock);
-	return ret ? IRQ_HANDLED : IRQ_NONE;
+	return irq_remaining ? IRQ_NONE : IRQ_HANDLED;
 }
 
 static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,