Re: [PATCH v3 07/16] xen/arm: segregate and split GIC low level functionality

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

On 04/28/2014 02:10 PM, Vijay Kilari wrote:
> On Mon, Apr 28, 2014 at 5:36 PM, Julien Grall <julien.grall@linaro.org> wrote:
>> Hello Vijaya
>>
>> On 04/28/2014 12:48 PM, Vijay Kilari wrote:
>>> On Wed, Apr 16, 2014 at 12:05 AM, Julien Grall <julien.grall@linaro.org> wrote:
>>>> Thank you for the patch.
>>>>
>>>>>      /* Enable routing */
>>>>> -    GICD[GICD_ISENABLER + irq / 32] = (1u << (irq % 32));
>>>>> +    gic_hw_ops->gic_irq_ops->enable(desc);
>>>>
>>>> This is not the right way to use gic_irq_ops. You should directly
>>>> assigned this structure to desc->handler.
>>>>
>>>  I think desc->handler is already filled with ops of below struct
>>> which is used
>>
>> No, with your patch, the desc->handler is filled with ops from the
>> generic code. You added an indirection to call the specific ops.
>>
> 
> Do you mean to move gic_host_irq_type & gic_quest_irq_type
> to gic-{2/3}.c and register these with generic driver?
> So with this gic_lock should also be moved to gic-{2,3}.c

Yes. As I said in my previous mail, the gic_lock is only used to protect
hardware in your patch. I don't see any good reason to keep it in the
generic code.

Regards,

-- 
Julien Grall