qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-arm] [PATCH 6/7] arm: Instantiate nRF51 peripherals


From: Peter Maydell
Subject: Re: [Qemu-arm] [PATCH 6/7] arm: Instantiate nRF51 peripherals
Date: Thu, 16 Aug 2018 16:19:31 +0100

On 11 August 2018 at 10:08, Steffen Görtz <address@hidden> wrote:
> Instantiate NVMs, NVMC, UART, RNG, GPIO and TIMERs.
>
> Signed-off-by: Steffen Görtz <address@hidden>
> ---
>  hw/arm/nrf51_soc.c         | 153 +++++++++++++++++++++++++++++++++++--
>  include/hw/arm/nrf51_soc.h |  16 +++-
>  2 files changed, 161 insertions(+), 8 deletions(-)
>
> diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
> index 88a848de8b..a395d3a00d 100644
> --- a/hw/arm/nrf51_soc.c
> +++ b/hw/arm/nrf51_soc.c
> @@ -25,15 +25,20 @@
>
>  #include "hw/arm/nrf51_soc.h"
>
> -#define IOMEM_BASE      0x40000000
> -#define IOMEM_SIZE      0x20000000
> -
> -#define FICR_BASE       0x10000000
> -#define FICR_SIZE       0x000000fc
> -
>  #define FLASH_BASE      0x00000000
> +#define FICR_BASE       0x10000000
> +#define UICR_BASE       0x10001000
>  #define SRAM_BASE       0x20000000
>
> +#define IOMEM_BASE      0x40000000
> +#define IOMEM_SIZE      0x20000000
> +
> +#define UART_BASE       0x40002000
> +#define TIMER_BASE      0x40008000
> +#define RNG_BASE        0x4000D000
> +#define NVMC_BASE       0x4001E000
> +#define GPIO_BASE       0x50000000
> +
>  #define PAGE_SIZE       1024
>
>  /* IRQ lines can be derived from peripheral base addresses */
> @@ -49,10 +54,33 @@ struct {
>          [NRF51_VARIANT_AC] = {.ram_size = 32, .flash_size = 256 },
>  };
>
> +
> +static uint64_t clock_read(void *opaque, hwaddr addr, unsigned int size)
> +{
> +    qemu_log_mask(LOG_UNIMP, "%s: 0x%" HWADDR_PRIx " [%u]\n",
> +                  __func__, addr, size);
> +    return 1;
> +}
> +
> +static void clock_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);
> +}
> +
> +static const MemoryRegionOps clock_ops = {
> +    .read = clock_read,
> +    .write = clock_write
> +};

You don't need to roll your own "do nothing but log" device:
you can use the TYPE_UNIMPLEMENTED_DEVICE to do this.

>  static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
>  {
>      NRF51State *s = NRF51_SOC(dev_soc);
>      Error *err = NULL;
> +    MemoryRegion *mr = NULL;
> +    size_t i;
> +    qemu_irq irq;
>
>      if (!s->board_memory) {
>          error_setg(errp, "memory property was not set");
> @@ -100,14 +128,102 @@ 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);
> +    irq = qdev_get_gpio_in(DEVICE(&s->cpu), BASE_TO_IRQ(UART_BASE));
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->uart), 0, irq);

Usual approach is
   sysbus_connect_irq(SYS_BUS_DEVICE(&s->uart), 0,
                      qdev_get_gpio_in(DEVICE(&s->cpu),
                                       BASE_TO_IRQ(UART_BASE));

rather than a local qemu_irq (which tends to look like the SoC itself
has an irq line floating around, rather than just doing plumbing).

> +
> +    /* TIMER */
> +    for (i = 0; i < NRF51_TIMER_NUM; i++) {
> +        object_property_set_bool(OBJECT(&s->timer[i]), true, "realized", 
> &err);
> +        if (err) {
> +            error_propagate(errp, err);
> +            return;
> +        }
> +
> +        sysbus_mmio_map(SYS_BUS_DEVICE(&s->timer[i]), 0,
> +                     TIMER_BASE + i * 0x1000);
> +
> +        irq = qdev_get_gpio_in(DEVICE(&s->cpu),
> +                               BASE_TO_IRQ(TIMER_BASE + i * 0x1000));

Put the timer_base in a local rather than recalculating the expression
for sysbus_mmio_map() and BASE_TO_IRQ().

> +        sysbus_connect_irq(SYS_BUS_DEVICE(&s->timer[i]), 0, irq);
> +    }
> +
> +    /* NVMC */
> +    object_property_set_link(OBJECT(&s->nvm), OBJECT(&s->container),
> +                                         "memory", &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +    object_property_set_uint(OBJECT(&s->nvm),
> +            NRF51VariantAttributes[s->part_variant].flash_size, "code_size",
> +            &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +    object_property_set_bool(OBJECT(&s->nvm), true, "realized", &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +
> +    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->nvm), 0);
> +    memory_region_add_subregion_overlap(&s->container, NVMC_BASE, mr, 0);
> +    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->nvm), 1);
> +    memory_region_add_subregion_overlap(&s->container, FICR_BASE, mr, 0);
> +    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->nvm), 2);
> +    memory_region_add_subregion_overlap(&s->container, UICR_BASE, mr, 0);
> +
> +    /* RNG */
> +    object_property_set_bool(OBJECT(&s->rng), true, "realized", &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +
> +    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->rng), 0);
> +    memory_region_add_subregion_overlap(&s->container, RNG_BASE, mr, 0);
> +    irq = qdev_get_gpio_in(DEVICE(&s->cpu), BASE_TO_IRQ(RNG_BASE));
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->rng), 0, irq);
> +
> +    /* GPIO */
> +    object_property_set_bool(OBJECT(&s->gpio), true, "realized", &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }
> +
> +    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->gpio), 0);
> +    memory_region_add_subregion_overlap(&s->container, GPIO_BASE, mr, 0);
> +
> +    /* Pass all GPIOs to the SOC layer so they are available to the board */
> +    qdev_pass_gpios(DEVICE(&s->gpio), dev_soc, NULL);

Consider making these named GPIO lines rather than unnamed ones?
Unnamed is clear enough for the GPIO device itself but a bit odd
on the SoC device.

> +
> +    /* STUB Peripherals */
> +    memory_region_init_io(&s->clock, NULL, &clock_ops, NULL,
> +                          "nrf51_soc.clock", 0x1000);
> +    memory_region_add_subregion_overlap(&s->container, IOMEM_BASE, &s->clock,
> +                                        -1);
> +
>      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);
>  }
>
>  static void nrf51_soc_init(Object *obj)
>  {
>      NRF51State *s = NRF51_SOC(obj);
> +    size_t i;
>
>      memory_region_init(&s->container, obj, "nrf51-container", UINT64_MAX);
>
> @@ -118,6 +234,29 @@ static void nrf51_soc_init(Object *obj)
>      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());

These should be using sys_bus_init_child_obj() (which probably didn't
exist when you wrote this code).

> +
> +    object_initialize(&s->nvm, sizeof(s->nvm), TYPE_NRF51_NVM);
> +    object_property_add_child(obj, "nvm", OBJECT(&s->nvm), &error_abort);
> +    qdev_set_parent_bus(DEVICE(&s->nvm), sysbus_get_default());
> +
> +    object_initialize(&s->rng, sizeof(s->rng), TYPE_NRF51_RNG);
> +    object_property_add_child(obj, "rng", OBJECT(&s->rng), &error_abort);
> +    qdev_set_parent_bus(DEVICE(&s->rng), sysbus_get_default());
> +
> +    object_initialize(&s->gpio, sizeof(s->gpio), TYPE_NRF51_GPIO);
> +    object_property_add_child(obj, "gpio", OBJECT(&s->gpio), &error_abort);
> +    qdev_set_parent_bus(DEVICE(&s->gpio), sysbus_get_default());
> +
> +    for (i = 0; i < NRF51_TIMER_NUM; i++) {
> +        object_initialize(&s->timer[i], sizeof(s->timer[i]), 
> TYPE_NRF51_TIMER);
> +        object_property_add_child(obj, "timer[*]", OBJECT(&s->timer[i]), 
> NULL);
> +        qdev_set_parent_bus(DEVICE(&s->timer[i]), 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 45d9671dc3..d47b42fa37 100644
> --- a/include/hw/arm/nrf51_soc.h
> +++ b/include/hw/arm/nrf51_soc.h
> @@ -14,11 +14,18 @@
>  #include "qemu/osdep.h"
>  #include "hw/sysbus.h"
>  #include "hw/arm/armv7m.h"
> +#include "hw/char/nrf51_uart.h"
> +#include "hw/misc/nrf51_rng.h"
> +#include "hw/nvram/nrf51_nvm.h"
> +#include "hw/gpio/nrf51_gpio.h"
> +#include "hw/timer/nrf51_timer.h"
>
>  #define TYPE_NRF51_SOC "nrf51-soc"
>  #define NRF51_SOC(obj) \
>      OBJECT_CHECK(NRF51State, (obj), TYPE_NRF51_SOC)
>
> +#define NRF51_TIMER_NUM 3

NRF51_NUM_TIMERS I think makes the code read a little better.


> +
>  typedef struct NRF51State {
>      /*< private >*/
>      SysBusDevice parent_obj;
> @@ -31,9 +38,16 @@ typedef struct NRF51State {
>      MemoryRegion flash;
>
>      MemoryRegion *board_memory;
> -
>      MemoryRegion container;
>
> +    MemoryRegion clock;
> +
> +    Nrf51UART uart;
> +    Nrf51NVMState nvm;
> +    Nrf51RNGState rng;
> +    Nrf51GPIOState gpio;
> +    Nrf51TimerState timer[NRF51_TIMER_NUM];

Can we be consistent about whether we name the structs
Nrf51Thingy (as here) or NRF51Thingy (as in the NRF51State
struct we're adding them to). NRF51Thingy fits our
conventions better, I think.

> +
>      /* Properties */
>      int32_t part_variant;
>  } NRF51State;
> --
> 2.18.0
>

thanks
-- PMM



reply via email to

[Prev in Thread] Current Thread [Next in Thread]