From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.0 required=3.0 tests=BAYES_00,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 46BA8C43381 for ; Tue, 9 Mar 2021 10:58:46 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 0E2366523D for ; Tue, 9 Mar 2021 10:58:46 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229553AbhCIK6N convert rfc822-to-8bit (ORCPT ); Tue, 9 Mar 2021 05:58:13 -0500 Received: from mail.kernel.org ([198.145.29.99]:45622 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229546AbhCIK6M (ORCPT ); Tue, 9 Mar 2021 05:58:12 -0500 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 311406522B; Tue, 9 Mar 2021 10:58:12 +0000 (UTC) Received: from 78.163-31-62.static.virginmediabusiness.co.uk ([62.31.163.78] helo=why.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94) (envelope-from ) id 1lJa41-000WL8-VS; Tue, 09 Mar 2021 10:58:10 +0000 Date: Tue, 09 Mar 2021 10:58:09 +0000 Message-ID: <87sg54y4ou.wl-maz@kernel.org> From: Marc Zyngier To: =?UTF-8?B?w4FsdmFybyBGZXJuw6FuZGV6?= Rojas Cc: Thomas Gleixner , Rob Herring , Florian Fainelli , Jonas Gorski , linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, bcm-kernel-feedback-list@broadcom.com, linux-mips@vger.kernel.org Subject: Re: [PATCH v2 2/2] irqchip: add support for BCM6345 external interrupt controller In-Reply-To: <20210224075640.20465-3-noltari@gmail.com> References: <20210223204340.312-1-noltari@gmail.com> <20210224075640.20465-1-noltari@gmail.com> <20210224075640.20465-3-noltari@gmail.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT X-SA-Exim-Connect-IP: 62.31.163.78 X-SA-Exim-Rcpt-To: noltari@gmail.com, tglx@linutronix.de, robh+dt@kernel.org, f.fainelli@gmail.com, jonas.gorski@gmail.com, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, bcm-kernel-feedback-list@broadcom.com, linux-mips@vger.kernel.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org On Wed, 24 Feb 2021 07:56:40 +0000, Álvaro Fernández Rojas wrote: > > This interrupt controller is present on bcm63xx SoCs in order to generate > interrupts based on GPIO status changes. > > Signed-off-by: Álvaro Fernández Rojas > Signed-off-by: Jonas Gorski > Reviewed-by: Florian Fainelli > --- > v3: no changes. > v2: no changes. > > drivers/irqchip/Kconfig | 4 + > drivers/irqchip/Makefile | 1 + > drivers/irqchip/irq-bcm6345-ext.c | 271 ++++++++++++++++++++++++++++++ > 3 files changed, 276 insertions(+) > create mode 100644 drivers/irqchip/irq-bcm6345-ext.c > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > index e74fa206240a..eaa101939a34 100644 > --- a/drivers/irqchip/Kconfig > +++ b/drivers/irqchip/Kconfig > @@ -113,6 +113,10 @@ config I8259 > bool > select IRQ_DOMAIN > > +config BCM6345_EXT_IRQ > + bool "BCM6345 External IRQ Controller" > + select IRQ_DOMAIN > + > config BCM6345_L1_IRQ > bool > select GENERIC_IRQ_CHIP > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > index c59b95a0532c..3cba65bc0aa5 100644 > --- a/drivers/irqchip/Makefile > +++ b/drivers/irqchip/Makefile > @@ -62,6 +62,7 @@ obj-$(CONFIG_XTENSA_MX) += irq-xtensa-mx.o > obj-$(CONFIG_XILINX_INTC) += irq-xilinx-intc.o > obj-$(CONFIG_IRQ_CROSSBAR) += irq-crossbar.o > obj-$(CONFIG_SOC_VF610) += irq-vf610-mscm-ir.o > +obj-$(CONFIG_BCM6345_EXT_IRQ) += irq-bcm6345-ext.o > obj-$(CONFIG_BCM6345_L1_IRQ) += irq-bcm6345-l1.o > obj-$(CONFIG_BCM7038_L1_IRQ) += irq-bcm7038-l1.o > obj-$(CONFIG_BCM7120_L2_IRQ) += irq-bcm7120-l2.o > diff --git a/drivers/irqchip/irq-bcm6345-ext.c b/drivers/irqchip/irq-bcm6345-ext.c > new file mode 100644 > index 000000000000..5721ac8de295 > --- /dev/null > +++ b/drivers/irqchip/irq-bcm6345-ext.c > @@ -0,0 +1,271 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Broadcom BCM6345 style external interrupt controller driver > + * > + * Copyright (C) 2021 Álvaro Fernández Rojas > + * Copyright (C) 2014 Jonas Gorski > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define MAX_IRQS 4 > + > +#define EXTIRQ_CFG_SENSE 0 > +#define EXTIRQ_CFG_STAT 1 > +#define EXTIRQ_CFG_CLEAR 2 > +#define EXTIRQ_CFG_MASK 3 > +#define EXTIRQ_CFG_BOTHEDGE 4 > +#define EXTIRQ_CFG_LEVELSENSE 5 > + > +struct intc_data { > + struct irq_chip chip; > + struct irq_domain *domain; > + raw_spinlock_t lock; > + > + int parent_irq[MAX_IRQS]; > + void __iomem *reg; > + int shift; > + unsigned int toggle_clear_on_ack:1; Please use the bool type. > +}; > + > +static void bcm6345_ext_intc_irq_handle(struct irq_desc *desc) > +{ > + struct intc_data *data = irq_desc_get_handler_data(desc); > + struct irq_chip *chip = irq_desc_get_chip(desc); > + unsigned int irq = irq_desc_get_irq(desc); > + unsigned int idx; > + > + chained_irq_enter(chip, desc); > + > + for (idx = 0; idx < MAX_IRQS; idx++) { > + if (data->parent_irq[idx] != irq) > + continue; > + > + generic_handle_irq(irq_find_mapping(data->domain, idx)); One parent IRQ per input? Why isn't this a hierarchical interrupt controller? Even *if* this really has to be a chained interrupt controller, I'm sure there are better ways to identify the input then this loop (offset from a base, for example). > + } > + > + chained_irq_exit(chip, desc); > +} > + > +static void bcm6345_ext_intc_irq_ack(struct irq_data *data) > +{ > + struct intc_data *priv = data->domain->host_data; > + irq_hw_number_t hwirq = irqd_to_hwirq(data); > + u32 reg; > + > + raw_spin_lock(&priv->lock); > + reg = __raw_readl(priv->reg); > + __raw_writel(reg | (1 << (hwirq + EXTIRQ_CFG_CLEAR * priv->shift)), > + priv->reg); > + if (priv->toggle_clear_on_ack) Under what condition do you need this? > + __raw_writel(reg, priv->reg); > + raw_spin_unlock(&priv->lock); > +} > + > +static void bcm6345_ext_intc_irq_mask(struct irq_data *data) > +{ > + struct intc_data *priv = data->domain->host_data; > + irq_hw_number_t hwirq = irqd_to_hwirq(data); > + u32 reg; > + > + raw_spin_lock(&priv->lock); > + reg = __raw_readl(priv->reg); > + reg &= ~(1 << (hwirq + EXTIRQ_CFG_MASK * priv->shift)); > + __raw_writel(reg, priv->reg); > + raw_spin_unlock(&priv->lock); > +} > + > +static void bcm6345_ext_intc_irq_unmask(struct irq_data *data) > +{ > + struct intc_data *priv = data->domain->host_data; > + irq_hw_number_t hwirq = irqd_to_hwirq(data); > + u32 reg; > + > + raw_spin_lock(&priv->lock); > + reg = __raw_readl(priv->reg); > + reg |= 1 << (hwirq + EXTIRQ_CFG_MASK * priv->shift); > + __raw_writel(reg, priv->reg); > + raw_spin_unlock(&priv->lock); > +} > + > +static int bcm6345_ext_intc_set_type(struct irq_data *data, > + unsigned int flow_type) > +{ > + struct intc_data *priv = data->domain->host_data; > + irq_hw_number_t hwirq = irqd_to_hwirq(data); > + bool levelsense = 0, sense = 0, bothedge = 0; > + u32 reg; > + > + flow_type &= IRQ_TYPE_SENSE_MASK; > + > + if (flow_type == IRQ_TYPE_NONE) You should never get NONE. If you have that value here, that's probably a bug somewhere else. > + flow_type = IRQ_TYPE_LEVEL_LOW; > + > + switch (flow_type) { > + case IRQ_TYPE_EDGE_BOTH: > + bothedge = 1; > + break; > + > + case IRQ_TYPE_EDGE_RISING: > + sense = 1; > + break; > + > + case IRQ_TYPE_EDGE_FALLING: > + break; > + > + case IRQ_TYPE_LEVEL_HIGH: > + levelsense = 1; > + sense = 1; > + break; > + > + case IRQ_TYPE_LEVEL_LOW: > + levelsense = 1; > + break; > + > + default: > + pr_err("bogus flow type combination given!\n"); > + return -EINVAL; How can this happen? > + } > + > + raw_spin_lock(&priv->lock); > + reg = __raw_readl(priv->reg); > + > + if (levelsense) > + reg |= 1 << (hwirq + EXTIRQ_CFG_LEVELSENSE * priv->shift); > + else > + reg &= ~(1 << (hwirq + EXTIRQ_CFG_LEVELSENSE * priv->shift)); > + if (sense) > + reg |= 1 << (hwirq + EXTIRQ_CFG_SENSE * priv->shift); > + else > + reg &= ~(1 << (hwirq + EXTIRQ_CFG_SENSE * priv->shift)); > + if (bothedge) > + reg |= 1 << (hwirq + EXTIRQ_CFG_BOTHEDGE * priv->shift); > + else > + reg &= ~(1 << (hwirq + EXTIRQ_CFG_BOTHEDGE * priv->shift)); So all these levelsense, sense and bothedge variables are already contained in flow_type. Why the need to reinvent the wheel? > + > + __raw_writel(reg, priv->reg); > + raw_spin_unlock(&priv->lock); > + > + irqd_set_trigger_type(data, flow_type); > + if (flow_type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH)) > + irq_set_handler_locked(data, handle_level_irq); > + else > + irq_set_handler_locked(data, handle_edge_irq); > + > + return 0; > +} > + > +static int bcm6345_ext_intc_map(struct irq_domain *d, unsigned int irq, > + irq_hw_number_t hw) > +{ > + struct intc_data *priv = d->host_data; > + > + irq_set_chip_and_handler(irq, &priv->chip, handle_level_irq); > + > + return 0; > +} > + > +static const struct irq_domain_ops bcm6345_ext_domain_ops = { > + .xlate = irq_domain_xlate_twocell, > + .map = bcm6345_ext_intc_map, > +}; > + > +static int __init bcm6345_ext_intc_init(struct device_node *node, > + int num_irqs, int *irqs, > + void __iomem *reg, int shift, > + bool toggle_clear_on_ack) > +{ > + struct intc_data *data; > + unsigned int i; > + > + data = kzalloc(sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + raw_spin_lock_init(&data->lock); > + > + for (i = 0; i < num_irqs; i++) { > + data->parent_irq[i] = irqs[i]; > + > + irq_set_handler_data(irqs[i], data); > + irq_set_chained_handler(irqs[i], bcm6345_ext_intc_irq_handle); > + } > + > + data->reg = reg; > + data->shift = shift; > + data->toggle_clear_on_ack = toggle_clear_on_ack; > + > + data->chip.name = "bcm6345-ext-intc"; > + data->chip.irq_ack = bcm6345_ext_intc_irq_ack; > + data->chip.irq_mask = bcm6345_ext_intc_irq_mask; > + data->chip.irq_unmask = bcm6345_ext_intc_irq_unmask; > + data->chip.irq_set_type = bcm6345_ext_intc_set_type; > + > + data->domain = irq_domain_add_simple(node, num_irqs, 0, > + &bcm6345_ext_domain_ops, data); Consider using irq_domain_add_linear(), given that you don't seem to care about the first interrupt number. > + if (!data->domain) { > + kfree(data); > + return -ENOMEM; > + } At this point, you have registered a bunch of chained handlers with data structures that you have freed. What could possibly go wrong? > + > + return 0; > +} > + > +static int __init bcm6345_ext_intc_of_init(struct device_node *node, > + struct device_node *parent) > +{ > + int num_irqs, ret = -EINVAL; > + unsigned i; > + void __iomem *base; > + int irqs[MAX_IRQS] = { 0 }; > + u32 shift; > + bool toggle_clear_on_ack = false; > + > + num_irqs = of_irq_count(node); > + > + if (!num_irqs || num_irqs > MAX_IRQS) > + return -EINVAL; > + > + if (of_property_read_u32(node, "brcm,field-width", &shift)) > + shift = 4; Why this default? Given that it is a new driver without any backward compatibility requirement, either make the property mandatory or get rid of it. > + > + /* On BCM6318 setting CLEAR seems to continuously mask interrupts */ > + if (of_device_is_compatible(node, "brcm,bcm6318-ext-intc")) > + toggle_clear_on_ack = true; Seems? What does the documentation says about this? > + > + for (i = 0; i < num_irqs; i++) { > + irqs[i] = irq_of_parse_and_map(node, i); > + if (!irqs[i]) > + return -ENOMEM; > + } > + > + base = of_iomap(node, 0); > + if (!base) > + return -ENXIO; > + > + ret = bcm6345_ext_intc_init(node, num_irqs, irqs, base, shift, > + toggle_clear_on_ack); > + if (!ret) > + return 0; > + > + iounmap(base); > + > + for (i = 0; i < num_irqs; i++) > + irq_dispose_mapping(irqs[i]); > + > + return ret; > +} > + > +IRQCHIP_DECLARE(bcm6318_ext_intc, "brcm,bcm6318-ext-intc", > + bcm6345_ext_intc_of_init); > +IRQCHIP_DECLARE(bcm6345_ext_intc, "brcm,bcm6345-ext-intc", > + bcm6345_ext_intc_of_init); > -- > 2.20.1 > > Thanks, M. -- Without deviation from the norm, progress is not possible.