RE: [PATCH v3 08/16] aspeed/smc: support 64 bits dma dram address

2024-05-27 Thread Jamin Lin
> 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

2024-05-27 Thread Cédric Le Goater

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

2024-05-19 Thread Jamin Lin
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

2024-05-17 Thread Cédric Le Goater

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

2024-05-15 Thread Jamin Lin
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

2024-05-15 Thread Jamin Lin
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

2024-05-07 Thread Cédric Le Goater



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

2024-05-03 Thread Cédric Le Goater

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

2024-04-30 Thread Jamin Lin
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

2024-04-30 Thread Cédric Le Goater

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

2024-04-19 Thread Jamin Lin
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

2024-04-18 Thread Cédric Le Goater

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

2024-04-16 Thread Jamin Lin via
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,