Message-ID: <06620a31-ed81-5e3e-c81a-047d986769fd@marcan.st>
Date:   Fri, 5 Feb 2021 16:41:39 +0900
From:   Hector Martin 'marcan' <marcan@...can.st>
To:     Arnd Bergmann <arnd@...nel.org>
Cc:     SoC Team <soc@...nel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Marc Zyngier <maz@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        DTML <devicetree@...r.kernel.org>,
        Olof Johansson <olof@...om.net>
Subject: Re: [PATCH 15/18] irqchip/apple-aic: Add support for the Apple
 Interrupt Controller

On 05/02/2021 08.04, Arnd Bergmann wrote:
> On Thu, Feb 4, 2021 at 11:06 PM Hector Martin 'marcan' <marcan@...can.st> wrote:
>> If we split it up again, one of the two still needs to be the root,
>> decide whether what fired is an IRQ or FIQ, and dispatch accordingly. Or
>> we could have three nodes and have one root handler dispatch to IRQ and
>> FIQ nodes, but that sounds like overkill... (?)
> 
> Maybe I'm misreading the low-level entry code, but my impression
> was that the fiq and irq exception vectors could just be pointed to
> two different root drivers from the code in kernel_ventry

Certainly, but we'd have to introduce a fiq handler global and duplicate 
the handler code; this is what was done in the previous submission, but 
I seem to recall someone (Marc?) mentioned it would be cleaner to just 
merge them into the single IRQ path and discriminate in the irqchip, 
which is what I did here.

I can certainly go with either solution; I don't have a strong 
preference here.

Advantages of split path:

* More orthogonal

Advantages of merged path:

* Minimizes common vector changes needed for a single platform
* Keeps FIQ/IRQ code common, so FIQs are less likely to be accidentally 
broken by people not testing on Apple platforms.

Unclear:

* Performance. Split path runs less code, merged path has lower icache 
pressure.

>> Are you proposing just having different drivers/nodes in the same file,
>> or implementing these as separate drivers in separate files?
> 
> I was thinking of separate driver files.

That's what I previously had then :)

If this is the way to go I can certainly go back to that.

> I looked at other architectures, and found that at least powerpc
> and sparc64 have a really minimal timer tick, with their timer_interrupt()
> function getting called directly from the exception vector, and
> doing a minimum of accounting (irq_enter(), statistics, ...) manually.
> 
> It's a different question if we want to do that, or if there should always
> be an irqchip for consistency.

I think the issue here is that those platforms presumably have *one* 
timer hard wired to a specific exception vector (e.g. on PowerPC that's 
the decrementer). So, that setup is shared by all implementations in 
that platform.

But on ARM64, the architectural timer is supposed to go through an 
irqchip (GIC in normal platforms), it's just that here it ended up 
hard-wired to FIQ - though not alone, since fast IPIs are also there, so 
we can't treat it as a strict "timer vector" either.

So even if we could do this for Apple SoCs, it would be a non-standard 
setup, since every other ARM64 platform puts the timer behind an 
irqchip. Therefore, I think it makes sense to always go through an 
irqchip, rather than introduce a bypass for these SoCs.

Also worth noting that we have at least two functional hardware timers 
here (not sure if there are more, we run with HCR_EL2.E2H=1 in m1n1 
which maps the EL2 timer to be the EL1 timer; I'm not yet sure if 
setting that to 0 will expose extra HV timers or not) wired to the same 
FIQ. I confirmed that both the virtual and physical timers function 
independently in m1n1.

I did confirm there are no secure timers, which is expected given that 
there is no EL3 on these chips.

> Benchmarking would at least help understand why there are two.

Well, they call them "Fast IPIs" so *presumably* they are faster, but 
we'll see :)

> I don't think we have to pay too much attention to preparing the
> code design for it, we can always change it when needed. However,
> anything that impacts the DT binding here would have to be designed
> to not get in the way of adding it later.

I think this shouldn't pose much of a problem, since IPIs aren't exposed 
in the DT anyway. As long as we decide how we're handling IRQs vs FIQs 
(one or two nodes/drivers), then either of them could take 
responsibility for handling IPIs depending on the platform. We should 
probably just add a "fast-ipi" property to both nodes on platforms that 
support that, so that the drivers can make the decision based on it. Or 
perhaps that should be done with different compatibles?

-- 
Hector Martin "marcan" (marcan@...can.st)
Public Key: https://mrcn.st/pub

Powered by blists - more mailing lists