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

Commit Message

Jamin Lin April 16, 2024, 9:18 a.m. UTC

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 <[e-mail]>
Signed-off-by: Jamin Lin <[e-mail]>
---
 hw/ssi/aspeed_smc.c | 66 +++++++++++++++++++++++++++++++++++++++------
 hw/ssi/trace-events |  2 +-
 2 files changed, 59 insertions(+), 9 deletions(-)

Comments

Cédric Le Goater April 18, 2024, 4:09 p.m. UTC | #1

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 <[e-mail]>
> Signed-off-by: Jamin Lin <[e-mail]>
> ---
>   hw/ssi/aspeed_smc.c | 66 +++++++++++++++++++++++++++++++++++++++------
>   hw/ssi/trace-events |  2 +-
>   2 files changed, 59 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index 71abc7a2d8..a67cac3d0f 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -132,6 +132,9 @@
>   #define   FMC_WDT2_CTRL_BOOT_SOURCE      BIT(4) /* O: primary 1: alternate */
>   #define   FMC_WDT2_CTRL_EN               BIT(0)
>   
> +/* DMA DRAM Side Address High Part (AST2700) */
> +#define R_DMA_DRAM_ADDR_HIGH   (0x7c / 4)
> +
>   /* DMA Control/Status Register */
>   #define R_DMA_CTRL        (0x80 / 4)
>   #define   DMA_CTRL_REQUEST      (1 << 31)
> @@ -187,6 +190,7 @@
>    *   0x1FFFFFF: 32M bytes
>    */
>   #define DMA_DRAM_ADDR(asc, val)   ((val) & (asc)->dma_dram_mask)
> +#define DMA_DRAM_ADDR_HIGH(val)   ((val) & 0xf)
>   #define DMA_FLASH_ADDR(asc, val)  ((val) & (asc)->dma_flash_mask)
>   #define DMA_LENGTH(val)         ((val) & 0x01FFFFFF)
>   
> @@ -207,6 +211,7 @@ static const AspeedSegments aspeed_2500_spi2_segments[];
>   #define ASPEED_SMC_FEATURE_DMA       0x1
>   #define ASPEED_SMC_FEATURE_DMA_GRANT 0x2
>   #define ASPEED_SMC_FEATURE_WDT_CONTROL 0x4
> +#define ASPEED_SMC_FEATURE_DMA_DRAM_ADDR_HIGH 0x08
>   
>   static inline bool aspeed_smc_has_dma(const AspeedSMCClass *asc)
>   {
> @@ -218,6 +223,11 @@ static inline bool aspeed_smc_has_wdt_control(const AspeedSMCClass *asc)
>       return !!(asc->features & ASPEED_SMC_FEATURE_WDT_CONTROL);
>   }
>   
> +static inline bool aspeed_smc_has_dma_dram_addr_high(const AspeedSMCClass *asc)

To ease the reading, I would call the helper aspeed_smc_has_dma64()

> +{
> +    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 = address_space_ldl_le(&s->dram_as, s->regs[R_DMA_DRAM_ADDR],
> +            data = address_space_ldl_le(&s->dram_as, dram_addr,
>                                           MEMTXATTRS_UNSPECIFIED, &result);
>               if (result != MEMTX_OK) {
> -                aspeed_smc_error("DRAM read failed @%08x",
> -                                 s->regs[R_DMA_DRAM_ADDR]);
> +                aspeed_smc_error("DRAM read failed @%" PRIx64, dram_addr);
>                   return;
>               }
>   
> @@ -951,11 +991,10 @@ static void aspeed_smc_dma_rw(AspeedSMCState *s)
>                   return;
>               }
>   
> -            address_space_stl_le(&s->dram_as, s->regs[R_DMA_DRAM_ADDR],
> +            address_space_stl_le(&s->dram_as, dram_addr,
>                                    data, MEMTXATTRS_UNSPECIFIED, &result);
>               if (result != MEMTX_OK) {
> -                aspeed_smc_error("DRAM write failed @%08x",
> -                                 s->regs[R_DMA_DRAM_ADDR]);
> +                aspeed_smc_error("DRAM write failed @%" PRIx64, dram_addr);
>                   return;
>               }
>           }
> @@ -964,8 +1003,15 @@ static void aspeed_smc_dma_rw(AspeedSMCState *s)
>            * When the DMA is on-going, the DMA registers are updated
>            * with the current working addresses and length.
>            */
> +        dram_addr += 4;
> +        dma_dram_addr += 4;
> +        if (aspeed_smc_has_dma_dram_addr_high(asc)) {
> +            dram_addr_high = dma_dram_addr >> 32;
> +            s->regs[R_DMA_DRAM_ADDR_HIGH] = dram_addr_high;

             s->regs[R_DMA_DRAM_ADDR_HIGH] = dma_dram_addr >> 32;

> +        }
> +
> +        s->regs[R_DMA_DRAM_ADDR] = dma_dram_addr & 0xffffffff;

use DMA_DRAM_ADDR() may be instead.

>           s->regs[R_DMA_FLASH_ADDR] += 4;
> -        s->regs[R_DMA_DRAM_ADDR] += 4;
>           dma_len -= 4;
>           s->regs[R_DMA_LEN] = dma_len;
>           s->regs[R_DMA_CHECKSUM] += data;
> @@ -1118,6 +1164,10 @@ static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
>       } else if (aspeed_smc_has_dma(asc) && addr == R_DMA_LEN &&
>                  aspeed_smc_dma_granted(s)) {
>           s->regs[addr] = DMA_LENGTH(value);
> +    } else if (aspeed_smc_has_dma(asc) &&
> +               aspeed_smc_has_dma_dram_addr_high(asc) &&
> +               addr == R_DMA_DRAM_ADDR_HIGH) {
> +        s->regs[addr] = DMA_DRAM_ADDR_HIGH(value);
>       } else {
>           qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx "\n",
>                         __func__, addr);
> diff --git a/hw/ssi/trace-events b/hw/ssi/trace-events
> index 2d5bd2b83d..7b5ad6a939 100644
> --- a/hw/ssi/trace-events
> +++ b/hw/ssi/trace-events
> @@ -6,7 +6,7 @@ aspeed_smc_do_snoop(int cs, int index, int dummies, int data) "CS%d index:0x%x d
>   aspeed_smc_flash_write(int cs, uint64_t addr,  uint32_t size, uint64_t data, int mode) "CS%d @0x%" PRIx64 " size %u: 0x%" PRIx64" mode:%d"
>   aspeed_smc_read(uint64_t addr,  uint32_t size, uint64_t data) "@0x%" PRIx64 " size %u: 0x%" PRIx64
>   aspeed_smc_dma_checksum(uint32_t addr, uint32_t data) "0x%08x: 0x%08x"
> -aspeed_smc_dma_rw(const char *dir, uint32_t flash_addr, uint32_t dram_addr, uint32_t size) "%s flash:@0x%08x dram:@0x%08x size:0x%08x"
> +aspeed_smc_dma_rw(const char *dir, uint32_t flash_addr, uint64_t dram_addr, uint32_t size) "%s flash:@0x%08x dram:@0x%" PRIx64 " size:0x%08x"
>   aspeed_smc_write(uint64_t addr,  uint32_t size, uint64_t data) "@0x%" PRIx64 " size %u: 0x%" PRIx64
>   aspeed_smc_flash_select(int cs, const char *prefix) "CS%d %sselect"
>

Jamin Lin April 19, 2024, 6 a.m. UTC | #2

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 <[e-mail]>
> > Signed-off-by: Jamin Lin <[e-mail]>
> > ---
> >   hw/ssi/aspeed_smc.c | 66
> +++++++++++++++++++++++++++++++++++++++------
> >   hw/ssi/trace-events |  2 +-
> >   2 files changed, 59 insertions(+), 9 deletions(-)
> >
> > diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c index
> > 71abc7a2d8..a67cac3d0f 100644
> > --- a/hw/ssi/aspeed_smc.c
> > +++ b/hw/ssi/aspeed_smc.c
> > @@ -132,6 +132,9 @@
> >   #define   FMC_WDT2_CTRL_BOOT_SOURCE      BIT(4) /* O: primary
> 1: alternate */
> >   #define   FMC_WDT2_CTRL_EN               BIT(0)
> >
> > +/* DMA DRAM Side Address High Part (AST2700) */
> > +#define R_DMA_DRAM_ADDR_HIGH   (0x7c / 4)
> > +
> >   /* DMA Control/Status Register */
> >   #define R_DMA_CTRL        (0x80 / 4)
> >   #define   DMA_CTRL_REQUEST      (1 << 31)
> > @@ -187,6 +190,7 @@
> >    *   0x1FFFFFF: 32M bytes
> >    */
> >   #define DMA_DRAM_ADDR(asc, val)   ((val) & (asc)->dma_dram_mask)
> > +#define DMA_DRAM_ADDR_HIGH(val)   ((val) & 0xf)
> >   #define DMA_FLASH_ADDR(asc, val)  ((val) & (asc)->dma_flash_mask)
> >   #define DMA_LENGTH(val)         ((val) & 0x01FFFFFF)
> >
> > @@ -207,6 +211,7 @@ static const AspeedSegments
> aspeed_2500_spi2_segments[];
> >   #define ASPEED_SMC_FEATURE_DMA       0x1
> >   #define ASPEED_SMC_FEATURE_DMA_GRANT 0x2
> >   #define ASPEED_SMC_FEATURE_WDT_CONTROL 0x4
> > +#define ASPEED_SMC_FEATURE_DMA_DRAM_ADDR_HIGH 0x08
> >
> >   static inline bool aspeed_smc_has_dma(const AspeedSMCClass *asc)
> >   {
> > @@ -218,6 +223,11 @@ static inline bool
> aspeed_smc_has_wdt_control(const AspeedSMCClass *asc)
> >       return !!(asc->features & ASPEED_SMC_FEATURE_WDT_CONTROL);
> >   }
> >
> > +static inline bool aspeed_smc_has_dma_dram_addr_high(const
> > +AspeedSMCClass *asc)
> 
> To ease the reading, I would call the helper aspeed_smc_has_dma64()
Will fix it
> 
> > +{
> > +    return !!(asc->features &
> ASPEED_SMC_FEATURE_DMA_DRAM_ADDR_HIGH);
> > +}
> > +
> >   #define aspeed_smc_error(fmt, ...)
> \
> >       qemu_log_mask(LOG_GUEST_ERROR, "%s: " fmt "\n", __func__, ##
> > __VA_ARGS__)
> >
> > @@ -747,6 +757,9 @@ static uint64_t aspeed_smc_read(void *opaque,
> hwaddr addr, unsigned int size)
> >           (aspeed_smc_has_dma(asc) && addr == R_DMA_CTRL) ||
> >           (aspeed_smc_has_dma(asc) && addr == R_DMA_FLASH_ADDR)
> ||
> >           (aspeed_smc_has_dma(asc) && addr == R_DMA_DRAM_ADDR)
> ||
> > +        (aspeed_smc_has_dma(asc) &&
> > +         aspeed_smc_has_dma_dram_addr_high(asc) &&
> > +         addr == R_DMA_DRAM_ADDR_HIGH) ||
> >           (aspeed_smc_has_dma(asc) && addr == R_DMA_LEN) ||
> >           (aspeed_smc_has_dma(asc) && addr == R_DMA_CHECKSUM)
> ||
> >           (addr >= R_SEG_ADDR0 &&
> > @@ -847,6 +860,23 @@ static bool
> aspeed_smc_inject_read_failure(AspeedSMCState *s)
> >       }
> >   }
> >
> > +static uint64_t aspeed_smc_dma_dram_addr(AspeedSMCState *s) {
> > +    AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
> > +    uint64_t dram_addr_high;
> > +    uint64_t dma_dram_addr;
> > +
> > +    if (aspeed_smc_has_dma_dram_addr_high(asc)) {
> > +        dram_addr_high = s->regs[R_DMA_DRAM_ADDR_HIGH];
> > +        dram_addr_high <<= 32;
> > +        dma_dram_addr = dram_addr_high |
> s->regs[R_DMA_DRAM_ADDR];
> 
> Here is a proposal to shorten the routine :
> 
>          return ((uint64_t) s->regs[R_DMA_DRAM_ADDR_HIGH] << 32) |
>              s->regs[R_DMA_DRAM_ADDR];
> 
> 
> > +    } else {
> > +        dma_dram_addr = s->regs[R_DMA_DRAM_ADDR];
> 
> and
>          return s->regs[R_DMA_DRAM_ADDR];
> 
> > +    }
> > +
> > +    return dma_dram_addr;
> > +}
> > +
Thanks for your suggestion. Will fix.
> >   static uint32_t aspeed_smc_dma_len(AspeedSMCState *s)
> >   {
> >       AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s); @@ -914,24
> > +944,34 @@ static void aspeed_smc_dma_checksum(AspeedSMCState *s)
> >
> >   static void aspeed_smc_dma_rw(AspeedSMCState *s)
> >   {
> > +    AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
> > +    uint64_t dram_addr_high;
> 
> This variable doesn't look very useful
Will try to remove it.
> 
> > +    uint64_t dma_dram_addr;
> > +    uint64_t dram_addr;
> 
> and dram_addr is redundant with dma_dram_addr. Please use only one.
Please see my below description and please give us any suggestion.
> 
> 
> >       MemTxResult result;
> >       uint32_t dma_len;
> >       uint32_t data;
> >
> >       dma_len = aspeed_smc_dma_len(s);
> > +    dma_dram_addr = aspeed_smc_dma_dram_addr(s);
> > +
> > +    if (aspeed_smc_has_dma_dram_addr_high(asc)) {
> > +        dram_addr = dma_dram_addr - s->dram_mr->container->addr;
> 
> Why do you truncate the address again ? It should already be done with
> 
> #define DMA_DRAM_ADDR_HIGH(val)   ((val) & 0xf)
> 
The reason is that our firmware set the real address in SMC registers.
For example: If users want to move data from flash to the DRAM at address 0,
It set R_DMA_DRAM_ADDR_HIGH 4 and R_DMA_DRAM_ADDR 0 because the
dram base address is 0x 4 00000000 for AST2700.

According to the design of QEMU, the dram container base address is 0x4 00000000(s->dram_mr).
Therefore, in the above example, the data should be moved to the dram container at address 0.

I created two variables to save the different base address.
A dma_dram_addr is used to save the real address.
A dram_addr is used to save the address for dram container(s->dram_as)

Example:
dma_dram_addr is 0x4 00000000  
(uint64) s->regs[R_DMA_DRAM_ADDR_HIGH] << 32) |s->regs[R_DMA_DRAM_ADDR]
dram_addr is 0
dram_addr = 0x400000000(real address) - 0x400000000(dram container base address) to get the address for dram contained for moving
dram_addr = dma_dram_addr - s->dram_mr->container->addr;

Finally, the following APIs could work properly.
data = address_space_ldl_le(&s->dram_as, dram_addr,
                       MEMTXATTRS_UNSPECIFIED, &result);
Thanks-Jamin
> > +    } else {
> > +        dram_addr = dma_dram_addr;
> > +    }
> >
> >       trace_aspeed_smc_dma_rw(s->regs[R_DMA_CTRL] &
> DMA_CTRL_WRITE ?
> >                               "write" : "read",
> >                               s->regs[R_DMA_FLASH_ADDR],
> > -                            s->regs[R_DMA_DRAM_ADDR],
> > +                            dram_addr,
> >                               dma_len);
> >       while (dma_len) {
> >           if (s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE) {
> > -            data = address_space_ldl_le(&s->dram_as,
> s->regs[R_DMA_DRAM_ADDR],
> > +            data = address_space_ldl_le(&s->dram_as, dram_addr,
> >
> MEMTXATTRS_UNSPECIFIED, &result);
> >               if (result != MEMTX_OK) {
> > -                aspeed_smc_error("DRAM read failed @%08x",
> > -                                 s->regs[R_DMA_DRAM_ADDR]);
> > +                aspeed_smc_error("DRAM read failed @%" PRIx64,
> > + dram_addr);
> >                   return;
> >               }
> >
> > @@ -951,11 +991,10 @@ static void aspeed_smc_dma_rw(AspeedSMCState
> *s)
> >                   return;
> >               }
> >
> > -            address_space_stl_le(&s->dram_as,
> s->regs[R_DMA_DRAM_ADDR],
> > +            address_space_stl_le(&s->dram_as, dram_addr,
> >                                    data,
> MEMTXATTRS_UNSPECIFIED, &result);
> >               if (result != MEMTX_OK) {
> > -                aspeed_smc_error("DRAM write failed @%08x",
> > -                                 s->regs[R_DMA_DRAM_ADDR]);
> > +                aspeed_smc_error("DRAM write failed @%" PRIx64,
> > + dram_addr);
> >                   return;
> >               }
> >           }
> > @@ -964,8 +1003,15 @@ static void aspeed_smc_dma_rw(AspeedSMCState
> *s)
> >            * When the DMA is on-going, the DMA registers are updated
> >            * with the current working addresses and length.
> >            */
> > +        dram_addr += 4;
> > +        dma_dram_addr += 4;
> > +        if (aspeed_smc_has_dma_dram_addr_high(asc)) {
> > +            dram_addr_high = dma_dram_addr >> 32;
> > +            s->regs[R_DMA_DRAM_ADDR_HIGH] = dram_addr_high;
> 
>              s->regs[R_DMA_DRAM_ADDR_HIGH] = dma_dram_addr >>
> 32;
> 
> > +        }
> > +
> > +        s->regs[R_DMA_DRAM_ADDR] = dma_dram_addr & 0xffffffff;
> 
> use DMA_DRAM_ADDR() may be instead.
> 
> >           s->regs[R_DMA_FLASH_ADDR] += 4;
> > -        s->regs[R_DMA_DRAM_ADDR] += 4;
> >           dma_len -= 4;
> >           s->regs[R_DMA_LEN] = dma_len;
> >           s->regs[R_DMA_CHECKSUM] += data; @@ -1118,6 +1164,10
> @@
> > static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
> >       } else if (aspeed_smc_has_dma(asc) && addr == R_DMA_LEN &&
> >                  aspeed_smc_dma_granted(s)) {
> >           s->regs[addr] = DMA_LENGTH(value);
> > +    } else if (aspeed_smc_has_dma(asc) &&
> > +               aspeed_smc_has_dma_dram_addr_high(asc) &&
> > +               addr == R_DMA_DRAM_ADDR_HIGH) {
> > +        s->regs[addr] = DMA_DRAM_ADDR_HIGH(value);
> >       } else {
> >           qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%"
> HWADDR_PRIx "\n",
> >                         __func__, addr); diff --git
> > a/hw/ssi/trace-events b/hw/ssi/trace-events index
> > 2d5bd2b83d..7b5ad6a939 100644
> > --- a/hw/ssi/trace-events
> > +++ b/hw/ssi/trace-events
> > @@ -6,7 +6,7 @@ aspeed_smc_do_snoop(int cs, int index, int dummies, int
> data) "CS%d index:0x%x d
> >   aspeed_smc_flash_write(int cs, uint64_t addr,  uint32_t size, uint64_t
> data, int mode) "CS%d @0x%" PRIx64 " size %u: 0x%" PRIx64" mode:%d"
> >   aspeed_smc_read(uint64_t addr,  uint32_t size, uint64_t data) "@0x%"
> PRIx64 " size %u: 0x%" PRIx64
> >   aspeed_smc_dma_checksum(uint32_t addr, uint32_t data) "0x%08x:
> 0x%08x"
> > -aspeed_smc_dma_rw(const char *dir, uint32_t flash_addr, uint32_t
> dram_addr, uint32_t size) "%s flash:@0x%08x dram:@0x%08x size:0x%08x"
> > +aspeed_smc_dma_rw(const char *dir, uint32_t flash_addr, uint64_t
> dram_addr, uint32_t size) "%s flash:@0x%08x dram:@0x%" PRIx64 "
> size:0x%08x"
> >   aspeed_smc_write(uint64_t addr,  uint32_t size, uint64_t data) "@0x%"
> PRIx64 " size %u: 0x%" PRIx64
> >   aspeed_smc_flash_select(int cs, const char *prefix) "CS%d %sselect"
> >

Cédric Le Goater April 30, 2024, 7:26 a.m. UTC | #3

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 <[e-mail]>
>>> Signed-off-by: Jamin Lin <[e-mail]>
>>> ---
>>>    hw/ssi/aspeed_smc.c | 66
>> +++++++++++++++++++++++++++++++++++++++------
>>>    hw/ssi/trace-events |  2 +-
>>>    2 files changed, 59 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c index
>>> 71abc7a2d8..a67cac3d0f 100644
>>> --- a/hw/ssi/aspeed_smc.c
>>> +++ b/hw/ssi/aspeed_smc.c
>>> @@ -132,6 +132,9 @@
>>>    #define   FMC_WDT2_CTRL_BOOT_SOURCE      BIT(4) /* O: primary
>> 1: alternate */
>>>    #define   FMC_WDT2_CTRL_EN               BIT(0)
>>>
>>> +/* DMA DRAM Side Address High Part (AST2700) */
>>> +#define R_DMA_DRAM_ADDR_HIGH   (0x7c / 4)
>>> +
>>>    /* DMA Control/Status Register */
>>>    #define R_DMA_CTRL        (0x80 / 4)
>>>    #define   DMA_CTRL_REQUEST      (1 << 31)
>>> @@ -187,6 +190,7 @@
>>>     *   0x1FFFFFF: 32M bytes
>>>     */
>>>    #define DMA_DRAM_ADDR(asc, val)   ((val) & (asc)->dma_dram_mask)
>>> +#define DMA_DRAM_ADDR_HIGH(val)   ((val) & 0xf)
>>>    #define DMA_FLASH_ADDR(asc, val)  ((val) & (asc)->dma_flash_mask)
>>>    #define DMA_LENGTH(val)         ((val) & 0x01FFFFFF)
>>>
>>> @@ -207,6 +211,7 @@ static const AspeedSegments
>> aspeed_2500_spi2_segments[];
>>>    #define ASPEED_SMC_FEATURE_DMA       0x1
>>>    #define ASPEED_SMC_FEATURE_DMA_GRANT 0x2
>>>    #define ASPEED_SMC_FEATURE_WDT_CONTROL 0x4
>>> +#define ASPEED_SMC_FEATURE_DMA_DRAM_ADDR_HIGH 0x08
>>>
>>>    static inline bool aspeed_smc_has_dma(const AspeedSMCClass *asc)
>>>    {
>>> @@ -218,6 +223,11 @@ static inline bool
>> aspeed_smc_has_wdt_control(const AspeedSMCClass *asc)
>>>        return !!(asc->features & ASPEED_SMC_FEATURE_WDT_CONTROL);
>>>    }
>>>
>>> +static inline bool aspeed_smc_has_dma_dram_addr_high(const
>>> +AspeedSMCClass *asc)
>>
>> To ease the reading, I would call the helper aspeed_smc_has_dma64()
> Will fix it
>>
>>> +{
>>> +    return !!(asc->features &
>> ASPEED_SMC_FEATURE_DMA_DRAM_ADDR_HIGH);
>>> +}
>>> +
>>>    #define aspeed_smc_error(fmt, ...)
>> \
>>>        qemu_log_mask(LOG_GUEST_ERROR, "%s: " fmt "\n", __func__, ##
>>> __VA_ARGS__)
>>>
>>> @@ -747,6 +757,9 @@ static uint64_t aspeed_smc_read(void *opaque,
>> hwaddr addr, unsigned int size)
>>>            (aspeed_smc_has_dma(asc) && addr == R_DMA_CTRL) ||
>>>            (aspeed_smc_has_dma(asc) && addr == R_DMA_FLASH_ADDR)
>> ||
>>>            (aspeed_smc_has_dma(asc) && addr == R_DMA_DRAM_ADDR)
>> ||
>>> +        (aspeed_smc_has_dma(asc) &&
>>> +         aspeed_smc_has_dma_dram_addr_high(asc) &&
>>> +         addr == R_DMA_DRAM_ADDR_HIGH) ||
>>>            (aspeed_smc_has_dma(asc) && addr == R_DMA_LEN) ||
>>>            (aspeed_smc_has_dma(asc) && addr == R_DMA_CHECKSUM)
>> ||
>>>            (addr >= R_SEG_ADDR0 &&
>>> @@ -847,6 +860,23 @@ static bool
>> aspeed_smc_inject_read_failure(AspeedSMCState *s)
>>>        }
>>>    }
>>>
>>> +static uint64_t aspeed_smc_dma_dram_addr(AspeedSMCState *s) {
>>> +    AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
>>> +    uint64_t dram_addr_high;
>>> +    uint64_t dma_dram_addr;
>>> +
>>> +    if (aspeed_smc_has_dma_dram_addr_high(asc)) {
>>> +        dram_addr_high = s->regs[R_DMA_DRAM_ADDR_HIGH];
>>> +        dram_addr_high <<= 32;
>>> +        dma_dram_addr = dram_addr_high |
>> s->regs[R_DMA_DRAM_ADDR];
>>
>> Here is a proposal to shorten the routine :
>>
>>           return ((uint64_t) s->regs[R_DMA_DRAM_ADDR_HIGH] << 32) |
>>               s->regs[R_DMA_DRAM_ADDR];
>>
>>
>>> +    } else {
>>> +        dma_dram_addr = s->regs[R_DMA_DRAM_ADDR];
>>
>> and
>>           return s->regs[R_DMA_DRAM_ADDR];
>>
>>> +    }
>>> +
>>> +    return dma_dram_addr;
>>> +}
>>> +
> Thanks for your suggestion. Will fix.
>>>    static uint32_t aspeed_smc_dma_len(AspeedSMCState *s)
>>>    {
>>>        AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s); @@ -914,24
>>> +944,34 @@ static void aspeed_smc_dma_checksum(AspeedSMCState *s)
>>>
>>>    static void aspeed_smc_dma_rw(AspeedSMCState *s)
>>>    {
>>> +    AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
>>> +    uint64_t dram_addr_high;
>>
>> This variable doesn't look very useful
> Will try to remove it.
>>
>>> +    uint64_t dma_dram_addr;
>>> +    uint64_t dram_addr;
>>
>> and dram_addr is redundant with dma_dram_addr. Please use only one.
> Please see my below description and please give us any suggestion.
>>
>>
>>>        MemTxResult result;
>>>        uint32_t dma_len;
>>>        uint32_t data;
>>>
>>>        dma_len = aspeed_smc_dma_len(s);
>>> +    dma_dram_addr = aspeed_smc_dma_dram_addr(s);
>>> +
>>> +    if (aspeed_smc_has_dma_dram_addr_high(asc)) {
>>> +        dram_addr = dma_dram_addr - s->dram_mr->container->addr;
>>
>> Why do you truncate the address again ? It should already be done with
>>
>> #define DMA_DRAM_ADDR_HIGH(val)   ((val) & 0xf)
>>
> The reason is that our firmware set the real address in SMC registers.
> For example: If users want to move data from flash to the DRAM at address 0,
> It set R_DMA_DRAM_ADDR_HIGH 4 and R_DMA_DRAM_ADDR 0 because the
> dram base address is 0x 4 00000000 for AST2700.


Could you please share the specs of the R_DMA_DRAM_ADDR and
R_DMA_DRAM_ADDR_HIGH registers to see what are the init values,
how these registers should be set, their alignment constraints
if any, how the values evolve while the HW does the DMA transactions,
etc.

Thanks,

C.

Jamin Lin April 30, 2024, 7:56 a.m. UTC | #4

Hi Cedric,

> -----Original Message-----
> From: Cédric Le Goater <[e-mail]>
> Sent: Tuesday, April 30, 2024 3:26 PM
> To: Jamin Lin <[e-mail]>; Peter Maydell
> <[e-mail]>; Andrew Jeffery <[e-mail]>;
> Joel Stanley <[e-mail]>; Alistair Francis <[e-mail]>; Cleber
> Rosa <[e-mail]>; Philippe Mathieu-Daudé <[e-mail]>;
> Wainer dos Santos Moschetta <[e-mail]>; Beraldo Leal
> <[e-mail]>; open list:ASPEED BMCs <[e-mail]>; open
> list:All patches CC here <[e-mail]>
> Cc: Troy Lee <[e-mail]>; Yunlin Tang
> <[e-mail]>
> 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 <[e-mail]>
> >>> Signed-off-by: Jamin Lin <[e-mail]>
> >>> ---
> >>>    hw/ssi/aspeed_smc.c | 66
> >> +++++++++++++++++++++++++++++++++++++++------
> >>>    hw/ssi/trace-events |  2 +-
> >>>    2 files changed, 59 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c index
> >>> 71abc7a2d8..a67cac3d0f 100644
> >>> --- a/hw/ssi/aspeed_smc.c
> >>> +++ b/hw/ssi/aspeed_smc.c
> >>> @@ -132,6 +132,9 @@
> >>>    #define   FMC_WDT2_CTRL_BOOT_SOURCE      BIT(4) /* O:
> primary
> >> 1: alternate */
> >>>    #define   FMC_WDT2_CTRL_EN               BIT(0)
> >>>
> >>> +/* DMA DRAM Side Address High Part (AST2700) */
> >>> +#define R_DMA_DRAM_ADDR_HIGH   (0x7c / 4)
> >>> +
> >>>    /* DMA Control/Status Register */
> >>>    #define R_DMA_CTRL        (0x80 / 4)
> >>>    #define   DMA_CTRL_REQUEST      (1 << 31)
> >>> @@ -187,6 +190,7 @@
> >>>     *   0x1FFFFFF: 32M bytes
> >>>     */
> >>>    #define DMA_DRAM_ADDR(asc, val)   ((val) &
> (asc)->dma_dram_mask)
> >>> +#define DMA_DRAM_ADDR_HIGH(val)   ((val) & 0xf)
> >>>    #define DMA_FLASH_ADDR(asc, val)  ((val) &
> (asc)->dma_flash_mask)
> >>>    #define DMA_LENGTH(val)         ((val) & 0x01FFFFFF)
> >>>
> >>> @@ -207,6 +211,7 @@ static const AspeedSegments
> >> aspeed_2500_spi2_segments[];
> >>>    #define ASPEED_SMC_FEATURE_DMA       0x1
> >>>    #define ASPEED_SMC_FEATURE_DMA_GRANT 0x2
> >>>    #define ASPEED_SMC_FEATURE_WDT_CONTROL 0x4
> >>> +#define ASPEED_SMC_FEATURE_DMA_DRAM_ADDR_HIGH 0x08
> >>>
> >>>    static inline bool aspeed_smc_has_dma(const AspeedSMCClass *asc)
> >>>    {
> >>> @@ -218,6 +223,11 @@ static inline bool
> >> aspeed_smc_has_wdt_control(const AspeedSMCClass *asc)
> >>>        return !!(asc->features &
> ASPEED_SMC_FEATURE_WDT_CONTROL);
> >>>    }
> >>>
> >>> +static inline bool aspeed_smc_has_dma_dram_addr_high(const
> >>> +AspeedSMCClass *asc)
> >>
> >> To ease the reading, I would call the helper aspeed_smc_has_dma64()
> > Will fix it
> >>
> >>> +{
> >>> +    return !!(asc->features &
> >> ASPEED_SMC_FEATURE_DMA_DRAM_ADDR_HIGH);
> >>> +}
> >>> +
> >>>    #define aspeed_smc_error(fmt, ...)
> >> \
> >>>        qemu_log_mask(LOG_GUEST_ERROR, "%s: " fmt "\n", __func__,
> ##
> >>> __VA_ARGS__)
> >>>
> >>> @@ -747,6 +757,9 @@ static uint64_t aspeed_smc_read(void *opaque,
> >> hwaddr addr, unsigned int size)
> >>>            (aspeed_smc_has_dma(asc) && addr == R_DMA_CTRL) ||
> >>>            (aspeed_smc_has_dma(asc) && addr ==
> R_DMA_FLASH_ADDR)
> >> ||
> >>>            (aspeed_smc_has_dma(asc) && addr ==
> R_DMA_DRAM_ADDR)
> >> ||
> >>> +        (aspeed_smc_has_dma(asc) &&
> >>> +         aspeed_smc_has_dma_dram_addr_high(asc) &&
> >>> +         addr == R_DMA_DRAM_ADDR_HIGH) ||
> >>>            (aspeed_smc_has_dma(asc) && addr == R_DMA_LEN) ||
> >>>            (aspeed_smc_has_dma(asc) && addr ==
> R_DMA_CHECKSUM)
> >> ||
> >>>            (addr >= R_SEG_ADDR0 &&
> >>> @@ -847,6 +860,23 @@ static bool
> >> aspeed_smc_inject_read_failure(AspeedSMCState *s)
> >>>        }
> >>>    }
> >>>
> >>> +static uint64_t aspeed_smc_dma_dram_addr(AspeedSMCState *s) {
> >>> +    AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
> >>> +    uint64_t dram_addr_high;
> >>> +    uint64_t dma_dram_addr;
> >>> +
> >>> +    if (aspeed_smc_has_dma_dram_addr_high(asc)) {
> >>> +        dram_addr_high = s->regs[R_DMA_DRAM_ADDR_HIGH];
> >>> +        dram_addr_high <<= 32;
> >>> +        dma_dram_addr = dram_addr_high |
> >> s->regs[R_DMA_DRAM_ADDR];
> >>
> >> Here is a proposal to shorten the routine :
> >>
> >>           return ((uint64_t) s->regs[R_DMA_DRAM_ADDR_HIGH] << 32)
> |
> >>               s->regs[R_DMA_DRAM_ADDR];
> >>
> >>
> >>> +    } else {
> >>> +        dma_dram_addr = s->regs[R_DMA_DRAM_ADDR];
> >>
> >> and
> >>           return s->regs[R_DMA_DRAM_ADDR];
> >>
> >>> +    }
> >>> +
> >>> +    return dma_dram_addr;
> >>> +}
> >>> +
> > Thanks for your suggestion. Will fix.
> >>>    static uint32_t aspeed_smc_dma_len(AspeedSMCState *s)
> >>>    {
> >>>        AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s); @@
> -914,24
> >>> +944,34 @@ static void aspeed_smc_dma_checksum(AspeedSMCState *s)
> >>>
> >>>    static void aspeed_smc_dma_rw(AspeedSMCState *s)
> >>>    {
> >>> +    AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
> >>> +    uint64_t dram_addr_high;
> >>
> >> This variable doesn't look very useful
> > Will try to remove it.
> >>
> >>> +    uint64_t dma_dram_addr;
> >>> +    uint64_t dram_addr;
> >>
> >> and dram_addr is redundant with dma_dram_addr. Please use only one.
> > Please see my below description and please give us any suggestion.
> >>
> >>
> >>>        MemTxResult result;
> >>>        uint32_t dma_len;
> >>>        uint32_t data;
> >>>
> >>>        dma_len = aspeed_smc_dma_len(s);
> >>> +    dma_dram_addr = aspeed_smc_dma_dram_addr(s);
> >>> +
> >>> +    if (aspeed_smc_has_dma_dram_addr_high(asc)) {
> >>> +        dram_addr = dma_dram_addr - s->dram_mr->container->addr;
> >>
> >> Why do you truncate the address again ? It should already be done
> >> with
> >>
> >> #define DMA_DRAM_ADDR_HIGH(val)   ((val) & 0xf)
> >>
> > The reason is that our firmware set the real address in SMC registers.
> > For example: If users want to move data from flash to the DRAM at
> > address 0, It set R_DMA_DRAM_ADDR_HIGH 4 and R_DMA_DRAM_ADDR 0
> because
> > the dram base address is 0x 4 00000000 for AST2700.
> 
> 
> Could you please share the specs of the R_DMA_DRAM_ADDR and
> R_DMA_DRAM_ADDR_HIGH registers to see what are the init values, how
> these registers should be set, their alignment constraints if any, how the values
> evolve while the HW does the DMA transactions, etc.
> 
DMA_DRAM_SIDE_ADDRESS 0x088 Init=0x00000000
31:0 RW DMA_RO
DRAM side start address
For DMA Read flash, this the destination address.
For DMA Write flash, the is the source address
DMA only execute on 4 bytes boundary
When read, it shows the current working address

DMA DRAM Side Address High Part 0x07c Init=0x00000000
32:8 RO reserved
7:0 RW DMA_RO_HI
      dma_ro[39:32]

Therefore, DMA address is dma_ro[39:0]

Our FW settings, 
In u-boot shell, execute the following command
cp.b 100220000 403000000 100
100220000 flash address for kernel fit image
403000000 dram address at offset 3000000
https://github.com/AspeedTech-BMC/u-boot/blob/aspeed-master-v2023.10/lib/string.c#L553C8-L553C29
https://github.com/AspeedTech-BMC/u-boot/blob/aspeed-master-v2023.10/arch/arm/mach-aspeed/ast2700/spi.c#L156 
https://github.com/AspeedTech-BMC/u-boot/blob/aspeed-master-v2023.10/arch/arm/mach-aspeed/ast2700/spi.c#L179-L183 
Thanks-Jamin

> Thanks,
> 
> C.
> 
>

Cédric Le Goater May 3, 2024, 2:20 p.m. UTC | #5

Hello Jamin,

On 4/30/24 09:56, Jamin Lin wrote:
> Hi Cedric,
> 
>> -----Original Message-----
>> From: Cédric Le Goater <[e-mail]>
>> Sent: Tuesday, April 30, 2024 3:26 PM
>> To: Jamin Lin <[e-mail]>; Peter Maydell
>> <[e-mail]>; Andrew Jeffery <[e-mail]>;
>> Joel Stanley <[e-mail]>; Alistair Francis <[e-mail]>; Cleber
>> Rosa <[e-mail]>; Philippe Mathieu-Daudé <[e-mail]>;
>> Wainer dos Santos Moschetta <[e-mail]>; Beraldo Leal
>> <[e-mail]>; open list:ASPEED BMCs <[e-mail]>; open
>> list:All patches CC here <[e-mail]>
>> Cc: Troy Lee <[e-mail]>; Yunlin Tang
>> <[e-mail]>
>> 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 <[e-mail]>
>>>>> Signed-off-by: Jamin Lin <[e-mail]>
>>>>> ---
>>>>>     hw/ssi/aspeed_smc.c | 66
>>>> +++++++++++++++++++++++++++++++++++++++------
>>>>>     hw/ssi/trace-events |  2 +-
>>>>>     2 files changed, 59 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c index
>>>>> 71abc7a2d8..a67cac3d0f 100644
>>>>> --- a/hw/ssi/aspeed_smc.c
>>>>> +++ b/hw/ssi/aspeed_smc.c
>>>>> @@ -132,6 +132,9 @@
>>>>>     #define   FMC_WDT2_CTRL_BOOT_SOURCE      BIT(4) /* O:
>> primary
>>>> 1: alternate */
>>>>>     #define   FMC_WDT2_CTRL_EN               BIT(0)
>>>>>
>>>>> +/* DMA DRAM Side Address High Part (AST2700) */
>>>>> +#define R_DMA_DRAM_ADDR_HIGH   (0x7c / 4)
>>>>> +
>>>>>     /* DMA Control/Status Register */
>>>>>     #define R_DMA_CTRL        (0x80 / 4)
>>>>>     #define   DMA_CTRL_REQUEST      (1 << 31)
>>>>> @@ -187,6 +190,7 @@
>>>>>      *   0x1FFFFFF: 32M bytes
>>>>>      */
>>>>>     #define DMA_DRAM_ADDR(asc, val)   ((val) &
>> (asc)->dma_dram_mask)
>>>>> +#define DMA_DRAM_ADDR_HIGH(val)   ((val) & 0xf)
>>>>>     #define DMA_FLASH_ADDR(asc, val)  ((val) &
>> (asc)->dma_flash_mask)
>>>>>     #define DMA_LENGTH(val)         ((val) & 0x01FFFFFF)
>>>>>
>>>>> @@ -207,6 +211,7 @@ static const AspeedSegments
>>>> aspeed_2500_spi2_segments[];
>>>>>     #define ASPEED_SMC_FEATURE_DMA       0x1
>>>>>     #define ASPEED_SMC_FEATURE_DMA_GRANT 0x2
>>>>>     #define ASPEED_SMC_FEATURE_WDT_CONTROL 0x4
>>>>> +#define ASPEED_SMC_FEATURE_DMA_DRAM_ADDR_HIGH 0x08
>>>>>
>>>>>     static inline bool aspeed_smc_has_dma(const AspeedSMCClass *asc)
>>>>>     {
>>>>> @@ -218,6 +223,11 @@ static inline bool
>>>> aspeed_smc_has_wdt_control(const AspeedSMCClass *asc)
>>>>>         return !!(asc->features &
>> ASPEED_SMC_FEATURE_WDT_CONTROL);
>>>>>     }
>>>>>
>>>>> +static inline bool aspeed_smc_has_dma_dram_addr_high(const
>>>>> +AspeedSMCClass *asc)
>>>>
>>>> To ease the reading, I would call the helper aspeed_smc_has_dma64()
>>> Will fix it
>>>>
>>>>> +{
>>>>> +    return !!(asc->features &
>>>> ASPEED_SMC_FEATURE_DMA_DRAM_ADDR_HIGH);
>>>>> +}
>>>>> +
>>>>>     #define aspeed_smc_error(fmt, ...)
>>>> \
>>>>>         qemu_log_mask(LOG_GUEST_ERROR, "%s: " fmt "\n", __func__,
>> ##
>>>>> __VA_ARGS__)
>>>>>
>>>>> @@ -747,6 +757,9 @@ static uint64_t aspeed_smc_read(void *opaque,
>>>> hwaddr addr, unsigned int size)
>>>>>             (aspeed_smc_has_dma(asc) && addr == R_DMA_CTRL) ||
>>>>>             (aspeed_smc_has_dma(asc) && addr ==
>> R_DMA_FLASH_ADDR)
>>>> ||
>>>>>             (aspeed_smc_has_dma(asc) && addr ==
>> R_DMA_DRAM_ADDR)
>>>> ||
>>>>> +        (aspeed_smc_has_dma(asc) &&
>>>>> +         aspeed_smc_has_dma_dram_addr_high(asc) &&
>>>>> +         addr == R_DMA_DRAM_ADDR_HIGH) ||
>>>>>             (aspeed_smc_has_dma(asc) && addr == R_DMA_LEN) ||
>>>>>             (aspeed_smc_has_dma(asc) && addr ==
>> R_DMA_CHECKSUM)
>>>> ||
>>>>>             (addr >= R_SEG_ADDR0 &&
>>>>> @@ -847,6 +860,23 @@ static bool
>>>> aspeed_smc_inject_read_failure(AspeedSMCState *s)
>>>>>         }
>>>>>     }
>>>>>
>>>>> +static uint64_t aspeed_smc_dma_dram_addr(AspeedSMCState *s) {
>>>>> +    AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
>>>>> +    uint64_t dram_addr_high;
>>>>> +    uint64_t dma_dram_addr;
>>>>> +
>>>>> +    if (aspeed_smc_has_dma_dram_addr_high(asc)) {
>>>>> +        dram_addr_high = s->regs[R_DMA_DRAM_ADDR_HIGH];
>>>>> +        dram_addr_high <<= 32;
>>>>> +        dma_dram_addr = dram_addr_high |
>>>> s->regs[R_DMA_DRAM_ADDR];
>>>>
>>>> Here is a proposal to shorten the routine :
>>>>
>>>>            return ((uint64_t) s->regs[R_DMA_DRAM_ADDR_HIGH] << 32)
>> |
>>>>                s->regs[R_DMA_DRAM_ADDR];
>>>>
>>>>
>>>>> +    } else {
>>>>> +        dma_dram_addr = s->regs[R_DMA_DRAM_ADDR];
>>>>
>>>> and
>>>>            return s->regs[R_DMA_DRAM_ADDR];
>>>>
>>>>> +    }
>>>>> +
>>>>> +    return dma_dram_addr;
>>>>> +}
>>>>> +
>>> Thanks for your suggestion. Will fix.
>>>>>     static uint32_t aspeed_smc_dma_len(AspeedSMCState *s)
>>>>>     {
>>>>>         AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s); @@
>> -914,24
>>>>> +944,34 @@ static void aspeed_smc_dma_checksum(AspeedSMCState *s)
>>>>>
>>>>>     static void aspeed_smc_dma_rw(AspeedSMCState *s)
>>>>>     {
>>>>> +    AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
>>>>> +    uint64_t dram_addr_high;
>>>>
>>>> This variable doesn't look very useful
>>> Will try to remove it.
>>>>
>>>>> +    uint64_t dma_dram_addr;
>>>>> +    uint64_t dram_addr;
>>>>
>>>> and dram_addr is redundant with dma_dram_addr. Please use only one.
>>> Please see my below description and please give us any suggestion.
>>>>
>>>>
>>>>>         MemTxResult result;
>>>>>         uint32_t dma_len;
>>>>>         uint32_t data;
>>>>>
>>>>>         dma_len = aspeed_smc_dma_len(s);
>>>>> +    dma_dram_addr = aspeed_smc_dma_dram_addr(s);
>>>>> +
>>>>> +    if (aspeed_smc_has_dma_dram_addr_high(asc)) {
>>>>> +        dram_addr = dma_dram_addr - s->dram_mr->container->addr;
>>>>
>>>> Why do you truncate the address again ? It should already be done
>>>> with
>>>>
>>>> #define DMA_DRAM_ADDR_HIGH(val)   ((val) & 0xf)
>>>>
>>> The reason is that our firmware set the real address in SMC registers.
>>> For example: If users want to move data from flash to the DRAM at
>>> address 0, It set R_DMA_DRAM_ADDR_HIGH 4 and R_DMA_DRAM_ADDR 0
>> because
>>> the dram base address is 0x 4 00000000 for AST2700.
>>
>>
>> Could you please share the specs of the R_DMA_DRAM_ADDR and
>> R_DMA_DRAM_ADDR_HIGH registers to see what are the init values, how
>> these registers should be set, their alignment constraints if any, how the values
>> evolve while the HW does the DMA transactions, etc.
>>
> DMA_DRAM_SIDE_ADDRESS 0x088 Init=0x00000000
> 31:0 RW DMA_RO
> DRAM side start address
> For DMA Read flash, this the destination address.
> For DMA Write flash, the is the source address
> DMA only execute on 4 bytes boundary
> When read, it shows the current working address
> 
> DMA DRAM Side Address High Part 0x07c Init=0x00000000
> 32:8 RO reserved
> 7:0 RW DMA_RO_HI
>        dma_ro[39:32]
> 
> Therefore, DMA address is dma_ro[39:0]

Thanks for the info. It seems that there are no alignment constraints.
I am surprised that there are no limits for the length in the specs.
It's 32MB right ?
  
Previous SoCs hardwire the high bits of the address where DRAM is mapped
in the DMA DRAM address reg. This is why the DRAM container was introduced,
so that HW units doing DMAs, like SMC, didn't have to manipulate real
physical addresses and offsets could be used instead. AST2700 HW interface
is different.

We can't use 's->dram_mr->container->addr' directly and I would like to
avoid passing 'sc->memmap[ASPEED_DEV_SDRAM]' as a property to the model.
Also, I was hoping that we didn't have to update the high part of the
address in the 0x07c reg. It seems we have to.

  
> 
> Our FW settings,
> In u-boot shell, execute the following command
> cp.b 100220000 403000000 100
> 100220000 flash address for kernel fit image
> 403000000 dram address at offset 3000000
> https://github.com/AspeedTech-BMC/u-boot/blob/aspeed-master-v2023.10/lib/string.c#L553C8-L553C29
> https://github.com/AspeedTech-BMC/u-boot/blob/aspeed-master-v2023.10/arch/arm/mach-aspeed/ast2700/spi.c#L156

So, U-Boot simply hardcodes the DRAM high bits

			if (dma_dest >= ASPEED_DRAM_BASE)
				writel(0x4, (void *)DRAM_HI_ADDR);

The fallback plan would simply be to ignore the high bits of the DMA
DRAM address. It should work today. Let me think about it more.

Thanks,

C.




> https://github.com/AspeedTech-BMC/u-boot/blob/aspeed-master-v2023.10/arch/arm/mach-aspeed/ast2700/spi.c#L179-L183
> Thanks-Jamin
> 
>> Thanks,
>>
>> C.
>>
>>
>

Cédric Le Goater May 7, 2024, 2:19 p.m. UTC | #6

Hello Jamin,

To handle the DMA DRAM Side Address High register, we should reintroduce
an "dram-base" property which I removed a while ago. Something like :



diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
index 7f32e43ff6f3..6d8ef6bc968f 100644
--- a/include/hw/ssi/aspeed_smc.h
+++ b/include/hw/ssi/aspeed_smc.h
@@ -76,6 +76,7 @@ struct AspeedSMCState {
      AddressSpace flash_as;
      MemoryRegion *dram_mr;
      AddressSpace dram_as;
+    uint64_t     dram_base;
  
      AddressSpace wdt2_as;
      MemoryRegion *wdt2_mr;
diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
index 38858e4fdec1..3417949ad8a3 100644
--- a/hw/arm/aspeed_ast27x0.c
+++ b/hw/arm/aspeed_ast27x0.c
@@ -500,6 +500,8 @@ static void aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp)
      }
  
      /* FMC, The number of CS is set at the board level */
+    object_property_set_int(OBJECT(&s->fmc), "dram-base",
+                            sc->memmap[ASPEED_DEV_SDRAM], &error_abort);
      object_property_set_link(OBJECT(&s->fmc), "dram", OBJECT(s->dram_mr),
                               &error_abort);
      if (!sysbus_realize(SYS_BUS_DEVICE(&s->fmc), errp)) {
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 3fa783578e9e..29ebfc0fd8c8 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -1372,6 +1372,7 @@ static const VMStateDescription vmstate_aspeed_smc = {
  
  static Property aspeed_smc_properties[] = {
      DEFINE_PROP_BOOL("inject-failure", AspeedSMCState, inject_failure, false),
+    DEFINE_PROP_UINT64("dram-base", AspeedSMCState, dram_base, 0),
      DEFINE_PROP_LINK("dram", AspeedSMCState, dram_mr,
                       TYPE_MEMORY_REGION, MemoryRegion *),
      DEFINE_PROP_LINK("wdt2", AspeedSMCState, wdt2_mr,



With that, see below for more comments,

On 4/16/24 11:18, Jamin Lin wrote:
> AST2700 support the maximum dram size is 8GiB
> and has a "DMA DRAM Side Address High Part(0x7C)"
> register to support 64 bits dma dram address.
> Add helper routines functions to compute the dma dram
> address, new features and update trace-event
> to support 64 bits dram address.
> 
> Signed-off-by: Troy Lee <[e-mail]>
> Signed-off-by: Jamin Lin <[e-mail]>
> ---
>   hw/ssi/aspeed_smc.c | 66 +++++++++++++++++++++++++++++++++++++++------
>   hw/ssi/trace-events |  2 +-
>   2 files changed, 59 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index 71abc7a2d8..a67cac3d0f 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -132,6 +132,9 @@
>   #define   FMC_WDT2_CTRL_BOOT_SOURCE      BIT(4) /* O: primary 1: alternate */
>   #define   FMC_WDT2_CTRL_EN               BIT(0)
>   
> +/* DMA DRAM Side Address High Part (AST2700) */
> +#define R_DMA_DRAM_ADDR_HIGH   (0x7c / 4)
> +
>   /* DMA Control/Status Register */
>   #define R_DMA_CTRL        (0x80 / 4)
>   #define   DMA_CTRL_REQUEST      (1 << 31)
> @@ -187,6 +190,7 @@
>    *   0x1FFFFFF: 32M bytes
>    */
>   #define DMA_DRAM_ADDR(asc, val)   ((val) & (asc)->dma_dram_mask)
> +#define DMA_DRAM_ADDR_HIGH(val)   ((val) & 0xf)
>   #define DMA_FLASH_ADDR(asc, val)  ((val) & (asc)->dma_flash_mask)
>   #define DMA_LENGTH(val)         ((val) & 0x01FFFFFF)
>   
> @@ -207,6 +211,7 @@ static const AspeedSegments aspeed_2500_spi2_segments[];
>   #define ASPEED_SMC_FEATURE_DMA       0x1
>   #define ASPEED_SMC_FEATURE_DMA_GRANT 0x2
>   #define ASPEED_SMC_FEATURE_WDT_CONTROL 0x4
> +#define ASPEED_SMC_FEATURE_DMA_DRAM_ADDR_HIGH 0x08
>   
>   static inline bool aspeed_smc_has_dma(const AspeedSMCClass *asc)
>   {
> @@ -218,6 +223,11 @@ static inline bool aspeed_smc_has_wdt_control(const AspeedSMCClass *asc)
>       return !!(asc->features & ASPEED_SMC_FEATURE_WDT_CONTROL);
>   }
>   
> +static inline bool aspeed_smc_has_dma_dram_addr_high(const AspeedSMCClass *asc)
> +{
> +    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;

dram_addr_high can be removed

> +    uint64_t dma_dram_addr;
> +    uint64_t dram_addr;

please call this variable dma_dram_offset, I found it less confusing.

>       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;
> +    }
>

With the new "dram-base" property, the above if can be replaced by :

     dma_dram_offset = dma_dram_addr - s->dram_base;


Thanks,

C.


>       trace_aspeed_smc_dma_rw(s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE ?
>                               "write" : "read",
>                               s->regs[R_DMA_FLASH_ADDR],
> -                            s->regs[R_DMA_DRAM_ADDR],
> +                            dram_addr,
>                               dma_len);
>       while (dma_len) {
>           if (s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE) {
> -            data = address_space_ldl_le(&s->dram_as, s->regs[R_DMA_DRAM_ADDR],
> +            data = address_space_ldl_le(&s->dram_as, dram_addr,
>                                           MEMTXATTRS_UNSPECIFIED, &result);
>               if (result != MEMTX_OK) {
> -                aspeed_smc_error("DRAM read failed @%08x",
> -                                 s->regs[R_DMA_DRAM_ADDR]);
> +                aspeed_smc_error("DRAM read failed @%" PRIx64, dram_addr);
>                   return;
>               }
>   
> @@ -951,11 +991,10 @@ static void aspeed_smc_dma_rw(AspeedSMCState *s)
>                   return;
>               }
>   
> -            address_space_stl_le(&s->dram_as, s->regs[R_DMA_DRAM_ADDR],
> +            address_space_stl_le(&s->dram_as, dram_addr,
>                                    data, MEMTXATTRS_UNSPECIFIED, &result);
>               if (result != MEMTX_OK) {
> -                aspeed_smc_error("DRAM write failed @%08x",
> -                                 s->regs[R_DMA_DRAM_ADDR]);
> +                aspeed_smc_error("DRAM write failed @%" PRIx64, dram_addr);
>                   return;
>               }
>           }
> @@ -964,8 +1003,15 @@ static void aspeed_smc_dma_rw(AspeedSMCState *s)
>            * When the DMA is on-going, the DMA registers are updated
>            * with the current working addresses and length.
>            */
> +        dram_addr += 4;
> +        dma_dram_addr += 4;
> +        if (aspeed_smc_has_dma_dram_addr_high(asc)) {
> +            dram_addr_high = dma_dram_addr >> 32;
> +            s->regs[R_DMA_DRAM_ADDR_HIGH] = dram_addr_high;
> +        }
> +
> +        s->regs[R_DMA_DRAM_ADDR] = dma_dram_addr & 0xffffffff;
>           s->regs[R_DMA_FLASH_ADDR] += 4;
> -        s->regs[R_DMA_DRAM_ADDR] += 4;
>           dma_len -= 4;
>           s->regs[R_DMA_LEN] = dma_len;
>           s->regs[R_DMA_CHECKSUM] += data;
> @@ -1118,6 +1164,10 @@ static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
>       } else if (aspeed_smc_has_dma(asc) && addr == R_DMA_LEN &&
>                  aspeed_smc_dma_granted(s)) {
>           s->regs[addr] = DMA_LENGTH(value);
> +    } else if (aspeed_smc_has_dma(asc) &&
> +               aspeed_smc_has_dma_dram_addr_high(asc) &&
> +               addr == R_DMA_DRAM_ADDR_HIGH) {
> +        s->regs[addr] = DMA_DRAM_ADDR_HIGH(value);
>       } else {
>           qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx "\n",
>                         __func__, addr);
> diff --git a/hw/ssi/trace-events b/hw/ssi/trace-events
> index 2d5bd2b83d..7b5ad6a939 100644
> --- a/hw/ssi/trace-events
> +++ b/hw/ssi/trace-events
> @@ -6,7 +6,7 @@ aspeed_smc_do_snoop(int cs, int index, int dummies, int data) "CS%d index:0x%x d
>   aspeed_smc_flash_write(int cs, uint64_t addr,  uint32_t size, uint64_t data, int mode) "CS%d @0x%" PRIx64 " size %u: 0x%" PRIx64" mode:%d"
>   aspeed_smc_read(uint64_t addr,  uint32_t size, uint64_t data) "@0x%" PRIx64 " size %u: 0x%" PRIx64
>   aspeed_smc_dma_checksum(uint32_t addr, uint32_t data) "0x%08x: 0x%08x"
> -aspeed_smc_dma_rw(const char *dir, uint32_t flash_addr, uint32_t dram_addr, uint32_t size) "%s flash:@0x%08x dram:@0x%08x size:0x%08x"
> +aspeed_smc_dma_rw(const char *dir, uint32_t flash_addr, uint64_t dram_addr, uint32_t size) "%s flash:@0x%08x dram:@0x%" PRIx64 " size:0x%08x"
>   aspeed_smc_write(uint64_t addr,  uint32_t size, uint64_t data) "@0x%" PRIx64 " size %u: 0x%" PRIx64
>   aspeed_smc_flash_select(int cs, const char *prefix) "CS%d %sselect"
>

Jamin Lin May 15, 2024, 8:56 a.m. UTC | #7

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 <[e-mail]>
> >> Sent: Tuesday, April 30, 2024 3:26 PM
> >> To: Jamin Lin <[e-mail]>; Peter Maydell
> >> <[e-mail]>; Andrew Jeffery
> >> <[e-mail]>; Joel Stanley <[e-mail]>;
> >> Alistair Francis <[e-mail]>; Cleber Rosa
> >> <[e-mail]>; Philippe Mathieu-Daudé <[e-mail]>;
> >> Wainer dos Santos Moschetta <[e-mail]>; Beraldo Leal
> >> <[e-mail]>; open list:ASPEED BMCs <[e-mail]>;
> open
> >> list:All patches CC here <[e-mail]>
> >> Cc: Troy Lee <[e-mail]>; Yunlin Tang
> >> <[e-mail]>
> >> 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 <[e-mail]>
> >>>>> Signed-off-by: Jamin Lin <[e-mail]>
> >>>>> ---
> >>>>>     hw/ssi/aspeed_smc.c | 66
> >>>> +++++++++++++++++++++++++++++++++++++++------
> >>>>>     hw/ssi/trace-events |  2 +-
> >>>>>     2 files changed, 59 insertions(+), 9 deletions(-)
> >>>>>
> >>>>> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c index
> >>>>> 71abc7a2d8..a67cac3d0f 100644
> >>>>> --- a/hw/ssi/aspeed_smc.c
> >>>>> +++ b/hw/ssi/aspeed_smc.c
> >>>>> @@ -132,6 +132,9 @@
> >>>>>     #define   FMC_WDT2_CTRL_BOOT_SOURCE      BIT(4) /* O:
> >> primary
> >>>> 1: alternate */
> >>>>>     #define   FMC_WDT2_CTRL_EN               BIT(0)
> >>>>>
> >>>>> +/* DMA DRAM Side Address High Part (AST2700) */
> >>>>> +#define R_DMA_DRAM_ADDR_HIGH   (0x7c / 4)
> >>>>> +
> >>>>>     /* DMA Control/Status Register */
> >>>>>     #define R_DMA_CTRL        (0x80 / 4)
> >>>>>     #define   DMA_CTRL_REQUEST      (1 << 31)
> >>>>> @@ -187,6 +190,7 @@
> >>>>>      *   0x1FFFFFF: 32M bytes
> >>>>>      */
> >>>>>     #define DMA_DRAM_ADDR(asc, val)   ((val) &
> >> (asc)->dma_dram_mask)
> >>>>> +#define DMA_DRAM_ADDR_HIGH(val)   ((val) & 0xf)
> >>>>>     #define DMA_FLASH_ADDR(asc, val)  ((val) &
> >> (asc)->dma_flash_mask)
> >>>>>     #define DMA_LENGTH(val)         ((val) & 0x01FFFFFF)
> >>>>>
> >>>>> @@ -207,6 +211,7 @@ static const AspeedSegments
> >>>> aspeed_2500_spi2_segments[];
> >>>>>     #define ASPEED_SMC_FEATURE_DMA       0x1
> >>>>>     #define ASPEED_SMC_FEATURE_DMA_GRANT 0x2
> >>>>>     #define ASPEED_SMC_FEATURE_WDT_CONTROL 0x4
> >>>>> +#define ASPEED_SMC_FEATURE_DMA_DRAM_ADDR_HIGH 0x08
> >>>>>
> >>>>>     static inline bool aspeed_smc_has_dma(const AspeedSMCClass
> *asc)
> >>>>>     {
> >>>>> @@ -218,6 +223,11 @@ static inline bool
> >>>> aspeed_smc_has_wdt_control(const AspeedSMCClass *asc)
> >>>>>         return !!(asc->features &
> >> ASPEED_SMC_FEATURE_WDT_CONTROL);
> >>>>>     }
> >>>>>
> >>>>> +static inline bool aspeed_smc_has_dma_dram_addr_high(const
> >>>>> +AspeedSMCClass *asc)
> >>>>
> >>>> To ease the reading, I would call the helper aspeed_smc_has_dma64()
> >>> Will fix it
> >>>>
> >>>>> +{
> >>>>> +    return !!(asc->features &
> >>>> ASPEED_SMC_FEATURE_DMA_DRAM_ADDR_HIGH);
> >>>>> +}
> >>>>> +
> >>>>>     #define aspeed_smc_error(fmt, ...)
> >>>> \
> >>>>>         qemu_log_mask(LOG_GUEST_ERROR, "%s: " fmt "\n",
> __func__,
> >> ##
> >>>>> __VA_ARGS__)
> >>>>>
> >>>>> @@ -747,6 +757,9 @@ static uint64_t aspeed_smc_read(void *opaque,
> >>>> hwaddr addr, unsigned int size)
> >>>>>             (aspeed_smc_has_dma(asc) && addr == R_DMA_CTRL)
> ||
> >>>>>             (aspeed_smc_has_dma(asc) && addr ==
> >> R_DMA_FLASH_ADDR)
> >>>> ||
> >>>>>             (aspeed_smc_has_dma(asc) && addr ==
> >> R_DMA_DRAM_ADDR)
> >>>> ||
> >>>>> +        (aspeed_smc_has_dma(asc) &&
> >>>>> +         aspeed_smc_has_dma_dram_addr_high(asc) &&
> >>>>> +         addr == R_DMA_DRAM_ADDR_HIGH) ||
> >>>>>             (aspeed_smc_has_dma(asc) && addr == R_DMA_LEN)
> ||
> >>>>>             (aspeed_smc_has_dma(asc) && addr ==
> >> R_DMA_CHECKSUM)
> >>>> ||
> >>>>>             (addr >= R_SEG_ADDR0 && @@ -847,6 +860,23 @@
> static
> >>>>> bool
> >>>> aspeed_smc_inject_read_failure(AspeedSMCState *s)
> >>>>>         }
> >>>>>     }
> >>>>>
> >>>>> +static uint64_t aspeed_smc_dma_dram_addr(AspeedSMCState *s) {
> >>>>> +    AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
> >>>>> +    uint64_t dram_addr_high;
> >>>>> +    uint64_t dma_dram_addr;
> >>>>> +
> >>>>> +    if (aspeed_smc_has_dma_dram_addr_high(asc)) {
> >>>>> +        dram_addr_high = s->regs[R_DMA_DRAM_ADDR_HIGH];
> >>>>> +        dram_addr_high <<= 32;
> >>>>> +        dma_dram_addr = dram_addr_high |
> >>>> s->regs[R_DMA_DRAM_ADDR];
> >>>>
> >>>> Here is a proposal to shorten the routine :
> >>>>
> >>>>            return ((uint64_t) s->regs[R_DMA_DRAM_ADDR_HIGH] <<
> 32)
> >> |
> >>>>                s->regs[R_DMA_DRAM_ADDR];
> >>>>
> >>>>
> >>>>> +    } else {
> >>>>> +        dma_dram_addr = s->regs[R_DMA_DRAM_ADDR];
> >>>>
> >>>> and
> >>>>            return s->regs[R_DMA_DRAM_ADDR];
> >>>>
> >>>>> +    }
> >>>>> +
> >>>>> +    return dma_dram_addr;
> >>>>> +}
> >>>>> +
> >>> Thanks for your suggestion. Will fix.
> >>>>>     static uint32_t aspeed_smc_dma_len(AspeedSMCState *s)
> >>>>>     {
> >>>>>         AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s); @@
> >> -914,24
> >>>>> +944,34 @@ static void aspeed_smc_dma_checksum(AspeedSMCState
> *s)
> >>>>>
> >>>>>     static void aspeed_smc_dma_rw(AspeedSMCState *s)
> >>>>>     {
> >>>>> +    AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
> >>>>> +    uint64_t dram_addr_high;
> >>>>
> >>>> This variable doesn't look very useful
> >>> Will try to remove it.
> >>>>
> >>>>> +    uint64_t dma_dram_addr;
> >>>>> +    uint64_t dram_addr;
> >>>>
> >>>> and dram_addr is redundant with dma_dram_addr. Please use only one.
> >>> Please see my below description and please give us any suggestion.
> >>>>
> >>>>
> >>>>>         MemTxResult result;
> >>>>>         uint32_t dma_len;
> >>>>>         uint32_t data;
> >>>>>
> >>>>>         dma_len = aspeed_smc_dma_len(s);
> >>>>> +    dma_dram_addr = aspeed_smc_dma_dram_addr(s);
> >>>>> +
> >>>>> +    if (aspeed_smc_has_dma_dram_addr_high(asc)) {
> >>>>> +        dram_addr = dma_dram_addr -
> s->dram_mr->container->addr;
> >>>>
> >>>> Why do you truncate the address again ? It should already be done
> >>>> with
> >>>>
> >>>> #define DMA_DRAM_ADDR_HIGH(val)   ((val) & 0xf)
> >>>>
> >>> The reason is that our firmware set the real address in SMC registers.
> >>> For example: If users want to move data from flash to the DRAM at
> >>> address 0, It set R_DMA_DRAM_ADDR_HIGH 4 and R_DMA_DRAM_ADDR
> 0
> >> because
> >>> the dram base address is 0x 4 00000000 for AST2700.
> >>
> >>
> >> Could you please share the specs of the R_DMA_DRAM_ADDR and
> >> R_DMA_DRAM_ADDR_HIGH registers to see what are the init values, how
> >> these registers should be set, their alignment constraints if any,
> >> how the values evolve while the HW does the DMA transactions, etc.
> >>
> > DMA_DRAM_SIDE_ADDRESS 0x088 Init=0x00000000
> > 31:0 RW DMA_RO
> > DRAM side start address
> > For DMA Read flash, this the destination address.
> > For DMA Write flash, the is the source address DMA only execute on 4
> > bytes boundary When read, it shows the current working address
> >
> > DMA DRAM Side Address High Part 0x07c Init=0x00000000
> > 32:8 RO reserved
> > 7:0 RW DMA_RO_HI
> >        dma_ro[39:32]
> >
> > Therefore, DMA address is dma_ro[39:0]
> 
> Thanks for the info. It seems that there are no alignment constraints.
> I am surprised that there are no limits for the length in the specs.
> It's 32MB right ?
> 
This register is used to set the dram address. AST2700 support the maximum dram size is 8GiB.
If dram base address is 0x400000000, the address range of 8GiB size would be 0x400000000 - 0x600000000
32bits only support dram size 4GiB. That was why we need this High Part register to support 8GIB size.
Yes, the maximum moving data length for flash is 32MB and is set by SPI8C DMA LENGTH Register.

> Previous SoCs hardwire the high bits of the address where DRAM is mapped in
> the DMA DRAM address reg. This is why the DRAM container was introduced,
> so that HW units doing DMAs, like SMC, didn't have to manipulate real physical
> addresses and offsets could be used instead. AST2700 HW interface is
> different.
> 
> We can't use 's->dram_mr->container->addr' directly and I would like to avoid
> passing 'sc->memmap[ASPEED_DEV_SDRAM]' as a property to the model.
> Also, I was hoping that we didn't have to update the high part of the address in
> the 0x07c reg. It seems we have to.
> 
> 
> >
> > Our FW settings,
> > In u-boot shell, execute the following command cp.b 100220000
> > 403000000 100
> > 100220000 flash address for kernel fit image
> > 403000000 dram address at offset 3000000
> > https://github.com/AspeedTech-BMC/u-boot/blob/aspeed-master-v2023.10/l
> > ib/string.c#L553C8-L553C29
> >
> https://github.com/AspeedTech-BMC/u-boot/blob/aspeed-master-v2023.10/a
> > rch/arm/mach-aspeed/ast2700/spi.c#L156
> 
> So, U-Boot simply hardcodes the DRAM high bits
> 
> 			if (dma_dest >= ASPEED_DRAM_BASE)
> 				writel(0x4, (void *)DRAM_HI_ADDR);
> 
After I checked with our SPI deriver designers, we confirmed it was a bug.
Example: If users use 8GIB dram size, the address range of 8GiB size would be 0x400000000 - 0x600000000
cp.b 100220000 503000000 101 --> This command would move the date at dram address 403000000 rather than 503000000
Our u-boot firmware will fix it. Thanks for report it.
Jamin

> The fallback plan would simply be to ignore the high bits of the DMA DRAM
> address. It should work today. Let me think about it more.
> 
> Thanks,
> 
> C.
> 
> 
> 
> 
> >
> https://github.com/AspeedTech-BMC/u-boot/blob/aspeed-master-v2023.10/a
> > rch/arm/mach-aspeed/ast2700/spi.c#L179-L183
> > Thanks-Jamin
> >
> >> Thanks,
> >>
> >> C.
> >>
> >>
> >

Jamin Lin May 15, 2024, 9:01 a.m. UTC | #8

Hi Cedric,

Sorry reply you late.
> Hello Jamin,
> 
> To handle the DMA DRAM Side Address High register, we should reintroduce an
> "dram-base" property which I removed a while ago. Something like :
> 
> 
> 
> diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h index
> 7f32e43ff6f3..6d8ef6bc968f 100644
> --- a/include/hw/ssi/aspeed_smc.h
> +++ b/include/hw/ssi/aspeed_smc.h
> @@ -76,6 +76,7 @@ struct AspeedSMCState {
>       AddressSpace flash_as;
>       MemoryRegion *dram_mr;
>       AddressSpace dram_as;
> +    uint64_t     dram_base;
> 
>       AddressSpace wdt2_as;
>       MemoryRegion *wdt2_mr;
> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c index
> 38858e4fdec1..3417949ad8a3 100644
> --- a/hw/arm/aspeed_ast27x0.c
> +++ b/hw/arm/aspeed_ast27x0.c
> @@ -500,6 +500,8 @@ static void aspeed_soc_ast2700_realize(DeviceState
> *dev, Error **errp)
>       }
> 
>       /* FMC, The number of CS is set at the board level */
> +    object_property_set_int(OBJECT(&s->fmc), "dram-base",
> +                            sc->memmap[ASPEED_DEV_SDRAM],
> + &error_abort);
>       object_property_set_link(OBJECT(&s->fmc), "dram",
> OBJECT(s->dram_mr),
>                                &error_abort);
>       if (!sysbus_realize(SYS_BUS_DEVICE(&s->fmc), errp)) { diff --git
> a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c index
> 3fa783578e9e..29ebfc0fd8c8 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -1372,6 +1372,7 @@ static const VMStateDescription
> vmstate_aspeed_smc = {
> 
>   static Property aspeed_smc_properties[] = {
>       DEFINE_PROP_BOOL("inject-failure", AspeedSMCState, inject_failure,
> false),
> +    DEFINE_PROP_UINT64("dram-base", AspeedSMCState, dram_base, 0),
>       DEFINE_PROP_LINK("dram", AspeedSMCState, dram_mr,
>                        TYPE_MEMORY_REGION, MemoryRegion *),
>       DEFINE_PROP_LINK("wdt2", AspeedSMCState, wdt2_mr,
> 
> 
I appreciate your kindly support and thanks for your suggestion.
Will add it. 

> 
> With that, see below for more comments,
> 
> On 4/16/24 11:18, Jamin Lin wrote:
> > AST2700 support the maximum dram size is 8GiB and has a "DMA DRAM
> Side
> > Address High Part(0x7C)"
> > register to support 64 bits dma dram address.
> > Add helper routines functions to compute the dma dram address, new
> > features and update trace-event to support 64 bits dram address.
> >
> > Signed-off-by: Troy Lee <[e-mail]>
> > Signed-off-by: Jamin Lin <[e-mail]>
> > ---
> >   hw/ssi/aspeed_smc.c | 66
> +++++++++++++++++++++++++++++++++++++++------
> >   hw/ssi/trace-events |  2 +-
> >   2 files changed, 59 insertions(+), 9 deletions(-)
> >
> > diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c index
> > 71abc7a2d8..a67cac3d0f 100644
> > --- a/hw/ssi/aspeed_smc.c
> > +++ b/hw/ssi/aspeed_smc.c
> > @@ -132,6 +132,9 @@
> >   #define   FMC_WDT2_CTRL_BOOT_SOURCE      BIT(4) /* O: primary
> 1: alternate */
> >   #define   FMC_WDT2_CTRL_EN               BIT(0)
> >
> > +/* DMA DRAM Side Address High Part (AST2700) */
> > +#define R_DMA_DRAM_ADDR_HIGH   (0x7c / 4)
> > +
> >   /* DMA Control/Status Register */
> >   #define R_DMA_CTRL        (0x80 / 4)
> >   #define   DMA_CTRL_REQUEST      (1 << 31)
> > @@ -187,6 +190,7 @@
> >    *   0x1FFFFFF: 32M bytes
> >    */
> >   #define DMA_DRAM_ADDR(asc, val)   ((val) & (asc)->dma_dram_mask)
> > +#define DMA_DRAM_ADDR_HIGH(val)   ((val) & 0xf)
> >   #define DMA_FLASH_ADDR(asc, val)  ((val) & (asc)->dma_flash_mask)
> >   #define DMA_LENGTH(val)         ((val) & 0x01FFFFFF)
> >
> > @@ -207,6 +211,7 @@ static const AspeedSegments
> aspeed_2500_spi2_segments[];
> >   #define ASPEED_SMC_FEATURE_DMA       0x1
> >   #define ASPEED_SMC_FEATURE_DMA_GRANT 0x2
> >   #define ASPEED_SMC_FEATURE_WDT_CONTROL 0x4
> > +#define ASPEED_SMC_FEATURE_DMA_DRAM_ADDR_HIGH 0x08
> >
> >   static inline bool aspeed_smc_has_dma(const AspeedSMCClass *asc)
> >   {
> > @@ -218,6 +223,11 @@ static inline bool
> aspeed_smc_has_wdt_control(const AspeedSMCClass *asc)
> >       return !!(asc->features & ASPEED_SMC_FEATURE_WDT_CONTROL);
> >   }
> >
> > +static inline bool aspeed_smc_has_dma_dram_addr_high(const
> > +AspeedSMCClass *asc) {
> > +    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;
> 
> dram_addr_high can be removed
> 
> > +    uint64_t dma_dram_addr;
> > +    uint64_t dram_addr;
> 
> please call this variable dma_dram_offset, I found it less confusing.
> 
> >       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;
> > +    }
> >
> 
> With the new "dram-base" property, the above if can be replaced by :
> 
>      dma_dram_offset = dma_dram_addr - s->dram_base;
> 
> 
Again, I appreciate your kindly support and thanks for your suggestion.
Will update them.

> Thanks,
> 
> C.
> 
> 
> >       trace_aspeed_smc_dma_rw(s->regs[R_DMA_CTRL] &
> DMA_CTRL_WRITE ?
> >                               "write" : "read",
> >                               s->regs[R_DMA_FLASH_ADDR],
> > -                            s->regs[R_DMA_DRAM_ADDR],
> > +                            dram_addr,
> >                               dma_len);
> >       while (dma_len) {
> >           if (s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE) {
> > -            data = address_space_ldl_le(&s->dram_as,
> s->regs[R_DMA_DRAM_ADDR],
> > +            data = address_space_ldl_le(&s->dram_as, dram_addr,
> >
> MEMTXATTRS_UNSPECIFIED, &result);
> >               if (result != MEMTX_OK) {
> > -                aspeed_smc_error("DRAM read failed @%08x",
> > -                                 s->regs[R_DMA_DRAM_ADDR]);
> > +                aspeed_smc_error("DRAM read failed @%" PRIx64,
> > + dram_addr);
> >                   return;
> >               }
> >
> > @@ -951,11 +991,10 @@ static void aspeed_smc_dma_rw(AspeedSMCState
> *s)
> >                   return;
> >               }
> >
> > -            address_space_stl_le(&s->dram_as,
> s->regs[R_DMA_DRAM_ADDR],
> > +            address_space_stl_le(&s->dram_as, dram_addr,
> >                                    data,
> MEMTXATTRS_UNSPECIFIED, &result);
> >               if (result != MEMTX_OK) {
> > -                aspeed_smc_error("DRAM write failed @%08x",
> > -                                 s->regs[R_DMA_DRAM_ADDR]);
> > +                aspeed_smc_error("DRAM write failed @%" PRIx64,
> > + dram_addr);
> >                   return;
> >               }
> >           }
> > @@ -964,8 +1003,15 @@ static void aspeed_smc_dma_rw(AspeedSMCState
> *s)
> >            * When the DMA is on-going, the DMA registers are updated
> >            * with the current working addresses and length.
> >            */
> > +        dram_addr += 4;
> > +        dma_dram_addr += 4;
> > +        if (aspeed_smc_has_dma_dram_addr_high(asc)) {
> > +            dram_addr_high = dma_dram_addr >> 32;
> > +            s->regs[R_DMA_DRAM_ADDR_HIGH] = dram_addr_high;
> > +        }
> > +
> > +        s->regs[R_DMA_DRAM_ADDR] = dma_dram_addr & 0xffffffff;
> >           s->regs[R_DMA_FLASH_ADDR] += 4;
> > -        s->regs[R_DMA_DRAM_ADDR] += 4;
> >           dma_len -= 4;
> >           s->regs[R_DMA_LEN] = dma_len;
> >           s->regs[R_DMA_CHECKSUM] += data; @@ -1118,6 +1164,10
> @@
> > static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
> >       } else if (aspeed_smc_has_dma(asc) && addr == R_DMA_LEN &&
> >                  aspeed_smc_dma_granted(s)) {
> >           s->regs[addr] = DMA_LENGTH(value);
> > +    } else if (aspeed_smc_has_dma(asc) &&
> > +               aspeed_smc_has_dma_dram_addr_high(asc) &&
> > +               addr == R_DMA_DRAM_ADDR_HIGH) {
> > +        s->regs[addr] = DMA_DRAM_ADDR_HIGH(value);
> >       } else {
> >           qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%"
> HWADDR_PRIx "\n",
> >                         __func__, addr); diff --git
> > a/hw/ssi/trace-events b/hw/ssi/trace-events index
> > 2d5bd2b83d..7b5ad6a939 100644
> > --- a/hw/ssi/trace-events
> > +++ b/hw/ssi/trace-events
> > @@ -6,7 +6,7 @@ aspeed_smc_do_snoop(int cs, int index, int dummies, int
> data) "CS%d index:0x%x d
> >   aspeed_smc_flash_write(int cs, uint64_t addr,  uint32_t size, uint64_t
> data, int mode) "CS%d @0x%" PRIx64 " size %u: 0x%" PRIx64" mode:%d"
> >   aspeed_smc_read(uint64_t addr,  uint32_t size, uint64_t data) "@0x%"
> PRIx64 " size %u: 0x%" PRIx64
> >   aspeed_smc_dma_checksum(uint32_t addr, uint32_t data) "0x%08x:
> 0x%08x"
> > -aspeed_smc_dma_rw(const char *dir, uint32_t flash_addr, uint32_t
> dram_addr, uint32_t size) "%s flash:@0x%08x dram:@0x%08x size:0x%08x"
> > +aspeed_smc_dma_rw(const char *dir, uint32_t flash_addr, uint64_t
> dram_addr, uint32_t size) "%s flash:@0x%08x dram:@0x%" PRIx64 "
> size:0x%08x"
> >   aspeed_smc_write(uint64_t addr,  uint32_t size, uint64_t data) "@0x%"
> PRIx64 " size %u: 0x%" PRIx64
> >   aspeed_smc_flash_select(int cs, const char *prefix) "CS%d %sselect"
> >

Cédric Le Goater May 17, 2024, 7:57 a.m. UTC | #9

Hello Jamin

On 5/15/24 11:01, Jamin Lin wrote:
> Hi Cedric,
> 
> Sorry reply you late.
>> Hello Jamin,
>>
>> To handle the DMA DRAM Side Address High register, we should reintroduce an
>> "dram-base" property which I removed a while ago. Something like :
>>
>>
>>
>> diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h index
>> 7f32e43ff6f3..6d8ef6bc968f 100644
>> --- a/include/hw/ssi/aspeed_smc.h
>> +++ b/include/hw/ssi/aspeed_smc.h
>> @@ -76,6 +76,7 @@ struct AspeedSMCState {
>>        AddressSpace flash_as;
>>        MemoryRegion *dram_mr;
>>        AddressSpace dram_as;
>> +    uint64_t     dram_base;
>>
>>        AddressSpace wdt2_as;
>>        MemoryRegion *wdt2_mr;
>> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c index
>> 38858e4fdec1..3417949ad8a3 100644
>> --- a/hw/arm/aspeed_ast27x0.c
>> +++ b/hw/arm/aspeed_ast27x0.c
>> @@ -500,6 +500,8 @@ static void aspeed_soc_ast2700_realize(DeviceState
>> *dev, Error **errp)
>>        }
>>
>>        /* FMC, The number of CS is set at the board level */
>> +    object_property_set_int(OBJECT(&s->fmc), "dram-base",
>> +                            sc->memmap[ASPEED_DEV_SDRAM],
>> + &error_abort);
>>        object_property_set_link(OBJECT(&s->fmc), "dram",
>> OBJECT(s->dram_mr),
>>                                 &error_abort);
>>        if (!sysbus_realize(SYS_BUS_DEVICE(&s->fmc), errp)) { diff --git
>> a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c index
>> 3fa783578e9e..29ebfc0fd8c8 100644
>> --- a/hw/ssi/aspeed_smc.c
>> +++ b/hw/ssi/aspeed_smc.c
>> @@ -1372,6 +1372,7 @@ static const VMStateDescription
>> vmstate_aspeed_smc = {
>>
>>    static Property aspeed_smc_properties[] = {
>>        DEFINE_PROP_BOOL("inject-failure", AspeedSMCState, inject_failure,
>> false),
>> +    DEFINE_PROP_UINT64("dram-base", AspeedSMCState, dram_base, 0),
>>        DEFINE_PROP_LINK("dram", AspeedSMCState, dram_mr,
>>                         TYPE_MEMORY_REGION, MemoryRegion *),
>>        DEFINE_PROP_LINK("wdt2", AspeedSMCState, wdt2_mr,
>>
>>
> I appreciate your kindly support and thanks for your suggestion.
> Will add it.

See my aspeed-9.1 branch, I did some changes, mostly in the last patch.

* aspeed_smc_dma_len()

   - can use QEMU_ALIGN_UP(). simpler.

* aspeed_smc_dma_rw():

   - dram_addr ->  dma_dram_offset
   - There is no need to protect updates of the R_DMA_DRAM_ADDR_HIGH
     register with aspeed_smc_has_dma_dram_addr_high() since it is
     already protected with MMIO accesses. Skip the check and update
     always.

* aspeed_smc_dma_dram_addr()

   - same as above.

You can merge the changes in the respective patches if you agree.

Still on the TODO list :

   - GIC review
   - aspeed/soc: fix incorrect dram size for AST2700
   



Thanks,

C.



> 
>>
>> With that, see below for more comments,
>>
>> On 4/16/24 11:18, Jamin Lin wrote:
>>> AST2700 support the maximum dram size is 8GiB and has a "DMA DRAM
>> Side
>>> Address High Part(0x7C)"
>>> register to support 64 bits dma dram address.
>>> Add helper routines functions to compute the dma dram address, new
>>> features and update trace-event to support 64 bits dram address.
>>>
>>> Signed-off-by: Troy Lee <[e-mail]>
>>> Signed-off-by: Jamin Lin <[e-mail]>
>>> ---
>>>    hw/ssi/aspeed_smc.c | 66
>> +++++++++++++++++++++++++++++++++++++++------
>>>    hw/ssi/trace-events |  2 +-
>>>    2 files changed, 59 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c index
>>> 71abc7a2d8..a67cac3d0f 100644
>>> --- a/hw/ssi/aspeed_smc.c
>>> +++ b/hw/ssi/aspeed_smc.c
>>> @@ -132,6 +132,9 @@
>>>    #define   FMC_WDT2_CTRL_BOOT_SOURCE      BIT(4) /* O: primary
>> 1: alternate */
>>>    #define   FMC_WDT2_CTRL_EN               BIT(0)
>>>
>>> +/* DMA DRAM Side Address High Part (AST2700) */
>>> +#define R_DMA_DRAM_ADDR_HIGH   (0x7c / 4)
>>> +
>>>    /* DMA Control/Status Register */
>>>    #define R_DMA_CTRL        (0x80 / 4)
>>>    #define   DMA_CTRL_REQUEST      (1 << 31)
>>> @@ -187,6 +190,7 @@
>>>     *   0x1FFFFFF: 32M bytes
>>>     */
>>>    #define DMA_DRAM_ADDR(asc, val)   ((val) & (asc)->dma_dram_mask)
>>> +#define DMA_DRAM_ADDR_HIGH(val)   ((val) & 0xf)
>>>    #define DMA_FLASH_ADDR(asc, val)  ((val) & (asc)->dma_flash_mask)
>>>    #define DMA_LENGTH(val)         ((val) & 0x01FFFFFF)
>>>
>>> @@ -207,6 +211,7 @@ static const AspeedSegments
>> aspeed_2500_spi2_segments[];
>>>    #define ASPEED_SMC_FEATURE_DMA       0x1
>>>    #define ASPEED_SMC_FEATURE_DMA_GRANT 0x2
>>>    #define ASPEED_SMC_FEATURE_WDT_CONTROL 0x4
>>> +#define ASPEED_SMC_FEATURE_DMA_DRAM_ADDR_HIGH 0x08
>>>
>>>    static inline bool aspeed_smc_has_dma(const AspeedSMCClass *asc)
>>>    {
>>> @@ -218,6 +223,11 @@ static inline bool
>> aspeed_smc_has_wdt_control(const AspeedSMCClass *asc)
>>>        return !!(asc->features & ASPEED_SMC_FEATURE_WDT_CONTROL);
>>>    }
>>>
>>> +static inline bool aspeed_smc_has_dma_dram_addr_high(const
>>> +AspeedSMCClass *asc) {
>>> +    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;
>>
>> dram_addr_high can be removed
>>
>>> +    uint64_t dma_dram_addr;
>>> +    uint64_t dram_addr;
>>
>> please call this variable dma_dram_offset, I found it less confusing.
>>
>>>        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;
>>> +    }
>>>
>>
>> With the new "dram-base" property, the above if can be replaced by :
>>
>>       dma_dram_offset = dma_dram_addr - s->dram_base;
>>
>>
> Again, I appreciate your kindly support and thanks for your suggestion.
> Will update them.
> 
>> Thanks,
>>
>> C.
>>
>>
>>>        trace_aspeed_smc_dma_rw(s->regs[R_DMA_CTRL] &
>> DMA_CTRL_WRITE ?
>>>                                "write" : "read",
>>>                                s->regs[R_DMA_FLASH_ADDR],
>>> -                            s->regs[R_DMA_DRAM_ADDR],
>>> +                            dram_addr,
>>>                                dma_len);
>>>        while (dma_len) {
>>>            if (s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE) {
>>> -            data = address_space_ldl_le(&s->dram_as,
>> s->regs[R_DMA_DRAM_ADDR],
>>> +            data = address_space_ldl_le(&s->dram_as, dram_addr,
>>>
>> MEMTXATTRS_UNSPECIFIED, &result);
>>>                if (result != MEMTX_OK) {
>>> -                aspeed_smc_error("DRAM read failed @%08x",
>>> -                                 s->regs[R_DMA_DRAM_ADDR]);
>>> +                aspeed_smc_error("DRAM read failed @%" PRIx64,
>>> + dram_addr);
>>>                    return;
>>>                }
>>>
>>> @@ -951,11 +991,10 @@ static void aspeed_smc_dma_rw(AspeedSMCState
>> *s)
>>>                    return;
>>>                }
>>>
>>> -            address_space_stl_le(&s->dram_as,
>> s->regs[R_DMA_DRAM_ADDR],
>>> +            address_space_stl_le(&s->dram_as, dram_addr,
>>>                                     data,
>> MEMTXATTRS_UNSPECIFIED, &result);
>>>                if (result != MEMTX_OK) {
>>> -                aspeed_smc_error("DRAM write failed @%08x",
>>> -                                 s->regs[R_DMA_DRAM_ADDR]);
>>> +                aspeed_smc_error("DRAM write failed @%" PRIx64,
>>> + dram_addr);
>>>                    return;
>>>                }
>>>            }
>>> @@ -964,8 +1003,15 @@ static void aspeed_smc_dma_rw(AspeedSMCState
>> *s)
>>>             * When the DMA is on-going, the DMA registers are updated
>>>             * with the current working addresses and length.
>>>             */
>>> +        dram_addr += 4;
>>> +        dma_dram_addr += 4;
>>> +        if (aspeed_smc_has_dma_dram_addr_high(asc)) {
>>> +            dram_addr_high = dma_dram_addr >> 32;
>>> +            s->regs[R_DMA_DRAM_ADDR_HIGH] = dram_addr_high;
>>> +        }
>>> +
>>> +        s->regs[R_DMA_DRAM_ADDR] = dma_dram_addr & 0xffffffff;
>>>            s->regs[R_DMA_FLASH_ADDR] += 4;
>>> -        s->regs[R_DMA_DRAM_ADDR] += 4;
>>>            dma_len -= 4;
>>>            s->regs[R_DMA_LEN] = dma_len;
>>>            s->regs[R_DMA_CHECKSUM] += data; @@ -1118,6 +1164,10
>> @@
>>> static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
>>>        } else if (aspeed_smc_has_dma(asc) && addr == R_DMA_LEN &&
>>>                   aspeed_smc_dma_granted(s)) {
>>>            s->regs[addr] = DMA_LENGTH(value);
>>> +    } else if (aspeed_smc_has_dma(asc) &&
>>> +               aspeed_smc_has_dma_dram_addr_high(asc) &&
>>> +               addr == R_DMA_DRAM_ADDR_HIGH) {
>>> +        s->regs[addr] = DMA_DRAM_ADDR_HIGH(value);
>>>        } else {
>>>            qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%"
>> HWADDR_PRIx "\n",
>>>                          __func__, addr); diff --git
>>> a/hw/ssi/trace-events b/hw/ssi/trace-events index
>>> 2d5bd2b83d..7b5ad6a939 100644
>>> --- a/hw/ssi/trace-events
>>> +++ b/hw/ssi/trace-events
>>> @@ -6,7 +6,7 @@ aspeed_smc_do_snoop(int cs, int index, int dummies, int
>> data) "CS%d index:0x%x d
>>>    aspeed_smc_flash_write(int cs, uint64_t addr,  uint32_t size, uint64_t
>> data, int mode) "CS%d @0x%" PRIx64 " size %u: 0x%" PRIx64" mode:%d"
>>>    aspeed_smc_read(uint64_t addr,  uint32_t size, uint64_t data) "@0x%"
>> PRIx64 " size %u: 0x%" PRIx64
>>>    aspeed_smc_dma_checksum(uint32_t addr, uint32_t data) "0x%08x:
>> 0x%08x"
>>> -aspeed_smc_dma_rw(const char *dir, uint32_t flash_addr, uint32_t
>> dram_addr, uint32_t size) "%s flash:@0x%08x dram:@0x%08x size:0x%08x"
>>> +aspeed_smc_dma_rw(const char *dir, uint32_t flash_addr, uint64_t
>> dram_addr, uint32_t size) "%s flash:@0x%08x dram:@0x%" PRIx64 "
>> size:0x%08x"
>>>    aspeed_smc_write(uint64_t addr,  uint32_t size, uint64_t data) "@0x%"
>> PRIx64 " size %u: 0x%" PRIx64
>>>    aspeed_smc_flash_select(int cs, const char *prefix) "CS%d %sselect"
>>>
>

Jamin Lin May 20, 2024, 2:40 a.m. UTC | #10

Hi Cedric,

> 
> Hello Jamin
> 
> On 5/15/24 11:01, Jamin Lin wrote:
> > Hi Cedric,
> >
> > Sorry reply you late.
> >> Hello Jamin,
> >>
> >> To handle the DMA DRAM Side Address High register, we should
> >> reintroduce an "dram-base" property which I removed a while ago.
> Something like :
> >>
> >>
> >>
> >> diff --git a/include/hw/ssi/aspeed_smc.h
> >> b/include/hw/ssi/aspeed_smc.h index 7f32e43ff6f3..6d8ef6bc968f 100644
> >> --- a/include/hw/ssi/aspeed_smc.h
> >> +++ b/include/hw/ssi/aspeed_smc.h
> >> @@ -76,6 +76,7 @@ struct AspeedSMCState {
> >>        AddressSpace flash_as;
> >>        MemoryRegion *dram_mr;
> >>        AddressSpace dram_as;
> >> +    uint64_t     dram_base;
> >>
> >>        AddressSpace wdt2_as;
> >>        MemoryRegion *wdt2_mr;
> >> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c index
> >> 38858e4fdec1..3417949ad8a3 100644
> >> --- a/hw/arm/aspeed_ast27x0.c
> >> +++ b/hw/arm/aspeed_ast27x0.c
> >> @@ -500,6 +500,8 @@ static void
> >> aspeed_soc_ast2700_realize(DeviceState
> >> *dev, Error **errp)
> >>        }
> >>
> >>        /* FMC, The number of CS is set at the board level */
> >> +    object_property_set_int(OBJECT(&s->fmc), "dram-base",
> >> +                            sc->memmap[ASPEED_DEV_SDRAM],
> >> + &error_abort);
> >>        object_property_set_link(OBJECT(&s->fmc), "dram",
> >> OBJECT(s->dram_mr),
> >>                                 &error_abort);
> >>        if (!sysbus_realize(SYS_BUS_DEVICE(&s->fmc), errp)) { diff
> >> --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c index
> >> 3fa783578e9e..29ebfc0fd8c8 100644
> >> --- a/hw/ssi/aspeed_smc.c
> >> +++ b/hw/ssi/aspeed_smc.c
> >> @@ -1372,6 +1372,7 @@ static const VMStateDescription
> >> vmstate_aspeed_smc = {
> >>
> >>    static Property aspeed_smc_properties[] = {
> >>        DEFINE_PROP_BOOL("inject-failure", AspeedSMCState,
> >> inject_failure, false),
> >> +    DEFINE_PROP_UINT64("dram-base", AspeedSMCState, dram_base,
> 0),
> >>        DEFINE_PROP_LINK("dram", AspeedSMCState, dram_mr,
> >>                         TYPE_MEMORY_REGION, MemoryRegion *),
> >>        DEFINE_PROP_LINK("wdt2", AspeedSMCState, wdt2_mr,
> >>
> >>
> > I appreciate your kindly support and thanks for your suggestion.
> > Will add it.
> 
> See my aspeed-9.1 branch, I did some changes, mostly in the last patch.
> 
> * aspeed_smc_dma_len()
> 
>    - can use QEMU_ALIGN_UP(). simpler.
> 
> * aspeed_smc_dma_rw():
> 
>    - dram_addr ->  dma_dram_offset
>    - There is no need to protect updates of the R_DMA_DRAM_ADDR_HIGH
>      register with aspeed_smc_has_dma_dram_addr_high() since it is
>      already protected with MMIO accesses. Skip the check and update
>      always.
> 
> * aspeed_smc_dma_dram_addr()
> 
>    - same as above.
> 
> You can merge the changes in the respective patches if you agree.
> 
> Still on the TODO list :
> 
>    - GIC review
>    - aspeed/soc: fix incorrect dram size for AST2700
> 
> 
> 
> 
> Thanks,
> 
> C.
> 
I merged this commit into my code base and thanks for your kindly support.
https://github.com/legoater/qemu/commit/d1bc2c776422a9d0d6af2b4414fae83fde1832ba 

About GIC settings, you can refer to our kernel DTS setting for detail.
https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi#L143-L164 

Thanks-Jamin

> 
> 
> >
> >>
> >> With that, see below for more comments,
> >>
> >> On 4/16/24 11:18, Jamin Lin wrote:
> >>> AST2700 support the maximum dram size is 8GiB and has a "DMA DRAM
> >> Side
> >>> Address High Part(0x7C)"
> >>> register to support 64 bits dma dram address.
> >>> Add helper routines functions to compute the dma dram address, new
> >>> features and update trace-event to support 64 bits dram address.
> >>>
> >>> Signed-off-by: Troy Lee <[e-mail]>
> >>> Signed-off-by: Jamin Lin <[e-mail]>
> >>> ---
> >>>    hw/ssi/aspeed_smc.c | 66
> >> +++++++++++++++++++++++++++++++++++++++------
> >>>    hw/ssi/trace-events |  2 +-
> >>>    2 files changed, 59 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c index
> >>> 71abc7a2d8..a67cac3d0f 100644
> >>> --- a/hw/ssi/aspeed_smc.c
> >>> +++ b/hw/ssi/aspeed_smc.c
> >>> @@ -132,6 +132,9 @@
> >>>    #define   FMC_WDT2_CTRL_BOOT_SOURCE      BIT(4) /* O:
> primary
> >> 1: alternate */
> >>>    #define   FMC_WDT2_CTRL_EN               BIT(0)
> >>>
> >>> +/* DMA DRAM Side Address High Part (AST2700) */
> >>> +#define R_DMA_DRAM_ADDR_HIGH   (0x7c / 4)
> >>> +
> >>>    /* DMA Control/Status Register */
> >>>    #define R_DMA_CTRL        (0x80 / 4)
> >>>    #define   DMA_CTRL_REQUEST      (1 << 31)
> >>> @@ -187,6 +190,7 @@
> >>>     *   0x1FFFFFF: 32M bytes
> >>>     */
> >>>    #define DMA_DRAM_ADDR(asc, val)   ((val) &
> (asc)->dma_dram_mask)
> >>> +#define DMA_DRAM_ADDR_HIGH(val)   ((val) & 0xf)
> >>>    #define DMA_FLASH_ADDR(asc, val)  ((val) &
> (asc)->dma_flash_mask)
> >>>    #define DMA_LENGTH(val)         ((val) & 0x01FFFFFF)
> >>>
> >>> @@ -207,6 +211,7 @@ static const AspeedSegments
> >> aspeed_2500_spi2_segments[];
> >>>    #define ASPEED_SMC_FEATURE_DMA       0x1
> >>>    #define ASPEED_SMC_FEATURE_DMA_GRANT 0x2
> >>>    #define ASPEED_SMC_FEATURE_WDT_CONTROL 0x4
> >>> +#define ASPEED_SMC_FEATURE_DMA_DRAM_ADDR_HIGH 0x08
> >>>
> >>>    static inline bool aspeed_smc_has_dma(const AspeedSMCClass *asc)
> >>>    {
> >>> @@ -218,6 +223,11 @@ static inline bool
> >> aspeed_smc_has_wdt_control(const AspeedSMCClass *asc)
> >>>        return !!(asc->features &
> ASPEED_SMC_FEATURE_WDT_CONTROL);
> >>>    }
> >>>
> >>> +static inline bool aspeed_smc_has_dma_dram_addr_high(const
> >>> +AspeedSMCClass *asc) {
> >>> +    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;
> >>
> >> dram_addr_high can be removed
> >>
> >>> +    uint64_t dma_dram_addr;
> >>> +    uint64_t dram_addr;
> >>
> >> please call this variable dma_dram_offset, I found it less confusing.
> >>
> >>>        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;
> >>> +    }
> >>>
> >>
> >> With the new "dram-base" property, the above if can be replaced by :
> >>
> >>       dma_dram_offset = dma_dram_addr - s->dram_base;
> >>
> >>
> > Again, I appreciate your kindly support and thanks for your suggestion.
> > Will update them.
> >
> >> Thanks,
> >>
> >> C.
> >>
> >>
> >>>        trace_aspeed_smc_dma_rw(s->regs[R_DMA_CTRL] &
> >> DMA_CTRL_WRITE ?
> >>>                                "write" : "read",
> >>>                                s->regs[R_DMA_FLASH_ADDR],
> >>> -                            s->regs[R_DMA_DRAM_ADDR],
> >>> +                            dram_addr,
> >>>                                dma_len);
> >>>        while (dma_len) {
> >>>            if (s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE) {
> >>> -            data = address_space_ldl_le(&s->dram_as,
> >> s->regs[R_DMA_DRAM_ADDR],
> >>> +            data = address_space_ldl_le(&s->dram_as, dram_addr,
> >>>
> >> MEMTXATTRS_UNSPECIFIED, &result);
> >>>                if (result != MEMTX_OK) {
> >>> -                aspeed_smc_error("DRAM read failed @%08x",
> >>> -                                 s->regs[R_DMA_DRAM_ADDR]);
> >>> +                aspeed_smc_error("DRAM read failed @%" PRIx64,
> >>> + dram_addr);
> >>>                    return;
> >>>                }
> >>>
> >>> @@ -951,11 +991,10 @@ static void
> aspeed_smc_dma_rw(AspeedSMCState
> >> *s)
> >>>                    return;
> >>>                }
> >>>
> >>> -            address_space_stl_le(&s->dram_as,
> >> s->regs[R_DMA_DRAM_ADDR],
> >>> +            address_space_stl_le(&s->dram_as, dram_addr,
> >>>                                     data,
> >> MEMTXATTRS_UNSPECIFIED, &result);
> >>>                if (result != MEMTX_OK) {
> >>> -                aspeed_smc_error("DRAM write failed @%08x",
> >>> -                                 s->regs[R_DMA_DRAM_ADDR]);
> >>> +                aspeed_smc_error("DRAM write failed @%" PRIx64,
> >>> + dram_addr);
> >>>                    return;
> >>>                }
> >>>            }
> >>> @@ -964,8 +1003,15 @@ static void
> aspeed_smc_dma_rw(AspeedSMCState
> >> *s)
> >>>             * When the DMA is on-going, the DMA registers are
> updated
> >>>             * with the current working addresses and length.
> >>>             */
> >>> +        dram_addr += 4;
> >>> +        dma_dram_addr += 4;
> >>> +        if (aspeed_smc_has_dma_dram_addr_high(asc)) {
> >>> +            dram_addr_high = dma_dram_addr >> 32;
> >>> +            s->regs[R_DMA_DRAM_ADDR_HIGH] = dram_addr_high;
> >>> +        }
> >>> +
> >>> +        s->regs[R_DMA_DRAM_ADDR] = dma_dram_addr & 0xffffffff;
> >>>            s->regs[R_DMA_FLASH_ADDR] += 4;
> >>> -        s->regs[R_DMA_DRAM_ADDR] += 4;
> >>>            dma_len -= 4;
> >>>            s->regs[R_DMA_LEN] = dma_len;
> >>>            s->regs[R_DMA_CHECKSUM] += data; @@ -1118,6
> +1164,10
> >> @@
> >>> static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
> >>>        } else if (aspeed_smc_has_dma(asc) && addr == R_DMA_LEN
> &&
> >>>                   aspeed_smc_dma_granted(s)) {
> >>>            s->regs[addr] = DMA_LENGTH(value);
> >>> +    } else if (aspeed_smc_has_dma(asc) &&
> >>> +               aspeed_smc_has_dma_dram_addr_high(asc) &&
> >>> +               addr == R_DMA_DRAM_ADDR_HIGH) {
> >>> +        s->regs[addr] = DMA_DRAM_ADDR_HIGH(value);
> >>>        } else {
> >>>            qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%"
> >> HWADDR_PRIx "\n",
> >>>                          __func__, addr); diff --git
> >>> a/hw/ssi/trace-events b/hw/ssi/trace-events index
> >>> 2d5bd2b83d..7b5ad6a939 100644
> >>> --- a/hw/ssi/trace-events
> >>> +++ b/hw/ssi/trace-events
> >>> @@ -6,7 +6,7 @@ aspeed_smc_do_snoop(int cs, int index, int dummies,
> >>> int
> >> data) "CS%d index:0x%x d
> >>>    aspeed_smc_flash_write(int cs, uint64_t addr,  uint32_t size,
> >>> uint64_t
> >> data, int mode) "CS%d @0x%" PRIx64 " size %u: 0x%" PRIx64" mode:%d"
> >>>    aspeed_smc_read(uint64_t addr,  uint32_t size, uint64_t data)
> "@0x%"
> >> PRIx64 " size %u: 0x%" PRIx64
> >>>    aspeed_smc_dma_checksum(uint32_t addr, uint32_t data) "0x%08x:
> >> 0x%08x"
> >>> -aspeed_smc_dma_rw(const char *dir, uint32_t flash_addr, uint32_t
> >> dram_addr, uint32_t size) "%s flash:@0x%08x dram:@0x%08x size:0x%08x"
> >>> +aspeed_smc_dma_rw(const char *dir, uint32_t flash_addr, uint64_t
> >> dram_addr, uint32_t size) "%s flash:@0x%08x dram:@0x%" PRIx64 "
> >> size:0x%08x"
> >>>    aspeed_smc_write(uint64_t addr,  uint32_t size, uint64_t data)
> "@0x%"
> >> PRIx64 " size %u: 0x%" PRIx64
> >>>    aspeed_smc_flash_select(int cs, const char *prefix) "CS%d %sselect"
> >>>
> >

Patch

diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index 71abc7a2d8..a67cac3d0f 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -132,6 +132,9 @@ 
 #define   FMC_WDT2_CTRL_BOOT_SOURCE      BIT(4) /* O: primary 1: alternate */
 #define   FMC_WDT2_CTRL_EN               BIT(0)
 
+/* DMA DRAM Side Address High Part (AST2700) */
+#define R_DMA_DRAM_ADDR_HIGH   (0x7c / 4)
+
 /* DMA Control/Status Register */
 #define R_DMA_CTRL        (0x80 / 4)
 #define   DMA_CTRL_REQUEST      (1 << 31)
@@ -187,6 +190,7 @@ 
  *   0x1FFFFFF: 32M bytes
  */
 #define DMA_DRAM_ADDR(asc, val)   ((val) & (asc)->dma_dram_mask)
+#define DMA_DRAM_ADDR_HIGH(val)   ((val) & 0xf)
 #define DMA_FLASH_ADDR(asc, val)  ((val) & (asc)->dma_flash_mask)
 #define DMA_LENGTH(val)         ((val) & 0x01FFFFFF)
 
@@ -207,6 +211,7 @@  static const AspeedSegments aspeed_2500_spi2_segments[];
 #define ASPEED_SMC_FEATURE_DMA       0x1
 #define ASPEED_SMC_FEATURE_DMA_GRANT 0x2
 #define ASPEED_SMC_FEATURE_WDT_CONTROL 0x4
+#define ASPEED_SMC_FEATURE_DMA_DRAM_ADDR_HIGH 0x08
 
 static inline bool aspeed_smc_has_dma(const AspeedSMCClass *asc)
 {
@@ -218,6 +223,11 @@  static inline bool aspeed_smc_has_wdt_control(const AspeedSMCClass *asc)
     return !!(asc->features & ASPEED_SMC_FEATURE_WDT_CONTROL);
 }
 
+static inline bool aspeed_smc_has_dma_dram_addr_high(const AspeedSMCClass *asc)
+{
+    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(&s->dram_as, s->regs[R_DMA_DRAM_ADDR],
+            data = address_space_ldl_le(&s->dram_as, dram_addr,
                                         MEMTXATTRS_UNSPECIFIED, &result);
             if (result != MEMTX_OK) {
-                aspeed_smc_error("DRAM read failed @%08x",
-                                 s->regs[R_DMA_DRAM_ADDR]);
+                aspeed_smc_error("DRAM read failed @%" PRIx64, dram_addr);
                 return;
             }
 
@@ -951,11 +991,10 @@  static void aspeed_smc_dma_rw(AspeedSMCState *s)
                 return;
             }
 
-            address_space_stl_le(&s->dram_as, s->regs[R_DMA_DRAM_ADDR],
+            address_space_stl_le(&s->dram_as, dram_addr,
                                  data, MEMTXATTRS_UNSPECIFIED, &result);
             if (result != MEMTX_OK) {
-                aspeed_smc_error("DRAM write failed @%08x",
-                                 s->regs[R_DMA_DRAM_ADDR]);
+                aspeed_smc_error("DRAM write failed @%" PRIx64, dram_addr);
                 return;
             }
         }
@@ -964,8 +1003,15 @@  static void aspeed_smc_dma_rw(AspeedSMCState *s)
          * When the DMA is on-going, the DMA registers are updated
          * with the current working addresses and length.
          */
+        dram_addr += 4;
+        dma_dram_addr += 4;
+        if (aspeed_smc_has_dma_dram_addr_high(asc)) {
+            dram_addr_high = dma_dram_addr >> 32;
+            s->regs[R_DMA_DRAM_ADDR_HIGH] = dram_addr_high;
+        }
+
+        s->regs[R_DMA_DRAM_ADDR] = dma_dram_addr & 0xffffffff;
         s->regs[R_DMA_FLASH_ADDR] += 4;
-        s->regs[R_DMA_DRAM_ADDR] += 4;
         dma_len -= 4;
         s->regs[R_DMA_LEN] = dma_len;
         s->regs[R_DMA_CHECKSUM] += data;
@@ -1118,6 +1164,10 @@  static void aspeed_smc_write(void *opaque, hwaddr addr, uint64_t data,
     } else if (aspeed_smc_has_dma(asc) && addr == R_DMA_LEN &&
                aspeed_smc_dma_granted(s)) {
         s->regs[addr] = DMA_LENGTH(value);
+    } else if (aspeed_smc_has_dma(asc) &&
+               aspeed_smc_has_dma_dram_addr_high(asc) &&
+               addr == R_DMA_DRAM_ADDR_HIGH) {
+        s->regs[addr] = DMA_DRAM_ADDR_HIGH(value);
     } else {
         qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx "\n",
                       __func__, addr);
diff --git a/hw/ssi/trace-events b/hw/ssi/trace-events
index 2d5bd2b83d..7b5ad6a939 100644
--- a/hw/ssi/trace-events
+++ b/hw/ssi/trace-events
@@ -6,7 +6,7 @@  aspeed_smc_do_snoop(int cs, int index, int dummies, int data) "CS%d index:0x%x d
 aspeed_smc_flash_write(int cs, uint64_t addr,  uint32_t size, uint64_t data, int mode) "CS%d @0x%" PRIx64 " size %u: 0x%" PRIx64" mode:%d"
 aspeed_smc_read(uint64_t addr,  uint32_t size, uint64_t data) "@0x%" PRIx64 " size %u: 0x%" PRIx64
 aspeed_smc_dma_checksum(uint32_t addr, uint32_t data) "0x%08x: 0x%08x"
-aspeed_smc_dma_rw(const char *dir, uint32_t flash_addr, uint32_t dram_addr, uint32_t size) "%s flash:@0x%08x dram:@0x%08x size:0x%08x"
+aspeed_smc_dma_rw(const char *dir, uint32_t flash_addr, uint64_t dram_addr, uint32_t size) "%s flash:@0x%08x dram:@0x%" PRIx64 " size:0x%08x"
 aspeed_smc_write(uint64_t addr,  uint32_t size, uint64_t data) "@0x%" PRIx64 " size %u: 0x%" PRIx64
 aspeed_smc_flash_select(int cs, const char *prefix) "CS%d %sselect"