[U-Boot] [PATCHV5 4/6] ARMv8/Layerscape: switch SMP method accordingly

[york sun]

On 06/11/2016 08:49 PM, Zhiqiang Hou wrote:
> Hi York,
>
> Thanks for your comments!
>
>> -----Original Message-----
>> From: York Sun [mailto:york.sun at nxp.com]
>> Sent: 2016年6月8日 8:51
>> To: Zhiqiang Hou <zhiqiang.hou at nxp.com>; u-boot at lists.denx.de;
>> albert.u.boot at aribaud.net; scottwood at freescale.com;
>> Mingkai.hu at freescale.com; yorksun at freescale.com; leoli at freescale.com;
>> prabhakar at freescale.com; bhupesh.sharma at freescale.com
>> Subject: Re: [PATCHV5 4/6] ARMv8/Layerscape: switch SMP method accordingly
>>
>> On 06/04/2016 11:40 PM, Zhiqiang Hou wrote:
>>> From: Hou Zhiqiang <Zhiqiang.Hou at nxp.com>
>>>
>>> If the PSCI and PPA is ready, skip the fixup for spin-table and waking
>>> secondary cores. If not, change SMP method to spin-table, and the
>>> device node of PSCI will be removed.
>>>
>>> Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou at nxp.com>
>>> ---
>>> V5:
>>>   - Changed the checking if the PSCI feature is ready to read the psci version.
>>>
>>> V4:
>>>   - Reordered this patch.
>>>
>>>   arch/arm/cpu/armv8/fsl-layerscape/cpu.c | 17 +++++++++++++---
>>> arch/arm/cpu/armv8/fsl-layerscape/fdt.c | 36
>>> +++++++++++++++++++++++++++++++++
>>>   2 files changed, 50 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/arm/cpu/armv8/fsl-layerscape/cpu.c
>>> b/arch/arm/cpu/armv8/fsl-layerscape/cpu.c
>>> index 672a453..eb566cd 100644
>>> --- a/arch/arm/cpu/armv8/fsl-layerscape/cpu.c
>>> +++ b/arch/arm/cpu/armv8/fsl-layerscape/cpu.c
>>> @@ -23,6 +23,9 @@
>>>   #ifdef CONFIG_FSL_ESDHC
>>>   #include <fsl_esdhc.h>
>>>   #endif
>>> +#ifdef CONFIG_ARMV8_SEC_FIRMWARE_SUPPORT #include
>>> +<asm/armv8/sec_firmware.h> #endif
>>>
>>>   DECLARE_GLOBAL_DATA_PTR;
>>>
>>> @@ -618,6 +621,7 @@ int arch_early_init_r(void)  {  #ifdef CONFIG_MP
>>>   	int rv = 1;
>>> +	bool psci_support = false;
>>>   #endif
>>>
>>>   #ifdef CONFIG_SYS_FSL_ERRATUM_A009635 @@ -625,9 +629,16 @@ int
>>> arch_early_init_r(void)  #endif
>>>
>>>   #ifdef CONFIG_MP
>>> -	rv = fsl_layerscape_wake_seconday_cores();
>>> -	if (rv)
>>> -		printf("Did not wake secondary cores\n");
>>> +#if defined(CONFIG_ARMV8_SEC_FIRMWARE_SUPPORT) &&
>> defined(CONFIG_ARMV8_PSCI)
>>> +	/* Check the psci version to determine if the psci is supported */
>>> +	psci_support = (int)sec_firmware_support_psci_version() > 0 ?
>>> +			true : false;
>>> +#endif
>>> +	if (!psci_support) {
>>> +		rv = fsl_layerscape_wake_seconday_cores();
>>> +		if (rv)
>>> +			printf("Did not wake secondary cores\n");
>>> +	}
>>>   #endif
>>>
>>
>> Zhiqiang,
>>
>> Here you presume the psci version returned by
>> sec_firmware_support_psci_version() is a proof of a functional psci. I think that is
>> flawed. This sec_firmware_support_psci_version() can return a positive number as
>> far as the image in specified location is valid. It doesn't guarantee the image is
>> running. The only place you would know the result of loading such image is by
>> calling sec_firmware_init() in board_init() in your 6th patch. But you didn't check
>> the return. That means you blindly believe a valid image would be successfully
>> loaded and started to run. Later when you need to decide to use spin-table or PSCI
>> for secondary cores, you don't know if such image is running.
>>
>> I wonder if you can make a psci call to return the version number.
>
> So far, the function sec_firmware_support_psci_version() do the psci call to get the
> supported psci version. And if the returned psci version is valid, then we assume the
> PPA is running, otherwise if the returned value is an error number, we assume the
> PPA isn't running regularly. The return value of the function was casted to type 'int',
> because the psci_version call will return the version by pattern major[31:16]:minor[15:0]
> in normal, but a negative error number while in exception.
>
I see. It verifies the image is valid first, then it makes an smc call. 
Thanks.

York