Re: [Qemu-devel] [PATCH v4 08/13] hw/gpio/nrf51_gpio: Add nRF51 GPIO peripheral

[Peter Maydell]

On 2 November 2018 at 17:07, Steffen Görtz <address@hidden> 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 <address@hidden>
> Reviewed-by: Stefan Hajnoczi <address@hidden>

Hi; I just had a few minor nits here...

> ---
>  Makefile.objs                |   1 +
>  hw/gpio/Makefile.objs        |   1 +
>  hw/gpio/nrf51_gpio.c         | 300 +++++++++++++++++++++++++++++++++++
>  hw/gpio/trace-events         |   7 +
>  include/hw/gpio/nrf51_gpio.h |  69 ++++++++
>  5 files changed, 378 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 1e1ff387d7..fbc3bad1e1 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -243,6 +243,7 @@ trace-events-subdirs += hw/vfio
>  trace-events-subdirs += hw/virtio
>  trace-events-subdirs += hw/watchdog
>  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..0a378e03ab
> --- /dev/null
> +++ b/hw/gpio/nrf51_gpio.c
> @@ -0,0 +1,300 @@
> +/*
> + * 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 <address@hidden>
> + *
> + * 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"
> +
> +/*
> + * 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;
> +    default:
> +        /* Some compilers can not infer the value range of extract32(.., 3) 
> */

Usually we handle this with g_assert_not_reached().

> +        state = false;
> +        break;
> +    }
> +
> +    return state;
> +}

> +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. */

Nonstandard multiline comment format; see CODING_STYLE for the
preferred form.

> +        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 VMStateDescription vmstate_nrf51_gpio = {
> +    .name = TYPE_NRF51_GPIO,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,

You don't need to specify minimum_version_id_old unless
you provide a load_state_old handler.

> +    .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()
> +    }
> +};

Otherwise
Reviewed-by: Peter Maydell <address@hidden>

thanks
-- PMM