[U-Boot] [PATCH] arm: stm32mp1: add PSCI support

[Patrick DELAUNAY]

Hi  Mark,


> -----Original Message-----
> From: Mark Rutland [mailto:mark.rutland at arm.com]
> 
> Hi,
> 
> On Tue, Mar 20, 2018 at 01:59:03PM +0100, Patrick Delaunay wrote:
> > Add minimal PSCI support for Linux.
> >
> > Signed-off-by: Patrick Delaunay <patrick.delaunay at st.com>
> 
> > +int __secure psci_features(unsigned int psci_fid) {
> > +	switch (psci_fid) {
> > +	case ARM_PSCI_0_2_FN_PSCI_VERSION:
> > +	case ARM_PSCI_0_2_FN_CPU_ON:
> > +	case ARM_PSCI_0_2_FN_CPU_OFF:
> > +	case ARM_PSCI_0_2_FN_SYSTEM_RESET:
> > +		return 0x0;
> > +	}
> > +	return ARM_PSCI_RET_NI;
> > +}
> >
> > +unsigned int __secure psci_version(void) {
> > +	return ARM_PSCI_VER_1_0;
> > +}
> 
> I'm a bit worried, because while PSCI_VERSION reports PSCI 1.0, this does not
> appear to be conformant with teh PSCI 1.0 spec, as some mandatory functions
> are missing:
> 
> * AFFINITY_INFO -- Linux relies on this to ensure that CPUs have exited
>   the kernel when calling CPU_OFF (e.g. so that we don't free any data /
>   code that said CPU may be using on the path to calling CPU_OFF).
> 
> * SYSTEM_OFF -- Can you implement this similarly to SYSTEM_RESET?

I push this patch for STM32MP1 to be able to boot the Kernel version on board
(requested by ST Linux team : https://patchwork.kernel.org/patch/10167343/)

This patch  a minimal PSCI support to be able to boot.... 
And I miss these part of the requirement PSCI v1.0 and also in issue in Linux trace:

[    0.000000] psci: probing for conduit method from DT.
[    0.000000] psci: PSCIv1.0 detected in firmware.
[    0.000000] psci: Using standard PSCI v0.2 function IDs
[    0.000000] psci: MIGRATE_INFO_TYPE not supported.

Thanks to point these point .
So I will add them in a second patch " arm: stm32mp1: complete PSCI v1.0 support"   

I want keep this patch in v1, to have it merged in v2018.05-rc1, I hope...
Even if it is only a first step for a full PSCI v1.0 support, that allow linux kernel boot.

And I need some time to check how to handle SYSTEM_OFF , to do the "completely removes power" because I don't think if it is possible today.

> > +int __secure psci_cpu_on(u32 __always_unused unused, u32 mpidr, u32
> > +pc) {
> 
> What about the context_id? PSCI 0.2+ mandates it, and some project rely upon
> it (though Linux currently does not).

Yes context_id is not used in my code.....

In fact, I use the same psci_cpu_on prototypes and logic than other platform :
 ./arch/arm/cpu/armv7/sunxi/psci.c:246
./arch/arm/mach-uniphier/arm32/psci.c:134
./arch/arm/cpu/armv7/ls102xa/psci.S:117

=> all are based on psci_save_target_pc() to save the PC in secure context (psci_target_pc[])
	./arch/arm/cpu/armv7/psci-common.c:29

But context is not saved or used in generic PSCI code, only pc is restored
	=> ./arch/arm/cpu/armv7/psci.S:330
		r0 = psci_get_target_pc() is called before _do_nonsec_entry

I will check deeper in this U-Boot PSCI code.

=> I try to solve the problem and push a RFC or a patch  to have feedback form other PSCI user.
     Even it is not block for Linux point of view, as context ID is not used in linux code always = 0:
     ./drivers/firmware/psci.c:
	static int psci_cpu_on(unsigned long cpuid, unsigned long entry_point)
	....
		err = invoke_psci_fn(fn, cpuid, entry_point, 0);
 
> Does some other part of U-Boot do some state tracking? What happens if this is
> called for a CPU that's already online?

No issue,  the CPU continue to run....
TAMP register is only used by BootRom to unpark the cpu on boot, update of the backup have no impact when the CPU is running.

I will add a comment in the next patch " arm: stm32mp1: complete PSCI v1.0 support"  
and also check if I can add test the cpu state to manage other possible return value (never used in U-Boot):
	PSCI_RET_ALREADY_ON 
	PSCI_RET_ON_PENDING

> 
> Thanks,
> Mark.

Very thanks for the review.
That allows me to read deeper the PSCI documents.

Tom:  do you think that this patch can be merged in v2018.05-rc1 ?
           and I will sent a 2nd patch for full PCSI v1.0 support and correct the missing parts reported by Mark.

Thanks

Patrick