On 2022-05-17 23:56, Chris Packham wrote:
On 17/05/22 18:42, Marc Zyngier wrote:
On Mon, 16 May 2022 22:56:44 +0100,
Chris Packham <Chris.Packham@xxxxxxxxxxxxxxxxxxx> wrote:
Please fix your firmware to program CNTFRQ_EL0, and
remove this useless property.
I'm kind of at the mercy of what Marvell have provided for ATF. I
am
working on the bootloader portion in parallel and am getting
things
ready for submitting the u-boot support upstream. I was hoping to
leave ATF alone I can at least see if they haven't fixed this
already
(the original dtsi I started with was fairly old) and if they
haven't
I'll raise it via their support system.
Seems to work fine without the clock so I'll drop it.
Thanks. If you can, please verify that this is set on both CPUs (I
have seen plenty of firmware only setting it on CPU0 in the past).
The arch_timer interrupts are counting up on both CPUs and things
generally seem to be getting scheduled (I don't have much of a
userland
at the moment so it's not exactly a stress test). Do you think that
is
sufficient to say the clock property is unnecessary and whatever
firmware I have is working as expected.
No, the counter always count, and CNTFRQ_EL0 is only an indication of
the frequency for SW to find out. You can directly read CNTFRQ_EL0
from userspace on each CPU and find whether they have the same value.
Here's my test program
$ cat CNTFRQ_EL0.c
#include <stdio.h>
#include <stdint.h>
#include <inttypes.h>
int main(int argc, char *argv[])
{
uint64_t val;
asm volatile("mrs %0, CNTFRQ_EL0" : "=r" (val));
printf("CNTFRQ_EL0 = %" PRIu64 "\n", val);
return 0;
}
And running on the RD-AC5X board
[root@linuxbox tmp]# taskset 0x1 ./CNTFRQ_EL0
CNTFRQ_EL0 = 25000000
[root@linuxbox tmp]# taskset 0x2 ./CNTFRQ_EL0
CNTFRQ_EL0 = 25000000
Great. So the DT attribute was only cargo-culted by MRVL,
and they don't realise what that's for...
Thanks for going the extra mile and checking this!
You are also missing a PPI for the EL2 virtual timer which is
present
on any ARMv8.1+ CPU (and since this system is using A55, it
definitely
has it).
[...]
Will add.
I assume you're talking about the 5th PPI per the
timer/arm,arch_timer.yaml ("hypervisor virtual timer irq").
Indeed.
Helpfully
Marvell don't include the PPI interrupt numbers in their datasheet.
But
then I also notice that none of the other boards that have a
"arm,armv8-timer" provide a 5th interrupt either, have I
misunderstood
something?
This was only recently added to the DT binding, but the interrupt
definitely exist at the CPU level for anything that implements
ARMv8.1
and up. AFAIK, the M1 is the only machine to expose this interrupt
in
DT, but this doesn't mean the interrupt doesn't exist on all the
other
systems that have the same architecture revision.
If you have contacts in Marvell, maybe try and find out whether they
have simply decided not to wire the interrupt (I wouldn't be
surprised). In this case, please add a comment.
I've reached out via their customer support portal. So far they just
want to know why I'm refusing to use their out of date SDK (maybe I
should direct them at some of Jon Corbet's presentations :P).
The fact that they are asking is already saying everything there is to
know, sadly...
These integrated chips are sometimes a bit problematic because the
support goes via the Switching group but these questions are really
about IP blocks that have been taken from the SoC group. It may take
a
while before I get a response from someone that actually knows the
internals.
Fair enough. Until then, please drop a comment in the DT indicating
that the fate of this PPI is unknown. If you eventually find out, just
add it to the DT (it is easy to add things, much harder to remove
them).
I'll include the following in the next round
diff --git a/arch/arm64/boot/dts/marvell/armada-98dx25xx.dtsi
b/arch/arm64/boot/dts/marvell/armada-98dx25xx.dtsi
index 88edc741c008..7a3693a2ad04 100644
--- a/arch/arm64/boot/dts/marvell/armada-98dx25xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-98dx25xx.dtsi
@@ -63,6 +63,7 @@ timer {
<GIC_PPI 8 IRQ_TYPE_LEVEL_HIGH>,
<GIC_PPI 10 IRQ_TYPE_LEVEL_HIGH>,
<GIC_PPI 7 IRQ_TYPE_LEVEL_HIGH>;
+ /* PPI for EL2 virtual timer is
undocumented */
};
pmu {
Looks good, thank you.
M.
--
Jazz is not dead. It just smells funny...
