[v3,3/5] ast2400: add SPI flash slave object

Each SPI flash slave can operate 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.

This object is a model proposal for a SPI flash module, gathering SPI
slave properties and a MemoryRegion handling the memory accesses.
Only the User mode is supported for now but we are preparing ground
for the Command mode.

The framework below is sufficient to support Linux which only uses
User Mode.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ssi/aspeed_smc.c         | 97 +++++++++++++++++++++++++++++++++++++++++++++
 include/hw/ssi/aspeed_smc.h | 16 ++++++++
 2 files changed, 113 insertions(+)

On 23 June 2016 at 17:33, Cédric Le Goater <clg@kaod.org> wrote:
> Each SPI flash slave can operate 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.
>
> This object is a model proposal for a SPI flash module, gathering SPI
> slave properties and a MemoryRegion handling the memory accesses.
> Only the User mode is supported for now but we are preparing ground
> for the Command mode.
>
> The framework below is sufficient to support Linux which only uses
> User Mode.
>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/ssi/aspeed_smc.c         | 97 +++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ssi/aspeed_smc.h | 16 ++++++++
>  2 files changed, 113 insertions(+)
>
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index 6b52123a67bb..d97c077565c3 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -112,6 +112,21 @@ static bool aspeed_smc_is_ce_stop_active(AspeedSMCState *s, int cs)
>      return s->regs[s->r_ctrl0 + cs] & CTRL_CE_STOP_ACTIVE;
>  }
>
> +static inline int aspeed_smc_flash_mode(AspeedSMCState *s, int cs)
> +{
> +    return s->regs[s->r_ctrl0 + cs] & CTRL_CMD_MODE_MASK;
> +}
> +
> +static inline bool aspeed_smc_is_usermode(AspeedSMCState *s, int cs)
> +{
> +    return (aspeed_smc_flash_mode(s, cs) == CTRL_USERMODE);

You don't need these brackets.

> +}
> +
> +static inline bool aspeed_smc_is_writable(AspeedSMCState *s, int cs)
> +{
> +    return s->regs[s->r_conf] & (1 << (s->conf_enable_w0 + cs));
> +}
> +
>  static void aspeed_smc_update_cs(AspeedSMCState *s)
>  {
>      int i;
> @@ -296,3 +311,85 @@ static void aspeed_smc_register_types(void)
>  }
>
>  type_init(aspeed_smc_register_types)
> +
> +static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    AspeedSMCFlashState *fl = ASPEED_SMC_FLASH(opaque);
> +    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 {
> +        error_report("%s: flash not in usermode", __func__);

This should probably be a LOG_UNIMP or LOG_GUEST_ERROR qemu_log
message. (Similarly below.)

> +        ret = -1;
> +    }
> +
> +    return ret;
> +}
> +
> +static void aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t data,
> +                           unsigned size)
> +{
> +    AspeedSMCFlashState *fl = ASPEED_SMC_FLASH(opaque);
> +    AspeedSMCState *s = fl->controller;
> +    int i;
> +
> +    if (!aspeed_smc_is_writable(s, fl->id)) {
> +        error_report("%s: flash is not writable", __func__);
> +        return;
> +    }
> +
> +    if (!aspeed_smc_is_usermode(s, fl->id)) {
> +        error_report("%s: flash not in usermode", __func__);
> +        return;
> +    }
> +
> +    for (i = 0; i < size; i++) {
> +        ssi_transfer(s->spi, (data >> (8 * i)) & 0xff);
> +    }
> +}
> +
> +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 const VMStateDescription vmstate_aspeed_smc_flash = {
> +    .name = "aspeed.smc_flash",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(id, AspeedSMCFlashState),

I can see where we read this, but not where we write it.
If it's read only, it doesn't need to be migrated. If it's
writeable, the device needs a reset function to reset it.
("currently r/o but will become r/w when later functionality
is added, and don't want to make a migration break later" is
ok, but we should be consistent about whether we treat it as
r/w for reset and migration purposes.)

> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void aspeed_smc_flash_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->vmsd = &vmstate_aspeed_smc_flash;
> +}
> +
> +static const TypeInfo aspeed_smc_flash_info = {
> +    .name           = TYPE_ASPEED_SMC_FLASH,
> +    .parent         = TYPE_SYS_BUS_DEVICE,
> +    .instance_size  = sizeof(AspeedSMCState),
> +    .class_init     = aspeed_smc_flash_class_init,
> +};
> +
> +static void aspeed_smc_flash_register_types(void)
> +{
> +    type_register_static(&aspeed_smc_flash_info);
> +}
> +
> +type_init(aspeed_smc_flash_register_types)
> diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
> index 2d3e9f6b46d5..abd0005b01c2 100644
> --- a/include/hw/ssi/aspeed_smc.h
> +++ b/include/hw/ssi/aspeed_smc.h
> @@ -27,6 +27,22 @@
>
>  #include "hw/ssi/ssi.h"
>
> +struct AspeedSMCState;
> +
> +typedef struct AspeedSMCFlashState {
> +    SysBusDevice parent_obj;
> +
> +    MemoryRegion mmio;
> +    uint8_t id;
> +    size_t size;
> +    struct AspeedSMCState *controller;
> +    DeviceState *flash;
> +} AspeedSMCFlashState;
> +
> +#define TYPE_ASPEED_SMC_FLASH "aspeed.smc.flash"
> +#define ASPEED_SMC_FLASH(obj) \
> +    OBJECT_CHECK(AspeedSMCFlashState, (obj), TYPE_ASPEED_SMC_FLASH)
> +
>  typedef struct AspeedSMCController {
>      const char *name;
>      uint8_t r_conf;

thanks
-- PMM

On 06/23/2016 08:56 PM, Peter Maydell wrote:
> On 23 June 2016 at 17:33, Cédric Le Goater <clg@kaod.org> wrote:
>> Each SPI flash slave can operate 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.
>>
>> This object is a model proposal for a SPI flash module, gathering SPI
>> slave properties and a MemoryRegion handling the memory accesses.
>> Only the User mode is supported for now but we are preparing ground
>> for the Command mode.
>>
>> The framework below is sufficient to support Linux which only uses
>> User Mode.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/ssi/aspeed_smc.c         | 97 +++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/ssi/aspeed_smc.h | 16 ++++++++
>>  2 files changed, 113 insertions(+)
>>
>> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
>> index 6b52123a67bb..d97c077565c3 100644
>> --- a/hw/ssi/aspeed_smc.c
>> +++ b/hw/ssi/aspeed_smc.c
>> @@ -112,6 +112,21 @@ static bool aspeed_smc_is_ce_stop_active(AspeedSMCState *s, int cs)
>>      return s->regs[s->r_ctrl0 + cs] & CTRL_CE_STOP_ACTIVE;
>>  }
>>
>> +static inline int aspeed_smc_flash_mode(AspeedSMCState *s, int cs)
>> +{
>> +    return s->regs[s->r_ctrl0 + cs] & CTRL_CMD_MODE_MASK;
>> +}
>> +
>> +static inline bool aspeed_smc_is_usermode(AspeedSMCState *s, int cs)
>> +{
>> +    return (aspeed_smc_flash_mode(s, cs) == CTRL_USERMODE);
> 
> You don't need these brackets.

true.

>> +}
>> +
>> +static inline bool aspeed_smc_is_writable(AspeedSMCState *s, int cs)
>> +{
>> +    return s->regs[s->r_conf] & (1 << (s->conf_enable_w0 + cs));
>> +}
>> +
>>  static void aspeed_smc_update_cs(AspeedSMCState *s)
>>  {
>>      int i;
>> @@ -296,3 +311,85 @@ static void aspeed_smc_register_types(void)
>>  }
>>
>>  type_init(aspeed_smc_register_types)
>> +
>> +static uint64_t aspeed_smc_flash_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> +    AspeedSMCFlashState *fl = ASPEED_SMC_FLASH(opaque);
>> +    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 {
>> +        error_report("%s: flash not in usermode", __func__);
> 
> This should probably be a LOG_UNIMP or LOG_GUEST_ERROR qemu_log
> message. (Similarly below.)

ok.

>> +        ret = -1;
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +static void aspeed_smc_flash_write(void *opaque, hwaddr addr, uint64_t data,
>> +                           unsigned size)
>> +{
>> +    AspeedSMCFlashState *fl = ASPEED_SMC_FLASH(opaque);
>> +    AspeedSMCState *s = fl->controller;
>> +    int i;
>> +
>> +    if (!aspeed_smc_is_writable(s, fl->id)) {
>> +        error_report("%s: flash is not writable", __func__);
>> +        return;
>> +    }
>> +
>> +    if (!aspeed_smc_is_usermode(s, fl->id)) {
>> +        error_report("%s: flash not in usermode", __func__);
>> +        return;
>> +    }
>> +
>> +    for (i = 0; i < size; i++) {
>> +        ssi_transfer(s->spi, (data >> (8 * i)) & 0xff);
>> +    }
>> +}
>> +
>> +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 const VMStateDescription vmstate_aspeed_smc_flash = {
>> +    .name = "aspeed.smc_flash",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT8(id, AspeedSMCFlashState),
> 
> I can see where we read this, but not where we write it.
> If it's read only, it doesn't need to be migrated. If it's
> writeable, the device needs a reset function to reset it.
> ("currently r/o but will become r/w when later functionality
> is added, and don't want to make a migration break later" is
> ok, but we should be consistent about whether we treat it as
> r/w for reset and migration purposes.)

I have not given enough attention to the migration of this object.
I will check that.

Thanks,

C.


>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +static void aspeed_smc_flash_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    dc->vmsd = &vmstate_aspeed_smc_flash;
>> +}
>> +
>> +static const TypeInfo aspeed_smc_flash_info = {
>> +    .name           = TYPE_ASPEED_SMC_FLASH,
>> +    .parent         = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size  = sizeof(AspeedSMCState),
>> +    .class_init     = aspeed_smc_flash_class_init,
>> +};
>> +
>> +static void aspeed_smc_flash_register_types(void)
>> +{
>> +    type_register_static(&aspeed_smc_flash_info);
>> +}
>> +
>> +type_init(aspeed_smc_flash_register_types)
>> diff --git a/include/hw/ssi/aspeed_smc.h b/include/hw/ssi/aspeed_smc.h
>> index 2d3e9f6b46d5..abd0005b01c2 100644
>> --- a/include/hw/ssi/aspeed_smc.h
>> +++ b/include/hw/ssi/aspeed_smc.h
>> @@ -27,6 +27,22 @@
>>
>>  #include "hw/ssi/ssi.h"
>>
>> +struct AspeedSMCState;
>> +
>> +typedef struct AspeedSMCFlashState {
>> +    SysBusDevice parent_obj;
>> +
>> +    MemoryRegion mmio;
>> +    uint8_t id;
>> +    size_t size;
>> +    struct AspeedSMCState *controller;
>> +    DeviceState *flash;
>> +} AspeedSMCFlashState;
>> +
>> +#define TYPE_ASPEED_SMC_FLASH "aspeed.smc.flash"
>> +#define ASPEED_SMC_FLASH(obj) \
>> +    OBJECT_CHECK(AspeedSMCFlashState, (obj), TYPE_ASPEED_SMC_FLASH)
>> +
>>  typedef struct AspeedSMCController {
>>      const char *name;
>>      uint8_t r_conf;
> 
> thanks
> -- PMM
>