Re: [PATCH v4 08/16] aspeed/smc: support 64 bits dma dram address

[Philippe Mathieu-Daudé]

Hi Jamin,

On 27/5/24 10:02, Jamin Lin wrote:
> AST2700 support the maximum dram size is 8GiB
> and has a "DMA DRAM Side Address High Part(0x7C)"
> register to support 64 bits dma dram address.
> Add helper routines functions to compute the dma dram
> address, new features and update trace-event
> to support 64 bits dram address.
>
> Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>   hw/ssi/aspeed_smc.c         | 52 +++++++++++++++++++++++++++++++------
>   hw/ssi/trace-events         |  2 +-
>   include/hw/ssi/aspeed_smc.h |  1 +
>   3 files changed, 46 insertions(+), 9 deletions(-)


> +static uint64_t aspeed_smc_dma_dram_addr(AspeedSMCState *s)
> +{
> +    return s->regs[R_DMA_DRAM_ADDR] |
> +        ((uint64_t) s->regs[R_DMA_DRAM_ADDR_HIGH] << 32);
> +}
> +
>   static uint32_t aspeed_smc_dma_len(AspeedSMCState *s)
>   {
>       AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
> @@ -903,24 +921,34 @@ static void aspeed_smc_dma_checksum(AspeedSMCState *s)
>   
>   static void aspeed_smc_dma_rw(AspeedSMCState *s)
>   {
> +    AspeedSMCClass *asc = ASPEED_SMC_GET_CLASS(s);
> +    uint64_t dma_dram_offset;
> +    uint64_t dma_dram_addr;
>       MemTxResult result;
>       uint32_t dma_len;
>       uint32_t data;
>   
>       dma_len = aspeed_smc_dma_len(s);
> +    dma_dram_addr = aspeed_smc_dma_dram_addr(s);
> +
> +    if (aspeed_smc_has_dma64(asc)) {
> +        dma_dram_offset = dma_dram_addr - s->dram_base;
> +    } else {
> +        dma_dram_offset = dma_dram_addr;

Here s->dram_base is 0x0. Do we really need to check
aspeed_smc_has_dma64?

> +    }

Maybe simplify improving aspeed_smc_dma_dram_addr() as:

  static uint64_t aspeed_smc_dma_dram_addr(AspeedSMCState *s)
  {
      return (s->regs[R_DMA_DRAM_ADDR]
              | ((uint64_t) s->regs[R_DMA_DRAM_ADDR_HIGH] << 32))
              - s->dram_base;
  }

Then no need for dma_dram_offset, dma_dram_addr is enough.

>   
>       trace_aspeed_smc_dma_rw(s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE ?
>                               "write" : "read",
>                               s->regs[R_DMA_FLASH_ADDR],
> -                            s->regs[R_DMA_DRAM_ADDR],
> +                            dma_dram_offset,
>                               dma_len);
>       while (dma_len) {
>           if (s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE) {
> -            data = address_space_ldl_le(&s->dram_as, s->regs[R_DMA_DRAM_ADDR],
> +            data = address_space_ldl_le(&s->dram_as, dma_dram_offset,
>                                           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,
> +                                 dma_dram_offset);
>                   return;
>               }
>   
> @@ -940,11 +968,11 @@ 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, dma_dram_offset,
>                                    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,
> +                                 dma_dram_offset);
>                   return;
>               }
>           }
> @@ -953,8 +981,12 @@ static void aspeed_smc_dma_rw(AspeedSMCState *s)
>            * When the DMA is on-going, the DMA registers are updated
>            * with the current working addresses and length.
>            */
> +        dma_dram_offset += 4;
> +        dma_dram_addr += 4;
> +
> +        s->regs[R_DMA_DRAM_ADDR_HIGH] = dma_dram_addr >> 32;
> +        s->regs[R_DMA_DRAM_ADDR] = dma_dram_addr & 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;
> @@ -1107,6 +1139,9 @@ static void aspeed_smc_write(void *opaque, hwaddr addr, 
> uint64_t data,
>       } else if (aspeed_smc_has_dma(asc) && addr == R_DMA_LEN &&
>                  aspeed_smc_dma_granted(s)) {
>           s->regs[addr] = DMA_LENGTH(value);
> +    } else if (aspeed_smc_has_dma(asc) && aspeed_smc_has_dma64(asc) &&
> +               addr == R_DMA_DRAM_ADDR_HIGH) {
> +        s->regs[addr] = DMA_DRAM_ADDR_HIGH(value);
>       } else {
>           qemu_log_mask(LOG_UNIMP, "%s: not implemented: 0x%" HWADDR_PRIx "\n",
>                         __func__, addr);
> @@ -1239,6 +1274,7 @@ static const VMStateDescription vmstate_aspeed_smc = {
>   
>   static Property aspeed_smc_properties[] = {
>       DEFINE_PROP_BOOL("inject-failure", AspeedSMCState, inject_failure, false),
> +    DEFINE_PROP_UINT64("dram-base", AspeedSMCState, dram_base, 0),
>       DEFINE_PROP_LINK("dram", AspeedSMCState, dram_mr,
>                        TYPE_MEMORY_REGION, MemoryRegion *),
>       DEFINE_PROP_END_OF_LIST(),