[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1b29abc6-7428-1528-864f-2a246332f72b@alliedtelesis.co.nz>
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