Message-ID: <8c1eaddd-577b-9c2a-aa6a-9ee716178d4a@sholland.org>
Date:   Sun, 3 Jan 2021 21:46:10 -0600
From:   Samuel Holland <samuel@...lland.org>
To:     Marc Zyngier <maz@...nel.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 1/3/21 7:10 AM, Marc Zyngier wrote:
> 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.

Yes, there is an internal S/R latch.
 - For edge interrupts, the latch is set once for each pulse.
 - For level interrupts, it gets set continuously as long as the
   pin is high/low.
 - Writing a "1" to bit 0 of PENDING resets the latch.
 - The output of the latch goes to the GIC.

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

For level interrupts, I have to reset the latch (see above) after the source of
the interrupt is cleared.

That was the bug with v2: I set IRQ_EOI_THREADED so .irq_eoi would run after the
thread. But with GICv2 EOImode==0, that blocked other interrupts from being
received during the IRQ thread. Which is why I moved it to .irq_unmask and
removed the flag: so .irq_eoi runs at the end of the hardirq (unblocking further
interrupts at the GIC), and .irq_unmask resets the latch at the end of the thread.

With the flag removed, but still clearing the latch in .irq_eoi, every edge IRQ
was followed by a second, spurious IRQ after the thread finished.

Does that make sense?

>>>> +
>>>> +	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.

Will do.

>>
>>>> +			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.

Okay, I'll do 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.

I will try that.

>>
>>>> +
>>>> +	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.
> 

Thanks,
Samuel

Powered by blists - more mailing lists