| Message ID | 1521550743-31907-1-git-send-email-patrick.delaunay@st.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot] arm: stm32mp1: add PSCI support | expand |
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@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? > +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). > + u32 cpu = (mpidr & 0x3); > + > + /* store target PC */ > + psci_save_target_pc(cpu, pc); > + > + /* write entrypoint in backup RAM register */ > + writel((u32)&psci_cpu_entry, TAMP_BACKUP_BRANCH_ADDRESS); > + > + /* write magic number in backup register */ > + writel(BOOT_API_A7_CORE1_MAGIC_NUMBER, TAMP_BACKUP_MAGIC_NUMBER); > + stm32mp_smp_kick_all_cpus(); > + > + return ARM_PSCI_RET_SUCCESS; Does some other part of U-Boot do some state tracking? What happens if this is called for a CPU that's already online? Thanks, Mark.
Hi Mark, > -----Original Message----- > From: Mark Rutland [mailto:mark.rutland@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@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
On Thu, Mar 29, 2018 at 04:59:21PM +0000, Patrick DELAUNAY wrote: > Hi Mark, > > > > -----Original Message----- > > From: Mark Rutland [mailto:mark.rutland@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@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. Lets see how far along you get before the v2018.05 final. I don't think it's a good idea to claim PSCI 1.0 support and not provide all of the required functionality, as that would be breaking the point of declaring that you support something like that :) Thanks!
Hi Tom > From: Tom Rini [mailto:trini@konsulko.com] > > On Thu, Mar 29, 2018 at 04:59:21PM +0000, Patrick DELAUNAY wrote: > > Hi Mark, > > > > > > > From: Mark Rutland [mailto:mark.rutland@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@st.com> > > > > > > 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. > > Lets see how far along you get before the v2018.05 final. I don't think it's a > good idea to claim PSCI 1.0 support and not provide all of the required > functionality, as that would be breaking the point of declaring that you support > something like that :) Thanks! > > -- > Tom I understood and I am agree that it is better, So I will push a v2 as soon as possible with minimal PSCI v1.0 (adding the 2 missing functions). And an other patch to save the context ID for PSCI on arm v7 (with perhaps impact on other platform). Patrick
diff --git a/arch/arm/mach-stm32mp/Kconfig b/arch/arm/mach-stm32mp/Kconfig index 9771af9..c1ad939 100644 --- a/arch/arm/mach-stm32mp/Kconfig +++ b/arch/arm/mach-stm32mp/Kconfig @@ -23,7 +23,10 @@ config SYS_SOC config TARGET_STM32MP1 bool "Support stm32mp1xx" + select ARCH_SUPPORT_PSCI select CPU_V7 + select CPU_V7_HAS_NONSEC + select CPU_V7_HAS_VIRT select PINCTRL_STM32 select STM32_RESET help diff --git a/arch/arm/mach-stm32mp/Makefile b/arch/arm/mach-stm32mp/Makefile index a495c53..f746691 100644 --- a/arch/arm/mach-stm32mp/Makefile +++ b/arch/arm/mach-stm32mp/Makefile @@ -9,3 +9,4 @@ obj-y += dram_init.o obj-y += syscon.o obj-$(CONFIG_SPL_BUILD) += spl.o +obj-$(CONFIG_ARMV7_PSCI) += psci.o diff --git a/arch/arm/mach-stm32mp/include/mach/stm32.h b/arch/arm/mach-stm32mp/include/mach/stm32.h index c7a2789..d6f960f 100644 --- a/arch/arm/mach-stm32mp/include/mach/stm32.h +++ b/arch/arm/mach-stm32mp/include/mach/stm32.h @@ -74,6 +74,8 @@ enum boot_device { /* TAMP registers */ #define TAMP_BACKUP_REGISTER(x) (STM32_TAMP_BASE + 0x100 + 4 * x) +#define TAMP_BACKUP_MAGIC_NUMBER TAMP_BACKUP_REGISTER(4) +#define TAMP_BACKUP_BRANCH_ADDRESS TAMP_BACKUP_REGISTER(5) #define TAMP_BOOT_CONTEXT TAMP_BACKUP_REGISTER(20) #define TAMP_BOOT_MODE_MASK GENMASK(15, 8) diff --git a/arch/arm/mach-stm32mp/psci.c b/arch/arm/mach-stm32mp/psci.c new file mode 100644 index 0000000..0912242 --- /dev/null +++ b/arch/arm/mach-stm32mp/psci.c @@ -0,0 +1,91 @@ +/* + * Copyright (C) 2018, STMicroelectronics - All Rights Reserved + * + * SPDX-License-Identifier: GPL-2.0+ BSD-3-Clause + */ + +#include <config.h> +#include <common.h> +#include <asm/armv7.h> +#include <asm/gic.h> +#include <asm/io.h> +#include <asm/psci.h> +#include <asm/secure.h> + +#define BOOT_API_A7_CORE1_MAGIC_NUMBER 0xCA7FACE1 + +#define RCC_MP_GRSTCSETR (STM32_RCC_BASE + 0x0404) +#define RCC_MP_GRSTCSETR_MPUP1RST BIT(5) +#define RCC_MP_GRSTCSETR_MPSYSRST BIT(0) + +static u32 __secure stm32mp_get_gicd_base_address(void) +{ + u32 periphbase; + + /* get the GIC base address from the CBAR register */ + asm("mrc p15, 4, %0, c15, c0, 0\n" : "=r" (periphbase)); + + return (periphbase & CBAR_MASK) + GIC_DIST_OFFSET; +} + +static void __secure stm32mp_smp_kick_all_cpus(void) +{ + u32 gic_dist_addr; + + gic_dist_addr = stm32mp_get_gicd_base_address(); + + /* kick all CPUs (except this one) by writing to GICD_SGIR */ + writel(1U << 24, gic_dist_addr + GICD_SGIR); +} + +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; +} + +int __secure psci_cpu_on(u32 __always_unused unused, u32 mpidr, u32 pc) +{ + u32 cpu = (mpidr & 0x3); + + /* store target PC */ + psci_save_target_pc(cpu, pc); + + /* write entrypoint in backup RAM register */ + writel((u32)&psci_cpu_entry, TAMP_BACKUP_BRANCH_ADDRESS); + + /* write magic number in backup register */ + writel(BOOT_API_A7_CORE1_MAGIC_NUMBER, TAMP_BACKUP_MAGIC_NUMBER); + stm32mp_smp_kick_all_cpus(); + + return ARM_PSCI_RET_SUCCESS; +} + +void __secure psci_cpu_off(void) +{ + /* reset core #1 : wfi is managed by BootRom */ + writel(RCC_MP_GRSTCSETR_MPUP1RST, RCC_MP_GRSTCSETR); + /* just waiting reset */ + while (1) + wfi(); +} + +void __secure psci_system_reset(u32 function_id) +{ + /* System reset */ + writel(RCC_MP_GRSTCSETR_MPSYSRST, RCC_MP_GRSTCSETR); + /* just waiting reset */ + while (1) + wfi(); +} diff --git a/include/configs/stm32mp1.h b/include/configs/stm32mp1.h index da0e259..e0e7747 100644 --- a/include/configs/stm32mp1.h +++ b/include/configs/stm32mp1.h @@ -19,6 +19,11 @@ #define CONFIG_SYS_HZ 1000 #define CONFIG_SYS_ARCH_TIMER +/* PSCI support */ +#define CONFIG_ARMV7_PSCI_1_0 +#define CONFIG_ARMV7_SECURE_BASE STM32_SYSRAM_BASE +#define CONFIG_ARMV7_SECURE_MAX_SIZE STM32_SYSRAM_SIZE + /* * malloc() pool size */
Add minimal PSCI support for Linux. Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com> --- arch/arm/mach-stm32mp/Kconfig | 3 + arch/arm/mach-stm32mp/Makefile | 1 + arch/arm/mach-stm32mp/include/mach/stm32.h | 2 + arch/arm/mach-stm32mp/psci.c | 91 ++++++++++++++++++++++++++++++ include/configs/stm32mp1.h | 5 ++ 5 files changed, 102 insertions(+) create mode 100644 arch/arm/mach-stm32mp/psci.c