Re: [PATCH V6] xen: arm: platforms: Adding reset support for xgene arm64 platform.

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

On 01/27/2014 11:34 AM, Pranavkumar Sawargaonkar wrote:
> This patch adds a reset support for xgene arm64 platform.
> 
> V6:
> - Incorporating comments received on V5 patch.
> V5:
> - Incorporating comments received on V4 patch.
> V4:
> - Removing TODO comment about retriving reset base address from dts
>   as that is done now.
> V3:
> - Retriving reset base address and reset mask from device tree.
> - Removed unnecssary header files included earlier.
> V2:
> - Removed unnecssary mdelay in code.
> - Adding iounmap of the base address.
> V1:
> -Initial patch.
> 
> Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
> Signed-off-by: Anup Patel <anup.patel@linaro.org>
> ---
>  xen/arch/arm/platforms/xgene-storm.c |   72 ++++++++++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
> index 5b0bd5f..4fc185b 100644
> --- a/xen/arch/arm/platforms/xgene-storm.c
> +++ b/xen/arch/arm/platforms/xgene-storm.c
> @@ -20,8 +20,16 @@
>  
>  #include <xen/config.h>
>  #include <asm/platform.h>
> +#include <xen/stdbool.h>
> +#include <xen/vmap.h>
> +#include <asm/io.h>
>  #include <asm/gic.h>
>  
> +/* Variables to save reset address of soc during platform initialization. */
> +static u64 reset_addr, reset_size;
> +static u32 reset_mask;
> +static bool reset_vals_valid = false;
> +
>  static uint32_t xgene_storm_quirks(void)
>  {
>      return PLATFORM_QUIRK_GIC_64K_STRIDE;
> @@ -107,6 +115,68 @@ err:
>      return ret;
>  }
>  
> +static void xgene_storm_reset(void)
> +{

I'm concerned about reset function in general, in common code we have
this code (arch/arm/shutdown.c machine_restart).

while (1)
{
   raw_machine_reset(); // which call platform_reset()
   mdelay(100);
}

If platform_reset failed, it's possible with this code, the console will
be spam with "XGENE: ...".

Do we really need to call raw_machine_reset multiple time?
Would it be more suitable to have this code:

raw_machine_reset();
mdelay(100);
printk("Failed to reset\n");
while (1);

Or even better, moving the mdelay  per platform...

> +    void __iomem *addr;
> +
> +    if ( !reset_vals_valid )
> +    {
> +        printk("XGENE: Invalid reset values, can not reset XGENE...\n");
> +        return;
> +    }
> +
> +    addr = ioremap_nocache(reset_addr, reset_size);
> +
> +    if ( !addr )
> +    {
> +        printk("XGENE: Unable to map xgene reset address, can not reset XGENE...\n");
> +        return;
> +    }
> +
> +    /* Write reset mask to base address */
> +    writel(reset_mask, addr);
> +
> +    iounmap(addr);
> +}
> +
> +static int xgene_storm_init(void)
> +{
> +    static const struct dt_device_match reset_ids[] __initconst =
> +    {
> +        DT_MATCH_COMPATIBLE("apm,xgene-reboot"),
> +        {},

Do you plan to have other ids in the future? If not, I would directly
use dt_find_compatible_node(NULL, NULL "arm,xgene-reboot"); instead of
dt_find_matching_node(...).

> +    };
> +    struct dt_device_node *dev;
> +    int res;
> +
> +    dev = dt_find_matching_node(NULL, reset_ids);
> +    if ( !dev )
> +    {
> +        printk("XGENE: Unable to find a compatible reset node in the device tree");
> +        return 0;
> +    }
> +
> +    dt_device_set_used_by(dev, DOMID_XEN);
> +
> +    /* Retrieve base address and size */
> +    res = dt_device_get_address(dev, 0, &reset_addr, &reset_size);
> +    if ( res )
> +    {
> +        printk("XGENE: Unable to retrieve the base address for reset\n");
> +        return 0;
> +    }
> +
> +    /* Get reset mask */
> +    res = dt_property_read_u32(dev, "mask", &reset_mask);
> +    if ( !res )
> +    {
> +        printk("XGENE: Unable to retrieve the reset mask\n");
> +        return 0;
> +    }
> +
> +    reset_vals_valid = true;
> +    return 0;
> +}
>  
>  static const char * const xgene_storm_dt_compat[] __initconst =
>  {
> @@ -116,6 +186,8 @@ static const char * const xgene_storm_dt_compat[] __initconst =
>  
>  PLATFORM_START(xgene_storm, "APM X-GENE STORM")
>      .compatible = xgene_storm_dt_compat,
> +    .init = xgene_storm_init,
> +    .reset = xgene_storm_reset,
>      .quirks = xgene_storm_quirks,
>      .specific_mapping = xgene_storm_specific_mapping,
>  
> 


-- 
Julien Grall