From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH V6] xen: arm: platforms: Adding reset support for xgene arm64 platform. Date: Mon, 27 Jan 2014 14:24:43 +0000 Message-ID: <52E66C2B.9020901@linaro.org> References: <1390822488-22183-1-git-send-email-pranavkumar@linaro.org> <52E660CF.4010606@linaro.org> <1390832020.12230.32.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1390832020.12230.32.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: Anup Patel , patches@linaro.org, patches@apm.com, xen-devel@lists.xen.org, stefano.stabellini@citrix.com, Pranavkumar Sawargaonkar List-Id: xen-devel@lists.xenproject.org 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 >>> Signed-off-by: Anup Patel >>> --- >>> 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 >>> #include >>> +#include >>> +#include >>> +#include >>> #include >>> >>> +/* 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