[v5,7/9] ast2400: add SPI flash slaves

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 <clg@kaod.org>
---

 Changes since v3:

 - Fixed error logging
 - Constantified a couple of routine arguments
 - Sorted out what was need for migration (nothing apriori, but flash
   module object will need some care)
 - Reworked the structures handling the IOs on the flash modules :
   . Replaced the SysBusDevice object with a simple struct
   . Added a global memory region for the flash memory address space
   . Added sub regions for each segment
   . Moved mapping in SOC initialization   
 - Rephrased commit log

 hw/arm/ast2400.c            |   5 ++
 hw/ssi/aspeed_smc.c         | 150 +++++++++++++++++++++++++++++++++++++++++++-
 include/hw/ssi/aspeed_smc.h |  21 +++++++
 3 files changed, 173 insertions(+), 3 deletions(-)

On 28 June 2016 at 19:24, Cédric Le Goater <clg@kaod.org> 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 <clg@kaod.org>

> +/*
> + * 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 <peter.maydell@linaro.org>

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

thanks
-- PMM

On 07/01/2016 06:24 PM, Peter Maydell wrote:
> On 28 June 2016 at 19:24, Cédric Le Goater <clg@kaod.org> 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 <clg@kaod.org>
> 
>> +/*
>> + * 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 <peter.maydell@linaro.org>
> 
> 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
>

On 1 July 2016 at 17:44, Cédric Le Goater <clg@kaod.org> wrote:
> I have some extra patches to use a rom device and boot from flash0.
> That is for next week.

We're in softfreeze now, so really I should stop
taking non-bugfix patches, though for a new board
with missing stuff that prevents boot there's a little
flexibility.

thanks
-- PMM

W dniu 28.06.2016 o 20:24, Cédric Le Goater pisze:
> 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 <clg@kaod.org>
> ---
>
>   Changes since v3:
>
>   - Fixed error logging
>   - Constantified a couple of routine arguments
>   - Sorted out what was need for migration (nothing apriori, but flash
>     module object will need some care)
>   - Reworked the structures handling the IOs on the flash modules :
>     . Replaced the SysBusDevice object with a simple struct
>     . Added a global memory region for the flash memory address space
>     . Added sub regions for each segment
>     . Moved mapping in SOC initialization
>   - Rephrased commit log
>
>   hw/arm/ast2400.c            |   5 ++
>   hw/ssi/aspeed_smc.c         | 150 +++++++++++++++++++++++++++++++++++++++++++-
>   include/hw/ssi/aspeed_smc.h |  21 +++++++
>   3 files changed, 173 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/ast2400.c b/hw/arm/ast2400.c
> index b16ba2d0c516..9c30b45f87a9 100644
> --- a/hw/arm/ast2400.c
> +++ b/hw/arm/ast2400.c
> @@ -31,6 +31,9 @@
>   #define AST2400_TIMER_BASE       0x1E782000
>   #define AST2400_I2C_BASE         0x1E78A000
>   
> +#define AST2400_FMC_FLASH_BASE   0x20000000
> +#define AST2400_SPI_FLASH_BASE   0x30000000
> +
>   #define AST2400_A0_SILICON_REV   0x02000303
>   
>   static const int uart_irqs[] = { 9, 32, 33, 34, 10 };
> @@ -167,6 +170,7 @@ static void ast2400_realize(DeviceState *dev, Error **errp)
>           return;
>       }
>       sysbus_mmio_map(SYS_BUS_DEVICE(&s->smc), 0, AST2400_FMC_BASE);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->smc), 1, AST2400_FMC_FLASH_BASE);
>       sysbus_connect_irq(SYS_BUS_DEVICE(&s->smc), 0,
>                          qdev_get_gpio_in(DEVICE(&s->vic), 19));
>   
> @@ -179,6 +183,7 @@ static void ast2400_realize(DeviceState *dev, Error **errp)
>           return;
>       }
>       sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi), 0, AST2400_SPI_BASE);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi), 1, AST2400_SPI_FLASH_BASE);
>   }
>   
>   static void ast2400_class_init(ObjectClass *oc, void *data)
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index 537635e18d64..cb0b23750bcf 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -127,13 +127,129 @@
>   #define R_SPI_MISC_CTRL   (0x10 / 4)
>   #define R_SPI_TIMINGS     (0x14 / 4)
>   
> +/*
> + * 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 },
> +};
> +
>   static const AspeedSMCController controllers[] = {
>       { "aspeed.smc.smc", R_CONF, R_CE_CTRL, R_CTRL0, R_TIMINGS,
> -      CONF_ENABLE_W0, 5 },
> +      CONF_ENABLE_W0, 5, aspeed_segments_legacy, 0x6000000 },
>       { "aspeed.smc.fmc", R_CONF, R_CE_CTRL, R_CTRL0, R_TIMINGS,
> -      CONF_ENABLE_W0, 5 },
> +      CONF_ENABLE_W0, 5, aspeed_segments_fmc, 0x10000000 },
>       { "aspeed.smc.spi", R_SPI_CONF, 0xff, R_SPI_CTRL0, R_SPI_TIMINGS,
> -      SPI_CONF_ENABLE_W0, 1 },
> +      SPI_CONF_ENABLE_W0, 1, aspeed_segments_spi, 0x10000000 },
> +};
> +
> +static uint64_t aspeed_smc_flash_default_read(void *opaque, hwaddr addr,
> +                                              unsigned size)
> +{
> +    qemu_log_mask(LOG_GUEST_ERROR, "%s: To 0x%" HWADDR_PRIx " of size %u"
> +                  PRIx64 "\n", __func__, addr, size);
> +    return 0;
> +}
> +
> +static void aspeed_smc_flash_default_write(void *opaque, hwaddr addr,
> +                                           uint64_t data, unsigned size)
> +{
> +   qemu_log_mask(LOG_GUEST_ERROR, "%s: To 0x%" HWADDR_PRIx " of size %u: 0x%"
> +                 PRIx64 "\n", __func__, addr, size, data);
> +}
> +
> +static const MemoryRegionOps aspeed_smc_flash_default_ops = {
> +    .read = aspeed_smc_flash_default_read,
> +    .write = aspeed_smc_flash_default_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 1,
> +        .max_access_size = 4,
> +    },
> +};
> +
> +static inline int aspeed_smc_flash_mode(const AspeedSMCState *s, int cs)
> +{
> +    return s->regs[s->r_ctrl0 + cs] & CTRL_CMD_MODE_MASK;
> +}
> +
> +static inline bool aspeed_smc_is_usermode(const AspeedSMCState *s, int cs)
> +{
> +    return aspeed_smc_flash_mode(s, cs) == CTRL_USERMODE;
> +}
> +
> +static inline bool aspeed_smc_is_writable(const AspeedSMCState *s, int cs)
> +{
> +    return s->regs[s->r_conf] & (1 << (s->conf_enable_w0 + cs));
> +}
> +
> +static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    AspeedSMCFlash *fl = opaque;
> +    const AspeedSMCState *s = fl->controller;
> +    uint64_t ret = 0;
> +    int i;
> +
> +    if (aspeed_smc_is_usermode(s, fl->id)) {
> +        for (i = 0; i < size; i++) {
> +            ret |= ssi_transfer(s->spi, 0x0) << (8 * i);
> +        }
> +    } else {
> +        qemu_log_mask(LOG_UNIMP, "%s: usermode not implemented\n",
> +                      __func__);
> +        ret = -1;
> +    }
> +
> +    return ret;
> +}
> +
> +static void aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t data,
> +                           unsigned size)
> +{
> +    AspeedSMCFlash *fl = opaque;
> +    const AspeedSMCState *s = fl->controller;
> +    int i;
> +
> +    if (!aspeed_smc_is_writable(s, fl->id)) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: flash is not writable at 0x%"
> +                      HWADDR_PRIx "\n", __func__, addr);
> +        return;
> +    }
> +
> +    if (!aspeed_smc_is_usermode(s, fl->id)) {
> +        qemu_log_mask(LOG_UNIMP, "%s: usermode not implemented\n",
> +                      __func__);
> +        return;
> +    }
> +
> +    for (i = 0; i < size; i++) {
> +        ssi_transfer(s->spi, (data >> (8 * i)) & 0xff);
Hi,

I think the error with m25p80 tests is here.
The flash device I know (eg. n25q*) takes address in BE, MSB first.
Since we do not care about MSB/LSB stuff, we should fallow BE/LE.
If your machine is BE, you will start to send address backwards.

Regard,
Marcin


> +    }
> +}
> +
> +static const MemoryRegionOps aspeed_smc_flash_ops = {
> +    .read = aspeed_smc_flash_read,
> +    .write = aspeed_smc_flash_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 1,
> +        .max_access_size = 4,
> +    },
>   };
>   
>   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);
> +
> +    for (i = 0; i < s->num_cs; ++i) {
> +        AspeedSMCFlash *fl = &s->flashes[i];
> +
> +        snprintf(name, sizeof(name), "%s.%d", s->ctrl->name, i);
> +
> +        fl->id = i;
> +        fl->controller = s;
> +        fl->size = s->ctrl->segments[i].size;
> +        memory_region_init_io(&fl->mmio, OBJECT(s), &aspeed_smc_flash_ops,
> +                              fl, name, fl->size);
> +        memory_region_add_subregion(&s->mmio_flash, offset, &fl->mmio);
> +        offset += fl->size;
> +    }
>   }
>   
>   static const VMStateDescription vmstate_aspeed_smc = {
> diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
> index c4a4960cd880..def3b4507e75 100644
> --- a/include/hw/ssi/aspeed_smc.h
> +++ b/include/hw/ssi/aspeed_smc.h
> @@ -27,6 +27,12 @@
>   
>   #include "hw/ssi/ssi.h"
>   
> +typedef struct AspeedSegments {
> +    hwaddr addr;
> +    uint32_t size;
> +} AspeedSegments;
> +
> +struct AspeedSMCState;
>   typedef struct AspeedSMCController {
>       const char *name;
>       uint8_t r_conf;
> @@ -35,8 +41,20 @@ typedef struct AspeedSMCController {
>       uint8_t r_timings;
>       uint8_t conf_enable_w0;
>       uint8_t max_slaves;
> +    const AspeedSegments *segments;
> +    uint32_t mapping_window_size;
>   } AspeedSMCController;
>   
> +typedef struct AspeedSMCFlash {
> +    const struct AspeedSMCState *controller;
> +
> +    uint8_t id;
> +    uint32_t size;
> +
> +    MemoryRegion mmio;
> +    DeviceState *flash;
> +} AspeedSMCFlash;
> +
>   #define TYPE_ASPEED_SMC "aspeed.smc"
>   #define ASPEED_SMC(obj) OBJECT_CHECK(AspeedSMCState, (obj), TYPE_ASPEED_SMC)
>   #define ASPEED_SMC_CLASS(klass) \
> @@ -57,6 +75,7 @@ typedef struct AspeedSMCState {
>       const AspeedSMCController *ctrl;
>   
>       MemoryRegion mmio;
> +    MemoryRegion mmio_flash;
>   
>       qemu_irq irq;
>       int irqline;
> @@ -74,6 +93,8 @@ typedef struct AspeedSMCState {
>       uint8_t r_ctrl0;
>       uint8_t r_timings;
>       uint8_t conf_enable_w0;
> +
> +    AspeedSMCFlash *flashes;
>   } AspeedSMCState;
>   
>   #endif /* ASPEED_SMC_H */

W dniu 02.07.2016 o 19:08, mar.krzeminski pisze:
>
>
> W dniu 28.06.2016 o 20:24, Cédric Le Goater pisze:
>> 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 <clg@kaod.org>
>> ---
>>
>>   Changes since v3:
>>
>>   - Fixed error logging
>>   - Constantified a couple of routine arguments
>>   - Sorted out what was need for migration (nothing apriori, but flash
>>     module object will need some care)
>>   - Reworked the structures handling the IOs on the flash modules :
>>     . Replaced the SysBusDevice object with a simple struct
>>     . Added a global memory region for the flash memory address space
>>     . Added sub regions for each segment
>>     . Moved mapping in SOC initialization
>>   - Rephrased commit log
>>
>>   hw/arm/ast2400.c            |   5 ++
>>   hw/ssi/aspeed_smc.c         | 150 
>> +++++++++++++++++++++++++++++++++++++++++++-
>>   include/hw/ssi/aspeed_smc.h |  21 +++++++
>>   3 files changed, 173 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/arm/ast2400.c b/hw/arm/ast2400.c
>> index b16ba2d0c516..9c30b45f87a9 100644
>> --- a/hw/arm/ast2400.c
>> +++ b/hw/arm/ast2400.c
>> @@ -31,6 +31,9 @@
>>   #define AST2400_TIMER_BASE       0x1E782000
>>   #define AST2400_I2C_BASE         0x1E78A000
>>   +#define AST2400_FMC_FLASH_BASE   0x20000000
>> +#define AST2400_SPI_FLASH_BASE   0x30000000
>> +
>>   #define AST2400_A0_SILICON_REV   0x02000303
>>     static const int uart_irqs[] = { 9, 32, 33, 34, 10 };
>> @@ -167,6 +170,7 @@ static void ast2400_realize(DeviceState *dev, 
>> Error **errp)
>>           return;
>>       }
>>       sysbus_mmio_map(SYS_BUS_DEVICE(&s->smc), 0, AST2400_FMC_BASE);
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->smc), 1, 
>> AST2400_FMC_FLASH_BASE);
>>       sysbus_connect_irq(SYS_BUS_DEVICE(&s->smc), 0,
>> qdev_get_gpio_in(DEVICE(&s->vic), 19));
>>   @@ -179,6 +183,7 @@ static void ast2400_realize(DeviceState *dev, 
>> Error **errp)
>>           return;
>>       }
>>       sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi), 0, AST2400_SPI_BASE);
>> +    sysbus_mmio_map(SYS_BUS_DEVICE(&s->spi), 1, 
>> AST2400_SPI_FLASH_BASE);
>>   }
>>     static void ast2400_class_init(ObjectClass *oc, void *data)
>> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
>> index 537635e18d64..cb0b23750bcf 100644
>> --- a/hw/ssi/aspeed_smc.c
>> +++ b/hw/ssi/aspeed_smc.c
>> @@ -127,13 +127,129 @@
>>   #define R_SPI_MISC_CTRL   (0x10 / 4)
>>   #define R_SPI_TIMINGS     (0x14 / 4)
>>   +/*
>> + * 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 },
>> +};
>> +
>>   static const AspeedSMCController controllers[] = {
>>       { "aspeed.smc.smc", R_CONF, R_CE_CTRL, R_CTRL0, R_TIMINGS,
>> -      CONF_ENABLE_W0, 5 },
>> +      CONF_ENABLE_W0, 5, aspeed_segments_legacy, 0x6000000 },
>>       { "aspeed.smc.fmc", R_CONF, R_CE_CTRL, R_CTRL0, R_TIMINGS,
>> -      CONF_ENABLE_W0, 5 },
>> +      CONF_ENABLE_W0, 5, aspeed_segments_fmc, 0x10000000 },
>>       { "aspeed.smc.spi", R_SPI_CONF, 0xff, R_SPI_CTRL0, R_SPI_TIMINGS,
>> -      SPI_CONF_ENABLE_W0, 1 },
>> +      SPI_CONF_ENABLE_W0, 1, aspeed_segments_spi, 0x10000000 },
>> +};
>> +
>> +static uint64_t aspeed_smc_flash_default_read(void *opaque, hwaddr 
>> addr,
>> +                                              unsigned size)
>> +{
>> +    qemu_log_mask(LOG_GUEST_ERROR, "%s: To 0x%" HWADDR_PRIx " of 
>> size %u"
>> +                  PRIx64 "\n", __func__, addr, size);
>> +    return 0;
>> +}
>> +
>> +static void aspeed_smc_flash_default_write(void *opaque, hwaddr addr,
>> +                                           uint64_t data, unsigned 
>> size)
>> +{
>> +   qemu_log_mask(LOG_GUEST_ERROR, "%s: To 0x%" HWADDR_PRIx " of size 
>> %u: 0x%"
>> +                 PRIx64 "\n", __func__, addr, size, data);
>> +}
>> +
>> +static const MemoryRegionOps aspeed_smc_flash_default_ops = {
>> +    .read = aspeed_smc_flash_default_read,
>> +    .write = aspeed_smc_flash_default_write,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>> +    .valid = {
>> +        .min_access_size = 1,
>> +        .max_access_size = 4,
>> +    },
>> +};
>> +
>> +static inline int aspeed_smc_flash_mode(const AspeedSMCState *s, int 
>> cs)
>> +{
>> +    return s->regs[s->r_ctrl0 + cs] & CTRL_CMD_MODE_MASK;
>> +}
>> +
>> +static inline bool aspeed_smc_is_usermode(const AspeedSMCState *s, 
>> int cs)
>> +{
>> +    return aspeed_smc_flash_mode(s, cs) == CTRL_USERMODE;
>> +}
>> +
>> +static inline bool aspeed_smc_is_writable(const AspeedSMCState *s, 
>> int cs)
>> +{
>> +    return s->regs[s->r_conf] & (1 << (s->conf_enable_w0 + cs));
>> +}
>> +
>> +static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, 
>> unsigned size)
>> +{
>> +    AspeedSMCFlash *fl = opaque;
>> +    const AspeedSMCState *s = fl->controller;
>> +    uint64_t ret = 0;
>> +    int i;
>> +
>> +    if (aspeed_smc_is_usermode(s, fl->id)) {
>> +        for (i = 0; i < size; i++) {
>> +            ret |= ssi_transfer(s->spi, 0x0) << (8 * i);
>> +        }
>> +    } else {
>> +        qemu_log_mask(LOG_UNIMP, "%s: usermode not implemented\n",
>> +                      __func__);
>> +        ret = -1;
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +static void aspeed_smc_flash_write(void *opaque, hwaddr addr, 
>> uint64_t data,
>> +                           unsigned size)
>> +{
>> +    AspeedSMCFlash *fl = opaque;
>> +    const AspeedSMCState *s = fl->controller;
>> +    int i;
>> +
>> +    if (!aspeed_smc_is_writable(s, fl->id)) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: flash is not writable at 
>> 0x%"
>> +                      HWADDR_PRIx "\n", __func__, addr);
>> +        return;
>> +    }
>> +
>> +    if (!aspeed_smc_is_usermode(s, fl->id)) {
>> +        qemu_log_mask(LOG_UNIMP, "%s: usermode not implemented\n",
>> +                      __func__);
>> +        return;
>> +    }
>> +
>> +    for (i = 0; i < size; i++) {
>> +        ssi_transfer(s->spi, (data >> (8 * i)) & 0xff);
> Hi,
>
> I think the error with m25p80 tests is here.
> The flash device I know (eg. n25q*) takes address in BE, MSB first.
> Since we do not care about MSB/LSB stuff, we should fallow BE/LE.
> If your machine is BE, you will start to send address backwards.
>
> Regard,
> Marcin
Sorry, I have just noticed DEVICE_LITTLE_ENDIAN functionality...

Regards,
Marcin

>
>> +    }
>> +}
>> +
>> +static const MemoryRegionOps aspeed_smc_flash_ops = {
>> +    .read = aspeed_smc_flash_read,
>> +    .write = aspeed_smc_flash_write,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>> +    .valid = {
>> +        .min_access_size = 1,
>> +        .max_access_size = 4,
>> +    },
>>   };
>>     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);
>> +
>> +    for (i = 0; i < s->num_cs; ++i) {
>> +        AspeedSMCFlash *fl = &s->flashes[i];
>> +
>> +        snprintf(name, sizeof(name), "%s.%d", s->ctrl->name, i);
>> +
>> +        fl->id = i;
>> +        fl->controller = s;
>> +        fl->size = s->ctrl->segments[i].size;
>> +        memory_region_init_io(&fl->mmio, OBJECT(s), 
>> &aspeed_smc_flash_ops,
>> +                              fl, name, fl->size);
>> +        memory_region_add_subregion(&s->mmio_flash, offset, &fl->mmio);
>> +        offset += fl->size;
>> +    }
>>   }
>>     static const VMStateDescription vmstate_aspeed_smc = {
>> diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
>> index c4a4960cd880..def3b4507e75 100644
>> --- a/include/hw/ssi/aspeed_smc.h
>> +++ b/include/hw/ssi/aspeed_smc.h
>> @@ -27,6 +27,12 @@
>>     #include "hw/ssi/ssi.h"
>>   +typedef struct AspeedSegments {
>> +    hwaddr addr;
>> +    uint32_t size;
>> +} AspeedSegments;
>> +
>> +struct AspeedSMCState;
>>   typedef struct AspeedSMCController {
>>       const char *name;
>>       uint8_t r_conf;
>> @@ -35,8 +41,20 @@ typedef struct AspeedSMCController {
>>       uint8_t r_timings;
>>       uint8_t conf_enable_w0;
>>       uint8_t max_slaves;
>> +    const AspeedSegments *segments;
>> +    uint32_t mapping_window_size;
>>   } AspeedSMCController;
>>   +typedef struct AspeedSMCFlash {
>> +    const struct AspeedSMCState *controller;
>> +
>> +    uint8_t id;
>> +    uint32_t size;
>> +
>> +    MemoryRegion mmio;
>> +    DeviceState *flash;
>> +} AspeedSMCFlash;
>> +
>>   #define TYPE_ASPEED_SMC "aspeed.smc"
>>   #define ASPEED_SMC(obj) OBJECT_CHECK(AspeedSMCState, (obj), 
>> TYPE_ASPEED_SMC)
>>   #define ASPEED_SMC_CLASS(klass) \
>> @@ -57,6 +75,7 @@ typedef struct AspeedSMCState {
>>       const AspeedSMCController *ctrl;
>>         MemoryRegion mmio;
>> +    MemoryRegion mmio_flash;
>>         qemu_irq irq;
>>       int irqline;
>> @@ -74,6 +93,8 @@ typedef struct AspeedSMCState {
>>       uint8_t r_ctrl0;
>>       uint8_t r_timings;
>>       uint8_t conf_enable_w0;
>> +
>> +    AspeedSMCFlash *flashes;
>>   } AspeedSMCState;
>>     #endif /* ASPEED_SMC_H */
>