On 1/3/21 5:27 AM, Marc Zyngier wrote: > On Sun, 03 Jan 2021 10:30:54 +0000, > Samuel Holland <samuel@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> >> --- >> 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 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? >> + >> + 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? >> + 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". 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. 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? >> + } >> + >> + return irq_chip_set_type_parent(data, type); >> +} >> + >> +static struct irq_chip sun6i_r_intc_edge_chip = { >> + .name = "sun6i-r-intc", >> + .irq_mask = sun6i_r_intc_irq_mask, >> + .irq_unmask = irq_chip_unmask_parent, >> + .irq_eoi = irq_chip_eoi_parent, >> + .irq_set_affinity = irq_chip_set_affinity_parent, >> + .irq_set_type = sun6i_r_intc_irq_set_type, >> + .irq_get_irqchip_state = irq_chip_get_parent_state, >> + .irq_set_irqchip_state = irq_chip_set_parent_state, >> + .irq_set_vcpu_affinity = irq_chip_set_vcpu_affinity_parent, >> + .flags = IRQCHIP_SET_TYPE_MASKED, >> +}; >> + >> +static struct irq_chip sun6i_r_intc_level_chip = { >> + .name = "sun6i-r-intc", >> + .irq_mask = irq_chip_mask_parent, >> + .irq_unmask = sun6i_r_intc_irq_unmask, >> + .irq_eoi = irq_chip_eoi_parent, >> + .irq_set_affinity = irq_chip_set_affinity_parent, >> + .irq_set_type = sun6i_r_intc_irq_set_type, >> + .irq_get_irqchip_state = irq_chip_get_parent_state, >> + .irq_set_irqchip_state = irq_chip_set_parent_state, >> + .irq_set_vcpu_affinity = irq_chip_set_vcpu_affinity_parent, >> + .flags = IRQCHIP_SET_TYPE_MASKED, >> +}; >> + >> +static int sun6i_r_intc_domain_translate(struct irq_domain *domain, >> + struct irq_fwspec *fwspec, >> + unsigned long *hwirq, >> + unsigned int *type) >> +{ >> + /* Accept the old two-cell binding for the NMI only. */ >> + if (fwspec->param_count == 2 && fwspec->param[0] == 0) { >> + *hwirq = nmi_hwirq; >> + *type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK; >> + return 0; >> + } >> + >> + /* Otherwise this binding should match the GIC SPI binding. */ >> + if (fwspec->param_count < 3) >> + return -EINVAL; >> + if (fwspec->param[0] != GIC_SPI) >> + return -EINVAL; >> + >> + *hwirq = fwspec->param[1]; >> + *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; >> + >> + return 0; >> +} >> + >> +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. 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?). >> + >> + 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. >> +} >> + >> +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! Samuel
