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

[Marc Zyngier <maz@kernel.org>]

On Wed, 04 Aug 2021 19:37:24 +0100,
Sam Protsenko <semen.protsenko@linaro.org> wrote:
> 
> On Wed, 4 Aug 2021 at 18:01, Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Wed, 04 Aug 2021 15:39:38 +0100,
> > Sam Protsenko <semen.protsenko@linaro.org> 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:

/* The vendor couldn't be bothered to wire the EL2 Virtual Timers */

Thanks,

	M.

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