| Message ID | 8f06c187c5eee0ce26864be6f41d7c37f80e3c80.1408838440.git.alistair23@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 24 August 2014 01:13, Alistair Francis <alistair23@gmail.com> 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 <alistair23@gmail.com> > --- > 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 <alistair@alistair23.me> > + * > + * 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
On Tue, Sep 2, 2014 at 2:29 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 24 August 2014 01:13, Alistair Francis <alistair23@gmail.com> 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. > It is part of the SoC. All of the devices I added are, I will rename as appropriate. >> Signed-off-by: Alistair Francis <alistair23@gmail.com> >> --- >> 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 <alistair@alistair23.me> >> + * >> + * 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. > Will fix >> + 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? > That's what they are called in the datasheet, I am happy to remove them though (except they are called GPIOx_*) >> + >> + /* 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 idea was to use an internal register to keep track of the data direction. This way it would be easier to see if I/O is in the correct direction, by just ANDing/NANDing with the direction register The problem comes from the GPIO_MODER register (the direction register) as it uses two bits to set the I/O direction/mode I will rephrase the 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. > That seems like overkill to just set three registers. I also think it is worth knowing which GPIOx the device is. Especially when the EXTI device (External Interrupts) is added >> + >> + 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? > That must be a Typo, it should be all-zeroes. Will fix >> + case GPIO_BSRR: >> + /* gpio_bsrr reads as zero */ >> + return 0x00000000; > > Comment doesn't tell us anything the code doesn't... > I was just trying to say that the register is a write only register. I will fix/remove the comment >> + 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.) > Ok, I will make it a 0 to 15 loop >> + /* 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". > I will replace with 3U << i. I didn't think of doing it that way >> + >> + 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? > No, this is just the first way I thought of >> + 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. > That is true, it doesn't have a state. I was thinking of removing it, but I left it there in case somebody wanted it for debugging. I can remove it though >> + 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" */ > Will fix >> + 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? > Yes, the outputs should be doing something. I just hadn't worked out how to get them to do anything. I will look into that and implement >> + case GPIO_LCKR: >> + s->gpio_lckr = (uint32_t) value; > > This doesn't seem to be implementing the lock functionality > the datasheet describes. > That's not implemented at the moment, I will either implement or add an unimplemented message >> + 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? > I will add them >> + 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
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 <alistair@alistair23.me> + * + * 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 { + 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; + + /* 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; + /* 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; + } + + 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 */ + return 0xFFFF; + case GPIO_BSRR: + /* gpio_bsrr reads as zero */ + return 0x00000000; + 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) { + /* Two bits determine the I/O direction/mode */ + mask = (1 << i) + (1 << (i + 1)); + + 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); + s->gpio_bsrr = (uint32_t) (value << 16); + 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 */ + s->gpio_odr |= (uint32_t) (value & 0xFFFF); + s->gpio_bsrr = (uint32_t) value; + DPRINTF("Output: 0x%x\n", s->gpio_odr); + return; + case GPIO_LCKR: + s->gpio_lckr = (uint32_t) value; + 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); + 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)
This patch adds the Netduino Plus 2 GPIO controller to QEMU. This allows reading and writing to the Netduino GPIO pins. Signed-off-by: Alistair Francis <alistair23@gmail.com> --- hw/gpio/Makefile.objs | 1 + hw/gpio/netduino_gpio.c | 285 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 286 insertions(+) create mode 100644 hw/gpio/netduino_gpio.c