| Message ID | 3145165.ZB7l3vNJ7a@amdc1032 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Mon, Aug 26, 2013 at 3:36 PM, Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> wrote: > > Hi, > > On Monday, August 26, 2013 09:14:42 AM Kukjin Kim wrote: >> The following changes since commit ad81f0545ef01ea651886dddac4bef6cec930092: >> >> Linux 3.11-rc1 (2013-07-14 15:18:27 -0700) >> >> are available in the git repository at: >> >> git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git >> tags/samsung-mach-exynos >> >> for you to fetch changes up to f52616f4233d71d0fb00f06f86d046d18d2b7f3b: >> >> ARM: EXYNOS: always enable PM domains support for EXYNOS4X12 (2013-08-19 >> 05:05:16 +0900) >> >> ---------------------------------------------------------------- >> update mach-exynos >> - enable ARCH_HAS_BANDGAP for exynos SoCs >> - skip C1 cpuidle for exynos5440 because non-supporting >> - always enable PM domains for exynos4x12 >> >> ---------------------------------------------------------------- >> Amit Daniel Kachhap (2): >> ARM: EXYNOS: enable ARCH_HAS_BANDGAP >> ARM: EXYNOS: Skip C1 cpuidle state for exynos5440 > > The patch "ARM: EXYNOS: Skip C1 cpuidle state for exynos5440": > > --- a/arch/arm/mach-exynos/cpuidle.c > +++ b/arch/arm/mach-exynos/cpuidle.c > @@ -210,7 +210,7 @@ static int __init exynos4_init_cpuidle(void) > device->cpu = cpu_id; > > /* Support IDLE only */ > - if (cpu_id != 0) > + if (soc_is_exynos5440() || cpu_id != 0) > device->state_count = 1; > > ret = cpuidle_register_device(device); > > is incorrect as noted a month ago in: > > http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/186355.html > > [ Because of the deficiency in the core cpuidle core (device->state_count > not being used by governors' code) only sysfs entries for C1 state will be > disabled and EXYNOS cpuidle driver will still attempt to use C1 state. > > also non-working device->state_count is planned to be removed by: > > http://permalink.gmane.org/gmane.linux.power-management.general/37390 I looked at your patch series and it seems reasonable. I will repost this patch on top of yours. But I suggest to keep this patch temporary till your patch series gets merged. Thanks, Amit Daniel > > > To disable C1 state on EXYNOS5440 something like: > > static int __init exynos4_init_cpuidle(void) > { > ... > if (soc_is_exynos5440()) > exynos4_idle_driver.state_count = 1; > ... > } > > should be done instead. > > Best regards, > -- > Bartlomiej Zolnierkiewicz > Samsung R&D Institute Poland > Samsung Electronics > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel On Mon, Aug 26, 2013 at 3:36 PM, Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> wrote: > > Hi, > > On Monday, August 26, 2013 09:14:42 AM Kukjin Kim wrote: >> The following changes since commit ad81f0545ef01ea651886dddac4bef6cec930092: >> >> Linux 3.11-rc1 (2013-07-14 15:18:27 -0700) >> >> are available in the git repository at: >> >> git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git >> tags/samsung-mach-exynos >> >> for you to fetch changes up to f52616f4233d71d0fb00f06f86d046d18d2b7f3b: >> >> ARM: EXYNOS: always enable PM domains support for EXYNOS4X12 (2013-08-19 >> 05:05:16 +0900) >> >> ---------------------------------------------------------------- >> update mach-exynos >> - enable ARCH_HAS_BANDGAP for exynos SoCs >> - skip C1 cpuidle for exynos5440 because non-supporting >> - always enable PM domains for exynos4x12 >> >> ---------------------------------------------------------------- >> Amit Daniel Kachhap (2): >> ARM: EXYNOS: enable ARCH_HAS_BANDGAP >> ARM: EXYNOS: Skip C1 cpuidle state for exynos5440 > > The patch "ARM: EXYNOS: Skip C1 cpuidle state for exynos5440": > > --- a/arch/arm/mach-exynos/cpuidle.c > +++ b/arch/arm/mach-exynos/cpuidle.c > @@ -210,7 +210,7 @@ static int __init exynos4_init_cpuidle(void) > device->cpu = cpu_id; > > /* Support IDLE only */ > - if (cpu_id != 0) > + if (soc_is_exynos5440() || cpu_id != 0) > device->state_count = 1; > > ret = cpuidle_register_device(device); > > is incorrect as noted a month ago in: > > http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/186355.html > > [ Because of the deficiency in the core cpuidle core (device->state_count > not being used by governors' code) only sysfs entries for C1 state will be > disabled and EXYNOS cpuidle driver will still attempt to use C1 state. > > also non-working device->state_count is planned to be removed by: > > http://permalink.gmane.org/gmane.linux.power-management.general/37390 > > > To disable C1 state on EXYNOS5440 something like: > > static int __init exynos4_init_cpuidle(void) > { > ... > if (soc_is_exynos5440()) > exynos4_idle_driver.state_count = 1; > ... > } > > should be done instead. > > Best regards, > -- > Bartlomiej Zolnierkiewicz > Samsung R&D Institute Poland > Samsung Electronics > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, August 26, 2013 04:33:55 PM amit daniel kachhap wrote: > Hi, > > On Mon, Aug 26, 2013 at 3:36 PM, Bartlomiej Zolnierkiewicz > <b.zolnierkie@samsung.com> wrote: > > > > Hi, > > > > On Monday, August 26, 2013 09:14:42 AM Kukjin Kim wrote: > >> The following changes since commit ad81f0545ef01ea651886dddac4bef6cec930092: > >> > >> Linux 3.11-rc1 (2013-07-14 15:18:27 -0700) > >> > >> are available in the git repository at: > >> > >> git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git > >> tags/samsung-mach-exynos > >> > >> for you to fetch changes up to f52616f4233d71d0fb00f06f86d046d18d2b7f3b: > >> > >> ARM: EXYNOS: always enable PM domains support for EXYNOS4X12 (2013-08-19 > >> 05:05:16 +0900) > >> > >> ---------------------------------------------------------------- > >> update mach-exynos > >> - enable ARCH_HAS_BANDGAP for exynos SoCs > >> - skip C1 cpuidle for exynos5440 because non-supporting > >> - always enable PM domains for exynos4x12 > >> > >> ---------------------------------------------------------------- > >> Amit Daniel Kachhap (2): > >> ARM: EXYNOS: enable ARCH_HAS_BANDGAP > >> ARM: EXYNOS: Skip C1 cpuidle state for exynos5440 > > > > The patch "ARM: EXYNOS: Skip C1 cpuidle state for exynos5440": > > > > --- a/arch/arm/mach-exynos/cpuidle.c > > +++ b/arch/arm/mach-exynos/cpuidle.c > > @@ -210,7 +210,7 @@ static int __init exynos4_init_cpuidle(void) > > device->cpu = cpu_id; > > > > /* Support IDLE only */ > > - if (cpu_id != 0) > > + if (soc_is_exynos5440() || cpu_id != 0) > > device->state_count = 1; > > > > ret = cpuidle_register_device(device); > > > > is incorrect as noted a month ago in: > > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/186355.html > > > > [ Because of the deficiency in the core cpuidle core (device->state_count > > not being used by governors' code) only sysfs entries for C1 state will be > > disabled and EXYNOS cpuidle driver will still attempt to use C1 state. > > > > also non-working device->state_count is planned to be removed by: > > > > http://permalink.gmane.org/gmane.linux.power-management.general/37390 > I looked at your patch series and it seems reasonable. I will repost > this patch on top of yours. If you correctly use driver's state_count (instead of device's) there will be no dependency on my patch series and the new patch can be applied immediately. > But I suggest to keep this patch temporary till your patch series gets merged. The current patch (the one Kukjin merged) is incorrect as it just doesn't do what it advertises. I see no reason to keep it. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics > Thanks, > Amit Daniel > > > > > > To disable C1 state on EXYNOS5440 something like: > > > > static int __init exynos4_init_cpuidle(void) > > { > > ... > > if (soc_is_exynos5440()) > > exynos4_idle_driver.state_count = 1; > > ... > > } > > > > should be done instead. > > > > Best regards, > > -- > > Bartlomiej Zolnierkiewicz > > Samsung R&D Institute Poland > > Samsung Electronics > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > On Mon, Aug 26, 2013 at 3:36 PM, Bartlomiej Zolnierkiewicz > <b.zolnierkie@samsung.com> wrote: > > > > Hi, > > > > On Monday, August 26, 2013 09:14:42 AM Kukjin Kim wrote: > >> The following changes since commit ad81f0545ef01ea651886dddac4bef6cec930092: > >> > >> Linux 3.11-rc1 (2013-07-14 15:18:27 -0700) > >> > >> are available in the git repository at: > >> > >> git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git > >> tags/samsung-mach-exynos > >> > >> for you to fetch changes up to f52616f4233d71d0fb00f06f86d046d18d2b7f3b: > >> > >> ARM: EXYNOS: always enable PM domains support for EXYNOS4X12 (2013-08-19 > >> 05:05:16 +0900) > >> > >> ---------------------------------------------------------------- > >> update mach-exynos > >> - enable ARCH_HAS_BANDGAP for exynos SoCs > >> - skip C1 cpuidle for exynos5440 because non-supporting > >> - always enable PM domains for exynos4x12 > >> > >> ---------------------------------------------------------------- > >> Amit Daniel Kachhap (2): > >> ARM: EXYNOS: enable ARCH_HAS_BANDGAP > >> ARM: EXYNOS: Skip C1 cpuidle state for exynos5440 > > > > The patch "ARM: EXYNOS: Skip C1 cpuidle state for exynos5440": > > > > --- a/arch/arm/mach-exynos/cpuidle.c > > +++ b/arch/arm/mach-exynos/cpuidle.c > > @@ -210,7 +210,7 @@ static int __init exynos4_init_cpuidle(void) > > device->cpu = cpu_id; > > > > /* Support IDLE only */ > > - if (cpu_id != 0) > > + if (soc_is_exynos5440() || cpu_id != 0) > > device->state_count = 1; > > > > ret = cpuidle_register_device(device); > > > > is incorrect as noted a month ago in: > > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/186355.html > > > > [ Because of the deficiency in the core cpuidle core (device->state_count > > not being used by governors' code) only sysfs entries for C1 state will be > > disabled and EXYNOS cpuidle driver will still attempt to use C1 state. > > > > also non-working device->state_count is planned to be removed by: > > > > http://permalink.gmane.org/gmane.linux.power-management.general/37390 > > > > > > To disable C1 state on EXYNOS5440 something like: > > > > static int __init exynos4_init_cpuidle(void) > > { > > ... > > if (soc_is_exynos5440()) > > exynos4_idle_driver.state_count = 1; > > ... > > } > > > > should be done instead. > > > > Best regards, > > -- > > Bartlomiej Zolnierkiewicz > > Samsung R&D Institute Poland > > Samsung Electronics > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Bartlomiej Zolnierkiewicz wrote: [...] > > > is incorrect as noted a month ago in: > > > > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2013- > July/186355.html > > > > > > [ Because of the deficiency in the core cpuidle core (device- > >state_count > > > not being used by governors' code) only sysfs entries for C1 state > will be > > > disabled and EXYNOS cpuidle driver will still attempt to use C1 > state. > > > > > > also non-working device->state_count is planned to be removed by: > > > > > > http://permalink.gmane.org/gmane.linux.power-management.general/37390 > > I looked at your patch series and it seems reasonable. I will repost > > this patch on top of yours. > > If you correctly use driver's state_count (instead of device's) there will > be > no dependency on my patch series and the new patch can be applied > immediately. > > > But I suggest to keep this patch temporary till your patch series gets > merged. > > The current patch (the one Kukjin merged) is incorrect as it just doesn't > do what it advertises. I see no reason to keep it. > Well, I don't think so, because if the patch is missing, following kernel panic happens on exynos5440 platform. Unable to handle kernel paging request at virtual address f8180608 pgd = c0003000 [f8180608] *pgd=80000080007003, *pmd=af7fb003, *pte=00000000 Internal error: Oops: a07 [#1] PREEMPT ARM Modules linked in: CPU: 0 PID: 0 Comm: swapper Not tainted 3.11.0-0-generic #1~d20130819t044008~3372c79 task: c0529d80 ti: c051e000 task.ti: c051e000 PC is at exynos4_enter_lowpower+0x18/0x130 LR is at cpuidle_enter_state+0x3c/0xe8 pc : [] lr : [] psr: 200f0093 sp : c051ff68 ip : 00000018 fp : 00000000 r10: 00000000 r9 : 412fc0f3 r8 : 00000000 r7 : c052c9cc r6 : 00000001 r5 : 00000000 r4 : d5c3cc94 r3 : f8180000 r2 : 0000ff3e r1 : c052c980 r0 : c052cc98 Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment kernel Control: 30c5387d Table: af139b00 DAC: fffffffd Process swapper (pid: 0, stack limit = 0xc051e230) Stack: (0xc051ff68 to 0xc0520000) ff60: d5c3cc94 00000000 c052cc98 c02c1310 d5c3cc94 00000000 ff80: c0550288 c052c980 c058898c c052cc98 00000001 c052c980 c058898c c02c1458 ffa0: c051e000 c0550288 c0550288 00000001 c05260dc c001b2cc 000001fe c00573e4 ffc0: ffffffff c04f07d8 ffffffff ffffffff c04f0290 00000000 00000000 c05134d8 ffe0: 30c7387d c0526064 c05134d4 c052b5b4 80007000 80008080 00000000 00000000 [] (exynos4_enter_lowpower+0x18/0x130) from [] (cpuidle_enter_state+0x3c/0xe8) [] (cpuidle_enter_state+0x3c/0xe8) from [] (cpuidle_idle_call+0x9c/0x140) [] (cpuidle_idle_call+0x9c/0x140) from [] (arch_cpu_idle+0x8/0x38) [] (arch_cpu_idle+0x8/0x38) from [] (cpu_startup_entry+0x4c/0x114) [] (cpu_startup_entry+0x4c/0x114) from [] (start_kernel+0x324/0x37c) Code: 0a000043 e3a03000 e30f2f3e e34f3818 (e5832608) ---[ end trace 617b9e1a4ff91d2f ]--- Kernel panic - not syncing: Attempted to kill the idle task! In addition, I didn't see your patches in linux-next for upcoming merge window. So I think, if any fixup is required, it should be done on top of current shape. - Kukjin -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Submitted the V2 version of this patch with your suggestion. Thanks Amit On Mon, Aug 26, 2013 at 4:49 PM, Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> wrote: > On Monday, August 26, 2013 04:33:55 PM amit daniel kachhap wrote: >> Hi, >> >> On Mon, Aug 26, 2013 at 3:36 PM, Bartlomiej Zolnierkiewicz >> <b.zolnierkie@samsung.com> wrote: >> > >> > Hi, >> > >> > On Monday, August 26, 2013 09:14:42 AM Kukjin Kim wrote: >> >> The following changes since commit ad81f0545ef01ea651886dddac4bef6cec930092: >> >> >> >> Linux 3.11-rc1 (2013-07-14 15:18:27 -0700) >> >> >> >> are available in the git repository at: >> >> >> >> git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git >> >> tags/samsung-mach-exynos >> >> >> >> for you to fetch changes up to f52616f4233d71d0fb00f06f86d046d18d2b7f3b: >> >> >> >> ARM: EXYNOS: always enable PM domains support for EXYNOS4X12 (2013-08-19 >> >> 05:05:16 +0900) >> >> >> >> ---------------------------------------------------------------- >> >> update mach-exynos >> >> - enable ARCH_HAS_BANDGAP for exynos SoCs >> >> - skip C1 cpuidle for exynos5440 because non-supporting >> >> - always enable PM domains for exynos4x12 >> >> >> >> ---------------------------------------------------------------- >> >> Amit Daniel Kachhap (2): >> >> ARM: EXYNOS: enable ARCH_HAS_BANDGAP >> >> ARM: EXYNOS: Skip C1 cpuidle state for exynos5440 >> > >> > The patch "ARM: EXYNOS: Skip C1 cpuidle state for exynos5440": >> > >> > --- a/arch/arm/mach-exynos/cpuidle.c >> > +++ b/arch/arm/mach-exynos/cpuidle.c >> > @@ -210,7 +210,7 @@ static int __init exynos4_init_cpuidle(void) >> > device->cpu = cpu_id; >> > >> > /* Support IDLE only */ >> > - if (cpu_id != 0) >> > + if (soc_is_exynos5440() || cpu_id != 0) >> > device->state_count = 1; >> > >> > ret = cpuidle_register_device(device); >> > >> > is incorrect as noted a month ago in: >> > >> > http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/186355.html >> > >> > [ Because of the deficiency in the core cpuidle core (device->state_count >> > not being used by governors' code) only sysfs entries for C1 state will be >> > disabled and EXYNOS cpuidle driver will still attempt to use C1 state. >> > >> > also non-working device->state_count is planned to be removed by: >> > >> > http://permalink.gmane.org/gmane.linux.power-management.general/37390 >> I looked at your patch series and it seems reasonable. I will repost >> this patch on top of yours. > > If you correctly use driver's state_count (instead of device's) there will be > no dependency on my patch series and the new patch can be applied immediately. > >> But I suggest to keep this patch temporary till your patch series gets merged. > > The current patch (the one Kukjin merged) is incorrect as it just doesn't > do what it advertises. I see no reason to keep it. > > Best regards, > -- > Bartlomiej Zolnierkiewicz > Samsung R&D Institute Poland > Samsung Electronics > >> Thanks, >> Amit Daniel >> > >> > >> > To disable C1 state on EXYNOS5440 something like: >> > >> > static int __init exynos4_init_cpuidle(void) >> > { >> > ... >> > if (soc_is_exynos5440()) >> > exynos4_idle_driver.state_count = 1; >> > ... >> > } >> > >> > should be done instead. >> > >> > Best regards, >> > -- >> > Bartlomiej Zolnierkiewicz >> > Samsung R&D Institute Poland >> > Samsung Electronics >> > >> > >> > _______________________________________________ >> > linux-arm-kernel mailing list >> > linux-arm-kernel@lists.infradead.org >> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >> >> >> On Mon, Aug 26, 2013 at 3:36 PM, Bartlomiej Zolnierkiewicz >> <b.zolnierkie@samsung.com> wrote: >> > >> > Hi, >> > >> > On Monday, August 26, 2013 09:14:42 AM Kukjin Kim wrote: >> >> The following changes since commit ad81f0545ef01ea651886dddac4bef6cec930092: >> >> >> >> Linux 3.11-rc1 (2013-07-14 15:18:27 -0700) >> >> >> >> are available in the git repository at: >> >> >> >> git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git >> >> tags/samsung-mach-exynos >> >> >> >> for you to fetch changes up to f52616f4233d71d0fb00f06f86d046d18d2b7f3b: >> >> >> >> ARM: EXYNOS: always enable PM domains support for EXYNOS4X12 (2013-08-19 >> >> 05:05:16 +0900) >> >> >> >> ---------------------------------------------------------------- >> >> update mach-exynos >> >> - enable ARCH_HAS_BANDGAP for exynos SoCs >> >> - skip C1 cpuidle for exynos5440 because non-supporting >> >> - always enable PM domains for exynos4x12 >> >> >> >> ---------------------------------------------------------------- >> >> Amit Daniel Kachhap (2): >> >> ARM: EXYNOS: enable ARCH_HAS_BANDGAP >> >> ARM: EXYNOS: Skip C1 cpuidle state for exynos5440 >> > >> > The patch "ARM: EXYNOS: Skip C1 cpuidle state for exynos5440": >> > >> > --- a/arch/arm/mach-exynos/cpuidle.c >> > +++ b/arch/arm/mach-exynos/cpuidle.c >> > @@ -210,7 +210,7 @@ static int __init exynos4_init_cpuidle(void) >> > device->cpu = cpu_id; >> > >> > /* Support IDLE only */ >> > - if (cpu_id != 0) >> > + if (soc_is_exynos5440() || cpu_id != 0) >> > device->state_count = 1; >> > >> > ret = cpuidle_register_device(device); >> > >> > is incorrect as noted a month ago in: >> > >> > http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/186355.html >> > >> > [ Because of the deficiency in the core cpuidle core (device->state_count >> > not being used by governors' code) only sysfs entries for C1 state will be >> > disabled and EXYNOS cpuidle driver will still attempt to use C1 state. >> > >> > also non-working device->state_count is planned to be removed by: >> > >> > http://permalink.gmane.org/gmane.linux.power-management.general/37390 >> > >> > >> > To disable C1 state on EXYNOS5440 something like: >> > >> > static int __init exynos4_init_cpuidle(void) >> > { >> > ... >> > if (soc_is_exynos5440()) >> > exynos4_idle_driver.state_count = 1; >> > ... >> > } >> > >> > should be done instead. >> > >> > Best regards, >> > -- >> > Bartlomiej Zolnierkiewicz >> > Samsung R&D Institute Poland >> > Samsung Electronics >> > >> > >> > _______________________________________________ >> > linux-arm-kernel mailing list >> > linux-arm-kernel@lists.infradead.org >> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, August 26, 2013 08:52:44 PM Kukjin Kim wrote: > Bartlomiej Zolnierkiewicz wrote: > > [...] > > > > > is incorrect as noted a month ago in: > > > > > > > > http://lists.infradead.org/pipermail/linux-arm-kernel/2013- > > July/186355.html > > > > > > > > [ Because of the deficiency in the core cpuidle core (device- > > >state_count > > > > not being used by governors' code) only sysfs entries for C1 state > > will be > > > > disabled and EXYNOS cpuidle driver will still attempt to use C1 > > state. > > > > > > > > also non-working device->state_count is planned to be removed by: > > > > > > > > http://permalink.gmane.org/gmane.linux.power-management.general/37390 > > > I looked at your patch series and it seems reasonable. I will repost > > > this patch on top of yours. > > > > If you correctly use driver's state_count (instead of device's) there will > > be > > no dependency on my patch series and the new patch can be applied > > immediately. > > > > > But I suggest to keep this patch temporary till your patch series gets > > merged. > > > > The current patch (the one Kukjin merged) is incorrect as it just doesn't > > do what it advertises. I see no reason to keep it. > > > Well, I don't think so, because if the patch is missing, following kernel > panic happens on exynos5440 platform. > > Unable to handle kernel paging request at virtual address f8180608 > pgd = c0003000 > [f8180608] *pgd=80000080007003, *pmd=af7fb003, *pte=00000000 > Internal error: Oops: a07 [#1] PREEMPT ARM > Modules linked in: > CPU: 0 PID: 0 Comm: swapper Not tainted 3.11.0-0-generic > #1~d20130819t044008~3372c79 > task: c0529d80 ti: c051e000 task.ti: c051e000 > PC is at exynos4_enter_lowpower+0x18/0x130 > LR is at cpuidle_enter_state+0x3c/0xe8 > pc : [] lr : [] psr: 200f0093 > sp : c051ff68 ip : 00000018 fp : 00000000 > r10: 00000000 r9 : 412fc0f3 r8 : 00000000 > r7 : c052c9cc r6 : 00000001 r5 : 00000000 r4 : d5c3cc94 > r3 : f8180000 r2 : 0000ff3e r1 : c052c980 r0 : c052cc98 > Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment kernel > Control: 30c5387d Table: af139b00 DAC: fffffffd > Process swapper (pid: 0, stack limit = 0xc051e230) > Stack: (0xc051ff68 to 0xc0520000) > ff60: d5c3cc94 00000000 c052cc98 c02c1310 d5c3cc94 > 00000000 > ff80: c0550288 c052c980 c058898c c052cc98 00000001 c052c980 c058898c > c02c1458 > ffa0: c051e000 c0550288 c0550288 00000001 c05260dc c001b2cc 000001fe > c00573e4 > ffc0: ffffffff c04f07d8 ffffffff ffffffff c04f0290 00000000 00000000 > c05134d8 > ffe0: 30c7387d c0526064 c05134d4 c052b5b4 80007000 80008080 00000000 > 00000000 > [] (exynos4_enter_lowpower+0x18/0x130) from [] > (cpuidle_enter_state+0x3c/0xe8) > [] (cpuidle_enter_state+0x3c/0xe8) from [] (cpuidle_idle_call+0x9c/0x140) > [] (cpuidle_idle_call+0x9c/0x140) from [] (arch_cpu_idle+0x8/0x38) > [] (arch_cpu_idle+0x8/0x38) from [] (cpu_startup_entry+0x4c/0x114) > [] (cpu_startup_entry+0x4c/0x114) from [] (start_kernel+0x324/0x37c) > Code: 0a000043 e3a03000 e30f2f3e e34f3818 (e5832608) > ---[ end trace 617b9e1a4ff91d2f ]--- > Kernel panic - not syncing: Attempted to kill the idle task! 1) samsung-mach-exynos kernel (from your pull request) doesn't have neither exynos5440_defconfig nor EXYNOS5440 enabled in exynos_defconfig so could you please tell me what kernel version and kernel config are you using to get the above panic? Could you also see what does exynos4_enter_lowpower+0x18 map to in your source code? 2) dev->state_count is used only for managing sysfs entries (and clearing dev->states_usage) in the cpuidle core. $ git grep state_count drivers/cpuidle/ drivers/cpuidle/cpuidle-calxeda.c: .state_count = 2, drivers/cpuidle/cpuidle-kirkwood.c: .state_count = KIRKWOOD_MAX_STATES, drivers/cpuidle/cpuidle-zynq.c: .state_count = ZYNQ_MAX_STATES, drivers/cpuidle/cpuidle.c: for (i = drv->state_count - 1; i >= CPUIDLE_DRIVER_STATE_START; i--) drivers/cpuidle/cpuidle.c: if (!dev->state_count) drivers/cpuidle/cpuidle.c: dev->state_count = drv->state_count; drivers/cpuidle/cpuidle.c: for (i = 0; i < dev->state_count; i++) { drivers/cpuidle/driver.c: for (i = drv->state_count - 1; i >= 0 ; i--) { drivers/cpuidle/driver.c: if (!drv || !drv->state_count) drivers/cpuidle/governors/ladder.c: if (last_idx < drv->state_count - 1 && drivers/cpuidle/governors/ladder.c: for (i = 0; i < drv->state_count; i++) { drivers/cpuidle/governors/ladder.c: if (i < drv->state_count - 1) drivers/cpuidle/governors/menu.c: for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) { drivers/cpuidle/sysfs.c: for (i = 0; i < device->state_count; i++) { drivers/cpuidle/sysfs.c: for (i = 0; i < device->state_count; i++) With the current code I fail to see how it is possible that dev->state_count smaller than drv->state_count affects anything besides sysfs cpuidle entries in the practice. IOW I worry that the current patch may be just masking some other issue. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
amit daniel kachhap <amit.daniel@samsung.com> writes: > Submitted the V2 version of this patch with your suggestion. So will there be an updated branch (and pull request) with these changes? Kevin > On Mon, Aug 26, 2013 at 4:49 PM, Bartlomiej Zolnierkiewicz > <b.zolnierkie@samsung.com> wrote: >> On Monday, August 26, 2013 04:33:55 PM amit daniel kachhap wrote: >>> Hi, >>> >>> On Mon, Aug 26, 2013 at 3:36 PM, Bartlomiej Zolnierkiewicz >>> <b.zolnierkie@samsung.com> wrote: >>> > >>> > Hi, >>> > >>> > On Monday, August 26, 2013 09:14:42 AM Kukjin Kim wrote: >>> >> The following changes since commit ad81f0545ef01ea651886dddac4bef6cec930092: >>> >> >>> >> Linux 3.11-rc1 (2013-07-14 15:18:27 -0700) >>> >> >>> >> are available in the git repository at: >>> >> >>> >> git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git >>> >> tags/samsung-mach-exynos >>> >> >>> >> for you to fetch changes up to f52616f4233d71d0fb00f06f86d046d18d2b7f3b: >>> >> >>> >> ARM: EXYNOS: always enable PM domains support for EXYNOS4X12 (2013-08-19 >>> >> 05:05:16 +0900) >>> >> >>> >> ---------------------------------------------------------------- >>> >> update mach-exynos >>> >> - enable ARCH_HAS_BANDGAP for exynos SoCs >>> >> - skip C1 cpuidle for exynos5440 because non-supporting >>> >> - always enable PM domains for exynos4x12 >>> >> >>> >> ---------------------------------------------------------------- >>> >> Amit Daniel Kachhap (2): >>> >> ARM: EXYNOS: enable ARCH_HAS_BANDGAP >>> >> ARM: EXYNOS: Skip C1 cpuidle state for exynos5440 >>> > >>> > The patch "ARM: EXYNOS: Skip C1 cpuidle state for exynos5440": >>> > >>> > --- a/arch/arm/mach-exynos/cpuidle.c >>> > +++ b/arch/arm/mach-exynos/cpuidle.c >>> > @@ -210,7 +210,7 @@ static int __init exynos4_init_cpuidle(void) >>> > device->cpu = cpu_id; >>> > >>> > /* Support IDLE only */ >>> > - if (cpu_id != 0) >>> > + if (soc_is_exynos5440() || cpu_id != 0) >>> > device->state_count = 1; >>> > >>> > ret = cpuidle_register_device(device); >>> > >>> > is incorrect as noted a month ago in: >>> > >>> > http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/186355.html >>> > >>> > [ Because of the deficiency in the core cpuidle core (device->state_count >>> > not being used by governors' code) only sysfs entries for C1 state will be >>> > disabled and EXYNOS cpuidle driver will still attempt to use C1 state. >>> > >>> > also non-working device->state_count is planned to be removed by: >>> > >>> > http://permalink.gmane.org/gmane.linux.power-management.general/37390 >>> I looked at your patch series and it seems reasonable. I will repost >>> this patch on top of yours. >> >> If you correctly use driver's state_count (instead of device's) there will be >> no dependency on my patch series and the new patch can be applied immediately. >> >>> But I suggest to keep this patch temporary till your patch series gets merged. >> >> The current patch (the one Kukjin merged) is incorrect as it just doesn't >> do what it advertises. I see no reason to keep it. >> >> Best regards, >> -- >> Bartlomiej Zolnierkiewicz >> Samsung R&D Institute Poland >> Samsung Electronics >> >>> Thanks, >>> Amit Daniel >>> > >>> > >>> > To disable C1 state on EXYNOS5440 something like: >>> > >>> > static int __init exynos4_init_cpuidle(void) >>> > { >>> > ... >>> > if (soc_is_exynos5440()) >>> > exynos4_idle_driver.state_count = 1; >>> > ... >>> > } >>> > >>> > should be done instead. >>> > >>> > Best regards, >>> > -- >>> > Bartlomiej Zolnierkiewicz >>> > Samsung R&D Institute Poland >>> > Samsung Electronics >>> > >>> > >>> > _______________________________________________ >>> > linux-arm-kernel mailing list >>> > linux-arm-kernel@lists.infradead.org >>> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >>> >>> >>> On Mon, Aug 26, 2013 at 3:36 PM, Bartlomiej Zolnierkiewicz >>> <b.zolnierkie@samsung.com> wrote: >>> > >>> > Hi, >>> > >>> > On Monday, August 26, 2013 09:14:42 AM Kukjin Kim wrote: >>> >> The following changes since commit ad81f0545ef01ea651886dddac4bef6cec930092: >>> >> >>> >> Linux 3.11-rc1 (2013-07-14 15:18:27 -0700) >>> >> >>> >> are available in the git repository at: >>> >> >>> >> git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git >>> >> tags/samsung-mach-exynos >>> >> >>> >> for you to fetch changes up to f52616f4233d71d0fb00f06f86d046d18d2b7f3b: >>> >> >>> >> ARM: EXYNOS: always enable PM domains support for EXYNOS4X12 (2013-08-19 >>> >> 05:05:16 +0900) >>> >> >>> >> ---------------------------------------------------------------- >>> >> update mach-exynos >>> >> - enable ARCH_HAS_BANDGAP for exynos SoCs >>> >> - skip C1 cpuidle for exynos5440 because non-supporting >>> >> - always enable PM domains for exynos4x12 >>> >> >>> >> ---------------------------------------------------------------- >>> >> Amit Daniel Kachhap (2): >>> >> ARM: EXYNOS: enable ARCH_HAS_BANDGAP >>> >> ARM: EXYNOS: Skip C1 cpuidle state for exynos5440 >>> > >>> > The patch "ARM: EXYNOS: Skip C1 cpuidle state for exynos5440": >>> > >>> > --- a/arch/arm/mach-exynos/cpuidle.c >>> > +++ b/arch/arm/mach-exynos/cpuidle.c >>> > @@ -210,7 +210,7 @@ static int __init exynos4_init_cpuidle(void) >>> > device->cpu = cpu_id; >>> > >>> > /* Support IDLE only */ >>> > - if (cpu_id != 0) >>> > + if (soc_is_exynos5440() || cpu_id != 0) >>> > device->state_count = 1; >>> > >>> > ret = cpuidle_register_device(device); >>> > >>> > is incorrect as noted a month ago in: >>> > >>> > http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/186355.html >>> > >>> > [ Because of the deficiency in the core cpuidle core (device->state_count >>> > not being used by governors' code) only sysfs entries for C1 state will be >>> > disabled and EXYNOS cpuidle driver will still attempt to use C1 state. >>> > >>> > also non-working device->state_count is planned to be removed by: >>> > >>> > http://permalink.gmane.org/gmane.linux.power-management.general/37390 >>> > >>> > >>> > To disable C1 state on EXYNOS5440 something like: >>> > >>> > static int __init exynos4_init_cpuidle(void) >>> > { >>> > ... >>> > if (soc_is_exynos5440()) >>> > exynos4_idle_driver.state_count = 1; >>> > ... >>> > } >>> > >>> > should be done instead. >>> > >>> > Best regards, >>> > -- >>> > Bartlomiej Zolnierkiewicz >>> > Samsung R&D Institute Poland >>> > Samsung Electronics >>> > >>> > >>> > _______________________________________________ >>> > linux-arm-kernel mailing list >>> > linux-arm-kernel@lists.infradead.org >>> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >> >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 28, 2013 at 12:57:25AM +0900, Kukjin Kim wrote: > On 08/27/13 13:08, Kevin Hilman wrote: > >amit daniel kachhap<amit.daniel@samsung.com> writes: > > > >>Submitted the V2 version of this patch with your suggestion. > > > >So will there be an updated branch (and pull request) with these changes? > > > OK, Bart's concern makes sense so I replaced with Amit's v2 patch. > > Kevin, here is updated branch so please pull following. > > If any problems, please kindly let me know. Please don't post pull requests in the middle of a comment thread; start a new thread or at least do a new subject for them -- they are much too easy to miss otherwise. -Olof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Olof Johansson wrote: > > On Wed, Aug 28, 2013 at 12:57:25AM +0900, Kukjin Kim wrote: > > On 08/27/13 13:08, Kevin Hilman wrote: > > >amit daniel kachhap<amit.daniel@samsung.com> writes: > > > > > >>Submitted the V2 version of this patch with your suggestion. > > > > > >So will there be an updated branch (and pull request) with these > changes? > > > > > OK, Bart's concern makes sense so I replaced with Amit's v2 patch. > > > > Kevin, here is updated branch so please pull following. > > > > If any problems, please kindly let me know. > > Please don't post pull requests in the middle of a comment thread; start a > new > thread or at least do a new subject for them -- they are much too easy to > miss > otherwise. > Oh, OK. I see. Thanks, Kukjin -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 28, 2013 at 08:34:58AM +0900, Kukjin Kim wrote: > The following changes since commit ad81f0545ef01ea651886dddac4bef6cec930092: > > Linux 3.11-rc1 (2013-07-14 15:18:27 -0700) > > are available in the git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git > tags/samsung-mach-exynos-v2 > Pulled into late/all. -Olof -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- a/arch/arm/mach-exynos/cpuidle.c +++ b/arch/arm/mach-exynos/cpuidle.c @@ -210,7 +210,7 @@ static int __init exynos4_init_cpuidle(void) device->cpu = cpu_id; /* Support IDLE only */ - if (cpu_id != 0) + if (soc_is_exynos5440() || cpu_id != 0) device->state_count = 1; ret = cpuidle_register_device(device);