[U-Boot] [PATCH 8/9] ARM: ARMv7: PSCI: ls102xa: add psci functions implemention

[Scott Wood <oss@buserror.net>]

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.wang@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.wang@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?

> 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.

> > > +
> > > +.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.

-Scott