Re: [PATCH v6 1/6] xen/arm: IRQ: Store IRQ type in arch_irq_desc

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

On 05/15/2014 04:45 PM, Ian Campbell wrote:
>> +int irq_set_type(unsigned int spi, unsigned int type)
>> +{
>> +    unsigned long flags;
>> +    struct irq_desc *desc = irq_to_desc(spi);
>> +    int ret = -EBUSY;
>> +
>> +    /* This function should not be used for other than SPIs */
> 
> Perhaps name the function irq_set_spi_type or something then?
> 
>> +    for_each_cpu( cpu, &cpu_online_map )
>> +    {
>> +        desc = &per_cpu(local_irq_desc, cpu)[irq];
>> +        spin_lock_irqsave(&desc->lock, flags);
>> +        desc->arch.type = type;
> 
> Don't you need to write ICFGR on each CPU?

This function will bail out if local_irqs_type[irq] == DT_IRQ_TYPE_INVALID.

This value means, the IRQ has not been configured and set up (see ASSERT
in configure which prevent it).

The ICFGR will be configure when Xen set up an handler to this IRQ (see
setup_irq).

> 
>> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
>> index 5542958..e677da9 100644
>> --- a/xen/include/xen/device_tree.h
>> +++ b/xen/include/xen/device_tree.h
>> @@ -131,6 +131,7 @@ struct dt_phandle_args {
>>   * DT_IRQ_TYPE_LEVEL_LOW       - low level triggered
>>   * DT_IRQ_TYPE_LEVEL_MASK      - Mask to filter out the level bits
>>   * DT_IRQ_TYPE_SENSE_MASK      - Mask for all the above bits
>> + * DT_IRQ_TYPE_INVALID         - Use to initialize the type
>>   */
>>  #define DT_IRQ_TYPE_NONE           0x00000000
>>  #define DT_IRQ_TYPE_EDGE_RISING    0x00000001
>> @@ -143,6 +144,8 @@ struct dt_phandle_args {
>>      (DT_IRQ_TYPE_LEVEL_LOW | DT_IRQ_TYPE_LEVEL_HIGH)
>>  #define DT_IRQ_TYPE_SENSE_MASK     0x0000000f
>>  
>> +#define DT_IRQ_TYPE_INVALID        0x00000010
> 
> 0xffffffff perhaps?

The value 0x010 will always return false for edge and level. This is not
the case for 0xffffffff because edge/level use different bit.

Regards,

-- 
Julien Grall