Skip to contentSkip to conversation

Update the Netduino 2 Machine

PATCH Superseded Alistair Francis 2 participants 14 messages Jan 19, 2016 – Feb 22, 2016 Arm STM32

v3 of 8 Superseded Latest is v8 →

lore
Share & tools
lore

Alistair Francis

author
#0 lore raw

v3 cover Update the Netduino 2 Machine

This patchset continues with the Netduino 2 and STM32F205 SoC work.

This patch series makes a small change to the STM32F2xx SoC to tidy up the code.

Next a feature is added to the STM32F2xx timer to display the PWM duty cycle, when debugging is enabled.

Then the STM32F2xx SPI and ADC devices are added and connected to the STM32F205 SoC.

Finally the maintainers file is updated to add myself as the maintainer for the Netdunio 2 and STM32F2xx.

V3:
- Rebase
V2:
- Update based on Peter C's coments
- Rebase
- Create an ADC folder for the ADC device

Alistair Francis (7):
STM32F205: Remove the individual device variables
STM32F2xx: Display PWM duty cycle from timer
STM32F2xx: Add the ADC device
STM32F2xx: Add the SPI device
STM32F205: Connect the ADC devices
STM32F205: Connect the SPI devices
MAINTAINERS: Add Alistair to the maintainers list

 MAINTAINERS                     |  15 +++
 default-configs/arm-softmmu.mak |   2 +
 hw/Makefile.objs                |   1 +
 hw/adc/Makefile.objs            |   1 +
 hw/adc/stm32f2xx_adc.c          | 283 ++++++++++++++++++++++++++++++++++++++++
 hw/arm/stm32f205_soc.c          |  76 ++++++++---
 hw/ssi/Makefile.objs            |   1 +
 hw/ssi/stm32f2xx_spi.c          | 205 +++++++++++++++++++++++++++++
 hw/timer/stm32f2xx_timer.c      |   9 ++
 include/hw/adc/stm32f2xx_adc.h  |  94 +++++++++++++
 include/hw/arm/stm32f205_soc.h  |   6 +
 include/hw/ssi/stm32f2xx_spi.h  |  72 ++++++++++
 12 files changed, 748 insertions(+), 17 deletions(-)
 create mode 100644 hw/adc/Makefile.objs
 create mode 100644 hw/adc/stm32f2xx_adc.c
 create mode 100644 hw/ssi/stm32f2xx_spi.c
 create mode 100644 include/hw/adc/stm32f2xx_adc.h
 create mode 100644 include/hw/ssi/stm32f2xx_spi.h
Signature · 1 line
2.5.0
author
#1 lore raw

PATCH v3 1/7 STM32F205: Remove the individual device variables

Cleanup the individual DeviceState and SysBusDevice variables to re-use the same variable for each device.

Signed-off-by Alistair Francis <[email protected]>
Reviewed-by Peter Crosthwaite <[email protected]>

---

 hw/arm/stm32f205_soc.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)
hw/arm/stm32f205_soc.c +15 -17
index 79bfe6d..a2bd970 100644
@@ -60,8 +60,8 @@ static void stm32f205_soc_initfn(Object *obj)
 static void stm32f205_soc_realize(DeviceState *dev_soc, Error **errp)
 {
     STM32F205State *s = STM32F205_SOC(dev_soc);
-    DeviceState *syscfgdev, *usartdev, *timerdev, *nvic;
-    SysBusDevice *syscfgbusdev, *usartbusdev, *timerbusdev;
+    DeviceState *dev, *nvic;
+    SysBusDevice *busdev;
     Error *err = NULL;
     int i;
 

@@ -92,43 +92,41 @@ static void stm32f205_soc_realize(DeviceState *dev_soc, Error **errp)
                        s->kernel_filename, s->cpu_model);
 
     /* System configuration controller */
-    syscfgdev = DEVICE(&s->syscfg);
+    dev = DEVICE(&s->syscfg);
     object_property_set_bool(OBJECT(&s->syscfg), true, "realized", &err);
     if (err != NULL) {
         error_propagate(errp, err);
         return;
     }
-    syscfgbusdev = SYS_BUS_DEVICE(syscfgdev);
-    sysbus_mmio_map(syscfgbusdev, 0, 0x40013800);
-    sysbus_connect_irq(syscfgbusdev, 0, qdev_get_gpio_in(nvic, 71));
+    busdev = SYS_BUS_DEVICE(dev);
+    sysbus_mmio_map(busdev, 0, 0x40013800);
+    sysbus_connect_irq(busdev, 0, qdev_get_gpio_in(nvic, 71));
 
     /* Attach UART (uses USART registers) and USART controllers */
     for (i = 0; i < STM_NUM_USARTS; i++) {
-        usartdev = DEVICE(&(s->usart[i]));
+        dev = DEVICE(&(s->usart[i]));
         object_property_set_bool(OBJECT(&s->usart[i]), true, "realized", &err);
         if (err != NULL) {
             error_propagate(errp, err);
             return;
         }
-        usartbusdev = SYS_BUS_DEVICE(usartdev);
-        sysbus_mmio_map(usartbusdev, 0, usart_addr[i]);
-        sysbus_connect_irq(usartbusdev, 0,
-                           qdev_get_gpio_in(nvic, usart_irq[i]));
+        busdev = SYS_BUS_DEVICE(dev);
+        sysbus_mmio_map(busdev, 0, usart_addr[i]);
+        sysbus_connect_irq(busdev, 0, qdev_get_gpio_in(nvic, usart_irq[i]));
     }
 
     /* Timer 2 to 5 */
     for (i = 0; i < STM_NUM_TIMERS; i++) {
-        timerdev = DEVICE(&(s->timer[i]));
-        qdev_prop_set_uint64(timerdev, "clock-frequency", 1000000000);
+        dev = DEVICE(&(s->timer[i]));
+        qdev_prop_set_uint64(dev, "clock-frequency", 1000000000);
         object_property_set_bool(OBJECT(&s->timer[i]), true, "realized", &err);
         if (err != NULL) {
             error_propagate(errp, err);
             return;
         }
-        timerbusdev = SYS_BUS_DEVICE(timerdev);
-        sysbus_mmio_map(timerbusdev, 0, timer_addr[i]);
-        sysbus_connect_irq(timerbusdev, 0,
-                           qdev_get_gpio_in(nvic, timer_irq[i]));
+        busdev = SYS_BUS_DEVICE(dev);
+        sysbus_mmio_map(busdev, 0, timer_addr[i]);
+        sysbus_connect_irq(busdev, 0, qdev_get_gpio_in(nvic, timer_irq[i]));
     }
 }
Signature · 1 line
2.5.0
author
#2 lore raw

PATCH v3 2/7 STM32F2xx: Display PWM duty cycle from timer

In reply to Alistair Francis

If correctly configured allow the STM32F2xx timer to print out the PWM duty cycle information.

Signed-off-by Alistair Francis <[email protected]>
Reviewed-by Peter Crosthwaite <[email protected]>

---
V3:
- Use OR instead of + for masking
- Improve clarity of print statement
V2:
- Fix up if statement braces
- Remove stm32f2xx_timer_set_alarm() call

 hw/timer/stm32f2xx_timer.c | 9 +++++++++
 1 file changed, 9 insertions(+)
hw/timer/stm32f2xx_timer.c +9 -0
index ecadf9d..6e33a99 100644
@@ -49,6 +49,15 @@ static void stm32f2xx_timer_interrupt(void *opaque)
         qemu_irq_pulse(s->irq);
         stm32f2xx_timer_set_alarm(s, s->hit_time);
     }
+
+    if (s->tim_ccmr1 & (TIM_CCMR1_OC2M2 | TIM_CCMR1_OC2M1) &&
+        !(s->tim_ccmr1 & TIM_CCMR1_OC2M0) &&
+        s->tim_ccmr1 & TIM_CCMR1_OC2PE &&
+        s->tim_ccer & TIM_CCER_CC2E) {
+        /* PWM 2 - Mode 1 */
+        DB_PRINT("PWM2 Duty Cycle: %d%%\n",
+                s->tim_ccr2 / (100 * (s->tim_psc + 1)));
+    }
 }
 
 static inline int64_t stm32f2xx_ns_to_ticks(STM32F2XXTimerState *s, int64_t t)
Signature · 1 line
2.5.0
author
#3 lore raw

PATCH v3 3/7 STM32F2xx: Add the ADC device

In reply to Alistair Francis

Add the STM32F2xx ADC device. This device randomly generates values on each read.

This also includes creating a hw/adc directory.

Signed-off-by Alistair Francis <[email protected]>

---
V2:
- Address Peter C's comments
- Create a ADC folder and move the file in there
- Move some of the registers into arrays

 default-configs/arm-softmmu.mak |   1 +
 hw/Makefile.objs                |   1 +
 hw/adc/Makefile.objs            |   1 +
 hw/adc/stm32f2xx_adc.c          | 283 ++++++++++++++++++++++++++++++++++++++++
 include/hw/adc/stm32f2xx_adc.h  |  94 +++++++++++++
 5 files changed, 380 insertions(+)
 create mode 100644 hw/adc/Makefile.objs
 create mode 100644 hw/adc/stm32f2xx_adc.c
 create mode 100644 include/hw/adc/stm32f2xx_adc.h
default-configs/arm-softmmu.mak +1 -0
index d9b90a5..520b209 100644
@@ -85,6 +85,7 @@ CONFIG_ZYNQ=y
 CONFIG_STM32F2XX_TIMER=y
 CONFIG_STM32F2XX_USART=y
 CONFIG_STM32F2XX_SYSCFG=y
+CONFIG_STM32F2XX_ADC=y
 CONFIG_STM32F205_SOC=y
 
 CONFIG_VERSATILE_PCI=y
hw/Makefile.objs +1 -0
index 4a07ed4..0ffd281 100644
@@ -1,5 +1,6 @@
 devices-dirs-$(call land, $(CONFIG_VIRTIO),$(call land,$(CONFIG_VIRTFS),$(CONFIG_PCI))) += 9pfs/
 devices-dirs-$(CONFIG_ACPI) += acpi/
+devices-dirs-$(CONFIG_SOFTMMU) += adc/
 devices-dirs-$(CONFIG_SOFTMMU) += audio/
 devices-dirs-$(CONFIG_SOFTMMU) += block/
 devices-dirs-$(CONFIG_SOFTMMU) += bt/
hw/adc/Makefile.objs new file +1 -0
new file mode 100644
index 0000000..3f6dfde
@@ -0,0 +1 @@
+obj-$(CONFIG_STM32F2XX_ADC) += stm32f2xx_adc.o
hw/adc/stm32f2xx_adc.c new file +283 -0

283 lines not on this page · raw message

include/hw/adc/stm32f2xx_adc.h new file +94 -0

94 lines not on this page · raw message

Signature · 1 line
2.5.0
#4 lore raw

Re: [Qemu-devel] [PATCH v3 3/7] STM32F2xx: Add the ADC device

Quoted 7 lines · Alistair Francis

Add the STM32F2xx ADC device. This device randomly generates values on each read.

This also includes creating a hw/adc directory.

Signed-off-by Alistair Francis <[email protected]>

hw/adc/stm32f2xx_adc.c line 74

Quoted 8 lines
hw/adc/stm32f2xx_adc.c new file +8 -0
+static uint32_t stm32f2xx_adc_generate_value(STM32F2XXADCState *s)
+{
+    /* Attempts to fake some ADC values */
+#ifdef RAND_AVALIABLE
+    s->adc_dr = s->adc_dr + rand();
+#else
+    s->adc_dr = s->adc_dr + 7;
+#endif

We shouldn't be using rand() in devices I think. (Among other things it means we won't be deterministic, which will break record-replay.)

In any case you've typoed your #ifdef constant name, which means that code is never used :-)

hw/adc/stm32f2xx_adc.c line 106

Quoted 6 lines
hw/adc/stm32f2xx_adc.c new file +6 -0
+static uint64_t stm32f2xx_adc_read(void *opaque, hwaddr addr,
+                                     unsigned int size)
+{
+    STM32F2XXADCState *s = opaque;
+
+    DB_PRINT("Address: 0x%"HWADDR_PRIx"\n", addr);

Spaces around the HWADDR_PRIx would be nice.

Quoted 14 lines
hw/adc/stm32f2xx_adc.c new file +14 -0
+
+    if (addr >= ADC_COMMON_ADDRESS) {
+        qemu_log_mask(LOG_UNIMP,
+                      "%s: ADC Common Register Unsupported\n", __func__);
+    }
+
+    switch (addr) {
+    case ADC_SR:
+        return s->adc_sr;
+    case ADC_CR1:
+        return s->adc_cr1;
+    case ADC_CR2:
+        return s->adc_cr2 & 0xFFFFFFF;
+    case ADC_SMPR1:

hw/adc/stm32f2xx_adc.c line 130

Quoted 10 lines
hw/adc/stm32f2xx_adc.c new file +10 -0
+        return s->adc_smpr1;
+    case ADC_SMPR2:
+        return s->adc_smpr2;
+    case ADC_JOFR1:
+    case ADC_JOFR2:
+    case ADC_JOFR3:
+    case ADC_JOFR4:
+        qemu_log_mask(LOG_UNIMP, "%s: " \
+                      "Injection ADC is not implemented, the registers are " \
+                      "included for compatability\n", __func__);

"compatibility"

Quoted 21 lines
hw/adc/stm32f2xx_adc.c new file +21 -0
+        return s->adc_jofr[(addr - ADC_JOFR1) / 4];
+    case ADC_HTR:
+        return s->adc_htr;
+    case ADC_LTR:
+        return s->adc_ltr;
+    case ADC_SQR1:
+        return s->adc_sqr1;
+    case ADC_SQR2:
+        return s->adc_sqr2;
+    case ADC_SQR3:
+        return s->adc_sqr3;
+    case ADC_JSQR:
+        qemu_log_mask(LOG_UNIMP, "%s: " \
+                      "Injection ADC is not implemented, the registers are " \
+                      "included for compatability\n", __func__);
+        return s->adc_jsqr;
+    case ADC_JDR1:
+    case ADC_JDR2:
+    case ADC_JDR3:
+    case ADC_JDR4:
+        qemu_log_mask(LOG_UNIMP, "%s: " \

hw/adc/stm32f2xx_adc.c line 161

Quoted 10 lines
hw/adc/stm32f2xx_adc.c new file +10 -0
+                      "Injection ADC is not implemented, the registers are " \
+                      "included for compatability\n", __func__);
+        return s->adc_jdr[(addr - ADC_JDR1) / 4] -
+               s->adc_jofr[(addr - ADC_JDR1) / 4];
+    case ADC_DR:
+        if ((s->adc_cr2 & ADC_CR2_ADON) && (s->adc_cr2 & ADC_CR2_SWSTART)) {
+            s->adc_cr2 ^= ADC_CR2_SWSTART;
+            return stm32f2xx_adc_generate_value(s);
+        } else {
+            return 0x00000000;

Just "0" seems more readable to me.

include/hw/adc/stm32f2xx_adc.h line 67

Quoted 6 lines
include/hw/adc/stm32f2xx_adc.h new file +6 -0
+#ifdef RAND_MAX
+/* The rand() function is avaliable */
+#define RAND_AVAILABLE
+#undef RAND_MAX
+#define RAND_MAX 0xFF
+#endif /* RAND_MAX */

What platforms don't have rand()?
If we need an "exists everywhere" random number function then there is one in glib.

(but as noted earlier I don't think we should be using rand() here)

Quoted 17 lines
include/hw/adc/stm32f2xx_adc.h new file +17 -0
+
+typedef struct {
+    /* <private> */
+    SysBusDevice parent_obj;
+
+    /* <public> */
+    MemoryRegion mmio;
+
+    uint32_t adc_sr;
+    uint32_t adc_cr1;
+    uint32_t adc_cr2;
+    uint32_t adc_smpr1;
+    uint32_t adc_smpr2;
+    uint32_t adc_jofr[4];
+    uint32_t adc_htr;
+    uint32_t adc_ltr;
+    uint32_t adc_sqr1;

include/hw/adc/stm32f2xx_adc.h line 94

Quoted 10 lines
include/hw/adc/stm32f2xx_adc.h new file +10 -0
+    uint32_t adc_sqr2;
+    uint32_t adc_sqr3;
+    uint32_t adc_jsqr;
+    uint32_t adc_jdr[4];
+    uint32_t adc_dr;
+
+    qemu_irq irq;
+} STM32F2XXADCState;
+
+#endif /* HW_STM32F2XX_ADC_H */

You need to implement the VMState structure for migration.

thanks
-- PMM

author
#5 lore raw

Re: [Qemu-devel] [PATCH v3 3/7] STM32F2xx: Add the ADC device

Quoted 20 lines · Peter Maydell
Quoted 7 lines · Alistair Francis

Add the STM32F2xx ADC device. This device randomly generates values on each read.

This also includes creating a hw/adc directory.

Signed-off-by Alistair Francis <[email protected]>

hw/adc/stm32f2xx_adc.c line 74

Quoted 8 lines
hw/adc/stm32f2xx_adc.c new file +8 -0
+static uint32_t stm32f2xx_adc_generate_value(STM32F2XXADCState *s)
+{
+    /* Attempts to fake some ADC values */
+#ifdef RAND_AVALIABLE
+    s->adc_dr = s->adc_dr + rand();
+#else
+    s->adc_dr = s->adc_dr + 7;
+#endif

We shouldn't be using rand() in devices I think. (Among other things it means we won't be deterministic, which will break record-replay.)

Quoted 2 lines

In any case you've typoed your #ifdef constant name, which means that code is never used :-)

Woops, I didn't realise that. I'll take the rand() function out then.

I have made all of the other changes as well.

Thanks,

Alistair

Quoted 116 lines

hw/adc/stm32f2xx_adc.c line 106

Quoted 7 lines
hw/adc/stm32f2xx_adc.c new file +6 -0
+static uint64_t stm32f2xx_adc_read(void *opaque, hwaddr addr,
+                                     unsigned int size)
+{
+    STM32F2XXADCState *s = opaque;
+
+    DB_PRINT("Address: 0x%"HWADDR_PRIx"\n", addr);

Spaces around the HWADDR_PRIx would be nice.

Quoted 14 lines
hw/adc/stm32f2xx_adc.c new file +14 -0
+
+    if (addr >= ADC_COMMON_ADDRESS) {
+        qemu_log_mask(LOG_UNIMP,
+                      "%s: ADC Common Register Unsupported\n", __func__);
+    }
+
+    switch (addr) {
+    case ADC_SR:
+        return s->adc_sr;
+    case ADC_CR1:
+        return s->adc_cr1;
+    case ADC_CR2:
+        return s->adc_cr2 & 0xFFFFFFF;
+    case ADC_SMPR1:

hw/adc/stm32f2xx_adc.c line 130

Quoted 10 lines
hw/adc/stm32f2xx_adc.c new file +10 -0
+        return s->adc_smpr1;
+    case ADC_SMPR2:
+        return s->adc_smpr2;
+    case ADC_JOFR1:
+    case ADC_JOFR2:
+    case ADC_JOFR3:
+    case ADC_JOFR4:
+        qemu_log_mask(LOG_UNIMP, "%s: " \
+                      "Injection ADC is not implemented, the registers are " \
+                      "included for compatability\n", __func__);

"compatibility"

Quoted 21 lines
hw/adc/stm32f2xx_adc.c new file +21 -0
+        return s->adc_jofr[(addr - ADC_JOFR1) / 4];
+    case ADC_HTR:
+        return s->adc_htr;
+    case ADC_LTR:
+        return s->adc_ltr;
+    case ADC_SQR1:
+        return s->adc_sqr1;
+    case ADC_SQR2:
+        return s->adc_sqr2;
+    case ADC_SQR3:
+        return s->adc_sqr3;
+    case ADC_JSQR:
+        qemu_log_mask(LOG_UNIMP, "%s: " \
+                      "Injection ADC is not implemented, the registers are " \
+                      "included for compatability\n", __func__);
+        return s->adc_jsqr;
+    case ADC_JDR1:
+    case ADC_JDR2:
+    case ADC_JDR3:
+    case ADC_JDR4:
+        qemu_log_mask(LOG_UNIMP, "%s: " \

hw/adc/stm32f2xx_adc.c line 161

Quoted 10 lines
hw/adc/stm32f2xx_adc.c new file +10 -0
+                      "Injection ADC is not implemented, the registers are " \
+                      "included for compatability\n", __func__);
+        return s->adc_jdr[(addr - ADC_JDR1) / 4] -
+               s->adc_jofr[(addr - ADC_JDR1) / 4];
+    case ADC_DR:
+        if ((s->adc_cr2 & ADC_CR2_ADON) && (s->adc_cr2 & ADC_CR2_SWSTART)) {
+            s->adc_cr2 ^= ADC_CR2_SWSTART;
+            return stm32f2xx_adc_generate_value(s);
+        } else {
+            return 0x00000000;

Just "0" seems more readable to me.

include/hw/adc/stm32f2xx_adc.h line 67

Quoted 6 lines
include/hw/adc/stm32f2xx_adc.h new file +6 -0
+#ifdef RAND_MAX
+/* The rand() function is avaliable */
+#define RAND_AVAILABLE
+#undef RAND_MAX
+#define RAND_MAX 0xFF
+#endif /* RAND_MAX */

What platforms don't have rand()?
If we need an "exists everywhere" random number function then there is one in glib.

(but as noted earlier I don't think we should be using rand() here)

Quoted 17 lines
include/hw/adc/stm32f2xx_adc.h new file +17 -0
+
+typedef struct {
+    /* <private> */
+    SysBusDevice parent_obj;
+
+    /* <public> */
+    MemoryRegion mmio;
+
+    uint32_t adc_sr;
+    uint32_t adc_cr1;
+    uint32_t adc_cr2;
+    uint32_t adc_smpr1;
+    uint32_t adc_smpr2;
+    uint32_t adc_jofr[4];
+    uint32_t adc_htr;
+    uint32_t adc_ltr;
+    uint32_t adc_sqr1;

include/hw/adc/stm32f2xx_adc.h line 94

Quoted 10 lines
include/hw/adc/stm32f2xx_adc.h new file +10 -0
+    uint32_t adc_sqr2;
+    uint32_t adc_sqr3;
+    uint32_t adc_jsqr;
+    uint32_t adc_jdr[4];
+    uint32_t adc_dr;
+
+    qemu_irq irq;
+} STM32F2XXADCState;
+
+#endif /* HW_STM32F2XX_ADC_H */

You need to implement the VMState structure for migration.

thanks
-- PMM

author
#6 lore raw

PATCH v3 4/7 STM32F2xx: Add the SPI device

In reply to Alistair Francis

Add the STM32F2xx SPI device.

Signed-off-by Alistair Francis <[email protected]>

---
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
default-configs/arm-softmmu.mak +1 -0
index 520b209..af43eb1 100644
@@ -86,6 +86,7 @@ CONFIG_STM32F2XX_TIMER=y
 CONFIG_STM32F2XX_USART=y
 CONFIG_STM32F2XX_SYSCFG=y
 CONFIG_STM32F2XX_ADC=y
+CONFIG_STM32F2XX_SPI=y
 CONFIG_STM32F205_SOC=y
 
 CONFIG_VERSATILE_PCI=y
hw/ssi/Makefile.objs +1 -0
index 9555825..c674247 100644
@@ -2,5 +2,6 @@ common-obj-$(CONFIG_PL022) += pl022.o
 common-obj-$(CONFIG_SSI) += ssi.o
 common-obj-$(CONFIG_XILINX_SPI) += xilinx_spi.o
 common-obj-$(CONFIG_XILINX_SPIPS) += xilinx_spips.o
+common-obj-$(CONFIG_STM32F2XX_SPI) += stm32f2xx_spi.o
 
 obj-$(CONFIG_OMAP) += omap_spi.o
hw/ssi/stm32f2xx_spi.c new file +205 -0

205 lines not on this page · raw message

include/hw/ssi/stm32f2xx_spi.h new file +72 -0

72 lines not on this page · raw message

Signature · 1 line
2.5.0
#7 lore raw

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

Quoted 30 lines · Alistair Francis

Add the STM32F2xx SPI device.

Signed-off-by Alistair Francis <[email protected]>

---
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
hw/ssi/stm32f2xx_spi.c new file +15 -0
+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;

hw/ssi/stm32f2xx_spi.c line 88

Quoted 10 lines
hw/ssi/stm32f2xx_spi.c new file +10 -0
+    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.

Quoted 9 lines
hw/ssi/stm32f2xx_spi.c new file +9 -0
+        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;

hw/ssi/stm32f2xx_spi.c line 107

Quoted 10 lines
hw/ssi/stm32f2xx_spi.c new file +10 -0
+    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.

Quoted 4 lines
hw/ssi/stm32f2xx_spi.c new file +4 -0
+static void stm32f2xx_spi_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+

hw/ssi/stm32f2xx_spi.c line 198

Quoted 10 lines
hw/ssi/stm32f2xx_spi.c new file +10 -0
+    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?

Quoted 16 lines
include/hw/ssi/stm32f2xx_spi.h new file +15 -0
+
+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;

include/hw/ssi/stm32f2xx_spi.h line 70

Quoted 10 lines
include/hw/ssi/stm32f2xx_spi.h new file +10 -0
+    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

author
#8 lore raw

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

Quoted 35 lines · Peter Maydell
Quoted 34 lines · Alistair Francis

Add the STM32F2xx SPI device.

Signed-off-by Alistair Francis <[email protected]>

---
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
hw/ssi/stm32f2xx_spi.c new file +19 -0
+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:
Quoted 8 lines

hw/ssi/stm32f2xx_spi.c line 88

Quoted 6 lines
hw/ssi/stm32f2xx_spi.c new file +6 -0
+        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

Quoted 22 lines
Quoted 11 lines
hw/ssi/stm32f2xx_spi.c new file +9 -0
+        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;

hw/ssi/stm32f2xx_spi.c line 107

Quoted 10 lines
hw/ssi/stm32f2xx_spi.c new file +10 -0
+    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",
Quoted 1 line

Spaces, please.

Fixed

Quoted 17 lines
Quoted 5 lines
hw/ssi/stm32f2xx_spi.c new file +4 -0
+static void stm32f2xx_spi_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+

hw/ssi/stm32f2xx_spi.c line 198

Quoted 10 lines
hw/ssi/stm32f2xx_spi.c new file +10 -0
+    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.

Quoted 25 lines
Quoted 25 lines
include/hw/ssi/stm32f2xx_spi.h new file +23 -0
+
+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;
Quoted 8 lines

include/hw/ssi/stm32f2xx_spi.h line 70

Quoted 2 lines
include/hw/ssi/stm32f2xx_spi.h new file +2 -0
+    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

Quoted 3 lines

thanks
-- PMM

author
#9 lore raw

PATCH v3 5/7 STM32F205: Connect the ADC devices

In reply to Alistair Francis

Connect the ADC devices to the STM32F205 SoC.

Signed-off-by Alistair Francis <[email protected]>

---
V2:
- Fix up the device/devices commit message

 hw/arm/stm32f205_soc.c         | 22 ++++++++++++++++++++++
 include/hw/arm/stm32f205_soc.h |  3 +++
 2 files changed, 25 insertions(+)
hw/arm/stm32f205_soc.c +22 -0
index a2bd970..28d4301 100644
@@ -32,9 +32,12 @@ static const uint32_t timer_addr[STM_NUM_TIMERS] = { 0x40000000, 0x40000400,
     0x40000800, 0x40000C00 };
 static const uint32_t usart_addr[STM_NUM_USARTS] = { 0x40011000, 0x40004400,
     0x40004800, 0x40004C00, 0x40005000, 0x40011400 };
+static const uint32_t adc_addr[STM_NUM_ADCS] = { 0x40012000, 0x40012100,
+    0x40012200 };
 
 static const int timer_irq[STM_NUM_TIMERS] = {28, 29, 30, 50};
 static const int usart_irq[STM_NUM_USARTS] = {37, 38, 39, 52, 53, 71};
+#define ADC_IRQ 18
 
 static void stm32f205_soc_initfn(Object *obj)
 {

@@ -55,6 +58,12 @@ static void stm32f205_soc_initfn(Object *obj)
                           TYPE_STM32F2XX_TIMER);
         qdev_set_parent_bus(DEVICE(&s->timer[i]), sysbus_get_default());
     }
+
+    for (i = 0; i < STM_NUM_ADCS; i++) {
+        object_initialize(&s->adc[i], sizeof(s->adc[i]),
+                          TYPE_STM32F2XX_ADC);
+        qdev_set_parent_bus(DEVICE(&s->adc[i]), sysbus_get_default());
+    }
 }
 
 static void stm32f205_soc_realize(DeviceState *dev_soc, Error **errp)

@@ -128,6 +137,19 @@ static void stm32f205_soc_realize(DeviceState *dev_soc, Error **errp)
         sysbus_mmio_map(busdev, 0, timer_addr[i]);
         sysbus_connect_irq(busdev, 0, qdev_get_gpio_in(nvic, timer_irq[i]));
     }
+
+    /* ADC 1 to 3 */
+    for (i = 0; i < STM_NUM_ADCS; i++) {
+        dev = DEVICE(&(s->adc[i]));
+        object_property_set_bool(OBJECT(&s->adc[i]), true, "realized", &err);
+        if (err != NULL) {
+            error_propagate(errp, err);
+            return;
+        }
+        busdev = SYS_BUS_DEVICE(dev);
+        sysbus_mmio_map(busdev, 0, adc_addr[i]);
+        sysbus_connect_irq(busdev, 0, qdev_get_gpio_in(nvic, ADC_IRQ));
+    }
 }
 
 static Property stm32f205_soc_properties[] = {
include/hw/arm/stm32f205_soc.h +3 -0
index 0390eff..091e4be 100644
@@ -28,6 +28,7 @@
 #include "hw/misc/stm32f2xx_syscfg.h"
 #include "hw/timer/stm32f2xx_timer.h"
 #include "hw/char/stm32f2xx_usart.h"
+#include "hw/adc/stm32f2xx_adc.h"
 
 #define TYPE_STM32F205_SOC "stm32f205-soc"
 #define STM32F205_SOC(obj) \

@@ -35,6 +36,7 @@
 
 #define STM_NUM_USARTS 6
 #define STM_NUM_TIMERS 4
+#define STM_NUM_ADCS 3
 
 #define FLASH_BASE_ADDRESS 0x08000000
 #define FLASH_SIZE (1024 * 1024)

@@ -52,6 +54,7 @@ typedef struct STM32F205State {
     STM32F2XXSyscfgState syscfg;
     STM32F2XXUsartState usart[STM_NUM_USARTS];
     STM32F2XXTimerState timer[STM_NUM_TIMERS];
+    STM32F2XXADCState adc[STM_NUM_ADCS];
 } STM32F205State;
 
 #endif
Signature · 1 line
2.5.0
#10 lore raw

Re: [Qemu-devel] [PATCH v3 5/7] STM32F205: Connect the ADC devices

Quoted 16 lines · Alistair Francis

Connect the ADC devices to the STM32F205 SoC.

Signed-off-by Alistair Francis <[email protected]>

---
V2:
- Fix up the device/devices commit message

 hw/arm/stm32f205_soc.c         | 22 ++++++++++++++++++++++
 include/hw/arm/stm32f205_soc.h |  3 +++
 2 files changed, 25 insertions(+)
hw/arm/stm32f205_soc.c +0 -0
index a2bd970..28d4301 100644

hw/arm/stm32f205_soc.c static const uint32_t timer_addr[STM_NUM_TIMERS] = { 0x40000000, 0x40000400, · line 40

Quoted 9 lines
hw/arm/stm32f205_soc.c +3 -0
@@ -32,9 +32,12 @@ static const uint32_t timer_addr[STM_NUM_TIMERS] = { 0x40000000, 0x40000400,
     0x40000800, 0x40000C00 };
 static const uint32_t usart_addr[STM_NUM_USARTS] = { 0x40011000, 0x40004400,
     0x40004800, 0x40004C00, 0x40005000, 0x40011400 };
+static const uint32_t adc_addr[STM_NUM_ADCS] = { 0x40012000, 0x40012100,
+    0x40012200 };
 
 static const int timer_irq[STM_NUM_TIMERS] = {28, 29, 30, 50};
 static const int usart_irq[STM_NUM_USARTS] = {37, 38, 39, 52, 53, 71};
+#define ADC_IRQ 18

Really three devices but only one IRQ ?

hw/arm/stm32f205_soc.c static void stm32f205_soc_realize(DeviceState *dev_soc, Error **errp) · line 151

Quoted 11 lines
hw/arm/stm32f205_soc.c +11 -0
+    /* ADC 1 to 3 */
+    for (i = 0; i < STM_NUM_ADCS; i++) {
+        dev = DEVICE(&(s->adc[i]));
+        object_property_set_bool(OBJECT(&s->adc[i]), true, "realized", &err);
+        if (err != NULL) {
+            error_propagate(errp, err);
+            return;
+        }
+        busdev = SYS_BUS_DEVICE(dev);
+        sysbus_mmio_map(busdev, 0, adc_addr[i]);
+        sysbus_connect_irq(busdev, 0, qdev_get_gpio_in(nvic, ADC_IRQ));

You can't just wire multiple irq lines up like this; I think if you do then if devices A and B both assert the IRQ and then A deasserts it, then the receiving device will see an IRQ deassert when it should not (since B still holds it high).

thanks
-- PMM

author
#11 lore raw

Re: [Qemu-devel] [PATCH v3 5/7] STM32F205: Connect the ADC devices

Quoted 23 lines · Peter Maydell
Quoted 22 lines · Alistair Francis

Connect the ADC devices to the STM32F205 SoC.

Signed-off-by Alistair Francis <[email protected]>

---
V2:
- Fix up the device/devices commit message

 hw/arm/stm32f205_soc.c         | 22 ++++++++++++++++++++++
 include/hw/arm/stm32f205_soc.h |  3 +++
 2 files changed, 25 insertions(+)
hw/arm/stm32f205_soc.c +2 -0
index a2bd970..28d4301 100644
@@ -32,9 +32,12 @@ static const uint32_t timer_addr[STM_NUM_TIMERS] = { 0x40000000, 0x40000400,
     0x40000800, 0x40000C00 };
 static const uint32_t usart_addr[STM_NUM_USARTS] = { 0x40011000, 0x40004400,
     0x40004800, 0x40004C00, 0x40005000, 0x40011400 };
+static const uint32_t adc_addr[STM_NUM_ADCS] = { 0x40012000, 0x40012100,
+    0x40012200 };
Quoted 5 lines

hw/arm/stm32f205_soc.c static const uint32_t timer_addr[STM_NUM_TIMERS] = { 0x40000000, 0x40000400, · line 40

Quoted 3 lines
hw/arm/stm32f205_soc.c +1 -0
 static const int timer_irq[STM_NUM_TIMERS] = {28, 29, 30, 50};
 static const int usart_irq[STM_NUM_USARTS] = {37, 38, 39, 52, 53, 71};
+#define ADC_IRQ 18

Really three devices but only one IRQ ?

Yep, that's how HW does it. At least according to the reference manual.

Quoted 13 lines

hw/arm/stm32f205_soc.c static void stm32f205_soc_realize(DeviceState *dev_soc, Error **errp) · line 151

Quoted 12 lines
hw/arm/stm32f205_soc.c +11 -0
+    /* ADC 1 to 3 */
+    for (i = 0; i < STM_NUM_ADCS; i++) {
+        dev = DEVICE(&(s->adc[i]));
+        object_property_set_bool(OBJECT(&s->adc[i]), true, "realized", &err);
+        if (err != NULL) {
+            error_propagate(errp, err);
+            return;
+        }
+        busdev = SYS_BUS_DEVICE(dev);
+        sysbus_mmio_map(busdev, 0, adc_addr[i]);
+        sysbus_connect_irq(busdev, 0, qdev_get_gpio_in(nvic, ADC_IRQ));
Quoted 4 lines

You can't just wire multiple irq lines up like this; I think if you do then if devices A and B both assert the IRQ and then A deasserts it, then the receiving device will see an IRQ deassert when it should not (since B still holds it high).

I can't figure out if that is how HW actually does it. I can't find too much in the data sheet on how these interrupts behave.

In saying that, I am fine with what you described being the behaviour. I don't know any better way to connect the 3 devices to one interrupt line. Do you have any suggestions?

Thanks,

Alistair

Quoted 3 lines

thanks
-- PMM

#12 lore raw

Re: [Qemu-devel] [PATCH v3 5/7] STM32F205: Connect the ADC devices

Quoted 7 lines · Alistair Francis
Quoted 5 lines · Peter Maydell

On 19 January 2016 at 07:23, Alistair Francis <[email protected]> wrote:
You can't just wire multiple irq lines up like this; I think if you do then if devices A and B both assert the IRQ and then A deasserts it, then the receiving device will see an IRQ deassert when it should not (since B still holds it high).

Quoted 6 lines

I can't figure out if that is how HW actually does it. I can't find too much in the data sheet on how these interrupts behave.

In saying that, I am fine with what you described being the behaviour. I don't know any better way to connect the 3 devices to one interrupt line. Do you have any suggestions?

You're right that the data sheet is unclear, but I think the only vaguely plausible setup is that the three lines are ORed together. That way if any ADC asserts the line then the guest presumably looks at all of them to find which one has asserted it, and then writes to the register to acknowledge the interrupt. So if two ADCs assert at the same time, the guest will still (correctly) see an interrupt until it acks the second ADC.

Unfortunately we don't have a qemu_irq OR gate at the moment I think, but it's a pretty simple thing to write.

thanks
-- PMM

author
#13 lore raw

PATCH v3 7/7 MAINTAINERS: Add Alistair to the maintainers list

In reply to Alistair Francis

Add Alistair Francis as the maintainer for the Netduino 2 and SMM32F205 SoC.

Signed-off-by Alistair Francis <[email protected]>
Reviewed-by Peter Crosthwaite <[email protected]>

---

 MAINTAINERS | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
MAINTAINERS +15 -0
index 4030e27..24aaef4 100644
@@ -457,6 +457,21 @@ S: Maintained
 F: hw/arm/virt-acpi-build.c
 F: include/hw/arm/virt-acpi-build.h
 
+STM32F205
+M: Alistair Francis <[email protected]>
+S: Maintained
+F: hw/arm/stm32f205_soc.c
+F: hw/misc/stm32f2xx_syscfg.c
+F: hw/char/stm32f2xx_usart.c
+F: hw/timer/stm32f2xx_timer.c
+F: hw/adc/*
+F: hw/ssi/stm32f2xx_spi.c
+
+Netduino 2
+M: Alistair Francis <[email protected]>
+S: Maintained
+F: hw/arm/netduino2.c
+
 CRIS Machines
 -------------
 Axis Dev88
Signature · 1 line
2.5.0