Re: [PATCH v2 13/16] xen/arm: IRQ: Store IRQ type in arch_irq_desc

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

Hi Ian,

I have sent a V3 few minutes ago. I will take into account of your
remarks in V4.

On 04/08/2014 04:30 PM, Ian Campbell wrote:
> On Tue, 2014-04-08 at 12:46 +0100, Julien Grall wrote:
>> On 04/07/2014 05:26 PM, Ian Campbell wrote:
>>> On Mon, 2014-04-07 at 17:06 +0100, Julien Grall wrote:
>>>>>> @@ -240,7 +245,7 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level,
>>>>>>      /* Set edge / level */
>>>>>>      cfg = GICD[GICD_ICFGR + irq / 16];
>>>>>>      edgebit = 2u << (2 * (irq % 16));
>>>>>> -    if ( level )
>>>>>> +    if ( (type & DT_IRQ_TYPE_LEVEL_MASK) || (type == DT_IRQ_TYPE_NONE) )
>>>>>
>>>>> Is getting DT_IRQ_TYPE_NONE here not an error?
>>>>
>>>> No, there is some DT like the exynos one which is using 0 (i.e
>>>> DT_IRQ_TYPE_NONE) for the IRQ type.
>>>
>>> The underlying physical interrupt must be one or the other though,
>>> surely?
>>>
>>> So either there is some implicit (or perhaps documented?) assumption
>>> that NONE==something or the DT is buggy.
>>
>> By default Linux is setting every interrupt to be level triggered,
>> active low.
> 
> Do we know if this is because ePAPR or some other standard say this is
> the default or just a random choice by Linux?

ePAPR only describes interrupts with 2 cells. I don't find anything
about the default value.

I don't know how Linux chose this value. I guess it's because most of
interrupt level-sensitive on ARM.

>>  I've just noticed we do the same thing in gic_dist_init.
> 
> Whatever the reason for them doing it it probably make sense to do the
> same, otherwise we are just making pain for ourselves.
> 
> I'm not convinced that the exynos DT doesn't also need to be fixed
> though.

In any case we have to be able to boot any valid device tree for Linux
no matter are the values.

It seems that Linux is ignoring NONE and doesn't update the ICFGR.

>> If you prefer I can add a common above the function to say 0 is used
>> when an error is occured.
> 
> Well, this is a more global thing than this one function really.

I can use the same solution as serial_irq callbacks, i.e returning -1 in
case of an error.

Regards,

-- 
Julien Grall