Re: [Qemu-devel] [PATCH 1/2] hw/exynos4210_gic.c: Introduce n_in and n_out propeties.

[Evgeny Voevodin]

On 11.04.2012 19:17, Peter Maydell wrote:
> On 11 April 2012 08:48, Evgeny Voevodin<address@hidden>  wrote:
> > With these properties irq gate could be tuned to mux up to
> > QDEV_MAX_IRQ inputs and ouputs. Gate will group inputs
> > into groups of size n_in/n_out each.
> >
> > Signed-off-by: Evgeny Voevodin<address@hidden>
> > ---
> >   hw/exynos4210_gic.c |   60 ++++++++++++++++++++++++++++++++------------------
> >   1 files changed, 38 insertions(+), 22 deletions(-)
> >
> > diff --git a/hw/exynos4210_gic.c b/hw/exynos4210_gic.c
> > index ec13140..1fe8225 100644
> > --- a/hw/exynos4210_gic.c
> > +++ b/hw/exynos4210_gic.c
> > @@ -361,18 +361,29 @@ type_init(exynos4210_gic_register_types)
> >   typedef struct {
> >      SysBusDevice busdev;
> >
> > -    qemu_irq pic_irq[NCPU]; /* output IRQs to PICs */
> > -    uint32_t gpio_level[EXYNOS4210_IRQ_GATE_NINPUTS]; /* Input levels */
> > +    qemu_irq out[QDEV_MAX_IRQ]; /* output IRQs */
> > +    uint32_t n_out;             /* outputs amount */
> > +    uint32_t gpio_level[QDEV_MAX_IRQ]; /* Input levels */
> > +    uint32_t n_in;              /* inputs amount */
> > +    uint32_t group_size;         /* input group size */
> >   } Exynos4210IRQGateState;
> >
> > +static Property exynos4210_irq_gate_properties[] = {
> > +    DEFINE_PROP_UINT32("n_out", Exynos4210IRQGateState, n_out, 1),
> > +    DEFINE_PROP_UINT32("n_in", Exynos4210IRQGateState, n_in, 1),
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> >   static const VMStateDescription vmstate_exynos4210_irq_gate = {
> >      .name = "exynos4210.irq_gate",
> >      .version_id = 1,
> >      .minimum_version_id = 1,
> >      .minimum_version_id_old = 1,
> >      .fields = (VMStateField[]) {
> > -        VMSTATE_UINT32_ARRAY(gpio_level, Exynos4210IRQGateState,
> > -                EXYNOS4210_IRQ_GATE_NINPUTS),
> > +        VMSTATE_UINT32(n_out, Exynos4210IRQGateState),
> > +        VMSTATE_UINT32_ARRAY(gpio_level, Exynos4210IRQGateState, QDEV_MAX_IRQ),
> > +        VMSTATE_UINT32(n_in, Exynos4210IRQGateState),
> > +        VMSTATE_UINT32(group_size, Exynos4210IRQGateState),
>
> VMState field changes require a version bump. I think we can forget
> backcompatibility in this case, so just set all of version_id,
> minumum_version_id and minimum_version_id_old to 2.

Ok!

> >          VMSTATE_END_OF_LIST()
> >      }
> >   };
> > @@ -382,25 +393,24 @@ static void exynos4210_irq_gate_handler(void *opaque, int 
> > irq, int level)
> >   {
> >      Exynos4210IRQGateState *s =
> >              (Exynos4210IRQGateState *)opaque;
> > -    uint32_t odd, even;
> > -
> > -    if (irq&  1) {
> > -        odd = irq;
> > -        even = irq&  ~1;
> > -    } else {
> > -        even = irq;
> > -        odd = irq | 1;
> > -    }
> > +    uint32_t n_out, n_group, i;
> > +
> > +    assert(irq<  s->n_in);
> > +
> > +    n_out = irq / s->group_size;
> > +    n_group = n_out * s->group_size;
> >
> > -    assert(irq<  EXYNOS4210_IRQ_GATE_NINPUTS);
> >      s->gpio_level[irq] = level;
> >
> > -    if (s->gpio_level[odd]>= 1 || s->gpio_level[even]>= 1) {
> > -        qemu_irq_raise(s->pic_irq[even>>  1]);
> > -    } else {
> > -        qemu_irq_lower(s->pic_irq[even>>  1]);
> > +    for (i = 0; i<  s->group_size; i++) {
> > +        if (s->gpio_level[n_group + i]>= 1) {
> > +            qemu_irq_raise(s->out[n_out]);
> > +            return;
> > +        }
> >      }
> >
> > +    qemu_irq_lower(s->out[n_out]);
> > +
> >      return;
> >   }
> >
> > @@ -420,14 +430,19 @@ static int exynos4210_irq_gate_init(SysBusDevice *dev)
> >      Exynos4210IRQGateState *s =
> >              FROM_SYSBUS(Exynos4210IRQGateState, dev);
> >
> > +    /* Gate will make each input group of size n_in / n_out */
> > +    assert((s->n_in % s->n_out) == 0);
>
> The right way to check for validity of properties in the init
> function is to use hw_error() with a reasonably friendly message.

Ok! What's about irq handlers? Is assert() possible?

> > +
> > +    s->group_size = s->n_in / s->n_out;
> > +
> >      /* Allocate general purpose input signals and connect a handler to each of
> >       * them */
> >      qdev_init_gpio_in(&s->busdev.qdev, exynos4210_irq_gate_handler,
> > -            EXYNOS4210_IRQ_GATE_NINPUTS);
> > +            s->n_in);
>
> I think this whole function call will fit on a single line without
> going over the 80 column limit now.

Ok!

> -- PMM

Thanks for review!

--
Kind regards,
Evgeny Voevodin,
Leading Software Engineer,
ASWG, Moscow R&D center, Samsung Electronics
e-mail: address@hidden