Hi Ayan,
On 10/08/2022 15:00, Ayan Kumar Halder wrote:
On 10/08/2022 14:34, Julien Grall wrote:
On 10/08/2022 11:58, Ayan Kumar Halder wrote:
Refer "Arm Architecture Registers DDI 0595", AArch32 system registers,
Checking the 'enable' is not going to add too much overhead. So I am
fine if this is added. That said, would you be able to provide more
details on how this was spotted?
This was spotted while debugging an unrelated problem while porting Xen
on R52. For a different reason, I was not able to get context switch to
work correctly.
When I was scrutinizing the timer_interrupt() with the documentation, I
found that we are not checking ENABLE.
Although the code works fine today (on aarch32 or aarch64), I thought it
is better to add the check for the sake of compliance with the
documentation.
Thanks for the clarification. I am quite curious to know why you think
our code is not compliant.
As I wrote before, when ENABLE is cleared, you should never have an
interrupt because the timer interrupt is level. So I believe our code is
compliant with the Arm Arm.
The only reason I am OK with checking ENABLE is because the overhead is
limited. If this wasn't the case, then I think I would have wanted clear
justification in the commit message *why* this is not compliant.
FWIW, Linux seems to use the same approach as us (see [1]). So, if you
think this is not compliant, then maybe this is something you also want
to consider to fix there?
Cheers,
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clocksource/arm_arch_timer.c#n644
--
Julien Grall
