[PATCH] ARM: tegra: don't timeout if CPU is powergated

[PATCH] ARM: tegra: don't timeout if CPU is powergated

[Stefan Agner]

When booting secondary CPU(s) which are not yet powergated, a wrong
check lead to a timeout after 100 jiffies. With this patch, we only
delay powergating if CPUs are still not powered yet.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
I happend to come accross this while working on Colibri T30 support.
Obviously, the downstream U-Boot doesn't powergate all CPUs, so
the Linux kernel always timed out when booting CPU 1 through 3...

 arch/arm/mach-tegra/platsmp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c
index eb72ae7..929d104 100644
--- a/arch/arm/mach-tegra/platsmp.c
+++ b/arch/arm/mach-tegra/platsmp.c
@@ -114,7 +114,7 @@ static int tegra30_boot_secondary(unsigned int cpu, struct task_struct *idle)
 
 		/* Wait for the power to come up. */
 		timeout = jiffies + msecs_to_jiffies(100);
-		while (tegra_pmc_cpu_is_powered(cpu)) {
+		while (!tegra_pmc_cpu_is_powered(cpu)) {
 			if (time_after(jiffies, timeout))
 				return -ETIMEDOUT;
 			udelay(10);
-- 
1.8.5.4

[PATCH] ARM: tegra: don't timeout if CPU is powergated

[Stefan Agner]

When booting secondary CPU(s) which are not yet powergated, a wrong
check lead to a timeout after 100 jiffies. With this patch, we only
delay powergating if CPUs are still not powered yet.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
I happend to come accross this while working on Colibri T30 support.
Obviously, the downstream U-Boot doesn't powergate all CPUs, so
the Linux kernel always timed out when booting CPU 1 through 3...

 arch/arm/mach-tegra/platsmp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c
index eb72ae7..929d104 100644
--- a/arch/arm/mach-tegra/platsmp.c
+++ b/arch/arm/mach-tegra/platsmp.c
@@ -114,7 +114,7 @@ static int tegra30_boot_secondary(unsigned int cpu, struct task_struct *idle)
 
 		/* Wait for the power to come up. */
 		timeout = jiffies + msecs_to_jiffies(100);
-		while (tegra_pmc_cpu_is_powered(cpu)) {
+		while (!tegra_pmc_cpu_is_powered(cpu)) {
 			if (time_after(jiffies, timeout))
 				return -ETIMEDOUT;
 			udelay(10);
-- 
1.8.5.4

Re: [PATCH] ARM: tegra: don't timeout if CPU is powergated

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2106 bytes --]

On Tue, Feb 11, 2014 at 01:44:13AM +0100, Stefan Agner wrote:
> When booting secondary CPU(s) which are not yet powergated, a wrong
> check lead to a timeout after 100 jiffies. With this patch, we only
> delay powergating if CPUs are still not powered yet.
> 
> Signed-off-by: Stefan Agner <stefan-XLVq0VzYD2Y@public.gmane.org>
> ---
> I happend to come accross this while working on Colibri T30 support.
> Obviously, the downstream U-Boot doesn't powergate all CPUs, so
> the Linux kernel always timed out when booting CPU 1 through 3...
> 
>  arch/arm/mach-tegra/platsmp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c
> index eb72ae7..929d104 100644
> --- a/arch/arm/mach-tegra/platsmp.c
> +++ b/arch/arm/mach-tegra/platsmp.c
> @@ -114,7 +114,7 @@ static int tegra30_boot_secondary(unsigned int cpu, struct task_struct *idle)
>  
>  		/* Wait for the power to come up. */
>  		timeout = jiffies + msecs_to_jiffies(100);
> -		while (tegra_pmc_cpu_is_powered(cpu)) {
> +		while (!tegra_pmc_cpu_is_powered(cpu)) {
>  			if (time_after(jiffies, timeout))
>  				return -ETIMEDOUT;
>  			udelay(10);

This is indeed what I'd expect the code to look like. Since the code
enables power to the CPU, the logical thing to do would be to then wait
for it to be powered up.

I don't quite understand when exactly this will fail, since the whole
block above is conditional on !tegra_pmc_cpu_is_powered() condition, so
the only way that this can happen is when the first check fails, then
the CPU is powered on and the PMC recognizes the CPU as powered before
the while (...) is executed.

Thinking about it I've seen an issue on Cardhu where occasionally only
three of the four CPUs actually came up (I've only noticed this since
the DRM panel patches because three penguins looks kind of weird =).
This bug would explain that issue.

Any way, the new code makes much more sense than the old one, so:

Reviewed-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] ARM: tegra: don't timeout if CPU is powergated

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2057 bytes --]

On Tue, Feb 11, 2014 at 01:44:13AM +0100, Stefan Agner wrote:
> When booting secondary CPU(s) which are not yet powergated, a wrong
> check lead to a timeout after 100 jiffies. With this patch, we only
> delay powergating if CPUs are still not powered yet.
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
> I happend to come accross this while working on Colibri T30 support.
> Obviously, the downstream U-Boot doesn't powergate all CPUs, so
> the Linux kernel always timed out when booting CPU 1 through 3...
> 
>  arch/arm/mach-tegra/platsmp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c
> index eb72ae7..929d104 100644
> --- a/arch/arm/mach-tegra/platsmp.c
> +++ b/arch/arm/mach-tegra/platsmp.c
> @@ -114,7 +114,7 @@ static int tegra30_boot_secondary(unsigned int cpu, struct task_struct *idle)
>  
>  		/* Wait for the power to come up. */
>  		timeout = jiffies + msecs_to_jiffies(100);
> -		while (tegra_pmc_cpu_is_powered(cpu)) {
> +		while (!tegra_pmc_cpu_is_powered(cpu)) {
>  			if (time_after(jiffies, timeout))
>  				return -ETIMEDOUT;
>  			udelay(10);

This is indeed what I'd expect the code to look like. Since the code
enables power to the CPU, the logical thing to do would be to then wait
for it to be powered up.

I don't quite understand when exactly this will fail, since the whole
block above is conditional on !tegra_pmc_cpu_is_powered() condition, so
the only way that this can happen is when the first check fails, then
the CPU is powered on and the PMC recognizes the CPU as powered before
the while (...) is executed.

Thinking about it I've seen an issue on Cardhu where occasionally only
three of the four CPUs actually came up (I've only noticed this since
the DRM panel patches because three penguins looks kind of weird =).
This bug would explain that issue.

Any way, the new code makes much more sense than the old one, so:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

[PATCH] ARM: tegra: don't timeout if CPU is powergated

[Thierry Reding]

On Tue, Feb 11, 2014 at 01:44:13AM +0100, Stefan Agner wrote:
> When booting secondary CPU(s) which are not yet powergated, a wrong
> check lead to a timeout after 100 jiffies. With this patch, we only
> delay powergating if CPUs are still not powered yet.
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
> I happend to come accross this while working on Colibri T30 support.
> Obviously, the downstream U-Boot doesn't powergate all CPUs, so
> the Linux kernel always timed out when booting CPU 1 through 3...
> 
>  arch/arm/mach-tegra/platsmp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-tegra/platsmp.c b/arch/arm/mach-tegra/platsmp.c
> index eb72ae7..929d104 100644
> --- a/arch/arm/mach-tegra/platsmp.c
> +++ b/arch/arm/mach-tegra/platsmp.c
> @@ -114,7 +114,7 @@ static int tegra30_boot_secondary(unsigned int cpu, struct task_struct *idle)
>  
>  		/* Wait for the power to come up. */
>  		timeout = jiffies + msecs_to_jiffies(100);
> -		while (tegra_pmc_cpu_is_powered(cpu)) {
> +		while (!tegra_pmc_cpu_is_powered(cpu)) {
>  			if (time_after(jiffies, timeout))
>  				return -ETIMEDOUT;
>  			udelay(10);

This is indeed what I'd expect the code to look like. Since the code
enables power to the CPU, the logical thing to do would be to then wait
for it to be powered up.

I don't quite understand when exactly this will fail, since the whole
block above is conditional on !tegra_pmc_cpu_is_powered() condition, so
the only way that this can happen is when the first check fails, then
the CPU is powered on and the PMC recognizes the CPU as powered before
the while (...) is executed.

Thinking about it I've seen an issue on Cardhu where occasionally only
three of the four CPUs actually came up (I've only noticed this since
the DRM panel patches because three penguins looks kind of weird =).
This bug would explain that issue.

Any way, the new code makes much more sense than the old one, so:

Reviewed-by: Thierry Reding <treding@nvidia.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140211/d690f873/attachment.sig>

Re: [PATCH] ARM: tegra: don't timeout if CPU is powergated

[Stephen Warren]

On 02/10/2014 05:44 PM, Stefan Agner wrote:
> When booting secondary CPU(s) which are not yet powergated, a wrong
> check lead to a timeout after 100 jiffies. With this patch, we only
> delay powergating if CPUs are still not powered yet.

I've applied this to Tegra's for-3.15/soc branch.

[PATCH] ARM: tegra: don't timeout if CPU is powergated

[Stephen Warren]

On 02/10/2014 05:44 PM, Stefan Agner wrote:
> When booting secondary CPU(s) which are not yet powergated, a wrong
> check lead to a timeout after 100 jiffies. With this patch, we only
> delay powergating if CPUs are still not powered yet.

I've applied this to Tegra's for-3.15/soc branch.

Re: [PATCH] ARM: tegra: don't timeout if CPU is powergated

[Marc Dietrich]

Am Mittwoch, 12. Februar 2014, 12:20:29 schrieb Stephen Warren:
> On 02/10/2014 05:44 PM, Stefan Agner wrote:
> > When booting secondary CPU(s) which are not yet powergated, a wrong
> > check lead to a timeout after 100 jiffies. With this patch, we only
> > delay powergating if CPUs are still not powered yet.
> 
> I've applied this to Tegra's for-3.15/soc branch.

also for 3.14 and maybe lower versioned kernels? Since this seems to fix a bug 
where some core doesn't come up.

Marc

Re: [PATCH] ARM: tegra: don't timeout if CPU is powergated

[Marc Dietrich]

Am Mittwoch, 12. Februar 2014, 12:20:29 schrieb Stephen Warren:
> On 02/10/2014 05:44 PM, Stefan Agner wrote:
> > When booting secondary CPU(s) which are not yet powergated, a wrong
> > check lead to a timeout after 100 jiffies. With this patch, we only
> > delay powergating if CPUs are still not powered yet.
> 
> I've applied this to Tegra's for-3.15/soc branch.

also for 3.14 and maybe lower versioned kernels? Since this seems to fix a bug 
where some core doesn't come up.

Marc

[PATCH] ARM: tegra: don't timeout if CPU is powergated

[Marc Dietrich]

Am Mittwoch, 12. Februar 2014, 12:20:29 schrieb Stephen Warren:
> On 02/10/2014 05:44 PM, Stefan Agner wrote:
> > When booting secondary CPU(s) which are not yet powergated, a wrong
> > check lead to a timeout after 100 jiffies. With this patch, we only
> > delay powergating if CPUs are still not powered yet.
> 
> I've applied this to Tegra's for-3.15/soc branch.

also for 3.14 and maybe lower versioned kernels? Since this seems to fix a bug 
where some core doesn't come up.

Marc

Re: [PATCH] ARM: tegra: don't timeout if CPU is powergated

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1189 bytes --]

On Thu, Feb 13, 2014 at 09:28:52AM +0100, Marc Dietrich wrote:
> Am Mittwoch, 12. Februar 2014, 12:20:29 schrieb Stephen Warren:
> > On 02/10/2014 05:44 PM, Stefan Agner wrote:
> > > When booting secondary CPU(s) which are not yet powergated, a wrong
> > > check lead to a timeout after 100 jiffies. With this patch, we only
> > > delay powergating if CPUs are still not powered yet.
> > 
> > I've applied this to Tegra's for-3.15/soc branch.
> 
> also for 3.14 and maybe lower versioned kernels? Since this seems to fix a bug 
> where some core doesn't come up.

Yeah, this bug has been there for pretty much forever it seems. Commit
86e51a2ee471 "ARM: tegra: support for secondary cores on Tegra30" added
tegra30_boot_secondary() (named tegra30_power_up_cpu() back then, which
was renamed to tegra30_boot_secondary() in commit 0d1f79b033bb "ARM:
tegra: refactor tegra{20,30}_boot_secondary". The latter was introduced
in v3.10, so I guess backporting it to stable releases all the way back
to v3.10 would be good.

Backporting to earlier versions (86e51a2ee471 went into v3.4) will be a
lot more difficult since some of the APIs were renamed since then.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] ARM: tegra: don't timeout if CPU is powergated

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1189 bytes --]

On Thu, Feb 13, 2014 at 09:28:52AM +0100, Marc Dietrich wrote:
> Am Mittwoch, 12. Februar 2014, 12:20:29 schrieb Stephen Warren:
> > On 02/10/2014 05:44 PM, Stefan Agner wrote:
> > > When booting secondary CPU(s) which are not yet powergated, a wrong
> > > check lead to a timeout after 100 jiffies. With this patch, we only
> > > delay powergating if CPUs are still not powered yet.
> > 
> > I've applied this to Tegra's for-3.15/soc branch.
> 
> also for 3.14 and maybe lower versioned kernels? Since this seems to fix a bug 
> where some core doesn't come up.

Yeah, this bug has been there for pretty much forever it seems. Commit
86e51a2ee471 "ARM: tegra: support for secondary cores on Tegra30" added
tegra30_boot_secondary() (named tegra30_power_up_cpu() back then, which
was renamed to tegra30_boot_secondary() in commit 0d1f79b033bb "ARM:
tegra: refactor tegra{20,30}_boot_secondary". The latter was introduced
in v3.10, so I guess backporting it to stable releases all the way back
to v3.10 would be good.

Backporting to earlier versions (86e51a2ee471 went into v3.4) will be a
lot more difficult since some of the APIs were renamed since then.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

[PATCH] ARM: tegra: don't timeout if CPU is powergated

[Thierry Reding]

On Thu, Feb 13, 2014 at 09:28:52AM +0100, Marc Dietrich wrote:
> Am Mittwoch, 12. Februar 2014, 12:20:29 schrieb Stephen Warren:
> > On 02/10/2014 05:44 PM, Stefan Agner wrote:
> > > When booting secondary CPU(s) which are not yet powergated, a wrong
> > > check lead to a timeout after 100 jiffies. With this patch, we only
> > > delay powergating if CPUs are still not powered yet.
> > 
> > I've applied this to Tegra's for-3.15/soc branch.
> 
> also for 3.14 and maybe lower versioned kernels? Since this seems to fix a bug 
> where some core doesn't come up.

Yeah, this bug has been there for pretty much forever it seems. Commit
86e51a2ee471 "ARM: tegra: support for secondary cores on Tegra30" added
tegra30_boot_secondary() (named tegra30_power_up_cpu() back then, which
was renamed to tegra30_boot_secondary() in commit 0d1f79b033bb "ARM:
tegra: refactor tegra{20,30}_boot_secondary". The latter was introduced
in v3.10, so I guess backporting it to stable releases all the way back
to v3.10 would be good.

Backporting to earlier versions (86e51a2ee471 went into v3.4) will be a
lot more difficult since some of the APIs were renamed since then.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140213/b74037ed/attachment-0001.sig>

Re: [PATCH] ARM: tegra: don't timeout if CPU is powergated

[Stephen Warren]

On 02/13/2014 01:49 AM, Thierry Reding wrote:
> On Thu, Feb 13, 2014 at 09:28:52AM +0100, Marc Dietrich wrote:
>> Am Mittwoch, 12. Februar 2014, 12:20:29 schrieb Stephen Warren:
>>> On 02/10/2014 05:44 PM, Stefan Agner wrote:
>>>> When booting secondary CPU(s) which are not yet powergated, a wrong
>>>> check lead to a timeout after 100 jiffies. With this patch, we only
>>>> delay powergating if CPUs are still not powered yet.
>>>
>>> I've applied this to Tegra's for-3.15/soc branch.
>>
>> also for 3.14 and maybe lower versioned kernels? Since this seems to fix a bug 
>> where some core doesn't come up.
> 
> Yeah, this bug has been there for pretty much forever it seems. Commit
> 86e51a2ee471 "ARM: tegra: support for secondary cores on Tegra30" added
> tegra30_boot_secondary() (named tegra30_power_up_cpu() back then, which
> was renamed to tegra30_boot_secondary() in commit 0d1f79b033bb "ARM:
> tegra: refactor tegra{20,30}_boot_secondary". The latter was introduced
> in v3.10, so I guess backporting it to stable releases all the way back
> to v3.10 would be good.
> 
> Backporting to earlier versions (86e51a2ee471 went into v3.4) will be a
> lot more difficult since some of the APIs were renamed since then.

I'm actually uninclined to backport this; I've never once seen an issue
because of this problem, and nobody has reported it in older kernels.

[PATCH] ARM: tegra: don't timeout if CPU is powergated

[Stephen Warren]

On 02/13/2014 01:49 AM, Thierry Reding wrote:
> On Thu, Feb 13, 2014 at 09:28:52AM +0100, Marc Dietrich wrote:
>> Am Mittwoch, 12. Februar 2014, 12:20:29 schrieb Stephen Warren:
>>> On 02/10/2014 05:44 PM, Stefan Agner wrote:
>>>> When booting secondary CPU(s) which are not yet powergated, a wrong
>>>> check lead to a timeout after 100 jiffies. With this patch, we only
>>>> delay powergating if CPUs are still not powered yet.
>>>
>>> I've applied this to Tegra's for-3.15/soc branch.
>>
>> also for 3.14 and maybe lower versioned kernels? Since this seems to fix a bug 
>> where some core doesn't come up.
> 
> Yeah, this bug has been there for pretty much forever it seems. Commit
> 86e51a2ee471 "ARM: tegra: support for secondary cores on Tegra30" added
> tegra30_boot_secondary() (named tegra30_power_up_cpu() back then, which
> was renamed to tegra30_boot_secondary() in commit 0d1f79b033bb "ARM:
> tegra: refactor tegra{20,30}_boot_secondary". The latter was introduced
> in v3.10, so I guess backporting it to stable releases all the way back
> to v3.10 would be good.
> 
> Backporting to earlier versions (86e51a2ee471 went into v3.4) will be a
> lot more difficult since some of the APIs were renamed since then.

I'm actually uninclined to backport this; I've never once seen an issue
because of this problem, and nobody has reported it in older kernels.