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

[Cédric Le Goater]

On 10/2/18 12:56 PM, Peter Maydell wrote:
> 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 ?

The Interrupt Control and Status Register (FM0C8) has a DMA Status
BIT(11) to indicate that a DMA transfer is in progress or not. Nothing
for errors.

There are a SPI command abort status BIT(10) and a SPI Write Address
Protected status BIT(11) but these are for the command and address
filters.
 
>> +
>> +/*
>> + * 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.

yes. sure.

>> +    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.

ok. I see, but it seems I have a chicken & egg problem. 

The MemoryRegion I would liked to grab is the bmc->ram one in aspeed.c 
but it is initialized after the SoC is. I don't know how to lazy bind 
this region in the Aspeed SMC model using the Aspeed SoC model as a 
proxy to pass the region property through a link or/and alias.

If I could find a way, the model would be much cleaner.  

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

For the moment the only use of these registers is in the Aspeed custom 
u-boot of the SDK : 

or in the rewrite I proposed in mainline :


Thanks,

C.