[v2,08/11] aspeed/smc: add support for DMAs

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 <clg@kaod.org>
---
 include/hw/ssi/aspeed_smc.h |   2 +
 hw/ssi/aspeed_smc.c         | 227 +++++++++++++++++++++++++++++++++++-
 2 files changed, 223 insertions(+), 6 deletions(-)

On 21 September 2018 at 17:19, Cédric Le Goater <clg@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 <clg@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

On 10/2/18 12:56 PM, Peter Maydell wrote:
> On 21 September 2018 at 17:19, Cédric Le Goater <clg@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 <clg@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.

With the links,

[ ... ]

>> 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 : 

https://github.com/openbmc/u-boot/blob/v2016.07-aspeed-openbmc/arch/arm/mach-aspeed/platform_g5.S#L2314
 
> or in the rewrite I proposed in mainline :

http://patchwork.ozlabs.org/patch/972868/


C.

On 2 October 2018 at 16:48, Cédric Le Goater <clg@kaod.org> wrote:
> On 10/2/18 12:56 PM, Peter Maydell wrote:
>> 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.

Usually what you want to pass is not the RAM region directly but
some container object (which contains the RAM region among
other things). This is often a container that you're passing
to the SoC anyway so it can put its devices in it.

(The direct answer to your chicken-and-egg problem is that you
can create a container before creating the SoC, pass that to
the SoC, then create the RAM afterwards and put it in the
container after. But that feels a bit like a workaround for
a less than ideal structure, somehow.)

thanks
-- PMM

On 10/2/18 6:12 PM, Peter Maydell wrote:
> On 2 October 2018 at 16:48, Cédric Le Goater <clg@kaod.org> wrote:
>> On 10/2/18 12:56 PM, Peter Maydell wrote:
>>> 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.
> 
> Usually what you want to pass is not the RAM region directly but
> some container object (which contains the RAM region among
> other things). This is often a container that you're passing
> to the SoC anyway so it can put its devices in it.
> 
> (The direct answer to your chicken-and-egg problem is that you
> can create a container before creating the SoC, pass that to
> the SoC, then create the RAM afterwards and put it in the
> container after. But that feels a bit like a workaround for
> a less than ideal structure, somehow.)

couldn't we just initialize the RAM under the SoC ? We don't need
it at the machine level AFAICT, nor does the raspi model I think.

What is the rational behind putting the RAM region in a state 
structure at the machine level ? 

Thanks,

C.