* [Qemu-devel] [PATCH 1/7] hw/misc/nrf51_rng: Add NRF51 random number generator peripheral
2018-08-06 10:01 [Qemu-devel] [PATCH 0/7] arm: nRF51 Devices and Microbit Support Steffen Görtz
@ 2018-08-06 10:01 ` Steffen Görtz
2018-08-06 14:00 ` Stefan Hajnoczi
2018-08-16 15:45 ` Peter Maydell
2018-08-06 10:01 ` [Qemu-devel] [PATCH 2/7] hw/nvram/nrf51_nvm: Add nRF51 non-volatile memories Steffen Görtz
` (6 subsequent siblings)
7 siblings, 2 replies; 25+ messages in thread
From: Steffen Görtz @ 2018-08-06 10:01 UTC (permalink / raw)
To: qemu-devel
Cc: Stefan Hajnoczi, Joel Stanley, Jim Mussared, Julia Suvorova,
Peter Maydell, Thomas Huth, Steffen Görtz
Add a model of the NRF51 random number generator peripheral.
This is a simple random generator that continuously generates
new random values after startup.
Reference Manual: http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.pdf
Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
---
hw/misc/Makefile.objs | 1 +
hw/misc/nrf51_rng.c | 273 ++++++++++++++++++++++++++++++++++++
include/hw/misc/nrf51_rng.h | 71 ++++++++++
3 files changed, 345 insertions(+)
create mode 100644 hw/misc/nrf51_rng.c
create mode 100644 include/hw/misc/nrf51_rng.h
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 9350900845..fc3fe57e13 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -72,3 +72,4 @@ obj-$(CONFIG_AUX) += auxbus.o
obj-$(CONFIG_ASPEED_SOC) += aspeed_scu.o aspeed_sdmc.o
obj-y += mmio_interface.o
obj-$(CONFIG_MSF2) += msf2-sysreg.o
+obj-$(CONFIG_NRF51_SOC) += nrf51_rng.o
diff --git a/hw/misc/nrf51_rng.c b/hw/misc/nrf51_rng.c
new file mode 100644
index 0000000000..7dad5d0cc4
--- /dev/null
+++ b/hw/misc/nrf51_rng.c
@@ -0,0 +1,273 @@
+/*
+ * nRF51 Random Number Generator
+ *
+ * Reference Manual: http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.pdf
+ *
+ * Copyright 2018 Steffen Görtz <contrib@steffen-goertz.de>
+ *
+ * This code is licensed under the GPL version 2 or later. See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qapi/error.h"
+#include "hw/misc/nrf51_rng.h"
+#include "crypto/random.h"
+
+#define NRF51_RNG_SIZE 0x1000
+
+#define NRF51_RNG_TASK_START 0x000
+#define NRF51_RNG_TASK_STOP 0x004
+#define NRF51_RNG_EVENT_VALRDY 0x100
+#define NRF51_RNG_REG_SHORTS 0x200
+#define NRF51_RNG_REG_SHORTS_VALRDY_STOP 0
+#define NRF51_RNG_REG_INTEN 0x300
+#define NRF51_RNG_REG_INTEN_VALRDY 0
+#define NRF51_RNG_REG_INTENSET 0x304
+#define NRF51_RNG_REG_INTENCLR 0x308
+#define NRF51_RNG_REG_CONFIG 0x504
+#define NRF51_RNG_REG_CONFIG_DECEN 0
+#define NRF51_RNG_REG_VALUE 0x508
+
+#define NRF51_TRIGGER_TASK 0x01
+#define NRF51_EVENT_CLEAR 0x00
+
+static void update_irq(Nrf51RNGState *s)
+{
+ bool irq = false;
+ irq |= s->interrupt_enabled && s->event_valrdy;
+ qemu_set_irq(s->irq, irq);
+}
+
+static uint64_t rng_read(void *opaque, hwaddr offset, unsigned int size)
+{
+ Nrf51RNGState *s = NRF51_RNG(opaque);
+ uint64_t r = 0;
+
+ switch (offset) {
+ case NRF51_RNG_EVENT_VALRDY:
+ r = s->event_valrdy;
+ break;
+ case NRF51_RNG_REG_SHORTS:
+ r = s->shortcut_stop_on_valrdy;
+ break;
+ case NRF51_RNG_REG_INTEN:
+ case NRF51_RNG_REG_INTENSET:
+ case NRF51_RNG_REG_INTENCLR:
+ r = s->interrupt_enabled;
+ break;
+ case NRF51_RNG_REG_CONFIG:
+ r = s->filter_enabled;
+ break;
+ case NRF51_RNG_REG_VALUE:
+ r = s->value;
+ break;
+
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: bad read offset 0x%" HWADDR_PRIx "\n",
+ __func__, offset);
+ }
+
+ return r;
+}
+
+static int64_t calc_next_timeout(Nrf51RNGState *s)
+{
+ int64_t timeout = qemu_clock_get_us(QEMU_CLOCK_VIRTUAL);
+ if (s->filter_enabled) {
+ timeout += s->period_filtered_us;
+ } else {
+ timeout += s->period_unfiltered_us;
+ }
+
+ return timeout;
+}
+
+
+static void rng_update_timer(Nrf51RNGState *s)
+{
+ if (s->active) {
+ timer_mod(&s->timer, calc_next_timeout(s));
+ } else {
+ timer_del(&s->timer);
+ }
+}
+
+
+static void rng_write(void *opaque, hwaddr offset,
+ uint64_t value, unsigned int size)
+{
+ Nrf51RNGState *s = NRF51_RNG(opaque);
+
+ switch (offset) {
+ case NRF51_RNG_TASK_START:
+ if (value == NRF51_TRIGGER_TASK) {
+ s->active = 1;
+ rng_update_timer(s);
+ }
+ break;
+ case NRF51_RNG_TASK_STOP:
+ if (value == NRF51_TRIGGER_TASK) {
+ s->active = 0;
+ rng_update_timer(s);
+ }
+ break;
+ case NRF51_RNG_EVENT_VALRDY:
+ if (value == NRF51_EVENT_CLEAR) {
+ s->event_valrdy = 0;
+ }
+ break;
+ case NRF51_RNG_REG_SHORTS:
+ s->shortcut_stop_on_valrdy =
+ (value & BIT_MASK(NRF51_RNG_REG_SHORTS_VALRDY_STOP)) ? 1 : 0;
+ break;
+ case NRF51_RNG_REG_INTEN:
+ s->interrupt_enabled =
+ (value & BIT_MASK(NRF51_RNG_REG_INTEN_VALRDY)) ? 1 : 0;
+ break;
+ case NRF51_RNG_REG_INTENSET:
+ if (value & BIT_MASK(NRF51_RNG_REG_INTEN_VALRDY)) {
+ s->interrupt_enabled = 1;
+ }
+ break;
+ case NRF51_RNG_REG_INTENCLR:
+ if (value & BIT_MASK(NRF51_RNG_REG_INTEN_VALRDY)) {
+ s->interrupt_enabled = 0;
+ }
+ break;
+ case NRF51_RNG_REG_CONFIG:
+ s->filter_enabled =
+ (value & BIT_MASK(NRF51_RNG_REG_CONFIG_DECEN)) ? 1 : 0;
+ break;
+
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: bad write offset 0x%" HWADDR_PRIx "\n",
+ __func__, offset);
+ }
+
+ update_irq(s);
+}
+
+static const MemoryRegionOps rng_ops = {
+ .read = rng_read,
+ .write = rng_write,
+ .endianness = DEVICE_LITTLE_ENDIAN,
+ .impl.min_access_size = 4,
+ .impl.max_access_size = 4
+};
+
+static void nrf51_rng_timer_expire(void *opaque)
+{
+ Nrf51RNGState *s = NRF51_RNG(opaque);
+
+ qcrypto_random_bytes(&s->value, 1, &error_abort);
+
+ s->event_valrdy = 1;
+ qemu_set_irq(s->eep_valrdy, 1);
+
+ if (s->shortcut_stop_on_valrdy) {
+ s->active = 0;
+ }
+
+ rng_update_timer(s);
+ update_irq(s);
+}
+
+static void nrf51_rng_tep_start(void *opaque, int n, int level)
+{
+ Nrf51RNGState *s = NRF51_RNG(opaque);
+
+ if (level) {
+ s->active = 1;
+ rng_update_timer(s);
+ }
+}
+
+static void nrf51_rng_tep_stop(void *opaque, int n, int level)
+{
+ Nrf51RNGState *s = NRF51_RNG(opaque);
+
+ if (level) {
+ s->active = 0;
+ rng_update_timer(s);
+ }
+}
+
+
+static void nrf51_rng_init(Object *obj)
+{
+ Nrf51RNGState *s = NRF51_RNG(obj);
+ SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+
+ memory_region_init_io(&s->mmio, obj, &rng_ops, s,
+ TYPE_NRF51_RNG, NRF51_RNG_SIZE);
+ sysbus_init_mmio(sbd, &s->mmio);
+
+ timer_init_us(&s->timer, QEMU_CLOCK_VIRTUAL, nrf51_rng_timer_expire, s);
+
+ qdev_init_gpio_out_named(DEVICE(s), &s->irq, "irq", 1);
+
+ /* Tasks */
+ qdev_init_gpio_in_named(DEVICE(s), nrf51_rng_tep_start, "tep_start", 1);
+ qdev_init_gpio_in_named(DEVICE(s), nrf51_rng_tep_stop, "tep_stop", 1);
+
+ /* Events */
+ qdev_init_gpio_out_named(DEVICE(s), &s->eep_valrdy, "eep_valrdy", 1);
+}
+
+static void nrf51_rng_reset(DeviceState *dev)
+{
+ Nrf51RNGState *s = NRF51_RNG(dev);
+
+ rng_update_timer(s);
+}
+
+
+static Property nrf51_rng_properties[] = {
+ DEFINE_PROP_UINT16("period_unfiltered_us", Nrf51RNGState,
+ period_unfiltered_us, 167),
+ DEFINE_PROP_UINT16("period_filtered_us", Nrf51RNGState,
+ period_filtered_us, 660),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static const VMStateDescription vmstate_rng = {
+ .name = "nrf51_soc.rng",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(active, Nrf51RNGState),
+ VMSTATE_UINT32(event_valrdy, Nrf51RNGState),
+ VMSTATE_UINT32(shortcut_stop_on_valrdy, Nrf51RNGState),
+ VMSTATE_UINT32(interrupt_enabled, Nrf51RNGState),
+ VMSTATE_UINT32(filter_enabled, Nrf51RNGState),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static void nrf51_rng_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->props = nrf51_rng_properties;
+ dc->vmsd = &vmstate_rng;
+ dc->reset = nrf51_rng_reset;
+}
+
+static const TypeInfo nrf51_rng_info = {
+ .name = TYPE_NRF51_RNG,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(Nrf51RNGState),
+ .instance_init = nrf51_rng_init,
+ .class_init = nrf51_rng_class_init
+};
+
+static void nrf51_rng_register_types(void)
+{
+ type_register_static(&nrf51_rng_info);
+}
+
+type_init(nrf51_rng_register_types)
diff --git a/include/hw/misc/nrf51_rng.h b/include/hw/misc/nrf51_rng.h
new file mode 100644
index 0000000000..b0a3b93d67
--- /dev/null
+++ b/include/hw/misc/nrf51_rng.h
@@ -0,0 +1,71 @@
+/*
+ * nRF51 Random Number Generator
+ *
+ * Copyright 2018 Steffen Görtz <contrib@steffen-goertz.de>
+ *
+ * This code is licensed under the GPL version 2 or later. See
+ * the COPYING file in the top-level directory.
+ *
+ * See NRF51 reference manual section 21 Random Number Generator
+ * See NRF51 product specification section 8.16 Random Number Generator
+ *
+ * QEMU interface:
+ * + Property "period_unfiltered_us": Time between two biased values in
+ * microseconds.
+ * + Property "period_filtered_us": Time between two unbiased values in
+ * microseconds.
+ * + sysbus MMIO regions 0: Memory Region with tasks, events and registers
+ * to be mapped to the peripherals instance address by the SOC.
+ * + Named GPIO output "irq": Interrupt line of the peripheral. Must be
+ * connected to the associated peripheral interrupt line of the NVIC.
+ * + Named GPIO output "eep_valrdy": Event set when new random value is ready
+ * to be read.
+ * + Named GPIO input "tep_start": Task that triggers start of continuous
+ * generation of random values.
+ * + Named GPIO input "tep_stop": Task that ends continuous generation of
+ * random values.
+ *
+ * Accuracy of the peripheral model:
+ * + Stochastic properties of different configurations of the random source
+ * are not modeled.
+ * + Generation of unfiltered and filtered random values take at least the
+ * average generation time stated in the production specification;
+ * non-deterministic generation times are not modeled.
+ *
+ */
+#ifndef NRF51_RNG_H
+#define NRF51_RNG_H
+
+#include "hw/sysbus.h"
+#include "qemu/timer.h"
+#define TYPE_NRF51_RNG "nrf51_soc.rng"
+#define NRF51_RNG(obj) OBJECT_CHECK(Nrf51RNGState, (obj), TYPE_NRF51_RNG)
+
+typedef struct Nrf51RNGState {
+ SysBusDevice parent_obj;
+
+ MemoryRegion mmio;
+ qemu_irq irq;
+
+ /* Event End Points */
+ qemu_irq eep_valrdy;
+
+ QEMUTimer timer;
+
+ /* Time between generation of successive unfiltered values in us */
+ uint16_t period_unfiltered_us;
+ /* Time between generation of successive filtered values in us */
+ uint16_t period_filtered_us;
+
+ uint8_t value;
+
+ uint32_t active;
+ uint32_t event_valrdy;
+ uint32_t shortcut_stop_on_valrdy;
+ uint32_t interrupt_enabled;
+ uint32_t filter_enabled;
+
+} Nrf51RNGState;
+
+
+#endif /* NRF51_RNG_H_ */
--
2.18.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [Qemu-devel] [PATCH 1/7] hw/misc/nrf51_rng: Add NRF51 random number generator peripheral
2018-08-06 10:01 ` [Qemu-devel] [PATCH 1/7] hw/misc/nrf51_rng: Add NRF51 random number generator peripheral Steffen Görtz
@ 2018-08-06 14:00 ` Stefan Hajnoczi
2018-08-16 15:45 ` Peter Maydell
1 sibling, 0 replies; 25+ messages in thread
From: Stefan Hajnoczi @ 2018-08-06 14:00 UTC (permalink / raw)
To: Steffen Görtz
Cc: qemu-devel, Joel Stanley, Jim Mussared, Julia Suvorova,
Peter Maydell, Thomas Huth
On Mon, Aug 6, 2018 at 11:01 AM, Steffen Görtz
<contrib@steffen-goertz.de> wrote:
> +#define NRF51_TRIGGER_TASK 0x01
> +#define NRF51_EVENT_CLEAR 0x00
Please consider putting these generic constants into hw/arm/nrf51.h or
a similar file that all nRF51 devices can include. That way code
duplication is eliminated.
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 1/7] hw/misc/nrf51_rng: Add NRF51 random number generator peripheral
2018-08-06 10:01 ` [Qemu-devel] [PATCH 1/7] hw/misc/nrf51_rng: Add NRF51 random number generator peripheral Steffen Görtz
2018-08-06 14:00 ` Stefan Hajnoczi
@ 2018-08-16 15:45 ` Peter Maydell
1 sibling, 0 replies; 25+ messages in thread
From: Peter Maydell @ 2018-08-16 15:45 UTC (permalink / raw)
To: Steffen Görtz
Cc: QEMU Developers, Stefan Hajnoczi, Joel Stanley, Jim Mussared,
Julia Suvorova, Thomas Huth
On 6 August 2018 at 11:01, Steffen Görtz <contrib@steffen-goertz.de> wrote:
> Add a model of the NRF51 random number generator peripheral.
> This is a simple random generator that continuously generates
> new random values after startup.
>
> Reference Manual: http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.pdf
>
> Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
> ---
> hw/misc/Makefile.objs | 1 +
> hw/misc/nrf51_rng.c | 273 ++++++++++++++++++++++++++++++++++++
> include/hw/misc/nrf51_rng.h | 71 ++++++++++
> 3 files changed, 345 insertions(+)
> create mode 100644 hw/misc/nrf51_rng.c
> create mode 100644 include/hw/misc/nrf51_rng.h
> +static void update_irq(Nrf51RNGState *s)
See remark on other patch about consistency of struct naming.
> +{
> + bool irq = false;
> + irq |= s->interrupt_enabled && s->event_valrdy;
You don't need to set irq to false and then modify it;
this is equivalent to
bool irq = s->interrupt_enabled && s->event_valrdy;
> + qemu_set_irq(s->irq, irq);
> +}
> +static void nrf51_rng_reset(DeviceState *dev)
> +{
> + Nrf51RNGState *s = NRF51_RNG(dev);
This seems to be missing code to actually reset the fields ?
> +
> + rng_update_timer(s);
> +}
Otherwise the device looks good.
thanks
-- PMM
^ permalink raw reply [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH 2/7] hw/nvram/nrf51_nvm: Add nRF51 non-volatile memories
2018-08-06 10:01 [Qemu-devel] [PATCH 0/7] arm: nRF51 Devices and Microbit Support Steffen Görtz
2018-08-06 10:01 ` [Qemu-devel] [PATCH 1/7] hw/misc/nrf51_rng: Add NRF51 random number generator peripheral Steffen Görtz
@ 2018-08-06 10:01 ` Steffen Görtz
2018-08-06 16:09 ` Stefan Hajnoczi
2018-08-16 16:03 ` Peter Maydell
2018-08-06 10:01 ` [Qemu-devel] [PATCH 3/7] tests: Add bbc:microbit / nRF51 test suite Steffen Görtz
` (5 subsequent siblings)
7 siblings, 2 replies; 25+ messages in thread
From: Steffen Görtz @ 2018-08-06 10:01 UTC (permalink / raw)
To: qemu-devel
Cc: Stefan Hajnoczi, Joel Stanley, Jim Mussared, Julia Suvorova,
Peter Maydell, Thomas Huth, Steffen Görtz
The nRF51 contains three regions of non-volatile memory (NVM):
- CODE (R/W): contains code
- FICR (R): Factory information like code size, chip id etc.
- UICR (R/W): Changeable configuration data. Lock bits, Code
protection configuration, Bootloader address, Nordic SoftRadio
configuration, Firmware configuration.
Read and write access to the memories is managed by the
Non-volatile memory controller.
Memory schema:
[ CPU ] -+- [ NVM, either FICR, UICR or CODE ]
| |
\- [ NVMC ]
---
hw/nvram/Makefile.objs | 1 +
hw/nvram/nrf51_nvm.c | 390 +++++++++++++++++++++++++++++++++++
include/hw/nvram/nrf51_nvm.h | 56 +++++
3 files changed, 447 insertions(+)
create mode 100644 hw/nvram/nrf51_nvm.c
create mode 100644 include/hw/nvram/nrf51_nvm.h
diff --git a/hw/nvram/Makefile.objs b/hw/nvram/Makefile.objs
index a912d25391..3f978e6212 100644
--- a/hw/nvram/Makefile.objs
+++ b/hw/nvram/Makefile.objs
@@ -5,3 +5,4 @@ common-obj-y += fw_cfg.o
common-obj-y += chrp_nvram.o
common-obj-$(CONFIG_MAC_NVRAM) += mac_nvram.o
obj-$(CONFIG_PSERIES) += spapr_nvram.o
+obj-$(CONFIG_NRF51_SOC) += nrf51_nvm.o
diff --git a/hw/nvram/nrf51_nvm.c b/hw/nvram/nrf51_nvm.c
new file mode 100644
index 0000000000..603dbd7ca2
--- /dev/null
+++ b/hw/nvram/nrf51_nvm.c
@@ -0,0 +1,390 @@
+/*
+ * Nordic Semiconductor nRF51 non-volatile memory
+ *
+ * It provides an interface to erase regions in flash memory.
+ * Furthermore it provides the user and factory information registers.
+ *
+ * Reference Manual: http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.pdf
+ *
+ * See nRF51 reference manual and product sheet sections:
+ * + Non-Volatile Memory Controller (NVMC)
+ * + Factory Information Configuration Registers (FICR)
+ * + User Information Configuration Registers (UICR)
+ *
+ * Copyright 2018 Steffen Görtz <contrib@steffen-goertz.de>
+ *
+ * This code is licensed under the GPL version 2 or later. See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu/log.h"
+#include "exec/address-spaces.h"
+#include "hw/nvram/nrf51_nvm.h"
+
+#define NRF51_NVMC_SIZE 0x1000
+
+#define NRF51_NVMC_READY 0x400
+#define NRF51_NVMC_READY_READY 0x01
+#define NRF51_NVMC_CONFIG 0x504
+#define NRF51_NVMC_CONFIG_MASK 0x03
+#define NRF51_NVMC_CONFIG_WEN 0x01
+#define NRF51_NVMC_CONFIG_EEN 0x02
+#define NRF51_NVMC_ERASEPCR1 0x508
+#define NRF51_NVMC_ERASEPCR0 0x510
+#define NRF51_NVMC_ERASEALL 0x50C
+#define NRF51_NVMC_ERASEUICR 0x514
+#define NRF51_NVMC_ERASE 0x01
+
+#define NRF51_FICR_SIZE 0x100
+
+#define NRF51_UICR_SIZE 0x100
+
+
+/* FICR Registers Assignments
+CODEPAGESIZE 0x010
+CODESIZE 0x014
+CLENR0 0x028
+PPFC 0x02C
+NUMRAMBLOCK 0x034
+SIZERAMBLOCKS 0x038
+SIZERAMBLOCK[0] 0x038
+SIZERAMBLOCK[1] 0x03C
+SIZERAMBLOCK[2] 0x040
+SIZERAMBLOCK[3] 0x044
+CONFIGID 0x05C
+DEVICEID[0] 0x060
+DEVICEID[1] 0x064
+ER[0] 0x080
+ER[1] 0x084
+ER[2] 0x088
+ER[3] 0x08C
+IR[0] 0x090
+IR[1] 0x094
+IR[2] 0x098
+IR[3] 0x09C
+DEVICEADDRTYPE 0x0A0
+DEVICEADDR[0] 0x0A4
+DEVICEADDR[1] 0x0A8
+OVERRIDEEN 0x0AC
+NRF_1MBIT[0] 0x0B0
+NRF_1MBIT[1] 0x0B4
+NRF_1MBIT[2] 0x0B8
+NRF_1MBIT[3] 0x0BC
+NRF_1MBIT[4] 0x0C0
+BLE_1MBIT[0] 0x0EC
+BLE_1MBIT[1] 0x0F0
+BLE_1MBIT[2] 0x0F4
+BLE_1MBIT[3] 0x0F8
+BLE_1MBIT[4] 0x0FC
+*/
+static const uint32_t ficr_content[64] = {
+ 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0x00000400,
+ 0x00000100, 0xFFFFFFFF, 0xFFFFFFFF, 0x00000002, 0x00002000,
+ 0x00002000, 0x00002000, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+ 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+ 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0x00000003,
+ 0x12345678, 0x9ABCDEF1, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+ 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+ 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+ 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+ 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+ 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+ 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+ 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF
+};
+
+static uint64_t ficr_read(void *opaque, hwaddr offset, unsigned int size)
+{
+ return ficr_content[offset >> 2];
+}
+
+static const MemoryRegionOps ficr_ops = {
+ .read = ficr_read,
+ .impl.min_access_size = 4,
+ .impl.max_access_size = 4,
+ .impl.unaligned = false,
+};
+
+/* UICR Registers Assignments
+CLENR0 0x000
+RBPCONF 0x004
+XTALFREQ 0x008
+FWID 0x010
+BOOTLOADERADDR 0x014
+NRFFW[0] 0x014
+NRFFW[1] 0x018
+NRFFW[2] 0x01C
+NRFFW[3] 0x020
+NRFFW[4] 0x024
+NRFFW[5] 0x028
+NRFFW[6] 0x02C
+NRFFW[7] 0x030
+NRFFW[8] 0x034
+NRFFW[9] 0x038
+NRFFW[10] 0x03C
+NRFFW[11] 0x040
+NRFFW[12] 0x044
+NRFFW[13] 0x048
+NRFFW[14] 0x04C
+NRFHW[0] 0x050
+NRFHW[1] 0x054
+NRFHW[2] 0x058
+NRFHW[3] 0x05C
+NRFHW[4] 0x060
+NRFHW[5] 0x064
+NRFHW[6] 0x068
+NRFHW[7] 0x06C
+NRFHW[8] 0x070
+NRFHW[9] 0x074
+NRFHW[10] 0x078
+NRFHW[11] 0x07C
+CUSTOMER[0] 0x080
+CUSTOMER[1] 0x084
+CUSTOMER[2] 0x088
+CUSTOMER[3] 0x08C
+CUSTOMER[4] 0x090
+CUSTOMER[5] 0x094
+CUSTOMER[6] 0x098
+CUSTOMER[7] 0x09C
+CUSTOMER[8] 0x0A0
+CUSTOMER[9] 0x0A4
+CUSTOMER[10] 0x0A8
+CUSTOMER[11] 0x0AC
+CUSTOMER[12] 0x0B0
+CUSTOMER[13] 0x0B4
+CUSTOMER[14] 0x0B8
+CUSTOMER[15] 0x0BC
+CUSTOMER[16] 0x0C0
+CUSTOMER[17] 0x0C4
+CUSTOMER[18] 0x0C8
+CUSTOMER[19] 0x0CC
+CUSTOMER[20] 0x0D0
+CUSTOMER[21] 0x0D4
+CUSTOMER[22] 0x0D8
+CUSTOMER[23] 0x0DC
+CUSTOMER[24] 0x0E0
+CUSTOMER[25] 0x0E4
+CUSTOMER[26] 0x0E8
+CUSTOMER[27] 0x0EC
+CUSTOMER[28] 0x0F0
+CUSTOMER[29] 0x0F4
+CUSTOMER[30] 0x0F8
+CUSTOMER[31] 0x0FC
+*/
+
+static const uint32_t uicr_fixture[NRF51_UICR_FIXTURE_SIZE] = {
+ 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+ 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+ 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+ 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+ 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+ 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+ 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+ 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+ 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+ 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+ 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+ 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+ 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF
+};
+
+static uint64_t uicr_read(void *opaque, hwaddr offset, unsigned int size)
+{
+ Nrf51NVMState *s = NRF51_NVM(opaque);
+
+ offset >>= 2;
+
+ return s->uicr_content[offset];
+}
+
+static void uicr_write(void *opaque, hwaddr offset, uint64_t value,
+ unsigned int size)
+{
+ Nrf51NVMState *s = NRF51_NVM(opaque);
+
+ offset >>= 2;
+
+ if (offset >= ARRAY_SIZE(s->uicr_content)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: bad read offset 0x%" HWADDR_PRIx "\n", __func__, offset);
+ return;
+ }
+
+ s->uicr_content[offset] = value;
+}
+
+static const MemoryRegionOps uicr_ops = {
+ .read = uicr_read,
+ .write = uicr_write,
+ .impl.min_access_size = 4,
+ .impl.max_access_size = 4,
+ .impl.unaligned = false,
+};
+
+
+static uint64_t io_read(void *opaque, hwaddr offset, unsigned int size)
+{
+ Nrf51NVMState *s = NRF51_NVM(opaque);
+ uint64_t r = 0;
+
+ switch (offset) {
+ case NRF51_NVMC_READY:
+ r = NRF51_NVMC_READY_READY;
+ break;
+ case NRF51_NVMC_CONFIG:
+ r = s->config;
+ break;
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: bad read offset 0x%" HWADDR_PRIx "\n", __func__, offset);
+ }
+
+ return r;
+}
+
+static void io_write(void *opaque, hwaddr offset, uint64_t value,
+ unsigned int size)
+{
+ Nrf51NVMState *s = NRF51_NVM(opaque);
+
+ switch (offset) {
+ case NRF51_NVMC_CONFIG:
+ s->config = value & NRF51_NVMC_CONFIG_MASK;
+ break;
+ case NRF51_NVMC_ERASEPCR0:
+ case NRF51_NVMC_ERASEPCR1:
+ value &= ~(s->page_size - 1);
+ if (value < (s->code_size * s->page_size)) {
+ address_space_write(&s->as, value, MEMTXATTRS_UNSPECIFIED,
+ s->empty_page, s->page_size);
+ }
+ break;
+ case NRF51_NVMC_ERASEALL:
+ if (value == NRF51_NVMC_ERASE) {
+ for (uint32_t i = 0; i < s->code_size; i++) {
+ address_space_write(&s->as, i * s->page_size,
+ MEMTXATTRS_UNSPECIFIED, s->empty_page, s->page_size);
+ }
+ memset(s->uicr_content, 0xFF, sizeof(s->uicr_content));
+ }
+ break;
+ case NRF51_NVMC_ERASEUICR:
+ if (value == NRF51_NVMC_ERASE) {
+ memset(s->uicr_content, 0xFF, sizeof(s->uicr_content));
+ }
+ break;
+
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: bad write offset 0x%" HWADDR_PRIx "\n", __func__, offset);
+ }
+}
+
+static const MemoryRegionOps io_ops = {
+ .read = io_read,
+ .write = io_write,
+ .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static void nrf51_nvm_init(Object *obj)
+{
+ Nrf51NVMState *s = NRF51_NVM(obj);
+ SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+
+ memory_region_init_io(&s->mmio, obj, &io_ops, s, "nrf51_soc.nvmc",
+ NRF51_NVMC_SIZE);
+ sysbus_init_mmio(sbd, &s->mmio);
+
+ memory_region_init_io(&s->ficr, NULL, &ficr_ops, s, "nrf51_soc.ficr",
+ sizeof(ficr_content));
+ memory_region_set_readonly(&s->ficr, true);
+ sysbus_init_mmio(sbd, &s->ficr);
+
+ memcpy(s->uicr_content, uicr_fixture, sizeof(s->uicr_content));
+ memory_region_init_io(&s->uicr, NULL, &uicr_ops, s, "nrf51_soc.uicr",
+ sizeof(s->uicr_content));
+ sysbus_init_mmio(sbd, &s->uicr);
+}
+
+static void nrf51_nvm_realize(DeviceState *dev, Error **errp)
+{
+ Nrf51NVMState *s = NRF51_NVM(dev);
+
+ if (!s->mr) {
+ error_setg(errp, "memory property was not set");
+ return;
+ }
+
+ if (s->page_size < NRF51_UICR_SIZE) {
+ error_setg(errp, "page size too small");
+ return;
+ }
+
+ s->empty_page = g_malloc(s->page_size);
+
+ address_space_init(&s->as, s->mr, "system-memory");
+}
+
+static void nrf51_nvm_unrealize(DeviceState *dev, Error **errp)
+{
+ Nrf51NVMState *s = NRF51_NVM(dev);
+
+ g_free(s->empty_page);
+ s->empty_page = NULL;
+}
+
+static void nrf51_nvm_reset(DeviceState *dev)
+{
+ Nrf51NVMState *s = NRF51_NVM(dev);
+
+ memset(s->empty_page, 0xFF, s->page_size);
+}
+
+
+static Property nrf51_nvm_properties[] = {
+ DEFINE_PROP_UINT16("page_size", Nrf51NVMState, page_size, 0x400),
+ DEFINE_PROP_UINT32("code_size", Nrf51NVMState, code_size, 0x100),
+ DEFINE_PROP_LINK("memory", Nrf51NVMState, mr, TYPE_MEMORY_REGION,
+ MemoryRegion *),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static const VMStateDescription vmstate_nvm = {
+ .name = "nrf51_soc.nvm",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32_ARRAY(uicr_content, Nrf51NVMState,
+ NRF51_UICR_FIXTURE_SIZE),
+ VMSTATE_UINT32(config, Nrf51NVMState),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static void nrf51_nvm_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->props = nrf51_nvm_properties;
+ dc->vmsd = &vmstate_nvm;
+ dc->realize = nrf51_nvm_realize;
+ dc->unrealize = nrf51_nvm_unrealize;
+ dc->reset = nrf51_nvm_reset;
+}
+
+static const TypeInfo nrf51_nvm_info = {
+ .name = TYPE_NRF51_NVM,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(Nrf51NVMState),
+ .instance_init = nrf51_nvm_init,
+ .class_init = nrf51_nvm_class_init
+};
+
+static void nrf51_nvm_register_types(void)
+{
+ type_register_static(&nrf51_nvm_info);
+}
+
+type_init(nrf51_nvm_register_types)
diff --git a/include/hw/nvram/nrf51_nvm.h b/include/hw/nvram/nrf51_nvm.h
new file mode 100644
index 0000000000..3d018de049
--- /dev/null
+++ b/include/hw/nvram/nrf51_nvm.h
@@ -0,0 +1,56 @@
+/*
+ * Nordic Semiconductor nRF51 non-volatile memory
+ *
+ * It provides an interface to erase regions in flash memory.
+ * Furthermore it provides the user and factory information registers.
+ *
+ * QEMU interface:
+ * + sysbus MMIO regions 0: NVMC peripheral registers
+ * + sysbus MMIO regions 1: FICR peripheral registers
+ * + sysbus MMIO regions 2: UICR peripheral registers
+ * + page_size property to set the page size in bytes.
+ * + code_size property to set the code size in number of pages.
+ *
+ * Accuracy of the peripheral model:
+ * + The NVMC is always ready, all requested erase operations succeed
+ * immediately.
+ * + CONFIG.WEN and CONFIG.EEN flags can be written and read back
+ * but are not evaluated to check whether a requested write/erase operation
+ * is legal.
+ * + Code regions (MPU configuration) are disregarded.
+ *
+ * Copyright 2018 Steffen Görtz <contrib@steffen-goertz.de>
+ *
+ * This code is licensed under the GPL version 2 or later. See
+ * the COPYING file in the top-level directory.
+ *
+ */
+#ifndef NRF51_NVM_H
+#define NRF51_NVM_H
+
+#include "hw/sysbus.h"
+#define TYPE_NRF51_NVM "nrf51_soc.nvm"
+#define NRF51_NVM(obj) OBJECT_CHECK(Nrf51NVMState, (obj), TYPE_NRF51_NVM)
+
+#define NRF51_UICR_FIXTURE_SIZE 64
+
+typedef struct Nrf51NVMState {
+ SysBusDevice parent_obj;
+
+ MemoryRegion mmio;
+ MemoryRegion ficr;
+ MemoryRegion uicr;
+
+ uint32_t uicr_content[NRF51_UICR_FIXTURE_SIZE];
+ uint32_t code_size;
+ uint16_t page_size;
+ uint8_t *empty_page;
+ MemoryRegion *mr;
+ AddressSpace as;
+
+ uint32_t config;
+
+} Nrf51NVMState;
+
+
+#endif
--
2.18.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [Qemu-devel] [PATCH 2/7] hw/nvram/nrf51_nvm: Add nRF51 non-volatile memories
2018-08-06 10:01 ` [Qemu-devel] [PATCH 2/7] hw/nvram/nrf51_nvm: Add nRF51 non-volatile memories Steffen Görtz
@ 2018-08-06 16:09 ` Stefan Hajnoczi
2018-08-08 9:58 ` Steffen Görtz
2018-08-16 16:03 ` Peter Maydell
1 sibling, 1 reply; 25+ messages in thread
From: Stefan Hajnoczi @ 2018-08-06 16:09 UTC (permalink / raw)
To: Steffen Görtz
Cc: qemu-devel, Joel Stanley, Jim Mussared, Julia Suvorova,
Peter Maydell, Thomas Huth
On Mon, Aug 6, 2018 at 11:01 AM, Steffen Görtz
<contrib@steffen-goertz.de> wrote:
> +static uint64_t uicr_read(void *opaque, hwaddr offset, unsigned int size)
> +{
> + Nrf51NVMState *s = NRF51_NVM(opaque);
> +
> + offset >>= 2;
> +
> + return s->uicr_content[offset];
> +}
> +
> +static void uicr_write(void *opaque, hwaddr offset, uint64_t value,
> + unsigned int size)
> +{
> + Nrf51NVMState *s = NRF51_NVM(opaque);
> +
> + offset >>= 2;
> +
> + if (offset >= ARRAY_SIZE(s->uicr_content)) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: bad read offset 0x%" HWADDR_PRIx "\n", __func__, offset);
> + return;
> + }
There is asymmetry here: uicr_read() doesn't check offset before
indexing the array but uicr_write() does. Either the check is
necessary or it's not.
I think this check isn't necessary since the memory region is sized
appropriately:
memory_region_init_io(&s->uicr, NULL, &uicr_ops, s, "nrf51_soc.uicr",
sizeof(s->uicr_content));
> +static void nrf51_nvm_reset(DeviceState *dev)
> +{
> + Nrf51NVMState *s = NRF51_NVM(dev);
> +
> + memset(s->empty_page, 0xFF, s->page_size);
> +}
Can this be done in ->realize()? Nothing changes the contents of
empty_page, so a ->reset() function seems unnecessary.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Qemu-devel] [PATCH 2/7] hw/nvram/nrf51_nvm: Add nRF51 non-volatile memories
2018-08-06 16:09 ` Stefan Hajnoczi
@ 2018-08-08 9:58 ` Steffen Görtz
0 siblings, 0 replies; 25+ messages in thread
From: Steffen Görtz @ 2018-08-08 9:58 UTC (permalink / raw)
To: Stefan Hajnoczi, Steffen Görtz
Cc: qemu-devel, Joel Stanley, Jim Mussared, Julia Suvorova,
Peter Maydell, Thomas Huth
Hi Stefan,
thank you for your review!
>
> There is asymmetry here: uicr_read() doesn't check offset before
> indexing the array but uicr_write() does. Either the check is
> necessary or it's not.
>
> I think this check isn't necessary since the memory region is sized
> appropriately:
>
> memory_region_init_io(&s->uicr, NULL, &uicr_ops, s, "nrf51_soc.uicr",
> sizeof(s->uicr_content));
>
>> +static void nrf51_nvm_reset(DeviceState *dev)
>> +{
>> + Nrf51NVMState *s = NRF51_NVM(dev);
>> +
>> + memset(s->empty_page, 0xFF, s->page_size);
>> +}
>
> Can this be done in ->realize()? Nothing changes the contents of
> empty_page, so a ->reset() function seems unnecessary.
>
Addressed in next revision of this patch.
Steffen
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 2/7] hw/nvram/nrf51_nvm: Add nRF51 non-volatile memories
2018-08-06 10:01 ` [Qemu-devel] [PATCH 2/7] hw/nvram/nrf51_nvm: Add nRF51 non-volatile memories Steffen Görtz
2018-08-06 16:09 ` Stefan Hajnoczi
@ 2018-08-16 16:03 ` Peter Maydell
2018-08-21 8:31 ` Steffen Görtz
1 sibling, 1 reply; 25+ messages in thread
From: Peter Maydell @ 2018-08-16 16:03 UTC (permalink / raw)
To: Steffen Görtz
Cc: QEMU Developers, Stefan Hajnoczi, Joel Stanley, Jim Mussared,
Julia Suvorova, Thomas Huth
On 6 August 2018 at 11:01, Steffen Görtz <contrib@steffen-goertz.de> wrote:
> The nRF51 contains three regions of non-volatile memory (NVM):
> - CODE (R/W): contains code
> - FICR (R): Factory information like code size, chip id etc.
> - UICR (R/W): Changeable configuration data. Lock bits, Code
> protection configuration, Bootloader address, Nordic SoftRadio
> configuration, Firmware configuration.
>
> Read and write access to the memories is managed by the
> Non-volatile memory controller.
>
> Memory schema:
> [ CPU ] -+- [ NVM, either FICR, UICR or CODE ]
> | |
> \- [ NVMC ]
> ---
> hw/nvram/Makefile.objs | 1 +
> hw/nvram/nrf51_nvm.c | 390 +++++++++++++++++++++++++++++++++++
> include/hw/nvram/nrf51_nvm.h | 56 +++++
> 3 files changed, 447 insertions(+)
> create mode 100644 hw/nvram/nrf51_nvm.c
> create mode 100644 include/hw/nvram/nrf51_nvm.h
>
> diff --git a/hw/nvram/Makefile.objs b/hw/nvram/Makefile.objs
> index a912d25391..3f978e6212 100644
> --- a/hw/nvram/Makefile.objs
> +++ b/hw/nvram/Makefile.objs
> @@ -5,3 +5,4 @@ common-obj-y += fw_cfg.o
> common-obj-y += chrp_nvram.o
> common-obj-$(CONFIG_MAC_NVRAM) += mac_nvram.o
> obj-$(CONFIG_PSERIES) += spapr_nvram.o
> +obj-$(CONFIG_NRF51_SOC) += nrf51_nvm.o
> diff --git a/hw/nvram/nrf51_nvm.c b/hw/nvram/nrf51_nvm.c
> new file mode 100644
> index 0000000000..603dbd7ca2
> --- /dev/null
> +++ b/hw/nvram/nrf51_nvm.c
> @@ -0,0 +1,390 @@
> +/*
> + * Nordic Semiconductor nRF51 non-volatile memory
> + *
> + * It provides an interface to erase regions in flash memory.
> + * Furthermore it provides the user and factory information registers.
> + *
> + * Reference Manual: http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.pdf
> + *
> + * See nRF51 reference manual and product sheet sections:
> + * + Non-Volatile Memory Controller (NVMC)
> + * + Factory Information Configuration Registers (FICR)
> + * + User Information Configuration Registers (UICR)
> + *
> + * Copyright 2018 Steffen Görtz <contrib@steffen-goertz.de>
> + *
> + * This code is licensed under the GPL version 2 or later. See
> + * the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu/log.h"
> +#include "exec/address-spaces.h"
> +#include "hw/nvram/nrf51_nvm.h"
> +
> +#define NRF51_NVMC_SIZE 0x1000
> +
> +#define NRF51_NVMC_READY 0x400
> +#define NRF51_NVMC_READY_READY 0x01
> +#define NRF51_NVMC_CONFIG 0x504
> +#define NRF51_NVMC_CONFIG_MASK 0x03
> +#define NRF51_NVMC_CONFIG_WEN 0x01
> +#define NRF51_NVMC_CONFIG_EEN 0x02
> +#define NRF51_NVMC_ERASEPCR1 0x508
> +#define NRF51_NVMC_ERASEPCR0 0x510
> +#define NRF51_NVMC_ERASEALL 0x50C
> +#define NRF51_NVMC_ERASEUICR 0x514
> +#define NRF51_NVMC_ERASE 0x01
> +
> +#define NRF51_FICR_SIZE 0x100
> +
> +#define NRF51_UICR_SIZE 0x100
> +
> +
> +/* FICR Registers Assignments
> +CODEPAGESIZE 0x010
> +CODESIZE 0x014
This is confusing because these lines look like code because they
don't have the leading " * ". Please keep multiline comments to
the standard format that CODING_STYLE (now) suggests.
> +CLENR0 0x028
> +PPFC 0x02C
> +NUMRAMBLOCK 0x034
> +SIZERAMBLOCKS 0x038
> +SIZERAMBLOCK[0] 0x038
> +SIZERAMBLOCK[1] 0x03C
> +SIZERAMBLOCK[2] 0x040
> +SIZERAMBLOCK[3] 0x044
> +CONFIGID 0x05C
> +DEVICEID[0] 0x060
> +DEVICEID[1] 0x064
> +ER[0] 0x080
> +ER[1] 0x084
> +ER[2] 0x088
> +ER[3] 0x08C
> +IR[0] 0x090
> +IR[1] 0x094
> +IR[2] 0x098
> +IR[3] 0x09C
> +DEVICEADDRTYPE 0x0A0
> +DEVICEADDR[0] 0x0A4
> +DEVICEADDR[1] 0x0A8
> +OVERRIDEEN 0x0AC
> +NRF_1MBIT[0] 0x0B0
> +NRF_1MBIT[1] 0x0B4
> +NRF_1MBIT[2] 0x0B8
> +NRF_1MBIT[3] 0x0BC
> +NRF_1MBIT[4] 0x0C0
> +BLE_1MBIT[0] 0x0EC
> +BLE_1MBIT[1] 0x0F0
> +BLE_1MBIT[2] 0x0F4
> +BLE_1MBIT[3] 0x0F8
> +BLE_1MBIT[4] 0x0FC
> +*/
> +static const uint32_t ficr_content[64] = {
> + 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0x00000400,
> + 0x00000100, 0xFFFFFFFF, 0xFFFFFFFF, 0x00000002, 0x00002000,
> + 0x00002000, 0x00002000, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
> + 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
> + 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0x00000003,
> + 0x12345678, 0x9ABCDEF1, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
> + 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
> + 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
> + 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
> + 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
> + 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
> + 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
> + 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF
> +};
> +
> +static uint64_t ficr_read(void *opaque, hwaddr offset, unsigned int size)
> +{
> + return ficr_content[offset >> 2];
We can't get here with a bogus offset because of the use of sizeof()
when registering the memory region. I think that an assert() that
offset is in range before doing the array dereference would be
helpful though, as a defensive guard against the MR size being
changed in future: it turns the consequences of that bug from
"read out of bounds" to "assertion failure". Similarly with uicr.
> +}
> +static const uint32_t uicr_fixture[NRF51_UICR_FIXTURE_SIZE] = {
> + 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
> + 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
> + 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
> + 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
> + 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
> + 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
> + 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
> + 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
> + 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
> + 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
> + 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
> + 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
> + 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF
> +};
Would this ever be anything other than "all 0xff" ? If not, might
be more obvious just to use memset() to init/reset the data.
> +
> +static uint64_t uicr_read(void *opaque, hwaddr offset, unsigned int size)
> +{
> + Nrf51NVMState *s = NRF51_NVM(opaque);
> +
> + offset >>= 2;
> +
> + return s->uicr_content[offset];
> +}
> +
> +static void uicr_write(void *opaque, hwaddr offset, uint64_t value,
> + unsigned int size)
> +{
> + Nrf51NVMState *s = NRF51_NVM(opaque);
> +
> + offset >>= 2;
> +
> + if (offset >= ARRAY_SIZE(s->uicr_content)) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: bad read offset 0x%" HWADDR_PRIx "\n", __func__, offset);
> + return;
> + }
> +
> + s->uicr_content[offset] = value;
> +}
> +
> +static const MemoryRegionOps uicr_ops = {
> + .read = uicr_read,
> + .write = uicr_write,
> + .impl.min_access_size = 4,
> + .impl.max_access_size = 4,
> + .impl.unaligned = false,
> +};
> +
> +
> +static uint64_t io_read(void *opaque, hwaddr offset, unsigned int size)
> +{
> + Nrf51NVMState *s = NRF51_NVM(opaque);
> + uint64_t r = 0;
> +
> + switch (offset) {
> + case NRF51_NVMC_READY:
> + r = NRF51_NVMC_READY_READY;
> + break;
> + case NRF51_NVMC_CONFIG:
> + r = s->config;
> + break;
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: bad read offset 0x%" HWADDR_PRIx "\n", __func__, offset);
Missing "break;" here (and at the end of the switches in the
other functions in this file). The compiler won't complain
(and the behaviour is correct) but Coverity likely will
produce a warning later.
> + }
> +
> + return r;
> +}
> +
> +static void io_write(void *opaque, hwaddr offset, uint64_t value,
> + unsigned int size)
> +{
> + Nrf51NVMState *s = NRF51_NVM(opaque);
> +
> + switch (offset) {
> + case NRF51_NVMC_CONFIG:
> + s->config = value & NRF51_NVMC_CONFIG_MASK;
> + break;
> + case NRF51_NVMC_ERASEPCR0:
> + case NRF51_NVMC_ERASEPCR1:
> + value &= ~(s->page_size - 1);
> + if (value < (s->code_size * s->page_size)) {
> + address_space_write(&s->as, value, MEMTXATTRS_UNSPECIFIED,
> + s->empty_page, s->page_size);
> + }
> + break;
> + case NRF51_NVMC_ERASEALL:
> + if (value == NRF51_NVMC_ERASE) {
> + for (uint32_t i = 0; i < s->code_size; i++) {
> + address_space_write(&s->as, i * s->page_size,
> + MEMTXATTRS_UNSPECIFIED, s->empty_page, s->page_size);
> + }
> + memset(s->uicr_content, 0xFF, sizeof(s->uicr_content));
> + }
> + break;
> + case NRF51_NVMC_ERASEUICR:
> + if (value == NRF51_NVMC_ERASE) {
> + memset(s->uicr_content, 0xFF, sizeof(s->uicr_content));
> + }
> + break;
> +
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: bad write offset 0x%" HWADDR_PRIx "\n", __func__, offset);
> + }
> +}
> +
> +static const MemoryRegionOps io_ops = {
> + .read = io_read,
> + .write = io_write,
> + .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static void nrf51_nvm_init(Object *obj)
> +{
> + Nrf51NVMState *s = NRF51_NVM(obj);
> + SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +
> + memory_region_init_io(&s->mmio, obj, &io_ops, s, "nrf51_soc.nvmc",
> + NRF51_NVMC_SIZE);
> + sysbus_init_mmio(sbd, &s->mmio);
> +
> + memory_region_init_io(&s->ficr, NULL, &ficr_ops, s, "nrf51_soc.ficr",
> + sizeof(ficr_content));
> + memory_region_set_readonly(&s->ficr, true);
memory_region_set_readonly() is only intended for use with
RAM regions, which this is not. Usually if you want an IO
region that has "ignores writes" behaviour you just give it
a write function that does nothing.
> + sysbus_init_mmio(sbd, &s->ficr);
> +
> + memcpy(s->uicr_content, uicr_fixture, sizeof(s->uicr_content));
The guest can change uicr_content[], so this memcpy() is a part
of reset and should be in the reset function, so that it gets
set back to its correct value on a reset.
> + memory_region_init_io(&s->uicr, NULL, &uicr_ops, s, "nrf51_soc.uicr",
> + sizeof(s->uicr_content));
> + sysbus_init_mmio(sbd, &s->uicr);
> +}
> +
> +static void nrf51_nvm_realize(DeviceState *dev, Error **errp)
> +{
> + Nrf51NVMState *s = NRF51_NVM(dev);
> +
> + if (!s->mr) {
> + error_setg(errp, "memory property was not set");
> + return;
> + }
> +
> + if (s->page_size < NRF51_UICR_SIZE) {
> + error_setg(errp, "page size too small");
> + return;
> + }
> +
> + s->empty_page = g_malloc(s->page_size);
> +
> + address_space_init(&s->as, s->mr, "system-memory");
> +}
> +
> +static void nrf51_nvm_unrealize(DeviceState *dev, Error **errp)
> +{
> + Nrf51NVMState *s = NRF51_NVM(dev);
> +
> + g_free(s->empty_page);
> + s->empty_page = NULL;
> +}
This device can't be hot-unplugged, so an unrealize function
is unreachable and not required.
> +
> +static void nrf51_nvm_reset(DeviceState *dev)
> +{
> + Nrf51NVMState *s = NRF51_NVM(dev);
> +
> + memset(s->empty_page, 0xFF, s->page_size);
> +}
> +
> +
> +static Property nrf51_nvm_properties[] = {
> + DEFINE_PROP_UINT16("page_size", Nrf51NVMState, page_size, 0x400),
Do the different NRF51 variants really have different NAND page sizes ?
> + DEFINE_PROP_UINT32("code_size", Nrf51NVMState, code_size, 0x100),
> + DEFINE_PROP_LINK("memory", Nrf51NVMState, mr, TYPE_MEMORY_REGION,
> + MemoryRegion *),
> + DEFINE_PROP_END_OF_LIST(),
> +};
thanks
-- PMM
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Qemu-devel] [PATCH 2/7] hw/nvram/nrf51_nvm: Add nRF51 non-volatile memories
2018-08-16 16:03 ` Peter Maydell
@ 2018-08-21 8:31 ` Steffen Görtz
0 siblings, 0 replies; 25+ messages in thread
From: Steffen Görtz @ 2018-08-21 8:31 UTC (permalink / raw)
To: Peter Maydell, Steffen Görtz
Cc: QEMU Developers, Stefan Hajnoczi, Joel Stanley, Jim Mussared,
Julia Suvorova, Thomas Huth
Hi Peter,
>> +
>> +static Property nrf51_nvm_properties[] = {
>> + DEFINE_PROP_UINT16("page_size", Nrf51NVMState, page_size, 0x400),
>
> Do the different NRF51 variants really have different NAND page sizes ?
>
it seems that is not the case at least for the NRF51 Series. I removed the property in favor for a nrf51.h-wide constant. I think this is NOR flash btw, because it is single-byte writable, which is usually a sign that it is NOR flash. But i could be wrong and the datasheet does not state anything.
Thank you for your remarks!
Steffen
^ permalink raw reply [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH 3/7] tests: Add bbc:microbit / nRF51 test suite
2018-08-06 10:01 [Qemu-devel] [PATCH 0/7] arm: nRF51 Devices and Microbit Support Steffen Görtz
2018-08-06 10:01 ` [Qemu-devel] [PATCH 1/7] hw/misc/nrf51_rng: Add NRF51 random number generator peripheral Steffen Görtz
2018-08-06 10:01 ` [Qemu-devel] [PATCH 2/7] hw/nvram/nrf51_nvm: Add nRF51 non-volatile memories Steffen Görtz
@ 2018-08-06 10:01 ` Steffen Görtz
2018-08-08 9:09 ` Stefan Hajnoczi
2018-08-06 10:01 ` [Qemu-devel] [PATCH 4/7] hw/gpio/nrf51_gpio: Add nRF51 GPIO peripheral Steffen Görtz
` (4 subsequent siblings)
7 siblings, 1 reply; 25+ messages in thread
From: Steffen Görtz @ 2018-08-06 10:01 UTC (permalink / raw)
To: qemu-devel
Cc: Stefan Hajnoczi, Joel Stanley, Jim Mussared, Julia Suvorova,
Peter Maydell, Thomas Huth, Steffen Görtz
The microbit-test includes tests for the nRF51 NVMC
peripheral and will host future nRF51 peripheral tests
and board-level bbc:microbit tests.
Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
---
tests/Makefile.include | 2 +
tests/microbit-test.c | 126 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 128 insertions(+)
create mode 100644 tests/microbit-test.c
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 760a0f18b6..1f1206e33a 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -378,6 +378,7 @@ check-qtest-sparc64-y += tests/boot-serial-test$(EXESUF)
check-qtest-arm-y = tests/tmp105-test$(EXESUF)
check-qtest-arm-y += tests/pca9552-test$(EXESUF)
check-qtest-arm-y += tests/ds1338-test$(EXESUF)
+check-qtest-arm-y += tests/microbit-test$(EXESUF)
check-qtest-arm-y += tests/m25p80-test$(EXESUF)
gcov-files-arm-y += hw/misc/tmp105.c
check-qtest-arm-y += tests/virtio-blk-test$(EXESUF)
@@ -793,6 +794,7 @@ tests/pxe-test$(EXESUF): tests/pxe-test.o tests/boot-sector.o $(libqos-obj-y)
tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(libqos-omap-obj-y)
tests/pca9552-test$(EXESUF): tests/pca9552-test.o $(libqos-omap-obj-y)
tests/ds1338-test$(EXESUF): tests/ds1338-test.o $(libqos-imx-obj-y)
+tests/microbit-test$(EXESUF): tests/microbit-test.o
tests/m25p80-test$(EXESUF): tests/m25p80-test.o
tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y)
tests/q35-test$(EXESUF): tests/q35-test.o $(libqos-pc-obj-y)
diff --git a/tests/microbit-test.c b/tests/microbit-test.c
new file mode 100644
index 0000000000..9f83063cf3
--- /dev/null
+++ b/tests/microbit-test.c
@@ -0,0 +1,126 @@
+ /*
+ * QTest testcase for Microbit board using the Nordic Semiconductor nRF51 SoC.
+ *
+ * nRF51:
+ * Reference Manual: http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.pdf
+ * Product Spec: http://infocenter.nordicsemi.com/pdf/nRF51822_PS_v3.1.pdf
+ *
+ * Microbit Board: http://microbit.org/
+ *
+ * Copyright 2018 Steffen Görtz <contrib@steffen-goertz.de>
+ *
+ * This code is licensed under the GPL version 2 or later. See
+ * the COPYING file in the top-level directory.
+ */
+
+
+#include "qemu/osdep.h"
+#include "exec/hwaddr.h"
+#include "libqtest.h"
+
+
+#define PAGE_SIZE 1024
+#define FLASH_SIZE (256 * PAGE_SIZE)
+#define FLASH_BASE 0x00000000
+#define UICR_BASE 0x10001000
+#define UICR_SIZE 0x100
+#define NVMC_BASE 0x4001E000UL
+#define NVMC_READY 0x400
+#define NVMC_CONFIG 0x504
+#define NVMC_ERASEPAGE 0x508
+#define NVMC_ERASEPCR1 0x508
+#define NVMC_ERASEALL 0x50C
+#define NVMC_ERASEPCR0 0x510
+#define NVMC_ERASEUICR 0x514
+
+
+static void fill_and_erase(hwaddr base, hwaddr size, uint32_t address_reg)
+{
+ /* Fill memory */
+ writel(NVMC_BASE + NVMC_CONFIG, 0x01);
+ for (hwaddr i = 0; i < size; ++i) {
+ writeb(base + i, i);
+ g_assert_cmpuint(readb(base + i), ==, i & 0xFF);
+ }
+ writel(NVMC_BASE + NVMC_CONFIG, 0x00);
+
+ /* Erase Page */
+ writel(NVMC_BASE + NVMC_CONFIG, 0x02);
+ writel(NVMC_BASE + address_reg, base);
+ writel(NVMC_BASE + NVMC_CONFIG, 0x00);
+
+ /* Check memory */
+ for (hwaddr i = 0; i < size; ++i) {
+ g_assert_cmpuint(readb(base + i), ==, 0xFF);
+ }
+}
+
+static void test_nrf51_nvmc(void)
+{
+ uint32_t value;
+ /* Test always ready */
+ value = readl(NVMC_BASE + NVMC_READY);
+ g_assert_cmpuint(value & 0x01, ==, 0x01);
+
+ /* Test write-read config register */
+ writel(NVMC_BASE + NVMC_CONFIG, 0x03);
+ g_assert_cmpuint(readl(NVMC_BASE + NVMC_CONFIG), ==, 0x03);
+ writel(NVMC_BASE + NVMC_CONFIG, 0x00);
+ g_assert_cmpuint(readl(NVMC_BASE + NVMC_CONFIG), ==, 0x00);
+
+ /* Test PCR0 */
+ fill_and_erase(FLASH_BASE, PAGE_SIZE, NVMC_ERASEPCR0);
+ fill_and_erase(FLASH_BASE + PAGE_SIZE, PAGE_SIZE, NVMC_ERASEPCR0);
+
+ /* Test PCR1 */
+ fill_and_erase(FLASH_BASE, PAGE_SIZE, NVMC_ERASEPCR1);
+ fill_and_erase(FLASH_BASE + PAGE_SIZE, PAGE_SIZE, NVMC_ERASEPCR1);
+
+ /* Erase all */
+ writel(NVMC_BASE + NVMC_CONFIG, 0x01);
+ for (hwaddr i = 0; i < FLASH_SIZE / 4; i++) {
+ writel(FLASH_BASE + i * 4, i);
+ g_assert_cmpuint(readl(FLASH_BASE + i * 4), ==, i);
+ }
+ writel(NVMC_BASE + NVMC_CONFIG, 0x00);
+
+ writel(NVMC_BASE + NVMC_CONFIG, 0x02);
+ writel(NVMC_BASE + NVMC_ERASEALL, 0x01);
+ writel(NVMC_BASE + NVMC_CONFIG, 0x00);
+
+ for (hwaddr i = 0; i < FLASH_SIZE / 4; i++) {
+ g_assert_cmpuint(readl(FLASH_BASE + i * 4), ==, 0xFFFFFFFF);
+ }
+
+ /* Erase UICR */
+ writel(NVMC_BASE + NVMC_CONFIG, 0x01);
+ for (hwaddr i = 0; i < UICR_SIZE / 4; i++) {
+ writel(UICR_BASE + i * 4, i);
+ g_assert_cmpuint(readl(UICR_BASE + i * 4), ==, i);
+ }
+ writel(NVMC_BASE + NVMC_CONFIG, 0x00);
+
+ writel(NVMC_BASE + NVMC_CONFIG, 0x02);
+ writel(NVMC_BASE + NVMC_ERASEUICR, 0x01);
+ writel(NVMC_BASE + NVMC_CONFIG, 0x00);
+
+ for (hwaddr i = 0; i < UICR_SIZE / 4; i++) {
+ g_assert_cmpuint(readl(UICR_BASE + i * 4), ==, 0xFFFFFFFF);
+ }
+}
+
+int main(int argc, char **argv)
+{
+ int ret;
+
+ g_test_init(&argc, &argv, NULL);
+
+ global_qtest = qtest_startf("-machine microbit");
+
+ qtest_add_func("/microbit/nrf51/nvmc", test_nrf51_nvmc);
+
+ ret = g_test_run();
+
+ qtest_quit(global_qtest);
+ return ret;
+}
--
2.18.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [Qemu-devel] [PATCH 3/7] tests: Add bbc:microbit / nRF51 test suite
2018-08-06 10:01 ` [Qemu-devel] [PATCH 3/7] tests: Add bbc:microbit / nRF51 test suite Steffen Görtz
@ 2018-08-08 9:09 ` Stefan Hajnoczi
2018-08-08 9:46 ` Julia Suvorova
0 siblings, 1 reply; 25+ messages in thread
From: Stefan Hajnoczi @ 2018-08-08 9:09 UTC (permalink / raw)
To: Steffen Görtz
Cc: qemu-devel, Joel Stanley, Jim Mussared, Julia Suvorova,
Peter Maydell, Thomas Huth
On Mon, Aug 6, 2018 at 11:01 AM, Steffen Görtz
<contrib@steffen-goertz.de> wrote:
> +#define PAGE_SIZE 1024
> +#define FLASH_SIZE (256 * PAGE_SIZE)
> +#define FLASH_BASE 0x00000000
> +#define UICR_BASE 0x10001000
> +#define UICR_SIZE 0x100
> +#define NVMC_BASE 0x4001E000UL
> +#define NVMC_READY 0x400
> +#define NVMC_CONFIG 0x504
> +#define NVMC_ERASEPAGE 0x508
> +#define NVMC_ERASEPCR1 0x508
> +#define NVMC_ERASEALL 0x50C
> +#define NVMC_ERASEPCR0 0x510
> +#define NVMC_ERASEUICR 0x514
All these constants could live in include/hw/arm/nrf51.h. That way
tests do not need to duplicate them. As more devices are implemented
we can expect this list to grow.
> +static void fill_and_erase(hwaddr base, hwaddr size, uint32_t address_reg)
Why hwaddr? writel() and friends use uint64_t.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 3/7] tests: Add bbc:microbit / nRF51 test suite
2018-08-08 9:09 ` Stefan Hajnoczi
@ 2018-08-08 9:46 ` Julia Suvorova
2018-08-09 16:16 ` Stefan Hajnoczi
0 siblings, 1 reply; 25+ messages in thread
From: Julia Suvorova @ 2018-08-08 9:46 UTC (permalink / raw)
To: Stefan Hajnoczi, Steffen Görtz
Cc: qemu-devel, Joel Stanley, Jim Mussared, Peter Maydell,
Thomas Huth
On 08.08.2018 12:09, Stefan Hajnoczi wrote:
> On Mon, Aug 6, 2018 at 11:01 AM, Steffen Görtz
> <contrib@steffen-goertz.de> wrote:
>> +#define PAGE_SIZE 1024
>> +#define FLASH_SIZE (256 * PAGE_SIZE)
>> +#define FLASH_BASE 0x00000000
>> +#define UICR_BASE 0x10001000
>> +#define UICR_SIZE 0x100
>> +#define NVMC_BASE 0x4001E000UL
>> +#define NVMC_READY 0x400
>> +#define NVMC_CONFIG 0x504
>> +#define NVMC_ERASEPAGE 0x508
>> +#define NVMC_ERASEPCR1 0x508
>> +#define NVMC_ERASEALL 0x50C
>> +#define NVMC_ERASEPCR0 0x510
>> +#define NVMC_ERASEUICR 0x514
>
> All these constants could live in include/hw/arm/nrf51.h. That way
> tests do not need to duplicate them. As more devices are implemented
> we can expect this list to grow.
Maybe it's better to move them to device headers?
include/hw/arm/nrf51.h has to include them in any case to use device
types.
Best regards, Julia Suvorova.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 3/7] tests: Add bbc:microbit / nRF51 test suite
2018-08-08 9:46 ` Julia Suvorova
@ 2018-08-09 16:16 ` Stefan Hajnoczi
0 siblings, 0 replies; 25+ messages in thread
From: Stefan Hajnoczi @ 2018-08-09 16:16 UTC (permalink / raw)
To: Julia Suvorova
Cc: Steffen Görtz, qemu-devel, Joel Stanley, Jim Mussared,
Peter Maydell, Thomas Huth
On Wed, Aug 8, 2018 at 10:46 AM, Julia Suvorova <jusual@mail.ru> wrote:
> On 08.08.2018 12:09, Stefan Hajnoczi wrote:
>>
>> On Mon, Aug 6, 2018 at 11:01 AM, Steffen Görtz
>> <contrib@steffen-goertz.de> wrote:
>>>
>>> +#define PAGE_SIZE 1024
>>> +#define FLASH_SIZE (256 * PAGE_SIZE)
>>> +#define FLASH_BASE 0x00000000
>>> +#define UICR_BASE 0x10001000
>>> +#define UICR_SIZE 0x100
>>> +#define NVMC_BASE 0x4001E000UL
>>> +#define NVMC_READY 0x400
>>> +#define NVMC_CONFIG 0x504
>>> +#define NVMC_ERASEPAGE 0x508
>>> +#define NVMC_ERASEPCR1 0x508
>>> +#define NVMC_ERASEALL 0x50C
>>> +#define NVMC_ERASEPCR0 0x510
>>> +#define NVMC_ERASEUICR 0x514
>>
>>
>> All these constants could live in include/hw/arm/nrf51.h. That way
>> tests do not need to duplicate them. As more devices are implemented
>> we can expect this list to grow.
>
>
> Maybe it's better to move them to device headers?
> include/hw/arm/nrf51.h has to include them in any case to use device
> types.
That would be fine. Each device (uart, rng, etc) would have it's own
include/hw/... header file and test cases can include that file to get
the constants/structs.
Stefan
^ permalink raw reply [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH 4/7] hw/gpio/nrf51_gpio: Add nRF51 GPIO peripheral
2018-08-06 10:01 [Qemu-devel] [PATCH 0/7] arm: nRF51 Devices and Microbit Support Steffen Görtz
` (2 preceding siblings ...)
2018-08-06 10:01 ` [Qemu-devel] [PATCH 3/7] tests: Add bbc:microbit / nRF51 test suite Steffen Görtz
@ 2018-08-06 10:01 ` Steffen Görtz
2018-08-08 9:11 ` Stefan Hajnoczi
2018-08-16 16:08 ` Peter Maydell
2018-08-06 10:01 ` [Qemu-devel] [PATCH 5/7] tests/microbit-test: Add Tests for nRF51 GPIO Steffen Görtz
` (3 subsequent siblings)
7 siblings, 2 replies; 25+ messages in thread
From: Steffen Görtz @ 2018-08-06 10:01 UTC (permalink / raw)
To: qemu-devel
Cc: Stefan Hajnoczi, Joel Stanley, Jim Mussared, Julia Suvorova,
Peter Maydell, Thomas Huth, Steffen Görtz
This adds a model of the nRF51 GPIO peripheral.
Reference Manual: http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.pdf
The nRF51 series microcontrollers support up to 32 GPIO pins in various configurations.
The pins can be used as input pins with pull-ups or pull-down.
Furthermore, three different output driver modes per level are
available (disconnected, standard, high-current).
The GPIO-Peripheral has a mechanism for detecting level changes which is
not featured in this model.
Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
---
Makefile.objs | 1 +
hw/gpio/Makefile.objs | 1 +
hw/gpio/nrf51_gpio.c | 305 +++++++++++++++++++++++++++++++++++
hw/gpio/trace-events | 7 +
include/hw/gpio/nrf51_gpio.h | 57 +++++++
5 files changed, 371 insertions(+)
create mode 100644 hw/gpio/nrf51_gpio.c
create mode 100644 hw/gpio/trace-events
create mode 100644 include/hw/gpio/nrf51_gpio.h
diff --git a/Makefile.objs b/Makefile.objs
index 7a9828da28..506ae4eae0 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -241,6 +241,7 @@ trace-events-subdirs += hw/usb
trace-events-subdirs += hw/vfio
trace-events-subdirs += hw/virtio
trace-events-subdirs += hw/xen
+trace-events-subdirs += hw/gpio
trace-events-subdirs += io
trace-events-subdirs += linux-user
trace-events-subdirs += migration
diff --git a/hw/gpio/Makefile.objs b/hw/gpio/Makefile.objs
index fa0a72e6d0..e5da0cb54f 100644
--- a/hw/gpio/Makefile.objs
+++ b/hw/gpio/Makefile.objs
@@ -8,3 +8,4 @@ common-obj-$(CONFIG_GPIO_KEY) += gpio_key.o
obj-$(CONFIG_OMAP) += omap_gpio.o
obj-$(CONFIG_IMX) += imx_gpio.o
obj-$(CONFIG_RASPI) += bcm2835_gpio.o
+obj-$(CONFIG_NRF51_SOC) += nrf51_gpio.o
diff --git a/hw/gpio/nrf51_gpio.c b/hw/gpio/nrf51_gpio.c
new file mode 100644
index 0000000000..c644239f10
--- /dev/null
+++ b/hw/gpio/nrf51_gpio.c
@@ -0,0 +1,305 @@
+/*
+ * nRF51 System-on-Chip general purpose input/output register definition
+ *
+ * Reference Manual: http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.pdf
+ * Product Spec: http://infocenter.nordicsemi.com/pdf/nRF51822_PS_v3.1.pdf
+ *
+ * Copyright 2018 Steffen Görtz <contrib@steffen-goertz.de>
+ *
+ * This code is licensed under the GPL version 2 or later. See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "hw/gpio/nrf51_gpio.h"
+#include "trace.h"
+
+#define NRF51_GPIO_SIZE 0x1000
+
+#define NRF51_GPIO_REG_OUT 0x504
+#define NRF51_GPIO_REG_OUTSET 0x508
+#define NRF51_GPIO_REG_OUTCLR 0x50C
+#define NRF51_GPIO_REG_IN 0x510
+#define NRF51_GPIO_REG_DIR 0x514
+#define NRF51_GPIO_REG_DIRSET 0x518
+#define NRF51_GPIO_REG_DIRCLR 0x51C
+#define NRF51_GPIO_REG_CNF_START 0x700
+#define NRF51_GPIO_REG_CNF_END 0x77F
+
+#define GPIO_PULLDOWN 1
+#define GPIO_PULLUP 3
+
+/**
+ * Check if the output driver is connected to the direction switch
+ * given the current configuration and logic level.
+ * It is not differentiated between standard and "high"(-power) drive modes.
+ */
+static bool is_connected(uint32_t config, uint32_t level)
+{
+ bool state;
+ uint32_t drive_config = extract32(config, 8, 3);
+
+ switch (drive_config) {
+ case 0 ... 3:
+ state = true;
+ break;
+ case 4 ... 5:
+ state = level != 0;
+ break;
+ case 6 ... 7:
+ state = level == 0;
+ break;
+ }
+
+ return state;
+}
+
+static void update_output_irq(Nrf51GPIOState *s, size_t i,
+ bool connected, bool level)
+{
+ int64_t irq_level = connected ? level : -1;
+ bool old_connected = extract32(s->old_out_connected, i, 1);
+ bool old_level = extract32(s->old_out, i, 1);
+
+ if ((old_connected != connected) || (old_level != level)) {
+ qemu_set_irq(s->output[i], irq_level);
+ trace_nrf51_gpio_update_output_irq(i, irq_level);
+ }
+
+ s->old_out = deposit32(s->old_out, i, 1, level);
+ s->old_out_connected = deposit32(s->old_out_connected, i, 1, connected);
+}
+
+static void update_state(Nrf51GPIOState *s)
+{
+ uint32_t pull;
+ bool connected_out, dir, connected_in, out, input;
+
+ for (size_t i = 0; i < NRF51_GPIO_PINS; i++) {
+ pull = extract32(s->cnf[i], 2, 2);
+ dir = extract32(s->cnf[i], 0, 1);
+ connected_in = extract32(s->in_mask, i, 1);
+ out = extract32(s->out, i, 1);
+ input = !extract32(s->cnf[i], 1, 1);
+ connected_out = is_connected(s->cnf[i], out) && dir;
+
+ update_output_irq(s, i, connected_out, out);
+
+ /** Pin both driven externally and internally */
+ if (connected_out && connected_in) {
+ qemu_log_mask(LOG_GUEST_ERROR, "GPIO pin %zu short circuited\n", i);
+ }
+
+ /**
+ * Input buffer disconnected from internal/external drives, so
+ * pull-up/pull-down becomes relevant
+ */
+ if (!input || (input && !connected_in && !connected_out)) {
+ if (pull == GPIO_PULLDOWN) {
+ s->in = deposit32(s->in, i, 1, 0);
+ } else if (pull == GPIO_PULLUP) {
+ s->in = deposit32(s->in, i, 1, 1);
+ }
+ }
+
+ /** Self stimulation through internal output driver **/
+ if (connected_out && !connected_in && input) {
+ s->in = deposit32(s->in, i, 1, out);
+ }
+ }
+
+}
+
+/**
+ * Direction is exposed in both the DIR register and the DIR bit
+ * of each PINs CNF configuration register. Reflect bits for pins in DIR
+ * to individual pin configuration registers.
+ */
+static void reflect_dir_bit_in_cnf(Nrf51GPIOState *s)
+{
+ uint32_t value = s->dir;
+ for (size_t i = 0; i < NRF51_GPIO_PINS; i++) {
+ s->cnf[i] = (s->cnf[i] & ~(1UL)) | ((value >> i) & 0x01);
+ }
+}
+
+static uint64_t nrf51_gpio_read(void *opaque, hwaddr offset, unsigned int size)
+{
+ Nrf51GPIOState *s = NRF51_GPIO(opaque);
+ uint64_t r = 0;
+ size_t idx;
+
+ switch (offset) {
+ case NRF51_GPIO_REG_OUT ... NRF51_GPIO_REG_OUTCLR:
+ r = s->out;
+ break;
+
+ case NRF51_GPIO_REG_IN:
+ r = s->in;
+ break;
+
+ case NRF51_GPIO_REG_DIR ... NRF51_GPIO_REG_DIRCLR:
+ r = s->dir;
+ break;
+
+ case NRF51_GPIO_REG_CNF_START ... NRF51_GPIO_REG_CNF_END:
+ idx = (offset - NRF51_GPIO_REG_CNF_START) / 4;
+ r = s->cnf[idx];
+ break;
+
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: bad read offset 0x%" HWADDR_PRIx "\n",
+ __func__, offset);
+ }
+
+ trace_nrf51_gpio_read(offset, r);
+
+ return r;
+}
+
+static void nrf51_gpio_write(void *opaque, hwaddr offset,
+ uint64_t value, unsigned int size)
+{
+ Nrf51GPIOState *s = NRF51_GPIO(opaque);
+ size_t idx;
+
+ trace_nrf51_gpio_write(offset, value);
+
+ switch (offset) {
+ case NRF51_GPIO_REG_OUT:
+ s->out = value;
+ break;
+
+ case NRF51_GPIO_REG_OUTSET:
+ s->out |= value;
+ break;
+
+ case NRF51_GPIO_REG_OUTCLR:
+ s->out &= ~value;
+ break;
+
+ case NRF51_GPIO_REG_DIR:
+ s->dir = value;
+ reflect_dir_bit_in_cnf(s);
+ break;
+
+ case NRF51_GPIO_REG_DIRSET:
+ s->dir |= value;
+ reflect_dir_bit_in_cnf(s);
+ break;
+
+ case NRF51_GPIO_REG_DIRCLR:
+ s->dir &= ~value;
+ reflect_dir_bit_in_cnf(s);
+ break;
+
+ case NRF51_GPIO_REG_CNF_START ... NRF51_GPIO_REG_CNF_END:
+ idx = (offset - NRF51_GPIO_REG_CNF_START) / 4;
+ s->cnf[idx] = value;
+ /* direction is exposed in both the DIR register and the DIR bit
+ * of each PINs CNF configuration register. */
+ s->dir = (s->dir & ~(1UL << idx)) | ((value & 0x01) << idx);
+ break;
+
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: bad write offset 0x%" HWADDR_PRIx "\n",
+ __func__, offset);
+ }
+
+ update_state(s);
+}
+
+static const MemoryRegionOps gpio_ops = {
+ .read = nrf51_gpio_read,
+ .write = nrf51_gpio_write,
+ .endianness = DEVICE_LITTLE_ENDIAN,
+ .impl.min_access_size = 4,
+ .impl.max_access_size = 4,
+};
+
+static void nrf51_gpio_set(void *opaque, int line, int value)
+{
+ Nrf51GPIOState *s = NRF51_GPIO(opaque);
+
+ trace_nrf51_gpio_set(line, value);
+
+ assert(line >= 0 && line < NRF51_GPIO_PINS);
+
+ s->in_mask = deposit32(s->in_mask, line, 1, value >= 0);
+ s->in = deposit32(s->in, line, 1, value > 0);
+
+ update_state(s);
+}
+
+static void nrf51_gpio_reset(DeviceState *dev)
+{
+ Nrf51GPIOState *s = NRF51_GPIO(dev);
+ size_t i;
+
+ s->out = 0;
+ s->old_out = 0;
+ s->old_out_connected = 0;
+ s->in = 0;
+ s->in_mask = 0;
+ s->dir = 0;
+
+ for (i = 0; i < NRF51_GPIO_PINS; i++) {
+ s->cnf[i] = 0x00000002;
+ }
+}
+
+static const VMStateDescription vmstate_nrf51_gpio = {
+ .name = TYPE_NRF51_GPIO,
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .minimum_version_id_old = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(out, Nrf51GPIOState),
+ VMSTATE_UINT32(in, Nrf51GPIOState),
+ VMSTATE_UINT32(in_mask, Nrf51GPIOState),
+ VMSTATE_UINT32(dir, Nrf51GPIOState),
+ VMSTATE_UINT32_ARRAY(cnf, Nrf51GPIOState, NRF51_GPIO_PINS),
+ VMSTATE_UINT32(old_out, Nrf51GPIOState),
+ VMSTATE_UINT32(old_out_connected, Nrf51GPIOState),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static void nrf51_gpio_init(Object *obj)
+{
+ Nrf51GPIOState *s = NRF51_GPIO(obj);
+
+ memory_region_init_io(&s->mmio, obj, &gpio_ops, s,
+ TYPE_NRF51_GPIO, NRF51_GPIO_SIZE);
+ sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
+
+ qdev_init_gpio_in(DEVICE(s), nrf51_gpio_set, NRF51_GPIO_PINS);
+ qdev_init_gpio_out(DEVICE(s), s->output, NRF51_GPIO_PINS);
+}
+
+static void nrf51_gpio_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->vmsd = &vmstate_nrf51_gpio;
+ dc->reset = nrf51_gpio_reset;
+ dc->desc = "nRF51 GPIO";
+}
+
+static const TypeInfo nrf51_gpio_info = {
+ .name = TYPE_NRF51_GPIO,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(Nrf51GPIOState),
+ .instance_init = nrf51_gpio_init,
+ .class_init = nrf51_gpio_class_init
+};
+
+static void nrf51_gpio_register_types(void)
+{
+ type_register_static(&nrf51_gpio_info);
+}
+
+type_init(nrf51_gpio_register_types)
diff --git a/hw/gpio/trace-events b/hw/gpio/trace-events
new file mode 100644
index 0000000000..11486b09b9
--- /dev/null
+++ b/hw/gpio/trace-events
@@ -0,0 +1,7 @@
+# See docs/devel/tracing.txt for syntax documentation.
+
+# hw/gpio/nrf51_gpio.c
+nrf51_gpio_read(uint64_t offset, uint64_t r) "offset 0x%" PRIx64 " value 0x%" PRIx64
+nrf51_gpio_write(uint64_t offset, uint64_t value) "offset 0x%" PRIx64 " value 0x%" PRIx64
+nrf51_gpio_set(int64_t line, int64_t value) "line %" PRIi64 " value 0x%" PRIi64
+nrf51_gpio_update_output_irq(int64_t line, int64_t value) "line %" PRIi64 " value 0x%" PRIi64
\ No newline at end of file
diff --git a/include/hw/gpio/nrf51_gpio.h b/include/hw/gpio/nrf51_gpio.h
new file mode 100644
index 0000000000..c5b8501521
--- /dev/null
+++ b/include/hw/gpio/nrf51_gpio.h
@@ -0,0 +1,57 @@
+/*
+ * nRF51 System-on-Chip general purpose input/output register definition
+ *
+ * Reference Manual: http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.pdf
+ * Product Spec: http://infocenter.nordicsemi.com/pdf/nRF51822_PS_v3.1.pdf
+ *
+ * QEMU interface:
+ * + sysbus MMIO regions 0: GPIO registers
+ * + Unnamed GPIO inputs 0-31: Set tri-state input level for GPIO pin.
+ * Level -1: Externally Disconnected/Floating; Pull-up/down will be regarded
+ * Level 0: Input externally driven LOW
+ * Level 1: Input externally driven HIGH
+ * + Unnamed GPIO outputs 0-31:
+ * Level -1: Disconnected/Floating
+ * Level 0: Driven LOW
+ * Level 1: Driven HIGH
+ *
+ * Accuracy of the peripheral model:
+ * + The nRF51 GPIO output driver supports two modes, standard and high-current
+ * mode. These different drive modes are not modeled and handled the same.
+ * + Pin SENSEing is not modeled/implemented.
+ *
+ * Copyright 2018 Steffen Görtz <contrib@steffen-goertz.de>
+ *
+ * This code is licensed under the GPL version 2 or later. See
+ * the COPYING file in the top-level directory.
+ *
+ */
+#ifndef NRF51_GPIO_H
+#define NRF51_GPIO_H
+
+#include "hw/sysbus.h"
+#define TYPE_NRF51_GPIO "nrf51_soc.gpio"
+#define NRF51_GPIO(obj) OBJECT_CHECK(Nrf51GPIOState, (obj), TYPE_NRF51_GPIO)
+
+#define NRF51_GPIO_PINS 32
+
+typedef struct Nrf51GPIOState {
+ SysBusDevice parent_obj;
+
+ MemoryRegion mmio;
+ qemu_irq irq;
+
+ uint32_t out;
+ uint32_t in;
+ uint32_t in_mask;
+ uint32_t dir;
+ uint32_t cnf[NRF51_GPIO_PINS];
+
+ uint32_t old_out;
+ uint32_t old_out_connected;
+
+ qemu_irq output[NRF51_GPIO_PINS];
+} Nrf51GPIOState;
+
+
+#endif
--
2.18.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [Qemu-devel] [PATCH 4/7] hw/gpio/nrf51_gpio: Add nRF51 GPIO peripheral
2018-08-06 10:01 ` [Qemu-devel] [PATCH 4/7] hw/gpio/nrf51_gpio: Add nRF51 GPIO peripheral Steffen Görtz
@ 2018-08-08 9:11 ` Stefan Hajnoczi
2018-08-16 16:08 ` Peter Maydell
1 sibling, 0 replies; 25+ messages in thread
From: Stefan Hajnoczi @ 2018-08-08 9:11 UTC (permalink / raw)
To: Steffen Görtz
Cc: qemu-devel, Joel Stanley, Jim Mussared, Julia Suvorova,
Peter Maydell, Thomas Huth
On Mon, Aug 6, 2018 at 11:01 AM, Steffen Görtz
<contrib@steffen-goertz.de> wrote:
> This adds a model of the nRF51 GPIO peripheral.
>
> Reference Manual: http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.pdf
>
> The nRF51 series microcontrollers support up to 32 GPIO pins in various configurations.
> The pins can be used as input pins with pull-ups or pull-down.
> Furthermore, three different output driver modes per level are
> available (disconnected, standard, high-current).
>
> The GPIO-Peripheral has a mechanism for detecting level changes which is
> not featured in this model.
>
> Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
> ---
> Makefile.objs | 1 +
> hw/gpio/Makefile.objs | 1 +
> hw/gpio/nrf51_gpio.c | 305 +++++++++++++++++++++++++++++++++++
> hw/gpio/trace-events | 7 +
> include/hw/gpio/nrf51_gpio.h | 57 +++++++
> 5 files changed, 371 insertions(+)
> create mode 100644 hw/gpio/nrf51_gpio.c
> create mode 100644 hw/gpio/trace-events
> create mode 100644 include/hw/gpio/nrf51_gpio.h
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 4/7] hw/gpio/nrf51_gpio: Add nRF51 GPIO peripheral
2018-08-06 10:01 ` [Qemu-devel] [PATCH 4/7] hw/gpio/nrf51_gpio: Add nRF51 GPIO peripheral Steffen Görtz
2018-08-08 9:11 ` Stefan Hajnoczi
@ 2018-08-16 16:08 ` Peter Maydell
1 sibling, 0 replies; 25+ messages in thread
From: Peter Maydell @ 2018-08-16 16:08 UTC (permalink / raw)
To: Steffen Görtz
Cc: QEMU Developers, Stefan Hajnoczi, Joel Stanley, Jim Mussared,
Julia Suvorova, Thomas Huth
On 6 August 2018 at 11:01, Steffen Görtz <contrib@steffen-goertz.de> wrote:
> This adds a model of the nRF51 GPIO peripheral.
>
> Reference Manual: http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.pdf
>
> The nRF51 series microcontrollers support up to 32 GPIO pins in various configurations.
> The pins can be used as input pins with pull-ups or pull-down.
> Furthermore, three different output driver modes per level are
> available (disconnected, standard, high-current).
>
> The GPIO-Peripheral has a mechanism for detecting level changes which is
> not featured in this model.
>
> Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
I have a couple of style nits below but otherwise this looks good.
> +#define NRF51_GPIO_SIZE 0x1000
> +
> +#define NRF51_GPIO_REG_OUT 0x504
> +#define NRF51_GPIO_REG_OUTSET 0x508
> +#define NRF51_GPIO_REG_OUTCLR 0x50C
> +#define NRF51_GPIO_REG_IN 0x510
> +#define NRF51_GPIO_REG_DIR 0x514
> +#define NRF51_GPIO_REG_DIRSET 0x518
> +#define NRF51_GPIO_REG_DIRCLR 0x51C
> +#define NRF51_GPIO_REG_CNF_START 0x700
> +#define NRF51_GPIO_REG_CNF_END 0x77F
> +
> +#define GPIO_PULLDOWN 1
> +#define GPIO_PULLUP 3
> +
> +/**
"/**" is for doc-comment format comments. Generally we only
need those for functions that are global, not file-local ones.
If you want to do a doc-comment, then you want to do it in the
right syntax, with description of input arguments and result
(eg see include/qemu/bitops.h for some examples). If not, just
start with "/*".
> + * Check if the output driver is connected to the direction switch
> + * given the current configuration and logic level.
> + * It is not differentiated between standard and "high"(-power) drive modes.
> + */
> +static bool is_connected(uint32_t config, uint32_t level)
> +{
> + bool state;
> + uint32_t drive_config = extract32(config, 8, 3);
> +
> + switch (drive_config) {
> + case 0 ... 3:
> + state = true;
> + break;
> + case 4 ... 5:
> + state = level != 0;
> + break;
> + case 6 ... 7:
> + state = level == 0;
> + break;
> + }
> +
> + return state;
> +}
> +
> +static void update_output_irq(Nrf51GPIOState *s, size_t i,
> + bool connected, bool level)
> +{
> + int64_t irq_level = connected ? level : -1;
> + bool old_connected = extract32(s->old_out_connected, i, 1);
> + bool old_level = extract32(s->old_out, i, 1);
> +
> + if ((old_connected != connected) || (old_level != level)) {
> + qemu_set_irq(s->output[i], irq_level);
> + trace_nrf51_gpio_update_output_irq(i, irq_level);
> + }
> +
> + s->old_out = deposit32(s->old_out, i, 1, level);
> + s->old_out_connected = deposit32(s->old_out_connected, i, 1, connected);
> +}
> +
> +static void update_state(Nrf51GPIOState *s)
> +{
> + uint32_t pull;
> + bool connected_out, dir, connected_in, out, input;
> +
> + for (size_t i = 0; i < NRF51_GPIO_PINS; i++) {
> + pull = extract32(s->cnf[i], 2, 2);
> + dir = extract32(s->cnf[i], 0, 1);
> + connected_in = extract32(s->in_mask, i, 1);
> + out = extract32(s->out, i, 1);
> + input = !extract32(s->cnf[i], 1, 1);
> + connected_out = is_connected(s->cnf[i], out) && dir;
> +
> + update_output_irq(s, i, connected_out, out);
> +
> + /** Pin both driven externally and internally */
Inline comments like these should definitely not have the doc-comment
starting indicator.
> + if (connected_out && connected_in) {
> + qemu_log_mask(LOG_GUEST_ERROR, "GPIO pin %zu short circuited\n", i);
> + }
> diff --git a/hw/gpio/trace-events b/hw/gpio/trace-events
> new file mode 100644
> index 0000000000..11486b09b9
> --- /dev/null
> +++ b/hw/gpio/trace-events
> @@ -0,0 +1,7 @@
> +# See docs/devel/tracing.txt for syntax documentation.
> +
> +# hw/gpio/nrf51_gpio.c
> +nrf51_gpio_read(uint64_t offset, uint64_t r) "offset 0x%" PRIx64 " value 0x%" PRIx64
> +nrf51_gpio_write(uint64_t offset, uint64_t value) "offset 0x%" PRIx64 " value 0x%" PRIx64
> +nrf51_gpio_set(int64_t line, int64_t value) "line %" PRIi64 " value 0x%" PRIi64
> +nrf51_gpio_update_output_irq(int64_t line, int64_t value) "line %" PRIi64 " value 0x%" PRIi64
> \ No newline at end of file
You want to make sure there's a newline at the end of the file...
thanks
-- PMM
^ permalink raw reply [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH 5/7] tests/microbit-test: Add Tests for nRF51 GPIO
2018-08-06 10:01 [Qemu-devel] [PATCH 0/7] arm: nRF51 Devices and Microbit Support Steffen Görtz
` (3 preceding siblings ...)
2018-08-06 10:01 ` [Qemu-devel] [PATCH 4/7] hw/gpio/nrf51_gpio: Add nRF51 GPIO peripheral Steffen Görtz
@ 2018-08-06 10:01 ` Steffen Görtz
2018-08-09 16:14 ` Stefan Hajnoczi
2018-08-06 10:01 ` [Qemu-devel] [PATCH 6/7] hw/timer/nrf51_timer: Add nRF51 Timer peripheral Steffen Görtz
` (2 subsequent siblings)
7 siblings, 1 reply; 25+ messages in thread
From: Steffen Görtz @ 2018-08-06 10:01 UTC (permalink / raw)
To: qemu-devel
Cc: Stefan Hajnoczi, Joel Stanley, Jim Mussared, Julia Suvorova,
Peter Maydell, Thomas Huth, Steffen Görtz
The test suite for the nRF51 GPIO peripheral for now
only tests initial state. Additionally a set of
tests testing an implementation detail of the model
are included.
Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
---
tests/microbit-test.c | 96 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 96 insertions(+)
diff --git a/tests/microbit-test.c b/tests/microbit-test.c
index 9f83063cf3..cc36171734 100644
--- a/tests/microbit-test.c
+++ b/tests/microbit-test.c
@@ -33,6 +33,21 @@
#define NVMC_ERASEPCR0 0x510
#define NVMC_ERASEUICR 0x514
+#define GPIO_BASE 0x50000000
+#define GPIO_OUT 0x504
+#define GPIO_OUTSET 0x508
+#define GPIO_OUTCLR 0x50C
+#define GPIO_IN 0x510
+#define GPIO_DIR 0x514
+#define GPIO_DIRSET 0x518
+#define GPIO_DIRCLR 0x51C
+#define GPIO_CNF_START 0x700
+#define GPIO_CNF_END 0x77F
+#define GPIO_PINS 32
+
+#define GPIO_PULLDOWN 1
+#define GPIO_PULLUP 3
+
static void fill_and_erase(hwaddr base, hwaddr size, uint32_t address_reg)
{
@@ -109,6 +124,86 @@ static void test_nrf51_nvmc(void)
}
}
+static void test_nrf51_gpio(void)
+{
+ size_t i;
+ uint32_t actual, expected;
+
+ struct {
+ hwaddr addr;
+ uint32_t expected;
+ } reset_state[] = {
+ {GPIO_OUT, 0x00000000}, {GPIO_OUTSET, 0x00000000},
+ {GPIO_OUTCLR, 0x00000000}, {GPIO_IN, 0x00000000},
+ {GPIO_DIR, 0x00000000}, {GPIO_DIRSET, 0x00000000},
+ {GPIO_DIRCLR, 0x00000000}
+ };
+
+ /** Check reset state **/
+ for (i = 0; i < ARRAY_SIZE(reset_state); i++) {
+ expected = reset_state[i].expected;
+ actual = readl(GPIO_BASE + reset_state[i].addr);
+ g_assert_cmpuint(actual, ==, expected);
+ }
+
+ for (i = 0; i < GPIO_PINS; i++) {
+ expected = 0x00000002;
+ actual = readl(GPIO_BASE + GPIO_CNF_START + i * 4);
+ g_assert_cmpuint(actual, ==, expected);
+ }
+
+ /** Check dir bit consistency between dir and cnf **/
+ /* Check set via DIRSET */
+ expected = 0x80000001;
+ writel(GPIO_BASE + GPIO_DIRSET, expected);
+ actual = readl(GPIO_BASE + GPIO_DIR);
+ g_assert_cmpuint(actual, ==, expected);
+ actual = readl(GPIO_BASE + GPIO_CNF_START) & 0x01;
+ g_assert_cmpuint(actual, ==, 0x01);
+ actual = readl(GPIO_BASE + GPIO_CNF_END) & 0x01;
+ g_assert_cmpuint(actual, ==, 0x01);
+
+ /* Check clear via DIRCLR */
+ writel(GPIO_BASE + GPIO_DIRCLR, 0x80000001);
+ actual = readl(GPIO_BASE + GPIO_DIR);
+ g_assert_cmpuint(actual, ==, 0x00000000);
+ actual = readl(GPIO_BASE + GPIO_CNF_START) & 0x01;
+ g_assert_cmpuint(actual, ==, 0x00);
+ actual = readl(GPIO_BASE + GPIO_CNF_END) & 0x01;
+ g_assert_cmpuint(actual, ==, 0x00);
+
+ /* Check set via DIR */
+ expected = 0x80000001;
+ writel(GPIO_BASE + GPIO_DIR, expected);
+ actual = readl(GPIO_BASE + GPIO_DIR);
+ g_assert_cmpuint(actual, ==, expected);
+ actual = readl(GPIO_BASE + GPIO_CNF_START) & 0x01;
+ g_assert_cmpuint(actual, ==, 0x01);
+ actual = readl(GPIO_BASE + GPIO_CNF_END) & 0x01;
+ g_assert_cmpuint(actual, ==, 0x01);
+
+ /* Reset DIR */
+ writel(GPIO_BASE + GPIO_DIR, 0x00000000);
+
+ /* Check Input propagates */
+ g_assert_false(true);
+
+ /* Check pull-up working */
+ g_assert_false(true);
+
+ /* Check pull-down working */
+ g_assert_false(true);
+
+ /* Check Output propagates */
+ g_assert_false(true);
+
+ /* Check self-stimulation */
+ g_assert_false(true);
+
+ /* Check short-circuit */
+ g_assert_false(true);
+}
+
int main(int argc, char **argv)
{
int ret;
@@ -118,6 +213,7 @@ int main(int argc, char **argv)
global_qtest = qtest_startf("-machine microbit");
qtest_add_func("/microbit/nrf51/nvmc", test_nrf51_nvmc);
+ qtest_add_func("/microbit/nrf51/gpio", test_nrf51_gpio);
ret = g_test_run();
--
2.18.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [Qemu-devel] [PATCH 5/7] tests/microbit-test: Add Tests for nRF51 GPIO
2018-08-06 10:01 ` [Qemu-devel] [PATCH 5/7] tests/microbit-test: Add Tests for nRF51 GPIO Steffen Görtz
@ 2018-08-09 16:14 ` Stefan Hajnoczi
0 siblings, 0 replies; 25+ messages in thread
From: Stefan Hajnoczi @ 2018-08-09 16:14 UTC (permalink / raw)
To: Steffen Görtz
Cc: qemu-devel, Joel Stanley, Jim Mussared, Julia Suvorova,
Peter Maydell, Thomas Huth
On Mon, Aug 6, 2018 at 11:01 AM, Steffen Görtz
<contrib@steffen-goertz.de> wrote:
> + /* Check Input propagates */
> + g_assert_false(true);
Each patch must compile and run successfully. This test case will
fail here. Please don't do this - even if a later patch will fill in
the missing part.
The reason for this requirement is that git-bisect(1) must work on any
commit. If the some commits don't compile or pass tests then it makes
git-bisect(1) much harder or impossible to use.
Stefan
^ permalink raw reply [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH 6/7] hw/timer/nrf51_timer: Add nRF51 Timer peripheral
2018-08-06 10:01 [Qemu-devel] [PATCH 0/7] arm: nRF51 Devices and Microbit Support Steffen Görtz
` (4 preceding siblings ...)
2018-08-06 10:01 ` [Qemu-devel] [PATCH 5/7] tests/microbit-test: Add Tests for nRF51 GPIO Steffen Görtz
@ 2018-08-06 10:01 ` Steffen Görtz
2018-08-09 16:45 ` Stefan Hajnoczi
2018-08-06 10:01 ` [Qemu-devel] [PATCH 7/7] hw/display/led_matrix: Add LED matrix display device Steffen Görtz
2018-08-06 10:09 ` [Qemu-devel] [PATCH 0/7] arm: nRF51 Devices and Microbit Support Peter Maydell
7 siblings, 1 reply; 25+ messages in thread
From: Steffen Görtz @ 2018-08-06 10:01 UTC (permalink / raw)
To: qemu-devel
Cc: Stefan Hajnoczi, Joel Stanley, Jim Mussared, Julia Suvorova,
Peter Maydell, Thomas Huth, Steffen Görtz
This patch adds the model for the nRF51 timer peripheral.
Currently, only the TIMER mode is implemented.
Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
---
hw/timer/Makefile.objs | 1 +
hw/timer/nrf51_timer.c | 382 +++++++++++++++++++++++++++++++++
hw/timer/trace-events | 5 +
include/hw/timer/nrf51_timer.h | 63 ++++++
4 files changed, 451 insertions(+)
create mode 100644 hw/timer/nrf51_timer.c
create mode 100644 include/hw/timer/nrf51_timer.h
diff --git a/hw/timer/Makefile.objs b/hw/timer/Makefile.objs
index e16b2b913c..18deca761e 100644
--- a/hw/timer/Makefile.objs
+++ b/hw/timer/Makefile.objs
@@ -23,6 +23,7 @@ common-obj-$(CONFIG_IMX) += imx_gpt.o
common-obj-$(CONFIG_LM32) += lm32_timer.o
common-obj-$(CONFIG_MILKYMIST) += milkymist-sysctl.o
common-obj-$(CONFIG_XLNX_ZYNQMP) += xlnx-zynqmp-rtc.o
+common-obj-$(CONFIG_NRF51_SOC) += nrf51_timer.o
obj-$(CONFIG_ALTERA_TIMER) += altera_timer.o
obj-$(CONFIG_EXYNOS4) += exynos4210_mct.o
diff --git a/hw/timer/nrf51_timer.c b/hw/timer/nrf51_timer.c
new file mode 100644
index 0000000000..2119426b2c
--- /dev/null
+++ b/hw/timer/nrf51_timer.c
@@ -0,0 +1,382 @@
+/*
+ * nRF51 System-on-Chip Timer peripheral
+ *
+ * Reference Manual: http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.pdf
+ * Product Spec: http://infocenter.nordicsemi.com/pdf/nRF51822_PS_v3.1.pdf
+ *
+ * Copyright 2018 Steffen Görtz <contrib@steffen-goertz.de>
+ *
+ * This code is licensed under the GPL version 2 or later. See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "hw/timer/nrf51_timer.h"
+#include "trace.h"
+
+#define NRF51_TIMER_SIZE 0x1000
+
+#define NRF51_TIMER_TASK_START 0x000
+#define NRF51_TIMER_TASK_STOP 0x004
+#define NRF51_TIMER_TASK_COUNT 0x008
+#define NRF51_TIMER_TASK_CLEAR 0x00C
+#define NRF51_TIMER_TASK_SHUTDOWN 0x010
+#define NRF51_TIMER_TASK_CAPTURE_0 0x040
+#define NRF51_TIMER_TASK_CAPTURE_3 0x04C
+
+#define NRF51_TIMER_EVENT_COMPARE_0 0x140
+#define NRF51_TIMER_EVENT_COMPARE_3 0x14C
+
+#define NRF51_TIMER_REG_SHORTS 0x200
+#define NRF51_TIMER_REG_SHORTS_MASK 0xf0f
+#define NRF51_TIMER_REG_INTENSET 0x304
+#define NRF51_TIMER_REG_INTENCLR 0x308
+#define NRF51_TIMER_REG_INTEN_MASK 0xf0000
+#define NRF51_TIMER_REG_MODE 0x504
+#define NRF51_TIMER_REG_MODE_MASK 0x01
+#define NRF51_TIMER_REG_BITMODE 0x508
+#define NRF51_TIMER_REG_BITMODE_MASK 0x03
+#define NRF51_TIMER_REG_PRESCALER 0x510
+#define NRF51_TIMER_REG_PRESCALER_MASK 0x0F
+#define NRF51_TIMER_REG_CC0 0x540
+#define NRF51_TIMER_REG_CC3 0x54C
+
+#define TIMER_CLK 16000000ULL
+
+/* Trigger */
+#define NRF51_TRIGGER_TASK 0x01
+
+/* Events */
+#define NRF51_EVENT_CLEAR 0x00
+
+static uint8_t const bitwidths[] = {16, 8, 24, 32};
+#define BWM(x) ((1UL << bitwidths[x]) - 1)
+
+static inline uint64_t ns_to_ticks(Nrf51TimerState *s, uint64_t ns)
+{
+ uint64_t t = NANOSECONDS_PER_SECOND * (1 << s->prescaler);
+ return muldiv64(ns, TIMER_CLK, t);
+}
+
+static inline uint64_t ticks_to_ns(Nrf51TimerState *s, uint64_t ticks)
+{
+ ticks *= (1 << s->prescaler);
+ return muldiv64(ticks, NANOSECONDS_PER_SECOND, TIMER_CLK);
+}
+
+static void update_irq(Nrf51TimerState *s)
+{
+ bool flag = false;
+ size_t i;
+
+ for (i = 0; i < NRF51_TIMER_REG_COUNT; i++) {
+ flag |= s->events_compare[i] && extract32(s->inten, 16 + i, 1);
+ }
+ qemu_set_irq(s->irq, flag);
+}
+
+static void update_events(Nrf51TimerState *s, uint64_t now)
+{
+ uint64_t strobe;
+ uint64_t tick;
+ uint64_t cc;
+ size_t i;
+ bool occured;
+
+ strobe = ns_to_ticks(s, now - s->last_visited);
+ tick = ns_to_ticks(s, s->last_visited - s->time_offset) & BWM(s->bitmode);
+
+ for (i = 0; i < NRF51_TIMER_REG_COUNT; i++) {
+ cc = s->cc[i];
+
+ if (tick < cc) {
+ occured = (cc - tick) <= strobe;
+ } else {
+ occured = ((cc + (1UL << bitwidths[s->bitmode])) - tick) <= strobe;
+ }
+
+ s->events_compare[i] |= occured;
+ }
+
+ s->last_visited = now;
+}
+
+static int cmpfunc(const void *a, const void *b)
+{
+ return *(uint32_t *)a - *(uint32_t *)b;
+}
+
+static uint64_t get_next_timeout(Nrf51TimerState *s, uint64_t now)
+{
+ uint64_t r;
+ size_t idx;
+
+ uint64_t tick = (ns_to_ticks(s, now - s->time_offset)) & BWM(s->bitmode);
+ int8_t next = -1;
+
+ for (idx = 0; idx < NRF51_TIMER_REG_COUNT; idx++) {
+ if (s->cc_sorted[idx] > tick) {
+ next = idx;
+ break;
+ }
+ }
+
+ if (next == -1) {
+ r = s->cc_sorted[0] + (1UL << bitwidths[s->bitmode]);
+ } else {
+ r = s->cc_sorted[next];
+ }
+
+ return now + ticks_to_ns(s, r - tick);
+}
+
+static void update_internal_state(Nrf51TimerState *s, uint64_t now)
+{
+ if (s->runstate == NRF51_TIMER_RUNNING) {
+ timer_mod(&s->timer, get_next_timeout(s, now));
+ } else {
+ timer_del(&s->timer);
+ }
+
+ update_irq(s);
+}
+
+static void timer_expire(void *opaque)
+{
+ Nrf51TimerState *s = NRF51_TIMER(opaque);
+ uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+ update_events(s, now);
+ update_internal_state(s, now);
+}
+
+static uint64_t nrf51_timer_read(void *opaque, hwaddr offset, unsigned int size)
+{
+ Nrf51TimerState *s = NRF51_TIMER(opaque);
+ uint64_t r = 0;
+
+ switch (offset) {
+ case NRF51_TIMER_EVENT_COMPARE_0 ... NRF51_TIMER_EVENT_COMPARE_3:
+ r = s->events_compare[(offset - NRF51_TIMER_EVENT_COMPARE_0) / 4];
+ break;
+ case NRF51_TIMER_REG_SHORTS:
+ r = s->shorts;
+ break;
+ case NRF51_TIMER_REG_INTENSET:
+ r = s->inten;
+ break;
+ case NRF51_TIMER_REG_INTENCLR:
+ r = s->inten;
+ break;
+ case NRF51_TIMER_REG_MODE:
+ r = s->mode;
+ break;
+ case NRF51_TIMER_REG_BITMODE:
+ r = s->bitmode;
+ break;
+ case NRF51_TIMER_REG_PRESCALER:
+ r = s->prescaler;
+ break;
+ case NRF51_TIMER_REG_CC0 ... NRF51_TIMER_REG_CC3:
+ r = s->cc[(offset - NRF51_TIMER_REG_CC0) / 4];
+ break;
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: bad read offset 0x%" HWADDR_PRIx "\n",
+ __func__, offset);
+ }
+
+ trace_nrf51_timer_read(offset, r, size);
+
+ return r;
+}
+
+static inline void update_cc_sorted(Nrf51TimerState *s)
+{
+ memcpy(s->cc_sorted, s->cc, sizeof(s->cc_sorted));
+ qsort(s->cc_sorted, NRF51_TIMER_REG_COUNT, sizeof(uint32_t), cmpfunc);
+}
+
+static void nrf51_timer_write(void *opaque, hwaddr offset,
+ uint64_t value, unsigned int size)
+{
+ Nrf51TimerState *s = NRF51_TIMER(opaque);
+ uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+ size_t idx;
+
+ trace_nrf51_timer_write(offset, value, size);
+
+ switch (offset) {
+ case NRF51_TIMER_TASK_START:
+ if (value == NRF51_TRIGGER_TASK) {
+ s->runstate = NRF51_TIMER_RUNNING;
+ }
+ break;
+ case NRF51_TIMER_TASK_STOP:
+ case NRF51_TIMER_TASK_SHUTDOWN:
+ if (value == NRF51_TRIGGER_TASK) {
+ s->runstate = NRF51_TIMER_STOPPED;
+ }
+ break;
+ case NRF51_TIMER_TASK_COUNT:
+ if (value == NRF51_TRIGGER_TASK) {
+ qemu_log_mask(LOG_UNIMP, "COUNTER mode not implemented\n");
+ }
+ break;
+ case NRF51_TIMER_TASK_CLEAR:
+ if (value == NRF51_TRIGGER_TASK) {
+ s->time_offset = now;
+ s->last_visited = now;
+ }
+ break;
+ case NRF51_TIMER_TASK_CAPTURE_0 ... NRF51_TIMER_TASK_CAPTURE_3:
+ if (value == NRF51_TRIGGER_TASK) {
+ idx = (offset - NRF51_TIMER_TASK_CAPTURE_0) / 4;
+ s->cc[idx] = ns_to_ticks(s, now - s->time_offset) & BWM(s->bitmode);
+ update_cc_sorted(s);
+ }
+ break;
+ case NRF51_TIMER_EVENT_COMPARE_0 ... NRF51_TIMER_EVENT_COMPARE_3:
+ if (value == NRF51_EVENT_CLEAR) {
+ s->events_compare[(offset - NRF51_TIMER_EVENT_COMPARE_0) / 4] = 0;
+ }
+ break;
+ case NRF51_TIMER_REG_SHORTS:
+ s->shorts = value & NRF51_TIMER_REG_SHORTS_MASK;
+ break;
+ case NRF51_TIMER_REG_INTENSET:
+ s->inten |= value & NRF51_TIMER_REG_INTEN_MASK;
+ break;
+ case NRF51_TIMER_REG_INTENCLR:
+ s->inten &= ~(value & NRF51_TIMER_REG_INTEN_MASK);
+ break;
+ case NRF51_TIMER_REG_MODE:
+ if (s->mode != NRF51_TIMER_TIMER) {
+ qemu_log_mask(LOG_UNIMP, "COUNTER mode not implemented\n");
+ return;
+ }
+ s->mode = value;
+ break;
+ case NRF51_TIMER_REG_BITMODE:
+ if (s->mode == NRF51_TIMER_TIMER && s->runstate != NRF51_TIMER_STOPPED) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: erroneous change of BITMODE while timer is running\n",
+ __func__);
+ }
+ s->bitmode = value & NRF51_TIMER_REG_BITMODE_MASK;
+ s->time_offset = now;
+ s->last_visited = now;
+ break;
+ case NRF51_TIMER_REG_PRESCALER:
+ if (s->mode == NRF51_TIMER_TIMER && s->runstate != NRF51_TIMER_STOPPED) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: erroneous change of PRESCALER while timer is running\n",
+ __func__);
+ }
+ s->prescaler = value & NRF51_TIMER_REG_PRESCALER_MASK;
+ s->time_offset = now;
+ s->last_visited = now;
+ break;
+ case NRF51_TIMER_REG_CC0 ... NRF51_TIMER_REG_CC3:
+ s->cc[(offset - NRF51_TIMER_REG_CC0) / 4] = value & BWM(s->bitmode);
+ update_cc_sorted(s);
+ break;
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: bad write offset 0x%" HWADDR_PRIx "\n",
+ __func__, offset);
+ }
+
+ update_internal_state(s, now);
+}
+
+static const MemoryRegionOps rng_ops = {
+ .read = nrf51_timer_read,
+ .write = nrf51_timer_write,
+ .endianness = DEVICE_LITTLE_ENDIAN,
+ .impl.min_access_size = 4,
+ .impl.max_access_size = 4,
+};
+
+static void nrf51_timer_init(Object *obj)
+{
+ Nrf51TimerState *s = NRF51_TIMER(obj);
+ SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+
+ memory_region_init_io(&s->iomem, obj, &rng_ops, s,
+ TYPE_NRF51_TIMER, NRF51_TIMER_SIZE);
+ sysbus_init_mmio(sbd, &s->iomem);
+ sysbus_init_irq(sbd, &s->irq);
+
+ timer_init_ns(&s->timer, QEMU_CLOCK_VIRTUAL, timer_expire, s);
+}
+
+static void nrf51_timer_reset(DeviceState *dev)
+{
+ Nrf51TimerState *s = NRF51_TIMER(dev);
+
+ s->runstate = NRF51_TIMER_STOPPED;
+
+ memset(s->events_compare, 0x00, sizeof(s->events_compare));
+ memset(s->cc, 0x00, sizeof(s->cc));
+ memset(s->cc_sorted, 0x00, sizeof(s->cc_sorted));
+ s->shorts = 0x00;
+ s->inten = 0x00;
+ s->mode = 0x00;
+ s->bitmode = 0x00;
+ s->prescaler = 0x00;
+
+ uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+ s->time_offset = now;
+ s->last_visited = now;
+}
+
+static const VMStateDescription vmstate_nrf51_timer = {
+ .name = TYPE_NRF51_TIMER,
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_TIMER(timer, Nrf51TimerState),
+ VMSTATE_UINT8(runstate, Nrf51TimerState),
+ VMSTATE_UINT64(time_offset, Nrf51TimerState),
+ VMSTATE_UINT64(last_visited, Nrf51TimerState),
+ VMSTATE_UINT8_ARRAY(events_compare, Nrf51TimerState,
+ NRF51_TIMER_REG_COUNT),
+ VMSTATE_UINT32_ARRAY(cc, Nrf51TimerState, NRF51_TIMER_REG_COUNT),
+ VMSTATE_UINT32_ARRAY(cc_sorted, Nrf51TimerState, NRF51_TIMER_REG_COUNT),
+ VMSTATE_UINT32(shorts, Nrf51TimerState),
+ VMSTATE_UINT32(inten, Nrf51TimerState),
+ VMSTATE_UINT32(mode, Nrf51TimerState),
+ VMSTATE_UINT32(bitmode, Nrf51TimerState),
+ VMSTATE_UINT32(prescaler, Nrf51TimerState),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static Property nrf51_timer_properties[] = {
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static void nrf51_timer_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->props = nrf51_timer_properties;
+ dc->reset = nrf51_timer_reset;
+ dc->vmsd = &vmstate_nrf51_timer;
+}
+
+static const TypeInfo nrf51_timer_info = {
+ .name = TYPE_NRF51_TIMER,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(Nrf51TimerState),
+ .instance_init = nrf51_timer_init,
+ .class_init = nrf51_timer_class_init
+};
+
+static void nrf51_timer_register_types(void)
+{
+ type_register_static(&nrf51_timer_info);
+}
+
+type_init(nrf51_timer_register_types)
diff --git a/hw/timer/trace-events b/hw/timer/trace-events
index e6e042fddb..91c20540a3 100644
--- a/hw/timer/trace-events
+++ b/hw/timer/trace-events
@@ -63,3 +63,8 @@ cmsdk_apb_timer_reset(void) "CMSDK APB timer: reset"
# hw/timer/xlnx-zynqmp-rtc.c
xlnx_zynqmp_rtc_gettime(int year, int month, int day, int hour, int min, int sec) "Get time from host: %d-%d-%d %2d:%02d:%02d"
+
+# hw/timer/nrf51_timer.c
+nrf51_timer_read(uint64_t addr, uint32_t value, unsigned size) "read addr 0x%" PRIx64 " data 0x%" PRIx32 " size %u"
+nrf51_timer_write(uint64_t addr, uint32_t value, unsigned size) "write addr 0x%" PRIx64 " data 0x%" PRIx32 " size %u"
+
diff --git a/include/hw/timer/nrf51_timer.h b/include/hw/timer/nrf51_timer.h
new file mode 100644
index 0000000000..a1278f17bd
--- /dev/null
+++ b/include/hw/timer/nrf51_timer.h
@@ -0,0 +1,63 @@
+/*
+* nRF51 System-on-Chip Timer peripheral
+ *
+ * Reference Manual: http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.pdf
+ * Product Spec: http://infocenter.nordicsemi.com/pdf/nRF51822_PS_v3.1.pdf
+ *
+ * QEMU interface:
+ * + sysbus MMIO regions 0: GPIO registers
+ * + sysbus irq
+ *
+ * Accuracy of the peripheral model:
+ * + Only TIMER mode is implemented, COUNTER mode is not implemented.
+ *
+ * Copyright 2018 Steffen Görtz <contrib@steffen-goertz.de>
+ *
+ * This code is licensed under the GPL version 2 or later. See
+ * the COPYING file in the top-level directory.
+ */
+#ifndef NRF51_TIMER_H
+#define NRF51_TIMER_H
+
+#include "hw/sysbus.h"
+#include "qemu/timer.h"
+#define TYPE_NRF51_TIMER "nrf51_soc.timer"
+#define NRF51_TIMER(obj) OBJECT_CHECK(Nrf51TimerState, (obj), TYPE_NRF51_TIMER)
+
+#define NRF51_TIMER_REG_COUNT 4
+
+typedef enum {
+ NRF51_TIMER_STOPPED = 0,
+ NRF51_TIMER_RUNNING
+} Nrf51TimerRunstate;
+
+typedef enum {
+ NRF51_TIMER_TIMER = 0,
+ NRF51_TIMER_COUNTER = 1
+} Nrf51TimerMode;
+
+typedef struct Nrf51TimerState {
+ SysBusDevice parent_obj;
+
+ MemoryRegion iomem;
+ qemu_irq irq;
+
+ QEMUTimer timer;
+
+ uint8_t runstate;
+
+ uint64_t time_offset;
+ uint64_t last_visited;
+
+ uint8_t events_compare[NRF51_TIMER_REG_COUNT];
+ uint32_t cc[NRF51_TIMER_REG_COUNT];
+ uint32_t cc_sorted[NRF51_TIMER_REG_COUNT];
+ uint32_t shorts;
+ uint32_t inten;
+ uint32_t mode;
+ uint32_t bitmode;
+ uint32_t prescaler;
+} Nrf51TimerState;
+
+
+#endif
--
2.18.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [Qemu-devel] [PATCH 6/7] hw/timer/nrf51_timer: Add nRF51 Timer peripheral
2018-08-06 10:01 ` [Qemu-devel] [PATCH 6/7] hw/timer/nrf51_timer: Add nRF51 Timer peripheral Steffen Görtz
@ 2018-08-09 16:45 ` Stefan Hajnoczi
0 siblings, 0 replies; 25+ messages in thread
From: Stefan Hajnoczi @ 2018-08-09 16:45 UTC (permalink / raw)
To: Steffen Görtz
Cc: qemu-devel, Joel Stanley, Jim Mussared, Julia Suvorova,
Peter Maydell, Thomas Huth
On Mon, Aug 6, 2018 at 11:01 AM, Steffen Görtz
<contrib@steffen-goertz.de> wrote:
> +/* Trigger */
> +#define NRF51_TRIGGER_TASK 0x01
> +
> +/* Events */
> +#define NRF51_EVENT_CLEAR 0x00
NRF51_TRIGGER_TASK and NRF51_EVENT_CLEAR are generic nRF51 SoC
constants. Please put them in a header to avoid duplication.
> +static void nrf51_timer_reset(DeviceState *dev)
> +{
> + Nrf51TimerState *s = NRF51_TIMER(dev);
> +
> + s->runstate = NRF51_TIMER_STOPPED;
This leaves the timer pending if it was scheduled and then reset is
called. I think timer_del() is necessary.
> diff --git a/include/hw/timer/nrf51_timer.h b/include/hw/timer/nrf51_timer.h
> new file mode 100644
> index 0000000000..a1278f17bd
> --- /dev/null
> +++ b/include/hw/timer/nrf51_timer.h
> @@ -0,0 +1,63 @@
> +/*
> +* nRF51 System-on-Chip Timer peripheral
> + *
> + * Reference Manual: http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.pdf
> + * Product Spec: http://infocenter.nordicsemi.com/pdf/nRF51822_PS_v3.1.pdf
> + *
> + * QEMU interface:
> + * + sysbus MMIO regions 0: GPIO registers
> + * + sysbus irq
> + *
> + * Accuracy of the peripheral model:
> + * + Only TIMER mode is implemented, COUNTER mode is not implemented.
This information was in a doc comment above the device state struct in
other patches you've sent. Here it's at the top of the file. For
consistency please put it in a doc comment.
> +typedef enum {
> + NRF51_TIMER_STOPPED = 0,
> + NRF51_TIMER_RUNNING
> +} Nrf51TimerRunstate;
This is basically a bool and the Nrf51TimerState->runstate field is
uint8_t. The code can be simplified by dropping the enum and changing
the field to "bool running".
> +typedef enum {
> + NRF51_TIMER_TIMER = 0,
> + NRF51_TIMER_COUNTER = 1
> +} Nrf51TimerMode;
Since Nrf51TimerMode isn't used in this header file (the
Nrf51TimerState->mode field is uint32_t), this enum is internal to the
device implementation. It can be moved to the .c file.
^ permalink raw reply [flat|nested] 25+ messages in thread
* [Qemu-devel] [PATCH 7/7] hw/display/led_matrix: Add LED matrix display device
2018-08-06 10:01 [Qemu-devel] [PATCH 0/7] arm: nRF51 Devices and Microbit Support Steffen Görtz
` (5 preceding siblings ...)
2018-08-06 10:01 ` [Qemu-devel] [PATCH 6/7] hw/timer/nrf51_timer: Add nRF51 Timer peripheral Steffen Görtz
@ 2018-08-06 10:01 ` Steffen Görtz
2018-08-09 17:08 ` Stefan Hajnoczi
2018-08-06 10:09 ` [Qemu-devel] [PATCH 0/7] arm: nRF51 Devices and Microbit Support Peter Maydell
7 siblings, 1 reply; 25+ messages in thread
From: Steffen Görtz @ 2018-08-06 10:01 UTC (permalink / raw)
To: qemu-devel
Cc: Stefan Hajnoczi, Joel Stanley, Jim Mussared, Julia Suvorova,
Peter Maydell, Thomas Huth, Steffen Görtz
The LEDs are not individually connected to the output
pins of the microcontroller. Instead, the LEDs share pins
for rows and columns.
The pattern for a row or a column is only displayed for a short moment.
The slowness of the human eye results in a complete and
flicker-free image (persistence of vision). More information
can be found here: https://en.wikipedia.org/wiki/Multiplexed_display .
This device demultiplexes the dot-matrix pattern to a grayscale
image as it would be perceived by the human eye.
The number of rows, columns and the refresh period can be configured.
At the moment it is assumed that the LEDs are connected in forward
direction from rows to columns.
The demultiplexed LEDs are drawn to the canvas of a graphics console.
In the future it is planed to send QMP events with the updated
matrix image.
Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
---
hw/display/Makefile.objs | 2 +
hw/display/led_matrix.c | 262 ++++++++++++++++++++++++++++++++
include/hw/display/led_matrix.h | 38 +++++
3 files changed, 302 insertions(+)
create mode 100644 hw/display/led_matrix.c
create mode 100644 include/hw/display/led_matrix.h
diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
index fb8408c6d0..c423f1eb1a 100644
--- a/hw/display/Makefile.objs
+++ b/hw/display/Makefile.objs
@@ -23,6 +23,8 @@ common-obj-$(CONFIG_FRAMEBUFFER) += framebuffer.o
common-obj-$(CONFIG_MILKYMIST) += milkymist-vgafb.o
common-obj-$(CONFIG_ZAURUS) += tc6393xb.o
+common-obj-$(CONFIG_NRF51_SOC) += led_matrix.o
+
common-obj-$(CONFIG_MILKYMIST_TMU2) += milkymist-tmu2.o
milkymist-tmu2.o-cflags := $(X11_CFLAGS)
milkymist-tmu2.o-libs := $(X11_LIBS)
diff --git a/hw/display/led_matrix.c b/hw/display/led_matrix.c
new file mode 100644
index 0000000000..34c3ad9bde
--- /dev/null
+++ b/hw/display/led_matrix.c
@@ -0,0 +1,262 @@
+/*
+ * LED Matrix Demultiplexer
+ *
+ * Copyright 2018 Steffen Görtz <contrib@steffen-goertz.de>
+ *
+ * This code is licensed under the GPL version 2 or later. See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qapi/error.h"
+#include "ui/console.h"
+#include "hw/display/led_matrix.h"
+#include "ui/pixel_ops.h"
+
+static bool led_was_on(LEDMatrixState *s, size_t x, size_t y)
+{
+ /* TODO implying Direction is ROW |-> COL */
+ /* TODO add direction flag and generalize */
+ bool row_level = extract64(s->row, x, 1);
+ bool col_level = extract64(s->col, y, 1);
+
+ return row_level && !col_level;
+}
+
+static void update_on_times(LEDMatrixState *s)
+{
+ size_t x;
+ int64_t now = qemu_clock_get_us(QEMU_CLOCK_VIRTUAL);
+ int64_t diff = now - s->timestamp;
+ s->timestamp = now;
+
+ for (x = 0; x < s->nrows ; x++) {
+ for (size_t y = 0; y < s->ncols; y++) {
+ if (led_was_on(s, x, y)) {
+ s->led_working_dc[x * s->ncols + y] += diff;
+ }
+ }
+ }
+}
+
+static void led_timer_expire(void *opaque)
+{
+ LEDMatrixState *s = LED_MATRIX(opaque);
+/*
+ uint8_t divider = s->strobe_row ? s->nrows : s->ncols;
+ int64_t max_on = (s->refresh_period * 1000) / divider;
+*/
+
+ update_on_times(s);
+
+ memcpy(s->led_frame_dc, s->led_working_dc,
+ sizeof(int64_t) * s->nrows * s->ncols);
+ memset(s->led_working_dc, 0x00, sizeof(int64_t) * s->nrows * s->ncols);
+
+ timer_mod(&s->timer,
+ qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + s->refresh_period);
+ s->redraw = true;
+}
+
+static void set_row(void *opaque, int line, int value)
+{
+ LEDMatrixState *s = LED_MATRIX(opaque);
+
+ update_on_times(s);
+
+ s->row = deposit32(s->row, line, 1, value > 0);
+}
+
+static void set_column(void *opaque, int line, int value)
+{
+ LEDMatrixState *s = LED_MATRIX(opaque);
+
+ update_on_times(s);
+
+ s->col = deposit32(s->col, line, 1, value > 0);
+}
+
+static void draw_pixel(DisplaySurface *ds, int x, int y, uint32_t color)
+{
+ int bpp;
+ uint8_t *d;
+ bpp = (surface_bits_per_pixel(ds) + 7) >> 3;
+ d = surface_data(ds) + surface_stride(ds) * y + bpp * x;
+ switch (bpp) {
+ case 1:
+ *((uint8_t *) d) = color;
+ d++;
+ break;
+ case 2:
+ *((uint16_t *) d) = color;
+ d += 2;
+ break;
+ case 4:
+ *((uint32_t *) d) = color;
+ d += 4;
+ break;
+ }
+}
+
+static void draw_box(DisplaySurface *ds,
+ int x0, int y0, int w, int h, uint32_t color)
+{
+ int x, y;
+ for (x = 0; x < w; x++) {
+ for (y = 0; y < h; y++) {
+ draw_pixel(ds, x0 + x, y0 + y, color);
+ }
+ }
+}
+
+typedef unsigned int (*color_func)(unsigned int, unsigned int, unsigned int);
+
+static void led_invalidate_display(void *opaque)
+{
+ LEDMatrixState *s = LED_MATRIX(opaque);
+ s->redraw = true;
+}
+
+static void led_update_display(void *opaque)
+{
+ LEDMatrixState *s = LED_MATRIX(opaque);
+ DisplaySurface *surface = qemu_console_surface(s->con);
+ color_func colorfunc;
+ uint32_t color_led;
+ int bpp;
+ uint8_t *d1;
+ uint8_t red;
+ size_t x, y, idx;
+
+ if (!s->redraw) {
+ return;
+ }
+
+ /* clear screen */
+ bpp = (surface_bits_per_pixel(surface) + 7) >> 3;
+ d1 = surface_data(surface);
+ for (y = 0; y < surface_height(surface); y++) {
+ memset(d1, 0x00, surface_width(surface) * bpp);
+ d1 += surface_stride(surface);
+ }
+
+ /* set colors according to bpp */
+ switch (surface_bits_per_pixel(surface)) {
+ case 8:
+ colorfunc = rgb_to_pixel8;
+ break;
+ case 15:
+ colorfunc = rgb_to_pixel15;
+ break;
+ case 16:
+ colorfunc = rgb_to_pixel16;
+ break;
+ case 24:
+ colorfunc = rgb_to_pixel24;
+ break;
+ case 32:
+ colorfunc = rgb_to_pixel32;
+ break;
+ default:
+ return;
+ }
+
+ for (x = 0; x < s->nrows ; x++) {
+ for (y = 0; y < s->ncols; y++) {
+ idx = x * s->ncols + y;
+ red = (s->led_frame_dc[idx] * 0xFF) / (s->refresh_period * 1000);
+ color_led = colorfunc(red, 0x00, 0x00);
+
+ draw_box(surface, idx * 10, 0, 5, 10, color_led);
+ }
+ }
+
+ s->redraw = 0;
+ dpy_gfx_update(s->con, 0, 0, 500, 500);
+}
+
+static const GraphicHwOps graphic_ops = {
+ .invalidate = led_invalidate_display,
+ .gfx_update = led_update_display,
+};
+
+static void led_matrix_init(Object *obj)
+{
+ LEDMatrixState *s = LED_MATRIX(obj);
+
+ timer_init_ms(&s->timer, QEMU_CLOCK_VIRTUAL, led_timer_expire, s);
+}
+
+static void led_matrix_realize(DeviceState *dev, Error **errp)
+{
+ LEDMatrixState *s = LED_MATRIX(dev);
+ if (!s->nrows || (s->nrows > 64)) {
+ error_setg(errp, "rows not set or larger than 64");
+ return;
+ }
+
+ if (!s->ncols || (s->ncols > 64)) {
+ error_setg(errp, "cols not set or larger than 64");
+ return;
+ }
+
+ s->led_working_dc = g_malloc0_n(s->ncols * s->nrows, sizeof(int64_t));
+ s->led_frame_dc = g_malloc0_n(s->ncols * s->nrows, sizeof(int64_t));
+
+ qdev_init_gpio_in_named(dev, set_row, "row", s->nrows);
+ qdev_init_gpio_in_named(dev, set_column, "col", s->ncols);
+
+ s->con = graphic_console_init(NULL, 0, &graphic_ops, s);
+ qemu_console_resize(s->con, 500, 500);
+}
+
+static void led_matrix_reset(DeviceState *dev)
+{
+ LEDMatrixState *s = LED_MATRIX(dev);
+
+ timer_mod(&s->timer,
+ qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + s->refresh_period);
+}
+
+static void led_matrix_unrealize(DeviceState *dev, Error **errp)
+{
+ LEDMatrixState *s = LED_MATRIX(dev);
+
+ g_free(s->led_working_dc);
+ s->led_working_dc = NULL;
+}
+
+static Property led_matrix_properties[] = {
+ DEFINE_PROP_UINT32("refresh_period", LEDMatrixState, refresh_period, 500),
+ DEFINE_PROP_UINT8("rows", LEDMatrixState, nrows, 0),
+ DEFINE_PROP_UINT8("cols", LEDMatrixState, ncols, 0),
+ DEFINE_PROP_BOOL("strobe_row", LEDMatrixState, strobe_row, true),
+ /* TODO Save/restore state of led_state matrix */
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static void led_matrix_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->props = led_matrix_properties;
+ dc->realize = led_matrix_realize;
+ dc->reset = led_matrix_reset;
+ dc->unrealize = led_matrix_unrealize;
+}
+
+static const TypeInfo led_matrix_info = {
+ .name = TYPE_LED_MATRIX,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(LEDMatrixState),
+ .instance_init = led_matrix_init,
+ .class_init = led_matrix_class_init
+};
+
+static void led_matrix_register_types(void)
+{
+ type_register_static(&led_matrix_info);
+}
+
+type_init(led_matrix_register_types)
diff --git a/include/hw/display/led_matrix.h b/include/hw/display/led_matrix.h
new file mode 100644
index 0000000000..4a43b69f5b
--- /dev/null
+++ b/include/hw/display/led_matrix.h
@@ -0,0 +1,38 @@
+/*
+ * LED Matrix Demultiplexer
+ *
+ * Copyright 2018 Steffen Görtz <contrib@steffen-goertz.de>
+ *
+ * This code is licensed under the GPL version 2 or later. See
+ * the COPYING file in the top-level directory.
+ */
+#ifndef LED_MATRIX_H
+#define LED_MATRIX_H
+
+#include "hw/sysbus.h"
+#include "qemu/timer.h"
+#define TYPE_LED_MATRIX "led_matrix"
+#define LED_MATRIX(obj) OBJECT_CHECK(LEDMatrixState, (obj), TYPE_LED_MATRIX)
+
+typedef struct LEDMatrixState {
+ SysBusDevice parent_obj;
+
+ QemuConsole *con;
+ bool redraw;
+
+ uint32_t refresh_period; /* refresh period in ms */
+ uint8_t nrows;
+ uint8_t ncols;
+ bool strobe_row;
+
+ QEMUTimer timer;
+ int64_t timestamp;
+
+ uint64_t row;
+ uint64_t col;
+ int64_t *led_working_dc; /* Current LED duty cycle acquisition */
+ int64_t *led_frame_dc; /* Last complete LED duty cycle acquisition */
+} LEDMatrixState;
+
+
+#endif
--
2.18.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [Qemu-devel] [PATCH 7/7] hw/display/led_matrix: Add LED matrix display device
2018-08-06 10:01 ` [Qemu-devel] [PATCH 7/7] hw/display/led_matrix: Add LED matrix display device Steffen Görtz
@ 2018-08-09 17:08 ` Stefan Hajnoczi
0 siblings, 0 replies; 25+ messages in thread
From: Stefan Hajnoczi @ 2018-08-09 17:08 UTC (permalink / raw)
To: Steffen Görtz
Cc: qemu-devel, Joel Stanley, Jim Mussared, Julia Suvorova,
Peter Maydell, Thomas Huth
On Mon, Aug 6, 2018 at 11:01 AM, Steffen Görtz
<contrib@steffen-goertz.de> wrote:
> +static void led_timer_expire(void *opaque)
> +{
> + LEDMatrixState *s = LED_MATRIX(opaque);
> +/*
> + uint8_t divider = s->strobe_row ? s->nrows : s->ncols;
> + int64_t max_on = (s->refresh_period * 1000) / divider;
> +*/
Please remove dead code.
> +
> + update_on_times(s);
> +
> + memcpy(s->led_frame_dc, s->led_working_dc,
> + sizeof(int64_t) * s->nrows * s->ncols);
> + memset(s->led_working_dc, 0x00, sizeof(int64_t) * s->nrows * s->ncols);
> +
> + timer_mod(&s->timer,
> + qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + s->refresh_period);
> + s->redraw = true;
> +}
Have you considered an approach based on the access pattern rather
than a timer (sampling)?
Sampling-based approaches are more CPU intensive and may result in
artifacts if the guest doesn't update pins at exactly the refresh
rate.
The access pattern approach assumes that the rows/columns are scanned
in a certain way. It uses the guest's pin accesses as the event for
checking the matrix state. It's more efficient, doesn't suffer from
sampling artifacts, but requires knowledge of how rows/columns are
scanned.
> +static void draw_pixel(DisplaySurface *ds, int x, int y, uint32_t color)
> +{
> + int bpp;
> + uint8_t *d;
> + bpp = (surface_bits_per_pixel(ds) + 7) >> 3;
> + d = surface_data(ds) + surface_stride(ds) * y + bpp * x;
> + switch (bpp) {
> + case 1:
> + *((uint8_t *) d) = color;
> + d++;
> + break;
> + case 2:
> + *((uint16_t *) d) = color;
> + d += 2;
> + break;
> + case 4:
> + *((uint32_t *) d) = color;
> + d += 4;
> + break;
> + }
There is no need to increment d.
> + for (x = 0; x < s->nrows ; x++) {
> + for (y = 0; y < s->ncols; y++) {
> + idx = x * s->ncols + y;
I'm confused by the naming here. "rows" is height/vertical/y.
"columns" is width/horizontal/x. Why is "x" used for vertical and "y"
for horizontal?
> + red = (s->led_frame_dc[idx] * 0xFF) / (s->refresh_period * 1000);
> + color_led = colorfunc(red, 0x00, 0x00);
> +
> + draw_box(surface, idx * 10, 0, 5, 10, color_led);
Are the LEDs layed out horizontally? The y coordinate is hardcoded to
0. I thought this would be a 2D LED matrix.
> +static void led_matrix_unrealize(DeviceState *dev, Error **errp)
> +{
> + LEDMatrixState *s = LED_MATRIX(dev);
> +
> + g_free(s->led_working_dc);
> + s->led_working_dc = NULL;
led_frame_dc is leaked.
graphic_console_close() is missing.
> diff --git a/include/hw/display/led_matrix.h b/include/hw/display/led_matrix.h
> new file mode 100644
> index 0000000000..4a43b69f5b
> --- /dev/null
> +++ b/include/hw/display/led_matrix.h
> @@ -0,0 +1,38 @@
> +/*
> + * LED Matrix Demultiplexer
> + *
> + * Copyright 2018 Steffen Görtz <contrib@steffen-goertz.de>
> + *
> + * This code is licensed under the GPL version 2 or later. See
> + * the COPYING file in the top-level directory.
> + */
> +#ifndef LED_MATRIX_H
> +#define LED_MATRIX_H
> +
> +#include "hw/sysbus.h"
> +#include "qemu/timer.h"
> +#define TYPE_LED_MATRIX "led_matrix"
> +#define LED_MATRIX(obj) OBJECT_CHECK(LEDMatrixState, (obj), TYPE_LED_MATRIX)
> +
> +typedef struct LEDMatrixState {
There is too little documentation here for a generic device that could
be reused by other boards. Please describe the purpose and
functionality of this device.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 0/7] arm: nRF51 Devices and Microbit Support
2018-08-06 10:01 [Qemu-devel] [PATCH 0/7] arm: nRF51 Devices and Microbit Support Steffen Görtz
` (6 preceding siblings ...)
2018-08-06 10:01 ` [Qemu-devel] [PATCH 7/7] hw/display/led_matrix: Add LED matrix display device Steffen Görtz
@ 2018-08-06 10:09 ` Peter Maydell
2018-08-06 10:31 ` Steffen Görtz
2018-08-16 16:10 ` Peter Maydell
7 siblings, 2 replies; 25+ messages in thread
From: Peter Maydell @ 2018-08-06 10:09 UTC (permalink / raw)
To: Steffen Görtz
Cc: QEMU Developers, Stefan Hajnoczi, Joel Stanley, Jim Mussared,
Julia Suvorova, Thomas Huth
On 6 August 2018 at 11:01, Steffen Görtz <contrib@steffen-goertz.de> wrote:
> This series contains additional peripheral devices for the nRF51822
> microcontroller. Furthermore it includes a device to demultiplex
> the row and column strobes used in embedded devices to drive
> 2D LED dot-matrices.
>
> Included devices:
> - Random Number Generator
> - Non-volatile Memories
> - General purpose I/O
> - Timer
>
> Microbit board-level Devices:
> - LED Matrix
>
> Instantiate of the devices is done in an upcoming patch series.
Thanks for gathering together these patches in one series.
This and the other Cortex-M0-related patchsets are on my queue
to review, but I won't be able to get to them for a little bit
as I'm currently dealing with 3.0 release work. Hopefully by
next week that will have calmed down.
thanks
-- PMM
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [Qemu-devel] [PATCH 0/7] arm: nRF51 Devices and Microbit Support
2018-08-06 10:09 ` [Qemu-devel] [PATCH 0/7] arm: nRF51 Devices and Microbit Support Peter Maydell
@ 2018-08-06 10:31 ` Steffen Görtz
2018-08-16 16:10 ` Peter Maydell
1 sibling, 0 replies; 25+ messages in thread
From: Steffen Görtz @ 2018-08-06 10:31 UTC (permalink / raw)
To: Peter Maydell, Steffen Görtz
Cc: QEMU Developers, Stefan Hajnoczi, Joel Stanley, Jim Mussared,
Julia Suvorova, Thomas Huth
>> Microbit board-level Devices:
>> - LED Matrix
>>
>> Instantiate of the devices is done in an upcoming patch series.
>
> Thanks for gathering together these patches in one series.
> This and the other Cortex-M0-related patchsets are on my queue
> to review, but I won't be able to get to them for a little bit
> as I'm currently dealing with 3.0 release work. Hopefully by
> next week that will have calmed down.
Great! Thank you!
Steffen
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Qemu-devel] [PATCH 0/7] arm: nRF51 Devices and Microbit Support
2018-08-06 10:09 ` [Qemu-devel] [PATCH 0/7] arm: nRF51 Devices and Microbit Support Peter Maydell
2018-08-06 10:31 ` Steffen Görtz
@ 2018-08-16 16:10 ` Peter Maydell
1 sibling, 0 replies; 25+ messages in thread
From: Peter Maydell @ 2018-08-16 16:10 UTC (permalink / raw)
To: Steffen Görtz
Cc: QEMU Developers, Stefan Hajnoczi, Joel Stanley, Jim Mussared,
Julia Suvorova, Thomas Huth
On 6 August 2018 at 11:09, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 6 August 2018 at 11:01, Steffen Görtz <contrib@steffen-goertz.de> wrote:
>> This series contains additional peripheral devices for the nRF51822
>> microcontroller. Furthermore it includes a device to demultiplex
>> the row and column strobes used in embedded devices to drive
>> 2D LED dot-matrices.
>>
>> Included devices:
>> - Random Number Generator
>> - Non-volatile Memories
>> - General purpose I/O
>> - Timer
>>
>> Microbit board-level Devices:
>> - LED Matrix
>>
>> Instantiate of the devices is done in an upcoming patch series.
>
> Thanks for gathering together these patches in one series.
> This and the other Cortex-M0-related patchsets are on my queue
> to review, but I won't be able to get to them for a little bit
> as I'm currently dealing with 3.0 release work. Hopefully by
> next week that will have calmed down.
I've now gone through and left some review comments. (Patches
where I haven't replied I thought were covered by Stefan's
review.)
thanks
-- PMM
^ permalink raw reply [flat|nested] 25+ messages in thread