Re: [PATCH v2 1/4] hw/i2c/aspeed: Fix I2CD_POOL_CTRL register bit field defination

[Cédric Le Goater]

Hello Hang,

It is good practice to send a cover letter giving a quick summary of the
changes.

On 8/11/23 07:42, Hang Yu wrote:
> Fixed inconsistency between the regisiter bit field defination in headfile

Fixed inconsistency between the register 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.
>
> Signed-off-by: Hang Yu <francis_yuu@stu.pku.edu.cn>


The initial macros included a '+ 1'

 #define   I2CD_POOL_RX_SIZE(x)             ((((x) >> 16) & 0xff) + 1)
 #define   I2CD_POOL_TX_COUNT(x)            ((((x) >> 8) & 0xff) + 1)

which was not reported in commit 3be3d6ccf2ad ("aspeed: i2c: Migrate to
registerfields API"). I think patch 1 and 2 should be folded, could you
please respin a v3 and add a Fixes tag ?

It is too late for the 8.1 cycle but these changes would be good candidate
for QEMU stable. I will queue them for 8.2.

Also, they fix booting v08.06 SDK which is great. Here is an extra patch
to change the QEMU tests to the latest version :

  
https://github.com/legoater/qemu/commit/08243703f48f5e9c263773a846f7dc655cd64bfa

You can include it in your series if you want.

Thanks,

C.



> ---
>   include/hw/i2c/aspeed_i2c.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> 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)