RE: [PATCH v3 08/16] aspeed/smc: support 64 bits dma dram address
> Hello Jamin, > > > [ ... ] > > >> See my aspeed-9.1 branch, I did some changes, mostly in the last patch. > >> > >> * aspeed_smc_dma_len() > >> > >> - can use QEMU_ALIGN_UP(). simpler. > >> > >> * aspeed_smc_dma_rw(): > >> > >> - dram_addr -> dma_dram_offset > >> - There is no need to protect updates of the > R_DMA_DRAM_ADDR_HIGH > >> register with aspeed_smc_has_dma_dram_addr_high() since it is > >> already protected with MMIO accesses. Skip the check and update > >> always. > >> > >> * aspeed_smc_dma_dram_addr() > >> > >> - same as above. > >> > >> You can merge the changes in the respective patches if you agree. > >> > >> Still on the TODO list : > >> > >> - GIC review > >> - aspeed/soc: fix incorrect dram size for AST2700 > >> > >> Thanks, > >> > >> C. > >> > > I merged this commit into my code base and thanks for your kindly support. > > > https://github.com/legoater/qemu/commit/d1bc2c776422a9d0d6af2b4414fae > 83fde1832ba > > > > About GIC settings, you can refer to our kernel DTS setting for detail. > > > https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/arm > 64/boot/dts/aspeed/aspeed-g7.dtsi#L143-L164 > > Could you please resend a v4 including all the changes we discussed ? > > Thanks, > > C. > > OK Thanks-Jamin
Re: [PATCH v3 08/16] aspeed/smc: support 64 bits dma dram address
Hello Jamin, [ ... ] See my aspeed-9.1 branch, I did some changes, mostly in the last patch. * aspeed_smc_dma_len() - can use QEMU_ALIGN_UP(). simpler. * aspeed_smc_dma_rw(): - dram_addr -> dma_dram_offset - There is no need to protect updates of the R_DMA_DRAM_ADDR_HIGH register with aspeed_smc_has_dma_dram_addr_high() since it is already protected with MMIO accesses. Skip the check and update always. * aspeed_smc_dma_dram_addr() - same as above. You can merge the changes in the respective patches if you agree. Still on the TODO list : - GIC review - aspeed/soc: fix incorrect dram size for AST2700 Thanks, C. I merged this commit into my code base and thanks for your kindly support. https://github.com/legoater/qemu/commit/d1bc2c776422a9d0d6af2b4414fae83fde1832ba About GIC settings, you can refer to our kernel DTS setting for detail. https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi#L143-L164 Could you please resend a v4 including all the changes we discussed ? Thanks, C.
RE: [PATCH v3 08/16] aspeed/smc: support 64 bits dma dram address
Hi Cedric, > > Hello Jamin > > On 5/15/24 11:01, Jamin Lin wrote: > > Hi Cedric, > > > > Sorry reply you late. > >> Hello Jamin, > >> > >> To handle the DMA DRAM Side Address High register, we should > >> reintroduce an "dram-base" property which I removed a while ago. > Something like : > >> > >> > >> > >> diff --git a/include/hw/ssi/aspeed_smc.h > >> b/include/hw/ssi/aspeed_smc.h index 7f32e43ff6f3..6d8ef6bc968f 100644 > >> --- a/include/hw/ssi/aspeed_smc.h > >> +++ b/include/hw/ssi/aspeed_smc.h > >> @@ -76,6 +76,7 @@ struct AspeedSMCState { > >>AddressSpace flash_as; > >>MemoryRegion *dram_mr; > >>AddressSpace dram_as; > >> +uint64_t dram_base; > >> > >>AddressSpace wdt2_as; > >>MemoryRegion *wdt2_mr; > >> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c index > >> 38858e4fdec1..3417949ad8a3 100644 > >> --- a/hw/arm/aspeed_ast27x0.c > >> +++ b/hw/arm/aspeed_ast27x0.c > >> @@ -500,6 +500,8 @@ static void > >> aspeed_soc_ast2700_realize(DeviceState > >> *dev, Error **errp) > >>} > >> > >>/* FMC, The number of CS is set at the board level */ > >> +object_property_set_int(OBJECT(&s->fmc), "dram-base", > >> +sc->memmap[ASPEED_DEV_SDRAM], > >> + &error_abort); > >>object_property_set_link(OBJECT(&s->fmc), "dram", > >> OBJECT(s->dram_mr), > >> &error_abort); > >>if (!sysbus_realize(SYS_BUS_DEVICE(&s->fmc), errp)) { diff > >> --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c index > >> 3fa783578e9e..29ebfc0fd8c8 100644 > >> --- a/hw/ssi/aspeed_smc.c > >> +++ b/hw/ssi/aspeed_smc.c > >> @@ -1372,6 +1372,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_LINK("wdt2", AspeedSMCState, wdt2_mr, > >> > >> > > I appreciate your kindly support and thanks for your suggestion. > > Will add it. > > See my aspeed-9.1 branch, I did some changes, mostly in the last patch. > > * aspeed_smc_dma_len() > >- can use QEMU_ALIGN_UP(). simpler. > > * aspeed_smc_dma_rw(): > >- dram_addr -> dma_dram_offset >- There is no need to protect updates of the R_DMA_DRAM_ADDR_HIGH > register with aspeed_smc_has_dma_dram_addr_high() since it is > already protected with MMIO accesses. Skip the check and update > always. > > * aspeed_smc_dma_dram_addr() > >- same as above. > > You can merge the changes in the respective patches if you agree. > > Still on the TODO list : > >- GIC review >- aspeed/soc: fix incorrect dram size for AST2700 > > > > > Thanks, > > C. > I merged this commit into my code base and thanks for your kindly support. https://github.com/legoater/qemu/commit/d1bc2c776422a9d0d6af2b4414fae83fde1832ba About GIC settings, you can refer to our kernel DTS setting for detail. https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi#L143-L164 Thanks-Jamin > > > > > >> > >> With that, see below for more comments, > >> > >> 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 > >>> Signed-off-by: Jamin Lin > >>> --- > >>>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 @@ > >>> * 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
Re: [PATCH v3 08/16] aspeed/smc: support 64 bits dma dram address
Hello Jamin On 5/15/24 11:01, Jamin Lin wrote: Hi Cedric, Sorry reply you late. Hello Jamin, To handle the DMA DRAM Side Address High register, we should reintroduce an "dram-base" property which I removed a while ago. Something like : diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h index 7f32e43ff6f3..6d8ef6bc968f 100644 --- a/include/hw/ssi/aspeed_smc.h +++ b/include/hw/ssi/aspeed_smc.h @@ -76,6 +76,7 @@ struct AspeedSMCState { AddressSpace flash_as; MemoryRegion *dram_mr; AddressSpace dram_as; +uint64_t dram_base; AddressSpace wdt2_as; MemoryRegion *wdt2_mr; diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c index 38858e4fdec1..3417949ad8a3 100644 --- a/hw/arm/aspeed_ast27x0.c +++ b/hw/arm/aspeed_ast27x0.c @@ -500,6 +500,8 @@ static void aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp) } /* FMC, The number of CS is set at the board level */ +object_property_set_int(OBJECT(&s->fmc), "dram-base", +sc->memmap[ASPEED_DEV_SDRAM], + &error_abort); object_property_set_link(OBJECT(&s->fmc), "dram", OBJECT(s->dram_mr), &error_abort); if (!sysbus_realize(SYS_BUS_DEVICE(&s->fmc), errp)) { diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c index 3fa783578e9e..29ebfc0fd8c8 100644 --- a/hw/ssi/aspeed_smc.c +++ b/hw/ssi/aspeed_smc.c @@ -1372,6 +1372,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_LINK("wdt2", AspeedSMCState, wdt2_mr, I appreciate your kindly support and thanks for your suggestion. Will add it. See my aspeed-9.1 branch, I did some changes, mostly in the last patch. * aspeed_smc_dma_len() - can use QEMU_ALIGN_UP(). simpler. * aspeed_smc_dma_rw(): - dram_addr -> dma_dram_offset - There is no need to protect updates of the R_DMA_DRAM_ADDR_HIGH register with aspeed_smc_has_dma_dram_addr_high() since it is already protected with MMIO accesses. Skip the check and update always. * aspeed_smc_dma_dram_addr() - same as above. You can merge the changes in the respective patches if you agree. Still on the TODO list : - GIC review - aspeed/soc: fix incorrect dram size for AST2700 Thanks, C. With that, see below for more comments, 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 Signed-off-by: Jamin Lin --- 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 @@ * 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_dma_dram_addr_high(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,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 ==
RE: [PATCH v3 08/16] aspeed/smc: support 64 bits dma dram address
Hi Cedric, Sorry reply you late. > Hello Jamin, > > To handle the DMA DRAM Side Address High register, we should reintroduce an > "dram-base" property which I removed a while ago. Something like : > > > > diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h index > 7f32e43ff6f3..6d8ef6bc968f 100644 > --- a/include/hw/ssi/aspeed_smc.h > +++ b/include/hw/ssi/aspeed_smc.h > @@ -76,6 +76,7 @@ struct AspeedSMCState { > AddressSpace flash_as; > MemoryRegion *dram_mr; > AddressSpace dram_as; > +uint64_t dram_base; > > AddressSpace wdt2_as; > MemoryRegion *wdt2_mr; > diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c index > 38858e4fdec1..3417949ad8a3 100644 > --- a/hw/arm/aspeed_ast27x0.c > +++ b/hw/arm/aspeed_ast27x0.c > @@ -500,6 +500,8 @@ static void aspeed_soc_ast2700_realize(DeviceState > *dev, Error **errp) > } > > /* FMC, The number of CS is set at the board level */ > +object_property_set_int(OBJECT(&s->fmc), "dram-base", > +sc->memmap[ASPEED_DEV_SDRAM], > + &error_abort); > object_property_set_link(OBJECT(&s->fmc), "dram", > OBJECT(s->dram_mr), >&error_abort); > if (!sysbus_realize(SYS_BUS_DEVICE(&s->fmc), errp)) { diff --git > a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c index > 3fa783578e9e..29ebfc0fd8c8 100644 > --- a/hw/ssi/aspeed_smc.c > +++ b/hw/ssi/aspeed_smc.c > @@ -1372,6 +1372,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_LINK("wdt2", AspeedSMCState, wdt2_mr, > > I appreciate your kindly support and thanks for your suggestion. Will add it. > > With that, see below for more comments, > > 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 > > Signed-off-by: Jamin Lin > > --- > > 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 @@ > >* 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_dma_dram_addr_high(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,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 >=
RE: [PATCH v3 08/16] aspeed/smc: support 64 bits dma dram address
Hi Cedric, Sorry reply you late. > Hello Jamin, > > On 4/30/24 09:56, Jamin Lin wrote: > > Hi Cedric, > > > >> -Original Message- > >> From: Cédric Le Goater > >> Sent: Tuesday, April 30, 2024 3:26 PM > >> To: Jamin Lin ; Peter Maydell > >> ; Andrew Jeffery > >> ; Joel Stanley ; > >> Alistair Francis ; Cleber Rosa > >> ; Philippe Mathieu-Daudé ; > >> Wainer dos Santos Moschetta ; Beraldo Leal > >> ; open list:ASPEED BMCs ; > open > >> list:All patches CC here > >> Cc: Troy Lee ; Yunlin Tang > >> > >> Subject: Re: [PATCH v3 08/16] aspeed/smc: support 64 bits dma dram > >> address > >> > >> On 4/19/24 08:00, Jamin Lin wrote: > >>> 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 > >>>>> Signed-off-by: Jamin Lin > >>>>> --- > >>>>> 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 @@ > >>>>> * 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_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__, > >> ## >
Re: [PATCH v3 08/16] aspeed/smc: support 64 bits dma dram address
Hello Jamin, To handle the DMA DRAM Side Address High register, we should reintroduce an "dram-base" property which I removed a while ago. Something like : diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h index 7f32e43ff6f3..6d8ef6bc968f 100644 --- a/include/hw/ssi/aspeed_smc.h +++ b/include/hw/ssi/aspeed_smc.h @@ -76,6 +76,7 @@ struct AspeedSMCState { AddressSpace flash_as; MemoryRegion *dram_mr; AddressSpace dram_as; +uint64_t dram_base; AddressSpace wdt2_as; MemoryRegion *wdt2_mr; diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c index 38858e4fdec1..3417949ad8a3 100644 --- a/hw/arm/aspeed_ast27x0.c +++ b/hw/arm/aspeed_ast27x0.c @@ -500,6 +500,8 @@ static void aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp) } /* FMC, The number of CS is set at the board level */ +object_property_set_int(OBJECT(&s->fmc), "dram-base", +sc->memmap[ASPEED_DEV_SDRAM], &error_abort); object_property_set_link(OBJECT(&s->fmc), "dram", OBJECT(s->dram_mr), &error_abort); if (!sysbus_realize(SYS_BUS_DEVICE(&s->fmc), errp)) { diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c index 3fa783578e9e..29ebfc0fd8c8 100644 --- a/hw/ssi/aspeed_smc.c +++ b/hw/ssi/aspeed_smc.c @@ -1372,6 +1372,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_LINK("wdt2", AspeedSMCState, wdt2_mr, With that, see below for more comments, 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 Signed-off-by: Jamin Lin --- 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 @@ * 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_dma_dram_addr_high(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,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 =
Re: [PATCH v3 08/16] aspeed/smc: support 64 bits dma dram address
Hello Jamin, On 4/30/24 09:56, Jamin Lin wrote: Hi Cedric, -Original Message- From: Cédric Le Goater Sent: Tuesday, April 30, 2024 3:26 PM To: Jamin Lin ; Peter Maydell ; Andrew Jeffery ; Joel Stanley ; Alistair Francis ; Cleber Rosa ; Philippe Mathieu-Daudé ; Wainer dos Santos Moschetta ; Beraldo Leal ; open list:ASPEED BMCs ; open list:All patches CC here Cc: Troy Lee ; Yunlin Tang Subject: Re: [PATCH v3 08/16] aspeed/smc: support 64 bits dma dram address On 4/19/24 08:00, Jamin Lin wrote: 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 Signed-off-by: Jamin Lin --- 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 @@ * 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_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 an
RE: [PATCH v3 08/16] aspeed/smc: support 64 bits dma dram address
Hi Cedric, > -Original Message- > From: Cédric Le Goater > Sent: Tuesday, April 30, 2024 3:26 PM > To: Jamin Lin ; Peter Maydell > ; Andrew Jeffery ; > Joel Stanley ; Alistair Francis ; > Cleber > Rosa ; Philippe Mathieu-Daudé ; > Wainer dos Santos Moschetta ; Beraldo Leal > ; open list:ASPEED BMCs ; open > list:All patches CC here > Cc: Troy Lee ; Yunlin Tang > > Subject: Re: [PATCH v3 08/16] aspeed/smc: support 64 bits dma dram address > > On 4/19/24 08:00, Jamin Lin wrote: > > 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 > >>> Signed-off-by: Jamin Lin > >>> --- > >>>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 @@ > >>> * 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_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_ADD
Re: [PATCH v3 08/16] aspeed/smc: support 64 bits dma dram address
On 4/19/24 08:00, Jamin Lin wrote: 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 Signed-off-by: Jamin Lin --- 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 @@ * 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_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 for AST2700. Could you
RE: [PATCH v3 08/16] aspeed/smc: support 64 bits dma dram address
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 > > Signed-off-by: Jamin Lin > > --- > > 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 @@ > >* 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_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
Re: [PATCH v3 08/16] aspeed/smc: support 64 bits dma dram address
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 Signed-off-by: Jamin Lin --- 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 @@ * 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_dma_dram_addr_high(const AspeedSMCClass *asc) To ease the reading, I would call the helper aspeed_smc_has_dma64() +{ +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; +} + 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 +uint64_t dma_dram_addr; +uint64_t dram_addr; and dram_addr is redundant with dma_dram_addr. Please use only one. 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) +} 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 =