Re: [Qemu-arm] [PATCH v5 7/9] ast2400: add SPI flash slaves

[Peter Maydell]

On 28 June 2016 at 19:24, Cédric Le Goater <address@hidden> wrote:
> Each controller on the ast2400 has a memory range on which it maps its
> flash module slaves. Each slave is assigned a memory segment for its
> mapping that can be changed at bootime with the Segment Address
> Register. This is not supported in the current implementation so we
> are using the defaults provided by the specs.
>
> Each SPI flash slave can then be accessed in two modes: Command and
> User. When in User mode, accesses to the memory segment of the slaves
> are translated in SPI transfers. When in Command mode, the HW
> generates the SPI commands automatically and the memory segment is
> accessed as if doing a MMIO. Other SPI controllers call that mode
> linear addressing mode.
>
> For this purpose, we are adding below each crontoller an array of
> structs gathering for each SPI flash module, a segment rank, a
> MemoryRegion to handle the memory accesses and the associated SPI
> slave device, which should be a m25p80.
>
> Only the User mode is supported for now but we are preparing ground
> for the Command mode. The framework is sufficient to support Linux.
>
> Signed-off-by: Cédric Le Goater <address@hidden>

> +/*
> + * Default segments mapping addresses and size for each slave per
> + * controller. These can be changed when board is initialized with the
> + * Segment Address Registers but they don't seem do be used on the
> + * field.
> + */
> +static const AspeedSegments aspeed_segments_legacy[] = {
> +    { 0x10000000, 32 * 1024 * 1024 },
> +};
> +
> +static const AspeedSegments aspeed_segments_fmc[] = {
> +    { 0x20000000, 64 * 1024 * 1024 },
> +    { 0x24000000, 32 * 1024 * 1024 },
> +    { 0x26000000, 32 * 1024 * 1024 },
> +    { 0x28000000, 32 * 1024 * 1024 },
> +    { 0x2A000000, 32 * 1024 * 1024 }
> +};
> +
> +static const AspeedSegments aspeed_segments_spi[] = {
> +    { 0x30000000, 64 * 1024 * 1024 },
> +};

If I understand this correctly, and the Segment Address Registers
are part of the SMC controller, then eventually if we want to
implement making these writable then the correct approach is
probably to have a container memory region which the SMC controller
provides to the board (and which the board then maps into the
system memory space), and then the controller is responsible for
mapping and unmapping the individual memory regions inside that
container. This is basically how we do PCI controllers, which also
allow guests to write to PCI BARs to map and unmap bits of memory.
This will be fine for now, though.

>  static bool aspeed_smc_is_ce_stop_active(const AspeedSMCState *s, int cs)
> @@ -237,6 +353,8 @@ static void aspeed_smc_realize(DeviceState *dev, Error 
> **errp)
>      AspeedSMCState *s = ASPEED_SMC(dev);
>      AspeedSMCClass *mc = ASPEED_SMC_GET_CLASS(s);
>      int i;
> +    char name[32];
> +    hwaddr offset = 0;
>
>      s->ctrl = mc->ctrl;
>
> @@ -270,6 +388,32 @@ static void aspeed_smc_realize(DeviceState *dev, Error 
> **errp)
>      memory_region_init_io(&s->mmio, OBJECT(s), &aspeed_smc_ops, s,
>                            s->ctrl->name, ASPEED_SMC_R_MAX * 4);
>      sysbus_init_mmio(sbd, &s->mmio);
> +
> +    /*
> +     * Memory region where flash modules are remapped
> +     */
> +    snprintf(name, sizeof(name), "%s.flash", s->ctrl->name);
> +
> +    memory_region_init_io(&s->mmio_flash, OBJECT(s),
> +                          &aspeed_smc_flash_default_ops, s, name,
> +                          s->ctrl->mapping_window_size);
> +    sysbus_init_mmio(sbd, &s->mmio_flash);
> +
> +    s->flashes = g_malloc0(sizeof(AspeedSMCFlash) * s->num_cs);

This should be g_new0(AspeedSMCFlash, s->num_cs);  -- multiplying
in a g_malloc0() is usually a sign you should use g_new0 instead.

Otherwise
Reviewed-by: Peter Maydell <address@hidden>

so I'll just fix that when I put the series in target-arm.next.

thanks
-- PMM