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

[Guenter Roeck <linux@roeck-us.net>]

[-- Attachment #1: Type: text/plain, Size: 2013 bytes --]

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

[-- Attachment #2: 0001-aspeed-i2c-Handle-receive-command-in-separate-functi.patch --]
[-- Type: text/x-patch, Size: 2453 bytes --]

From 9eb05b7f6d7ceb2919fa8b865772ab9207d1afed Mon Sep 17 00:00:00 2001
From: Guenter Roeck <linux@roeck-us.net>
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 <linux@roeck-us.net>
---
 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) {
-- 
2.7.4


[-- Attachment #3: 0002-aspeed-i2c-Fix-receive-done-interrupt-handling.patch --]
[-- Type: text/x-patch, Size: 2347 bytes --]

From b5e1879de9c347ab03a09d71f12d40aad0a16d6d Mon Sep 17 00:00:00 2001
From: Guenter Roeck <linux@roeck-us.net>
Date: Wed, 12 Sep 2018 14:19:30 -0700
Subject: [PATCH 2/2] aspeed/i2c: Fix receive done interrupt handling

The AST2500 datasheet says:

I2CD10 Interrupt Status Register
       bit 2 Receive Done Interrupt status
             S/W needs to clear this status bit to allow next data receiving

The Rx interrrupt done interrupt status bit needs to be cleared
explicitly before the next byte can be received, and must therefore
not be auto-cleared. Also, receiving the next byte must be delayed
until the bit has been cleared.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 hw/i2c/aspeed_i2c.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index d81f865..7ae99bc 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -257,7 +257,8 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
         aspeed_i2c_set_state(bus, I2CD_MACTIVE);
     }
 
-    if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) {
+    if ((bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) &&
+        !(bus->intr_status & I2CD_INTR_RX_DONE)) {
         aspeed_i2c_handle_rx_cmd(bus);
     }
 
@@ -279,6 +280,7 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
                                  uint64_t value, unsigned size)
 {
     AspeedI2CBus *bus = opaque;
+    bool handle_rx;
 
     switch (offset) {
     case I2CD_FUN_CTRL_REG:
@@ -299,11 +301,17 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
         bus->intr_ctrl = value & 0x7FFF;
         break;
     case I2CD_INTR_STS_REG:
+        handle_rx = (bus->intr_status & I2CD_INTR_RX_DONE) &&
+                (value & I2CD_INTR_RX_DONE);
         bus->intr_status &= ~(value & 0x7FFF);
         if (!bus->intr_status) {
             bus->controller->intr_status &= ~(1 << bus->id);
             qemu_irq_lower(bus->controller->irq);
         }
+        if (handle_rx && (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST))) {
+            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",
-- 
2.7.4