diff mbox series

[v2,2/4] hw/arm/nrf51_soc: Connect UART to nRF51 SoC

Message ID 20180808210750.3915-3-jusual@mail.ru (mailing list archive)
State New, archived
Headers show
Series arm: Add nRF51 SoC UART support | expand

Commit Message

Zhijian Li (Fujitsu)" via Aug. 8, 2018, 9:07 p.m. UTC 
Wire up nRF51 UART in the corresponding SoC using in-place init/realize.

Based-on: <20180803052137.10602-1-joel@jms.id.au>

Signed-off-by: Julia Suvorova <jusual@mail.ru>
---
 hw/arm/nrf51_soc.c         | 20 ++++++++++++++++++++
 include/hw/arm/nrf51_soc.h |  3 +++
 2 files changed, 23 insertions(+)

Comments

Stefan Hajnoczi Aug. 10, 2018, 6:05 a.m. UTC | #1 
On Wed, Aug 8, 2018 at 10:07 PM, Julia Suvorova <jusual@mail.ru> wrote:
> @@ -70,6 +73,19 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
>      }
>      memory_region_add_subregion(&s->container, SRAM_BASE, &s->sram);
>
> +    /* UART */
> +    qdev_prop_set_chr(DEVICE(&s->uart), "chardev", serial_hd(0));
> +    object_property_set_bool(OBJECT(&s->uart), true, "realized", &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;

Is there cleanup missing (e.g. unrealizing sub-devices or freeing resources)?
Peter Maydell Aug. 16, 2018, 4:30 p.m. UTC | #2 
On 8 August 2018 at 22:07, Julia Suvorova <jusual@mail.ru> wrote:
> Wire up nRF51 UART in the corresponding SoC using in-place init/realize.
>
> Based-on: <20180803052137.10602-1-joel@jms.id.au>
>
> Signed-off-by: Julia Suvorova <jusual@mail.ru>
> ---
>  hw/arm/nrf51_soc.c         | 20 ++++++++++++++++++++
>  include/hw/arm/nrf51_soc.h |  3 +++
>  2 files changed, 23 insertions(+)
>
> diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
> index 9f9649c780..8b5602f363 100644
> --- a/hw/arm/nrf51_soc.c
> +++ b/hw/arm/nrf51_soc.c
> @@ -38,9 +38,12 @@
>  #define NRF51822_FLASH_SIZE     (256 * 1024)
>  #define NRF51822_SRAM_SIZE      (16 * 1024)
>
> +#define BASE_TO_IRQ(base) ((base >> 12) & 0x1F)
> +
>  static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
>  {
>      NRF51State *s = NRF51_SOC(dev_soc);
> +    MemoryRegion *mr = NULL;

This variable is unconditionally assigned before use later,
so you don't need to initialize it here. (Some compilers and
analysis tools will complain that the value assigned here is
never used.)

>      Error *err = NULL;
>
>      if (!s->board_memory) {
> @@ -70,6 +73,19 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
>      }
>      memory_region_add_subregion(&s->container, SRAM_BASE, &s->sram);
>
> +    /* UART */
> +    qdev_prop_set_chr(DEVICE(&s->uart), "chardev", serial_hd(0));

serial_hd() ideally is something that should only be used by board
code. It's better to have the SoC object define a chardev property
which the board then connects up to the serial_hd() chardev.

You can do this by calling
   object_property_add_alias(obj, "serial0", OBJECT(&s->uart), "chardev",
                             &error_abort);
in the SoC's init function. That will create a property "serial0"
on the SoC object that just passes through to the "chardev" property
on the UART. Then in the board code you can set the SoC serial0
property to serial_hd(0).

> +    object_property_set_bool(OBJECT(&s->uart), true, "realized", &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->uart), 0);
> +    memory_region_add_subregion_overlap(&s->container, UART_BASE, mr, 0);
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->uart), 0,
> +                       qdev_get_gpio_in(DEVICE(&s->cpu),
> +                                        BASE_TO_IRQ(UART_BASE)));
> +
>      create_unimplemented_device("nrf51_soc.io", IOMEM_BASE, IOMEM_SIZE);
>      create_unimplemented_device("nrf51_soc.ficr", FICR_BASE, FICR_SIZE);
>      create_unimplemented_device("nrf51_soc.private", 0xF0000000, 0x10000000);
> @@ -86,6 +102,10 @@ static void nrf51_soc_init(Object *obj)
>      qdev_set_parent_bus(DEVICE(&s->cpu), sysbus_get_default());
>      qdev_prop_set_string(DEVICE(&s->cpu), "cpu-type", ARM_CPU_TYPE_NAME("cortex-m0"));
>      qdev_prop_set_uint32(DEVICE(&s->cpu), "num-irq", 32);
> +
> +    object_initialize(&s->uart, sizeof(s->uart), TYPE_NRF51_UART);
> +    object_property_add_child(obj, "uart", OBJECT(&s->uart), &error_abort);
> +    qdev_set_parent_bus(DEVICE(&s->uart), sysbus_get_default());

Instead of these three lines, use sysbus_init_child_obj() (which
probably didn't exist when you wrote this code). This avoids a
leak of a refcount on the object.

>  }
>
>  static Property nrf51_soc_properties[] = {
> diff --git a/include/hw/arm/nrf51_soc.h b/include/hw/arm/nrf51_soc.h
> index e380ec26b8..46a1c1a66c 100644
> --- a/include/hw/arm/nrf51_soc.h
> +++ b/include/hw/arm/nrf51_soc.h
> @@ -13,6 +13,7 @@
>  #include "qemu/osdep.h"
>  #include "hw/sysbus.h"
>  #include "hw/arm/armv7m.h"
> +#include "hw/char/nrf51_uart.h"
>
>  #define TYPE_NRF51_SOC "nrf51-soc"
>  #define NRF51_SOC(obj) \
> @@ -25,6 +26,8 @@ typedef struct NRF51State {
>      /*< public >*/
>      ARMv7MState cpu;
>
> +    NRF51UARTState uart;
> +
>      MemoryRegion iomem;
>      MemoryRegion sram;
>      MemoryRegion flash;
> --
> 2.17.1

thanks
-- PMM
diff mbox series

Patch

diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
index 9f9649c780..8b5602f363 100644
--- a/hw/arm/nrf51_soc.c
+++ b/hw/arm/nrf51_soc.c
@@ -38,9 +38,12 @@ 
 #define NRF51822_FLASH_SIZE     (256 * 1024)
 #define NRF51822_SRAM_SIZE      (16 * 1024)
 
+#define BASE_TO_IRQ(base) ((base >> 12) & 0x1F)
+
 static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
 {
     NRF51State *s = NRF51_SOC(dev_soc);
+    MemoryRegion *mr = NULL;
     Error *err = NULL;
 
     if (!s->board_memory) {
@@ -70,6 +73,19 @@  static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
     }
     memory_region_add_subregion(&s->container, SRAM_BASE, &s->sram);
 
+    /* UART */
+    qdev_prop_set_chr(DEVICE(&s->uart), "chardev", serial_hd(0));
+    object_property_set_bool(OBJECT(&s->uart), true, "realized", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->uart), 0);
+    memory_region_add_subregion_overlap(&s->container, UART_BASE, mr, 0);
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->uart), 0,
+                       qdev_get_gpio_in(DEVICE(&s->cpu),
+                                        BASE_TO_IRQ(UART_BASE)));
+
     create_unimplemented_device("nrf51_soc.io", IOMEM_BASE, IOMEM_SIZE);
     create_unimplemented_device("nrf51_soc.ficr", FICR_BASE, FICR_SIZE);
     create_unimplemented_device("nrf51_soc.private", 0xF0000000, 0x10000000);
@@ -86,6 +102,10 @@  static void nrf51_soc_init(Object *obj)
     qdev_set_parent_bus(DEVICE(&s->cpu), sysbus_get_default());
     qdev_prop_set_string(DEVICE(&s->cpu), "cpu-type", ARM_CPU_TYPE_NAME("cortex-m0"));
     qdev_prop_set_uint32(DEVICE(&s->cpu), "num-irq", 32);
+
+    object_initialize(&s->uart, sizeof(s->uart), TYPE_NRF51_UART);
+    object_property_add_child(obj, "uart", OBJECT(&s->uart), &error_abort);
+    qdev_set_parent_bus(DEVICE(&s->uart), sysbus_get_default());
 }
 
 static Property nrf51_soc_properties[] = {
diff --git a/include/hw/arm/nrf51_soc.h b/include/hw/arm/nrf51_soc.h
index e380ec26b8..46a1c1a66c 100644
--- a/include/hw/arm/nrf51_soc.h
+++ b/include/hw/arm/nrf51_soc.h
@@ -13,6 +13,7 @@ 
 #include "qemu/osdep.h"
 #include "hw/sysbus.h"
 #include "hw/arm/armv7m.h"
+#include "hw/char/nrf51_uart.h"
 
 #define TYPE_NRF51_SOC "nrf51-soc"
 #define NRF51_SOC(obj) \
@@ -25,6 +26,8 @@  typedef struct NRF51State {
     /*< public >*/
     ARMv7MState cpu;
 
+    NRF51UARTState uart;
+
     MemoryRegion iomem;
     MemoryRegion sram;
     MemoryRegion flash;