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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxxx>]

> 
> Hello Frediano,
> 
> On 03/03/15 11:19, Frediano Ziglio wrote:
> > The GIC in this platform is mainly compatible with the standard
> > GICv2 beside:
> > - ITARGET is extended to 16 bit to support 16 CPUs;
> > - SGI mask is extended to support 16 CPUs;
> > - maximum supported interrupt is 510;
> 
> 510 is not a multiple of 32. Is it normal?
> 
> This will result to having nr_lines = 512. What happen is we are trying
> to access IRQ 510 and 511?
> 

I don't know. I think it's the same reason why in xen/arch/arm/gic.c the limit 
for irq is 1021 and not 1024 (see "if ( likely(irq >= 16 && irq < 1021) )" line)

> Also, is it possible to have GICH.VirtualID >= 510?
> 

I think so, GICH have the same interface of normal GICv2.

...
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index 0f04742..b17aab1 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -138,6 +138,11 @@ F:     xen/drivers/char/omap-uart.c
> >  F: xen/drivers/char/pl011.c
> >  F: xen/drivers/passthrough/arm/
> >
> > +HISILICON HIP04 SUPPORT
> > +M: Frediano Ziglio <frediano.ziglio@xxxxxxxxxx>
> 
> It might be good to have 2 maintainers form Huawei on this file. Ian
> any though?
> 

Added Zoltan Kiss.

> > +S: Supported
> > +F: xen/arch/arm/git-hip04.c
> 
> gic-hip04.c
> 

Too many git commands :)

...
> > 20cdbc9..94abdc4 100644
> > --- a/xen/arch/arm/gic-hip04.c
> > +++ b/xen/arch/arm/gic-hip04.c
> > @@ -1,7 +1,8 @@
> >  /*
> > - * xen/arch/arm/gic-v2.c
> > + * xen/arch/arm/gic-hip04.c
> >   *
> > - * ARM Generic Interrupt Controller support v2
> > + * Generic Interrupt Controller for HiSilicon Hip04 platform
> > + * Based heavily from gic-v2.c
> 
> Please add a commit ID. It would help you to keep track of the GIC.
> 
> >   *
> >   * Tim Deegan <tim@xxxxxxx>
> >   * Copyright (c) 2011 Citrix Systems.
> > @@ -71,59 +72,69 @@ static struct {
> >      void __iomem * map_hbase; /* IO Address of virtual interface
> registers */
> >      paddr_t vbase;            /* Address of virtual cpu interface
> registers */
> >      spinlock_t lock;
> > -} gicv2;
> > +} hip04gic;
> >
> > -static struct gic_info gicv2_info;
> > +static struct gic_info hip04gic_info;
> 
> I think the renaming of gicv2 and gicv2_info is pointless here. Instead
> of function name, it doesn't help for debugging.
> 
> It would also reduce the diff of this patch.
> 

No problem

> [..]
> 
> > -DT_DEVICE_START(gicv2, "GICv2", DEVICE_GIC)
> > -        .dt_match = gicv2_dt_match,
> > -        .init = gicv2_init,
> > +DT_DEVICE_START(hip04gic, "GIC-HIP04", DEVICE_GIC)
> > +    .dt_match = hip04gic_dt_match,
> > +    .init = hip04gic_init,
> >  DT_DEVICE_END
> 
> Please keep the same indentation as before.
> 

I was wondering why the indentation is different. Ok

Frediano


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