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

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

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.

> Tested-by: Christoffer Dall <christoffer.dall@linaro.org>
> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
> ---
>   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