Re: [PATCH for-4.5 4/8] xen/arm: irq: Don't need to have a specific function to route IRQ to Xen

[Julien Grall <julien.grall@linaro.org>]

Hello Ian,

On 02/19/2014 11:45 AM, Ian Campbell wrote:
> On Fri, 2014-01-24 at 16:43 +0000, Julien Grall wrote:
>> Actually, when the IRQ is handling by Xen, the setup is done in 2 steps:
> 
> s/Actually, //
> 
> I'd also go with a title like "remove need to have specific..." or
> "remove function to route...".
> 
>>     - Route the IRQ to the current CPU and set priorities
>>     - Set up the handler
>>
>> For PPIs, these step are called on every cpus. For SPIs, it's called only
> 
>                      ^s                    cpu             they are only called
> 
>> on the boot CPU.
>>
>> Divided the setup in two step complicated the code when a new driver is
> 
> Dividing           into two steps complicates
> 
>> added by Xen (for instance a SMMU driver). Xen can safely route the IRQ
> 
>        to Xen
> 
>> when the driver setup the interrupt handler.
> 
>                  sets up

Thanks to look at my grammar nits :).

> Although for this final para I'm not sure why a new driver is needed --
> either it is already complicated or not.

I will remove this para then.

>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>  xen/arch/arm/gic.c         |   67 +++++++++++++++-----------------------------
>>  xen/arch/arm/setup.c       |    3 --
>>  xen/arch/arm/smpboot.c     |    2 --
>>  xen/arch/arm/time.c        |   11 --------
>>  xen/include/asm-arm/gic.h  |    7 -----
>>  xen/include/asm-arm/time.h |    3 --
>>  6 files changed, 23 insertions(+), 70 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index d68bde3..58bcba3 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -254,43 +254,25 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level,
>>      spin_unlock(&gic.lock);
>>  }
>>  
>> -/* Program the GIC to route an interrupt */
>> +/* Program the GIC to route an interrupt to the host (eg Xen)
>> + * - needs to be called with desc.lock held
> 
> This suggests that the caller must have desc in its hand, but it then
> passes irq here so we can look it up again. It may as well pass desc I
> think.

Right, I will update release_irq to take an irq_desc in parameters
instead of the IRQ.

> 
>>  void __init release_irq(unsigned int irq)
>>  {
>>      struct irq_desc *desc;
>> @@ -601,6 +561,7 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
>>      int rc;
>>      unsigned long flags;
>>      struct irq_desc *desc;
>> +    bool_t disabled = 0;
>>  
>>      desc = irq_to_desc(irq->irq);
>>  
>> @@ -620,6 +581,24 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
>>          return -EADDRINUSE;
>>      }
>>  
>> +    disabled = (desc->action == NULL);
>> +
>> +    /* First time the IRQ is setup */
>> +    if ( disabled )
>> +    {
>> +        bool_t level;
>> +
>> +        level = dt_irq_is_level_triggered(irq);
>> +        /* It's fine to use smp_processor_id() because:
>> +         * For PPI: irq_desc is banked
>> +         * For SGI: we don't care for now which CPU will receive the
>> +         * interrupt
>> +         * TODO: Handle case where SGI is setup on different CPU than
>> +         * the targeted CPU and the priority.
> 
> Do you mean s/SGI/SPI/ here? SGIs are inherently per CPU, like PPIs.

Yes, SPI.

Cheers,

-- 
Julien Grall