Re: [RFC PATCH 0/3] Add HiSilicon system timer driver

[Yicong Yang]

On 2023/10/10 23:43, Mark Rutland wrote:
> Hi,
> 
> On Tue, Oct 10, 2023 at 08:30:30PM +0800, Yicong Yang wrote:
>> From: Yicong Yang <yangyicong@xxxxxxxxxxxxx>
>>
>> HiSilicon system timer is a memory mapped platform timer compatible with
>> the arm's generic timer specification. The timer supports both SPI and
>> LPI interrupt and can be enumerated through ACPI DSDT table. Since the
>> timer is fully compatible with the spec, it can reuse most codes of the
>> arm_arch_timer driver. However since the arm_arch_timer driver only
>> supports GTDT and SPI interrupt, this series support the HiSilicon system
>> timer by:
>>
>> - refactor some of the arm_arch_timer codes and export the function to
>>   register a arch memory timer by other drivers
>> - retrieve the IO memory and interrupt resource through DSDT in a separate
>>   driver, then setup and register the clockevent device reuse the arm_arch_timer
>>   function
>>
>> Using LPI for the timer is mentioned in BSA Spec section 3.8.1 (DEN0094C 1.0C).
> 
> This seems like an oversight; there *should* be a generic way of describing
> this, and I've poked our BSA and ACPI architects to figure out how this is
> supposed to work. The lack of a way to do that seems like a major oversight and
> something that needs to be solved.
> 
> I'll try to get back to you shortly on that.
> 

Looking forward to the conclusion/solution.

> Regardless of that, I do not think this should be a separate driver, and I'm
> very much not keen on having vendor-specific companion drivers like this. Using
> LPIs isn't specific to HiSilicon, and this should be entirely common (and if we
> need a DSDT device, should use a common HID too).
> 

This is reasonable. Actually we're using most funtions of the arch timer driver, only
do the resource enumeration in the separate driver which is lack in the arch timer driver.
So if there's a common solution in the arch timer driver as well as a common HID we're
willing to use it.

Thanks.

> Thanks,
> Mark.
> 
>>
>> Yicong Yang (3):
>>   clocksource/drivers/arm_arch_timer: Split the function of
>>     __arch_timer_setup()
>>   clocksource/drivers/arm_arch_timer: Extend and export
>>     arch_timer_mem_register()
>>   clocksource/drivers: Add HiSilicon system timer driver
>>
>>  drivers/clocksource/Kconfig          |  10 +++
>>  drivers/clocksource/Makefile         |   1 +
>>  drivers/clocksource/arm_arch_timer.c | 123 +++++++++++++++------------
>>  drivers/clocksource/timer-hisi-sys.c |  68 +++++++++++++++
>>  include/clocksource/arm_arch_timer.h |   2 +
>>  5 files changed, 148 insertions(+), 56 deletions(-)
>>  create mode 100644 drivers/clocksource/timer-hisi-sys.c
>>
>> -- 
>> 2.24.0
>>
> 
> .
>