Re: [SPAM] Re: [PATCH v4 08/16] aspeed/smc: support 64 bits dma dram address
On 5/27/24 18:06, Philippe Mathieu-Daudé wrote: Hi Jamin, On 27/5/24 10:02, 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 Signed-off-by: Jamin Lin --- hw/ssi/aspeed_smc.c | 52 +++-- hw/ssi/trace-events | 2 +- include/hw/ssi/aspeed_smc.h | 1 + 3 files changed, 46 insertions(+), 9 deletions(-) +static uint64_t aspeed_smc_dma_dram_addr(AspeedSMCState *s) +{ + return s->regs[R_DMA_DRAM_ADDR] | + ((uint64_t) s->regs[R_DMA_DRAM_ADDR_HIGH] << 32); +} + static uint32_t aspeed_smc_dma_len(AspeedSMCState *s) { AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s); @@ -903,24 +921,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 dma_dram_offset; + uint64_t dma_dram_addr; 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_dma64(asc)) { + dma_dram_offset = dma_dram_addr - s->dram_base; + } else { + dma_dram_offset = dma_dram_addr; Here s->dram_base is 0x0. Do we really need to check aspeed_smc_has_dma64? You are right, it could be done as your proposal below. However, we should add a comment regarding some values : R_DMA_DRAM_ADDR_HIGH and s->dram_base are only set on the AST2700 SoC and zero on other Aspeed SoCs. + } Maybe simplify improving aspeed_smc_dma_dram_addr() as: static uint64_t aspeed_smc_dma_dram_addr(AspeedSMCState *s) { return (s->regs[R_DMA_DRAM_ADDR] | ((uint64_t) s->regs[R_DMA_DRAM_ADDR_HIGH] << 32)) - s->dram_base; } Then no need for dma_dram_offset, dma_dram_addr is enough. we need both, dma_dram_offset for the transaction and dma_dram_addr to update the R_DMA_DRAM_ADDR_HIGH reg. A bit cumbersome, I agree. Thanks, C. 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], + dma_dram_offset, dma_len); while (dma_len) { if (s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE) { - data = address_space_ldl_le(>dram_as, s->regs[R_DMA_DRAM_ADDR], + data = address_space_ldl_le(>dram_as, dma_dram_offset, MEMTXATTRS_UNSPECIFIED, ); if (result != MEMTX_OK) { - aspeed_smc_error("DRAM read failed @%08x", - s->regs[R_DMA_DRAM_ADDR]); + aspeed_smc_error("DRAM read failed @%" PRIx64, + dma_dram_offset); return; } @@ -940,11 +968,11 @@ static void aspeed_smc_dma_rw(AspeedSMCState *s) return; } - address_space_stl_le(>dram_as, s->regs[R_DMA_DRAM_ADDR], + address_space_stl_le(>dram_as, dma_dram_offset, data, MEMTXATTRS_UNSPECIFIED, ); if (result != MEMTX_OK) { - aspeed_smc_error("DRAM write failed @%08x", - s->regs[R_DMA_DRAM_ADDR]); + aspeed_smc_error("DRAM write failed @%" PRIx64, + dma_dram_offset); return; } } @@ -953,8 +981,12 @@ 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. */ + dma_dram_offset += 4; + dma_dram_addr += 4; + + s->regs[R_DMA_DRAM_ADDR_HIGH] = dma_dram_addr >> 32; + s->regs[R_DMA_DRAM_ADDR] = dma_dram_addr & 0x; 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; @@ -1107,6 +1139,9 @@ 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_dma64(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); @@ -1239,6 +1274,7 @@
Re: [PATCH v4 08/16] aspeed/smc: support 64 bits dma dram address
On 5/28/24 03:34, Jamin Lin wrote: Hi Cedric, On 5/27/24 10:02, 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 Signed-off-by: Jamin Lin I will move the addition of the "dram-base" property to another patch. See : https://patchew.org/QEMU/20240527124315.35356-1-...@redhat.com/ (Please review) Review done. If I need to resend v5 patch series, I will remove "dram-base property" from this patch. ok. Wait a bit before resending though. We are not done with v4 yet ! Thanks, C. Thanks for your help, Jamin Else, Reviewed-by: Cédric Le Goater Thanks, C. --- hw/ssi/aspeed_smc.c | 52 +++-- hw/ssi/trace-events | 2 +- include/hw/ssi/aspeed_smc.h | 1 + 3 files changed, 46 insertions(+), 9 deletions(-) diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c index ffb13a12e8..df0c63469c 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 @@ * 0x1FF: 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) & 0x01FF) @@ -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_dma64(const AspeedSMCClass *asc) { +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,8 @@ 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_dma64(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 +859,12 @@ static bool aspeed_smc_inject_read_failure(AspeedSMCState *s) } } +static uint64_t aspeed_smc_dma_dram_addr(AspeedSMCState *s) { +return s->regs[R_DMA_DRAM_ADDR] | +((uint64_t) s->regs[R_DMA_DRAM_ADDR_HIGH] << 32); } + static uint32_t aspeed_smc_dma_len(AspeedSMCState *s) { AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s); @@ -903,24 +921,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 dma_dram_offset; +uint64_t dma_dram_addr; 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_dma64(asc)) { +dma_dram_offset = dma_dram_addr - s->dram_base; +} else { +dma_dram_offset = 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], +dma_dram_offset, dma_len); while (dma_len) { if (s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE) { -data = address_space_ldl_le(>dram_as, s->regs[R_DMA_DRAM_ADDR], +data = address_space_ldl_le(>dram_as, dma_dram_offset, MEMTXATTRS_UNSPECIFIED, ); if (result != MEMTX_OK) { -aspeed_smc_error("DRAM read failed @%08x", - s->regs[R_DMA_DRAM_ADDR]); +aspeed_smc_error("DRAM read failed @%" PRIx64, +
RE: [PATCH v4 08/16] aspeed/smc: support 64 bits dma dram address
Hi Philippe, > Hi Jamin, > > On 27/5/24 10:02, 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 > > Signed-off-by: Jamin Lin > > --- > > hw/ssi/aspeed_smc.c | 52 > +++-- > > hw/ssi/trace-events | 2 +- > > include/hw/ssi/aspeed_smc.h | 1 + > > 3 files changed, 46 insertions(+), 9 deletions(-) > > > > +static uint64_t aspeed_smc_dma_dram_addr(AspeedSMCState *s) { > > +return s->regs[R_DMA_DRAM_ADDR] | > > +((uint64_t) s->regs[R_DMA_DRAM_ADDR_HIGH] << 32); } > > + > > static uint32_t aspeed_smc_dma_len(AspeedSMCState *s) > > { > > AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s); @@ -903,24 > > +921,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 dma_dram_offset; > > +uint64_t dma_dram_addr; > > 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_dma64(asc)) { > > +dma_dram_offset = dma_dram_addr - s->dram_base; > > +} else { > > +dma_dram_offset = dma_dram_addr; > > Here s->dram_base is 0x0. Do we really need to check > aspeed_smc_has_dma64? > Yes, it is required to check aspeed_smc_has_dma64 to support dram 64bit address. s->dram_base has been changed to "0x4 ". Thanks-Jamin > > +} > > Maybe simplify improving aspeed_smc_dma_dram_addr() as: > >static uint64_t aspeed_smc_dma_dram_addr(AspeedSMCState *s) >{ >return (s->regs[R_DMA_DRAM_ADDR] >| ((uint64_t) s->regs[R_DMA_DRAM_ADDR_HIGH] << 32)) >- s->dram_base; >} > > Then no need for dma_dram_offset, dma_dram_addr is enough. > > > > > 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], > > +dma_dram_offset, > > dma_len); > > while (dma_len) { > > if (s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE) { > > -data = address_space_ldl_le(>dram_as, > s->regs[R_DMA_DRAM_ADDR], > > +data = address_space_ldl_le(>dram_as, > dma_dram_offset, > > > MEMTXATTRS_UNSPECIFIED, ); > > if (result != MEMTX_OK) { > > -aspeed_smc_error("DRAM read failed @%08x", > > - s->regs[R_DMA_DRAM_ADDR]); > > +aspeed_smc_error("DRAM read failed @%" PRIx64, > > + dma_dram_offset); > > return; > > } > > > > @@ -940,11 +968,11 @@ static void aspeed_smc_dma_rw(AspeedSMCState > *s) > > return; > > } > > > > -address_space_stl_le(>dram_as, > s->regs[R_DMA_DRAM_ADDR], > > +address_space_stl_le(>dram_as, dma_dram_offset, > >data, > MEMTXATTRS_UNSPECIFIED, ); > > if (result != MEMTX_OK) { > > -aspeed_smc_error("DRAM write failed @%08x", > > - s->regs[R_DMA_DRAM_ADDR]); > > +aspeed_smc_error("DRAM write failed @%" PRIx64, > > + dma_dram_offset); > > return; > > } > > } > > @@ -953,8 +981,12 @@ 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. > >*/ > > +dma_dram_offset += 4; > > +dma_dram_addr += 4; > > + > > +s->regs[R_DMA_DRAM_ADDR_HIGH] = dma_dram_addr >> 32; > > +s->regs[R_DMA_DRAM_ADDR] = dma_dram_addr & 0x; > > 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; @@ -1107,6 +1139,9 > @@ > > 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_dma64(asc) > && > > + addr == R_DMA_DRAM_ADDR_HIGH) { > > +s->regs[addr] = DMA_DRAM_ADDR_HIGH(value); > > } else { > >
RE: [PATCH v4 08/16] aspeed/smc: support 64 bits dma dram address
Hi Cedric, > On 5/27/24 10:02, 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 > > Signed-off-by: Jamin Lin > > I will move the addition of the "dram-base" property to another patch. See : > >https://patchew.org/QEMU/20240527124315.35356-1-...@redhat.com/ > > (Please review) Review done. If I need to resend v5 patch series, I will remove "dram-base property" from this patch. Thanks for your help, Jamin > > Else, > > Reviewed-by: Cédric Le Goater > > Thanks, > > C. > > > > --- > > hw/ssi/aspeed_smc.c | 52 > +++-- > > hw/ssi/trace-events | 2 +- > > include/hw/ssi/aspeed_smc.h | 1 + > > 3 files changed, 46 insertions(+), 9 deletions(-) > > > > diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c index > > ffb13a12e8..df0c63469c 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 @@ > >* 0x1FF: 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) & 0x01FF) > > > > @@ -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_dma64(const AspeedSMCClass *asc) { > > +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,8 @@ 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_dma64(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 +859,12 @@ static bool > aspeed_smc_inject_read_failure(AspeedSMCState *s) > > } > > } > > > > +static uint64_t aspeed_smc_dma_dram_addr(AspeedSMCState *s) { > > +return s->regs[R_DMA_DRAM_ADDR] | > > +((uint64_t) s->regs[R_DMA_DRAM_ADDR_HIGH] << 32); } > > + > > static uint32_t aspeed_smc_dma_len(AspeedSMCState *s) > > { > > AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s); @@ -903,24 > > +921,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 dma_dram_offset; > > +uint64_t dma_dram_addr; > > 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_dma64(asc)) { > > +dma_dram_offset = dma_dram_addr - s->dram_base; > > +} else { > > +dma_dram_offset = 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], > > +dma_dram_offset, > > dma_len); > > while (dma_len) { > > if (s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE) { > > -data = address_space_ldl_le(>dram_as, > s->regs[R_DMA_DRAM_ADDR], > > +data = address_space_ldl_le(>dram_as, >
Re: [PATCH v4 08/16] aspeed/smc: support 64 bits dma dram address
Hi Jamin, On 27/5/24 10:02, 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 Signed-off-by: Jamin Lin --- hw/ssi/aspeed_smc.c | 52 +++-- hw/ssi/trace-events | 2 +- include/hw/ssi/aspeed_smc.h | 1 + 3 files changed, 46 insertions(+), 9 deletions(-) +static uint64_t aspeed_smc_dma_dram_addr(AspeedSMCState *s) +{ +return s->regs[R_DMA_DRAM_ADDR] | +((uint64_t) s->regs[R_DMA_DRAM_ADDR_HIGH] << 32); +} + static uint32_t aspeed_smc_dma_len(AspeedSMCState *s) { AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s); @@ -903,24 +921,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 dma_dram_offset; +uint64_t dma_dram_addr; 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_dma64(asc)) { +dma_dram_offset = dma_dram_addr - s->dram_base; +} else { +dma_dram_offset = dma_dram_addr; Here s->dram_base is 0x0. Do we really need to check aspeed_smc_has_dma64? +} Maybe simplify improving aspeed_smc_dma_dram_addr() as: static uint64_t aspeed_smc_dma_dram_addr(AspeedSMCState *s) { return (s->regs[R_DMA_DRAM_ADDR] | ((uint64_t) s->regs[R_DMA_DRAM_ADDR_HIGH] << 32)) - s->dram_base; } Then no need for dma_dram_offset, dma_dram_addr is enough. 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], +dma_dram_offset, dma_len); while (dma_len) { if (s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE) { -data = address_space_ldl_le(>dram_as, s->regs[R_DMA_DRAM_ADDR], +data = address_space_ldl_le(>dram_as, dma_dram_offset, MEMTXATTRS_UNSPECIFIED, ); if (result != MEMTX_OK) { -aspeed_smc_error("DRAM read failed @%08x", - s->regs[R_DMA_DRAM_ADDR]); +aspeed_smc_error("DRAM read failed @%" PRIx64, + dma_dram_offset); return; } @@ -940,11 +968,11 @@ static void aspeed_smc_dma_rw(AspeedSMCState *s) return; } -address_space_stl_le(>dram_as, s->regs[R_DMA_DRAM_ADDR], +address_space_stl_le(>dram_as, dma_dram_offset, data, MEMTXATTRS_UNSPECIFIED, ); if (result != MEMTX_OK) { -aspeed_smc_error("DRAM write failed @%08x", - s->regs[R_DMA_DRAM_ADDR]); +aspeed_smc_error("DRAM write failed @%" PRIx64, + dma_dram_offset); return; } } @@ -953,8 +981,12 @@ 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. */ +dma_dram_offset += 4; +dma_dram_addr += 4; + +s->regs[R_DMA_DRAM_ADDR_HIGH] = dma_dram_addr >> 32; +s->regs[R_DMA_DRAM_ADDR] = dma_dram_addr & 0x; 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; @@ -1107,6 +1139,9 @@ 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_dma64(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); @@ -1239,6 +1274,7 @@ static const VMStateDescription vmstate_aspeed_smc = { static Property aspeed_smc_properties[] = { DEFINE_PROP_BOOL("inject-failure", AspeedSMCState, inject_failure, false), +DEFINE_PROP_UINT64("dram-base", AspeedSMCState, dram_base, 0), DEFINE_PROP_LINK("dram", AspeedSMCState, dram_mr, TYPE_MEMORY_REGION, MemoryRegion *), DEFINE_PROP_END_OF_LIST(),
Re: [PATCH v4 08/16] aspeed/smc: support 64 bits dma dram address
On 5/27/24 10:02, 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 Signed-off-by: Jamin Lin I will move the addition of the "dram-base" property to another patch. See : https://patchew.org/QEMU/20240527124315.35356-1-...@redhat.com/ (Please review) Else, Reviewed-by: Cédric Le Goater Thanks, C. --- hw/ssi/aspeed_smc.c | 52 +++-- hw/ssi/trace-events | 2 +- include/hw/ssi/aspeed_smc.h | 1 + 3 files changed, 46 insertions(+), 9 deletions(-) diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c index ffb13a12e8..df0c63469c 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 @@ * 0x1FF: 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) & 0x01FF) @@ -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_dma64(const AspeedSMCClass *asc) +{ +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,8 @@ 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_dma64(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 +859,12 @@ static bool aspeed_smc_inject_read_failure(AspeedSMCState *s) } } +static uint64_t aspeed_smc_dma_dram_addr(AspeedSMCState *s) +{ +return s->regs[R_DMA_DRAM_ADDR] | +((uint64_t) s->regs[R_DMA_DRAM_ADDR_HIGH] << 32); +} + static uint32_t aspeed_smc_dma_len(AspeedSMCState *s) { AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s); @@ -903,24 +921,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 dma_dram_offset; +uint64_t dma_dram_addr; 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_dma64(asc)) { +dma_dram_offset = dma_dram_addr - s->dram_base; +} else { +dma_dram_offset = 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], +dma_dram_offset, dma_len); while (dma_len) { if (s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE) { -data = address_space_ldl_le(>dram_as, s->regs[R_DMA_DRAM_ADDR], +data = address_space_ldl_le(>dram_as, dma_dram_offset, MEMTXATTRS_UNSPECIFIED, ); if (result != MEMTX_OK) { -aspeed_smc_error("DRAM read failed @%08x", - s->regs[R_DMA_DRAM_ADDR]); +aspeed_smc_error("DRAM read failed @%" PRIx64, + dma_dram_offset); return; } @@ -940,11 +968,11 @@ static void aspeed_smc_dma_rw(AspeedSMCState *s) return; } -