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(>fmc), "dram-base",
> >> +sc->memmap[ASPEED_DEV_SDRAM],
> >> + _abort);
> >>object_property_set_link(OBJECT(>fmc), "dram",
> >> OBJECT(s->dram_mr),
> >> _abort);
> >>if (!sysbus_realize(SYS_BUS_DEVICE(>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 const AspeedSegments
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(>fmc), "dram-base",
+sc->memmap[ASPEED_DEV_SDRAM],
+ _abort);
object_property_set_link(OBJECT(>fmc), "dram",
OBJECT(s->dram_mr),
_abort);
if (!sysbus_realize(SYS_BUS_DEVICE(>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 == R_DMA_FLASH_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(>fmc), "dram-base",
> +sc->memmap[ASPEED_DEV_SDRAM],
> + _abort);
> object_property_set_link(OBJECT(>fmc), "dram",
> OBJECT(s->dram_mr),
>_abort);
> if (!sysbus_realize(SYS_BUS_DEVICE(>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 >= R_SEG_ADDR0 &&
> >
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(>fmc), "dram-base",
+sc->memmap[ASPEED_DEV_SDRAM], _abort);
object_property_set_link(OBJECT(>fmc), "dram", OBJECT(s->dram_mr),
_abort);
if (!sysbus_realize(SYS_BUS_DEVICE(>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 = dram_addr_high |
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 and ple
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
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
[PATCH v3 08/16] aspeed/smc: support 64 bits dma dram address
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 = dram_addr_high | s->regs[R_DMA_DRAM_ADDR];
+} else {
+dma_dram_addr = 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;
+uint64_t dma_dram_addr;
+uint64_t 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_dma_dram_addr_high(asc)) {
+dram_addr = dma_dram_addr - s->dram_mr->container->addr;
+} 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(>dram_as, s->regs[R_DMA_DRAM_ADDR],
+data = address_space_ldl_le(>dram_as, dram_addr,
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, dram_addr);
return;
}
@@ -951,11 +991,10 @@ static void aspeed_smc_dma_rw(AspeedSMCState *s)
return;
}
-address_space_stl_le(>dram_as,
