On 10/08/2022 15:51, Julien Grall wrote:
Hi Ayan,
Hi Julien,
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.
Sorry, I think I misunderstood this part of the documentation
"When the value of the ENABLE bit is 1, ISTATUS indicates whether the
timer condition is met."
I understood this as "ENABLE" need to be checked before "ISTATUS" is
checked.
- Ayan
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
