Date:   Tue, 17 May 2022 22:56:35 +0000
From:   Chris Packham <Chris.Packham@...iedtelesis.co.nz>
To:     Marc Zyngier <maz@...nel.org>
CC:     "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "catalin.marinas@....com" <catalin.marinas@....com>,
        "will@...nel.org" <will@...nel.org>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "gregory.clement@...tlin.com" <gregory.clement@...tlin.com>,
        "sebastian.hesselbarth@...il.com" <sebastian.hesselbarth@...il.com>,
        "kostap@...vell.com" <kostap@...vell.com>,
        "robert.marko@...tura.hr" <robert.marko@...tura.hr>,
        "vadym.kochan@...ision.eu" <vadym.kochan@...ision.eu>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v7 2/3] arm64: dts: marvell: Add Armada 98DX2530 SoC and
 RD-AC5X board


On 17/05/22 18:42, Marc Zyngier wrote:
> On Mon, 16 May 2022 22:56:44 +0100,
> Chris Packham <Chris.Packham@...iedtelesis.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@...uxbox tmp]# taskset 0x1 ./CNTFRQ_EL0
CNTFRQ_EL0 = 25000000
[root@...uxbox tmp]# taskset 0x2 ./CNTFRQ_EL0
CNTFRQ_EL0 = 25000000


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

> Thanks,
>
> 	M.
>

Powered by blists - more mailing lists