* Re: [PATCH] ARM: shmobile: Fix wrong calculation in shmobile_init_delay()
2014-09-12 8:02 [PATCH] ARM: shmobile: Fix wrong calculation in shmobile_init_delay() Khiem Nguyen
@ 2014-09-18 5:24 ` Khiem Nguyen
2014-09-18 9:12 ` Simon Horman
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Khiem Nguyen @ 2014-09-18 5:24 UTC (permalink / raw)
To: linux-sh
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);
>
--
Best regards,
KHIEM Nguyen
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ARM: shmobile: Fix wrong calculation in shmobile_init_delay()
2014-09-12 8:02 [PATCH] ARM: shmobile: Fix wrong calculation in shmobile_init_delay() Khiem Nguyen
2014-09-18 5:24 ` Khiem Nguyen
@ 2014-09-18 9:12 ` Simon Horman
2014-09-19 1:04 ` Magnus Damm
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Simon Horman @ 2014-09-18 9:12 UTC (permalink / raw)
To: linux-sh
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ARM: shmobile: Fix wrong calculation in shmobile_init_delay()
2014-09-12 8:02 [PATCH] ARM: shmobile: Fix wrong calculation in shmobile_init_delay() Khiem Nguyen
2014-09-18 5:24 ` Khiem Nguyen
2014-09-18 9:12 ` Simon Horman
@ 2014-09-19 1:04 ` Magnus Damm
2014-09-19 1:57 ` Khiem Nguyen
2014-09-19 6:07 ` Magnus Damm
4 siblings, 0 replies; 6+ messages in thread
From: Magnus Damm @ 2014-09-19 1:04 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ARM: shmobile: Fix wrong calculation in shmobile_init_delay()
2014-09-12 8:02 [PATCH] ARM: shmobile: Fix wrong calculation in shmobile_init_delay() Khiem Nguyen
` (2 preceding siblings ...)
2014-09-19 1:04 ` Magnus Damm
@ 2014-09-19 1:57 ` Khiem Nguyen
2014-09-19 6:07 ` Magnus Damm
4 siblings, 0 replies; 6+ messages in thread
From: Khiem Nguyen @ 2014-09-19 1:57 UTC (permalink / raw)
To: linux-sh
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
>
--
Best regards,
KHIEM Nguyen
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ARM: shmobile: Fix wrong calculation in shmobile_init_delay()
2014-09-12 8:02 [PATCH] ARM: shmobile: Fix wrong calculation in shmobile_init_delay() Khiem Nguyen
` (3 preceding siblings ...)
2014-09-19 1:57 ` Khiem Nguyen
@ 2014-09-19 6:07 ` Magnus Damm
4 siblings, 0 replies; 6+ messages in thread
From: Magnus Damm @ 2014-09-19 6:07 UTC (permalink / raw)
To: linux-sh
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
^ permalink raw reply [flat|nested] 6+ messages in thread