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



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