[v22,07/11] acpi/arm64: Add GTDT table parse driver

Commit Message

[e-mail] March 21, 2017, 4:31 p.m. UTC

From: Fu Wei <[e-mail]>

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 <[e-mail]>
Signed-off-by: Hanjun Guo <[e-mail]>
Acked-by: Rafael J. Wysocki <[e-mail]>
Tested-by: Xiongfeng Wang <[e-mail]>
Reviewed-by: Hanjun Guo <[e-mail]>
Tested-by: Hanjun Guo <[e-mail]>
---
 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

Lorenzo Pieralisi March 28, 2017, 11:35 a.m. UTC | #1

On Wed, Mar 22, 2017 at 12:31:18AM +0800, [e-mail] wrote:
> From: Fu Wei <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Hanjun Guo <[e-mail]>
> Acked-by: Rafael J. Wysocki <[e-mail]>

Acked-by: Lorenzo Pieralisi <[e-mail]>

Some nits below.

> Tested-by: Xiongfeng Wang <[e-mail]>
> Reviewed-by: Hanjun Guo <[e-mail]>
> Tested-by: Hanjun Guo <[e-mail]>
> ---
>  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 3741859..7e2baec 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..8a03b4b
> --- /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 <[e-mail]>
> + *         Fu Wei <[e-mail]>
> + *         Hanjun Guo <[e-mail]>
> + *
> + * 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.

Note: Secure state is not managed by the kernel on ARM64 systems.

Is that what you wanted to say ?

> + * 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.

Return: true if the timer HW state is lost when a CPU enters an idle
	state, false 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

I do not understand what this means.

> + *				number of platform timers. It can be NULL, if
> + *				driver don't need this info.

driver doesn't

> + *
> + * 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_warn("Revision:%d doesn't support Platform Timers.\n",
> +			table->revision);

Ok, two points here. First, I am not sure why you should warn if the
table revision is < 2, is that a FW bug ? I do not think it is, you
can just return 0.

> +	else if (!gtdt->platform_timer_count)
> +		pr_debug("No Platform Timer.\n");

Ditto. Why keep executing ? There is nothing to do, just bail out
with a return 0.

> +	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;

Same here, I suspect you want to consolidate the return path (I missed
previous rounds of reviews so you should not worry too much, I can clean
this up later) but to me a:

return -EINVAL;

would just do.

Lorenzo

> +		}
> +	}
> +
> +	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 9b05886..4b5c146 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -595,6 +595,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
> -- 
> 2.9.3
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[e-mail] March 29, 2017, 9:48 a.m. UTC | #2

Hi Lorenzo,

Great thanks for your review and help, I will take most of your suggestions,
But one or two comments have been discussed in previous patchset,
please allow me to explain these. :-)

On 28 March 2017 at 19:35, Lorenzo Pieralisi <[e-mail]> wrote:
> On Wed, Mar 22, 2017 at 12:31:18AM +0800, [e-mail] wrote:
>> From: Fu Wei <[e-mail]>
>>
>> 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 <[e-mail]>
>> Signed-off-by: Hanjun Guo <[e-mail]>
>> Acked-by: Rafael J. Wysocki <[e-mail]>
>
> Acked-by: Lorenzo Pieralisi <[e-mail]>
>
> Some nits below.

Great thanks!

>
>> Tested-by: Xiongfeng Wang <[e-mail]>
>> Reviewed-by: Hanjun Guo <[e-mail]>
>> Tested-by: Hanjun Guo <[e-mail]>
>> ---
>>  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 3741859..7e2baec 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..8a03b4b
>> --- /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 <[e-mail]>
>> + *         Fu Wei <[e-mail]>
>> + *         Hanjun Guo <[e-mail]>
>> + *
>> + * 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.
>
> Note: Secure state is not managed by the kernel on ARM64 systems.
>
> Is that what you wanted to say ?

yes, you are right, thanks! :-)
Will do so.

>
>> + * 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.
>
> Return: true if the timer HW state is lost when a CPU enters an idle
>         state, false otherwise

Thanks , will do

>
>> + */
>> +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
>
> I do not understand what this means.
>
>> + *                           number of platform timers. It can be NULL, if
>> + *                           driver don't need this info.
>
> driver doesn't

Sorry, this is not clear, I will fix it by :

 * @platform_timer_count: It points to a integer variable which is used
 *                           for storing the number of platform timers.
 *                           This pointer could be NULL, if the caller
 *                           doesn'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_warn("Revision:%d doesn't support Platform Timers.\n",
>> +                     table->revision);
>
> Ok, two points here. First, I am not sure why you should warn if the
> table revision is < 2, is that a FW bug ? I do not think it is, you
> can just return 0.

I used pr_debug here before v20, then I got Hanjun's suggestion:
-------
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).
-------
https://lkml.org/lkml/2017/1/19/82

So I started to use pr_warn.

>
>> +     else if (!gtdt->platform_timer_count)
>> +             pr_debug("No Platform Timer.\n");
>
> Ditto. Why keep executing ? There is nothing to do, just bail out
> with a return 0.
>
>> +     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;
>
> Same here, I suspect you want to consolidate the return path (I missed
> previous rounds of reviews so you should not worry too much, I can clean
> this up later) but to me a:
>
> return -EINVAL;
>
> would just do.

yes, you are right, I try to consolidate the return path. :-)
It may can return earlier, let me try.

>
> Lorenzo
>
>> +             }
>> +     }
>> +
>> +     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 9b05886..4b5c146 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -595,6 +595,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
>> --
>> 2.9.3
>>

Lorenzo Pieralisi March 29, 2017, 10:21 a.m. UTC | #3

On Wed, Mar 29, 2017 at 05:48:17PM +0800, Fu Wei wrote:

[...]

>  * @platform_timer_count: It points to a integer variable which is used
>  *                           for storing the number of platform timers.
>  *                           This pointer could be NULL, if the caller
>  *                           doesn'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_warn("Revision:%d doesn't support Platform Timers.\n",
> >> +                     table->revision);
> >
> > Ok, two points here. First, I am not sure why you should warn if the
> > table revision is < 2, is that a FW bug ? I do not think it is, you
> > can just return 0.
> 
> I used pr_debug here before v20, then I got Hanjun's suggestion:
> -------
> 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).
> -------
> https://lkml.org/lkml/2017/1/19/82
> 
> So I started to use pr_warn.

Thanks for the explanation, so it is a FW bug and the warning
is granted :) just leave it there.

Still, please check my comment on acpi_gtdt_init() being called
multiple times on patch 11.

Thanks,
Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[e-mail] March 29, 2017, 10:48 a.m. UTC | #4

Hi Lorenzo,

On 29 March 2017 at 18:21, Lorenzo Pieralisi <[e-mail]> wrote:
> On Wed, Mar 29, 2017 at 05:48:17PM +0800, Fu Wei wrote:
>
> [...]
>
>>  * @platform_timer_count: It points to a integer variable which is used
>>  *                           for storing the number of platform timers.
>>  *                           This pointer could be NULL, if the caller
>>  *                           doesn'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_warn("Revision:%d doesn't support Platform Timers.\n",
>> >> +                     table->revision);
>> >
>> > Ok, two points here. First, I am not sure why you should warn if the
>> > table revision is < 2, is that a FW bug ? I do not think it is, you
>> > can just return 0.
>>
>> I used pr_debug here before v20, then I got Hanjun's suggestion:
>> -------
>> 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).
>> -------
>> https://lkml.org/lkml/2017/1/19/82
>>
>> So I started to use pr_warn.
>
> Thanks for the explanation, so it is a FW bug and the warning
> is granted :) just leave it there.
>
> Still, please check my comment on acpi_gtdt_init() being called
> multiple times on patch 11.

Thanks

For calling acpi_gtdt_init() twice:
(1) 1st time: in early boot(bootmem), for init arch_timer and
memory-mapped timer, we initialize the acpi_gtdt_desc.
you can see that all the items in this struct are pointer.
(2) 2nd time: when system switch from bootmem to slab, all the
pointers in the acpi_gtdt_desc are invalid, so we have to
re-initialize(re-map) them.

I have tested it, if we don't re-initialize  the acpi_gtdt_desc,
system will go wrong.

>
> Thanks,
> Lorenzo

Lorenzo Pieralisi March 29, 2017, 11:33 a.m. UTC | #5

On Wed, Mar 29, 2017 at 06:48:26PM +0800, Fu Wei wrote:
> Hi Lorenzo,
> 
> On 29 March 2017 at 18:21, Lorenzo Pieralisi <[e-mail]> wrote:
> > On Wed, Mar 29, 2017 at 05:48:17PM +0800, Fu Wei wrote:
> >
> > [...]
> >
> >>  * @platform_timer_count: It points to a integer variable which is used
> >>  *                           for storing the number of platform timers.
> >>  *                           This pointer could be NULL, if the caller
> >>  *                           doesn'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_warn("Revision:%d doesn't support Platform Timers.\n",
> >> >> +                     table->revision);
> >> >
> >> > Ok, two points here. First, I am not sure why you should warn if the
> >> > table revision is < 2, is that a FW bug ? I do not think it is, you
> >> > can just return 0.
> >>
> >> I used pr_debug here before v20, then I got Hanjun's suggestion:
> >> -------
> >> 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).
> >> -------
> >> https://lkml.org/lkml/2017/1/19/82
> >>
> >> So I started to use pr_warn.
> >
> > Thanks for the explanation, so it is a FW bug and the warning
> > is granted :) just leave it there.
> >
> > Still, please check my comment on acpi_gtdt_init() being called
> > multiple times on patch 11.
> 
> Thanks
> 
> For calling acpi_gtdt_init() twice:
> (1) 1st time: in early boot(bootmem), for init arch_timer and
> memory-mapped timer, we initialize the acpi_gtdt_desc.
> you can see that all the items in this struct are pointer.
> (2) 2nd time: when system switch from bootmem to slab, all the
> pointers in the acpi_gtdt_desc are invalid, so we have to
> re-initialize(re-map) them.
> 
> I have tested it, if we don't re-initialize  the acpi_gtdt_desc,
> system will go wrong.

Ok, that's what I feared. My complaint on patch 11 is that:

1) Stashing the GTDT pointer in acpi_gtdt_desc is not needed to
   parse SBSA watchdogs
2) It is not clear at all from the code or the commit log _why_ you
   need to call acpi_gtdt_init() again (ie technically you don't need
   to call it - you grab a valid pointer to the table and parse the
   watchdogs in the _same_ function gtdt_sbsa_gwdt_init())

I do not think there is much you can do to improve the arch timer gtdt
interface (and it is too late for that anyway) but in patch 11 it would
be ideal if you avoid calling acpi_gtdt_init() again, just parse GTDT
entries and initialize the corresponding watchdogs (ie pointers stashed
in acpi_gtdt_desc are stale anyway but that's __initdata so I can live
with that).

You should add comments to summarize this issue so that it can be
easily understood by anyone maintaining this code, it is not crystal
clear by reading the code why you need to multiple acpi_gtdt_init()
calls and that's not a piffling detail.

The ACPI patches are fine with me otherwise, I will complete the
review shortly.

Thanks !
Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[e-mail] March 29, 2017, 1:42 p.m. UTC | #6

Hi Lorenzo,

On 29 March 2017 at 19:33, Lorenzo Pieralisi <[e-mail]> wrote:
> On Wed, Mar 29, 2017 at 06:48:26PM +0800, Fu Wei wrote:
>> Hi Lorenzo,
>>
>> On 29 March 2017 at 18:21, Lorenzo Pieralisi <[e-mail]> wrote:
>> > On Wed, Mar 29, 2017 at 05:48:17PM +0800, Fu Wei wrote:
>> >
>> > [...]
>> >
>> >>  * @platform_timer_count: It points to a integer variable which is used
>> >>  *                           for storing the number of platform timers.
>> >>  *                           This pointer could be NULL, if the caller
>> >>  *                           doesn'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_warn("Revision:%d doesn't support Platform Timers.\n",
>> >> >> +                     table->revision);
>> >> >
>> >> > Ok, two points here. First, I am not sure why you should warn if the
>> >> > table revision is < 2, is that a FW bug ? I do not think it is, you
>> >> > can just return 0.
>> >>
>> >> I used pr_debug here before v20, then I got Hanjun's suggestion:
>> >> -------
>> >> 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).
>> >> -------
>> >> https://lkml.org/lkml/2017/1/19/82
>> >>
>> >> So I started to use pr_warn.
>> >
>> > Thanks for the explanation, so it is a FW bug and the warning
>> > is granted :) just leave it there.
>> >
>> > Still, please check my comment on acpi_gtdt_init() being called
>> > multiple times on patch 11.
>>
>> Thanks
>>
>> For calling acpi_gtdt_init() twice:
>> (1) 1st time: in early boot(bootmem), for init arch_timer and
>> memory-mapped timer, we initialize the acpi_gtdt_desc.
>> you can see that all the items in this struct are pointer.
>> (2) 2nd time: when system switch from bootmem to slab, all the
>> pointers in the acpi_gtdt_desc are invalid, so we have to
>> re-initialize(re-map) them.
>>
>> I have tested it, if we don't re-initialize  the acpi_gtdt_desc,
>> system will go wrong.
>
> Ok, that's what I feared. My complaint on patch 11 is that:
>
> 1) Stashing the GTDT pointer in acpi_gtdt_desc is not needed to
>    parse SBSA watchdogs

The acpi_gtdt_desc is for sharing the info between acpi_gtdt_init and
acpi_gtdt_c3stop, ;acpi_gtdt_map_ppi
I re-use it in parsing SBSA watchdogs, because I try to re-use acpi_gtdt_init.

> 2) It is not clear at all from the code or the commit log _why_ you
>    need to call acpi_gtdt_init() again (ie technically you don't need
>    to call it - you grab a valid pointer to the table and parse the
>    watchdogs in the _same_ function gtdt_sbsa_gwdt_init())

yes, we can avoid calling acpi_gtdt_init(), do the same thing in
parsing SBSA watchdogs info.
But if we will do the same thing(getting gtdt, platform_timer,
timer_count), why not just re-using the same function?

So my suggestion is that:
Could we re-use acpi_gtdt_init, and a comment at the head of SBSA
watchdogs info parsing function to summarize this issue?
The comment like this

Note: although the global variable acpi_gtdt_desc has been initialized
by acpi_gtdt_init, when we initialized arch_timer. But when we call this
function to get SBSA watchdogs info from GTDT, the system has switch
from bootmem  to slab, so the pointers in acpi_gtdt_desc are dated, we
need to  re-initialize(remap) them. So we call acpi_gtdt_init again here.

Is that OK for you? :-)

>
> I do not think there is much you can do to improve the arch timer gtdt
> interface (and it is too late for that anyway) but in patch 11 it would
> be ideal if you avoid calling acpi_gtdt_init() again, just parse GTDT
> entries and initialize the corresponding watchdogs (ie pointers stashed
> in acpi_gtdt_desc are stale anyway but that's __initdata so I can live
> with that).
>
> You should add comments to summarize this issue so that it can be
> easily understood by anyone maintaining this code, it is not crystal
> clear by reading the code why you need to multiple acpi_gtdt_init()
> calls and that's not a piffling detail.
>
> The ACPI patches are fine with me otherwise, I will complete the
> review shortly.
>
> Thanks !
> Lorenzo

[e-mail] March 29, 2017, 2:29 p.m. UTC | #7

Hi Lorenzo,

On 28 March 2017 at 19:35, Lorenzo Pieralisi <[e-mail]> wrote:
> On Wed, Mar 22, 2017 at 12:31:18AM +0800, [e-mail] wrote:
>> From: Fu Wei <[e-mail]>
>>
>> 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 <[e-mail]>
>> Signed-off-by: Hanjun Guo <[e-mail]>
>> Acked-by: Rafael J. Wysocki <[e-mail]>
>
> Acked-by: Lorenzo Pieralisi <[e-mail]>
>
> Some nits below.
>
>> Tested-by: Xiongfeng Wang <[e-mail]>
>> Reviewed-by: Hanjun Guo <[e-mail]>
>> Tested-by: Hanjun Guo <[e-mail]>
>> ---
>>  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 3741859..7e2baec 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..8a03b4b
>> --- /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 <[e-mail]>
>> + *         Fu Wei <[e-mail]>
>> + *         Hanjun Guo <[e-mail]>
>> + *
>> + * 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.
>
> Note: Secure state is not managed by the kernel on ARM64 systems.
>
> Is that what you wanted to say ?
>
>> + * 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.
>
> Return: true if the timer HW state is lost when a CPU enters an idle
>         state, false 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
>
> I do not understand what this means.
>
>> + *                           number of platform timers. It can be NULL, if
>> + *                           driver don't need this info.
>
> driver doesn't
>
>> + *
>> + * 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_warn("Revision:%d doesn't support Platform Timers.\n",
>> +                     table->revision);
>
> Ok, two points here. First, I am not sure why you should warn if the
> table revision is < 2, is that a FW bug ? I do not think it is, you
> can just return 0.
>
>> +     else if (!gtdt->platform_timer_count)
>> +             pr_debug("No Platform Timer.\n");
>
> Ditto. Why keep executing ? There is nothing to do, just bail out
> with a return 0.
>
>> +     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;
>
> Same here, I suspect you want to consolidate the return path (I missed
> previous rounds of reviews so you should not worry too much, I can clean
> this up later) but to me a:
>
> return -EINVAL;
>
> would just do.

For this problem, Can I do this:

/**
 * acpi_gtdt_init() - Get the info of GTDT table to prepare for further init.
 * @table:                        The pointer to GTDT table.
 * @platform_timer_count:        It points to a integer variable which is used
 *                                for storing the number of platform timers.
 *                                This pointer could be NULL, if the caller
 *                                doesn'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)
{
        void *platform_timer;
        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;
        acpi_gtdt_desc.platform_timer = NULL;
        if (platform_timer_count)
                *platform_timer_count = 0;

        if (table->revision < 2) {
                pr_warn("Revision:%d doesn't support Platform Timers.\n",
                        table->revision);
                return 0;
        }

        if (!gtdt->platform_timer_count) {
                pr_debug("No Platform Timer.\n");
                return 0;
        }
        if (platform_timer_count)
                *platform_timer_count = gtdt->platform_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");
                return -EINVAL;
        }
        acpi_gtdt_desc.platform_timer = platform_timer;

        return 0;
}


>
> Lorenzo
>
>> +             }
>> +     }
>> +
>> +     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 9b05886..4b5c146 100644
>> --- a/include/linux/acpi.h
>> +++ b/include/linux/acpi.h
>> @@ -595,6 +595,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
>> --
>> 2.9.3
>>

[e-mail] March 29, 2017, 2:31 p.m. UTC | #8

On 29 March 2017 at 22:29, Fu Wei <[e-mail]> wrote:
> Hi Lorenzo,
>
> On 28 March 2017 at 19:35, Lorenzo Pieralisi <[e-mail]> wrote:
>> On Wed, Mar 22, 2017 at 12:31:18AM +0800, [e-mail] wrote:
>>> From: Fu Wei <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> Signed-off-by: Hanjun Guo <[e-mail]>
>>> Acked-by: Rafael J. Wysocki <[e-mail]>
>>
>> Acked-by: Lorenzo Pieralisi <[e-mail]>
>>
>> Some nits below.
>>
>>> Tested-by: Xiongfeng Wang <[e-mail]>
>>> Reviewed-by: Hanjun Guo <[e-mail]>
>>> Tested-by: Hanjun Guo <[e-mail]>
>>> ---
>>>  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 3741859..7e2baec 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..8a03b4b
>>> --- /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 <[e-mail]>
>>> + *         Fu Wei <[e-mail]>
>>> + *         Hanjun Guo <[e-mail]>
>>> + *
>>> + * 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.
>>
>> Note: Secure state is not managed by the kernel on ARM64 systems.
>>
>> Is that what you wanted to say ?
>>
>>> + * 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.
>>
>> Return: true if the timer HW state is lost when a CPU enters an idle
>>         state, false 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
>>
>> I do not understand what this means.
>>
>>> + *                           number of platform timers. It can be NULL, if
>>> + *                           driver don't need this info.
>>
>> driver doesn't
>>
>>> + *
>>> + * 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_warn("Revision:%d doesn't support Platform Timers.\n",
>>> +                     table->revision);
>>
>> Ok, two points here. First, I am not sure why you should warn if the
>> table revision is < 2, is that a FW bug ? I do not think it is, you
>> can just return 0.
>>
>>> +     else if (!gtdt->platform_timer_count)
>>> +             pr_debug("No Platform Timer.\n");
>>
>> Ditto. Why keep executing ? There is nothing to do, just bail out
>> with a return 0.
>>
>>> +     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;
>>
>> Same here, I suspect you want to consolidate the return path (I missed
>> previous rounds of reviews so you should not worry too much, I can clean
>> this up later) but to me a:
>>
>> return -EINVAL;
>>
>> would just do.
>
> For this problem, Can I do this:
>
> /**
>  * acpi_gtdt_init() - Get the info of GTDT table to prepare for further init.
>  * @table:                        The pointer to GTDT table.
>  * @platform_timer_count:        It points to a integer variable which is used
>  *                                for storing the number of platform timers.
>  *                                This pointer could be NULL, if the caller
>  *                                doesn'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)
> {
>         void *platform_timer;
>         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;
>         acpi_gtdt_desc.platform_timer = NULL;
>         if (platform_timer_count)
>                 *platform_timer_count = 0;
>
>         if (table->revision < 2) {
>                 pr_warn("Revision:%d doesn't support Platform Timers.\n",
>                         table->revision);
>                 return 0;
>         }
>
>         if (!gtdt->platform_timer_count) {
>                 pr_debug("No Platform Timer.\n");
>                 return 0;
>         }
>         if (platform_timer_count)
>                 *platform_timer_count = gtdt->platform_timer_count;

sorry , this should be moved,


>
>         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");
>                 return -EINVAL;
>         }
>         acpi_gtdt_desc.platform_timer = platform_timer;

         if (platform_timer_count)
                 *platform_timer_count = gtdt->platform_timer_count;

Here is the right place

>
>         return 0;
> }
>
>
>>
>> Lorenzo
>>
>>> +             }
>>> +     }
>>> +
>>> +     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 9b05886..4b5c146 100644
>>> --- a/include/linux/acpi.h
>>> +++ b/include/linux/acpi.h
>>> @@ -595,6 +595,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
>>> --
>>> 2.9.3
>>>
>
>
>
> --
> Best regards,
>
> Fu Wei
> Software Engineer
> Red Hat

Lorenzo Pieralisi March 29, 2017, 3:19 p.m. UTC | #9

On Wed, Mar 29, 2017 at 10:31:42PM +0800, Fu Wei wrote:
> On 29 March 2017 at 22:29, Fu Wei <[e-mail]> wrote:
> > Hi Lorenzo,
> >
> > On 28 March 2017 at 19:35, Lorenzo Pieralisi <[e-mail]> wrote:
> >> On Wed, Mar 22, 2017 at 12:31:18AM +0800, [e-mail] wrote:
> >>> From: Fu Wei <[e-mail]>
> >>>
> >>> 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 <[e-mail]>
> >>> Signed-off-by: Hanjun Guo <[e-mail]>
> >>> Acked-by: Rafael J. Wysocki <[e-mail]>
> >>
> >> Acked-by: Lorenzo Pieralisi <[e-mail]>
> >>
> >> Some nits below.
> >>
> >>> Tested-by: Xiongfeng Wang <[e-mail]>
> >>> Reviewed-by: Hanjun Guo <[e-mail]>
> >>> Tested-by: Hanjun Guo <[e-mail]>
> >>> ---
> >>>  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 3741859..7e2baec 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..8a03b4b
> >>> --- /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 <[e-mail]>
> >>> + *         Fu Wei <[e-mail]>
> >>> + *         Hanjun Guo <[e-mail]>
> >>> + *
> >>> + * 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.
> >>
> >> Note: Secure state is not managed by the kernel on ARM64 systems.
> >>
> >> Is that what you wanted to say ?
> >>
> >>> + * 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.
> >>
> >> Return: true if the timer HW state is lost when a CPU enters an idle
> >>         state, false 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
> >>
> >> I do not understand what this means.
> >>
> >>> + *                           number of platform timers. It can be NULL, if
> >>> + *                           driver don't need this info.
> >>
> >> driver doesn't
> >>
> >>> + *
> >>> + * 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_warn("Revision:%d doesn't support Platform Timers.\n",
> >>> +                     table->revision);
> >>
> >> Ok, two points here. First, I am not sure why you should warn if the
> >> table revision is < 2, is that a FW bug ? I do not think it is, you
> >> can just return 0.
> >>
> >>> +     else if (!gtdt->platform_timer_count)
> >>> +             pr_debug("No Platform Timer.\n");
> >>
> >> Ditto. Why keep executing ? There is nothing to do, just bail out
> >> with a return 0.
> >>
> >>> +     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;
> >>
> >> Same here, I suspect you want to consolidate the return path (I missed
> >> previous rounds of reviews so you should not worry too much, I can clean
> >> this up later) but to me a:
> >>
> >> return -EINVAL;
> >>
> >> would just do.
> >
> > For this problem, Can I do this:
> >
> > /**
> >  * acpi_gtdt_init() - Get the info of GTDT table to prepare for further init.
> >  * @table:                        The pointer to GTDT table.
> >  * @platform_timer_count:        It points to a integer variable which is used
> >  *                                for storing the number of platform timers.
> >  *                                This pointer could be NULL, if the caller
> >  *                                doesn'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)
> > {
> >         void *platform_timer;
> >         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;
> >         acpi_gtdt_desc.platform_timer = NULL;
> >         if (platform_timer_count)
> >                 *platform_timer_count = 0;
> >
> >         if (table->revision < 2) {
> >                 pr_warn("Revision:%d doesn't support Platform Timers.\n",
> >                         table->revision);
> >                 return 0;
> >         }
> >
> >         if (!gtdt->platform_timer_count) {
> >                 pr_debug("No Platform Timer.\n");
> >                 return 0;
> >         }
> >         if (platform_timer_count)
> >                 *platform_timer_count = gtdt->platform_timer_count;
> 
> sorry , this should be moved,
> 
> 
> >
> >         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");
> >                 return -EINVAL;
> >         }
> >         acpi_gtdt_desc.platform_timer = platform_timer;
> 
>          if (platform_timer_count)
>                  *platform_timer_count = gtdt->platform_timer_count;
> 
> Here is the right place

Ok, to avoid adding bugs to code that has been tested already keep
the current function version (as of v22) and send me a patch to clean
this up at -rc1.

Multiple calls to acpi_gtdt_init() were my main concern.

Thanks,
Lorenzo

> 
> >
> >         return 0;
> > }
> >
> >
> >>
> >> Lorenzo
> >>
> >>> +             }
> >>> +     }
> >>> +
> >>> +     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 9b05886..4b5c146 100644
> >>> --- a/include/linux/acpi.h
> >>> +++ b/include/linux/acpi.h
> >>> @@ -595,6 +595,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
> >>> --
> >>> 2.9.3
> >>>
> >
> >
> >
> > --
> > Best regards,
> >
> > Fu Wei
> > Software Engineer
> > Red Hat
> 
> 
> 
> -- 
> Best regards,
> 
> Fu Wei
> Software Engineer
> Red Hat
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Lorenzo Pieralisi March 29, 2017, 4:02 p.m. UTC | #10

On Wed, Mar 29, 2017 at 09:42:39PM +0800, Fu Wei wrote:

[...]

> >> For calling acpi_gtdt_init() twice:
> >> (1) 1st time: in early boot(bootmem), for init arch_timer and
> >> memory-mapped timer, we initialize the acpi_gtdt_desc.
> >> you can see that all the items in this struct are pointer.
> >> (2) 2nd time: when system switch from bootmem to slab, all the
> >> pointers in the acpi_gtdt_desc are invalid, so we have to
> >> re-initialize(re-map) them.
> >>
> >> I have tested it, if we don't re-initialize  the acpi_gtdt_desc,
> >> system will go wrong.
> >
> > Ok, that's what I feared. My complaint on patch 11 is that:
> >
> > 1) Stashing the GTDT pointer in acpi_gtdt_desc is not needed to
> >    parse SBSA watchdogs
> 
> The acpi_gtdt_desc is for sharing the info between acpi_gtdt_init and
> acpi_gtdt_c3stop, ;acpi_gtdt_map_ppi
> I re-use it in parsing SBSA watchdogs, because I try to re-use acpi_gtdt_init.
> 
> > 2) It is not clear at all from the code or the commit log _why_ you
> >    need to call acpi_gtdt_init() again (ie technically you don't need
> >    to call it - you grab a valid pointer to the table and parse the
> >    watchdogs in the _same_ function gtdt_sbsa_gwdt_init())
> 
> yes, we can avoid calling acpi_gtdt_init(), do the same thing in
> parsing SBSA watchdogs info.
> But if we will do the same thing(getting gtdt, platform_timer,
> timer_count), why not just re-using the same function?

Because that's not trivial to understand why it is needed,
it actually isn't :). Anyway, instead of painting the bikeshed
let's add the comment below and be done with this.

> So my suggestion is that:
> Could we re-use acpi_gtdt_init, and a comment at the head of SBSA
> watchdogs info parsing function to summarize this issue?
> The comment like this
> 
> Note: although the global variable acpi_gtdt_desc has been initialized
> by acpi_gtdt_init, when we initialized arch_timer. But when we call this
> function to get SBSA watchdogs info from GTDT, the system has switch
> from bootmem  to slab, so the pointers in acpi_gtdt_desc are dated, we
> need to  re-initialize(remap) them. So we call acpi_gtdt_init again here.
"Note: Even though the global variable acpi_gtdt_desc has been
initialized by acpi_gtdt_init() while initializing the arch timers, when
we call this function to get SBSA watchdogs info from GTDT, the pointers
stashed in it are stale (since they are early temporary mappings carried
out before acpi_permanent_mmap is set) and we need to
re-initialize them with permanent mapped pointer values to let the
GTDT parsing possible".

I still think that if we write the code by omitting acpi_gtdt_init()
call (which does things that are completely useless for SBSA watchdog)
it becomes simpler and much easier to understand, up to you, either you
do it or I will at -rc1 ;-)

Lorenzo

> 
> 
> Is that OK for you? :-)
> 
> >
> > I do not think there is much you can do to improve the arch timer gtdt
> > interface (and it is too late for that anyway) but in patch 11 it would
> > be ideal if you avoid calling acpi_gtdt_init() again, just parse GTDT
> > entries and initialize the corresponding watchdogs (ie pointers stashed
> > in acpi_gtdt_desc are stale anyway but that's __initdata so I can live
> > with that).
> >
> > You should add comments to summarize this issue so that it can be
> > easily understood by anyone maintaining this code, it is not crystal
> > clear by reading the code why you need to multiple acpi_gtdt_init()
> > calls and that's not a piffling detail.
> >
> > The ACPI patches are fine with me otherwise, I will complete the
> > review shortly.
> >
> > Thanks !
> > Lorenzo
> 
> 
> 
> -- 
> Best regards,
> 
> Fu Wei
> Software Engineer
> Red Hat
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 3741859..7e2baec 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..8a03b4b
--- /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 <[e-mail]>
+ *         Fu Wei <[e-mail]>
+ *         Hanjun Guo <[e-mail]>
+ *
+ * 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_warn("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 9b05886..4b5c146 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -595,6 +595,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