Message-ID: <8735zikvyq.wl-maz@kernel.org>
Date:   Sun, 03 Jan 2021 13:10:05 +0000
From:   Marc Zyngier <maz@...nel.org>
To:     Samuel Holland <samuel@...lland.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Rob Herring <robh+dt@...nel.org>,
        Maxime Ripard <mripard@...nel.org>,
        Chen-Yu Tsai <wens@...e.org>,
        Jernej Skrabec <jernej.skrabec@...l.net>,
        Russell King <linux@...linux.org.uk>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        Ondrej Jirman <megous@...ous.com>, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 03/10] irqchip/sun6i-r: Use a stacked irqchip driver

On Sun, 03 Jan 2021 12:08:43 +0000,
Samuel Holland <samuel@...lland.org> wrote:
> 
> On 1/3/21 5:27 AM, Marc Zyngier wrote:
> > On Sun, 03 Jan 2021 10:30:54 +0000,
> > Samuel Holland <samuel@...lland.org> 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:
> >>  - To control the trigger, latch, and mask for the NMI input pin
> >>  - To provide the interrupt input for the ARISC coprocessor
> >>
> >> As this interrupt controller is not documented, information about it
> >> comes from vendor-provided firmware blobs 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 only has one or two registers of each kind (max 32 or 64 IRQs)
> >>  - Multiplexing logic is added to support additional inputs
> >>  - There is no FIQ-related logic
> >>  - There is no interrupt priority logic
> >>
> >> In order to fulfill its two purposes, this hardware block combines four
> >> 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. In other words, bit 0 of IRQ_ENABLE_REG *does* affect
> >> the NMI IRQ seen at the GIC.
> >>
> >> The NMI is followed by a contiguous block of 15 "direct" (my name for
> >> them) IRQ inputs that are connected in parallel to both R_INTC and the
> >> GIC. Or in other words, these bits of IRQ_ENABLE_REG *do not* affect the
> >> IRQs seen at the GIC.
> >>
> >> Following the direct IRQs are the ARISC's copy of banked IRQs for shared
> >> peripherals. These are not relevant to Linux. The remaining IRQs are
> >> connected to a multiplexer and provide access to the first (up to) 128
> >> SPIs from the ARISC. This range of SPIs overlaps with the direct IRQs.
> >>
> >> 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. This path is also irrelevant to Linux.
> > 
> > An ASCII-art version of this would help a lot, and would look good in
> > the driver code...
> 
> There is this diagram which I forgot to include in the cover letter:
> 
>   https://linux-sunxi.org/images/5/5c/R_INTC.png
> 
> I can try to come up with some ASCII art.
> 
> >> 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 the next patch; for now, just the NMI is moved over.
> >>
> >> To allow access to all multiplexed IRQs, this driver requires a new
> >> binding where the interrupt number matches the GIC interrupt number.
> >> (This moves the NMI number from 0 to 32 or 96, depending on the SoC.)
> >> For simplicity, copy the three-cell GIC binding; this disambiguates
> >> interrupt 0 in the old binding (the NMI) from interrupt 0 in the new
> >> binding (SPI 0) by the number of cells.
> >>
> >> This commit mostly reverts commit 173bda53b340 ("irqchip/sunxi-nmi:
> >> Support sun6i-a31-r-intc compatible").
> >>
> >> Signed-off-by: Samuel Holland <samuel@...lland.org>
> >> ---
> >>  arch/arm/mach-sunxi/Kconfig     |   1 +
> >>  arch/arm64/Kconfig.platforms    |   1 +
> >>  drivers/irqchip/Makefile        |   1 +
> >>  drivers/irqchip/irq-sun6i-r.c   | 267 ++++++++++++++++++++++++++++++++
> >>  drivers/irqchip/irq-sunxi-nmi.c |  26 +---
> >>  5 files changed, 273 insertions(+), 23 deletions(-)
> >>  create mode 100644 drivers/irqchip/irq-sun6i-r.c
> >>
> > 
> > [...]
> > 
> >> diff --git a/drivers/irqchip/irq-sun6i-r.c b/drivers/irqchip/irq-sun6i-r.c
> >> new file mode 100644
> >> index 000000000000..7490ade7b254
> >> --- /dev/null
> >> +++ b/drivers/irqchip/irq-sun6i-r.c
> >> @@ -0,0 +1,267 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +//
> >> +// R_INTC driver for Allwinner A31 and newer SoCs
> >> +//
> >> +
> >> +#include <linux/irq.h>
> >> +#include <linux/irqchip.h>
> >> +#include <linux/irqdomain.h>
> >> +#include <linux/of.h>
> >> +#include <linux/of_address.h>
> >> +#include <linux/of_irq.h>
> >> +
> >> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> >> +
> >> +/*
> >> + * The R_INTC manages between 32 and 64 IRQs, divided into four groups. Example
> >> + * bit numbers are for the original variant in the A31:
> >> + *
> >> + *   Bit      0: The "External NMI" input, connected in series to a GIC SPI.
> >> + *   Bits  1-15: "Direct" IRQs for ARISC peripherals, connected in parallel to
> >> + *               the GIC and mapped 1:1 to SPIs numerically following the NMI.
> >> + *   Bits 16-18: "Banked" IRQs for peripherals that have separate interfaces
> >> + *               for the ARM CPUs and ARISC. These do not map to any GIC SPI.
> >> + *   Bits 19-31: "Muxed" IRQs, each corresponding to a group of up to 8 SPIs.
> >> + *               Later variants added a second PENDING and ENABLE register to
> >> + *               make use of all 128 mux inputs (16 IRQ lines).
> >> + *
> >> + * Since the direct IRQs are inside the muxed IRQ range, they do not increase
> >> + * the number of HWIRQs needed.
> >> + */
> >> +#define SUN6I_NR_IRQS			64
> >> +#define SUN6I_NR_DIRECT_IRQS		16
> >> +#define SUN6I_NR_MUX_INPUTS		128
> >> +#define SUN6I_NR_HWIRQS			SUN6I_NR_MUX_INPUTS
> >> +
> >> +#define SUN6I_NMI_CTRL			(0x0c)
> >> +#define SUN6I_IRQ_PENDING(n)		(0x10 + 4 * (n))
> >> +#define SUN6I_IRQ_ENABLE(n)		(0x40 + 4 * (n))
> >> +#define SUN6I_MUX_ENABLE(n)		(0xc0 + 4 * (n))
> >> +
> >> +#define SUN6I_NMI_IRQ_BIT		BIT(0)
> >> +
> >> +static void __iomem *base;
> >> +static irq_hw_number_t nmi_hwirq;
> >> +static u32 nmi_type;
> >> +
> >> +static struct irq_chip sun6i_r_intc_edge_chip;
> >> +static struct irq_chip sun6i_r_intc_level_chip;
> >> +
> >> +static void sun6i_r_intc_nmi_ack(void)
> >> +{
> >> +	/*
> >> +	 * The NMI channel has a latch separate from its trigger type.
> >> +	 * This latch must be cleared to clear the signal to the GIC.
> >> +	 */
> >> +	writel_relaxed(SUN6I_NMI_IRQ_BIT, base + SUN6I_IRQ_PENDING(0));
> >> +}
> >> +
> >> +static void sun6i_r_intc_irq_mask(struct irq_data *data)
> >> +{
> >> +	if (data->hwirq == nmi_hwirq)
> >> +		sun6i_r_intc_nmi_ack();
> > 
> > I'm a bit worried by this. I can see it working with level interrupts
> > (you can clear the input, and if the interrupt is asserted, it will
> > fire again), but I'm worried that it will simply result in lost
> > interrupts for edge signalling.
> 
> For edge interrupts, don't you want to ack as early as possible,
> before the handler clears the source of the interrupt? That way if a
> second interrupt comes in while you're handling the first one, you
> don't ack the second one without handling it?

It completely depends on what this block does. If, as I expect, it
latches the interrupt, then it needs clearing after the GIC has acked
the incoming interrupt.

> > It also begs the question: why would you want to clear the signal to
> > the GIC on mask (or unmask)? The expectations are that a pending
> > interrupt is preserved across a mask/unmask sequence.
> 
> I hadn't thought about anything masking the IRQ outside of the
> handler; but you're right, this breaks that case. I'm trying to work
> within the constraints of stacking the GIC driver, which assumes
> handle_fasteoi_irq, so it sounds like I should switch back to
> handle_fasteoi_ack_irq and use .irq_ack. Or based on your previous
> paragraph, maybe I'm missing some other consideration?

handle_fasteoi_ack_irq() sounds like a good match for edge
interrupts. Do you actually need to do anything for level signals? If
you do, piggybacking on .irq_eoi would do the trick.

> 
> >> +
> >> +	irq_chip_mask_parent(data);
> >> +}
> >> +
> >> +static void sun6i_r_intc_irq_unmask(struct irq_data *data)
> >> +{
> >> +	if (data->hwirq == nmi_hwirq)
> >> +		sun6i_r_intc_nmi_ack();
> >> +
> >> +	irq_chip_unmask_parent(data);
> >> +}
> >> +
> >> +static int sun6i_r_intc_irq_set_type(struct irq_data *data, unsigned int type)
> >> +{
> >> +	/*
> >> +	 * The GIC input labeled "External NMI" connects to bit 0 of the R_INTC
> >> +	 * PENDING register, not to the pin directly. So the trigger type of the
> >> +	 * GIC input does not depend on the trigger type of the NMI pin itself.
> >> +	 *
> >> +	 * Only the NMI channel is routed through this interrupt controller on
> >> +	 * its way to the GIC. Other IRQs are routed to the GIC and R_INTC in
> >> +	 * parallel; they must have a trigger type appropriate for the GIC.
> >> +	 */
> >> +	if (data->hwirq == nmi_hwirq) {
> >> +		struct irq_chip *chip;
> >> +		u32 nmi_src_type;
> >> +
> >> +		switch (type) {
> >> +		case IRQ_TYPE_LEVEL_LOW:
> >> +			chip = &sun6i_r_intc_level_chip;
> >> +			nmi_src_type = 0;
> > 
> > Please add symbolic names for these types.
> 
> I removed them based on your previous comment:
> 
> https://lkml.org/lkml/2020/1/20/278
> > It is unusual to use an enum for values that get directly programmed into the HW.
> 
> Do you want them to be specifically #defines?

Yes please.

> 
> >> +			break;
> >> +		case IRQ_TYPE_EDGE_FALLING:
> >> +			chip = &sun6i_r_intc_edge_chip;
> >> +			nmi_src_type = 1;
> >> +			break;
> >> +		case IRQ_TYPE_LEVEL_HIGH:
> >> +			chip = &sun6i_r_intc_level_chip;
> >> +			nmi_src_type = 2;
> >> +			break;
> >> +		case IRQ_TYPE_EDGE_RISING:
> >> +			chip = &sun6i_r_intc_edge_chip;
> >> +			nmi_src_type = 3;
> >> +			break;
> >> +		default:
> >> +			pr_err("%pOF: invalid trigger type %d for IRQ %d\n",
> >> +			       irq_domain_get_of_node(data->domain), type,
> >> +			       data->irq);
> > 
> > A failing set_type already triggers a kernel message.
> 
> I'll remove this.
> 
> >> +			return -EBADR;
> > 
> > That's a pretty odd error. I see it used in 3 drivers (including the
> > one this driver replaces), but the canonical error code is -EINVAL.
> 
> I'll change it to -EINVAL.
> 
> >> +		}
> >> +
> >> +		irq_set_chip_handler_name_locked(data, chip,
> >> +						 handle_fasteoi_irq, NULL);
> >> +
> >> +		writel_relaxed(nmi_src_type, base + SUN6I_NMI_CTRL);
> >> +
> >> +		/*
> >> +		 * Use the trigger type from the OF node for the NMI's
> >> +		 * R_INTC to GIC connection.
> >> +		 */
> >> +		type = nmi_type;
> > 
> > This looks wrong. The GIC only supports level-high and edge-rising, so
> > half of the possible settings will result in an error. I assume the
> > R_INTC has an inverter controlled by nmi_src_type, and only outputs
> > something the GIC can grok.
> 
> nmi_type isn't the setting from the incoming IRQ. nmi_type is from
> the interrupts property in the irqchip OF node itself. So this is a
> fwspec for `interrupt-parent = <&gic>;`, i.e. already assumed to be
> GIC-compatible. I'm not sure what you mean by "half of the possible
> settings".

Ah, I see. I misread where nmi_type was coming from. Please ignore me.

> 
> Maybe I should remove the `interrupts` property and hardcode the
> number and type connecting to the GIC? If I did that, I'm not quite
> sure whether it would be high or rising.

I guess that the output of this block is a level signal, so you should
probably enforce that.

> The output to the GIC is literally the bit in the pending register:
> 1 for pending, 0 for not. Since that is after a latch (regardless of
> the input trigger type) it sounds like it should be level-high,
> because a pulse would have already been converted to a steady
> signal. Or does it depend on when I clear the latch?

Level and edge will have different latch clearing requirements, as
outlined above. For level, it needs to be cleared after handling the
interrupt, at the point where you would deactivate it in the GIC
(.irq_eoi). For edge, that's an Ack.

[...]

> >> +static int sun6i_r_intc_domain_alloc(struct irq_domain *domain,
> >> +				     unsigned int virq,
> >> +				     unsigned int nr_irqs, void *arg)
> >> +{
> >> +	struct irq_fwspec *fwspec = arg;
> >> +	struct irq_fwspec gic_fwspec;
> >> +	unsigned long hwirq;
> >> +	unsigned int type;
> >> +	int i, ret;
> >> +
> >> +	ret = sun6i_r_intc_domain_translate(domain, fwspec, &hwirq, &type);
> >> +	if (ret)
> >> +		return ret;
> >> +	if (hwirq + nr_irqs > SUN6I_NR_HWIRQS)
> >> +		return -EINVAL;
> >> +
> >> +	/* Construct a GIC-compatible fwspec from this fwspec. */
> >> +	gic_fwspec = (struct irq_fwspec) {
> >> +		.fwnode      = domain->parent->fwnode,
> >> +		.param_count = 3,
> >> +		.param       = { GIC_SPI, hwirq, type },
> >> +	};
> >> +
> >> +	for (i = 0; i < nr_irqs; ++i)
> >> +		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> >> +					      &sun6i_r_intc_level_chip, NULL);
> > 
> > Unconditionally level, without looking at the requested type?
> 
> __setup_irq calls __irq_set_trigger if any trigger is provided, which calls
> chip->irq_set_type unconditionally. So I don't think the chip here matters,
> because .irq_set_type will be called before the IRQ is enabled anyway. Again,
> maybe I'm missing something.

It doesn't really matter, but it is a bit odd to have the information
at hand, and ignore it. This at least deserves a comment.

> For non-NMI IRQs, the choice of chip doesn't matter, because this
> driver handles nothing but .irq_set_wake for them (should I provide
> a third chip for this that doesn't provide its own
> ack/mask/unmask?).

I think you could handle the NMI with its own single irqchip (for both
level and edge, only checking for the configuration in ack/eoi), and
have a non-NMI chip for the rest.

> 
> >> +
> >> +	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &gic_fwspec);
> >> +}
> >> +
> >> +static const struct irq_domain_ops sun6i_r_intc_domain_ops = {
> >> +	.translate	= sun6i_r_intc_domain_translate,
> >> +	.alloc		= sun6i_r_intc_domain_alloc,
> >> +	.free		= irq_domain_free_irqs_common,
> >> +};
> >> +
> >> +static void sun6i_r_intc_resume(void)
> >> +{
> >> +	int i;
> >> +
> >> +	/* Only the NMI is relevant during normal operation. */
> >> +	writel_relaxed(SUN6I_NMI_IRQ_BIT, base + SUN6I_IRQ_ENABLE(0));
> >> +	for (i = 1; i < BITS_TO_U32(SUN6I_NR_IRQS); ++i)
> >> +		writel_relaxed(0, base + SUN6I_IRQ_ENABLE(i));
> > 
> > If only the NMI is relevant, why are the other interrupts enabled?
> > Shouldn't this be moved to the following patch (I presume this is
> > wake-up related...).
> 
> The other IRQs aren't enabled? I'm writing all zeroes to the enable register.

Duh. -ECANTREAD.

> >> +}
> >> +
> >> +static int __init sun6i_r_intc_init(struct device_node *node,
> >> +				    struct device_node *parent)
> >> +{
> >> +	struct irq_domain *domain, *parent_domain;
> >> +	struct of_phandle_args parent_irq;
> >> +	int ret;
> >> +
> >> +	/* Extract the NMI's R_INTC to GIC mapping from the OF node. */
> >> +	ret = of_irq_parse_one(node, 0, &parent_irq);
> >> +	if (ret)
> >> +		return ret;
> >> +	if (parent_irq.args_count < 3 || parent_irq.args[0] != GIC_SPI)
> >> +		return -EINVAL;
> >> +	nmi_hwirq = parent_irq.args[1];
> >> +	nmi_type = parent_irq.args[2];
> > 
> > This looks a lot like the translate callback.
> 
> Yes, I could use that here.
> 
> >> +
> >> +	parent_domain = irq_find_host(parent);
> >> +	if (!parent_domain) {
> >> +		pr_err("%pOF: Failed to obtain parent domain\n", node);
> >> +		return -ENXIO;
> >> +	}
> >> +
> >> +	base = of_io_request_and_map(node, 0, NULL);
> >> +	if (IS_ERR(base)) {
> >> +		pr_err("%pOF: Failed to map MMIO region\n", node);
> >> +		return PTR_ERR(base);
> >> +	}
> >> +
> >> +	sun6i_r_intc_nmi_ack();
> >> +	sun6i_r_intc_resume();
> >> +
> >> +	domain = irq_domain_add_hierarchy(parent_domain, 0,
> >> +					  SUN6I_NR_HWIRQS, node,
> >> +					  &sun6i_r_intc_domain_ops, NULL);
> >> +	if (!domain) {
> >> +		pr_err("%pOF: Failed to allocate domain\n", node);
> >> +		iounmap(base);
> >> +		return -ENOMEM;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +IRQCHIP_DECLARE(sun6i_r_intc, "allwinner,sun6i-a31-r-intc", sun6i_r_intc_init);
> > 
> > Thanks,
> > 
> > 	M.
> > 
> 
> Thank you for your (extremely quick, I must say) review!

Locked up home until Wednesday. I try to keep myself busy...

Thanks,

	M.

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

Powered by blists - more mailing lists