On Tue, Jun 28, 2016 at 12:10 PM, Chen-Yu Tsai <w...@csie.org> wrote:
> On Tue, Jun 14, 2016 at 3:01 PM, <macro.wav...@gmail.com> wrote:
>> From: Hongbo Zhang <hongbo.zh...@nxp.com>
>>
>> The input parameter CPU ID needs to be validated before furher oprations such
>> as CPU_ON, this patch introduces the function to do this.
>>
>> Signed-off-by: Wang Dongsheng <dongsheng.w...@nxp.com>
>> Signed-off-by: Hongbo Zhang <hongbo.zh...@nxp.com>
>> ---
>> arch/arm/cpu/armv7/ls102xa/psci.S | 34 +++++++++++++++++++++++++++++++++-
>> 1 file changed, 33 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/cpu/armv7/ls102xa/psci.S
>> b/arch/arm/cpu/armv7/ls102xa/psci.S
>> index 548c507..a4482e4 100644
>> --- a/arch/arm/cpu/armv7/ls102xa/psci.S
>> +++ b/arch/arm/cpu/armv7/ls102xa/psci.S
>> @@ -25,6 +25,34 @@
>> #define ONE_MS (GENERIC_TIMER_CLK / 1000)
>> #define RESET_WAIT (30 * ONE_MS)
>>
>
> A note describing the arguments, the return value, and any other affected
> registers would be nice. For example r1 is clobbered.
Good suggestion, thanks.
>
>> +.globl psci_check_target_cpu_id
>> +psci_check_target_cpu_id:
>
> You probably don't need .globl.
>
Oh, this is due to a carelessness.
> Also you can use ENTRY() or LENTRY() here. These and ENDPROC() below
> are defined in linux/linkage.h
>
Yes, sure.
>> + @ Get the real CPU number
>> + and r0, r1, #0xff
>> +
>> + @ Verify bit[31:24], bits must be zero.
>> + tst r1, #0xff000000
>> + bne out_psci_invalid_target_cpu_id
>> +
>> + @ Verify Affinity level 2: Cluster, only one cluster in LS1021xa SoC.
>> + tst r1, #0xff0000
>> + bne out_psci_invalid_target_cpu_id
>> +
>> + @ Verify Affinity level 1: Processors, should be in 0xf00 format.
>> + lsr r1, r1, #8
>> + teq r1, #0xf
>> + bne out_psci_invalid_target_cpu_id
>> +
>> + @ Verify Affinity level 0: CPU, only 0, 1 are valid values.
>> + cmp r0, #2
>> + bge out_psci_invalid_target_cpu_id
>> +
>> + bx lr
>> +
>> +out_psci_invalid_target_cpu_id:
>> + mov r0, #ARM_PSCI_RET_INVAL
>> + bx lr
>> +
>
> And you could have an ENDPROC() here.
>
> About the whole function, you could use an extra scratch register,
> store ARM_PSCI_RET_INVAL in r0 at the beginning, and directly return
> at errors. Kind of like:
>
> if (bit[31:24] != 0)
> return ARM_PSCI_RET_INVAL;
> if (cluster != 0)
> return ARM_PSCI_RET_INVAL;
> if (processor != 0xf)
> return ARM_PSCI_RET_INVAL;
> if (cpu >= 2)
> return ARM_PSCI_RET_INVAL;
> return cpu;
>
> It's just a different style though. Feel free to keep the current
> structure.
Yes, thanks.
>
> The code itself looks good.
>
> Regards
> ChenYu
>
>> @ r1 = target CPU
>> @ r2 = target PC
>> .globl psci_cpu_on
>> @@ -33,7 +61,10 @@ psci_cpu_on:
>>
>> @ Clear and Get the correct CPU number
>> @ r1 = 0xf01
>> - and r1, r1, #0xff
>> + bl psci_check_target_cpu_id
>> + cmp r0, #ARM_PSCI_RET_INVAL
>> + beq out_psci_cpu_on
>> + mov r1, r0
>>
>> bl psci_cpu_on_common
>>
>> @@ -98,6 +129,7 @@ holdoff_release:
>> @ Return
>> mov r0, #ARM_PSCI_RET_SUCCESS
>>
>> +out_psci_cpu_on:
>> pop {lr}
>> bx lr
>>
>> --
>> 2.1.4
>>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
