Hi Cedric,
> Subject: Re: [PATCH v2 04/11] hw/i2c/aspeed: introduce a new
> dma_dram_offset attribute in AspeedI2Cbus
>
> Jamin,
>
> In case you resend, would you mind changing the commit title and use a
> capital letter on the first word :
>
Okay, I will change all commit title to use a capital letter on the first word
in the next patch series(v3)
> hw/i2c/aspeed: Introduce a new dma_dram_offset attribute in
> AspeedI2Cbus
>
> On 8/8/24 04:49, Jamin Lin wrote:
> > The "Current DMA Operating Address Status(0x50)" register of I2C new
> > mode has been removed in AST2700.
> > This register is used for debugging and it is a read only register.
> >
> > To support AST2700 DMA mode, introduce a new dma_dram_offset class
> > attribute in AspeedI2Cbus to save the current DMA operating address.
> >
> > ASPEED AST2700 SOC is a 64 bits quad core CPUs (Cortex-a35) And the
> > base address of dram is "0x4 00000000" which is 64bits address.
> >
> > Set the dma_dram_offset data type to uint64_t for
> > 64 bits dram address DMA support.
> >
> > Both "DMA Mode Buffer Address Register(I2CD24 old mode)" and "DMA
> > Operating Address Status (I2CC50 new mode)" are used for showing the
> > low part dram offset bits [31:0], so change to read/write both
> > register bits [31:0] in bus register read/write functions.
>
> 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.
>
Got it will add this description in this commit message.
Thanks for review and suggestion.
Jamin
>
> Thanks,
>
> C.
>
>
>
> > Signed-off-by: Jamin Lin <jamin_...@aspeedtech.com>
> > ---
> > hw/i2c/aspeed_i2c.c | 51
> +++++++++++++++++++++++--------------
> > include/hw/i2c/aspeed_i2c.h | 9 +------
> > 2 files changed, 33 insertions(+), 27 deletions(-)
> >
> > diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c index
> > abcb1d5330..2dea3a42a0 100644
> > --- a/hw/i2c/aspeed_i2c.c
> > +++ b/hw/i2c/aspeed_i2c.c
> > @@ -114,7 +114,10 @@ static uint64_t
> aspeed_i2c_bus_old_read(AspeedI2CBus *bus, hwaddr offset,
> > if (!aic->has_dma) {
> > qemu_log_mask(LOG_GUEST_ERROR, "%s: No DMA
> support\n", __func__);
> > value = -1;
> > + break;
> > }
> > +
> > + value = extract64(bus->dma_dram_offset, 0, 32);
> > break;
> > case A_I2CD_DMA_LEN:
> > if (!aic->has_dma) {
> > @@ -150,9 +153,7 @@ static uint64_t
> aspeed_i2c_bus_new_read(AspeedI2CBus *bus, hwaddr offset,
> > case A_I2CM_DMA_TX_ADDR:
> > case A_I2CM_DMA_RX_ADDR:
> > case A_I2CM_DMA_LEN_STS:
> > - case A_I2CC_DMA_ADDR:
> > case A_I2CC_DMA_LEN:
> > -
> > case A_I2CS_DEV_ADDR:
> > case A_I2CS_DMA_RX_ADDR:
> > case A_I2CS_DMA_LEN:
> > @@ -161,6 +162,9 @@ static uint64_t
> aspeed_i2c_bus_new_read(AspeedI2CBus *bus, hwaddr offset,
> > case A_I2CS_DMA_LEN_STS:
> > /* Value is already set, don't do anything. */
> > break;
> > + case A_I2CC_DMA_ADDR:
> > + value = extract64(bus->dma_dram_offset, 0, 32);
> > + break;
> > case A_I2CS_INTR_STS:
> > break;
> > case A_I2CM_CMD:
> > @@ -210,18 +214,18 @@ static int aspeed_i2c_dma_read(AspeedI2CBus
> *bus, uint8_t *data)
> > {
> > MemTxResult result;
> > AspeedI2CState *s = bus->controller;
> > - uint32_t reg_dma_addr = aspeed_i2c_bus_dma_addr_offset(bus);
> > uint32_t reg_dma_len = aspeed_i2c_bus_dma_len_offset(bus);
> >
> > - result = address_space_read(&s->dram_as, bus->regs[reg_dma_addr],
> > + result = address_space_read(&s->dram_as, bus->dma_dram_offset,
> > MEMTXATTRS_UNSPECIFIED, data,
> 1);
> > if (result != MEMTX_OK) {
> > - qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM read failed
> @%08x\n",
> > - __func__, bus->regs[reg_dma_addr]);
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: DRAM read failed @%" PRIx64 "\n",
> > + __func__, bus->dma_dram_offset);
> > return -1;
> > }
> >
> > - bus->regs[reg_dma_addr]++;
> > + bus->dma_dram_offset++;
> > bus->regs[reg_dma_len]--;
> > return 0;
> > }
> > @@ -291,7 +295,6 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus *bus)
> > uint32_t reg_pool_ctrl = aspeed_i2c_bus_pool_ctrl_offset(bus);
> > uint32_t reg_byte_buf = aspeed_i2c_bus_byte_buf_offset(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_SIZE) + 1;
> >
> > @@ -323,14 +326,17 @@ static void aspeed_i2c_bus_recv(AspeedI2CBus
> *bus)
> > data = i2c_recv(bus->bus);
> > trace_aspeed_i2c_bus_recv("DMA",
> bus->regs[reg_dma_len],
> > bus->regs[reg_dma_len],
> data);
> > - result = address_space_write(&s->dram_as,
> bus->regs[reg_dma_addr],
> > +
> > + result = address_space_write(&s->dram_as,
> > + bus->dma_dram_offset,
> >
> MEMTXATTRS_UNSPECIFIED, &data, 1);
> > if (result != MEMTX_OK) {
> > - qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM write
> failed @%08x\n",
> > - __func__, bus->regs[reg_dma_addr]);
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "%s: DRAM write failed @%" PRIx64
> "\n",
> > + __func__, bus->dma_dram_offset);
> > return;
> > }
> > - bus->regs[reg_dma_addr]++;
> > +
> > + bus->dma_dram_offset++;
> > bus->regs[reg_dma_len]--;
> > /* In new mode, keep track of how many bytes we RXed */
> > if (aspeed_i2c_is_new_mode(bus->controller)) { @@
> > -636,14 +642,18 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus
> *bus, hwaddr offset,
> > case A_I2CM_DMA_TX_ADDR:
> > bus->regs[R_I2CM_DMA_TX_ADDR] = FIELD_EX32(value,
> I2CM_DMA_TX_ADDR,
> > ADDR);
> > - bus->regs[R_I2CC_DMA_ADDR] = FIELD_EX32(value,
> I2CM_DMA_TX_ADDR, ADDR);
> > + bus->dma_dram_offset =
> > + deposit64(bus->dma_dram_offset, 0, 32,
> > + FIELD_EX32(value, I2CM_DMA_TX_ADDR,
> ADDR));
> > bus->regs[R_I2CC_DMA_LEN] = ARRAY_FIELD_EX32(bus->regs,
> I2CM_DMA_LEN,
> >
> TX_BUF_LEN) + 1;
> > break;
> > case A_I2CM_DMA_RX_ADDR:
> > bus->regs[R_I2CM_DMA_RX_ADDR] = FIELD_EX32(value,
> I2CM_DMA_RX_ADDR,
> > ADDR);
> > - bus->regs[R_I2CC_DMA_ADDR] = FIELD_EX32(value,
> I2CM_DMA_RX_ADDR, ADDR);
> > + bus->dma_dram_offset =
> > + deposit64(bus->dma_dram_offset, 0, 32,
> > + FIELD_EX32(value, I2CM_DMA_RX_ADDR,
> ADDR));
> > bus->regs[R_I2CC_DMA_LEN] = ARRAY_FIELD_EX32(bus->regs,
> I2CM_DMA_LEN,
> >
> RX_BUF_LEN) + 1;
> > break;
> > @@ -811,7 +821,8 @@ static void aspeed_i2c_bus_old_write(AspeedI2CBus
> *bus, hwaddr offset,
> > break;
> > }
> >
> > - bus->regs[R_I2CD_DMA_ADDR] = value & 0x3ffffffc;
> > + bus->dma_dram_offset = deposit64(bus->dma_dram_offset, 0,
> 32,
> > + value & 0x3ffffffc);
> > break;
> >
> > case A_I2CD_DMA_LEN:
> > @@ -983,6 +994,7 @@ static const VMStateDescription
> aspeed_i2c_bus_vmstate = {
> > .fields = (const VMStateField[]) {
> > VMSTATE_UINT32_ARRAY(regs, AspeedI2CBus,
> ASPEED_I2C_NEW_NUM_REG),
> > VMSTATE_UINT8_ARRAY(pool, AspeedI2CBus,
> > ASPEED_I2C_BUS_POOL_SIZE),
> > + VMSTATE_UINT64(dma_dram_offset, AspeedI2CBus),
> > VMSTATE_END_OF_LIST()
> > }
> > };
> > @@ -1188,8 +1200,9 @@ static int
> aspeed_i2c_bus_new_slave_event(AspeedI2CBus *bus,
> > return -1;
> > }
> > ARRAY_FIELD_DP32(bus->regs, I2CS_DMA_LEN_STS, RX_LEN,
> 0);
> > - bus->regs[R_I2CC_DMA_ADDR] =
> > - ARRAY_FIELD_EX32(bus->regs, I2CS_DMA_RX_ADDR, ADDR);
> > + bus->dma_dram_offset =
> > + deposit64(bus->dma_dram_offset, 0, 32,
> > + ARRAY_FIELD_EX32(bus->regs,
> I2CS_DMA_RX_ADDR,
> > + ADDR));
> > bus->regs[R_I2CC_DMA_LEN] =
> > ARRAY_FIELD_EX32(bus->regs, I2CS_DMA_LEN,
> RX_BUF_LEN) + 1;
> > i2c_ack(bus->bus);
> > @@ -1255,10 +1268,10 @@ static int aspeed_i2c_bus_slave_event(I2CSlave
> *slave, enum i2c_event event)
> > static void aspeed_i2c_bus_new_slave_send_async(AspeedI2CBus *bus,
> uint8_t data)
> > {
> > assert(address_space_write(&bus->controller->dram_as,
> > - bus->regs[R_I2CC_DMA_ADDR],
> > + bus->dma_dram_offset,
> > MEMTXATTRS_UNSPECIFIED, &data,
> 1) ==
> > MEMTX_OK);
> >
> > - bus->regs[R_I2CC_DMA_ADDR]++;
> > + bus->dma_dram_offset++;
> > bus->regs[R_I2CC_DMA_LEN]--;
> > ARRAY_FIELD_DP32(bus->regs, I2CS_DMA_LEN_STS, RX_LEN,
> > ARRAY_FIELD_EX32(bus->regs,
> I2CS_DMA_LEN_STS,
> > RX_LEN) + 1); diff --git a/include/hw/i2c/aspeed_i2c.h
> > b/include/hw/i2c/aspeed_i2c.h index b42c4dc584..bdaea3207d 100644
> > --- a/include/hw/i2c/aspeed_i2c.h
> > +++ b/include/hw/i2c/aspeed_i2c.h
> > @@ -248,6 +248,7 @@ struct AspeedI2CBus {
> >
> > uint32_t regs[ASPEED_I2C_NEW_NUM_REG];
> > uint8_t pool[ASPEED_I2C_BUS_POOL_SIZE];
> > + uint64_t dma_dram_offset;
> > };
> >
> > struct AspeedI2CState {
> > @@ -369,14 +370,6 @@ static inline uint32_t
> aspeed_i2c_bus_dma_len_offset(AspeedI2CBus *bus)
> > return R_I2CD_DMA_LEN;
> > }
> >
> > -static inline uint32_t aspeed_i2c_bus_dma_addr_offset(AspeedI2CBus
> > *bus) -{
> > - if (aspeed_i2c_is_new_mode(bus->controller)) {
> > - return R_I2CC_DMA_ADDR;
> > - }
> > - return R_I2CD_DMA_ADDR;
> > -}
> > -
> > static inline bool aspeed_i2c_bus_is_master(AspeedI2CBus *bus)
> > {
> > return SHARED_ARRAY_FIELD_EX32(bus->regs,
> > aspeed_i2c_bus_ctrl_offset(bus),
