From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [RFC PATCH v1 01/10] xen/arm: make secondary gic init as notifier call Date: Thu, 20 Mar 2014 12:48:18 +0000 Message-ID: <532AE392.1080107@linaro.org> References: <1395238631-2024-1-git-send-email-vijay.kilari@gmail.com> <1395238631-2024-2-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: <1395238631-2024-2-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: Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, Prasun.Kapoor@caviumnetworks.com, vijaya.kumar@caviumnetworks.com, xen-devel@lists.xen.org, stefano.stabellini@citrix.com List-Id: xen-devel@lists.xenproject.org Hello Vijay, Thanks for your patch. On 03/19/2014 02:17 PM, vijay.kilari@gmail.com wrote: > From: Vijaya Kumar K > > make gic init for secondary cpus as notifier call > instead calling directly from secondary boot for > each cpu. This makes secondary gic init generic and runtime. > > Signed-off-by: Vijaya Kumar K > --- > xen/arch/arm/gic.c | 35 ++++++++++++++++++++++++++--------- > xen/arch/arm/smpboot.c | 3 +-- > xen/include/asm-arm/gic.h | 2 -- > 3 files changed, 27 insertions(+), 13 deletions(-) > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index 91a2982..4be0897 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -20,6 +20,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -380,6 +381,30 @@ static void __cpuinit gic_hyp_disable(void) > GICH[GICH_HCR] = 0; > } > > +/* Set up the per-CPU parts of the GIC for a secondary CPU */ > +static int __cpuinit gic_init_secondary_cpu(struct notifier_block *nfb, > + unsigned long action, void *hcpu) > +{ > + if (action == CPU_STARTING) > + { > + spin_lock(&gic.lock); > + gic_cpu_init(); > + gic_hyp_init(); > + spin_unlock(&gic.lock); > + } > + return NOTIFY_DONE; > +} > + This is not the correct way to create a notifier block. You should have a good similar to (see an example in common/timer.c): static cpu_callback(struct notifier_block* nfb, unsigned long action, void *hcpu) { unsigned int cpu = (unsigned long)hcpu; switch ( action ) case CPU_STARTING: gic_init_secondary_cpu(); break; default: break; return NOTIFY_DONE; } > +static struct notifier_block gic_cpu_nb = { > + .notifier_call = gic_init_secondary_cpu, > + .priority = 100 > +}; > +static void gic_smp_init(void) > +{ > + register_cpu_notifier(&gic_cpu_nb); > +} > + You don't need to create a separate function to register the notifier. You can directly call it in gic_init. > int gic_irq_xlate(const u32 *intspec, unsigned int intsize, > unsigned int *out_hwirq, > unsigned int *out_type) > @@ -469,6 +494,7 @@ void __init gic_init(void) > spin_lock_init(&gic.lock); > spin_lock(&gic.lock); > > + gic_smp_init(); > gic_dist_init(); > gic_cpu_init(); > gic_hyp_init(); > @@ -524,15 +550,6 @@ void smp_send_state_dump(unsigned int cpu) > send_SGI_one(cpu, GIC_SGI_DUMP_STATE); > } > > -/* Set up the per-CPU parts of the GIC for a secondary CPU */ > -void __cpuinit gic_init_secondary_cpu(void) > -{ > - spin_lock(&gic.lock); > - gic_cpu_init(); > - gic_hyp_init(); > - spin_unlock(&gic.lock); > -} > - > /* Shut down the per-CPU GIC interface */ > void gic_disable_cpu(void) > { > diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c > index a829957..765efcf 100644 > --- a/xen/arch/arm/smpboot.c > +++ b/xen/arch/arm/smpboot.c > @@ -283,7 +283,7 @@ void __cpuinit start_secondary(unsigned long boot_phys_offset, > > mmu_init_secondary_cpu(); > > - gic_init_secondary_cpu(); > + notify_cpu_starting(cpuid); Can you explain why it's safe to move notify_cpu_starting earlier? > > init_secondary_IRQ(); > > @@ -297,7 +297,6 @@ void __cpuinit start_secondary(unsigned long boot_phys_offset, > setup_cpu_sibling_map(cpuid); > > /* Run local notifiers */ Please move also the comment, it's part of notify_cpu_starting. It doesn't make any sense alone here. > - notify_cpu_starting(cpuid); > wmb(); Regards, -- Julien Grall