From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH v2 05/15] xen/arm: segregate GIC low level functionality Date: Wed, 9 Apr 2014 16:54:10 +0100 Message-ID: <1397058850.6275.132.camel@kazak.uk.xensource.com> References: <1396612593-443-1-git-send-email-vijay.kilari@gmail.com> <1396612593-443-6-git-send-email-vijay.kilari@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1396612593-443-6-git-send-email-vijay.kilari@gmail.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: vijay.kilari@gmail.com Cc: stefano.stabellini@eu.citrix.com, Prasun.Kapoor@caviumnetworks.com, vijaya.kumar@caviumnetworks.com, julien.grall@linaro.org, xen-devel@lists.xen.org, stefano.stabellini@citrix.com List-Id: xen-devel@lists.xenproject.org On Fri, 2014-04-04 at 17:26 +0530, vijay.kilari@gmail.com wrote: > From: Vijaya Kumar K > > GIC low level functionality is segregated into > separate functions and are called using registered > callback wherever required. > > This helps to separate generic and hardware functionality > later > > Signed-off-by: Vijaya Kumar K > --- > xen/arch/arm/gic.c | 362 ++++++++++++++++++++++++++++--------- > xen/include/asm-arm/gic.h | 50 +++++ > xen/include/asm-arm/gic_v2_defs.h | 16 +- > 3 files changed, 328 insertions(+), 100 deletions(-) > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index 64699e4..9f03135 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -57,8 +57,21 @@ static irq_desc_t irq_desc[NR_IRQS]; > static DEFINE_PER_CPU(irq_desc_t[NR_LOCAL_IRQS], local_irq_desc); > static DEFINE_PER_CPU(uint64_t, lr_mask); > > +static struct gic_hw_operations *gic_hw_ops; > +static struct gic_hw_operations gic_ops; Why two? Is the second one actually gic_v2_ops? > +void register_gic_ops(struct gic_hw_operations *ops) > +{ > + gic_hw_ops = ops; > +} > + > +void update_cpu_lr_mask(void) > +{ > + this_cpu(lr_mask) = 0ULL; > +} This looks more like init_cpu_lr_mask. > + > static uint8_t nr_lrs; Shouldn't everywhere be using gic_hw_ops->nr_lrs() rendering this unused? > -#define lr_all_full() (this_cpu(lr_mask) == ((1 << nr_lrs) - 1)) > +#define lr_all_full() (this_cpu(lr_mask) == ((1 << gic_hw_ops->nr_lrs()) - 1)) I think nr_lrs can just be an integer field of gic_hw_ops, rather than using a function to return it, or perhaps a global in gic. set by the lower level driver somehow. Unless nr_lrs is somehow for gicv3? > +static void restore_state(struct vcpu *v) I suspect that all of these callbacks shoujld actually be gic_v2_restore_state or some such? > @@ -230,7 +319,7 @@ static hw_irq_controller gic_guest_irq_type = { > * - needs to be called with a valid cpu_mask, ie each cpu in the mask has > * already called gic_cpu_init > */ > -static void gic_set_irq_properties(unsigned int irq, bool_t level, > +static void gic_set_irq_property(unsigned int irq, bool_t level, > const cpumask_t *cpu_mask, > unsigned int priority) Why did this need to become singular? > +static struct dt_irq * gic_maintenance_irq(void) > +{ > + return &gic.maintenance; > +} This isn't using gic_hw_ops, so what is it for? Ian.