From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754763AbeEHHjS (ORCPT ); Tue, 8 May 2018 03:39:18 -0400 Received: from foss.arm.com ([217.140.101.70]:53656 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754737AbeEHHjQ (ORCPT ); Tue, 8 May 2018 03:39:16 -0400 Subject: Re: [PATCH] irqchip/irq-ath79-intc: add irq cascade driver for QCA9556 SoCs To: John Crispin , Thomas Gleixner , Jason Cooper Cc: linux-kernel@vger.kernel.org, linux-mips@linux-mips.org References: <20180507133714.17384-1-john@phrozen.org> From: Marc Zyngier Organization: ARM Ltd Message-ID: 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 List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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 > + * > + * 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 > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > + > +#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...