Hi Cedric,
> Subject: Re: [PATCH v2 06/11] hw/i2c/aspeed: support Tx/Rx buffer 64 bits
> address
>
> Jamin,
>
> Please change commit title to
>
Thanks for review and suggestion.
Will fix them in v3 patch.
Jamin
> hw/i2c/aspeed: Add support for Tx/Rx buffer 64 bits address
>
> So more proposals to improve the commit log below,
>
> On 8/8/24 04:49, Jamin Lin wrote:
> > ASPEED AST2700 SOC is a 64 bits quad core CPUs (Cortex-a35) And the
> > base address of dram is "0x4 00000000" which
>
> and
>
> > is 64bits address.
> >
> > It have "Master DMA Mode Tx Buffer Base Address[39:32](0x60)"
>
> it has
>
> > and "Master DMA Mode Rx Buffer Base Address[39:32](0x64)"
>
> ... registers
>
> > to save the high part physical address of Tx/Rx buffer address for
> > master mode.
> >
> > It have "Slave DMA Mode Tx Buffer Base Address[39:32](0x68)" and
>
> It has
>
> > "Slave DMA Mode Rx Buffer Base Address[39:32](0x6C)"
>
> registers
>
> > to save the high part physical address of Tx/Rx buffer address for
> > slave mode.
> >
> > Ex: Tx buffer address for master mode [39:0] The "Master DMA Mode Tx
> > Buffer Base Address[39:32](0x60)"
> > bits [7:0] which corresponds the bits [39:32] of the 64 bits address
> > of the Tx buffer address.
> > The "Master DMA Mode Tx Buffer Base Address(0x30)" bits [31:0] which
> > corresponds the bits [31:0] of the 64 bits address of the Tx buffer
> > address.
> >
> > Introduce a new has_dma64 class attribute and new registers of
>
> ... new registers for the
>
> > new mode to support DMA 64 bits dram address.
> > Update new mode register number to 28.
>
>
> I think it is worth mentioning that aspeed_i2c_bus_vmstate is changed again
> and version is not increased because it was done earlier in the same series.
>
>
> Thanks,
>
> C.
>
> >
> > Signed-off-by: Jamin Lin <jamin_...@aspeedtech.com>
> > Reviewed-by: Cédric Le Goater <c...@redhat.com>
> > ---
> > hw/i2c/aspeed_i2c.c | 48
> +++++++++++++++++++++++++++++++++++++
> > include/hw/i2c/aspeed_i2c.h | 12 +++++++++-
> > 2 files changed, 59 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c index
> > 819a875839..c1ff80b1cf 100644
> > --- a/hw/i2c/aspeed_i2c.c
> > +++ b/hw/i2c/aspeed_i2c.c
> > @@ -140,6 +140,7 @@ static uint64_t
> aspeed_i2c_bus_old_read(AspeedI2CBus *bus, hwaddr offset,
> > static uint64_t aspeed_i2c_bus_new_read(AspeedI2CBus *bus, hwaddr
> offset,
> > unsigned size)
> > {
> > + AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
> > uint64_t value = bus->regs[offset / sizeof(*bus->regs)];
> >
> > switch (offset) {
> > @@ -170,6 +171,16 @@ static uint64_t
> aspeed_i2c_bus_new_read(AspeedI2CBus *bus, hwaddr offset,
> > case A_I2CM_CMD:
> > value = SHARED_FIELD_DP32(value, BUS_BUSY_STS,
> i2c_bus_busy(bus->bus));
> > break;
> > + case A_I2CM_DMA_TX_ADDR_HI:
> > + case A_I2CM_DMA_RX_ADDR_HI:
> > + case A_I2CS_DMA_TX_ADDR_HI:
> > + case A_I2CS_DMA_RX_ADDR_HI:
> > + if (!aic->has_dma64) {
> > + qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA 64 bits
> support\n",
> > + __func__);
> > + value = -1;
> > + }
> > + break;
> > default:
> > qemu_log_mask(LOG_GUEST_ERROR,
> > "%s: Bad offset 0x%" HWADDR_PRIx "\n",
> > __func__, offset); @@ -731,6 +742,42 @@ static void
> aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
> > qemu_log_mask(LOG_UNIMP, "%s: Slave mode DMA TX is not
> implemented\n",
> > __func__);
> > break;
> > +
> > + case A_I2CM_DMA_TX_ADDR_HI:
> > + if (!aic->has_dma64) {
> > + qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA 64 bits
> support\n",
> > + __func__);
> > + break;
> > + }
> > + bus->regs[R_I2CM_DMA_TX_ADDR_HI] = FIELD_EX32(value,
> > +
> I2CM_DMA_TX_ADDR_HI,
> > +
> ADDR_HI);
> > + break;
> > + case A_I2CM_DMA_RX_ADDR_HI:
> > + if (!aic->has_dma64) {
> > + qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA 64 bits
> support\n",
> > + __func__);
> > + break;
> > + }
> > + bus->regs[R_I2CM_DMA_RX_ADDR_HI] = FIELD_EX32(value,
> > +
> I2CM_DMA_RX_ADDR_HI,
> > +
> ADDR_HI);
> > + break;
> > + case A_I2CS_DMA_TX_ADDR_HI:
> > + qemu_log_mask(LOG_UNIMP,
> > + "%s: Slave mode DMA TX Addr high is not
> implemented\n",
> > + __func__);
> > + break;
> > + case A_I2CS_DMA_RX_ADDR_HI:
> > + if (!aic->has_dma64) {
> > + qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA 64 bits
> support\n",
> > + __func__);
> > + break;
> > + }
> > + bus->regs[R_I2CS_DMA_RX_ADDR_HI] = FIELD_EX32(value,
> > +
> I2CS_DMA_RX_ADDR_HI,
> > +
> ADDR_HI);
> > + break;
> > default:
> > qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%"
> HWADDR_PRIx "\n",
> > __func__, offset); @@ -1554,6 +1601,7 @@
> > static void aspeed_2700_i2c_class_init(ObjectClass *klass, void *data)
> > aic->bus_pool_base = aspeed_2500_i2c_bus_pool_base;
> > aic->has_dma = true;
> > aic->mem_size = 0x2000;
> > + aic->has_dma64 = true;
> > }
> >
> > static const TypeInfo aspeed_2700_i2c_info = { diff --git
> > a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h index
> > 4f23dc10c3..2c4c81bd20 100644
> > --- a/include/hw/i2c/aspeed_i2c.h
> > +++ b/include/hw/i2c/aspeed_i2c.h
> > @@ -38,7 +38,7 @@ OBJECT_DECLARE_TYPE(AspeedI2CState,
> AspeedI2CClass, ASPEED_I2C)
> > #define ASPEED_I2C_SHARE_POOL_SIZE 0x800
> > #define ASPEED_I2C_BUS_POOL_SIZE 0x20
> > #define ASPEED_I2C_OLD_NUM_REG 11
> > -#define ASPEED_I2C_NEW_NUM_REG 22
> > +#define ASPEED_I2C_NEW_NUM_REG 28
> >
> > #define A_I2CD_M_STOP_CMD BIT(5)
> > #define A_I2CD_M_RX_CMD BIT(3)
> > @@ -227,6 +227,15 @@ REG32(I2CS_DMA_LEN_STS, 0x4c)
> > FIELD(I2CS_DMA_LEN_STS, TX_LEN, 0, 13)
> > REG32(I2CC_DMA_ADDR, 0x50)
> > REG32(I2CC_DMA_LEN, 0x54)
> > +/* DMA 64bits */
> > +REG32(I2CM_DMA_TX_ADDR_HI, 0x60)
> > + FIELD(I2CM_DMA_TX_ADDR_HI, ADDR_HI, 0, 7)
> > +REG32(I2CM_DMA_RX_ADDR_HI, 0x64)
> > + FIELD(I2CM_DMA_RX_ADDR_HI, ADDR_HI, 0, 7)
> > +REG32(I2CS_DMA_TX_ADDR_HI, 0x68)
> > + FIELD(I2CS_DMA_TX_ADDR_HI, ADDR_HI, 0, 7)
> > +REG32(I2CS_DMA_RX_ADDR_HI, 0x6c)
> > + FIELD(I2CS_DMA_RX_ADDR_HI, ADDR_HI, 0, 7)
> >
> > struct AspeedI2CState;
> >
> > @@ -292,6 +301,7 @@ struct AspeedI2CClass {
> > bool has_dma;
> > bool has_share_pool;
> > uint64_t mem_size;
> > + bool has_dma64;
> > };
> >
> > static inline bool aspeed_i2c_is_new_mode(AspeedI2CState *s)
