Hi Cédric > Subject: Re: [PATCH v1 01/10] hw/i2c/aspeed_i2c: Introduce > dma_addr_lo_mask to unify DMA address handling > > On 5/29/26 08:42, Jamin Lin wrote: > > The Aspeed I2C controller has two register layouts. > > > > The AST2500 uses the old mode with a single DMA address register > > (I2CD_DMA_ADDR) where the address is 4-byte aligned and masked to > 0x3ffffffc. > > > > From AST2600 onwards, the new mode provides separate master TX/RX > and > > slave RX DMA address registers (I2CM_DMA_TX_ADDR, > I2CM_DMA_RX_ADDR, > > I2CS_DMA_RX_ADDR) with different address widths per SoC: > > AST2600 (new mode): 0x7fffffff - bits[30:0] > > AST1030 (new mode): 0x7fffffff - bits[30:0] > > AST1060 (new mode): 0x7fffffff - bits[30:0] > > AST2700 (new mode): 0xffffffff - bits[31:0] > > > > Introduce dma_addr_lo_mask as a per-class attribute and apply it > > uniformly when storing DMA address register writes and when loading > > the address into dma_dram_offset for both master and slave paths. > > This replaces the previous FIELD_EX32 extractions (which incorrectly > > stripped bit 31 on AST2700) and the hardcoded 0x3ffffffc literal in > > the old-mode path. > > > Should we add a Fixes: tag ? > Will add the following Fixes-tag https://github.com/qemu/qemu/commit/1809ab6a67359e0876981cd05d2a50b2843eabad hw/i2c/aspeed: Add AST2700 support
Thanks, Jamin > Thanks, > > C. > > > > > > Signed-off-by: Jamin Lin <[email protected]> > > --- > > include/hw/i2c/aspeed_i2c.h | 5 +---- > > hw/i2c/aspeed_i2c.c | 22 +++++++++++++--------- > > 2 files changed, 14 insertions(+), 13 deletions(-) > > > > diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h > > index d42cb4865a..1fc229f699 100644 > > --- a/include/hw/i2c/aspeed_i2c.h > > +++ b/include/hw/i2c/aspeed_i2c.h > > @@ -209,13 +209,9 @@ REG32(I2CS_DMA_LEN, 0x2c) > > FIELD(I2CS_DMA_LEN, TX_BUF_LEN_W1T, 15, 1) > > FIELD(I2CS_DMA_LEN, TX_BUF_LEN, 0, 11) > > REG32(I2CM_DMA_TX_ADDR, 0x30) > > - FIELD(I2CM_DMA_TX_ADDR, ADDR, 0, 31) > > REG32(I2CM_DMA_RX_ADDR, 0x34) > > - FIELD(I2CM_DMA_RX_ADDR, ADDR, 0, 31) > > REG32(I2CS_DMA_TX_ADDR, 0x38) > > - FIELD(I2CS_DMA_TX_ADDR, ADDR, 0, 31) > > REG32(I2CS_DMA_RX_ADDR, 0x3c) > > - FIELD(I2CS_DMA_RX_ADDR, ADDR, 0, 31) > > REG32(I2CS_DEV_ADDR, 0x40) > > REG32(I2CM_DMA_LEN_STS, 0x48) > > FIELD(I2CM_DMA_LEN_STS, RX_LEN, 16, 13) @@ -303,6 +299,7 @@ > > struct AspeedI2CClass { > > bool has_share_pool; > > uint64_t mem_size; > > bool has_dma64; > > + uint32_t dma_addr_lo_mask; > > }; > > > > static inline bool aspeed_i2c_is_new_mode(AspeedI2CState *s) diff > > --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c index > > 80c4457500..9c0387a394 100644 > > --- a/hw/i2c/aspeed_i2c.c > > +++ b/hw/i2c/aspeed_i2c.c > > @@ -236,7 +236,7 @@ static void > aspeed_i2c_set_tx_dma_dram_offset(AspeedI2CBus *bus) > > value = bus->regs[R_I2CM_DMA_TX_ADDR]; > > bus->dma_dram_offset = > > deposit64(bus->dma_dram_offset, 0, 32, > > - FIELD_EX32(value, I2CM_DMA_TX_ADDR, > ADDR)); > > + value & aic->dma_addr_lo_mask); > > if (aic->has_dma64) { > > value = bus->regs[R_I2CM_DMA_TX_ADDR_HI]; > > bus->dma_dram_offset = > > @@ -246,7 +246,7 @@ static void > aspeed_i2c_set_tx_dma_dram_offset(AspeedI2CBus *bus) > > } else { > > value = bus->regs[R_I2CD_DMA_ADDR]; > > bus->dma_dram_offset = deposit64(bus->dma_dram_offset, 0, > 32, > > - value & 0x3ffffffc); > > + value & > > + aic->dma_addr_lo_mask); > > } > > } > > > > @@ -261,7 +261,7 @@ static void > aspeed_i2c_set_rx_dma_dram_offset(AspeedI2CBus *bus) > > value = bus->regs[R_I2CM_DMA_RX_ADDR]; > > bus->dma_dram_offset = > > deposit64(bus->dma_dram_offset, 0, 32, > > - FIELD_EX32(value, I2CM_DMA_RX_ADDR, > ADDR)); > > + value & aic->dma_addr_lo_mask); > > if (aic->has_dma64) { > > value = bus->regs[R_I2CM_DMA_RX_ADDR_HI]; > > bus->dma_dram_offset = > > @@ -271,7 +271,7 @@ static void > aspeed_i2c_set_rx_dma_dram_offset(AspeedI2CBus *bus) > > } else { > > value = bus->regs[R_I2CD_DMA_ADDR]; > > bus->dma_dram_offset = deposit64(bus->dma_dram_offset, 0, > 32, > > - value & 0x3ffffffc); > > + value & > > + aic->dma_addr_lo_mask); > > } > > } > > > > @@ -735,12 +735,10 @@ static void > aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset, > > aspeed_i2c_bus_raise_interrupt(bus); > > break; > > case A_I2CM_DMA_TX_ADDR: > > - bus->regs[R_I2CM_DMA_TX_ADDR] = FIELD_EX32(value, > I2CM_DMA_TX_ADDR, > > - ADDR); > > + bus->regs[R_I2CM_DMA_TX_ADDR] = value & > > + aic->dma_addr_lo_mask; > > 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_I2CM_DMA_RX_ADDR] = value & > > + aic->dma_addr_lo_mask; > > break; > > case A_I2CM_DMA_LEN: > > w1t = FIELD_EX32(value, I2CM_DMA_LEN, RX_BUF_LEN_W1T) > || @@ > > -1385,6 +1383,8 @@ static const TypeInfo aspeed_i2c_info = { > > static int aspeed_i2c_bus_new_slave_event(AspeedI2CBus *bus, > > enum i2c_event > event) > > { > > + AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller); > > + > > switch (event) { > > case I2C_START_SEND_ASYNC: > > if (!SHARED_ARRAY_FIELD_EX32(bus->regs, R_I2CS_CMD, > > RX_DMA_EN)) { @@ -1395,7 +1395,7 @@ static int > aspeed_i2c_bus_new_slave_event(AspeedI2CBus *bus, > > ARRAY_FIELD_DP32(bus->regs, I2CS_DMA_LEN_STS, RX_LEN, > 0); > > bus->dma_dram_offset = > > deposit64(bus->dma_dram_offset, 0, 32, > > - ARRAY_FIELD_EX32(bus->regs, > I2CS_DMA_RX_ADDR, ADDR)); > > + bus->regs[R_I2CS_DMA_RX_ADDR] & > > + aic->dma_addr_lo_mask); > > bus->regs[R_I2CC_DMA_LEN] = > > ARRAY_FIELD_EX32(bus->regs, I2CS_DMA_LEN, > RX_BUF_LEN) + 1; > > i2c_ack(bus->bus); > > @@ -1638,6 +1638,7 @@ static void > aspeed_2500_i2c_class_init(ObjectClass *klass, const void *data) > > aic->check_sram = true; > > aic->has_dma = true; > > aic->mem_size = 0x1000; > > + aic->dma_addr_lo_mask = 0x3ffffffc; > > } > > > > static const TypeInfo aspeed_2500_i2c_info = { @@ -1667,6 +1668,7 @@ > > static void aspeed_2600_i2c_class_init(ObjectClass *klass, const void *data) > > aic->bus_pool_base = aspeed_2500_i2c_bus_pool_base; > > aic->has_dma = true; > > aic->mem_size = 0x1000; > > + aic->dma_addr_lo_mask = 0x7fffffff; > > } > > > > static const TypeInfo aspeed_2600_i2c_info = { @@ -1691,6 +1693,7 @@ > > static void aspeed_1030_i2c_class_init(ObjectClass *klass, const void *data) > > aic->bus_pool_base = aspeed_2500_i2c_bus_pool_base; > > aic->has_dma = true; > > aic->mem_size = 0x10000; > > + aic->dma_addr_lo_mask = 0x7fffffff; > > } > > > > static const TypeInfo aspeed_1030_i2c_info = { @@ -1718,6 +1721,7 @@ > > static void aspeed_2700_i2c_class_init(ObjectClass *klass, const void *data) > > aic->has_dma = true; > > aic->mem_size = 0x2000; > > aic->has_dma64 = true; > > + aic->dma_addr_lo_mask = 0xffffffff; > > } > > > > static const TypeInfo aspeed_2700_i2c_info = {
