[v20,13/17] acpi/arm64: Add GTDT table parse driver
diff mbox

Message ID 20170118132541.8989-14-fu.wei@linaro.org
State Not Applicable, archived
Headers show

Commit Message

fu.wei@linaro.org Jan. 18, 2017, 1:25 p.m. UTC
From: Fu Wei <fu.wei@linaro.org>

This patch adds support for parsing arch timer info 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@linaro.org>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
---
 arch/arm64/Kconfig          |   1 +
 drivers/acpi/arm64/Kconfig  |   3 +
 drivers/acpi/arm64/Makefile |   1 +
 drivers/acpi/arm64/gtdt.c   | 157 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/acpi.h        |   6 ++
 5 files changed, 168 insertions(+)

Comments

Hanjun Guo Jan. 19, 2017, 9:11 a.m. UTC | #1
On 2017/1/18 21:25, fu.wei@linaro.org wrote:
> From: Fu Wei <fu.wei@linaro.org>
>
> This patch adds support for parsing arch timer info 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@linaro.org>
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Tested-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
> ---
>  arch/arm64/Kconfig          |   1 +
>  drivers/acpi/arm64/Kconfig  |   3 +
>  drivers/acpi/arm64/Makefile |   1 +
>  drivers/acpi/arm64/gtdt.c   | 157 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/acpi.h        |   6 ++
>  5 files changed, 168 insertions(+)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 1117421..ab1ee10 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -2,6 +2,7 @@ config ARM64
>  	def_bool y
>  	select ACPI_CCA_REQUIRED if ACPI
>  	select ACPI_GENERIC_GSI if ACPI
> +	select ACPI_GTDT if ACPI
>  	select ACPI_REDUCED_HARDWARE_ONLY if ACPI
>  	select ACPI_MCFG if ACPI
>  	select ACPI_SPCR_TABLE if ACPI
> diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig
> index 4616da4..5a6f80f 100644
> --- a/drivers/acpi/arm64/Kconfig
> +++ b/drivers/acpi/arm64/Kconfig
> @@ -4,3 +4,6 @@
>
>  config ACPI_IORT
>  	bool
> +
> +config ACPI_GTDT
> +	bool
> diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile
> index 72331f2..1017def 100644
> --- a/drivers/acpi/arm64/Makefile
> +++ b/drivers/acpi/arm64/Makefile
> @@ -1 +1,2 @@
>  obj-$(CONFIG_ACPI_IORT) 	+= iort.o
> +obj-$(CONFIG_ACPI_GTDT) 	+= gtdt.o
> diff --git a/drivers/acpi/arm64/gtdt.c b/drivers/acpi/arm64/gtdt.c
> new file mode 100644
> index 0000000..d93a790
> --- /dev/null
> +++ b/drivers/acpi/arm64/gtdt.c
> @@ -0,0 +1,157 @@
[...]
> +
> +/**
> + * acpi_gtdt_init() - Get the info of GTDT table to prepare for further init.
> + * @table:	The pointer to GTDT table.
> + * @platform_timer_count:	The pointer of int variate for returning the
> + *				number of platform timers. It can be NULL, if
> + *				driver don't need this info.
> + *
> + * Return: 0 if success, -EINVAL if error.
> + */
> +int __init acpi_gtdt_init(struct acpi_table_header *table,
> +			  int *platform_timer_count)
> +{
> +	int ret = 0;
> +	int timer_count = 0;
> +	void *platform_timer = NULL;
> +	struct acpi_table_gtdt *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_debug("Revision:%d doesn't support Platform Timers.\n",
> +			 table->revision);

GTDT table revision is updated to 2 in ACPI 5.1, we will
not support ACPI version under 5.1 and disable ACPI in FADT
parse before this code is called, so if we get revision
<2 here, I think we need to print warning (we need to keep
the firmware stick to the spec on ARM64).

> +	else if (!gtdt->platform_timer_count)
> +		pr_debug("No Platform Timer.\n");
> +	else
> +		timer_count = gtdt->platform_timer_count;
> +
> +	if (timer_count) {
> +		platform_timer = (void *)gtdt + gtdt->platform_timer_offset;
> +		if (platform_timer < (void *)table +
> +				     sizeof(struct acpi_table_gtdt)) {
> +			pr_err(FW_BUG "invalid timer data.\n");

It's ok but I didn't see other ACPI tables parsing did this check,
maybe we can just remove it :)

> +			timer_count = 0;
> +			platform_timer = NULL;
> +			ret = -EINVAL;
> +		}
> +	}
> +
> +	acpi_gtdt_desc.platform_timer = platform_timer;
> +	if (platform_timer_count)
> +		*platform_timer_count = timer_count;

Then the code will much simple:

if (gtdt->platform_timer_count) {
	acpi_gtdt_desc.platform_timer = (void *)gtdt + gtdt->platform_timer_offset;
	if (platform_timer_count)
		*platform_timer_count = gtdt->platform_timer_count;
}

return 0;

and remove ret, timer_count and platform_timer.

Thanks
Hanjun
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
fu.wei@linaro.org Jan. 19, 2017, 10:32 a.m. UTC | #2
Hi Hanjun,

On 19 January 2017 at 17:11, Hanjun Guo <hanjun.guo@linaro.org> wrote:
> On 2017/1/18 21:25, fu.wei@linaro.org wrote:
>>
>> From: Fu Wei <fu.wei@linaro.org>
>>
>> This patch adds support for parsing arch timer info 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@linaro.org>
>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Tested-by: Xiongfeng Wang <wangxiongfeng2@huawei.com>
>> ---
>>  arch/arm64/Kconfig          |   1 +
>>  drivers/acpi/arm64/Kconfig  |   3 +
>>  drivers/acpi/arm64/Makefile |   1 +
>>  drivers/acpi/arm64/gtdt.c   | 157
>> ++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/acpi.h        |   6 ++
>>  5 files changed, 168 insertions(+)
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 1117421..ab1ee10 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -2,6 +2,7 @@ config ARM64
>>         def_bool y
>>         select ACPI_CCA_REQUIRED if ACPI
>>         select ACPI_GENERIC_GSI if ACPI
>> +       select ACPI_GTDT if ACPI
>>         select ACPI_REDUCED_HARDWARE_ONLY if ACPI
>>         select ACPI_MCFG if ACPI
>>         select ACPI_SPCR_TABLE if ACPI
>> diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig
>> index 4616da4..5a6f80f 100644
>> --- a/drivers/acpi/arm64/Kconfig
>> +++ b/drivers/acpi/arm64/Kconfig
>> @@ -4,3 +4,6 @@
>>
>>  config ACPI_IORT
>>         bool
>> +
>> +config ACPI_GTDT
>> +       bool
>> diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile
>> index 72331f2..1017def 100644
>> --- a/drivers/acpi/arm64/Makefile
>> +++ b/drivers/acpi/arm64/Makefile
>> @@ -1 +1,2 @@
>>  obj-$(CONFIG_ACPI_IORT)        += iort.o
>> +obj-$(CONFIG_ACPI_GTDT)        += gtdt.o
>> diff --git a/drivers/acpi/arm64/gtdt.c b/drivers/acpi/arm64/gtdt.c
>> new file mode 100644
>> index 0000000..d93a790
>> --- /dev/null
>> +++ b/drivers/acpi/arm64/gtdt.c
>> @@ -0,0 +1,157 @@
>
> [...]
>
>> +
>> +/**
>> + * acpi_gtdt_init() - Get the info of GTDT table to prepare for further
>> init.
>> + * @table:     The pointer to GTDT table.
>> + * @platform_timer_count:      The pointer of int variate for returning
>> the
>> + *                             number of platform timers. It can be NULL,
>> if
>> + *                             driver don't need this info.
>> + *
>> + * Return: 0 if success, -EINVAL if error.
>> + */
>> +int __init acpi_gtdt_init(struct acpi_table_header *table,
>> +                         int *platform_timer_count)
>> +{
>> +       int ret = 0;
>> +       int timer_count = 0;
>> +       void *platform_timer = NULL;
>> +       struct acpi_table_gtdt *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_debug("Revision:%d doesn't support Platform Timers.\n",
>> +                        table->revision);
>
>
> GTDT table revision is updated to 2 in ACPI 5.1, we will
> not support ACPI version under 5.1 and disable ACPI in FADT
> parse before this code is called, so if we get revision
> <2 here, I think we need to print warning (we need to keep
> the firmware stick to the spec on ARM64).

agree, will change pr_debug to pr_warn.

Thanks :-)

>
>> +       else if (!gtdt->platform_timer_count)
>> +               pr_debug("No Platform Timer.\n");
>> +       else
>> +               timer_count = gtdt->platform_timer_count;
>> +
>> +       if (timer_count) {
>> +               platform_timer = (void *)gtdt +
>> gtdt->platform_timer_offset;
>> +               if (platform_timer < (void *)table +
>> +                                    sizeof(struct acpi_table_gtdt)) {
>> +                       pr_err(FW_BUG "invalid timer data.\n");
>
>
> It's ok but I didn't see other ACPI tables parsing did this check,
> maybe we can just remove it :)

here, I want to make sure the FW is valid.
Once there is a FW bug, we could just return with error.  :-)

>
>> +                       timer_count = 0;
>> +                       platform_timer = NULL;
>> +                       ret = -EINVAL;
>> +               }
>> +       }
>> +
>> +       acpi_gtdt_desc.platform_timer = platform_timer;
>> +       if (platform_timer_count)
>> +               *platform_timer_count = timer_count;
>
>
> Then the code will much simple:
>
> if (gtdt->platform_timer_count) {
>         acpi_gtdt_desc.platform_timer = (void *)gtdt +
> gtdt->platform_timer_offset;
>         if (platform_timer_count)
>                 *platform_timer_count = gtdt->platform_timer_count;
> }
>
> return 0;
>
> and remove ret, timer_count and platform_timer.

yes, this may can simplify the function, but this will be released at
the end of init because of "__init" :-)
So how about let's just keep this check to make sure the FW is OK.  :-)

>
> Thanks
> Hanjun
Mark Rutland Jan. 19, 2017, 11:16 a.m. UTC | #3
On Thu, Jan 19, 2017 at 06:32:55PM +0800, Fu Wei wrote:
> On 19 January 2017 at 17:11, Hanjun Guo <hanjun.guo@linaro.org> wrote:
> > On 2017/1/18 21:25, fu.wei@linaro.org wrote:
> >> From: Fu Wei <fu.wei@linaro.org>

> >> +       else if (!gtdt->platform_timer_count)
> >> +               pr_debug("No Platform Timer.\n");
> >> +       else
> >> +               timer_count = gtdt->platform_timer_count;
> >> +
> >> +       if (timer_count) {
> >> +               platform_timer = (void *)gtdt +
> >> gtdt->platform_timer_offset;
> >> +               if (platform_timer < (void *)table +
> >> +                                    sizeof(struct acpi_table_gtdt)) {
> >> +                       pr_err(FW_BUG "invalid timer data.\n");
> >
> >
> > It's ok but I didn't see other ACPI tables parsing did this check,
> > maybe we can just remove it :)
> 
> here, I want to make sure the FW is valid.
> Once there is a FW bug, we could just return with error.  :-)

Yes, please keep the check!

If anything, it would be nicer for the other ACPI code to verify things
a little more stringently.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
fu.wei@linaro.org Jan. 19, 2017, 12:28 p.m. UTC | #4
Hi Mark,

On 19 January 2017 at 19:16, Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Jan 19, 2017 at 06:32:55PM +0800, Fu Wei wrote:
>> On 19 January 2017 at 17:11, Hanjun Guo <hanjun.guo@linaro.org> wrote:
>> > On 2017/1/18 21:25, fu.wei@linaro.org wrote:
>> >> From: Fu Wei <fu.wei@linaro.org>
>
>> >> +       else if (!gtdt->platform_timer_count)
>> >> +               pr_debug("No Platform Timer.\n");
>> >> +       else
>> >> +               timer_count = gtdt->platform_timer_count;
>> >> +
>> >> +       if (timer_count) {
>> >> +               platform_timer = (void *)gtdt +
>> >> gtdt->platform_timer_offset;
>> >> +               if (platform_timer < (void *)table +
>> >> +                                    sizeof(struct acpi_table_gtdt)) {
>> >> +                       pr_err(FW_BUG "invalid timer data.\n");
>> >
>> >
>> > It's ok but I didn't see other ACPI tables parsing did this check,
>> > maybe we can just remove it :)
>>
>> here, I want to make sure the FW is valid.
>> Once there is a FW bug, we could just return with error.  :-)
>
> Yes, please keep the check!

Yes, we will keep this check   :-)

Thanks!
>
> If anything, it would be nicer for the other ACPI code to verify things
> a little more stringently.
>
> Thanks,
> Mark.

Patch
diff mbox

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 1117421..ab1ee10 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -2,6 +2,7 @@  config ARM64
 	def_bool y
 	select ACPI_CCA_REQUIRED if ACPI
 	select ACPI_GENERIC_GSI if ACPI
+	select ACPI_GTDT if ACPI
 	select ACPI_REDUCED_HARDWARE_ONLY if ACPI
 	select ACPI_MCFG if ACPI
 	select ACPI_SPCR_TABLE if ACPI
diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig
index 4616da4..5a6f80f 100644
--- a/drivers/acpi/arm64/Kconfig
+++ b/drivers/acpi/arm64/Kconfig
@@ -4,3 +4,6 @@ 
 
 config ACPI_IORT
 	bool
+
+config ACPI_GTDT
+	bool
diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile
index 72331f2..1017def 100644
--- a/drivers/acpi/arm64/Makefile
+++ b/drivers/acpi/arm64/Makefile
@@ -1 +1,2 @@ 
 obj-$(CONFIG_ACPI_IORT) 	+= iort.o
+obj-$(CONFIG_ACPI_GTDT) 	+= gtdt.o
diff --git a/drivers/acpi/arm64/gtdt.c b/drivers/acpi/arm64/gtdt.c
new file mode 100644
index 0000000..d93a790
--- /dev/null
+++ b/drivers/acpi/arm64/gtdt.c
@@ -0,0 +1,157 @@ 
+/*
+ * ARM Specific GTDT table Support
+ *
+ * Copyright (C) 2016, Linaro Ltd.
+ * Author: Daniel Lezcano <daniel.lezcano@linaro.org>
+ *         Fu Wei <fu.wei@linaro.org>
+ *         Hanjun Guo <hanjun.guo@linaro.org>
+ *
+ * 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 <clocksource/arm_arch_timer.h>
+
+#undef pr_fmt
+#define pr_fmt(fmt) "ACPI GTDT: " fmt
+
+/**
+ * struct acpi_gtdt_descriptor - Store the key info of GTDT for all functions
+ * @gtdt:	The pointer to the struct acpi_table_gtdt of GTDT table.
+ * @gtdt_end:	The pointer to the end of GTDT table.
+ * @platform_timer:	The pointer to the start of Platform Timer Structure
+ *
+ * The struct store the key info of GTDT table, it should be initialized by
+ * acpi_gtdt_init.
+ */
+struct acpi_gtdt_descriptor {
+	struct acpi_table_gtdt *gtdt;
+	void *gtdt_end;
+	void *platform_timer;
+};
+
+static struct acpi_gtdt_descriptor acpi_gtdt_desc __initdata;
+
+static int __init map_gt_gsi(u32 interrupt, u32 flags)
+{
+	int trigger, polarity;
+
+	trigger = (flags & ACPI_GTDT_INTERRUPT_MODE) ? ACPI_EDGE_SENSITIVE
+			: ACPI_LEVEL_SENSITIVE;
+
+	polarity = (flags & ACPI_GTDT_INTERRUPT_POLARITY) ? ACPI_ACTIVE_LOW
+			: ACPI_ACTIVE_HIGH;
+
+	return acpi_register_gsi(NULL, interrupt, trigger, polarity);
+}
+
+/**
+ * acpi_gtdt_map_ppi() - Map the PPIs of per-cpu arch_timer.
+ * @type:	the type of PPI.
+ *
+ * Note: Linux on arm64 isn't supported on the secure side.
+ * So we only handle the non-secure timer PPIs,
+ * ARCH_TIMER_PHYS_SECURE_PPI is treated as invalid type.
+ *
+ * Return: the mapped PPI value, 0 if error.
+ */
+int __init acpi_gtdt_map_ppi(int type)
+{
+	struct acpi_table_gtdt *gtdt = acpi_gtdt_desc.gtdt;
+
+	switch (type) {
+	case ARCH_TIMER_PHYS_NONSECURE_PPI:
+		return map_gt_gsi(gtdt->non_secure_el1_interrupt,
+				  gtdt->non_secure_el1_flags);
+	case ARCH_TIMER_VIRT_PPI:
+		return map_gt_gsi(gtdt->virtual_timer_interrupt,
+				  gtdt->virtual_timer_flags);
+
+	case ARCH_TIMER_HYP_PPI:
+		return map_gt_gsi(gtdt->non_secure_el2_interrupt,
+				  gtdt->non_secure_el2_flags);
+	default:
+		pr_err("Failed to map timer interrupt: invalid type.\n");
+	}
+
+	return 0;
+}
+
+/**
+ * acpi_gtdt_c3stop() - Got c3stop info from GTDT according to the type of PPI.
+ * @type:	the type of PPI.
+ *
+ * Return: 1 if the timer can be in deep idle state, 0 otherwise.
+ */
+bool __init acpi_gtdt_c3stop(int type)
+{
+	struct acpi_table_gtdt *gtdt = acpi_gtdt_desc.gtdt;
+
+	switch (type) {
+	case ARCH_TIMER_PHYS_NONSECURE_PPI:
+		return !(gtdt->non_secure_el1_flags & ACPI_GTDT_ALWAYS_ON);
+
+	case ARCH_TIMER_VIRT_PPI:
+		return !(gtdt->virtual_timer_flags & ACPI_GTDT_ALWAYS_ON);
+
+	case ARCH_TIMER_HYP_PPI:
+		return !(gtdt->non_secure_el2_flags & ACPI_GTDT_ALWAYS_ON);
+
+	default:
+		pr_err("Failed to get c3stop info: invalid type.\n");
+	}
+
+	return 0;
+}
+
+/**
+ * acpi_gtdt_init() - Get the info of GTDT table to prepare for further init.
+ * @table:	The pointer to GTDT table.
+ * @platform_timer_count:	The pointer of int variate for returning the
+ *				number of platform timers. It can be NULL, if
+ *				driver don't need this info.
+ *
+ * Return: 0 if success, -EINVAL if error.
+ */
+int __init acpi_gtdt_init(struct acpi_table_header *table,
+			  int *platform_timer_count)
+{
+	int ret = 0;
+	int timer_count = 0;
+	void *platform_timer = NULL;
+	struct acpi_table_gtdt *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_debug("Revision:%d doesn't support Platform Timers.\n",
+			 table->revision);
+	else if (!gtdt->platform_timer_count)
+		pr_debug("No Platform Timer.\n");
+	else
+		timer_count = gtdt->platform_timer_count;
+
+	if (timer_count) {
+		platform_timer = (void *)gtdt + gtdt->platform_timer_offset;
+		if (platform_timer < (void *)table +
+				     sizeof(struct acpi_table_gtdt)) {
+			pr_err(FW_BUG "invalid timer data.\n");
+			timer_count = 0;
+			platform_timer = NULL;
+			ret = -EINVAL;
+		}
+	}
+
+	acpi_gtdt_desc.platform_timer = platform_timer;
+	if (platform_timer_count)
+		*platform_timer_count = timer_count;
+
+	return ret;
+}
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 5b36974..d0b271e 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -597,6 +597,12 @@  enum acpi_reconfig_event  {
 int acpi_reconfig_notifier_register(struct notifier_block *nb);
 int acpi_reconfig_notifier_unregister(struct notifier_block *nb);
 
+#ifdef CONFIG_ACPI_GTDT
+int acpi_gtdt_init(struct acpi_table_header *table, int *platform_timer_count);
+int acpi_gtdt_map_ppi(int type);
+bool acpi_gtdt_c3stop(int type);
+#endif
+
 #else	/* !CONFIG_ACPI */
 
 #define acpi_disabled 1