Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] irqchip/irq-ath79-intc: add irq cascade driver for
 QCA9556 SoCs
To:     John Crispin <john@phrozen.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Jason Cooper <jason@lakedaemon.net>
Cc:     linux-kernel@vger.kernel.org, linux-mips@linux-mips.org
References: <20180507133714.17384-1-john@phrozen.org>
From:   Marc Zyngier <marc.zyngier@arm.com>
Organization: ARM Ltd
Message-ID: <ec4da5fd-0ade-e6bb-e779-d70d56c0da55@arm.com>
Date:   Tue, 8 May 2018 08:39:13 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.7.0
MIME-Version: 1.0
In-Reply-To: <20180507133714.17384-1-john@phrozen.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-GB
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi John,

On 07/05/18 14:37, John Crispin wrote:
> The QCA ATH79 MIPS target is being converted to pure OF. Right now the
> platform code will setup the IRQ cascade found on the QCA9556 and newer
> SoCs and uses fixed IRQ numbers for the peripherals attached to the
> cascade. This patch adds a proper driver based on the code previously
> located inside arch/mips/ath79/irq.c.
> 
> Signed-off-by: John Crispin <john@phrozen.org>
> ---
>  drivers/irqchip/Makefile         |   1 +
>  drivers/irqchip/irq-ath79-intc.c | 108 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 109 insertions(+)
>  create mode 100644 drivers/irqchip/irq-ath79-intc.c
> 
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index d27e3e3619e0..f63c94a92e25 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -3,6 +3,7 @@ obj-$(CONFIG_IRQCHIP)			+= irqchip.o
>  
>  obj-$(CONFIG_ALPINE_MSI)		+= irq-alpine-msi.o
>  obj-$(CONFIG_ATH79)			+= irq-ath79-cpu.o
> +obj-$(CONFIG_ATH79)			+= irq-ath79-intc.o
>  obj-$(CONFIG_ATH79)			+= irq-ath79-misc.o
>  obj-$(CONFIG_ARCH_BCM2835)		+= irq-bcm2835.o
>  obj-$(CONFIG_ARCH_BCM2835)		+= irq-bcm2836.o
> diff --git a/drivers/irqchip/irq-ath79-intc.c b/drivers/irqchip/irq-ath79-intc.c
> new file mode 100644
> index 000000000000..ba15b1ac98b3
> --- /dev/null
> +++ b/drivers/irqchip/irq-ath79-intc.c
> @@ -0,0 +1,108 @@
> +/*
> + *  Atheros QCA955X specific interrupt cascade handling
> + *
> + *  Copyright (C) 2018 John Crispin <john@phrozen.org>
> + *
> + *  This program is free software; you can redistribute it and/or modify it
> + *  under the terms of the GNU General Public License version 2 as published
> + *  by the Free Software Foundation.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/irqchip.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/irqdomain.h>
> +
> +#include <asm/irq_cpu.h>
> +#include <asm/mach-ath79/ath79.h>
> +#include <asm/mach-ath79/ar71xx_regs.h>
> +
> +#define ATH79_MAX_INTC_CASCADE	3

Why 3? Is that a property of the HW? Or could it be inferred from the DT?

> +
> +struct ath79_intc {
> +	struct irq_chip chip;
> +	u32 irq;
> +	u32 pending_mask;
> +	u32 irq_mask[ATH79_MAX_INTC_CASCADE];
> +};
> +
> +static void ath79_intc_irq_handler(struct irq_desc *desc)
> +{
> +	struct irq_domain *domain = irq_desc_get_handler_data(desc);
> +	struct ath79_intc *intc = domain->host_data;
> +	u32 pending;
> +
> +	pending = ath79_reset_rr(QCA955X_RESET_REG_EXT_INT_STATUS);
> +	pending &= intc->pending_mask;

Isn't this "pending_mask" more of an "enabled"?

> +
> +	if (pending) {
> +		int i;
> +
> +		for (i = 0; i < domain->hwirq_max; i++)

Don't. This is an implementation detail of the irq domain, and you're
not supposed to access that field.

> +			if (pending & intc->irq_mask[i])

What are you trying to do here? Can't you directly infer the pending
interrupt from the pending field?

> +				generic_handle_irq(irq_find_mapping(domain, i));
> +	} else {
> +		spurious_interrupt();
> +	}

Missing chained_irq_enter/exit calls.

> +}
> +
> +static void ath79_intc_irq_unmask(struct irq_data *d)
> +{
> +}
> +
> +static void ath79_intc_irq_mask(struct irq_data *d)
> +{
> +}

So you cannot mask or unmask an interrupt? What is this thing? An OR gate?

> +
> +static int ath79_intc_map(struct irq_domain *d, unsigned int irq,
> +			  irq_hw_number_t hw)
> +{
> +	struct ath79_intc *intc = d->host_data;
> +
> +	irq_set_chip_and_handler(irq, &intc->chip, handle_level_irq);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops ath79_irq_domain_ops = {
> +	.xlate = irq_domain_xlate_onecell,
> +	.map = ath79_intc_map,
> +};
> +
> +static int __init qca9556_intc_of_init(
> +	struct device_node *node, struct device_node *parent)
> +{
> +	struct irq_domain *domain;
> +	struct ath79_intc *intc;
> +	int cnt, i;
> +
> +	cnt = of_property_count_u32_elems(node, "qcom,pending-bits");

Where is this binding documented? What does "pending_bits" even mean if
it is statically defined?

> +	if (cnt > ATH79_MAX_INTC_CASCADE)
> +		panic("Too many INTC pending bits\n");
> +
> +	intc = kzalloc(sizeof(*intc), GFP_KERNEL);
> +	if (!intc)
> +		panic("Failed to allocate INTC memory\n");
> +	intc->chip.name = "INTC";
> +	intc->chip.irq_unmask = ath79_intc_irq_unmask,
> +	intc->chip.irq_mask = ath79_intc_irq_mask,
> +
> +	of_property_read_u32_array(node, "qcom,pending-bits", intc->irq_mask,
> +				   cnt);
> +	for (i = 0; i < cnt; i++)
> +		intc->pending_mask |= intc->irq_mask[i];
> +
> +	intc->irq = irq_of_parse_and_map(node, 0);
> +	if (!intc->irq)
> +		panic("Failed to get INTC IRQ");

Do you really need the panics in this function? That seem a bit of a
harsh treatment for something that is not necessarily fatal.

> +
> +	domain = irq_domain_add_linear(node, cnt, &ath79_irq_domain_ops,
> +				       intc);
> +	irq_set_chained_handler_and_data(intc->irq, ath79_intc_irq_handler,
> +					 domain);
> +
> +	return 0;
> +}
> +IRQCHIP_DECLARE(qca9556_intc, "qcom,qca9556-intc",
> +		qca9556_intc_of_init);
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...