From mboxrd@z Thu Jan  1 00:00:00 1970
From: Julien Grall <julien.grall@citrix.com>
Subject: Re: [PATCH] xen: arm: X-Gene Storm check GIC DIST
 address for EOI quirk
Date: Tue, 7 Apr 2015 11:29:52 +0100
Message-ID: <5523B1A0.9040802@citrix.com>
References: <1428310441-5412-1-git-send-email-pranavkumar@linaro.org>
	<55229C89.9080400@citrix.com>
	<alpine.DEB.2.02.1504071039320.7690@kaball.uk.xensource.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <alpine.DEB.2.02.1504071039320.7690@kaball.uk.xensource.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Stefano Stabellini <stefano.stabellini@eu.citrix.com>, Julien Grall <julien.grall@citrix.com>
Cc: patches@apm.com, Pranavkumar Sawargaonkar <pranavkumar@linaro.org>, stefano.stabellini@citrix.com, christoffer.dall@linaro.org, xen-devel@lists.xen.org
List-Id: xen-devel@lists.xenproject.org

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