RE: [PATCH v1 08/15] hw/i2c/aspeed: introduce a new dma_dram_offset attribute in AspeedI2Cbus

[Jamin Lin]

Hi Cedric,

> Subject: Re: [PATCH v1 08/15] hw/i2c/aspeed: introduce a new
> dma_dram_offset attribute in AspeedI2Cbus
>
> On 7/18/24 08: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.
> >
> > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > ---
> >   hw/i2c/aspeed_i2c.c         | 50
> +++++++++++++++++++++++--------------
> >   include/hw/i2c/aspeed_i2c.h |  9 +------
> >   2 files changed, 32 insertions(+), 27 deletions(-)
> >
> > diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c index
> > abcb1d5330..c0d3ac3867 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:
> > @@ -1188,8 +1199,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 +1267,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;
>
>
> aspeed_i2c_bus_vmstate needs an update. No need to increment again the
> vmstate version if all patches are merged in the same release cycle.
>

Thanks for review.
Will update
Jamin


> Thanks,
>
> C.
>
> >   };
> >
> >   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),

************* Email Confidentiality Notice ********************
免責聲明:
本信件(或其附件)可能包含機密資訊，並受法律保護。如 台端非指定之收件者，請以電子郵件通知本電子郵件之發送者, 
並請立即刪除本電子郵件及其附件和銷毀所有複印件。謝謝您的合作!

DISCLAIMER:
This message (and any attachments) may contain legally privileged and/or other 
confidential information. If you have received it in error, please notify the 
sender by reply e-mail and immediately delete the e-mail and any attachments 
without copying or disclosing the contents. Thank you.