Re: [PATCH] xen: arm: X-Gene Storm check GIC DIST address for EOI quirk

[Julien Grall <julien.grall@citrix.com>]

Hi Stefano,

On 07/04/2015 10:40, Stefano Stabellini wrote:
> On Mon, 6 Apr 2015, Julien Grall wrote:
>> Hi Pranav,
>>
>> Thank you for the patch.
>>
>> On 06/04/2015 10:54, Pranavkumar Sawargaonkar wrote:
>>> In old X-Gene Storm firmware and DT, secure mode addresses have been
>>> mentioned in GICv2 node. In this case maintenance interrupt is used
>>> instead of EOI HW method.
>>>
>>> This patch checks the GIC Distributor Base Address to enable EOI quirk
>>> for old firmware.
>>>
>>> Ref:
>>> http://lists.xen.org/archives/html/xen-devel/2014-07/msg01263.html
>>>
>>> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
>>> ---
>>>    xen/arch/arm/platforms/xgene-storm.c |   37
>>> +++++++++++++++++++++++++++++++++-
>>>    1 file changed, 36 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/platforms/xgene-storm.c
>>> b/xen/arch/arm/platforms/xgene-storm.c
>>> index eee650e..dd7cbfc 100644
>>> --- a/xen/arch/arm/platforms/xgene-storm.c
>>> +++ b/xen/arch/arm/platforms/xgene-storm.c
>>> @@ -22,6 +22,7 @@
>>>    #include <asm/platform.h>
>>>    #include <xen/stdbool.h>
>>>    #include <xen/vmap.h>
>>> +#include <xen/device_tree.h>
>>>    #include <asm/io.h>
>>>    #include <asm/gic.h>
>>>
>>> @@ -35,9 +36,41 @@ static u64 reset_addr, reset_size;
>>>    static u32 reset_mask;
>>>    static bool reset_vals_valid = false;
>>>
>>> +#define XGENE_SEC_GICV2_DIST_ADDR    0x78010000
>>> +static u32 quirk_guest_pirq_need_eoi;
>>
>> This variable will mostly be read. So, I would add __read_mostly.
>>
>>> +
>>> +static void xgene_check_pirq_eoi(void)
>>
>> If I'm not mistaken, this function is only called during Xen initialization.
>> So, I would add __init.
>>
>>> +{
>>> +    struct dt_device_node *node;
>>> +    int res;
>>> +    paddr_t dbase;
>>> +
>>> +    dt_for_each_device_node( dt_host, node )
>>> +    {
>>
>> It would be better to create a new callback for platform specific GIC
>> initialization and use dt_interrupt_controller.
>>
>> This would avoid to have this loop and rely on there is always only one
>> interrupt controller in the DT.
>
> That is true, however we do know that on this SoC there is only one GIC,
> so it might be acceptable to rely on this knowledge: this is already
> very platform specific code.

If we keep the loop, I would add a comment on the above the loop 
explaining that we rely on there is always only GIC.

Although, I was wondering if we could simply rely on the node path to 
get the DT node?

Regards,

-- 
Julien Grall