[Qemu-devel] [PATCH v2 0/2] arm: Stub out NRF51 TWI magnetometer/accelerometer detection

[Qemu-devel] [PATCH v2 0/2] arm: Stub out NRF51 TWI magnetometer/accelerometer detection

[Stefan Hajnoczi]

v2:
 * Move stub code into a separate device [Peter]
 * Instantiate stub from microbit board instead of nRF51 SoC since this is
   microbit-specific.  Other boards using the nRF51 wouldn't necessarily want
   to see these TWI/I2C devices.
 * Add test case

This series stubs out the microbit's TWI/I2C devices so the microbit-dal
firmware boots successfully.  The accelerometer and magnetometer are probed at
startup.  If they are not found the firmware panics.

Stefan Hajnoczi (1):
  tests/microbit-test: add TWI stub device test

Steffen Görtz (1):
  arm: Stub out NRF51 TWI magnetometer/accelerometer detection

 hw/i2c/Makefile.objs          |   1 +
 include/hw/arm/nrf51.h        |   2 +
 include/hw/arm/nrf51_soc.h    |   1 +
 include/hw/i2c/microbit_i2c.h |  42 +++++++++++
 hw/arm/microbit.c             |  15 ++++
 hw/i2c/microbit_i2c.c         | 127 ++++++++++++++++++++++++++++++++++
 tests/microbit-test.c         |  44 ++++++++++++
 7 files changed, 232 insertions(+)
 create mode 100644 include/hw/i2c/microbit_i2c.h
 create mode 100644 hw/i2c/microbit_i2c.c

-- 
2.20.1

[Qemu-devel] [PATCH v2 1/2] arm: Stub out NRF51 TWI magnetometer/accelerometer detection

[Stefan Hajnoczi]

From: Steffen Görtz <contrib@steffen-goertz.de>

Recent microbit firmwares panic if the TWI magnetometer/accelerometer
devices are not detected during startup.  We don't implement TWI (I2C)
so let's stub out these devices just to let the firmware boot.

Signed-off by: Steffen Görtz <contrib@steffen-goertz.de>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/i2c/Makefile.objs          |   1 +
 include/hw/arm/nrf51.h        |   2 +
 include/hw/arm/nrf51_soc.h    |   1 +
 include/hw/i2c/microbit_i2c.h |  42 +++++++++++
 hw/arm/microbit.c             |  15 ++++
 hw/i2c/microbit_i2c.c         | 127 ++++++++++++++++++++++++++++++++++
 6 files changed, 188 insertions(+)
 create mode 100644 include/hw/i2c/microbit_i2c.h
 create mode 100644 hw/i2c/microbit_i2c.c

diff --git a/hw/i2c/Makefile.objs b/hw/i2c/Makefile.objs
index 37cacde978..82e747e1cd 100644
--- a/hw/i2c/Makefile.objs
+++ b/hw/i2c/Makefile.objs
@@ -7,5 +7,6 @@ common-obj-$(CONFIG_BITBANG_I2C) += bitbang_i2c.o
 common-obj-$(CONFIG_EXYNOS4) += exynos4210_i2c.o
 common-obj-$(CONFIG_IMX_I2C) += imx_i2c.o
 common-obj-$(CONFIG_ASPEED_SOC) += aspeed_i2c.o
+common-obj-$(CONFIG_NRF51_SOC) += microbit_i2c.o
 obj-$(CONFIG_OMAP) += omap_i2c.o
 obj-$(CONFIG_PPC4XX) += ppc4xx_i2c.o
diff --git a/include/hw/arm/nrf51.h b/include/hw/arm/nrf51.h
index 175bb6c301..1008fee6c9 100644
--- a/include/hw/arm/nrf51.h
+++ b/include/hw/arm/nrf51.h
@@ -25,6 +25,8 @@
 #define NRF51_IOMEM_SIZE      0x20000000
 
 #define NRF51_UART_BASE       0x40002000
+#define NRF51_TWI_BASE        0x40003000
+#define NRF51_TWI_SIZE        0x00001000
 #define NRF51_TIMER_BASE      0x40008000
 #define NRF51_TIMER_SIZE      0x00001000
 #define NRF51_RNG_BASE        0x4000D000
diff --git a/include/hw/arm/nrf51_soc.h b/include/hw/arm/nrf51_soc.h
index e06f0304b4..fbdefc07e4 100644
--- a/include/hw/arm/nrf51_soc.h
+++ b/include/hw/arm/nrf51_soc.h
@@ -39,6 +39,7 @@ typedef struct NRF51State {
     MemoryRegion sram;
     MemoryRegion flash;
     MemoryRegion clock;
+    MemoryRegion twi;
 
     uint32_t sram_size;
     uint32_t flash_size;
diff --git a/include/hw/i2c/microbit_i2c.h b/include/hw/i2c/microbit_i2c.h
new file mode 100644
index 0000000000..aad636127e
--- /dev/null
+++ b/include/hw/i2c/microbit_i2c.h
@@ -0,0 +1,42 @@
+/*
+ * Microbit stub for Nordic Semiconductor nRF51 SoC Two-Wire Interface
+ * http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.1.pdf
+ *
+ * Copyright 2019 Red Hat, Inc.
+ *
+ * This code is licensed under the GPL version 2 or later.  See
+ * the COPYING file in the top-level directory.
+ */
+
+#ifndef MICROBIT_I2C_H
+#define MICROBIT_I2C_H
+
+#include "hw/sysbus.h"
+#include "hw/arm/nrf51.h"
+
+#define NRF51_TWI_TASK_STARTRX 0x000
+#define NRF51_TWI_TASK_STARTTX 0x008
+#define NRF51_TWI_TASK_STOP 0x014
+#define NRF51_TWI_EVENT_STOPPED 0x104
+#define NRF51_TWI_EVENT_RXDREADY 0x108
+#define NRF51_TWI_EVENT_TXDSENT 0x11c
+#define NRF51_TWI_REG_ENABLE 0x500
+#define NRF51_TWI_REG_RXD 0x518
+#define NRF51_TWI_REG_TXD 0x51c
+#define NRF51_TWI_REG_ADDRESS 0x588
+
+#define TYPE_MICROBIT_I2C "microbit.i2c"
+#define MICROBIT_I2C(obj) \
+    OBJECT_CHECK(MicrobitI2CState, (obj), TYPE_MICROBIT_I2C)
+
+#define MICROBIT_I2C_NREGS (NRF51_TWI_SIZE / sizeof(uint32_t))
+
+typedef struct {
+    SysBusDevice parent_obj;
+
+    MemoryRegion iomem;
+    uint32_t regs[MICROBIT_I2C_NREGS];
+    uint32_t read_idx;
+} MicrobitI2CState;
+
+#endif /* MICROBIT_I2C_H */
diff --git a/hw/arm/microbit.c b/hw/arm/microbit.c
index a734e7f650..84d1b8dd2b 100644
--- a/hw/arm/microbit.c
+++ b/hw/arm/microbit.c
@@ -16,11 +16,13 @@
 #include "exec/address-spaces.h"
 
 #include "hw/arm/nrf51_soc.h"
+#include "hw/i2c/microbit_i2c.h"
 
 typedef struct {
     MachineState parent;
 
     NRF51State nrf51;
+    MicrobitI2CState i2c;
 } MicrobitMachineState;
 
 #define TYPE_MICROBIT_MACHINE MACHINE_TYPE_NAME("microbit")
@@ -32,7 +34,9 @@ static void microbit_init(MachineState *machine)
 {
     MicrobitMachineState *s = MICROBIT_MACHINE(machine);
     MemoryRegion *system_memory = get_system_memory();
+    MemoryRegion *mr;
     Object *soc = OBJECT(&s->nrf51);
+    Object *i2c = OBJECT(&s->i2c);
 
     sysbus_init_child_obj(OBJECT(machine), "nrf51", soc, sizeof(s->nrf51),
                           TYPE_NRF51_SOC);
@@ -41,6 +45,17 @@ static void microbit_init(MachineState *machine)
                              &error_fatal);
     object_property_set_bool(soc, true, "realized", &error_fatal);
 
+    /* Overlap the TWI stub device into the SoC.  This is a microbit-specific
+     * hack until we implement the nRF51 TWI controller properly and the
+     * magnetometer/accelerometer devices.
+     */
+    sysbus_init_child_obj(OBJECT(machine), "microbit.twi", i2c,
+                          sizeof(s->i2c), TYPE_MICROBIT_I2C);
+    object_property_set_bool(i2c, true, "realized", &error_fatal);
+    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(i2c), 0);
+    memory_region_add_subregion_overlap(&s->nrf51.container, NRF51_TWI_BASE,
+                                        mr, -1);
+
     armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename,
                        NRF51_SOC(soc)->flash_size);
 }
diff --git a/hw/i2c/microbit_i2c.c b/hw/i2c/microbit_i2c.c
new file mode 100644
index 0000000000..793f1b0f8b
--- /dev/null
+++ b/hw/i2c/microbit_i2c.c
@@ -0,0 +1,127 @@
+/*
+ * Microbit stub for Nordic Semiconductor nRF51 SoC Two-Wire Interface
+ * http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.1.pdf
+ *
+ * This is a microbit-specific stub for the TWI controller on the nRF51 SoC.
+ * We don't emulate I2C devices but the firmware probes the
+ * accelerometer/magnetometer on startup and panics if they are not found.
+ * Therefore we stub out the probing.
+ *
+ * In the future this file could evolve into a full nRF51 TWI controller
+ * device.
+ *
+ * Copyright 2018 Steffen Görtz <contrib@steffen-goertz.de>
+ * Copyright 2019 Red Hat, Inc.
+ *
+ * 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/i2c/microbit_i2c.h"
+
+static const uint32_t twi_read_sequence[] = {0x5A, 0x5A, 0x40};
+
+static uint64_t microbit_i2c_read(void *opaque, hwaddr addr, unsigned int size)
+{
+    MicrobitI2CState *s = opaque;
+    uint64_t data = 0x00;
+
+    switch (addr) {
+    case NRF51_TWI_EVENT_STOPPED:
+        data = 0x01;
+        break;
+    case NRF51_TWI_EVENT_RXDREADY:
+        data = 0x01;
+        break;
+    case NRF51_TWI_EVENT_TXDSENT:
+        data = 0x01;
+        break;
+    case NRF51_TWI_REG_RXD:
+        data = twi_read_sequence[s->read_idx];
+        if (s->read_idx < G_N_ELEMENTS(twi_read_sequence)) {
+            s->read_idx++;
+        }
+        break;
+    default:
+        data = s->regs[addr / sizeof(s->regs[0])];
+        break;
+    }
+
+    qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " [%u] = %" PRIx32 "\n",
+                  __func__, addr, size, (uint32_t)data);
+
+
+    return data;
+}
+
+static void microbit_i2c_write(void *opaque, hwaddr addr, uint64_t data,
+                               unsigned int size)
+{
+    MicrobitI2CState *s = opaque;
+
+    qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " <- 0x%" PRIx64 " [%u]\n",
+                  __func__, addr, data, size);
+    s->regs[addr / sizeof(s->regs[0])] = data;
+}
+
+static const MemoryRegionOps microbit_i2c_ops = {
+    .read = microbit_i2c_read,
+    .write = microbit_i2c_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl.min_access_size = 4,
+    .impl.max_access_size = 4,
+};
+
+static const VMStateDescription microbit_i2c_vmstate = {
+    .name = TYPE_MICROBIT_I2C,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_ARRAY(regs, MicrobitI2CState, MICROBIT_I2C_NREGS),
+        VMSTATE_UINT32(read_idx, MicrobitI2CState),
+    },
+};
+
+static void microbit_i2c_reset(DeviceState *dev)
+{
+    MicrobitI2CState *s = MICROBIT_I2C(dev);
+
+    memset(s->regs, 0, sizeof(s->regs));
+    s->read_idx = 0;
+}
+
+static void microbit_i2c_realize(DeviceState *dev, Error **errp)
+{
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+    MicrobitI2CState *s = MICROBIT_I2C(dev);
+
+    memory_region_init_io(&s->iomem, OBJECT(s), &microbit_i2c_ops, s,
+                          "microbit.twi", NRF51_TWI_SIZE);
+    sysbus_init_mmio(sbd, &s->iomem);
+}
+
+static void microbit_i2c_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->vmsd = &microbit_i2c_vmstate;
+    dc->reset = microbit_i2c_reset;
+    dc->realize = microbit_i2c_realize;
+    dc->desc = "Microbit I2C controller";
+}
+
+static const TypeInfo microbit_i2c_info = {
+    .name = TYPE_MICROBIT_I2C,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(MicrobitI2CState),
+    .class_init = microbit_i2c_class_init,
+};
+
+static void microbit_i2c_register_types(void)
+{
+    type_register_static(&microbit_i2c_info);
+}
+
+type_init(microbit_i2c_register_types)
-- 
2.20.1

[Qemu-devel] [PATCH v2 2/2] tests/microbit-test: add TWI stub device test

[Stefan Hajnoczi]

This test verifies that we read back the expected I2C WHO_AM_I register
values for the accelerometer/magnetometer.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/microbit-test.c | 44 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/tests/microbit-test.c b/tests/microbit-test.c
index 0c125535f6..dcdc0cd41a 100644
--- a/tests/microbit-test.c
+++ b/tests/microbit-test.c
@@ -21,6 +21,49 @@
 #include "hw/arm/nrf51.h"
 #include "hw/gpio/nrf51_gpio.h"
 #include "hw/timer/nrf51_timer.h"
+#include "hw/i2c/microbit_i2c.h"
+
+/* Read a byte from I2C device at @addr from register @reg */
+static uint32_t i2c_read_byte(uint32_t addr, uint32_t reg)
+{
+    uint32_t val;
+
+    writel(NRF51_TWI_BASE + NRF51_TWI_REG_ADDRESS, addr);
+    writel(NRF51_TWI_BASE + NRF51_TWI_TASK_STARTTX, 1);
+    writel(NRF51_TWI_BASE + NRF51_TWI_REG_TXD, reg);
+    val = readl(NRF51_TWI_BASE + NRF51_TWI_EVENT_TXDSENT);
+    g_assert_cmpuint(val, ==, 1);
+    writel(NRF51_TWI_BASE + NRF51_TWI_TASK_STOP, 1);
+
+    writel(NRF51_TWI_BASE + NRF51_TWI_TASK_STARTRX, 1);
+    val = readl(NRF51_TWI_BASE + NRF51_TWI_EVENT_RXDREADY);
+    g_assert_cmpuint(val, ==, 1);
+    val = readl(NRF51_TWI_BASE + NRF51_TWI_REG_RXD);
+    writel(NRF51_TWI_BASE + NRF51_TWI_TASK_STOP, 1);
+
+    return val;
+}
+
+static void test_microbit_i2c(void)
+{
+    uint32_t val;
+
+    /* We don't program pins/irqs but at least enable the device */
+    writel(NRF51_TWI_BASE + NRF51_TWI_REG_ENABLE, 5);
+
+    /* MMA8653 magnetometer detection */
+    val = i2c_read_byte(0x3A, 0x0D);
+    g_assert_cmpuint(val, ==, 0x5A);
+
+    val = i2c_read_byte(0x3A, 0x0D);
+    g_assert_cmpuint(val, ==, 0x5A);
+
+    /* LSM303 accelerometer detection */
+    val = i2c_read_byte(0x3C, 0x4F);
+    g_assert_cmpuint(val, ==, 0x40);
+
+    writel(NRF51_TWI_BASE + NRF51_TWI_REG_ENABLE, 0);
+}
 
 static void test_nrf51_gpio(void)
 {
@@ -247,6 +290,7 @@ int main(int argc, char **argv)
 
     qtest_add_func("/microbit/nrf51/gpio", test_nrf51_gpio);
     qtest_add_func("/microbit/nrf51/timer", test_nrf51_timer);
+    qtest_add_func("/microbit/microbit/i2c", test_microbit_i2c);
 
     ret = g_test_run();
 
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 2/2] tests/microbit-test: add TWI stub device test

[Thomas Huth]

On 2019-01-10 10:40, Stefan Hajnoczi wrote:
> This test verifies that we read back the expected I2C WHO_AM_I register
> values for the accelerometer/magnetometer.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/microbit-test.c | 44 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/tests/microbit-test.c b/tests/microbit-test.c
> index 0c125535f6..dcdc0cd41a 100644
> --- a/tests/microbit-test.c
> +++ b/tests/microbit-test.c
> @@ -21,6 +21,49 @@
>  #include "hw/arm/nrf51.h"
>  #include "hw/gpio/nrf51_gpio.h"
>  #include "hw/timer/nrf51_timer.h"
> +#include "hw/i2c/microbit_i2c.h"
> +
> +/* Read a byte from I2C device at @addr from register @reg */
> +static uint32_t i2c_read_byte(uint32_t addr, uint32_t reg)
> +{
> +    uint32_t val;
> +
> +    writel(NRF51_TWI_BASE + NRF51_TWI_REG_ADDRESS, addr);
> +    writel(NRF51_TWI_BASE + NRF51_TWI_TASK_STARTTX, 1);
> +    writel(NRF51_TWI_BASE + NRF51_TWI_REG_TXD, reg);
> +    val = readl(NRF51_TWI_BASE + NRF51_TWI_EVENT_TXDSENT);
> +    g_assert_cmpuint(val, ==, 1);
> +    writel(NRF51_TWI_BASE + NRF51_TWI_TASK_STOP, 1);
> +
> +    writel(NRF51_TWI_BASE + NRF51_TWI_TASK_STARTRX, 1);
> +    val = readl(NRF51_TWI_BASE + NRF51_TWI_EVENT_RXDREADY);
> +    g_assert_cmpuint(val, ==, 1);
> +    val = readl(NRF51_TWI_BASE + NRF51_TWI_REG_RXD);
> +    writel(NRF51_TWI_BASE + NRF51_TWI_TASK_STOP, 1);

Any chance that you could use qemu_writel() right from the start here?
That will make it easier to finally get rid of global_qtest one day...

 Thomas

Re: [Qemu-devel] [PATCH v2 2/2] tests/microbit-test: add TWI stub device test

[Julia Suvorova]

On 10.01.2019 12:48, Thomas Huth wrote:
> On 2019-01-10 10:40, Stefan Hajnoczi wrote:
>> This test verifies that we read back the expected I2C WHO_AM_I register
>> values for the accelerometer/magnetometer.
>>
>> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>   tests/microbit-test.c | 44 +++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 44 insertions(+)
>>
>> diff --git a/tests/microbit-test.c b/tests/microbit-test.c
>> index 0c125535f6..dcdc0cd41a 100644
>> --- a/tests/microbit-test.c
>> +++ b/tests/microbit-test.c
>> @@ -21,6 +21,49 @@
>>   #include "hw/arm/nrf51.h"
>>   #include "hw/gpio/nrf51_gpio.h"
>>   #include "hw/timer/nrf51_timer.h"
>> +#include "hw/i2c/microbit_i2c.h"
>> +
>> +/* Read a byte from I2C device at @addr from register @reg */
>> +static uint32_t i2c_read_byte(uint32_t addr, uint32_t reg)
>> +{
>> +    uint32_t val;
>> +
>> +    writel(NRF51_TWI_BASE + NRF51_TWI_REG_ADDRESS, addr);
>> +    writel(NRF51_TWI_BASE + NRF51_TWI_TASK_STARTTX, 1);
>> +    writel(NRF51_TWI_BASE + NRF51_TWI_REG_TXD, reg);
>> +    val = readl(NRF51_TWI_BASE + NRF51_TWI_EVENT_TXDSENT);
>> +    g_assert_cmpuint(val, ==, 1);
>> +    writel(NRF51_TWI_BASE + NRF51_TWI_TASK_STOP, 1);
>> +
>> +    writel(NRF51_TWI_BASE + NRF51_TWI_TASK_STARTRX, 1);
>> +    val = readl(NRF51_TWI_BASE + NRF51_TWI_EVENT_RXDREADY);
>> +    g_assert_cmpuint(val, ==, 1);
>> +    val = readl(NRF51_TWI_BASE + NRF51_TWI_REG_RXD);
>> +    writel(NRF51_TWI_BASE + NRF51_TWI_TASK_STOP, 1);
> 
> Any chance that you could use qemu_writel() right from the start here?
> That will make it easier to finally get rid of global_qtest one day...

I'll send a patch soon to fix it.
But this test can be merged without corrections.

Best regards, Julia Suvorova.

Re: [Qemu-devel] [PATCH v2 2/2] tests/microbit-test: add TWI stub device test

[Stefan Hajnoczi]

On Thu, Jan 10, 2019 at 10:12 AM Julia Suvorova via Qemu-devel
<qemu-devel@nongnu.org> wrote:
> On 10.01.2019 12:48, Thomas Huth wrote:
> > On 2019-01-10 10:40, Stefan Hajnoczi wrote:
> > Any chance that you could use qemu_writel() right from the start here?
> > That will make it easier to finally get rid of global_qtest one day...
>
> I'll send a patch soon to fix it.
> But this test can be merged without corrections.

Thank you, Julia!

Stefan

Re: [Qemu-devel] [PATCH v2 1/2] arm: Stub out NRF51 TWI magnetometer/accelerometer detection

[Peter Maydell]

On Thu, 10 Jan 2019 at 09:40, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> From: Steffen Görtz <contrib@steffen-goertz.de>
>
> Recent microbit firmwares panic if the TWI magnetometer/accelerometer
> devices are not detected during startup.  We don't implement TWI (I2C)
> so let's stub out these devices just to let the firmware boot.
>

> --- a/hw/arm/microbit.c
> +++ b/hw/arm/microbit.c
> @@ -16,11 +16,13 @@
>  #include "exec/address-spaces.h"
>
>  #include "hw/arm/nrf51_soc.h"
> +#include "hw/i2c/microbit_i2c.h"
>
>  typedef struct {
>      MachineState parent;
>
>      NRF51State nrf51;
> +    MicrobitI2CState i2c;
>  } MicrobitMachineState;
>
>  #define TYPE_MICROBIT_MACHINE MACHINE_TYPE_NAME("microbit")
> @@ -32,7 +34,9 @@ static void microbit_init(MachineState *machine)
>  {
>      MicrobitMachineState *s = MICROBIT_MACHINE(machine);
>      MemoryRegion *system_memory = get_system_memory();
> +    MemoryRegion *mr;
>      Object *soc = OBJECT(&s->nrf51);
> +    Object *i2c = OBJECT(&s->i2c);
>
>      sysbus_init_child_obj(OBJECT(machine), "nrf51", soc, sizeof(s->nrf51),
>                            TYPE_NRF51_SOC);
> @@ -41,6 +45,17 @@ static void microbit_init(MachineState *machine)
>                               &error_fatal);
>      object_property_set_bool(soc, true, "realized", &error_fatal);
>
> +    /* Overlap the TWI stub device into the SoC.  This is a microbit-specific
> +     * hack until we implement the nRF51 TWI controller properly and the
> +     * magnetometer/accelerometer devices.
> +     */
> +    sysbus_init_child_obj(OBJECT(machine), "microbit.twi", i2c,
> +                          sizeof(s->i2c), TYPE_MICROBIT_I2C);
> +    object_property_set_bool(i2c, true, "realized", &error_fatal);
> +    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(i2c), 0);
> +    memory_region_add_subregion_overlap(&s->nrf51.container, NRF51_TWI_BASE,
> +                                        mr, -1);

This is an SoC device, right? Why are we creating it here
in the board code rather than in the SoC object's code ?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/2] arm: Stub out NRF51 TWI magnetometer/accelerometer detection

[Stefan Hajnoczi]

On Fri, Jan 18, 2019 at 1:59 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 10 Jan 2019 at 09:40, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > From: Steffen Görtz <contrib@steffen-goertz.de>
> >
> > Recent microbit firmwares panic if the TWI magnetometer/accelerometer
> > devices are not detected during startup.  We don't implement TWI (I2C)
> > so let's stub out these devices just to let the firmware boot.
> >
>
> > --- a/hw/arm/microbit.c
> > +++ b/hw/arm/microbit.c
> > @@ -16,11 +16,13 @@
> >  #include "exec/address-spaces.h"
> >
> >  #include "hw/arm/nrf51_soc.h"
> > +#include "hw/i2c/microbit_i2c.h"
> >
> >  typedef struct {
> >      MachineState parent;
> >
> >      NRF51State nrf51;
> > +    MicrobitI2CState i2c;
> >  } MicrobitMachineState;
> >
> >  #define TYPE_MICROBIT_MACHINE MACHINE_TYPE_NAME("microbit")
> > @@ -32,7 +34,9 @@ static void microbit_init(MachineState *machine)
> >  {
> >      MicrobitMachineState *s = MICROBIT_MACHINE(machine);
> >      MemoryRegion *system_memory = get_system_memory();
> > +    MemoryRegion *mr;
> >      Object *soc = OBJECT(&s->nrf51);
> > +    Object *i2c = OBJECT(&s->i2c);
> >
> >      sysbus_init_child_obj(OBJECT(machine), "nrf51", soc, sizeof(s->nrf51),
> >                            TYPE_NRF51_SOC);
> > @@ -41,6 +45,17 @@ static void microbit_init(MachineState *machine)
> >                               &error_fatal);
> >      object_property_set_bool(soc, true, "realized", &error_fatal);
> >
> > +    /* Overlap the TWI stub device into the SoC.  This is a microbit-specific
> > +     * hack until we implement the nRF51 TWI controller properly and the
> > +     * magnetometer/accelerometer devices.
> > +     */
> > +    sysbus_init_child_obj(OBJECT(machine), "microbit.twi", i2c,
> > +                          sizeof(s->i2c), TYPE_MICROBIT_I2C);
> > +    object_property_set_bool(i2c, true, "realized", &error_fatal);
> > +    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(i2c), 0);
> > +    memory_region_add_subregion_overlap(&s->nrf51.container, NRF51_TWI_BASE,
> > +                                        mr, -1);
>
> This is an SoC device, right? Why are we creating it here
> in the board code rather than in the SoC object's code ?

Yes, this is a microbit-specific hack.  We don't implement a full
TWI/I2C bus for the nRF51 SoC.  This implementation is just aimed at
stubbing out the microbit-specific accelerometer/magnetometer devices.

Therefore I decided it was cleanest to place it in the microbit
machine type instead of dirtying the nRF51 SoC with a microbit hack.

If the I2C passthrough GSoC 2019 project happens we'll get proper
nRF51 TWI/I2C controller emulation and can eliminate this stub.  But
for now this allows the microbit to boot and doesn't mess up any other
potential machine types that will use the nRF51 SoC.

Stefan

Re: [Qemu-devel] [PATCH v2 0/2] arm: Stub out NRF51 TWI magnetometer/accelerometer detection

[Peter Maydell]

On Thu, 10 Jan 2019 at 09:40, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> v2:
>  * Move stub code into a separate device [Peter]
>  * Instantiate stub from microbit board instead of nRF51 SoC since this is
>    microbit-specific.  Other boards using the nRF51 wouldn't necessarily want
>    to see these TWI/I2C devices.
>  * Add test case
>
> This series stubs out the microbit's TWI/I2C devices so the microbit-dal
> firmware boots successfully.  The accelerometer and magnetometer are probed at
> startup.  If they are not found the firmware panics.
>
> Stefan Hajnoczi (1):
>   tests/microbit-test: add TWI stub device test
>
> Steffen Görtz (1):
>   arm: Stub out NRF51 TWI magnetometer/accelerometer detection
>
>  hw/i2c/Makefile.objs          |   1 +
>  include/hw/arm/nrf51.h        |   2 +
>  include/hw/arm/nrf51_soc.h    |   1 +
>  include/hw/i2c/microbit_i2c.h |  42 +++++++++++
>  hw/arm/microbit.c             |  15 ++++
>  hw/i2c/microbit_i2c.c         | 127 ++++++++++++++++++++++++++++++++++
>  tests/microbit-test.c         |  44 ++++++++++++
>  7 files changed, 232 insertions(+)
>  create mode 100644 include/hw/i2c/microbit_i2c.h
>  create mode 100644 hw/i2c/microbit_i2c.c

Applied to target-arm.next, thanks. Can you provide a
MAINTAINERS file update for the new files, please?
(You can send that separately or stick it in another
nrf51 series if you're sending one.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 0/2] arm: Stub out NRF51 TWI magnetometer/accelerometer detection

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1591 bytes --]

On Mon, Jan 21, 2019 at 04:21:03PM +0000, Peter Maydell wrote:
> On Thu, 10 Jan 2019 at 09:40, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > v2:
> >  * Move stub code into a separate device [Peter]
> >  * Instantiate stub from microbit board instead of nRF51 SoC since this is
> >    microbit-specific.  Other boards using the nRF51 wouldn't necessarily want
> >    to see these TWI/I2C devices.
> >  * Add test case
> >
> > This series stubs out the microbit's TWI/I2C devices so the microbit-dal
> > firmware boots successfully.  The accelerometer and magnetometer are probed at
> > startup.  If they are not found the firmware panics.
> >
> > Stefan Hajnoczi (1):
> >   tests/microbit-test: add TWI stub device test
> >
> > Steffen Görtz (1):
> >   arm: Stub out NRF51 TWI magnetometer/accelerometer detection
> >
> >  hw/i2c/Makefile.objs          |   1 +
> >  include/hw/arm/nrf51.h        |   2 +
> >  include/hw/arm/nrf51_soc.h    |   1 +
> >  include/hw/i2c/microbit_i2c.h |  42 +++++++++++
> >  hw/arm/microbit.c             |  15 ++++
> >  hw/i2c/microbit_i2c.c         | 127 ++++++++++++++++++++++++++++++++++
> >  tests/microbit-test.c         |  44 ++++++++++++
> >  7 files changed, 232 insertions(+)
> >  create mode 100644 include/hw/i2c/microbit_i2c.h
> >  create mode 100644 hw/i2c/microbit_i2c.c
> 
> Applied to target-arm.next, thanks. Can you provide a
> MAINTAINERS file update for the new files, please?
> (You can send that separately or stick it in another
> nrf51 series if you're sending one.)

Sure, will do.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]