All of lore.kernel.org
 help / color / mirror / Atom feed
From: Tomasz Figa <tomasz.figa@gmail.com>
To: linux-arm-kernel@lists.infradead.org
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>,
	kgene.kim@samsung.com, rjw@sisk.pl, patches@linaro.org,
	linaro-kernel@lists.linaro.org, linux-pm@vger.kernel.org
Subject: Re: [PATCH 1/4] ARM: exynos: cpuidle: Fallback to WFI, when AFTR is selected
Date: Wed, 24 Jul 2013 10:32:47 +0200	[thread overview]
Message-ID: <1566146.MINY8cDBRi@flatron> (raw)
In-Reply-To: <1374152070-21008-1-git-send-email-daniel.lezcano@linaro.org>

Hi Daniel,

On Thursday 18 of July 2013 14:54:27 Daniel Lezcano wrote:
> When there are several cpus online, the AFTR state does not work.
> 
> The AFTR must be selected only when there is one cpu online.
> 
> The previous code was already handling this case.
> 
> Simplified the code, especially the init routine to have the same init
> pattern than the other drivers.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  arch/arm/mach-exynos/cpuidle.c |   13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/cpuidle.c
> b/arch/arm/mach-exynos/cpuidle.c index 17a18ff..cc4b097 100644
> --- a/arch/arm/mach-exynos/cpuidle.c
> +++ b/arch/arm/mach-exynos/cpuidle.c
> @@ -147,16 +147,11 @@ static int exynos4_enter_lowpower(struct
> cpuidle_device *dev, struct cpuidle_driver *drv,
>  				int index)
>  {
> -	int new_index = index;
> -
>  	/* This mode only can be entered when other core's are offline */
>  	if (num_online_cpus() > 1)
> -		new_index = drv->safe_state_index;
> +		return drv->states[0].enter(dev, drv, 0);
> 
> -	if (new_index == 0)
> -		return arm_cpuidle_simple_enter(dev, drv, new_index);
> -	else
> -		return exynos4_enter_core0_aftr(dev, drv, new_index);
> +	return exynos4_enter_core0_aftr(dev, drv, index);
>  }
> 
>  static void __init exynos5_core_down_clk(void)
> @@ -209,10 +204,6 @@ static int __init exynos4_init_cpuidle(void)
>  		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
>  		device->cpu = cpu_id;
> 
> -		/* Support IDLE only */
> -		if (cpu_id != 0)
> -			device->state_count = 1;
> -

Are you sure that this is okay? It means, assuming that you have CPU0 
hotplug enabled, that you can enter the AFTR state if _any_ single core 
remains plugged in, which is technically possible, but then wake-up will 
occur on CPU0, which is not what cpuidle and hotplug code expect.

P.S. Please keep linux-samsung-soc@vger.kernel.org mailing list on Cc for 
patches related to Samsung SoCs.

Best regards,
Tomasz


WARNING: multiple messages have this Message-ID (diff)
From: tomasz.figa@gmail.com (Tomasz Figa)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 1/4] ARM: exynos: cpuidle: Fallback to WFI, when AFTR is selected
Date: Wed, 24 Jul 2013 10:32:47 +0200	[thread overview]
Message-ID: <1566146.MINY8cDBRi@flatron> (raw)
In-Reply-To: <1374152070-21008-1-git-send-email-daniel.lezcano@linaro.org>

Hi Daniel,

On Thursday 18 of July 2013 14:54:27 Daniel Lezcano wrote:
> When there are several cpus online, the AFTR state does not work.
> 
> The AFTR must be selected only when there is one cpu online.
> 
> The previous code was already handling this case.
> 
> Simplified the code, especially the init routine to have the same init
> pattern than the other drivers.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  arch/arm/mach-exynos/cpuidle.c |   13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/cpuidle.c
> b/arch/arm/mach-exynos/cpuidle.c index 17a18ff..cc4b097 100644
> --- a/arch/arm/mach-exynos/cpuidle.c
> +++ b/arch/arm/mach-exynos/cpuidle.c
> @@ -147,16 +147,11 @@ static int exynos4_enter_lowpower(struct
> cpuidle_device *dev, struct cpuidle_driver *drv,
>  				int index)
>  {
> -	int new_index = index;
> -
>  	/* This mode only can be entered when other core's are offline */
>  	if (num_online_cpus() > 1)
> -		new_index = drv->safe_state_index;
> +		return drv->states[0].enter(dev, drv, 0);
> 
> -	if (new_index == 0)
> -		return arm_cpuidle_simple_enter(dev, drv, new_index);
> -	else
> -		return exynos4_enter_core0_aftr(dev, drv, new_index);
> +	return exynos4_enter_core0_aftr(dev, drv, index);
>  }
> 
>  static void __init exynos5_core_down_clk(void)
> @@ -209,10 +204,6 @@ static int __init exynos4_init_cpuidle(void)
>  		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
>  		device->cpu = cpu_id;
> 
> -		/* Support IDLE only */
> -		if (cpu_id != 0)
> -			device->state_count = 1;
> -

Are you sure that this is okay? It means, assuming that you have CPU0 
hotplug enabled, that you can enter the AFTR state if _any_ single core 
remains plugged in, which is technically possible, but then wake-up will 
occur on CPU0, which is not what cpuidle and hotplug code expect.

P.S. Please keep linux-samsung-soc at vger.kernel.org mailing list on Cc for 
patches related to Samsung SoCs.

Best regards,
Tomasz

  parent reply	other threads:[~2013-07-24  8:32 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-07-18 12:54 [PATCH 1/4] ARM: exynos: cpuidle: Fallback to WFI, when AFTR is selected Daniel Lezcano
2013-07-18 12:54 ` Daniel Lezcano
2013-07-18 12:54 ` [PATCH 2/4] ARM: exynos: cpuidle: Use the common cpuidle register routine Daniel Lezcano
2013-07-18 12:54   ` Daniel Lezcano
2013-07-19 16:03   ` Bartlomiej Zolnierkiewicz
2013-07-19 16:03     ` Bartlomiej Zolnierkiewicz
2013-07-22  4:36     ` Daniel Lezcano
2013-07-22  4:36       ` Daniel Lezcano
2013-07-22  9:35       ` Bartlomiej Zolnierkiewicz
2013-07-22  9:35         ` Bartlomiej Zolnierkiewicz
2013-07-24  8:36   ` Tomasz Figa
2013-07-24  8:36     ` Tomasz Figa
2013-07-18 12:54 ` [PATCH 3/4] ARM: exynos: cpuidle: Move exynos4_idle_driver declaration below in the code Daniel Lezcano
2013-07-18 12:54   ` Daniel Lezcano
2013-07-24  8:34   ` Tomasz Figa
2013-07-24  8:34     ` Tomasz Figa
2013-07-18 12:54 ` [PATCH 4/4] ARM: exynos: cpuidle: Remove useless headers Daniel Lezcano
2013-07-18 12:54   ` Daniel Lezcano
2013-07-24  8:35   ` Tomasz Figa
2013-07-24  8:35     ` Tomasz Figa
2013-07-24  8:32 ` Tomasz Figa [this message]
2013-07-24  8:32   ` [PATCH 1/4] ARM: exynos: cpuidle: Fallback to WFI, when AFTR is selected Tomasz Figa
2013-07-24 15:16   ` Bartlomiej Zolnierkiewicz
2013-07-24 15:16     ` Bartlomiej Zolnierkiewicz

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1566146.MINY8cDBRi@flatron \
    --to=tomasz.figa@gmail.com \
    --cc=daniel.lezcano@linaro.org \
    --cc=kgene.kim@samsung.com \
    --cc=linaro-kernel@lists.linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=patches@linaro.org \
    --cc=rjw@sisk.pl \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.