From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [RFC PATCH v1 06/10] xen/arm: split gic driver into generic and gicv2 driver Date: Sun, 23 Mar 2014 15:05:37 +0000 Message-ID: <532EF841.5010701@linaro.org> References: <1395238631-2024-1-git-send-email-vijay.kilari@gmail.com> <1395238631-2024-7-git-send-email-vijay.kilari@gmail.com> <532B1114.7030002@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: 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 Cc: Ian Campbell , Stefano Stabellini , Prasun Kapoor , Vijaya Kumar K , xen-devel@lists.xen.org, Stefano Stabellini List-Id: xen-devel@lists.xenproject.org Hello Vijay, On 22/03/14 09:40, Vijay Kilari wrote: > On Thu, Mar 20, 2014 at 9:32 PM, Julien Grall wrote: >> Hello Vijay, >> >> Thanks for the patch. >> >> On 03/19/2014 02:17 PM, vijay.kilari@gmail.com wrote: >>> From: Vijaya Kumar K >>> >>> Existing GIC driver has both generic code and hw specific >>> code. Segregate GIC low level driver into gic-v2.c and >>> keep generic code in existing gic.c file >>> >>> GIC v2 driver registers required functions >>> to generic GIC driver. This helps to plug in next version >>> of GIC drivers like GIC v3. >>> >>> Signed-off-by: Vijaya Kumar K >>> --- >> >> [..] >> >>> + >>> +void __init gicv2_init(void) >>> +{ >> >> Instead of calling gicv2_init and gicv3_init from generic, it would be >> better to the device API (see xen/include/asm-arm/device.h). An example >> of use is my iommu series (see https://patches.linaro.org/26032/ >> iommu_hardware_setup). >> > OK. I will check > >> [..] >> >>> -void send_SGI_one(unsigned int cpu, enum gic_sgi sgi) >>> +static void send_SGI_one(unsigned int cpu, enum gic_sgi sgi) >> >> Why did you put static here? >> > I think it is not being called from outside of this file. > Should I keep it non static for future use? I think this change should not be part of this patch. As you said this patch should be only code movement, and adding prototype is definitely not code movement. If you want to keep it, I would prefer a separate patch. >>> { >>> - ASSERT(cpu < NR_GIC_CPU_IF); /* Targets bitmap only supports 8 >> CPUs */ >>> send_SGI_mask(cpumask_of(cpu), sgi); >>> } >>> It might interesting to keep it just in case. If you plan to remove it from >>> -void send_SGI_self(enum gic_sgi sgi) >>> -{ >>> - ASSERT(sgi < 16); /* There are only 16 SGIs */ >>> - >>> - dsb(); >>> - >>> - GICD[GICD_SGIR] = GICD_SGI_TARGET_SELF >>> - | sgi; >>> -} >>> - >> >> Why did you remove send_SGI_self? >> > It is not used at all. Same as send_SGI_one. It's not part of this patch. >>> void send_SGI_allbutself(enum gic_sgi sgi) >>> { >>> - ASSERT(sgi < 16); /* There are only 16 SGIs */ >>> + cpumask_t all_others_mask; >>> + ASSERT(sgi < 16); /* There are only 16 SGIs */ >>> >>> - dsb(); >>> + cpumask_andnot(&all_others_mask, &cpu_possible_map, >> cpumask_of(smp_processor_id())); >>> >>> - GICD[GICD_SGIR] = GICD_SGI_TARGET_OTHERS >>> - | sgi; >>> + dsb(); >>> + send_SGI_mask(&all_others_mask, sgi); >> >> Why did you remove the optmization for GICv2? > What was the optimization that was there earlier? GICD_SGI_TARGET_OTHERS, will send an SGI to every CPU even if the CPU is not yet online (e.g not registered by Xen). It's used during secondary boot (see cpu_up_send_sgi). Here you are breaking SMP boot on GICv2. >> [..] >>> + void (*enable_irq)(int); >>> + void (*disable_irq)(int); >>> + void (*eoi_irq)(int); >>> + void (*deactivate_irq)(int); >>> + unsigned int (*ack_irq)(void); >> >> I would prefer to introduce a new hw_controller here rather than adding >> another abstraction. >> > Can you please explain more on what is meant by new hw_controller? oops, I meant hw_irq_controller sorry. (see xen/include/xen/irq.h). Regards, -- Julien Grall