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