| Message ID | 20190322114833.12686-4-m.szyprowski@samsung.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Little cleanup of mach-exynos code | expand |
On Fri, 22 Mar 2019 at 12:48, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > Add timeout to infinite loops during the CPU powerup procedures. It > is better to report an error instead of busylooping for infinite time > in case of failure. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > arch/arm/mach-exynos/mcpm-exynos.c | 11 ++++++++++- > arch/arm/mach-exynos/platsmp.c | 9 ++++++++- > 2 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/mach-exynos/mcpm-exynos.c b/arch/arm/mach-exynos/mcpm-exynos.c > index 72bc035bedbe..43d6f7755842 100644 > --- a/arch/arm/mach-exynos/mcpm-exynos.c > +++ b/arch/arm/mach-exynos/mcpm-exynos.c > @@ -75,14 +75,23 @@ static int exynos_cpu_powerup(unsigned int cpu, unsigned int cluster) > */ > if (cluster && > cluster == MPIDR_AFFINITY_LEVEL(cpu_logical_map(0), 1)) { > + unsigned int timeout = 16; One empty line please. > /* > * Before we reset the Little cores, we should wait > * the SPARE2 register is set to 1 because the init > * codes of the iROM will set the register after > * initialization. > */ > - while (!pmu_raw_readl(S5P_PMU_SPARE2)) > + while (timeout && !pmu_raw_readl(S5P_PMU_SPARE2)) { > + timeout--; > udelay(10); > + } > + > + if (timeout == 0) { > + pr_err("cpu %d cluster %d powerup failed\n", > + cpu, cluster); %u - you should have a warning here. > + return -ETIMEDOUT; I wander whether reversing is necessary on error path (exynos_cluster_power_down()). In case of timeout, CPU will be left with stuck with power supplied. In the same time the next power up try might not work properly because CPU was not shutdown. > + } > > pmu_raw_writel(EXYNOS5420_KFC_CORE_RESET(cpu), > EXYNOS_SWRESET); > diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c > index d5d48fbdab17..c01e2f7ba1ec 100644 > --- a/arch/arm/mach-exynos/platsmp.c > +++ b/arch/arm/mach-exynos/platsmp.c > @@ -214,13 +214,20 @@ static inline void __iomem *cpu_boot_reg(int cpu) > */ > void exynos_core_restart(u32 core_id) > { > + unsigned int timeout = 16; > u32 val; > > if (!soc_is_exynos3250()) > return; > > - while (!pmu_raw_readl(S5P_PMU_SPARE2)) > + while (timeout && !pmu_raw_readl(S5P_PMU_SPARE2)) { > + timeout--; > udelay(10); > + } > + if (timeout == 0) { > + pr_err("cpu core %d restart failed\n", core_id); %u - you should have a warning here. Best regards, Krzysztof
Hi Krzysztof, On 2019-03-22 15:54, Krzysztof Kozlowski wrote: > On Fri, 22 Mar 2019 at 12:48, Marek Szyprowski <m.szyprowski@samsung.com> wrote: >> Add timeout to infinite loops during the CPU powerup procedures. It >> is better to report an error instead of busylooping for infinite time >> in case of failure. >> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >> --- >> arch/arm/mach-exynos/mcpm-exynos.c | 11 ++++++++++- >> arch/arm/mach-exynos/platsmp.c | 9 ++++++++- >> 2 files changed, 18 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm/mach-exynos/mcpm-exynos.c b/arch/arm/mach-exynos/mcpm-exynos.c >> index 72bc035bedbe..43d6f7755842 100644 >> --- a/arch/arm/mach-exynos/mcpm-exynos.c >> +++ b/arch/arm/mach-exynos/mcpm-exynos.c >> @@ -75,14 +75,23 @@ static int exynos_cpu_powerup(unsigned int cpu, unsigned int cluster) >> */ >> if (cluster && >> cluster == MPIDR_AFFINITY_LEVEL(cpu_logical_map(0), 1)) { >> + unsigned int timeout = 16; > One empty line please. > >> /* >> * Before we reset the Little cores, we should wait >> * the SPARE2 register is set to 1 because the init >> * codes of the iROM will set the register after >> * initialization. >> */ >> - while (!pmu_raw_readl(S5P_PMU_SPARE2)) >> + while (timeout && !pmu_raw_readl(S5P_PMU_SPARE2)) { >> + timeout--; >> udelay(10); >> + } >> + >> + if (timeout == 0) { >> + pr_err("cpu %d cluster %d powerup failed\n", >> + cpu, cluster); > %u - you should have a warning here. arm-linux-gnueabi-gcc v4.7.3 doesn't complain, but right, %u is a proper way to handle it. >> + return -ETIMEDOUT; > I wander whether reversing is necessary on error path > (exynos_cluster_power_down()). In case of timeout, CPU will be left > with stuck with power supplied. In the same time the next power up try > might not work properly because CPU was not shutdown. Well, I can add it for the symmetry. The error path seems to be broken anyway, because failed suspend attempt leaves the non-boot CPUs in some meta state, from which they cannot be onlined. This needs further investigation, but with this patch at least we get an error message instead of the complete freeze. >> + } >> >> pmu_raw_writel(EXYNOS5420_KFC_CORE_RESET(cpu), >> EXYNOS_SWRESET); >> diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c >> index d5d48fbdab17..c01e2f7ba1ec 100644 >> --- a/arch/arm/mach-exynos/platsmp.c >> +++ b/arch/arm/mach-exynos/platsmp.c >> @@ -214,13 +214,20 @@ static inline void __iomem *cpu_boot_reg(int cpu) >> */ >> void exynos_core_restart(u32 core_id) >> { >> + unsigned int timeout = 16; >> u32 val; >> >> if (!soc_is_exynos3250()) >> return; >> >> - while (!pmu_raw_readl(S5P_PMU_SPARE2)) >> + while (timeout && !pmu_raw_readl(S5P_PMU_SPARE2)) { >> + timeout--; >> udelay(10); >> + } >> + if (timeout == 0) { >> + pr_err("cpu core %d restart failed\n", core_id); > %u - you should have a warning here. > Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland
diff --git a/arch/arm/mach-exynos/mcpm-exynos.c b/arch/arm/mach-exynos/mcpm-exynos.c index 72bc035bedbe..43d6f7755842 100644 --- a/arch/arm/mach-exynos/mcpm-exynos.c +++ b/arch/arm/mach-exynos/mcpm-exynos.c @@ -75,14 +75,23 @@ static int exynos_cpu_powerup(unsigned int cpu, unsigned int cluster) */ if (cluster && cluster == MPIDR_AFFINITY_LEVEL(cpu_logical_map(0), 1)) { + unsigned int timeout = 16; /* * Before we reset the Little cores, we should wait * the SPARE2 register is set to 1 because the init * codes of the iROM will set the register after * initialization. */ - while (!pmu_raw_readl(S5P_PMU_SPARE2)) + while (timeout && !pmu_raw_readl(S5P_PMU_SPARE2)) { + timeout--; udelay(10); + } + + if (timeout == 0) { + pr_err("cpu %d cluster %d powerup failed\n", + cpu, cluster); + return -ETIMEDOUT; + } pmu_raw_writel(EXYNOS5420_KFC_CORE_RESET(cpu), EXYNOS_SWRESET); diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c index d5d48fbdab17..c01e2f7ba1ec 100644 --- a/arch/arm/mach-exynos/platsmp.c +++ b/arch/arm/mach-exynos/platsmp.c @@ -214,13 +214,20 @@ static inline void __iomem *cpu_boot_reg(int cpu) */ void exynos_core_restart(u32 core_id) { + unsigned int timeout = 16; u32 val; if (!soc_is_exynos3250()) return; - while (!pmu_raw_readl(S5P_PMU_SPARE2)) + while (timeout && !pmu_raw_readl(S5P_PMU_SPARE2)) { + timeout--; udelay(10); + } + if (timeout == 0) { + pr_err("cpu core %d restart failed\n", core_id); + return; + } udelay(10); val = pmu_raw_readl(EXYNOS_ARM_CORE_STATUS(core_id));
Add timeout to infinite loops during the CPU powerup procedures. It is better to report an error instead of busylooping for infinite time in case of failure. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- arch/arm/mach-exynos/mcpm-exynos.c | 11 ++++++++++- arch/arm/mach-exynos/platsmp.c | 9 ++++++++- 2 files changed, 18 insertions(+), 2 deletions(-) -- 2.17.1