Re: [Qemu-devel] [PATCH v2 08/11] aspeed/smc: add support for DMAs

[Peter Maydell]

On 21 September 2018 at 17:19, 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 a custom address space for DMAs populated with
> the required regions : an alias region on the AHB window for the flash
> devices and another alias on the SDRAM.
>
> Our primary need is to support the checksum calculation mode and the
> model only implements synchronous DMA accesses. Something to improve
> in the future.
>
> Signed-off-by: Cédric Le Goater <address@hidden>


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

Does the device really not report DMA read/write failures via
a status register bit or similar ?


> +
> +/*
> + * Populate our custom address space for DMAs with only the regions we
> + * need : the AHB window for the flash devices and the SDRAM.
> + */
> +static void aspeed_smc_dma_setup(AspeedSMCState *s)
> +{
> +    char name[32];
> +    MemoryRegion *sysmem = get_system_memory();
> +    MemoryRegion *flash_alias = g_new(MemoryRegion, 1);
> +    MemoryRegion *sdram_alias = g_new(MemoryRegion, 1);
> +
> +    snprintf(name, sizeof(name), "%s-dma", s->ctrl->name);

I would suggest using g_strdup_printf()/g_free(), since it's not
immediately obvious here that s->ctrl->name is guaranteed
to fit into the fixed-size array.

> +    memory_region_init(&s->dma_mr, OBJECT(s), name,
> +                       s->sdram_base + s->max_ram_size);
> +    address_space_init(&s->dma_as, &s->dma_mr, name);
> +
> +    snprintf(name, sizeof(name), "%s.flash", s->ctrl->name);
> +    memory_region_init_alias(flash_alias, OBJECT(s), name, &s->mmio_flash,
> +                             0, s->ctrl->flash_window_size);
> +    memory_region_add_subregion(&s->dma_mr, s->ctrl->flash_window_base,
> +                                flash_alias);
> +
> +    memory_region_init_alias(sdram_alias, OBJECT(s), "ram", sysmem,
> +                             s->sdram_base, s->max_ram_size);
> +    memory_region_add_subregion(&s->dma_mr, s->sdram_base, sdram_alias);

Rather than having the DMA device directly grab the system_memory
MR like this, it's better to have the device have a MemoryRegion
property, which the SoC sets with whatever the DMA device should
be able to see.

Otherwise, patch looks good, though I don't know enough about
the device/SoC to review those details.

thanks
-- PMM