| Message ID | 1929175.yd8jnLbgsd@amdc1032 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/14/2014 12:55 PM, Bartlomiej Zolnierkiewicz wrote: > > Hi, > > On Monday, July 21, 2014 01:06:32 PM Daniel Lezcano wrote: >> On 07/16/2014 07:34 PM, Bartlomiej Zolnierkiewicz wrote: >>> >>> Hi, >>> >>> On Friday, May 30, 2014 03:53:09 PM Bartlomiej Zolnierkiewicz wrote: >>>> >>>> Hi, >>>> >>>> On Friday, May 30, 2014 01:34:45 PM Tomasz Figa wrote: >>>>> Hi Daniel, >>>>> >>>>> On 30.05.2014 11:30, Daniel Lezcano wrote: >>>>>> On 04/24/2014 07:42 PM, Tomasz Figa wrote: >>>>>>> Hi Daniel, >>>>>>> >>>>>>> Please see my comments inline. >>>>>>> >>>>>>> Btw. Please fix your e-mail composer to properly wrap your messages >>>>>>> around 7xth column, as otherwise they're hard to read. >>>>>>> >>>>>>> On 04.04.2014 11:48, Daniel Lezcano wrote: >>>>>>>> The following driver is for exynos4210. I did not yet finished the >>>>>>>> other boards, so >>>>>>>> I created a specific driver for 4210 which could be merged later. >>>>>>>> >>>>>>>> The driver is based on Colin Cross's driver found at: >>>>>>>> >>>>>>>> https://android.googlesource.com/kernel/exynos/+/e686b1ec67423c40b4fdf811f9a4dfa3b393a010%5E%5E!/ >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> This one was based on a 3.4 kernel and an old API. >>>>>>>> >>>>>>>> It has been refreshed, simplified and based on the recent code cleanup >>>>>>>> I sent >>>>>>>> today. >>>>>>>> >>>>>>>> The AFTR could be entered when all the cpus (except cpu0) are down. In >>>>>>>> order to >>>>>>>> reach this situation, the couple idle states are used. >>>>>>>> >>>>>>>> There is a sync barrier at the entry and the exit of the low power >>>>>>>> function. So >>>>>>>> all cpus will enter and exit the function at the same time. >>>>>>>> >>>>>>>> At this point, CPU0 knows the other cpu will power down itself. CPU0 >>>>>>>> waits for >>>>>>>> the CPU1 to be powered down and then initiate the AFTR power down >>>>>>>> sequence. >>>>>>>> >>>>>>>> No interrupts are handled by CPU1, this is why we switch to the timer >>>>>>>> broadcast >>>>>>>> even if the local timer is not impacted by the idle state. >>>>>>>> >>>>>>>> When CPU0 wakes up, it powers up CPU1 and waits for it to boot. Then >>>>>>>> they both >>>>>>>> exit the idle function. >>>>>>>> >>>>>>>> This driver allows the exynos4210 to have the same power consumption >>>>>>>> at idle >>>>>>>> time than the one when we have to unplug CPU1 in order to let CPU0 to >>>>>>>> reach >>>>>>>> the AFTR state. >>>>>>>> >>>>>>>> This patch is a RFC because, we have to find a way to remove the macros >>>>>>>> definitions and cpu powerdown function without pulling the arch >>>>>>>> dependent >>>>>>>> headers. >>>>>>>> >>>>>>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>>>>>>> --- >>>>>>>> arch/arm/mach-exynos/common.c | 11 +- >>>>>>>> drivers/cpuidle/Kconfig.arm | 8 ++ >>>>>>>> drivers/cpuidle/Makefile | 1 + >>>>>>>> drivers/cpuidle/cpuidle-exynos4210.c | 226 >>>>>>>> ++++++++++++++++++++++++++++++++++ >>>>>> >>>>>> [ ... ] >>>>>> >>>>>> >>>>>>> Otherwise, I quite like the whole idea. I need to play a bit with CPU >>>>>>> hotplug and PMU to verify that things couldn't really be simplified a >>>>>>> bit, but in general this looks reasonably. >>>>>> >>>>>> Hi Tomasz, >>>>>> >>>>>> did you have time to look at this simplification ? >>>>> >>>>> Unfortunately I got preempted with other things to do and now I'm on >>>>> vacation. I worked a bit with Bart (added on CC) on this and generally >>>>> the conclusion was that all the things are necessary. He was also >>>>> working to extend the driver to support Exynos4x12. >>>>> >>>>> Bart, has anything interesting showed up since we talked about this last >>>>> time? >>>> >>>> Since we last looked into this I have fixed the "standard" AFTR support >>>> for Exynos3250 and started to work on adding Exynos3250 support to this >>>> driver (as Exynos3250 support has bigger priority than Exynos4x12 one). >>>> Unfortunately I also got preempted with other things so it is still >>>> unfinished and doesn't work yet. Moreover I had no possibility to test >>>> the new driver on Exynos4210 based Origen yet (I hope to do this next >>>> week). >>> >>> I have tested this patch on Origen board (Exynos4210 rev 1.1) and it >>> causes system lockup (it seems that the code gets stuck on waiting for >>> CPU1 to wake up). >>> >>> The kernels I have tried: >>> * current -next + this patch + few fixes to bring it up to date >>> * commit cd245f5 + this patch + some fixes >>> * next-20140403 + your Exynos cpuidle patch series + this patch >>> >>> I have also tried with S5P_VA_SYSRAM replaced by S5P_INFORM5 to >>> match Exynos 4210 rev 1.1 but it didn't help. >>> >>> U-Boot version used is: >>> U-Boot 2012.07 (Sep 22 2012 - 05:12:41) for ORIGEN >>> >>> Could you please tell me which hardware/bootloader/kernel exactly >>> have you used to test this patch? >> >> When I used it, it was on top of 3.15-rc1: >> >> https://git.linaro.org/people/daniel.lezcano/linux.git/shortlog/refs/heads/cpuidle/samsung-next >> >> The hardware was a exynos-4210 origen board ver A. > > I need the following patch to make this driver work on my hardware. Thanks for the patch ! > [ Unfortunately even with this patch the driver doesn't work reliably. > After 30-40 minutes of stress testing it lockups the system (I can > send you testing app+script if needed). ] Yes, please. I will try to reproduce it. > Best regards, > -- > Bartlomiej Zolnierkiewicz > Samsung R&D Institute Poland > Samsung Electronics > > > From: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > Subject: [PATCH] cpuidle: Exynos4210: make coupled driver work on Revision 1.1 > > * Add static inline helper cpu_boot_reg_base() and use it instead of > BOOT_VECTOR macro. > > * Use S5P_INFORM register instead of S5P_VA_SYSRAM one on Revison 1.1 > (this matches platform code in arch/arm/mach-exynos/platsmp.c). > > * Retry poking CPU1 out of the BOOT ROM if necessary > (this matches platform code in arch/arm/mach-exynos/platsmp.c). > > Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > Acked-by: Kyungmin Park <kyungmin.park@samsung.com> > --- > drivers/cpuidle/cpuidle-exynos4210.c | 27 ++++++++++++++------------- > 1 file changed, 14 insertions(+), 13 deletions(-) > > Index: b/drivers/cpuidle/cpuidle-exynos4210.c > =================================================================== > --- a/drivers/cpuidle/cpuidle-exynos4210.c 2014-07-22 15:42:11.580365939 +0200 > +++ b/drivers/cpuidle/cpuidle-exynos4210.c 2014-07-22 16:11:01.112369130 +0200 > @@ -29,12 +29,19 @@ > static atomic_t exynos_idle_barrier; > static atomic_t cpu1_wakeup = ATOMIC_INIT(0); > > -#define BOOT_VECTOR S5P_VA_SYSRAM > #define S5P_VA_PMU S3C_ADDR(0x02180000) > #define S5P_PMUREG(x) (S5P_VA_PMU + (x)) > +#define S5P_INFORM5 S5P_PMUREG(0x0814) > #define S5P_ARM_CORE1_CONFIGURATION S5P_PMUREG(0x2080) > #define S5P_ARM_CORE1_STATUS S5P_PMUREG(0x2084) > > +static inline void __iomem *cpu_boot_reg_base(void) > +{ > + if (samsung_rev() == EXYNOS4210_REV_1_1) > + return S5P_INFORM5; > + return S5P_VA_SYSRAM; > +} > + > static void (*exynos_aftr)(void); > extern void exynos_cpu_resume(void); > static int cpu_suspend_finish(unsigned long flags) > @@ -76,7 +83,7 @@ static int exynos_cpu0_enter_aftr(void) > * boot back up again, getting stuck in the > * boot rom code > */ > - if (__raw_readl(BOOT_VECTOR) == 0) > + if (__raw_readl(cpu_boot_reg_base()) == 0) > goto abort; > > cpu_relax(); > @@ -95,7 +102,7 @@ abort: > * Set the boot vector to something non-zero > */ > __raw_writel(virt_to_phys(exynos_cpu_resume), > - BOOT_VECTOR); > + cpu_boot_reg_base()); > dsb(); > > /* > @@ -108,24 +115,18 @@ abort: > /* > * Wait for cpu1 to get stuck in the boot rom > */ > - while ((__raw_readl(BOOT_VECTOR) != 0) && > + while ((__raw_readl(cpu_boot_reg_base()) != 0) && > !atomic_read(&cpu1_wakeup)) > cpu_relax(); > > - if (!atomic_read(&cpu1_wakeup)) { > + while (!atomic_read(&cpu1_wakeup)) { > /* > * Poke cpu1 out of the boot rom > */ > __raw_writel(virt_to_phys(exynos_cpu_resume), > - BOOT_VECTOR); > + cpu_boot_reg_base()); > dsb_sev(); > } > - > - /* > - * Wait for cpu1 to finish booting > - */ > - while (!atomic_read(&cpu1_wakeup)) > - cpu_relax(); > } > > return ret; > @@ -165,7 +166,7 @@ static int exynos_enter_aftr(struct cpui > { > int ret; > > - __raw_writel(virt_to_phys(exynos_cpu_resume), BOOT_VECTOR); > + __raw_writel(virt_to_phys(exynos_cpu_resume), cpu_boot_reg_base()); > > /* > * Waiting all cpus to reach this point at the same moment > > >
Index: b/drivers/cpuidle/cpuidle-exynos4210.c =================================================================== --- a/drivers/cpuidle/cpuidle-exynos4210.c 2014-07-22 15:42:11.580365939 +0200 +++ b/drivers/cpuidle/cpuidle-exynos4210.c 2014-07-22 16:11:01.112369130 +0200 @@ -29,12 +29,19 @@ static atomic_t exynos_idle_barrier; static atomic_t cpu1_wakeup = ATOMIC_INIT(0); -#define BOOT_VECTOR S5P_VA_SYSRAM #define S5P_VA_PMU S3C_ADDR(0x02180000) #define S5P_PMUREG(x) (S5P_VA_PMU + (x)) +#define S5P_INFORM5 S5P_PMUREG(0x0814) #define S5P_ARM_CORE1_CONFIGURATION S5P_PMUREG(0x2080) #define S5P_ARM_CORE1_STATUS S5P_PMUREG(0x2084) +static inline void __iomem *cpu_boot_reg_base(void) +{ + if (samsung_rev() == EXYNOS4210_REV_1_1) + return S5P_INFORM5; + return S5P_VA_SYSRAM; +} + static void (*exynos_aftr)(void); extern void exynos_cpu_resume(void); static int cpu_suspend_finish(unsigned long flags) @@ -76,7 +83,7 @@ static int exynos_cpu0_enter_aftr(void) * boot back up again, getting stuck in the * boot rom code */ - if (__raw_readl(BOOT_VECTOR) == 0) + if (__raw_readl(cpu_boot_reg_base()) == 0) goto abort; cpu_relax(); @@ -95,7 +102,7 @@ abort: * Set the boot vector to something non-zero */ __raw_writel(virt_to_phys(exynos_cpu_resume), - BOOT_VECTOR); + cpu_boot_reg_base()); dsb(); /* @@ -108,24 +115,18 @@ abort: /* * Wait for cpu1 to get stuck in the boot rom */ - while ((__raw_readl(BOOT_VECTOR) != 0) && + while ((__raw_readl(cpu_boot_reg_base()) != 0) && !atomic_read(&cpu1_wakeup)) cpu_relax(); - if (!atomic_read(&cpu1_wakeup)) { + while (!atomic_read(&cpu1_wakeup)) { /* * Poke cpu1 out of the boot rom */ __raw_writel(virt_to_phys(exynos_cpu_resume), - BOOT_VECTOR); + cpu_boot_reg_base()); dsb_sev(); } - - /* - * Wait for cpu1 to finish booting - */ - while (!atomic_read(&cpu1_wakeup)) - cpu_relax(); } return ret; @@ -165,7 +166,7 @@ static int exynos_enter_aftr(struct cpui { int ret; - __raw_writel(virt_to_phys(exynos_cpu_resume), BOOT_VECTOR); + __raw_writel(virt_to_phys(exynos_cpu_resume), cpu_boot_reg_base()); /* * Waiting all cpus to reach this point at the same moment