| Message ID | 20231019053501.46899-1-xingyu.wu@starfivetech.com |
|---|---|
| Headers | show |
| Series | Add timer driver for StarFive JH7110 RISC-V SoC | expand |
Hi Xingyu, On 02/11/2023 14:15, Xingyu Wu wrote: [ ... ] >>>>>>> +struct jh7110_clkevt { + struct clock_event_device >>>>>>> evt; + struct clocksource cs; + bool cs_is_valid; + >>>>>>> struct clk *clk; + struct reset_control *rst; + u32 >>>>>>> rate; + u32 reload_val; + void __iomem *base; + >>>>>>> char name[sizeof("jh7110-timer.chX")]; +}; + +struct >>>>>>> jh7110_timer_priv { + struct clk *pclk; + struct >>>>>>> reset_control *prst; + struct jh7110_clkevt >>>>>>> clkevt[JH7110_TIMER_CH_MAX]; >>>>>> >>>>>> Why do you need several clock events and clock sources ? >>>>> >>>>> This timer has four counters (channels) which run >>>>> independently. So each counter can have its own clock event >>>>> and clock source to configure different settings. >>>> >>>> The kernel only needs one clocksource. Usually multiple >>>> clockevents are per-cpu based system. >>>> >>>> The driver does not seem to have a per cpu timer but just >>>> initializing multiple clockevents which will end up unused, >>>> wasting energy. >>>> >>>> >>> >>> The board of the StarFive JH7110 SoC has two types of timer : >>> riscv-timer and jh7110-timer. It boots by >>> riscv-timer(clocksource) and the jh7110-timer is optional and >>> additional. I think I should initialize the four channels of >>> jh7110-timer as clockevents not clocksource pre-cpu. >> >> If no clocksource is needed on this SoC because riscv timers are >> used, then it is not useful to register a clocksource for this >> timer and the corresponding code can go away. >> >> If the clockevent is optional why do you need this driver at all? >> >> >> > > Hi Daniel, > > Sorry, maybe I didn't express it clearly enough. I use this > jh7110-timer as a global timer on the SoC and riscv-timer as cpu > local timer. So these are something different. > > These four counters in this jh7110-timer are exactly the same and > independent of each other. If this timer is used as a global timer, > do I use only one or all of the counters to register clocksource and > clockevent? Yes. The global timer is only there when the CPU is powered down at idle time, so the time framework will switch to the broadcast timer and there can be only one instance. If you register all the counters, only one will be used by the kernel, so it pointless to add them all. On the clocksource side, you may want to question if it is really useful. The riscv has a clocksource with a higher rate and flagged as continuous [1]. So if the JH7110 clocksource is registered, it won't be used too. Hope that helps -- Daniel [1] https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/drivers/clocksource/timer-riscv.c#n68
On 2023/11/2 22:29, Daniel Lezcano wrote: > > Hi Xingyu, > > On 02/11/2023 14:15, Xingyu Wu wrote: > > [ ... ] > >>>>>>>> +struct jh7110_clkevt { + struct clock_event_device >>>>>>>> evt; + struct clocksource cs; + bool cs_is_valid; + >>>>>>>> struct clk *clk; + struct reset_control *rst; + u32 >>>>>>>> rate; + u32 reload_val; + void __iomem *base; + >>>>>>>> char name[sizeof("jh7110-timer.chX")]; +}; + +struct jh7110_timer_priv { + struct clk *pclk; + struct reset_control *prst; + struct jh7110_clkevt clkevt[JH7110_TIMER_CH_MAX]; >>>>>>> >>>>>>> Why do you need several clock events and clock sources ? >>>>>> >>>>>> This timer has four counters (channels) which run >>>>>> independently. So each counter can have its own clock event >>>>>> and clock source to configure different settings. >>>>> >>>>> The kernel only needs one clocksource. Usually multiple >>>>> clockevents are per-cpu based system. >>>>> >>>>> The driver does not seem to have a per cpu timer but just initializing multiple clockevents which will end up unused, >>>>> wasting energy. >>>>> >>>>> >>>> >>>> The board of the StarFive JH7110 SoC has two types of timer : riscv-timer and jh7110-timer. It boots by >>>> riscv-timer(clocksource) and the jh7110-timer is optional and >>>> additional. I think I should initialize the four channels of >>>> jh7110-timer as clockevents not clocksource pre-cpu. >>> >>> If no clocksource is needed on this SoC because riscv timers are >>> used, then it is not useful to register a clocksource for this >>> timer and the corresponding code can go away. >>> >>> If the clockevent is optional why do you need this driver at all? >>> >>> >>> >> >> Hi Daniel, >> >> Sorry, maybe I didn't express it clearly enough. I use this >> jh7110-timer as a global timer on the SoC and riscv-timer as cpu >> local timer. So these are something different. >> >> These four counters in this jh7110-timer are exactly the same and >> independent of each other. If this timer is used as a global timer, >> do I use only one or all of the counters to register clocksource and >> clockevent? > > Yes. > > The global timer is only there when the CPU is powered down at idle time, so the time framework will switch to the broadcast timer and there can be only one instance. > > If you register all the counters, only one will be used by the kernel, so it pointless to add them all. > > On the clocksource side, you may want to question if it is really useful. The riscv has a clocksource with a higher rate and flagged as continuous [1]. So if the JH7110 clocksource is registered, it won't be used too. > > Hope that helps > > -- Daniel > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/drivers/clocksource/timer-riscv.c#n68 > 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? [1] https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/drivers/clocksource/timer-riscv.c#n45 Thanks, Xingyu Wu
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. Unused clocksource, clockevents should be stopped in case the firmware let them in a undetermined state. > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/drivers/clocksource/timer-riscv.c#n45 > > Thanks, Xingyu Wu
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. > > 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. I think it is enough to implement a clockevent as a global timer. Thank you for your advice. Best regards, Xingyu Wu >> [1 >> https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/tree/drivers/clocksource/timer-riscv.c#n45 >> >> Thanks, Xingyu Wu >
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.
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