Hi Scott,

Thanks for your review.

> On Tue, 2016-01-19 at 06:28 +0000, Dongsheng Wang wrote:
> > Hi Scott,
> >
> > > On Mon, 2016-01-18 at 12:27 +0800, Dongsheng Wang wrote:
> > > > From: Wang Dongsheng <dongsheng.w...@nxp.com>
> > > >
> > > > Based on PSCI v1.0, implement interface for ls102xa SoC:
> > > > psci_version,
> > > > psci_features,
> > > > psci_cpu_suspend,
> > > > psci_affinity_info,
> > > > psci_system_reset,
> > > > psci_system_off.
> > > >
> > > > Tested on LS1021aQDS, LS1021aTWR.
> > > >
> > > > Signed-off-by: Wang Dongsheng <dongsheng.w...@nxp.com>
> > > > ---
> > > >  arch/arm/cpu/armv7/ls102xa/psci.S          | 110
> > > > +++++++++++++++++++++++++++--
> > > >  arch/arm/include/asm/arch-ls102xa/config.h |   1 +
> > > >  board/freescale/ls1021aqds/Makefile        |   1 +
> > > >  board/freescale/ls1021aqds/psci.S          |  36 ++++++++++
> > > >  board/freescale/ls1021atwr/Makefile        |   1 +
> > > >  board/freescale/ls1021atwr/psci.S          |  28 ++++++++
> > > >  include/configs/ls1021aqds.h               |   3 +
> > > >  include/configs/ls1021atwr.h               |   1 +
> > > >  8 files changed, 177 insertions(+), 4 deletions(-)  create mode
> > > > 100644 board/freescale/ls1021aqds/psci.S  create mode 100644
> > > > board/freescale/ls1021atwr/psci.S
> > > >
> > > > diff --git a/arch/arm/cpu/armv7/ls102xa/psci.S
> > > > b/arch/arm/cpu/armv7/ls102xa/psci.S
> > > > index 3091362..bfc908e 100644
> > > > --- a/arch/arm/cpu/armv7/ls102xa/psci.S
> > > > +++ b/arch/arm/cpu/armv7/ls102xa/psci.S
> > > > @@ -12,19 +12,72 @@
> > > >  #include <asm/arch-armv7/generictimer.h>  #include <asm/psci.h>
> > > >
> > > > +#define RCPM_TWAITSR           0x04C
> > > > +
> > > >  #define SCFG_CORE0_SFT_RST      0x130
> > > >  #define SCFG_CORESRENCR         0x204
> > > >
> > > > -#define DCFG_CCSR_BRR           0x0E4
> > > > -#define DCFG_CCSR_SCRATCHRW1    0x200
> > > > +#define DCFG_CCSR_RSTCR                        0x0B0
> > > > +#define DCFG_CCSR_RSTCR_RESET_REQ      0x2
> > > > +#define DCFG_CCSR_BRR                  0x0E4
> > > > +#define DCFG_CCSR_SCRATCHRW1           0x200
> > > > +
> > > > +#define PSCI_FN_PSCI_VERSION_FEATURE_MASK      0x0
> > > > +#define PSCI_FN_CPU_SUSPEND_FEATURE_MASK       0x0
> > > > +#define PSCI_FN_CPU_OFF_FEATURE_MASK           0x0
> > > > +#define PSCI_FN_CPU_ON_FEATURE_MASK            0x0
> > > > +#define PSCI_FN_AFFINITY_INFO_FEATURE_MASK     0x0
> > > > +#define PSCI_FN_SYSTEM_OFF_FEATURE_MASK                0x0
> > > > +#define PSCI_FN_SYSTEM_RESET_FEATURE_MASK      0x0
> > > >
> > > >         .pushsection ._secure.text, "ax"
> > > >
> > > >         .arch_extension sec
> > > >
> > > > +       .align  5
> > > > +
> > > >  #define        ONE_MS          (GENERIC_TIMER_CLK / 1000)
> > > >  #define        RESET_WAIT      (30 * ONE_MS)
> > > >
> > > > +.globl psci_version
> > > > +psci_version:
> > > > +       movw    r0, #0
> > > > +       movt    r0, #1
> > > > +
> > > > +       bx      lr
> > > > +
> > > > +_ls102x_psci_supported_table:
> > > > +       .word   PSCI_FN_PSCI_VERSION
> > > > +       .word   PSCI_FN_PSCI_VERSION_FEATURE_MASK
> > > > +       .word   PSCI_FN_CPU_SUSPEND
> > > > +       .word   PSCI_FN_CPU_SUSPEND_FEATURE_MASK
> > > > +       .word   PSCI_FN_CPU_OFF
> > > > +       .word   PSCI_FN_CPU_OFF_FEATURE_MASK
> > > > +       .word   PSCI_FN_CPU_ON
> > > > +       .word   PSCI_FN_CPU_ON_FEATURE_MASK
> > > > +       .word   PSCI_FN_AFFINITY_INFO
> > > > +       .word   PSCI_FN_AFFINITY_INFO_FEATURE_MASK
> > > > +       .word   PSCI_FN_SYSTEM_OFF
> > > > +       .word   PSCI_FN_SYSTEM_OFF_FEATURE_MASK
> > > > +       .word   PSCI_FN_SYSTEM_RESET
> > > > +       .word   PSCI_FN_SYSTEM_RESET_FEATURE_MASK
> > > > +       .word   0
> > > > +       .word   PSCI_RET_NOT_SUPPORTED
> > >
> > > Can you use the main _psci_table instead of duplicating it?
> > >
> >
> > The main table does not apply here. Because this table shows what is
> > supported in our platform.
> 
> How does that set differ from what's in the main table?
> 

The main table include all of functions about PSCI spec. If we use main table
to match the pass in parameter, it will always return SUPPORT.

This table just we supported table, if the parameter not matched in this table
it will finally return NOT_SUPPORTED.

The main table include in functions ID and function handle, not match this 
feature
return value, we not need to get the functions handler, just need to return 
sub-feature,
and this table will return sub-feature if the PSCI ID be matched.

> > And this table also contains the sub-feature mask of PSCI functions.
> 
> ...which is always zero.  As of PSCI 1.0 there's only one function that 
> supports
> subfeatures, and you could put an explicit check in for that if it ever needs 
> a
> non-zero value.
> 

Yes, for now there is only one function that supports sub-features. But this 
table is
easy to extend, even if PSCI defines some other sub-features we just need to
take care this table, and not need to modify the code logic. :)

> > > > +
> > > > +.globl psci_features
> > > > +psci_features:
> > > > +       adr     r2, _ls102x_psci_supported_table
> > > > +1:     ldr     r3, [r2]
> > > > +       cmp     r3, #0
> > > > +       beq     out_psci_features
> > > > +       cmp     r1, r3
> > > > +       addne   r2, r2, #8
> > > > +       bne     1b
> > >
> > > Why are you adding 8 here?
> > >
> >
> > +4 is the sub-feature mask of the PSCI function. So we need to +8 to
> > +jump to
> > next PSCI function.
> > .word       PSCI_FN_PSCI_VERSION
> > .word       PSCI_FN_PSCI_VERSION_FEATURE_MASK
> >
> > > > +
> > > > +out_psci_features:
> > > > +       ldr     r0, [r2, #4]
> > > > +       bx      lr
> > >
> > > If you find a match, you're supposed to return zero, not the next
> > > function id in the table.
> > > How did you test this?  There should really be a test suite for
> > > runtime services such as this, especially when trying to comply with
> > > a standard.
> >
> > I think maybe you missed something about this code. The return value
> > is PSCI_FN_PSCI_XXXXXX_FEATURE_MASK, not return next function ID.
> 
> Yes, I misread the table and missed the masks.  But see above about them
> being unnecessary.
> 
> In any case, a test suite would be very helpful.
> 
Yes, thanks for your friendly reminder. And this table has tested on ls1021 
platform. :)

Regards,
-Dongsheng

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

Reply via email to