qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Cédric Le Goater
Subject: Re: [Qemu-arm] [PATCH v5 7/9] ast2400: add SPI flash slaves
Date: Fri, 1 Jul 2016 18:44:31 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Icedove/45.1.0

On 07/01/2016 06:24 PM, Peter Maydell wrote:
> 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.

OK. I will take a look at the approach. The default setting seems to 
satisfy the different implementations I know of.   

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

ah yes. I have changed that back and forth and kept the wrong one ...

> Otherwise
> Reviewed-by: Peter Maydell <address@hidden>
> 
> so I'll just fix that when I put the series in target-arm.next.

I have some extra patches to use a rom device and boot from flash0.
That is for next week. 

Thanks,

C.


 
> thanks
> -- PMM
> 




reply via email to

[Prev in Thread] Current Thread [Next in Thread]