diff mbox

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

Message ID 1485254391-51551-3-git-send-email-guohanjun@huawei.com (mailing list archive)
State New, archived
Headers show

Commit Message

Hanjun Guo Jan. 24, 2017, 10:39 a.m. UTC
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(+)

Comments

Mark Rutland Jan. 24, 2017, 10:57 a.m. UTC | #1
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.
Marc Zyngier Jan. 24, 2017, 11:32 a.m. UTC | #2
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.
John Garry Jan. 24, 2017, 12:35 p.m. UTC | #3
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.
>
Marc Zyngier Jan. 24, 2017, 1:08 p.m. UTC | #4
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.
Hanjun Guo Jan. 24, 2017, 1:22 p.m. UTC | #5
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
Shameer Kolothum Jan. 24, 2017, 1:28 p.m. UTC | #6
> -----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
Hanjun Guo Jan. 24, 2017, 1:50 p.m. UTC | #7
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
Hanjun Guo Feb. 10, 2017, 7:10 a.m. UTC | #8
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
Alexander Graf Feb. 16, 2017, 8:42 a.m. UTC | #9
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
Daniel Lezcano Feb. 16, 2017, 9:14 a.m. UTC | #10
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
Alexander Graf Feb. 16, 2017, 9:32 a.m. UTC | #11
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
Ding Tianhong Feb. 16, 2017, 9:41 a.m. UTC | #12
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<o<	

Thanks
Ding


> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> .
>
Hanjun Guo Feb. 16, 2017, 9:42 a.m. UTC | #13
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
Sanil Kumar Feb. 16, 2017, 9:46 a.m. UTC | #14
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
>>
>> .
>>
>
> .
>
Daniel Lezcano Feb. 16, 2017, 9:49 a.m. UTC | #15
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.
Marc Zyngier Feb. 16, 2017, 10:04 a.m. UTC | #16
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.
Marc Zyngier Feb. 20, 2017, 7 p.m. UTC | #17
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.
Ding Tianhong Feb. 21, 2017, 12:46 p.m. UTC | #18
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.
>
diff mbox

Patch

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,