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

[Pranavkumar Sawargaonkar <pranavkumar@xxxxxxxxxx>]

Hi Ian,

On 24 January 2014 17:31, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> 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.

Sure i will fix this,

>
>> +        {},
>> +    };
>> +    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.

Yes will do it.
>
>> +        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)
Yeah actually i also wonder about correct return value since system is
mostly functional without this.
I will return a success here.
>
>> +    }
>> +
>> +    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.
Thanks, will send out a new patch today.
>
>
>> +
>> +    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,
>>
>
>
Thanks,
Pranav

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