On 21 September 2018 at 17:19, Cédric Le Goater <c...@kaod.org> 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 <c...@kaod.org>
> +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
