Re: [Qemu-devel] [PATCH v1 1/3] i.MX: Add GPIO device

[Peter Crosthwaite]

On Sat, Sep 5, 2015 at 1:17 AM, Jean-Christophe Dubois
<address@hidden> wrote:
> Signed-off-by: Jean-Christophe Dubois <address@hidden>
> ---
>  hw/gpio/Makefile.objs      |   1 +
>  hw/gpio/imx_gpio.c         | 358 
> +++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/gpio/imx_gpio.h |  60 ++++++++
>  3 files changed, 419 insertions(+)
>  create mode 100644 hw/gpio/imx_gpio.c
>  create mode 100644 include/hw/gpio/imx_gpio.h
>
> diff --git a/hw/gpio/Makefile.objs b/hw/gpio/Makefile.objs
> index 1abcf17..52233f7 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_IMX) += imx_gpio.o
> diff --git a/hw/gpio/imx_gpio.c b/hw/gpio/imx_gpio.c
> new file mode 100644
> index 0000000..8ec1d4c
> --- /dev/null
> +++ b/hw/gpio/imx_gpio.c
> @@ -0,0 +1,358 @@
> +/*
> + * i.MX processors GPIO emulation.
> + *
> + * Copyright (C) 2015 Jean-Christophe Dubois <address@hidden>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 or
> + * (at your option) version 3 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "hw/gpio/imx_gpio.h"
> +
> +#ifndef IMX_GPIO_DEBUG
> +#define IMX_GPIO_DEBUG                 0
> +#endif
> +
> +#if IMX_GPIO_DEBUG
> +#  define DPRINTF(fmt, args...) \
> +          do { fprintf(stder, "%s: " fmt , __func__, ##args); } while (0)
> +

Use a regular if for debug conditional. This is so the debug code is
always compiled.

> +static char const *imx_gpio_reg_name(uint32_t reg)
> +{
> +    switch (reg) {
> +    case DR_ADDR:
> +        return "DR";
> +    case GDIR_ADDR:
> +        return "GDIR";
> +    case PSR_ADDR:
> +        return "PSR";
> +    case ICR1_ADDR:
> +        return "ICR1";
> +    case ICR2_ADDR:
> +        return "ICR2";
> +    case IMR_ADDR:
> +        return "IMR";
> +    case ISR_ADDR:
> +        return "ISR";
> +    case EDGE_SEL_ADDR:
> +        /* This register is not present in i.MX31 */
> +        return "EDGE_SEL";
> +    default:
> +        return "[?]";
> +    }

But I'm guessing this will be an issue for the non debug case. Perhaps
return "" from here in non-debug or just leave this always compiled
(optimisiser should be smart enough to trim it).

> +}
> +#else
> +#  define DPRINTF(fmt, args...) do {} while (0)
> +#endif
> +
> +static void imx_gpio_update_int(IMXGPIOState *s)
> +{
> +    if (s->isr) {
> +        qemu_irq_raise(s->irq);
> +    } else {
> +        qemu_irq_lower(s->irq);
> +    }

qemu_set_irq.

> +}
> +
> +static void imx_gpio_set_int_line(IMXGPIOState *s, int line, int level)
> +{
> +    /* is this signal configured as an interrupt source */
> +    if (extract32(s->imr, line, 1)) {

Short return on this condition (negated) rather than indent entire logic.

> +        /* When set EDGE_SEL override the ICR config */
> +        if (extract32(s->edge_sel, line, 1)) {
> +            /* we detect interrupt on rising and falling edge */
> +            if (extract32(s->psr, line, 1) != level) {

This is dangerous, as level is boolean and this will promote to
integer for comparison. I think last time people talked about this on
list, we decided that all users or GPIO setters should use 0 vs 1 but
it may not be the in-tree case. !!level will resolve.

> +                /* level changed */
> +                s->isr = deposit32(s->isr, line, 1, 1);
> +            }
> +        } else if (extract64(s->icr, 2*line + 1, 1)) {
> +            /* interrupt is edge sensitive */
> +            if (extract32(s->psr, line, 1) != level) {
> +                /* level changed */
> +                int falling = extract64(s->icr, 2*line, 1);
> +
> +                if ((falling && !level) || (!falling && level)) {

falling == !level

> +                    s->isr = deposit32(s->isr, line, 1, 1);
> +                }
> +            }
> +        } else {
> +            /* interrupt is level sensitive */
> +            int high = extract64(s->icr, 2*line, 1);
> +
> +            if ((high && level) || (!high && !level)) {

==

> +                s->isr = deposit32(s->isr, line, 1, 1);
> +            }
> +        }
> +    }
> +}
> +
> +static void imx_gpio_set(void *opaque, int line, int level)
> +{
> +    IMXGPIOState *s = IMX_GPIO(opaque);
> +
> +    /* if the line is configured as output nothing to do */
> +    if (extract32(s->gdir, line, 1)) {
> +        /* actually we should not be called */
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s[%s]: requesting to set line %d"
> +                      " to %d but this is an output line\n",
> +                      TYPE_IMX_GPIO, __func__, line, level);

Short return.

> +    } else {
> +        imx_gpio_set_int_line(s, line, level);
> +
> +        /* this is an input signal, so set PSR */
> +        s->psr = deposit32(s->psr, line, 1, level);
> +
> +        imx_gpio_update_int(s);
> +    }
> +}
> +
> +static void imx_gpio_set_all_int_lines(IMXGPIOState *s)
> +{
> +    int i;
> +
> +    for (i = 0; i < 32; i++) {

32 should be macroified.

> +        imx_gpio_set_int_line(s, i, extract32(s->psr, i, 1));
> +    }
> +
> +    imx_gpio_update_int(s);
> +}
> +
> +static uint64_t imx_gpio_read(void *opaque, hwaddr offset, unsigned size)
> +{
> +    IMXGPIOState *s = IMX_GPIO(opaque);
> +    uint32_t reg_value = 0;
> +    int i;
> +
> +    switch (offset) {
> +    case DR_ADDR: /* DATA REGISTER */
> +        for (i = 0; i < 32; i++) {
> +            uint32_t ref_value;
> +
> +            /*
> +             * depending on the "line" configuration, the bit values
> +             * are comming either from DR ou PSR

"coming" "or"

> +             */
> +            if (extract32(s->gdir, i, 1)) {
> +                ref_value = s->dr;
> +            } else {
> +                ref_value = s->psr;
> +            }
> +
> +            reg_value = deposit32(reg_value, i, 1,
> +                                  extract32(ref_value, i, 1));

This can be one-shot with some bitwise logicals to avoid the loop.

reg_value = s->dr & s->gdir | s->psr & ~s->gdir

I think.

> +        }
> +        break;
> +
> +    case GDIR_ADDR: /* DIRECTION REGISTER */

These trailing comments are duped between read and write handlers. I
would move to header as a more global single reference.

> +        reg_value = s->gdir;
> +        break;
> +
> +    case PSR_ADDR: /* PAD STATUS REGISTER */
> +        reg_value = s->psr;
> +        break;
> +
> +    case ICR1_ADDR: /* INTERRUPT CONFIGURATION REGISTER 1 */
> +        reg_value = (uint32_t) (s->icr & 0xffffffff);
> +        break;
> +
> +    case ICR2_ADDR: /* INTERRUPT CONFIGURATION REGISTER 2 */
> +        reg_value = (uint32_t) (s->icr >> 32);
> +        break;
> +
> +    case IMR_ADDR: /* INTERRUPT MASK REGISTER */
> +        reg_value = s->imr;
> +        break;
> +
> +    case ISR_ADDR: /* INTERRUPT STATUS REGISTER */
> +        reg_value = s->isr;
> +        break;
> +
> +    case EDGE_SEL_ADDR: /* EDGE SEL REGISTER */
> +        /* This register is not present in i.MX31 */
> +        reg_value = s->edge_sel;
> +        break;
> +
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s[%s]: Bad register at offset %d\n",
> +                      TYPE_IMX_GPIO, __func__, (int)offset);
> +        break;
> +    }
> +
> +    DPRINTF("(%s) = 0x%08x\n", imx_gpio_reg_name(offset), reg_value);

PRIx32.

> +
> +    return reg_value;
> +}
> +
> +static inline void imx_gpio_set_output(IMXGPIOState *s)

set_all_outputs to be consistent with set_all_int_lines. Group the two
functions and any similars together (either here or above with
int_lines).

> +{
> +    int i;
> +
> +    for (i = 0; i < 32; i++) {
> +        /*
> +         * if the line is set as output, then forward the line
> +         * level to its user.
> +         */
> +        if (extract32(s->gdir, i, 1) && s->handler[i]) {
> +            qemu_set_irq(s->handler[i], extract32(s->dr, i, 1));
> +        }
> +    }
> +}
> +
> +static void imx_gpio_write(void *opaque, hwaddr offset, uint64_t value,
> +                           unsigned size)
> +{
> +    IMXGPIOState *s = IMX_GPIO(opaque);
> +
> +    DPRINTF("(%s, value = 0x%08x)\n", imx_gpio_reg_name(offset),
> +            (uint32_t)value);

PRIx32.

> +
> +    switch (offset) {
> +    case DR_ADDR: /* DATA REGISTER */
> +        s->dr = (uint32_t)value;

cast un-needed.

> +
> +        imx_gpio_set_output(s);
> +
> +        break;
> +
> +    case GDIR_ADDR: /* DIRECTION REGISTER */
> +        s->gdir = (uint32_t)value;

cast un-needed (theres a few more below).

> +
> +        imx_gpio_set_output(s);
> +        imx_gpio_set_all_int_lines(s);
> +
> +        break;
> +
> +    case ICR1_ADDR: /* INTERRUPT CONFIGURATION REGISTER 1 */
> +        s->icr = (s->icr | 0xffffffff) & (uint32_t)value;

deposit64.

> +
> +        imx_gpio_set_all_int_lines(s);
> +
> +        break;
> +
> +    case ICR2_ADDR: /* INTERRUPT CONFIGURATION REGISTER 2 */
> +        s->icr = (s->icr | 0xffffffff00000000) & (value << 32);

deposit64.

> +
> +        imx_gpio_set_all_int_lines(s);
> +
> +        break;
> +
> +    case IMR_ADDR: /* INTERRUPT MASK REGISTER */
> +        s->imr = (uint32_t)value;
> +
> +        imx_gpio_set_all_int_lines(s);
> +
> +        break;
> +
> +    case ISR_ADDR: /* INTERRUPT STATUS REGISTER */
> +        s->isr |= ~(uint32_t)value;
> +
> +        imx_gpio_set_all_int_lines(s);
> +
> +        break;
> +
> +    case EDGE_SEL_ADDR: /* EDGE SEL REGISTER */
> +        /* This register is not present in i.MX31 */
> +        s->edge_sel = (uint32_t)value;
> +
> +        imx_gpio_set_all_int_lines(s);
> +
> +        break;
> +

I don't think you need the extra white. For short side effects it is
usual to just format as:

case ADDR:
    register = value;
    side_effects();
    break;

> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s[%s]: Bad register at offset %d\n",
> +                      TYPE_IMX_GPIO, __func__, (int)offset);
> +        break;
> +    }
> +
> +    return;
> +}
> +
> +static void imx_gpio_reset(DeviceState *dev)
> +{
> +    IMXGPIOState *s = IMX_GPIO(dev);
> +
> +    s->dr       = 0;
> +    s->gdir     = 0;
> +    s->psr      = 0;
> +    s->icr      = 0;
> +    s->imr      = 0;
> +    s->isr      = 0;
> +    s->edge_sel = 0;
> +
> +    imx_gpio_set_output(s);
> +    imx_gpio_update_int(s);
> +}
> +
> +static const MemoryRegionOps imx_gpio_ops = {
> +    .read = imx_gpio_read,
> +    .write = imx_gpio_write,
> +    .valid.min_access_size = 4,
> +    .valid.max_access_size = 4,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static const VMStateDescription vmstate_imx_gpio = {
> +    .name = TYPE_IMX_GPIO,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(dr, IMXGPIOState),
> +        VMSTATE_UINT32(gdir, IMXGPIOState),
> +        VMSTATE_UINT32(psr, IMXGPIOState),
> +        VMSTATE_UINT64(icr, IMXGPIOState),
> +        VMSTATE_UINT32(imr, IMXGPIOState),
> +        VMSTATE_UINT32(isr, IMXGPIOState),
> +        /* This register is not present in i.MX31 */
> +        VMSTATE_UINT32(edge_sel, IMXGPIOState),

You can make a boolean property of the device and turn its presence on
and off to get the functionality.

> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void imx_gpio_realize(DeviceState *dev, Error **errp)
> +{
> +    IMXGPIOState *s = IMX_GPIO(dev);
> +
> +    memory_region_init_io(&s->iomem, OBJECT(s), &imx_gpio_ops, s,
> +                          TYPE_IMX_GPIO, IMX_GPIO_MEM_SIZE);
> +
> +    qdev_init_gpio_in(DEVICE(s), imx_gpio_set, 32);
> +    qdev_init_gpio_out(DEVICE(s), s->handler, 32);
> +
> +    sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem);
> +}
> +
> +static void imx_gpio_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = imx_gpio_realize;
> +    dc->reset = imx_gpio_reset;
> +    dc->vmsd = &vmstate_imx_gpio;
> +    dc->desc = "i.MX GPIO controller";

dc->props = ...

for the "has-edge-sel" property.

> +}
> +
> +static const TypeInfo imx_gpio_info = {
> +    .name = TYPE_IMX_GPIO,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(IMXGPIOState),
> +    .class_init = imx_gpio_class_init,
> +};
> +
> +static void imx_gpio_register_types(void)
> +{
> +    type_register_static(&imx_gpio_info);
> +}
> +
> +type_init(imx_gpio_register_types)
> diff --git a/include/hw/gpio/imx_gpio.h b/include/hw/gpio/imx_gpio.h
> new file mode 100644
> index 0000000..6d3f149
> --- /dev/null
> +++ b/include/hw/gpio/imx_gpio.h
> @@ -0,0 +1,60 @@
> +/*
> + * i.MX processors GPIO registers definition.
> + *
> + * Copyright (C) 2015 Jean-Christophe Dubois <address@hidden>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 or
> + * (at your option) version 3 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef __IMX_GPIO_H_
> +#define __IMX_GPIO_H_
> +
> +#include <hw/sysbus.h>
> +
> +#define TYPE_IMX_GPIO "imx.gpio"
> +#define IMX_GPIO(obj) OBJECT_CHECK(IMXGPIOState, (obj), TYPE_IMX_GPIO)
> +
> +#define IMX_GPIO_MEM_SIZE 0x20
> +
> +/* i.MX GPIO memory map */
> +#define DR_ADDR             0x00
> +#define GDIR_ADDR           0x04
> +#define PSR_ADDR            0x08
> +#define ICR1_ADDR           0x0c
> +#define ICR2_ADDR           0x10
> +#define IMR_ADDR            0x14
> +#define ISR_ADDR            0x18
> +#define EDGE_SEL_ADDR       0x1c
> +
> +typedef struct IMXGPIOState {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +
> +    /*< public >*/
> +    MemoryRegion iomem;
> +
> +    uint32_t dr;
> +    uint32_t gdir;
> +    uint32_t psr;
> +    uint64_t icr;
> +    uint32_t imr;
> +    uint32_t isr;
> +    /* This register is not present in i.MX31 */
> +    uint32_t edge_sel;
> +
> +    qemu_irq irq;
> +    qemu_irq handler[32];

Should this just be "outputs"? Im guessing this is a bidirectional pin
which QEMU has trouble modelleing and this is the output mode variant
of that (hence it probably has no name in the TRM to use).

Regards,
Peter

> +} IMXGPIOState;
> +
> +#endif /* __IMX_GPIO_H_ */
> --
> 2.1.4
>
>