| Message ID | 5412A889.2000209@renesas.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
Ping ? On 9/12/2014 5:02 PM, Khiem Nguyen wrote: > 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() 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. :) > 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. > > Signed-off-by: Khiem Nguyen <khiem.nguyen.xt@renesas.com> > --- > arch/arm/mach-shmobile/timer.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/mach-shmobile/timer.c b/arch/arm/mach-shmobile/timer.c > index 87c6be1..9394cad 100644 > --- a/arch/arm/mach-shmobile/timer.c > +++ b/arch/arm/mach-shmobile/timer.c > @@ -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); >
On Thu, Sep 18, 2014 at 02:24:28PM +0900, Khiem Nguyen wrote: > Ping ? > > On 9/12/2014 5:02 PM, Khiem Nguyen wrote: > > 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() > > 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? > Although I saw some minor issues in changelog, > I'd like to hear some comments before I sent v2. :) > > > 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. > > > > Signed-off-by: Khiem Nguyen <khiem.nguyen.xt@renesas.com> > > --- > > arch/arm/mach-shmobile/timer.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm/mach-shmobile/timer.c b/arch/arm/mach-shmobile/timer.c > > index 87c6be1..9394cad 100644 > > --- a/arch/arm/mach-shmobile/timer.c > > +++ b/arch/arm/mach-shmobile/timer.c > > @@ -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); > > > > -- > Best regards, > KHIEM Nguyen > -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Khiem-san, Thanks for your patch. On Fri, Sep 12, 2014 at 5:02 PM, Khiem Nguyen <khiem.nguyen.xt@renesas.com> wrote: > 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. > > Signed-off-by: Khiem Nguyen <khiem.nguyen.xt@renesas.com> > --- > arch/arm/mach-shmobile/timer.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/mach-shmobile/timer.c b/arch/arm/mach-shmobile/timer.c > index 87c6be1..9394cad 100644 > --- a/arch/arm/mach-shmobile/timer.c > +++ b/arch/arm/mach-shmobile/timer.c > @@ -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); 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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Magnus-san, Thanks for your reply. On 9/19/2014 10:04 AM, Magnus Damm wrote: > Hi Khiem-san, > > Thanks for your patch. > > On Fri, Sep 12, 2014 at 5:02 PM, Khiem Nguyen > <khiem.nguyen.xt@renesas.com> wrote: >> 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. >> >> Signed-off-by: Khiem Nguyen <khiem.nguyen.xt@renesas.com> >> --- >> arch/arm/mach-shmobile/timer.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm/mach-shmobile/timer.c b/arch/arm/mach-shmobile/timer.c >> index 87c6be1..9394cad 100644 >> --- a/arch/arm/mach-shmobile/timer.c >> +++ b/arch/arm/mach-shmobile/timer.c >> @@ -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); > > 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. > > 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 ? :) > > 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 Khiem-san, Thanks for your email. On Fri, Sep 19, 2014 at 10:57 AM, Khiem Nguyen <khiem.nguyen.xt@renesas.com> wrote: > Hi Magnus-san, > > Thanks for your reply. > > On 9/19/2014 10:04 AM, Magnus Damm wrote: >> Hi Khiem-san, >> >> Thanks for your patch. >> >> On Fri, Sep 12, 2014 at 5:02 PM, Khiem Nguyen >> <khiem.nguyen.xt@renesas.com> wrote: >>> 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. >>> >>> Signed-off-by: Khiem Nguyen <khiem.nguyen.xt@renesas.com> >> 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. >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/mach-shmobile/timer.c b/arch/arm/mach-shmobile/timer.c index 87c6be1..9394cad 100644 --- a/arch/arm/mach-shmobile/timer.c +++ b/arch/arm/mach-shmobile/timer.c @@ -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);
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. Signed-off-by: Khiem Nguyen <khiem.nguyen.xt@renesas.com> --- arch/arm/mach-shmobile/timer.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)