Hi, Stefan Anson Huang Best Regards!
> -----Original Message----- > From: Stefan Agner [mailto:[email protected]] > Sent: Wednesday, August 8, 2018 1:54 PM > To: Anson Huang <[email protected]> > Cc: [email protected]; Fabio Estevam <[email protected]>; > [email protected]; [email protected]; Stefan Agner > <[email protected]>; Peng Fan <[email protected]>; > [email protected]; [email protected]; Ye Li <[email protected]>; > [email protected]; dl-linux-imx <[email protected]> > Subject: Re: [U-Boot] [RESEND 1/3] imx: mx7: psci: add cpu hotplug support > > On 08.08.2018 02:38, Anson Huang wrote: > > Hi, Stefan > > > > Anson Huang > > Best Regards! > > > > > >> -----Original Message----- > >> From: Stefan Agner [mailto:[email protected]] > >> Sent: Tuesday, August 7, 2018 6:48 PM > >> To: Anson Huang <[email protected]> > >> Cc: [email protected]; Fabio Estevam <[email protected]>; > >> [email protected]; [email protected]; Stefan Agner > >> <[email protected]>; Peng Fan <[email protected]>; > >> [email protected]; [email protected]; Ye Li <[email protected]>; > >> [email protected]; dl-linux-imx <[email protected]> > >> 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%2Fpa > >> tch > 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? I think most of people familiar with the Linux power management feature should know the cpu kill flow/method, if there are other comments for this patch set, I will add a detail note here in V3 patch set, if no comments, I think it should be also just fine, is that OK for you? Anson. > > -- > 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 <[email protected]> > >> > --- > >> > 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 [email protected] https://lists.denx.de/listinfo/u-boot
