Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 05 Aug 2021 08:39:20 +0100
Message-ID: <87y29gbas7.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Sam Protsenko <semen.protsenko@linaro.org>
Cc:     Sylwester Nawrocki <s.nawrocki@samsung.com>,
        Chanwoo Choi <cw00.choi@samsung.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        Tomasz Figa <tomasz.figa@gmail.com>,
        Rob Herring <robh+dt@kernel.org>,
        Stephen Boyd <sboyd@kernel.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Jiri Slaby <jirislaby@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Charles Keepax <ckeepax@opensource.wolfsonmicro.com>,
        Ryu Euiyoul <ryu.real@samsung.com>,
        Tom Gall <tom.gall@linaro.org>,
        Sumit Semwal <sumit.semwal@linaro.org>,
        John Stultz <john.stultz@linaro.org>,
        Amit Pundir <amit.pundir@linaro.org>,
        devicetree <devicetree@vger.kernel.org>,
        linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
        linux-clk <linux-clk@vger.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux Samsung SOC <linux-samsung-soc@vger.kernel.org>,
        "open list:SERIAL DRIVERS" <linux-serial@vger.kernel.org>
Subject: Re: [PATCH 12/12] arm64: dts: exynos: Add Exynos850 SoC support
In-Reply-To: <CAPLW+4mMF9B2BiY2hTgHz5=DNbDJZ7TDzt=Xefb5tDKwQhpEew@mail.gmail.com>
References: <20210730144922.29111-1-semen.protsenko@linaro.org>
        <20210730144922.29111-13-semen.protsenko@linaro.org>
        <15871f8ced3c757fad1ab3b6e62c4e64@misterjones.org>
        <CAPLW+4=v4bDcuxGVqs06mobGj34At4cD+vg48b4dPujarS07Tg@mail.gmail.com>
        <87k0l1w8y5.wl-maz@kernel.org>
        <CAPLW+4mMF9B2BiY2hTgHz5=DNbDJZ7TDzt=Xefb5tDKwQhpEew@mail.gmail.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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.