Re: [PATCH v7 2/3] clocksource: Add JH7110 timer driver

[Samuel Holland <samuel.holland@sifive.com>]

On 2023-11-08 11:51 PM, Xingyu Wu wrote:
> On 2023/11/8 17:10, Daniel Lezcano wrote:
>> On 08/11/2023 04:45, Xingyu Wu wrote:
>>> On 2023/11/2 22:29, Daniel Lezcano wrote:
>> 
>> [ ... ]
>> 
>>> Thanks. The riscv-timer has a clocksource with a higher rating but a 
>>> clockevent with lower rating[1] than jh7110-timer. I tested the 
>>> jh7110-timer as clockevent and flagged as one shot, which could do some
>>> of the works instead of riscv-timer. And the current_clockevent changed
>>> to jh7110-timer.
>>> 
>>> Because the jh7110-timer works as clocksource with lower rating and only
>>> will be used as global timer at CPU idle time. Is it necessary to be
>>> registered as clocksource? If not, should it just be registered as
>>> clockevent?
>> 
>> Yes, you can register the clockevent without the clocksource.
>> 
>> You mentioned the JH7110 has a better rating than the CPU architected
>> timers. The rating is there to "choose" the best timer, so it is up to the
>> author of the driver check against which timers it compares on the
>> platform.
>> 
>> Usually, CPU timers are the best.
>> 
>> It is surprising the timer-riscv has a so low rating. You may double check
>> if jh7110 is really better. If it is the case, then implementing a
>> clockevent per cpu would make more sense, otherwise one clockevent as a
>> global timer is enough.

The timer-riscv clockevent has a low rating because it requires a call to
firmware to set the timer, as well as a trap to firmware to handle the
interrupt, which both add overhead. Implementations which support the Sstc
extension[1] do not require firmware assistance to implement the clockevent, so
in that case we register the clockevent with a higher rating.

[1]: https://github.com/riscv/riscv-time-compare

>> Unused clocksource, clockevents should be stopped in case the firmware let
>> them in a undetermined state.
> 
> The interrupts of jh7110-timer each channel are global interrupts like
> SPI(Shared Peripheral Interrupt) not PPI (Private Peripheral Interrupt). They
> are up to PLIC to select which core to respond to. So it is hard to implement
> a clockevent per cpu core. I tested this with request_percpu_irq() and it
> failed.

You cannot use request_percpu_irq(), but the driver should be able to set the
affinity of each IRQ to a separate CPU.

Regards,
Samuel

> I think it is enough to implement a clockevent as a global timer. Thank you
> for your advice.
> 
> Best regards, Xingyu Wu