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

[Jean-Christophe DUBOIS]

Le 09/09/2015 09:09, Peter Crosthwaite a écrit :
> On Tue, Sep 8, 2015 at 5:38 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.
> >
> > Changes since v2:
> >    * always compile DEBUG functions
> >    * Fix imx_gpio_update_int to use isr, imr and gdir
> >    * Fix imx_gpio_set_int_line to check gdir instead of imr
> >    * Fix imx_gpio_set to update psr even if line is output
> >    * Fix PSR read to use gdir.
> >
> >   hw/gpio/Makefile.objs      |   1 +
> >   hw/gpio/imx_gpio.c         | 340 +++++++++++++++++++++++++++++++++++++++++++++
> >   include/hw/gpio/imx_gpio.h |  63 +++++++++
> >   3 files changed, 404 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..f8d7ef8
> > --- /dev/null
> > +++ b/hw/gpio/imx_gpio.c
> > @@ -0,0 +1,340 @@
> > +/*
> > + * 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 DEBUG_IMX_GPIO
> > +#define DEBUG_IMX_GPIO 0
> > +#endif
> > +
> > +typedef enum IMXGPIOLevel {
> > +    IMX_GPIO_LEVEL_LOW = 0,
> > +    IMX_GPIO_LEVEL_HIGH = 1,
> > +} IMXGPIOLevel;
> > +
> > +#define DPRINTF(fmt, args...) \
> > +          do { \
> > +              if (DEBUG_IMX_GPIO) { \
> > +                  fprintf(stderr, "%s: " fmt , __func__, ##args); \
> > +              } \
> > +          } while (0)
> > +
> > +static const char *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:
> > +        return "EDGE_SEL";
> > +    default:
> > +        return "[?]";
> > +    }
> > +}
> > +
> > +static void imx_gpio_update_int(IMXGPIOState *s)
> > +{
> > +    qemu_set_irq(s->irq, (s->isr & s->imr & ~s->gdir) ? 1 : 0);
> This doesn't look right, having it conditional on the gdir. I think
> you already implement the gdir masking in set_int_line, why do you
> need this extra mask?
This might be extra cautious (and unnecessary) but in order to raise an 
interrupt a "line"/pin needs to be set as an input line.

But OK, I'll remove it as it is also taken care of elsewhere (set_int_line).
> > +}
> > +
> > +static void imx_gpio_set_int_line(IMXGPIOState *s, int line, IMXGPIOLevel 
> > level)
> > +{
> > +    /* if this signal isn't configured as an input signal, nothing to do */
> >
> > +/* 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_PIN_COUNT 32
> > +
> > +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 */
> Drop comment.
OK
> Otherwise,
>
> Reviewed-by: Peter Crosthwaite <address@hidden>
>
> Regards,
> Peter
>
> > +    uint32_t edge_sel;
> > +
> > +    qemu_irq irq;
> > +    qemu_irq output[IMX_GPIO_PIN_COUNT];
> > +} IMXGPIOState;
> > +
> > +#endif /* __IMX_GPIO_H_ */
> > --
> > 2.1.4