mbox series

[v4,00/10] sunxi: Support IRQ wakeup from deep sleep

Message ID 20210112055950.21209-1-samuel@sholland.org
Headers show
Series sunxi: Support IRQ wakeup from deep sleep | expand

Message

Samuel Holland Jan. 12, 2021, 5:59 a.m. UTC
Allwinner sun6i/sun8i/sun50i SoCs (A31 and newer) have two interrupt
controllers: GIC and R_INTC. GIC does not support wakeup. R_INTC handles
the external NMI pin, and provides 32+ IRQs to the ARISC. The first 16
of these correspond 1:1 to a block of GIC IRQs starting with the NMI.
The last 13-16 multiplex the first (up to) 128 GIC SPIs.

This series replaces the existing chained irqchip driver that could only
control the NMI, with a stacked irqchip driver that also provides wakeup
capability for those multiplexed SPI IRQs. The idea is to preconfigure
the ARISC's IRQ controller, and then the ARISC firmware knows to wake up
as soon as it receives an IRQ. It can also decide how deep it can
suspend based on the enabled wakeup IRQs.

As future work, it may be useful to do the chained->stacked conversion
on the sunxi-nmi driver as well.

Patches 1-2 add the new bindings.
Patch 3 adds the new driver.
Patch 4 adds wakeup capability.
Remaining patches update the device trees to use R_INTC where beneficial.

With appropriate firmware and configuration, this series allows waking
from (and it has been tested with) the RTC, NMI/PMIC (power button, A/C
plug, etc.), all GPIO ports (button, lid switch, modem, etc.), LRADC,
and UARTs. I have tested this patch set on the H3, A64, H5, and H6 SoCs.

---
Changes from v3:
 - Removed A31 fallback from H6 compatible.
 - Switch to additionalProperties in binding.
 - Replace wall of text with ASCII art.
 - Added macros for NMI_SRC_TYPE constants.
 - Renamed NR_IRQS to NR_TOP_LEVEL_IRQS to hopefully be more clear.
 - Use non-relaxed writel in sun6i_r_intc_ack_nmi to fix spurious level
   interrupts (reordering with gic_unmask_irq).
 - Use a single irq_chip for edge and level NMI configurations.
   - For edge, ack ASAP using handle_fasteoi_ack_irq.
   - For level, ack in .irq_unmask if masked at EOI, else in .irq_eoi.
 - Enforce that the R_INTC->GIC trigger is IRQ_TYPE_LEVEL_HIGH.
 - Implement .irq_set_irqchip_state.
 - Move other IRQs to a new irq_chip that only intercepts .irq_set_wake.
 - Use radix instead of linear for the IRQ domain since only a handful
   of the 128 hwirqs will ever be used.

Changes from v2:
 - Fix edge IRQs on GICv2 with EOImode == 0, as found on A83T and older.
   - Replace .irq_ack callback with .irq_mask.
   - Drop IRQCHIP_EOI_THREADED.
   - This removes the dependency on IRQ_FASTEOI_HIERARCHY_HANDLERS.
 - Move IRQ_DOMAIN_HIERARCHY selection to ARCH_SUNXI to fix A83T build.
 - Add support for the second IRQ ENABLE/PENDING register on H6 and up.
 - Add support for multiplexed IRQs beyond the initial 16.
   - This requires a new binding, but keeps old binding compatibility.
   - This requires a separate mux mapping for H6 and up.
 - Rename parent_* => nmi_* because they only apply to the NMI.
 - Merge code common to probe and resume functions.
 - Also run suspend callback at syscore shutdown, for boards with no
   PMIC where firmware is also responsible for poweroff/poweron.
   - These two changes mean nothing is conditional on CONFIG_PM_SLEEP
     anymore, since all code is used even without it.
 - Since the binding changed, update all SoC DTs, A31 and up.
 - Drop r_ir from inclusion (it needs more than an IRQ to wake) and
   include pio (the main pin controller) and (r_)lradc.
 - As there are significant changes, I did not carry forward Maxime's
   Acked-by or Rob's Reviewed-by.

Changes from v1:
 - Use writel_relaxed() instead if writel().
 - Remove use of the MASK register, as it doesn't affect the NMI as seen
   by the GIC. It only affects the IRQs seen by the coprocessor.
 - Leave NMI_HWIRQ enabled at all times, since it can be masked at the
   GIC level (removed .irq_enable and .irq_disable).
 - Use .irq_ack vs .irq_eoi depending on the trigger type, to avoid
   missing interrupts or double interrupts.
   - Because of this change, the driver needs two "irq_chip"s, one
     with .irq_eoi set to our function and one without.
   - Also because of this, we need IRQ_FASTEOI_HIERARCHY_HANDLERS for
     handle_fasteoi_ack_irq(), so our .irq_ack function gets called
     while the GIC driver works as if handle_fasteoi_irq() was used.
 - Inline the SUNXI_SRC_TYPE_* enum into sun6i_r_intc_irq_set_type().
 - Add a comment explaining how the trigger type is used.
 - Don't call irqd_set_trigger_type().
 - Set IRQCHIP_SET_TYPE_MASKED to match the GIC (since flags from this
   driver mask flags from that one).
 - Set IRQCHIP_EOI_THREADED to avoid doubled level interrupts, since the
   latch will be set again as long as the trigger is met.
 - Replace sun6i_r_intc_domain_translate() with
   irq_domain_translate_twocell().
 - Use an enum for the device tree binding.
 - Update commit messages for accuracy and typos.

Samuel Holland (10):
  dt-bindings: irq: sun6i-r: Split the binding from sun7i-nmi
  dt-bindings: irq: sun6i-r: Add a compatible for the H3
  irqchip/sun6i-r: Use a stacked irqchip driver
  irqchip/sun6i-r: Add wakeup support
  ARM: dts: sunxi: Rename nmi_intc to r_intc
  ARM: dts: sunxi: Use the new r_intc binding
  ARM: dts: sunxi: h3/h5: Add r_intc node
  ARM: dts: sunxi: Move wakeup-capable IRQs to r_intc
  arm64: dts: allwinner: Use the new r_intc binding
  arm64: dts: allwinner: Move wakeup-capable IRQs to r_intc

 .../allwinner,sun6i-a31-r-intc.yaml           |  67 ++++
 .../allwinner,sun7i-a20-sc-nmi.yaml           |  10 -
 arch/arm/boot/dts/sun6i-a31-hummingbird.dts   |   4 +-
 arch/arm/boot/dts/sun6i-a31-m9.dts            |   4 +-
 .../boot/dts/sun6i-a31-mele-a1000g-quad.dts   |   4 +-
 arch/arm/boot/dts/sun6i-a31.dtsi              |   8 +-
 arch/arm/boot/dts/sun6i-a31s-primo81.dts      |   4 +-
 .../arm/boot/dts/sun6i-a31s-sina31s-core.dtsi |   4 +-
 .../boot/dts/sun6i-a31s-sinovoip-bpi-m2.dts   |   4 +-
 .../sun6i-a31s-yones-toptech-bs1078-v2.dts    |   4 +-
 .../dts/sun6i-reference-design-tablet.dtsi    |   4 +-
 arch/arm/boot/dts/sun8i-a23-a33.dtsi          |   8 +-
 arch/arm/boot/dts/sun8i-a33-olinuxino.dts     |   4 +-
 .../arm/boot/dts/sun8i-a33-sinlinx-sina33.dts |   4 +-
 .../dts/sun8i-a83t-allwinner-h8homlet-v2.dts  |   4 +-
 arch/arm/boot/dts/sun8i-a83t-bananapi-m3.dts  |   4 +-
 .../boot/dts/sun8i-a83t-cubietruck-plus.dts   |   4 +-
 arch/arm/boot/dts/sun8i-a83t-tbs-a711.dts     |   4 +-
 arch/arm/boot/dts/sun8i-a83t.dtsi             |   5 +-
 arch/arm/boot/dts/sun8i-r16-bananapi-m2m.dts  |   4 +-
 arch/arm/boot/dts/sun8i-r16-parrot.dts        |   4 +-
 .../dts/sun8i-reference-design-tablet.dtsi    |   4 +-
 arch/arm/boot/dts/sunxi-h3-h5.dtsi            |  12 +
 arch/arm/mach-sunxi/Kconfig                   |   2 +
 arch/arm64/Kconfig.platforms                  |   2 +
 .../allwinner/sun50i-a64-amarula-relic.dts    |   2 +-
 .../dts/allwinner/sun50i-a64-bananapi-m64.dts |   2 +-
 .../dts/allwinner/sun50i-a64-nanopi-a64.dts   |   2 +-
 .../dts/allwinner/sun50i-a64-olinuxino.dts    |   2 +-
 .../dts/allwinner/sun50i-a64-orangepi-win.dts |   2 +-
 .../boot/dts/allwinner/sun50i-a64-pine64.dts  |   2 +-
 .../dts/allwinner/sun50i-a64-pinebook.dts     |   2 +-
 .../dts/allwinner/sun50i-a64-pinephone.dtsi   |   2 +-
 .../boot/dts/allwinner/sun50i-a64-pinetab.dts |   2 +-
 .../boot/dts/allwinner/sun50i-a64-sopine.dtsi |   2 +-
 .../boot/dts/allwinner/sun50i-a64-teres-i.dts |   2 +-
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi |   6 +-
 .../dts/allwinner/sun50i-h6-beelink-gs1.dts   |   2 +-
 .../dts/allwinner/sun50i-h6-orangepi-3.dts    |   2 +-
 .../dts/allwinner/sun50i-h6-orangepi.dtsi     |   2 +-
 .../boot/dts/allwinner/sun50i-h6-pine-h64.dts |   4 +-
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi  |   8 +-
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-sun6i-r.c                 | 379 ++++++++++++++++++
 drivers/irqchip/irq-sunxi-nmi.c               |  26 +-
 45 files changed, 542 insertions(+), 92 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/allwinner,sun6i-a31-r-intc.yaml
 create mode 100644 drivers/irqchip/irq-sun6i-r.c

Comments

Maxime Ripard Jan. 14, 2021, 12:16 p.m. UTC | #1
On Mon, Jan 11, 2021 at 11:59:40PM -0600, Samuel Holland wrote:
> Allwinner sun6i/sun8i/sun50i SoCs (A31 and newer) have two interrupt
> controllers: GIC and R_INTC. GIC does not support wakeup. R_INTC handles
> the external NMI pin, and provides 32+ IRQs to the ARISC. The first 16
> of these correspond 1:1 to a block of GIC IRQs starting with the NMI.
> The last 13-16 multiplex the first (up to) 128 GIC SPIs.
> 
> This series replaces the existing chained irqchip driver that could only
> control the NMI, with a stacked irqchip driver that also provides wakeup
> capability for those multiplexed SPI IRQs. The idea is to preconfigure
> the ARISC's IRQ controller, and then the ARISC firmware knows to wake up
> as soon as it receives an IRQ. It can also decide how deep it can
> suspend based on the enabled wakeup IRQs.
> 
> As future work, it may be useful to do the chained->stacked conversion
> on the sunxi-nmi driver as well.
> 
> Patches 1-2 add the new bindings.
> Patch 3 adds the new driver.
> Patch 4 adds wakeup capability.
> Remaining patches update the device trees to use R_INTC where beneficial.
> 
> With appropriate firmware and configuration, this series allows waking
> from (and it has been tested with) the RTC, NMI/PMIC (power button, A/C
> plug, etc.), all GPIO ports (button, lid switch, modem, etc.), LRADC,
> and UARTs. I have tested this patch set on the H3, A64, H5, and H6 SoCs.

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime
Marc Zyngier Jan. 14, 2021, 9:06 p.m. UTC | #2
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.
Marc Zyngier Jan. 14, 2021, 9:44 p.m. UTC | #3
On Tue, 12 Jan 2021 05:59:44 +0000,
Samuel Holland <samuel@sholland.org> wrote:
> 
> Maintain bitmaps of wake-enabled IRQs and mux inputs, and program them
> to the hardware during the syscore phase of suspend and shutdown. Then
> restore the original set of enabled IRQs (only the NMI) during resume.
> 
> This serves two purposes. First, it lets power management firmware
> running on the ARISC coprocessor know which wakeup sources Linux wants
> to have enabled. That way, it can avoid turning them off when it shuts
> down the remainder of the clock tree. Second, it preconfigures the
> coprocessor's interrupt controller, so the firmware's wakeup logic
> is as simple as waiting for an interrupt to arrive.
> 
> The suspend/resume logic is not conditional on PM_SLEEP because it is
> identical to the init/shutdown logic. Wake IRQs may be enabled during
> shutdown to allow powering the board back on. As an example, see
> commit a5c5e50cce9d ("Input: gpio-keys - add shutdown callback").
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>  drivers/irqchip/irq-sun6i-r.c | 107 ++++++++++++++++++++++++++++++++--
>  1 file changed, 101 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-sun6i-r.c b/drivers/irqchip/irq-sun6i-r.c
> index d04d067423f4..a1b58c98d6ca 100644
> --- a/drivers/irqchip/irq-sun6i-r.c
> +++ b/drivers/irqchip/irq-sun6i-r.c
> @@ -39,6 +39,7 @@
>   * set of 128 mux bits. This requires a second set of top-level registers.
>   */
>  
> +#include <linux/bitmap.h>
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
>  #include <linux/irqchip.h>
> @@ -46,6 +47,7 @@
>  #include <linux/of.h>
>  #include <linux/of_address.h>
>  #include <linux/of_irq.h>
> +#include <linux/syscore_ops.h>
>  
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>  
> @@ -67,8 +69,17 @@
>  #define SUN6I_NR_DIRECT_IRQS		16
>  #define SUN6I_NR_MUX_BITS		128
>  
> +struct sun6i_r_intc_variant {
> +	u32		first_mux_irq;
> +	u32		nr_mux_irqs;
> +	u32		mux_valid[BITS_TO_U32(SUN6I_NR_MUX_BITS)];
> +};
> +
>  static void __iomem *base;
>  static irq_hw_number_t nmi_hwirq;
> +static DECLARE_BITMAP(wake_irq_enabled, SUN6I_NR_TOP_LEVEL_IRQS);
> +static DECLARE_BITMAP(wake_mux_enabled, SUN6I_NR_MUX_BITS);
> +static DECLARE_BITMAP(wake_mux_valid, SUN6I_NR_MUX_BITS);
>  
>  static void sun6i_r_intc_ack_nmi(void)
>  {
> @@ -145,6 +156,21 @@ static int sun6i_r_intc_nmi_set_irqchip_state(struct irq_data *data,
>  	return irq_chip_set_parent_state(data, which, state);
>  }
>  
> +static int sun6i_r_intc_irq_set_wake(struct irq_data *data, unsigned int on)
> +{
> +	unsigned long offset_from_nmi = data->hwirq - nmi_hwirq;
> +
> +	if (offset_from_nmi < SUN6I_NR_DIRECT_IRQS)
> +		assign_bit(offset_from_nmi, wake_irq_enabled, on);
> +	else if (test_bit(data->hwirq, wake_mux_valid))
> +		assign_bit(data->hwirq, wake_mux_enabled, on);
> +	else
> +		/* Not wakeup capable. */
> +		return -EPERM;
> +
> +	return 0;
> +}
> +
>  static struct irq_chip sun6i_r_intc_nmi_chip = {
>  	.name			= "sun6i-r-intc",
>  	.irq_ack		= sun6i_r_intc_nmi_ack,
> @@ -154,8 +180,19 @@ static struct irq_chip sun6i_r_intc_nmi_chip = {
>  	.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,
> -	.flags			= IRQCHIP_SET_TYPE_MASKED |
> -				  IRQCHIP_SKIP_SET_WAKE,
> +	.irq_set_wake		= sun6i_r_intc_irq_set_wake,
> +	.flags			= IRQCHIP_SET_TYPE_MASKED,
> +};
> +
> +static struct irq_chip sun6i_r_intc_wakeup_chip = {
> +	.name			= "sun6i-r-intc",
> +	.irq_mask		= irq_chip_mask_parent,
> +	.irq_unmask		= irq_chip_unmask_parent,
> +	.irq_eoi		= irq_chip_eoi_parent,
> +	.irq_set_affinity	= irq_chip_set_affinity_parent,
> +	.irq_set_type		= irq_chip_set_type_parent,
> +	.irq_set_wake		= sun6i_r_intc_irq_set_wake,
> +	.flags			= IRQCHIP_SET_TYPE_MASKED,

Worth implementing irq_get/set_irqchip_state() using the _parent
helper, I guess.

Thanks,

	M.
Samuel Holland Jan. 15, 2021, 4:01 a.m. UTC | #4
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.
>
Samuel Holland Jan. 15, 2021, 4:04 a.m. UTC | #5
On 1/14/21 3:44 PM, Marc Zyngier wrote:
> On Tue, 12 Jan 2021 05:59:44 +0000,
> Samuel Holland <samuel@sholland.org> wrote:
>>
>> Maintain bitmaps of wake-enabled IRQs and mux inputs, and program them
>> to the hardware during the syscore phase of suspend and shutdown. Then
>> restore the original set of enabled IRQs (only the NMI) during resume.
>>
>> This serves two purposes. First, it lets power management firmware
>> running on the ARISC coprocessor know which wakeup sources Linux wants
>> to have enabled. That way, it can avoid turning them off when it shuts
>> down the remainder of the clock tree. Second, it preconfigures the
>> coprocessor's interrupt controller, so the firmware's wakeup logic
>> is as simple as waiting for an interrupt to arrive.
>>
>> The suspend/resume logic is not conditional on PM_SLEEP because it is
>> identical to the init/shutdown logic. Wake IRQs may be enabled during
>> shutdown to allow powering the board back on. As an example, see
>> commit a5c5e50cce9d ("Input: gpio-keys - add shutdown callback").
>>
>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>  drivers/irqchip/irq-sun6i-r.c | 107 ++++++++++++++++++++++++++++++++--
>>  1 file changed, 101 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-sun6i-r.c b/drivers/irqchip/irq-sun6i-r.c
>> index d04d067423f4..a1b58c98d6ca 100644
>> --- a/drivers/irqchip/irq-sun6i-r.c
>> +++ b/drivers/irqchip/irq-sun6i-r.c
>> @@ -39,6 +39,7 @@
>>   * set of 128 mux bits. This requires a second set of top-level registers.
>>   */
>>  
>> +#include <linux/bitmap.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/irq.h>
>>  #include <linux/irqchip.h>
>> @@ -46,6 +47,7 @@
>>  #include <linux/of.h>
>>  #include <linux/of_address.h>
>>  #include <linux/of_irq.h>
>> +#include <linux/syscore_ops.h>
>>  
>>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>>  
>> @@ -67,8 +69,17 @@
>>  #define SUN6I_NR_DIRECT_IRQS		16
>>  #define SUN6I_NR_MUX_BITS		128
>>  
>> +struct sun6i_r_intc_variant {
>> +	u32		first_mux_irq;
>> +	u32		nr_mux_irqs;
>> +	u32		mux_valid[BITS_TO_U32(SUN6I_NR_MUX_BITS)];
>> +};
>> +
>>  static void __iomem *base;
>>  static irq_hw_number_t nmi_hwirq;
>> +static DECLARE_BITMAP(wake_irq_enabled, SUN6I_NR_TOP_LEVEL_IRQS);
>> +static DECLARE_BITMAP(wake_mux_enabled, SUN6I_NR_MUX_BITS);
>> +static DECLARE_BITMAP(wake_mux_valid, SUN6I_NR_MUX_BITS);
>>  
>>  static void sun6i_r_intc_ack_nmi(void)
>>  {
>> @@ -145,6 +156,21 @@ static int sun6i_r_intc_nmi_set_irqchip_state(struct irq_data *data,
>>  	return irq_chip_set_parent_state(data, which, state);
>>  }
>>  
>> +static int sun6i_r_intc_irq_set_wake(struct irq_data *data, unsigned int on)
>> +{
>> +	unsigned long offset_from_nmi = data->hwirq - nmi_hwirq;
>> +
>> +	if (offset_from_nmi < SUN6I_NR_DIRECT_IRQS)
>> +		assign_bit(offset_from_nmi, wake_irq_enabled, on);
>> +	else if (test_bit(data->hwirq, wake_mux_valid))
>> +		assign_bit(data->hwirq, wake_mux_enabled, on);
>> +	else
>> +		/* Not wakeup capable. */
>> +		return -EPERM;
>> +
>> +	return 0;
>> +}
>> +
>>  static struct irq_chip sun6i_r_intc_nmi_chip = {
>>  	.name			= "sun6i-r-intc",
>>  	.irq_ack		= sun6i_r_intc_nmi_ack,
>> @@ -154,8 +180,19 @@ static struct irq_chip sun6i_r_intc_nmi_chip = {
>>  	.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,
>> -	.flags			= IRQCHIP_SET_TYPE_MASKED |
>> -				  IRQCHIP_SKIP_SET_WAKE,
>> +	.irq_set_wake		= sun6i_r_intc_irq_set_wake,
>> +	.flags			= IRQCHIP_SET_TYPE_MASKED,
>> +};
>> +
>> +static struct irq_chip sun6i_r_intc_wakeup_chip = {
>> +	.name			= "sun6i-r-intc",
>> +	.irq_mask		= irq_chip_mask_parent,
>> +	.irq_unmask		= irq_chip_unmask_parent,
>> +	.irq_eoi		= irq_chip_eoi_parent,
>> +	.irq_set_affinity	= irq_chip_set_affinity_parent,
>> +	.irq_set_type		= irq_chip_set_type_parent,
>> +	.irq_set_wake		= sun6i_r_intc_irq_set_wake,
>> +	.flags			= IRQCHIP_SET_TYPE_MASKED,
> 
> Worth implementing irq_get/set_irqchip_state() using the _parent
> helper, I guess.

This is the same situation as the previous patch. Assuming it is safe to
rely on the behavior of the top-level functions, adding the callbacks
here would be redundant.

Cheers,
Samuel

> Thanks,
> 
> 	M.
>
Marc Zyngier Jan. 15, 2021, 9:30 a.m. UTC | #6
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.