* [PATCH v3 1/3] hw/i2c/aspeed: Fix Tx count and Rx size error in buffer pool mode
2023-08-12 6:52 [PATCH v3 0/3] hw/i2c/aspeed: Fix Tx and Rx error Hang Yu
@ 2023-08-12 6:52 ` Hang Yu
2023-08-16 8:32 ` Cédric Le Goater
2023-08-12 6:52 ` [PATCH v3 2/3] hw/i2c/aspeed: Fix TXBUF transmission start position error Hang Yu
2023-08-12 6:52 ` [PATCH v3 3/3] hw/i2c/aspeed: Add support for buffer organization Hang Yu
2 siblings, 1 reply; 9+ messages in thread
From: Hang Yu @ 2023-08-12 6:52 UTC (permalink / raw)
To: qemu-devel
Cc: komlodi, peter, Hang Yu, Cédric Le Goater, Peter Maydell,
Andrew Jeffery, Joel Stanley, open list:ASPEED BMCs
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)
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v3 1/3] hw/i2c/aspeed: Fix Tx count and Rx size error in buffer pool mode
2023-08-12 6:52 ` [PATCH v3 1/3] hw/i2c/aspeed: Fix Tx count and Rx size error in buffer pool mode Hang Yu
@ 2023-08-16 8:32 ` Cédric Le Goater
2023-08-16 9:44 ` Hang Yu
0 siblings, 1 reply; 9+ messages in thread
From: Cédric Le Goater @ 2023-08-16 8:32 UTC (permalink / raw)
To: Hang Yu, qemu-devel
Cc: komlodi, peter, Peter Maydell, Andrew Jeffery, Joel Stanley,
open list:ASPEED BMCs, qemu-stable
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)
^ permalink raw reply [flat|nested] 9+ messages in thread* Re:Re: [PATCH v3 1/3] hw/i2c/aspeed: Fix Tx count and Rx size error in buffer pool mode
2023-08-16 8:32 ` Cédric Le Goater
@ 2023-08-16 9:44 ` Hang Yu
2023-08-28 8:54 ` Cédric Le Goater
0 siblings, 1 reply; 9+ messages in thread
From: Hang Yu @ 2023-08-16 9:44 UTC (permalink / raw)
To: Cédric Le Goater
Cc: qemu-devel, komlodi, peter, Peter Maydell, Andrew Jeffery,
Joel Stanley, open list:ASPEED BMCs, qemu-stable
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v3 1/3] hw/i2c/aspeed: Fix Tx count and Rx size error in buffer pool mode
2023-08-16 9:44 ` Hang Yu
@ 2023-08-28 8:54 ` Cédric Le Goater
0 siblings, 0 replies; 9+ messages in thread
From: Cédric Le Goater @ 2023-08-28 8:54 UTC (permalink / raw)
To: Hang Yu
Cc: qemu-devel, komlodi, peter, Peter Maydell, Andrew Jeffery,
Joel Stanley, open list:ASPEED BMCs, qemu-stable
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)
>>
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 2/3] hw/i2c/aspeed: Fix TXBUF transmission start position error
2023-08-12 6:52 [PATCH v3 0/3] hw/i2c/aspeed: Fix Tx and Rx error Hang Yu
2023-08-12 6:52 ` [PATCH v3 1/3] hw/i2c/aspeed: Fix Tx count and Rx size error in buffer pool mode Hang Yu
@ 2023-08-12 6:52 ` Hang Yu
2023-08-16 8:33 ` Cédric Le Goater
2023-08-12 6:52 ` [PATCH v3 3/3] hw/i2c/aspeed: Add support for buffer organization Hang Yu
2 siblings, 1 reply; 9+ messages in thread
From: Hang Yu @ 2023-08-12 6:52 UTC (permalink / raw)
To: qemu-devel
Cc: komlodi, peter, Hang Yu, Cédric Le Goater, Peter Maydell,
Andrew Jeffery, Joel Stanley, open list:ASPEED BMCs
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)
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v3 2/3] hw/i2c/aspeed: Fix TXBUF transmission start position error
2023-08-12 6:52 ` [PATCH v3 2/3] hw/i2c/aspeed: Fix TXBUF transmission start position error Hang Yu
@ 2023-08-16 8:33 ` Cédric Le Goater
0 siblings, 0 replies; 9+ messages in thread
From: Cédric Le Goater @ 2023-08-16 8:33 UTC (permalink / raw)
To: Hang Yu, qemu-devel
Cc: komlodi, peter, Peter Maydell, Andrew Jeffery, Joel Stanley,
open list:ASPEED BMCs, qemu-stable
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 {
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 3/3] hw/i2c/aspeed: Add support for buffer organization
2023-08-12 6:52 [PATCH v3 0/3] hw/i2c/aspeed: Fix Tx and Rx error Hang Yu
2023-08-12 6:52 ` [PATCH v3 1/3] hw/i2c/aspeed: Fix Tx count and Rx size error in buffer pool mode Hang Yu
2023-08-12 6:52 ` [PATCH v3 2/3] hw/i2c/aspeed: Fix TXBUF transmission start position error Hang Yu
@ 2023-08-12 6:52 ` Hang Yu
2023-08-16 8:33 ` Cédric Le Goater
2 siblings, 1 reply; 9+ messages in thread
From: Hang Yu @ 2023-08-12 6:52 UTC (permalink / raw)
To: qemu-devel
Cc: komlodi, peter, Hang Yu, Cédric Le Goater, Peter Maydell,
Andrew Jeffery, Joel Stanley, open list:ASPEED BMCs
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)
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v3 3/3] hw/i2c/aspeed: Add support for buffer organization
2023-08-12 6:52 ` [PATCH v3 3/3] hw/i2c/aspeed: Add support for buffer organization Hang Yu
@ 2023-08-16 8:33 ` Cédric Le Goater
0 siblings, 0 replies; 9+ messages in thread
From: Cédric Le Goater @ 2023-08-16 8:33 UTC (permalink / raw)
To: Hang Yu, qemu-devel
Cc: komlodi, peter, Peter Maydell, Andrew Jeffery, Joel Stanley,
open list:ASPEED BMCs
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)
^ permalink raw reply [flat|nested] 9+ messages in thread