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

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

[Pranavkumar Sawargaonkar]

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)
+{
+    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"),
+        {},
+    };
+    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,
 
-- 
1.7.9.5

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

[Ian Campbell]

On Mon, 2014-01-27 at 17:04 +0530, 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>

Looks good, thanks:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

George, I'd like to put this in 4.4. The impact on non-xgene is non
existent and I think reset is basic functionality which we should
enable.

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

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

[George Dunlap]

On 01/27/2014 11:38 AM, Ian Campbell wrote:
> On Mon, 2014-01-27 at 17:04 +0530, 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>
> Looks good, thanks:
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> George, I'd like to put this in 4.4. The impact on non-xgene is non
> existent and I think reset is basic functionality which we should
> enable.

So without this patch, support for xgene would be considered 
"experimental" (missing basic functionality)?

  -George

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

[Ian Campbell]

On Mon, 2014-01-27 at 12:01 +0000, George Dunlap wrote:
> On 01/27/2014 11:38 AM, Ian Campbell wrote:
> > On Mon, 2014-01-27 at 17:04 +0530, 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>
> > Looks good, thanks:
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> >
> > George, I'd like to put this in 4.4. The impact on non-xgene is non
> > existent and I think reset is basic functionality which we should
> > enable.
> 
> So without this patch, support for xgene would be considered 
> "experimental" (missing basic functionality)?

I don't think I would go so far as to say "experimental", we could
probably live with it, but having to go find the board and press the
tiny little button to reboot it is certainly not "complete".

Ian.

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

[George Dunlap]

On 01/27/2014 12:04 PM, Ian Campbell wrote:
> On Mon, 2014-01-27 at 12:01 +0000, George Dunlap wrote:
>> On 01/27/2014 11:38 AM, Ian Campbell wrote:
>>> On Mon, 2014-01-27 at 17:04 +0530, 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>
>>> Looks good, thanks:
>>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>>>
>>> George, I'd like to put this in 4.4. The impact on non-xgene is non
>>> existent and I think reset is basic functionality which we should
>>> enable.
>> So without this patch, support for xgene would be considered
>> "experimental" (missing basic functionality)?
> I don't think I would go so far as to say "experimental", we could
> probably live with it, but having to go find the board and press the
> tiny little button to reboot it is certainly not "complete".

Yes, "can't reboot via software" is a pretty big lack of functionality; 
it weighs in pretty heavily against whatever potential regressions might 
be introduced.

Release-acked-by: George Dunlap <george.dunlap@eu.citrix.com>

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

[Julien Grall]

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

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

[Ian Campbell]

On Mon, 2014-01-27 at 13:36 +0000, Julien Grall wrote:
> 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: ...".

Hrm, yes, I hadn't thought of this.

> Do we really need to call raw_machine_reset multiple time?

I suppose the logic is that there is no harm in trying again, we aren't
doing anything else and depending on the failure it might eventually
work.

I think it would be reasonable to remove the print from
xgene_storm_reset, or use a static int once construct.

> 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...

I don't think that makes sense.

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

[Julien Grall]

On 01/27/2014 02:13 PM, Ian Campbell wrote:
> On Mon, 2014-01-27 at 13:36 +0000, Julien Grall wrote:
>> 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: ...".
> 
> Hrm, yes, I hadn't thought of this.
> 
>> Do we really need to call raw_machine_reset multiple time?
> 
> I suppose the logic is that there is no harm in trying again, we aren't
> doing anything else and depending on the failure it might eventually
> work.

If it doesn't work the first time, why it should work the second time?
I think it's platform specific to retry again if the "restart" has failed.

> I think it would be reasonable to remove the print from
> xgene_storm_reset, or use a static int once construct.

Print are useful for debugging.

-- 
Julien Grall

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

[Ian Campbell]

On Mon, 2014-01-27 at 14:24 +0000, Julien Grall wrote:
> On 01/27/2014 02:13 PM, Ian Campbell wrote:
> > On Mon, 2014-01-27 at 13:36 +0000, Julien Grall wrote:
> >> 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: ...".
> > 
> > Hrm, yes, I hadn't thought of this.
> > 
> >> Do we really need to call raw_machine_reset multiple time?
> > 
> > I suppose the logic is that there is no harm in trying again, we aren't
> > doing anything else and depending on the failure it might eventually
> > work.
> 
> If it doesn't work the first time, why it should work the second time?

For some platform specific reason.

> I think it's platform specific to retry again if the "restart" has
> failed.

All that does is force every platform to reimplement the loop.

> 
> > I think it would be reasonable to remove the print from
> > xgene_storm_reset, or use a static int once construct.
> 
> Print are useful for debugging.
> 

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

[Julien Grall]

On 01/27/2014 02:27 PM, Ian Campbell wrote:
> On Mon, 2014-01-27 at 14:24 +0000, Julien Grall wrote:
>> On 01/27/2014 02:13 PM, Ian Campbell wrote:
>>> On Mon, 2014-01-27 at 13:36 +0000, Julien Grall wrote:
>>>> 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: ...".
>>>
>>> Hrm, yes, I hadn't thought of this.
>>>
>>>> Do we really need to call raw_machine_reset multiple time?
>>>
>>> I suppose the logic is that there is no harm in trying again, we aren't
>>> doing anything else and depending on the failure it might eventually
>>> work.
>>
>> If it doesn't work the first time, why it should work the second time?
> 
> For some platform specific reason.
> 
>> I think it's platform specific to retry again if the "restart" has
>> failed.
> 
> All that does is force every platform to reimplement the loop.

Not every platform. For instance on the Arndale and the Versatile
Express we don't need the loop.

After looking to the reset code of X-Gene on Linux it's the same. I'm
surprised that we would need the loop in Xen.

The only ways we can fail are:
	- ioremap return NULL;
	- reset address is not set.

Both won't work at the second attempd, neither the third.

-- 
Julien Grall

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

[Ian Campbell]

On Mon, 2014-01-27 at 14:36 +0000, Julien Grall wrote:

> The only ways we can fail are:
> 	- ioremap return NULL;
> 	- reset address is not set.
> 
> Both won't work at the second attempd, neither the third.


You are missing the fact that the write to the reset address itself may
"succeed" but not do anything, for example because of firmware oddities
or some strange platform specific property. It might succeed on a second
or third attempt, and there is no harm in trying.

Ian.

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

[Ian Campbell]

On Mon, 2014-01-27 at 12:07 +0000, George Dunlap wrote:
> Yes, "can't reboot via software" is a pretty big lack of functionality; 
> it weighs in pretty heavily against whatever potential regressions might 
> be introduced.
> 
> Release-acked-by: George Dunlap <george.dunlap@eu.citrix.com>

I've applied this. I think Julien's concerns about the boot loop
spamming the consoleif reboot fails are 4.5 material.

Ian.

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

[Ian Campbell]

On Mon, 2014-01-27 at 17:04 +0530, Pranavkumar Sawargaonkar wrote:
> +        DT_MATCH_COMPATIBLE("apm,xgene-reboot"),

I should have asked this sooner -- can you point me to the bindings
documentation for this device?

http://www.gossamer-threads.com/lists/linux/kernel/1845585 suggests it
is not yet agreed, so having Xen depend on it may have been a mistake.

Ian.

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

[Pranavkumar Sawargaonkar]

Hi Ian,

On 28 January 2014 21:17, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Mon, 2014-01-27 at 17:04 +0530, Pranavkumar Sawargaonkar wrote:
>> +        DT_MATCH_COMPATIBLE("apm,xgene-reboot"),
>
> I should have asked this sooner -- can you point me to the bindings
> documentation for this device?
>
> http://www.gossamer-threads.com/lists/linux/kernel/1845585 suggests it
> is not yet agreed, so having Xen depend on it may have been a mistake.

Above patch is still under discussion so i can not take changes from
that to xen driver immediately.

For now i have added xen reset code based on
"drivers/power/reset/xgene-reboot.c" driver which is already merged in
linux.

http://www.spinics.net/lists/arm-kernel/msg266039.html
For now DTS bindings for xen are similar as mentioned in above link.

Actually if you see new patch and old one (from reboot point of view) -
Only difference in both the dts bindings is "mask" filed in dts.
In old patch it used to be read from dts but in latest it is
hard-coded to 1 in actual code and being removed from dts in new
patch.

Now if you want this to be fixed , i can quickly submit a V7 in which
mask field will be just hard-coded to 1 hence xen code will always
work even if linux code does gets changed.

Thanks,
Pranav








>
> Ian.
>

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

[Ian Campbell]

On Tue, 2014-01-28 at 23:05 +0530, Pranavkumar Sawargaonkar wrote:
> Hi Ian,
> 
> On 28 January 2014 21:17, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Mon, 2014-01-27 at 17:04 +0530, Pranavkumar Sawargaonkar wrote:
> >> +        DT_MATCH_COMPATIBLE("apm,xgene-reboot"),
> >
> > I should have asked this sooner -- can you point me to the bindings
> > documentation for this device?
> >
> > http://www.gossamer-threads.com/lists/linux/kernel/1845585 suggests it
> > is not yet agreed, so having Xen depend on it may have been a mistake.
> 
> Above patch is still under discussion so i can not take changes from
> that to xen driver immediately.
> 
> For now i have added xen reset code based on
> "drivers/power/reset/xgene-reboot.c" driver which is already merged in
> linux.
> 
> http://www.spinics.net/lists/arm-kernel/msg266039.html
> For now DTS bindings for xen are similar as mentioned in above link.
> 
> Actually if you see new patch and old one (from reboot point of view) -
> Only difference in both the dts bindings is "mask" filed in dts.
> In old patch it used to be read from dts but in latest it is
> hard-coded to 1 in actual code and being removed from dts in new
> patch.

Do you have a ref for that new patch?

I also don't see any patch to linux/Documentation/devicetree/bindings,
as was requested in that posting from 6 months ago. Where can I find
that?

It seems like the patch to arch/arm64/boot/dts/apm-storm.dtsi also
hasn't landed?

> Now if you want this to be fixed , i can quickly submit a V7 in which
> mask field will be just hard-coded to 1 hence xen code will always
> work even if linux code does gets changed.

Looks like the Linux driver uses 0xffffffff if the mask isn't given --
that seems like a good approach.

I think we'll just have to accept that until the binding is specified
and documented (in linux/Documentation/devicetree/bindings) then we may
have to be prepared to change the Xen implementation to match the final
spec without regard to backwards compat. If we aren't happy with that
then I should revert the patch now and we will have to live without
reboot support in the meantime.

Ian

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

[Pranavkumar Sawargaonkar]

Hi Ian,

On 28 January 2014 23:13, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Tue, 2014-01-28 at 23:05 +0530, Pranavkumar Sawargaonkar wrote:
>> Hi Ian,
>>
>> On 28 January 2014 21:17, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> > On Mon, 2014-01-27 at 17:04 +0530, Pranavkumar Sawargaonkar wrote:
>> >> +        DT_MATCH_COMPATIBLE("apm,xgene-reboot"),
>> >
>> > I should have asked this sooner -- can you point me to the bindings
>> > documentation for this device?
>> >
>> > http://www.gossamer-threads.com/lists/linux/kernel/1845585 suggests it
>> > is not yet agreed, so having Xen depend on it may have been a mistake.
>>
>> Above patch is still under discussion so i can not take changes from
>> that to xen driver immediately.
>>
>> For now i have added xen reset code based on
>> "drivers/power/reset/xgene-reboot.c" driver which is already merged in
>> linux.
>>
>> http://www.spinics.net/lists/arm-kernel/msg266039.html
>> For now DTS bindings for xen are similar as mentioned in above link.
>>
>> Actually if you see new patch and old one (from reboot point of view) -
>> Only difference in both the dts bindings is "mask" filed in dts.
>> In old patch it used to be read from dts but in latest it is
>> hard-coded to 1 in actual code and being removed from dts in new
>> patch.
>
> Do you have a ref for that new patch?
New patch is the one you have mentioned i.e.
http://www.gossamer-threads.com/lists/linux/kernel/1845585
It has new DT bindings.
>

> I also don't see any patch to linux/Documentation/devicetree/bindings,
> as was requested in that posting from 6 months ago. Where can I find
> that?
>
> It seems like the patch to arch/arm64/boot/dts/apm-storm.dtsi also
> hasn't landed?
Yeah it is dangling and since new patch is already posted i think we
can wait for final DT bindings.
>
>> Now if you want this to be fixed , i can quickly submit a V7 in which
>> mask field will be just hard-coded to 1 hence xen code will always
>> work even if linux code does gets changed.
>
> Looks like the Linux driver uses 0xffffffff if the mask isn't given --
> that seems like a good approach.
>
> I think we'll just have to accept that until the binding is specified
> and documented (in linux/Documentation/devicetree/bindings) then we may
> have to be prepared to change the Xen implementation to match the final
> spec without regard to backwards compat. If we aren't happy with that
> then I should revert the patch now and we will have to live without
> reboot support in the meantime.
Please do not revert the patch , I think we can go ahead with current patch.
Once linux side is concluded i will fix minor changes in xen code
based on new DT bindigs..
>
> Ian
>
Thanks,
Pranav

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

[Ian Campbell]

On Tue, 2014-01-28 at 23:27 +0530, Pranavkumar Sawargaonkar wrote:

> > I also don't see any patch to linux/Documentation/devicetree/bindings,
> > as was requested in that posting from 6 months ago. Where can I find
> > that?
> >
> > It seems like the patch to arch/arm64/boot/dts/apm-storm.dtsi also
> > hasn't landed?
> Yeah it is dangling and since new patch is already posted i think we
> can wait for final DT bindings.

It seems from the thread that the final bindings are going to differ
significantly from what is implemented in Xen and proposed in the above
thread. (with a syscon driver that the reset driver references).

> >> Now if you want this to be fixed , i can quickly submit a V7 in which
> >> mask field will be just hard-coded to 1 hence xen code will always
> >> work even if linux code does gets changed.
> >
> > Looks like the Linux driver uses 0xffffffff if the mask isn't given --
> > that seems like a good approach.
> >
> > I think we'll just have to accept that until the binding is specified
> > and documented (in linux/Documentation/devicetree/bindings) then we may
> > have to be prepared to change the Xen implementation to match the final
> > spec without regard to backwards compat. If we aren't happy with that
> > then I should revert the patch now and we will have to live without
> > reboot support in the meantime.
> Please do not revert the patch , I think we can go ahead with current patch.
> Once linux side is concluded i will fix minor changes in xen code
> based on new DT bindigs..

It doesn't sounds to me like it is going to be minor changes.

Ian.

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

[Pranavkumar Sawargaonkar]

Hi Ian,

On 29 January 2014 18:08, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Tue, 2014-01-28 at 23:27 +0530, Pranavkumar Sawargaonkar wrote:
>
>> > I also don't see any patch to linux/Documentation/devicetree/bindings,
>> > as was requested in that posting from 6 months ago. Where can I find
>> > that?
>> >
>> > It seems like the patch to arch/arm64/boot/dts/apm-storm.dtsi also
>> > hasn't landed?
>> Yeah it is dangling and since new patch is already posted i think we
>> can wait for final DT bindings.
>
> It seems from the thread that the final bindings are going to differ
> significantly from what is implemented in Xen and proposed in the above
> thread. (with a syscon driver that the reset driver references).
>
>> >> Now if you want this to be fixed , i can quickly submit a V7 in which
>> >> mask field will be just hard-coded to 1 hence xen code will always
>> >> work even if linux code does gets changed.
>> >
>> > Looks like the Linux driver uses 0xffffffff if the mask isn't given --
>> > that seems like a good approach.
>> >
>> > I think we'll just have to accept that until the binding is specified
>> > and documented (in linux/Documentation/devicetree/bindings) then we may
>> > have to be prepared to change the Xen implementation to match the final
>> > spec without regard to backwards compat. If we aren't happy with that
>> > then I should revert the patch now and we will have to live without
>> > reboot support in the meantime.
>> Please do not revert the patch , I think we can go ahead with current patch.
>> Once linux side is concluded i will fix minor changes in xen code
>> based on new DT bindigs..
>
> It doesn't sounds to me like it is going to be minor changes.
Yes binding are changed in new drivers but now question is what to do
in current state where new driver is not submitted ?

My take is we have 3 opts :
1. Keep current reboot driver in xen as it is and use it with old
bindings. (since that is the one merged in linux)
2. I will send a new patch (will take 1hr max for me to do it) with
addresses hardcoded instead of reading it from dts.
    This will help for xen to have reboot driver for xgene.
3. Remove this driver completely from xen as of now.

>
> Ian.
>

Thanks,
Pranav

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

[Ian Campbell]

On Thu, 2014-01-30 at 11:38 +0530, Pranavkumar Sawargaonkar wrote:
> Hi Ian,
> 
> On 29 January 2014 18:08, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Tue, 2014-01-28 at 23:27 +0530, Pranavkumar Sawargaonkar wrote:
> >
> >> > I also don't see any patch to linux/Documentation/devicetree/bindings,
> >> > as was requested in that posting from 6 months ago. Where can I find
> >> > that?
> >> >
> >> > It seems like the patch to arch/arm64/boot/dts/apm-storm.dtsi also
> >> > hasn't landed?
> >> Yeah it is dangling and since new patch is already posted i think we
> >> can wait for final DT bindings.
> >
> > It seems from the thread that the final bindings are going to differ
> > significantly from what is implemented in Xen and proposed in the above
> > thread. (with a syscon driver that the reset driver references).
> >
> >> >> Now if you want this to be fixed , i can quickly submit a V7 in which
> >> >> mask field will be just hard-coded to 1 hence xen code will always
> >> >> work even if linux code does gets changed.
> >> >
> >> > Looks like the Linux driver uses 0xffffffff if the mask isn't given --
> >> > that seems like a good approach.
> >> >
> >> > I think we'll just have to accept that until the binding is specified
> >> > and documented (in linux/Documentation/devicetree/bindings) then we may
> >> > have to be prepared to change the Xen implementation to match the final
> >> > spec without regard to backwards compat. If we aren't happy with that
> >> > then I should revert the patch now and we will have to live without
> >> > reboot support in the meantime.
> >> Please do not revert the patch , I think we can go ahead with current patch.
> >> Once linux side is concluded i will fix minor changes in xen code
> >> based on new DT bindigs..
> >
> > It doesn't sounds to me like it is going to be minor changes.
> Yes binding are changed in new drivers but now question is what to do
> in current state where new driver is not submitted ?
> 
> My take is we have 3 opts :
> 1. Keep current reboot driver in xen as it is and use it with old
> bindings. (since that is the one merged in linux)
> 2. I will send a new patch (will take 1hr max for me to do it) with
> addresses hardcoded instead of reading it from dts.
>     This will help for xen to have reboot driver for xgene.
> 3. Remove this driver completely from xen as of now.

None of the options are brilliant :-/

I think on balance #2 is probably the way to go.

#1 would set a precedent for using formally undefined bindings which I
think we should avoid.

#3 has obvious downsides, but given that we have already accepted the
functionality it seems a shame to revert it entirely.

Ian.

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

[Pranavkumar Sawargaonkar]

Hi Ian,

On 3 February 2014 22:34, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Thu, 2014-01-30 at 11:38 +0530, Pranavkumar Sawargaonkar wrote:
>> Hi Ian,
>>
>> On 29 January 2014 18:08, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> > On Tue, 2014-01-28 at 23:27 +0530, Pranavkumar Sawargaonkar wrote:
>> >
>> >> > I also don't see any patch to linux/Documentation/devicetree/bindings,
>> >> > as was requested in that posting from 6 months ago. Where can I find
>> >> > that?
>> >> >
>> >> > It seems like the patch to arch/arm64/boot/dts/apm-storm.dtsi also
>> >> > hasn't landed?
>> >> Yeah it is dangling and since new patch is already posted i think we
>> >> can wait for final DT bindings.
>> >
>> > It seems from the thread that the final bindings are going to differ
>> > significantly from what is implemented in Xen and proposed in the above
>> > thread. (with a syscon driver that the reset driver references).
>> >
>> >> >> Now if you want this to be fixed , i can quickly submit a V7 in which
>> >> >> mask field will be just hard-coded to 1 hence xen code will always
>> >> >> work even if linux code does gets changed.
>> >> >
>> >> > Looks like the Linux driver uses 0xffffffff if the mask isn't given --
>> >> > that seems like a good approach.
>> >> >
>> >> > I think we'll just have to accept that until the binding is specified
>> >> > and documented (in linux/Documentation/devicetree/bindings) then we may
>> >> > have to be prepared to change the Xen implementation to match the final
>> >> > spec without regard to backwards compat. If we aren't happy with that
>> >> > then I should revert the patch now and we will have to live without
>> >> > reboot support in the meantime.
>> >> Please do not revert the patch , I think we can go ahead with current patch.
>> >> Once linux side is concluded i will fix minor changes in xen code
>> >> based on new DT bindigs..
>> >
>> > It doesn't sounds to me like it is going to be minor changes.
>> Yes binding are changed in new drivers but now question is what to do
>> in current state where new driver is not submitted ?
>>
>> My take is we have 3 opts :
>> 1. Keep current reboot driver in xen as it is and use it with old
>> bindings. (since that is the one merged in linux)
>> 2. I will send a new patch (will take 1hr max for me to do it) with
>> addresses hardcoded instead of reading it from dts.
>>     This will help for xen to have reboot driver for xgene.
>> 3. Remove this driver completely from xen as of now.
>
> None of the options are brilliant :-/
>
> I think on balance #2 is probably the way to go.
Even i think this is the best way among all these patchy options :P
Tomorrow I will send a new patch with removal of dts stuff from xen
driver so that it will always work irrespective of linux stuff.
Once linux side comes to conclusion and merged i will reintroduce dts
stuff in this driver.

>
> #1 would set a precedent for using formally undefined bindings which I
> think we should avoid.
>
> #3 has obvious downsides, but given that we have already accepted the
> functionality it seems a shame to revert it entirely.
>
> Ian.
>
-
Pranav

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

[George Dunlap]

On 02/03/2014 05:04 PM, Ian Campbell wrote:
> On Thu, 2014-01-30 at 11:38 +0530, Pranavkumar Sawargaonkar wrote:
>> Hi Ian,
>>
>> On 29 January 2014 18:08, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>>> On Tue, 2014-01-28 at 23:27 +0530, Pranavkumar Sawargaonkar wrote:
>>>
>>>>> I also don't see any patch to linux/Documentation/devicetree/bindings,
>>>>> as was requested in that posting from 6 months ago. Where can I find
>>>>> that?
>>>>>
>>>>> It seems like the patch to arch/arm64/boot/dts/apm-storm.dtsi also
>>>>> hasn't landed?
>>>> Yeah it is dangling and since new patch is already posted i think we
>>>> can wait for final DT bindings.
>>> It seems from the thread that the final bindings are going to differ
>>> significantly from what is implemented in Xen and proposed in the above
>>> thread. (with a syscon driver that the reset driver references).
>>>
>>>>>> Now if you want this to be fixed , i can quickly submit a V7 in which
>>>>>> mask field will be just hard-coded to 1 hence xen code will always
>>>>>> work even if linux code does gets changed.
>>>>> Looks like the Linux driver uses 0xffffffff if the mask isn't given --
>>>>> that seems like a good approach.
>>>>>
>>>>> I think we'll just have to accept that until the binding is specified
>>>>> and documented (in linux/Documentation/devicetree/bindings) then we may
>>>>> have to be prepared to change the Xen implementation to match the final
>>>>> spec without regard to backwards compat. If we aren't happy with that
>>>>> then I should revert the patch now and we will have to live without
>>>>> reboot support in the meantime.
>>>> Please do not revert the patch , I think we can go ahead with current patch.
>>>> Once linux side is concluded i will fix minor changes in xen code
>>>> based on new DT bindigs..
>>> It doesn't sounds to me like it is going to be minor changes.
>> Yes binding are changed in new drivers but now question is what to do
>> in current state where new driver is not submitted ?
>>
>> My take is we have 3 opts :
>> 1. Keep current reboot driver in xen as it is and use it with old
>> bindings. (since that is the one merged in linux)
>> 2. I will send a new patch (will take 1hr max for me to do it) with
>> addresses hardcoded instead of reading it from dts.
>>      This will help for xen to have reboot driver for xgene.
>> 3. Remove this driver completely from xen as of now.
> None of the options are brilliant :-/
>
> I think on balance #2 is probably the way to go.
>
> #1 would set a precedent for using formally undefined bindings which I
> think we should avoid.
>
> #3 has obvious downsides, but given that we have already accepted the
> functionality it seems a shame to revert it entirely.

That sounds reasonable to me.

  -George