Message-ID: <56EC1479.7060902@linaro.org>
Date:	Fri, 18 Mar 2016 15:45:13 +0100
From:	Daniel Lezcano <daniel.lezcano@...aro.org>
To:	fu.wei@...aro.org, rjw@...ysocki.net, lenb@...nel.org,
	tglx@...utronix.de, marc.zyngier@....com, hanjun.guo@...aro.org
Cc:	linux-arm-kernel@...ts.infradead.org, linaro-acpi@...ts.linaro.org,
	linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
	rruigrok@...eaurora.org, harba@...eaurora.org, cov@...eaurora.org,
	timur@...eaurora.org, graeme.gregory@...aro.org,
	al.stone@...aro.org, jcm@...hat.com, wei@...hat.com, arnd@...db.de,
	wim@...ana.be, catalin.marinas@....com, will.deacon@....com,
	Suravee.Suthikulpanit@....com, leo.duran@....com
Subject: Re: [RESEND PATCH v4 2/5] ACPI: add GTDT table parse driver into ACPI
 driver

On 03/18/2016 09:00 AM, fu.wei@...aro.org wrote:
> From: Fu Wei <fu.wei@...aro.org>
>
> This driver adds support for parsing all kinds of timer in GTDT:
> (1)arch timer: provide a kernel API to parse all the PPIs and
> always-on info in GTDT and export them by arch_timer_data struct.
>
> (2)memory-mapped timer: provide several kernel APIs to parse
> GT Block Structure in GTDT, export those info by return value
> and arch_timer_mem_data struct.
>
> (3)SBSA Generic Watchdog: parse all info in SBSA Generic Watchdog
> Structure in GTDT, and creating a platform device with that
> information. This allows the operating system to obtain device
> data from the resource of platform device.
> The platform device named "sbsa-gwdt" can be used by the ARM SBSA
> Generic Watchdog driver.
>
> By this driver, we can simplify all the relevant drivers, and
> separate all the ACPI GTDT knowledge from them.
>
> Signed-off-by: Fu Wei <fu.wei@...aro.org>
> Signed-off-by: Hanjun Guo <hanjun.guo@...aro.org>
> ---
>   drivers/acpi/Kconfig                 |   9 +
>   drivers/acpi/Makefile                |   1 +
>   drivers/acpi/gtdt.c                  | 376 +++++++++++++++++++++++++++++++++++
>   include/clocksource/arm_arch_timer.h |  13 ++
>   include/linux/acpi.h                 |  17 ++
>   5 files changed, 416 insertions(+)
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 82b96ee..abf33d3 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -521,4 +521,13 @@ config XPOWER_PMIC_OPREGION
>
>   endif
>
> +config ACPI_GTDT
> +	bool "ACPI GTDT Support"
> +	depends on ARM64
> +	help
> +	  GTDT (Generic Timer Description Table) provides information
> +	  for per-processor timers and Platform (memory-mapped) timers
> +	  for ARM platforms. Select this option to provide information
> +	  needed for the timers init.

Why not ARM64's Kconfig select ACPI_GTDT ?

This config option assumes an user will manually select this option 
which I believe it makes sense to have always on when ARM64=y. So why 
not create a silent option and select it directly from the ARM64 
platform Kconfig ?


>   endif	# ACPI
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index edeb2d1..f7ea779 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -98,5 +98,6 @@ obj-$(CONFIG_ACPI_EXTLOG)	+= acpi_extlog.o
>   obj-$(CONFIG_PMIC_OPREGION)	+= pmic/intel_pmic.o
>   obj-$(CONFIG_CRC_PMIC_OPREGION) += pmic/intel_pmic_crc.o
>   obj-$(CONFIG_XPOWER_PMIC_OPREGION) += pmic/intel_pmic_xpower.o
> +obj-$(CONFIG_ACPI_GTDT)		+= gtdt.o

acpi_gtdt.o ?

>   video-objs			+= acpi_video.o video_detect.o
> diff --git a/drivers/acpi/gtdt.c b/drivers/acpi/gtdt.c
> new file mode 100644
> index 0000000..d1b851c
> --- /dev/null
> +++ b/drivers/acpi/gtdt.c
> @@ -0,0 +1,376 @@
> +/*
> + * ARM Specific GTDT table Support
> + *
> + * Copyright (C) 2015, Linaro Ltd.
> + * Author: Fu Wei <fu.wei@...aro.org>
> + *         Hanjun Guo <hanjun.guo@...aro.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/device.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +#include <clocksource/arm_arch_timer.h>
> +
> +#undef pr_fmt

Why #undef ?

> +#define pr_fmt(fmt) "GTDT: " fmt
> +
> +static u32 platform_timer_count __initdata;
> +static int gtdt_timers_existence __initdata;
> +
> +/*
> + * Get some basic info from GTDT table, and init the global variables above
> + * for all timers initialization of Generic Timer.
> + * This function does some validation on GTDT table, and will be run only once.
> + */
> +static void __init *platform_timer_info_init(struct acpi_table_header *table)
> +{
> +	void *gtdt_end, *platform_timer_struct, *platform_timer;
> +	struct acpi_gtdt_header *header;
> +	struct acpi_table_gtdt *gtdt;
> +	u32 i;
> +
> +	gtdt = container_of(table, struct acpi_table_gtdt, header);
> +	if (!gtdt) {
> +		pr_err("table pointer error.\n");
> +		return NULL;
> +	}
> +	gtdt_end = (void *)table + table->length;
> +	gtdt_timers_existence |= ARCH_CP15_TIMER;
> +
> +	if (table->revision < 2) {
> +		pr_info("Revision:%d doesn't support Platform Timers.\n",
> +			table->revision);
> +		return NULL;
> +	}
> +
> +	platform_timer_count = gtdt->platform_timer_count;
> +	if (!platform_timer_count) {
> +		pr_info("No Platform Timer structures.\n");
> +		return NULL;
> +	}
> +
> +	platform_timer_struct = (void *)gtdt + gtdt->platform_timer_offset;
> +	if (platform_timer_struct < (void *)table +
> +				    sizeof(struct acpi_table_gtdt)) {
> +		pr_err(FW_BUG "Platform Timer pointer error.\n");
> +		return NULL;
> +	}
> +
> +	platform_timer = platform_timer_struct;
> +	for (i = 0; i < platform_timer_count; i++) {
> +		if (platform_timer > gtdt_end) {
> +			pr_err(FW_BUG "subtable pointer overflows.\n");
> +			platform_timer_count = i;
> +			break;
> +		}
> +		header = (struct acpi_gtdt_header *)platform_timer;
> +		if (header->type == ACPI_GTDT_TYPE_TIMER_BLOCK)
> +			gtdt_timers_existence |= ARCH_MEM_TIMER;
> +		else if (header->type == ACPI_GTDT_TYPE_WATCHDOG)
> +			gtdt_timers_existence |= ARCH_WD_TIMER;
> +		platform_timer += header->length;
> +	}
> +
> +	return platform_timer_struct;
> +}
> +
> +static int __init map_generic_timer_interrupt(u32 interrupt, u32 flags)
> +{
> +	int trigger, polarity;
> +
> +	if (!interrupt)
> +		return 0;
> +
> +	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);
> +}
> +
> +/*
> + * Get the necessary info of arch_timer from GTDT table.
> + */
> +int __init gtdt_arch_timer_data_init(struct acpi_table_header *table,
> +				     struct arch_timer_data *data)
> +{
> +	struct acpi_table_gtdt *gtdt;
> +
> +	if (acpi_disabled || !data)
> +		return -EINVAL;
> +
> +	if (!table) {
> +		if (ACPI_FAILURE(acpi_get_table(ACPI_SIG_GTDT, 0, &table)))
> +			return -EINVAL;
> +	}
> +
> +	if (!gtdt_timers_existence)
> +		platform_timer_info_init(table);

Same comment than the one below in the gtdt_gt_block function. There is 
something wrong in the init sequence.

> +	gtdt = container_of(table, struct acpi_table_gtdt, header);
> +
> +	data->phys_secure_ppi =
> +		map_generic_timer_interrupt(gtdt->secure_el1_interrupt,
> +					    gtdt->secure_el1_flags);
> +
> +	data->phys_nonsecure_ppi =
> +		map_generic_timer_interrupt(gtdt->non_secure_el1_interrupt,
> +					    gtdt->non_secure_el1_flags);
> +
> +	data->virt_ppi =
> +		map_generic_timer_interrupt(gtdt->virtual_timer_interrupt,
> +					    gtdt->virtual_timer_flags);
> +
> +	data->hyp_ppi =
> +		map_generic_timer_interrupt(gtdt->non_secure_el2_interrupt,
> +					    gtdt->non_secure_el2_flags);
> +
> +	data->c3stop = !(gtdt->non_secure_el1_flags & ACPI_GTDT_ALWAYS_ON);
> +
> +	return 0;
> +}
> +
> +bool __init gtdt_timer_is_available(int type)
> +{
> +	return gtdt_timers_existence | type;
> +}
> +
> +/*
> + * Helper function for getting the pointer of platform_timer_struct.
> + */
> +static void __init *get_platform_timer_struct(struct acpi_table_header *table)
> +{
> +	struct acpi_table_gtdt *gtdt;
> +
> +	if (!table) {
> +		pr_err("table pointer error.\n");
> +		return NULL;
> +	}

IMO, this check is not necessary as the caller should have checked it 
before calling this function.

> +	gtdt = container_of(table, struct acpi_table_gtdt, header);
> +
> +	return (void *)gtdt + gtdt->platform_timer_offset;
> +}
> +
> + /*
> + * Get the pointer of GT Block Structure in GTDT table
> + */
> +void __init *gtdt_gt_block(struct acpi_table_header *table, int index)
> +{
> +	struct acpi_gtdt_header *header;
> +	void *platform_timer;
> +	int i, j;
> +
> +	if (!gtdt_timers_existence)
> +		platform_timer = platform_timer_info_init(table);
> +	else
> +		platform_timer = get_platform_timer_struct(table);

This portion of code suggests there is a lost of control of the init 
sequence or will give the opportunity of the caller to do it so.

I would suggest to be more strict:

if (!gtdt_timers_existence)
	return NULL;

and let the user of this function to ensure gtdt_gt_block is called 
after the gtdt tables are initialized.

> +	if (!gtdt_timer_is_available(ARCH_MEM_TIMER))
> +		return NULL;
> +
> +	for (i = 0, j = 0; i < platform_timer_count; i++) {
> +		header = (struct acpi_gtdt_header *)platform_timer;
> +		if (header->type == ACPI_GTDT_TYPE_TIMER_BLOCK && j++ == index)
> +			return platform_timer;
> +		platform_timer += header->length;
> +	}
> +
> +	return NULL;
> +}
> +
> +/*
> + * Get the timer_count(the number of timer frame) of a GT Block in GTDT table
> + */
> +u32 __init gtdt_gt_timer_count(struct acpi_gtdt_timer_block *gt_block)
> +{
> +	if (!gt_block) {
> +		pr_err("invalid GT Block baseaddr.\n");
> +		return 0;
> +	}

In the patch 5/5, !gt_block is already checked.

> +	return gt_block->timer_count;
> +}
> +
> +/*
> + * Get the physical address of GT Block in GTDT table
> + */
> +void __init *gtdt_gt_cntctlbase(struct acpi_gtdt_timer_block *gt_block)
> +{
> +	if (!gt_block) {
> +		pr_err("invalid GT Block baseaddr.\n");
> +		return NULL;
> +	}

Same comment here.

arch_timer_mem_best_frame in patch 5/5 checks !gt_block then calls 
arch_timer_mem_cnttidr which in turn calls gtdt_gt_cntctlbase

> +	return (void *)gt_block->block_address;
> +}
> +
> +/*
> + * Helper function for getting the pointer of a timer frame in GT block.
> + */
> +static void __init *gtdt_gt_timer_frame(struct acpi_gtdt_timer_block *gt_block,
> +					int index)
> +{
> +	void *timer_frame;
> +
> +	if (!(gt_block && gt_block->timer_count))
> +		return NULL;

Pointless check.

Ok, I am giving up the review at this point.

Don't introduce a family of helpers to access gtdt structure internal. I 
think it would much more nice if you create a structure for the timer, 
pass it to the acpi subsystem and let it fill it.

-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Powered by blists - more mailing lists