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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <bbd2ee8d-4349-4752-859a-02fb0252d496@linaro.org>
Date:   Fri, 10 Nov 2023 19:02:39 +0100
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     Samuel Holland <samuel.holland@...ive.com>,
        Xingyu Wu <xingyu.wu@...rfivetech.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Emil Renner Berthing <emil.renner.berthing@...onical.com>,
        Christophe JAILLET <christophe.jaillet@...adoo.fr>,
        linux-riscv@...ts.infradead.org, devicetree@...r.kernel.org,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Walker Chen <walker.chen@...rfivetech.com>,
        Samin Guo <samin.guo@...rfivetech.com>,
        linux-kernel@...r.kernel.org, Conor Dooley <conor@...nel.org>
Subject: Re: [PATCH v7 2/3] clocksource: Add JH7110 timer driver


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.



-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog