Hello! Thank you for your review!Sorry I forgot to cc other maintainers so I
resend this mail.
From: "Cédric Le Goater" <c...@kaod.org>
Date: 2023-08-16 16:32:58
To: Hang Yu <francis_...@stu.pku.edu.cn>,qemu-devel@nongnu.org
Cc: koml...@google.com,pe...@pjd.dev,Peter Maydell
<peter.mayd...@linaro.org>,Andrew Jeffery <and...@aj.id.au>,Joel Stanley
<j...@jms.id.au>,"open list:ASPEED BMCs"
<qemu-...@nongnu.org>,qemu-sta...@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_...@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 <c...@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)
>
