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

[Marc Zyngier <maz@kernel.org>]

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