| Message ID | 20141005235920.3532.40364.sendpatchset@w520 (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 27f3c70874fc1d114e26c807e900fec4b50b1217 |
| Headers | show |
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) > -- 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 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 >
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 -- 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, 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 -- 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, 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 >
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 -- 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
--- 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)