On 10/08/2022 14:34, Julien Grall wrote:
Hi Ayan,
Hi Julien,
Bertrand already made some comments. I will try to avoid repeating the
same comments.
On 10/08/2022 11:58, Ayan Kumar Halder wrote:
Refer "Arm Architecture Registers DDI 0595", AArch32 system registers,
You are modifying code that is common between AArch64 and AArch32. So
I would mention this behavior is common. Also, please specify the version
of the spec. This helps in case the behavior has changed.
ack
Also, NIT: I would prefer if you quote the Arm Arm rather than auxiliary
specifications.
ack
CNTHP_CTL :- "When the value of the ENABLE bit is 1, ISTATUS indicates
whether the timer condition is met."
I think the key point here is the field ISTATUS is "unknown" when the
ENABLE bit is 0.
yes, this is the key thing.
Also similar description applies to CNTP_CTL as well.
One should always check that the timer is enabled and status is set, to
determine if the timer interrupt has been generated.
I understand the theory. In practice, I am not sure this could ever
happen because the timer interrupt is level and by clearing *_CTL you
will lower the line and therefore you should not receive the interrupt
again.
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.
I can send the v2 patch (addressing yours and Bertrand's comment) if you
think it is fine.
- Ayan
Cheers,
