Re: [Qemu-devel] [PATCH v1 2/5] Netduino_GPIO: Add the Netduino Plus 2 GPIO controller

[Peter Maydell]

On 24 August 2014 01:13, Alistair Francis <address@hidden> wrote:
> This patch adds the Netduino Plus 2 GPIO controller to QEMU.
> This allows reading and writing to the Netduino GPIO pins.

Are you sure this isn't actually the GPIO module in the STM32F4xx
SoC ? The datasheets and schematic suggest this isn't a
board level feature, which would imply it shouldn't have a
board level name like "netduino_gpio". Instead you should
have an SoC QOM object/device which instantiates all
the SoC level components like the GPIO, UART, etc,
and the board model should just create the SoC and any
devices which are genuinely separate components from the
SoC.

> Signed-off-by: Alistair Francis <address@hidden>
> ---
>  hw/gpio/Makefile.objs   |   1 +
>  hw/gpio/netduino_gpio.c | 285 
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 286 insertions(+)
>  create mode 100644 hw/gpio/netduino_gpio.c
>
> diff --git a/hw/gpio/Makefile.objs b/hw/gpio/Makefile.objs
> index 2c8b51f..e61c4d3 100644
> --- a/hw/gpio/Makefile.objs
> +++ b/hw/gpio/Makefile.objs
> @@ -2,5 +2,6 @@ common-obj-$(CONFIG_MAX7310) += max7310.o
>  common-obj-$(CONFIG_PL061) += pl061.o
>  common-obj-$(CONFIG_PUV3) += puv3_gpio.o
>  common-obj-$(CONFIG_ZAURUS) += zaurus.o
> +common-obj-$(CONFIG_NETDUINOP2) += netduino_gpio.o
>
>  obj-$(CONFIG_OMAP) += omap_gpio.o
> diff --git a/hw/gpio/netduino_gpio.c b/hw/gpio/netduino_gpio.c
> new file mode 100644
> index 0000000..10d0bbd
> --- /dev/null
> +++ b/hw/gpio/netduino_gpio.c
> @@ -0,0 +1,285 @@
> +/*
> + * Netduino Plus 2 GPIO
> + *
> + * Copyright (c) 2014 Alistair Francis <address@hidden>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "hw/sysbus.h"
> +
> +/* #define DEBUG_NETGPIO */
> +
> +#ifdef DEBUG_NETGPIO
> +#define DPRINTF(fmt, ...) \
> +do { printf("netduino_gpio: " fmt , ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...) do {} while (0)
> +#endif
> +
> +#define GPIO_MODER     0x00
> +#define GPIO_OTYPER    0x04
> +#define GPIO_OSPEEDR   0x08
> +#define GPIO_PUPDR     0x0C
> +#define GPIO_IDR       0x10
> +#define GPIO_ODR       0x14
> +#define GPIO_BSRR      0x18
> +#define GPIO_BSRR_HIGH 0x1A
> +#define GPIO_LCKR      0x1C
> +#define GPIO_AFRL      0x20
> +#define GPIO_AFRH      0x24
> +
> +#define GPIO_MODER_INPUT       0
> +#define GPIO_MODER_GENERAL_OUT 1
> +#define GPIO_MODER_ALT         2
> +#define GPIO_MODER_ANALOG      3
> +
> +#define TYPE_NETDUINO_GPIO "netduino_gpio"
> +#define NETDUINO_GPIO(obj) OBJECT_CHECK(NETDUINO_GPIOState, (obj), \
> +                           TYPE_NETDUINO_GPIO)
> +
> +typedef struct NETDUINO_GPIOState {

This should be "Netduino_GPIOState", since the official
boardname isn't an all-caps word.

> +    SysBusDevice parent_obj;
> +
> +    MemoryRegion iomem;
> +
> +    uint32_t gpio_moder;
> +    uint32_t gpio_otyper;
> +    uint32_t gpio_ospeedr;
> +    uint32_t gpio_pupdr;
> +    uint32_t gpio_idr;
> +    uint32_t gpio_odr;
> +    uint32_t gpio_bsrr;
> +    uint32_t gpio_lckr;
> +    uint32_t gpio_afrl;
> +    uint32_t gpio_afrh;

Do these really all need a 'gpio_' prefix in their names?

> +
> +    /* This is an internal QEMU Register, used to determine the
> +     * GPIO direction as set by gpio_moder
> +     * 1: Input; 0: Output
> +     */
> +    uint16_t gpio_direction;

I can't really figure out what you mean by this comment.

> +    /* The GPIO letter (a - k) from the datasheet */
> +    uint8_t gpio_letter;
> +
> +    qemu_irq irq;
> +    const unsigned char *id;
> +} NETDUINO_GPIOState;
> +
> +static const VMStateDescription vmstate_netduino_gpio = {
> +    .name = "netduino_gpio",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(gpio_moder, NETDUINO_GPIOState),
> +        VMSTATE_UINT32(gpio_otyper, NETDUINO_GPIOState),
> +        VMSTATE_UINT32(gpio_ospeedr, NETDUINO_GPIOState),
> +        VMSTATE_UINT32(gpio_pupdr, NETDUINO_GPIOState),
> +        VMSTATE_UINT32(gpio_idr, NETDUINO_GPIOState),
> +        VMSTATE_UINT32(gpio_odr, NETDUINO_GPIOState),
> +        VMSTATE_UINT32(gpio_bsrr, NETDUINO_GPIOState),
> +        VMSTATE_UINT32(gpio_lckr, NETDUINO_GPIOState),
> +        VMSTATE_UINT32(gpio_afrl, NETDUINO_GPIOState),
> +        VMSTATE_UINT32(gpio_afrh, NETDUINO_GPIOState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void gpio_reset(DeviceState *dev)
> +{
> +    NETDUINO_GPIOState *s = NETDUINO_GPIO(dev);
> +
> +    if (s->gpio_letter == 'a') {
> +        s->gpio_moder = 0xA8000000;
> +        s->gpio_pupdr = 0x64000000;
> +        s->gpio_ospeedr = 0x00000000;
> +    } else if (s->gpio_letter == 'b') {
> +        s->gpio_moder = 0x00000280;
> +        s->gpio_pupdr = 0x00000100;
> +        s->gpio_ospeedr = 0x000000C0;
> +    } else {
> +        s->gpio_moder = 0x00000000;
> +        s->gpio_pupdr = 0x00000000;
> +        s->gpio_ospeedr = 0x00000000;
> +    }

I don't think this is the right way to implement this.
You should give the GPIO device some QOM properties
which allow it to be configured (eg reset values for
various registers) and let the SoC QOM device
instantiate and set QOM properties appropriately
for each of the GPIO modules in the SoC.

> +
> +    s->gpio_otyper = 0x00000000;
> +    s->gpio_idr = 0x00000000;
> +    s->gpio_odr = 0x00000000;
> +    s->gpio_bsrr = 0x00000000;
> +    s->gpio_lckr = 0x00000000;
> +    s->gpio_afrl = 0x00000000;
> +    s->gpio_afrh = 0x00000000;
> +    s->gpio_direction = 0x0000;
> +}
> +
> +static uint64_t netduino_gpio_read(void *opaque, hwaddr offset,
> +                           unsigned size)
> +{
> +    NETDUINO_GPIOState *s = (NETDUINO_GPIOState *)opaque;
> +
> +    DPRINTF("Read 0x%x\n", (uint) offset);
> +
> +    switch (offset) {
> +    case GPIO_MODER:
> +        return s->gpio_moder;
> +    case GPIO_OTYPER:
> +        return s->gpio_otyper;
> +    case GPIO_OSPEEDR:
> +        return s->gpio_ospeedr;
> +    case GPIO_PUPDR:
> +        return s->gpio_pupdr;
> +    case GPIO_IDR:
> +        /* This register changes based on the external GPIO pins */
> +        return s->gpio_idr & s->gpio_direction;
> +    case GPIO_ODR:
> +        return s->gpio_odr;
> +    case GPIO_BSRR_HIGH:
> +        /* gpio_bsrr reads as zero */

Dup of comment from a couple of lines down?

> +        return 0xFFFF;

Why does reading the top half of this as a 16 bit read
return all-ones when reading the full 32 bits returns
all-zeroes?

> +    case GPIO_BSRR:
> +        /* gpio_bsrr reads as zero */
> +        return 0x00000000;

Comment doesn't tell us anything the code doesn't...

> +    case GPIO_LCKR:
> +        return s->gpio_lckr;
> +    case GPIO_AFRL:
> +        return s->gpio_afrl;
> +    case GPIO_AFRH:
> +        return s->gpio_afrh;
> +    }
> +    return 0;
> +}
> +
> +static void netduino_gpio_write(void *opaque, hwaddr offset,
> +                        uint64_t value, unsigned size)
> +{
> +    NETDUINO_GPIOState *s = (NETDUINO_GPIOState *)opaque;
> +    int i, mask;
> +
> +    DPRINTF("Write 0x%x, 0x%x\n", (uint) value, (uint) offset);
> +
> +    switch (offset) {
> +    case GPIO_MODER:
> +        s->gpio_moder = (uint32_t) value;
> +        for (i = 0; i < 32; i = i + 2) {

"i += 2" is more idiomatic. In any case I think this
loop would be more cleanly phrased as a simple loop
from 0 to 15 (giving you one multiply-by-two rather
than two divide-by-twos, and probably making the
loop termination easier for static analysis tools to
understand.)

> +            /* Two bits determine the I/O direction/mode */
> +            mask = (1 << i) + (1 << (i + 1));

You need to say "1U" if you want to shift it by 31,
since shifting into the sign bit of a signed value is
undefined behaviour in C. Also this is a funny way
of writing "3U << i".

> +
> +            if ((s->gpio_moder & mask) == GPIO_MODER_INPUT) {
> +                s->gpio_direction |= (1 << (i/2));
> +            } else if ((s->gpio_moder & mask) == GPIO_MODER_GENERAL_OUT) {
> +                s->gpio_direction &= (0xFFFF ^ (1 << (i/2)));
> +            } else {
> +                /* Not supported at the moment */
> +            }
> +        }
> +        return;
> +    case GPIO_OTYPER:
> +        s->gpio_otyper = (uint32_t) value;
> +        return;
> +    case GPIO_OSPEEDR:
> +        s->gpio_ospeedr = (uint32_t) value;
> +        return;
> +    case GPIO_PUPDR:
> +        s->gpio_pupdr = (uint32_t) value;
> +        return;
> +    case GPIO_IDR:
> +        /* Read Only Register */
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "net_gpio%c_write: Read Only Register 0x%x\n",
> +                      s->gpio_letter, (int)offset);
> +        return;
> +    case GPIO_ODR:
> +        s->gpio_odr = ((uint32_t) value & (s->gpio_direction ^ 0xFFFF));
> +        return;
> +    case GPIO_BSRR_HIGH:
> +        /* Reset the output value */
> +        s->gpio_odr &= (uint32_t) (value ^ 0xFFFF);

Any particular reason for using the XOR with 0xffff
rather than just inverting?

> +        s->gpio_bsrr = (uint32_t) (value << 16);

BSRR is write-only and doesn't have any underlying state
as far as I can tell from the  manual. Indeed this file never
reads the gpio_bsrr field in the struct. That suggests you
should remove it entirely.

> +        DPRINTF("Output: 0x%x\n", s->gpio_odr);
> +        return;
> +    case GPIO_BSRR:
> +        /* Reset the output value */
> +        s->gpio_odr &= (uint32_t) ((value >> 16) ^ 0xFFFF);
> +        /* Sets the output value */

These comments would be more helpful if they read
 /* Top 16 bits are "write one to clear output" */
 /* Bottom 16 bits are "write one to set output" */

> +        s->gpio_odr |= (uint32_t) (value & 0xFFFF);
> +        s->gpio_bsrr = (uint32_t) value;
> +        DPRINTF("Output: 0x%x\n", s->gpio_odr);
> +        return;

Surely we should actually be doing something with our
outputs (ie setting qdev GPIO output lines) here?

> +    case GPIO_LCKR:
> +        s->gpio_lckr = (uint32_t) value;

This doesn't seem to be implementing the lock functionality
the datasheet describes.

> +        return;
> +    case GPIO_AFRL:
> +        s->gpio_afrl = (uint32_t) value;
> +        return;
> +    case GPIO_AFRH:
> +        s->gpio_afrh = (uint32_t) value;
> +        return;
> +    }
> +}
> +
> +static const MemoryRegionOps netduino_gpio_ops = {
> +    .read = netduino_gpio_read,
> +    .write = netduino_gpio_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static Property net_gpio_properties[] = {
> +    DEFINE_PROP_UINT8("gpio-letter", NETDUINO_GPIOState, gpio_letter,
> +                      (uint) 'a'),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +
> +static int netduino_gpio_initfn(SysBusDevice *sbd)
> +{
> +    DeviceState *dev = DEVICE(sbd);
> +    NETDUINO_GPIOState *s = NETDUINO_GPIO(dev);
> +
> +    memory_region_init_io(&s->iomem, OBJECT(s), &netduino_gpio_ops, s,
> +                          "netduino_gpio", 0x2000);
> +    sysbus_init_mmio(sbd, &s->iomem);
> +    sysbus_init_irq(sbd, &s->irq);

Where are the GPIO output lines?

> +    return 0;
> +}
> +
> +static void netduino_gpio_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +
> +    k->init = netduino_gpio_initfn;
> +    dc->vmsd = &vmstate_netduino_gpio;
> +    dc->props = net_gpio_properties;
> +    dc->reset = gpio_reset;
> +}
> +
> +static const TypeInfo netduino_gpio_info = {
> +    .name          = TYPE_NETDUINO_GPIO,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(NETDUINO_GPIOState),
> +    .class_init    = netduino_gpio_class_init,
> +};
> +
> +static void netduino_gpio_register_types(void)
> +{
> +    type_register_static(&netduino_gpio_info);
> +}
> +
> +type_init(netduino_gpio_register_types)
> --
> 1.9.1

thanks
-- PMM