Message-ID: <87tuptzrtl.wl-maz@kernel.org>
Date:   Tue, 02 Mar 2021 12:02:46 +0000
From:   Marc Zyngier <maz@...nel.org>
To:     Claudiu Beznea <claudiu.beznea@...rochip.com>
Cc:     <tglx@...utronix.de>, <robh+dt@...nel.org>,
        <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <nicolas.ferre@...rochip.com>
Subject: Re: [PATCH 2/2] irqchip/mchp-eic: add support

On Tue, 02 Mar 2021 10:28:46 +0000,
Claudiu Beznea <claudiu.beznea@...rochip.com> wrote:
> 
> Add support for Microchip External Interrupt Controller.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@...rochip.com>
> ---
>  MAINTAINERS                    |   6 +
>  drivers/irqchip/Kconfig        |   7 +
>  drivers/irqchip/Makefile       |   1 +
>  drivers/irqchip/irq-mchp-eic.c | 350 +++++++++++++++++++++++++++++++++
>  4 files changed, 364 insertions(+)
>  create mode 100644 drivers/irqchip/irq-mchp-eic.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a008b70f3c16..ea99e5a7f519 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11585,6 +11585,12 @@ L:	linux-arm-kernel@...ts.infradead.org (moderated for non-subscribers)
>  S:	Supported
>  F:	drivers/usb/gadget/udc/atmel_usba_udc.*
>  
> +MICROCHIP EIC DRIVER
> +M:	Claudiu Beznea <claudiu.beznea@...rochip.com>
> +L:	linux-arm-kernel@...ts.infradead.org (moderated for non-subscribers)
> +S:	Supported
> +F:	drivers/irqchip/irq-mchp-eic.c
> +
>  MICROCHIP WILC1000 WIFI DRIVER
>  M:	Ajay Singh <ajay.kathat@...rochip.com>
>  M:	Claudiu Beznea <claudiu.beznea@...rochip.com>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 2aa79c32ee22..7c7ca33c3f7d 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -596,4 +596,11 @@ config MST_IRQ
>  	help
>  	  Support MStar Interrupt Controller.
>  
> +config MCHP_EIC
> +	bool "Microchip External Interrupt Controller"
> +	depends on ARCH_AT91 || COMPILE_TEST
> +	select IRQ_DOMAIN
> +	help
> +	  Support for Microchip External Interrupt Controller.
> +
>  endmenu
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 94c2885882ee..f3df6eb7c4c3 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -114,3 +114,4 @@ obj-$(CONFIG_LOONGSON_PCH_PIC)		+= irq-loongson-pch-pic.o
>  obj-$(CONFIG_LOONGSON_PCH_MSI)		+= irq-loongson-pch-msi.o
>  obj-$(CONFIG_MST_IRQ)			+= irq-mst-intc.o
>  obj-$(CONFIG_SL28CPLD_INTC)		+= irq-sl28cpld.o
> +obj-$(CONFIG_MCHP_EIC)			+= irq-mchp-eic.o
> diff --git a/drivers/irqchip/irq-mchp-eic.c b/drivers/irqchip/irq-mchp-eic.c
> new file mode 100644
> index 000000000000..ec01877fc75f
> --- /dev/null
> +++ b/drivers/irqchip/irq-mchp-eic.c
> @@ -0,0 +1,350 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Microchip External Interrupt Controller driver
> + *
> + * Copyright (C) 2021 Microchip Technology Inc. and its subsidiaries
> + *
> + * Author: Claudiu Beznea <claudiu.beznea@...rochip.com>
> + */
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqdomain.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/irqchip/chained_irq.h>
> +
> +#define MCHP_EIC_GFCS			(0x0)
> +#define MCHP_EIC_SCFG(x)		(0x4 + (x) * 0x4)
> +#define MCHP_EIC_SCFG_FRZ		BIT(31)
> +#define MCHP_EIC_SCFG_EN		BIT(16)
> +#define MCHP_EIC_SCFG_LVL		BIT(9)
> +#define MCHP_EIC_SCFG_POL		BIT(8)
> +#define MCHP_EIC_SCFG_GFEN		BIT(4)
> +#define MCHP_EIC_SCFG_GFSEL		GENMASK(1, 0)
> +
> +#define MCHP_EIC_NIRQ			(2)
> +#define MCHP_EIC_DEFAULT_WAIT_US	(100)
> +
> +/**
> + * struct mchp_eic - EIC private data structure
> + * @base: base address
> + * @dev: eic device
> + * @clk: peripheral clock
> + * @chip: irq chip
> + * @domain: irq domain
> + * @irqs: irqs b/w eic and gic
> + * @scfg: backup for scfg registers (necessary for backup and self-refresh mode)
> + * @wakeup_source: wakeup source mask
> + */
> +struct mchp_eic {
> +	void __iomem *base;
> +	struct device *dev;
> +	struct clk *clk;
> +	struct irq_chip *chip;
> +	struct irq_domain *domain;
> +	u32 irqs[MCHP_EIC_NIRQ];
> +	u32 scfg[MCHP_EIC_NIRQ];
> +	u32 wakeup_source;
> +};
> +
> +static void mchp_eic_wait_rdy(struct mchp_eic *eic, unsigned int hwirq,
> +			      int delay_us)
> +{
> +	unsigned int tmp = readl(eic->base + MCHP_EIC_SCFG(hwirq));

Please use relaxed accessors everywhere unless you need ordering with
things like DMA (you don't). Specially if oyu are going to use that
in loops as below.

> +
> +	while (delay_us >= 0 && tmp & BIT(hwirq)) {
> +		udelay(1);
> +		delay_us -= 1;
> +		tmp = readl(eic->base + MCHP_EIC_SCFG(hwirq));
> +	}
> +
> +	if (delay_us < 0)
> +		dev_err(eic->dev, "Failed to configure IRQ=%u!\n", hwirq);

Given that this is used on some interrupt paths, you should *at least*
rate-limit it.

> +}
> +
> +static void mchp_eic_irq_mask(struct irq_data *d)
> +{
> +	struct mchp_eic *eic = irq_data_get_irq_chip_data(d);
> +	unsigned int tmp;
> +
> +	tmp = readl(eic->base + MCHP_EIC_SCFG(d->hwirq));
> +	tmp &= ~MCHP_EIC_SCFG_EN;
> +	writel(tmp, eic->base + MCHP_EIC_SCFG(d->hwirq));
> +}
> +
> +static void mchp_eic_irq_unmask(struct irq_data *d)
> +{
> +	struct mchp_eic *eic = irq_data_get_irq_chip_data(d);
> +	unsigned int tmp;
> +
> +	tmp = readl(eic->base + MCHP_EIC_SCFG(d->hwirq));
> +	tmp |= MCHP_EIC_SCFG_EN;
> +	writel(tmp, eic->base + MCHP_EIC_SCFG(d->hwirq));
> +	mchp_eic_wait_rdy(eic, d->hwirq, MCHP_EIC_DEFAULT_WAIT_US);
> +}
> +
> +static int mchp_eic_irq_set_type(struct irq_data *d, unsigned int type)
> +{
> +	struct mchp_eic *eic = irq_data_get_irq_chip_data(d);
> +	unsigned int tmp;
> +
> +	tmp = readl(eic->base + MCHP_EIC_SCFG(d->hwirq));
> +	tmp &= ~(MCHP_EIC_SCFG_POL | MCHP_EIC_SCFG_LVL);
> +	switch (type) {
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		irq_set_handler_locked(d, handle_level_irq);
> +		tmp |= MCHP_EIC_SCFG_POL | MCHP_EIC_SCFG_LVL;
> +		break;
> +	case IRQ_TYPE_LEVEL_LOW:
> +		irq_set_handler_locked(d, handle_level_irq);
> +		tmp |= MCHP_EIC_SCFG_LVL;
> +		break;
> +	case IRQ_TYPE_EDGE_RISING:
> +		irq_set_handler_locked(d, handle_edge_irq);
> +		tmp |= MCHP_EIC_SCFG_POL;
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		irq_set_handler_locked(d, handle_edge_irq);

How about the parent interrupt? Is the output of this irqchip always
LEVEL_HIGH, and not influenced by the triggering of its input?

> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	writel(tmp, eic->base + MCHP_EIC_SCFG(d->hwirq));
> +
> +	return 0;
> +}
> +
> +static void mchp_eic_irq_ack(struct irq_data *d)
> +{
> +	/* Nothing to do here. */
> +}

Are you sure there is really nothing to do, even for an edge
interrupt? This is pretty unlikely. The interrupt controller must be
told when to stop coalescing edges, and without an Ack, it can't.

> +
> +static int mchp_eic_irq_set_wake(struct irq_data *d, unsigned int on)
> +{
> +	struct mchp_eic *eic = irq_data_get_irq_chip_data(d);
> +
> +	irq_set_irq_wake(eic->irqs[d->hwirq], on);
> +	if (on)
> +		eic->wakeup_source |= BIT(d->hwirq);
> +	else
> +		eic->wakeup_source &= BIT(d->hwirq);
> +
> +	return 0;
> +}
> +
> +static struct irq_chip mchp_eic_chip = {
> +	.name		= "eic-irq",
> +	.flags		= IRQCHIP_MASK_ON_SUSPEND,
> +	.irq_mask	= mchp_eic_irq_mask,
> +	.irq_unmask	= mchp_eic_irq_unmask,
> +	.irq_set_type	= mchp_eic_irq_set_type,
> +	.irq_ack	= mchp_eic_irq_ack,
> +	.irq_set_wake	= mchp_eic_irq_set_wake,
> +};
> +
> +static int mchp_eic_domain_map(struct irq_domain *d, unsigned int virq,
> +			       irq_hw_number_t hw)
> +{
> +	struct mchp_eic *eic = d->host_data;
> +
> +	irq_set_chip_data(virq, eic);
> +	irq_set_chip_and_handler(virq, eic->chip, handle_bad_irq);
> +	irq_set_probe(virq);
> +
> +	return 0;
> +}
> +
> +static int mchp_eic_domain_xlate(struct irq_domain *d, struct device_node *node,
> +				 const u32 *intspec, unsigned int intsize,
> +				 irq_hw_number_t *out_hwirq,
> +				 unsigned int *out_type)
> +{
> +	struct mchp_eic *eic = d->host_data;
> +	unsigned int tmp, i;
> +
> +	if (WARN_ON(intsize < 2))
> +		return -EINVAL;
> +
> +	if (WARN_ON(intspec[0] > MCHP_EIC_NIRQ))
> +		return -EINVAL;
> +
> +	*out_hwirq = intspec[0];
> +	*out_type = intspec[1] & IRQ_TYPE_SENSE_MASK;
> +
> +	if (intsize == 3) {

What if intsize isn't 3?

> +		/* Validate against available filters. */
> +		for (i = 0; i <= MCHP_EIC_SCFG_GFSEL; i++) {
> +			if (intspec[2] == (2 << i))
> +				break;
> +		}

Why don't you just take the log2() of the input value, and use that?

> +		if (i == MCHP_EIC_SCFG_GFSEL + 1)
> +			return -EINVAL;
> +
> +		/* Enable glitch filter. */
> +		tmp = readl(eic->base + MCHP_EIC_SCFG(*out_hwirq));
> +		tmp |= MCHP_EIC_SCFG_GFEN;
> +		tmp &= ~MCHP_EIC_SCFG_GFSEL;
> +		tmp |= i;
> +		writel(tmp, eic->base + MCHP_EIC_SCFG(*out_hwirq));

Please move this out of the xlate path. For a start, xlate shouldn't
have any effect on the HW. Also, this should be a static configuration
(see my comment in the first patch).

> +
> +		/* Wait for glitch filter to be enabled. */
> +		mchp_eic_wait_rdy(eic, *out_hwirq, MCHP_EIC_DEFAULT_WAIT_US);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops mchp_eic_domain_ops = {
> +	.map		= mchp_eic_domain_map,
> +	.xlate		= mchp_eic_domain_xlate,
> +};
> +
> +static void mchp_eic_irq_handler(struct irq_desc *desc)
> +{
> +	struct mchp_eic *eic = irq_desc_get_handler_data(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	unsigned int hwirq, irq = irq_desc_get_irq(desc);
> +
> +	for (hwirq = 0; hwirq < MCHP_EIC_NIRQ; hwirq++) {
> +		if (eic->irqs[hwirq] == irq)
> +			break;
> +	}

Why don't you just use the GIC input INTID as a base, and use the
offset from that base to compute the hwirq?

> +	if (hwirq == MCHP_EIC_NIRQ)
> +		return;
> +
> +	chained_irq_enter(chip, desc);
> +	generic_handle_irq(irq_find_mapping(eic->domain, hwirq));
> +	chained_irq_exit(chip, desc);

Same question as above: are you sure the output of this block is
always LEVEL_HIGH? because if it isn't this, should be treated as a
hierarchical irqchip, and not a chained one.

> +}
> +
> +static int mchp_eic_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	struct mchp_eic *eic;
> +	int ret, i;
> +
> +	eic = devm_kzalloc(&pdev->dev, sizeof(*eic), GFP_KERNEL);
> +	if (!eic)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	eic->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(eic->base))
> +		return PTR_ERR(eic->base);
> +
> +	eic->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(eic->clk))
> +		return PTR_ERR(eic->clk);
> +
> +	ret = clk_prepare_enable(eic->clk);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < MCHP_EIC_NIRQ; i++) {
> +		/* Disable it, if any. */
> +		writel(0UL, eic->base + MCHP_EIC_SCFG(i));
> +
> +		res = platform_get_resource(pdev, IORESOURCE_IRQ, i);
> +		if (!res) {
> +			ret = dev_err_probe(&pdev->dev, -EINVAL,
> +					    "Failed to get hwirq=%d\n", i);
> +			goto clk_unprepare;
> +		}
> +
> +		eic->irqs[i] = res->start;
> +		irq_set_chained_handler(res->start, mchp_eic_irq_handler);
> +		irq_set_handler_data(res->start, eic);
> +	}
> +
> +	eic->dev = &pdev->dev;
> +	eic->chip = &mchp_eic_chip;
> +
> +	eic->domain = irq_domain_add_linear(pdev->dev.of_node, MCHP_EIC_NIRQ,
> +					    &mchp_eic_domain_ops, eic);
> +	if (!eic->domain) {
> +		ret = dev_err_probe(&pdev->dev, -ENODEV,
> +				    "Failed to regiter irq domain!\n");
> +		goto clk_unprepare;
> +	}
> +	eic->domain->name = "eic";

Why this override? The only benefits for that are a memory leak and a
memory corruption if we ever try to free the domain...

> +
> +	platform_set_drvdata(pdev, eic);
> +
> +	dev_info(&pdev->dev, "EIC registered, nr_irqs %u\n", MCHP_EIC_NIRQ);
> +
> +	return 0;
> +
> +clk_unprepare:
> +	clk_disable_unprepare(eic->clk);
> +	return ret;
> +}
> +
> +static int mchp_eic_remove(struct platform_device *pdev)
> +{
> +	struct mchp_eic *eic = platform_get_drvdata(pdev);
> +
> +	irq_domain_remove(eic->domain);
> +	clk_disable_unprepare(eic->clk);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused mchp_eic_suspend(struct device *dev)
> +{
> +	struct mchp_eic *eic = dev_get_drvdata(dev);
> +	unsigned int hwirq;
> +
> +	for (hwirq = 0; hwirq < MCHP_EIC_NIRQ; hwirq++)
> +		eic->scfg[hwirq] = readl(eic->base + MCHP_EIC_SCFG(hwirq));
> +
> +	if (!eic->wakeup_source)
> +		clk_disable_unprepare(eic->clk);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused mchp_eic_resume(struct device *dev)
> +{
> +	struct mchp_eic *eic = dev_get_drvdata(dev);
> +	unsigned int hwirq;
> +
> +	if (!eic->wakeup_source)
> +		clk_prepare_enable(eic->clk);
> +
> +	for (hwirq = 0; hwirq < MCHP_EIC_NIRQ; hwirq++) {
> +		writel(eic->scfg[hwirq], eic->base + MCHP_EIC_SCFG(hwirq));
> +		mchp_eic_wait_rdy(eic, hwirq, MCHP_EIC_DEFAULT_WAIT_US);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops mchp_eic_pm_ops = {
> +	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(mchp_eic_suspend, mchp_eic_resume)
> +};
> +
> +static const struct of_device_id mchp_eic_dt_ids[] = {
> +	{ .compatible = "microchip,sama7g5-eic", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, mchp_eic_dt_ids);
> +
> +static struct platform_driver mchp_eic_device_driver = {
> +	.probe		= mchp_eic_probe,
> +	.remove		= mchp_eic_remove,
> +	.driver		= {
> +		.name = "mchp-eic",
> +		.of_match_table = of_match_ptr(mchp_eic_dt_ids),
> +		.pm = pm_ptr(&mchp_eic_pm_ops),
> +	},
> +};
> +module_platform_driver(mchp_eic_device_driver);
> +
> +MODULE_DESCRIPTION("Microchip External Interrupt Controller");
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Claudiu Beznea <claudiu.beznea@...rochip.com>");
> -- 
> 2.25.1
> 
> 

Thanks,

	M.

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

Powered by blists - more mailing lists