Re: [Qemu-devel] [PATCH v3 4/7] STM32F2xx: Add the SPI device

[Peter Maydell]

On 19 January 2016 at 07:23, Alistair Francis <address@hidden> wrote:
> Add the STM32F2xx SPI device.
>
> Signed-off-by: Alistair Francis <address@hidden>
> ---
> V2:
>  - Address Peter C's comments
>
>  default-configs/arm-softmmu.mak |   1 +
>  hw/ssi/Makefile.objs            |   1 +
>  hw/ssi/stm32f2xx_spi.c          | 205 
> ++++++++++++++++++++++++++++++++++++++++
>  include/hw/ssi/stm32f2xx_spi.h  |  72 ++++++++++++++
>  4 files changed, 279 insertions(+)
>  create mode 100644 hw/ssi/stm32f2xx_spi.c
>  create mode 100644 include/hw/ssi/stm32f2xx_spi.h

> +static uint64_t stm32f2xx_spi_read(void *opaque, hwaddr addr,
> +                                     unsigned int size)
> +{
> +    STM32F2XXSPIState *s = opaque;
> +    uint32_t retval;
> +
> +    DB_PRINT("Address: 0x%"HWADDR_PRIx"\n", addr);
> +
> +    switch (addr) {
> +    case STM_SPI_CR1:
> +        return s->spi_cr1;
> +    case STM_SPI_CR2:
> +        qemu_log_mask(LOG_UNIMP, "%s: Interrupts and DMA are not 
> implemented\n",
> +                      __func__);
> +        return s->spi_cr2;
> +    case STM_SPI_SR:
> +        retval = s->spi_sr;
> +        return retval;
> +    case STM_SPI_DR:
> +        stm32f2xx_spi_transfer(s);
> +        s->spi_sr &= ~STM_SPI_SR_RXNE;
> +        return s->spi_dr;
> +    case STM_SPI_CRCPR:
> +        qemu_log_mask(LOG_UNIMP, "%s: CRC is not implemented, the registers 
> " \
> +                      "are included for compatability\n", __func__);

"compatibility" again, here and below.


> +        return s->spi_crcpr;
> +    case STM_SPI_RXCRCR:
> +        qemu_log_mask(LOG_UNIMP, "%s: CRC is not implemented, the registers 
> " \
> +                      "are included for compatability\n", __func__);
> +        return s->spi_rxcrcr;
> +    case STM_SPI_TXCRCR:
> +        qemu_log_mask(LOG_UNIMP, "%s: CRC is not implemented, the registers 
> " \
> +                      "are included for compatability\n", __func__);
> +        return s->spi_txcrcr;
> +    case STM_SPI_I2SCFGR:
> +        qemu_log_mask(LOG_UNIMP, "%s: I2S is not implemented, the registers 
> " \
> +                      "are included for compatability\n", __func__);
> +        return s->spi_i2scfgr;
> +    case STM_SPI_I2SPR:
> +        qemu_log_mask(LOG_UNIMP, "%s: I2S is not implemented, the registers 
> " \
> +                      "are included for compatability\n", __func__);
> +        return s->spi_i2spr;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%"HWADDR_PRIx"\n",

Spaces, please.

> +static void stm32f2xx_spi_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->reset = stm32f2xx_spi_reset;
> +}
> +
> +static const TypeInfo stm32f2xx_spi_info = {
> +    .name          = TYPE_STM32F2XX_SPI,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(STM32F2XXSPIState),
> +    .instance_init = stm32f2xx_spi_init,
> +    .class_init    = stm32f2xx_spi_class_init,
> +};

Can we have a VMState for migration, please?

> +
> +static void stm32f2xx_spi_register_types(void)
> +{
> +    type_register_static(&stm32f2xx_spi_info);
> +}

> +typedef struct {
> +    /* <private> */
> +    SysBusDevice parent_obj;
> +
> +    /* <public> */
> +    MemoryRegion mmio;
> +
> +    uint32_t spi_cr1;
> +    uint32_t spi_cr2;
> +    uint32_t spi_sr;
> +    uint32_t spi_dr;
> +    uint32_t spi_crcpr;
> +    uint32_t spi_rxcrcr;
> +    uint32_t spi_txcrcr;
> +    uint32_t spi_i2scfgr;
> +    uint32_t spi_i2spr;
> +
> +    qemu_irq irq;
> +    SSIBus *ssi;
> +} STM32F2XXSPIState;

Personally I like to order the struct fields of a device to put all
the not-migration data first (so here, mmio, irq, ssi), and then
the fields that correspond to real device state after that.
I don't feel very strongly about that though and of course a
lot of our devices don't do it.

thanks
-- PMM