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

[Marc Zyngier <maz@kernel.org>]

On Mon, 16 May 2022 22:56:44 +0100,
Chris Packham <Chris.Packham@alliedtelesis.co.nz> 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.

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

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.