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

[Julien Grall <julien.grall@linaro.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 <pranavkumar@linaro.org>
>>> Signed-off-by: Anup Patel <anup.patel@linaro.org>
>>> ---
>>>  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?
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