Re: [Xen-devel] [PATCH V3] xen: arm: platforms: Adding reset support for xgene arm64 platform.

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2014-01-24 at 17:18 +0530, Pranavkumar Sawargaonkar wrote:
> This patch adds a reset support for xgene arm64 platform.
> 
> 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@xxxxxxxxxx>
> Signed-off-by: Anup Patel <anup.patel@xxxxxxxxxx>
> ---
>  xen/arch/arm/platforms/xgene-storm.c |   70 
> ++++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
> 
> diff --git a/xen/arch/arm/platforms/xgene-storm.c 
> b/xen/arch/arm/platforms/xgene-storm.c
> index 5b0bd5f..986284c 100644
> --- a/xen/arch/arm/platforms/xgene-storm.c
> +++ b/xen/arch/arm/platforms/xgene-storm.c
> @@ -20,8 +20,17 @@
>  
>  #include <xen/config.h>
>  #include <asm/platform.h>
> +#include <xen/vmap.h>
> +#include <asm/io.h>
>  #include <asm/gic.h>
>  
> +#define DT_MATCH_RESET                      \
> +    DT_MATCH_COMPATIBLE("apm,xgene-reboot")

The gic and timer use a #define here because it is needed in multiple
places, for this use case you can just inline it into the array in
xgene_storm_init. i.e.:

> +static int xgene_storm_init(void)
> +{
> +    static const struct dt_device_match reset_ids[] __initconst =
> +    {
> +        DT_MATCH_RESET,

           DT_MATCH_COMPATIBLE("apm,xgene-reboot")

is fine IMHO.

> +        {},
> +    };
> +    struct dt_device_node *dev;
> +    int res;
> +
> +    dev = dt_find_matching_node(NULL, reset_ids);
> +    if ( !dev )
> +    {
> +        printk("Unable to find a compatible reset node in "
> +               "the device tree");

Please don't wrap string constants, it makes it hard to grep and I'd
rather have a long line (in this case it's not too long either).

Please can you add an xgene: (or whatever is appropriate) prefix too.

> +        return -ENODEV;

I wonder if it is worth returning success here? The system would be
mostly functional after all.

(You could apply this logic to the other returns if you wish, although
if the node is present then an error in the content could be considered
more critical to abort on)

> +    }
> +
> +    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("Unable to retrieve the base address for reset\n");
> +        return res;
> +    }
> +
> +    /* Get reset mask */
> +    res = dt_property_read_u32(dev, "mask", &reset_mask);
> +    if ( !res )
> +    {
> +        printk("Unable to retrieve the reset mask\n");
> +        return res;
> +    }

All looks good, thanks.


> +
> +    return 0;
> +}
>  
>  static const char * const xgene_storm_dt_compat[] __initconst =
>  {
> @@ -116,6 +184,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,
>  



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