On Friday, July 15, 2016 03:32:35 PM Fu Wei wrote:
> Hi Rafael,
>
>
>
> On 14 July 2016 at 04:30, Rafael J. Wysocki <rafael@xxxxxxxxxx> wrote:
> > On Wed, Jul 13, 2016 at 7:53 PM, <fu.wei@xxxxxxxxxx> wrote:
> >> From: Fu Wei <fu.wei@xxxxxxxxxx>
> >>
> >> This patch adds support for parsing arch timer in GTDT,
> >> provides some kernel APIs to parse all the PPIs and
> >> always-on info in GTDT and export them.
> >>
> >> By this driver, we can simplify arm_arch_timer drivers, and
> >> separate the ACPI GTDT knowledge from it.
> >>
> >> Signed-off-by: Fu Wei <fu.wei@xxxxxxxxxx>
> >> Signed-off-by: Hanjun Guo <hanjun.guo@xxxxxxxxxx>
> >> ---
> >> drivers/acpi/Kconfig | 5 ++
> >> drivers/acpi/Makefile | 1 +
> >> drivers/acpi/arm64/Kconfig | 15 ++++
> >> drivers/acpi/arm64/Makefile | 1 +
> >> drivers/acpi/arm64/acpi_gtdt.c | 170 +++++++++++++++++++++++++++++++++++++++++
> >> include/linux/acpi.h | 6 ++
> >> 6 files changed, 198 insertions(+)
> >>
> >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> >> index b7e2e77..1cdc7d2 100644
> >> --- a/drivers/acpi/Kconfig
> >> +++ b/drivers/acpi/Kconfig
> >> @@ -521,4 +521,9 @@ config XPOWER_PMIC_OPREGION
> >>
> >> endif
> >>
> >> +if ARM64
> >> +source "drivers/acpi/arm64/Kconfig"
> >> +
> >> +endif
> >> +
> >> endif # ACPI
> >> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> >> index 251ce85..1a94ff7 100644
> >> --- a/drivers/acpi/Makefile
> >> +++ b/drivers/acpi/Makefile
> >> @@ -99,5 +99,6 @@ obj-$(CONFIG_ACPI_EXTLOG) += acpi_extlog.o
> >> obj-$(CONFIG_PMIC_OPREGION) += pmic/intel_pmic.o
> >> obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o
> >> obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o
> >> +obj-$(CONFIG_ARM64) += arm64/
> >>
> >> video-objs += acpi_video.o video_detect.o
> >> diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig
> >> new file mode 100644
> >> index 0000000..ff5c253
> >> --- /dev/null
> >> +++ b/drivers/acpi/arm64/Kconfig
> >> @@ -0,0 +1,15 @@
> >> +#
> >> +# ACPI Configuration for ARM64
> >> +#
> >> +
> >> +menu "The ARM64-specific ACPI Support"
> >> +
> >> +config ACPI_GTDT
> >> + bool "ACPI GTDT table Support"
> >
> > This should depend on ARM64.
> >
> > Also I wonder if it needs to be user-selectable? Wouldn't it be
> > better to enable it by default when building for ARM64 with ACPI?
> >
> >> + help
> >> + GTDT (Generic Timer Description Table) provides information
> >> + for per-processor timers and Platform (memory-mapped) timers
> >> + for ARM platforms. Select this option to provide information
> >> + needed for the timers init.
> >> +
> >> +endmenu
> >> diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile
> >> new file mode 100644
> >> index 0000000..466de6b
> >> --- /dev/null
> >> +++ b/drivers/acpi/arm64/Makefile
> >> @@ -0,0 +1 @@
> >> +obj-$(CONFIG_ACPI_GTDT) += acpi_gtdt.o
> >> diff --git a/drivers/acpi/arm64/acpi_gtdt.c b/drivers/acpi/arm64/acpi_gtdt.c
> >> new file mode 100644
> >> index 0000000..9ee977d
> >> --- /dev/null
> >> +++ b/drivers/acpi/arm64/acpi_gtdt.c
> >> @@ -0,0 +1,170 @@
> >> +/*
> >> + * ARM Specific GTDT table Support
> >> + *
> >> + * Copyright (C) 2016, Linaro Ltd.
> >> + * Author: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> >> + * Fu Wei <fu.wei@xxxxxxxxxx>
> >> + * Hanjun Guo <hanjun.guo@xxxxxxxxxx>
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License version 2 as
> >> + * published by the Free Software Foundation.
> >> + */
> >> +
> >> +#include <linux/acpi.h>
> >> +#include <linux/init.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +
> >> +#include <clocksource/arm_arch_timer.h>
> >> +
> >> +#undef pr_fmt
> >> +#define pr_fmt(fmt) "GTDT: " fmt
> >
> > I would add "ACPI" to the prefix too if I were you, but that's me.
>
> good idea, you are right, will do
>
> >
> >> +
> >> +typedef struct {
> >> + struct acpi_table_gtdt *gtdt;
> >> + void *platform_timer_start;
> >> + void *gtdt_end;
> >> +} acpi_gtdt_desc_t;
> >> +
> >> +static acpi_gtdt_desc_t acpi_gtdt_desc __initdata;
> >> +
> >> +static inline void *next_platform_timer(void *platform_timer)
> >> +{
> >> + struct acpi_gtdt_header *gh = platform_timer;
> >> +
> >> + platform_timer += gh->length;
> >> + if (platform_timer < acpi_gtdt_desc.gtdt_end)
> >> + return platform_timer;
> >> +
> >> + return NULL;
> >> +}
> >> +
> >> +#define for_each_platform_timer(_g) \
> >> + for (_g = acpi_gtdt_desc.platform_timer_start; _g; \
> >> + _g = next_platform_timer(_g))
> >> +
> >> +static inline bool is_timer_block(void *platform_timer)
> >> +{
> >> + struct acpi_gtdt_header *gh = platform_timer;
> >> +
> >> + if (gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK)
> >> + return true;
> >> +
> >> + return false;
> >
> > This is just too much code. It would suffice to do
> >
> > return gh->type == ACPI_GTDT_TYPE_TIMER_BLOCK;
> >
> >> +}
> >> +
> >> +static inline bool is_watchdog(void *platform_timer)
> >> +{
> >> + struct acpi_gtdt_header *gh = platform_timer;
> >> +
> >> + if (gh->type == ACPI_GTDT_TYPE_WATCHDOG)
> >> + return true;
> >> +
> >> + return false;
> >
> > Just like above.
>
>
> Thanks, this is better :-) will do
>
>
> >
> >> +}
> >> +
> >> +/*
> >> + * Get some basic info from GTDT table, and init the global variables above
> >> + * for all timers initialization of Generic Timer.
> >> + * This function does some validation on GTDT table.
> >> + */
> >> +static int __init acpi_gtdt_desc_init(struct acpi_table_header *table)
> >> +{
> >> + struct acpi_table_gtdt *gtdt = container_of(table,
> >> + struct acpi_table_gtdt,
> >> + header);
> >> +
> >> + acpi_gtdt_desc.gtdt = gtdt;
> >> + acpi_gtdt_desc.gtdt_end = (void *)table + table->length;
> >> +
> >> + if (table->revision < 2) {
> >> + pr_info("Revision:%d doesn't support Platform Timers.\n",
> >> + table->revision);
> >
> > Is it really useful to print this message (and the one below) at the
> > "info" level? What about changing them to pr_debug()?
>
> yes, pr_debug is better, thanks :-) will do
>
>
> >
> >> + return 0;
> >> + }
> >> +
> >> + if (!gtdt->platform_timer_count) {
> >> + pr_info("No Platform Timer.\n");
> >> + return 0;
> >> + }
> >> +
> >> + acpi_gtdt_desc.platform_timer_start = (void *)gtdt +
> >> + gtdt->platform_timer_offset;
> >> + if (acpi_gtdt_desc.platform_timer_start <
> >> + (void *)table + sizeof(struct acpi_table_gtdt)) {
> >> + pr_err(FW_BUG "Platform Timer pointer error.\n");
> >
> > Why pr_err()?
>
> if (true), that means the GTDT table has bugs.
>
And that's not a very useful piece of information unless you're debugging the
platform, is it?
Thanks,
Rafael
--
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
