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

[Pranavkumar Sawargaonkar <pranavkumar@xxxxxxxxxx>]

Hi Julien,

On 25 April 2015 at 21:48, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> Hi Pranav,
>
> On 24/04/2015 15:46, 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
>>
>> ChangeLog:
>>
>> V2:
>> - Fine tune interrupt controller node search as per comments on V1 patch
>> - Incorporating other misc comments on V1.
>> V1:
>> - Initial patch.
>
>
> The changelog should be separated from the commit message by a "---" follow
> by a new line. So git automatically will stripped the changelog when Ian
> will apply the patch to Xen git.

Thanks I will fix this along with other comments to post v3.

-
Pranav
>
>
>> Tested-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx>
>> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@xxxxxxxxxx>
>> ---
>>   xen/arch/arm/platforms/xgene-storm.c |   42
>> +++++++++++++++++++++++++++++++++-
>>   1 file changed, 41 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/platforms/xgene-storm.c
>> b/xen/arch/arm/platforms/xgene-storm.c
>> index 1812e5b..c9a6dfc 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,46 @@ 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 __read_mostly xgene_quirks = PLATFORM_QUIRK_GIC_64K_STRIDE;
>> +
>> +static void __init xgene_check_pirq_eoi(void)
>> +{
>> +    struct dt_device_node *node;
>
>
> I think this can be const.
>
>> +    int res;
>> +    paddr_t dbase;
>> +    static const struct dt_device_match xgene_dt_int_ctrl_match[] =
>
>
> The static is not necessary here.
>
>> +    {
>> +        DT_MATCH_COMPATIBLE("arm,cortex-a15-gic"),
>> +        { /*sentinel*/ },
>> +    };
>> +
>> +    node = dt_find_interrupt_controller(xgene_dt_int_ctrl_match);
>> +    if ( !node )
>> +        panic("%s: Can not find interrupt controller node\n", __func__);
>
>
> s/Can not/Cannot/ ?
>
> Panic will add a newline. So it's not necessary here.
>
> Although I'm not sure if we should use panic here. All the platform code is
> returning an error code which will be handled to an upper function in the
> stack (currently it's platform_init).
>
> I don't have a strong opinion on it. I will let the ARM maintainers decide.
>
>> +
>> +    res = dt_device_get_address(node, 0, &dbase, NULL);
>> +    if ( !dbase )
>> +        panic("%s: Cannot find a valid address for the "
>> +                "distributor", __func__);
>
>
> The indentation looks wrong here. Also, I'm not sure why you split the
> message. The line won't be longer than 80 columns.
>
>> +
>> +    /*
>> +     * In old X-Gene Storm firmware and DT, secure mode addresses have
>> +     * been mentioned in GICv2 node. We have to use maintenance interrupt
>> +     * instead of EOI HW in this case. We check the GIC Distributor Base
>> +     * Address to maintain compatibility with older firmware.
>> +     */
>> +    if ( dbase == XGENE_SEC_GICV2_DIST_ADDR )
>> +    {
>> +        xgene_quirks |= PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI;
>> +        printk("Xen: warning: Using OLD X-Gene Firmware,"
>> +                "disabling PIRQ EOI mode ...\n");
>
>
> Indentation
>
> Regards,
>
> --
> Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel