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

Stefan Hajnoczi posted 1 patch 5 years, 2 months ago
Test asan passed
Test checkpatch passed
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
Test docker-clang@ubuntu passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190105144429.8702-1-stefanha@redhat.com
There is a newer version of this series
include/hw/arm/nrf51.h     |  1 +
include/hw/arm/nrf51_soc.h |  1 +
hw/arm/nrf51_soc.c         | 62 ++++++++++++++++++++++++++++++++++++++
3 files changed, 64 insertions(+)
[Qemu-devel] [PATCH] arm: Stub out NRF51 TWI magnetometer/accelerometer detection
Posted by Stefan Hajnoczi 5 years, 2 months ago
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: Stefan Hajnoczi <stefanha@redhat.com>
Based-on: <20190103091119.9367-1-stefanha@redhat.com>
---
Steffen: Please post your Signed-off-by.  I did some minor cleanups so
your patch can be merged.  Thanks!

 include/hw/arm/nrf51.h     |  1 +
 include/hw/arm/nrf51_soc.h |  1 +
 hw/arm/nrf51_soc.c         | 62 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 64 insertions(+)

diff --git a/include/hw/arm/nrf51.h b/include/hw/arm/nrf51.h
index 175bb6c301..5411121b66 100644
--- a/include/hw/arm/nrf51.h
+++ b/include/hw/arm/nrf51.h
@@ -25,6 +25,7 @@
 #define NRF51_IOMEM_SIZE      0x20000000
 
 #define NRF51_UART_BASE       0x40002000
+#define NRF51_TWI_BASE        0x40003000
 #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/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
index 1630c27594..265438f916 100644
--- a/hw/arm/nrf51_soc.c
+++ b/hw/arm/nrf51_soc.c
@@ -32,6 +32,11 @@
 #define NRF51822_FLASH_SIZE     (256 * NRF51_PAGE_SIZE)
 #define NRF51822_SRAM_SIZE      (16 * NRF51_PAGE_SIZE)
 
+#define NRF51_TWI_EVENT_STOPPED 0x104
+#define NRF51_TWI_EVENT_RXDREADY 0x108
+#define NRF51_TWI_EVENT_TXDSENT 0x11c
+#define NRF51_TWI_REG_RXD 0x518
+
 #define BASE_TO_IRQ(base) ((base >> 12) & 0x1F)
 
 static uint64_t clock_read(void *opaque, hwaddr addr, unsigned int size)
@@ -53,6 +58,58 @@ static const MemoryRegionOps clock_ops = {
     .write = clock_write
 };
 
+/* Two-Wire Interface (TWI) is not implemented but the microbit-dal firmware
+ * panics if the TWI accelerometer and magnetometer WHO_AM_I registers cannot
+ * be read.  Stub out this read sequence so microbit-dal starts up.
+ */
+static uint32_t twi_read_sequence[] = {0x5A, 0x5A, 0x40};
+static uint32_t twi_regs[0x1000 / 4];
+
+static uint64_t twi_read(void *opaque, hwaddr addr, unsigned int size)
+{
+    static int i;
+    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[i];
+        if (i < ARRAY_SIZE(twi_read_sequence)) {
+            i++;
+        }
+        break;
+    default:
+        data = twi_regs[addr / 4];
+        break;
+    }
+
+    qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " [%u] = %" PRIx32 "\n",
+                  __func__, addr, size, (uint32_t)data);
+
+
+    return data;
+}
+
+static void twi_write(void *opaque, hwaddr addr, uint64_t data,
+                      unsigned int size)
+{
+    qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " <- 0x%" PRIx64 " [%u]\n",
+                  __func__, addr, data, size);
+    twi_regs[addr / 4] = data;
+}
+
+static const MemoryRegionOps twi_ops = {
+    .read = twi_read,
+    .write = twi_write
+};
 
 static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
 {
@@ -156,6 +213,11 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
     memory_region_add_subregion_overlap(&s->container,
                                         NRF51_IOMEM_BASE, &s->clock, -1);
 
+    memory_region_init_io(&s->twi, NULL, &twi_ops, NULL,
+                          "nrf51_soc.twi", 0x1000);
+    memory_region_add_subregion_overlap(&s->container,
+                                        NRF51_TWI_BASE, &s->twi, -1);
+
     create_unimplemented_device("nrf51_soc.io", NRF51_IOMEM_BASE,
                                 NRF51_IOMEM_SIZE);
     create_unimplemented_device("nrf51_soc.ficr", NRF51_FICR_BASE,
-- 
2.20.1


Re: [Qemu-devel] [PATCH] arm: Stub out NRF51 TWI magnetometer/accelerometer detection
Posted by Steffen Görtz 5 years, 2 months ago
Signed-off by: Steffen Görtz <contrib@steffen-goertz.de>

On 05.01.19 15:44, Stefan Hajnoczi 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.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Based-on: <20190103091119.9367-1-stefanha@redhat.com>
> ---
> Steffen: Please post your Signed-off-by.  I did some minor cleanups so
> your patch can be merged.  Thanks!
> 
>  include/hw/arm/nrf51.h     |  1 +
>  include/hw/arm/nrf51_soc.h |  1 +
>  hw/arm/nrf51_soc.c         | 62 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 64 insertions(+)
> 
> diff --git a/include/hw/arm/nrf51.h b/include/hw/arm/nrf51.h
> index 175bb6c301..5411121b66 100644
> --- a/include/hw/arm/nrf51.h
> +++ b/include/hw/arm/nrf51.h
> @@ -25,6 +25,7 @@
>  #define NRF51_IOMEM_SIZE      0x20000000
>  
>  #define NRF51_UART_BASE       0x40002000
> +#define NRF51_TWI_BASE        0x40003000
>  #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/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
> index 1630c27594..265438f916 100644
> --- a/hw/arm/nrf51_soc.c
> +++ b/hw/arm/nrf51_soc.c
> @@ -32,6 +32,11 @@
>  #define NRF51822_FLASH_SIZE     (256 * NRF51_PAGE_SIZE)
>  #define NRF51822_SRAM_SIZE      (16 * NRF51_PAGE_SIZE)
>  
> +#define NRF51_TWI_EVENT_STOPPED 0x104
> +#define NRF51_TWI_EVENT_RXDREADY 0x108
> +#define NRF51_TWI_EVENT_TXDSENT 0x11c
> +#define NRF51_TWI_REG_RXD 0x518
> +
>  #define BASE_TO_IRQ(base) ((base >> 12) & 0x1F)
>  
>  static uint64_t clock_read(void *opaque, hwaddr addr, unsigned int size)
> @@ -53,6 +58,58 @@ static const MemoryRegionOps clock_ops = {
>      .write = clock_write
>  };
>  
> +/* Two-Wire Interface (TWI) is not implemented but the microbit-dal firmware
> + * panics if the TWI accelerometer and magnetometer WHO_AM_I registers cannot
> + * be read.  Stub out this read sequence so microbit-dal starts up.
> + */
> +static uint32_t twi_read_sequence[] = {0x5A, 0x5A, 0x40};
> +static uint32_t twi_regs[0x1000 / 4];
> +
> +static uint64_t twi_read(void *opaque, hwaddr addr, unsigned int size)
> +{
> +    static int i;
> +    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[i];
> +        if (i < ARRAY_SIZE(twi_read_sequence)) {
> +            i++;
> +        }
> +        break;
> +    default:
> +        data = twi_regs[addr / 4];
> +        break;
> +    }
> +
> +    qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " [%u] = %" PRIx32 "\n",
> +                  __func__, addr, size, (uint32_t)data);
> +
> +
> +    return data;
> +}
> +
> +static void twi_write(void *opaque, hwaddr addr, uint64_t data,
> +                      unsigned int size)
> +{
> +    qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " <- 0x%" PRIx64 " [%u]\n",
> +                  __func__, addr, data, size);
> +    twi_regs[addr / 4] = data;
> +}
> +
> +static const MemoryRegionOps twi_ops = {
> +    .read = twi_read,
> +    .write = twi_write
> +};
>  
>  static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
>  {
> @@ -156,6 +213,11 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
>      memory_region_add_subregion_overlap(&s->container,
>                                          NRF51_IOMEM_BASE, &s->clock, -1);
>  
> +    memory_region_init_io(&s->twi, NULL, &twi_ops, NULL,
> +                          "nrf51_soc.twi", 0x1000);
> +    memory_region_add_subregion_overlap(&s->container,
> +                                        NRF51_TWI_BASE, &s->twi, -1);
> +
>      create_unimplemented_device("nrf51_soc.io", NRF51_IOMEM_BASE,
>                                  NRF51_IOMEM_SIZE);
>      create_unimplemented_device("nrf51_soc.ficr", NRF51_FICR_BASE,
> 

Re: [Qemu-devel] [PATCH] arm: Stub out NRF51 TWI magnetometer/accelerometer detection
Posted by Peter Maydell 5 years, 2 months ago
On Sat, 5 Jan 2019 at 15:02, 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.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Based-on: <20190103091119.9367-1-stefanha@redhat.com>
> ---
> Steffen: Please post your Signed-off-by.  I did some minor cleanups so
> your patch can be merged.  Thanks!
>
>  include/hw/arm/nrf51.h     |  1 +
>  include/hw/arm/nrf51_soc.h |  1 +
>  hw/arm/nrf51_soc.c         | 62 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 64 insertions(+)

>
> +/* Two-Wire Interface (TWI) is not implemented but the microbit-dal firmware
> + * panics if the TWI accelerometer and magnetometer WHO_AM_I registers cannot
> + * be read.  Stub out this read sequence so microbit-dal starts up.
> + */
> +static uint32_t twi_read_sequence[] = {0x5A, 0x5A, 0x40};
> +static uint32_t twi_regs[0x1000 / 4];
> +
> +static uint64_t twi_read(void *opaque, hwaddr addr, unsigned int size)
> +{
> +    static int i;
> +    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[i];
> +        if (i < ARRAY_SIZE(twi_read_sequence)) {
> +            i++;
> +        }
> +        break;
> +    default:
> +        data = twi_regs[addr / 4];
> +        break;
> +    }
> +
> +    qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " [%u] = %" PRIx32 "\n",
> +                  __func__, addr, size, (uint32_t)data);
> +
> +
> +    return data;
> +}
> +
> +static void twi_write(void *opaque, hwaddr addr, uint64_t data,
> +                      unsigned int size)
> +{
> +    qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " <- 0x%" PRIx64 " [%u]\n",
> +                  __func__, addr, data, size);
> +    twi_regs[addr / 4] = data;
> +}
> +
> +static const MemoryRegionOps twi_ops = {
> +    .read = twi_read,
> +    .write = twi_write
> +};

Can you put this in its own device in its own source file,
please? This is too much device-specific code to have
in the SoC's source file.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] arm: Stub out NRF51 TWI magnetometer/accelerometer detection
Posted by Stefan Hajnoczi 5 years, 2 months ago
On Tue, Jan 8, 2019 at 11:57 AM Peter Maydell <peter.maydell@linaro.org> wrote:
> On Sat, 5 Jan 2019 at 15:02, 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.
> >
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Based-on: <20190103091119.9367-1-stefanha@redhat.com>
> > ---
> > Steffen: Please post your Signed-off-by.  I did some minor cleanups so
> > your patch can be merged.  Thanks!
> >
> >  include/hw/arm/nrf51.h     |  1 +
> >  include/hw/arm/nrf51_soc.h |  1 +
> >  hw/arm/nrf51_soc.c         | 62 ++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 64 insertions(+)
>
> >
> > +/* Two-Wire Interface (TWI) is not implemented but the microbit-dal firmware
> > + * panics if the TWI accelerometer and magnetometer WHO_AM_I registers cannot
> > + * be read.  Stub out this read sequence so microbit-dal starts up.
> > + */
> > +static uint32_t twi_read_sequence[] = {0x5A, 0x5A, 0x40};
> > +static uint32_t twi_regs[0x1000 / 4];
> > +
> > +static uint64_t twi_read(void *opaque, hwaddr addr, unsigned int size)
> > +{
> > +    static int i;
> > +    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[i];
> > +        if (i < ARRAY_SIZE(twi_read_sequence)) {
> > +            i++;
> > +        }
> > +        break;
> > +    default:
> > +        data = twi_regs[addr / 4];
> > +        break;
> > +    }
> > +
> > +    qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " [%u] = %" PRIx32 "\n",
> > +                  __func__, addr, size, (uint32_t)data);
> > +
> > +
> > +    return data;
> > +}
> > +
> > +static void twi_write(void *opaque, hwaddr addr, uint64_t data,
> > +                      unsigned int size)
> > +{
> > +    qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " <- 0x%" PRIx64 " [%u]\n",
> > +                  __func__, addr, data, size);
> > +    twi_regs[addr / 4] = data;
> > +}
> > +
> > +static const MemoryRegionOps twi_ops = {
> > +    .read = twi_read,
> > +    .write = twi_write
> > +};
>
> Can you put this in its own device in its own source file,
> please? This is too much device-specific code to have
> in the SoC's source file.

Sure, will send v2.  It should also be hooked up by the microbit
machine type instead of being hardcoded into the nRF51 SoC.

Stefan