* Re: [PATCH] ARM: shmobile: Handle CA7 arch timer delay
2014-10-05 23:59 [PATCH] ARM: shmobile: Handle CA7 arch timer delay Magnus Damm
@ 2014-10-06 23:59 ` Simon Horman
2014-10-07 0:50 ` Khiem Nguyen
` (4 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Simon Horman @ 2014-10-06 23:59 UTC (permalink / raw)
To: linux-sh
On Mon, Oct 06, 2014 at 08:59:20AM +0900, Magnus Damm wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
>
> Update the delay code to include arch timer checks
> for CA7. From a arch timer availability perspective
> CA7 should be treated same as CA15.
>
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> ---
>
> Written against renesas-devel-20141002-v3.17-rc7
Thanks Magnus, I have queued this up.
There is an outstanding question from Arnd
relating to the last round of init delay updates.
If you have a moment could you look into it?
* Re: [GIT PULL] Renesas ARM Based SoC Init Delay Updates For v3.18
http://www.spinics.net/lists/arm-kernel/msg360580.html
> arch/arm/mach-shmobile/timer.c | 23 +++++++++++++++--------
> 1 file changed, 15 insertions(+), 8 deletions(-)
>
> --- 0001/arch/arm/mach-shmobile/timer.c
> +++ work/arch/arm/mach-shmobile/timer.c 2014-10-06 08:44:21.000000000 +0900
> @@ -45,6 +45,7 @@ void __init shmobile_init_delay(void)
> struct device_node *np, *cpus;
> bool is_a7_a8_a9 = false;
> bool is_a15 = false;
> + bool has_arch_timer = false;
> u32 max_freq = 0;
>
> cpus = of_find_node_by_path("/cpus");
> @@ -57,12 +58,16 @@ void __init shmobile_init_delay(void)
> if (!of_property_read_u32(np, "clock-frequency", &freq))
> max_freq = max(max_freq, freq);
>
> - 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"))
> + if (of_device_is_compatible(np, "arm,cortex-a8") ||
> + of_device_is_compatible(np, "arm,cortex-a9")) {
> is_a7_a8_a9 = true;
> - else if (of_device_is_compatible(np, "arm,cortex-a15"))
> + } else if (of_device_is_compatible(np, "arm,cortex-a7")) {
> + is_a7_a8_a9 = true;
> + has_arch_timer = true;
> + } else if (of_device_is_compatible(np, "arm,cortex-a15")) {
> is_a15 = true;
> + has_arch_timer = true;
> + }
> }
>
> of_node_put(cpus);
> @@ -70,10 +75,12 @@ void __init shmobile_init_delay(void)
> if (!max_freq)
> return;
>
> - if (is_a7_a8_a9)
> - shmobile_setup_delay_hz(max_freq, 1, 3);
> - else if (is_a15 && !IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
> - shmobile_setup_delay_hz(max_freq, 2, 4);
> + if (!has_arch_timer || !IS_ENABLED(CONFIG_ARM_ARCH_TIMER)) {
> + if (is_a7_a8_a9)
> + shmobile_setup_delay_hz(max_freq, 1, 3);
> + else if (is_a15)
> + shmobile_setup_delay_hz(max_freq, 2, 4);
> + }
> }
>
> static void __init shmobile_late_time_init(void)
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] ARM: shmobile: Handle CA7 arch timer delay
2014-10-05 23:59 [PATCH] ARM: shmobile: Handle CA7 arch timer delay Magnus Damm
2014-10-06 23:59 ` Simon Horman
@ 2014-10-07 0:50 ` Khiem Nguyen
2014-10-07 3:25 ` Magnus Damm
` (3 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Khiem Nguyen @ 2014-10-07 0:50 UTC (permalink / raw)
To: linux-sh
Hi Magnus-san,
Thanks for your patch.
I have 2 points to confirm.
Please kindly share your opinions.
On 10/6/2014 8:59 AM, Magnus Damm wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
>
> Update the delay code to include arch timer checks
> for CA7. From a arch timer availability perspective
> CA7 should be treated same as CA15.
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> ---
>
> Written against renesas-devel-20141002-v3.17-rc7
>
> arch/arm/mach-shmobile/timer.c | 23 +++++++++++++++--------
> 1 file changed, 15 insertions(+), 8 deletions(-)
>
> --- 0001/arch/arm/mach-shmobile/timer.c
> +++ work/arch/arm/mach-shmobile/timer.c 2014-10-06 08:44:21.000000000 +0900
> @@ -45,6 +45,7 @@ void __init shmobile_init_delay(void)
> struct device_node *np, *cpus;
> bool is_a7_a8_a9 = false;
> bool is_a15 = false;
> + bool has_arch_timer = false;
> u32 max_freq = 0;
>
> cpus = of_find_node_by_path("/cpus");
> @@ -57,12 +58,16 @@ void __init shmobile_init_delay(void)
> if (!of_property_read_u32(np, "clock-frequency", &freq))
> max_freq = max(max_freq, freq);
>
> - 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"))
> + if (of_device_is_compatible(np, "arm,cortex-a8") ||
> + of_device_is_compatible(np, "arm,cortex-a9")) {
> is_a7_a8_a9 = true;
> - else if (of_device_is_compatible(np, "arm,cortex-a15"))
> + } else if (of_device_is_compatible(np, "arm,cortex-a7")) {
> + is_a7_a8_a9 = true;
> + has_arch_timer = true;
> + } else if (of_device_is_compatible(np, "arm,cortex-a15")) {
> is_a15 = true;
> + has_arch_timer = true;
Because has_arch_timer set true here, below delay setting for a15 will never be entered.
However, before this patch, it will be executed in case CONFIG_ARM_ARCH_TIMER is disable.
Is it your intention ?
> + }
> }
>
> of_node_put(cpus);
> @@ -70,10 +75,12 @@ void __init shmobile_init_delay(void)
> if (!max_freq)
> return;
>
> - if (is_a7_a8_a9)
> - shmobile_setup_delay_hz(max_freq, 1, 3);
> - else if (is_a15 && !IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
> - shmobile_setup_delay_hz(max_freq, 2, 4);
> + if (!has_arch_timer || !IS_ENABLED(CONFIG_ARM_ARCH_TIMER)) {
Above condition checking will apply to all cortex cores, including a8 and a9.
However, before this patch, delay setting will be set for a8 and a9,
regardless of CONFIG_ARM_ARCH_TIMER.
Is it your intention ?
> + if (is_a7_a8_a9)
> + shmobile_setup_delay_hz(max_freq, 1, 3);
> + else if (is_a15)
> + shmobile_setup_delay_hz(max_freq, 2, 4);
> + }
> }
>
> static void __init shmobile_late_time_init(void)
> --
> 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
>
--
Best regards,
KHIEM Nguyen
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] ARM: shmobile: Handle CA7 arch timer delay
2014-10-05 23:59 [PATCH] ARM: shmobile: Handle CA7 arch timer delay Magnus Damm
2014-10-06 23:59 ` Simon Horman
2014-10-07 0:50 ` Khiem Nguyen
@ 2014-10-07 3:25 ` Magnus Damm
2014-10-07 3:32 ` Magnus Damm
` (2 subsequent siblings)
5 siblings, 0 replies; 7+ messages in thread
From: Magnus Damm @ 2014-10-07 3:25 UTC (permalink / raw)
To: linux-sh
Hi Simon,
On Tue, Oct 7, 2014 at 8:59 AM, Simon Horman <horms@verge.net.au> wrote:
> On Mon, Oct 06, 2014 at 08:59:20AM +0900, Magnus Damm wrote:
>> From: Magnus Damm <damm+renesas@opensource.se>
>>
>> Update the delay code to include arch timer checks
>> for CA7. From a arch timer availability perspective
>> CA7 should be treated same as CA15.
>>
>> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>> ---
>>
>> Written against renesas-devel-20141002-v3.17-rc7
>
> Thanks Magnus, I have queued this up.
>
> There is an outstanding question from Arnd
> relating to the last round of init delay updates.
> If you have a moment could you look into it?
>
> * Re: [GIT PULL] Renesas ARM Based SoC Init Delay Updates For v3.18
> http://www.spinics.net/lists/arm-kernel/msg360580.html
Thanks, I will. It's been on my TODO for quite some time now!
Cheers,
/ magnus
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] ARM: shmobile: Handle CA7 arch timer delay
2014-10-05 23:59 [PATCH] ARM: shmobile: Handle CA7 arch timer delay Magnus Damm
` (2 preceding siblings ...)
2014-10-07 3:25 ` Magnus Damm
@ 2014-10-07 3:32 ` Magnus Damm
2014-10-07 4:17 ` Khiem Nguyen
2014-10-07 5:52 ` Magnus Damm
5 siblings, 0 replies; 7+ messages in thread
From: Magnus Damm @ 2014-10-07 3:32 UTC (permalink / raw)
To: linux-sh
Hi Khiem-san,
On Tue, Oct 7, 2014 at 9:50 AM, Khiem Nguyen
<khiem.nguyen.xt@renesas.com> wrote:
> Hi Magnus-san,
>
>
>
> Thanks for your patch.
Thanks for your email.
> I have 2 points to confirm.
>
> Please kindly share your opinions.
Sure.
> On 10/6/2014 8:59 AM, Magnus Damm wrote:
>> From: Magnus Damm <damm+renesas@opensource.se>
>>
>> Update the delay code to include arch timer checks
>> for CA7. From a arch timer availability perspective
>> CA7 should be treated same as CA15.
>
>> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>> ---
>>
>> Written against renesas-devel-20141002-v3.17-rc7
>>
>> arch/arm/mach-shmobile/timer.c | 23 +++++++++++++++--------
>> 1 file changed, 15 insertions(+), 8 deletions(-)
>>
>> --- 0001/arch/arm/mach-shmobile/timer.c
>> +++ work/arch/arm/mach-shmobile/timer.c 2014-10-06 08:44:21.000000000 +0900
>> @@ -45,6 +45,7 @@ void __init shmobile_init_delay(void)
>> struct device_node *np, *cpus;
>> bool is_a7_a8_a9 = false;
>> bool is_a15 = false;
>> + bool has_arch_timer = false;
>> u32 max_freq = 0;
>>
>> cpus = of_find_node_by_path("/cpus");
>> @@ -57,12 +58,16 @@ void __init shmobile_init_delay(void)
>> if (!of_property_read_u32(np, "clock-frequency", &freq))
>> max_freq = max(max_freq, freq);
>>
>> - 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"))
>> + if (of_device_is_compatible(np, "arm,cortex-a8") ||
>> + of_device_is_compatible(np, "arm,cortex-a9")) {
>> is_a7_a8_a9 = true;
>> - else if (of_device_is_compatible(np, "arm,cortex-a15"))
>> + } else if (of_device_is_compatible(np, "arm,cortex-a7")) {
>> + is_a7_a8_a9 = true;
>> + has_arch_timer = true;
>> + } else if (of_device_is_compatible(np, "arm,cortex-a15")) {
>> is_a15 = true;
>> + has_arch_timer = true;
>
> Because has_arch_timer set true here, below delay setting for a15 will never be entered.
> However, before this patch, it will be executed in case CONFIG_ARM_ARCH_TIMER is disable.
> Is it your intention ?
No, it is not my intention. "has_arch_timer" here is used to tell if
the arch timer hardware may be available. This only happens on CA7 and
CA15.
>> + }
>> }
>>
>> of_node_put(cpus);
>> @@ -70,10 +75,12 @@ void __init shmobile_init_delay(void)
>> if (!max_freq)
>> return;
>>
>> - if (is_a7_a8_a9)
>> - shmobile_setup_delay_hz(max_freq, 1, 3);
>> - else if (is_a15 && !IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
>> - shmobile_setup_delay_hz(max_freq, 2, 4);
>> + if (!has_arch_timer || !IS_ENABLED(CONFIG_ARM_ARCH_TIMER)) {
>
> Above condition checking will apply to all cortex cores, including a8 and a9.
> However, before this patch, delay setting will be set for a8 and a9,
> regardless of CONFIG_ARM_ARCH_TIMER.
> Is it your intention ?
My intention is to only call shmobile_setup_delay_hz() in the following cases:
A) arch timer hardware is unavailable (has_arch_timer is false)
or
B) arch timer is available (has_arch_timer is true) and
CONFIG_ARM_ARCH_TIMER is disabled.
Case A) above covers CPU cores without arch timer, like CA8 and CA9.
Is there any problem?
Cheers,
/ magnus
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] ARM: shmobile: Handle CA7 arch timer delay
2014-10-05 23:59 [PATCH] ARM: shmobile: Handle CA7 arch timer delay Magnus Damm
` (3 preceding siblings ...)
2014-10-07 3:32 ` Magnus Damm
@ 2014-10-07 4:17 ` Khiem Nguyen
2014-10-07 5:52 ` Magnus Damm
5 siblings, 0 replies; 7+ messages in thread
From: Khiem Nguyen @ 2014-10-07 4:17 UTC (permalink / raw)
To: linux-sh
Hi Magnus-san,
On 10/7/2014 12:32 PM, Magnus Damm wrote:
> Hi Khiem-san,
>
> On Tue, Oct 7, 2014 at 9:50 AM, Khiem Nguyen
> <khiem.nguyen.xt@renesas.com> wrote:
>> Hi Magnus-san,
>>
>>
>>
>> Thanks for your patch.
>
> Thanks for your email.
>
>> I have 2 points to confirm.
>>
>> Please kindly share your opinions.
>
> Sure.
>
>> On 10/6/2014 8:59 AM, Magnus Damm wrote:
>>> From: Magnus Damm <damm+renesas@opensource.se>
>>>
>>> Update the delay code to include arch timer checks
>>> for CA7. From a arch timer availability perspective
>>> CA7 should be treated same as CA15.
>>
>>> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>>> ---
>>>
>>> Written against renesas-devel-20141002-v3.17-rc7
>>>
>>> arch/arm/mach-shmobile/timer.c | 23 +++++++++++++++--------
>>> 1 file changed, 15 insertions(+), 8 deletions(-)
>>>
>>> --- 0001/arch/arm/mach-shmobile/timer.c
>>> +++ work/arch/arm/mach-shmobile/timer.c 2014-10-06 08:44:21.000000000 +0900
>>> @@ -45,6 +45,7 @@ void __init shmobile_init_delay(void)
>>> struct device_node *np, *cpus;
>>> bool is_a7_a8_a9 = false;
>>> bool is_a15 = false;
>>> + bool has_arch_timer = false;
>>> u32 max_freq = 0;
>>>
>>> cpus = of_find_node_by_path("/cpus");
>>> @@ -57,12 +58,16 @@ void __init shmobile_init_delay(void)
>>> if (!of_property_read_u32(np, "clock-frequency", &freq))
>>> max_freq = max(max_freq, freq);
>>>
>>> - 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"))
>>> + if (of_device_is_compatible(np, "arm,cortex-a8") ||
>>> + of_device_is_compatible(np, "arm,cortex-a9")) {
>>> is_a7_a8_a9 = true;
>>> - else if (of_device_is_compatible(np, "arm,cortex-a15"))
>>> + } else if (of_device_is_compatible(np, "arm,cortex-a7")) {
>>> + is_a7_a8_a9 = true;
>>> + has_arch_timer = true;
>>> + } else if (of_device_is_compatible(np, "arm,cortex-a15")) {
>>> is_a15 = true;
>>> + has_arch_timer = true;
>>
>> Because has_arch_timer set true here, below delay setting for a15 will never be entered.
>> However, before this patch, it will be executed in case CONFIG_ARM_ARCH_TIMER is disable.
>> Is it your intention ?
>
> No, it is not my intention. "has_arch_timer" here is used to tell if
> the arch timer hardware may be available. This only happens on CA7 and
> CA15.
>
>>> + }
>>> }
>>>
>>> of_node_put(cpus);
>>> @@ -70,10 +75,12 @@ void __init shmobile_init_delay(void)
>>> if (!max_freq)
>>> return;
>>>
>>> - if (is_a7_a8_a9)
>>> - shmobile_setup_delay_hz(max_freq, 1, 3);
>>> - else if (is_a15 && !IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
>>> - shmobile_setup_delay_hz(max_freq, 2, 4);
>>> + if (!has_arch_timer || !IS_ENABLED(CONFIG_ARM_ARCH_TIMER)) {
>>
>> Above condition checking will apply to all cortex cores, including a8 and a9.
>> However, before this patch, delay setting will be set for a8 and a9,
>> regardless of CONFIG_ARM_ARCH_TIMER.
>> Is it your intention ?
>
> My intention is to only call shmobile_setup_delay_hz() in the following cases:
>
> A) arch timer hardware is unavailable (has_arch_timer is false)
> or
> B) arch timer is available (has_arch_timer is true) and
> CONFIG_ARM_ARCH_TIMER is disabled.
>
> Case A) above covers CPU cores without arch timer, like CA8 and CA9.
>
> Is there any problem?
Your implementation is OK.
At the beginning, I confused the meaning of using 'has_arch_timer'
and CONFIG_ARM_ARCH_TIMER.
Now, I understood the implementation via your explanation.
> Cheers,
>
> / magnus
>
--
Best regards,
KHIEM Nguyen
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] ARM: shmobile: Handle CA7 arch timer delay
2014-10-05 23:59 [PATCH] ARM: shmobile: Handle CA7 arch timer delay Magnus Damm
` (4 preceding siblings ...)
2014-10-07 4:17 ` Khiem Nguyen
@ 2014-10-07 5:52 ` Magnus Damm
5 siblings, 0 replies; 7+ messages in thread
From: Magnus Damm @ 2014-10-07 5:52 UTC (permalink / raw)
To: linux-sh
Hi Khiem-san,
On Tue, Oct 7, 2014 at 1:17 PM, Khiem Nguyen
<khiem.nguyen.xt@renesas.com> wrote:
> Hi Magnus-san,
>
> On 10/7/2014 12:32 PM, Magnus Damm wrote:
>> Hi Khiem-san,
>>
>> On Tue, Oct 7, 2014 at 9:50 AM, Khiem Nguyen
>> <khiem.nguyen.xt@renesas.com> wrote:
>>> Hi Magnus-san,
>>>
>>>
>>>
>>> Thanks for your patch.
>>
>> Thanks for your email.
>>
>>> I have 2 points to confirm.
>>>
>>> Please kindly share your opinions.
>>
>> Sure.
>>
>>> On 10/6/2014 8:59 AM, Magnus Damm wrote:
>>>> From: Magnus Damm <damm+renesas@opensource.se>
>>>>
>>>> Update the delay code to include arch timer checks
>>>> for CA7. From a arch timer availability perspective
>>>> CA7 should be treated same as CA15.
>>>
>>>> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>>>> ---
>>>>
>>>> Written against renesas-devel-20141002-v3.17-rc7
>>>>
>>>> arch/arm/mach-shmobile/timer.c | 23 +++++++++++++++--------
>>>> 1 file changed, 15 insertions(+), 8 deletions(-)
>>>>
>>>> --- 0001/arch/arm/mach-shmobile/timer.c
>>>> +++ work/arch/arm/mach-shmobile/timer.c 2014-10-06 08:44:21.000000000 +0900
>>>> @@ -45,6 +45,7 @@ void __init shmobile_init_delay(void)
>>>> struct device_node *np, *cpus;
>>>> bool is_a7_a8_a9 = false;
>>>> bool is_a15 = false;
>>>> + bool has_arch_timer = false;
>>>> u32 max_freq = 0;
>>>>
>>>> cpus = of_find_node_by_path("/cpus");
>>>> @@ -57,12 +58,16 @@ void __init shmobile_init_delay(void)
>>>> if (!of_property_read_u32(np, "clock-frequency", &freq))
>>>> max_freq = max(max_freq, freq);
>>>>
>>>> - 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"))
>>>> + if (of_device_is_compatible(np, "arm,cortex-a8") ||
>>>> + of_device_is_compatible(np, "arm,cortex-a9")) {
>>>> is_a7_a8_a9 = true;
>>>> - else if (of_device_is_compatible(np, "arm,cortex-a15"))
>>>> + } else if (of_device_is_compatible(np, "arm,cortex-a7")) {
>>>> + is_a7_a8_a9 = true;
>>>> + has_arch_timer = true;
>>>> + } else if (of_device_is_compatible(np, "arm,cortex-a15")) {
>>>> is_a15 = true;
>>>> + has_arch_timer = true;
>>>
>>> Because has_arch_timer set true here, below delay setting for a15 will never be entered.
>>> However, before this patch, it will be executed in case CONFIG_ARM_ARCH_TIMER is disable.
>>> Is it your intention ?
>>
>> No, it is not my intention. "has_arch_timer" here is used to tell if
>> the arch timer hardware may be available. This only happens on CA7 and
>> CA15.
>>
>>>> + }
>>>> }
>>>>
>>>> of_node_put(cpus);
>>>> @@ -70,10 +75,12 @@ void __init shmobile_init_delay(void)
>>>> if (!max_freq)
>>>> return;
>>>>
>>>> - if (is_a7_a8_a9)
>>>> - shmobile_setup_delay_hz(max_freq, 1, 3);
>>>> - else if (is_a15 && !IS_ENABLED(CONFIG_ARM_ARCH_TIMER))
>>>> - shmobile_setup_delay_hz(max_freq, 2, 4);
>>>> + if (!has_arch_timer || !IS_ENABLED(CONFIG_ARM_ARCH_TIMER)) {
>>>
>>> Above condition checking will apply to all cortex cores, including a8 and a9.
>>> However, before this patch, delay setting will be set for a8 and a9,
>>> regardless of CONFIG_ARM_ARCH_TIMER.
>>> Is it your intention ?
>>
>> My intention is to only call shmobile_setup_delay_hz() in the following cases:
>>
>> A) arch timer hardware is unavailable (has_arch_timer is false)
>> or
>> B) arch timer is available (has_arch_timer is true) and
>> CONFIG_ARM_ARCH_TIMER is disabled.
>>
>> Case A) above covers CPU cores without arch timer, like CA8 and CA9.
>>
>> Is there any problem?
>
> Your implementation is OK.
> At the beginning, I confused the meaning of using 'has_arch_timer'
> and CONFIG_ARM_ARCH_TIMER.
> Now, I understood the implementation via your explanation.
Ok, good! Thanks for checking!
Cheers,
/ magnus
^ permalink raw reply [flat|nested] 7+ messages in thread