On 08.08.2018 02:38, Anson Huang wrote: > Hi, Stefan > > Anson Huang > Best Regards! > > >> -----Original Message----- >> From: Stefan Agner [mailto:ste...@agner.ch] >> Sent: Tuesday, August 7, 2018 6:48 PM >> To: Anson Huang <anson.hu...@nxp.com> >> Cc: sba...@denx.de; Fabio Estevam <fabio.este...@nxp.com>; >> albert.u.b...@aribaud.net; bryan.odonog...@linaro.org; Stefan Agner >> <stefan.ag...@toradex.com>; Peng Fan <peng....@nxp.com>; >> patrick.delau...@st.com; s...@chromium.org; Ye Li <ye...@nxp.com>; >> u-boot@lists.denx.de; dl-linux-imx <linux-...@nxp.com> >> Subject: Re: [U-Boot] [RESEND 1/3] imx: mx7: psci: add cpu hotplug support >> >> Hi Anson, >> >> Thanks for this patchset, looking forward to have full suspend/resume for >> i.MX >> 7 in mainline! >> >> On 07.08.2018 08:46, Anson Huang wrote: >> > This patch adds cpu hotplug support, previous cpu_off implementation >> > is NOT safe, a CPU can NOT power down itself in runtime, it will cause >> > system bus hang due to pending transaction. So need to use other >> > online CPU to kill it when it is ready for killed. >> >> How does this exactly show? > > Such kind of issue has been showed a lot and verified on all i.MX6 > SoCs and i.MX7D, the > best solution should be to let GPC module has ability to kill CPU by > monitoring its > wfi signal when the cpu off request is triggered. A CPU runtime kills > itself will cause > system bus hang when doing stress cpu hotplug test with multi tasks running in > background. Using handshake to let other online CPU to kill the CPU > being offline > is a safe way, this is implemented in all our i.MX6 SoCs and i.MX7D in > Linux kernel. > > You can check the Linux kernel code for details: > > arch/arm/mach-imx/hotplug.c > >> >> It seemed to work somehow here? >> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch >> work.ozlabs.org%2Fpatch%2F933948%2F&data=02%7C01%7CAnson.Hua >> ng%40nxp.com%7Cb302f3b035bd448819c708d5fc53502a%7C686ea1d3bc2b4c >> 6fa92cd99c5c301635%7C0%7C0%7C636692357116793775&sdata=90jsTS >> t9GdKA1f%2B%2BkUdGy3G8Cj2WnnGSarC34dZic0A%3D&reserved=0 >> > > Simple test is passed, but stress test may fail.
I see. > > >> Is it guaranteed that psci_affinity_info gets called afterwards? >> > > Yes, the psci_affinity_info will be called 10 times with 10ms > intervals until dying CPU being OFF, if > within 100ms it is still online, kernel will have warning message > output, but 100ms is enough > for a CPU to be OFF, otherwise, there must be issue. > > arch/arm/kernel/psci_smp.c > 98 for (i = 0; i < 10; i++) { > 99 err = psci_ops.affinity_info(cpu_logical_map(cpu), 0); > 100 if (err == PSCI_0_2_AFFINITY_LEVEL_OFF) { > 101 pr_info("CPU%d killed.\n", cpu); > 102 return 1; > 103 } > 104 > 105 msleep(10); > 106 pr_info("Retrying again to check for CPU kill\n"); > 107 } > > >> The solution seems somewhat hacky, but unfortunately I do not have a better >> idea... Thanks for the clarifications! Can you maybe mention somewhere that the kernel will poll affinity info after a CPU shutdown to make clear that this is guaranteed to be called? -- Stefan > > Yes, we can treat it as a "software workaround", since our GPC does NOT have > hardware function to kill a CPU by monitoring its wfi signal, latest i.MX8 > SoCs > have this feature added. > > Anson. > >> >> -- >> Stefan >> >> > >> > Here use SRC parameter register and a magic number of ~0 as handshake >> > for killing a offline CPU, when the online CPU checks the >> > psci_affinity_info, it will help kill the offline CPU according to the >> > magic number stored in SRC parameter register. >> > >> > Signed-off-by: Anson Huang <anson.hu...@nxp.com> >> > --- >> > arch/arm/mach-imx/mx7/psci-mx7.c | 23 +++++++++++++++++++++-- >> > 1 file changed, 21 insertions(+), 2 deletions(-) >> > >> > diff --git a/arch/arm/mach-imx/mx7/psci-mx7.c >> > b/arch/arm/mach-imx/mx7/psci-mx7.c >> > index aae96c5..d6c4263 100644 >> > --- a/arch/arm/mach-imx/mx7/psci-mx7.c >> > +++ b/arch/arm/mach-imx/mx7/psci-mx7.c >> > @@ -44,6 +44,14 @@ >> > #error "invalid value for CONFIG_ARMV7_PSCI_NR_CPUS" >> > #endif >> > >> > +#define imx_cpu_gpr_entry_offset(cpu) \ >> > + (SRC_BASE_ADDR + SRC_GPR1_MX7D + cpu * 8) #define >> > +imx_cpu_gpr_para_offset(cpu) \ >> > + (imx_cpu_gpr_entry_offset(cpu) + 4) >> > + >> > +#define IMX_CPU_SYNC_OFF ~0 >> > +#define IMX_CPU_SYNC_ON 0 >> > + >> > u8 psci_state[IMX7D_PSCI_NR_CPUS] __secure_data = { >> > PSCI_AFFINITY_LEVEL_ON, >> > PSCI_AFFINITY_LEVEL_OFF}; >> > @@ -116,7 +124,7 @@ __secure s32 psci_cpu_on(u32 __always_unused >> > function_id, u32 mpidr, u32 ep, >> > >> > psci_save(cpu, ep, context_id); >> > >> > - writel((u32)psci_cpu_entry, SRC_BASE_ADDR + cpu * 8 + >> SRC_GPR1_MX7D); >> > + writel((u32)psci_cpu_entry, imx_cpu_gpr_entry_offset(cpu)); >> > >> > psci_set_state(cpu, PSCI_AFFINITY_LEVEL_ON_PENDING); >> > >> > @@ -137,7 +145,11 @@ __secure s32 psci_cpu_off(void) >> > >> > imx_enable_cpu_ca7(cpu, false); >> > imx_gpcv2_set_core_power(cpu, false); >> > - writel(0, SRC_BASE_ADDR + cpu * 8 + SRC_GPR1_MX7D + 4); >> > + /* >> > + * We use the cpu jumping argument register to sync with >> > + * psci_affinity_info() which is running on cpu0 to kill the cpu. >> > + */ >> > + writel(IMX_CPU_SYNC_OFF, imx_cpu_gpr_para_offset(cpu)); >> > >> > while (1) >> > wfi(); >> > @@ -198,6 +210,13 @@ __secure s32 psci_affinity_info(u32 >> > __always_unused function_id, >> > if (cpu >= IMX7D_PSCI_NR_CPUS) >> > return ARM_PSCI_RET_INVAL; >> > >> > + /* CPU is waiting for killed */ >> > + if (readl(imx_cpu_gpr_para_offset(cpu)) == IMX_CPU_SYNC_OFF) { >> > + imx_enable_cpu_ca7(cpu, false); >> > + imx_gpcv2_set_core_power(cpu, false); >> > + writel(IMX_CPU_SYNC_ON, imx_cpu_gpr_para_offset(cpu)); >> > + } >> > + >> > return psci_state[cpu]; >> > } _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot