On Tue, Feb 2, 2016 at 7:30 AM, Peter Maydell <peter.mayd...@linaro.org> wrote: > On 19 January 2016 at 07:23, Alistair Francis <alistai...@gmail.com> wrote: >> Add the STM32F2xx SPI device. >> >> Signed-off-by: Alistair Francis <alist...@alistair23.me> >> --- >> 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