-----Original Message-----
From: Marc Zyngier [mailto:marc.zyngier at arm.com]
Sent: Tuesday, January 24, 2017 1:09 PM
To: John Garry; Mark Rutland; Guohanjun (Hanjun Guo)
Cc: Will Deacon; Daniel Lezcano; Rafael J. Wysocki; Lorenzo Pieralisi;
Fu Wei; Dingtianhong; linux-acpi at vger.kernel.org; linux-arm-
kernel at lists.infradead.org; Linuxarm; Hanjun Guo; Shameerali Kolothum
Thodi
Subject: Re: [PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

On 24/01/17 12:35, John Garry wrote:

quoted

On 24/01/2017 11:32, Marc Zyngier wrote:

quoted

On 24/01/17 10:57, Mark Rutland wrote:

quoted

On Tue, Jan 24, 2017 at 06:39:51PM +0800, Hanjun Guo wrote:

quoted

From: Hanjun Guo <redacted>

Add hisilicon timer specific erratum fixes.

Signed-off-by: Hanjun Guo <redacted>
---
 drivers/clocksource/arm_arch_timer.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/clocksource/arm_arch_timer.c

b/drivers/clocksource/arm_arch_timer.c

quoted

quoted

quoted

quoted

index 80d6f76..3e62a09 100644

--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c

@@ -1156,10 +1156,32 @@ struct gtdt_arch_timer_fixup {
 	void *context;
 };

+#ifdef CONFIG_HISILICON_ERRATUM_161010101
+static void __init hisi_erratum_workaroud_enable(void *context)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ool_workarounds); i++) {
+		if (!strcmp(context, ool_workarounds[i].id)) {
+			timer_unstable_counter_workaround =

&ool_workarounds[i];

quoted

quoted

quoted

quoted

+

	static_branch_enable(&arch_timer_read_ool_enabled);

quoted

quoted

quoted

quoted

+			pr_info("arch_timer: Enabling workaround for

%s\n",

quoted

quoted

quoted

quoted

+				timer_unstable_counter_workaround->id);
+			break;
+		}
+	}
+}
+#endif
+
 /* note: this needs to be updated according to the doc of OEM ID
  * and TABLE ID for different board.
  */
 static struct gtdt_arch_timer_fixup arch_timer_quirks[]

__initdata = {

quoted

quoted

quoted

quoted

+#ifdef CONFIG_HISILICON_ERRATUM_161010101
+	{"HISI  ", "HIP05   ", 0, &hisi_erratum_workaroud_enable,

"hisilicon,erratum-161010101"},

quoted

quoted

quoted

quoted

+	{"HISI  ", "HIP06   ", 0, &hisi_erratum_workaroud_enable,

"hisilicon,erratum-161010101"},

quoted

quoted

quoted

quoted

+	{"HISI  ", "HIP07   ", 0, &hisi_erratum_workaroud_enable,

"hisilicon,erratum-161010101"},

quoted

quoted

quoted

quoted

+#endif
 };

NAK. This duplicates logic unnecessarily (for enabling the

workaround),

quoted

quoted

quoted

and (ab)uses the id, which was intended to be specific to DT (since

it

quoted

quoted

quoted

is a DT property name).

Agreed, that's properly revolting.

quoted

We should split the matching from the particular workaround (and
enabling thereof), so that we can go straight from ACPI match to
workaround (without having to use the DT id in this manner), and

don't

quoted

quoted

quoted

have to duplicate the logic to enable the workaround.

I believe Marc is looking at some rework in this area which may

enable

quoted

quoted

quoted

this, so please wait for that to appear.

Yeah, I'm implementing a semi-flexible way to add new quirk types,

and

quoted

quoted

the last thing I want to see is two (or more) tables describing the

same

quoted

quoted

thing.

FYI, We have a similar requirement to enable a quirk on the GICv3 ITS
driver. For that driver the current quirk framework relies on

matching

quoted

the IIDR, which is not properly populated for our hardware. So will

this

quoted

framework cover this/many/all drivers?

Most probably not. Each driver has its own requirements, and it is
unlikely that the timer's fit with the GIC's. But maybe we can use
similar patterns.

quoted

Shameer has prepared the patchset for this quirk - should it send it?

It

quoted

will be rejected for the same reason as this one, but at least it is
more reference for this framework wishlist.

Well, try and see it the other way around. If you don't send the patch,
it can't be rejected! ;-) Now, if you're genuinely interested in
finding
out what I think of it, and possibly some advise on how to address the
issue, then please post it.

Sure :).

Thanks,
Shameer