Re: [Qemu-arm] [PATCH v2 15/21] aspeed/smc: add support for DMAs

[Peter Maydell]

On Tue, 18 Jun 2019 at 17:55, Cédric Le Goater <address@hidden> wrote:
>
> The FMC controller on the Aspeed SoCs support DMA to access the flash
> modules. It can operate in a normal mode, to copy to or from the flash
> module mapping window, or in a checksum calculation mode, to evaluate
> the best clock settings for reads.
>
> The model introduces two custom address spaces for DMAs: one for the
> AHB window of the FMC flash devices and one for the DRAM. The latter
> is populated using a "dram" link set from the machine with the RAM
> container region.
>
> Signed-off-by: Cédric Le Goater <address@hidden>
> Acked-by: Joel Stanley <address@hidden>
> +/*
> + * Accumulate the result of the reads to provide a checksum that will
> + * be used to validate the read timing settings.
> + */
> +static void aspeed_smc_dma_checksum(AspeedSMCState *s)
> +{
> +    MemTxResult result;
> +    uint32_t data;
> +
> +    if (s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: invalid direction for DMA checksum\n",  __func__);
> +        return;
> +    }
> +
> +    while (s->regs[R_DMA_LEN]) {
> +        result = address_space_read(&s->flash_as, s->regs[R_DMA_FLASH_ADDR],
> +                                    MEMTXATTRS_UNSPECIFIED,
> +                                    (uint8_t *)&data, 4);

This does a byte-by-byte read into a local uint32_t...

> +        if (result != MEMTX_OK) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: Flash read failed @%08x\n",
> +                          __func__, s->regs[R_DMA_FLASH_ADDR]);
> +            return;
> +        }
> +
> +        /*
> +         * When the DMA is on-going, the DMA registers are updated
> +         * with the current working addresses and length.
> +         */
> +        s->regs[R_DMA_CHECKSUM] += data;

...which we then use as a (host-endian) 32-bit value.

This will behave differently on big endian and little endian hosts.
If the h/w behaviour is to to load a 32-bit data type you probably
want address_space_ldl_le() (or _be() if it's doing a big-endian load).

> +        s->regs[R_DMA_FLASH_ADDR] += 4;
> +        s->regs[R_DMA_LEN] -= 4;
> +    }
> +}
> +
> +static void aspeed_smc_dma_rw(AspeedSMCState *s)
> +{
> +    MemTxResult result;
> +    uint32_t data;
> +
> +    while (s->regs[R_DMA_LEN]) {
> +        if (s->regs[R_DMA_CTRL] & DMA_CTRL_WRITE) {
> +            result = address_space_read(&s->dram_as, 
> s->regs[R_DMA_DRAM_ADDR],
> +                                        MEMTXATTRS_UNSPECIFIED,
> +                                        (uint8_t *)&data, 4);
> +            if (result != MEMTX_OK) {
> +                qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM read failed 
> @%08x\n",
> +                              __func__, s->regs[R_DMA_DRAM_ADDR]);
> +                return;
> +            }
> +
> +            result = address_space_write(&s->flash_as,
> +                                         s->regs[R_DMA_FLASH_ADDR],
> +                                         MEMTXATTRS_UNSPECIFIED,
> +                                         (uint8_t *)&data, 4);
> +            if (result != MEMTX_OK) {
> +                qemu_log_mask(LOG_GUEST_ERROR, "%s: Flash write failed 
> @%08x\n",
> +                              __func__, s->regs[R_DMA_FLASH_ADDR]);
> +                return;
> +            }
> +        } else {
> +            result = address_space_read(&s->flash_as, 
> s->regs[R_DMA_FLASH_ADDR],
> +                                        MEMTXATTRS_UNSPECIFIED,
> +                                        (uint8_t *)&data, 4);
> +            if (result != MEMTX_OK) {
> +                qemu_log_mask(LOG_GUEST_ERROR, "%s: Flash read failed 
> @%08x\n",
> +                              __func__, s->regs[R_DMA_FLASH_ADDR]);
> +                return;
> +            }
> +
> +            result = address_space_write(&s->dram_as, 
> s->regs[R_DMA_DRAM_ADDR],
> +                                         MEMTXATTRS_UNSPECIFIED,
> +                                         (uint8_t *)&data, 4);
> +            if (result != MEMTX_OK) {
> +                qemu_log_mask(LOG_GUEST_ERROR, "%s: DRAM write failed 
> @%08x\n",
> +                              __func__, s->regs[R_DMA_DRAM_ADDR]);
> +                return;
> +            }
> +        }

Since the code here doesn't do anything with the data the
address_space_read/write here aren't wrong, but you might
prefer to use the ldl and stl functions to avoid the casts
to uint8_t* and need to specify the length.

thanks
-- PMM