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

[Jean-Christophe DUBOIS]

Le 07/09/2015 04:01, Peter Crosthwaite a écrit :
> On Sun, Sep 6, 2015 at 1:45 PM, Jean-Christophe Dubois
> <address@hidden> wrote:
> > Signed-off-by: Jean-Christophe Dubois <address@hidden>
> > ---
> >
> > Changes since V1:
> >    * added "has-edge-sel" property
> >    * use extract64() and deposit64() in read/write icr access
> >    * set "number of GPIO pin" as a #define
> >    * reorganize functions in file to group them
> >    * various coding style cleanup
> >    * rename state handler array in output array.
> >
> >   hw/gpio/Makefile.objs      |   1 +
> >   hw/gpio/imx_gpio.c         | 347 +++++++++++++++++++++++++++++++++++++++++++++
> >   include/hw/gpio/imx_gpio.h |  63 ++++++++
> >   3 files changed, 411 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..fc43c73
> > --- /dev/null
> > +++ b/hw/gpio/imx_gpio.c
> > @@ -0,0 +1,347 @@
> > +/*
> > + * 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"
> > +
> Cutting from here [1] ...
>
> > +#ifdef DEBUG_GPIO
> This needs a less generic name, such as DEBUG_IMX_GPIO. This allows
> defining at configure as a -D CFLAG without other consequences.
OK
> > +#  define DPRINTF(fmt, args...) \
> > +          do { fprintf(stder, "%s: " fmt , __func__, ##args); } while (0)
> stderr.
OK (it is caught by the "always compile" scheme now)
> > +
> > +static char const *imx_gpio_reg_name(uint32_t reg)
> const char *. Just for consistency with code base. A grep of "const
> char \*" suggests this is the norm. We could add a CODING_STYLE.
OK
> > +{
> > +    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 "[?]";
> > +    }
> > +}
> > +#else /* DEBUG_GPIO */
> > +#  define DPRINTF(fmt, args...) do {} while (0)
> > +#endif /* DEBUG_GPIO */
> [1] ... to here. I would try something like:
>
> #ifndef DEBUG_GPIO
> #define DEBUG_GPIO 1
> #endif
>
> #define DPRINTF(fmt, args...) \
>            do { fprintf(stder, "%s: " fmt , __func__, ##args); } while (0)
>
> static char const *imx_gpio_reg_name(uint32_t reg)
> {
>      switch (reg) {
>      case DR_ADDR:
>          return "DR";
> ...
>          return "EDGE_SEL";
>      default:
>          return "[?]";
>      }
> }
>
> No, #else needed. As the DPRINTF bodies are always compiled you also
> need (and want) always-compilation of gpio_reg_name and without the
> unused static warn.
OK
> > +
> > +typedef enum IMXGPIOLevel {
> > +    IMX_GPIO_LEVEL_LOW = 0,
> > +    IMX_GPIO_LEVEL_HIGH = 1,
> > +} IMXGPIOLevel;
> > +
> > +static void imx_gpio_update_int(IMXGPIOState *s)
> > +{
> > +    qemu_set_irq(s->irq, s->isr ? 1 : 0);
> > +}
> > +
> > +static void imx_gpio_set_int_line(IMXGPIOState *s, int line, IMXGPIOLevel 
> > level)
> > +{
> > +    /* if this signal isn't configured as an interrupt source, nothing to do */
> > +    if (!extract32(s->imr, line, 1)) {
> > +        return;
> > +    }
> I had a read up of the TRM and found this:
>
> "
> Interrupt Status bits. Bit i of this register is asserted (active
> high) when the active condition is detected on the GPIO
> input and is waiting for service. The value of this register is
> independent of the value in the IMR register. When the
> active condition has been detected, the corresponding bit remains set
> until cleared by software.
> "
>
> IMR does not affect ISR, IMR only bitwise gates the top level
> interrupt signalling. ISR is unaffected. I think this short return
> should be dropped IIUC.

OK, then I have to also use IMR when raising interrupts in 
imx_gpio_update_int()
> > +
> > +    /* When set, EDGE_SEL override the ICR config */
> "overrides"
OK
> > +    if (extract32(s->edge_sel, line, 1)) {
> > +        /* we detect interrupt on rising and falling edge */
> > +        if (extract32(s->psr, line, 1) != level) {
> > +            /* 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 */
> > +            if (extract64(s->icr, 2*line, 1) != level) {
> > +                s->isr = deposit32(s->isr, line, 1, 1);
> > +            }
> > +        }
> > +    } else {
> > +        /* interrupt is level sensitive */
> > +        if (extract64(s->icr, 2*line, 1) == 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);
> > +    IMXGPIOLevel imx_level = level ? IMX_GPIO_LEVEL_HIGH : IMX_GPIO_LEVEL_LOW;
> > +
> > +    /* 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);
> > +        return;
> > +    }
> > +
> > +    imx_gpio_set_int_line(s, line, imx_level);
> > +
> > +    /* this is an input signal, so set PSR */
> > +    s->psr = deposit32(s->psr, line, 1, imx_level);
> > +
> > +    imx_gpio_update_int(s);
> > +}
> > +
> > +static void imx_gpio_set_all_int_lines(IMXGPIOState *s)
> > +{
> > +    int i;
> > +
> > +    for (i = 0; i < IMX_GPIO_MAX_PIN_COUNT; i++) {
> > +        IMXGPIOLevel imx_level = extract32(s->psr, i, 1);
> > +        imx_gpio_set_int_line(s, i, imx_level);
> > +    }
> > +
> > +    imx_gpio_update_int(s);
> > +}
> > +
> > +static inline void imx_gpio_set_all_output_lines(IMXGPIOState *s)
> > +{
> > +    int i;
> > +
> > +    for (i = 0; i < IMX_GPIO_MAX_PIN_COUNT; i++) {
> > +        /*
> > +         * if the line is set as output, then forward the line
> > +         * level to its user.
> > +         */
> > +        if (extract32(s->gdir, i, 1) && s->output[i]) {
> > +            qemu_set_irq(s->output[i], extract32(s->dr, i, 1));
> > +        }
> > +    }
> > +}
> > +
> > +static uint64_t imx_gpio_read(void *opaque, hwaddr offset, unsigned size)
> > +{
> > +    IMXGPIOState *s = IMX_GPIO(opaque);
> > +    uint32_t reg_value = 0;
> > +
> > +    switch (offset) {
> > +    case DR_ADDR:
> > +        /*
> > +         * depending on the "line" configuration, the bit values
> > +         * are coming either from DR or PSR
> > +         */
> > +        reg_value = (s->dr & s->gdir) | (s->psr & ~s->gdir);
> > +        break;
> > +
> > +    case GDIR_ADDR:
> > +        reg_value = s->gdir;
> > +        break;
> > +
> > +    case PSR_ADDR:
> > +        reg_value = s->psr;
> > +        break;
> > +
> > +    case ICR1_ADDR:
> > +        reg_value = extract64(s->icr, 0, 32);
> > +        break;
> > +
> > +    case ICR2_ADDR:
> > +        reg_value = extract64(s->icr, 32, 32);
> > +        break;
> > +
> > +    case IMR_ADDR:
> > +        reg_value = s->imr;
> > +        break;
> > +
> > +    case ISR_ADDR:
> > +        reg_value = s->isr;
> > +        break;
> > +
> > +    case EDGE_SEL_ADDR:
> > +        /* This register is not present in i.MX31 */
> I would remove all references to i.MX31, as this model is not specific
> to either 31 or 25. A comment like this could go in the 31 machine
> model however on the boolean prop setter.
OK
> > +        if (s->has_edge_sel) {
> > +            reg_value = s->edge_sel;
> > +        } else {
> > +            qemu_log_mask(LOG_GUEST_ERROR, "%s[%s]: EDGE_SEL register not "
> > +                          "present on imx31 type\n", TYPE_IMX_GPIO, __func__);
> -"imx31"
OK
> > +        }
> > +        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%"PRIx32"\n", imx_gpio_reg_name(offset), reg_value);
> I would work __func__ in there like with the LOG_GUEST_ERRORS to make
> it more identifiable in big logs.
It is taken care of by DPRINTF()
> > +
> > +    return reg_value;
> > +}
> > +
> > +static void imx_gpio_write(void *opaque, hwaddr offset, uint64_t value,
> > +                           unsigned size)
> > +{
> > +    IMXGPIOState *s = IMX_GPIO(opaque);
> > +
> > +    DPRINTF("(%s, value = 0x%"PRIx32")\n", imx_gpio_reg_name(offset),
> > +            (uint32_t)value);
> > +
> > +    switch (offset) {
> > +    case DR_ADDR:
> > +        s->dr = value;
> > +        imx_gpio_set_all_output_lines(s);
> > +        break;
> > +
> > +    case GDIR_ADDR:
> > +        s->gdir = value;
> > +        imx_gpio_set_all_output_lines(s);
> > +        imx_gpio_set_all_int_lines(s);
> So does this need to bitwise set PSR to the most input values on a
> transition from output to input? You may need extra state, a uint32_t
> which captures and saves the input state regardless or gdir. Then
> set_all_output_lines is renamed to set_psr() or something like,
> setting psr to input values on transitions (that is set_psr() will
> always set every pad value). I think the rest of the logic will just
> work, as forcing psr to the last (and current) input value will non
> trigger edge interrupts, which should be the case if my interpretation
> of TRM is correct.
>
> This handles cases where there is circuitry that can change
> directionality at runtime with the proper clamps (e.g. I2C).

So does this mean we should accept (as opposed to the v2 code) the 
"guest" setting the pin even if it is configured as output (just to be 
able to remember it has been set at some point)? And so without emitting 
a log to warm the user of the "strange behavior ...

Now is there so many "boards" using a GPIO both ways? You could if you 
were doing some kind of bitbanging i2C device for example but usually 
freescale devices have enough built in I2C devices to avoid this.

But OK, let's do it this way ...

> > +        break;
> > +
> > +    case ICR1_ADDR:
> > +        s->icr = deposit64(s->icr, 0, 32, value);
> > +        imx_gpio_set_all_int_lines(s);
> > +        break;
> > +
> > +    case ICR2_ADDR:
> > +        s->icr = deposit64(s->icr, 32, 32, value);
> > +        imx_gpio_set_all_int_lines(s);
> > +        break;
> > +
> > +    case IMR_ADDR:
> > +        s->imr = value;
> > +        imx_gpio_set_all_int_lines(s);
> Following on from IMR comment, this would be replacable with update_int().
OK, I changed update_int() to use IMR.
> > +        break;
> > +
> > +    case ISR_ADDR:
> > +        s->isr |= ~value;
> > +        imx_gpio_set_all_int_lines(s);
> > +        break;
> > +
> > +    case EDGE_SEL_ADDR:
> > +        /* This register is not present in i.MX31 */
> > +        if (s->has_edge_sel) {
> > +            s->edge_sel = value;
> > +            imx_gpio_set_all_int_lines(s);
> > +        } else {
> > +            qemu_log_mask(LOG_GUEST_ERROR, "%s[%s]: EDGE_SEL register not "
> > +                          "present on imx31 type\n", TYPE_IMX_GPIO, __func__);
> > +        }
> > +        break;
> > +
> > +    default:
> > +        qemu_log_mask(LOG_GUEST_ERROR, "%s[%s]: Bad register at offset %d\n",
> > +                      TYPE_IMX_GPIO, __func__, (int)offset);
> > +        break;
> > +    }
> > +
> Same comments as for read.
OK
> > +    return;
> > +}
> > +
> > +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 */
> Drop comment.
OK
> > +        VMSTATE_UINT32(edge_sel, IMXGPIOState),
> > +        VMSTATE_BOOL(has_edge_sel, IMXGPIOState),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> > +static Property imx_gpio_properties[] = {
> > +    DEFINE_PROP_BOOL("has-edge-sel", IMXGPIOState, has_edge_sel, true),
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> > +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_all_output_lines(s);
> > +    imx_gpio_update_int(s);
> > +}
> > +
> > +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, IMX_GPIO_MAX_PIN_COUNT);
> > +    qdev_init_gpio_out(DEVICE(s), s->output, IMX_GPIO_MAX_PIN_COUNT);
> > +
> > +    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->props = imx_gpio_properties;
> > +    dc->vmsd = &vmstate_imx_gpio;
> > +    dc->desc = "i.MX GPIO controller";
> > +}
> > +
> > +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..d91997a
> > --- /dev/null
> > +++ b/include/hw/gpio/imx_gpio.h
> > @@ -0,0 +1,63 @@
> > +/*
> > + * 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 /* DATA REGISTER */
> > +#define GDIR_ADDR           0x04 /* DIRECTION REGISTER */
> > +#define PSR_ADDR            0x08 /* PAD STATUS REGISTER */
> > +#define ICR1_ADDR           0x0c /* INTERRUPT CONFIGURATION REGISTER 1 */
> > +#define ICR2_ADDR           0x10 /* INTERRUPT CONFIGURATION REGISTER 2 */
> > +#define IMR_ADDR            0x14 /* INTERRUPT MASK REGISTER */
> > +#define ISR_ADDR            0x18 /* INTERRUPT STATUS REGISTER */
> > +#define EDGE_SEL_ADDR       0x1c /* EDGE SEL REGISTER */
> > +
> > +#define IMX_GPIO_MAX_PIN_COUNT 32
> > +
> MAX indicates it is configurable. Does the 25 and 31 down configure
> the IP to the max of 32? If so you can probably drop the MAX.
OK
>   If not,
> it should be converted to a property.

We will revisit this when we add modifications for i.MX6.

> Regards,
> Peter
>
> > +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;
> > +    bool has_edge_sel;
> > +    /* This register is not present in i.MX31 */
> > +    uint32_t edge_sel;
> > +
> > +    qemu_irq irq;
> > +    qemu_irq output[IMX_GPIO_MAX_PIN_COUNT];
> > +} IMXGPIOState;
> > +
> > +#endif /* __IMX_GPIO_H_ */
> > --
> > 2.1.4