Re: [Xen-devel] [PATCH v6 3/5] xen/arm: Make gic-v2 code handle hip04-d01 platform

[Frediano Ziglio <frediano.ziglio@xxxxxxxxxx>]

> 
> Hi Frediano,
> 
> On 26/02/15 12:40, Frediano Ziglio wrote:
> > diff --git a/xen/arch/arm/domain_build.c
> b/xen/arch/arm/domain_build.c
> > index c2dcb49..0834053 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -1038,6 +1038,7 @@ static int handle_node(struct domain *d, struct
> kernel_info *kinfo,
> >      static const struct dt_device_match gic_matches[] __initconst =
> >      {
> >          DT_MATCH_GIC_V2,
> > +        DT_MATCH_GIC_HIP04,
> >          DT_MATCH_GIC_V3,
> >          { /* sentinel */ },
> >      };
> 
> I think this is the perfect time to introduce a callback to check if
> the node is a GIC.
> 
> This would avoid to grow up this structure.
> 
> [..]
> 

Actually looking at assignment of dt_interrupt_controller (a dt_device_node) 
and calls to register_gic_ops there is a one-to-one correspondence so you can 
check if node == dt_interrupt_controller in handle_node instead of using a 
manually compiled gic_matches and checking with dt_match_node? This way I could 
even avoid to defined in a header the DT compatible string.

> > -static const char * const gicv2_dt_compat[] __initconst =
> > +static const char * const hip04gic_dt_compat[] __initconst =
> >  {
> > -    DT_COMPAT_GIC_CORTEX_A15,
> > -    DT_COMPAT_GIC_CORTEX_A7,
> > -    DT_COMPAT_GIC_400,
> > +    DT_COMPAT_GIC_HIP04,
> >      NULL
> >  };
> >
> > -DT_DEVICE_START(gicv2, "GICv2:", DEVICE_GIC)
> > -        .compatible = gicv2_dt_compat,
> > -        .init = gicv2_init,
> > +DT_DEVICE_START(hip04gic, "GIC-HIP04:", DEVICE_GIC)
> 
> The ":" was a mistake in the GICv2. Please don't reproduce it here.
> 
> > +        .compatible = hip04gic_dt_compat,
> > +        .init = hip04gic_init,
> >  DT_DEVICE_END
> >
> >  /*
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index
> > 390c8b0..e4512a8 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -565,12 +565,13 @@ static void do_sgi(struct cpu_user_regs *regs,
> > enum gic_sgi sgi)  void gic_interrupt(struct cpu_user_regs *regs, int
> > is_fiq)  {
> >      unsigned int irq;
> > +    unsigned int max_irq = gic_hw_ops->info->nr_lines;
> >
> >      do  {
> >          /* Reading IRQ will ACK it */
> >          irq = gic_hw_ops->read_irq();
> >
> > -        if ( likely(irq >= 16 && irq < 1021) )
> > +        if ( likely(irq >= 16 && irq < max_irq) )
> >          {
> >              local_irq_enable();
> >              do_IRQ(regs, irq, is_fiq);
> 
> This change should belong to a separate patch.
> 
> > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> > index 187dc46..a36f486 100644
> > --- a/xen/include/asm-arm/gic.h
> > +++ b/xen/include/asm-arm/gic.h
> > @@ -160,6 +160,10 @@
> >                          DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_CORTEX_A7),
> \
> >                          DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_400)
> >
> > +#define DT_COMPAT_GIC_HIP04          "hisilicon,hip04-intc"
> > +
> > +#define DT_MATCH_GIC_HIP04 DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_HIP04)
> > +
> >  #define DT_COMPAT_GIC_V3             "arm,gic-v3"
> >
> >  #define DT_MATCH_GIC_V3 DT_MATCH_COMPATIBLE(DT_COMPAT_GIC_V3)
> 
> I would prefer if we avoid to add more compatibles like that in gic.h.
> 
> I have a patch to drop a part of this mess. I would advise your to use
> cherry-pick the commit [1] in your branch.
> 
> [1]
> http://xenbits.xen.org/gitweb/?p=people/julieng/xen-
> unstable.git;a=commit;h=7ba87910e557b06c589c3c0fbc6757fa664d029e
> 
> Regards,
> 
> --
> Julien Grall

Is this patch in staging?

Frediano


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel