Message-ID: <20200626133230.yawpmhrhgd2pxzqn@gilmour.lan>
Date:   Fri, 26 Jun 2020 15:32:30 +0200
From:   Maxime Ripard <maxime@...no.tech>
To:     Samuel Holland <samuel@...lland.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Jason Cooper <jason@...edaemon.net>,
        Marc Zyngier <maz@...nel.org>,
        Rob Herring <robh+dt@...nel.org>, Chen-Yu Tsai <wens@...e.org>,
        Russell King <linux@...linux.org.uk>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-sunxi@...glegroups.com
Subject: Re: [PATCH v2 1/9] irqchip/sun6i-r: Use a stacked irqchip driver

Hi Samuel,

On Mon, Jun 15, 2020 at 12:29:50AM -0500, Samuel Holland wrote:
> On 6/8/20 3:48 AM, Maxime Ripard wrote:
> > On Sun, May 24, 2020 at 11:12:54PM -0500, Samuel Holland wrote:
> >> The R_INTC in the A31 and newer sun8i/sun50i SoCs is more similar to the
> >> original sun4i interrupt controller than the sun7i/sun9i NMI controller.
> >> It is used for two distinct purposes:
> >>  1) To control the trigger, latch, and mask for the NMI input pin
> >>  2) To provide the interrupt input for the ARISC coprocessor
> >>
> >> As this interrupt controller is not documented, information about it
> >> comes from vendor-provided ARISC firmware and from experimentation.
> >>
> >> Like the original sun4i interrupt controller, it has:
> >>  - A VECTOR_REG at 0x00 (configurable via the BASE_ADDR_REG at 0x04)
> >>  - A NMI_CTRL_REG, PENDING_REG, and ENABLE_REG as used by both the
> >>    sun4i and sunxi-nmi drivers
> >>  - A MASK_REG at 0x50
> >>  - A RESP_REG at 0x60
> >>
> >> Differences from the sun4i interrupt controller appear to be:
> >>  - It is only known to have one register of each kind (max 32 inputs)
> >>  - There is no FIQ-related logic
> >>  - There is no interrupt priority logic
> >>
> >> In order to fulfill its two purposes, this hardware block combines two
> >> types of IRQs. First, the NMI pin is routed to the "IRQ 0" input on this
> >> chip, with a trigger type controlled by the NMI_CTRL_REG. The "IRQ 0
> >> pending" output from this chip, if enabled, is then routed to a SPI IRQ
> >> input on the GIC, as IRQ_TYPE_LEVEL_HIGH. In other words, bit 0 of
> >> ENABLE_REG *does* affect the NMI IRQ seen at the GIC.
> >>
> >> The NMI is then followed by a contiguous block of (at least) 15 IRQ
> >> inputs that are connected in parallel to both R_INTC and the GIC. Or
> >> in other words, the other bits of ENABLE_REG *do not* affect the IRQs
> >> seen at the GIC.
> >>
> >> Finally, the global "IRQ pending" output from R_INTC, after being masked
> >> by MASK_REG and RESP_REG, is connected to the "external interrupt" input
> >> of the ARISC CPU (an OR1200). This path is not relevant to Linux.
> >>
> >> Because of the 1:1 correspondence between R_INTC and GIC inputs, this is
> >> a perfect scenario for using a stacked irqchip driver. We want to hook
> >> into enabling/disabling IRQs to add more features to the GIC
> >> (specifically to allow masking the NMI and setting its trigger type),
> >> but we don't need to actually handle the IRQ in this driver.
> >>
> >> And since R_INTC is in the always-on power domain, and its output is
> >> connected directly in to the power management coprocessor, a stacked
> >> irqchip driver provides a simple way to add wakeup support to this set
> >> of IRQs. That is a future patch; for now, just the NMI is moved over.
> >>
> >> This driver keeps the same DT binding as the existing driver. The
> >> "interrupt" property of the R_INTC node is used to determine 1) the
> >> offset between GIC and R_INTC hwirq numbers and 2) the type of trigger
> >> between the R_INTC "IRQ 0 pending" output and the GIC NMI input.
> >>
> >> This commit mostly reverts commit 173bda53b340 ("irqchip/sunxi-nmi:
> >> Support sun6i-a31-r-intc compatible").
> >>
> >> Signed-off-by: Samuel Holland <samuel@...lland.org>
> > 
> > As usual, thanks for that commit log (and the experiments you did to
> > write it in the first place).
> > 
> > Acked-by: Maxime Ripard <mripard@...nel.org>
> > 
> > Maxime
> 
> I've done more experimenting, and I've learned what comes after the first 16
> IRQs: all of the other SPI IRQs, multiplexed in clusters of 8, with per-IRQ
> masks for the inputs to each cluster.
> 
> In fact, the H6 has so many IRQs that it begins to use the the second register
> in each group (0x14, 0x44, 0x54). This means that more than one register in each
> group are in fact implemented.
> 
> See https://linux-sunxi.org/INTC#IRQ_Mapping for more details.
> 
> The ability to send other IRQs to the AR100 makes it possible to implement
> functionality like USB Remote Wakeup or Wake on LAN without adding complexity to
> the AR100 firmware.
> 
> I will need to update the driver to take advantage of this ability, and it
> raises some questions about the binding. Since the NMI is not the
> lowest-numbered IRQ that can be mapped,

I'm not quite sure I get that part. From the link you mentionned above,
the NMI is the interrupt 0 for all the SoCs, so we shouldn't have any
number lower?

> the numbering scheme would need to change.

As far as I know, upstream we only ever use the 0 interrupt for the AXP,
so it should be fairly easy to come up with a numbering scheme that is
backward compatible with that.

> Maybe the IRQ number should be the same as the GIC SPI IRQ number? But
> this would mean a new compatible.
> 
> The other question is which devices should be routed through this irqchip
> driver? Anything that provides a wakeup source needs to go through it, so it can
> intercept irq_set_wake. Probably other devices should not, as 1) not quite all
> IRQs can even be sent to the AR100 for wakeup (e.g. the A64 appears to stop in
> the middle of the GPU IRQs), and 2) stacking on another irqchip driver adds a
> (tiny) overhead to masking/unmasking during IRQ handling.

It's probably a bit of a non-answer, but I guess all the devices for
which it makes sense? The ethernet / USB controllers that you already
mentionned would make sense, the GPIO banks too, possibly the UARTs?

I guess we could just enable most of the one that makes sense at first,
and then discuss cases we didn't consider as we discover them?

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists