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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > Signed-off-by: Anup Patel <anup.patel@xxxxxxxxxx>
> > ---
> >  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.



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