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

[Julien Grall <julien.grall@linaro.org>]

On 01/27/2014 02:27 PM, Ian Campbell wrote:
> 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@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?
> 
> 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.

Not every platform. For instance on the Arndale and the Versatile
Express we don't need the loop.

After looking to the reset code of X-Gene on Linux it's the same. I'm
surprised that we would need the loop in Xen.

The only ways we can fail are:
	- ioremap return NULL;
	- reset address is not set.

Both won't work at the second attempd, neither the third.

-- 
Julien Grall