Re: [PATCH 12/12] arm64: dts: exynos: Add Exynos850 SoC support

[Marc Zyngier <maz@xxxxxxxxxx>]

On Thu, 05 Aug 2021 16:30:23 +0100,
Sam Protsenko <semen.protsenko@xxxxxxxxxx> wrote:
> 
> On Thu, 5 Aug 2021 at 10:39, Marc Zyngier <maz@xxxxxxxxxx> wrote:
> >
> > On Wed, 04 Aug 2021 19:37:24 +0100,
> > Sam Protsenko <semen.protsenko@xxxxxxxxxx> wrote:
> > >
> > > On Wed, 4 Aug 2021 at 18:01, Marc Zyngier <maz@xxxxxxxxxx> wrote:
> > > >
> > > > On Wed, 04 Aug 2021 15:39:38 +0100,
> > > > Sam Protsenko <semen.protsenko@xxxxxxxxxx> wrote:
> > > >
> > > > > > You are also missing the hypervisor virtual timer interrupt.
> > > > > >
> > > > >
> > > > > Checked SoC TRM, there is no PPI for hypervisor virtual timer
> > > > > interrupt, and no mentioning of it at all. Likewise, I checked ARMv8
> > > > > ARM and TRM, almost no description of it. Also, I checked other
> > > > > platforms, and seems like everyone does the same (having only 4
> > > > > interrupts). And I wasn't able to find any documentation on that, so I
> > > > > guess I'll leave it as is, if you don't mind.
> > > >
> > > > I *do* mind, and other DTs being wrong isn't a good enough excuse! ;-)
> > > >
> > > > From the ARMv8 ARM (ARM DDI 0487G.b)
> > > > <quote>
> > > > D11.2.4 Timers
> > > >
> > > > In an implementation of the Generic Timer that includes EL3, if EL3
> > > > can use AArch64, the following timers are implemented:
> > > >
> > > > * An EL1 physical timer, that:
> > > >   - In Secure state, can be accessed from EL1.
> > > >   - In Non-secure state, can be accessed from EL1 unless those
> > > >     accesses are trapped to EL2.
> > > >     When this timer can be accessed from EL1, an EL1 control
> > > >     determines whether it can be accessed from EL0.
> > > > * A Non-secure EL2 physical timer.
> > > > * A Secure EL3 physical timer. An EL3 control determines whether this
> > > >   register is accessible from Secure EL1.
> > > > * An EL1 virtual timer.
> > > > * When FEAT_VHE is implemented, a Non-secure EL2 virtual timer.
> > > > * When FEAT_SEL2 is implemented, a Secure EL2 physical timer.
> > > > * When FEAT_SEL2 is implemented, a Secure EL2 virtual timer.
> > > > </quote>
> > > >
> > > > Cortex-A55 being an ARMv8.2 implementation, it has FEAT_VHE, and thus
> > > > it does have a NS-EL2 virtual timer. This is further confirmed by the
> > > > TRM which documents CNTHV*_EL2 as valid system registers[1].
> > > >
> > > > So the timer exists, the signal is routed out of the core, and it
> > > > is likely that it is connected to the GIC.
> > > >
> > > > If the designers have omitted it, then it needs to be documented as
> > > > such.
> > > >
> > >
> > > Ok, I've checked thoroughly all docs again, and it seems like there is
> > > no dedicated PPI number for this "EL2 Hypervisor Virtual Timer" in
> > > Exynos850 SoC. The timer instance itself might exist of course, but
> > > interrupt line is probably wasn't connected to GIC by SoC designers,
> > > at least it's not documented.
> >
> > Can you try and check this? You can directly program the virtual timer
> > so that it has a pending interrupt, and then check the pending
> > register on the same CPU to see if there is anything appearing there.
> >
> > > Moreover, from [1,2] it looks like if it were existing it would have
> > > been PPI=12 (INTID=28). But in GIC-400 TRM this PPI is assigned to
> > > "Legacy FIQ signal",
> >
> > No. That's only if you set the bypass bits in GICD_CTLR, which nobody
> > with half a brain would consider doing.
> >
> > > and all there is no PPI for Hypervisor Virtual
> > > Timer documented there as well. In Exynos850 TRM the source for this
> > > PPI's interrupt source is marked as "-", which means it's not used.
> > >
> > > So if you know something that I don't know -- please point me out the
> > > doc where this PPI line is documented. Otherwise I can add the comment
> > > to device tree, stating that this interrupt line is not present in
> > > SoC's GIC, i.e. something like this:
> > >
> > > 8<------------------------------------------------------------------------------->8
> > >     timer {
> > >         compatible = "arm,armv8-timer";
> > >         interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(8) |
> > >                       IRQ_TYPE_LEVEL_LOW)>,
> > >                  <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(8) |
> > >                       IRQ_TYPE_LEVEL_LOW)>,
> > >                  <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(8) |
> > >                       IRQ_TYPE_LEVEL_LOW)>,
> > >                  <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(8) |
> > >                       IRQ_TYPE_LEVEL_LOW)>;
> > >         /* Hypervisor Virtual Timer PPI is not present in this SoC GIC */
> > >     };
> > > 8<------------------------------------------------------------------------------->8
> > >
> > > Is that ok with you?
> >
> > I'd rather you verify the above first. And if you can't, I'd like a
> > comment that is a bit more explicit:
> >
> 
> I'm afraid I won't be able to verify your idea: seems like CNTHV_EL2
> can be only modified (or read) in EL2. I tried to read that reg
> anyway, which unsurprisingly resulted in el1_undef() BUG. The kernel
> on my board is running in EL1, and I don't have access to the source
> code for EL3 bootloaders. I have the source code for the last
> bootloader, but it's already running in EL1.

Excellent. Yet another half-usable machine on the block. Just what we
need.

> 
> > /* The vendor couldn't be bothered to wire the EL2 Virtual Timers */
> >
> 
> I'll add the comment as you suggested. I propose we come back to this
> issue later, either when the need for HV timer arises or when I have
> some means to test your theory about existing PPI.

If you depend on the vendor to get EL2 access, it is a lost cause,
unfortunately.

	M.

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