On 2023/11/11 2:02, Daniel Lezcano wrote: > > Hi Samuel, > > On 10/11/2023 18:40, Samuel Holland wrote: >> 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 > > Thanks for the pointer and the clarification. > >>>> 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. > > Absolutely. And given the bad rating of the local timers, it may be worth to implement this driver in a per CPU (affinity set) basis. > > At the first glance, the arm_global_timer can be used as an example. > > Note in this case, you may want to double check what does with an idle state with a local timer stop flag and this timer which is always on. > > > Hi Daniel and Samuel, Thanks for your pointers. I will check it. If it works, I will send the new version of this patch. Best regards, Xingyu Wu
