On 06/11/2016 08:49 PM, Zhiqiang Hou wrote:
> Hi York,
>
> Thanks for your comments!
>
>> -----Original Message-----
>> From: York Sun [mailto:york....@nxp.com]
>> Sent: 2016年6月8日 8:51
>> To: Zhiqiang Hou <zhiqiang....@nxp.com>; u-boot@lists.denx.de;
>> albert.u.b...@aribaud.net; scottw...@freescale.com;
>> mingkai...@freescale.com; york...@freescale.com; le...@freescale.com;
>> prabha...@freescale.com; bhupesh.sha...@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....@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....@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


_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to