lkml.org 
[lkml]   [2015]   [Feb]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v8 17/21] clocksource / arch_timer: Parse GTDT to initialize arch timer
    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@linaro.org>
    > Originally-by: Amit Daniel Kachhap <amit.daniel@samsung.com>
    > Tested-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
    > Tested-by: Yijing Wang <wangyijing@huawei.com>
    > Tested-by: Mark Langsdorf <mlangsdo@redhat.com>
    > Tested-by: Jon Masters <jcm@redhat.com>
    > Tested-by: Timur Tabi <timur@codeaurora.org>
    > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
    > ---
    > 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

    \
     
     \ /
      Last update: 2015-02-04 20:21    [W:2.484 / U:2.584 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site