* [Qemu-devel] [PATCH v1 1/5] Netduino_USART: Add the Netduino Plus 2 USART Controller
2014-08-24 0:13 [Qemu-devel] [PATCH v1 0/5] Netduino Plus 2 Machine Model Alistair Francis
@ 2014-08-24 0:13 ` Alistair Francis
2014-08-24 2:09 ` Peter Crosthwaite
2014-08-24 0:13 ` [Qemu-devel] [PATCH v1 2/5] Netduino_GPIO: Add the Netduino Plus 2 GPIO controller Alistair Francis
` (4 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: Alistair Francis @ 2014-08-24 0:13 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, peter.crosthwaite, konstanty
This patch adds the Netduino Plus 2 USART controller
(UART also uses the same controller).
It can be used for reading and writing to the guest.
Signed-off-by: Alistair Francis <alistair23@gmail.com>
---
default-configs/arm-softmmu.mak | 1 +
hw/char/Makefile.objs | 1 +
hw/char/netduino_usart.c | 252 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 254 insertions(+)
create mode 100644 hw/char/netduino_usart.c
diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index f3513fa..46471ad 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -78,6 +78,7 @@ CONFIG_NSERIES=y
CONFIG_REALVIEW=y
CONFIG_ZAURUS=y
CONFIG_ZYNQ=y
+CONFIG_NETDUINOP2=y
CONFIG_VERSATILE_PCI=y
CONFIG_VERSATILE_I2C=y
diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
index 317385d..c03aa98 100644
--- a/hw/char/Makefile.objs
+++ b/hw/char/Makefile.objs
@@ -15,6 +15,7 @@ obj-$(CONFIG_OMAP) += omap_uart.o
obj-$(CONFIG_SH4) += sh_serial.o
obj-$(CONFIG_PSERIES) += spapr_vty.o
obj-$(CONFIG_DIGIC) += digic-uart.o
+obj-$(CONFIG_NETDUINOP2) += netduino_usart.o
common-obj-$(CONFIG_ETRAXFS) += etraxfs_ser.o
common-obj-$(CONFIG_ISA_DEBUG) += debugcon.o
diff --git a/hw/char/netduino_usart.c b/hw/char/netduino_usart.c
new file mode 100644
index 0000000..437af2b
--- /dev/null
+++ b/hw/char/netduino_usart.c
@@ -0,0 +1,252 @@
+/*
+ * Netduino Plus 2 USART
+ *
+ * Copyright (c) 2014 Alistair Francis <alistair@alistair23.me>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "hw/sysbus.h"
+#include "sysemu/char.h"
+#include "hw/hw.h"
+
+/* #define DEBUG_NETUSART */
+
+#ifdef DEBUG_NETUSART
+#define DPRINTF(fmt, ...) \
+do { printf("netduino_usart: " fmt , ## __VA_ARGS__); } while (0)
+#else
+#define DPRINTF(fmt, ...) do {} while (0)
+#endif
+
+#define USART_SR 0x00
+#define USART_DR 0x04
+#define USART_BRR 0x08
+#define USART_CR1 0x0C
+#define USART_CR2 0x10
+#define USART_CR3 0x14
+#define USART_GTPR 0x18
+
+#define USART_SR_TXE (1 << 7)
+#define USART_SR_TC (1 << 6)
+#define USART_SR_RXNE (1 << 5)
+
+#define USART_CR1_UE (1 << 13)
+#define USART_CR1_RXNEIE (1 << 5)
+#define USART_CR1_TE (1 << 3)
+
+#define TYPE_NETDUINO_USART "netduino_usart"
+#define NETDUINO_USART(obj) \
+ OBJECT_CHECK(struct net_usart, (obj), TYPE_NETDUINO_USART)
+
+struct net_usart {
+ SysBusDevice parent_obj;
+
+ MemoryRegion mmio;
+
+ uint32_t usart_sr;
+ uint32_t usart_dr;
+ uint32_t usart_brr;
+ uint32_t usart_cr1;
+ uint32_t usart_cr2;
+ uint32_t usart_cr3;
+ uint32_t usart_gtpr;
+
+ CharDriverState *chr;
+ qemu_irq irq;
+};
+
+static int usart_can_receive(void *opaque)
+{
+ struct net_usart *s = opaque;
+
+ if (s->usart_cr1 & USART_CR1_UE && s->usart_cr1 & USART_CR1_TE) {
+ return 1;
+ }
+
+ return 0;
+}
+
+static void usart_receive(void *opaque, const uint8_t *buf, int size)
+{
+ struct net_usart *s = opaque;
+ int i, num;
+
+ num = size > 8 ? 8 : size;
+ s->usart_dr = 0;
+
+ for (i = 0; i < num; i++) {
+ s->usart_dr |= (buf[i] << i);
+ }
+
+ s->usart_sr |= USART_SR_RXNE;
+
+ if (s->usart_cr1 & USART_CR1_RXNEIE) {
+ qemu_set_irq(s->irq, 1);
+ }
+
+ DPRINTF("Inputting: %c\n", s->usart_dr);
+}
+
+static void usart_reset(DeviceState *dev)
+{
+ struct net_usart *s = NETDUINO_USART(dev);
+
+ s->usart_sr = 0x00C00000;
+ s->usart_dr = 0x00000000;
+ s->usart_brr = 0x00000000;
+ s->usart_cr1 = 0x00000000;
+ s->usart_cr2 = 0x00000000;
+ s->usart_cr3 = 0x00000000;
+ s->usart_gtpr = 0x00000000;
+}
+
+static uint64_t netduino_usart_read(void *opaque, hwaddr addr,
+ unsigned int size)
+{
+ struct net_usart *s = opaque;
+ uint64_t retvalue;
+
+ DPRINTF("Read 0x%x\n", (uint) addr);
+
+ switch (addr) {
+ case USART_SR:
+ retvalue = s->usart_sr;
+ s->usart_sr &= (USART_SR_TC ^ 0xFFFF);
+ return retvalue;
+ case USART_DR:
+ DPRINTF("Value: %x, %c\n", s->usart_dr, s->usart_dr);
+ s->usart_sr |= USART_SR_TXE;
+ return s->usart_dr & 0x3FF;
+ case USART_BRR:
+ return s->usart_brr;
+ case USART_CR1:
+ return s->usart_cr1;
+ case USART_CR2:
+ return s->usart_cr2;
+ case USART_CR3:
+ return s->usart_cr3;
+ case USART_GTPR:
+ return s->usart_gtpr;
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "net_usart_read: Bad offset %x\n", (int)addr);
+ return 0;
+ }
+
+ return 0;
+}
+
+static void netduino_usart_write(void *opaque, hwaddr addr,
+ uint64_t val64, unsigned int size)
+{
+ struct net_usart *s = opaque;
+ uint32_t value = (uint32_t) val64;
+ unsigned char ch;
+
+ DPRINTF("Write 0x%x, 0x%x\n", value, (uint) addr);
+
+ switch (addr) {
+ case USART_SR:
+ if (value <= 0x3FF) {
+ s->usart_sr = value;
+ } else {
+ s->usart_sr &= value;
+ }
+ return;
+ case USART_DR:
+ if (value < 0xF000) {
+ ch = value;
+ if (s->chr) {
+ qemu_chr_fe_write(s->chr, &ch, 1);
+ }
+ s->usart_sr |= USART_SR_TC;
+ }
+ return;
+ case USART_BRR:
+ s->usart_brr = value;
+ return;
+ case USART_CR1:
+ s->usart_cr1 = value;
+ return;
+ case USART_CR2:
+ s->usart_cr2 = value;
+ return;
+ case USART_CR3:
+ s->usart_cr3 = value;
+ return;
+ case USART_GTPR:
+ s->usart_gtpr = value;
+ return;
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "net_usart_write: Bad offset %x\n", (int)addr);
+ }
+}
+
+static const MemoryRegionOps netduino_usart_ops = {
+ .read = netduino_usart_read,
+ .write = netduino_usart_write,
+ .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static int netduino_usart_init(SysBusDevice *sbd)
+{
+ DeviceState *dev = DEVICE(sbd);
+ struct net_usart *s = NETDUINO_USART(dev);
+
+ sysbus_init_irq(sbd, &s->irq);
+
+ memory_region_init_io(&s->mmio, OBJECT(s), &netduino_usart_ops, s,
+ TYPE_NETDUINO_USART, 0x2000);
+ sysbus_init_mmio(sbd, &s->mmio);
+
+ s->chr = qemu_char_get_next_serial();
+
+ if (s->chr) {
+ qemu_chr_add_handlers(s->chr, usart_can_receive, usart_receive,
+ NULL, s);
+ }
+
+ return 0;
+}
+
+static void netduino_usart_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+
+ k->init = netduino_usart_init;
+ dc->props = NULL;
+ dc->reset = usart_reset;
+}
+
+static const TypeInfo netduino_usart_info = {
+ .name = TYPE_NETDUINO_USART,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(struct net_usart),
+ .class_init = netduino_usart_class_init,
+};
+
+static void netduino_usart_register_types(void)
+{
+ type_register_static(&netduino_usart_info);
+}
+
+type_init(netduino_usart_register_types)
--
1.9.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v1 1/5] Netduino_USART: Add the Netduino Plus 2 USART Controller
2014-08-24 0:13 ` [Qemu-devel] [PATCH v1 1/5] Netduino_USART: Add the Netduino Plus 2 USART Controller Alistair Francis
@ 2014-08-24 2:09 ` Peter Crosthwaite
2014-08-26 14:19 ` Alistair Francis
2014-09-01 16:30 ` Peter Maydell
0 siblings, 2 replies; 22+ messages in thread
From: Peter Crosthwaite @ 2014-08-24 2:09 UTC (permalink / raw)
To: Alistair Francis
Cc: Peter Maydell, qemu-devel@nongnu.org Developers, Konstanty Bialkowski
On Sun, Aug 24, 2014 at 10:13 AM, Alistair Francis <alistair23@gmail.com> wrote:
> This patch adds the Netduino Plus 2 USART controller
> (UART also uses the same controller).
>
> It can be used for reading and writing to the guest.
>
Do you just mean doing IO with the system?
> Signed-off-by: Alistair Francis <alistair23@gmail.com>
> ---
> default-configs/arm-softmmu.mak | 1 +
> hw/char/Makefile.objs | 1 +
> hw/char/netduino_usart.c | 252 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 254 insertions(+)
> create mode 100644 hw/char/netduino_usart.c
>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index f3513fa..46471ad 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -78,6 +78,7 @@ CONFIG_NSERIES=y
> CONFIG_REALVIEW=y
> CONFIG_ZAURUS=y
> CONFIG_ZYNQ=y
> +CONFIG_NETDUINOP2=y
>
> CONFIG_VERSATILE_PCI=y
> CONFIG_VERSATILE_I2C=y
> diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
> index 317385d..c03aa98 100644
> --- a/hw/char/Makefile.objs
> +++ b/hw/char/Makefile.objs
> @@ -15,6 +15,7 @@ obj-$(CONFIG_OMAP) += omap_uart.o
> obj-$(CONFIG_SH4) += sh_serial.o
> obj-$(CONFIG_PSERIES) += spapr_vty.o
> obj-$(CONFIG_DIGIC) += digic-uart.o
> +obj-$(CONFIG_NETDUINOP2) += netduino_usart.o
>
> common-obj-$(CONFIG_ETRAXFS) += etraxfs_ser.o
> common-obj-$(CONFIG_ISA_DEBUG) += debugcon.o
> diff --git a/hw/char/netduino_usart.c b/hw/char/netduino_usart.c
> new file mode 100644
> index 0000000..437af2b
> --- /dev/null
> +++ b/hw/char/netduino_usart.c
> @@ -0,0 +1,252 @@
> +/*
> + * Netduino Plus 2 USART
> + *
I thing the UART IP is really part of the ST Microcontroller, and
should be named as such. Do ST give this controller a meaningful name
in their documentation? Looking at the schematic for Netduino it looks
like there is very little board level IP. All the sysbus IP is local
to the microcontroller. I think the name "netduino" should probably
only be limited to the board level.
> + * Copyright (c) 2014 Alistair Francis <alistair@alistair23.me>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "hw/sysbus.h"
> +#include "sysemu/char.h"
> +#include "hw/hw.h"
> +
> +/* #define DEBUG_NETUSART */
Don't worry about commented out //#defines
> +
> +#ifdef DEBUG_NETUSART
> +#define DPRINTF(fmt, ...) \
You should change this to a always-compiled print using a regular if.
Check hw/dma/pl330.c
> +do { printf("netduino_usart: " fmt , ## __VA_ARGS__); } while (0)
Multiple lines to keep formatting standard even within #define. use \ as needed.
Use qemu_log instead of printf. (pl330 fails at this too).
> +#else
> +#define DPRINTF(fmt, ...) do {} while (0)
> +#endif
> +
> +#define USART_SR 0x00
> +#define USART_DR 0x04
> +#define USART_BRR 0x08
> +#define USART_CR1 0x0C
> +#define USART_CR2 0x10
> +#define USART_CR3 0x14
> +#define USART_GTPR 0x18
> +
> +#define USART_SR_TXE (1 << 7)
> +#define USART_SR_TC (1 << 6)
> +#define USART_SR_RXNE (1 << 5)
> +
> +#define USART_CR1_UE (1 << 13)
> +#define USART_CR1_RXNEIE (1 << 5)
> +#define USART_CR1_TE (1 << 3)
> +
> +#define TYPE_NETDUINO_USART "netduino_usart"
Change "_" to "-" in QOM typename.
> +#define NETDUINO_USART(obj) \
> + OBJECT_CHECK(struct net_usart, (obj), TYPE_NETDUINO_USART)
> +
> +struct net_usart {
CamelCase in typename. Use a typedef to avoid the repeated struct Foo
all throughout.
> + SysBusDevice parent_obj;
> +
> + MemoryRegion mmio;
> +
> + uint32_t usart_sr;
> + uint32_t usart_dr;
> + uint32_t usart_brr;
> + uint32_t usart_cr1;
> + uint32_t usart_cr2;
> + uint32_t usart_cr3;
> + uint32_t usart_gtpr;
> +
> + CharDriverState *chr;
> + qemu_irq irq;
> +};
The struct, QOM typename and register defines should go in a device
specific header. Check include/hw/char/digic_uart.h for an example.
> +
> +static int usart_can_receive(void *opaque)
> +{
> + struct net_usart *s = opaque;
> +
> + if (s->usart_cr1 & USART_CR1_UE && s->usart_cr1 & USART_CR1_TE) {
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +static void usart_receive(void *opaque, const uint8_t *buf, int size)
> +{
> + struct net_usart *s = opaque;
> + int i, num;
> +
> + num = size > 8 ? 8 : size;
MAX macro.
> + s->usart_dr = 0;
> +
> + for (i = 0; i < num; i++) {
> + s->usart_dr |= (buf[i] << i);
> + }
This seems strange. dr is an oring of multiple rxed characters? Is RX
information lost here when size > 1?
> +
> + s->usart_sr |= USART_SR_RXNE;
> +
> + if (s->usart_cr1 & USART_CR1_RXNEIE) {
> + qemu_set_irq(s->irq, 1);
> + }
> +
> + DPRINTF("Inputting: %c\n", s->usart_dr);
"receiving"
> +}
> +
> +static void usart_reset(DeviceState *dev)
> +{
> + struct net_usart *s = NETDUINO_USART(dev);
> +
> + s->usart_sr = 0x00C00000;
> + s->usart_dr = 0x00000000;
> + s->usart_brr = 0x00000000;
> + s->usart_cr1 = 0x00000000;
> + s->usart_cr2 = 0x00000000;
> + s->usart_cr3 = 0x00000000;
> + s->usart_gtpr = 0x00000000;
> +}
> +
> +static uint64_t netduino_usart_read(void *opaque, hwaddr addr,
> + unsigned int size)
> +{
> + struct net_usart *s = opaque;
> + uint64_t retvalue;
> +
> + DPRINTF("Read 0x%x\n", (uint) addr);
> +
> + switch (addr) {
> + case USART_SR:
> + retvalue = s->usart_sr;
> + s->usart_sr &= (USART_SR_TC ^ 0xFFFF);
> + return retvalue;
> + case USART_DR:
> + DPRINTF("Value: %x, %c\n", s->usart_dr, s->usart_dr);
PRIx32 instead of %x. Cast the %c version.
> + s->usart_sr |= USART_SR_TXE;
> + return s->usart_dr & 0x3FF;
> + case USART_BRR:
> + return s->usart_brr;
> + case USART_CR1:
> + return s->usart_cr1;
> + case USART_CR2:
> + return s->usart_cr2;
> + case USART_CR3:
> + return s->usart_cr3;
> + case USART_GTPR:
> + return s->usart_gtpr;
If you make your registers an array, you can index the with the offset
and remove the repeated s->foo return logic. You can also use a single
memset to do all the 0 resets.
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "net_usart_read: Bad offset %x\n", (int)addr);
> + return 0;
> + }
> +
> + return 0;
> +}
> +
> +static void netduino_usart_write(void *opaque, hwaddr addr,
> + uint64_t val64, unsigned int size)
> +{
> + struct net_usart *s = opaque;
> + uint32_t value = (uint32_t) val64;
> + unsigned char ch;
> +
> + DPRINTF("Write 0x%x, 0x%x\n", value, (uint) addr);
> +
> + switch (addr) {
> + case USART_SR:
> + if (value <= 0x3FF) {
> + s->usart_sr = value;
> + } else {
> + s->usart_sr &= value;
> + }
> + return;
> + case USART_DR:
> + if (value < 0xF000) {
> + ch = value;
> + if (s->chr) {
> + qemu_chr_fe_write(s->chr, &ch, 1);
> + }
> + s->usart_sr |= USART_SR_TC;
> + }
> + return;
> + case USART_BRR:
> + s->usart_brr = value;
> + return;
> + case USART_CR1:
> + s->usart_cr1 = value;
> + return;
> + case USART_CR2:
> + s->usart_cr2 = value;
> + return;
> + case USART_CR3:
> + s->usart_cr3 = value;
> + return;
> + case USART_GTPR:
> + s->usart_gtpr = value;
> + return;
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "net_usart_write: Bad offset %x\n", (int)addr);
> + }
> +}
> +
> +static const MemoryRegionOps netduino_usart_ops = {
> + .read = netduino_usart_read,
> + .write = netduino_usart_write,
> + .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static int netduino_usart_init(SysBusDevice *sbd)
> +{
> + DeviceState *dev = DEVICE(sbd);
> + struct net_usart *s = NETDUINO_USART(dev);
> +
> + sysbus_init_irq(sbd, &s->irq);
> +
> + memory_region_init_io(&s->mmio, OBJECT(s), &netduino_usart_ops, s,
> + TYPE_NETDUINO_USART, 0x2000);
> + sysbus_init_mmio(sbd, &s->mmio);
> +
> + s->chr = qemu_char_get_next_serial();
> +
> + if (s->chr) {
> + qemu_chr_add_handlers(s->chr, usart_can_receive, usart_receive,
> + NULL, s);
> + }
> +
> + return 0;
> +}
> +
> +static void netduino_usart_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +
> + k->init = netduino_usart_init;
Use of Sysbus::init is deprecated, please use device init and realize instead.
> + dc->props = NULL;
Unneeded.
Some comments also apply to the following patches.
Regards,
Peter
> + dc->reset = usart_reset;
> +}
> +
> +static const TypeInfo netduino_usart_info = {
> + .name = TYPE_NETDUINO_USART,
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(struct net_usart),
> + .class_init = netduino_usart_class_init,
> +};
> +
> +static void netduino_usart_register_types(void)
> +{
> + type_register_static(&netduino_usart_info);
> +}
> +
> +type_init(netduino_usart_register_types)
> --
> 1.9.1
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v1 1/5] Netduino_USART: Add the Netduino Plus 2 USART Controller
2014-08-24 2:09 ` Peter Crosthwaite
@ 2014-08-26 14:19 ` Alistair Francis
2014-09-01 16:30 ` Peter Maydell
1 sibling, 0 replies; 22+ messages in thread
From: Alistair Francis @ 2014-08-26 14:19 UTC (permalink / raw)
To: Peter Crosthwaite
Cc: Peter Maydell, qemu-devel@nongnu.org Developers, Konstanty Bialkowski
On Sun, Aug 24, 2014 at 12:09 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Sun, Aug 24, 2014 at 10:13 AM, Alistair Francis <alistair23@gmail.com> wrote:
>> This patch adds the Netduino Plus 2 USART controller
>> (UART also uses the same controller).
>>
>> It can be used for reading and writing to the guest.
>>
>
> Do you just mean doing IO with the system?
Sorry, that was a bit confusing. At the moment it's just with itself
>> Signed-off-by: Alistair Francis <alistair23@gmail.com>
>> ---
>> default-configs/arm-softmmu.mak | 1 +
>> hw/char/Makefile.objs | 1 +
>> hw/char/netduino_usart.c | 252 ++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 254 insertions(+)
>> create mode 100644 hw/char/netduino_usart.c
>>
>> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
>> index f3513fa..46471ad 100644
>> --- a/default-configs/arm-softmmu.mak
>> +++ b/default-configs/arm-softmmu.mak
>> @@ -78,6 +78,7 @@ CONFIG_NSERIES=y
>> CONFIG_REALVIEW=y
>> CONFIG_ZAURUS=y
>> CONFIG_ZYNQ=y
>> +CONFIG_NETDUINOP2=y
>>
>> CONFIG_VERSATILE_PCI=y
>> CONFIG_VERSATILE_I2C=y
>> diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs
>> index 317385d..c03aa98 100644
>> --- a/hw/char/Makefile.objs
>> +++ b/hw/char/Makefile.objs
>> @@ -15,6 +15,7 @@ obj-$(CONFIG_OMAP) += omap_uart.o
>> obj-$(CONFIG_SH4) += sh_serial.o
>> obj-$(CONFIG_PSERIES) += spapr_vty.o
>> obj-$(CONFIG_DIGIC) += digic-uart.o
>> +obj-$(CONFIG_NETDUINOP2) += netduino_usart.o
>>
>> common-obj-$(CONFIG_ETRAXFS) += etraxfs_ser.o
>> common-obj-$(CONFIG_ISA_DEBUG) += debugcon.o
>> diff --git a/hw/char/netduino_usart.c b/hw/char/netduino_usart.c
>> new file mode 100644
>> index 0000000..437af2b
>> --- /dev/null
>> +++ b/hw/char/netduino_usart.c
>> @@ -0,0 +1,252 @@
>> +/*
>> + * Netduino Plus 2 USART
>> + *
>
> I thing the UART IP is really part of the ST Microcontroller, and
> should be named as such. Do ST give this controller a meaningful name
> in their documentation? Looking at the schematic for Netduino it looks
> like there is very little board level IP. All the sysbus IP is local
> to the microcontroller. I think the name "netduino" should probably
> only be limited to the board level.
ST don't give it a name, besides USART. I can change change the
Netduino part to 'STM32F405xx'. That probably models the documentation
the best.
>
>> + * Copyright (c) 2014 Alistair Francis <alistair@alistair23.me>
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#include "hw/sysbus.h"
>> +#include "sysemu/char.h"
>> +#include "hw/hw.h"
>> +
>> +/* #define DEBUG_NETUSART */
>
> Don't worry about commented out //#defines
>
>> +
>> +#ifdef DEBUG_NETUSART
>> +#define DPRINTF(fmt, ...) \
>
> You should change this to a always-compiled print using a regular if.
> Check hw/dma/pl330.c
Yep, can do
>
>> +do { printf("netduino_usart: " fmt , ## __VA_ARGS__); } while (0)
>
> Multiple lines to keep formatting standard even within #define. use \ as needed.
>
> Use qemu_log instead of printf. (pl330 fails at this too).
>
Will do
>> +#else
>> +#define DPRINTF(fmt, ...) do {} while (0)
>> +#endif
>> +
>> +#define USART_SR 0x00
>> +#define USART_DR 0x04
>> +#define USART_BRR 0x08
>> +#define USART_CR1 0x0C
>> +#define USART_CR2 0x10
>> +#define USART_CR3 0x14
>> +#define USART_GTPR 0x18
>> +
>> +#define USART_SR_TXE (1 << 7)
>> +#define USART_SR_TC (1 << 6)
>> +#define USART_SR_RXNE (1 << 5)
>> +
>> +#define USART_CR1_UE (1 << 13)
>> +#define USART_CR1_RXNEIE (1 << 5)
>> +#define USART_CR1_TE (1 << 3)
>> +
>> +#define TYPE_NETDUINO_USART "netduino_usart"
>
> Change "_" to "-" in QOM typename.
>
Done
>> +#define NETDUINO_USART(obj) \
>> + OBJECT_CHECK(struct net_usart, (obj), TYPE_NETDUINO_USART)
>> +
>> +struct net_usart {
>
> CamelCase in typename. Use a typedef to avoid the repeated struct Foo
> all throughout.
>
Done
>> + SysBusDevice parent_obj;
>> +
>> + MemoryRegion mmio;
>> +
>> + uint32_t usart_sr;
>> + uint32_t usart_dr;
>> + uint32_t usart_brr;
>> + uint32_t usart_cr1;
>> + uint32_t usart_cr2;
>> + uint32_t usart_cr3;
>> + uint32_t usart_gtpr;
>> +
>> + CharDriverState *chr;
>> + qemu_irq irq;
>> +};
>
> The struct, QOM typename and register defines should go in a device
> specific header. Check include/hw/char/digic_uart.h for an example.
>
Is that necessary for this case? All of these are only called from one place
and there aren't that many lines to be moved to a header.
>> +
>> +static int usart_can_receive(void *opaque)
>> +{
>> + struct net_usart *s = opaque;
>> +
>> + if (s->usart_cr1 & USART_CR1_UE && s->usart_cr1 & USART_CR1_TE) {
>> + return 1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void usart_receive(void *opaque, const uint8_t *buf, int size)
>> +{
>> + struct net_usart *s = opaque;
>> + int i, num;
>> +
>> + num = size > 8 ? 8 : size;
>
> MAX macro.
>
Will do
>> + s->usart_dr = 0;
>> +
>> + for (i = 0; i < num; i++) {
>> + s->usart_dr |= (buf[i] << i);
>> + }
>
> This seems strange. dr is an oring of multiple rxed characters? Is RX
> information lost here when size > 1?
>
This is really wrong, it will be accessing invalid memory if size > 1
I will fix this, I am getting too used to VHDL
>> +
>> + s->usart_sr |= USART_SR_RXNE;
>> +
>> + if (s->usart_cr1 & USART_CR1_RXNEIE) {
>> + qemu_set_irq(s->irq, 1);
>> + }
>> +
>> + DPRINTF("Inputting: %c\n", s->usart_dr);
>
> "receiving"
>
Done
>> +}
>> +
>> +static void usart_reset(DeviceState *dev)
>> +{
>> + struct net_usart *s = NETDUINO_USART(dev);
>> +
>> + s->usart_sr = 0x00C00000;
>> + s->usart_dr = 0x00000000;
>> + s->usart_brr = 0x00000000;
>> + s->usart_cr1 = 0x00000000;
>> + s->usart_cr2 = 0x00000000;
>> + s->usart_cr3 = 0x00000000;
>> + s->usart_gtpr = 0x00000000;
>> +}
>> +
>> +static uint64_t netduino_usart_read(void *opaque, hwaddr addr,
>> + unsigned int size)
>> +{
>> + struct net_usart *s = opaque;
>> + uint64_t retvalue;
>> +
>> + DPRINTF("Read 0x%x\n", (uint) addr);
>> +
>> + switch (addr) {
>> + case USART_SR:
>> + retvalue = s->usart_sr;
>> + s->usart_sr &= (USART_SR_TC ^ 0xFFFF);
>> + return retvalue;
>> + case USART_DR:
>> + DPRINTF("Value: %x, %c\n", s->usart_dr, s->usart_dr);
>
> PRIx32 instead of %x. Cast the %c version.
>
>> + s->usart_sr |= USART_SR_TXE;
>> + return s->usart_dr & 0x3FF;
>> + case USART_BRR:
>> + return s->usart_brr;
>> + case USART_CR1:
>> + return s->usart_cr1;
>> + case USART_CR2:
>> + return s->usart_cr2;
>> + case USART_CR3:
>> + return s->usart_cr3;
>> + case USART_GTPR:
>> + return s->usart_gtpr;
>
> If you make your registers an array, you can index the with the offset
> and remove the repeated s->foo return logic. You can also use a single
> memset to do all the 0 resets.
>
Does that comply with QEMU's style. I haven't seen any other devices
that do that.
>> + default:
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "net_usart_read: Bad offset %x\n", (int)addr);
>> + return 0;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void netduino_usart_write(void *opaque, hwaddr addr,
>> + uint64_t val64, unsigned int size)
>> +{
>> + struct net_usart *s = opaque;
>> + uint32_t value = (uint32_t) val64;
>> + unsigned char ch;
>> +
>> + DPRINTF("Write 0x%x, 0x%x\n", value, (uint) addr);
>> +
>> + switch (addr) {
>> + case USART_SR:
>> + if (value <= 0x3FF) {
>> + s->usart_sr = value;
>> + } else {
>> + s->usart_sr &= value;
>> + }
>> + return;
>> + case USART_DR:
>> + if (value < 0xF000) {
>> + ch = value;
>> + if (s->chr) {
>> + qemu_chr_fe_write(s->chr, &ch, 1);
>> + }
>> + s->usart_sr |= USART_SR_TC;
>> + }
>> + return;
>> + case USART_BRR:
>> + s->usart_brr = value;
>> + return;
>> + case USART_CR1:
>> + s->usart_cr1 = value;
>> + return;
>> + case USART_CR2:
>> + s->usart_cr2 = value;
>> + return;
>> + case USART_CR3:
>> + s->usart_cr3 = value;
>> + return;
>> + case USART_GTPR:
>> + s->usart_gtpr = value;
>> + return;
>> + default:
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "net_usart_write: Bad offset %x\n", (int)addr);
>> + }
>> +}
>> +
>> +static const MemoryRegionOps netduino_usart_ops = {
>> + .read = netduino_usart_read,
>> + .write = netduino_usart_write,
>> + .endianness = DEVICE_NATIVE_ENDIAN,
>> +};
>> +
>> +static int netduino_usart_init(SysBusDevice *sbd)
>> +{
>> + DeviceState *dev = DEVICE(sbd);
>> + struct net_usart *s = NETDUINO_USART(dev);
>> +
>> + sysbus_init_irq(sbd, &s->irq);
>> +
>> + memory_region_init_io(&s->mmio, OBJECT(s), &netduino_usart_ops, s,
>> + TYPE_NETDUINO_USART, 0x2000);
>> + sysbus_init_mmio(sbd, &s->mmio);
>> +
>> + s->chr = qemu_char_get_next_serial();
>> +
>> + if (s->chr) {
>> + qemu_chr_add_handlers(s->chr, usart_can_receive, usart_receive,
>> + NULL, s);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void netduino_usart_class_init(ObjectClass *klass, void *data)
>> +{
>> + DeviceClass *dc = DEVICE_CLASS(klass);
>> + SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>> +
>> + k->init = netduino_usart_init;
>
> Use of Sysbus::init is deprecated, please use device init and realize instead.
>
Yep, will fix
>> + dc->props = NULL;
>
> Unneeded.
>
> Some comments also apply to the following patches.
Thanks, I'll make the changes
>
> Regards,
> Peter
>
>> + dc->reset = usart_reset;
>> +}
>> +
>> +static const TypeInfo netduino_usart_info = {
>> + .name = TYPE_NETDUINO_USART,
>> + .parent = TYPE_SYS_BUS_DEVICE,
>> + .instance_size = sizeof(struct net_usart),
>> + .class_init = netduino_usart_class_init,
>> +};
>> +
>> +static void netduino_usart_register_types(void)
>> +{
>> + type_register_static(&netduino_usart_info);
>> +}
>> +
>> +type_init(netduino_usart_register_types)
>> --
>> 1.9.1
>>
>>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v1 1/5] Netduino_USART: Add the Netduino Plus 2 USART Controller
2014-08-24 2:09 ` Peter Crosthwaite
2014-08-26 14:19 ` Alistair Francis
@ 2014-09-01 16:30 ` Peter Maydell
2014-09-02 2:04 ` Alistair Francis
1 sibling, 1 reply; 22+ messages in thread
From: Peter Maydell @ 2014-09-01 16:30 UTC (permalink / raw)
To: Peter Crosthwaite
Cc: Alistair Francis, qemu-devel@nongnu.org Developers, Konstanty Bialkowski
On 24 August 2014 03:09, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> If you make your registers an array, you can index the with the offset
> and remove the repeated s->foo return logic. You can also use a single
> memset to do all the 0 resets.
Hmm. I dislike that style personally, especially for devices
like this which only have a handful of registers.
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v1 1/5] Netduino_USART: Add the Netduino Plus 2 USART Controller
2014-09-01 16:30 ` Peter Maydell
@ 2014-09-02 2:04 ` Alistair Francis
0 siblings, 0 replies; 22+ messages in thread
From: Alistair Francis @ 2014-09-02 2:04 UTC (permalink / raw)
To: Peter Maydell
Cc: Peter Crosthwaite, qemu-devel@nongnu.org Developers,
Konstanty Bialkowski
On Tue, Sep 2, 2014 at 2:30 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 24 August 2014 03:09, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> If you make your registers an array, you can index the with the offset
>> and remove the repeated s->foo return logic. You can also use a single
>> memset to do all the 0 resets.
>
> Hmm. I dislike that style personally, especially for devices
> like this which only have a handful of registers.
I think it'll end up being just as complex, as some of the offsets
cause multiple registers to change (status registers and things
like that)
I'll keep it as a switch case for now
Thanks,
Alistair
>
> -- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH v1 2/5] Netduino_GPIO: Add the Netduino Plus 2 GPIO controller
2014-08-24 0:13 [Qemu-devel] [PATCH v1 0/5] Netduino Plus 2 Machine Model Alistair Francis
2014-08-24 0:13 ` [Qemu-devel] [PATCH v1 1/5] Netduino_USART: Add the Netduino Plus 2 USART Controller Alistair Francis
@ 2014-08-24 0:13 ` Alistair Francis
2014-09-01 16:29 ` Peter Maydell
2014-08-24 0:14 ` [Qemu-devel] [PATCH v1 3/5] Netduino_SYSCFG: Add the Netduino Plus 2 SYSCFG Alistair Francis
` (3 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: Alistair Francis @ 2014-08-24 0:13 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, peter.crosthwaite, konstanty
This patch adds the Netduino Plus 2 GPIO controller to QEMU.
This allows reading and writing to the Netduino GPIO pins.
Signed-off-by: Alistair Francis <alistair23@gmail.com>
---
hw/gpio/Makefile.objs | 1 +
hw/gpio/netduino_gpio.c | 285 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 286 insertions(+)
create mode 100644 hw/gpio/netduino_gpio.c
diff --git a/hw/gpio/Makefile.objs b/hw/gpio/Makefile.objs
index 2c8b51f..e61c4d3 100644
--- a/hw/gpio/Makefile.objs
+++ b/hw/gpio/Makefile.objs
@@ -2,5 +2,6 @@ common-obj-$(CONFIG_MAX7310) += max7310.o
common-obj-$(CONFIG_PL061) += pl061.o
common-obj-$(CONFIG_PUV3) += puv3_gpio.o
common-obj-$(CONFIG_ZAURUS) += zaurus.o
+common-obj-$(CONFIG_NETDUINOP2) += netduino_gpio.o
obj-$(CONFIG_OMAP) += omap_gpio.o
diff --git a/hw/gpio/netduino_gpio.c b/hw/gpio/netduino_gpio.c
new file mode 100644
index 0000000..10d0bbd
--- /dev/null
+++ b/hw/gpio/netduino_gpio.c
@@ -0,0 +1,285 @@
+/*
+ * Netduino Plus 2 GPIO
+ *
+ * Copyright (c) 2014 Alistair Francis <alistair@alistair23.me>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "hw/sysbus.h"
+
+/* #define DEBUG_NETGPIO */
+
+#ifdef DEBUG_NETGPIO
+#define DPRINTF(fmt, ...) \
+do { printf("netduino_gpio: " fmt , ## __VA_ARGS__); } while (0)
+#else
+#define DPRINTF(fmt, ...) do {} while (0)
+#endif
+
+#define GPIO_MODER 0x00
+#define GPIO_OTYPER 0x04
+#define GPIO_OSPEEDR 0x08
+#define GPIO_PUPDR 0x0C
+#define GPIO_IDR 0x10
+#define GPIO_ODR 0x14
+#define GPIO_BSRR 0x18
+#define GPIO_BSRR_HIGH 0x1A
+#define GPIO_LCKR 0x1C
+#define GPIO_AFRL 0x20
+#define GPIO_AFRH 0x24
+
+#define GPIO_MODER_INPUT 0
+#define GPIO_MODER_GENERAL_OUT 1
+#define GPIO_MODER_ALT 2
+#define GPIO_MODER_ANALOG 3
+
+#define TYPE_NETDUINO_GPIO "netduino_gpio"
+#define NETDUINO_GPIO(obj) OBJECT_CHECK(NETDUINO_GPIOState, (obj), \
+ TYPE_NETDUINO_GPIO)
+
+typedef struct NETDUINO_GPIOState {
+ SysBusDevice parent_obj;
+
+ MemoryRegion iomem;
+
+ uint32_t gpio_moder;
+ uint32_t gpio_otyper;
+ uint32_t gpio_ospeedr;
+ uint32_t gpio_pupdr;
+ uint32_t gpio_idr;
+ uint32_t gpio_odr;
+ uint32_t gpio_bsrr;
+ uint32_t gpio_lckr;
+ uint32_t gpio_afrl;
+ uint32_t gpio_afrh;
+
+ /* This is an internal QEMU Register, used to determine the
+ * GPIO direction as set by gpio_moder
+ * 1: Input; 0: Output
+ */
+ uint16_t gpio_direction;
+ /* The GPIO letter (a - k) from the datasheet */
+ uint8_t gpio_letter;
+
+ qemu_irq irq;
+ const unsigned char *id;
+} NETDUINO_GPIOState;
+
+static const VMStateDescription vmstate_netduino_gpio = {
+ .name = "netduino_gpio",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(gpio_moder, NETDUINO_GPIOState),
+ VMSTATE_UINT32(gpio_otyper, NETDUINO_GPIOState),
+ VMSTATE_UINT32(gpio_ospeedr, NETDUINO_GPIOState),
+ VMSTATE_UINT32(gpio_pupdr, NETDUINO_GPIOState),
+ VMSTATE_UINT32(gpio_idr, NETDUINO_GPIOState),
+ VMSTATE_UINT32(gpio_odr, NETDUINO_GPIOState),
+ VMSTATE_UINT32(gpio_bsrr, NETDUINO_GPIOState),
+ VMSTATE_UINT32(gpio_lckr, NETDUINO_GPIOState),
+ VMSTATE_UINT32(gpio_afrl, NETDUINO_GPIOState),
+ VMSTATE_UINT32(gpio_afrh, NETDUINO_GPIOState),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static void gpio_reset(DeviceState *dev)
+{
+ NETDUINO_GPIOState *s = NETDUINO_GPIO(dev);
+
+ if (s->gpio_letter == 'a') {
+ s->gpio_moder = 0xA8000000;
+ s->gpio_pupdr = 0x64000000;
+ s->gpio_ospeedr = 0x00000000;
+ } else if (s->gpio_letter == 'b') {
+ s->gpio_moder = 0x00000280;
+ s->gpio_pupdr = 0x00000100;
+ s->gpio_ospeedr = 0x000000C0;
+ } else {
+ s->gpio_moder = 0x00000000;
+ s->gpio_pupdr = 0x00000000;
+ s->gpio_ospeedr = 0x00000000;
+ }
+
+ s->gpio_otyper = 0x00000000;
+ s->gpio_idr = 0x00000000;
+ s->gpio_odr = 0x00000000;
+ s->gpio_bsrr = 0x00000000;
+ s->gpio_lckr = 0x00000000;
+ s->gpio_afrl = 0x00000000;
+ s->gpio_afrh = 0x00000000;
+ s->gpio_direction = 0x0000;
+}
+
+static uint64_t netduino_gpio_read(void *opaque, hwaddr offset,
+ unsigned size)
+{
+ NETDUINO_GPIOState *s = (NETDUINO_GPIOState *)opaque;
+
+ DPRINTF("Read 0x%x\n", (uint) offset);
+
+ switch (offset) {
+ case GPIO_MODER:
+ return s->gpio_moder;
+ case GPIO_OTYPER:
+ return s->gpio_otyper;
+ case GPIO_OSPEEDR:
+ return s->gpio_ospeedr;
+ case GPIO_PUPDR:
+ return s->gpio_pupdr;
+ case GPIO_IDR:
+ /* This register changes based on the external GPIO pins */
+ return s->gpio_idr & s->gpio_direction;
+ case GPIO_ODR:
+ return s->gpio_odr;
+ case GPIO_BSRR_HIGH:
+ /* gpio_bsrr reads as zero */
+ return 0xFFFF;
+ case GPIO_BSRR:
+ /* gpio_bsrr reads as zero */
+ return 0x00000000;
+ case GPIO_LCKR:
+ return s->gpio_lckr;
+ case GPIO_AFRL:
+ return s->gpio_afrl;
+ case GPIO_AFRH:
+ return s->gpio_afrh;
+ }
+ return 0;
+}
+
+static void netduino_gpio_write(void *opaque, hwaddr offset,
+ uint64_t value, unsigned size)
+{
+ NETDUINO_GPIOState *s = (NETDUINO_GPIOState *)opaque;
+ int i, mask;
+
+ DPRINTF("Write 0x%x, 0x%x\n", (uint) value, (uint) offset);
+
+ switch (offset) {
+ case GPIO_MODER:
+ s->gpio_moder = (uint32_t) value;
+ for (i = 0; i < 32; i = i + 2) {
+ /* Two bits determine the I/O direction/mode */
+ mask = (1 << i) + (1 << (i + 1));
+
+ if ((s->gpio_moder & mask) == GPIO_MODER_INPUT) {
+ s->gpio_direction |= (1 << (i/2));
+ } else if ((s->gpio_moder & mask) == GPIO_MODER_GENERAL_OUT) {
+ s->gpio_direction &= (0xFFFF ^ (1 << (i/2)));
+ } else {
+ /* Not supported at the moment */
+ }
+ }
+ return;
+ case GPIO_OTYPER:
+ s->gpio_otyper = (uint32_t) value;
+ return;
+ case GPIO_OSPEEDR:
+ s->gpio_ospeedr = (uint32_t) value;
+ return;
+ case GPIO_PUPDR:
+ s->gpio_pupdr = (uint32_t) value;
+ return;
+ case GPIO_IDR:
+ /* Read Only Register */
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "net_gpio%c_write: Read Only Register 0x%x\n",
+ s->gpio_letter, (int)offset);
+ return;
+ case GPIO_ODR:
+ s->gpio_odr = ((uint32_t) value & (s->gpio_direction ^ 0xFFFF));
+ return;
+ case GPIO_BSRR_HIGH:
+ /* Reset the output value */
+ s->gpio_odr &= (uint32_t) (value ^ 0xFFFF);
+ s->gpio_bsrr = (uint32_t) (value << 16);
+ DPRINTF("Output: 0x%x\n", s->gpio_odr);
+ return;
+ case GPIO_BSRR:
+ /* Reset the output value */
+ s->gpio_odr &= (uint32_t) ((value >> 16) ^ 0xFFFF);
+ /* Sets the output value */
+ s->gpio_odr |= (uint32_t) (value & 0xFFFF);
+ s->gpio_bsrr = (uint32_t) value;
+ DPRINTF("Output: 0x%x\n", s->gpio_odr);
+ return;
+ case GPIO_LCKR:
+ s->gpio_lckr = (uint32_t) value;
+ return;
+ case GPIO_AFRL:
+ s->gpio_afrl = (uint32_t) value;
+ return;
+ case GPIO_AFRH:
+ s->gpio_afrh = (uint32_t) value;
+ return;
+ }
+}
+
+static const MemoryRegionOps netduino_gpio_ops = {
+ .read = netduino_gpio_read,
+ .write = netduino_gpio_write,
+ .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static Property net_gpio_properties[] = {
+ DEFINE_PROP_UINT8("gpio-letter", NETDUINO_GPIOState, gpio_letter,
+ (uint) 'a'),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+
+static int netduino_gpio_initfn(SysBusDevice *sbd)
+{
+ DeviceState *dev = DEVICE(sbd);
+ NETDUINO_GPIOState *s = NETDUINO_GPIO(dev);
+
+ memory_region_init_io(&s->iomem, OBJECT(s), &netduino_gpio_ops, s,
+ "netduino_gpio", 0x2000);
+ sysbus_init_mmio(sbd, &s->iomem);
+ sysbus_init_irq(sbd, &s->irq);
+ return 0;
+}
+
+static void netduino_gpio_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+
+ k->init = netduino_gpio_initfn;
+ dc->vmsd = &vmstate_netduino_gpio;
+ dc->props = net_gpio_properties;
+ dc->reset = gpio_reset;
+}
+
+static const TypeInfo netduino_gpio_info = {
+ .name = TYPE_NETDUINO_GPIO,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(NETDUINO_GPIOState),
+ .class_init = netduino_gpio_class_init,
+};
+
+static void netduino_gpio_register_types(void)
+{
+ type_register_static(&netduino_gpio_info);
+}
+
+type_init(netduino_gpio_register_types)
--
1.9.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v1 2/5] Netduino_GPIO: Add the Netduino Plus 2 GPIO controller
2014-08-24 0:13 ` [Qemu-devel] [PATCH v1 2/5] Netduino_GPIO: Add the Netduino Plus 2 GPIO controller Alistair Francis
@ 2014-09-01 16:29 ` Peter Maydell
2014-09-02 2:37 ` Alistair Francis
0 siblings, 1 reply; 22+ messages in thread
From: Peter Maydell @ 2014-09-01 16:29 UTC (permalink / raw)
To: Alistair Francis; +Cc: Peter Crosthwaite, QEMU Developers, Konstanty Bialkowski
On 24 August 2014 01:13, Alistair Francis <alistair23@gmail.com> wrote:
> This patch adds the Netduino Plus 2 GPIO controller to QEMU.
> This allows reading and writing to the Netduino GPIO pins.
Are you sure this isn't actually the GPIO module in the STM32F4xx
SoC ? The datasheets and schematic suggest this isn't a
board level feature, which would imply it shouldn't have a
board level name like "netduino_gpio". Instead you should
have an SoC QOM object/device which instantiates all
the SoC level components like the GPIO, UART, etc,
and the board model should just create the SoC and any
devices which are genuinely separate components from the
SoC.
> Signed-off-by: Alistair Francis <alistair23@gmail.com>
> ---
> hw/gpio/Makefile.objs | 1 +
> hw/gpio/netduino_gpio.c | 285 ++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 286 insertions(+)
> create mode 100644 hw/gpio/netduino_gpio.c
>
> diff --git a/hw/gpio/Makefile.objs b/hw/gpio/Makefile.objs
> index 2c8b51f..e61c4d3 100644
> --- a/hw/gpio/Makefile.objs
> +++ b/hw/gpio/Makefile.objs
> @@ -2,5 +2,6 @@ common-obj-$(CONFIG_MAX7310) += max7310.o
> common-obj-$(CONFIG_PL061) += pl061.o
> common-obj-$(CONFIG_PUV3) += puv3_gpio.o
> common-obj-$(CONFIG_ZAURUS) += zaurus.o
> +common-obj-$(CONFIG_NETDUINOP2) += netduino_gpio.o
>
> obj-$(CONFIG_OMAP) += omap_gpio.o
> diff --git a/hw/gpio/netduino_gpio.c b/hw/gpio/netduino_gpio.c
> new file mode 100644
> index 0000000..10d0bbd
> --- /dev/null
> +++ b/hw/gpio/netduino_gpio.c
> @@ -0,0 +1,285 @@
> +/*
> + * Netduino Plus 2 GPIO
> + *
> + * Copyright (c) 2014 Alistair Francis <alistair@alistair23.me>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "hw/sysbus.h"
> +
> +/* #define DEBUG_NETGPIO */
> +
> +#ifdef DEBUG_NETGPIO
> +#define DPRINTF(fmt, ...) \
> +do { printf("netduino_gpio: " fmt , ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...) do {} while (0)
> +#endif
> +
> +#define GPIO_MODER 0x00
> +#define GPIO_OTYPER 0x04
> +#define GPIO_OSPEEDR 0x08
> +#define GPIO_PUPDR 0x0C
> +#define GPIO_IDR 0x10
> +#define GPIO_ODR 0x14
> +#define GPIO_BSRR 0x18
> +#define GPIO_BSRR_HIGH 0x1A
> +#define GPIO_LCKR 0x1C
> +#define GPIO_AFRL 0x20
> +#define GPIO_AFRH 0x24
> +
> +#define GPIO_MODER_INPUT 0
> +#define GPIO_MODER_GENERAL_OUT 1
> +#define GPIO_MODER_ALT 2
> +#define GPIO_MODER_ANALOG 3
> +
> +#define TYPE_NETDUINO_GPIO "netduino_gpio"
> +#define NETDUINO_GPIO(obj) OBJECT_CHECK(NETDUINO_GPIOState, (obj), \
> + TYPE_NETDUINO_GPIO)
> +
> +typedef struct NETDUINO_GPIOState {
This should be "Netduino_GPIOState", since the official
boardname isn't an all-caps word.
> + SysBusDevice parent_obj;
> +
> + MemoryRegion iomem;
> +
> + uint32_t gpio_moder;
> + uint32_t gpio_otyper;
> + uint32_t gpio_ospeedr;
> + uint32_t gpio_pupdr;
> + uint32_t gpio_idr;
> + uint32_t gpio_odr;
> + uint32_t gpio_bsrr;
> + uint32_t gpio_lckr;
> + uint32_t gpio_afrl;
> + uint32_t gpio_afrh;
Do these really all need a 'gpio_' prefix in their names?
> +
> + /* This is an internal QEMU Register, used to determine the
> + * GPIO direction as set by gpio_moder
> + * 1: Input; 0: Output
> + */
> + uint16_t gpio_direction;
I can't really figure out what you mean by this comment.
> + /* The GPIO letter (a - k) from the datasheet */
> + uint8_t gpio_letter;
> +
> + qemu_irq irq;
> + const unsigned char *id;
> +} NETDUINO_GPIOState;
> +
> +static const VMStateDescription vmstate_netduino_gpio = {
> + .name = "netduino_gpio",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT32(gpio_moder, NETDUINO_GPIOState),
> + VMSTATE_UINT32(gpio_otyper, NETDUINO_GPIOState),
> + VMSTATE_UINT32(gpio_ospeedr, NETDUINO_GPIOState),
> + VMSTATE_UINT32(gpio_pupdr, NETDUINO_GPIOState),
> + VMSTATE_UINT32(gpio_idr, NETDUINO_GPIOState),
> + VMSTATE_UINT32(gpio_odr, NETDUINO_GPIOState),
> + VMSTATE_UINT32(gpio_bsrr, NETDUINO_GPIOState),
> + VMSTATE_UINT32(gpio_lckr, NETDUINO_GPIOState),
> + VMSTATE_UINT32(gpio_afrl, NETDUINO_GPIOState),
> + VMSTATE_UINT32(gpio_afrh, NETDUINO_GPIOState),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static void gpio_reset(DeviceState *dev)
> +{
> + NETDUINO_GPIOState *s = NETDUINO_GPIO(dev);
> +
> + if (s->gpio_letter == 'a') {
> + s->gpio_moder = 0xA8000000;
> + s->gpio_pupdr = 0x64000000;
> + s->gpio_ospeedr = 0x00000000;
> + } else if (s->gpio_letter == 'b') {
> + s->gpio_moder = 0x00000280;
> + s->gpio_pupdr = 0x00000100;
> + s->gpio_ospeedr = 0x000000C0;
> + } else {
> + s->gpio_moder = 0x00000000;
> + s->gpio_pupdr = 0x00000000;
> + s->gpio_ospeedr = 0x00000000;
> + }
I don't think this is the right way to implement this.
You should give the GPIO device some QOM properties
which allow it to be configured (eg reset values for
various registers) and let the SoC QOM device
instantiate and set QOM properties appropriately
for each of the GPIO modules in the SoC.
> +
> + s->gpio_otyper = 0x00000000;
> + s->gpio_idr = 0x00000000;
> + s->gpio_odr = 0x00000000;
> + s->gpio_bsrr = 0x00000000;
> + s->gpio_lckr = 0x00000000;
> + s->gpio_afrl = 0x00000000;
> + s->gpio_afrh = 0x00000000;
> + s->gpio_direction = 0x0000;
> +}
> +
> +static uint64_t netduino_gpio_read(void *opaque, hwaddr offset,
> + unsigned size)
> +{
> + NETDUINO_GPIOState *s = (NETDUINO_GPIOState *)opaque;
> +
> + DPRINTF("Read 0x%x\n", (uint) offset);
> +
> + switch (offset) {
> + case GPIO_MODER:
> + return s->gpio_moder;
> + case GPIO_OTYPER:
> + return s->gpio_otyper;
> + case GPIO_OSPEEDR:
> + return s->gpio_ospeedr;
> + case GPIO_PUPDR:
> + return s->gpio_pupdr;
> + case GPIO_IDR:
> + /* This register changes based on the external GPIO pins */
> + return s->gpio_idr & s->gpio_direction;
> + case GPIO_ODR:
> + return s->gpio_odr;
> + case GPIO_BSRR_HIGH:
> + /* gpio_bsrr reads as zero */
Dup of comment from a couple of lines down?
> + return 0xFFFF;
Why does reading the top half of this as a 16 bit read
return all-ones when reading the full 32 bits returns
all-zeroes?
> + case GPIO_BSRR:
> + /* gpio_bsrr reads as zero */
> + return 0x00000000;
Comment doesn't tell us anything the code doesn't...
> + case GPIO_LCKR:
> + return s->gpio_lckr;
> + case GPIO_AFRL:
> + return s->gpio_afrl;
> + case GPIO_AFRH:
> + return s->gpio_afrh;
> + }
> + return 0;
> +}
> +
> +static void netduino_gpio_write(void *opaque, hwaddr offset,
> + uint64_t value, unsigned size)
> +{
> + NETDUINO_GPIOState *s = (NETDUINO_GPIOState *)opaque;
> + int i, mask;
> +
> + DPRINTF("Write 0x%x, 0x%x\n", (uint) value, (uint) offset);
> +
> + switch (offset) {
> + case GPIO_MODER:
> + s->gpio_moder = (uint32_t) value;
> + for (i = 0; i < 32; i = i + 2) {
"i += 2" is more idiomatic. In any case I think this
loop would be more cleanly phrased as a simple loop
from 0 to 15 (giving you one multiply-by-two rather
than two divide-by-twos, and probably making the
loop termination easier for static analysis tools to
understand.)
> + /* Two bits determine the I/O direction/mode */
> + mask = (1 << i) + (1 << (i + 1));
You need to say "1U" if you want to shift it by 31,
since shifting into the sign bit of a signed value is
undefined behaviour in C. Also this is a funny way
of writing "3U << i".
> +
> + if ((s->gpio_moder & mask) == GPIO_MODER_INPUT) {
> + s->gpio_direction |= (1 << (i/2));
> + } else if ((s->gpio_moder & mask) == GPIO_MODER_GENERAL_OUT) {
> + s->gpio_direction &= (0xFFFF ^ (1 << (i/2)));
> + } else {
> + /* Not supported at the moment */
> + }
> + }
> + return;
> + case GPIO_OTYPER:
> + s->gpio_otyper = (uint32_t) value;
> + return;
> + case GPIO_OSPEEDR:
> + s->gpio_ospeedr = (uint32_t) value;
> + return;
> + case GPIO_PUPDR:
> + s->gpio_pupdr = (uint32_t) value;
> + return;
> + case GPIO_IDR:
> + /* Read Only Register */
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "net_gpio%c_write: Read Only Register 0x%x\n",
> + s->gpio_letter, (int)offset);
> + return;
> + case GPIO_ODR:
> + s->gpio_odr = ((uint32_t) value & (s->gpio_direction ^ 0xFFFF));
> + return;
> + case GPIO_BSRR_HIGH:
> + /* Reset the output value */
> + s->gpio_odr &= (uint32_t) (value ^ 0xFFFF);
Any particular reason for using the XOR with 0xffff
rather than just inverting?
> + s->gpio_bsrr = (uint32_t) (value << 16);
BSRR is write-only and doesn't have any underlying state
as far as I can tell from the manual. Indeed this file never
reads the gpio_bsrr field in the struct. That suggests you
should remove it entirely.
> + DPRINTF("Output: 0x%x\n", s->gpio_odr);
> + return;
> + case GPIO_BSRR:
> + /* Reset the output value */
> + s->gpio_odr &= (uint32_t) ((value >> 16) ^ 0xFFFF);
> + /* Sets the output value */
These comments would be more helpful if they read
/* Top 16 bits are "write one to clear output" */
/* Bottom 16 bits are "write one to set output" */
> + s->gpio_odr |= (uint32_t) (value & 0xFFFF);
> + s->gpio_bsrr = (uint32_t) value;
> + DPRINTF("Output: 0x%x\n", s->gpio_odr);
> + return;
Surely we should actually be doing something with our
outputs (ie setting qdev GPIO output lines) here?
> + case GPIO_LCKR:
> + s->gpio_lckr = (uint32_t) value;
This doesn't seem to be implementing the lock functionality
the datasheet describes.
> + return;
> + case GPIO_AFRL:
> + s->gpio_afrl = (uint32_t) value;
> + return;
> + case GPIO_AFRH:
> + s->gpio_afrh = (uint32_t) value;
> + return;
> + }
> +}
> +
> +static const MemoryRegionOps netduino_gpio_ops = {
> + .read = netduino_gpio_read,
> + .write = netduino_gpio_write,
> + .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static Property net_gpio_properties[] = {
> + DEFINE_PROP_UINT8("gpio-letter", NETDUINO_GPIOState, gpio_letter,
> + (uint) 'a'),
> + DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +
> +static int netduino_gpio_initfn(SysBusDevice *sbd)
> +{
> + DeviceState *dev = DEVICE(sbd);
> + NETDUINO_GPIOState *s = NETDUINO_GPIO(dev);
> +
> + memory_region_init_io(&s->iomem, OBJECT(s), &netduino_gpio_ops, s,
> + "netduino_gpio", 0x2000);
> + sysbus_init_mmio(sbd, &s->iomem);
> + sysbus_init_irq(sbd, &s->irq);
Where are the GPIO output lines?
> + return 0;
> +}
> +
> +static void netduino_gpio_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +
> + k->init = netduino_gpio_initfn;
> + dc->vmsd = &vmstate_netduino_gpio;
> + dc->props = net_gpio_properties;
> + dc->reset = gpio_reset;
> +}
> +
> +static const TypeInfo netduino_gpio_info = {
> + .name = TYPE_NETDUINO_GPIO,
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(NETDUINO_GPIOState),
> + .class_init = netduino_gpio_class_init,
> +};
> +
> +static void netduino_gpio_register_types(void)
> +{
> + type_register_static(&netduino_gpio_info);
> +}
> +
> +type_init(netduino_gpio_register_types)
> --
> 1.9.1
thanks
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v1 2/5] Netduino_GPIO: Add the Netduino Plus 2 GPIO controller
2014-09-01 16:29 ` Peter Maydell
@ 2014-09-02 2:37 ` Alistair Francis
0 siblings, 0 replies; 22+ messages in thread
From: Alistair Francis @ 2014-09-02 2:37 UTC (permalink / raw)
To: Peter Maydell; +Cc: Peter Crosthwaite, QEMU Developers, Konstanty Bialkowski
On Tue, Sep 2, 2014 at 2:29 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 24 August 2014 01:13, Alistair Francis <alistair23@gmail.com> wrote:
>> This patch adds the Netduino Plus 2 GPIO controller to QEMU.
>> This allows reading and writing to the Netduino GPIO pins.
>
> Are you sure this isn't actually the GPIO module in the STM32F4xx
> SoC ? The datasheets and schematic suggest this isn't a
> board level feature, which would imply it shouldn't have a
> board level name like "netduino_gpio". Instead you should
> have an SoC QOM object/device which instantiates all
> the SoC level components like the GPIO, UART, etc,
> and the board model should just create the SoC and any
> devices which are genuinely separate components from the
> SoC.
>
It is part of the SoC. All of the devices I added are, I will rename
as appropriate.
>> Signed-off-by: Alistair Francis <alistair23@gmail.com>
>> ---
>> hw/gpio/Makefile.objs | 1 +
>> hw/gpio/netduino_gpio.c | 285 ++++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 286 insertions(+)
>> create mode 100644 hw/gpio/netduino_gpio.c
>>
>> diff --git a/hw/gpio/Makefile.objs b/hw/gpio/Makefile.objs
>> index 2c8b51f..e61c4d3 100644
>> --- a/hw/gpio/Makefile.objs
>> +++ b/hw/gpio/Makefile.objs
>> @@ -2,5 +2,6 @@ common-obj-$(CONFIG_MAX7310) += max7310.o
>> common-obj-$(CONFIG_PL061) += pl061.o
>> common-obj-$(CONFIG_PUV3) += puv3_gpio.o
>> common-obj-$(CONFIG_ZAURUS) += zaurus.o
>> +common-obj-$(CONFIG_NETDUINOP2) += netduino_gpio.o
>>
>> obj-$(CONFIG_OMAP) += omap_gpio.o
>> diff --git a/hw/gpio/netduino_gpio.c b/hw/gpio/netduino_gpio.c
>> new file mode 100644
>> index 0000000..10d0bbd
>> --- /dev/null
>> +++ b/hw/gpio/netduino_gpio.c
>> @@ -0,0 +1,285 @@
>> +/*
>> + * Netduino Plus 2 GPIO
>> + *
>> + * Copyright (c) 2014 Alistair Francis <alistair@alistair23.me>
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#include "hw/sysbus.h"
>> +
>> +/* #define DEBUG_NETGPIO */
>> +
>> +#ifdef DEBUG_NETGPIO
>> +#define DPRINTF(fmt, ...) \
>> +do { printf("netduino_gpio: " fmt , ## __VA_ARGS__); } while (0)
>> +#else
>> +#define DPRINTF(fmt, ...) do {} while (0)
>> +#endif
>> +
>> +#define GPIO_MODER 0x00
>> +#define GPIO_OTYPER 0x04
>> +#define GPIO_OSPEEDR 0x08
>> +#define GPIO_PUPDR 0x0C
>> +#define GPIO_IDR 0x10
>> +#define GPIO_ODR 0x14
>> +#define GPIO_BSRR 0x18
>> +#define GPIO_BSRR_HIGH 0x1A
>> +#define GPIO_LCKR 0x1C
>> +#define GPIO_AFRL 0x20
>> +#define GPIO_AFRH 0x24
>> +
>> +#define GPIO_MODER_INPUT 0
>> +#define GPIO_MODER_GENERAL_OUT 1
>> +#define GPIO_MODER_ALT 2
>> +#define GPIO_MODER_ANALOG 3
>> +
>> +#define TYPE_NETDUINO_GPIO "netduino_gpio"
>> +#define NETDUINO_GPIO(obj) OBJECT_CHECK(NETDUINO_GPIOState, (obj), \
>> + TYPE_NETDUINO_GPIO)
>> +
>> +typedef struct NETDUINO_GPIOState {
>
> This should be "Netduino_GPIOState", since the official
> boardname isn't an all-caps word.
>
Will fix
>> + SysBusDevice parent_obj;
>> +
>> + MemoryRegion iomem;
>> +
>> + uint32_t gpio_moder;
>> + uint32_t gpio_otyper;
>> + uint32_t gpio_ospeedr;
>> + uint32_t gpio_pupdr;
>> + uint32_t gpio_idr;
>> + uint32_t gpio_odr;
>> + uint32_t gpio_bsrr;
>> + uint32_t gpio_lckr;
>> + uint32_t gpio_afrl;
>> + uint32_t gpio_afrh;
>
> Do these really all need a 'gpio_' prefix in their names?
>
That's what they are called in the datasheet, I am happy to remove
them though (except they are called GPIOx_*)
>> +
>> + /* This is an internal QEMU Register, used to determine the
>> + * GPIO direction as set by gpio_moder
>> + * 1: Input; 0: Output
>> + */
>> + uint16_t gpio_direction;
>
> I can't really figure out what you mean by this comment.
The idea was to use an internal register to keep track of the data direction.
This way it would be easier to see if I/O is in the correct direction, by just
ANDing/NANDing with the direction register
The problem comes from the GPIO_MODER register (the direction register)
as it uses two bits to set the I/O direction/mode
I will rephrase the comment
>
>> + /* The GPIO letter (a - k) from the datasheet */
>> + uint8_t gpio_letter;
>> +
>> + qemu_irq irq;
>> + const unsigned char *id;
>> +} NETDUINO_GPIOState;
>> +
>> +static const VMStateDescription vmstate_netduino_gpio = {
>> + .name = "netduino_gpio",
>> + .version_id = 1,
>> + .minimum_version_id = 1,
>> + .fields = (VMStateField[]) {
>> + VMSTATE_UINT32(gpio_moder, NETDUINO_GPIOState),
>> + VMSTATE_UINT32(gpio_otyper, NETDUINO_GPIOState),
>> + VMSTATE_UINT32(gpio_ospeedr, NETDUINO_GPIOState),
>> + VMSTATE_UINT32(gpio_pupdr, NETDUINO_GPIOState),
>> + VMSTATE_UINT32(gpio_idr, NETDUINO_GPIOState),
>> + VMSTATE_UINT32(gpio_odr, NETDUINO_GPIOState),
>> + VMSTATE_UINT32(gpio_bsrr, NETDUINO_GPIOState),
>> + VMSTATE_UINT32(gpio_lckr, NETDUINO_GPIOState),
>> + VMSTATE_UINT32(gpio_afrl, NETDUINO_GPIOState),
>> + VMSTATE_UINT32(gpio_afrh, NETDUINO_GPIOState),
>> + VMSTATE_END_OF_LIST()
>> + }
>> +};
>> +
>> +static void gpio_reset(DeviceState *dev)
>> +{
>> + NETDUINO_GPIOState *s = NETDUINO_GPIO(dev);
>> +
>> + if (s->gpio_letter == 'a') {
>> + s->gpio_moder = 0xA8000000;
>> + s->gpio_pupdr = 0x64000000;
>> + s->gpio_ospeedr = 0x00000000;
>> + } else if (s->gpio_letter == 'b') {
>> + s->gpio_moder = 0x00000280;
>> + s->gpio_pupdr = 0x00000100;
>> + s->gpio_ospeedr = 0x000000C0;
>> + } else {
>> + s->gpio_moder = 0x00000000;
>> + s->gpio_pupdr = 0x00000000;
>> + s->gpio_ospeedr = 0x00000000;
>> + }
>
> I don't think this is the right way to implement this.
> You should give the GPIO device some QOM properties
> which allow it to be configured (eg reset values for
> various registers) and let the SoC QOM device
> instantiate and set QOM properties appropriately
> for each of the GPIO modules in the SoC.
>
That seems like overkill to just set three registers. I also think
it is worth knowing which GPIOx the device is. Especially when
the EXTI device (External Interrupts) is added
>> +
>> + s->gpio_otyper = 0x00000000;
>> + s->gpio_idr = 0x00000000;
>> + s->gpio_odr = 0x00000000;
>> + s->gpio_bsrr = 0x00000000;
>> + s->gpio_lckr = 0x00000000;
>> + s->gpio_afrl = 0x00000000;
>> + s->gpio_afrh = 0x00000000;
>> + s->gpio_direction = 0x0000;
>> +}
>> +
>> +static uint64_t netduino_gpio_read(void *opaque, hwaddr offset,
>> + unsigned size)
>> +{
>> + NETDUINO_GPIOState *s = (NETDUINO_GPIOState *)opaque;
>> +
>> + DPRINTF("Read 0x%x\n", (uint) offset);
>> +
>> + switch (offset) {
>> + case GPIO_MODER:
>> + return s->gpio_moder;
>> + case GPIO_OTYPER:
>> + return s->gpio_otyper;
>> + case GPIO_OSPEEDR:
>> + return s->gpio_ospeedr;
>> + case GPIO_PUPDR:
>> + return s->gpio_pupdr;
>> + case GPIO_IDR:
>> + /* This register changes based on the external GPIO pins */
>> + return s->gpio_idr & s->gpio_direction;
>> + case GPIO_ODR:
>> + return s->gpio_odr;
>> + case GPIO_BSRR_HIGH:
>> + /* gpio_bsrr reads as zero */
>
> Dup of comment from a couple of lines down?
>
>> + return 0xFFFF;
>
> Why does reading the top half of this as a 16 bit read
> return all-ones when reading the full 32 bits returns
> all-zeroes?
>
That must be a Typo, it should be all-zeroes. Will fix
>> + case GPIO_BSRR:
>> + /* gpio_bsrr reads as zero */
>> + return 0x00000000;
>
> Comment doesn't tell us anything the code doesn't...
>
I was just trying to say that the register is a write only register.
I will fix/remove the comment
>> + case GPIO_LCKR:
>> + return s->gpio_lckr;
>> + case GPIO_AFRL:
>> + return s->gpio_afrl;
>> + case GPIO_AFRH:
>> + return s->gpio_afrh;
>> + }
>> + return 0;
>> +}
>> +
>> +static void netduino_gpio_write(void *opaque, hwaddr offset,
>> + uint64_t value, unsigned size)
>> +{
>> + NETDUINO_GPIOState *s = (NETDUINO_GPIOState *)opaque;
>> + int i, mask;
>> +
>> + DPRINTF("Write 0x%x, 0x%x\n", (uint) value, (uint) offset);
>> +
>> + switch (offset) {
>> + case GPIO_MODER:
>> + s->gpio_moder = (uint32_t) value;
>> + for (i = 0; i < 32; i = i + 2) {
>
> "i += 2" is more idiomatic. In any case I think this
> loop would be more cleanly phrased as a simple loop
> from 0 to 15 (giving you one multiply-by-two rather
> than two divide-by-twos, and probably making the
> loop termination easier for static analysis tools to
> understand.)
>
Ok, I will make it a 0 to 15 loop
>> + /* Two bits determine the I/O direction/mode */
>> + mask = (1 << i) + (1 << (i + 1));
>
> You need to say "1U" if you want to shift it by 31,
> since shifting into the sign bit of a signed value is
> undefined behaviour in C. Also this is a funny way
> of writing "3U << i".
>
I will replace with 3U << i. I didn't think of doing it that way
>> +
>> + if ((s->gpio_moder & mask) == GPIO_MODER_INPUT) {
>> + s->gpio_direction |= (1 << (i/2));
>> + } else if ((s->gpio_moder & mask) == GPIO_MODER_GENERAL_OUT) {
>> + s->gpio_direction &= (0xFFFF ^ (1 << (i/2)));
>> + } else {
>> + /* Not supported at the moment */
>> + }
>> + }
>> + return;
>> + case GPIO_OTYPER:
>> + s->gpio_otyper = (uint32_t) value;
>> + return;
>> + case GPIO_OSPEEDR:
>> + s->gpio_ospeedr = (uint32_t) value;
>> + return;
>> + case GPIO_PUPDR:
>> + s->gpio_pupdr = (uint32_t) value;
>> + return;
>> + case GPIO_IDR:
>> + /* Read Only Register */
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "net_gpio%c_write: Read Only Register 0x%x\n",
>> + s->gpio_letter, (int)offset);
>> + return;
>> + case GPIO_ODR:
>> + s->gpio_odr = ((uint32_t) value & (s->gpio_direction ^ 0xFFFF));
>> + return;
>> + case GPIO_BSRR_HIGH:
>> + /* Reset the output value */
>> + s->gpio_odr &= (uint32_t) (value ^ 0xFFFF);
>
> Any particular reason for using the XOR with 0xffff
> rather than just inverting?
>
No, this is just the first way I thought of
>> + s->gpio_bsrr = (uint32_t) (value << 16);
>
> BSRR is write-only and doesn't have any underlying state
> as far as I can tell from the manual. Indeed this file never
> reads the gpio_bsrr field in the struct. That suggests you
> should remove it entirely.
>
That is true, it doesn't have a state. I was thinking of removing it,
but I left it there in case somebody wanted it for debugging.
I can remove it though
>> + DPRINTF("Output: 0x%x\n", s->gpio_odr);
>> + return;
>> + case GPIO_BSRR:
>> + /* Reset the output value */
>> + s->gpio_odr &= (uint32_t) ((value >> 16) ^ 0xFFFF);
>> + /* Sets the output value */
>
> These comments would be more helpful if they read
> /* Top 16 bits are "write one to clear output" */
> /* Bottom 16 bits are "write one to set output" */
>
Will fix
>> + s->gpio_odr |= (uint32_t) (value & 0xFFFF);
>> + s->gpio_bsrr = (uint32_t) value;
>> + DPRINTF("Output: 0x%x\n", s->gpio_odr);
>> + return;
>
> Surely we should actually be doing something with our
> outputs (ie setting qdev GPIO output lines) here?
>
Yes, the outputs should be doing something. I just hadn't worked out
how to get them to do anything. I will look into that and implement
>> + case GPIO_LCKR:
>> + s->gpio_lckr = (uint32_t) value;
>
> This doesn't seem to be implementing the lock functionality
> the datasheet describes.
>
That's not implemented at the moment, I will either implement or
add an unimplemented message
>> + return;
>> + case GPIO_AFRL:
>> + s->gpio_afrl = (uint32_t) value;
>> + return;
>> + case GPIO_AFRH:
>> + s->gpio_afrh = (uint32_t) value;
>> + return;
>> + }
>> +}
>> +
>> +static const MemoryRegionOps netduino_gpio_ops = {
>> + .read = netduino_gpio_read,
>> + .write = netduino_gpio_write,
>> + .endianness = DEVICE_NATIVE_ENDIAN,
>> +};
>> +
>> +static Property net_gpio_properties[] = {
>> + DEFINE_PROP_UINT8("gpio-letter", NETDUINO_GPIOState, gpio_letter,
>> + (uint) 'a'),
>> + DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +
>> +static int netduino_gpio_initfn(SysBusDevice *sbd)
>> +{
>> + DeviceState *dev = DEVICE(sbd);
>> + NETDUINO_GPIOState *s = NETDUINO_GPIO(dev);
>> +
>> + memory_region_init_io(&s->iomem, OBJECT(s), &netduino_gpio_ops, s,
>> + "netduino_gpio", 0x2000);
>> + sysbus_init_mmio(sbd, &s->iomem);
>> + sysbus_init_irq(sbd, &s->irq);
>
> Where are the GPIO output lines?
>
I will add them
>> + return 0;
>> +}
>> +
>> +static void netduino_gpio_class_init(ObjectClass *klass, void *data)
>> +{
>> + DeviceClass *dc = DEVICE_CLASS(klass);
>> + SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>> +
>> + k->init = netduino_gpio_initfn;
>> + dc->vmsd = &vmstate_netduino_gpio;
>> + dc->props = net_gpio_properties;
>> + dc->reset = gpio_reset;
>> +}
>> +
>> +static const TypeInfo netduino_gpio_info = {
>> + .name = TYPE_NETDUINO_GPIO,
>> + .parent = TYPE_SYS_BUS_DEVICE,
>> + .instance_size = sizeof(NETDUINO_GPIOState),
>> + .class_init = netduino_gpio_class_init,
>> +};
>> +
>> +static void netduino_gpio_register_types(void)
>> +{
>> + type_register_static(&netduino_gpio_info);
>> +}
>> +
>> +type_init(netduino_gpio_register_types)
>> --
>> 1.9.1
>
> thanks
> -- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH v1 3/5] Netduino_SYSCFG: Add the Netduino Plus 2 SYSCFG
2014-08-24 0:13 [Qemu-devel] [PATCH v1 0/5] Netduino Plus 2 Machine Model Alistair Francis
2014-08-24 0:13 ` [Qemu-devel] [PATCH v1 1/5] Netduino_USART: Add the Netduino Plus 2 USART Controller Alistair Francis
2014-08-24 0:13 ` [Qemu-devel] [PATCH v1 2/5] Netduino_GPIO: Add the Netduino Plus 2 GPIO controller Alistair Francis
@ 2014-08-24 0:14 ` Alistair Francis
2014-08-24 3:09 ` Peter Crosthwaite
2014-08-24 0:14 ` [Qemu-devel] [PATCH v1 4/5] Netduino_Timer: Add the Netduino Plus 2 Timer2 to 5 Alistair Francis
` (2 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: Alistair Francis @ 2014-08-24 0:14 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, peter.crosthwaite, konstanty
This patch adds the Netduino Plus 2 System Configuration
Controller. This is used to configure what memory is mapped
at address 0 (although that is not supported) as well
as configure how the EXTI interrupts work (also not
supported at the moment).
Signed-off-by: Alistair Francis <alistair23@gmail.com>
---
hw/misc/Makefile.objs | 1 +
hw/misc/netduino_syscfg.c | 201 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 202 insertions(+)
create mode 100644 hw/misc/netduino_syscfg.c
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 979e532..794f510 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -39,5 +39,6 @@ obj-$(CONFIG_OMAP) += omap_sdrc.o
obj-$(CONFIG_OMAP) += omap_tap.o
obj-$(CONFIG_SLAVIO) += slavio_misc.o
obj-$(CONFIG_ZYNQ) += zynq_slcr.o
+obj-$(CONFIG_NETDUINOP2) += netduino_syscfg.o
obj-$(CONFIG_PVPANIC) += pvpanic.o
diff --git a/hw/misc/netduino_syscfg.c b/hw/misc/netduino_syscfg.c
new file mode 100644
index 0000000..3db5295
--- /dev/null
+++ b/hw/misc/netduino_syscfg.c
@@ -0,0 +1,201 @@
+/*
+ * Netduino Plus 2 SYSCFG
+ *
+ * Copyright (c) 2014 Alistair Francis <alistair@alistair23.me>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "hw/sysbus.h"
+#include "sysemu/char.h"
+#include "hw/hw.h"
+#include "net/net.h"
+
+/* #define DEBUG_NETSYSCFG */
+
+#ifdef DEBUG_NETSYSCFG
+#define DPRINTF(fmt, ...) \
+do { printf("netduino_syscfg: " fmt , ## __VA_ARGS__); } while (0)
+#else
+#define DPRINTF(fmt, ...) do {} while (0)
+#endif
+
+#define SYSCFG_MEMRMP 0x00
+#define SYSCFG_PMC 0x04
+#define SYSCFG_EXTICR1 0x08
+#define SYSCFG_EXTICR2 0x0C
+#define SYSCFG_EXTICR3 0x10
+#define SYSCFG_EXTICR4 0x14
+#define SYSCFG_CMPCR 0x20
+
+#define TYPE_NETDUINO_SYSCFG "netduino_syscfg"
+#define NETDUINO_SYSCFG(obj) \
+ OBJECT_CHECK(struct net_syscfg, (obj), TYPE_NETDUINO_SYSCFG)
+
+struct net_syscfg {
+ SysBusDevice parent_obj;
+
+ MemoryRegion mmio;
+
+ uint32_t syscfg_memrmp;
+ uint32_t syscfg_pmc;
+ uint32_t syscfg_exticr1;
+ uint32_t syscfg_exticr2;
+ uint32_t syscfg_exticr3;
+ uint32_t syscfg_exticr4;
+ uint32_t syscfg_cmpcr;
+
+ qemu_irq irq;
+};
+
+static void syscfg_reset(DeviceState *dev)
+{
+ struct net_syscfg *s = NETDUINO_SYSCFG(dev);
+
+ s->syscfg_memrmp = 0x00000000;
+ s->syscfg_pmc = 0x00000000;
+ s->syscfg_exticr1 = 0x00000000;
+ s->syscfg_exticr2 = 0x00000000;
+ s->syscfg_exticr3 = 0x00000000;
+ s->syscfg_exticr4 = 0x00000000;
+ s->syscfg_cmpcr = 0x00000000;
+}
+
+static uint64_t netduino_syscfg_read(void *opaque, hwaddr addr,
+ unsigned int size)
+{
+ struct net_syscfg *s = opaque;
+
+ if (addr >= 0x400) {
+ addr = addr >> 7;
+ }
+
+ DPRINTF("Read 0x%x\n", (uint) addr);
+
+ switch (addr) {
+ case SYSCFG_MEMRMP:
+ return s->syscfg_memrmp;
+ case SYSCFG_PMC:
+ return s->syscfg_pmc;
+ case SYSCFG_EXTICR1:
+ return s->syscfg_exticr1;
+ case SYSCFG_EXTICR2:
+ return s->syscfg_exticr2;
+ case SYSCFG_EXTICR3:
+ return s->syscfg_exticr3;
+ case SYSCFG_EXTICR4:
+ return s->syscfg_exticr4;
+ case SYSCFG_CMPCR:
+ return s->syscfg_cmpcr;
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "net_syscfg_read: Bad offset %x\n", (int)addr);
+ return 0;
+ }
+
+ return 0;
+}
+
+static void netduino_syscfg_write(void *opaque, hwaddr addr,
+ uint64_t val64, unsigned int size)
+{
+ struct net_syscfg *s = opaque;
+ uint32_t value = (uint32_t) val64;
+
+ if (addr >= 0x400) {
+ addr = addr >> 7;
+ }
+
+ DPRINTF("Write 0x%x, 0x%x\n", value, (uint) addr);
+
+ switch (addr) {
+ case SYSCFG_MEMRMP:
+ /* This isn't supported, so don't allow the guest to write to it
+ * s->syscfg_memrmp = value;
+ */
+ return;
+ case SYSCFG_PMC:
+ /* This isn't supported, so don't allow the guest to write to it
+ * s->syscfg_pmc = value;
+ */
+ return;
+ case SYSCFG_EXTICR1:
+ s->syscfg_exticr1 = (value & 0xFF);
+ return;
+ case SYSCFG_EXTICR2:
+ s->syscfg_exticr2 = (value & 0xFF);
+ return;
+ case SYSCFG_EXTICR3:
+ s->syscfg_exticr3 = (value & 0xFF);
+ return;
+ case SYSCFG_EXTICR4:
+ s->syscfg_exticr4 = (value & 0xFF);
+ return;
+ case SYSCFG_CMPCR:
+ s->syscfg_cmpcr = value;
+ return;
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "net_syscfg_write: Bad offset %x\n", (int)addr);
+ }
+}
+
+static const MemoryRegionOps netduino_syscfg_ops = {
+ .read = netduino_syscfg_read,
+ .write = netduino_syscfg_write,
+ .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static int netduino_syscfg_init(SysBusDevice *sbd)
+{
+ DeviceState *dev = DEVICE(sbd);
+ struct net_syscfg *s = NETDUINO_SYSCFG(dev);
+
+ sysbus_init_irq(sbd, &s->irq);
+
+ memory_region_init_io(&s->mmio, OBJECT(s), &netduino_syscfg_ops, s,
+ TYPE_NETDUINO_SYSCFG, 0x2000);
+ sysbus_init_mmio(sbd, &s->mmio);
+
+ return 0;
+}
+
+static void netduino_syscfg_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+
+ k->init = netduino_syscfg_init;
+ dc->props = NULL;
+ dc->reset = syscfg_reset;
+}
+
+static const TypeInfo netduino_syscfg_info = {
+ .name = TYPE_NETDUINO_SYSCFG,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(struct net_syscfg),
+ .class_init = netduino_syscfg_class_init,
+};
+
+static void netduino_syscfg_register_types(void)
+{
+ type_register_static(&netduino_syscfg_info);
+}
+
+type_init(netduino_syscfg_register_types)
--
1.9.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v1 3/5] Netduino_SYSCFG: Add the Netduino Plus 2 SYSCFG
2014-08-24 0:14 ` [Qemu-devel] [PATCH v1 3/5] Netduino_SYSCFG: Add the Netduino Plus 2 SYSCFG Alistair Francis
@ 2014-08-24 3:09 ` Peter Crosthwaite
2014-09-01 11:49 ` Alistair Francis
0 siblings, 1 reply; 22+ messages in thread
From: Peter Crosthwaite @ 2014-08-24 3:09 UTC (permalink / raw)
To: Alistair Francis
Cc: Peter Maydell, qemu-devel@nongnu.org Developers, Konstanty Bialkowski
On Sun, Aug 24, 2014 at 10:14 AM, Alistair Francis <alistair23@gmail.com> wrote:
> This patch adds the Netduino Plus 2 System Configuration
> Controller. This is used to configure what memory is mapped
> at address 0 (although that is not supported) as well
> as configure how the EXTI interrupts work (also not
> supported at the moment).
>
Curious, Is this core needed to run the examples your posted on cover
letter or is it possible to just remove it? Do you need the dummy regs
to avoid crashes? (that's worth mentioning in commit msg).
> Signed-off-by: Alistair Francis <alistair23@gmail.com>
> ---
> hw/misc/Makefile.objs | 1 +
> hw/misc/netduino_syscfg.c | 201 ++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 202 insertions(+)
> create mode 100644 hw/misc/netduino_syscfg.c
>
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index 979e532..794f510 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -39,5 +39,6 @@ obj-$(CONFIG_OMAP) += omap_sdrc.o
> obj-$(CONFIG_OMAP) += omap_tap.o
> obj-$(CONFIG_SLAVIO) += slavio_misc.o
> obj-$(CONFIG_ZYNQ) += zynq_slcr.o
> +obj-$(CONFIG_NETDUINOP2) += netduino_syscfg.o
>
> obj-$(CONFIG_PVPANIC) += pvpanic.o
> diff --git a/hw/misc/netduino_syscfg.c b/hw/misc/netduino_syscfg.c
> new file mode 100644
> index 0000000..3db5295
> --- /dev/null
> +++ b/hw/misc/netduino_syscfg.c
> @@ -0,0 +1,201 @@
> +/*
> + * Netduino Plus 2 SYSCFG
> + *
> + * Copyright (c) 2014 Alistair Francis <alistair@alistair23.me>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "hw/sysbus.h"
> +#include "sysemu/char.h"
> +#include "hw/hw.h"
> +#include "net/net.h"
> +
> +/* #define DEBUG_NETSYSCFG */
> +
> +#ifdef DEBUG_NETSYSCFG
> +#define DPRINTF(fmt, ...) \
> +do { printf("netduino_syscfg: " fmt , ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...) do {} while (0)
> +#endif
> +
> +#define SYSCFG_MEMRMP 0x00
> +#define SYSCFG_PMC 0x04
> +#define SYSCFG_EXTICR1 0x08
> +#define SYSCFG_EXTICR2 0x0C
> +#define SYSCFG_EXTICR3 0x10
> +#define SYSCFG_EXTICR4 0x14
> +#define SYSCFG_CMPCR 0x20
> +
> +#define TYPE_NETDUINO_SYSCFG "netduino_syscfg"
> +#define NETDUINO_SYSCFG(obj) \
> + OBJECT_CHECK(struct net_syscfg, (obj), TYPE_NETDUINO_SYSCFG)
> +
> +struct net_syscfg {
> + SysBusDevice parent_obj;
> +
> + MemoryRegion mmio;
> +
> + uint32_t syscfg_memrmp;
> + uint32_t syscfg_pmc;
> + uint32_t syscfg_exticr1;
> + uint32_t syscfg_exticr2;
> + uint32_t syscfg_exticr3;
> + uint32_t syscfg_exticr4;
> + uint32_t syscfg_cmpcr;
> +
> + qemu_irq irq;
> +};
> +
> +static void syscfg_reset(DeviceState *dev)
> +{
> + struct net_syscfg *s = NETDUINO_SYSCFG(dev);
> +
> + s->syscfg_memrmp = 0x00000000;
> + s->syscfg_pmc = 0x00000000;
> + s->syscfg_exticr1 = 0x00000000;
> + s->syscfg_exticr2 = 0x00000000;
> + s->syscfg_exticr3 = 0x00000000;
> + s->syscfg_exticr4 = 0x00000000;
> + s->syscfg_cmpcr = 0x00000000;
> +}
> +
> +static uint64_t netduino_syscfg_read(void *opaque, hwaddr addr,
> + unsigned int size)
> +{
> + struct net_syscfg *s = opaque;
> +
> + if (addr >= 0x400) {
> + addr = addr >> 7;
> + }
What's this do? I think it's worthy of a comment :).
> +
> + DPRINTF("Read 0x%x\n", (uint) addr);
> +
> + switch (addr) {
> + case SYSCFG_MEMRMP:
> + return s->syscfg_memrmp;
> + case SYSCFG_PMC:
> + return s->syscfg_pmc;
> + case SYSCFG_EXTICR1:
> + return s->syscfg_exticr1;
> + case SYSCFG_EXTICR2:
> + return s->syscfg_exticr2;
> + case SYSCFG_EXTICR3:
> + return s->syscfg_exticr3;
> + case SYSCFG_EXTICR4:
> + return s->syscfg_exticr4;
> + case SYSCFG_CMPCR:
> + return s->syscfg_cmpcr;
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "net_syscfg_read: Bad offset %x\n", (int)addr);
> + return 0;
> + }
> +
> + return 0;
> +}
> +
> +static void netduino_syscfg_write(void *opaque, hwaddr addr,
> + uint64_t val64, unsigned int size)
> +{
> + struct net_syscfg *s = opaque;
> + uint32_t value = (uint32_t) val64;
> +
> + if (addr >= 0x400) {
> + addr = addr >> 7;
> + }
> +
> + DPRINTF("Write 0x%x, 0x%x\n", value, (uint) addr);
> +
> + switch (addr) {
> + case SYSCFG_MEMRMP:
> + /* This isn't supported, so don't allow the guest to write to it
> + * s->syscfg_memrmp = value;
> + */
qemu_log_mask(LOG_UNIMP
Regards,
Peter
> + return;
> + case SYSCFG_PMC:
> + /* This isn't supported, so don't allow the guest to write to it
> + * s->syscfg_pmc = value;
> + */
> + return;
> + case SYSCFG_EXTICR1:
> + s->syscfg_exticr1 = (value & 0xFF);
> + return;
> + case SYSCFG_EXTICR2:
> + s->syscfg_exticr2 = (value & 0xFF);
> + return;
> + case SYSCFG_EXTICR3:
> + s->syscfg_exticr3 = (value & 0xFF);
> + return;
> + case SYSCFG_EXTICR4:
> + s->syscfg_exticr4 = (value & 0xFF);
> + return;
> + case SYSCFG_CMPCR:
> + s->syscfg_cmpcr = value;
> + return;
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "net_syscfg_write: Bad offset %x\n", (int)addr);
> + }
> +}
> +
> +static const MemoryRegionOps netduino_syscfg_ops = {
> + .read = netduino_syscfg_read,
> + .write = netduino_syscfg_write,
> + .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static int netduino_syscfg_init(SysBusDevice *sbd)
> +{
> + DeviceState *dev = DEVICE(sbd);
> + struct net_syscfg *s = NETDUINO_SYSCFG(dev);
> +
> + sysbus_init_irq(sbd, &s->irq);
> +
> + memory_region_init_io(&s->mmio, OBJECT(s), &netduino_syscfg_ops, s,
> + TYPE_NETDUINO_SYSCFG, 0x2000);
> + sysbus_init_mmio(sbd, &s->mmio);
> +
> + return 0;
> +}
> +
> +static void netduino_syscfg_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +
> + k->init = netduino_syscfg_init;
> + dc->props = NULL;
> + dc->reset = syscfg_reset;
> +}
> +
> +static const TypeInfo netduino_syscfg_info = {
> + .name = TYPE_NETDUINO_SYSCFG,
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(struct net_syscfg),
> + .class_init = netduino_syscfg_class_init,
> +};
> +
> +static void netduino_syscfg_register_types(void)
> +{
> + type_register_static(&netduino_syscfg_info);
> +}
> +
> +type_init(netduino_syscfg_register_types)
> --
> 1.9.1
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v1 3/5] Netduino_SYSCFG: Add the Netduino Plus 2 SYSCFG
2014-08-24 3:09 ` Peter Crosthwaite
@ 2014-09-01 11:49 ` Alistair Francis
0 siblings, 0 replies; 22+ messages in thread
From: Alistair Francis @ 2014-09-01 11:49 UTC (permalink / raw)
To: Peter Crosthwaite
Cc: Peter Maydell, qemu-devel@nongnu.org Developers, Konstanty Bialkowski
On Sun, Aug 24, 2014 at 1:09 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Sun, Aug 24, 2014 at 10:14 AM, Alistair Francis <alistair23@gmail.com> wrote:
>> This patch adds the Netduino Plus 2 System Configuration
>> Controller. This is used to configure what memory is mapped
>> at address 0 (although that is not supported) as well
>> as configure how the EXTI interrupts work (also not
>> supported at the moment).
>>
>
> Curious, Is this core needed to run the examples your posted on cover
> letter or is it possible to just remove it? Do you need the dummy regs
> to avoid crashes? (that's worth mentioning in commit msg).
It's not required for basic examples, but more complex examples will
expect it to be there. It's also required for external interrupts
Thanks,
Alistair
>> Signed-off-by: Alistair Francis <alistair23@gmail.com>
>> ---
>> hw/misc/Makefile.objs | 1 +
>> hw/misc/netduino_syscfg.c | 201 ++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 202 insertions(+)
>> create mode 100644 hw/misc/netduino_syscfg.c
>>
>> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
>> index 979e532..794f510 100644
>> --- a/hw/misc/Makefile.objs
>> +++ b/hw/misc/Makefile.objs
>> @@ -39,5 +39,6 @@ obj-$(CONFIG_OMAP) += omap_sdrc.o
>> obj-$(CONFIG_OMAP) += omap_tap.o
>> obj-$(CONFIG_SLAVIO) += slavio_misc.o
>> obj-$(CONFIG_ZYNQ) += zynq_slcr.o
>> +obj-$(CONFIG_NETDUINOP2) += netduino_syscfg.o
>>
>> obj-$(CONFIG_PVPANIC) += pvpanic.o
>> diff --git a/hw/misc/netduino_syscfg.c b/hw/misc/netduino_syscfg.c
>> new file mode 100644
>> index 0000000..3db5295
>> --- /dev/null
>> +++ b/hw/misc/netduino_syscfg.c
>> @@ -0,0 +1,201 @@
>> +/*
>> + * Netduino Plus 2 SYSCFG
>> + *
>> + * Copyright (c) 2014 Alistair Francis <alistair@alistair23.me>
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#include "hw/sysbus.h"
>> +#include "sysemu/char.h"
>> +#include "hw/hw.h"
>> +#include "net/net.h"
>> +
>> +/* #define DEBUG_NETSYSCFG */
>> +
>> +#ifdef DEBUG_NETSYSCFG
>> +#define DPRINTF(fmt, ...) \
>> +do { printf("netduino_syscfg: " fmt , ## __VA_ARGS__); } while (0)
>> +#else
>> +#define DPRINTF(fmt, ...) do {} while (0)
>> +#endif
>> +
>> +#define SYSCFG_MEMRMP 0x00
>> +#define SYSCFG_PMC 0x04
>> +#define SYSCFG_EXTICR1 0x08
>> +#define SYSCFG_EXTICR2 0x0C
>> +#define SYSCFG_EXTICR3 0x10
>> +#define SYSCFG_EXTICR4 0x14
>> +#define SYSCFG_CMPCR 0x20
>> +
>> +#define TYPE_NETDUINO_SYSCFG "netduino_syscfg"
>> +#define NETDUINO_SYSCFG(obj) \
>> + OBJECT_CHECK(struct net_syscfg, (obj), TYPE_NETDUINO_SYSCFG)
>> +
>> +struct net_syscfg {
>> + SysBusDevice parent_obj;
>> +
>> + MemoryRegion mmio;
>> +
>> + uint32_t syscfg_memrmp;
>> + uint32_t syscfg_pmc;
>> + uint32_t syscfg_exticr1;
>> + uint32_t syscfg_exticr2;
>> + uint32_t syscfg_exticr3;
>> + uint32_t syscfg_exticr4;
>> + uint32_t syscfg_cmpcr;
>> +
>> + qemu_irq irq;
>> +};
>> +
>> +static void syscfg_reset(DeviceState *dev)
>> +{
>> + struct net_syscfg *s = NETDUINO_SYSCFG(dev);
>> +
>> + s->syscfg_memrmp = 0x00000000;
>> + s->syscfg_pmc = 0x00000000;
>> + s->syscfg_exticr1 = 0x00000000;
>> + s->syscfg_exticr2 = 0x00000000;
>> + s->syscfg_exticr3 = 0x00000000;
>> + s->syscfg_exticr4 = 0x00000000;
>> + s->syscfg_cmpcr = 0x00000000;
>> +}
>> +
>> +static uint64_t netduino_syscfg_read(void *opaque, hwaddr addr,
>> + unsigned int size)
>> +{
>> + struct net_syscfg *s = opaque;
>> +
>> + if (addr >= 0x400) {
>> + addr = addr >> 7;
>> + }
>
> What's this do? I think it's worthy of a comment :).
>
>> +
>> + DPRINTF("Read 0x%x\n", (uint) addr);
>> +
>> + switch (addr) {
>> + case SYSCFG_MEMRMP:
>> + return s->syscfg_memrmp;
>> + case SYSCFG_PMC:
>> + return s->syscfg_pmc;
>> + case SYSCFG_EXTICR1:
>> + return s->syscfg_exticr1;
>> + case SYSCFG_EXTICR2:
>> + return s->syscfg_exticr2;
>> + case SYSCFG_EXTICR3:
>> + return s->syscfg_exticr3;
>> + case SYSCFG_EXTICR4:
>> + return s->syscfg_exticr4;
>> + case SYSCFG_CMPCR:
>> + return s->syscfg_cmpcr;
>> + default:
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "net_syscfg_read: Bad offset %x\n", (int)addr);
>> + return 0;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void netduino_syscfg_write(void *opaque, hwaddr addr,
>> + uint64_t val64, unsigned int size)
>> +{
>> + struct net_syscfg *s = opaque;
>> + uint32_t value = (uint32_t) val64;
>> +
>> + if (addr >= 0x400) {
>> + addr = addr >> 7;
>> + }
>> +
>> + DPRINTF("Write 0x%x, 0x%x\n", value, (uint) addr);
>> +
>> + switch (addr) {
>> + case SYSCFG_MEMRMP:
>> + /* This isn't supported, so don't allow the guest to write to it
>> + * s->syscfg_memrmp = value;
>> + */
>
> qemu_log_mask(LOG_UNIMP
>
> Regards,
> Peter
>
>> + return;
>> + case SYSCFG_PMC:
>> + /* This isn't supported, so don't allow the guest to write to it
>> + * s->syscfg_pmc = value;
>> + */
>> + return;
>> + case SYSCFG_EXTICR1:
>> + s->syscfg_exticr1 = (value & 0xFF);
>> + return;
>> + case SYSCFG_EXTICR2:
>> + s->syscfg_exticr2 = (value & 0xFF);
>> + return;
>> + case SYSCFG_EXTICR3:
>> + s->syscfg_exticr3 = (value & 0xFF);
>> + return;
>> + case SYSCFG_EXTICR4:
>> + s->syscfg_exticr4 = (value & 0xFF);
>> + return;
>> + case SYSCFG_CMPCR:
>> + s->syscfg_cmpcr = value;
>> + return;
>> + default:
>> + qemu_log_mask(LOG_GUEST_ERROR,
>> + "net_syscfg_write: Bad offset %x\n", (int)addr);
>> + }
>> +}
>> +
>> +static const MemoryRegionOps netduino_syscfg_ops = {
>> + .read = netduino_syscfg_read,
>> + .write = netduino_syscfg_write,
>> + .endianness = DEVICE_NATIVE_ENDIAN,
>> +};
>> +
>> +static int netduino_syscfg_init(SysBusDevice *sbd)
>> +{
>> + DeviceState *dev = DEVICE(sbd);
>> + struct net_syscfg *s = NETDUINO_SYSCFG(dev);
>> +
>> + sysbus_init_irq(sbd, &s->irq);
>> +
>> + memory_region_init_io(&s->mmio, OBJECT(s), &netduino_syscfg_ops, s,
>> + TYPE_NETDUINO_SYSCFG, 0x2000);
>> + sysbus_init_mmio(sbd, &s->mmio);
>> +
>> + return 0;
>> +}
>> +
>> +static void netduino_syscfg_class_init(ObjectClass *klass, void *data)
>> +{
>> + DeviceClass *dc = DEVICE_CLASS(klass);
>> + SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
>> +
>> + k->init = netduino_syscfg_init;
>> + dc->props = NULL;
>> + dc->reset = syscfg_reset;
>> +}
>> +
>> +static const TypeInfo netduino_syscfg_info = {
>> + .name = TYPE_NETDUINO_SYSCFG,
>> + .parent = TYPE_SYS_BUS_DEVICE,
>> + .instance_size = sizeof(struct net_syscfg),
>> + .class_init = netduino_syscfg_class_init,
>> +};
>> +
>> +static void netduino_syscfg_register_types(void)
>> +{
>> + type_register_static(&netduino_syscfg_info);
>> +}
>> +
>> +type_init(netduino_syscfg_register_types)
>> --
>> 1.9.1
>>
>>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH v1 4/5] Netduino_Timer: Add the Netduino Plus 2 Timer2 to 5
2014-08-24 0:13 [Qemu-devel] [PATCH v1 0/5] Netduino Plus 2 Machine Model Alistair Francis
` (2 preceding siblings ...)
2014-08-24 0:14 ` [Qemu-devel] [PATCH v1 3/5] Netduino_SYSCFG: Add the Netduino Plus 2 SYSCFG Alistair Francis
@ 2014-08-24 0:14 ` Alistair Francis
2014-08-24 3:27 ` Peter Crosthwaite
2014-08-24 0:14 ` [Qemu-devel] [PATCH v1 5/5] Netduino: Add the Netduino Plus 2 Machine Model Alistair Francis
2014-09-01 16:39 ` [Qemu-devel] [PATCH v1 0/5] " Peter Maydell
5 siblings, 1 reply; 22+ messages in thread
From: Alistair Francis @ 2014-08-24 0:14 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, peter.crosthwaite, konstanty
This patch adds the Netduino Plus 2 timers: TIM2, TIM3, TIM4 and TIM5
to QEMU.
Signed-off-by: Alistair Francis <alistair23@gmail.com>
---
hw/timer/Makefile.objs | 1 +
hw/timer/netduino_timer.c | 384 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 385 insertions(+)
create mode 100644 hw/timer/netduino_timer.c
diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
index 2c86c3d..104fc46 100644
--- a/hw/timer/Makefile.objs
+++ b/hw/timer/Makefile.objs
@@ -17,6 +17,7 @@ common-obj-$(CONFIG_IMX) += imx_epit.o
common-obj-$(CONFIG_IMX) += imx_gpt.o
common-obj-$(CONFIG_LM32) += lm32_timer.o
common-obj-$(CONFIG_MILKYMIST) += milkymist-sysctl.o
+common-obj-$(CONFIG_NETDUINOP2) += netduino_timer.o
obj-$(CONFIG_EXYNOS4) += exynos4210_mct.o
obj-$(CONFIG_EXYNOS4) += exynos4210_pwm.o
diff --git a/hw/timer/netduino_timer.c b/hw/timer/netduino_timer.c
new file mode 100644
index 0000000..212ba48
--- /dev/null
+++ b/hw/timer/netduino_timer.c
@@ -0,0 +1,384 @@
+/*
+ * Netduino Plus 2 USART
+ *
+ * Copyright (c) 2014 Alistair Francis <alistair@alistair23.me>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "hw/sysbus.h"
+#include "qemu/timer.h"
+#include "sysemu/sysemu.h"
+
+#define DEBUG_NETTIMER
+
+#ifdef DEBUG_NETTIMER
+#define DPRINTF(fmt, ...) \
+do { printf("netduino_timer: " fmt , ## __VA_ARGS__); } while (0)
+#else
+#define DPRINTF(fmt, ...) do {} while (0)
+#endif
+
+#define TIM_CR1 0x00
+#define TIM_CR2 0x04
+#define TIM_SMCR 0x08
+#define TIM_DIER 0x0C
+#define TIM_SR 0x10
+#define TIM_EGR 0x14
+#define TIM_CCMR1 0x18
+#define TIM_CCMR2 0x1C
+#define TIM_CCER 0x20
+#define TIM_CNT 0x24
+#define TIM_PSC 0x28
+#define TIM_ARR 0x2C
+#define TIM_CCR1 0x34
+#define TIM_CCR2 0x38
+#define TIM_CCR3 0x3C
+#define TIM_CCR4 0x40
+#define TIM_DCR 0x48
+#define TIM_DMAR 0x4C
+#define TIM_OR 0x50
+
+#define TIM_CR1_CEN 1
+
+#define TYPE_NETTIMER "netduino_timer"
+#define NETTIMER(obj) OBJECT_CHECK(NETTIMERState, (obj), TYPE_NETTIMER)
+
+typedef struct NETTIMERState {
+ SysBusDevice parent_obj;
+
+ MemoryRegion iomem;
+ QEMUTimer *timer;
+ qemu_irq irq;
+
+ uint32_t tick_offset_vmstate;
+ uint32_t tick_offset;
+
+ uint32_t tim_cr1;
+ uint32_t tim_cr2;
+ uint32_t tim_smcr;
+ uint32_t tim_dier;
+ uint32_t tim_sr;
+ uint32_t tim_egr;
+ uint32_t tim_ccmr1;
+ uint32_t tim_ccmr2;
+ uint32_t tim_ccer;
+ uint32_t tim_cnt;
+ uint32_t tim_psc;
+ uint32_t tim_arr;
+ uint32_t tim_ccr1;
+ uint32_t tim_ccr2;
+ uint32_t tim_ccr3;
+ uint32_t tim_ccr4;
+ uint32_t tim_dcr;
+ uint32_t tim_dmar;
+ uint32_t tim_or;
+} NETTIMERState;
+
+static void netduino_timer_update(NETTIMERState *s)
+{
+ s->tim_sr |= 1;
+ qemu_set_irq(s->irq, 1);
+ qemu_set_irq(s->irq, 0);
+}
+
+static void netduino_timer_interrupt(void *opaque)
+{
+ NETTIMERState *s = (NETTIMERState *)opaque;
+
+ DPRINTF("INterrupt\n");
+
+ if (s->tim_dier == 0x01 && s->tim_cr1 & TIM_CR1_CEN) {
+ netduino_timer_update(s);
+ }
+}
+
+static uint32_t netduino_timer_get_count(NETTIMERState *s)
+{
+ int64_t now = qemu_clock_get_ns(rtc_clock);
+ return s->tick_offset + now / get_ticks_per_sec();
+}
+
+static void netduino_timer_set_alarm(NETTIMERState *s)
+{
+ uint32_t ticks;
+
+ DPRINTF("Alarm raised: 0x%x\n", s->tim_cr1);
+
+ ticks = (uint32_t) (s->tim_arr - netduino_timer_get_count(s)/
+ (s->tim_psc + 1));
+ DPRINTF("Alarm set in %u ticks\n", ticks);
+ if (ticks <= 0) {
+ timer_del(s->timer);
+ netduino_timer_interrupt(s);
+ } else {
+ int64_t now = qemu_clock_get_ns(rtc_clock);
+ timer_mod(s->timer, now + (int64_t)ticks);
+ DPRINTF("Wait Time: 0x%x\n", (uint32_t) (now + ticks));
+ }
+}
+
+static void netduino_timer_reset(DeviceState *dev)
+{
+ struct NETTIMERState *s = NETTIMER(dev);
+
+ s->tim_cr1 = 0;
+ s->tim_cr2 = 0;
+ s->tim_smcr = 0;
+ s->tim_dier = 0;
+ s->tim_sr = 0;
+ s->tim_egr = 0;
+ s->tim_ccmr1 = 0;
+ s->tim_ccmr2 = 0;
+ s->tim_ccer = 0;
+ s->tim_cnt = 0;
+ s->tim_psc = 0;
+ s->tim_arr = 0;
+ s->tim_ccr1 = 0;
+ s->tim_ccr2 = 0;
+ s->tim_ccr3 = 0;
+ s->tim_ccr4 = 0;
+ s->tim_dcr = 0;
+ s->tim_dmar = 0;
+ s->tim_or = 0;
+}
+
+static uint64_t netduino_timer_read(void *opaque, hwaddr offset,
+ unsigned size)
+{
+ NETTIMERState *s = (NETTIMERState *)opaque;
+
+ DPRINTF("Read 0x%x\n", (uint) offset);
+
+ switch (offset) {
+ case TIM_CR1:
+ return s->tim_cr1;
+ case TIM_CR2:
+ return s->tim_cr2;
+ case TIM_SMCR:
+ return s->tim_smcr;
+ case TIM_DIER:
+ return s->tim_dier;
+ case TIM_SR:
+ return s->tim_sr;
+ case TIM_EGR:
+ return s->tim_egr;
+ case TIM_CCMR1:
+ return s->tim_ccmr1;
+ case TIM_CCMR2:
+ return s->tim_ccmr2;
+ case TIM_CCER:
+ return s->tim_ccer;
+ case TIM_CNT:
+ return s->tim_cnt;
+ case TIM_PSC:
+ return s->tim_psc;
+ case TIM_ARR:
+ return s->tim_arr;
+ case TIM_CCR1:
+ return s->tim_ccr1;
+ case TIM_CCR2:
+ return s->tim_ccr2;
+ case TIM_CCR3:
+ return s->tim_ccr3;
+ case TIM_CCR4:
+ return s->tim_ccr4;
+ case TIM_DCR:
+ return s->tim_dcr;
+ case TIM_DMAR:
+ return s->tim_dmar;
+ case TIM_OR:
+ return s->tim_or;
+ }
+
+ return 0;
+}
+
+static void netduino_timer_write(void *opaque, hwaddr offset,
+ uint64_t val64, unsigned size)
+{
+ NETTIMERState *s = (NETTIMERState *)opaque;
+ uint32_t value = (uint32_t) val64;
+
+ DPRINTF("Write 0x%x, 0x%x\n", value, (uint) offset);
+
+ switch (offset) {
+ case TIM_CR1:
+ s->tim_cr1 = value;
+ return;
+ case TIM_CR2:
+ s->tim_cr2 = value;
+ return;
+ case TIM_SMCR:
+ s->tim_smcr = value;
+ return;
+ case TIM_DIER:
+ s->tim_dier = value;
+ return;
+ case TIM_SR:
+ s->tim_sr &= value;
+ netduino_timer_set_alarm(s);
+ return;
+ case TIM_EGR:
+ s->tim_egr = value;
+ return;
+ case TIM_CCMR1:
+ s->tim_ccmr1 = value;
+ return;
+ case TIM_CCMR2:
+ s->tim_ccmr2 = value;
+ return;
+ case TIM_CCER:
+ s->tim_ccer = value;
+ return;
+ case TIM_CNT:
+ s->tim_cnt = value;
+ netduino_timer_set_alarm(s);
+ return;
+ case TIM_PSC:
+ s->tim_psc = value;
+ return;
+ case TIM_ARR:
+ s->tim_arr = value;
+ netduino_timer_set_alarm(s);
+ return;
+ case TIM_CCR1:
+ s->tim_ccr1 = value;
+ return;
+ case TIM_CCR2:
+ s->tim_ccr2 = value;
+ return;
+ case TIM_CCR3:
+ s->tim_ccr3 = value;
+ return;
+ case TIM_CCR4:
+ s->tim_ccr4 = value;
+ return;
+ case TIM_DCR:
+ s->tim_dcr = value;
+ return;
+ case TIM_DMAR:
+ s->tim_dmar = value;
+ return;
+ case TIM_OR:
+ s->tim_or = value;
+ return;
+ }
+}
+
+static const MemoryRegionOps netduino_timer_ops = {
+ .read = netduino_timer_read,
+ .write = netduino_timer_write,
+ .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static int netduino_timer_init(SysBusDevice *dev)
+{
+ NETTIMERState *s = NETTIMER(dev);
+ struct tm tm;
+
+ sysbus_init_irq(dev, &s->irq);
+
+ memory_region_init_io(&s->iomem, OBJECT(s), &netduino_timer_ops, s,
+ "netduino_timer", 0x2000);
+ sysbus_init_mmio(dev, &s->iomem);
+
+ qemu_get_timedate(&tm, 0);
+ s->tick_offset = mktimegm(&tm) -
+ qemu_clock_get_ns(rtc_clock) / get_ticks_per_sec();
+
+ s->timer = timer_new_ns(rtc_clock, netduino_timer_interrupt, s);
+
+ return 0;
+}
+
+static void netduino_timer_pre_save(void *opaque)
+{
+ NETTIMERState *s = (NETTIMERState *)opaque;
+
+ int64_t delta = qemu_clock_get_ns(rtc_clock) -
+ qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+ s->tick_offset_vmstate = s->tick_offset + delta / get_ticks_per_sec();
+}
+
+static int netduino_timer_post_load(void *opaque, int version_id)
+{
+ NETTIMERState *s = (NETTIMERState *)opaque;
+
+ int64_t delta = qemu_clock_get_ns(rtc_clock) -
+ qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+ s->tick_offset = s->tick_offset_vmstate - delta / get_ticks_per_sec();
+ netduino_timer_set_alarm(s);
+ return 0;
+}
+
+static const VMStateDescription vmstate_netduino_timer = {
+ .name = "netduino_timer",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .pre_save = netduino_timer_pre_save,
+ .post_load = netduino_timer_post_load,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(tick_offset_vmstate, NETTIMERState),
+ VMSTATE_UINT32(tim_cr1, NETTIMERState),
+ VMSTATE_UINT32(tim_cr2, NETTIMERState),
+ VMSTATE_UINT32(tim_smcr, NETTIMERState),
+ VMSTATE_UINT32(tim_dier, NETTIMERState),
+ VMSTATE_UINT32(tim_sr, NETTIMERState),
+ VMSTATE_UINT32(tim_egr, NETTIMERState),
+ VMSTATE_UINT32(tim_ccmr1, NETTIMERState),
+ VMSTATE_UINT32(tim_ccmr1, NETTIMERState),
+ VMSTATE_UINT32(tim_ccer, NETTIMERState),
+ VMSTATE_UINT32(tim_cnt, NETTIMERState),
+ VMSTATE_UINT32(tim_psc, NETTIMERState),
+ VMSTATE_UINT32(tim_arr, NETTIMERState),
+ VMSTATE_UINT32(tim_ccr1, NETTIMERState),
+ VMSTATE_UINT32(tim_ccr2, NETTIMERState),
+ VMSTATE_UINT32(tim_ccr3, NETTIMERState),
+ VMSTATE_UINT32(tim_ccr4, NETTIMERState),
+ VMSTATE_UINT32(tim_dcr, NETTIMERState),
+ VMSTATE_UINT32(tim_dmar, NETTIMERState),
+ VMSTATE_UINT32(tim_or, NETTIMERState),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static void netduino_timer_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
+
+ k->init = netduino_timer_init;
+ dc->vmsd = &vmstate_netduino_timer;
+ dc->reset = netduino_timer_reset;
+}
+
+static const TypeInfo netduino_timer_info = {
+ .name = TYPE_NETTIMER,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(NETTIMERState),
+ .class_init = netduino_timer_class_init,
+};
+
+static void netduino_timer_register_types(void)
+{
+ type_register_static(&netduino_timer_info);
+}
+
+type_init(netduino_timer_register_types)
--
1.9.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v1 4/5] Netduino_Timer: Add the Netduino Plus 2 Timer2 to 5
2014-08-24 0:14 ` [Qemu-devel] [PATCH v1 4/5] Netduino_Timer: Add the Netduino Plus 2 Timer2 to 5 Alistair Francis
@ 2014-08-24 3:27 ` Peter Crosthwaite
0 siblings, 0 replies; 22+ messages in thread
From: Peter Crosthwaite @ 2014-08-24 3:27 UTC (permalink / raw)
To: Alistair Francis
Cc: Peter Maydell, qemu-devel@nongnu.org Developers, Konstanty Bialkowski
On Sun, Aug 24, 2014 at 10:14 AM, Alistair Francis <alistair23@gmail.com> wrote:
> This patch adds the Netduino Plus 2 timers: TIM2, TIM3, TIM4 and TIM5
> to QEMU.
>
> Signed-off-by: Alistair Francis <alistair23@gmail.com>
> ---
> hw/timer/Makefile.objs | 1 +
> hw/timer/netduino_timer.c | 384 ++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 385 insertions(+)
> create mode 100644 hw/timer/netduino_timer.c
>
> diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
> index 2c86c3d..104fc46 100644
> --- a/hw/timer/Makefile.objs
> +++ b/hw/timer/Makefile.objs
> @@ -17,6 +17,7 @@ common-obj-$(CONFIG_IMX) += imx_epit.o
> common-obj-$(CONFIG_IMX) += imx_gpt.o
> common-obj-$(CONFIG_LM32) += lm32_timer.o
> common-obj-$(CONFIG_MILKYMIST) += milkymist-sysctl.o
> +common-obj-$(CONFIG_NETDUINOP2) += netduino_timer.o
>
> obj-$(CONFIG_EXYNOS4) += exynos4210_mct.o
> obj-$(CONFIG_EXYNOS4) += exynos4210_pwm.o
> diff --git a/hw/timer/netduino_timer.c b/hw/timer/netduino_timer.c
> new file mode 100644
> index 0000000..212ba48
> --- /dev/null
> +++ b/hw/timer/netduino_timer.c
> @@ -0,0 +1,384 @@
> +/*
> + * Netduino Plus 2 USART
> + *
> + * Copyright (c) 2014 Alistair Francis <alistair@alistair23.me>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "hw/sysbus.h"
> +#include "qemu/timer.h"
> +#include "sysemu/sysemu.h"
> +
> +#define DEBUG_NETTIMER
> +
> +#ifdef DEBUG_NETTIMER
> +#define DPRINTF(fmt, ...) \
> +do { printf("netduino_timer: " fmt , ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...) do {} while (0)
> +#endif
> +
> +#define TIM_CR1 0x00
> +#define TIM_CR2 0x04
> +#define TIM_SMCR 0x08
> +#define TIM_DIER 0x0C
> +#define TIM_SR 0x10
> +#define TIM_EGR 0x14
> +#define TIM_CCMR1 0x18
> +#define TIM_CCMR2 0x1C
> +#define TIM_CCER 0x20
> +#define TIM_CNT 0x24
> +#define TIM_PSC 0x28
> +#define TIM_ARR 0x2C
> +#define TIM_CCR1 0x34
> +#define TIM_CCR2 0x38
> +#define TIM_CCR3 0x3C
> +#define TIM_CCR4 0x40
> +#define TIM_DCR 0x48
> +#define TIM_DMAR 0x4C
> +#define TIM_OR 0x50
> +
> +#define TIM_CR1_CEN 1
> +
> +#define TYPE_NETTIMER "netduino_timer"
> +#define NETTIMER(obj) OBJECT_CHECK(NETTIMERState, (obj), TYPE_NETTIMER)
> +
> +typedef struct NETTIMERState {
Why the all Caps? Is it a symbolic name from documentation?
> + SysBusDevice parent_obj;
> +
> + MemoryRegion iomem;
> + QEMUTimer *timer;
> + qemu_irq irq;
> +
> + uint32_t tick_offset_vmstate;
> + uint32_t tick_offset;
> +
> + uint32_t tim_cr1;
> + uint32_t tim_cr2;
> + uint32_t tim_smcr;
> + uint32_t tim_dier;
> + uint32_t tim_sr;
> + uint32_t tim_egr;
> + uint32_t tim_ccmr1;
> + uint32_t tim_ccmr2;
> + uint32_t tim_ccer;
> + uint32_t tim_cnt;
> + uint32_t tim_psc;
> + uint32_t tim_arr;
> + uint32_t tim_ccr1;
> + uint32_t tim_ccr2;
> + uint32_t tim_ccr3;
> + uint32_t tim_ccr4;
> + uint32_t tim_dcr;
> + uint32_t tim_dmar;
> + uint32_t tim_or;
> +} NETTIMERState;
> +
> +static void netduino_timer_update(NETTIMERState *s)
> +{
> + s->tim_sr |= 1;
> + qemu_set_irq(s->irq, 1);
> + qemu_set_irq(s->irq, 0);
qemu_irq_pulse
> +}
> +
> +static void netduino_timer_interrupt(void *opaque)
> +{
> + NETTIMERState *s = (NETTIMERState *)opaque;
> +
> + DPRINTF("INterrupt\n");
"Interrupt". Probably add a %s __func__ in there too for clarity.
> +
> + if (s->tim_dier == 0x01 && s->tim_cr1 & TIM_CR1_CEN) {
> + netduino_timer_update(s);
> + }
> +}
> +
> +static uint32_t netduino_timer_get_count(NETTIMERState *s)
> +{
> + int64_t now = qemu_clock_get_ns(rtc_clock);
> + return s->tick_offset + now / get_ticks_per_sec();
> +}
> +
> +static void netduino_timer_set_alarm(NETTIMERState *s)
> +{
> + uint32_t ticks;
> +
> + DPRINTF("Alarm raised: 0x%x\n", s->tim_cr1);
> +
%s __func__ or something to identify this IP as owning this message.
> + ticks = (uint32_t) (s->tim_arr - netduino_timer_get_count(s)/
> + (s->tim_psc + 1));
cast not needed.
> + DPRINTF("Alarm set in %u ticks\n", ticks);
> + if (ticks <= 0) {
ticks is unsigned, so == would be same semantics or simply "if
(!ticks)". Are you going wanting signed logic?
> + timer_del(s->timer);
> + netduino_timer_interrupt(s);
> + } else {
> + int64_t now = qemu_clock_get_ns(rtc_clock);
> + timer_mod(s->timer, now + (int64_t)ticks);
now seems to be in ns, while ticks seems to be in clock-cycles - do
you need a translation factor for ticks? netduino_timer_get_count has
a division factor of get_ticks_per_sec that coverts from ns to "ticks"
so do you need a converse here to go back the other way?
> + DPRINTF("Wait Time: 0x%x\n", (uint32_t) (now + ticks));
> + }
> +}
> +
> +static void netduino_timer_reset(DeviceState *dev)
> +{
> + struct NETTIMERState *s = NETTIMER(dev);
> +
> + s->tim_cr1 = 0;
> + s->tim_cr2 = 0;
> + s->tim_smcr = 0;
> + s->tim_dier = 0;
> + s->tim_sr = 0;
> + s->tim_egr = 0;
> + s->tim_ccmr1 = 0;
> + s->tim_ccmr2 = 0;
> + s->tim_ccer = 0;
> + s->tim_cnt = 0;
> + s->tim_psc = 0;
> + s->tim_arr = 0;
> + s->tim_ccr1 = 0;
> + s->tim_ccr2 = 0;
> + s->tim_ccr3 = 0;
> + s->tim_ccr4 = 0;
> + s->tim_dcr = 0;
> + s->tim_dmar = 0;
> + s->tim_or = 0;
> +}
> +
> +static uint64_t netduino_timer_read(void *opaque, hwaddr offset,
> + unsigned size)
> +{
> + NETTIMERState *s = (NETTIMERState *)opaque;
> +
> + DPRINTF("Read 0x%x\n", (uint) offset);
> +
> + switch (offset) {
> + case TIM_CR1:
> + return s->tim_cr1;
> + case TIM_CR2:
> + return s->tim_cr2;
> + case TIM_SMCR:
> + return s->tim_smcr;
> + case TIM_DIER:
> + return s->tim_dier;
> + case TIM_SR:
> + return s->tim_sr;
> + case TIM_EGR:
> + return s->tim_egr;
> + case TIM_CCMR1:
> + return s->tim_ccmr1;
> + case TIM_CCMR2:
> + return s->tim_ccmr2;
> + case TIM_CCER:
> + return s->tim_ccer;
> + case TIM_CNT:
> + return s->tim_cnt;
> + case TIM_PSC:
> + return s->tim_psc;
> + case TIM_ARR:
> + return s->tim_arr;
> + case TIM_CCR1:
> + return s->tim_ccr1;
> + case TIM_CCR2:
> + return s->tim_ccr2;
> + case TIM_CCR3:
> + return s->tim_ccr3;
> + case TIM_CCR4:
> + return s->tim_ccr4;
> + case TIM_DCR:
> + return s->tim_dcr;
> + case TIM_DMAR:
> + return s->tim_dmar;
> + case TIM_OR:
> + return s->tim_or;
Guest error default needed?
Regards,
Peter
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH v1 5/5] Netduino: Add the Netduino Plus 2 Machine Model
2014-08-24 0:13 [Qemu-devel] [PATCH v1 0/5] Netduino Plus 2 Machine Model Alistair Francis
` (3 preceding siblings ...)
2014-08-24 0:14 ` [Qemu-devel] [PATCH v1 4/5] Netduino_Timer: Add the Netduino Plus 2 Timer2 to 5 Alistair Francis
@ 2014-08-24 0:14 ` Alistair Francis
2014-08-24 13:20 ` Peter Crosthwaite
2014-09-01 16:39 ` [Qemu-devel] [PATCH v1 0/5] " Peter Maydell
5 siblings, 1 reply; 22+ messages in thread
From: Alistair Francis @ 2014-08-24 0:14 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, peter.crosthwaite, konstanty
This patch adds the Netduion Plus 2 machine to QEMU.
Signed-off-by: Alistair Francis <alistair23@gmail.com>
---
hw/arm/Makefile.objs | 2 +-
hw/arm/netduinoplus2.c | 202 +++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 203 insertions(+), 1 deletion(-)
create mode 100644 hw/arm/netduinoplus2.c
diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index 6088e53..616f1ae 100644
--- a/hw/arm/Makefile.objs
+++ b/hw/arm/Makefile.objs
@@ -2,7 +2,7 @@ obj-y += boot.o collie.o exynos4_boards.o gumstix.o highbank.o
obj-$(CONFIG_DIGIC) += digic_boards.o
obj-y += integratorcp.o kzm.o mainstone.o musicpal.o nseries.o
obj-y += omap_sx1.o palm.o realview.o spitz.o stellaris.o
-obj-y += tosa.o versatilepb.o vexpress.o virt.o xilinx_zynq.o z2.o
+obj-y += tosa.o versatilepb.o vexpress.o virt.o xilinx_zynq.o z2.o netduinoplus2.o
obj-y += armv7m.o exynos4210.o pxa2xx.o pxa2xx_gpio.o pxa2xx_pic.o
obj-$(CONFIG_DIGIC) += digic.o
diff --git a/hw/arm/netduinoplus2.c b/hw/arm/netduinoplus2.c
new file mode 100644
index 0000000..32acac5
--- /dev/null
+++ b/hw/arm/netduinoplus2.c
@@ -0,0 +1,202 @@
+/*
+ * Netduino Plus 2 Machine Model
+ *
+ * Copyright (c) 2014 Alistair Francis <alistair@alistair23.me>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "hw/sysbus.h"
+#include "hw/ssi.h"
+#include "hw/arm/arm.h"
+#include "hw/devices.h"
+#include "qemu/timer.h"
+#include "net/net.h"
+#include "elf.h"
+#include "hw/loader.h"
+#include "hw/boards.h"
+#include "exec/address-spaces.h"
+#include "qemu/error-report.h"
+#include "sysemu/qtest.h"
+
+#define FLASH_BASE_ADDRESS 0x08000000
+#define FLASH_SIZE (1024 * 1024)
+#define SRAM_BASE_ADDRESS 0x20000000
+#define SRAM_SIZE (192 * 1024)
+
+typedef struct ARMV7MResetArgs {
+ ARMCPU *cpu;
+ uint32_t reset_sp;
+ uint32_t reset_pc;
+} ARMV7MResetArgs;
+
+static void armv7m_reset(void *opaque)
+{
+ ARMV7MResetArgs *args = opaque;
+
+ cpu_reset(CPU(args->cpu));
+
+ args->cpu->env.regs[13] = args->reset_sp & 0xFFFFFFFC;
+ args->cpu->env.thumb = args->reset_pc & 1;
+ args->cpu->env.regs[15] = args->reset_pc & ~1;
+}
+
+static void netduinoplus2_init(MachineState *machine)
+{
+ static const uint32_t gpio_addr[] = { 0x40020000, 0x40020400, 0x40020800,
+ 0x40020C00, 0x40021000, 0x40021400, 0x40021800, 0x40021C00,
+ 0x40022000, 0x40022400, 0x40022800 };
+ static const uint8_t gpio_letters[] = { 'a', 'b', 'c',
+ 'd', 'e', 'f', 'g', 'h',
+ 'i', 'j', 'k' };
+ static const uint32_t tim2_5_addr[] = { 0x40000000, 0x40000400,
+ 0x40000800, 0x40000C00 };
+ static const uint32_t usart_addr[] = { 0x40011000, 0x40004400,
+ 0x40004800, 0x40004C00, 0x40005000, 0x40011400, 0x40007800,
+ 0x40007C00 };
+ const char *kernel_filename = machine->kernel_filename;
+
+ static const int tim2_5_irq[] = {28, 29, 30, 50};
+ static const int usart_irq[] = {37, 38, 39, 52, 53, 71, 82, 83};
+
+ MemoryRegion *address_space_mem = get_system_memory();
+ MemoryRegion *sram = g_new(MemoryRegion, 1);
+ MemoryRegion *flash = g_new(MemoryRegion, 1);
+ MemoryRegion *flash_alias = g_new(MemoryRegion, 1);
+ MemoryRegion *hack = g_new(MemoryRegion, 1);
+ ARMV7MResetArgs reset_args;
+
+ qemu_irq pic[96];
+ ARMCPU *cpu;
+ CPUARMState *env;
+ DeviceState *nvic;
+ DeviceState *gpio;
+ SysBusDevice *busdev;
+
+ int image_size;
+ uint64_t entry;
+ uint64_t lowaddr;
+ int i;
+ int big_endian = 0;
+
+ /* The Netduinio Plus 2 uses a Cortex-M4, while QEMU currently supports
+ * the Cortex-M3, so that is being used instead
+ */
+ cpu = cpu_arm_init("cortex-m3");
+ env = &cpu->env;
+
+ memory_region_init_ram(flash, NULL, "netduino.flash", FLASH_SIZE);
+ memory_region_init_alias(flash_alias, NULL, "netduino.flash.alias",
+ flash, 0, FLASH_SIZE);
+
+ vmstate_register_ram_global(flash);
+
+ memory_region_set_readonly(flash, true);
+ memory_region_set_readonly(flash_alias, true);
+
+ memory_region_add_subregion(address_space_mem, FLASH_BASE_ADDRESS, flash);
+ memory_region_add_subregion(address_space_mem, 0, flash_alias);
+
+ memory_region_init_ram(sram, NULL, "netduino.sram", SRAM_SIZE);
+ vmstate_register_ram_global(sram);
+ memory_region_add_subregion(address_space_mem, SRAM_BASE_ADDRESS, sram);
+
+ nvic = qdev_create(NULL, "armv7m_nvic");
+ qdev_prop_set_uint32(nvic, "num-irq", 96);
+ env->nvic = nvic;
+ qdev_init_nofail(nvic);
+ sysbus_connect_irq(SYS_BUS_DEVICE(nvic), 0,
+ qdev_get_gpio_in(DEVICE(cpu), ARM_CPU_IRQ));
+ for (i = 0; i < 96; i++) {
+ pic[i] = qdev_get_gpio_in(nvic, i);
+ }
+
+ /* Load the kernel */
+ if (!kernel_filename && !qtest_enabled()) {
+ fprintf(stderr, "Guest image must be specified (using -kernel)\n");
+ exit(1);
+ }
+ if (kernel_filename) {
+ image_size = load_elf(kernel_filename, NULL, NULL, &entry, &lowaddr,
+ NULL, big_endian, ELF_MACHINE, 1);
+ if (image_size < 0) {
+ image_size = load_image_targphys(kernel_filename, 0, FLASH_SIZE);
+ lowaddr = 0;
+ }
+ if (image_size < 0) {
+ error_report("Could not load kernel '%s'", kernel_filename);
+ exit(1);
+ }
+ }
+
+ /* System configuration controller */
+ sysbus_create_simple("netduino_syscfg", 0x40013800,
+ pic[71]);
+
+ /* Attach a UART (uses USART registers) and USART controllers */
+ for (i = 0; i < 7; i++) {
+ sysbus_create_simple("netduino_usart", usart_addr[i],
+ pic[usart_irq[i]]);
+ }
+
+ /* Timer 2 to 5 */
+ for (i = 0; i < 4; i++) {
+ sysbus_create_simple("netduino_timer", tim2_5_addr[i],
+ pic[tim2_5_irq[i]]);
+ }
+
+ /* Attach GPIO devices */
+ for (i = 0; i < 11; i++) {
+ gpio = qdev_create(NULL, "netduino_gpio");
+ qdev_prop_set_uint8(gpio, "gpio-letter", gpio_letters[i]);
+ qdev_init_nofail(gpio);
+ busdev = SYS_BUS_DEVICE(gpio);
+ sysbus_mmio_map(busdev, 0, gpio_addr[i]);
+ }
+
+ /* Hack to map an additional page of ram at the top of the address
+ * space. This stops qemu complaining about executing code outside RAM
+ * when returning from an exception.
+ */
+ memory_region_init_ram(hack, NULL, "netduino.hack", 0x1000);
+ vmstate_register_ram_global(hack);
+ memory_region_set_readonly(hack, true);
+ memory_region_add_subregion(address_space_mem, 0xfffff000, hack);
+
+ reset_args = (ARMV7MResetArgs) {
+ .cpu = cpu,
+ .reset_pc = entry,
+ .reset_sp = (SRAM_BASE_ADDRESS + (SRAM_SIZE * 2)/3),
+ };
+ qemu_register_reset(armv7m_reset,
+ g_memdup(&reset_args, sizeof(reset_args)));
+}
+
+static QEMUMachine netduinoplus2_machine = {
+ .name = "netduinoplus2",
+ .desc = "Netduino Plus 2 Machine (Except with a Cortex-M3)",
+ .init = netduinoplus2_init,
+};
+
+static void netduinoplus2_machine_init(void)
+{
+ qemu_register_machine(&netduinoplus2_machine);
+}
+
+machine_init(netduinoplus2_machine_init);
--
1.9.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v1 5/5] Netduino: Add the Netduino Plus 2 Machine Model
2014-08-24 0:14 ` [Qemu-devel] [PATCH v1 5/5] Netduino: Add the Netduino Plus 2 Machine Model Alistair Francis
@ 2014-08-24 13:20 ` Peter Crosthwaite
2014-09-01 12:34 ` Alistair Francis
0 siblings, 1 reply; 22+ messages in thread
From: Peter Crosthwaite @ 2014-08-24 13:20 UTC (permalink / raw)
To: Alistair Francis
Cc: Peter Maydell, qemu-devel@nongnu.org Developers, Konstanty Bialkowski
On Sun, Aug 24, 2014 at 10:14 AM, Alistair Francis <alistair23@gmail.com> wrote:
> This patch adds the Netduion Plus 2 machine to QEMU.
>
> Signed-off-by: Alistair Francis <alistair23@gmail.com>
> ---
> hw/arm/Makefile.objs | 2 +-
> hw/arm/netduinoplus2.c | 202 +++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 203 insertions(+), 1 deletion(-)
> create mode 100644 hw/arm/netduinoplus2.c
>
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index 6088e53..616f1ae 100644
> --- a/hw/arm/Makefile.objs
> +++ b/hw/arm/Makefile.objs
> @@ -2,7 +2,7 @@ obj-y += boot.o collie.o exynos4_boards.o gumstix.o highbank.o
> obj-$(CONFIG_DIGIC) += digic_boards.o
> obj-y += integratorcp.o kzm.o mainstone.o musicpal.o nseries.o
> obj-y += omap_sx1.o palm.o realview.o spitz.o stellaris.o
> -obj-y += tosa.o versatilepb.o vexpress.o virt.o xilinx_zynq.o z2.o
> +obj-y += tosa.o versatilepb.o vexpress.o virt.o xilinx_zynq.o z2.o netduinoplus2.o
>
> obj-y += armv7m.o exynos4210.o pxa2xx.o pxa2xx_gpio.o pxa2xx_pic.o
> obj-$(CONFIG_DIGIC) += digic.o
> diff --git a/hw/arm/netduinoplus2.c b/hw/arm/netduinoplus2.c
> new file mode 100644
> index 0000000..32acac5
> --- /dev/null
> +++ b/hw/arm/netduinoplus2.c
> @@ -0,0 +1,202 @@
> +/*
> + * Netduino Plus 2 Machine Model
> + *
> + * Copyright (c) 2014 Alistair Francis <alistair@alistair23.me>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "hw/sysbus.h"
> +#include "hw/ssi.h"
> +#include "hw/arm/arm.h"
> +#include "hw/devices.h"
> +#include "qemu/timer.h"
> +#include "net/net.h"
> +#include "elf.h"
> +#include "hw/loader.h"
> +#include "hw/boards.h"
> +#include "exec/address-spaces.h"
> +#include "qemu/error-report.h"
> +#include "sysemu/qtest.h"
> +
> +#define FLASH_BASE_ADDRESS 0x08000000
> +#define FLASH_SIZE (1024 * 1024)
> +#define SRAM_BASE_ADDRESS 0x20000000
> +#define SRAM_SIZE (192 * 1024)
> +
> +typedef struct ARMV7MResetArgs {
> + ARMCPU *cpu;
> + uint32_t reset_sp;
> + uint32_t reset_pc;
> +} ARMV7MResetArgs;
> +
> +static void armv7m_reset(void *opaque)
> +{
> + ARMV7MResetArgs *args = opaque;
> +
> + cpu_reset(CPU(args->cpu));
> +
> + args->cpu->env.regs[13] = args->reset_sp & 0xFFFFFFFC;
> + args->cpu->env.thumb = args->reset_pc & 1;
> + args->cpu->env.regs[15] = args->reset_pc & ~1;
> +}
> +
> +static void netduinoplus2_init(MachineState *machine)
> +{
> + static const uint32_t gpio_addr[] = { 0x40020000, 0x40020400, 0x40020800,
> + 0x40020C00, 0x40021000, 0x40021400, 0x40021800, 0x40021C00,
> + 0x40022000, 0x40022400, 0x40022800 };
> + static const uint8_t gpio_letters[] = { 'a', 'b', 'c',
> + 'd', 'e', 'f', 'g', 'h',
> + 'i', 'j', 'k' };
> + static const uint32_t tim2_5_addr[] = { 0x40000000, 0x40000400,
> + 0x40000800, 0x40000C00 };
> + static const uint32_t usart_addr[] = { 0x40011000, 0x40004400,
> + 0x40004800, 0x40004C00, 0x40005000, 0x40011400, 0x40007800,
> + 0x40007C00 };
> + const char *kernel_filename = machine->kernel_filename;
> +
> + static const int tim2_5_irq[] = {28, 29, 30, 50};
> + static const int usart_irq[] = {37, 38, 39, 52, 53, 71, 82, 83};
> +
> + MemoryRegion *address_space_mem = get_system_memory();
system_memory. address_space is a miselading name for a MR.
> + MemoryRegion *sram = g_new(MemoryRegion, 1);
> + MemoryRegion *flash = g_new(MemoryRegion, 1);
> + MemoryRegion *flash_alias = g_new(MemoryRegion, 1);
> + MemoryRegion *hack = g_new(MemoryRegion, 1);
> + ARMV7MResetArgs reset_args;
> +
> + qemu_irq pic[96];
> + ARMCPU *cpu;
> + CPUARMState *env;
> + DeviceState *nvic;
> + DeviceState *gpio;
> + SysBusDevice *busdev;
> +
> + int image_size;
> + uint64_t entry;
> + uint64_t lowaddr;
> + int i;
> + int big_endian = 0;
> +
> + /* The Netduinio Plus 2 uses a Cortex-M4, while QEMU currently supports
> + * the Cortex-M3, so that is being used instead
> + */
> + cpu = cpu_arm_init("cortex-m3");
> + env = &cpu->env;
> +
> + memory_region_init_ram(flash, NULL, "netduino.flash", FLASH_SIZE);
> + memory_region_init_alias(flash_alias, NULL, "netduino.flash.alias",
> + flash, 0, FLASH_SIZE);
> +
> + vmstate_register_ram_global(flash);
> +
> + memory_region_set_readonly(flash, true);
> + memory_region_set_readonly(flash_alias, true);
> +
> + memory_region_add_subregion(address_space_mem, FLASH_BASE_ADDRESS, flash);
> + memory_region_add_subregion(address_space_mem, 0, flash_alias);
> +
> + memory_region_init_ram(sram, NULL, "netduino.sram", SRAM_SIZE);
> + vmstate_register_ram_global(sram);
> + memory_region_add_subregion(address_space_mem, SRAM_BASE_ADDRESS, sram);
> +
Most of this function is the same as armv7m_init(). This seems the be
the main difference - the SRAM and FLASH setup. Everything else is
board-level additive. The fact that it's different means that either
ARMv7M should be parameterized, or Stellaris specific setup should be
pushed up to stellaris.c to get armv7m_init cleaned up into a state
where it's usable for you.
Specifically, what in armv7m_init() is stopping the code share?
> + nvic = qdev_create(NULL, "armv7m_nvic");
> + qdev_prop_set_uint32(nvic, "num-irq", 96);
nvic num-irqs sounds config specific so that's one that should
probably be a parameter.
> + env->nvic = nvic;
> + qdev_init_nofail(nvic);
> + sysbus_connect_irq(SYS_BUS_DEVICE(nvic), 0,
> + qdev_get_gpio_in(DEVICE(cpu), ARM_CPU_IRQ));
> + for (i = 0; i < 96; i++) {
> + pic[i] = qdev_get_gpio_in(nvic, i);
> + }
> +
> + /* Load the kernel */
> + if (!kernel_filename && !qtest_enabled()) {
> + fprintf(stderr, "Guest image must be specified (using -kernel)\n");
> + exit(1);
> + }
> + if (kernel_filename) {
> + image_size = load_elf(kernel_filename, NULL, NULL, &entry, &lowaddr,
> + NULL, big_endian, ELF_MACHINE, 1);
> + if (image_size < 0) {
> + image_size = load_image_targphys(kernel_filename, 0, FLASH_SIZE);
> + lowaddr = 0;
> + }
> + if (image_size < 0) {
> + error_report("Could not load kernel '%s'", kernel_filename);
> + exit(1);
> + }
> + }
It seems strange to bootload before you have finished creating the
machine (although it should be functionally safe).
> +
> + /* System configuration controller */
> + sysbus_create_simple("netduino_syscfg", 0x40013800,
> + pic[71]);
> +
> + /* Attach a UART (uses USART registers) and USART controllers */
> + for (i = 0; i < 7; i++) {
> + sysbus_create_simple("netduino_usart", usart_addr[i],
> + pic[usart_irq[i]]);
> + }
> +
> + /* Timer 2 to 5 */
> + for (i = 0; i < 4; i++) {
> + sysbus_create_simple("netduino_timer", tim2_5_addr[i],
> + pic[tim2_5_irq[i]]);
> + }
> +
> + /* Attach GPIO devices */
> + for (i = 0; i < 11; i++) {
> + gpio = qdev_create(NULL, "netduino_gpio");
> + qdev_prop_set_uint8(gpio, "gpio-letter", gpio_letters[i]);
> + qdev_init_nofail(gpio);
> + busdev = SYS_BUS_DEVICE(gpio);
> + sysbus_mmio_map(busdev, 0, gpio_addr[i]);
> + }
In the code-shared approach, these devs are added after the armv7_init() call.
Regards,
Peter
> +
> + /* Hack to map an additional page of ram at the top of the address
> + * space. This stops qemu complaining about executing code outside RAM
> + * when returning from an exception.
> + */
> + memory_region_init_ram(hack, NULL, "netduino.hack", 0x1000);
> + vmstate_register_ram_global(hack);
> + memory_region_set_readonly(hack, true);
> + memory_region_add_subregion(address_space_mem, 0xfffff000, hack);
> +
> + reset_args = (ARMV7MResetArgs) {
> + .cpu = cpu,
> + .reset_pc = entry,
> + .reset_sp = (SRAM_BASE_ADDRESS + (SRAM_SIZE * 2)/3),
> + };
> + qemu_register_reset(armv7m_reset,
> + g_memdup(&reset_args, sizeof(reset_args)));
> +}
> +
> +static QEMUMachine netduinoplus2_machine = {
> + .name = "netduinoplus2",
> + .desc = "Netduino Plus 2 Machine (Except with a Cortex-M3)",
> + .init = netduinoplus2_init,
> +};
> +
> +static void netduinoplus2_machine_init(void)
> +{
> + qemu_register_machine(&netduinoplus2_machine);
> +}
> +
> +machine_init(netduinoplus2_machine_init);
> --
> 1.9.1
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v1 5/5] Netduino: Add the Netduino Plus 2 Machine Model
2014-08-24 13:20 ` Peter Crosthwaite
@ 2014-09-01 12:34 ` Alistair Francis
2014-09-01 12:44 ` Peter Maydell
0 siblings, 1 reply; 22+ messages in thread
From: Alistair Francis @ 2014-09-01 12:34 UTC (permalink / raw)
To: Peter Crosthwaite
Cc: Peter Maydell, qemu-devel@nongnu.org Developers, Konstanty Bialkowski
On Sun, Aug 24, 2014 at 11:20 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Sun, Aug 24, 2014 at 10:14 AM, Alistair Francis <alistair23@gmail.com> wrote:
>> This patch adds the Netduion Plus 2 machine to QEMU.
>>
>> Signed-off-by: Alistair Francis <alistair23@gmail.com>
>> ---
>> hw/arm/Makefile.objs | 2 +-
>> hw/arm/netduinoplus2.c | 202 +++++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 203 insertions(+), 1 deletion(-)
>> create mode 100644 hw/arm/netduinoplus2.c
>>
>> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
>> index 6088e53..616f1ae 100644
>> --- a/hw/arm/Makefile.objs
>> +++ b/hw/arm/Makefile.objs
>> @@ -2,7 +2,7 @@ obj-y += boot.o collie.o exynos4_boards.o gumstix.o highbank.o
>> obj-$(CONFIG_DIGIC) += digic_boards.o
>> obj-y += integratorcp.o kzm.o mainstone.o musicpal.o nseries.o
>> obj-y += omap_sx1.o palm.o realview.o spitz.o stellaris.o
>> -obj-y += tosa.o versatilepb.o vexpress.o virt.o xilinx_zynq.o z2.o
>> +obj-y += tosa.o versatilepb.o vexpress.o virt.o xilinx_zynq.o z2.o netduinoplus2.o
>>
>> obj-y += armv7m.o exynos4210.o pxa2xx.o pxa2xx_gpio.o pxa2xx_pic.o
>> obj-$(CONFIG_DIGIC) += digic.o
>> diff --git a/hw/arm/netduinoplus2.c b/hw/arm/netduinoplus2.c
>> new file mode 100644
>> index 0000000..32acac5
>> --- /dev/null
>> +++ b/hw/arm/netduinoplus2.c
>> @@ -0,0 +1,202 @@
>> +/*
>> + * Netduino Plus 2 Machine Model
>> + *
>> + * Copyright (c) 2014 Alistair Francis <alistair@alistair23.me>
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a copy
>> + * of this software and associated documentation files (the "Software"), to deal
>> + * in the Software without restriction, including without limitation the rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> + * THE SOFTWARE.
>> + */
>> +
>> +#include "hw/sysbus.h"
>> +#include "hw/ssi.h"
>> +#include "hw/arm/arm.h"
>> +#include "hw/devices.h"
>> +#include "qemu/timer.h"
>> +#include "net/net.h"
>> +#include "elf.h"
>> +#include "hw/loader.h"
>> +#include "hw/boards.h"
>> +#include "exec/address-spaces.h"
>> +#include "qemu/error-report.h"
>> +#include "sysemu/qtest.h"
>> +
>> +#define FLASH_BASE_ADDRESS 0x08000000
>> +#define FLASH_SIZE (1024 * 1024)
>> +#define SRAM_BASE_ADDRESS 0x20000000
>> +#define SRAM_SIZE (192 * 1024)
>> +
>> +typedef struct ARMV7MResetArgs {
>> + ARMCPU *cpu;
>> + uint32_t reset_sp;
>> + uint32_t reset_pc;
>> +} ARMV7MResetArgs;
>> +
>> +static void armv7m_reset(void *opaque)
>> +{
>> + ARMV7MResetArgs *args = opaque;
>> +
>> + cpu_reset(CPU(args->cpu));
>> +
>> + args->cpu->env.regs[13] = args->reset_sp & 0xFFFFFFFC;
>> + args->cpu->env.thumb = args->reset_pc & 1;
>> + args->cpu->env.regs[15] = args->reset_pc & ~1;
>> +}
>> +
>> +static void netduinoplus2_init(MachineState *machine)
>> +{
>> + static const uint32_t gpio_addr[] = { 0x40020000, 0x40020400, 0x40020800,
>> + 0x40020C00, 0x40021000, 0x40021400, 0x40021800, 0x40021C00,
>> + 0x40022000, 0x40022400, 0x40022800 };
>> + static const uint8_t gpio_letters[] = { 'a', 'b', 'c',
>> + 'd', 'e', 'f', 'g', 'h',
>> + 'i', 'j', 'k' };
>> + static const uint32_t tim2_5_addr[] = { 0x40000000, 0x40000400,
>> + 0x40000800, 0x40000C00 };
>> + static const uint32_t usart_addr[] = { 0x40011000, 0x40004400,
>> + 0x40004800, 0x40004C00, 0x40005000, 0x40011400, 0x40007800,
>> + 0x40007C00 };
>> + const char *kernel_filename = machine->kernel_filename;
>> +
>> + static const int tim2_5_irq[] = {28, 29, 30, 50};
>> + static const int usart_irq[] = {37, 38, 39, 52, 53, 71, 82, 83};
>> +
>> + MemoryRegion *address_space_mem = get_system_memory();
>
> system_memory. address_space is a miselading name for a MR.
>
>> + MemoryRegion *sram = g_new(MemoryRegion, 1);
>> + MemoryRegion *flash = g_new(MemoryRegion, 1);
>> + MemoryRegion *flash_alias = g_new(MemoryRegion, 1);
>> + MemoryRegion *hack = g_new(MemoryRegion, 1);
>> + ARMV7MResetArgs reset_args;
>> +
>> + qemu_irq pic[96];
>> + ARMCPU *cpu;
>> + CPUARMState *env;
>> + DeviceState *nvic;
>> + DeviceState *gpio;
>> + SysBusDevice *busdev;
>> +
>> + int image_size;
>> + uint64_t entry;
>> + uint64_t lowaddr;
>> + int i;
>> + int big_endian = 0;
>> +
>> + /* The Netduinio Plus 2 uses a Cortex-M4, while QEMU currently supports
>> + * the Cortex-M3, so that is being used instead
>> + */
>> + cpu = cpu_arm_init("cortex-m3");
>> + env = &cpu->env;
>> +
>> + memory_region_init_ram(flash, NULL, "netduino.flash", FLASH_SIZE);
>> + memory_region_init_alias(flash_alias, NULL, "netduino.flash.alias",
>> + flash, 0, FLASH_SIZE);
>> +
>> + vmstate_register_ram_global(flash);
>> +
>> + memory_region_set_readonly(flash, true);
>> + memory_region_set_readonly(flash_alias, true);
>> +
>> + memory_region_add_subregion(address_space_mem, FLASH_BASE_ADDRESS, flash);
>> + memory_region_add_subregion(address_space_mem, 0, flash_alias);
>> +
>> + memory_region_init_ram(sram, NULL, "netduino.sram", SRAM_SIZE);
>> + vmstate_register_ram_global(sram);
>> + memory_region_add_subregion(address_space_mem, SRAM_BASE_ADDRESS, sram);
>> +
>
>
> Most of this function is the same as armv7m_init(). This seems the be
> the main difference - the SRAM and FLASH setup. Everything else is
> board-level additive. The fact that it's different means that either
> ARMv7M should be parameterized, or Stellaris specific setup should be
> pushed up to stellaris.c to get armv7m_init cleaned up into a state
> where it's usable for you.
>
> Specifically, what in armv7m_init() is stopping the code share?
>
It is generally similar, although there are enough differences that I don't
think it's wroth it. Ideally one day the Netduino Plus 2 will use a
Cortex-M4 (one can hope) and this will need to be parameterised.
The Netduino also has different base addresses
which would need to be parameterised into armv7m_init(). The Netduio
then has the memory alias as well as a different number of interrupt lines.
It is because of all of these differences that I don't think it is worth using
the armv7m_init() function. It's really not that much code.
It could be taken out of the Stellaris boards, but that is somebody
else's decision.
Thanks,
Alistair
>> + nvic = qdev_create(NULL, "armv7m_nvic");
>> + qdev_prop_set_uint32(nvic, "num-irq", 96);
>
> nvic num-irqs sounds config specific so that's one that should
> probably be a parameter.
>
>> + env->nvic = nvic;
>> + qdev_init_nofail(nvic);
>> + sysbus_connect_irq(SYS_BUS_DEVICE(nvic), 0,
>> + qdev_get_gpio_in(DEVICE(cpu), ARM_CPU_IRQ));
>> + for (i = 0; i < 96; i++) {
>> + pic[i] = qdev_get_gpio_in(nvic, i);
>> + }
>> +
>> + /* Load the kernel */
>> + if (!kernel_filename && !qtest_enabled()) {
>> + fprintf(stderr, "Guest image must be specified (using -kernel)\n");
>> + exit(1);
>> + }
>> + if (kernel_filename) {
>> + image_size = load_elf(kernel_filename, NULL, NULL, &entry, &lowaddr,
>> + NULL, big_endian, ELF_MACHINE, 1);
>> + if (image_size < 0) {
>> + image_size = load_image_targphys(kernel_filename, 0, FLASH_SIZE);
>> + lowaddr = 0;
>> + }
>> + if (image_size < 0) {
>> + error_report("Could not load kernel '%s'", kernel_filename);
>> + exit(1);
>> + }
>> + }
>
> It seems strange to bootload before you have finished creating the
> machine (although it should be functionally safe).
>
>> +
>> + /* System configuration controller */
>> + sysbus_create_simple("netduino_syscfg", 0x40013800,
>> + pic[71]);
>> +
>> + /* Attach a UART (uses USART registers) and USART controllers */
>> + for (i = 0; i < 7; i++) {
>> + sysbus_create_simple("netduino_usart", usart_addr[i],
>> + pic[usart_irq[i]]);
>> + }
>> +
>> + /* Timer 2 to 5 */
>> + for (i = 0; i < 4; i++) {
>> + sysbus_create_simple("netduino_timer", tim2_5_addr[i],
>> + pic[tim2_5_irq[i]]);
>> + }
>> +
>> + /* Attach GPIO devices */
>> + for (i = 0; i < 11; i++) {
>> + gpio = qdev_create(NULL, "netduino_gpio");
>> + qdev_prop_set_uint8(gpio, "gpio-letter", gpio_letters[i]);
>> + qdev_init_nofail(gpio);
>> + busdev = SYS_BUS_DEVICE(gpio);
>> + sysbus_mmio_map(busdev, 0, gpio_addr[i]);
>> + }
>
> In the code-shared approach, these devs are added after the armv7_init() call.
>
> Regards,
> Peter
>
>> +
>> + /* Hack to map an additional page of ram at the top of the address
>> + * space. This stops qemu complaining about executing code outside RAM
>> + * when returning from an exception.
>> + */
>> + memory_region_init_ram(hack, NULL, "netduino.hack", 0x1000);
>> + vmstate_register_ram_global(hack);
>> + memory_region_set_readonly(hack, true);
>> + memory_region_add_subregion(address_space_mem, 0xfffff000, hack);
>> +
>> + reset_args = (ARMV7MResetArgs) {
>> + .cpu = cpu,
>> + .reset_pc = entry,
>> + .reset_sp = (SRAM_BASE_ADDRESS + (SRAM_SIZE * 2)/3),
>> + };
>> + qemu_register_reset(armv7m_reset,
>> + g_memdup(&reset_args, sizeof(reset_args)));
>> +}
>> +
>> +static QEMUMachine netduinoplus2_machine = {
>> + .name = "netduinoplus2",
>> + .desc = "Netduino Plus 2 Machine (Except with a Cortex-M3)",
>> + .init = netduinoplus2_init,
>> +};
>> +
>> +static void netduinoplus2_machine_init(void)
>> +{
>> + qemu_register_machine(&netduinoplus2_machine);
>> +}
>> +
>> +machine_init(netduinoplus2_machine_init);
>> --
>> 1.9.1
>>
>>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v1 5/5] Netduino: Add the Netduino Plus 2 Machine Model
2014-09-01 12:34 ` Alistair Francis
@ 2014-09-01 12:44 ` Peter Maydell
2014-09-01 16:33 ` Peter Maydell
2014-09-02 2:02 ` Alistair Francis
0 siblings, 2 replies; 22+ messages in thread
From: Peter Maydell @ 2014-09-01 12:44 UTC (permalink / raw)
To: Alistair Francis
Cc: Peter Crosthwaite, qemu-devel@nongnu.org Developers,
Konstanty Bialkowski
On 1 September 2014 13:34, Alistair Francis <alistair23@gmail.com> wrote:
> On Sun, Aug 24, 2014 at 11:20 PM, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> Most of this function is the same as armv7m_init(). This seems the be
>> the main difference - the SRAM and FLASH setup. Everything else is
>> board-level additive. The fact that it's different means that either
>> ARMv7M should be parameterized, or Stellaris specific setup should be
>> pushed up to stellaris.c to get armv7m_init cleaned up into a state
>> where it's usable for you.
>>
>> Specifically, what in armv7m_init() is stopping the code share?
> It is generally similar, although there are enough differences that I don't
> think it's wroth it. Ideally one day the Netduino Plus 2 will use a
> Cortex-M4 (one can hope) and this will need to be parameterised.
> The Netduino also has different base addresses
> which would need to be parameterised into armv7m_init(). The Netduio
> then has the memory alias as well as a different number of interrupt lines.
>
> It is because of all of these differences that I don't think it is worth using
> the armv7m_init() function. It's really not that much code.
Yeah, but it's not really the right design IMHO. We should
have common armv7m init code for the parts which are
really "the CPU has all this" (including at least the bitbanding,
NVIC, memory mapped system registers, reset/ELF file
loading, etc), and board specific code in the board files.
Copying this code just because it's easier in the short
term leaves us still with the cleanup to do in the long term.
You don't want to do this by parameterising flash/RAM
base addresses, incidentally -- you want to use memory
regions so the board sets up its devices in the right
places in system memory and then hands that off to
the v7M/SoC code level which adds the inside-the-CPU
devices.
(There's almost certainly scope for QOMification in the
process of separating out the generic v7m from the board
code here too.)
thanks
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v1 5/5] Netduino: Add the Netduino Plus 2 Machine Model
2014-09-01 12:44 ` Peter Maydell
@ 2014-09-01 16:33 ` Peter Maydell
2014-09-02 2:02 ` Alistair Francis
1 sibling, 0 replies; 22+ messages in thread
From: Peter Maydell @ 2014-09-01 16:33 UTC (permalink / raw)
To: Alistair Francis
Cc: Peter Crosthwaite, qemu-devel@nongnu.org Developers,
Konstanty Bialkowski
On 1 September 2014 13:44, Peter Maydell <peter.maydell@linaro.org> wrote:
> Yeah, but it's not really the right design IMHO. We should
> have common armv7m init code for the parts which are
> really "the CPU has all this" (including at least the bitbanding,
> NVIC, memory mapped system registers, reset/ELF file
> loading, etc), and board specific code in the board files.
> Copying this code just because it's easier in the short
> term leaves us still with the cleanup to do in the long term.
In fact, looking at the data sheets and the other patches in
this series there's another level or two here too:
* init common to all ARMv7M
* (maybe) init common to Cortex-M3
* init for this STM SoC
* init for the board
thanks
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v1 5/5] Netduino: Add the Netduino Plus 2 Machine Model
2014-09-01 12:44 ` Peter Maydell
2014-09-01 16:33 ` Peter Maydell
@ 2014-09-02 2:02 ` Alistair Francis
1 sibling, 0 replies; 22+ messages in thread
From: Alistair Francis @ 2014-09-02 2:02 UTC (permalink / raw)
To: Peter Maydell
Cc: Peter Crosthwaite, qemu-devel@nongnu.org Developers,
Konstanty Bialkowski
On Mon, Sep 1, 2014 at 10:44 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 1 September 2014 13:34, Alistair Francis <alistair23@gmail.com> wrote:
>> On Sun, Aug 24, 2014 at 11:20 PM, Peter Crosthwaite
>> <peter.crosthwaite@xilinx.com> wrote:
>>> Most of this function is the same as armv7m_init(). This seems the be
>>> the main difference - the SRAM and FLASH setup. Everything else is
>>> board-level additive. The fact that it's different means that either
>>> ARMv7M should be parameterized, or Stellaris specific setup should be
>>> pushed up to stellaris.c to get armv7m_init cleaned up into a state
>>> where it's usable for you.
>>>
>>> Specifically, what in armv7m_init() is stopping the code share?
>
>> It is generally similar, although there are enough differences that I don't
>> think it's wroth it. Ideally one day the Netduino Plus 2 will use a
>> Cortex-M4 (one can hope) and this will need to be parameterised.
>> The Netduino also has different base addresses
>> which would need to be parameterised into armv7m_init(). The Netduio
>> then has the memory alias as well as a different number of interrupt lines.
>>
>> It is because of all of these differences that I don't think it is worth using
>> the armv7m_init() function. It's really not that much code.
>
> Yeah, but it's not really the right design IMHO. We should
> have common armv7m init code for the parts which are
> really "the CPU has all this" (including at least the bitbanding,
> NVIC, memory mapped system registers, reset/ELF file
> loading, etc), and board specific code in the board files.
> Copying this code just because it's easier in the short
> term leaves us still with the cleanup to do in the long term.
>
> You don't want to do this by parameterising flash/RAM
> base addresses, incidentally -- you want to use memory
> regions so the board sets up its devices in the right
> places in system memory and then hands that off to
> the v7M/SoC code level which adds the inside-the-CPU
> devices.
I'm confused about exactly what you mean. Do you mean take
the memory initialisation out of the armv7m_init() function and let
the board do that? Then the init function just does CPU, NVIC, ELF
loading, bitbanding and the reset.
Then I guess there could be a STM32F405 SoC device, similar
to the A9 MPCore patch that I sent a while ago:
https://lists.nongnu.org/archive/html/qemu-devel/2014-06/msg03614.html
Thanks,
Alistair
>
> (There's almost certainly scope for QOMification in the
> process of separating out the generic v7m from the board
> code here too.)
>
> thanks
> -- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v1 0/5] Netduino Plus 2 Machine Model
2014-08-24 0:13 [Qemu-devel] [PATCH v1 0/5] Netduino Plus 2 Machine Model Alistair Francis
` (4 preceding siblings ...)
2014-08-24 0:14 ` [Qemu-devel] [PATCH v1 5/5] Netduino: Add the Netduino Plus 2 Machine Model Alistair Francis
@ 2014-09-01 16:39 ` Peter Maydell
2014-09-02 1:55 ` Alistair Francis
5 siblings, 1 reply; 22+ messages in thread
From: Peter Maydell @ 2014-09-01 16:39 UTC (permalink / raw)
To: Alistair Francis; +Cc: Peter Crosthwaite, QEMU Developers, Konstanty Bialkowski
On 24 August 2014 01:13, Alistair Francis <alistair23@gmail.com> wrote:
> The Netduino Plus 2 has a Cortex-M4 in it, while this model uses
> a Cortex-M3 as that is supported by QEMU. This means that the code
> that runs on the Netduino Plus 2 is recompiled for a Cortex-M3 with
> out Floating Point or DSP optimisations.
>
> For my use of this it doesn't matter if the code has to be recompiled,
> so I have no desire to add support for the Cortex-M4.
That does make things a bit awkward. I don't really want to
add something that claims to be a model of board X if in
fact it won't run anything that isn't specifically built for
"QEMU's deficient model of board X". It seems to me that
would just be a recipe for lots of user complaints...
thanks
-- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v1 0/5] Netduino Plus 2 Machine Model
2014-09-01 16:39 ` [Qemu-devel] [PATCH v1 0/5] " Peter Maydell
@ 2014-09-02 1:55 ` Alistair Francis
0 siblings, 0 replies; 22+ messages in thread
From: Alistair Francis @ 2014-09-02 1:55 UTC (permalink / raw)
To: Peter Maydell; +Cc: Peter Crosthwaite, QEMU Developers, Konstanty Bialkowski
On Tue, Sep 2, 2014 at 2:39 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 24 August 2014 01:13, Alistair Francis <alistair23@gmail.com> wrote:
>> The Netduino Plus 2 has a Cortex-M4 in it, while this model uses
>> a Cortex-M3 as that is supported by QEMU. This means that the code
>> that runs on the Netduino Plus 2 is recompiled for a Cortex-M3 with
>> out Floating Point or DSP optimisations.
>>
>> For my use of this it doesn't matter if the code has to be recompiled,
>> so I have no desire to add support for the Cortex-M4.
>
> That does make things a bit awkward. I don't really want to
> add something that claims to be a model of board X if in
> fact it won't run anything that isn't specifically built for
> "QEMU's deficient model of board X". It seems to me that
> would just be a recipe for lots of user complaints...
So the only way you would take this machine is with Cortex-M4 support?
I agree it is a bit awkward, but at the moment it is the best I have. I am going
to look into the Netduino 2 as well, as that uses a Cortex-M3, but I'm not sure
how similar everything else is.
It's my understanding that the FPU in the Cortex-M4 is optional (as in not
every M4 has it). Does this mean that the only differences between a M3 and M4
in QEMU would be registers and instructions for DSP?
Thanks,
Alistair
>
> thanks
> -- PMM
^ permalink raw reply [flat|nested] 22+ messages in thread