The Microchip LAN966x outband interrupt controller (OIC) maps the
internal interrupt sources of the LAN966x device to an external
interrupt.
When the LAN966x device is used as a PCI device, the external interrupt
is routed to the PCI interrupt.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/irqchip/Kconfig           |  12 ++
 drivers/irqchip/Makefile          |   1 +
 drivers/irqchip/irq-lan966x-oic.c | 308 ++++++++++++++++++++++++++++++
 3 files changed, 321 insertions(+)
 create mode 100644 drivers/irqchip/irq-lan966x-oic.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 14464716bacb..348f34525d23 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -169,6 +169,18 @@ config IXP4XX_IRQ
 	select IRQ_DOMAIN
 	select SPARSE_IRQ
 
+config LAN966X_OIC
+	tristate "Microchip LAN966x OIC Support"
+	select GENERIC_IRQ_CHIP
+	select IRQ_DOMAIN
+	help
+	  Enable support for the LAN966x Outbound Interrupt Controller.
+	  This controller is present on the Microchip LAN966x PCI device and
+	  maps the internal interrupts sources to PCIe interrupt.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called irq-lan966x-oic.
+
 config MADERA_IRQ
 	tristate
 
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index d9dc3d99aaa8..9f6f88274bec 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -104,6 +104,7 @@ obj-$(CONFIG_IMX_IRQSTEER)		+= irq-imx-irqsteer.o
 obj-$(CONFIG_IMX_INTMUX)		+= irq-imx-intmux.o
 obj-$(CONFIG_IMX_MU_MSI)		+= irq-imx-mu-msi.o
 obj-$(CONFIG_MADERA_IRQ)		+= irq-madera.o
+obj-$(CONFIG_LAN966X_OIC)		+= irq-lan966x-oic.o
 obj-$(CONFIG_LS1X_IRQ)			+= irq-ls1x.o
 obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o
 obj-$(CONFIG_TI_SCI_INTA_IRQCHIP)	+= irq-ti-sci-inta.o
diff --git a/drivers/irqchip/irq-lan966x-oic.c b/drivers/irqchip/irq-lan966x-oic.c
new file mode 100644
index 000000000000..a5f64610e62d
--- /dev/null
+++ b/drivers/irqchip/irq-lan966x-oic.c
@@ -0,0 +1,308 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for the Microchip LAN966x outbound interrupt controller
+ *
+ * Copyright (c) 2024 Technology Inc. and its subsidiaries.
+ *
+ * Authors:
+ *	Horatiu Vultur <horatiu.vultur@microchip.com>
+ *	Clément Léger <clement.leger@bootlin.com>
+ *	Herve Codina <herve.codina@bootlin.com>
+ */
+
+#include <linux/bitops.h>
+#include <linux/build_bug.h>
+#include <linux/interrupt.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqchip.h>
+#include <linux/irq.h>
+#include <linux/iopoll.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/delay.h>
+
+struct lan966x_oic_chip_regs {
+	int reg_off_ena_set;
+	int reg_off_ena_clr;
+	int reg_off_sticky;
+	int reg_off_ident;
+	int reg_off_map;
+};
+
+struct lan966x_oic_data {
+	struct irq_domain *domain;
+	void __iomem *regs;
+	int irq;
+};
+
+#define LAN966X_OIC_NR_IRQ 86
+
+/* Interrupt sticky status */
+#define LAN966X_OIC_INTR_STICKY		0x30
+#define LAN966X_OIC_INTR_STICKY1	0x34
+#define LAN966X_OIC_INTR_STICKY2	0x38
+
+/* Interrupt enable */
+#define LAN966X_OIC_INTR_ENA		0x48
+#define LAN966X_OIC_INTR_ENA1		0x4c
+#define LAN966X_OIC_INTR_ENA2		0x50
+
+/* Atomic clear of interrupt enable */
+#define LAN966X_OIC_INTR_ENA_CLR	0x54
+#define LAN966X_OIC_INTR_ENA_CLR1	0x58
+#define LAN966X_OIC_INTR_ENA_CLR2	0x5c
+
+/* Atomic set of interrupt */
+#define LAN966X_OIC_INTR_ENA_SET	0x60
+#define LAN966X_OIC_INTR_ENA_SET1	0x64
+#define LAN966X_OIC_INTR_ENA_SET2	0x68
+
+/* Mapping of source to destination interrupts (_n = 0..8) */
+#define LAN966X_OIC_DST_INTR_MAP(_n)	(0x78 + (_n) * 4)
+#define LAN966X_OIC_DST_INTR_MAP1(_n)	(0x9c + (_n) * 4)
+#define LAN966X_OIC_DST_INTR_MAP2(_n)	(0xc0 + (_n) * 4)
+
+/* Currently active interrupt sources per destination (_n = 0..8) */
+#define LAN966X_OIC_DST_INTR_IDENT(_n)	(0xe4 + (_n) * 4)
+#define LAN966X_OIC_DST_INTR_IDENT1(_n)	(0x108 + (_n) * 4)
+#define LAN966X_OIC_DST_INTR_IDENT2(_n)	(0x12c + (_n) * 4)
+
+static unsigned int lan966x_oic_irq_startup(struct irq_data *data)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(data);
+	struct irq_chip_type *ct = irq_data_get_chip_type(data);
+	struct lan966x_oic_chip_regs *chip_regs = gc->private;
+	u32 map;
+
+	irq_gc_lock(gc);
+
+	/* Map the source interrupt to the destination */
+	map = irq_reg_readl(gc, chip_regs->reg_off_map);
+	map |= data->mask;
+	irq_reg_writel(gc, map, chip_regs->reg_off_map);
+
+	irq_gc_unlock(gc);
+
+	ct->chip.irq_ack(data);
+	ct->chip.irq_unmask(data);
+
+	return 0;
+}
+
+static void lan966x_oic_irq_shutdown(struct irq_data *data)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(data);
+	struct irq_chip_type *ct = irq_data_get_chip_type(data);
+	struct lan966x_oic_chip_regs *chip_regs = gc->private;
+	u32 map;
+
+	ct->chip.irq_mask(data);
+
+	irq_gc_lock(gc);
+
+	/* Unmap the interrupt */
+	map = irq_reg_readl(gc, chip_regs->reg_off_map);
+	map &= ~data->mask;
+	irq_reg_writel(gc, map, chip_regs->reg_off_map);
+
+	irq_gc_unlock(gc);
+}
+
+static int lan966x_oic_irq_set_type(struct irq_data *data,
+				    unsigned int flow_type)
+{
+	if (flow_type != IRQ_TYPE_LEVEL_HIGH) {
+		pr_err("lan966x oic doesn't support flow type %d\n", flow_type);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void lan966x_oic_irq_handler_domain(struct irq_domain *d, u32 first_irq)
+{
+	struct irq_chip_generic *gc = irq_get_domain_generic_chip(d, first_irq);
+	struct lan966x_oic_chip_regs *chip_regs = gc->private;
+	unsigned long ident;
+	unsigned int hwirq;
+
+	ident = irq_reg_readl(gc, chip_regs->reg_off_ident);
+	if (!ident)
+		return;
+
+	for_each_set_bit(hwirq, &ident, 32)
+		generic_handle_domain_irq(d, hwirq + first_irq);
+}
+
+static void lan966x_oic_irq_handler(struct irq_desc *desc)
+{
+	struct irq_domain *d = irq_desc_get_handler_data(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+
+	chained_irq_enter(chip, desc);
+	lan966x_oic_irq_handler_domain(d, 0);
+	lan966x_oic_irq_handler_domain(d, 32);
+	lan966x_oic_irq_handler_domain(d, 64);
+	chained_irq_exit(chip, desc);
+}
+
+static struct lan966x_oic_chip_regs lan966x_oic_chip_regs[3] = {
+	{
+		.reg_off_ena_set = LAN966X_OIC_INTR_ENA_SET,
+		.reg_off_ena_clr = LAN966X_OIC_INTR_ENA_CLR,
+		.reg_off_sticky = LAN966X_OIC_INTR_STICKY,
+		.reg_off_ident = LAN966X_OIC_DST_INTR_IDENT(0),
+		.reg_off_map = LAN966X_OIC_DST_INTR_MAP(0),
+	}, {
+		.reg_off_ena_set = LAN966X_OIC_INTR_ENA_SET1,
+		.reg_off_ena_clr = LAN966X_OIC_INTR_ENA_CLR1,
+		.reg_off_sticky = LAN966X_OIC_INTR_STICKY1,
+		.reg_off_ident = LAN966X_OIC_DST_INTR_IDENT1(0),
+		.reg_off_map = LAN966X_OIC_DST_INTR_MAP1(0),
+	}, {
+		.reg_off_ena_set = LAN966X_OIC_INTR_ENA_SET2,
+		.reg_off_ena_clr = LAN966X_OIC_INTR_ENA_CLR2,
+		.reg_off_sticky = LAN966X_OIC_INTR_STICKY2,
+		.reg_off_ident = LAN966X_OIC_DST_INTR_IDENT2(0),
+		.reg_off_map = LAN966X_OIC_DST_INTR_MAP2(0),
+	}
+};
+
+static void lan966x_oic_chip_init(struct lan966x_oic_data *lan966x_oic,
+				  struct irq_chip_generic *gc,
+				  struct lan966x_oic_chip_regs *chip_regs)
+{
+	gc->reg_base = lan966x_oic->regs;
+	gc->chip_types[0].regs.enable = chip_regs->reg_off_ena_set;
+	gc->chip_types[0].regs.disable = chip_regs->reg_off_ena_clr;
+	gc->chip_types[0].regs.ack = chip_regs->reg_off_sticky;
+	gc->chip_types[0].chip.irq_startup = lan966x_oic_irq_startup;
+	gc->chip_types[0].chip.irq_shutdown = lan966x_oic_irq_shutdown;
+	gc->chip_types[0].chip.irq_set_type = lan966x_oic_irq_set_type;
+	gc->chip_types[0].chip.irq_mask = irq_gc_mask_disable_reg;
+	gc->chip_types[0].chip.irq_unmask = irq_gc_unmask_enable_reg;
+	gc->chip_types[0].chip.irq_ack = irq_gc_ack_set_bit;
+	gc->private = chip_regs;
+
+	/* Disable all interrupts handled by this chip */
+	irq_reg_writel(gc, ~0, chip_regs->reg_off_ena_clr);
+}
+
+static void lan966x_oic_chip_exit(struct irq_chip_generic *gc)
+{
+	/* Disable and ack all interrupts handled by this chip */
+	irq_reg_writel(gc, ~0, gc->chip_types[0].regs.disable);
+	irq_reg_writel(gc, ~0, gc->chip_types[0].regs.ack);
+}
+
+static int lan966x_oic_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	struct lan966x_oic_data *lan966x_oic;
+	struct device *dev = &pdev->dev;
+	struct irq_chip_generic *gc;
+	int ret;
+	int i;
+
+	lan966x_oic = devm_kmalloc(dev, sizeof(*lan966x_oic), GFP_KERNEL);
+	if (!lan966x_oic)
+		return -ENOMEM;
+
+	lan966x_oic->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(lan966x_oic->regs))
+		return dev_err_probe(dev, PTR_ERR(lan966x_oic->regs),
+				     "failed to map resource\n");
+
+	lan966x_oic->domain = irq_domain_alloc_linear(of_node_to_fwnode(node),
+						      LAN966X_OIC_NR_IRQ,
+						      &irq_generic_chip_ops,
+						      NULL);
+	if (!lan966x_oic->domain) {
+		dev_err(dev, "failed to create an IRQ domain\n");
+		return -EINVAL;
+	}
+
+	lan966x_oic->irq = platform_get_irq(pdev, 0);
+	if (lan966x_oic->irq < 0) {
+		ret = dev_err_probe(dev, lan966x_oic->irq,
+				    "failed to get the IRQ\n");
+		goto err_domain_free;
+	}
+
+	ret = irq_alloc_domain_generic_chips(lan966x_oic->domain, 32, 1,
+					     "lan966x-oic", handle_level_irq, 0,
+					     0, 0);
+	if (ret) {
+		dev_err_probe(dev, ret, "failed to alloc irq domain gc\n");
+		goto err_domain_free;
+	}
+
+	/* Init chips */
+	BUILD_BUG_ON(DIV_ROUND_UP(LAN966X_OIC_NR_IRQ, 32) !=
+		     ARRAY_SIZE(lan966x_oic_chip_regs));
+	for (i = 0; i < ARRAY_SIZE(lan966x_oic_chip_regs); i++) {
+		gc = irq_get_domain_generic_chip(lan966x_oic->domain, i * 32);
+		lan966x_oic_chip_init(lan966x_oic, gc,
+				      &lan966x_oic_chip_regs[i]);
+	}
+
+	irq_set_chained_handler_and_data(lan966x_oic->irq,
+					 lan966x_oic_irq_handler,
+					 lan966x_oic->domain);
+
+	irq_domain_publish(lan966x_oic->domain);
+	platform_set_drvdata(pdev, lan966x_oic);
+	return 0;
+
+err_domain_free:
+	irq_domain_free(lan966x_oic->domain);
+	return ret;
+}
+
+static void lan966x_oic_remove(struct platform_device *pdev)
+{
+	struct lan966x_oic_data *lan966x_oic = platform_get_drvdata(pdev);
+	struct irq_chip_generic *gc;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(lan966x_oic_chip_regs); i++) {
+		gc = irq_get_domain_generic_chip(lan966x_oic->domain, i * 32);
+		lan966x_oic_chip_exit(gc);
+	}
+
+	irq_set_chained_handler_and_data(lan966x_oic->irq, NULL, NULL);
+
+	for (i = 0; i < LAN966X_OIC_NR_IRQ; i++)
+		irq_dispose_mapping(irq_find_mapping(lan966x_oic->domain, i));
+
+	irq_domain_unpublish(lan966x_oic->domain);
+
+	for (i = 0; i < ARRAY_SIZE(lan966x_oic_chip_regs); i++) {
+		gc = irq_get_domain_generic_chip(lan966x_oic->domain, i * 32);
+		irq_remove_generic_chip(gc, ~0, 0, 0);
+	}
+
+	kfree(lan966x_oic->domain->gc);
+	irq_domain_free(lan966x_oic->domain);
+}
+
+static const struct of_device_id lan966x_oic_of_match[] = {
+	{ .compatible = "microchip,lan966x-oic" },
+	{} /* sentinel */
+};
+MODULE_DEVICE_TABLE(of, lan966x_oic_of_match);
+
+static struct platform_driver lan966x_oic_driver = {
+	.probe = lan966x_oic_probe,
+	.remove_new = lan966x_oic_remove,
+	.driver = {
+		.name = "lan966x-oic",
+		.of_match_table = lan966x_oic_of_match,
+	},
+};
+module_platform_driver(lan966x_oic_driver);
+
+MODULE_AUTHOR("Herve Codina <herve.codina@bootlin.com>");
+MODULE_DESCRIPTION("Microchip LAN966x OIC driver");
+MODULE_LICENSE("GPL");
-- 
2.45.0

On Mon, May 27 2024 at 18:14, Herve Codina wrote:
> +struct lan966x_oic_data {
> +	struct irq_domain *domain;
> +	void __iomem *regs;
> +	int irq;
> +};

Please read Documentation/process/maintainers-tip.rst

> +static int lan966x_oic_irq_set_type(struct irq_data *data,
> +				    unsigned int flow_type)

Please use the 100 character limit

> +static struct lan966x_oic_chip_regs lan966x_oic_chip_regs[3] = {
> +	{
> +		.reg_off_ena_set = LAN966X_OIC_INTR_ENA_SET,
> +		.reg_off_ena_clr = LAN966X_OIC_INTR_ENA_CLR,
> +		.reg_off_sticky = LAN966X_OIC_INTR_STICKY,
> +		.reg_off_ident = LAN966X_OIC_DST_INTR_IDENT(0),
> +		.reg_off_map = LAN966X_OIC_DST_INTR_MAP(0),

Please make this tabular. See doc.

> +static void lan966x_oic_chip_init(struct lan966x_oic_data *lan966x_oic,
> +				  struct irq_chip_generic *gc,
> +				  struct lan966x_oic_chip_regs *chip_regs)
> +{
> +	gc->reg_base = lan966x_oic->regs;
> +	gc->chip_types[0].regs.enable = chip_regs->reg_off_ena_set;
> +	gc->chip_types[0].regs.disable = chip_regs->reg_off_ena_clr;
> +	gc->chip_types[0].regs.ack = chip_regs->reg_off_sticky;
> +	gc->chip_types[0].chip.irq_startup = lan966x_oic_irq_startup;
> +	gc->chip_types[0].chip.irq_shutdown = lan966x_oic_irq_shutdown;
> +	gc->chip_types[0].chip.irq_set_type = lan966x_oic_irq_set_type;
> +	gc->chip_types[0].chip.irq_mask = irq_gc_mask_disable_reg;
> +	gc->chip_types[0].chip.irq_unmask = irq_gc_unmask_enable_reg;
> +	gc->chip_types[0].chip.irq_ack = irq_gc_ack_set_bit;
> +	gc->private = chip_regs;
> +
> +	/* Disable all interrupts handled by this chip */
> +	irq_reg_writel(gc, ~0, chip_regs->reg_off_ena_clr);
> +}
> +
> +static void lan966x_oic_chip_exit(struct irq_chip_generic *gc)
> +{
> +	/* Disable and ack all interrupts handled by this chip */
> +	irq_reg_writel(gc, ~0, gc->chip_types[0].regs.disable);

~0U
  
> +	irq_reg_writel(gc, ~0, gc->chip_types[0].regs.ack);
> +}
> +
> +static int lan966x_oic_probe(struct platform_device *pdev)
> +{
> +	struct device_node *node = pdev->dev.of_node;
> +	struct lan966x_oic_data *lan966x_oic;
> +	struct device *dev = &pdev->dev;
> +	struct irq_chip_generic *gc;
> +	int ret;
> +	int i;

int ret, i;

> +
> +	lan966x_oic = devm_kmalloc(dev, sizeof(*lan966x_oic), GFP_KERNEL);
> +	if (!lan966x_oic)
> +		return -ENOMEM;
> +
> +	lan966x_oic->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(lan966x_oic->regs))
> +		return dev_err_probe(dev, PTR_ERR(lan966x_oic->regs),
> +				     "failed to map resource\n");
> +
> +	lan966x_oic->domain = irq_domain_alloc_linear(of_node_to_fwnode(node),
> +						      LAN966X_OIC_NR_IRQ,
> +						      &irq_generic_chip_ops,
> +						      NULL);
> +	if (!lan966x_oic->domain) {
> +		dev_err(dev, "failed to create an IRQ domain\n");
> +		return -EINVAL;
> +	}
> +
> +	lan966x_oic->irq = platform_get_irq(pdev, 0);
> +	if (lan966x_oic->irq < 0) {
> +		ret = dev_err_probe(dev, lan966x_oic->irq,
> +				    "failed to get the IRQ\n");
> +		goto err_domain_free;
> +	}
> +
> +	ret = irq_alloc_domain_generic_chips(lan966x_oic->domain, 32, 1,
> +					     "lan966x-oic", handle_level_irq, 0,
> +					     0, 0);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "failed to alloc irq domain gc\n");
> +		goto err_domain_free;
> +	}
> +
> +	/* Init chips */
> +	BUILD_BUG_ON(DIV_ROUND_UP(LAN966X_OIC_NR_IRQ, 32) !=
> +		     ARRAY_SIZE(lan966x_oic_chip_regs));
> +	for (i = 0; i < ARRAY_SIZE(lan966x_oic_chip_regs); i++) {
> +		gc = irq_get_domain_generic_chip(lan966x_oic->domain, i * 32);
> +		lan966x_oic_chip_init(lan966x_oic, gc,
> +				      &lan966x_oic_chip_regs[i]);
> +	}
> +
> +	irq_set_chained_handler_and_data(lan966x_oic->irq,
> +					 lan966x_oic_irq_handler,
> +					 lan966x_oic->domain);
> +
> +	irq_domain_publish(lan966x_oic->domain);
> +	platform_set_drvdata(pdev, lan966x_oic);
> +	return 0;

This is exactly what can be avoided.

> +
> +err_domain_free:
> +	irq_domain_free(lan966x_oic->domain);
> +	return ret;
> +}
> +
> +static void lan966x_oic_remove(struct platform_device *pdev)
> +{
> +	struct lan966x_oic_data *lan966x_oic = platform_get_drvdata(pdev);
> +	struct irq_chip_generic *gc;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(lan966x_oic_chip_regs); i++) {
> +		gc = irq_get_domain_generic_chip(lan966x_oic->domain, i * 32);
> +		lan966x_oic_chip_exit(gc);
> +	}
> +
> +	irq_set_chained_handler_and_data(lan966x_oic->irq, NULL, NULL);
> +
> +	for (i = 0; i < LAN966X_OIC_NR_IRQ; i++)
> +		irq_dispose_mapping(irq_find_mapping(lan966x_oic->domain, i));

This is just wrong. You cannot remove the chip when there are still interrupts
mapped.

I just did a quick conversion to the template approach. Unsurprisingly
it removes 30 lines of boiler plate code:

+static void lan966x_oic_chip_init(struct irq_chip_generic *gc)
+{
+	struct lan966x_oic_data *lan966x_oic = gc->domain->host_data;
+	struct lan966x_oic_chip_regs *chip_regs;
+
+	gc->reg_base = lan966x_oic->regs;
+
+	chip_regs = lan966x_oic_chip_regs + gc->irq_base / 32;
+	gc->chip_types[0].regs.enable = chip_regs->reg_off_ena_set;
+	gc->chip_types[0].regs.disable = chip_regs->reg_off_ena_clr;
+	gc->chip_types[0].regs.ack = chip_regs->reg_off_sticky;
+
+	gc->chip_types[0].chip.irq_startup = lan966x_oic_irq_startup;
+	gc->chip_types[0].chip.irq_shutdown = lan966x_oic_irq_shutdown;
+	gc->chip_types[0].chip.irq_set_type = lan966x_oic_irq_set_type;
+	gc->chip_types[0].chip.irq_mask = irq_gc_mask_disable_reg;
+	gc->chip_types[0].chip.irq_unmask = irq_gc_unmask_enable_reg;
+	gc->chip_types[0].chip.irq_ack = irq_gc_ack_set_bit;
+	gc->private = chip_regs;
+
+	/* Disable all interrupts handled by this chip */
+	irq_reg_writel(gc, ~0, chip_regs->reg_off_ena_clr);
+}
+
+static void lan966x_oic_chip_exit(struct irq_chip_generic *gc)
+{
+	/* Disable and ack all interrupts handled by this chip */
+	irq_reg_writel(gc, ~0, gc->chip_types[0].regs.disable);
+	irq_reg_writel(gc, ~0, gc->chip_types[0].regs.ack);
+}
+
+static void lan966x_oic_domain_init(struct irq_domain *d)
+{
+	struct lan966x_oic_data *lan966x_oic = d->host_data;
+
+	irq_set_chained_handler_and_data(lan966x_oic->irq, lan966x_oic_irq_handler, d);
+}
+
+static int lan966x_oic_probe(struct platform_device *pdev)
+{
+	struct irq_domain_chip_generic_info gc_info = {
+		.irqs_per_chip		= 32,
+		.num_chips		= 1,
+		.name			= "lan966x-oic"
+		.handler		= handle_level_irq,
+		.init			= lan966x_oic_chip_init,
+		.destroy		= lan966x_oic_chip_exit,
+	};
+
+	struct irq_domain_info info = {
+		.fwnode			= of_node_to_fwnode(pdev->dev.of_node),
+		.size			= LAN966X_OIC_NR_IRQ,
+		.hwirq_max		= LAN966X_OIC_NR_IRQ,
+		.ops			= &irq_generic_chip_ops,
+		.gc_info		= &gc_info,
+		.init			= lan966x_oic_domain_init,
+	};
+	struct lan966x_oic_data *lan966x_oic;
+	struct device *dev = &pdev->dev;
+
+	lan966x_oic = devm_kmalloc(dev, sizeof(*lan966x_oic), GFP_KERNEL);
+	if (!lan966x_oic)
+		return -ENOMEM;
+
+	lan966x_oic->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(lan966x_oic->regs))
+		return dev_err_probe(dev, PTR_ERR(lan966x_oic->regs), "failed to map resource\n");
+
+	lan966x_oic->irq = platform_get_irq(pdev, 0);
+	if (lan966x_oic->irq < 0)
+		return dev_err_probe(dev, lan966x_oic->irq, "failed to get the IRQ\n");
+
+	lan966x_oic->domain = irq_domain_instantiate(&info);
+	if (!lan966x_oic->domain)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, lan966x_oic);
+	return 0;
+}
+
+static void lan966x_oic_remove(struct platform_device *pdev)
+{
+	struct lan966x_oic_data *lan966x_oic = platform_get_drvdata(pdev);
+
+	irq_set_chained_handler_and_data(lan966x_oic->irq, NULL, NULL);
+	irq_domain_remove(lan966x_oic->domain);
+}

See?

Thanks,

        tglx

Hi Thomas,

On Wed, 05 Jun 2024 16:17:53 +0200
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Mon, May 27 2024 at 18:14, Herve Codina wrote:
> > +struct lan966x_oic_data {
> > +	struct irq_domain *domain;
> > +	void __iomem *regs;
> > +	int irq;
> > +};  
> 
> Please read Documentation/process/maintainers-tip.rst

I suppose you pointed out the un-tabular struct member names here.
I will fix that in the next iteration.

> 
> > +static int lan966x_oic_irq_set_type(struct irq_data *data,
> > +				    unsigned int flow_type)  
> 
> Please use the 100 character limit

Sure, will be fixed.

> 
> > +static struct lan966x_oic_chip_regs lan966x_oic_chip_regs[3] = {
> > +	{
> > +		.reg_off_ena_set = LAN966X_OIC_INTR_ENA_SET,
> > +		.reg_off_ena_clr = LAN966X_OIC_INTR_ENA_CLR,
> > +		.reg_off_sticky = LAN966X_OIC_INTR_STICKY,
> > +		.reg_off_ident = LAN966X_OIC_DST_INTR_IDENT(0),
> > +		.reg_off_map = LAN966X_OIC_DST_INTR_MAP(0),  
> 
> Please make this tabular. See doc.

Will be fixed.

> 
> > +static void lan966x_oic_chip_init(struct lan966x_oic_data *lan966x_oic,
> > +				  struct irq_chip_generic *gc,
> > +				  struct lan966x_oic_chip_regs *chip_regs)
> > +{
> > +	gc->reg_base = lan966x_oic->regs;
> > +	gc->chip_types[0].regs.enable = chip_regs->reg_off_ena_set;
> > +	gc->chip_types[0].regs.disable = chip_regs->reg_off_ena_clr;
> > +	gc->chip_types[0].regs.ack = chip_regs->reg_off_sticky;
> > +	gc->chip_types[0].chip.irq_startup = lan966x_oic_irq_startup;
> > +	gc->chip_types[0].chip.irq_shutdown = lan966x_oic_irq_shutdown;
> > +	gc->chip_types[0].chip.irq_set_type = lan966x_oic_irq_set_type;
> > +	gc->chip_types[0].chip.irq_mask = irq_gc_mask_disable_reg;
> > +	gc->chip_types[0].chip.irq_unmask = irq_gc_unmask_enable_reg;
> > +	gc->chip_types[0].chip.irq_ack = irq_gc_ack_set_bit;
> > +	gc->private = chip_regs;
> > +
> > +	/* Disable all interrupts handled by this chip */
> > +	irq_reg_writel(gc, ~0, chip_regs->reg_off_ena_clr);
> > +}
> > +
> > +static void lan966x_oic_chip_exit(struct irq_chip_generic *gc)
> > +{
> > +	/* Disable and ack all interrupts handled by this chip */
> > +	irq_reg_writel(gc, ~0, gc->chip_types[0].regs.disable);  
> 
> ~0U

Will be changed.

>   
> > +	irq_reg_writel(gc, ~0, gc->chip_types[0].regs.ack);
> > +}
> > +
> > +static int lan966x_oic_probe(struct platform_device *pdev)
> > +{
> > +	struct device_node *node = pdev->dev.of_node;
> > +	struct lan966x_oic_data *lan966x_oic;
> > +	struct device *dev = &pdev->dev;
> > +	struct irq_chip_generic *gc;
> > +	int ret;
> > +	int i;  
> 
> int ret, i;

Will be changed.

> 
> > +
> > +	lan966x_oic = devm_kmalloc(dev, sizeof(*lan966x_oic), GFP_KERNEL);
> > +	if (!lan966x_oic)
> > +		return -ENOMEM;
> > +
> > +	lan966x_oic->regs = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(lan966x_oic->regs))
> > +		return dev_err_probe(dev, PTR_ERR(lan966x_oic->regs),
> > +				     "failed to map resource\n");
> > +
> > +	lan966x_oic->domain = irq_domain_alloc_linear(of_node_to_fwnode(node),
> > +						      LAN966X_OIC_NR_IRQ,
> > +						      &irq_generic_chip_ops,
> > +						      NULL);
> > +	if (!lan966x_oic->domain) {
> > +		dev_err(dev, "failed to create an IRQ domain\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	lan966x_oic->irq = platform_get_irq(pdev, 0);
> > +	if (lan966x_oic->irq < 0) {
> > +		ret = dev_err_probe(dev, lan966x_oic->irq,
> > +				    "failed to get the IRQ\n");
> > +		goto err_domain_free;
> > +	}
> > +
> > +	ret = irq_alloc_domain_generic_chips(lan966x_oic->domain, 32, 1,
> > +					     "lan966x-oic", handle_level_irq, 0,
> > +					     0, 0);
> > +	if (ret) {
> > +		dev_err_probe(dev, ret, "failed to alloc irq domain gc\n");
> > +		goto err_domain_free;
> > +	}
> > +
> > +	/* Init chips */
> > +	BUILD_BUG_ON(DIV_ROUND_UP(LAN966X_OIC_NR_IRQ, 32) !=
> > +		     ARRAY_SIZE(lan966x_oic_chip_regs));
> > +	for (i = 0; i < ARRAY_SIZE(lan966x_oic_chip_regs); i++) {
> > +		gc = irq_get_domain_generic_chip(lan966x_oic->domain, i * 32);
> > +		lan966x_oic_chip_init(lan966x_oic, gc,
> > +				      &lan966x_oic_chip_regs[i]);
> > +	}
> > +
> > +	irq_set_chained_handler_and_data(lan966x_oic->irq,
> > +					 lan966x_oic_irq_handler,
> > +					 lan966x_oic->domain);
> > +
> > +	irq_domain_publish(lan966x_oic->domain);
> > +	platform_set_drvdata(pdev, lan966x_oic);
> > +	return 0;  
> 
> This is exactly what can be avoided.
> 
> > +
> > +err_domain_free:
> > +	irq_domain_free(lan966x_oic->domain);
> > +	return ret;
> > +}
> > +
> > +static void lan966x_oic_remove(struct platform_device *pdev)
> > +{
> > +	struct lan966x_oic_data *lan966x_oic = platform_get_drvdata(pdev);
> > +	struct irq_chip_generic *gc;
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(lan966x_oic_chip_regs); i++) {
> > +		gc = irq_get_domain_generic_chip(lan966x_oic->domain, i * 32);
> > +		lan966x_oic_chip_exit(gc);
> > +	}
> > +
> > +	irq_set_chained_handler_and_data(lan966x_oic->irq, NULL, NULL);
> > +
> > +	for (i = 0; i < LAN966X_OIC_NR_IRQ; i++)
> > +		irq_dispose_mapping(irq_find_mapping(lan966x_oic->domain, i));  
> 
> This is just wrong. You cannot remove the chip when there are still interrupts
> mapped.
> 
> I just did a quick conversion to the template approach. Unsurprisingly
> it removes 30 lines of boiler plate code:
> 
> +static void lan966x_oic_chip_init(struct irq_chip_generic *gc)
> +{
> +	struct lan966x_oic_data *lan966x_oic = gc->domain->host_data;
> +	struct lan966x_oic_chip_regs *chip_regs;
> +
> +	gc->reg_base = lan966x_oic->regs;
> +
> +	chip_regs = lan966x_oic_chip_regs + gc->irq_base / 32;
> +	gc->chip_types[0].regs.enable = chip_regs->reg_off_ena_set;
> +	gc->chip_types[0].regs.disable = chip_regs->reg_off_ena_clr;
> +	gc->chip_types[0].regs.ack = chip_regs->reg_off_sticky;
> +
> +	gc->chip_types[0].chip.irq_startup = lan966x_oic_irq_startup;
> +	gc->chip_types[0].chip.irq_shutdown = lan966x_oic_irq_shutdown;
> +	gc->chip_types[0].chip.irq_set_type = lan966x_oic_irq_set_type;
> +	gc->chip_types[0].chip.irq_mask = irq_gc_mask_disable_reg;
> +	gc->chip_types[0].chip.irq_unmask = irq_gc_unmask_enable_reg;
> +	gc->chip_types[0].chip.irq_ack = irq_gc_ack_set_bit;
> +	gc->private = chip_regs;
> +
> +	/* Disable all interrupts handled by this chip */
> +	irq_reg_writel(gc, ~0, chip_regs->reg_off_ena_clr);
> +}
> +
> +static void lan966x_oic_chip_exit(struct irq_chip_generic *gc)
> +{
> +	/* Disable and ack all interrupts handled by this chip */
> +	irq_reg_writel(gc, ~0, gc->chip_types[0].regs.disable);
> +	irq_reg_writel(gc, ~0, gc->chip_types[0].regs.ack);
> +}
> +
> +static void lan966x_oic_domain_init(struct irq_domain *d)
> +{
> +	struct lan966x_oic_data *lan966x_oic = d->host_data;
> +
> +	irq_set_chained_handler_and_data(lan966x_oic->irq, lan966x_oic_irq_handler, d);
> +}
> +
> +static int lan966x_oic_probe(struct platform_device *pdev)
> +{
> +	struct irq_domain_chip_generic_info gc_info = {
> +		.irqs_per_chip		= 32,
> +		.num_chips		= 1,
> +		.name			= "lan966x-oic"
> +		.handler		= handle_level_irq,
> +		.init			= lan966x_oic_chip_init,
> +		.destroy		= lan966x_oic_chip_exit,
> +	};
> +
> +	struct irq_domain_info info = {
> +		.fwnode			= of_node_to_fwnode(pdev->dev.of_node),
> +		.size			= LAN966X_OIC_NR_IRQ,
> +		.hwirq_max		= LAN966X_OIC_NR_IRQ,
> +		.ops			= &irq_generic_chip_ops,
> +		.gc_info		= &gc_info,
> +		.init			= lan966x_oic_domain_init,
> +	};
> +	struct lan966x_oic_data *lan966x_oic;
> +	struct device *dev = &pdev->dev;
> +
> +	lan966x_oic = devm_kmalloc(dev, sizeof(*lan966x_oic), GFP_KERNEL);
> +	if (!lan966x_oic)
> +		return -ENOMEM;
> +
> +	lan966x_oic->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(lan966x_oic->regs))
> +		return dev_err_probe(dev, PTR_ERR(lan966x_oic->regs), "failed to map resource\n");
> +
> +	lan966x_oic->irq = platform_get_irq(pdev, 0);
> +	if (lan966x_oic->irq < 0)
> +		return dev_err_probe(dev, lan966x_oic->irq, "failed to get the IRQ\n");
> +
> +	lan966x_oic->domain = irq_domain_instantiate(&info);
> +	if (!lan966x_oic->domain)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, lan966x_oic);
> +	return 0;
> +}
> +
> +static void lan966x_oic_remove(struct platform_device *pdev)
> +{
> +	struct lan966x_oic_data *lan966x_oic = platform_get_drvdata(pdev);
> +
> +	irq_set_chained_handler_and_data(lan966x_oic->irq, NULL, NULL);
> +	irq_domain_remove(lan966x_oic->domain);
> +}
> 
> See?

Perfectly.
I will rework patches in this way.
Again, thanks for pointing out this solution.

Best regards,
Hervé

> 
> Thanks,
> 
>         tglx



-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Wed, Jun 05, 2024 at 04:17:53PM +0200, Thomas Gleixner kirjoitti:
> On Mon, May 27 2024 at 18:14, Herve Codina wrote:

...

> > +	irq_reg_writel(gc, ~0, gc->chip_types[0].regs.disable);
> 
> ~0U
>   
> > +	irq_reg_writel(gc, ~0, gc->chip_types[0].regs.ack);

...

Below just to annoy people a bit :)

(Yes, I understand that this is a prototype, it's just a pre-review in case one
want to blindly copy'n'paste it).

Other than that, I like the result!

> I just did a quick conversion to the template approach. Unsurprisingly
> it removes 30 lines of boiler plate code:
> 
> +static void lan966x_oic_chip_init(struct irq_chip_generic *gc)
> +{
> +	struct lan966x_oic_data *lan966x_oic = gc->domain->host_data;
> +	struct lan966x_oic_chip_regs *chip_regs;
> +
> +	gc->reg_base = lan966x_oic->regs;
> +
> +	chip_regs = lan966x_oic_chip_regs + gc->irq_base / 32;
> +	gc->chip_types[0].regs.enable = chip_regs->reg_off_ena_set;
> +	gc->chip_types[0].regs.disable = chip_regs->reg_off_ena_clr;
> +	gc->chip_types[0].regs.ack = chip_regs->reg_off_sticky;
> +
> +	gc->chip_types[0].chip.irq_startup = lan966x_oic_irq_startup;
> +	gc->chip_types[0].chip.irq_shutdown = lan966x_oic_irq_shutdown;
> +	gc->chip_types[0].chip.irq_set_type = lan966x_oic_irq_set_type;
> +	gc->chip_types[0].chip.irq_mask = irq_gc_mask_disable_reg;
> +	gc->chip_types[0].chip.irq_unmask = irq_gc_unmask_enable_reg;
> +	gc->chip_types[0].chip.irq_ack = irq_gc_ack_set_bit;
> +	gc->private = chip_regs;
> +
> +	/* Disable all interrupts handled by this chip */
> +	irq_reg_writel(gc, ~0, chip_regs->reg_off_ena_clr);
> +}
> +
> +static void lan966x_oic_chip_exit(struct irq_chip_generic *gc)
> +{
> +	/* Disable and ack all interrupts handled by this chip */
> +	irq_reg_writel(gc, ~0, gc->chip_types[0].regs.disable);
> +	irq_reg_writel(gc, ~0, gc->chip_types[0].regs.ack);

~0U :-)

But I, for example, think that GENMASK() even better as it shows exactly what
bits we set for the HW writes.

> +}
> +
> +static void lan966x_oic_domain_init(struct irq_domain *d)
> +{
> +	struct lan966x_oic_data *lan966x_oic = d->host_data;
> +
> +	irq_set_chained_handler_and_data(lan966x_oic->irq, lan966x_oic_irq_handler, d);
> +}
> +
> +static int lan966x_oic_probe(struct platform_device *pdev)
> +{
> +	struct irq_domain_chip_generic_info gc_info = {
> +		.irqs_per_chip		= 32,
> +		.num_chips		= 1,
> +		.name			= "lan966x-oic"
> +		.handler		= handle_level_irq,
> +		.init			= lan966x_oic_chip_init,
> +		.destroy		= lan966x_oic_chip_exit,
> +	};
> +
> +	struct irq_domain_info info = {

> +		.fwnode			= of_node_to_fwnode(pdev->dev.of_node),

It's as simple as dev_fwnode()

> +		.size			= LAN966X_OIC_NR_IRQ,
> +		.hwirq_max		= LAN966X_OIC_NR_IRQ,
> +		.ops			= &irq_generic_chip_ops,
> +		.gc_info		= &gc_info,
> +		.init			= lan966x_oic_domain_init,
> +	};
> +	struct lan966x_oic_data *lan966x_oic;
> +	struct device *dev = &pdev->dev;
> +
> +	lan966x_oic = devm_kmalloc(dev, sizeof(*lan966x_oic), GFP_KERNEL);
> +	if (!lan966x_oic)
> +		return -ENOMEM;
> +
> +	lan966x_oic->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(lan966x_oic->regs))
> +		return dev_err_probe(dev, PTR_ERR(lan966x_oic->regs), "failed to map resource\n");
> +
> +	lan966x_oic->irq = platform_get_irq(pdev, 0);
> +	if (lan966x_oic->irq < 0)
> +		return dev_err_probe(dev, lan966x_oic->irq, "failed to get the IRQ\n");
> +
> +	lan966x_oic->domain = irq_domain_instantiate(&info);
> +	if (!lan966x_oic->domain)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, lan966x_oic);
> +	return 0;
> +}
> +
> +static void lan966x_oic_remove(struct platform_device *pdev)
> +{
> +	struct lan966x_oic_data *lan966x_oic = platform_get_drvdata(pdev);
> +
> +	irq_set_chained_handler_and_data(lan966x_oic->irq, NULL, NULL);
> +	irq_domain_remove(lan966x_oic->domain);
> +}

-- 
With Best Regards,
Andy Shevchenko