Re: [Qemu-devel] [PATCH v9 04/20] i.MX: Split AVIC emulator in a header file and a source file

[Peter Crosthwaite]

On Sat, Jul 4, 2015 at 7:34 AM, Jean-Christophe Dubois
<address@hidden> wrote:
> Signed-off-by: Jean-Christophe Dubois <address@hidden>
> ---
>
> Changes since v1:
>     * not present on v1
>
> Changes since v2:
>     * not present on v2
>
> Changes since v3:
>     * not present on v3
>
> Changes since v4:
>     * not present on v4
>
> Changes since v5:
>     * not present on v5
>
> Changes since v6:
>     * not present on v6
>
> Changes since v7:
>     * Splited the i.MX AVIC emulator into a header file and a source file
>
> Changes since v8:
>     * no changes
>
>  hw/intc/imx_avic.c         | 40 +++------------------------------
>  include/hw/intc/imx_avic.h | 56 
> ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 59 insertions(+), 37 deletions(-)
>  create mode 100644 include/hw/intc/imx_avic.h
>
> diff --git a/hw/intc/imx_avic.c b/hw/intc/imx_avic.c
> index e48f66c..c5eecb5 100644
> --- a/hw/intc/imx_avic.c
> +++ b/hw/intc/imx_avic.c
> @@ -7,6 +7,7 @@
>   * Copyright (c) 2008 OKL
>   * Copyright (c) 2011 NICTA Pty Ltd
>   * Originally written by Hans Jiang
> + * Updated by Jean-Christophe Dubois <address@hidden>
>   *
>   * This code is licensed under the GPL version 2 or later.  See
>   * the COPYING file in the top-level directory.
> @@ -14,9 +15,7 @@
>   * TODO: implement vectors.
>   */
>
> -#include "hw/hw.h"
> -#include "hw/sysbus.h"

> -#include "qemu/host-utils.h"

Curious, was this one un-needed in original code?

> +#include "hw/intc/imx_avic.h"
>
>  #define DEBUG_INT 1
>  #undef DEBUG_INT /* comment out for debugging */
> @@ -40,39 +39,6 @@ do { printf("imx_avic: " fmt , ##args); } while (0)
>  #  define IPRINTF(fmt, args...) do {} while (0)
>  #endif
>
> -#define IMX_AVIC_NUM_IRQS 64
> -
> -/* Interrupt Control Bits */
> -#define ABFLAG (1<<25)
> -#define ABFEN (1<<24)
> -#define NIDIS (1<<22) /* Normal Interrupt disable */
> -#define FIDIS (1<<21) /* Fast interrupt disable */
> -#define NIAD  (1<<20) /* Normal Interrupt Arbiter Rise ARM level */
> -#define FIAD  (1<<19) /* Fast Interrupt Arbiter Rise ARM level */
> -#define NM    (1<<18) /* Normal interrupt mode */
> -
> -
> -#define PRIO_PER_WORD (sizeof(uint32_t) * 8 / 4)
> -#define PRIO_WORDS (IMX_AVIC_NUM_IRQS/PRIO_PER_WORD)
> -
> -#define TYPE_IMX_AVIC "imx_avic"
> -#define IMX_AVIC(obj) \
> -    OBJECT_CHECK(IMXAVICState, (obj), TYPE_IMX_AVIC)
> -
> -typedef struct IMXAVICState {
> -    SysBusDevice parent_obj;
> -
> -    MemoryRegion iomem;
> -    uint64_t pending;
> -    uint64_t enabled;
> -    uint64_t is_fiq;
> -    uint32_t intcntl;
> -    uint32_t intmask;
> -    qemu_irq irq;
> -    qemu_irq fiq;
> -    uint32_t prio[PRIO_WORDS]; /* Priorities are 4-bits each */
> -} IMXAVICState;
> -
>  static const VMStateDescription vmstate_imx_avic = {
>      .name = "imx-avic",
>      .version_id = 1,
> @@ -370,7 +336,7 @@ static int imx_avic_init(SysBusDevice *sbd)
>      IMXAVICState *s = IMX_AVIC(dev);
>
>      memory_region_init_io(&s->iomem, OBJECT(s), &imx_avic_ops, s,
> -                          "imx_avic", 0x1000);
> +                          TYPE_IMX_AVIC, 0x1000);
>      sysbus_init_mmio(sbd, &s->iomem);
>
>      qdev_init_gpio_in(dev, imx_avic_set_irq, IMX_AVIC_NUM_IRQS);
> diff --git a/include/hw/intc/imx_avic.h b/include/hw/intc/imx_avic.h
> new file mode 100644
> index 0000000..1645ba6
> --- /dev/null
> +++ b/include/hw/intc/imx_avic.h
> @@ -0,0 +1,56 @@
> +/*
> + * i.MX31 Vectored Interrupt Controller
> + *
> + * Note this is NOT the PL192 provided by ARM, but
> + * a custom implementation by Freescale.
> + *
> + * Copyright (c) 2008 OKL
> + * Copyright (c) 2011 NICTA Pty Ltd
> + * Originally written by Hans Jiang
> + * Updated by Jean-Christophe Dubois <address@hidden>
> + *
> + * This code is licensed under the GPL version 2 or later.  See
> + * the COPYING file in the top-level directory.
> + *
> + * TODO: implement vectors.
> + */
> +#ifndef IMX_AVIC_H
> +#define IMX_AVIC_H
> +
> +#include "hw/sysbus.h"
> +
> +#define TYPE_IMX_AVIC "imx.avic"
> +#define IMX_AVIC(obj) OBJECT_CHECK(IMXAVICState, (obj), TYPE_IMX_AVIC)
> +
> +#define IMX_AVIC_NUM_IRQS 64
> +
> +/* Interrupt Control Bits */
> +#define ABFLAG (1<<25)
> +#define ABFEN (1<<24)
> +#define NIDIS (1<<22) /* Normal Interrupt disable */
> +#define FIDIS (1<<21) /* Fast interrupt disable */
> +#define NIAD  (1<<20) /* Normal Interrupt Arbiter Rise ARM level */
> +#define FIAD  (1<<19) /* Fast Interrupt Arbiter Rise ARM level */
> +#define NM    (1<<18) /* Normal interrupt mode */
> +
> +

One too many blanks.

> +#define PRIO_PER_WORD (sizeof(uint32_t) * 8 / 4)
> +#define PRIO_WORDS (IMX_AVIC_NUM_IRQS/PRIO_PER_WORD)
> +
> +typedef struct {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +
> +    /*< public >*/
> +    MemoryRegion iomem;
> +    uint64_t pending;
> +    uint64_t enabled;
> +    uint64_t is_fiq;
> +    uint32_t intcntl;
> +    uint32_t intmask;
> +    qemu_irq irq;
> +    qemu_irq fiq;
> +    uint32_t prio[PRIO_WORDS]; /* Priorities are 4-bits each */
> +} IMXAVICState;
> +
> +#endif // IMX_AVIC_H

convert to /* */ comment.

Otherwise:

Reviewed-by: Peter Crosthwaite <address@hidden>

Regards,
Peter

> --
> 2.1.4
>
>