[PATCH 0/2] arch_timer: acpi: Add workaround for hisilicon-161010101 erratum

[PATCH 0/2] arch_timer: acpi: Add workaround for hisilicon-161010101 erratum

[Hanjun Guo]

From: Hanjun Guo <hanjun.guo@linaro.org>

Introduce a general quirk framework for arch timer erratum in ACPI,
which use the oem information in GTDT table for platform specific
erratums, then add specific handler and data for hisilicon-161010101
erratum.

Hanjun Guo (2):
  arm64: arch_timer: acpi: Introduce a generic aquirk framework for
    erratum
  arch_timer: acpi: add hisi timer erratum data

 drivers/clocksource/arm_arch_timer.c | 58 ++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

-- 
1.7.12.4

[PATCH 0/2] arch_timer: acpi: Add workaround for hisilicon-161010101 erratum

[Hanjun Guo]

From: Hanjun Guo <hanjun.guo@linaro.org>

Introduce a general quirk framework for arch timer erratum in ACPI,
which use the oem information in GTDT table for platform specific
erratums, then add specific handler and data for hisilicon-161010101
erratum.

Hanjun Guo (2):
  arm64: arch_timer: acpi: Introduce a generic aquirk framework for
    erratum
  arch_timer: acpi: add hisi timer erratum data

 drivers/clocksource/arm_arch_timer.c | 58 ++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

-- 
1.7.12.4

[PATCH 1/2] arm64: arch_timer: acpi: Introduce a generic aquirk framework for erratum

[Hanjun Guo]

From: Hanjun Guo <hanjun.guo@linaro.org>

Introduce a general quirk framework for arch timer erratum in ACPI,
which use the oem information in GTDT table for platform specific
erratums. The struct gtdt_arch_timer_fixup is introduced to record
the oem information to match the quirk and handle the erratum.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 drivers/clocksource/arm_arch_timer.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 2dc571d..80d6f76 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -1146,6 +1146,39 @@ static int __init arch_timer_mem_of_init(struct device_node *np)
 		       arch_timer_mem_of_init);
 
 #ifdef CONFIG_ACPI_GTDT
+struct gtdt_arch_timer_fixup {
+	char oem_id[ACPI_OEM_ID_SIZE + 1];
+	char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
+	u32 oem_revision;
+
+	/* quirk handler for arch timer erratum */
+	void (*handler)(void *context);
+	void *context;
+};
+
+/* 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 = {
+};
+
+static void __init arch_timer_acpi_quirks_handler(char *oem_id,
+						  char *oem_table_id,
+						  u32 oem_revision)
+{
+	struct gtdt_arch_timer_fixup *quirks = arch_timer_quirks;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(arch_timer_quirks); i++, quirks++) {
+		if (!memcmp(quirks->oem_id, oem_id, ACPI_OEM_ID_SIZE) &&
+		    !memcmp(quirks->oem_table_id, oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
+		    quirks->oem_revision == oem_revision) {
+			if (quirks->handler && quirks->context)
+				quirks->handler(quirks->context);
+		}
+	}
+}
+
 static int __init arch_timer_mem_acpi_init(int platform_timer_count)
 {
 	struct arch_timer_mem *timer_mem;
@@ -1182,6 +1215,9 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *table)
 		return -EINVAL;
 	}
 
+	arch_timer_acpi_quirks_handler(table->oem_id, table->oem_table_id,
+				       table->oem_revision);
+
 	arch_timers_present |= ARCH_TIMER_TYPE_CP15;
 
 	ret = acpi_gtdt_init(table, &platform_timer_count);
-- 
1.7.12.4

[PATCH 1/2] arm64: arch_timer: acpi: Introduce a generic aquirk framework for erratum

[Hanjun Guo]

From: Hanjun Guo <hanjun.guo@linaro.org>

Introduce a general quirk framework for arch timer erratum in ACPI,
which use the oem information in GTDT table for platform specific
erratums. The struct gtdt_arch_timer_fixup is introduced to record
the oem information to match the quirk and handle the erratum.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 drivers/clocksource/arm_arch_timer.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 2dc571d..80d6f76 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -1146,6 +1146,39 @@ static int __init arch_timer_mem_of_init(struct device_node *np)
 		       arch_timer_mem_of_init);
 
 #ifdef CONFIG_ACPI_GTDT
+struct gtdt_arch_timer_fixup {
+	char oem_id[ACPI_OEM_ID_SIZE + 1];
+	char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
+	u32 oem_revision;
+
+	/* quirk handler for arch timer erratum */
+	void (*handler)(void *context);
+	void *context;
+};
+
+/* 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 = {
+};
+
+static void __init arch_timer_acpi_quirks_handler(char *oem_id,
+						  char *oem_table_id,
+						  u32 oem_revision)
+{
+	struct gtdt_arch_timer_fixup *quirks = arch_timer_quirks;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(arch_timer_quirks); i++, quirks++) {
+		if (!memcmp(quirks->oem_id, oem_id, ACPI_OEM_ID_SIZE) &&
+		    !memcmp(quirks->oem_table_id, oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
+		    quirks->oem_revision == oem_revision) {
+			if (quirks->handler && quirks->context)
+				quirks->handler(quirks->context);
+		}
+	}
+}
+
 static int __init arch_timer_mem_acpi_init(int platform_timer_count)
 {
 	struct arch_timer_mem *timer_mem;
@@ -1182,6 +1215,9 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *table)
 		return -EINVAL;
 	}
 
+	arch_timer_acpi_quirks_handler(table->oem_id, table->oem_table_id,
+				       table->oem_revision);
+
 	arch_timers_present |= ARCH_TIMER_TYPE_CP15;
 
 	ret = acpi_gtdt_init(table, &platform_timer_count);
-- 
1.7.12.4

[PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Hanjun Guo]

From: Hanjun Guo <hanjun.guo@linaro.org>

Add hisilicon timer specific erratum fixes.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 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
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];
+			static_branch_enable(&arch_timer_read_ool_enabled);
+			pr_info("arch_timer: Enabling workaround for %s\n",
+				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 = {
+#ifdef CONFIG_HISILICON_ERRATUM_161010101
+	{"HISI  ", "HIP05   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
+	{"HISI  ", "HIP06   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
+	{"HISI  ", "HIP07   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
+#endif
 };
 
 static void __init arch_timer_acpi_quirks_handler(char *oem_id,
-- 
1.7.12.4

[PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Hanjun Guo]

From: Hanjun Guo <hanjun.guo@linaro.org>

Add hisilicon timer specific erratum fixes.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 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
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];
+			static_branch_enable(&arch_timer_read_ool_enabled);
+			pr_info("arch_timer: Enabling workaround for %s\n",
+				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 = {
+#ifdef CONFIG_HISILICON_ERRATUM_161010101
+	{"HISI  ", "HIP05   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
+	{"HISI  ", "HIP06   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
+	{"HISI  ", "HIP07   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
+#endif
 };
 
 static void __init arch_timer_acpi_quirks_handler(char *oem_id,
-- 
1.7.12.4

Re: [PATCH 0/2] arch_timer: acpi: Add workaround for hisilicon-161010101 erratum

[Hanjun Guo]

On 2017/1/24 18:39, Hanjun Guo wrote:
> From: Hanjun Guo <hanjun.guo@linaro.org>
>
> Introduce a general quirk framework for arch timer erratum in ACPI,
> which use the oem information in GTDT table for platform specific
> erratums, then add specific handler and data for hisilicon-161010101
> erratum.

Sorry, forgot to mention that this patch set is based on Mark's git
tree [1] and plus remaining patches for GTDT patch set.

[1] git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git 
arch-timer/updates

Thanks
Hanjun

[PATCH 0/2] arch_timer: acpi: Add workaround for hisilicon-161010101 erratum

[Hanjun Guo]

On 2017/1/24 18:39, Hanjun Guo wrote:
> From: Hanjun Guo <hanjun.guo@linaro.org>
>
> Introduce a general quirk framework for arch timer erratum in ACPI,
> which use the oem information in GTDT table for platform specific
> erratums, then add specific handler and data for hisilicon-161010101
> erratum.

Sorry, forgot to mention that this patch set is based on Mark's git
tree [1] and plus remaining patches for GTDT patch set.

[1] git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git 
arch-timer/updates

Thanks
Hanjun

Re: [PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Mark Rutland]

On Tue, Jan 24, 2017 at 06:39:51PM +0800, Hanjun Guo wrote:
> From: Hanjun Guo <hanjun.guo@linaro.org>
> 
> Add hisilicon timer specific erratum fixes.
> 
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> ---
>  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
> 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];
> +			static_branch_enable(&arch_timer_read_ool_enabled);
> +			pr_info("arch_timer: Enabling workaround for %s\n",
> +				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 = {
> +#ifdef CONFIG_HISILICON_ERRATUM_161010101
> +	{"HISI  ", "HIP05   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
> +	{"HISI  ", "HIP06   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
> +	{"HISI  ", "HIP07   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
> +#endif
>  };

NAK. This duplicates logic unnecessarily (for enabling the workaround),
and (ab)uses the id, which was intended to be specific to DT (since it
is a DT property name).

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
have to duplicate the logic to enable the workaround.

I believe Marc is looking at some rework in this area which may enable
this, so please wait for that to appear.

Thanks,
Mark.

[PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Mark Rutland]

On Tue, Jan 24, 2017 at 06:39:51PM +0800, Hanjun Guo wrote:
> From: Hanjun Guo <hanjun.guo@linaro.org>
> 
> Add hisilicon timer specific erratum fixes.
> 
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> ---
>  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
> 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];
> +			static_branch_enable(&arch_timer_read_ool_enabled);
> +			pr_info("arch_timer: Enabling workaround for %s\n",
> +				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 = {
> +#ifdef CONFIG_HISILICON_ERRATUM_161010101
> +	{"HISI  ", "HIP05   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
> +	{"HISI  ", "HIP06   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
> +	{"HISI  ", "HIP07   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
> +#endif
>  };

NAK. This duplicates logic unnecessarily (for enabling the workaround),
and (ab)uses the id, which was intended to be specific to DT (since it
is a DT property name).

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
have to duplicate the logic to enable the workaround.

I believe Marc is looking at some rework in this area which may enable
this, so please wait for that to appear.

Thanks,
Mark.

Re: [PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Marc Zyngier]

On 24/01/17 10:57, Mark Rutland wrote:
> On Tue, Jan 24, 2017 at 06:39:51PM +0800, Hanjun Guo wrote:
>> From: Hanjun Guo <hanjun.guo@linaro.org>
>>
>> Add hisilicon timer specific erratum fixes.
>>
>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>> ---
>>  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
>> 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];
>> +			static_branch_enable(&arch_timer_read_ool_enabled);
>> +			pr_info("arch_timer: Enabling workaround for %s\n",
>> +				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 = {
>> +#ifdef CONFIG_HISILICON_ERRATUM_161010101
>> +	{"HISI  ", "HIP05   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>> +	{"HISI  ", "HIP06   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>> +	{"HISI  ", "HIP07   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>> +#endif
>>  };
> 
> NAK. This duplicates logic unnecessarily (for enabling the workaround),
> and (ab)uses the id, which was intended to be specific to DT (since it
> is a DT property name).

Agreed, that's properly revolting.

> 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
> have to duplicate the logic to enable the workaround.
> 
> I believe Marc is looking at some rework in this area which may enable
> this, so please wait for that to appear.

Yeah, I'm implementing a semi-flexible way to add new quirk types, and
the last thing I want to see is two (or more) tables describing the same
thing.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Marc Zyngier]

On 24/01/17 10:57, Mark Rutland wrote:
> On Tue, Jan 24, 2017 at 06:39:51PM +0800, Hanjun Guo wrote:
>> From: Hanjun Guo <hanjun.guo@linaro.org>
>>
>> Add hisilicon timer specific erratum fixes.
>>
>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>> ---
>>  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
>> 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];
>> +			static_branch_enable(&arch_timer_read_ool_enabled);
>> +			pr_info("arch_timer: Enabling workaround for %s\n",
>> +				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 = {
>> +#ifdef CONFIG_HISILICON_ERRATUM_161010101
>> +	{"HISI  ", "HIP05   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>> +	{"HISI  ", "HIP06   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>> +	{"HISI  ", "HIP07   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>> +#endif
>>  };
> 
> NAK. This duplicates logic unnecessarily (for enabling the workaround),
> and (ab)uses the id, which was intended to be specific to DT (since it
> is a DT property name).

Agreed, that's properly revolting.

> 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
> have to duplicate the logic to enable the workaround.
> 
> I believe Marc is looking at some rework in this area which may enable
> this, so please wait for that to appear.

Yeah, I'm implementing a semi-flexible way to add new quirk types, and
the last thing I want to see is two (or more) tables describing the same
thing.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[John Garry]

On 24/01/2017 11:32, Marc Zyngier wrote:
> On 24/01/17 10:57, Mark Rutland wrote:
>> On Tue, Jan 24, 2017 at 06:39:51PM +0800, Hanjun Guo wrote:
>>> From: Hanjun Guo <hanjun.guo@linaro.org>
>>>
>>> Add hisilicon timer specific erratum fixes.
>>>
>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>> ---
>>>  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
>>> 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];
>>> +			static_branch_enable(&arch_timer_read_ool_enabled);
>>> +			pr_info("arch_timer: Enabling workaround for %s\n",
>>> +				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 = {
>>> +#ifdef CONFIG_HISILICON_ERRATUM_161010101
>>> +	{"HISI  ", "HIP05   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>> +	{"HISI  ", "HIP06   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>> +	{"HISI  ", "HIP07   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>> +#endif
>>>  };
>>
>> NAK. This duplicates logic unnecessarily (for enabling the workaround),
>> and (ab)uses the id, which was intended to be specific to DT (since it
>> is a DT property name).
>
> Agreed, that's properly revolting.
>
>> 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
>> have to duplicate the logic to enable the workaround.
>>
>> I believe Marc is looking at some rework in this area which may enable
>> this, so please wait for that to appear.
>
> Yeah, I'm implementing a semi-flexible way to add new quirk types, and
> the last thing I want to see is two (or more) tables describing the same
> 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 
the IIDR, which is not properly populated for our hardware. So will this 
framework cover this/many/all drivers?

Shameer has prepared the patchset for this quirk - should it send it? It 
will be rejected for the same reason as this one, but at least it is 
more reference for this framework wishlist.

Cheers,
John


> Thanks,
>
> 	M.
>

[PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[John Garry]

On 24/01/2017 11:32, Marc Zyngier wrote:
> On 24/01/17 10:57, Mark Rutland wrote:
>> On Tue, Jan 24, 2017 at 06:39:51PM +0800, Hanjun Guo wrote:
>>> From: Hanjun Guo <hanjun.guo@linaro.org>
>>>
>>> Add hisilicon timer specific erratum fixes.
>>>
>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>> ---
>>>  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
>>> 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];
>>> +			static_branch_enable(&arch_timer_read_ool_enabled);
>>> +			pr_info("arch_timer: Enabling workaround for %s\n",
>>> +				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 = {
>>> +#ifdef CONFIG_HISILICON_ERRATUM_161010101
>>> +	{"HISI  ", "HIP05   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>> +	{"HISI  ", "HIP06   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>> +	{"HISI  ", "HIP07   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>> +#endif
>>>  };
>>
>> NAK. This duplicates logic unnecessarily (for enabling the workaround),
>> and (ab)uses the id, which was intended to be specific to DT (since it
>> is a DT property name).
>
> Agreed, that's properly revolting.
>
>> 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
>> have to duplicate the logic to enable the workaround.
>>
>> I believe Marc is looking at some rework in this area which may enable
>> this, so please wait for that to appear.
>
> Yeah, I'm implementing a semi-flexible way to add new quirk types, and
> the last thing I want to see is two (or more) tables describing the same
> 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 
the IIDR, which is not properly populated for our hardware. So will this 
framework cover this/many/all drivers?

Shameer has prepared the patchset for this quirk - should it send it? It 
will be rejected for the same reason as this one, but at least it is 
more reference for this framework wishlist.

Cheers,
John


> Thanks,
>
> 	M.
>

Re: [PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Marc Zyngier]

On 24/01/17 12:35, John Garry wrote:
> On 24/01/2017 11:32, Marc Zyngier wrote:
>> On 24/01/17 10:57, Mark Rutland wrote:
>>> On Tue, Jan 24, 2017 at 06:39:51PM +0800, Hanjun Guo wrote:
>>>> From: Hanjun Guo <hanjun.guo@linaro.org>
>>>>
>>>> Add hisilicon timer specific erratum fixes.
>>>>
>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>> ---
>>>>  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
>>>> 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];
>>>> +			static_branch_enable(&arch_timer_read_ool_enabled);
>>>> +			pr_info("arch_timer: Enabling workaround for %s\n",
>>>> +				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 = {
>>>> +#ifdef CONFIG_HISILICON_ERRATUM_161010101
>>>> +	{"HISI  ", "HIP05   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>> +	{"HISI  ", "HIP06   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>> +	{"HISI  ", "HIP07   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>> +#endif
>>>>  };
>>>
>>> NAK. This duplicates logic unnecessarily (for enabling the workaround),
>>> and (ab)uses the id, which was intended to be specific to DT (since it
>>> is a DT property name).
>>
>> Agreed, that's properly revolting.
>>
>>> 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
>>> have to duplicate the logic to enable the workaround.
>>>
>>> I believe Marc is looking at some rework in this area which may enable
>>> this, so please wait for that to appear.
>>
>> Yeah, I'm implementing a semi-flexible way to add new quirk types, and
>> the last thing I want to see is two (or more) tables describing the same
>> 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 
> the IIDR, which is not properly populated for our hardware. So will this 
> 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.

> Shameer has prepared the patchset for this quirk - should it send it? It 
> 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.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Marc Zyngier]

On 24/01/17 12:35, John Garry wrote:
> On 24/01/2017 11:32, Marc Zyngier wrote:
>> On 24/01/17 10:57, Mark Rutland wrote:
>>> On Tue, Jan 24, 2017 at 06:39:51PM +0800, Hanjun Guo wrote:
>>>> From: Hanjun Guo <hanjun.guo@linaro.org>
>>>>
>>>> Add hisilicon timer specific erratum fixes.
>>>>
>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>> ---
>>>>  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
>>>> 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];
>>>> +			static_branch_enable(&arch_timer_read_ool_enabled);
>>>> +			pr_info("arch_timer: Enabling workaround for %s\n",
>>>> +				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 = {
>>>> +#ifdef CONFIG_HISILICON_ERRATUM_161010101
>>>> +	{"HISI  ", "HIP05   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>> +	{"HISI  ", "HIP06   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>> +	{"HISI  ", "HIP07   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>> +#endif
>>>>  };
>>>
>>> NAK. This duplicates logic unnecessarily (for enabling the workaround),
>>> and (ab)uses the id, which was intended to be specific to DT (since it
>>> is a DT property name).
>>
>> Agreed, that's properly revolting.
>>
>>> 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
>>> have to duplicate the logic to enable the workaround.
>>>
>>> I believe Marc is looking at some rework in this area which may enable
>>> this, so please wait for that to appear.
>>
>> Yeah, I'm implementing a semi-flexible way to add new quirk types, and
>> the last thing I want to see is two (or more) tables describing the same
>> 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 
> the IIDR, which is not properly populated for our hardware. So will this 
> 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.

> Shameer has prepared the patchset for this quirk - should it send it? It 
> 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.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Hanjun Guo]

On 01/24/2017 07:32 PM, Marc Zyngier wrote:
> On 24/01/17 10:57, Mark Rutland wrote:
>> On Tue, Jan 24, 2017 at 06:39:51PM +0800, Hanjun Guo wrote:
>>> From: Hanjun Guo <hanjun.guo@linaro.org>
>>>
>>> Add hisilicon timer specific erratum fixes.
>>>
>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>> ---
>>>   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
>>> 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];
>>> +			static_branch_enable(&arch_timer_read_ool_enabled);
>>> +			pr_info("arch_timer: Enabling workaround for %s\n",
>>> +				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 = {
>>> +#ifdef CONFIG_HISILICON_ERRATUM_161010101
>>> +	{"HISI  ", "HIP05   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>> +	{"HISI  ", "HIP06   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>> +	{"HISI  ", "HIP07   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>> +#endif
>>>   };
>>
>> NAK. This duplicates logic unnecessarily (for enabling the workaround),
>> and (ab)uses the id, which was intended to be specific to DT (since it
>> is a DT property name).
>
> Agreed, that's properly revolting.

I had a bad feeling about duplicating using of DT based ID string
in ACPI world and it's confirmed :)

>
>> 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
>> have to duplicate the logic to enable the workaround.
>>
>> I believe Marc is looking at some rework in this area which may enable
>> this, so please wait for that to appear.
>
> Yeah, I'm implementing a semi-flexible way to add new quirk types, and
> the last thing I want to see is two (or more) tables describing the same
> thing.

Thank you for doing this, I will wait for your patches.

Hanjun

[PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Hanjun Guo]

On 01/24/2017 07:32 PM, Marc Zyngier wrote:
> On 24/01/17 10:57, Mark Rutland wrote:
>> On Tue, Jan 24, 2017 at 06:39:51PM +0800, Hanjun Guo wrote:
>>> From: Hanjun Guo <hanjun.guo@linaro.org>
>>>
>>> Add hisilicon timer specific erratum fixes.
>>>
>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>> ---
>>>   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
>>> 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];
>>> +			static_branch_enable(&arch_timer_read_ool_enabled);
>>> +			pr_info("arch_timer: Enabling workaround for %s\n",
>>> +				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 = {
>>> +#ifdef CONFIG_HISILICON_ERRATUM_161010101
>>> +	{"HISI  ", "HIP05   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>> +	{"HISI  ", "HIP06   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>> +	{"HISI  ", "HIP07   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>> +#endif
>>>   };
>>
>> NAK. This duplicates logic unnecessarily (for enabling the workaround),
>> and (ab)uses the id, which was intended to be specific to DT (since it
>> is a DT property name).
>
> Agreed, that's properly revolting.

I had a bad feeling about duplicating using of DT based ID string
in ACPI world and it's confirmed :)

>
>> 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
>> have to duplicate the logic to enable the workaround.
>>
>> I believe Marc is looking at some rework in this area which may enable
>> this, so please wait for that to appear.
>
> Yeah, I'm implementing a semi-flexible way to add new quirk types, and
> the last thing I want to see is two (or more) tables describing the same
> thing.

Thank you for doing this, I will wait for your patches.

Hanjun

RE: [PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Marc Zyngier [mailto:marc.zyngier@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@vger.kernel.org; linux-arm-
> kernel@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:
> > On 24/01/2017 11:32, Marc Zyngier wrote:
> >> On 24/01/17 10:57, Mark Rutland wrote:
> >>> On Tue, Jan 24, 2017 at 06:39:51PM +0800, Hanjun Guo wrote:
> >>>> From: Hanjun Guo <hanjun.guo@linaro.org>
> >>>>
> >>>> Add hisilicon timer specific erratum fixes.
> >>>>
> >>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> >>>> ---
> >>>>  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
> >>>> 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];
> >>>> +
> 	static_branch_enable(&arch_timer_read_ool_enabled);
> >>>> +			pr_info("arch_timer: Enabling workaround for
> %s\n",
> >>>> +				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 = {
> >>>> +#ifdef CONFIG_HISILICON_ERRATUM_161010101
> >>>> +	{"HISI  ", "HIP05   ", 0, &hisi_erratum_workaroud_enable,
> "hisilicon,erratum-161010101"},
> >>>> +	{"HISI  ", "HIP06   ", 0, &hisi_erratum_workaroud_enable,
> "hisilicon,erratum-161010101"},
> >>>> +	{"HISI  ", "HIP07   ", 0, &hisi_erratum_workaroud_enable,
> "hisilicon,erratum-161010101"},
> >>>> +#endif
> >>>>  };
> >>>
> >>> NAK. This duplicates logic unnecessarily (for enabling the
> workaround),
> >>> and (ab)uses the id, which was intended to be specific to DT (since
> it
> >>> is a DT property name).
> >>
> >> Agreed, that's properly revolting.
> >>
> >>> 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
> >>> have to duplicate the logic to enable the workaround.
> >>>
> >>> I believe Marc is looking at some rework in this area which may
> enable
> >>> this, so please wait for that to appear.
> >>
> >> Yeah, I'm implementing a semi-flexible way to add new quirk types,
> and
> >> the last thing I want to see is two (or more) tables describing the
> same
> >> 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
> > the IIDR, which is not properly populated for our hardware. So will
> this
> > 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.
> 
> > Shameer has prepared the patchset for this quirk - should it send it?
> It
> > 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

[PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Shameerali Kolothum Thodi]

> -----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:
> > On 24/01/2017 11:32, Marc Zyngier wrote:
> >> On 24/01/17 10:57, Mark Rutland wrote:
> >>> On Tue, Jan 24, 2017 at 06:39:51PM +0800, Hanjun Guo wrote:
> >>>> From: Hanjun Guo <hanjun.guo@linaro.org>
> >>>>
> >>>> Add hisilicon timer specific erratum fixes.
> >>>>
> >>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> >>>> ---
> >>>>  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
> >>>> 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];
> >>>> +
> 	static_branch_enable(&arch_timer_read_ool_enabled);
> >>>> +			pr_info("arch_timer: Enabling workaround for
> %s\n",
> >>>> +				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 = {
> >>>> +#ifdef CONFIG_HISILICON_ERRATUM_161010101
> >>>> +	{"HISI  ", "HIP05   ", 0, &hisi_erratum_workaroud_enable,
> "hisilicon,erratum-161010101"},
> >>>> +	{"HISI  ", "HIP06   ", 0, &hisi_erratum_workaroud_enable,
> "hisilicon,erratum-161010101"},
> >>>> +	{"HISI  ", "HIP07   ", 0, &hisi_erratum_workaroud_enable,
> "hisilicon,erratum-161010101"},
> >>>> +#endif
> >>>>  };
> >>>
> >>> NAK. This duplicates logic unnecessarily (for enabling the
> workaround),
> >>> and (ab)uses the id, which was intended to be specific to DT (since
> it
> >>> is a DT property name).
> >>
> >> Agreed, that's properly revolting.
> >>
> >>> 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
> >>> have to duplicate the logic to enable the workaround.
> >>>
> >>> I believe Marc is looking at some rework in this area which may
> enable
> >>> this, so please wait for that to appear.
> >>
> >> Yeah, I'm implementing a semi-flexible way to add new quirk types,
> and
> >> the last thing I want to see is two (or more) tables describing the
> same
> >> 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
> > the IIDR, which is not properly populated for our hardware. So will
> this
> > 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.
> 
> > Shameer has prepared the patchset for this quirk - should it send it?
> It
> > 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

Re: [PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Hanjun Guo]

On 01/24/2017 06:57 PM, Mark Rutland wrote:
> On Tue, Jan 24, 2017 at 06:39:51PM +0800, Hanjun Guo wrote:
>> From: Hanjun Guo <hanjun.guo@linaro.org>
>>
>> Add hisilicon timer specific erratum fixes.
>>
>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>> ---
>>   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
>> 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];
>> +			static_branch_enable(&arch_timer_read_ool_enabled);
>> +			pr_info("arch_timer: Enabling workaround for %s\n",
>> +				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 = {
>> +#ifdef CONFIG_HISILICON_ERRATUM_161010101
>> +	{"HISI  ", "HIP05   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>> +	{"HISI  ", "HIP06   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>> +	{"HISI  ", "HIP07   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>> +#endif
>>   };
>
> NAK. This duplicates logic unnecessarily (for enabling the workaround),
> and (ab)uses the id, which was intended to be specific to DT (since it
> is a DT property name).
>
> 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
> have to duplicate the logic to enable the workaround.

maybe we can add ACPI quirk matching information in
struct arch_timer_erratum_workaround, then reuse the
code for both ACPI and DT, but it needs further cleanup
to support multi platforms sharing the same erratum in
ACPI way.

Thanks
Hanjun

[PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Hanjun Guo]

On 01/24/2017 06:57 PM, Mark Rutland wrote:
> On Tue, Jan 24, 2017 at 06:39:51PM +0800, Hanjun Guo wrote:
>> From: Hanjun Guo <hanjun.guo@linaro.org>
>>
>> Add hisilicon timer specific erratum fixes.
>>
>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>> ---
>>   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
>> 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];
>> +			static_branch_enable(&arch_timer_read_ool_enabled);
>> +			pr_info("arch_timer: Enabling workaround for %s\n",
>> +				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 = {
>> +#ifdef CONFIG_HISILICON_ERRATUM_161010101
>> +	{"HISI  ", "HIP05   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>> +	{"HISI  ", "HIP06   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>> +	{"HISI  ", "HIP07   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>> +#endif
>>   };
>
> NAK. This duplicates logic unnecessarily (for enabling the workaround),
> and (ab)uses the id, which was intended to be specific to DT (since it
> is a DT property name).
>
> 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
> have to duplicate the logic to enable the workaround.

maybe we can add ACPI quirk matching information in
struct arch_timer_erratum_workaround, then reuse the
code for both ACPI and DT, but it needs further cleanup
to support multi platforms sharing the same erratum in
ACPI way.

Thanks
Hanjun

Re: [PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Hanjun Guo]

Hi Marc,

On 2017/1/24 19:32, Marc Zyngier wrote:
> On 24/01/17 10:57, Mark Rutland wrote:
>> On Tue, Jan 24, 2017 at 06:39:51PM +0800, Hanjun Guo wrote:
>>> From: Hanjun Guo <hanjun.guo@linaro.org>
>>>
>>> Add hisilicon timer specific erratum fixes.
>>>
>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>> ---
>>>  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
>>> 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];
>>> +			static_branch_enable(&arch_timer_read_ool_enabled);
>>> +			pr_info("arch_timer: Enabling workaround for %s\n",
>>> +				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 = {
>>> +#ifdef CONFIG_HISILICON_ERRATUM_161010101
>>> +	{"HISI  ", "HIP05   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>> +	{"HISI  ", "HIP06   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>> +	{"HISI  ", "HIP07   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>> +#endif
>>>  };
>> NAK. This duplicates logic unnecessarily (for enabling the workaround),
>> and (ab)uses the id, which was intended to be specific to DT (since it
>> is a DT property name).
> Agreed, that's properly revolting.
>
>> 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
>> have to duplicate the logic to enable the workaround.
>>
>> I believe Marc is looking at some rework in this area which may enable
>> this, so please wait for that to appear.
> Yeah, I'm implementing a semi-flexible way to add new quirk types, and
> the last thing I want to see is two (or more) tables describing the same
> thing.

Kindly ping, if you have patches in hand, I can test against our platforms,
thank you very much.

Best Regards
Hanjun

[PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Hanjun Guo]

Hi Marc,

On 2017/1/24 19:32, Marc Zyngier wrote:
> On 24/01/17 10:57, Mark Rutland wrote:
>> On Tue, Jan 24, 2017 at 06:39:51PM +0800, Hanjun Guo wrote:
>>> From: Hanjun Guo <hanjun.guo@linaro.org>
>>>
>>> Add hisilicon timer specific erratum fixes.
>>>
>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>> ---
>>>  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
>>> 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];
>>> +			static_branch_enable(&arch_timer_read_ool_enabled);
>>> +			pr_info("arch_timer: Enabling workaround for %s\n",
>>> +				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 = {
>>> +#ifdef CONFIG_HISILICON_ERRATUM_161010101
>>> +	{"HISI  ", "HIP05   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>> +	{"HISI  ", "HIP06   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>> +	{"HISI  ", "HIP07   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>> +#endif
>>>  };
>> NAK. This duplicates logic unnecessarily (for enabling the workaround),
>> and (ab)uses the id, which was intended to be specific to DT (since it
>> is a DT property name).
> Agreed, that's properly revolting.
>
>> 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
>> have to duplicate the logic to enable the workaround.
>>
>> I believe Marc is looking at some rework in this area which may enable
>> this, so please wait for that to appear.
> Yeah, I'm implementing a semi-flexible way to add new quirk types, and
> the last thing I want to see is two (or more) tables describing the same
> thing.

Kindly ping, if you have patches in hand, I can test against our platforms,
thank you very much.

Best Regards
Hanjun

Re: [PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Alexander Graf]

On 10/02/2017 08:10, Hanjun Guo wrote:
> Hi Marc,
>
> On 2017/1/24 19:32, Marc Zyngier wrote:
>> On 24/01/17 10:57, Mark Rutland wrote:
>>> On Tue, Jan 24, 2017 at 06:39:51PM +0800, Hanjun Guo wrote:
>>>> From: Hanjun Guo <hanjun.guo@linaro.org>
>>>>
>>>> Add hisilicon timer specific erratum fixes.
>>>>
>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>> ---
>>>>  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
>>>> 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];
>>>> +			static_branch_enable(&arch_timer_read_ool_enabled);
>>>> +			pr_info("arch_timer: Enabling workaround for %s\n",
>>>> +				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 = {
>>>> +#ifdef CONFIG_HISILICON_ERRATUM_161010101
>>>> +	{"HISI  ", "HIP05   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>> +	{"HISI  ", "HIP06   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>> +	{"HISI  ", "HIP07   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>> +#endif
>>>>  };
>>> NAK. This duplicates logic unnecessarily (for enabling the workaround),
>>> and (ab)uses the id, which was intended to be specific to DT (since it
>>> is a DT property name).
>> Agreed, that's properly revolting.
>>
>>> 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
>>> have to duplicate the logic to enable the workaround.
>>>
>>> I believe Marc is looking at some rework in this area which may enable
>>> this, so please wait for that to appear.
>> Yeah, I'm implementing a semi-flexible way to add new quirk types, and
>> the last thing I want to see is two (or more) tables describing the same
>> thing.
>
> Kindly ping, if you have patches in hand, I can test against our platforms,
> thank you very much.

Any progress on this? I'm not a big fan of stalling erratum fixes 
upstream because of potential future refactoring.


Alex

[PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Alexander Graf]

On 10/02/2017 08:10, Hanjun Guo wrote:
> Hi Marc,
>
> On 2017/1/24 19:32, Marc Zyngier wrote:
>> On 24/01/17 10:57, Mark Rutland wrote:
>>> On Tue, Jan 24, 2017 at 06:39:51PM +0800, Hanjun Guo wrote:
>>>> From: Hanjun Guo <hanjun.guo@linaro.org>
>>>>
>>>> Add hisilicon timer specific erratum fixes.
>>>>
>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>> ---
>>>>  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
>>>> 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];
>>>> +			static_branch_enable(&arch_timer_read_ool_enabled);
>>>> +			pr_info("arch_timer: Enabling workaround for %s\n",
>>>> +				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 = {
>>>> +#ifdef CONFIG_HISILICON_ERRATUM_161010101
>>>> +	{"HISI  ", "HIP05   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>> +	{"HISI  ", "HIP06   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>> +	{"HISI  ", "HIP07   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>> +#endif
>>>>  };
>>> NAK. This duplicates logic unnecessarily (for enabling the workaround),
>>> and (ab)uses the id, which was intended to be specific to DT (since it
>>> is a DT property name).
>> Agreed, that's properly revolting.
>>
>>> 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
>>> have to duplicate the logic to enable the workaround.
>>>
>>> I believe Marc is looking at some rework in this area which may enable
>>> this, so please wait for that to appear.
>> Yeah, I'm implementing a semi-flexible way to add new quirk types, and
>> the last thing I want to see is two (or more) tables describing the same
>> thing.
>
> Kindly ping, if you have patches in hand, I can test against our platforms,
> thank you very much.

Any progress on this? I'm not a big fan of stalling erratum fixes 
upstream because of potential future refactoring.


Alex

Re: [PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Daniel Lezcano]

On Thu, Feb 16, 2017 at 09:42:11AM +0100, Alexander Graf wrote:
 
[ ... ]

> >>>I believe Marc is looking at some rework in this area which may enable
> >>>this, so please wait for that to appear.
> >>Yeah, I'm implementing a semi-flexible way to add new quirk types, and
> >>the last thing I want to see is two (or more) tables describing the same
> >>thing.
> >
> >Kindly ping, if you have patches in hand, I can test against our platforms,
> >thank you very much.
> 
> Any progress on this? I'm not a big fan of stalling erratum fixes upstream
> because of potential future refactoring.

The fix is in the tip tree, soon in v4.11-rc1 when the merge will be finished.

http://goo.gl/x90qwE
http://goo.gl/asCBBD

  -- Daniel


-- 

 <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

[PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Daniel Lezcano]

On Thu, Feb 16, 2017 at 09:42:11AM +0100, Alexander Graf wrote:
 
[ ... ]

> >>>I believe Marc is looking at some rework in this area which may enable
> >>>this, so please wait for that to appear.
> >>Yeah, I'm implementing a semi-flexible way to add new quirk types, and
> >>the last thing I want to see is two (or more) tables describing the same
> >>thing.
> >
> >Kindly ping, if you have patches in hand, I can test against our platforms,
> >thank you very much.
> 
> Any progress on this? I'm not a big fan of stalling erratum fixes upstream
> because of potential future refactoring.

The fix is in the tip tree, soon in v4.11-rc1 when the merge will be finished.

http://goo.gl/x90qwE
http://goo.gl/asCBBD

  -- Daniel


-- 

 <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

Re: [PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Alexander Graf]

On 16/02/2017 10:14, Daniel Lezcano wrote:
> On Thu, Feb 16, 2017 at 09:42:11AM +0100, Alexander Graf wrote:
>
> [ ... ]
>
>>>>> I believe Marc is looking at some rework in this area which may enable
>>>>> this, so please wait for that to appear.
>>>> Yeah, I'm implementing a semi-flexible way to add new quirk types, and
>>>> the last thing I want to see is two (or more) tables describing the same
>>>> thing.
>>>
>>> Kindly ping, if you have patches in hand, I can test against our platforms,
>>> thank you very much.
>>
>> Any progress on this? I'm not a big fan of stalling erratum fixes upstream
>> because of potential future refactoring.
>
> The fix is in the tip tree, soon in v4.11-rc1 when the merge will be finished.
>
> http://goo.gl/x90qwE
> http://goo.gl/asCBBD

Oh, I missed that. Sorry for the fuss :).


Alex

[PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Alexander Graf]

On 16/02/2017 10:14, Daniel Lezcano wrote:
> On Thu, Feb 16, 2017 at 09:42:11AM +0100, Alexander Graf wrote:
>
> [ ... ]
>
>>>>> I believe Marc is looking at some rework in this area which may enable
>>>>> this, so please wait for that to appear.
>>>> Yeah, I'm implementing a semi-flexible way to add new quirk types, and
>>>> the last thing I want to see is two (or more) tables describing the same
>>>> thing.
>>>
>>> Kindly ping, if you have patches in hand, I can test against our platforms,
>>> thank you very much.
>>
>> Any progress on this? I'm not a big fan of stalling erratum fixes upstream
>> because of potential future refactoring.
>
> The fix is in the tip tree, soon in v4.11-rc1 when the merge will be finished.
>
> http://goo.gl/x90qwE
> http://goo.gl/asCBBD

Oh, I missed that. Sorry for the fuss :).


Alex

Re: [PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Ding Tianhong]

On 2017/2/16 17:32, Alexander Graf wrote:
> 
> 
> On 16/02/2017 10:14, Daniel Lezcano wrote:
>> On Thu, Feb 16, 2017 at 09:42:11AM +0100, Alexander Graf wrote:
>>
>> [ ... ]
>>
>>>>>> I believe Marc is looking at some rework in this area which may enable
>>>>>> this, so please wait for that to appear.
>>>>> Yeah, I'm implementing a semi-flexible way to add new quirk types, and
>>>>> the last thing I want to see is two (or more) tables describing the same
>>>>> thing.
>>>>
>>>> Kindly ping, if you have patches in hand, I can test against our platforms,
>>>> thank you very much.
>>>
>>> Any progress on this? I'm not a big fan of stalling erratum fixes upstream
>>> because of potential future refactoring.
>>
>> The fix is in the tip tree, soon in v4.11-rc1 when the merge will be finished.
>>
>> http://goo.gl/x90qwE
>> http://goo.gl/asCBBD
> 
> Oh, I missed that. Sorry for the fuss :).
> 
> 
> Alex
> 

Hi, I thinks the tip tree is only applied the erratum for DT, not for ACPI, the patch
for ACPI is not applied yet, so the work isn't finished. o<\x1ao<	

Thanks
Ding


> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> .
> 

[PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Ding Tianhong]

On 2017/2/16 17:32, Alexander Graf wrote:
> 
> 
> On 16/02/2017 10:14, Daniel Lezcano wrote:
>> On Thu, Feb 16, 2017 at 09:42:11AM +0100, Alexander Graf wrote:
>>
>> [ ... ]
>>
>>>>>> I believe Marc is looking at some rework in this area which may enable
>>>>>> this, so please wait for that to appear.
>>>>> Yeah, I'm implementing a semi-flexible way to add new quirk types, and
>>>>> the last thing I want to see is two (or more) tables describing the same
>>>>> thing.
>>>>
>>>> Kindly ping, if you have patches in hand, I can test against our platforms,
>>>> thank you very much.
>>>
>>> Any progress on this? I'm not a big fan of stalling erratum fixes upstream
>>> because of potential future refactoring.
>>
>> The fix is in the tip tree, soon in v4.11-rc1 when the merge will be finished.
>>
>> http://goo.gl/x90qwE
>> http://goo.gl/asCBBD
> 
> Oh, I missed that. Sorry for the fuss :).
> 
> 
> Alex
> 

Hi, I thinks the tip tree is only applied the erratum for DT, not for ACPI, the patch
for ACPI is not applied yet, so the work isn't finished. o<\x1ao<	

Thanks
Ding


> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> .
> 

Re: [PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Hanjun Guo]

Hi Daniel,

On 2017/2/16 17:14, Daniel Lezcano wrote:
> On Thu, Feb 16, 2017 at 09:42:11AM +0100, Alexander Graf wrote:
>  
> [ ... ]
>
>>>>> I believe Marc is looking at some rework in this area which may enable
>>>>> this, so please wait for that to appear.
>>>> Yeah, I'm implementing a semi-flexible way to add new quirk types, and
>>>> the last thing I want to see is two (or more) tables describing the same
>>>> thing.
>>> Kindly ping, if you have patches in hand, I can test against our platforms,
>>> thank you very much.
>> Any progress on this? I'm not a big fan of stalling erratum fixes upstream
>> because of potential future refactoring.
> The fix is in the tip tree, soon in v4.11-rc1 when the merge will be finished.
>
> http://goo.gl/x90qwE
> http://goo.gl/asCBBD

I think it's the erratum core code and the device tree support are merged by you, not the
ACPI support in this patch set, did I miss something?

Thanks
Hanjun

[PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Hanjun Guo]

Hi Daniel,

On 2017/2/16 17:14, Daniel Lezcano wrote:
> On Thu, Feb 16, 2017 at 09:42:11AM +0100, Alexander Graf wrote:
>  
> [ ... ]
>
>>>>> I believe Marc is looking at some rework in this area which may enable
>>>>> this, so please wait for that to appear.
>>>> Yeah, I'm implementing a semi-flexible way to add new quirk types, and
>>>> the last thing I want to see is two (or more) tables describing the same
>>>> thing.
>>> Kindly ping, if you have patches in hand, I can test against our platforms,
>>> thank you very much.
>> Any progress on this? I'm not a big fan of stalling erratum fixes upstream
>> because of potential future refactoring.
> The fix is in the tip tree, soon in v4.11-rc1 when the merge will be finished.
>
> http://goo.gl/x90qwE
> http://goo.gl/asCBBD

I think it's the erratum core code and the device tree support are merged by you, not the
ACPI support in this patch set, did I miss something?

Thanks
Hanjun

Re: [PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Sanil Kumar]

On 2/16/2017 3:11 PM, Ding Tianhong wrote:
>
> On 2017/2/16 17:32, Alexander Graf wrote:
>>
>> On 16/02/2017 10:14, Daniel Lezcano wrote:
>>> On Thu, Feb 16, 2017 at 09:42:11AM +0100, Alexander Graf wrote:
>>>
>>> [ ... ]
>>>
>>>>>>> I believe Marc is looking at some rework in this area which may enable
>>>>>>> this, so please wait for that to appear.
>>>>>> Yeah, I'm implementing a semi-flexible way to add new quirk types, and
>>>>>> the last thing I want to see is two (or more) tables describing the same
>>>>>> thing.
>>>>> Kindly ping, if you have patches in hand, I can test against our platforms,
>>>>> thank you very much.
>>>> Any progress on this? I'm not a big fan of stalling erratum fixes upstream
>>>> because of potential future refactoring.
>>> The fix is in the tip tree, soon in v4.11-rc1 when the merge will be finished.
>>>
>>> http://goo.gl/x90qwE
>>> http://goo.gl/asCBBD
>> Oh, I missed that. Sorry for the fuss :).
>>
>>
>> Alex
>>
> Hi, I thinks the tip tree is only applied the erratum for DT, not for ACPI, the patch
> for ACPI is not applied yet, so the work isn't finished. ：）
Yes, initial thread was about ACPI part. I think Alex was asking about that.
>
> Thanks
> Ding
>
>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>> .
>>
>
> .
>


-- 
Thanks and Regards
Sanil.
"Lets make things simple and better, today, tomorrow and always!"
www.huawei.com , www.hisilicon.com
-----------------------------------
This e-mail and its attachments contain confidential information from HUAWEI, which
is intended only for the person or entity whose address is listed above. Any use of
the information contained herein in any way (including, but not limited to, total
or partial disclosure, reproduction, or dissemination) by persons other than the
intended recipient(s) is prohibited. If you receive this e-mail in error, please
notify the sender by phone or email immediately and delete it!

[PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Sanil Kumar]

On 2/16/2017 3:11 PM, Ding Tianhong wrote:
>
> On 2017/2/16 17:32, Alexander Graf wrote:
>>
>> On 16/02/2017 10:14, Daniel Lezcano wrote:
>>> On Thu, Feb 16, 2017 at 09:42:11AM +0100, Alexander Graf wrote:
>>>
>>> [ ... ]
>>>
>>>>>>> I believe Marc is looking at some rework in this area which may enable
>>>>>>> this, so please wait for that to appear.
>>>>>> Yeah, I'm implementing a semi-flexible way to add new quirk types, and
>>>>>> the last thing I want to see is two (or more) tables describing the same
>>>>>> thing.
>>>>> Kindly ping, if you have patches in hand, I can test against our platforms,
>>>>> thank you very much.
>>>> Any progress on this? I'm not a big fan of stalling erratum fixes upstream
>>>> because of potential future refactoring.
>>> The fix is in the tip tree, soon in v4.11-rc1 when the merge will be finished.
>>>
>>> http://goo.gl/x90qwE
>>> http://goo.gl/asCBBD
>> Oh, I missed that. Sorry for the fuss :).
>>
>>
>> Alex
>>
> Hi, I thinks the tip tree is only applied the erratum for DT, not for ACPI, the patch
> for ACPI is not applied yet, so the work isn't finished. ??
Yes, initial thread was about ACPI part. I think Alex was asking about that.
>
> Thanks
> Ding
>
>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>> .
>>
>
> .
>


-- 
Thanks and Regards
Sanil.
"Lets make things simple and better, today, tomorrow and always!"
www.huawei.com , www.hisilicon.com
-----------------------------------
This e-mail and its attachments contain confidential information from HUAWEI, which
is intended only for the person or entity whose address is listed above. Any use of
the information contained herein in any way (including, but not limited to, total
or partial disclosure, reproduction, or dissemination) by persons other than the
intended recipient(s) is prohibited. If you receive this e-mail in error, please
notify the sender by phone or email immediately and delete it!

Re: [PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Daniel Lezcano]

On 16/02/2017 10:46, Sanil Kumar wrote:
> On 2/16/2017 3:11 PM, Ding Tianhong wrote:
>>
>> On 2017/2/16 17:32, Alexander Graf wrote:
>>>
>>> On 16/02/2017 10:14, Daniel Lezcano wrote:
>>>> On Thu, Feb 16, 2017 at 09:42:11AM +0100, Alexander Graf wrote:
>>>>
>>>> [ ... ]
>>>>
>>>>>>>> I believe Marc is looking at some rework in this area which may
>>>>>>>> enable
>>>>>>>> this, so please wait for that to appear.
>>>>>>> Yeah, I'm implementing a semi-flexible way to add new quirk
>>>>>>> types, and
>>>>>>> the last thing I want to see is two (or more) tables describing
>>>>>>> the same
>>>>>>> thing.
>>>>>> Kindly ping, if you have patches in hand, I can test against our
>>>>>> platforms,
>>>>>> thank you very much.
>>>>> Any progress on this? I'm not a big fan of stalling erratum fixes
>>>>> upstream
>>>>> because of potential future refactoring.
>>>> The fix is in the tip tree, soon in v4.11-rc1 when the merge will be
>>>> finished.
>>>>
>>>> http://goo.gl/x90qwE
>>>> http://goo.gl/asCBBD
>>> Oh, I missed that. Sorry for the fuss :).
>>>
>>>
>>> Alex
>>>
>> Hi, I thinks the tip tree is only applied the erratum for DT, not for
>> ACPI, the patch
>> for ACPI is not applied yet, so the work isn't finished. ：）
> Yes, initial thread was about ACPI part. I think Alex was asking about
> that.

Ah yes. Indeed.

Thanks.


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

[PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Daniel Lezcano]

On 16/02/2017 10:46, Sanil Kumar wrote:
> On 2/16/2017 3:11 PM, Ding Tianhong wrote:
>>
>> On 2017/2/16 17:32, Alexander Graf wrote:
>>>
>>> On 16/02/2017 10:14, Daniel Lezcano wrote:
>>>> On Thu, Feb 16, 2017 at 09:42:11AM +0100, Alexander Graf wrote:
>>>>
>>>> [ ... ]
>>>>
>>>>>>>> I believe Marc is looking at some rework in this area which may
>>>>>>>> enable
>>>>>>>> this, so please wait for that to appear.
>>>>>>> Yeah, I'm implementing a semi-flexible way to add new quirk
>>>>>>> types, and
>>>>>>> the last thing I want to see is two (or more) tables describing
>>>>>>> the same
>>>>>>> thing.
>>>>>> Kindly ping, if you have patches in hand, I can test against our
>>>>>> platforms,
>>>>>> thank you very much.
>>>>> Any progress on this? I'm not a big fan of stalling erratum fixes
>>>>> upstream
>>>>> because of potential future refactoring.
>>>> The fix is in the tip tree, soon in v4.11-rc1 when the merge will be
>>>> finished.
>>>>
>>>> http://goo.gl/x90qwE
>>>> http://goo.gl/asCBBD
>>> Oh, I missed that. Sorry for the fuss :).
>>>
>>>
>>> Alex
>>>
>> Hi, I thinks the tip tree is only applied the erratum for DT, not for
>> ACPI, the patch
>> for ACPI is not applied yet, so the work isn't finished. ??
> Yes, initial thread was about ACPI part. I think Alex was asking about
> that.

Ah yes. Indeed.

Thanks.


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

Re: [PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Marc Zyngier]

On Fri, Feb 10 2017 at 07:10:46 AM, Hanjun Guo <guohanjun@huawei.com> wrote:
> Hi Marc,
>
> On 2017/1/24 19:32, Marc Zyngier wrote:
>> On 24/01/17 10:57, Mark Rutland wrote:
>>> On Tue, Jan 24, 2017 at 06:39:51PM +0800, Hanjun Guo wrote:
>>>> From: Hanjun Guo <hanjun.guo@linaro.org>
>>>>
>>>> Add hisilicon timer specific erratum fixes.
>>>>
>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>> ---
>>>>  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
>>>> 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];
>>>> +			static_branch_enable(&arch_timer_read_ool_enabled);
>>>> +			pr_info("arch_timer: Enabling workaround for %s\n",
>>>> +				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 = {
>>>> +#ifdef CONFIG_HISILICON_ERRATUM_161010101
>>>> +	{"HISI  ", "HIP05   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>> +	{"HISI  ", "HIP06   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>> +	{"HISI  ", "HIP07   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>> +#endif
>>>>  };
>>> NAK. This duplicates logic unnecessarily (for enabling the workaround),
>>> and (ab)uses the id, which was intended to be specific to DT (since it
>>> is a DT property name).
>> Agreed, that's properly revolting.
>>
>>> 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
>>> have to duplicate the logic to enable the workaround.
>>>
>>> I believe Marc is looking at some rework in this area which may enable
>>> this, so please wait for that to appear.
>> Yeah, I'm implementing a semi-flexible way to add new quirk types, and
>> the last thing I want to see is two (or more) tables describing the same
>> thing.
>
> Kindly ping, if you have patches in hand, I can test against our platforms,
> thank you very much.

I'm planning to make something available next week. I don't think there
is any sense of urgency anyway (it is not like we've broken something,
as this platform has always been defective).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.

[PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Marc Zyngier]

On Fri, Feb 10 2017 at 07:10:46 AM, Hanjun Guo <guohanjun@huawei.com> wrote:
> Hi Marc,
>
> On 2017/1/24 19:32, Marc Zyngier wrote:
>> On 24/01/17 10:57, Mark Rutland wrote:
>>> On Tue, Jan 24, 2017 at 06:39:51PM +0800, Hanjun Guo wrote:
>>>> From: Hanjun Guo <hanjun.guo@linaro.org>
>>>>
>>>> Add hisilicon timer specific erratum fixes.
>>>>
>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>> ---
>>>>  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
>>>> 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];
>>>> +			static_branch_enable(&arch_timer_read_ool_enabled);
>>>> +			pr_info("arch_timer: Enabling workaround for %s\n",
>>>> +				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 = {
>>>> +#ifdef CONFIG_HISILICON_ERRATUM_161010101
>>>> +	{"HISI  ", "HIP05   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>> +	{"HISI  ", "HIP06   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>> +	{"HISI  ", "HIP07   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>> +#endif
>>>>  };
>>> NAK. This duplicates logic unnecessarily (for enabling the workaround),
>>> and (ab)uses the id, which was intended to be specific to DT (since it
>>> is a DT property name).
>> Agreed, that's properly revolting.
>>
>>> 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
>>> have to duplicate the logic to enable the workaround.
>>>
>>> I believe Marc is looking at some rework in this area which may enable
>>> this, so please wait for that to appear.
>> Yeah, I'm implementing a semi-flexible way to add new quirk types, and
>> the last thing I want to see is two (or more) tables describing the same
>> thing.
>
> Kindly ping, if you have patches in hand, I can test against our platforms,
> thank you very much.

I'm planning to make something available next week. I don't think there
is any sense of urgency anyway (it is not like we've broken something,
as this platform has always been defective).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny.

Re: [PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Marc Zyngier]

On 10/02/17 07:10, Hanjun Guo wrote:
> Hi Marc,
> 
> On 2017/1/24 19:32, Marc Zyngier wrote:
>> On 24/01/17 10:57, Mark Rutland wrote:
>>> On Tue, Jan 24, 2017 at 06:39:51PM +0800, Hanjun Guo wrote:
>>>> From: Hanjun Guo <hanjun.guo@linaro.org>
>>>>
>>>> Add hisilicon timer specific erratum fixes.
>>>>
>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>> ---
>>>>  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
>>>> 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];
>>>> +			static_branch_enable(&arch_timer_read_ool_enabled);
>>>> +			pr_info("arch_timer: Enabling workaround for %s\n",
>>>> +				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 = {
>>>> +#ifdef CONFIG_HISILICON_ERRATUM_161010101
>>>> +	{"HISI  ", "HIP05   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>> +	{"HISI  ", "HIP06   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>> +	{"HISI  ", "HIP07   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>> +#endif
>>>>  };
>>> NAK. This duplicates logic unnecessarily (for enabling the workaround),
>>> and (ab)uses the id, which was intended to be specific to DT (since it
>>> is a DT property name).
>> Agreed, that's properly revolting.
>>
>>> 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
>>> have to duplicate the logic to enable the workaround.
>>>
>>> I believe Marc is looking at some rework in this area which may enable
>>> this, so please wait for that to appear.
>> Yeah, I'm implementing a semi-flexible way to add new quirk types, and
>> the last thing I want to see is two (or more) tables describing the same
>> thing.
> 
> Kindly ping, if you have patches in hand, I can test against our platforms,
> thank you very much.

I've just pushed out a branch based on arm64/for-next/core, plus the
HiSi timer workaround:

https://git.kernel.org/cgit/linux/kernel/git/maz/arm-platforms.git/log/?h=timers/errata-rework

I'll post the patches for review once the merge window is open, but
hopefully that should get you going.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Marc Zyngier]

On 10/02/17 07:10, Hanjun Guo wrote:
> Hi Marc,
> 
> On 2017/1/24 19:32, Marc Zyngier wrote:
>> On 24/01/17 10:57, Mark Rutland wrote:
>>> On Tue, Jan 24, 2017 at 06:39:51PM +0800, Hanjun Guo wrote:
>>>> From: Hanjun Guo <hanjun.guo@linaro.org>
>>>>
>>>> Add hisilicon timer specific erratum fixes.
>>>>
>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>> ---
>>>>  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
>>>> 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];
>>>> +			static_branch_enable(&arch_timer_read_ool_enabled);
>>>> +			pr_info("arch_timer: Enabling workaround for %s\n",
>>>> +				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 = {
>>>> +#ifdef CONFIG_HISILICON_ERRATUM_161010101
>>>> +	{"HISI  ", "HIP05   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>> +	{"HISI  ", "HIP06   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>> +	{"HISI  ", "HIP07   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>> +#endif
>>>>  };
>>> NAK. This duplicates logic unnecessarily (for enabling the workaround),
>>> and (ab)uses the id, which was intended to be specific to DT (since it
>>> is a DT property name).
>> Agreed, that's properly revolting.
>>
>>> 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
>>> have to duplicate the logic to enable the workaround.
>>>
>>> I believe Marc is looking at some rework in this area which may enable
>>> this, so please wait for that to appear.
>> Yeah, I'm implementing a semi-flexible way to add new quirk types, and
>> the last thing I want to see is two (or more) tables describing the same
>> thing.
> 
> Kindly ping, if you have patches in hand, I can test against our platforms,
> thank you very much.

I've just pushed out a branch based on arm64/for-next/core, plus the
HiSi timer workaround:

https://git.kernel.org/cgit/linux/kernel/git/maz/arm-platforms.git/log/?h=timers/errata-rework

I'll post the patches for review once the merge window is open, but
hopefully that should get you going.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Hanjun Guo]

Hi Marc,

On 2017/2/21 3:00, Marc Zyngier wrote:
> On 10/02/17 07:10, Hanjun Guo wrote:
>> Hi Marc,
>>
>> On 2017/1/24 19:32, Marc Zyngier wrote:
>>> On 24/01/17 10:57, Mark Rutland wrote:
>>>> On Tue, Jan 24, 2017 at 06:39:51PM +0800, Hanjun Guo wrote:
>>>>> From: Hanjun Guo <hanjun.guo@linaro.org>
>>>>>
>>>>> Add hisilicon timer specific erratum fixes.
>>>>>
>>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>>> ---
>>>>>  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
>>>>> 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];
>>>>> +			static_branch_enable(&arch_timer_read_ool_enabled);
>>>>> +			pr_info("arch_timer: Enabling workaround for %s\n",
>>>>> +				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 = {
>>>>> +#ifdef CONFIG_HISILICON_ERRATUM_161010101
>>>>> +	{"HISI  ", "HIP05   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>>> +	{"HISI  ", "HIP06   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>>> +	{"HISI  ", "HIP07   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>>> +#endif
>>>>>  };
>>>> NAK. This duplicates logic unnecessarily (for enabling the workaround),
>>>> and (ab)uses the id, which was intended to be specific to DT (since it
>>>> is a DT property name).
>>> Agreed, that's properly revolting.
>>>
>>>> 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
>>>> have to duplicate the logic to enable the workaround.
>>>>
>>>> I believe Marc is looking at some rework in this area which may enable
>>>> this, so please wait for that to appear.
>>> Yeah, I'm implementing a semi-flexible way to add new quirk types, and
>>> the last thing I want to see is two (or more) tables describing the same
>>> thing.
>> Kindly ping, if you have patches in hand, I can test against our platforms,
>> thank you very much.
> I've just pushed out a branch based on arm64/for-next/core, plus the
> HiSi timer workaround:
>
> https://git.kernel.org/cgit/linux/kernel/git/maz/arm-platforms.git/log/?h=timers/errata-rework
>
> I'll post the patches for review once the merge window is open, but
> hopefully that should get you going.

Thank you very much! I prepared an ACPI patch based on your branch, I was
trying to reuse the "const void *id" for ACPI as well but we need to match the
oem_id, oem_table_id and oem_revision in GTDT table, so I introduced new
matching information for ACPI, please take a look if it's OK.

commit 02d531ae4a88c483db849a611bd9bf7c39d2e1bf
Author: Hanjun Guo <hanjun.guo@linaro.org>
Date:   Tue Feb 21 15:17:14 2017 +0800

    arm64: arch_timer: enable the erratums in ACPI way
   
    Match OEM information which are oem_id, oem_table_id and oem_revision
    to enable the erratum for specific platforms.
   
    Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 arch/arm64/include/asm/arch_timer.h  |  7 +++++++
 drivers/clocksource/arm_arch_timer.c | 31 +++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
index 1595134..79d033c 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -22,6 +22,7 @@
 #include <asm/barrier.h>
 #include <asm/sysreg.h>
 
+#include <linux/acpi.h>
 #include <linux/bug.h>
 #include <linux/init.h>
 #include <linux/jump_label.h>
@@ -42,6 +43,7 @@ enum arch_timer_erratum_match_type {
        ate_match_dt,
        ate_match_global_cap_id,
        ate_match_local_cap_id,
+ ate_match_acpi,
 };
 
 struct clock_event_device;
@@ -49,6 +51,11 @@ enum arch_timer_erratum_match_type {
 struct arch_timer_erratum_workaround {
        enum arch_timer_erratum_match_type match_type;
        const void *id;         /* Indicate the Erratum ID */
+#ifdef CONFIG_ACPI
+ char oem_id[ACPI_OEM_ID_SIZE + 1];
+ char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
+ u32 oem_revision;
+#endif
        const char *desc_str;
        u32 (*read_cntp_tval_el0)(void);
        u32 (*read_cntv_tval_el0)(void);
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index bbd9361..82ff6e4 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -363,6 +363,32 @@ bool arch_timer_check_dt_erratum(const struct arch_timer_erratum_workaround
*wa,
        return of_property_read_bool(np, wa->id);
 }
 
+#ifdef CONFIG_ACPI
+static bool
+arch_timer_check_acpi_erratum(const struct arch_timer_erratum_workaround *wa,
+                       const void *arg)
+{
+ const struct acpi_table_header *table = arg;
+
+ if (!wa->oem_id || !wa->oem_table_id)
+         return false;
+
+ if (!memcmp(wa->oem_id, table->oem_id, ACPI_OEM_ID_SIZE) &&
+     !memcmp(wa->oem_table_id, table->oem_table_id,
+             ACPI_OEM_TABLE_ID_SIZE) &&
+     wa->oem_revision == table->oem_revision) {
+         return true;
+ }
+
+ return false;
+}
+#else
+static inline bool
+arch_timer_check_acpi_erratum(const struct arch_timer_erratum_workaround *wa,
+                       const void *arg)
+{ return false; }
+#endif
+
 static
 bool arch_timer_check_global_cap_erratum(const struct arch_timer_erratum_workaround *wa,
                                         const void *arg)
@@ -440,6 +466,9 @@ static void arch_timer_check_ool_workaround(enum arch_timer_erratum_match_typ
e t
                match_fn = arch_timer_check_local_cap_erratum;
                local = true;
                break;
+ case ate_match_acpi:
+         match_fn = arch_timer_check_acpi_erratum;
+         break;
        }

        wa = arch_timer_iterate_errata(type, match_fn, arg);
@@ -1284,6 +1313,8 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *table)
        /* Check for globally applicable workarounds */
        arch_timer_check_ool_workaround(ate_match_global_cap_id, NULL);
 
+ arch_timer_check_ool_workaround(ate_match_acpi, table);
+
        arch_timer_init();
        return 0;
 }

And I got compile errors for this patch [1], seems "#include <linux/acpi.h>" in
arch/arm64/include/asm/arch_timer.h triggers the problem of head
file inclusions (too early to include <linux/sched.h>?), I'm looking into it,
if you can help to take a look too, that will be great :)

Thanks
Hanjun

[1]:
In file included from ./arch/arm64/include/asm/spinlock.h:21:0,
                 from ./include/linux/spinlock.h:87,
                 from ./include/linux/seqlock.h:35,
                 from ./include/linux/time.h:5,
                 from ./include/uapi/linux/timex.h:56,
                 from ./include/linux/timex.h:56,
                 from ./include/linux/sched.h:19,
                 from arch/arm64/kernel/asm-offsets.c:21:
./arch/arm64/include/asm/compat.h: In function ‘arch_compat_alloc_user_space’:
./arch/arm64/include/asm/processor.h:157:11: error: implicit declaration of function ‘task_stack_page’ [-Werror=implicit-function-declaration]
  ((struct pt_regs *)(THREAD_START_SP + task_stack_page(p)) - 1)
           ^
./arch/arm64/include/asm/compat.h:236:57: note: in expansion of macro ‘task_pt_regs’
 #define compat_user_stack_pointer() (user_stack_pointer(task_pt_regs(current)))
                                                         ^
./arch/arm64/include/asm/compat.h:240:24: note: in expansion of macro ‘compat_user_stack_pointer’
  return (void __user *)compat_user_stack_pointer() - len;
                        ^
./arch/arm64/include/asm/processor.h:157:3: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
  ((struct pt_regs *)(THREAD_START_SP + task_stack_page(p)) - 1)
   ^
./arch/arm64/include/asm/compat.h:236:57: note: in expansion of macro ‘task_pt_regs’
 #define compat_user_stack_pointer() (user_stack_pointer(task_pt_regs(current)))
                                                         ^
./arch/arm64/include/asm/compat.h:240:24: note: in expansion of macro ‘compat_user_stack_pointer’
  return (void __user *)compat_user_stack_pointer() - len;
                        ^
In file included from ./include/linux/kernel.h:13:0,
                 from ./include/linux/sched.h:17,
                 from arch/arm64/kernel/asm-offsets.c:21:
./include/linux/ratelimit.h: In function ‘ratelimit_state_exit’:
./include/linux/ratelimit.h:62:11: error: dereferencing pointer to incomplete type
    current->comm, rs->missed);
           ^
./include/linux/printk.h:294:37: note: in definition of macro ‘pr_warning’
  printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
                                     ^
./include/linux/ratelimit.h:61:3: note: in expansion of macro ‘pr_warn’
   pr_warn("%s: %d output lines suppressed due to ratelimiting\n",
   ^
In file included from ./arch/arm64/include/asm/arch_timer.h:25:0,
                 from ./arch/arm64/include/asm/timex.h:19,
                 from ./include/linux/timex.h:65,
                 from ./include/linux/sched.h:19,
                 from arch/arm64/kernel/asm-offsets.c:21:
./include/linux/acpi.h: At top level:
./include/linux/acpi.h:604:37: warning: ‘struct arch_timer_mem’ declared inside parameter list
 int acpi_arch_timer_mem_init(struct arch_timer_mem *data, int *timer_count);
                                     ^
./include/linux/acpi.h:604:37: warning: its scope is only this definition or declaration, which is probably not what you want
In file included from arch/arm64/kernel/asm-offsets.c:21:0:
./include/linux/sched.h:3190:21: error: conflicting types for ‘task_stack_page’
 static inline void *task_stack_page(const struct task_struct *task)
                     ^
In file included from ./arch/arm64/include/asm/spinlock.h:21:0,
                 from ./include/linux/spinlock.h:87,
                 from ./include/linux/seqlock.h:35,
                 from ./include/linux/time.h:5,
                 from ./include/uapi/linux/timex.h:56,
                 from ./include/linux/timex.h:56,
                 from ./include/linux/sched.h:19,
                 from arch/arm64/kernel/asm-offsets.c:21:
./arch/arm64/include/asm/processor.h:157:40: note: previous implicit declaration of ‘task_stack_page’ was here
  ((struct pt_regs *)(THREAD_START_SP + task_stack_page(p)) - 1)
                                        ^
./arch/arm64/include/asm/compat.h:236:57: note: in expansion of macro ‘task_pt_regs’
 #define compat_user_stack_pointer() (user_stack_pointer(task_pt_regs(current)))
                                                         ^
./arch/arm64/include/asm/compat.h:240:24: note: in expansion of macro ‘compat_user_stack_pointer’
  return (void __user *)compat_user_stack_pointer() - len;
                        ^
cc1: some warnings being treated as errors
make[1]: *** [arch/arm64/kernel/asm-offsets.s] Error 1
make: *** [prepare0] Error 2

[PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Hanjun Guo]

Hi Marc,

On 2017/2/21 3:00, Marc Zyngier wrote:
> On 10/02/17 07:10, Hanjun Guo wrote:
>> Hi Marc,
>>
>> On 2017/1/24 19:32, Marc Zyngier wrote:
>>> On 24/01/17 10:57, Mark Rutland wrote:
>>>> On Tue, Jan 24, 2017 at 06:39:51PM +0800, Hanjun Guo wrote:
>>>>> From: Hanjun Guo <hanjun.guo@linaro.org>
>>>>>
>>>>> Add hisilicon timer specific erratum fixes.
>>>>>
>>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>>> ---
>>>>>  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
>>>>> 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];
>>>>> +			static_branch_enable(&arch_timer_read_ool_enabled);
>>>>> +			pr_info("arch_timer: Enabling workaround for %s\n",
>>>>> +				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 = {
>>>>> +#ifdef CONFIG_HISILICON_ERRATUM_161010101
>>>>> +	{"HISI  ", "HIP05   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>>> +	{"HISI  ", "HIP06   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>>> +	{"HISI  ", "HIP07   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>>> +#endif
>>>>>  };
>>>> NAK. This duplicates logic unnecessarily (for enabling the workaround),
>>>> and (ab)uses the id, which was intended to be specific to DT (since it
>>>> is a DT property name).
>>> Agreed, that's properly revolting.
>>>
>>>> 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
>>>> have to duplicate the logic to enable the workaround.
>>>>
>>>> I believe Marc is looking at some rework in this area which may enable
>>>> this, so please wait for that to appear.
>>> Yeah, I'm implementing a semi-flexible way to add new quirk types, and
>>> the last thing I want to see is two (or more) tables describing the same
>>> thing.
>> Kindly ping, if you have patches in hand, I can test against our platforms,
>> thank you very much.
> I've just pushed out a branch based on arm64/for-next/core, plus the
> HiSi timer workaround:
>
> https://git.kernel.org/cgit/linux/kernel/git/maz/arm-platforms.git/log/?h=timers/errata-rework
>
> I'll post the patches for review once the merge window is open, but
> hopefully that should get you going.

Thank you very much! I prepared an ACPI patch based on your branch, I was
trying to reuse the "const void *id" for ACPI as well but we need to match the
oem_id, oem_table_id and oem_revision in GTDT table, so I introduced new
matching information for ACPI, please take a look if it's OK.

commit 02d531ae4a88c483db849a611bd9bf7c39d2e1bf
Author: Hanjun Guo <hanjun.guo@linaro.org>
Date:   Tue Feb 21 15:17:14 2017 +0800

    arm64: arch_timer: enable the erratums in ACPI way
   
    Match OEM information which are oem_id, oem_table_id and oem_revision
    to enable the erratum for specific platforms.
   
    Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 arch/arm64/include/asm/arch_timer.h  |  7 +++++++
 drivers/clocksource/arm_arch_timer.c | 31 +++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
index 1595134..79d033c 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -22,6 +22,7 @@
 #include <asm/barrier.h>
 #include <asm/sysreg.h>
 
+#include <linux/acpi.h>
 #include <linux/bug.h>
 #include <linux/init.h>
 #include <linux/jump_label.h>
@@ -42,6 +43,7 @@ enum arch_timer_erratum_match_type {
        ate_match_dt,
        ate_match_global_cap_id,
        ate_match_local_cap_id,
+ ate_match_acpi,
 };
 
 struct clock_event_device;
@@ -49,6 +51,11 @@ enum arch_timer_erratum_match_type {
 struct arch_timer_erratum_workaround {
        enum arch_timer_erratum_match_type match_type;
        const void *id;         /* Indicate the Erratum ID */
+#ifdef CONFIG_ACPI
+ char oem_id[ACPI_OEM_ID_SIZE + 1];
+ char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
+ u32 oem_revision;
+#endif
        const char *desc_str;
        u32 (*read_cntp_tval_el0)(void);
        u32 (*read_cntv_tval_el0)(void);
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index bbd9361..82ff6e4 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -363,6 +363,32 @@ bool arch_timer_check_dt_erratum(const struct arch_timer_erratum_workaround
*wa,
        return of_property_read_bool(np, wa->id);
 }
 
+#ifdef CONFIG_ACPI
+static bool
+arch_timer_check_acpi_erratum(const struct arch_timer_erratum_workaround *wa,
+                       const void *arg)
+{
+ const struct acpi_table_header *table = arg;
+
+ if (!wa->oem_id || !wa->oem_table_id)
+         return false;
+
+ if (!memcmp(wa->oem_id, table->oem_id, ACPI_OEM_ID_SIZE) &&
+     !memcmp(wa->oem_table_id, table->oem_table_id,
+             ACPI_OEM_TABLE_ID_SIZE) &&
+     wa->oem_revision == table->oem_revision) {
+         return true;
+ }
+
+ return false;
+}
+#else
+static inline bool
+arch_timer_check_acpi_erratum(const struct arch_timer_erratum_workaround *wa,
+                       const void *arg)
+{ return false; }
+#endif
+
 static
 bool arch_timer_check_global_cap_erratum(const struct arch_timer_erratum_workaround *wa,
                                         const void *arg)
@@ -440,6 +466,9 @@ static void arch_timer_check_ool_workaround(enum arch_timer_erratum_match_typ
e t
                match_fn = arch_timer_check_local_cap_erratum;
                local = true;
                break;
+ case ate_match_acpi:
+         match_fn = arch_timer_check_acpi_erratum;
+         break;
        }

        wa = arch_timer_iterate_errata(type, match_fn, arg);
@@ -1284,6 +1313,8 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *table)
        /* Check for globally applicable workarounds */
        arch_timer_check_ool_workaround(ate_match_global_cap_id, NULL);
 
+ arch_timer_check_ool_workaround(ate_match_acpi, table);
+
        arch_timer_init();
        return 0;
 }

And I got compile errors for this patch [1], seems "#include <linux/acpi.h>" in
arch/arm64/include/asm/arch_timer.h triggers the problem of head
file inclusions (too early to include <linux/sched.h>?), I'm looking into it,
if you can help to take a look too, that will be great :)

Thanks
Hanjun

[1]:
In file included from ./arch/arm64/include/asm/spinlock.h:21:0,
                 from ./include/linux/spinlock.h:87,
                 from ./include/linux/seqlock.h:35,
                 from ./include/linux/time.h:5,
                 from ./include/uapi/linux/timex.h:56,
                 from ./include/linux/timex.h:56,
                 from ./include/linux/sched.h:19,
                 from arch/arm64/kernel/asm-offsets.c:21:
./arch/arm64/include/asm/compat.h: In function ?arch_compat_alloc_user_space?:
./arch/arm64/include/asm/processor.h:157:11: error: implicit declaration of function ?task_stack_page? [-Werror=implicit-function-declaration]
  ((struct pt_regs *)(THREAD_START_SP + task_stack_page(p)) - 1)
           ^
./arch/arm64/include/asm/compat.h:236:57: note: in expansion of macro ?task_pt_regs?
 #define compat_user_stack_pointer() (user_stack_pointer(task_pt_regs(current)))
                                                         ^
./arch/arm64/include/asm/compat.h:240:24: note: in expansion of macro ?compat_user_stack_pointer?
  return (void __user *)compat_user_stack_pointer() - len;
                        ^
./arch/arm64/include/asm/processor.h:157:3: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
  ((struct pt_regs *)(THREAD_START_SP + task_stack_page(p)) - 1)
   ^
./arch/arm64/include/asm/compat.h:236:57: note: in expansion of macro ?task_pt_regs?
 #define compat_user_stack_pointer() (user_stack_pointer(task_pt_regs(current)))
                                                         ^
./arch/arm64/include/asm/compat.h:240:24: note: in expansion of macro ?compat_user_stack_pointer?
  return (void __user *)compat_user_stack_pointer() - len;
                        ^
In file included from ./include/linux/kernel.h:13:0,
                 from ./include/linux/sched.h:17,
                 from arch/arm64/kernel/asm-offsets.c:21:
./include/linux/ratelimit.h: In function ?ratelimit_state_exit?:
./include/linux/ratelimit.h:62:11: error: dereferencing pointer to incomplete type
    current->comm, rs->missed);
           ^
./include/linux/printk.h:294:37: note: in definition of macro ?pr_warning?
  printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
                                     ^
./include/linux/ratelimit.h:61:3: note: in expansion of macro ?pr_warn?
   pr_warn("%s: %d output lines suppressed due to ratelimiting\n",
   ^
In file included from ./arch/arm64/include/asm/arch_timer.h:25:0,
                 from ./arch/arm64/include/asm/timex.h:19,
                 from ./include/linux/timex.h:65,
                 from ./include/linux/sched.h:19,
                 from arch/arm64/kernel/asm-offsets.c:21:
./include/linux/acpi.h: At top level:
./include/linux/acpi.h:604:37: warning: ?struct arch_timer_mem? declared inside parameter list
 int acpi_arch_timer_mem_init(struct arch_timer_mem *data, int *timer_count);
                                     ^
./include/linux/acpi.h:604:37: warning: its scope is only this definition or declaration, which is probably not what you want
In file included from arch/arm64/kernel/asm-offsets.c:21:0:
./include/linux/sched.h:3190:21: error: conflicting types for ?task_stack_page?
 static inline void *task_stack_page(const struct task_struct *task)
                     ^
In file included from ./arch/arm64/include/asm/spinlock.h:21:0,
                 from ./include/linux/spinlock.h:87,
                 from ./include/linux/seqlock.h:35,
                 from ./include/linux/time.h:5,
                 from ./include/uapi/linux/timex.h:56,
                 from ./include/linux/timex.h:56,
                 from ./include/linux/sched.h:19,
                 from arch/arm64/kernel/asm-offsets.c:21:
./arch/arm64/include/asm/processor.h:157:40: note: previous implicit declaration of ?task_stack_page? was here
  ((struct pt_regs *)(THREAD_START_SP + task_stack_page(p)) - 1)
                                        ^
./arch/arm64/include/asm/compat.h:236:57: note: in expansion of macro ?task_pt_regs?
 #define compat_user_stack_pointer() (user_stack_pointer(task_pt_regs(current)))
                                                         ^
./arch/arm64/include/asm/compat.h:240:24: note: in expansion of macro ?compat_user_stack_pointer?
  return (void __user *)compat_user_stack_pointer() - len;
                        ^
cc1: some warnings being treated as errors
make[1]: *** [arch/arm64/kernel/asm-offsets.s] Error 1
make: *** [prepare0] Error 2

Re: [PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Ding Tianhong]

On 2017/2/21 3:00, Marc Zyngier wrote:
> On 10/02/17 07:10, Hanjun Guo wrote:
>> Hi Marc,
>>
>> On 2017/1/24 19:32, Marc Zyngier wrote:
>>> On 24/01/17 10:57, Mark Rutland wrote:
>>>> On Tue, Jan 24, 2017 at 06:39:51PM +0800, Hanjun Guo wrote:
>>>>> From: Hanjun Guo <hanjun.guo@linaro.org>
>>>>>
>>>>> Add hisilicon timer specific erratum fixes.
>>>>>
>>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>>> ---
>>>>>  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
>>>>> 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];
>>>>> +			static_branch_enable(&arch_timer_read_ool_enabled);
>>>>> +			pr_info("arch_timer: Enabling workaround for %s\n",
>>>>> +				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 = {
>>>>> +#ifdef CONFIG_HISILICON_ERRATUM_161010101
>>>>> +	{"HISI  ", "HIP05   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>>> +	{"HISI  ", "HIP06   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>>> +	{"HISI  ", "HIP07   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>>> +#endif
>>>>>  };
>>>> NAK. This duplicates logic unnecessarily (for enabling the workaround),
>>>> and (ab)uses the id, which was intended to be specific to DT (since it
>>>> is a DT property name).
>>> Agreed, that's properly revolting.
>>>
>>>> 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
>>>> have to duplicate the logic to enable the workaround.
>>>>
>>>> I believe Marc is looking at some rework in this area which may enable
>>>> this, so please wait for that to appear.
>>> Yeah, I'm implementing a semi-flexible way to add new quirk types, and
>>> the last thing I want to see is two (or more) tables describing the same
>>> thing.
>>
>> Kindly ping, if you have patches in hand, I can test against our platforms,
>> thank you very much.
> 
> I've just pushed out a branch based on arm64/for-next/core, plus the
> HiSi timer workaround:
> 
> https://git.kernel.org/cgit/linux/kernel/git/maz/arm-platforms.git/log/?h=timers/errata-rework
> 
> I'll post the patches for review once the merge window is open, but
> hopefully that should get you going.
> 
> Thanks,
> 


Test this branch and could work fine for Hisilicon board, great work !

Still need time to review the details, but this should not seem very difficult. :)

Thanks
Ding

> 	M.
> 

[PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Ding Tianhong]

On 2017/2/21 3:00, Marc Zyngier wrote:
> On 10/02/17 07:10, Hanjun Guo wrote:
>> Hi Marc,
>>
>> On 2017/1/24 19:32, Marc Zyngier wrote:
>>> On 24/01/17 10:57, Mark Rutland wrote:
>>>> On Tue, Jan 24, 2017 at 06:39:51PM +0800, Hanjun Guo wrote:
>>>>> From: Hanjun Guo <hanjun.guo@linaro.org>
>>>>>
>>>>> Add hisilicon timer specific erratum fixes.
>>>>>
>>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>>> ---
>>>>>  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
>>>>> 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];
>>>>> +			static_branch_enable(&arch_timer_read_ool_enabled);
>>>>> +			pr_info("arch_timer: Enabling workaround for %s\n",
>>>>> +				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 = {
>>>>> +#ifdef CONFIG_HISILICON_ERRATUM_161010101
>>>>> +	{"HISI  ", "HIP05   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>>> +	{"HISI  ", "HIP06   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>>> +	{"HISI  ", "HIP07   ", 0, &hisi_erratum_workaroud_enable, "hisilicon,erratum-161010101"},
>>>>> +#endif
>>>>>  };
>>>> NAK. This duplicates logic unnecessarily (for enabling the workaround),
>>>> and (ab)uses the id, which was intended to be specific to DT (since it
>>>> is a DT property name).
>>> Agreed, that's properly revolting.
>>>
>>>> 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
>>>> have to duplicate the logic to enable the workaround.
>>>>
>>>> I believe Marc is looking at some rework in this area which may enable
>>>> this, so please wait for that to appear.
>>> Yeah, I'm implementing a semi-flexible way to add new quirk types, and
>>> the last thing I want to see is two (or more) tables describing the same
>>> thing.
>>
>> Kindly ping, if you have patches in hand, I can test against our platforms,
>> thank you very much.
> 
> I've just pushed out a branch based on arm64/for-next/core, plus the
> HiSi timer workaround:
> 
> https://git.kernel.org/cgit/linux/kernel/git/maz/arm-platforms.git/log/?h=timers/errata-rework
> 
> I'll post the patches for review once the merge window is open, but
> hopefully that should get you going.
> 
> Thanks,
> 


Test this branch and could work fine for Hisilicon board, great work !

Still need time to review the details, but this should not seem very difficult. :)

Thanks
Ding

> 	M.
> 

Re: [PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Marc Zyngier]

On Tue, Feb 21 2017 at 11:56:31 am GMT, Hanjun Guo <guohanjun@huawei.com> wrote:
> Hi Marc,

[...]

> Thank you very much! I prepared an ACPI patch based on your branch, I was
> trying to reuse the "const void *id" for ACPI as well but we need to match the
> oem_id, oem_table_id and oem_revision in GTDT table, so I introduced new
> matching information for ACPI, please take a look if it's OK.
>
> commit 02d531ae4a88c483db849a611bd9bf7c39d2e1bf
> Author: Hanjun Guo <hanjun.guo@linaro.org>
> Date:   Tue Feb 21 15:17:14 2017 +0800
>
>     arm64: arch_timer: enable the erratums in ACPI way
>    
>     Match OEM information which are oem_id, oem_table_id and oem_revision
>     to enable the erratum for specific platforms.
>    
>     Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> ---
>  arch/arm64/include/asm/arch_timer.h  |  7 +++++++
>  drivers/clocksource/arm_arch_timer.c | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+)
>
> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
> index 1595134..79d033c 100644
> --- a/arch/arm64/include/asm/arch_timer.h
> +++ b/arch/arm64/include/asm/arch_timer.h
> @@ -22,6 +22,7 @@
>  #include <asm/barrier.h>
>  #include <asm/sysreg.h>
>  
> +#include <linux/acpi.h>
>  #include <linux/bug.h>
>  #include <linux/init.h>
>  #include <linux/jump_label.h>
> @@ -42,6 +43,7 @@ enum arch_timer_erratum_match_type {
>         ate_match_dt,
>         ate_match_global_cap_id,
>         ate_match_local_cap_id,
> + ate_match_acpi,
>  };
>  
>  struct clock_event_device;
> @@ -49,6 +51,11 @@ enum arch_timer_erratum_match_type {
>  struct arch_timer_erratum_workaround {
>         enum arch_timer_erratum_match_type match_type;
>         const void *id;         /* Indicate the Erratum ID */
> +#ifdef CONFIG_ACPI
> + char oem_id[ACPI_OEM_ID_SIZE + 1];
> + char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
> + u32 oem_revision;
> +#endif
>         const char *desc_str;
>         u32 (*read_cntp_tval_el0)(void);
>         u32 (*read_cntv_tval_el0)(void);
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index bbd9361..82ff6e4 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -363,6 +363,32 @@ bool arch_timer_check_dt_erratum(const struct arch_timer_erratum_workaround
> *wa,
>         return of_property_read_bool(np, wa->id);
>  }
>  
> +#ifdef CONFIG_ACPI
> +static bool
> +arch_timer_check_acpi_erratum(const struct arch_timer_erratum_workaround *wa,
> +                       const void *arg)
> +{
> + const struct acpi_table_header *table = arg;
> +
> + if (!wa->oem_id || !wa->oem_table_id)
> +         return false;
> +
> + if (!memcmp(wa->oem_id, table->oem_id, ACPI_OEM_ID_SIZE) &&
> +     !memcmp(wa->oem_table_id, table->oem_table_id,
> +             ACPI_OEM_TABLE_ID_SIZE) &&
> +     wa->oem_revision == table->oem_revision) {
> +         return true;
> + }
> +
> + return false;
> +}
> +#else
> +static inline bool
> +arch_timer_check_acpi_erratum(const struct arch_timer_erratum_workaround *wa,
> +                       const void *arg)
> +{ return false; }
> +#endif
> +
>  static
>  bool arch_timer_check_global_cap_erratum(const struct arch_timer_erratum_workaround *wa,
>                                          const void *arg)
> @@ -440,6 +466,9 @@ static void arch_timer_check_ool_workaround(enum arch_timer_erratum_match_typ
> e t
>                 match_fn = arch_timer_check_local_cap_erratum;
>                 local = true;
>                 break;
> + case ate_match_acpi:
> +         match_fn = arch_timer_check_acpi_erratum;
> +         break;
>         }
>
>         wa = arch_timer_iterate_errata(type, match_fn, arg);
> @@ -1284,6 +1313,8 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *table)
>         /* Check for globally applicable workarounds */
>         arch_timer_check_ool_workaround(ate_match_global_cap_id, NULL);
>  
> + arch_timer_check_ool_workaround(ate_match_acpi, table);
> +
>         arch_timer_init();
>         return 0;
>  }
>
> And I got compile errors for this patch [1], seems "#include <linux/acpi.h>" in
> arch/arm64/include/asm/arch_timer.h triggers the problem of head
> file inclusions (too early to include <linux/sched.h>?), I'm looking into it,
> if you can help to take a look too, that will be great :)

This is really much more complicated (and uglier) than what I had in
mind, and I don't want to add more cruft to the erratum description
structure. So instead of trying to explain what I wanted to see, here's
the patches I whipped together.

Please let me know if they work for you (as I have no way of testing
them).

Thanks,

	M.

>From 50eb735436f9f6482285939b39b52d858d848537 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <marc.zyngier@arm.com>
Date: Tue, 21 Feb 2017 14:37:30 +0000
Subject: [PATCH 1/2] arm64: arch_timer: Allow erratum matching with ACPI OEM
 information

Just as we're able to identify a broken platform using some DT
information, let's enable a way to spot the offenders with ACPI.

The difference is that we can only match on some OEM info instead
of implementation-specific properties. So in order to avoid the
insane multiplication of errata structures, we allow an array
of OEM descriptions to be attached to an erratum structure.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/arch_timer.h  |  1 +
 drivers/clocksource/arm_arch_timer.c | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
index 159513479f6e..2e635deba776 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -42,6 +42,7 @@ enum arch_timer_erratum_match_type {
 	ate_match_dt,
 	ate_match_global_cap_id,
 	ate_match_local_cap_id,
+	ate_match_acpi_oem_info,
 };
 
 struct clock_event_device;
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index bbd9361c7d66..1de18113e5ae 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -189,6 +189,12 @@ static struct cyclecounter cyclecounter = {
 	.mask	= CLOCKSOURCE_MASK(56),
 };
 
+struct ate_acpi_oem_info {
+	char oem_id[ACPI_OEM_ID_SIZE + 1];
+	char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
+	u32 oem_revision;
+};
+
 #ifdef CONFIG_FSL_ERRATUM_A008585
 /*
  * The number of retries is an arbitrary value well beyond the highest number
@@ -377,6 +383,29 @@ bool arch_timer_check_local_cap_erratum(const struct arch_timer_erratum_workarou
 	return this_cpu_has_cap((uintptr_t)wa->id);
 }
 
+
+static
+bool arch_timer_check_acpi_oem_erratum(const struct arch_timer_erratum_workaround *wa,
+				       const void *arg)
+{
+	static const struct ate_acpi_oem_info empty_oem_info = {};
+	const struct ate_acpi_oem_info *info = wa->id;
+	const struct acpi_table_header *table = arg;
+
+	/* Iterate over the ACPI EOM info array, looking for a match */
+	while (memcmp(info, &empty_oem_info, sizeof(*info))) {
+		if (!memcmp(info->oem_id, table->oem_id, ACPI_OEM_ID_SIZE) &&
+		    !memcmp(info->oem_table_id, table->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
+		    info->oem_revision == table->oem_revision)
+
+			return true;
+
+		info++;
+	}
+
+	return false;
+}
+
 static const struct arch_timer_erratum_workaround *
 arch_timer_iterate_errata(enum arch_timer_erratum_match_type type,
 			  ate_match_fn_t match_fn,
@@ -440,6 +469,9 @@ static void arch_timer_check_ool_workaround(enum arch_timer_erratum_match_type t
 		match_fn = arch_timer_check_local_cap_erratum;
 		local = true;
 		break;
+	case ate_match_acpi_oem_info:
+		match_fn = arch_timer_check_acpi_oem_erratum;
+		break;
 	}
 
 	wa = arch_timer_iterate_errata(type, match_fn, arg);
@@ -1283,6 +1315,7 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *table)
 
 	/* Check for globally applicable workarounds */
 	arch_timer_check_ool_workaround(ate_match_global_cap_id, NULL);
+	arch_timer_check_ool_workaround(ate_match_acpi_oem_info, table);
 
 	arch_timer_init();
 	return 0;
-- 
2.11.0


>From 857eae645c7dd0cf6c7cd652dd87bbe8041a5d70 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <marc.zyngier@arm.com>
Date: Tue, 21 Feb 2017 15:04:27 +0000
Subject: [PATCH 2/2] arm64: arch_timer: Add HISILICON_ERRATUM_161010101 ACPI
 matching data

In order to deal with ACPI enabled platforms suffering from the
HISILICON_ERRATUM_161010101, let's add the required OEM data that
allow the workaround to be enabled.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/clocksource/arm_arch_timer.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 1de18113e5ae..062046bca9eb 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -269,6 +269,25 @@ static u64 notrace hisi_161010101_read_cntvct_el0(void)
 {
 	return __hisi_161010101_read_reg(cntvct_el0);
 }
+
+static struct ate_acpi_oem_info hisi_161010101_oem_info[] = {
+	{
+		.oem_id		= "HISI  ",
+		.oem_table_id	= "HIP05   ",
+		.oem_revision	= 0,
+	},
+	{
+		.oem_id		= "HISI  ",
+		.oem_table_id	= "HIP06   ",
+		.oem_revision	= 0,
+	},
+	{
+		.oem_id		= "HISI  ",
+		.oem_table_id	= "HIP07   ",
+		.oem_revision	= 0,
+	},
+	{ },
+};
 #endif
 
 #ifdef CONFIG_ARM64_ERRATUM_858921
@@ -346,6 +365,16 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
 		.set_next_event_phys = erratum_set_next_event_tval_phys,
 		.set_next_event_virt = erratum_set_next_event_tval_virt,
 	},
+	{
+		.match_type = ate_match_acpi_oem_info,
+		.id = hisi_161010101_oem_info,
+		.desc_str = "HiSilicon erratum 161010101",
+		.read_cntp_tval_el0 = hisi_161010101_read_cntp_tval_el0,
+		.read_cntv_tval_el0 = hisi_161010101_read_cntv_tval_el0,
+		.read_cntvct_el0 = hisi_161010101_read_cntvct_el0,
+		.set_next_event_phys = erratum_set_next_event_tval_phys,
+		.set_next_event_virt = erratum_set_next_event_tval_virt,
+	},
 #endif
 #ifdef CONFIG_ARM64_ERRATUM_858921
 	{
-- 
2.11.0


-- 
Jazz is not dead, it just smell funny.

[PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Marc Zyngier]

On Tue, Feb 21 2017 at 11:56:31 am GMT, Hanjun Guo <guohanjun@huawei.com> wrote:
> Hi Marc,

[...]

> Thank you very much! I prepared an ACPI patch based on your branch, I was
> trying to reuse the "const void *id" for ACPI as well but we need to match the
> oem_id, oem_table_id and oem_revision in GTDT table, so I introduced new
> matching information for ACPI, please take a look if it's OK.
>
> commit 02d531ae4a88c483db849a611bd9bf7c39d2e1bf
> Author: Hanjun Guo <hanjun.guo@linaro.org>
> Date:   Tue Feb 21 15:17:14 2017 +0800
>
>     arm64: arch_timer: enable the erratums in ACPI way
>    
>     Match OEM information which are oem_id, oem_table_id and oem_revision
>     to enable the erratum for specific platforms.
>    
>     Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> ---
>  arch/arm64/include/asm/arch_timer.h  |  7 +++++++
>  drivers/clocksource/arm_arch_timer.c | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+)
>
> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
> index 1595134..79d033c 100644
> --- a/arch/arm64/include/asm/arch_timer.h
> +++ b/arch/arm64/include/asm/arch_timer.h
> @@ -22,6 +22,7 @@
>  #include <asm/barrier.h>
>  #include <asm/sysreg.h>
>  
> +#include <linux/acpi.h>
>  #include <linux/bug.h>
>  #include <linux/init.h>
>  #include <linux/jump_label.h>
> @@ -42,6 +43,7 @@ enum arch_timer_erratum_match_type {
>         ate_match_dt,
>         ate_match_global_cap_id,
>         ate_match_local_cap_id,
> + ate_match_acpi,
>  };
>  
>  struct clock_event_device;
> @@ -49,6 +51,11 @@ enum arch_timer_erratum_match_type {
>  struct arch_timer_erratum_workaround {
>         enum arch_timer_erratum_match_type match_type;
>         const void *id;         /* Indicate the Erratum ID */
> +#ifdef CONFIG_ACPI
> + char oem_id[ACPI_OEM_ID_SIZE + 1];
> + char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
> + u32 oem_revision;
> +#endif
>         const char *desc_str;
>         u32 (*read_cntp_tval_el0)(void);
>         u32 (*read_cntv_tval_el0)(void);
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index bbd9361..82ff6e4 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -363,6 +363,32 @@ bool arch_timer_check_dt_erratum(const struct arch_timer_erratum_workaround
> *wa,
>         return of_property_read_bool(np, wa->id);
>  }
>  
> +#ifdef CONFIG_ACPI
> +static bool
> +arch_timer_check_acpi_erratum(const struct arch_timer_erratum_workaround *wa,
> +                       const void *arg)
> +{
> + const struct acpi_table_header *table = arg;
> +
> + if (!wa->oem_id || !wa->oem_table_id)
> +         return false;
> +
> + if (!memcmp(wa->oem_id, table->oem_id, ACPI_OEM_ID_SIZE) &&
> +     !memcmp(wa->oem_table_id, table->oem_table_id,
> +             ACPI_OEM_TABLE_ID_SIZE) &&
> +     wa->oem_revision == table->oem_revision) {
> +         return true;
> + }
> +
> + return false;
> +}
> +#else
> +static inline bool
> +arch_timer_check_acpi_erratum(const struct arch_timer_erratum_workaround *wa,
> +                       const void *arg)
> +{ return false; }
> +#endif
> +
>  static
>  bool arch_timer_check_global_cap_erratum(const struct arch_timer_erratum_workaround *wa,
>                                          const void *arg)
> @@ -440,6 +466,9 @@ static void arch_timer_check_ool_workaround(enum arch_timer_erratum_match_typ
> e t
>                 match_fn = arch_timer_check_local_cap_erratum;
>                 local = true;
>                 break;
> + case ate_match_acpi:
> +         match_fn = arch_timer_check_acpi_erratum;
> +         break;
>         }
>
>         wa = arch_timer_iterate_errata(type, match_fn, arg);
> @@ -1284,6 +1313,8 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *table)
>         /* Check for globally applicable workarounds */
>         arch_timer_check_ool_workaround(ate_match_global_cap_id, NULL);
>  
> + arch_timer_check_ool_workaround(ate_match_acpi, table);
> +
>         arch_timer_init();
>         return 0;
>  }
>
> And I got compile errors for this patch [1], seems "#include <linux/acpi.h>" in
> arch/arm64/include/asm/arch_timer.h triggers the problem of head
> file inclusions (too early to include <linux/sched.h>?), I'm looking into it,
> if you can help to take a look too, that will be great :)

This is really much more complicated (and uglier) than what I had in
mind, and I don't want to add more cruft to the erratum description
structure. So instead of trying to explain what I wanted to see, here's
the patches I whipped together.

Please let me know if they work for you (as I have no way of testing
them).

Thanks,

	M.

>From 50eb735436f9f6482285939b39b52d858d848537 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <marc.zyngier@arm.com>
Date: Tue, 21 Feb 2017 14:37:30 +0000
Subject: [PATCH 1/2] arm64: arch_timer: Allow erratum matching with ACPI OEM
 information

Just as we're able to identify a broken platform using some DT
information, let's enable a way to spot the offenders with ACPI.

The difference is that we can only match on some OEM info instead
of implementation-specific properties. So in order to avoid the
insane multiplication of errata structures, we allow an array
of OEM descriptions to be attached to an erratum structure.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/arch_timer.h  |  1 +
 drivers/clocksource/arm_arch_timer.c | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
index 159513479f6e..2e635deba776 100644
--- a/arch/arm64/include/asm/arch_timer.h
+++ b/arch/arm64/include/asm/arch_timer.h
@@ -42,6 +42,7 @@ enum arch_timer_erratum_match_type {
 	ate_match_dt,
 	ate_match_global_cap_id,
 	ate_match_local_cap_id,
+	ate_match_acpi_oem_info,
 };
 
 struct clock_event_device;
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index bbd9361c7d66..1de18113e5ae 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -189,6 +189,12 @@ static struct cyclecounter cyclecounter = {
 	.mask	= CLOCKSOURCE_MASK(56),
 };
 
+struct ate_acpi_oem_info {
+	char oem_id[ACPI_OEM_ID_SIZE + 1];
+	char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
+	u32 oem_revision;
+};
+
 #ifdef CONFIG_FSL_ERRATUM_A008585
 /*
  * The number of retries is an arbitrary value well beyond the highest number
@@ -377,6 +383,29 @@ bool arch_timer_check_local_cap_erratum(const struct arch_timer_erratum_workarou
 	return this_cpu_has_cap((uintptr_t)wa->id);
 }
 
+
+static
+bool arch_timer_check_acpi_oem_erratum(const struct arch_timer_erratum_workaround *wa,
+				       const void *arg)
+{
+	static const struct ate_acpi_oem_info empty_oem_info = {};
+	const struct ate_acpi_oem_info *info = wa->id;
+	const struct acpi_table_header *table = arg;
+
+	/* Iterate over the ACPI EOM info array, looking for a match */
+	while (memcmp(info, &empty_oem_info, sizeof(*info))) {
+		if (!memcmp(info->oem_id, table->oem_id, ACPI_OEM_ID_SIZE) &&
+		    !memcmp(info->oem_table_id, table->oem_table_id, ACPI_OEM_TABLE_ID_SIZE) &&
+		    info->oem_revision == table->oem_revision)
+
+			return true;
+
+		info++;
+	}
+
+	return false;
+}
+
 static const struct arch_timer_erratum_workaround *
 arch_timer_iterate_errata(enum arch_timer_erratum_match_type type,
 			  ate_match_fn_t match_fn,
@@ -440,6 +469,9 @@ static void arch_timer_check_ool_workaround(enum arch_timer_erratum_match_type t
 		match_fn = arch_timer_check_local_cap_erratum;
 		local = true;
 		break;
+	case ate_match_acpi_oem_info:
+		match_fn = arch_timer_check_acpi_oem_erratum;
+		break;
 	}
 
 	wa = arch_timer_iterate_errata(type, match_fn, arg);
@@ -1283,6 +1315,7 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *table)
 
 	/* Check for globally applicable workarounds */
 	arch_timer_check_ool_workaround(ate_match_global_cap_id, NULL);
+	arch_timer_check_ool_workaround(ate_match_acpi_oem_info, table);
 
 	arch_timer_init();
 	return 0;
-- 
2.11.0


>From 857eae645c7dd0cf6c7cd652dd87bbe8041a5d70 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <marc.zyngier@arm.com>
Date: Tue, 21 Feb 2017 15:04:27 +0000
Subject: [PATCH 2/2] arm64: arch_timer: Add HISILICON_ERRATUM_161010101 ACPI
 matching data

In order to deal with ACPI enabled platforms suffering from the
HISILICON_ERRATUM_161010101, let's add the required OEM data that
allow the workaround to be enabled.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 drivers/clocksource/arm_arch_timer.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 1de18113e5ae..062046bca9eb 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -269,6 +269,25 @@ static u64 notrace hisi_161010101_read_cntvct_el0(void)
 {
 	return __hisi_161010101_read_reg(cntvct_el0);
 }
+
+static struct ate_acpi_oem_info hisi_161010101_oem_info[] = {
+	{
+		.oem_id		= "HISI  ",
+		.oem_table_id	= "HIP05   ",
+		.oem_revision	= 0,
+	},
+	{
+		.oem_id		= "HISI  ",
+		.oem_table_id	= "HIP06   ",
+		.oem_revision	= 0,
+	},
+	{
+		.oem_id		= "HISI  ",
+		.oem_table_id	= "HIP07   ",
+		.oem_revision	= 0,
+	},
+	{ },
+};
 #endif
 
 #ifdef CONFIG_ARM64_ERRATUM_858921
@@ -346,6 +365,16 @@ static const struct arch_timer_erratum_workaround ool_workarounds[] = {
 		.set_next_event_phys = erratum_set_next_event_tval_phys,
 		.set_next_event_virt = erratum_set_next_event_tval_virt,
 	},
+	{
+		.match_type = ate_match_acpi_oem_info,
+		.id = hisi_161010101_oem_info,
+		.desc_str = "HiSilicon erratum 161010101",
+		.read_cntp_tval_el0 = hisi_161010101_read_cntp_tval_el0,
+		.read_cntv_tval_el0 = hisi_161010101_read_cntv_tval_el0,
+		.read_cntvct_el0 = hisi_161010101_read_cntvct_el0,
+		.set_next_event_phys = erratum_set_next_event_tval_phys,
+		.set_next_event_virt = erratum_set_next_event_tval_virt,
+	},
 #endif
 #ifdef CONFIG_ARM64_ERRATUM_858921
 	{
-- 
2.11.0


-- 
Jazz is not dead, it just smell funny.

Re: [PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Alexander Graf]

> Am 21.02.2017 um 07:22 schrieb Marc Zyngier <marc.zyngier@arm.com>:
> 
>> On Tue, Feb 21 2017 at 11:56:31 am GMT, Hanjun Guo <guohanjun@huawei.com> wrote:
>> Hi Marc,
> 
> [...]
> 
>> Thank you very much! I prepared an ACPI patch based on your branch, I was
>> trying to reuse the "const void *id" for ACPI as well but we need to match the
>> oem_id, oem_table_id and oem_revision in GTDT table, so I introduced new
>> matching information for ACPI, please take a look if it's OK.
>> 
>> commit 02d531ae4a88c483db849a611bd9bf7c39d2e1bf
>> Author: Hanjun Guo <hanjun.guo@linaro.org>
>> Date:   Tue Feb 21 15:17:14 2017 +0800
>> 
>>    arm64: arch_timer: enable the erratums in ACPI way
>> 
>>    Match OEM information which are oem_id, oem_table_id and oem_revision
>>    to enable the erratum for specific platforms.
>> 
>>    Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>> ---
>> arch/arm64/include/asm/arch_timer.h  |  7 +++++++
>> drivers/clocksource/arm_arch_timer.c | 31 +++++++++++++++++++++++++++++++
>> 2 files changed, 38 insertions(+)
>> 
>> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
>> index 1595134..79d033c 100644
>> --- a/arch/arm64/include/asm/arch_timer.h
>> +++ b/arch/arm64/include/asm/arch_timer.h
>> @@ -22,6 +22,7 @@
>> #include <asm/barrier.h>
>> #include <asm/sysreg.h>
>> 
>> +#include <linux/acpi.h>
>> #include <linux/bug.h>
>> #include <linux/init.h>
>> #include <linux/jump_label.h>
>> @@ -42,6 +43,7 @@ enum arch_timer_erratum_match_type {
>>        ate_match_dt,
>>        ate_match_global_cap_id,
>>        ate_match_local_cap_id,
>> + ate_match_acpi,
>> };
>> 
>> struct clock_event_device;
>> @@ -49,6 +51,11 @@ enum arch_timer_erratum_match_type {
>> struct arch_timer_erratum_workaround {
>>        enum arch_timer_erratum_match_type match_type;
>>        const void *id;         /* Indicate the Erratum ID */
>> +#ifdef CONFIG_ACPI
>> + char oem_id[ACPI_OEM_ID_SIZE + 1];
>> + char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
>> + u32 oem_revision;
>> +#endif
>>        const char *desc_str;
>>        u32 (*read_cntp_tval_el0)(void);
>>        u32 (*read_cntv_tval_el0)(void);
>> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
>> index bbd9361..82ff6e4 100644
>> --- a/drivers/clocksource/arm_arch_timer.c
>> +++ b/drivers/clocksource/arm_arch_timer.c
>> @@ -363,6 +363,32 @@ bool arch_timer_check_dt_erratum(const struct arch_timer_erratum_workaround
>> *wa,
>>        return of_property_read_bool(np, wa->id);
>> }
>> 
>> +#ifdef CONFIG_ACPI
>> +static bool
>> +arch_timer_check_acpi_erratum(const struct arch_timer_erratum_workaround *wa,
>> +                       const void *arg)
>> +{
>> + const struct acpi_table_header *table = arg;
>> +
>> + if (!wa->oem_id || !wa->oem_table_id)
>> +         return false;
>> +
>> + if (!memcmp(wa->oem_id, table->oem_id, ACPI_OEM_ID_SIZE) &&
>> +     !memcmp(wa->oem_table_id, table->oem_table_id,
>> +             ACPI_OEM_TABLE_ID_SIZE) &&
>> +     wa->oem_revision == table->oem_revision) {
>> +         return true;
>> + }
>> +
>> + return false;
>> +}
>> +#else
>> +static inline bool
>> +arch_timer_check_acpi_erratum(const struct arch_timer_erratum_workaround *wa,
>> +                       const void *arg)
>> +{ return false; }
>> +#endif
>> +
>> static
>> bool arch_timer_check_global_cap_erratum(const struct arch_timer_erratum_workaround *wa,
>>                                         const void *arg)
>> @@ -440,6 +466,9 @@ static void arch_timer_check_ool_workaround(enum arch_timer_erratum_match_typ
>> e t
>>                match_fn = arch_timer_check_local_cap_erratum;
>>                local = true;
>>                break;
>> + case ate_match_acpi:
>> +         match_fn = arch_timer_check_acpi_erratum;
>> +         break;
>>        }
>> 
>>        wa = arch_timer_iterate_errata(type, match_fn, arg);
>> @@ -1284,6 +1313,8 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *table)
>>        /* Check for globally applicable workarounds */
>>        arch_timer_check_ool_workaround(ate_match_global_cap_id, NULL);
>> 
>> + arch_timer_check_ool_workaround(ate_match_acpi, table);
>> +
>>        arch_timer_init();
>>        return 0;
>> }
>> 
>> And I got compile errors for this patch [1], seems "#include <linux/acpi.h>" in
>> arch/arm64/include/asm/arch_timer.h triggers the problem of head
>> file inclusions (too early to include <linux/sched.h>?), I'm looking into it,
>> if you can help to take a look too, that will be great :)
> 
> This is really much more complicated (and uglier) than what I had in
> mind, and I don't want to add more cruft to the erratum description
> structure. So instead of trying to explain what I wanted to see, here's
> the patches I whipped together.
> 
> Please let me know if they work for you (as I have no way of testing
> them).

Thanks for handling this quickly :).

One thing I can't get my head around yet is VM propagation of these erratas. For the dt property based approach, I can at least see how someone adds that property into the guest dt when running on broken hosts.

But how would that work when the erratum matching happens based on the OEM identifier? Would a VM suddenly have to have HiSilicon as OEM? Or would we forbid kvm usage altogether on those CPUs?

Alex

[PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Alexander Graf]

> Am 21.02.2017 um 07:22 schrieb Marc Zyngier <marc.zyngier@arm.com>:
> 
>> On Tue, Feb 21 2017 at 11:56:31 am GMT, Hanjun Guo <guohanjun@huawei.com> wrote:
>> Hi Marc,
> 
> [...]
> 
>> Thank you very much! I prepared an ACPI patch based on your branch, I was
>> trying to reuse the "const void *id" for ACPI as well but we need to match the
>> oem_id, oem_table_id and oem_revision in GTDT table, so I introduced new
>> matching information for ACPI, please take a look if it's OK.
>> 
>> commit 02d531ae4a88c483db849a611bd9bf7c39d2e1bf
>> Author: Hanjun Guo <hanjun.guo@linaro.org>
>> Date:   Tue Feb 21 15:17:14 2017 +0800
>> 
>>    arm64: arch_timer: enable the erratums in ACPI way
>> 
>>    Match OEM information which are oem_id, oem_table_id and oem_revision
>>    to enable the erratum for specific platforms.
>> 
>>    Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>> ---
>> arch/arm64/include/asm/arch_timer.h  |  7 +++++++
>> drivers/clocksource/arm_arch_timer.c | 31 +++++++++++++++++++++++++++++++
>> 2 files changed, 38 insertions(+)
>> 
>> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
>> index 1595134..79d033c 100644
>> --- a/arch/arm64/include/asm/arch_timer.h
>> +++ b/arch/arm64/include/asm/arch_timer.h
>> @@ -22,6 +22,7 @@
>> #include <asm/barrier.h>
>> #include <asm/sysreg.h>
>> 
>> +#include <linux/acpi.h>
>> #include <linux/bug.h>
>> #include <linux/init.h>
>> #include <linux/jump_label.h>
>> @@ -42,6 +43,7 @@ enum arch_timer_erratum_match_type {
>>        ate_match_dt,
>>        ate_match_global_cap_id,
>>        ate_match_local_cap_id,
>> + ate_match_acpi,
>> };
>> 
>> struct clock_event_device;
>> @@ -49,6 +51,11 @@ enum arch_timer_erratum_match_type {
>> struct arch_timer_erratum_workaround {
>>        enum arch_timer_erratum_match_type match_type;
>>        const void *id;         /* Indicate the Erratum ID */
>> +#ifdef CONFIG_ACPI
>> + char oem_id[ACPI_OEM_ID_SIZE + 1];
>> + char oem_table_id[ACPI_OEM_TABLE_ID_SIZE + 1];
>> + u32 oem_revision;
>> +#endif
>>        const char *desc_str;
>>        u32 (*read_cntp_tval_el0)(void);
>>        u32 (*read_cntv_tval_el0)(void);
>> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
>> index bbd9361..82ff6e4 100644
>> --- a/drivers/clocksource/arm_arch_timer.c
>> +++ b/drivers/clocksource/arm_arch_timer.c
>> @@ -363,6 +363,32 @@ bool arch_timer_check_dt_erratum(const struct arch_timer_erratum_workaround
>> *wa,
>>        return of_property_read_bool(np, wa->id);
>> }
>> 
>> +#ifdef CONFIG_ACPI
>> +static bool
>> +arch_timer_check_acpi_erratum(const struct arch_timer_erratum_workaround *wa,
>> +                       const void *arg)
>> +{
>> + const struct acpi_table_header *table = arg;
>> +
>> + if (!wa->oem_id || !wa->oem_table_id)
>> +         return false;
>> +
>> + if (!memcmp(wa->oem_id, table->oem_id, ACPI_OEM_ID_SIZE) &&
>> +     !memcmp(wa->oem_table_id, table->oem_table_id,
>> +             ACPI_OEM_TABLE_ID_SIZE) &&
>> +     wa->oem_revision == table->oem_revision) {
>> +         return true;
>> + }
>> +
>> + return false;
>> +}
>> +#else
>> +static inline bool
>> +arch_timer_check_acpi_erratum(const struct arch_timer_erratum_workaround *wa,
>> +                       const void *arg)
>> +{ return false; }
>> +#endif
>> +
>> static
>> bool arch_timer_check_global_cap_erratum(const struct arch_timer_erratum_workaround *wa,
>>                                         const void *arg)
>> @@ -440,6 +466,9 @@ static void arch_timer_check_ool_workaround(enum arch_timer_erratum_match_typ
>> e t
>>                match_fn = arch_timer_check_local_cap_erratum;
>>                local = true;
>>                break;
>> + case ate_match_acpi:
>> +         match_fn = arch_timer_check_acpi_erratum;
>> +         break;
>>        }
>> 
>>        wa = arch_timer_iterate_errata(type, match_fn, arg);
>> @@ -1284,6 +1313,8 @@ static int __init arch_timer_acpi_init(struct acpi_table_header *table)
>>        /* Check for globally applicable workarounds */
>>        arch_timer_check_ool_workaround(ate_match_global_cap_id, NULL);
>> 
>> + arch_timer_check_ool_workaround(ate_match_acpi, table);
>> +
>>        arch_timer_init();
>>        return 0;
>> }
>> 
>> And I got compile errors for this patch [1], seems "#include <linux/acpi.h>" in
>> arch/arm64/include/asm/arch_timer.h triggers the problem of head
>> file inclusions (too early to include <linux/sched.h>?), I'm looking into it,
>> if you can help to take a look too, that will be great :)
> 
> This is really much more complicated (and uglier) than what I had in
> mind, and I don't want to add more cruft to the erratum description
> structure. So instead of trying to explain what I wanted to see, here's
> the patches I whipped together.
> 
> Please let me know if they work for you (as I have no way of testing
> them).

Thanks for handling this quickly :).

One thing I can't get my head around yet is VM propagation of these erratas. For the dt property based approach, I can at least see how someone adds that property into the guest dt when running on broken hosts.

But how would that work when the erratum matching happens based on the OEM identifier? Would a VM suddenly have to have HiSilicon as OEM? Or would we forbid kvm usage altogether on those CPUs?

Alex

Re: [PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Hanjun Guo]

On 2017/2/21 23:22, Marc Zyngier wrote:
> On Tue, Feb 21 2017 at 11:56:31 am GMT, Hanjun Guo <guohanjun@huawei.com> wrote:
>> Hi Marc,
> [...]
>
[...]
>>
>> And I got compile errors for this patch [1], seems "#include <linux/acpi.h>" in
>> arch/arm64/include/asm/arch_timer.h triggers the problem of head
>> file inclusions (too early to include <linux/sched.h>?), I'm looking into it,
>> if you can help to take a look too, that will be great :)
> This is really much more complicated (and uglier) than what I had in
> mind, and I don't want to add more cruft to the erratum description
> structure. So instead of trying to explain what I wanted to see, here's
> the patches I whipped together.
>
> Please let me know if they work for you (as I have no way of testing
> them).
>
> Thanks,
>
> 	M.
>
> >From 50eb735436f9f6482285939b39b52d858d848537 Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <marc.zyngier@arm.com>
> Date: Tue, 21 Feb 2017 14:37:30 +0000
> Subject: [PATCH 1/2] arm64: arch_timer: Allow erratum matching with ACPI OEM
>  information
>
> Just as we're able to identify a broken platform using some DT
> information, let's enable a way to spot the offenders with ACPI.
>
> The difference is that we can only match on some OEM info instead
> of implementation-specific properties. So in order to avoid the
> insane multiplication of errata structures, we allow an array
> of OEM descriptions to be attached to an erratum structure.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/include/asm/arch_timer.h  |  1 +
>  drivers/clocksource/arm_arch_timer.c | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+)

Marc, thank you for the help, I tested your patches on D03, I got:

[    0.000000] arm_arch_timer: Enabling global workaround for HiSilicon erratum 161010101
[    0.000000] arm_arch_timer: CPU0: Trapping CNTVCT access

in the boot log, which means the framework and ACPI patches work fine.

Thanks
Hanjun

[PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Hanjun Guo]

On 2017/2/21 23:22, Marc Zyngier wrote:
> On Tue, Feb 21 2017 at 11:56:31 am GMT, Hanjun Guo <guohanjun@huawei.com> wrote:
>> Hi Marc,
> [...]
>
[...]
>>
>> And I got compile errors for this patch [1], seems "#include <linux/acpi.h>" in
>> arch/arm64/include/asm/arch_timer.h triggers the problem of head
>> file inclusions (too early to include <linux/sched.h>?), I'm looking into it,
>> if you can help to take a look too, that will be great :)
> This is really much more complicated (and uglier) than what I had in
> mind, and I don't want to add more cruft to the erratum description
> structure. So instead of trying to explain what I wanted to see, here's
> the patches I whipped together.
>
> Please let me know if they work for you (as I have no way of testing
> them).
>
> Thanks,
>
> 	M.
>
> >From 50eb735436f9f6482285939b39b52d858d848537 Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <marc.zyngier@arm.com>
> Date: Tue, 21 Feb 2017 14:37:30 +0000
> Subject: [PATCH 1/2] arm64: arch_timer: Allow erratum matching with ACPI OEM
>  information
>
> Just as we're able to identify a broken platform using some DT
> information, let's enable a way to spot the offenders with ACPI.
>
> The difference is that we can only match on some OEM info instead
> of implementation-specific properties. So in order to avoid the
> insane multiplication of errata structures, we allow an array
> of OEM descriptions to be attached to an erratum structure.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/include/asm/arch_timer.h  |  1 +
>  drivers/clocksource/arm_arch_timer.c | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+)

Marc, thank you for the help, I tested your patches on D03, I got:

[    0.000000] arm_arch_timer: Enabling global workaround for HiSilicon erratum 161010101
[    0.000000] arm_arch_timer: CPU0: Trapping CNTVCT access

in the boot log, which means the framework and ACPI patches work fine.

Thanks
Hanjun

Re: [PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Marc Zyngier]

On 22/02/17 04:08, Alexander Graf wrote:

[irrelevant stuff]

> Thanks for handling this quickly :).
> 
> One thing I can't get my head around yet is VM propagation of these
> erratas. For the dt property based approach, I can at least see how
> someone adds that property into the guest dt when running on broken
> hosts.
> 
> But how would that work when the erratum matching happens based on
> the OEM identifier? Would a VM suddenly have to have HiSilicon as
> OEM? Or would we forbid kvm usage altogether on those CPUs?

s/CPU/SoC/

Simple: you only use DT as a guest, because it is the only firmware
interface that allows you to reliably describe an arbitrary erratum.
ACPI is too inflexible for that. Injecting bits of the host's ACPI
tables in the guest doesn't strike me as a reliable solution, as you're
not describing the erratum itself. I imagine you 'd have some userspace
tool to dump the host ACPI table, match it against a list of known
errata, and inject the corresponding property into the guest's DT.

Furthermore, I don't see any reason to actively prevent KVM from running
on any system. If we did, we'd start with the RPi...

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 2/2] arch_timer: acpi: add hisi timer erratum data

[Marc Zyngier]

On 22/02/17 04:08, Alexander Graf wrote:

[irrelevant stuff]

> Thanks for handling this quickly :).
> 
> One thing I can't get my head around yet is VM propagation of these
> erratas. For the dt property based approach, I can at least see how
> someone adds that property into the guest dt when running on broken
> hosts.
> 
> But how would that work when the erratum matching happens based on
> the OEM identifier? Would a VM suddenly have to have HiSilicon as
> OEM? Or would we forbid kvm usage altogether on those CPUs?

s/CPU/SoC/

Simple: you only use DT as a guest, because it is the only firmware
interface that allows you to reliably describe an arbitrary erratum.
ACPI is too inflexible for that. Injecting bits of the host's ACPI
tables in the guest doesn't strike me as a reliable solution, as you're
not describing the erratum itself. I imagine you 'd have some userspace
tool to dump the host ACPI table, match it against a list of known
errata, and inject the corresponding property into the guest's DT.

Furthermore, I don't see any reason to actively prevent KVM from running
on any system. If we did, we'd start with the RPi...

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...