[1/2] zynq_gpio: GPIO model for Zynq SoC

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 <colin.leitner@gmail.com>
---
 hw/gpio/Makefile.objs |    1 +
 hw/gpio/zynq_gpio.c   |  441 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 442 insertions(+)
 create mode 100644 hw/gpio/zynq_gpio.c

On Tue, Dec 30, 2014 at 5:13 AM, Colin Leitner
<colin.leitner@googlemail.com> 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 <colin.leitner@gmail.com>
> ---
>  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 <colin.leitner@gmail.com>
> + *
> + * 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
>
>

Hi Peter,

thanks for the review! I'll rework the patch ASAP.

> 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.

The reason for chosing the MIO/EMIO names was simply for easier mapping
to real hardware where I've been usually confronted with MIO/EMIO.
Changing this to the banks makes sense of course if Xilinx choses to
reuse that IP again.

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

This is a fragment from pl061. If we don't explicitly drive a output
line through the direction register, we assume it floats high. We
still have to drive the qemu IRQ line to some state.

I'll write a better comment.

>> +    zynq_gpio_reset(s);
> 
> Don't reset in init fns. You shuold use a device-reset function ...

Another 1:1 copy from pl061. I'll take the time to read up how the
device model is meant to be implemented correctly.

Thanks again for the review and you'll hear from me shortly.

Regards,
	Colin