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

Message ID 1390812173-12081-1-git-send-email-pranavkumar@linaro.org
State New
Headers show

Commit Message

PranavkumarSawargaonkar Jan. 27, 2014, 8:42 a.m.
This patch adds a reset support for xgene arm64 platform.

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 |   63 ++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

Comments

Ian Campbell Jan. 27, 2014, 10:32 a.m. | #1
On Mon, 2014-01-27 at 14:12 +0530, Pranavkumar Sawargaonkar wrote:
> +    /* 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 )
> +    {

At this point reset_addr and reset_size are set and xgene_storm_reset
will try to use them with reset_mask -- which I suppose is 0 at this
point (due to .bss initialisation + dt_prop_read not touching it on
failure).

Is that safe / ok?

In fact, on failure of the dt_device_get_address xgene_storm_reset will
try and use the values too and they may be uninitialised or may be
DT_BAD_ADDR depending on the location of the failure.

Could this be potentially harmful? Obviously it is not expected to
successfully reset under these circumstances, but what else could it do?
(e.g. turn off the fan and melt the system?)

I'd suggest to set a flag right at the end which xgene_storm_reset can
check. Either an explicit boolean or initialise reset_addr to ~(u64)0
when it is declared and gather the address into a local variable before
setting the global only after init has succeeded.

(I'd also accept your assurances that writing to random memory locations
is safe, on the off chance that this is true ;-))

Ian.
PranavkumarSawargaonkar Jan. 27, 2014, 11:05 a.m. | #2
Hi Ian,

On 27 January 2014 16:02, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Mon, 2014-01-27 at 14:12 +0530, Pranavkumar Sawargaonkar wrote:
>> +    /* 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 )
>> +    {
>
> At this point reset_addr and reset_size are set and xgene_storm_reset
> will try to use them with reset_mask -- which I suppose is 0 at this
> point (due to .bss initialisation + dt_prop_read not touching it on
> failure).
I thought that if reset_addr or reset_size is 0 then ioremap will not
return me a success hence code will return it without writing the reg.
But i think mask can cause issue if uninitialized, so i will add a
global flag  (valid_reset_vals) and will set it at the end of the
xgene_storm_init, and in fuction xgene_storm_reset i will first check
if this falg is set or not before proceeding further in reset.
Is this fine ?

>
> Is that safe / ok?
>
> In fact, on failure of the dt_device_get_address xgene_storm_reset will
> try and use the values too and they may be uninitialised or may be
> DT_BAD_ADDR depending on the location of the failure.
>
> Could this be potentially harmful? Obviously it is not expected to
> successfully reset under these circumstances, but what else could it do?
> (e.g. turn off the fan and melt the system?)
>
> I'd suggest to set a flag right at the end which xgene_storm_reset can
> check. Either an explicit boolean or initialise reset_addr to ~(u64)0
> when it is declared and gather the address into a local variable before
> setting the global only after init has succeeded.

Ok will do that.
>
> (I'd also accept your assurances that writing to random memory locations
> is safe, on the off chance that this is true ;-))
>
> Ian.
>

Thanks,
Pranav
Ian Campbell Jan. 27, 2014, 11:13 a.m. | #3
On Mon, 2014-01-27 at 16:35 +0530, Pranavkumar Sawargaonkar wrote:
> Hi Ian,
> 
> On 27 January 2014 16:02, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Mon, 2014-01-27 at 14:12 +0530, Pranavkumar Sawargaonkar wrote:
> >> +    /* 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 )
> >> +    {
> >
> > At this point reset_addr and reset_size are set and xgene_storm_reset
> > will try to use them with reset_mask -- which I suppose is 0 at this
> > point (due to .bss initialisation + dt_prop_read not touching it on
> > failure).
> I thought that if reset_addr or reset_size is 0 then ioremap will not
> return me a success hence code will return it without writing the reg.

I'm not sure about the reset_size==0, I'd hope that ioremap does work
for reset_addr == 0 though, since it isn't implausible that something
might live there on some platform.

But in any case after partial success gathering the parameters these may
no longer be zero.

> But i think mask can cause issue if uninitialized, so i will add a
> global flag  (valid_reset_vals) and will set it at the end of the
> xgene_storm_init, and in fuction xgene_storm_reset i will first check
> if this falg is set or not before proceeding further in reset.
> Is this fine ?

Sounds good to me.

Patch

diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
index 5b0bd5f..3825269 100644
--- a/xen/arch/arm/platforms/xgene-storm.c
+++ b/xen/arch/arm/platforms/xgene-storm.c
@@ -20,8 +20,14 @@ 
 
 #include <xen/config.h>
 #include <asm/platform.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 uint32_t xgene_storm_quirks(void)
 {
     return PLATFORM_QUIRK_GIC_64K_STRIDE;
@@ -107,6 +113,61 @@  err:
     return ret;
 }
 
+static void xgene_storm_reset(void)
+{
+    void __iomem *addr;
+
+    addr = ioremap_nocache(reset_addr, reset_size);
+
+    if ( !addr )
+    {
+        printk("XGENE: Unable to map xgene reset address\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;
+    }
+
+    return 0;
+}
 
 static const char * const xgene_storm_dt_compat[] __initconst =
 {
@@ -116,6 +177,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,