diff mbox

[GIT,PULL,3/3] 2nd Round Samsung mach-exynos for v3.12

Message ID 3145165.ZB7l3vNJ7a@amdc1032 (mailing list archive)
State New, archived
Headers show

Commit Message

Bartlomiej Zolnierkiewicz Aug. 26, 2013, 10:06 a.m. UTC
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":


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

Comments

Amit Kachhap Aug. 26, 2013, 11:03 a.m. UTC | #1
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
Bartlomiej Zolnierkiewicz Aug. 26, 2013, 11:19 a.m. UTC | #2
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
kgene@kernel.org Aug. 26, 2013, 11:52 a.m. UTC | #3
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
Amit Kachhap Aug. 26, 2013, 12:18 p.m. UTC | #4
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
Bartlomiej Zolnierkiewicz Aug. 26, 2013, 1:51 p.m. UTC | #5
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
Kevin Hilman Aug. 27, 2013, 4:08 a.m. UTC | #6
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
Olof Johansson Aug. 27, 2013, 6:57 p.m. UTC | #7
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
Kim Kukjin Aug. 27, 2013, 11:33 p.m. UTC | #8
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
Olof Johansson Aug. 29, 2013, 8:29 p.m. UTC | #9
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
diff mbox

Patch

--- 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);