[PATCH v3 0/3] hw/i2c/aspeed: Fix Tx and Rx error

[PATCH v3 0/3] hw/i2c/aspeed: Fix Tx and Rx error

[Hang Yu]

Hi!Thanks for your advice! I made the following changes from v2 to v3:
1. Merge the submissions of patch1 and patch2 in v2, and add the fixes tag
2. Fixed typos
3. patch3:
  3.1 Simplified the judgment logic of buffer organization.
  3.2 Change the buffer organization field to a shared field, and move it 
  from I2CC_POOL_CTRL to I2CD_POOL_CTRL.

Hang Yu.

Hang Yu (3):
  hw/i2c/aspeed: Fix Tx count and Rx size error in buffer pool mode
  hw/i2c/aspeed: Fix TXBUF transmission start position error
  hw/i2c/aspeed: Add support for buffer organization

 hw/i2c/aspeed_i2c.c         | 39 ++++++++++++-------------------------
 include/hw/i2c/aspeed_i2c.h |  5 +++--
 2 files changed, 15 insertions(+), 29 deletions(-)

-- 
2.39.2 (Apple Git-143)

[PATCH v3 1/3] hw/i2c/aspeed: Fix Tx count and Rx size error in buffer pool mode

[Hang Yu]

Fixed inconsistency between the regisiter bit field definition header file
and the ast2600 datasheet. The reg name is I2CD1C:Pool Buffer Control
Register in old register mode and  I2CC0C: Master/Slave Pool Buffer Control
Register in new register mode. They share bit field
[12:8]:Transmit Data Byte Count and bit field
[29:24]:Actual Received Pool Buffer Size according to the datasheet.
According to the ast2600 datasheet,the actual Tx count is
Transmit Data Byte Count plus 1, and the max Rx size is
Receive Pool Buffer Size plus 1, both in Pool Buffer Control Register.
The version before forgot to plus 1, and mistake Rx count for Rx size.

Signed-off-by: Hang Yu <francis_yuu@stu.pku.edu.cn>
Fixes: 3be3d6ccf2ad ("aspeed: i2c: Migrate to registerfields API")
---
v2-->v3:
1. Merged patch1 and patch2 in v2
2. added fixes tag
3. Fixed typos

 hw/i2c/aspeed_i2c.c         | 8 ++++----
 include/hw/i2c/aspeed_i2c.h | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index 1f071a3811..e485d8bfb8 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -236,7 +236,7 @@ static int aspeed_i2c_bus_send(AspeedI2CBus *bus, uint8_t pool_start)
     uint32_t reg_byte_buf = aspeed_i2c_bus_byte_buf_offset(bus);
     uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
     int pool_tx_count = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl,
-                                                TX_COUNT);
+                                                TX_COUNT) + 1;
 
     if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_BUFF_EN)) {
         for (i = pool_start; i < pool_tx_count; i++) {
@@ -293,7 +293,7 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
     uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
     uint32_t reg_dma_addr = aspeed_i2c_bus_dma_addr_offset(bus);
     int pool_rx_count = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl,
-                                                RX_COUNT);
+                                                RX_SIZE) + 1;
 
     if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, RX_BUFF_EN)) {
         uint8_t *pool_base = aic->bus_pool_base(bus);
@@ -418,7 +418,7 @@ static void aspeed_i2c_bus_cmd_dump(AspeedI2CBus *bus)
     uint32_t reg_intr_sts = aspeed_i2c_bus_intr_sts_offset(bus);
     uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
     if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, RX_BUFF_EN)) {
-        count = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl, TX_COUNT);
+        count = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl, TX_COUNT) + 1;
     } else if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, RX_DMA_EN)) {
         count = bus->regs[reg_dma_len];
     } else { /* BYTE mode */
@@ -490,7 +490,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
          */
         if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_BUFF_EN)) {
             if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl, TX_COUNT)
-                == 1) {
+                == 0) {
                 SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_TX_CMD, 0);
             } else {
                 /*
diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
index 51c944efea..2e1e15aaf0 100644
--- a/include/hw/i2c/aspeed_i2c.h
+++ b/include/hw/i2c/aspeed_i2c.h
@@ -139,9 +139,9 @@ REG32(I2CD_CMD, 0x14) /* I2CD Command/Status */
 REG32(I2CD_DEV_ADDR, 0x18) /* Slave Device Address */
     SHARED_FIELD(SLAVE_DEV_ADDR1, 0, 7)
 REG32(I2CD_POOL_CTRL, 0x1C) /* Pool Buffer Control */
-    SHARED_FIELD(RX_COUNT, 24, 5)
+    SHARED_FIELD(RX_COUNT, 24, 6)
     SHARED_FIELD(RX_SIZE, 16, 5)
-    SHARED_FIELD(TX_COUNT, 9, 5)
+    SHARED_FIELD(TX_COUNT, 8, 5)
     FIELD(I2CD_POOL_CTRL, OFFSET, 2, 6) /* AST2400 */
 REG32(I2CD_BYTE_BUF, 0x20) /* Transmit/Receive Byte Buffer */
     SHARED_FIELD(RX_BUF, 8, 8)
-- 
2.39.2 (Apple Git-143)

[PATCH v3 2/3] hw/i2c/aspeed: Fix TXBUF transmission start position error

[Hang Yu]

According to the ast2600 datasheet and the linux aspeed i2c driver,
the TXBUF transmission start position should be TXBUF[0] instead
of TXBUF[1],so the arg pool_start is useless,and the address is not
included in TXBUF.So even if Tx Count equals zero,there is at least
1 byte data needs to be transmitted,and M_TX_CMD should not be cleared
at this condition.The driver url is:
https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v5.15/drivers/i2c/busses/i2c-ast2600.c

Signed-off-by: Hang Yu <francis_yuu@stu.pku.edu.cn>
---
v2-->v3:
No change.

 hw/i2c/aspeed_i2c.c | 30 ++++++------------------------
 1 file changed, 6 insertions(+), 24 deletions(-)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index e485d8bfb8..44905d7899 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -226,7 +226,7 @@ static int aspeed_i2c_dma_read(AspeedI2CBus *bus, uint8_t *data)
     return 0;
 }
 
-static int aspeed_i2c_bus_send(AspeedI2CBus *bus, uint8_t pool_start)
+static int aspeed_i2c_bus_send(AspeedI2CBus *bus)
 {
     AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
     int ret = -1;
@@ -239,7 +239,7 @@ static int aspeed_i2c_bus_send(AspeedI2CBus *bus, uint8_t pool_start)
                                                 TX_COUNT) + 1;
 
     if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_BUFF_EN)) {
-        for (i = pool_start; i < pool_tx_count; i++) {
+        for (i = 0; i < pool_tx_count; i++) {
             uint8_t *pool_base = aic->bus_pool_base(bus);
 
             trace_aspeed_i2c_bus_send("BUF", i + 1, pool_tx_count,
@@ -273,7 +273,7 @@ static int aspeed_i2c_bus_send(AspeedI2CBus *bus, uint8_t pool_start)
         }
         SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, TX_DMA_EN, 0);
     } else {
-        trace_aspeed_i2c_bus_send("BYTE", pool_start, 1,
+        trace_aspeed_i2c_bus_send("BYTE", 0, 1,
                                   bus->regs[reg_byte_buf]);
         ret = i2c_send(bus->bus, bus->regs[reg_byte_buf]);
     }
@@ -446,10 +446,8 @@ static void aspeed_i2c_bus_cmd_dump(AspeedI2CBus *bus)
  */
 static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
 {
-    uint8_t pool_start = 0;
     uint32_t reg_intr_sts = aspeed_i2c_bus_intr_sts_offset(bus);
     uint32_t reg_cmd = aspeed_i2c_bus_cmd_offset(bus);
-    uint32_t reg_pool_ctrl = aspeed_i2c_bus_pool_ctrl_offset(bus);
     uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
 
     if (!aspeed_i2c_check_sram(bus)) {
@@ -483,27 +481,11 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
 
         SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_START_CMD, 0);
 
-        /*
-         * The START command is also a TX command, as the slave
-         * address is sent on the bus. Drop the TX flag if nothing
-         * else needs to be sent in this sequence.
-         */
-        if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_BUFF_EN)) {
-            if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl, TX_COUNT)
-                == 0) {
-                SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_TX_CMD, 0);
-            } else {
-                /*
-                 * Increase the start index in the TX pool buffer to
-                 * skip the address byte.
-                 */
-                pool_start++;
-            }
-        } else if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_DMA_EN)) {
+        if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_DMA_EN)) {
             if (bus->regs[reg_dma_len] == 0) {
                 SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_TX_CMD, 0);
             }
-        } else {
+        } else if (!SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_BUFF_EN)) {
             SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_TX_CMD, 0);
         }
 
@@ -520,7 +502,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
 
     if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, M_TX_CMD)) {
         aspeed_i2c_set_state(bus, I2CD_MTXD);
-        if (aspeed_i2c_bus_send(bus, pool_start)) {
+        if (aspeed_i2c_bus_send(bus)) {
             SHARED_ARRAY_FIELD_DP32(bus->regs, reg_intr_sts, TX_NAK, 1);
             i2c_end_transfer(bus->bus);
         } else {
-- 
2.39.2 (Apple Git-143)

[PATCH v3 3/3] hw/i2c/aspeed: Add support for buffer organization

[Hang Yu]

Added support for the buffer organization option in pool buffer control
register.when set to 1,The buffer is split into two parts: Lower 16 bytes
for Tx and higher 16 bytes for Rx.

Signed-off-by: Hang Yu <francis_yuu@stu.pku.edu.cn>
---
v2-->v3:
1. Fixed typos
2. Simplified the judgment logic of buffer organization.
3. Change the buffer organization field to a shared field, and move it 
from I2CC_POOL_CTRL to I2CD_POOL_CTRL.

 hw/i2c/aspeed_i2c.c         | 3 +++
 include/hw/i2c/aspeed_i2c.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index 44905d7899..5d84797027 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -297,6 +297,9 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
 
     if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, RX_BUFF_EN)) {
         uint8_t *pool_base = aic->bus_pool_base(bus);
+        if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl, BUF_ORGANIZATION)) {
+            pool_base += 16;
+        }
 
         for (i = 0; i < pool_rx_count; i++) {
             pool_base[i] = i2c_recv(bus->bus);
diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
index 2e1e15aaf0..a064479e59 100644
--- a/include/hw/i2c/aspeed_i2c.h
+++ b/include/hw/i2c/aspeed_i2c.h
@@ -143,6 +143,7 @@ REG32(I2CD_POOL_CTRL, 0x1C) /* Pool Buffer Control */
     SHARED_FIELD(RX_SIZE, 16, 5)
     SHARED_FIELD(TX_COUNT, 8, 5)
     FIELD(I2CD_POOL_CTRL, OFFSET, 2, 6) /* AST2400 */
+    SHARED_FIELD(BUF_ORGANIZATION, 0, 1) /* AST2600 */
 REG32(I2CD_BYTE_BUF, 0x20) /* Transmit/Receive Byte Buffer */
     SHARED_FIELD(RX_BUF, 8, 8)
     SHARED_FIELD(TX_BUF, 0, 8)
-- 
2.39.2 (Apple Git-143)

Re: [PATCH v3 1/3] hw/i2c/aspeed: Fix Tx count and Rx size error in buffer pool mode

[Cédric Le Goater]

On 8/12/23 08:52, Hang Yu wrote:
> Fixed inconsistency between the regisiter bit field definition header file
> and the ast2600 datasheet. The reg name is I2CD1C:Pool Buffer Control
> Register in old register mode and  I2CC0C: Master/Slave Pool Buffer Control
> Register in new register mode. They share bit field
> [12:8]:Transmit Data Byte Count and bit field
> [29:24]:Actual Received Pool Buffer Size according to the datasheet.
> According to the ast2600 datasheet,the actual Tx count is
> Transmit Data Byte Count plus 1, and the max Rx size is
> Receive Pool Buffer Size plus 1, both in Pool Buffer Control Register.
> The version before forgot to plus 1, and mistake Rx count for Rx size.
> 
> Signed-off-by: Hang Yu <francis_yuu@stu.pku.edu.cn>
> Fixes: 3be3d6ccf2ad ("aspeed: i2c: Migrate to registerfields API")

This is -stable material with the following patch. It fixes support for
the v08.06 SDK.

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.


> ---
> v2-->v3:
> 1. Merged patch1 and patch2 in v2
> 2. added fixes tag
> 3. Fixed typos
> 
>   hw/i2c/aspeed_i2c.c         | 8 ++++----
>   include/hw/i2c/aspeed_i2c.h | 4 ++--
>   2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index 1f071a3811..e485d8bfb8 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -236,7 +236,7 @@ static int aspeed_i2c_bus_send(AspeedI2CBus *bus, uint8_t pool_start)
>       uint32_t reg_byte_buf = aspeed_i2c_bus_byte_buf_offset(bus);
>       uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
>       int pool_tx_count = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl,
> -                                                TX_COUNT);
> +                                                TX_COUNT) + 1;
>   
>       if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_BUFF_EN)) {
>           for (i = pool_start; i < pool_tx_count; i++) {
> @@ -293,7 +293,7 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
>       uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
>       uint32_t reg_dma_addr = aspeed_i2c_bus_dma_addr_offset(bus);
>       int pool_rx_count = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl,
> -                                                RX_COUNT);
> +                                                RX_SIZE) + 1;
>   
>       if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, RX_BUFF_EN)) {
>           uint8_t *pool_base = aic->bus_pool_base(bus);
> @@ -418,7 +418,7 @@ static void aspeed_i2c_bus_cmd_dump(AspeedI2CBus *bus)
>       uint32_t reg_intr_sts = aspeed_i2c_bus_intr_sts_offset(bus);
>       uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
>       if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, RX_BUFF_EN)) {
> -        count = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl, TX_COUNT);
> +        count = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl, TX_COUNT) + 1;
>       } else if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, RX_DMA_EN)) {
>           count = bus->regs[reg_dma_len];
>       } else { /* BYTE mode */
> @@ -490,7 +490,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>            */
>           if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_BUFF_EN)) {
>               if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl, TX_COUNT)
> -                == 1) {
> +                == 0) {
>                   SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_TX_CMD, 0);
>               } else {
>                   /*
> diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
> index 51c944efea..2e1e15aaf0 100644
> --- a/include/hw/i2c/aspeed_i2c.h
> +++ b/include/hw/i2c/aspeed_i2c.h
> @@ -139,9 +139,9 @@ REG32(I2CD_CMD, 0x14) /* I2CD Command/Status */
>   REG32(I2CD_DEV_ADDR, 0x18) /* Slave Device Address */
>       SHARED_FIELD(SLAVE_DEV_ADDR1, 0, 7)
>   REG32(I2CD_POOL_CTRL, 0x1C) /* Pool Buffer Control */
> -    SHARED_FIELD(RX_COUNT, 24, 5)
> +    SHARED_FIELD(RX_COUNT, 24, 6)
>       SHARED_FIELD(RX_SIZE, 16, 5)
> -    SHARED_FIELD(TX_COUNT, 9, 5)
> +    SHARED_FIELD(TX_COUNT, 8, 5)
>       FIELD(I2CD_POOL_CTRL, OFFSET, 2, 6) /* AST2400 */
>   REG32(I2CD_BYTE_BUF, 0x20) /* Transmit/Receive Byte Buffer */
>       SHARED_FIELD(RX_BUF, 8, 8)

Re: [PATCH v3 2/3] hw/i2c/aspeed: Fix TXBUF transmission start position error

[Cédric Le Goater]

On 8/12/23 08:52, Hang Yu wrote:
> According to the ast2600 datasheet and the linux aspeed i2c driver,
> the TXBUF transmission start position should be TXBUF[0] instead
> of TXBUF[1],so the arg pool_start is useless,and the address is not
> included in TXBUF.So even if Tx Count equals zero,there is at least
> 1 byte data needs to be transmitted,and M_TX_CMD should not be cleared
> at this condition.The driver url is:
> https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v5.15/drivers/i2c/busses/i2c-ast2600.c
> 
> Signed-off-by: Hang Yu <francis_yuu@stu.pku.edu.cn>

This is -stable material with the previous patch. It fixes support for
the v08.06 SDK.

Fixes: 6054fc73e8f4 ("aspeed/i2c: Add support for pool buffer transfers")

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.


> ---
> v2-->v3:
> No change.
> 
>   hw/i2c/aspeed_i2c.c | 30 ++++++------------------------
>   1 file changed, 6 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index e485d8bfb8..44905d7899 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -226,7 +226,7 @@ static int aspeed_i2c_dma_read(AspeedI2CBus *bus, uint8_t *data)
>       return 0;
>   }
>   
> -static int aspeed_i2c_bus_send(AspeedI2CBus *bus, uint8_t pool_start)
> +static int aspeed_i2c_bus_send(AspeedI2CBus *bus)
>   {
>       AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
>       int ret = -1;
> @@ -239,7 +239,7 @@ static int aspeed_i2c_bus_send(AspeedI2CBus *bus, uint8_t pool_start)
>                                                   TX_COUNT) + 1;
>   
>       if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_BUFF_EN)) {
> -        for (i = pool_start; i < pool_tx_count; i++) {
> +        for (i = 0; i < pool_tx_count; i++) {
>               uint8_t *pool_base = aic->bus_pool_base(bus);
>   
>               trace_aspeed_i2c_bus_send("BUF", i + 1, pool_tx_count,
> @@ -273,7 +273,7 @@ static int aspeed_i2c_bus_send(AspeedI2CBus *bus, uint8_t pool_start)
>           }
>           SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, TX_DMA_EN, 0);
>       } else {
> -        trace_aspeed_i2c_bus_send("BYTE", pool_start, 1,
> +        trace_aspeed_i2c_bus_send("BYTE", 0, 1,
>                                     bus->regs[reg_byte_buf]);
>           ret = i2c_send(bus->bus, bus->regs[reg_byte_buf]);
>       }
> @@ -446,10 +446,8 @@ static void aspeed_i2c_bus_cmd_dump(AspeedI2CBus *bus)
>    */
>   static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>   {
> -    uint8_t pool_start = 0;
>       uint32_t reg_intr_sts = aspeed_i2c_bus_intr_sts_offset(bus);
>       uint32_t reg_cmd = aspeed_i2c_bus_cmd_offset(bus);
> -    uint32_t reg_pool_ctrl = aspeed_i2c_bus_pool_ctrl_offset(bus);
>       uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
>   
>       if (!aspeed_i2c_check_sram(bus)) {
> @@ -483,27 +481,11 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>   
>           SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_START_CMD, 0);
>   
> -        /*
> -         * The START command is also a TX command, as the slave
> -         * address is sent on the bus. Drop the TX flag if nothing
> -         * else needs to be sent in this sequence.
> -         */
> -        if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_BUFF_EN)) {
> -            if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl, TX_COUNT)
> -                == 0) {
> -                SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_TX_CMD, 0);
> -            } else {
> -                /*
> -                 * Increase the start index in the TX pool buffer to
> -                 * skip the address byte.
> -                 */
> -                pool_start++;
> -            }
> -        } else if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_DMA_EN)) {
> +        if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_DMA_EN)) {
>               if (bus->regs[reg_dma_len] == 0) {
>                   SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_TX_CMD, 0);
>               }
> -        } else {
> +        } else if (!SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_BUFF_EN)) {
>               SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_TX_CMD, 0);
>           }
>   
> @@ -520,7 +502,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>   
>       if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, M_TX_CMD)) {
>           aspeed_i2c_set_state(bus, I2CD_MTXD);
> -        if (aspeed_i2c_bus_send(bus, pool_start)) {
> +        if (aspeed_i2c_bus_send(bus)) {
>               SHARED_ARRAY_FIELD_DP32(bus->regs, reg_intr_sts, TX_NAK, 1);
>               i2c_end_transfer(bus->bus);
>           } else {

Re: [PATCH v3 3/3] hw/i2c/aspeed: Add support for buffer organization

[Cédric Le Goater]

On 8/12/23 08:52, Hang Yu wrote:
> Added support for the buffer organization option in pool buffer control
> register.when set to 1,The buffer is split into two parts: Lower 16 bytes
> for Tx and higher 16 bytes for Rx.
> 
> Signed-off-by: Hang Yu <francis_yuu@stu.pku.edu.cn>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.


> ---
> v2-->v3:
> 1. Fixed typos
> 2. Simplified the judgment logic of buffer organization.
> 3. Change the buffer organization field to a shared field, and move it
> from I2CC_POOL_CTRL to I2CD_POOL_CTRL.
> 
>   hw/i2c/aspeed_i2c.c         | 3 +++
>   include/hw/i2c/aspeed_i2c.h | 1 +
>   2 files changed, 4 insertions(+)
> 
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index 44905d7899..5d84797027 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -297,6 +297,9 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
>   
>       if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, RX_BUFF_EN)) {
>           uint8_t *pool_base = aic->bus_pool_base(bus);
> +        if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl, BUF_ORGANIZATION)) {
> +            pool_base += 16;
> +        }
>   
>           for (i = 0; i < pool_rx_count; i++) {
>               pool_base[i] = i2c_recv(bus->bus);
> diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
> index 2e1e15aaf0..a064479e59 100644
> --- a/include/hw/i2c/aspeed_i2c.h
> +++ b/include/hw/i2c/aspeed_i2c.h
> @@ -143,6 +143,7 @@ REG32(I2CD_POOL_CTRL, 0x1C) /* Pool Buffer Control */
>       SHARED_FIELD(RX_SIZE, 16, 5)
>       SHARED_FIELD(TX_COUNT, 8, 5)
>       FIELD(I2CD_POOL_CTRL, OFFSET, 2, 6) /* AST2400 */
> +    SHARED_FIELD(BUF_ORGANIZATION, 0, 1) /* AST2600 */
>   REG32(I2CD_BYTE_BUF, 0x20) /* Transmit/Receive Byte Buffer */
>       SHARED_FIELD(RX_BUF, 8, 8)
>       SHARED_FIELD(TX_BUF, 0, 8)

Re:Re: [PATCH v3 1/3] hw/i2c/aspeed: Fix Tx count and Rx size error in buffer pool mode

[Hang Yu]

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

Hello! Thank you for your review!Sorry I forgot to cc other maintainers so I resend this mail.


From: "Cédric Le Goater" <clg@kaod.org>
Date: 2023-08-16 16:32:58
To:  Hang Yu <francis_yuu@stu.pku.edu.cn>,qemu-devel@nongnu.org
Cc:  komlodi@google.com,peter@pjd.dev,Peter Maydell <peter.maydell@linaro.org>,Andrew Jeffery <andrew@aj.id.au>,Joel Stanley <joel@jms.id.au>,"open list:ASPEED BMCs" <qemu-arm@nongnu.org>,qemu-stable@nongnu.org
Subject: Re: [PATCH v3 1/3] hw/i2c/aspeed: Fix Tx count and Rx size error in buffer pool mode>On 8/12/23 08:52, Hang Yu wrote:
>> Fixed inconsistency between the regisiter bit field definition header file
>> and the ast2600 datasheet. The reg name is I2CD1C:Pool Buffer Control
>> Register in old register mode and  I2CC0C: Master/Slave Pool Buffer Control
>> Register in new register mode. They share bit field
>> [12:8]:Transmit Data Byte Count and bit field
>> [29:24]:Actual Received Pool Buffer Size according to the datasheet.
>> According to the ast2600 datasheet,the actual Tx count is
>> Transmit Data Byte Count plus 1, and the max Rx size is
>> Receive Pool Buffer Size plus 1, both in Pool Buffer Control Register.
>> The version before forgot to plus 1, and mistake Rx count for Rx size.
>> 
>> Signed-off-by: Hang Yu <francis_yuu@stu.pku.edu.cn>
>> Fixes: 3be3d6ccf2ad ("aspeed: i2c: Migrate to registerfields API")
>
>This is -stable material with the following patch. It fixes support for
>the v08.06 SDK.
Should I add this line into the commit in next version?

>
>Reviewed-by: Cédric Le Goater <clg@kaod.org>
Should I add your Reviewed-by tag and send v4 now?Or just wait for 
other maintainers to reply?


Thanks,
Hang.
>
>Thanks,
>
>C.
>
>
>> ---
>> v2-->v3:
>> 1. Merged patch1 and patch2 in v2
>> 2. added fixes tag
>> 3. Fixed typos
>> 
>>   hw/i2c/aspeed_i2c.c         | 8 ++++----
>>   include/hw/i2c/aspeed_i2c.h | 4 ++--
>>   2 files changed, 6 insertions(+), 6 deletions(-)
>> 
>> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
>> index 1f071a3811..e485d8bfb8 100644
>> --- a/hw/i2c/aspeed_i2c.c
>> +++ b/hw/i2c/aspeed_i2c.c
>> @@ -236,7 +236,7 @@ static int aspeed_i2c_bus_send(AspeedI2CBus *bus, uint8_t pool_start)
>>       uint32_t reg_byte_buf = aspeed_i2c_bus_byte_buf_offset(bus);
>>       uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
>>       int pool_tx_count = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl,
>> -                                                TX_COUNT);
>> +                                                TX_COUNT) + 1;
>>   
>>       if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_BUFF_EN)) {
>>           for (i = pool_start; i < pool_tx_count; i++) {
>> @@ -293,7 +293,7 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
>>       uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
>>       uint32_t reg_dma_addr = aspeed_i2c_bus_dma_addr_offset(bus);
>>       int pool_rx_count = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl,
>> -                                                RX_COUNT);
>> +                                                RX_SIZE) + 1;
>>   
>>       if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, RX_BUFF_EN)) {
>>           uint8_t *pool_base = aic->bus_pool_base(bus);
>> @@ -418,7 +418,7 @@ static void aspeed_i2c_bus_cmd_dump(AspeedI2CBus *bus)
>>       uint32_t reg_intr_sts = aspeed_i2c_bus_intr_sts_offset(bus);
>>       uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
>>       if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, RX_BUFF_EN)) {
>> -        count = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl, TX_COUNT);
>> +        count = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl, TX_COUNT) + 1;
>>       } else if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, RX_DMA_EN)) {
>>           count = bus->regs[reg_dma_len];
>>       } else { /* BYTE mode */
>> @@ -490,7 +490,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>>            */
>>           if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_BUFF_EN)) {
>>               if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl, TX_COUNT)
>> -                == 1) {
>> +                == 0) {
>>                   SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_TX_CMD, 0);
>>               } else {
>>                   /*
>> diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
>> index 51c944efea..2e1e15aaf0 100644
>> --- a/include/hw/i2c/aspeed_i2c.h
>> +++ b/include/hw/i2c/aspeed_i2c.h
>> @@ -139,9 +139,9 @@ REG32(I2CD_CMD, 0x14) /* I2CD Command/Status */
>>   REG32(I2CD_DEV_ADDR, 0x18) /* Slave Device Address */
>>       SHARED_FIELD(SLAVE_DEV_ADDR1, 0, 7)
>>   REG32(I2CD_POOL_CTRL, 0x1C) /* Pool Buffer Control */
>> -    SHARED_FIELD(RX_COUNT, 24, 5)
>> +    SHARED_FIELD(RX_COUNT, 24, 6)
>>       SHARED_FIELD(RX_SIZE, 16, 5)
>> -    SHARED_FIELD(TX_COUNT, 9, 5)
>> +    SHARED_FIELD(TX_COUNT, 8, 5)
>>       FIELD(I2CD_POOL_CTRL, OFFSET, 2, 6) /* AST2400 */
>>   REG32(I2CD_BYTE_BUF, 0x20) /* Transmit/Receive Byte Buffer */
>>       SHARED_FIELD(RX_BUF, 8, 8)
>






[-- Attachment #2: Type: text/html, Size: 5898 bytes --]

Re: [PATCH v3 1/3] hw/i2c/aspeed: Fix Tx count and Rx size error in buffer pool mode

[Cédric Le Goater]

On 8/16/23 11:44, Hang Yu wrote:
> Hello! Thank you for your review!Sorry I forgot to cc other maintainers so I resend this mail.
> 
> From: "Cédric Le Goater" <clg@kaod.org>
> Date: 2023-08-16 16:32:58
> To:  Hang Yu <francis_yuu@stu.pku.edu.cn>,qemu-devel@nongnu.org
> Cc:  komlodi@google.com,peter@pjd.dev,Peter Maydell <peter.maydell@linaro.org>,Andrew Jeffery <andrew@aj.id.au>,Joel Stanley <joel@jms.id.au>,"open list:ASPEED BMCs" <qemu-arm@nongnu.org>,qemu-stable@nongnu.org
> Subject: Re: [PATCH v3 1/3] hw/i2c/aspeed: Fix Tx count and Rx size error in buffer pool mode>On 8/12/23 08:52, Hang Yu wrote:
>>> Fixed inconsistency between the regisiter bit field definition header file
>>> and the ast2600 datasheet. The reg name is I2CD1C:Pool Buffer Control
>>> Register in old register mode and  I2CC0C: Master/Slave Pool Buffer Control
>>> Register in new register mode. They share bit field
>>> [12:8]:Transmit Data Byte Count and bit field
>>> [29:24]:Actual Received Pool Buffer Size according to the datasheet.
>>> According to the ast2600 datasheet,the actual Tx count is
>>> Transmit Data Byte Count plus 1, and the max Rx size is
>>> Receive Pool Buffer Size plus 1, both in Pool Buffer Control Register.
>>> The version before forgot to plus 1, and mistake Rx count for Rx size.
>>> 
>>> Signed-off-by: Hang Yu <francis_yuu@stu.pku.edu.cn>
>>> Fixes: 3be3d6ccf2ad ("aspeed: i2c: Migrate to registerfields API")
>>
>>This is -stable material with the following patch. It fixes support for
>  >the v08.06 SDK.
> Should I add this line into the commit in next version?
>>
>  >Reviewed-by: Cédric Le Goater <clg@kaod.org>
> Should I add your Reviewed-by tag and send v4 now?Or just wait for
> other maintainers to reply?

No need for a v4. The tags are pulled automatically with the tooling
we use, b4, pwclient, etc.

I will prepare an aspeed PR for QEMU 8.1 with these patches. Then,
the first two could be backported to -stable.

Thanks,

C.

> 
> Thanks,
> Hang.
>>
>>Thanks,
>>
>>C.
>>
>>
>>> ---
>>> v2-->v3:
>>> 1. Merged patch1 and patch2 in v2
>>> 2. added fixes tag
>>> 3. Fixed typos
>>> 
>>>   hw/i2c/aspeed_i2c.c         | 8 ++++----
>>>   include/hw/i2c/aspeed_i2c.h | 4 ++--
>>>   2 files changed, 6 insertions(+), 6 deletions(-)
>>> 
>>> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
>>> index 1f071a3811..e485d8bfb8 100644
>>> --- a/hw/i2c/aspeed_i2c.c
>>> +++ b/hw/i2c/aspeed_i2c.c
>>> @@ -236,7 +236,7 @@ static int aspeed_i2c_bus_send(AspeedI2CBus *bus, uint8_t pool_start)
>>>       uint32_t reg_byte_buf = aspeed_i2c_bus_byte_buf_offset(bus);
>>>       uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
>>>       int pool_tx_count = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl,
>>> -                                                TX_COUNT);
>>> +                                                TX_COUNT) + 1;
>>>   
>>>       if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_BUFF_EN)) {
>>>           for (i = pool_start; i < pool_tx_count; i++) {
>>> @@ -293,7 +293,7 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
>>>       uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
>>>       uint32_t reg_dma_addr = aspeed_i2c_bus_dma_addr_offset(bus);
>>>       int pool_rx_count = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl,
>>> -                                                RX_COUNT);
>>> +                                                RX_SIZE) + 1;
>>>   
>>>       if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, RX_BUFF_EN)) {
>>>           uint8_t *pool_base = aic->bus_pool_base(bus);
>>> @@ -418,7 +418,7 @@ static void aspeed_i2c_bus_cmd_dump(AspeedI2CBus *bus)
>>>       uint32_t reg_intr_sts = aspeed_i2c_bus_intr_sts_offset(bus);
>>>       uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
>>>       if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, RX_BUFF_EN)) {
>>> -        count = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl, TX_COUNT);
>>> +        count = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl, TX_COUNT) + 1;
>>>       } else if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, RX_DMA_EN)) {
>>>           count = bus->regs[reg_dma_len];
>>>       } else { /* BYTE mode */
>>> @@ -490,7 +490,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>>>            */
>>>           if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_cmd, TX_BUFF_EN)) {
>>>               if (SHARED_ARRAY_FIELD_EX32(bus->regs, reg_pool_ctrl, TX_COUNT)
>>> -                == 1) {
>>> +                == 0) {
>>>                   SHARED_ARRAY_FIELD_DP32(bus->regs, reg_cmd, M_TX_CMD, 0);
>>>               } else {
>>>                   /*
>>> diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
>>> index 51c944efea..2e1e15aaf0 100644
>>> --- a/include/hw/i2c/aspeed_i2c.h
>>> +++ b/include/hw/i2c/aspeed_i2c.h
>>> @@ -139,9 +139,9 @@ REG32(I2CD_CMD, 0x14) /* I2CD Command/Status */
>>>   REG32(I2CD_DEV_ADDR, 0x18) /* Slave Device Address */
>>>       SHARED_FIELD(SLAVE_DEV_ADDR1, 0, 7)
>>>   REG32(I2CD_POOL_CTRL, 0x1C) /* Pool Buffer Control */
>>> -    SHARED_FIELD(RX_COUNT, 24, 5)
>>> +    SHARED_FIELD(RX_COUNT, 24, 6)
>>>       SHARED_FIELD(RX_SIZE, 16, 5)
>>> -    SHARED_FIELD(TX_COUNT, 9, 5)
>>> +    SHARED_FIELD(TX_COUNT, 8, 5)
>>>       FIELD(I2CD_POOL_CTRL, OFFSET, 2, 6) /* AST2400 */
>>>   REG32(I2CD_BYTE_BUF, 0x20) /* Transmit/Receive Byte Buffer */
>>>       SHARED_FIELD(RX_BUF, 8, 8)
>>
> 
>