Re: [PATCH v4 03/10] irqchip/sun6i-r: Use a stacked irqchip driver
On 2021-01-15 04:01, Samuel Holland wrote:
Hello,
On 1/14/21 3:06 PM, Marc Zyngier wrote:
Hi Samuel,
On 2021-01-12 05:59, Samuel Holland wrote:
[...]
+static void sun6i_r_intc_ack_nmi(void)
+{
+ writel(SUN6I_NMI_BIT, base + SUN6I_IRQ_PENDING(0));
writel_relaxed()
irq_chip_unmask_parent(), which calls gic_unmask_irq(), is called
immediately after this in .irq_unmask. Since gic_unmask_irq() also uses
writel_relaxed(), the GIC write could be ordered before the write here.
That's odd. writel() places a barrier *before* the actual write,
ensuring that this write is ordered w.r.t. previous accesses.
If you are trying to ensure ordering with what follows, you need
an explicit barrier after this access.
I guess that in the end, you may need both, as what you have orders
the access to GICC_AIR to take place before the write to this pending
register, and you also need to provide the ordering you just described.
I was getting occasional spurious interrupts (1 out of each 20-25) when
using a level trigger, which were resolved by switching to writel()
here.
I mentioned this in the changelog, but it probably deserves a comment
in
the code as well. Or maybe I should use an explicit barrier somewhere?
Please document it in the code. This is subtle enough to warrant a good
description.
+}
+
+static void sun6i_r_intc_nmi_ack(struct irq_data *data)
+{
+ if (irqd_get_trigger_type(data) & IRQ_TYPE_EDGE_BOTH)
+ sun6i_r_intc_ack_nmi();
+ else
+ data->chip_data = SUN6I_NMI_NEEDS_ACK;
+}
+
+static void sun6i_r_intc_nmi_eoi(struct irq_data *data)
+{
+ /* For oneshot IRQs, delay the ack until the IRQ is unmasked. */
+ if (data->chip_data == SUN6I_NMI_NEEDS_ACK &&
!irqd_irq_masked(data))
{
+ sun6i_r_intc_ack_nmi();
+ data->chip_data = 0;
nit: NULL rather than 0?
NULL seemed less appropriate since I'm not using the field as a
pointer,
but I don't have a strong opinion about it.
chip_data *is* a pointer, which is why we conventionally use NULL rather
than an integer value. Up to you.
[...]
+static struct irq_chip sun6i_r_intc_nmi_chip = {
+ .name = "sun6i-r-intc",
+ .irq_ack= sun6i_r_intc_nmi_ack,
+ .irq_mask = irq_chip_mask_parent,
+ .irq_unmask = sun6i_r_intc_nmi_unmask,
+ .irq_eoi= sun6i_r_intc_nmi_eoi,
+ .irq_set_affinity = irq_chip_set_affinity_parent,
+ .irq_set_type = sun6i_r_intc_nmi_set_type,
+ .irq_set_irqchip_state = sun6i_r_intc_nmi_set_irqchip_state,
You probably also want to wire irq_get_irqchip_state(), while
you're at it.
I thought if the interrupt was pending here, it would necessarily also
be pending at the GIC, so adding a separate layer would be redundant.
irq_set_vcpu_affinity(), __irq_get_irqchip_state(), and
irq_set_irqchip_state() [the functions, not the callbacks] have the
interesting property that they search up the irqdomain hierarchy for
the
first irqdomain with the callback. So if all the callback would do is
defer to its parent, it doesn't need to be provided at all*.
Ah, of course... I even wrote that code!
Thanks,
M.
--
Jazz is not dead. It just smells funny...
Re: [PATCH v4 03/10] irqchip/sun6i-r: Use a stacked irqchip driver
Hello,
On 1/14/21 3:06 PM, Marc Zyngier wrote:
> Hi Samuel,
>
> On 2021-01-12 05:59, Samuel Holland wrote:
>
> [...]
>
>> +static void sun6i_r_intc_ack_nmi(void)
>> +{
>> +writel(SUN6I_NMI_BIT, base + SUN6I_IRQ_PENDING(0));
>
> writel_relaxed()
irq_chip_unmask_parent(), which calls gic_unmask_irq(), is called
immediately after this in .irq_unmask. Since gic_unmask_irq() also uses
writel_relaxed(), the GIC write could be ordered before the write here.
I was getting occasional spurious interrupts (1 out of each 20-25) when
using a level trigger, which were resolved by switching to writel() here.
I mentioned this in the changelog, but it probably deserves a comment in
the code as well. Or maybe I should use an explicit barrier somewhere?
>> +}
>> +
>> +static void sun6i_r_intc_nmi_ack(struct irq_data *data)
>> +{
>> +if (irqd_get_trigger_type(data) & IRQ_TYPE_EDGE_BOTH)
>> +sun6i_r_intc_ack_nmi();
>> +else
>> +data->chip_data = SUN6I_NMI_NEEDS_ACK;
>> +}
>> +
>> +static void sun6i_r_intc_nmi_eoi(struct irq_data *data)
>> +{
>> +/* For oneshot IRQs, delay the ack until the IRQ is unmasked. */
>> +if (data->chip_data == SUN6I_NMI_NEEDS_ACK && !irqd_irq_masked(data))
>> {
>> +sun6i_r_intc_ack_nmi();
>> +data->chip_data = 0;
>
> nit: NULL rather than 0?
NULL seemed less appropriate since I'm not using the field as a pointer,
but I don't have a strong opinion about it.
> [...]
>
>> +static struct irq_chip sun6i_r_intc_nmi_chip = {
>> +.name = "sun6i-r-intc",
>> +.irq_ack= sun6i_r_intc_nmi_ack,
>> +.irq_mask = irq_chip_mask_parent,
>> +.irq_unmask = sun6i_r_intc_nmi_unmask,
>> +.irq_eoi= sun6i_r_intc_nmi_eoi,
>> +.irq_set_affinity = irq_chip_set_affinity_parent,
>> +.irq_set_type = sun6i_r_intc_nmi_set_type,
>> +.irq_set_irqchip_state = sun6i_r_intc_nmi_set_irqchip_state,
>
> You probably also want to wire irq_get_irqchip_state(), while
> you're at it.
I thought if the interrupt was pending here, it would necessarily also
be pending at the GIC, so adding a separate layer would be redundant.
irq_set_vcpu_affinity(), __irq_get_irqchip_state(), and
irq_set_irqchip_state() [the functions, not the callbacks] have the
interesting property that they search up the irqdomain hierarchy for the
first irqdomain with the callback. So if all the callback would do is
defer to its parent, it doesn't need to be provided at all*.
*except in case this irqdomain has a child which calls
irq_chip_get_parent_state(), which does not look past its immediate
parent. But I did not think that case was worth worrying about.
Cheers,
Samuel
> Otherwise, looks pretty good now.
>
> Thanks,
>
> M.
>
Re: [PATCH v4 03/10] irqchip/sun6i-r: Use a stacked irqchip driver
Hi Samuel,
On 2021-01-12 05:59, Samuel Holland wrote:
[...]
> +static void sun6i_r_intc_ack_nmi(void)
> +{
> + writel(SUN6I_NMI_BIT, base + SUN6I_IRQ_PENDING(0));
writel_relaxed()
> +}
> +
> +static void sun6i_r_intc_nmi_ack(struct irq_data *data)
> +{
> + if (irqd_get_trigger_type(data) & IRQ_TYPE_EDGE_BOTH)
> + sun6i_r_intc_ack_nmi();
> + else
> + data->chip_data = SUN6I_NMI_NEEDS_ACK;
> +}
> +
> +static void sun6i_r_intc_nmi_eoi(struct irq_data *data)
> +{
> + /* For oneshot IRQs, delay the ack until the IRQ is unmasked. */
> + if (data->chip_data == SUN6I_NMI_NEEDS_ACK && !irqd_irq_masked(data))
> {
> + sun6i_r_intc_ack_nmi();
> + data->chip_data = 0;
nit: NULL rather than 0?
[...]
> +static struct irq_chip sun6i_r_intc_nmi_chip = {
> + .name = "sun6i-r-intc",
> + .irq_ack= sun6i_r_intc_nmi_ack,
> + .irq_mask = irq_chip_mask_parent,
> + .irq_unmask = sun6i_r_intc_nmi_unmask,
> + .irq_eoi= sun6i_r_intc_nmi_eoi,
> + .irq_set_affinity = irq_chip_set_affinity_parent,
> + .irq_set_type = sun6i_r_intc_nmi_set_type,
> + .irq_set_irqchip_state = sun6i_r_intc_nmi_set_irqchip_state,
You probably also want to wire irq_get_irqchip_state(), while
you're at it.
Otherwise, looks pretty good now.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
[PATCH v4 03/10] irqchip/sun6i-r: Use a stacked irqchip driver
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.
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.
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 setting the NMI trigger type, but not actually handle any IRQ here.
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 from number 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.
Since R_INTC is in the always-on power domain, and its output is visible
to the power management coprocessor, a stacked irqchip driver provides a
simple way to add wakeup support to any of its IRQs. That is the next
patch; for now, just the NMI is moved over.
This commit mostly reverts commit 173bda53b340 ("irqchip/sunxi-nmi:
Support sun6i-a31-r-intc compatible").
Signed-off-by: Samuel Holland
---
arch/arm/mach-sunxi/Kconfig | 2 +
arch/arm64/Kconfig.platforms| 2 +
drivers/irqchip/Makefile| 1 +
drivers/irqchip/irq-sun6i-r.c | 284
drivers/irqchip/irq-sunxi-nmi.c | 26 +--
5 files changed, 292 insertions(+), 23 deletions(-)
create mode 100644 drivers/irqchip/irq-sun6i-r.c
diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
index eeadb1a4dcfe..e5c2fce281cd 100644
--- a/arch/arm/mach-sunxi/Kconfig
+++ b/arch/arm/mach-sunxi/Kconfig
@@ -6,6 +6,8 @@ menuconfig ARCH_SUNXI
select CLKSRC_MMIO
select GENERIC_IRQ_CHIP
select GPIOLIB
+ select IRQ_DOMAIN_HIERARCHY
+ select IRQ_FASTEOI_HIERARCHY_HANDLERS
select PINCTRL
select PM_OPP
select SUN4I_TIMER
diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index 6eecdef538bd..f2aa1518c6f4 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -17,6 +17,8 @@ config ARCH_SUNXI
bool "Allwinner sunxi 64-bit SoC Family"
select ARCH_HAS_RESET_CONTROLLER
select GENERIC_IRQ_CHIP
+ select IRQ_DOMAIN_HIERARCHY
+ select IRQ_FASTEOI_HIERARCHY_HANDLERS
select PINCTRL
select RESET_CONTROLLER
help
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 0ac93bfaec61..95221e74ee99 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_OR1K_PIC)+=
irq-or1k-pic.o
obj-$(CONFIG_ORION_IRQCHIP)+= irq-orion.o
obj-$(CONFIG_OMAP_IRQCHIP) += irq-omap-intc.o
obj-$(CONFIG_ARCH_SUNXI) += irq-sun4i.o
+obj-$(CONFIG_ARCH_SUNXI) += irq-sun6i-r.o
obj-$(CONFIG_ARCH_SUNXI) += irq-sunxi-nmi.o
obj-$(CONFIG_ARCH_SPEAR3XX)+= spear-shirq.o
obj-$(CONFIG_ARM_GIC) += irq-gic.o irq-gic-common.o
diff --git a/drivers/irqchip/irq-sun6i-r.c b/drivers/irqchip/irq-sun6i-r.c
new file mode 100644
index ..d04d067423f4
--- /dev/null
+++ b/drivers/irqchip/irq-sun6i-r.c
@@ -0,0 +1,284 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * The R_INTC in Allwinner A31 and newer SoCs manages several types of
+ * interrupts, as shown below:
+ *
+ * NMI IRQDIRECT IRQs MUXED IRQ
