[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] zynq_gpio: GPIO model for Zynq SoC
|
From: |
Peter Crosthwaite |
|
Subject: |
Re: [Qemu-devel] [PATCH 1/2] zynq_gpio: GPIO model for Zynq SoC |
|
Date: |
Tue, 30 Dec 2014 15:24:03 -0800 |
On Tue, Dec 30, 2014 at 5:13 AM, Colin Leitner
<address@hidden> wrote:
> Based on the pl061 model. This model implements all four banks with 32 I/Os
> each.
>
> The I/Os are placed in four named groups:
>
> * mio_in/out[0..63], where mio_in/out[0..31] map to bank 0 and the rest to
> bank 1
> * emio_in/out[0..63], where emio_in/out[0..31] map to bank 2 and the rest to
> bank 3
>
> Basic I/O tested with the Zynq GPIO driver in Linux 3.12.
>
> Signed-off-by: Colin Leitner <address@hidden>
> ---
> hw/gpio/Makefile.objs | 1 +
> hw/gpio/zynq_gpio.c | 441
> +++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 442 insertions(+)
> create mode 100644 hw/gpio/zynq_gpio.c
>
> diff --git a/hw/gpio/Makefile.objs b/hw/gpio/Makefile.objs
> index 1abcf17..32b99e0 100644
> --- a/hw/gpio/Makefile.objs
> +++ b/hw/gpio/Makefile.objs
> @@ -5,3 +5,4 @@ common-obj-$(CONFIG_ZAURUS) += zaurus.o
> common-obj-$(CONFIG_E500) += mpc8xxx.o
>
> obj-$(CONFIG_OMAP) += omap_gpio.o
> +obj-$(CONFIG_ZYNQ) += zynq_gpio.o
I think we are trying to slowly covert filenames to use - separators.
Should be followed for new files.
> diff --git a/hw/gpio/zynq_gpio.c b/hw/gpio/zynq_gpio.c
> new file mode 100644
> index 0000000..2119561
> --- /dev/null
> +++ b/hw/gpio/zynq_gpio.c
> @@ -0,0 +1,441 @@
> +/*
> + * Zynq General Purpose IO
> + *
> + * Copyright (C) 2014 Colin Leitner <address@hidden>
> + *
> + * Based on the PL061 model:
> + * Copyright (c) 2007 CodeSourcery.
> + * Written by Paul Brook
> + *
> + * This code is licensed under the GPL.
> + */
> +
> +/*
> + * We model all banks as if they were fully populated. MIO pins are usually
> + * limited to 54 pins, but this is probably device dependent and shouldn't
> + * cause too much trouble. One noticable difference is the reset value of
"noticeable"
> + * INT_TYPE_1, which is 0x003fffff according to the TRM and 0xffffffff here.
> + *
> + * The output enable pins are not modeled.
> + */
> +
> +#include "hw/sysbus.h"
> +
> +//#define DEBUG_ZYNQ_GPIO 1
Don't worry about commented out debug switches.
> +
> +#ifdef DEBUG_ZYNQ_GPIO
> +#define DPRINTF(fmt, ...) \
> +do { printf("zynq-gpio: " fmt , ## __VA_ARGS__); } while (0)
use qemu_log.
> +#define BADF(fmt, ...) \
BADF is unused.
> +do { fprintf(stderr, "zynq-gpio: error: " fmt , ## __VA_ARGS__); exit(1);}
> while (0)
and exit(1) is probably not a good semantic for an error condition.
Such conditions should just be asserts. You should just drop BADF
completely.
> +#else
> +#define DPRINTF(fmt, ...) do {} while(0)
> +#define BADF(fmt, ...) \
> +do { fprintf(stderr, "zynq-gpio: error: " fmt , ## __VA_ARGS__);} while (0)
> +#endif
> +
It's better to use a regular if for debug instrumentation. check
hw/dma/pl330.c for an example of this. The reason is to always compile
test the contents of printfs.
> +#define TYPE_ZYNQ_GPIO "zynq-gpio"
> +#define ZYNQ_GPIO(obj) OBJECT_CHECK(ZynqGPIOState, (obj), TYPE_ZYNQ_GPIO)
> +
> +typedef struct {
Modern device-model conventions require the type struct, typename and
cast macros and the register offset #defines to be in a header file
specific to the device. These bits should be in zynq-gpio.h
> + uint32_t mask_data;
> + uint32_t out_data;
> + uint32_t old_out_data;
> + uint32_t in_data;
> + uint32_t old_in_data;
> + uint32_t dir;
> + uint32_t oen;
> + uint32_t imask;
> + uint32_t istat;
> + uint32_t itype;
> + uint32_t ipolarity;
> + uint32_t iany;
> +
> + qemu_irq *out;
> +} GPIOBank;
Preface the struct name with "Zynq" for consistency with other
identifiers defined.
> +
> +typedef struct ZynqGPIOState {
> + SysBusDevice parent_obj;
> +
> + MemoryRegion iomem;
> + GPIOBank banks[4];
> + qemu_irq mio_out[64];
> + qemu_irq emio_out[64];
Is it better to just model the GPIO controller as a standalone GPIO,
and leave the mio vs emio distinction to the SoC/Board level?
This would mean the bank GPIOs are on the top level entity, and the
core would then have no EMIO/MIO awareness. This also makes QEMU a
little less awkward considering there is no sense of MIO and EMIO in
QEMU to date.
> + qemu_irq irq;
> +} ZynqGPIOState;
> +
> +static void zynq_gpio_update_out(GPIOBank *b)
> +{
> + uint32_t changed;
> + uint32_t mask;
> + uint32_t out;
> + int i;
> +
> + DPRINTF("dir = %d, data = %d\n", b->dir, b->out_data);
> +
> + /* Outputs float high. */
> + /* FIXME: This is board dependent. */
How so? Looks pretty generic to me (not sure what needs fixing here).
Are you saying that the IO width should truncate based on Zynq
specifics?
> + out = (b->out_data & b->dir) | ~b->dir;
> + changed = b->old_out_data ^ out;
> + if (changed) {
This if doesn't save much in optimization, as the expensive part (the
qemu_set_irq) is already change-guarded per-bit below anyway. Just
drop the if.
> + b->old_out_data = out;
> + for (i = 0; i < 32; i++) {
Macroify hardcoded constant 32.
> + mask = 1 << i;
> + if (changed & mask) {
> + DPRINTF("Set output %d = %d\n", i, (out & mask) != 0);
> + qemu_set_irq(b->out[i], (out & mask) != 0);
> + }
> + }
> + }
> +}
> +
> +static void zynq_gpio_update_in(GPIOBank *b)
> +{
> + uint32_t changed;
> + uint32_t mask;
> + int i;
> +
> + changed = b->old_in_data ^ b->in_data;
> + if (changed) {
Same as before. I don't think this is needed.
> + b->old_in_data = b->in_data;
> + for (i = 0; i < 32; i++) {
> + mask = 1 << i;
> + if (changed & mask) {
> + DPRINTF("Changed input %d = %d\n", i, (b->in_data & mask) !=
> 0);
> +
> + if (b->itype & mask) {
> + /* Edge interrupt */
> + if (b->iany & mask) {
> + /* Any edge triggers the interrupt */
> + b->istat |= mask;
> + } else {
> + /* Edge is selected by INT_POLARITY */
> + b->istat |= ~(b->in_data ^ b->ipolarity) & mask;
> + }
> + }
> + }
> + }
> + }
> +
> + /* Level interrupt */
> + b->istat |= ~(b->in_data ^ b->ipolarity) & ~b->itype;
> +
> + DPRINTF("istat = %08X\n", b->istat);
> +}
> +
> +static void zynq_gpio_set_in_irq(ZynqGPIOState *s)
> +{
> + int b;
> + uint32_t istat = 0;
> +
> + for (b = 0; b < 4; b++) {
> + istat |= s->banks[b].istat & ~s->banks[b].imask;
> + }
> +
> + DPRINTF("IRQ = %d\n", istat != 0);
> +
> + qemu_set_irq(s->irq, istat != 0);
> +}
> +
> +static void zynq_gpio_update(ZynqGPIOState *s)
> +{
> + int b;
> +
> + for (b = 0; b < 4; b++) {
> + zynq_gpio_update_out(&s->banks[b]);
> + zynq_gpio_update_in(&s->banks[b]);
> + }
> +
> + zynq_gpio_set_in_irq(s);
> +}
> +
> +static uint64_t zynq_gpio_read(void *opaque, hwaddr offset,
> + unsigned size)
> +{
> + ZynqGPIOState *s = (ZynqGPIOState *)opaque;
> + int b;
> + GPIOBank *bank;
> +
> + switch (offset) {
> + case 0x000: /* MASK_DATA_0_LSW */
Define these register offsets as macros as use them for the case labels.
> + return (s->banks[0].mask_data >> 0) & 0xffff;
Use extract32 to get field values rather than >> & logic.
> + case 0x004: /* MASK_DATA_0_MSW */
> + return (s->banks[0].mask_data >> 16) & 0xffff;
> + case 0x008: /* MASK_DATA_1_LSW */
> + return (s->banks[1].mask_data >> 0) & 0xffff;
> + case 0x00C: /* MASK_DATA_1_MSW */
> + return (s->banks[1].mask_data >> 16) & 0xffff;
> + case 0x010: /* MASK_DATA_2_LSW */
> + return (s->banks[2].mask_data >> 0) & 0xffff;
> + case 0x014: /* MASK_DATA_2_MSW */
> + return (s->banks[2].mask_data >> 16) & 0xffff;
> + case 0x018: /* MASK_DATA_3_LSW */
> + return (s->banks[3].mask_data >> 0) & 0xffff;
> + case 0x01C: /* MASK_DATA_3_MSW */
> + return (s->banks[3].mask_data >> 16) & 0xffff;
I would look into doing this shorthand though with:
case MASK_DATA_0_LSW ... MASK_DATA_3_MSW:
bank = extract32(offset, 3, 2);
shift = offset & 0x8 ? 16 : 0;
return extract32(s->banks[bank].mask_data, shift, 16);
> +
> + case 0x040: /* DATA_0 */
> + return s->banks[0].out_data;
> + case 0x044: /* DATA_1 */
> + return s->banks[1].out_data;
> + case 0x048: /* DATA_2 */
> + return s->banks[2].out_data;
> + case 0x04C: /* DATA_3 */
> + return s->banks[3].out_data;
> +
> + case 0x060: /* DATA_0_RO */
> + return s->banks[0].in_data;
> + case 0x064: /* DATA_1_RO */
> + return s->banks[1].in_data;
> + case 0x068: /* DATA_2_RO */
> + return s->banks[2].in_data;
> + case 0x06C: /* DATA_3_RO */
> + return s->banks[3].in_data;
Similar here (slightly simpler logic).
> + }
> +
> + if (offset < 0x204 || offset > 0x2e4) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "zynq_gpio_read: Bad offset %x\n", (int)offset);
> + return 0;
> + }
> +
> + b = (offset - 0x200) / 0x40;
> + bank = &s->banks[b];
> +
> + switch (offset - 0x200 - b * 0x40) {
> + case 0x04: /* DIRM_x */
You can macroify theses offset labels. Probably with exactly the names
you have already used in the comments.
> + return bank->dir;
> + case 0x08: /* OEN_x */
> + return bank->oen;
> + case 0x0C: /* INT_MASK_x */
> + return bank->imask;
> + case 0x10: /* INT_EN_x */
> + return 0;
> + case 0x14: /* INT_DIS_x */
> + return 0;
> + case 0x18: /* INT_STAT_x */
> + return bank->istat;
> + case 0x1C: /* INT_TYPE_x */
> + return bank->itype;
> + case 0x20: /* INT_POLARITY_x */
> + return bank->ipolarity;
> + case 0x24: /* INT_ANY_x */
> + return bank->iany;
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "zynq_gpio_read: Bad offset %x\n", (int)offset);
> + return 0;
> + }
> +}
> +
> +static void zynq_gpio_mask_data(GPIOBank *bank, int bit_offset,
> + uint32_t mask_data)
> +{
> + DPRINTF("mask data offset = %d, mask_data = %08X\n", bit_offset,
> mask_data);
> +
> + /* MASK_DATA registers are R/W on their data part */
> + bank->mask_data = (bank->mask_data & ~(0xffff << bit_offset)) |
> + ((mask_data & 0xffff) << bit_offset);
You can use deposit32 here.
> + bank->out_data = (bank->out_data & ~((~mask_data >> 16) << bit_offset)) |
> + ((mask_data & 0xffff) << bit_offset);
> + zynq_gpio_update_out(bank);
> +}
> +
> +static void zynq_gpio_data(GPIOBank *bank, uint32_t data)
> +{
> + bank->out_data = data;
> + zynq_gpio_update_out(bank);
> +}
> +
> +static void zynq_gpio_write(void *opaque, hwaddr offset,
> + uint64_t value, unsigned size)
> +{
> + ZynqGPIOState *s = (ZynqGPIOState *)opaque;
cast un-needed.
> + int b;
> + GPIOBank *bank;
> +
> + switch (offset) {
> + case 0x000: /* MASK_DATA_0_LSW */
> + zynq_gpio_mask_data(&s->banks[0], 0, value);
> + return;
> + case 0x004: /* MASK_DATA_0_MSW */
> + zynq_gpio_mask_data(&s->banks[0], 16, value);
> + return;
> + case 0x008: /* MASK_DATA_1_LSW */
> + zynq_gpio_mask_data(&s->banks[1], 0, value);
> + return;
> + case 0x00C: /* MASK_DATA_1_MSW */
> + zynq_gpio_mask_data(&s->banks[1], 16, value);
> + return;
> + case 0x010: /* MASK_DATA_2_LSW */
> + zynq_gpio_mask_data(&s->banks[2], 0, value);
> + return;
> + case 0x014: /* MASK_DATA_2_MSW */
> + zynq_gpio_mask_data(&s->banks[2], 16, value);
> + return;
> + case 0x018: /* MASK_DATA_3_LSW */
> + zynq_gpio_mask_data(&s->banks[3], 0, value);
> + return;
> + case 0x01C: /* MASK_DATA_3_MSW */
> + zynq_gpio_mask_data(&s->banks[3], 16, value);
> + return;
> +
> + case 0x040: /* DATA_0 */
> + zynq_gpio_data(&s->banks[0], value);
> + return;
> + case 0x044: /* DATA_1 */
> + zynq_gpio_data(&s->banks[1], value);
> + return;
> + case 0x048: /* DATA_2 */
> + zynq_gpio_data(&s->banks[2], value);
> + return;
> + case 0x04C: /* DATA_3 */
> + zynq_gpio_data(&s->banks[3], value);
> + return;
> +
> + case 0x060: /* DATA_0_RO */
> + case 0x064: /* DATA_1_RO */
> + case 0x068: /* DATA_2_RO */
> + case 0x06C: /* DATA_3_RO */
> + return;
> + }
> +
> + if (offset < 0x204 || offset > 0x2e4) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "zynq_gpio_write: Bad offset %x\n", (int)offset);
> + return;
> + }
> +
> + b = (offset - 0x200) / 0x40;
> + bank = &s->banks[b];
> +
> + switch (offset - 0x200 - b * 0x40) {
> + case 0x04: /* DIRM_x */
> + bank->dir = value;
> + break;
> + case 0x08: /* OEN_x */
> + bank->oen = value;
Probably worth a LOG_UNIMP.
> + break;
> + case 0x0C: /* INT_MASK_x */
> + return;
> + case 0x10: /* INT_EN_x */
> + bank->imask &= ~value;
> + break;
> + case 0x14: /* INT_DIS_x */
> + bank->imask |= value;
> + break;
> + case 0x18: /* INT_STAT_x */
> + bank->istat &= ~value;
> + break;
> + case 0x1C: /* INT_TYPE_x */
> + bank->itype = value;
> + break;
> + case 0x20: /* INT_POLARITY_x */
> + bank->ipolarity = value;
> + break;
> + case 0x24: /* INT_ANY_x */
> + bank->iany = value;
> + break;
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "zynq_gpio_write: Bad offset %x\n", (int)offset);
> + return;
> + }
> +
> + zynq_gpio_update(s);
> +}
> +
> +static void zynq_gpio_reset(ZynqGPIOState *s)
> +{
> + int b;
> +
> + for (b = 0; b < 4; b++) {
> + s->banks[b].mask_data = 0x00000000;
> + s->banks[b].out_data = 0x00000000;
> + s->banks[b].dir = 0x00000000;
> + s->banks[b].oen = 0x00000000;
> + s->banks[b].imask = 0x00000000;
> + s->banks[b].istat = 0x00000000;
> + s->banks[b].itype = 0xffffffff;
> + s->banks[b].ipolarity = 0x00000000;
> + s->banks[b].iany = 0x00000000;
> + }
> +}
> +
> +static void zynq_gpio_set_irq(void * opaque, int irq, int level)
> +{
> + ZynqGPIOState *s = (ZynqGPIOState *)opaque;
> +
> + GPIOBank *bank = &s->banks[irq / 32];
> + uint32_t mask = 1 << (irq % 32);
> +
> + bank->in_data &= ~mask;
> + if (level)
> + bank->in_data |= mask;
> +
> + zynq_gpio_update_in(bank);
> +
> + zynq_gpio_set_in_irq(s);
> +}
> +
> +static void zynq_gpio_set_mio_irq(void * opaque, int irq, int level)
> +{
> + zynq_gpio_set_irq(opaque, irq + 0, level);
> +}
> +
> +static void zynq_gpio_set_emio_irq(void * opaque, int irq, int level)
> +{
> + zynq_gpio_set_irq(opaque, irq + 64, level);
> +}
You shouldn't need to re-linearise the IRQs as a set of 128 like this.
Following on from my comment about getting rid of MIO vs EMIO, what
you can probably do is qdev_init_gpio_in_named() the 4 banks in a 4x
loop and pass the bank pointer as opaque data instead. add the
containing ZynqGPIOState as a *parent field to the bank struct. Then
you do not need the /32 logic to do bank identification.
> +
> +static const MemoryRegionOps zynq_gpio_ops = {
> + .read = zynq_gpio_read,
> + .write = zynq_gpio_write,
> + .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static int zynq_gpio_initfn(SysBusDevice *sbd)
> +{
> + DeviceState *dev = DEVICE(sbd);
> + ZynqGPIOState *s = ZYNQ_GPIO(dev);
> +
> + s->banks[0].out = &s->mio_out[0];
> + s->banks[1].out = &s->mio_out[32];
> + s->banks[2].out = &s->emio_out[0];
> + s->banks[3].out = &s->emio_out[32];
This would disappear with getting rid of MIO/EMIO.
> +
> + memory_region_init_io(&s->iomem, OBJECT(s), &zynq_gpio_ops, s,
> "zynq_gpio", 0x1000);
> + sysbus_init_mmio(sbd, &s->iomem);
> + sysbus_init_irq(sbd, &s->irq);
> +
> + qdev_init_gpio_in_named(dev, zynq_gpio_set_mio_irq, "mio_in", 64);
> + qdev_init_gpio_in_named(dev, zynq_gpio_set_emio_irq, "emio_in", 64);
> +
> + qdev_init_gpio_out_named(dev, s->mio_out, "mio_out", 64);
> + qdev_init_gpio_out_named(dev, s->emio_out, "emio_out", 64);
> +
> + zynq_gpio_reset(s);
Don't reset in init fns. You shuold use a device-reset function ...
> +
> + return 0;
> +}
> +
> +static void zynq_gpio_class_init(ObjectClass *klass, void *data)
> +{
> + SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +
> + k->init = zynq_gpio_initfn;
... like this:
k->reset = zynq_gpio_reset;
Regards,
Peter
> +}
> +
> +static const TypeInfo zynq_gpio_info = {
> + .name = TYPE_ZYNQ_GPIO,
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(ZynqGPIOState),
> + .class_init = zynq_gpio_class_init,
> +};
> +
> +static void zynq_gpio_register_type(void)
> +{
> + type_register_static(&zynq_gpio_info);
> +}
> +
> +type_init(zynq_gpio_register_type)
> --
> 1.7.10.4
>
>