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