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

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(+)

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.

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

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.