On Wed, Mar 22, 2017 at 12:31:18AM +0800, fu.wei at linaro.org wrote:

From: Fu Wei <redacted>

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 <redacted>
Signed-off-by: Hanjun Guo <redacted>
Acked-by: Rafael J. Wysocki <redacted>

Acked-by: Lorenzo Pieralisi <redacted>

Some nits below.

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