[RFC part3 PATCH 2/2] ARM64 / clocksource: Use arch_timer_acpi_init()

[RFC part3 PATCH 2/2] ARM64 / clocksource: Use arch_timer_acpi_init()

[Linus Walleij]

On Tue, Dec 3, 2013 at 12:15 PM, Hanjun Guo <hanjun.guo@linaro.org> wrote:

> +       /* if can't be initialised from DT, try ACPI way */
> +       if (!arch_timer_get_rate())
> +               arch_timer_acpi_init();
> +
>         arch_timer_rate = arch_timer_get_rate();

This looks a bit fragile. Having a call like arch_timer_get_rate()
to check whether there is a DT node for the timer doesn't seem
right, can you refactor the code to provide some
has_arch_timer_node() or similar call instead, so it's a bit easier
to understand & maintain at least?

Yours,
Linus Walleij

[RFC part3 PATCH 2/2] ARM64 / clocksource: Use arch_timer_acpi_init()

[Hanjun Guo]

On 2013?12?03? 20:27, Linus Walleij wrote:
> On Tue, Dec 3, 2013 at 12:15 PM, Hanjun Guo <hanjun.guo@linaro.org> wrote:
>
>> +       /* if can't be initialised from DT, try ACPI way */
>> +       if (!arch_timer_get_rate())
>> +               arch_timer_acpi_init();
>> +
>>          arch_timer_rate = arch_timer_get_rate();
> This looks a bit fragile. Having a call like arch_timer_get_rate()
> to check whether there is a DT node for the timer doesn't seem
> right, can you refactor the code to provide some
> has_arch_timer_node() or similar call instead, so it's a bit easier
> to understand & maintain at least?

Good point, thanks for the guidance.
I will introduce has_arch_timer_node() as you said and use
it as follows:

if (has_arch_timer_node())
clocksource_of_init();
esle
arch_timer_acpi_init(); /* try ACPI way */

Is this make sense to you?

Thanks
Hanjun

[RFC part3 PATCH 2/2] ARM64 / clocksource: Use arch_timer_acpi_init()

[Linus Walleij]

On Tue, Dec 3, 2013 at 2:52 PM, Hanjun Guo <hanjun.guo@linaro.org> wrote:

> I will introduce has_arch_timer_node() as you said and use
> it as follows:
>
> if (has_arch_timer_node())
> clocksource_of_init();
> esle
> arch_timer_acpi_init(); /* try ACPI way */
>
> Is this make sense to you?

Sure, go head.

Thanks,
Linus Walleij

[RFC part3 PATCH 2/2] ARM64 / clocksource: Use arch_timer_acpi_init()

[Mark Rutland]

On Tue, Dec 03, 2013 at 02:13:49PM +0000, Linus Walleij wrote:
> On Tue, Dec 3, 2013 at 2:52 PM, Hanjun Guo <hanjun.guo@linaro.org> wrote:
> 
> > I will introduce has_arch_timer_node() as you said and use
> > it as follows:
> >
> > if (has_arch_timer_node())
> > clocksource_of_init();
> > esle
> > arch_timer_acpi_init(); /* try ACPI way */
> >
> > Is this make sense to you?

What does arch_timer_acpi_init() do? Is it just detecting the presence
of the timer, or grabbing the rate from a property in an ACPI table?

Mark.

[RFC part3 PATCH 2/2] ARM64 / clocksource: Use arch_timer_acpi_init()

[Hanjun Guo]

On 2013?12?03? 22:43, Mark Rutland wrote:
> On Tue, Dec 03, 2013 at 02:13:49PM +0000, Linus Walleij wrote:
>> On Tue, Dec 3, 2013 at 2:52 PM, Hanjun Guo <hanjun.guo@linaro.org> wrote:
>>
>>> I will introduce has_arch_timer_node() as you said and use
>>> it as follows:
>>>
>>> if (has_arch_timer_node())
>>> clocksource_of_init();
>>> esle
>>> arch_timer_acpi_init(); /* try ACPI way */
>>>
>>> Is this make sense to you?
> What does arch_timer_acpi_init() do? Is it just detecting the presence
> of the timer, or grabbing the rate from a property in an ACPI table?

It seems that you didn't get the PATCH 1/2, and my part1/part2 patch set
is also missing, I will resend all the patch set, sorry for the noise.

Thanks
Hanjun

[RFC part3 PATCH 2/2] ARM64 / clocksource: Use arch_timer_acpi_init()

[Hanjun Guo]

Use arch_timer_acpi_init() on ARM64 to initialise arch timer
in ACPI way when DT is not available.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 arch/arm64/kernel/time.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c
index 29c39d5..fb009da 100644
--- a/arch/arm64/kernel/time.c
+++ b/arch/arm64/kernel/time.c
@@ -67,6 +67,10 @@ void __init time_init(void)
 
 	clocksource_of_init();
 
+	/* if can't be initialised from DT, try ACPI way */
+	if (!arch_timer_get_rate())
+		arch_timer_acpi_init();
+
 	arch_timer_rate = arch_timer_get_rate();
 	if (!arch_timer_rate)
 		panic("Unable to initialise architected timer.\n");
-- 
1.7.9.5

[RFC part3 PATCH 2/2] ARM64 / clocksource: Use arch_timer_acpi_init()

[Mark Rutland]

On Tue, Dec 03, 2013 at 04:41:31PM +0000, Hanjun Guo wrote:
> Use arch_timer_acpi_init() on ARM64 to initialise arch timer
> in ACPI way when DT is not available.
> 
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> ---
>  arch/arm64/kernel/time.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c
> index 29c39d5..fb009da 100644
> --- a/arch/arm64/kernel/time.c
> +++ b/arch/arm64/kernel/time.c
> @@ -67,6 +67,10 @@ void __init time_init(void)
>  
>  	clocksource_of_init();
>  
> +	/* if can't be initialised from DT, try ACPI way */
> +	if (!arch_timer_get_rate())
> +		arch_timer_acpi_init();
> +

As mentioned on the previous patch, I think for the timebeing we should
rely on CNTFREQ.

Additionally, if you need to do this we should have an analagous
mechanism to clocksource_of_init() that performs this initialisation for
ACPI, and here we can call a clocksource_*_init function that does the
right thing.

There's no need for this file to know anything about ACPI.

Mark.

[RFC part3 PATCH 2/2] ARM64 / clocksource: Use arch_timer_acpi_init()

[Hanjun Guo]

On 2013?12?04? 01:08, Mark Rutland wrote:
> On Tue, Dec 03, 2013 at 04:41:31PM +0000, Hanjun Guo wrote:
>> Use arch_timer_acpi_init() on ARM64 to initialise arch timer
>> in ACPI way when DT is not available.
>>
>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>> ---
>>   arch/arm64/kernel/time.c |    4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c
>> index 29c39d5..fb009da 100644
>> --- a/arch/arm64/kernel/time.c
>> +++ b/arch/arm64/kernel/time.c
>> @@ -67,6 +67,10 @@ void __init time_init(void)
>>   
>>   	clocksource_of_init();
>>   
>> +	/* if can't be initialised from DT, try ACPI way */
>> +	if (!arch_timer_get_rate())
>> +		arch_timer_acpi_init();
>> +
> As mentioned on the previous patch, I think for the timebeing we should
> rely on CNTFREQ.
>
> Additionally, if you need to do this we should have an analagous
> mechanism to clocksource_of_init() that performs this initialisation for
> ACPI, and here we can call a clocksource_*_init function that does the
> right thing.
>
> There's no need for this file to know anything about ACPI.

Oh, Amit already have some patches to introduce clocksource_acpi_init()
like clocksource_of_init() did, please refer to the link below.

http://marc.info/?l=linaro-acpi&m=138131929721943&w=2

is this the idea you mentioned in your comments?

Thanks
Hanjun

[RFC part3 PATCH 2/2] ARM64 / clocksource: Use arch_timer_acpi_init()

[Mark Rutland]

On Wed, Dec 04, 2013 at 02:27:22PM +0000, Hanjun Guo wrote:
> On 2013?12?04? 01:08, Mark Rutland wrote:
> > On Tue, Dec 03, 2013 at 04:41:31PM +0000, Hanjun Guo wrote:
> >> Use arch_timer_acpi_init() on ARM64 to initialise arch timer
> >> in ACPI way when DT is not available.
> >>
> >> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> >> ---
> >>   arch/arm64/kernel/time.c |    4 ++++
> >>   1 file changed, 4 insertions(+)
> >>
> >> diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c
> >> index 29c39d5..fb009da 100644
> >> --- a/arch/arm64/kernel/time.c
> >> +++ b/arch/arm64/kernel/time.c
> >> @@ -67,6 +67,10 @@ void __init time_init(void)
> >>   
> >>   	clocksource_of_init();
> >>   
> >> +	/* if can't be initialised from DT, try ACPI way */
> >> +	if (!arch_timer_get_rate())
> >> +		arch_timer_acpi_init();
> >> +
> > As mentioned on the previous patch, I think for the timebeing we should
> > rely on CNTFREQ.
> >
> > Additionally, if you need to do this we should have an analagous
> > mechanism to clocksource_of_init() that performs this initialisation for
> > ACPI, and here we can call a clocksource_*_init function that does the
> > right thing.
> >
> > There's no need for this file to know anything about ACPI.
> 
> Oh, Amit already have some patches to introduce clocksource_acpi_init()
> like clocksource_of_init() did, please refer to the link below.
> 
> http://marc.info/?l=linaro-acpi&m=138131929721943&w=2
> 
> is this the idea you mentioned in your comments?

Something along those lines is far better than hardcoding
arch_timer_acpi_init here.

Thanks,
Mark.

[RFC part3 PATCH 1/2] clocksource / arch_timer: Use ACPI GTDT table to initialize arch timer

[Rob Herring]

On Tue, Dec 3, 2013 at 5:15 AM, Hanjun Guo <hanjun.guo@linaro.org> wrote:
> ACPI GTDT (Generic Timer Description Table) contains information for
> arch timer initialization, this patch use this table to probe arm timer.
>
> GTDT table is used for ARM/ARM64 only, please refer to chapter 5.2.24
> of ACPI 5.0 spec for detailed inforamtion
>
> Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> ---
>  drivers/clocksource/arm_arch_timer.c |  129 ++++++++++++++++++++++++++++++----
>  include/clocksource/arm_arch_timer.h |    7 +-
>  2 files changed, 120 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 95fb944..c968041 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -21,6 +21,7 @@
>  #include <linux/io.h>
>  #include <linux/slab.h>
>  #include <linux/sched_clock.h>
> +#include <linux/acpi.h>
>
>  #include <asm/arch_timer.h>
>  #include <asm/virt.h>
> @@ -632,20 +633,8 @@ static void __init arch_timer_common_init(void)
>         arch_timer_arch_init();
>  }
>
> -static void __init arch_timer_init(struct device_node *np)
> +static void __init arch_timer_init(void)
>  {
> -       int i;
> -
> -       if (arch_timers_present & ARCH_CP15_TIMER) {
> -               pr_warn("arch_timer: multiple nodes in dt, skipping\n");
> -               return;
> -       }
> -
> -       arch_timers_present |= ARCH_CP15_TIMER;
> -       for (i = PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++)
> -               arch_timer_ppi[i] = irq_of_parse_and_map(np, i);
> -       arch_timer_detect_rate(NULL, np);
> -
>         /*
>          * If HYP mode is available, we know that the physical timer
>          * has been configured to be accessible from PL1. Use it, so
> @@ -667,8 +656,118 @@ static void __init arch_timer_init(struct device_node *np)
>         arch_timer_register();
>         arch_timer_common_init();
>  }
> -CLOCKSOURCE_OF_DECLARE(armv7_arch_timer, "arm,armv7-timer", arch_timer_init);
> -CLOCKSOURCE_OF_DECLARE(armv8_arch_timer, "arm,armv8-timer", arch_timer_init);
> +
> +static void __init arch_timer_of_init(struct device_node *np)
> +{
> +       int i;
> +
> +       if (arch_timers_present & ARCH_CP15_TIMER) {
> +               pr_warn("arch_timer: multiple nodes in dt, skipping\n");
> +               return;
> +       }
> +
> +       arch_timers_present |= ARCH_CP15_TIMER;
> +       for (i = PHYS_SECURE_PPI; i < MAX_TIMER_PPI; i++)
> +               arch_timer_ppi[i] = irq_of_parse_and_map(np, i);
> +       arch_timer_detect_rate(NULL, np);
> +
> +       arch_timer_init();
> +}
> +CLOCKSOURCE_OF_DECLARE(armv7_arch_timer, "arm,armv7-timer", arch_timer_of_init);
> +CLOCKSOURCE_OF_DECLARE(armv8_arch_timer, "arm,armv8-timer", arch_timer_of_init);
> +
> +#ifdef CONFIG_ACPI
> +void __init arch_timer_acpi_init(void)
> +{
> +       struct acpi_table_gtdt *gtdt;
> +       acpi_size tbl_size;
> +       int trigger, polarity;
> +       void __iomem *base = NULL;
> +
> +       if (acpi_disabled)

Wouldn't the core ACPI code never call this function if ACPI is disabled?

> +               return;
> +
> +       if (arch_timers_present & ARCH_CP15_TIMER) {
> +               pr_warn("arch_timer: already initialized, skipping\n");
> +               return;
> +       }
> +
> +       if (ACPI_FAILURE(acpi_get_table_with_size(ACPI_SIG_GTDT, 0,
> +                       (struct acpi_table_header **)&gtdt, &tbl_size))) {
> +               pr_err("arch_timer: GTDT table not defined\n");
> +               return;
> +       }
> +
> +       arch_timers_present |= ARCH_CP15_TIMER;

So you have marked the timer as initialized, but then may fail on
error later on here.

> +
> +       /*
> +        * Get the timer frequency. Since there is no frequency info
> +        * in the GTDT table, so we should read it from CNTFREG register
> +        * or hard code here to wait for the new ACPI spec available.
> +        */
> +       if (!gtdt->address) {
> +               arch_timer_rate = arch_timer_get_cntfrq();
> +       } else {
> +               base = ioremap(gtdt->address, CNTFRQ);
> +               if (!base) {
> +                       pr_warn("arch_timer: unable to map arch timer base address\n");
> +                       return;
> +               }
> +
> +               arch_timer_rate = readl_relaxed(base + CNTFRQ);
> +               iounmap(base);

This is for memory mapped timer? If so, then isn't setting
ARCH_CP15_TIMER the wrong thing to do?

> +       }
> +
> +       if (!arch_timer_rate) {
> +               /* Hard code here to set frequence ? */
> +               pr_warn("arch_timer: Could not get frequency from GTDT table or CNTFREG\n");
> +       }
> +
> +       if (gtdt->secure_pl1_interrupt) {

Really, I think the kernel should just ignore the secure interrupt.
The DT code has the same issue, but that doesn't affect the code size.

> +               trigger = (gtdt->secure_pl1_flags & ACPI_GTDT_INTERRUPT_MODE) ?
> +                       ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;

Why not use the already defined linux irq trigger types here and make
acpi_register_gsi use them?

> +               polarity =
> +                       (gtdt->secure_pl1_flags & ACPI_GTDT_INTERRUPT_POLARITY)
> +                       ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
> +               arch_timer_ppi[0] = acpi_register_gsi(NULL,
> +                               gtdt->secure_pl1_interrupt, trigger, polarity);
> +       }
> +       if (gtdt->non_secure_pl1_interrupt) {
> +               trigger =
> +                       (gtdt->non_secure_pl1_flags & ACPI_GTDT_INTERRUPT_MODE)
> +                       ? ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;
> +               polarity =
> +               (gtdt->non_secure_pl1_flags & ACPI_GTDT_INTERRUPT_POLARITY)
> +                       ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
> +               arch_timer_ppi[1] = acpi_register_gsi(NULL,
> +                       gtdt->non_secure_pl1_interrupt, trigger, polarity);
> +       }
> +       if (gtdt->virtual_timer_interrupt) {
> +               trigger = (gtdt->virtual_timer_flags & ACPI_GTDT_INTERRUPT_MODE)
> +                       ? ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;
> +               polarity =
> +               (gtdt->virtual_timer_flags & ACPI_GTDT_INTERRUPT_POLARITY)
> +                       ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
> +               arch_timer_ppi[2] = acpi_register_gsi(NULL,
> +                       gtdt->virtual_timer_interrupt, trigger, polarity);
> +       }
> +       if (gtdt->non_secure_pl2_interrupt) {
> +               trigger =
> +                       (gtdt->non_secure_pl2_flags & ACPI_GTDT_INTERRUPT_MODE)
> +                       ? ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;
> +               polarity =
> +               (gtdt->non_secure_pl2_flags & ACPI_GTDT_INTERRUPT_POLARITY)
> +                       ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
> +               arch_timer_ppi[3] = acpi_register_gsi(NULL,
> +                       gtdt->non_secure_pl2_interrupt, trigger, polarity);
> +       }
> +
> +       early_acpi_os_unmap_memory(gtdt, tbl_size);

Who did the mapping? acpi_get_table_with_size? I think the core code
should handle the mapping and unmapping of ACPI tables. We don't want
to have to duplicate this in every initialization function. This seems
error prone.

Rob

[RFC part3 PATCH 2/2] ARM64 / clocksource: Use arch_timer_acpi_init()

[Hanjun Guo]

On 2013?12?04? 23:07, Mark Rutland wrote:
> On Wed, Dec 04, 2013 at 02:27:22PM +0000, Hanjun Guo wrote:
>> On 2013?12?04? 01:08, Mark Rutland wrote:
>>> On Tue, Dec 03, 2013 at 04:41:31PM +0000, Hanjun Guo wrote:
>>>> Use arch_timer_acpi_init() on ARM64 to initialise arch timer
>>>> in ACPI way when DT is not available.
>>>>
>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>> ---
>>>>    arch/arm64/kernel/time.c |    4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c
>>>> index 29c39d5..fb009da 100644
>>>> --- a/arch/arm64/kernel/time.c
>>>> +++ b/arch/arm64/kernel/time.c
>>>> @@ -67,6 +67,10 @@ void __init time_init(void)
>>>>    
>>>>    	clocksource_of_init();
>>>>    
>>>> +	/* if can't be initialised from DT, try ACPI way */
>>>> +	if (!arch_timer_get_rate())
>>>> +		arch_timer_acpi_init();
>>>> +
>>> As mentioned on the previous patch, I think for the timebeing we should
>>> rely on CNTFREQ.
>>>
>>> Additionally, if you need to do this we should have an analagous
>>> mechanism to clocksource_of_init() that performs this initialisation for
>>> ACPI, and here we can call a clocksource_*_init function that does the
>>> right thing.
>>>
>>> There's no need for this file to know anything about ACPI.
>> Oh, Amit already have some patches to introduce clocksource_acpi_init()
>> like clocksource_of_init() did, please refer to the link below.
>>
>> http://marc.info/?l=linaro-acpi&m=138131929721943&w=2
>>
>> is this the idea you mentioned in your comments?
> Something along those lines is far better than hardcoding
> arch_timer_acpi_init here.

Thanks for the suggestion, will update it.

Hanjun

[RFC part3 PATCH 1/2] clocksource / arch_timer: Use ACPI GTDT table to initialize arch timer

[Hanjun Guo]

On 2013?12?04? 23:33, Rob Herring wrote:
> On Tue, Dec 3, 2013 at 5:15 AM, Hanjun Guo <hanjun.guo@linaro.org> wrote:
[...]
>> +#ifdef CONFIG_ACPI
>> +void __init arch_timer_acpi_init(void)
>> +{
>> +       struct acpi_table_gtdt *gtdt;
>> +       acpi_size tbl_size;
>> +       int trigger, polarity;
>> +       void __iomem *base = NULL;
>> +
>> +       if (acpi_disabled)
> Wouldn't the core ACPI code never call this function if ACPI is disabled?

You inspired me for patches to remove some redundant if (acpi_disabled)
check for the current ACPI code, but this function will be called even
ACPI is disabled.

>> +               return;
>> +
>> +       if (arch_timers_present & ARCH_CP15_TIMER) {
>> +               pr_warn("arch_timer: already initialized, skipping\n");
>> +               return;
>> +       }
>> +
>> +       if (ACPI_FAILURE(acpi_get_table_with_size(ACPI_SIG_GTDT, 0,
>> +                       (struct acpi_table_header **)&gtdt, &tbl_size))) {
>> +               pr_err("arch_timer: GTDT table not defined\n");
>> +               return;
>> +       }
>> +
>> +       arch_timers_present |= ARCH_CP15_TIMER;
> So you have marked the timer as initialized, but then may fail on
> error later on here.
>
>> +
>> +       /*
>> +        * Get the timer frequency. Since there is no frequency info
>> +        * in the GTDT table, so we should read it from CNTFREG register
>> +        * or hard code here to wait for the new ACPI spec available.
>> +        */
>> +       if (!gtdt->address) {
>> +               arch_timer_rate = arch_timer_get_cntfrq();
>> +       } else {
>> +               base = ioremap(gtdt->address, CNTFRQ);
>> +               if (!base) {
>> +                       pr_warn("arch_timer: unable to map arch timer base address\n");
>> +                       return;
>> +               }
>> +
>> +               arch_timer_rate = readl_relaxed(base + CNTFRQ);
>> +               iounmap(base);
> This is for memory mapped timer? If so, then isn't setting
> ARCH_CP15_TIMER the wrong thing to do?

I'm trying to do that but it is wrong as you said, I will remove above code
and only keep

arch_timer_rate = arch_timer_get_cntfrq() here.

>> +       }
>> +
>> +       if (!arch_timer_rate) {
>> +               /* Hard code here to set frequence ? */
>> +               pr_warn("arch_timer: Could not get frequency from GTDT table or CNTFREG\n");
>> +       }
>> +
>> +       if (gtdt->secure_pl1_interrupt) {
> Really, I think the kernel should just ignore the secure interrupt.
> The DT code has the same issue, but that doesn't affect the code size.
>
>> +               trigger = (gtdt->secure_pl1_flags & ACPI_GTDT_INTERRUPT_MODE) ?
>> +                       ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;
> Why not use the already defined linux irq trigger types here and make
> acpi_register_gsi use them?
>
>> +               polarity =
>> +                       (gtdt->secure_pl1_flags & ACPI_GTDT_INTERRUPT_POLARITY)
>> +                       ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
>> +               arch_timer_ppi[0] = acpi_register_gsi(NULL,
>> +                               gtdt->secure_pl1_interrupt, trigger, polarity);
>> +       }
>> +       if (gtdt->non_secure_pl1_interrupt) {
>> +               trigger =
>> +                       (gtdt->non_secure_pl1_flags & ACPI_GTDT_INTERRUPT_MODE)
>> +                       ? ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;
>> +               polarity =
>> +               (gtdt->non_secure_pl1_flags & ACPI_GTDT_INTERRUPT_POLARITY)
>> +                       ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
>> +               arch_timer_ppi[1] = acpi_register_gsi(NULL,
>> +                       gtdt->non_secure_pl1_interrupt, trigger, polarity);
>> +       }
>> +       if (gtdt->virtual_timer_interrupt) {
>> +               trigger = (gtdt->virtual_timer_flags & ACPI_GTDT_INTERRUPT_MODE)
>> +                       ? ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;
>> +               polarity =
>> +               (gtdt->virtual_timer_flags & ACPI_GTDT_INTERRUPT_POLARITY)
>> +                       ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
>> +               arch_timer_ppi[2] = acpi_register_gsi(NULL,
>> +                       gtdt->virtual_timer_interrupt, trigger, polarity);
>> +       }
>> +       if (gtdt->non_secure_pl2_interrupt) {
>> +               trigger =
>> +                       (gtdt->non_secure_pl2_flags & ACPI_GTDT_INTERRUPT_MODE)
>> +                       ? ACPI_EDGE_SENSITIVE : ACPI_LEVEL_SENSITIVE;
>> +               polarity =
>> +               (gtdt->non_secure_pl2_flags & ACPI_GTDT_INTERRUPT_POLARITY)
>> +                       ? ACPI_ACTIVE_LOW : ACPI_ACTIVE_HIGH;
>> +               arch_timer_ppi[3] = acpi_register_gsi(NULL,
>> +                       gtdt->non_secure_pl2_interrupt, trigger, polarity);
>> +       }
>> +
>> +       early_acpi_os_unmap_memory(gtdt, tbl_size);
> Who did the mapping? acpi_get_table_with_size? I think the core code
> should handle the mapping and unmapping of ACPI tables. We don't want
> to have to duplicate this in every initialization function. This seems
> error prone.

Yes, you are right, I will use the ACPI core function acpi_table_parse()
to fix it, thanks for you guidance.

Hanjun

[RFC part3 PATCH 2/2] ARM64 / clocksource: Use arch_timer_acpi_init()

[Olof Johansson]

On Tue, Dec 03, 2013 at 09:52:30PM +0800, Hanjun Guo wrote:
> On 2013?12?03? 20:27, Linus Walleij wrote:
> >On Tue, Dec 3, 2013 at 12:15 PM, Hanjun Guo <hanjun.guo@linaro.org> wrote:
> >
> >>+       /* if can't be initialised from DT, try ACPI way */
> >>+       if (!arch_timer_get_rate())
> >>+               arch_timer_acpi_init();
> >>+
> >>         arch_timer_rate = arch_timer_get_rate();
> >This looks a bit fragile. Having a call like arch_timer_get_rate()
> >to check whether there is a DT node for the timer doesn't seem
> >right, can you refactor the code to provide some
> >has_arch_timer_node() or similar call instead, so it's a bit easier
> >to understand & maintain at least?
> 
> Good point, thanks for the guidance.
> I will introduce has_arch_timer_node() as you said and use
> it as follows:
> 
> if (has_arch_timer_node())
> clocksource_of_init();
> esle
> arch_timer_acpi_init(); /* try ACPI way */
> 
> Is this make sense to you?

Even when we boot with ACPI, the boot stub will still create a minimal
DTB. We should just make sure that the clocksource (which will be
architectured timers anyway, I believe?) is described in that stub.

I would rather do that than have dual-path booting in the lowlevel setup, it
increases test requirements and makes it hard for someone without ACPI hardware
to check for regressions in this code, etc.


-Olof