On Mon, Feb 02, 2015 at 12:45:45PM +0000, Hanjun Guo wrote:
> Using the information presented by GTDT (Generic Timer Description Table)
> to initialize the arch timer (not memory-mapped).
Why are you not initializing the memory mapped timer ?
> CC: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> Originally-by: Amit Daniel Kachhap <amit.daniel@xxxxxxxxxxx>
> Tested-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@xxxxxxx>
> Tested-by: Yijing Wang <wangyijing@xxxxxxxxxx>
> Tested-by: Mark Langsdorf <mlangsdo@xxxxxxxxxx>
> Tested-by: Jon Masters <jcm@xxxxxxxxxx>
> Tested-by: Timur Tabi <timur@xxxxxxxxxxxxxx>
> Signed-off-by: Hanjun Guo <hanjun.guo@xxxxxxxxxx>
> ---
> arch/arm64/kernel/time.c | 7 ++
> drivers/clocksource/arm_arch_timer.c | 132 ++++++++++++++++++++++++++++-------
> include/linux/clocksource.h | 6 ++
> 3 files changed, 118 insertions(+), 27 deletions(-)
>
> diff --git a/arch/arm64/kernel/time.c b/arch/arm64/kernel/time.c
> index 1a7125c..42f9195 100644
> --- a/arch/arm64/kernel/time.c
> +++ b/arch/arm64/kernel/time.c
> @@ -35,6 +35,7 @@
> #include <linux/delay.h>
> #include <linux/clocksource.h>
> #include <linux/clk-provider.h>
> +#include <linux/acpi.h>
>
> #include <clocksource/arm_arch_timer.h>
>
> @@ -72,6 +73,12 @@ void __init time_init(void)
>
> tick_setup_hrtimer_broadcast();
>
> + /*
> + * Since ACPI or FDT will only one be available in the system,
> + * we can use acpi_generic_timer_init() here safely
> + */
> + acpi_generic_timer_init();
> +
> arch_timer_rate = arch_timer_get_rate();
> if (!arch_timer_rate)
> panic("Unable to initialise architected timer.\n");
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 095c177..407aa63 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>
> @@ -370,8 +371,12 @@ arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
> if (arch_timer_rate)
> return;
>
> - /* Try to determine the frequency from the device tree or CNTFRQ */
> - if (of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
> + /*
> + * Try to determine the frequency from the device tree or CNTFRQ,
> + * if ACPI is enabled, get the frequency from CNTFRQ ONLY.
> + */
> + if (!acpi_disabled ||
> + of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
This is getting a mess. cntbase tells you it is a memory mapped timer,
node pointer that you are probing through DT, and to top it all
acpi_disabled detects if you are probing in ACPI or DT mode.
I think this function should be simplified, this driver is also
pending a refactoring to split arch timer and the memory mapped one
so I think you'd better wait that work to make things simpler.
[...]
> +/* Initialize all the generic timers presented in GTDT */
> +void __init acpi_generic_timer_init(void)
> +{
> + if (acpi_disabled)
> + return;
acpi_disabled used again here, I repeat myself this is going to be
hard to track. You should try to organize the code something like:
if (acpi_disabled)
timer_dt_probe();
else
timer_acpi_probe();
mixing the code paths is getting unwieldy, see above to see my
reasoning.
> +
> + acpi_table_parse(ACPI_SIG_GTDT, arch_timer_acpi_init);
> +}
> +#endif
> diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
> index abcafaa..af6155a 100644
> --- a/include/linux/clocksource.h
> +++ b/include/linux/clocksource.h
> @@ -346,4 +346,10 @@ extern void clocksource_of_init(void);
> static inline void clocksource_of_init(void) {}
> #endif
>
> +#ifdef CONFIG_ACPI
> +void acpi_generic_timer_init(void);
> +#else
> +static inline void acpi_generic_timer_init(void) { }
> +#endif
> +
That's not nice, it is a generic header, arch specific stuff should be
avoided. I think you should have an ACPI generic layer similar to
clocksource_of_init(), and probe from there when matching the respective
timers.
Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
