Messages in this thread | | | Date | Wed, 18 May 2022 08:11:47 +0100 | From | Marc Zyngier <> | Subject | Re: [PATCH v7 2/3] arm64: dts: marvell: Add Armada 98DX2530 SoC and RD-AC5X board |
| |
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@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. > > 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...
| |