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

[Alistair Francis]

On Tue, Feb 2, 2016 at 7:30 AM, Peter Maydell <address@hidden> wrote:
> 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.

Fixed

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

Fixed

>
>> +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?

Yep, I added one.

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

So far I think all the STM devices are all like this, so I'm going to
keep it as is. I don't really mind which way it is either, but I think
they should all be consistent and at the moment this is the consistent
way :)

Thanks,

Alistair

>
> thanks
> -- PMM