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

[Sam Protsenko]

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.

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

Thanks!

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