Re: [PATCH 2/2] irqchip/mchp-eic: add support
From: Claudiu.Beznea
Date: Mon Mar 08 2021 - 08:44:46 EST
Hi Mark,
Appologies for late reply, I had to double check some of your questions w/
IP designer.
On 02.03.2021 14:02, Marc Zyngier wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Tue, 02 Mar 2021 10:28:46 +0000,
> Claudiu Beznea <claudiu.beznea@xxxxxxxxxxxxx> wrote:
>>
>> Add support for Microchip External Interrupt Controller.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@xxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxx (moderated for non-subscribers)
>> S: Supported
>> F: drivers/usb/gadget/udc/atmel_usba_udc.*
>>
>> +MICROCHIP EIC DRIVER
>> +M: Claudiu Beznea <claudiu.beznea@xxxxxxxxxxxxx>
>> +L: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx (moderated for non-subscribers)
>> +S: Supported
>> +F: drivers/irqchip/irq-mchp-eic.c
>> +
>> MICROCHIP WILC1000 WIFI DRIVER
>> M: Ajay Singh <ajay.kathat@xxxxxxxxxxxxx>
>> M: Claudiu Beznea <claudiu.beznea@xxxxxxxxxxxxx>
>> 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@xxxxxxxxxxxxx>
>> + */
>> +#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.
OK.
>
>> +
>> + 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.
I used the MCHP_EIC_DEFAULT_WAIT_US as the rate limiter for this. I will
move it inside the function to be more clear.
>
>> +}
>> +
>> +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?
According to the discusion I had with the IP designer, yes, the output
active level of this IP is always 1.
The level or edge detection refers to the input signal of EIC. No matter
the input edge detection for EIC, the EIC will generate a high pulse for GIC.
>
>> + 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.
There is nothing to be done to acknowledge this (I double checked with the
designer). The IP has only 2 registers:
1/ Glitch Filter Configuration Status Register: with 2 readable bits (bit 0
for interrupt 0, bit 1 for interrupt 1); reading a 1 to any of these bits
means the interrupt line (0 or 1) glitch filter configuration is done and
the glitch filter is active. The glitch filter can be programmed in SCFGR.
2/ Source Configuration Register (SCFGR): with bits having the following
meaning:
- bit 31: writing a 1 freezes the SCFGR values until hardware reset.
- bit 16: writing a 1 enables the source level/edge detection; writing a 0
disables the source level/edge detection
- bit 9: for input level configuration detection
- bit 8: for input polarity configuration detection
- bit 4: for enabling/disbabling glitch filter
- bit 1..0: glitch filter settings
>
>> +
>> +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?
This escaped me.
>
>> + /* 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?
I'll do it in the next version.
>
>> + 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).
Sure, I'll address it in the next version.
>
>> +
>> + /* 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?
I'll dig into this.
>
>> + 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.
Yes, as per discusion with designer on our side the output of EIC is always
high.
>
>> +}
>> +
>> +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...
I used the approach from another driver and forgot to double check it. Will
be removed in the next version.
Thank you for your review,
Claudiu Beznea
>
>> +
>> + 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@xxxxxxxxxxxxx>");
>> --
>> 2.25.1
>>
>>
>
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
>