[v1,1/3] i.MX: Add GPIO device

Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
---
 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

On Sat, Sep 5, 2015 at 1:17 AM, Jean-Christophe Dubois
<jcd@tribudubois.net> wrote:
> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
> ---
>  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 <jcd@tribudubois.net>
> + *
> + * 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 <jcd@tribudubois.net>
> + *
> + * 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
>
>

Le 05/09/2015 11:03, Peter Crosthwaite a écrit :
> On Sat, Sep 5, 2015 at 1:17 AM, Jean-Christophe Dubois
> <jcd@tribudubois.net> wrote:
>> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
>> ---
>>   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 <jcd@tribudubois.net>
>> + *
>> + * 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.
Not sure what you mean here ... Most files in qemu have DEBUG functions 
compiled out in the normal case ...

Is there a change in "politic" on this topic?

Could you point to an implementation doing things right?
>
>> +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.
OK
>
>> +}
>> +
>> +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.
OK
>
>> +        /* 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.
OK, I'll work out something with an enum to be explicit.
>
>> +                /* 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
OK
>
>> +                    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)) {
> ==
OK
>
>> +                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.
OK
>
>> +    } 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.
OK
>
>> +        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"
OK
>
>> +             */
>> +            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.
OK
>
>> +        }
>> +        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.
I was thinking it was more helpful here to help understand the code 
without having to refer to a header file.
The point is that the abreviated spec name is not always very helpful.
But, OK, I'll move them to the header file ...
>
>> +        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.
OK
>
>> +
>> +    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).

OK
>
>> +{
>> +    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.
OK
>
>> +
>> +    switch (offset) {
>> +    case DR_ADDR: /* DATA REGISTER */
>> +        s->dr = (uint32_t)value;
> cast un-needed.
OK
>
>> +
>> +        imx_gpio_set_output(s);
>> +
>> +        break;
>> +
>> +    case GDIR_ADDR: /* DIRECTION REGISTER */
>> +        s->gdir = (uint32_t)value;
> cast un-needed (theres a few more below).
OK
>
>> +
>> +        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.
OK
>
>> +
>> +        imx_gpio_set_all_int_lines(s);
>> +
>> +        break;
>> +
>> +    case ICR2_ADDR: /* INTERRUPT CONFIGURATION REGISTER 2 */
>> +        s->icr = (s->icr | 0xffffffff00000000) & (value << 32);
> deposit64.
OK
>
>> +
>> +        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;
OK
>
>> +    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.
OK
>
>> +        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.
OK
>
>> +}
>> +
>> +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 <jcd@tribudubois.net>
>> + *
>> + * 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).
OK
>
> Regards,
> Peter
>
>> +} IMXGPIOState;
>> +
>> +#endif /* __IMX_GPIO_H_ */
>> --
>> 2.1.4
>>
>>

On Sun, Sep 6, 2015 at 5:55 AM, Jean-Christophe DUBOIS
<jcd@tribudubois.net> wrote:
> Le 05/09/2015 11:03, Peter Crosthwaite a écrit :
>>
>> On Sat, Sep 5, 2015 at 1:17 AM, Jean-Christophe Dubois
>> <jcd@tribudubois.net> wrote:
>>>
>>> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
>>> ---
>>>   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 <jcd@tribudubois.net>
>>> + *
>>> + * 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.
>
> Not sure what you mean here ... Most files in qemu have DEBUG functions
> compiled out in the normal case ...
>

We are trying to slowly change that.

> Is there a change in "politic" on this topic?
>

No this has been the policy for a while now.

> Could you point to an implementation doing things right?
>

hw/dma/pl330.c

Regards,
Peter

>>
>>> +static char const *imx_gpio_reg_name(uint32_t reg)
>>> +{
>>> +    switch (reg) {
>>> +    case DR_ADDR:
>>> +        return "DR";
>>> +    case GDIR_ADDR:

Le 06/09/2015 20:23, Peter Crosthwaite a écrit :
> On Sun, Sep 6, 2015 at 5:55 AM, Jean-Christophe DUBOIS
> <jcd@tribudubois.net> wrote:
>> Le 05/09/2015 11:03, Peter Crosthwaite a écrit :
>>> On Sat, Sep 5, 2015 at 1:17 AM, Jean-Christophe Dubois
>>> <jcd@tribudubois.net> wrote:
>>>> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
>>>> ---
>>>>    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 <jcd@tribudubois.net>
>>>> + *
>>>> + * 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.
>> Not sure what you mean here ... Most files in qemu have DEBUG functions
>> compiled out in the normal case ...
>>
> We are trying to slowly change that.
>
>> Is there a change in "politic" on this topic?
>>
> No this has been the policy for a while now.
>
>> Could you point to an implementation doing things right?
>>
> hw/dma/pl330.c

Not sure, I understand. It is still compiled out as PL330_ERR_DEBUG is 0 
in the "normal" case.

JC

>
> Regards,
> Peter
>
>>>> +static char const *imx_gpio_reg_name(uint32_t reg)
>>>> +{
>>>> +    switch (reg) {
>>>> +    case DR_ADDR:
>>>> +        return "DR";
>>>> +    case GDIR_ADDR:

On Sun, Sep 6, 2015 at 2:19 PM, Jean-Christophe DUBOIS
<jcd@tribudubois.net> wrote:
> Le 06/09/2015 20:23, Peter Crosthwaite a écrit :
>>
>> On Sun, Sep 6, 2015 at 5:55 AM, Jean-Christophe DUBOIS
>> <jcd@tribudubois.net> wrote:
>>>
>>> Le 05/09/2015 11:03, Peter Crosthwaite a écrit :
>>>>
>>>> On Sat, Sep 5, 2015 at 1:17 AM, Jean-Christophe Dubois
>>>> <jcd@tribudubois.net> wrote:
>>>>>
>>>>> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
>>>>> ---
>>>>>    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 <jcd@tribudubois.net>
>>>>> + *
>>>>> + * 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.
>>>
>>> Not sure what you mean here ... Most files in qemu have DEBUG functions
>>> compiled out in the normal case ...
>>>
>> We are trying to slowly change that.
>>
>>> Is there a change in "politic" on this topic?
>>>
>> No this has been the policy for a while now.
>>
>>> Could you point to an implementation doing things right?
>>>
>> hw/dma/pl330.c
>
>
> Not sure, I understand. It is still compiled out as PL330_ERR_DEBUG is 0 in
> the "normal" case.
>

It is not compiled out textually, it is "if (0) {}"''d.

#ifndef PL330_ERR_DEBUG
#define PL330_ERR_DEBUG 0
#endif

#define DB_PRINT_L(lvl, fmt, args...) do {\
    if (PL330_ERR_DEBUG >= lvl) {\
        fprintf(stderr, "PL330: %s:" fmt, __func__, ## args);\
    } \
} while (0);

Note that #define DB_PRINT_L has no conditioning #ifdef which is what
we are going for. The idea is, even if the code is never run, it must
always still be compiled. This catches issues at compile time, when
major changes happen to core headers etc.

Regards,
Peter

> JC
>
>
>>
>> Regards,
>> Peter
>>
>>>>> +static char const *imx_gpio_reg_name(uint32_t reg)
>>>>> +{
>>>>> +    switch (reg) {
>>>>> +    case DR_ADDR:
>>>>> +        return "DR";
>>>>> +    case GDIR_ADDR:
>
>