Abhilash Kesavan wrote:
>
> Hi Russell and Tomasz,
>
> +Arnd
> On Tue, Jun 24, 2014 at 9:41 PM, Russell King - ARM Linux
> <linux@xxxxxxxxxxxxxxxx> wrote:
> > On Mon, Jun 16, 2014 at 09:37:14AM +0530, Abhilash Kesavan wrote:
> >> Hi Kukjin,
> >>
> >> On Fri, May 23, 2014 at 8:31 AM, Abhilash Kesavan <a.kesavan@xxxxxxxxxxx>
> wrote:
> >> > Signed-off-by: Abhilash Kesavan <a.kesavan@xxxxxxxxxxx>
> >>
> >> Do you have any comments on this patch ?
> >
> > I do.
> >
> >> > diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
> >> > index d10c351..6dd4a11 100644
> >> > --- a/arch/arm/mach-exynos/pm.c
> >> > +++ b/arch/arm/mach-exynos/pm.c
> >> > @@ -300,7 +300,7 @@ static int exynos_pm_suspend(void)
> >> > tmp = (S5P_USE_STANDBY_WFI0 | S5P_USE_STANDBY_WFE0);
> >> > __raw_writel(tmp, S5P_CENTRAL_SEQ_OPTION);
> >> >
> >> > - if (!soc_is_exynos5250())
> >> > + if (read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A9)
> >> > exynos_cpu_save_register();
> > ...
> >> > @@ -334,7 +334,7 @@ static void exynos_pm_resume(void)
> >> > if (exynos_pm_central_resume())
> >> > goto early_wakeup;
> >> >
> >> > - if (!soc_is_exynos5250())
> >> > + if (read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A9)
> >> > exynos_cpu_restore_register();
> >
> > It is invalid to check just the part number. The part number on its
> > own is meaningless without taking account of the implementor. Both
> > the implementor and the part number should be checked at each of these
> > sites.
> Thanks for pointing this out. I was not aware of the implementor id
> check requirement.
> >
> > Another point: exynos have taken it upon themselves to add code which
> > saves various ARM core registers. This is a bad idea, it brings us
> > back to the days where every platform did their own suspend implementations.
> >
> > CPU level registers should be handled by CPU level code, not by platform
> > code. Is there a reason why this can't be added to the Cortex-A9
> > support code in proc-v7.S ?
> >
Got it. Thanks for pointing out.
> >> > @@ -353,7 +353,7 @@ static void exynos_pm_resume(void)
> >> >
> >> > s3c_pm_do_restore_core(exynos_core_save,
> ARRAY_SIZE(exynos_core_save));
> >> >
> >> > - if (!soc_is_exynos5250())
> >> > + if (read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A9)
> >> > scu_enable(S5P_VA_SCU);
> >> >
> >> > early_wakeup:
> >> > @@ -440,15 +440,18 @@ static int exynos_cpu_pm_notifier(struct
> notifier_block *self,
> >> > case CPU_PM_ENTER:
> >> > if (cpu == 0) {
> >> > exynos_pm_central_suspend();
> >> > - exynos_cpu_save_register();
> >> > + if (read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A9)
> >> > + exynos_cpu_save_register();
> >> > }
> >> > break;
> >> >
> >> > case CPU_PM_EXIT:
> >> > if (cpu == 0) {
> >> > - if (!soc_is_exynos5250())
> >> > + if (read_cpuid_part_number() ==
> >> > + ARM_CPU_PART_CORTEX_A9) {
> >> > scu_enable(S5P_VA_SCU);
> >> > - exynos_cpu_restore_register();
> >> > + exynos_cpu_restore_register();
> >> > + }
> >> > exynos_pm_central_resume();
> >> > }
> >> > break;
> >
> > And presumably with the CPU level code dealing with those registers,
> > you don't need the calls to save and restore these registers in this
> > notifier.
> Regarding save/restore of these registers, I could send out a patch
> cleaning these out once Shawn's patch gets merged. I'd need some help
> testing it out on Exynos4 boards though. For now, is it OK if I just
> update to the new function ?
> >
> > Which, by the way, is probably illegal to run as it runs in a read-
> > lock code path and with the SCU disabled. As you're calling
> > scu_enable() that means you're non-coherent with the other CPUs,
> > and therefore locks don't work.
> >
> > I think this code is very broken and wrongly architected, and shows
> > that we're continuing to make the same mistakes that we made all
> > through the 2000s with platforms doing their own crap rather than
> > properly thinking about this stuff.
> I see that you have sent a patch out that ensures both part and
> implementor number are checked. Currently, my patch has been applied
> to the fixes branch of the arm-soc tree and I wanted to know how to
> proceed (without it there is a crash on the 5420):
> Should I request Arnd to drop it (if possible) and send out a new
> patch using your updated function ?
>
Oops, Abhilash please send new fix on top of the current patch.
Thanks,
Kukjin
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
