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=-6.9 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED autolearn=ham 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 1C8DFC43387 for ; Sat, 15 Dec 2018 09:45:21 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id C19EC2084D for ; Sat, 15 Dec 2018 09:45:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729908AbeLOJpT (ORCPT ); Sat, 15 Dec 2018 04:45:19 -0500 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:33950 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728731AbeLOJpT (ORCPT ); Sat, 15 Dec 2018 04:45:19 -0500 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 9169DEBD; Sat, 15 Dec 2018 01:45:18 -0800 (PST) Received: from big-swifty.misterjones.org (usa-sjc-mx-foss1.foss.arm.com [217.140.101.70]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 7431B3F575; Sat, 15 Dec 2018 01:45:10 -0800 (PST) Date: Sat, 15 Dec 2018 09:45:02 +0000 Message-ID: <86r2ejaztt.wl-marc.zyngier@arm.com> From: Marc Zyngier To: Lucas Stach Cc: Thomas Gleixner , Jason Cooper , Rob Herring , Mark Rutland , , , , Subject: Re: [PATCH v2 2/2] irqchip: add driver for imx-irqsteer controller In-Reply-To: <20181214102244.21509-3-l.stach@pengutronix.de> References: <20181214102244.21509-1-l.stach@pengutronix.de> <20181214102244.21509-3-l.stach@pengutronix.de> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 EasyPG/1.0.0 Emacs/25.1 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) Organization: ARM Ltd MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Lucas, On Fri, 14 Dec 2018 10:22:44 +0000, Lucas Stach wrote: > > The irqsteer block is a interrupt multiplexer/remapper found on the > i.MX8 line of SoCs. > > Signed-off-by: Fugang Duan > Signed-off-by: Lucas Stach > --- > v2: > - use raw_spinlock > - add proper clk error handling > - align register offset calculation with reality > - use correct SPDX identifier > - simplify DT configuration > --- > drivers/irqchip/Kconfig | 8 + > drivers/irqchip/Makefile | 1 + > drivers/irqchip/irq-imx-irqsteer.c | 269 +++++++++++++++++++++++++++++ > 3 files changed, 278 insertions(+) > create mode 100644 drivers/irqchip/irq-imx-irqsteer.c > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > index 51a5ef0e96ed..25607a002bf1 100644 > --- a/drivers/irqchip/Kconfig > +++ b/drivers/irqchip/Kconfig > @@ -391,6 +391,14 @@ config CSKY_APB_INTC > by C-SKY single core SOC system. It use mmio map apb-bus to visit > the controller's register. > > +config IMX_IRQSTEER > + bool "i.MX IRQSTEER support" > + depends on ARCH_MXC || COMPILE_TEST > + default ARCH_MXC > + select IRQ_DOMAIN > + help > + Support for the i.MX IRQSTEER interrupt multiplexer/remapper. > + > endmenu > > config SIFIVE_PLIC > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > index 794c13d3ac3d..8ef99b3f7e4e 100644 > --- a/drivers/irqchip/Makefile > +++ b/drivers/irqchip/Makefile > @@ -91,3 +91,4 @@ obj-$(CONFIG_QCOM_PDC) += qcom-pdc.o > obj-$(CONFIG_CSKY_MPINTC) += irq-csky-mpintc.o > obj-$(CONFIG_CSKY_APB_INTC) += irq-csky-apb-intc.o > obj-$(CONFIG_SIFIVE_PLIC) += irq-sifive-plic.o > +obj-$(CONFIG_IMX_IRQSTEER) += irq-imx-irqsteer.o > diff --git a/drivers/irqchip/irq-imx-irqsteer.c b/drivers/irqchip/irq-imx-irqsteer.c > new file mode 100644 > index 000000000000..099148045657 > --- /dev/null > +++ b/drivers/irqchip/irq-imx-irqsteer.c > @@ -0,0 +1,269 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright 2017 NXP > + * Copyright (C) 2018 Pengutronix, Lucas Stach > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define CTRL_STRIDE_OFF(_t, _r) (_t * 8 * _r) > +#define CHANCTRL 0x0 > +#define CHANMASK(n, t) (CTRL_STRIDE_OFF(t, 0) + 0x4 * (n) + 0x4) > +#define CHANSET(n, t) (CTRL_STRIDE_OFF(t, 1) + 0x4 * (n) + 0x4) > +#define CHANSTATUS(n, t) (CTRL_STRIDE_OFF(t, 2) + 0x4 * (n) + 0x4) > +#define CHAN_MINTDIS(t) (CTRL_STRIDE_OFF(t, 3) + 0x4) > +#define CHAN_MASTRSTAT(t) (CTRL_STRIDE_OFF(t, 3) + 0x8) > + > +struct irqsteer_data { > + void __iomem *regs; > + struct clk *ipg_clk; > + int irq; > + raw_spinlock_t lock; > + int irq_groups; > + int channel; > + struct irq_domain *domain; > + u32 *saved_reg; > +}; > + > +static int imx_irqsteer_get_reg_index(struct irqsteer_data *data, > + unsigned long irqnum) > +{ > + return (data->irq_groups * 2 - irqnum / 32 - 1); > +} > + > +static void imx_irqsteer_irq_unmask(struct irq_data *d) > +{ > + struct irqsteer_data *data = d->chip_data; > + int idx = imx_irqsteer_get_reg_index(data, d->hwirq); > + unsigned long flags; > + u32 val; > + > + raw_spin_lock_irqsave(&data->lock, flags); > + val = readl_relaxed(data->regs + CHANMASK(idx, data->irq_groups)); > + val |= BIT(d->hwirq % 32); > + writel_relaxed(val, data->regs + CHANMASK(idx, data->irq_groups)); > + raw_spin_unlock_irqrestore(&data->lock, flags); > +} > + > +static void imx_irqsteer_irq_mask(struct irq_data *d) > +{ > + struct irqsteer_data *data = d->chip_data; > + int idx = imx_irqsteer_get_reg_index(data, d->hwirq); > + unsigned long flags; > + u32 val; > + > + raw_spin_lock_irqsave(&data->lock, flags); > + val = readl_relaxed(data->regs + CHANMASK(idx, data->irq_groups)); > + val &= ~BIT(d->hwirq % 32); > + writel_relaxed(val, data->regs + CHANMASK(idx, data->irq_groups)); > + raw_spin_unlock_irqrestore(&data->lock, flags); > +} > + > +static int imx_irqsteer_set_type(struct irq_data *data, unsigned int flow_type) > +{ > + if ((flow_type & IRQ_TYPE_SENSE_MASK) == IRQ_TYPE_LEVEL_HIGH) > + return 0; > + > + return -EINVAL; > +} > + > +static struct irq_chip imx_irqsteer_irq_chip = { > + .name = "irqsteer", > + .irq_mask = imx_irqsteer_irq_mask, > + .irq_unmask = imx_irqsteer_irq_unmask, > + .irq_set_type = imx_irqsteer_set_type, > +}; > + > +static int imx_irqsteer_irq_map(struct irq_domain *h, unsigned int irq, > + irq_hw_number_t hwirq) > +{ > + irq_set_chip_data(irq, h->host_data); > + irq_set_chip_and_handler(irq, &imx_irqsteer_irq_chip, handle_level_irq); > + > + return 0; > +} > + > +static const struct irq_domain_ops imx_irqsteer_domain_ops = { > + .map = imx_irqsteer_irq_map, > + .xlate = irq_domain_xlate_twocell, I've tried to understand why you are using two cells, but couldn't find an explanation from the DT binding. As far as I can see, you only support LEVEL_HIGH signalling, meaning that you could perfectly encode this in a single cell. What am I missing? > +}; > + > +static void imx_irqsteer_irq_handler(struct irq_desc *desc) > +{ > + struct irqsteer_data *data = irq_desc_get_handler_data(desc); > + int i; > + > + chained_irq_enter(irq_desc_get_chip(desc), desc); > + > + for (i = 0; i < data->irq_groups * 64; i += 32) { > + int idx = imx_irqsteer_get_reg_index(data, i); > + unsigned long irqmap; > + int pos, virq; > + > + irqmap = readl_relaxed(data->regs + > + CHANSTATUS(idx, data->irq_groups)); > + > + for_each_set_bit(pos, &irqmap, 32) { > + virq = irq_find_mapping(data->domain, pos + i); > + if (virq) > + generic_handle_irq(virq); > + } > + } > + > + chained_irq_exit(irq_desc_get_chip(desc), desc); > +} > + > +static int imx_irqsteer_probe(struct platform_device *pdev) > +{ > + struct device_node *np = pdev->dev.of_node; > + struct irqsteer_data *data; > + struct resource *res; > + int ret; > + > + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + data->regs = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(data->regs)) { > + dev_err(&pdev->dev, "failed to initialize reg\n"); > + return PTR_ERR(data->regs); > + } > + > + data->irq = platform_get_irq(pdev, 0); > + if (data->irq <= 0) { > + dev_err(&pdev->dev, "failed to get irq\n"); > + return -ENODEV; > + } > + > + data->ipg_clk = devm_clk_get(&pdev->dev, "ipg"); > + if (IS_ERR(data->ipg_clk)) { > + ret = PTR_ERR(data->ipg_clk); > + if (ret != -EPROBE_DEFER) > + dev_err(&pdev->dev, "failed to get ipg clk: %d\n", ret); > + return ret; > + } > + > + raw_spin_lock_init(&data->lock); > + > + of_property_read_u32(np, "fsl,irq-groups", &data->irq_groups); > + of_property_read_u32(np, "fsl,channel", &data->channel); > + > + if (IS_ENABLED(CONFIG_PM_SLEEP)) { > + data->saved_reg = devm_kzalloc(&pdev->dev, > + sizeof(u32) * data->irq_groups * 2, > + GFP_KERNEL); > + if (!data->saved_reg) > + return -ENOMEM; > + } > + > + ret = clk_prepare_enable(data->ipg_clk); > + if (ret) { > + dev_err(&pdev->dev, "failed to enable ipg clk: %d\n", ret); > + return ret; > + } > + > + /* steer all IRQs into configured channel */ > + writel_relaxed(BIT(data->channel), data->regs + CHANCTRL); Could you explain what this channel is exactly? > + > + data->domain = irq_domain_add_linear(np, data->irq_groups * 64, > + &imx_irqsteer_domain_ops, data); > + if (!data->domain) { > + dev_err(&pdev->dev, "failed to create IRQ domain\n"); > + clk_disable_unprepare(data->ipg_clk); > + return -ENOMEM; > + } > + > + irq_set_chained_handler_and_data(data->irq, imx_irqsteer_irq_handler, > + data); > + > + platform_set_drvdata(pdev, data); > + > + return 0; > +} > + > +static int imx_irqsteer_remove(struct platform_device *pdev) > +{ > + struct irqsteer_data *irqsteer_data = platform_get_drvdata(pdev); > + > + irq_set_chained_handler_and_data(irqsteer_data->irq, NULL, NULL); > + irq_domain_remove(irqsteer_data->domain); > + > + clk_disable_unprepare(irqsteer_data->ipg_clk); > + > + return 0; > +} > + > +#ifdef CONFIG_PM_SLEEP > +static void imx_irqsteer_save_regs(struct irqsteer_data *data) > +{ > + int num; > + > + for (num = 0; num < data->irq_groups; num++) > + data->saved_reg[num + 1] = readl_relaxed(data->regs + > + CHANMASK(num, data->irq_groups)); You seem to only use one u32 per IRQ group. Yet, you've allocated twice as much memory to that effect. Why? Also, you're avoiding to use index 0, which I find rather odd. Given that the DT binding states that each group manages 64 interrupts, I have the feeling that you're missing some interrupt enable state. > +} > + > +static void imx_irqsteer_restore_regs(struct irqsteer_data *data) > +{ > + int num; > + > + writel_relaxed(BIT(data->channel), data->regs + CHANCTRL); > + for (num = 0; num < data->irq_groups; num++) > + writel_relaxed(data->saved_reg[num + 1], > + data->regs + CHANMASK(num, data->irq_groups)); > +} > + > +static int imx_irqsteer_suspend(struct device *dev) > +{ > + struct irqsteer_data *irqsteer_data = dev_get_drvdata(dev); > + > + imx_irqsteer_save_regs(irqsteer_data); > + clk_disable_unprepare(irqsteer_data->ipg_clk); > + > + return 0; > +} > + > +static int imx_irqsteer_resume(struct device *dev) > +{ > + struct irqsteer_data *irqsteer_data = dev_get_drvdata(dev); > + int ret; > + > + ret = clk_prepare_enable(irqsteer_data->ipg_clk); > + if (ret) { > + dev_err(dev, "failed to enable ipg clk: %d\n", ret); > + return ret; > + } > + imx_irqsteer_restore_regs(irqsteer_data); > + > + return 0; > +} > +#endif > + > +static const struct dev_pm_ops imx_irqsteer_pm_ops = { > + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx_irqsteer_suspend, imx_irqsteer_resume) > +}; > + > +static const struct of_device_id imx_irqsteer_dt_ids[] = { > + { .compatible = "fsl,imx-irqsteer", }, > + {}, > +}; > + > +static struct platform_driver imx_irqsteer_driver = { > + .driver = { > + .name = "imx-irqsteer", > + .of_match_table = imx_irqsteer_dt_ids, > + .pm = &imx_irqsteer_pm_ops, > + }, > + .probe = imx_irqsteer_probe, > + .remove = imx_irqsteer_remove, > +}; > +builtin_platform_driver(imx_irqsteer_driver); > -- > 2.19.1 > This looks much better than the original submission. If you quickly clarify the few questions I have, we can probably move this quickly enough. Thanks, M. -- Jazz is not dead, it just smell funny.