Re: [PATCH v7 2/3] arm64: dts: marvell: Add Armada 98DX2530 SoC and RD-AC5X board

[Marc Zyngier]

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...