ARM: shmobile: Fix wrong calculation in shmobile_init_delay()
PATCH 1/1 ARM: shmobile: Fix wrong calculation in shmobile_init_delay()
The processing to get CPU information loop on all available CPUs and decide whether calculating preset_lpj for for CA15 or CA7/8/9. The calculation will go for CA7/8/9 prior to CA15.
The commit 0dc50fd ARM: shmobile: support Cortex-A7 in shmobile_init_delay() introduced wrong CPU selection in case CA15 and CA7/8/9 coexist, e.g R-CarH2. At the end, it leads to the wrong calculation of preset_lpj for R-CarH2.
Fix it by selecting the first CPU in DTS file.
--- arch/arm/mach-shmobile/timer.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
arch/arm/mach-shmobile/timer.c +6 -2
@@ -59,10 +59,14 @@ void __init shmobile_init_delay(void)if (of_device_is_compatible (np,"arm,cortex-a7" ) ||of_device_is_compatible (np,"arm,cortex-a8" ) || -of_device_is_compatible (np,"arm,cortex-a9" )) +of_device_is_compatible (np,"arm,cortex-a9" )) { is_a7_a8_a9 = true; -else if (of_device_is_compatible (np,"arm,cortex-a15" )) +break ; + } +else if (of_device_is_compatible (np,"arm,cortex-a15" )) { is_a15 = true; +break ; + } }of_node_put (cpus);
Signature · 1 line
1.7.9.5
Ping ?
Quoted 5 lines · Khiem Nguyen
The processing to get CPU information loop on all available CPUs and decide whether calculating
preset_lpjfor for CA15 or CA7/8/9. The calculation will go for CA7/8/9 prior to CA15.The commit 0dc50fd ARM: shmobile: support Cortex-A7 in
shmobile_init_delay()
As the commit 0dc50fd was already submitted in a PULL request for v3.18, I think this patch should be considered/reviewed soon.
If this patch made sense, I think this patch should go to v3.18, too. ... or we will introduce a regression in v3.18, I guess.
Although I saw some minor issues in changelog, I'd like to hear some comments before I sent v2. :)
Quoted 32 lines
introduced wrong CPU selection in case CA15 and CA7/8/9 coexist, e.g R-CarH2. At the end, it leads to the wrong calculation of
preset_lpjfor R-CarH2.Fix it by selecting the first CPU in DTS file.
Signed-off-by Khiem Nguyen <[email protected]>--- arch/arm/mach-shmobile/timer.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)arch/arm/mach-shmobile/timer.c +6
-2@@ -59,10 +59,14 @@ void __init shmobile_init_delay(void)if (of_device_is_compatible (np,"arm,cortex-a7" ) ||of_device_is_compatible (np,"arm,cortex-a8" ) || -of_device_is_compatible (np,"arm,cortex-a9" )) +of_device_is_compatible (np,"arm,cortex-a9" )) { is_a7_a8_a9 = true; -else if (of_device_is_compatible (np,"arm,cortex-a15" )) +break ; + } +else if (of_device_is_compatible (np,"arm,cortex-a15" )) { is_a15 = true; +break ; + } }of_node_put (cpus);
Signature · 2 lines
Best regards, KHIEM Nguyen
Quoted 7 lines · Khiem Nguyen
Ping ?
Quoted 4 lines · Khiem Nguyen
The processing to get CPU information loop on all available CPUs and decide whether calculating
preset_lpjfor for CA15 or CA7/8/9. The calculation will go for CA7/8/9 prior to CA15.
Quoted 7 lines
Quoted 1 line
The commit 0dc50fd ARM: shmobile: support Cortex-A7 in
shmobile_init_delay()As the commit 0dc50fd was already submitted in a PULL request for v3.18, I think this patch should be considered/reviewed soon.
If this patch made sense, I think this patch should go to v3.18, too. ... or we will introduce a regression in v3.18, I guess.
Yes, I agree.
Magnus do you have any thoughts?
Quoted 40 lines
Although I saw some minor issues in changelog, I'd like to hear some comments before I sent v2. :)
Quoted 32 lines
introduced wrong CPU selection in case CA15 and CA7/8/9 coexist, e.g R-CarH2. At the end, it leads to the wrong calculation of
preset_lpjfor R-CarH2.Fix it by selecting the first CPU in DTS file.
Signed-off-by Khiem Nguyen <[email protected]>--- arch/arm/mach-shmobile/timer.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)arch/arm/mach-shmobile/timer.c +6
-2@@ -59,10 +59,14 @@ void __init shmobile_init_delay(void)if (of_device_is_compatible (np,"arm,cortex-a7" ) ||of_device_is_compatible (np,"arm,cortex-a8" ) || -of_device_is_compatible (np,"arm,cortex-a9" )) +of_device_is_compatible (np,"arm,cortex-a9" )) { is_a7_a8_a9 = true; -else if (of_device_is_compatible (np,"arm,cortex-a15" )) +break ; + } +else if (of_device_is_compatible (np,"arm,cortex-a15" )) { is_a15 = true; +break ; + } }of_node_put (cpus);Signature · 2 lines
Best regards, KHIEM Nguyen
Magnus Damm
maintainerHi Khiem-san,
Thanks for your patch.
Quoted 26 lines · Khiem Nguyen
The processing to get CPU information loop on all available CPUs and decide whether calculating
preset_lpjfor for CA15 or CA7/8/9. The calculation will go for CA7/8/9 prior to CA15.The commit 0dc50fd ARM: shmobile: support Cortex-A7 in
shmobile_init_delay()introduced wrong CPU selection in case CA15 and CA7/8/9 coexist, e.g R-CarH2. At the end, it leads to the wrong calculation ofpreset_lpjfor R-CarH2.Fix it by selecting the first CPU in DTS file.
Signed-off-by Khiem Nguyen <[email protected]>--- arch/arm/mach-shmobile/timer.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)arch/arm/mach-shmobile/timer.c +1
-1@@ -59,10 +59,14 @@ void __init shmobile_init_delay(void)if (of_device_is_compatible (np,"arm,cortex-a7" ) ||of_device_is_compatible (np,"arm,cortex-a8" ) || -of_device_is_compatible (np,"arm,cortex-a9" )) +of_device_is_compatible (np,"arm,cortex-a9" )) { is_a7_a8_a9 = true;
arch/arm/mach-shmobile/timer.c void __init shmobile_init_delay(void) · line 72
Quoted 10 lines
arch/arm/mach-shmobile/timer.c +5
-1-else if (of_device_is_compatible (np,"arm,cortex-a15" )) +break ; + } +else if (of_device_is_compatible (np,"arm,cortex-a15" )) { is_a15 = true; +break ; + } }of_node_put (cpus);
I may misunderstand the problem here, but I don't think this patch is needed.
Basically, my view of things is that if we have a CA7 core on the system then a longer delay may be necessary. This to make sure that delays running on CA7 will work as expected as well. This patch introduces some ordering dependency and just considers the first CPU.
A different fix for CA7 may however be necessary to handle arch timer as expected. So you are right that commit "0dc50fd ARM: shmobile:
support Cortex-A7 in shmobile_init_delay()" may need some more work.
Thanks,
/ magnus
Hi Magnus-san,
Thanks for your reply.
Quoted 41 lines · Magnus Damm
Hi Khiem-san,
Thanks for your patch.
Quoted 35 lines · Khiem Nguyen
The processing to get CPU information loop on all available CPUs and decide whether calculating
preset_lpjfor for CA15 or CA7/8/9. The calculation will go for CA7/8/9 prior to CA15.The commit 0dc50fd ARM: shmobile: support Cortex-A7 in
shmobile_init_delay()introduced wrong CPU selection in case CA15 and CA7/8/9 coexist, e.g R-CarH2. At the end, it leads to the wrong calculation ofpreset_lpjfor R-CarH2.Fix it by selecting the first CPU in DTS file.
Signed-off-by Khiem Nguyen <[email protected]>--- arch/arm/mach-shmobile/timer.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)arch/arm/mach-shmobile/timer.c +6
-2@@ -59,10 +59,14 @@ void __init shmobile_init_delay(void)if (of_device_is_compatible (np,"arm,cortex-a7" ) ||of_device_is_compatible (np,"arm,cortex-a8" ) || -of_device_is_compatible (np,"arm,cortex-a9" )) +of_device_is_compatible (np,"arm,cortex-a9" )) { is_a7_a8_a9 = true; -else if (of_device_is_compatible (np,"arm,cortex-a15" )) +break ; + } +else if (of_device_is_compatible (np,"arm,cortex-a15" )) { is_a15 = true; +break ; + } }
Quoted 3 lines
arch/arm/mach-shmobile/timer.c void __init shmobile_init_delay(void) · line 72
Quoted 1 line
arch/arm/mach-shmobile/timer.c +0
-0of_node_put (cpus);I may misunderstand the problem here, but I don't think this patch is needed.
Base on your explanation, I think it's not needed. Sorry for the noise.
Quoted 5 lines
Basically, my view of things is that if we have a CA7 core on the system then a longer delay may be necessary. This to make sure that delays running on CA7 will work as expected as well. This patch introduces some ordering dependency and just considers the first CPU.
So, before 0dc50fd was merged, if all CPUs (CA15 and CA7) in R-CarH2 are enabled, like big.LITTLE configuration, system may not run correctly (due to insufficient delay of CA7).
Is it correct ? :)
Quoted 9 lines
A different fix for CA7 may however be necessary to handle arch timer as expected. So you are right that commit "0dc50fd ARM: shmobile:
support Cortex-A7 inshmobile_init_delay()" may need some more work.Thanks,
/ magnus
Signature · 2 lines
Best regards, KHIEM Nguyen
Magnus Damm
maintainerHi Khiem-san,
Thanks for your email.
Quoted 19 lines · Khiem Nguyen
Hi Magnus-san,
Thanks for your reply.
Quoted 14 lines · Magnus Damm
Hi Khiem-san,
Thanks for your patch.
Quoted 8 lines · Khiem Nguyen
The processing to get CPU information loop on all available CPUs and decide whether calculating
preset_lpjfor for CA15 or CA7/8/9. The calculation will go for CA7/8/9 prior to CA15.The commit 0dc50fd ARM: shmobile: support Cortex-A7 in
shmobile_init_delay()introduced wrong CPU selection in case CA15 and CA7/8/9 coexist, e.g R-CarH2. At the end, it leads to the wrong calculation ofpreset_lpjfor R-CarH2.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Fix it by selecting the first CPU in DTS file.
Signed-off-by Khiem Nguyen <[email protected]>I may misunderstand the problem here, but I don't think this patch is needed.
Base on your explanation, I think it's not needed. Sorry for the noise.
No problem, thanks for sending patches.
Quoted 9 lines
Quoted 4 lines
Basically, my view of things is that if we have a CA7 core on the system then a longer delay may be necessary. This to make sure that delays running on CA7 will work as expected as well. This patch introduces some ordering dependency and just considers the first CPU.
So, before 0dc50fd was merged, if all CPUs (CA15 and CA7) in R-CarH2 are enabled, like big.LITTLE configuration, system may not run correctly (due to insufficient delay of CA7).
Is it correct ? :)
You are correct that on r8a7790 in upstream only "Big boot mode" with 4 x CA15 SMP mode is working as expected. "Little boot mode" and CA7 seems to lack proper delay handling. =)
I do however wish to get "Little boot mode" correctly supported in upstream, but we are not 100% there yet. Today there are various issues related to arch timer and delay calculation in case of "Little boot mode".
So from my side the "0dc50f5" commit brings us one step closer to improved CA7 support by correctly handling delays. Arch timer support needs more work though - both for r8a7794 and r8a7790!
Thanks,
/ magnus