Hi Cedric, > > Hello Jamin, > > On 4/16/24 11:18, Jamin Lin wrote: > > AST2700 support the maximum dram size is 8GiB and has a "DMA DRAM > Side > > Address High Part(0x7C)" > > register to support 64 bits dma dram address. > > Add helper routines functions to compute the dma dram address, new > > features and update trace-event to support 64 bits dram address. > > > > Signed-off-by: Troy Lee <troy_...@aspeedtech.com> > > Signed-off-by: Jamin Lin <jamin_...@aspeedtech.com> > > --- > > hw/ssi/aspeed_smc.c | 66 > +++++++++++++++++++++++++++++++++++++++------ > > hw/ssi/trace-events | 2 +- > > 2 files changed, 59 insertions(+), 9 deletions(-) > > > > diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c index > > 71abc7a2d8..a67cac3d0f 100644 > > --- a/hw/ssi/aspeed_smc.c > > +++ b/hw/ssi/aspeed_smc.c > > @@ -132,6 +132,9 @@ > > #define FMC_WDT2_CTRL_BOOT_SOURCE BIT(4) /* O: primary > 1: alternate */ > > #define FMC_WDT2_CTRL_EN BIT(0) > > > > +/* DMA DRAM Side Address High Part (AST2700) */ > > +#define R_DMA_DRAM_ADDR_HIGH (0x7c / 4) > > + > > /* DMA Control/Status Register */ > > #define R_DMA_CTRL (0x80 / 4) > > #define DMA_CTRL_REQUEST (1 << 31) > > @@ -187,6 +190,7 @@ > > * 0x1FFFFFF: 32M bytes > > */ > > #define DMA_DRAM_ADDR(asc, val) ((val) & (asc)->dma_dram_mask) > > +#define DMA_DRAM_ADDR_HIGH(val) ((val) & 0xf) > > #define DMA_FLASH_ADDR(asc, val) ((val) & (asc)->dma_flash_mask) > > #define DMA_LENGTH(val) ((val) & 0x01FFFFFF) > > > > @@ -207,6 +211,7 @@ static const AspeedSegments > aspeed_2500_spi2_segments[]; > > #define ASPEED_SMC_FEATURE_DMA 0x1 > > #define ASPEED_SMC_FEATURE_DMA_GRANT 0x2 > > #define ASPEED_SMC_FEATURE_WDT_CONTROL 0x4 > > +#define ASPEED_SMC_FEATURE_DMA_DRAM_ADDR_HIGH 0x08 > > > > static inline bool aspeed_smc_has_dma(const AspeedSMCClass *asc) > > { > > @@ -218,6 +223,11 @@ static inline bool > aspeed_smc_has_wdt_control(const AspeedSMCClass *asc) > > return !!(asc->features & ASPEED_SMC_FEATURE_WDT_CONTROL); > > } > > > > +static inline bool aspeed_smc_has_dma_dram_addr_high(const > > +AspeedSMCClass *asc) > > To ease the reading, I would call the helper aspeed_smc_has_dma64() Will fix it > > > +{ > > + return !!(asc->features & > ASPEED_SMC_FEATURE_DMA_DRAM_ADDR_HIGH); > > +} > > + > > #define aspeed_smc_error(fmt, ...) > \ > > qemu_log_mask(LOG_GUEST_ERROR, "%s: " fmt "\n", __func__, ## > > __VA_ARGS__) > > > > @@ -747,6 +757,9 @@ static uint64_t aspeed_smc_read(void *opaque, > hwaddr addr, unsigned int size) > > (aspeed_smc_has_dma(asc) && addr == R_DMA_CTRL) || > > (aspeed_smc_has_dma(asc) && addr == R_DMA_FLASH_ADDR) > || > > (aspeed_smc_has_dma(asc) && addr == R_DMA_DRAM_ADDR) > || > > + (aspeed_smc_has_dma(asc) && > > + aspeed_smc_has_dma_dram_addr_high(asc) && > > + addr == R_DMA_DRAM_ADDR_HIGH) || > > (aspeed_smc_has_dma(asc) && addr == R_DMA_LEN) || > > (aspeed_smc_has_dma(asc) && addr == R_DMA_CHECKSUM) > || > > (addr >= R_SEG_ADDR0 && > > @@ -847,6 +860,23 @@ static bool > aspeed_smc_inject_read_failure(AspeedSMCState *s) > > } > > } > > > > +static uint64_t aspeed_smc_dma_dram_addr(AspeedSMCState *s) { > > + AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s); > > + uint64_t dram_addr_high; > > + uint64_t dma_dram_addr; > > + > > + if (aspeed_smc_has_dma_dram_addr_high(asc)) { > > + dram_addr_high = s->regs[R_DMA_DRAM_ADDR_HIGH]; > > + dram_addr_high <<= 32; > > + dma_dram_addr = dram_addr_high | > s->regs[R_DMA_DRAM_ADDR]; > > Here is a proposal to shorten the routine : > > return ((uint64_t) s->regs[R_DMA_DRAM_ADDR_HIGH] << 32) | > s->regs[R_DMA_DRAM_ADDR]; > > > > + } else { > > + dma_dram_addr = s->regs[R_DMA_DRAM_ADDR]; > > and > return s->regs[R_DMA_DRAM_ADDR]; > > > + } > > + > > + return dma_dram_addr; > > +} > > + Thanks for your suggestion. Will fix. > > static uint32_t aspeed_smc_dma_len(AspeedSMCState *s) > > { > > AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s); @@ -914,24 > > +944,34 @@ static void aspeed_smc_dma_checksum(AspeedSMCState *s) > > > > static void aspeed_smc_dma_rw(AspeedSMCState *s) > > { > > + AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s); > > + uint64_t dram_addr_high; > > This variable doesn't look very useful Will try to remove it. > > > + uint64_t dma_dram_addr; > > + uint64_t dram_addr; > > and dram_addr is redundant with dma_dram_addr. Please use only one. Please see my below description and please give us any suggestion. > > > > MemTxResult result; > > uint32_t dma_len; > > uint32_t data; > > > > dma_len = aspeed_smc_dma_len(s); > > + dma_dram_addr = aspeed_smc_dma_dram_addr(s); > > + > > + if (aspeed_smc_has_dma_dram_addr_high(asc)) { > > + dram_addr = dma_dram_addr - s->dram_mr->container->addr; > > Why do you truncate the address again ? It should already be done with > > #define DMA_DRAM_ADDR_HIGH(val) ((val) & 0xf) > The reason is that our firmware set the real address in SMC registers. For example: If users want to move data from flash to the DRAM at address 0, It set R_DMA_DRAM_ADDR_HIGH 4 and R_DMA_DRAM_ADDR 0 because the dram base address is 0x 4 00000000 for AST2700.
According to the design of QEMU, the dram container base address is 0x4 00000000(s->dram_mr). Therefore, in the above example, the data should be moved to the dram container at address 0. I created two variables to save the different base address. A dma_dram_addr is used to save the real address. A dram_addr is used to save the address for dram container(s->dram_as) Example: dma_dram_addr is 0x4 00000000 (uint64) s->regs[R_DMA_DRAM_ADDR_HIGH] << 32) |s->regs[R_DMA_DRAM_ADDR] dram_addr is 0 dram_addr = 0x400000000(real address) - 0x400000000(dram container base address) to get the address for dram contained for moving dram_addr = dma_dram_addr - s->dram_mr->container->addr; Finally, the following APIs could work properly. data = address_space_ldl_le(&s->dram_as, dram_addr, MEMTXATTRS_UNSPECIFIED, &result); Thanks-Jamin > > + } else { > > + dram_addr = dma_dram_addr; > > + } > > > > trace_aspeed_smc_dma_rw(s->regs[R_DMA_CTRL] & > DMA_CTRL_WRITE ? > > "write" : "read", > > s->regs[R_DMA_FLASH_ADDR], > > - s->regs[R_DMA_DRAM_ADDR], > > + dram_addr, > > dma_len); > > while (dma_len) { > > if (s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE) { > > - data = address_space_ldl_le(&s->dram_as, > s->regs[R_DMA_DRAM_ADDR], > > + data = address_space_ldl_le(&s->dram_as, dram_addr, > > > MEMTXATTRS_UNSPECIFIED, &result); > > if (result != MEMTX_OK) { > > - aspeed_smc_error("DRAM read failed @%08x", > > - s->regs[R_DMA_DRAM_ADDR]); > > + aspeed_smc_error("DRAM read failed @%" PRIx64, > > + dram_addr); > > return; > > } > > > > @@ -951,11 +991,10 @@ static void aspeed_smc_dma_rw(AspeedSMCState > *s) > > return; > > } > > > > - address_space_stl_le(&s->dram_as, > s->regs[R_DMA_DRAM_ADDR], > > + address_space_stl_le(&s->dram_as, dram_addr, > > data, > MEMTXATTRS_UNSPECIFIED, &result); > > if (result != MEMTX_OK) { > > - aspeed_smc_error("DRAM write failed @%08x", > > - s->regs[R_DMA_DRAM_ADDR]); > > + aspeed_smc_error("DRAM write failed @%" PRIx64, > > + dram_addr); > > return; > > } > > } > > @@ -964,8 +1003,15 @@ static void aspeed_smc_dma_rw(AspeedSMCState > *s) > > * When the DMA is on-going, the DMA registers are updated > > * with the current working addresses and length. > > */ > > + dram_addr += 4; > > + dma_dram_addr += 4; > > + if (aspeed_smc_has_dma_dram_addr_high(asc)) { > > + dram_addr_high = dma_dram_addr >> 32; > > + s->regs[R_DMA_DRAM_ADDR_HIGH] = dram_addr_high; > > s->regs[R_DMA_DRAM_ADDR_HIGH] = dma_dram_addr >> > 32; > > > + } > > + > > + s->regs[R_DMA_DRAM_ADDR] = dma_dram_addr & 0xffffffff; > > use DMA_DRAM_ADDR() may be instead. > > > s->regs[R_DMA_FLASH_ADDR] += 4; > > - s->regs[R_DMA_DRAM_ADDR] += 4; > > dma_len -= 4; > > s->regs[R_DMA_LEN] = dma_len; > > s->regs[R_DMA_CHECKSUM] += data; @@ -1118,6 +1164,10 > @@ > > static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data, > > } else if (aspeed_smc_has_dma(asc) && addr == R_DMA_LEN && > > aspeed_smc_dma_granted(s)) { > > s->regs[addr] = DMA_LENGTH(value); > > + } else if (aspeed_smc_has_dma(asc) && > > + aspeed_smc_has_dma_dram_addr_high(asc) && > > + addr == R_DMA_DRAM_ADDR_HIGH) { > > + s->regs[addr] = DMA_DRAM_ADDR_HIGH(value); > > } else { > > qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" > HWADDR_PRIx "\n", > > __func__, addr); diff --git > > a/hw/ssi/trace-events b/hw/ssi/trace-events index > > 2d5bd2b83d..7b5ad6a939 100644 > > --- a/hw/ssi/trace-events > > +++ b/hw/ssi/trace-events > > @@ -6,7 +6,7 @@ aspeed_smc_do_snoop(int cs, int index, int dummies, int > data) "CS%d index:0x%x d > > aspeed_smc_flash_write(int cs, uint64_t addr, uint32_t size, uint64_t > data, int mode) "CS%d @0x%" PRIx64 " size %u: 0x%" PRIx64" mode:%d" > > aspeed_smc_read(uint64_t addr, uint32_t size, uint64_t data) "@0x%" > PRIx64 " size %u: 0x%" PRIx64 > > aspeed_smc_dma_checksum(uint32_t addr, uint32_t data) "0x%08x: > 0x%08x" > > -aspeed_smc_dma_rw(const char *dir, uint32_t flash_addr, uint32_t > dram_addr, uint32_t size) "%s flash:@0x%08x dram:@0x%08x size:0x%08x" > > +aspeed_smc_dma_rw(const char *dir, uint32_t flash_addr, uint64_t > dram_addr, uint32_t size) "%s flash:@0x%08x dram:@0x%" PRIx64 " > size:0x%08x" > > aspeed_smc_write(uint64_t addr, uint32_t size, uint64_t data) "@0x%" > PRIx64 " size %u: 0x%" PRIx64 > > aspeed_smc_flash_select(int cs, const char *prefix) "CS%d %sselect" > >