On 08.08.2018 08:02, Anson Huang wrote:
> Hi, Stefan
>
> Anson Huang
> Best Regards!
>
>
>> -----Original Message-----
>> From: Stefan Agner [mailto:ste...@agner.ch]
>> Sent: Wednesday, August 8, 2018 1:54 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
>>
>> 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%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?
Oh I see you already sent v2. Yeah that is fine for me, its not that
important.
--
Stefan
>
> 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 <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
