Re: [PATCH v3 2/3] irqchip: dw-apb-ictl: support hierarchy irq domain

[Marc Zyngier]

On Wed, 09 Sep 2020 07:58:35 +0100,
Zhen Lei <thunder.leizhen@xxxxxxxxxx> wrote:
> 
> Add support to use dw-apb-ictl as primary interrupt controller.
> 
> Suggested-by: Marc Zyngier <maz@xxxxxxxxxx>
> Signed-off-by: Zhen Lei <thunder.leizhen@xxxxxxxxxx>
> Tested-by: Haoyu Lv <lvhaoyu@xxxxxxxxxx>
> ---
>  drivers/irqchip/Kconfig           |  2 +-
>  drivers/irqchip/irq-dw-apb-ictl.c | 76 +++++++++++++++++++++++++++----
>  2 files changed, 69 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index bfc9719dbcdc..7c2d1c8fa551 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -148,7 +148,7 @@ config DAVINCI_CP_INTC
>  config DW_APB_ICTL
>  	bool
>  	select GENERIC_IRQ_CHIP
> -	select IRQ_DOMAIN
> +	select IRQ_DOMAIN_HIERARCHY
>  
>  config FARADAY_FTINTC010
>  	bool
> diff --git a/drivers/irqchip/irq-dw-apb-ictl.c b/drivers/irqchip/irq-dw-apb-ictl.c
> index 5458004242e9..3c7bebe1b947 100644
> --- a/drivers/irqchip/irq-dw-apb-ictl.c
> +++ b/drivers/irqchip/irq-dw-apb-ictl.c
> @@ -17,6 +17,7 @@
>  #include <linux/irqchip/chained_irq.h>
>  #include <linux/of_address.h>
>  #include <linux/of_irq.h>
> +#include <linux/interrupt.h>
>  
>  #define APB_INT_ENABLE_L	0x00
>  #define APB_INT_ENABLE_H	0x04
> @@ -26,6 +27,27 @@
>  #define APB_INT_FINALSTATUS_H	0x34
>  #define APB_INT_BASE_OFFSET	0x04
>  
> +/* irq domain of the primary interrupt controller. */
> +static struct irq_domain *dw_apb_ictl_irq_domain;
> +
> +static void __irq_entry dw_apb_ictl_handle_irq(struct pt_regs *regs)
> +{
> +	struct irq_domain *d = dw_apb_ictl_irq_domain;
> +	int n;
> +
> +	for (n = 0; n < d->revmap_size; n += 32) {
> +		struct irq_chip_generic *gc = irq_get_domain_generic_chip(d, n);
> +		u32 stat = readl_relaxed(gc->reg_base + APB_INT_FINALSTATUS_L);
> +
> +		while (stat) {
> +			u32 hwirq = ffs(stat) - 1;
> +
> +			handle_domain_irq(d, hwirq, regs);
> +			stat &= ~BIT(hwirq);
> +		}
> +	}
> +}
> +
>  static void dw_apb_ictl_handle_irq_cascaded(struct irq_desc *desc)
>  {
>  	struct irq_domain *d = irq_desc_get_handler_data(desc);
> @@ -50,6 +72,30 @@ static void dw_apb_ictl_handle_irq_cascaded(struct irq_desc *desc)
>  	chained_irq_exit(chip, desc);
>  }
>  
> +static int dw_apb_ictl_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> +				unsigned int nr_irqs, void *arg)
> +{
> +	int i, ret;
> +	irq_hw_number_t hwirq;
> +	unsigned int type = IRQ_TYPE_NONE;
> +	struct irq_fwspec *fwspec = arg;
> +
> +	ret = irq_domain_translate_onecell(domain, fwspec, &hwirq, &type);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < nr_irqs; i++)
> +		irq_map_generic_chip(domain, virq + i, hwirq + i);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops dw_apb_ictl_irq_domain_ops = {
> +	.translate = irq_domain_translate_onecell,
> +	.alloc = dw_apb_ictl_irq_domain_alloc,
> +	.free = irq_domain_free_irqs_top,
> +};
> +
>  #ifdef CONFIG_PM
>  static void dw_apb_ictl_resume(struct irq_data *d)
>  {
> @@ -75,13 +121,20 @@ static int __init dw_apb_ictl_init(struct device_node *np,
>  	void __iomem *iobase;
>  	int ret, nrirqs, parent_irq, i;
>  	u32 reg;
> -	const struct irq_domain_ops *domain_ops = &irq_generic_chip_ops;
> -
> -	/* Map the parent interrupt for the chained handler */
> -	parent_irq = irq_of_parse_and_map(np, 0);
> -	if (parent_irq <= 0) {
> -		pr_err("%pOF: unable to parse irq\n", np);
> -		return -EINVAL;
> +	const struct irq_domain_ops *domain_ops;
> +
> +	if (!parent || (np == parent)) {
> +		/* It's used as the primary interrupt controller */
> +		parent_irq = 0;
> +		domain_ops = &dw_apb_ictl_irq_domain_ops;
> +	} else {
> +		/* Map the parent interrupt for the chained handler */
> +		parent_irq = irq_of_parse_and_map(np, 0);
> +		if (parent_irq <= 0) {
> +			pr_err("%pOF: unable to parse irq\n", np);
> +			return -EINVAL;
> +		}
> +		domain_ops = &irq_generic_chip_ops;
>  	}
>  
>  	ret = of_address_to_resource(np, 0, &r);
> @@ -144,10 +197,17 @@ static int __init dw_apb_ictl_init(struct device_node *np,
>  		gc->chip_types[0].chip.irq_mask = irq_gc_mask_set_bit;
>  		gc->chip_types[0].chip.irq_unmask = irq_gc_mask_clr_bit;
>  		gc->chip_types[0].chip.irq_resume = dw_apb_ictl_resume;
> +		if (!parent_irq)
> +			gc->chip_types[0].chip.irq_eoi = irq_gc_noop;

Again: what is that for? The level flow doesn't use any EOI callback.

>  	}
>  
> -	irq_set_chained_handler_and_data(parent_irq,
> +	if (parent_irq) {
> +		irq_set_chained_handler_and_data(parent_irq,
>  				dw_apb_ictl_handle_irq_cascaded, domain);
> +	} else {
> +		dw_apb_ictl_irq_domain = domain;
> +		set_handle_irq(dw_apb_ictl_handle_irq);

This only exists on architectures that select GENERIC_IRQ_MULTI_HANDLER,
and yet this driver is used on some arc system (AXS10x), which doesn't
have this option selected.

Please make sure this at least builds on all supported architectures.

	M.

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