* [PATCH v2] irqchip: add TS7800v1 fpga based controller driver
@ 2022-10-20 14:13 Firas Ashkar
2022-10-21 9:22 ` Marc Zyngier
0 siblings, 1 reply; 7+ messages in thread
From: Firas Ashkar @ 2022-10-20 14:13 UTC (permalink / raw)
To: alex, tglx, maz; +Cc: linux-kernel, linux-arm-kernel, Firas Ashkar
1. add TS-7800v1 fpga based irq controller driver, and
2. add related memory and irq resources
By default only mapped FPGA interrupts will be chained/multiplexed, these
chained interrupts will then be available to other device drivers to
request via the corresponding Linux IRQs.
$ cat /proc/cpuinfo
processor : 0
model name : Feroceon rev 0 (v5l)
BogoMIPS : 333.33
Features : swp half thumb fastmult edsp
CPU implementer : 0x41
CPU architecture: 5TEJ
CPU variant : 0x0
CPU part : 0x926
CPU revision : 0
Hardware : Technologic Systems TS-78xx SBC
Revision : 0000
Serial : 0000000000000000
$
$ cat /proc/interrupts
CPU0
1: 902 orion_irq Level orion_tick
4: 795 orion_irq Level ttyS0
13: 0 orion_irq Level ehci_hcd:usb2
18: 0 orion_irq Level ehci_hcd:usb1
22: 69 orion_irq Level eth0
23: 171 orion_irq Level orion-mdio
29: 0 orion_irq Level mv_crypto
31: 2 orion_irq Level mv_xor.0
65: 1 ts7800-irqc 0 Edge ts-dmac-cpupci
Err: 0
$
$ uname -a
Linux ts-7800 6.1.0-rc1 #2 PREEMPT Mon Oct 17 11:19:12 EDT 2022 armv5tel
GNU/Linux
$
Signed-off-by: Firas Ashkar <firas.ashkar@savoirfairelinux.com>
---
Notes:
Changes in V2
* limit the commit message
* format comments in source code
* use raw spin locks to protect mask/unmask ops
* use 'handle_edge_irq' and 'irq_ack' logic
* remove 'irq_domain_xlate_onecell'
* remove unnecessary status flags
* use 'builtin_platform_driver' helper routine
:100644 100644 2f4fe3ca5c1a ed8378893208 M arch/arm/mach-orion5x/ts78xx-fpga.h
:100644 100644 af810e7ccd79 d319a68b7160 M arch/arm/mach-orion5x/ts78xx-setup.c
:100644 100644 7ef9f5e696d3 d184fb435c5d M drivers/irqchip/Kconfig
:100644 100644 87b49a10962c b022eece2042 M drivers/irqchip/Makefile
:000000 100644 000000000000 e975607fa4d5 A drivers/irqchip/irq-ts7800.c
arch/arm/mach-orion5x/ts78xx-fpga.h | 1 +
arch/arm/mach-orion5x/ts78xx-setup.c | 55 +++++
drivers/irqchip/Kconfig | 12 +
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-ts7800.c | 347 +++++++++++++++++++++++++++
5 files changed, 416 insertions(+)
diff --git a/arch/arm/mach-orion5x/ts78xx-fpga.h b/arch/arm/mach-orion5x/ts78xx-fpga.h
index 2f4fe3ca5c1a..ed8378893208 100644
--- a/arch/arm/mach-orion5x/ts78xx-fpga.h
+++ b/arch/arm/mach-orion5x/ts78xx-fpga.h
@@ -32,6 +32,7 @@ struct fpga_devices {
struct fpga_device ts_rtc;
struct fpga_device ts_nand;
struct fpga_device ts_rng;
+ struct fpga_device ts_irqc;
};
struct ts78xx_fpga_data {
diff --git a/arch/arm/mach-orion5x/ts78xx-setup.c b/arch/arm/mach-orion5x/ts78xx-setup.c
index af810e7ccd79..d319a68b7160 100644
--- a/arch/arm/mach-orion5x/ts78xx-setup.c
+++ b/arch/arm/mach-orion5x/ts78xx-setup.c
@@ -322,6 +322,49 @@ static void ts78xx_ts_rng_unload(void)
platform_device_del(&ts78xx_ts_rng_device);
}
+/*****************************************************************************
+ * fpga irq controller
+ ****************************************************************************/
+#define TS_IRQC_CTRL (TS78XX_FPGA_REGS_PHYS_BASE + 0x200)
+#define TS_BRIDGE_CTRL (ORION5X_REGS_PHYS_BASE + 0x20400)
+#define TS_IRQC_PARENT 0x2
+static struct resource ts_irqc_resources[] = {
+ DEFINE_RES_MEM_NAMED(TS_IRQC_CTRL, 0x100, "ts_irqc"),
+ DEFINE_RES_MEM_NAMED(TS_BRIDGE_CTRL, 0x10, "ts_bridge"),
+ DEFINE_RES_IRQ_NAMED(TS_IRQC_PARENT, "ts_irqc_parent"),
+};
+
+static struct platform_device ts_irqc_device = {
+ .name = "ts7800-irqc",
+ .id = -1,
+ .resource = ts_irqc_resources,
+ .num_resources = ARRAY_SIZE(ts_irqc_resources),
+};
+
+static int ts_irqc_load(void)
+{
+ int rc;
+
+ if (ts78xx_fpga.supports.ts_irqc.init == 0) {
+ rc = platform_device_register(&ts_irqc_device);
+ if (!rc)
+ ts78xx_fpga.supports.ts_irqc.init = 1;
+ } else {
+ rc = platform_device_add(&ts_irqc_device);
+ }
+
+ if (rc)
+ pr_info("FPGA based IRQ controller could not be registered: %d\n",
+ rc);
+
+ return rc;
+}
+
+static void ts_irqc_unload(void)
+{
+ platform_device_del(&ts_irqc_device);
+}
+
/*****************************************************************************
* FPGA 'hotplug' support code
****************************************************************************/
@@ -330,6 +373,7 @@ static void ts78xx_fpga_devices_zero_init(void)
ts78xx_fpga.supports.ts_rtc.init = 0;
ts78xx_fpga.supports.ts_nand.init = 0;
ts78xx_fpga.supports.ts_rng.init = 0;
+ ts78xx_fpga.supports.ts_irqc.init = 0;
}
static void ts78xx_fpga_supports(void)
@@ -348,6 +392,7 @@ static void ts78xx_fpga_supports(void)
ts78xx_fpga.supports.ts_rtc.present = 1;
ts78xx_fpga.supports.ts_nand.present = 1;
ts78xx_fpga.supports.ts_rng.present = 1;
+ ts78xx_fpga.supports.ts_irqc.present = 1;
break;
default:
/* enable devices if magic matches */
@@ -358,11 +403,13 @@ static void ts78xx_fpga_supports(void)
ts78xx_fpga.supports.ts_rtc.present = 1;
ts78xx_fpga.supports.ts_nand.present = 1;
ts78xx_fpga.supports.ts_rng.present = 1;
+ ts78xx_fpga.supports.ts_irqc.present = 1;
break;
default:
ts78xx_fpga.supports.ts_rtc.present = 0;
ts78xx_fpga.supports.ts_nand.present = 0;
ts78xx_fpga.supports.ts_rng.present = 0;
+ ts78xx_fpga.supports.ts_irqc.present = 0;
}
}
}
@@ -371,6 +418,12 @@ static int ts78xx_fpga_load_devices(void)
{
int tmp, ret = 0;
+ if (ts78xx_fpga.supports.ts_irqc.present == 1) {
+ tmp = ts_irqc_load();
+ if (tmp)
+ ts78xx_fpga.supports.ts_irqc.present = 0;
+ ret |= tmp;
+ }
if (ts78xx_fpga.supports.ts_rtc.present == 1) {
tmp = ts78xx_ts_rtc_load();
if (tmp)
@@ -402,6 +455,8 @@ static int ts78xx_fpga_unload_devices(void)
ts78xx_ts_nand_unload();
if (ts78xx_fpga.supports.ts_rng.present == 1)
ts78xx_ts_rng_unload();
+ if (ts78xx_fpga.supports.ts_irqc.present == 1)
+ ts_irqc_unload();
return 0;
}
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 7ef9f5e696d3..d184fb435c5d 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -290,6 +290,18 @@ config TS4800_IRQ
help
Support for the TS-4800 FPGA IRQ controller
+config TS7800_IRQ
+ tristate "TS-7800 IRQ controller"
+ select IRQ_DOMAIN
+ depends on HAS_IOMEM
+ depends on MACH_TS78XX
+ help
+ Support for TS-7800 FPGA based IRQ controller.
+ This IRQ controller acts as a multiplexer chaining mapped
+ FPGA interrupts to a single system bridge interrupt.
+
+ If you have an FPGA IP corresponding to this driver, say Y or M here.
+
config VERSATILE_FPGA_IRQ
bool
select IRQ_DOMAIN
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 87b49a10962c..b022eece2042 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -58,6 +58,7 @@ obj-$(CONFIG_ARCH_VT8500) += irq-vt8500.o
obj-$(CONFIG_ST_IRQCHIP) += irq-st.o
obj-$(CONFIG_TB10X_IRQC) += irq-tb10x.o
obj-$(CONFIG_TS4800_IRQ) += irq-ts4800.o
+obj-$(CONFIG_TS7800_IRQ) += irq-ts7800.o
obj-$(CONFIG_XTENSA) += irq-xtensa-pic.o
obj-$(CONFIG_XTENSA_MX) += irq-xtensa-mx.o
obj-$(CONFIG_XILINX_INTC) += irq-xilinx-intc.o
diff --git a/drivers/irqchip/irq-ts7800.c b/drivers/irqchip/irq-ts7800.c
new file mode 100644
index 000000000000..e975607fa4d5
--- /dev/null
+++ b/drivers/irqchip/irq-ts7800.c
@@ -0,0 +1,347 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * FPGA based IRQ contorller driver for TS-7800v1
+ *
+ * Copyright (C) 2022 Savoir-faire Linux Inc.
+ *
+ */
+
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+
+#define DRIVER_NAME "ts7800-irqc"
+
+#define IRQ_MASK_REG 0x4
+#define IRQ_STATUS_REG 0x8
+#define TS7800_IRQ_NUM 0x20
+
+/*
+ * FPGA IRQ Controller, list of all mappable/chainable interrupts
+ *
+ * IRQ Description
+ * --- -----------
+ * 0x0 dma_cpu_pci_dack_o
+ * 0x1 dma_fpga_dack_o
+ * 0x2 SD Busy#
+ * 0x3 isa_irq3
+ * 0x4 isa_irq4
+ * 0x5 isa_irq5
+ * 0x6 isa_irq6
+ * 0x7 isa_irq7
+ * 0x8 Reserved
+ * 0x9 isa_irq9
+ * 0xa isa_irq10
+ * 0xb isa_irq11
+ * 0xc isa_irq12
+ * 0xd isa_irq14
+ * 0xe isa_irq15
+ * 0x10:0x19 tsuart_irqs
+ *
+ * To map or enable a specific FPGA interrupt, add its corresponding number to
+ * 'enabled_mappings'.
+ * Example:
+ * 1. For 'dma_cpu_pci_dack_o' (FPGA based DMA controller), add 0x0 to
+ * 'enabled_mappings'
+ * 2. For 'tsuart_irqs' (FPGA based UARTs), add their FPGA interrupt range of
+ * 0x10-0x19 to 'enabled_mappings'
+ *
+ * By default only mapped/enabled interrupts will be chained/multiplexed,
+ * these chained interrupts will then be available to other device drivers to
+ * request via the corresponding Linux IRQs.
+ *
+ * Each mapped FPGA interrupt will have a corresponding Linux IRQ, this new IRQ
+ * will be appended to the system's current list of Linux IRQs, on TS78xx, the
+ * first mapped FPGA interrupt will have a corresponding new Linux IRQ starting
+ * at 65. The next mapped FPGA interrupt will have a Linux IRQ of 66 and so
+ * forth.
+ *
+ * Example-1:
+ * In order for a device driver, say the FPGA based DMA controller driver
+ * 'TS-DMAC', to use either of the corresponding FPGA interrupts
+ * 'dma_cpu_pci_dack_o' and 'dma_fpga_dack_o', the 'TS-DMAC' driver has to:
+ *
+ * 1. add FPGA interrupt numbers 0x0 and 0x1 to 'enabled_mappings', in this
+ * file, and
+ * 2. request the corresponding Linux IRQ numbers 65 and 66 respectively in its
+ * implementation.
+ *
+ * Eample-2:
+ * In order for another device driver, say the FPGA based 'TSUART' driver, to
+ * use the related FPGA interrupts, the 'TSUART' driver has to:
+ *
+ * 1. append FPGA interrupt numbers 0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16,
+ * 0x17, 0x18, 0x19 to 'enabled_mappings', in this file, and
+ * 2. request the corresponding Linux IRQ numbers, these IRQs could start from
+ * 65 till 74, if no previous FPGA interrupts where mapped. However, if
+ * other FPGA interrupts, say those of 'TS-DMAC' in Example-1 were also
+ * mapped/enabled, then the correct corresponding Linux IRQs would start
+ * from 67 till 76.
+ */
+
+static irq_hw_number_t enabled_mappings[] = { 0x0, 0x1 };
+
+struct ts7800_irq_data {
+ int mpp7_virq;
+ raw_spinlock_t lock;
+ void __iomem *base;
+ void __iomem *bridge;
+ struct irq_domain *domain;
+ struct irq_chip irq_chip;
+};
+
+static void ts7800_irq_mask(struct irq_data *d)
+{
+ struct ts7800_irq_data *data = irq_data_get_irq_chip_data(d);
+ u32 fpga_mask_reg = readl(data->base + IRQ_MASK_REG);
+ u32 mask_bits = 1 << d->hwirq;
+ unsigned long flags;
+
+ raw_spin_lock_irqsave(&data->lock, flags);
+ writel(fpga_mask_reg & ~mask_bits, data->base + IRQ_MASK_REG);
+ writel(fpga_mask_reg & ~mask_bits, data->bridge + IRQ_MASK_REG);
+ raw_spin_unlock_irqrestore(&data->lock, flags);
+}
+
+static void ts7800_irq_unmask(struct irq_data *d)
+{
+ struct ts7800_irq_data *data = irq_data_get_irq_chip_data(d);
+ u32 fpga_mask_reg = readl(data->base + IRQ_MASK_REG);
+ u32 mask_bits = 1 << d->hwirq;
+ unsigned long flags;
+
+ raw_spin_lock_irqsave(&data->lock, flags);
+ writel(fpga_mask_reg | mask_bits, data->base + IRQ_MASK_REG);
+ writel(fpga_mask_reg | mask_bits, data->bridge + IRQ_MASK_REG);
+ raw_spin_unlock_irqrestore(&data->lock, flags);
+}
+
+static void ts7800_irq_ack(struct irq_data *d)
+{
+ struct ts7800_irq_data *data = irq_data_get_irq_chip_data(d);
+ u32 status = readl(data->bridge);
+ u32 mask_bits = 1 << d->hwirq;
+ unsigned int bit = __ffs(mask_bits);
+ unsigned long flags;
+
+ status &= ~(1 << bit);
+
+ raw_spin_lock_irqsave(&data->lock, flags);
+ writel(status, data->bridge);
+ raw_spin_unlock_irqrestore(&data->lock, flags);
+}
+
+static int ts7800_irqdomain_map(struct irq_domain *d, unsigned int irq,
+ irq_hw_number_t hwirq)
+{
+ struct ts7800_irq_data *data = d->host_data;
+
+ irq_set_chip_and_handler(irq, &data->irq_chip, handle_edge_irq);
+ irq_clear_status_flags(irq, IRQ_LEVEL);
+ irq_set_chip_data(irq, data);
+ irq_set_noprobe(irq);
+
+ return 0;
+}
+
+static void ts7800_irqdomain_unmap(struct irq_domain *d, unsigned int irq)
+{
+ irq_set_chip_and_handler(irq, NULL, NULL);
+ irq_set_chip_data(irq, NULL);
+}
+
+static const struct irq_domain_ops ts7800_ic_ops = {
+ .map = ts7800_irqdomain_map,
+ .unmap = ts7800_irqdomain_unmap
+};
+
+static void ts7800_ic_chained_handle_irq(struct irq_desc *desc)
+{
+ struct ts7800_irq_data *data = irq_desc_get_handler_data(desc);
+ struct irq_chip *chip = irq_desc_get_chip(desc);
+ u32 mask_bits = readl(data->base + IRQ_MASK_REG);
+ u32 status = readl(data->bridge);
+
+ chained_irq_enter(chip, desc);
+
+ if (unlikely(status == 0)) {
+ handle_bad_irq(desc);
+ goto out;
+ }
+
+ do {
+ unsigned int bit = __ffs(status);
+ int irq = irq_find_mapping(data->domain, bit);
+
+ if (irq && (mask_bits & BIT(bit)))
+ generic_handle_irq(irq);
+
+ status &= ~(1 << bit);
+
+ } while (status);
+
+out:
+ chained_irq_exit(chip, desc);
+}
+
+static int ts7800_ic_probe(struct platform_device *pdev)
+{
+ struct ts7800_irq_data *data = NULL;
+ struct resource *mem_res = NULL, *brdg_res = NULL, *irq_res = NULL;
+ unsigned int irqdomain;
+ int i, ret;
+
+ data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+ if (IS_ERR_OR_NULL(data)) {
+ dev_err(&pdev->dev,
+ "Failed to allocate TS7800 data, error %ld\n",
+ PTR_ERR(data));
+ ret = PTR_ERR(data);
+ goto devm_kzalloc_err;
+ }
+
+ mem_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ts_irqc");
+ if (IS_ERR_OR_NULL(mem_res)) {
+ dev_err(&pdev->dev,
+ "Failed to get platform memory resource, error %ld\n",
+ PTR_ERR(mem_res));
+ ret = PTR_ERR(mem_res);
+ goto pltfrm_get_res_mem_err;
+ }
+
+ data->base = devm_ioremap_resource(&pdev->dev, mem_res);
+ if (IS_ERR_OR_NULL(data->base)) {
+ dev_err(&pdev->dev,
+ "Failed to IO map mem-region %s, error %ld\n",
+ mem_res->name, PTR_ERR(data->base));
+ ret = PTR_ERR(data->base);
+ goto devm_ioremap_res_mem_err;
+ }
+
+ brdg_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ts_bridge");
+ if (IS_ERR_OR_NULL(brdg_res)) {
+ dev_err(&pdev->dev,
+ "Failed to get platform bridge resource, error %ld\n",
+ PTR_ERR(brdg_res));
+ ret = PTR_ERR(brdg_res);
+ goto pltfrm_get_res_brdg_err;
+ }
+
+ data->bridge = devm_ioremap_resource(&pdev->dev, brdg_res);
+ if (IS_ERR_OR_NULL(data->bridge)) {
+ dev_err(&pdev->dev,
+ "Failed to IO map bridge-region %s, error %ld\n",
+ mem_res->name, PTR_ERR(data->bridge));
+ ret = PTR_ERR(data->bridge);
+ goto devm_ioremap_res_brdge_err;
+ }
+
+ irq_res = platform_get_resource_byname(pdev, IORESOURCE_IRQ,
+ "ts_irqc_parent");
+ if (IS_ERR_OR_NULL(irq_res)) {
+ dev_err(&pdev->dev,
+ "Failed to get platform parent irq resource, error %ld\n",
+ PTR_ERR(irq_res));
+ ret = PTR_ERR(irq_res);
+ goto pltfrm_get_res_irq_err;
+ }
+
+ raw_spin_lock_init(&data->lock);
+ data->mpp7_virq = irq_res->start;
+ data->irq_chip.name = dev_name(&pdev->dev);
+ data->irq_chip.irq_mask = ts7800_irq_mask;
+ data->irq_chip.irq_unmask = ts7800_irq_unmask;
+ data->irq_chip.irq_ack = ts7800_irq_ack;
+
+ writel(0x0, data->bridge);
+ writel(0x0, data->bridge + IRQ_MASK_REG);
+ writel(0x0, data->base + IRQ_MASK_REG);
+
+ data->domain = irq_domain_add_linear(pdev->dev.of_node, TS7800_IRQ_NUM,
+ &ts7800_ic_ops, data);
+ if (IS_ERR_OR_NULL(data->domain)) {
+ dev_err(&pdev->dev, "cannot add IRQ domain\n");
+ ret = PTR_ERR(data->domain);
+ goto irq_domain_add_linear_err;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(enabled_mappings); ++i) {
+ irqdomain =
+ irq_create_mapping(data->domain, enabled_mappings[i]);
+ }
+
+ irq_set_chained_handler_and_data(data->mpp7_virq,
+ ts7800_ic_chained_handle_irq, data);
+
+ platform_set_drvdata(pdev, data);
+
+ return 0;
+
+irq_domain_add_linear_err:
+pltfrm_get_res_irq_err:
+ devm_iounmap(&pdev->dev, data->bridge);
+
+devm_ioremap_res_brdge_err:
+pltfrm_get_res_brdg_err:
+ devm_iounmap(&pdev->dev, data->base);
+
+devm_ioremap_res_mem_err:
+pltfrm_get_res_mem_err:
+ devm_kfree(&pdev->dev, data);
+
+devm_kzalloc_err:
+ return ret;
+}
+
+static int ts7800_ic_remove(struct platform_device *pdev)
+{
+ struct ts7800_irq_data *data = platform_get_drvdata(pdev);
+ int i;
+
+ if (!IS_ERR_OR_NULL(data)) {
+ irq_set_chained_handler_and_data(data->mpp7_virq, NULL, NULL);
+
+ for (i = 0; i < ARRAY_SIZE(enabled_mappings); ++i)
+ irq_dispose_mapping(irq_find_mapping(
+ data->domain, enabled_mappings[i]));
+
+ irq_domain_remove(data->domain);
+ }
+
+ return 0;
+}
+
+static const struct platform_device_id ts7800v1_ic_ids[] = {
+ {
+ .name = DRIVER_NAME,
+ },
+ {
+ /* sentinel */
+ }
+};
+
+MODULE_DEVICE_TABLE(platform, ts7800v1_ic_ids);
+
+static struct platform_driver ts7800_ic_driver = {
+ .probe = ts7800_ic_probe,
+ .remove = ts7800_ic_remove,
+ .id_table = ts7800v1_ic_ids,
+ .driver = {
+ .name = DRIVER_NAME,
+ .probe_type = PROBE_PREFER_ASYNCHRONOUS,
+ },
+};
+builtin_platform_driver(ts7800_ic_driver);
+
+MODULE_ALIAS("platform:ts7800-irqc");
+MODULE_DESCRIPTION("TS-7800v1 FPGA based IRQ controller Driver");
+MODULE_AUTHOR("Firas Ashkar <firas.ashkar@savoirfairelinux.com>");
+MODULE_LICENSE("GPL");
--
2.34.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] irqchip: add TS7800v1 fpga based controller driver
2022-10-20 14:13 [PATCH v2] irqchip: add TS7800v1 fpga based controller driver Firas Ashkar
@ 2022-10-21 9:22 ` Marc Zyngier
2022-10-21 16:19 ` firas ashkar
0 siblings, 1 reply; 7+ messages in thread
From: Marc Zyngier @ 2022-10-21 9:22 UTC (permalink / raw)
To: Firas Ashkar; +Cc: alex, tglx, linux-kernel, linux-arm-kernel
On Thu, 20 Oct 2022 15:13:51 +0100,
Firas Ashkar <firas.ashkar@savoirfairelinux.com> wrote:
>
> 1. add TS-7800v1 fpga based irq controller driver, and
> 2. add related memory and irq resources
>
> By default only mapped FPGA interrupts will be chained/multiplexed, these
> chained interrupts will then be available to other device drivers to
> request via the corresponding Linux IRQs.
>
> $ cat /proc/cpuinfo
> processor : 0
> model name : Feroceon rev 0 (v5l)
> BogoMIPS : 333.33
> Features : swp half thumb fastmult edsp
> CPU implementer : 0x41
> CPU architecture: 5TEJ
> CPU variant : 0x0
> CPU part : 0x926
> CPU revision : 0
>
> Hardware : Technologic Systems TS-78xx SBC
> Revision : 0000
> Serial : 0000000000000000
> $
>
> $ cat /proc/interrupts
> CPU0
> 1: 902 orion_irq Level orion_tick
> 4: 795 orion_irq Level ttyS0
> 13: 0 orion_irq Level ehci_hcd:usb2
> 18: 0 orion_irq Level ehci_hcd:usb1
> 22: 69 orion_irq Level eth0
> 23: 171 orion_irq Level orion-mdio
> 29: 0 orion_irq Level mv_crypto
> 31: 2 orion_irq Level mv_xor.0
> 65: 1 ts7800-irqc 0 Edge ts-dmac-cpupci
> Err: 0
> $
>
> $ uname -a
> Linux ts-7800 6.1.0-rc1 #2 PREEMPT Mon Oct 17 11:19:12 EDT 2022 armv5tel
> GNU/Linux
> $
>
> Signed-off-by: Firas Ashkar <firas.ashkar@savoirfairelinux.com>
> ---
>
> Notes:
> Changes in V2
> * limit the commit message
Well, there is still a lot of work to do. Most of this commit message
could be reduced to a single paragraph:
"Add support for FooBar IRQ controller usually found on Zorglub
platform."
The rest is plain obvious.
> * format comments in source code
> * use raw spin locks to protect mask/unmask ops
> * use 'handle_edge_irq' and 'irq_ack' logic
> * remove 'irq_domain_xlate_onecell'
> * remove unnecessary status flags
> * use 'builtin_platform_driver' helper routine
>
> :100644 100644 2f4fe3ca5c1a ed8378893208 M arch/arm/mach-orion5x/ts78xx-fpga.h
> :100644 100644 af810e7ccd79 d319a68b7160 M arch/arm/mach-orion5x/ts78xx-setup.c
> :100644 100644 7ef9f5e696d3 d184fb435c5d M drivers/irqchip/Kconfig
> :100644 100644 87b49a10962c b022eece2042 M drivers/irqchip/Makefile
> :000000 100644 000000000000 e975607fa4d5 A drivers/irqchip/irq-ts7800.c
> arch/arm/mach-orion5x/ts78xx-fpga.h | 1 +
> arch/arm/mach-orion5x/ts78xx-setup.c | 55 +++++
> drivers/irqchip/Kconfig | 12 +
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-ts7800.c | 347 +++++++++++++++++++++++++++
> 5 files changed, 416 insertions(+)
>
> diff --git a/arch/arm/mach-orion5x/ts78xx-fpga.h b/arch/arm/mach-orion5x/ts78xx-fpga.h
> index 2f4fe3ca5c1a..ed8378893208 100644
> --- a/arch/arm/mach-orion5x/ts78xx-fpga.h
> +++ b/arch/arm/mach-orion5x/ts78xx-fpga.h
> @@ -32,6 +32,7 @@ struct fpga_devices {
> struct fpga_device ts_rtc;
> struct fpga_device ts_nand;
> struct fpga_device ts_rng;
> + struct fpga_device ts_irqc;
> };
>
> struct ts78xx_fpga_data {
> diff --git a/arch/arm/mach-orion5x/ts78xx-setup.c b/arch/arm/mach-orion5x/ts78xx-setup.c
> index af810e7ccd79..d319a68b7160 100644
> --- a/arch/arm/mach-orion5x/ts78xx-setup.c
> +++ b/arch/arm/mach-orion5x/ts78xx-setup.c
> @@ -322,6 +322,49 @@ static void ts78xx_ts_rng_unload(void)
> platform_device_del(&ts78xx_ts_rng_device);
> }
>
> +/*****************************************************************************
> + * fpga irq controller
> + ****************************************************************************/
[...]
Sorry, but I have to ask: what part of "we're not taking any
additional non-DT changes to these obsolete setups" did I fail to
accurately communicate?
Until this board is entirely converted to DT, I'm not taking any
irqchip changes other than the most obvious bug fixes.
[...]
> +static void ts7800_irq_mask(struct irq_data *d)
> +{
> + struct ts7800_irq_data *data = irq_data_get_irq_chip_data(d);
> + u32 fpga_mask_reg = readl(data->base + IRQ_MASK_REG);
> + u32 mask_bits = 1 << d->hwirq;
> + unsigned long flags;
> +
> + raw_spin_lock_irqsave(&data->lock, flags);
> + writel(fpga_mask_reg & ~mask_bits, data->base + IRQ_MASK_REG);
> + writel(fpga_mask_reg & ~mask_bits, data->bridge + IRQ_MASK_REG);
> + raw_spin_unlock_irqrestore(&data->lock, flags);
OMFG. What do you expect this protects? Same question applies to all
the instances where you take this pointless lock.
[...]
> +static void ts7800_ic_chained_handle_irq(struct irq_desc *desc)
> +{
> + struct ts7800_irq_data *data = irq_desc_get_handler_data(desc);
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> + u32 mask_bits = readl(data->base + IRQ_MASK_REG);
> + u32 status = readl(data->bridge);
> +
> + chained_irq_enter(chip, desc);
> +
> + if (unlikely(status == 0)) {
> + handle_bad_irq(desc);
> + goto out;
> + }
> +
> + do {
> + unsigned int bit = __ffs(status);
> + int irq = irq_find_mapping(data->domain, bit);
> +
> + if (irq && (mask_bits & BIT(bit)))
> + generic_handle_irq(irq);
Again, this is not appropriate. I've pointed you to the right API in
my previous review of this patch.
[...]
> +static struct platform_driver ts7800_ic_driver = {
> + .probe = ts7800_ic_probe,
> + .remove = ts7800_ic_remove,
> + .id_table = ts7800v1_ic_ids,
> + .driver = {
> + .name = DRIVER_NAME,
> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> + },
> +};
> +builtin_platform_driver(ts7800_ic_driver);
Again, this isn't appropriate either, and I pointed it out last
time. We have specific helpers for irqchip, and using them isn't
optional. But of course, you'll need to move to DT for that.
Anyway, this is the last time I review this patch until you convert
the corresponding platform to DT.
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] irqchip: add TS7800v1 fpga based controller driver
2022-10-21 9:22 ` Marc Zyngier
@ 2022-10-21 16:19 ` firas ashkar
2022-10-21 17:18 ` Marc Zyngier
0 siblings, 1 reply; 7+ messages in thread
From: firas ashkar @ 2022-10-21 16:19 UTC (permalink / raw)
To: Marc Zyngier; +Cc: alex, tglx, linux-kernel, linux-arm-kernel
On Fri, 2022-10-21 at 10:22 +0100, Marc Zyngier wrote:
> On Thu, 20 Oct 2022 15:13:51 +0100,
> Firas Ashkar <firas.ashkar@savoirfairelinux.com> wrote:
> >
> > 1. add TS-7800v1 fpga based irq controller driver, and
> > 2. add related memory and irq resources
> >
> > By default only mapped FPGA interrupts will be chained/multiplexed,
> > these
> > chained interrupts will then be available to other device drivers
> > to
> > request via the corresponding Linux IRQs.
> >
> > $ cat /proc/cpuinfo
> > processor : 0
> > model name : Feroceon rev 0 (v5l)
> > BogoMIPS : 333.33
> > Features : swp half thumb fastmult edsp
> > CPU implementer : 0x41
> > CPU architecture: 5TEJ
> > CPU variant : 0x0
> > CPU part : 0x926
> > CPU revision : 0
> >
> > Hardware : Technologic Systems TS-78xx SBC
> > Revision : 0000
> > Serial : 0000000000000000
> > $
> >
> > $ cat /proc/interrupts
> > CPU0
> > 1: 902 orion_irq Level orion_tick
> > 4: 795 orion_irq Level ttyS0
> > 13: 0 orion_irq Level ehci_hcd:usb2
> > 18: 0 orion_irq Level ehci_hcd:usb1
> > 22: 69 orion_irq Level eth0
> > 23: 171 orion_irq Level orion-mdio
> > 29: 0 orion_irq Level mv_crypto
> > 31: 2 orion_irq Level mv_xor.0
> > 65: 1 ts7800-irqc 0 Edge ts-dmac-cpupci
> > Err: 0
> > $
> >
> > $ uname -a
> > Linux ts-7800 6.1.0-rc1 #2 PREEMPT Mon Oct 17 11:19:12 EDT 2022
> > armv5tel
> > GNU/Linux
> > $
> >
> > Signed-off-by: Firas Ashkar <firas.ashkar@savoirfairelinux.com>
> > ---
> >
> > Notes:
> > Changes in V2
> > * limit the commit message
>
> Well, there is still a lot of work to do. Most of this commit message
> could be reduced to a single paragraph:
>
> "Add support for FooBar IRQ controller usually found on Zorglub
> platform."
>
> The rest is plain obvious.
nack, commit message is fine as is!
>
> > * format comments in source code
> > * use raw spin locks to protect mask/unmask ops
> > * use 'handle_edge_irq' and 'irq_ack' logic
> > * remove 'irq_domain_xlate_onecell'
> > * remove unnecessary status flags
> > * use 'builtin_platform_driver' helper routine
> >
> > :100644 100644 2f4fe3ca5c1a ed8378893208 M arch/arm/mach-
> > orion5x/ts78xx-fpga.h
> > :100644 100644 af810e7ccd79 d319a68b7160 M arch/arm/mach-
> > orion5x/ts78xx-setup.c
> > :100644 100644 7ef9f5e696d3 d184fb435c5d
> > M drivers/irqchip/Kconfig
> > :100644 100644 87b49a10962c b022eece2042
> > M drivers/irqchip/Makefile
> > :000000 100644 000000000000 e975607fa4d5
> > A drivers/irqchip/irq-ts7800.c
> > arch/arm/mach-orion5x/ts78xx-fpga.h | 1 +
> > arch/arm/mach-orion5x/ts78xx-setup.c | 55 +++++
> > drivers/irqchip/Kconfig | 12 +
> > drivers/irqchip/Makefile | 1 +
> > drivers/irqchip/irq-ts7800.c | 347
> > +++++++++++++++++++++++++++
> > 5 files changed, 416 insertions(+)
> >
> > diff --git a/arch/arm/mach-orion5x/ts78xx-fpga.h b/arch/arm/mach-
> > orion5x/ts78xx-fpga.h
> > index 2f4fe3ca5c1a..ed8378893208 100644
> > --- a/arch/arm/mach-orion5x/ts78xx-fpga.h
> > +++ b/arch/arm/mach-orion5x/ts78xx-fpga.h
> > @@ -32,6 +32,7 @@ struct fpga_devices {
> > struct fpga_device ts_rtc;
> > struct fpga_device ts_nand;
> > struct fpga_device ts_rng;
> > + struct fpga_device ts_irqc;
> > };
> >
> > struct ts78xx_fpga_data {
> > diff --git a/arch/arm/mach-orion5x/ts78xx-setup.c b/arch/arm/mach-
> > orion5x/ts78xx-setup.c
> > index af810e7ccd79..d319a68b7160 100644
> > --- a/arch/arm/mach-orion5x/ts78xx-setup.c
> > +++ b/arch/arm/mach-orion5x/ts78xx-setup.c
> > @@ -322,6 +322,49 @@ static void ts78xx_ts_rng_unload(void)
> > platform_device_del(&ts78xx_ts_rng_device);
> > }
> >
> > +/*****************************************************************
> > ************
> > + * fpga irq controller
> > +
> > *******************************************************************
> > *********/
>
> [...]
>
> Sorry, but I have to ask: what part of "we're not taking any
> additional non-DT changes to these obsolete setups" did I fail to
> accurately communicate?
>
> Until this board is entirely converted to DT, I'm not taking any
> irqchip changes other than the most obvious bug fixes.
as long as this board is present in the kernel in its current legacy
form, this is a valid patch!
>
> [...]
>
> > +static void ts7800_irq_mask(struct irq_data *d)
> > +{
> > + struct ts7800_irq_data *data =
> > irq_data_get_irq_chip_data(d);
> > + u32 fpga_mask_reg = readl(data->base + IRQ_MASK_REG);
> > + u32 mask_bits = 1 << d->hwirq;
> > + unsigned long flags;
> > +
> > + raw_spin_lock_irqsave(&data->lock, flags);
> > + writel(fpga_mask_reg & ~mask_bits, data->base +
> > IRQ_MASK_REG);
> > + writel(fpga_mask_reg & ~mask_bits, data->bridge +
> > IRQ_MASK_REG);
> > + raw_spin_unlock_irqrestore(&data->lock, flags);
>
> OMFG. What do you expect this protects? Same question applies to all
> the instances where you take this pointless lock.
don't jump now
the locks added as per your previous comment, quoting:
"I know your HW is UP, but seeing these RMW sequences without a lock
makes me jump."
On this single CPU based arch TS78xx, locks are waste of cpu cycles,
also note that IRQs/preemption are/is already off in this context
maybe you meant adding locks as to promote general correct coding ?
or maybe it was like this previous nonsense comment, quoting :
"We don't remove interrupt controllers. What happens if someone still
had a mapping?"
>
> [...]
>
> > +static void ts7800_ic_chained_handle_irq(struct irq_desc *desc)
> > +{
> > + struct ts7800_irq_data *data =
> > irq_desc_get_handler_data(desc);
> > + struct irq_chip *chip = irq_desc_get_chip(desc);
> > + u32 mask_bits = readl(data->base + IRQ_MASK_REG);
> > + u32 status = readl(data->bridge);
> > +
> > + chained_irq_enter(chip, desc);
> > +
> > + if (unlikely(status == 0)) {
> > + handle_bad_irq(desc);
> > + goto out;
> > + }
> > +
> > + do {
> > + unsigned int bit = __ffs(status);
> > + int irq = irq_find_mapping(data->domain, bit);
> > +
> > + if (irq && (mask_bits & BIT(bit)))
> > + generic_handle_irq(irq);
>
> Again, this is not appropriate. I've pointed you to the right API in
> my previous review of this patch.
'generic_handle_domain_irq' causing some issues
>
> [...]
>
> > +static struct platform_driver ts7800_ic_driver = {
> > + .probe = ts7800_ic_probe,
> > + .remove = ts7800_ic_remove,
> > + .id_table = ts7800v1_ic_ids,
> > + .driver = {
> > + .name = DRIVER_NAME,
> > + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> > + },
> > +};
> > +builtin_platform_driver(ts7800_ic_driver);
>
> Again, this isn't appropriate either, and I pointed it out last
> time. We have specific helpers for irqchip, and using them isn't
> optional. But of course, you'll need to move to DT for that.
>
> Anyway, this is the last time I review this patch until you convert
> the corresponding platform to DT.
no problems, though have to note this is not constructive!
>
> M.
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] irqchip: add TS7800v1 fpga based controller driver
2022-10-21 16:19 ` firas ashkar
@ 2022-10-21 17:18 ` Marc Zyngier
2022-10-21 18:14 ` firas ashkar
0 siblings, 1 reply; 7+ messages in thread
From: Marc Zyngier @ 2022-10-21 17:18 UTC (permalink / raw)
To: firas ashkar; +Cc: alex, tglx, linux-kernel, linux-arm-kernel
On Fri, 21 Oct 2022 17:19:02 +0100,
firas ashkar <firas.ashkar@savoirfairelinux.com> wrote:
>
> On Fri, 2022-10-21 at 10:22 +0100, Marc Zyngier wrote:
> > On Thu, 20 Oct 2022 15:13:51 +0100,
> > Firas Ashkar <firas.ashkar@savoirfairelinux.com> wrote:
> > >
> > > 1. add TS-7800v1 fpga based irq controller driver, and
> > > 2. add related memory and irq resources
> > >
> > > By default only mapped FPGA interrupts will be chained/multiplexed,
> > > these
> > > chained interrupts will then be available to other device drivers
> > > to
> > > request via the corresponding Linux IRQs.
> > >
> > > $ cat /proc/cpuinfo
> > > processor : 0
> > > model name : Feroceon rev 0 (v5l)
> > > BogoMIPS : 333.33
> > > Features : swp half thumb fastmult edsp
> > > CPU implementer : 0x41
> > > CPU architecture: 5TEJ
> > > CPU variant : 0x0
> > > CPU part : 0x926
> > > CPU revision : 0
> > >
> > > Hardware : Technologic Systems TS-78xx SBC
> > > Revision : 0000
> > > Serial : 0000000000000000
> > > $
> > >
> > > $ cat /proc/interrupts
> > > CPU0
> > > 1: 902 orion_irq Level orion_tick
> > > 4: 795 orion_irq Level ttyS0
> > > 13: 0 orion_irq Level ehci_hcd:usb2
> > > 18: 0 orion_irq Level ehci_hcd:usb1
> > > 22: 69 orion_irq Level eth0
> > > 23: 171 orion_irq Level orion-mdio
> > > 29: 0 orion_irq Level mv_crypto
> > > 31: 2 orion_irq Level mv_xor.0
> > > 65: 1 ts7800-irqc 0 Edge ts-dmac-cpupci
> > > Err: 0
> > > $
> > >
> > > $ uname -a
> > > Linux ts-7800 6.1.0-rc1 #2 PREEMPT Mon Oct 17 11:19:12 EDT 2022
> > > armv5tel
> > > GNU/Linux
> > > $
> > >
> > > Signed-off-by: Firas Ashkar <firas.ashkar@savoirfairelinux.com>
> > > ---
> > >
> > > Notes:
> > > Changes in V2
> > > * limit the commit message
> >
> > Well, there is still a lot of work to do. Most of this commit message
> > could be reduced to a single paragraph:
> >
> > "Add support for FooBar IRQ controller usually found on Zorglub
> > platform."
> >
> > The rest is plain obvious.
> nack, commit message is fine as is!
Allow me to be the judge of that.
> >
> > > * format comments in source code
> > > * use raw spin locks to protect mask/unmask ops
> > > * use 'handle_edge_irq' and 'irq_ack' logic
> > > * remove 'irq_domain_xlate_onecell'
> > > * remove unnecessary status flags
> > > * use 'builtin_platform_driver' helper routine
> > >
> > > :100644 100644 2f4fe3ca5c1a ed8378893208 M arch/arm/mach-
> > > orion5x/ts78xx-fpga.h
> > > :100644 100644 af810e7ccd79 d319a68b7160 M arch/arm/mach-
> > > orion5x/ts78xx-setup.c
> > > :100644 100644 7ef9f5e696d3 d184fb435c5d
> > > M drivers/irqchip/Kconfig
> > > :100644 100644 87b49a10962c b022eece2042
> > > M drivers/irqchip/Makefile
> > > :000000 100644 000000000000 e975607fa4d5
> > > A drivers/irqchip/irq-ts7800.c
> > > arch/arm/mach-orion5x/ts78xx-fpga.h | 1 +
> > > arch/arm/mach-orion5x/ts78xx-setup.c | 55 +++++
> > > drivers/irqchip/Kconfig | 12 +
> > > drivers/irqchip/Makefile | 1 +
> > > drivers/irqchip/irq-ts7800.c | 347
> > > +++++++++++++++++++++++++++
> > > 5 files changed, 416 insertions(+)
> > >
> > > diff --git a/arch/arm/mach-orion5x/ts78xx-fpga.h b/arch/arm/mach-
> > > orion5x/ts78xx-fpga.h
> > > index 2f4fe3ca5c1a..ed8378893208 100644
> > > --- a/arch/arm/mach-orion5x/ts78xx-fpga.h
> > > +++ b/arch/arm/mach-orion5x/ts78xx-fpga.h
> > > @@ -32,6 +32,7 @@ struct fpga_devices {
> > > struct fpga_device ts_rtc;
> > > struct fpga_device ts_nand;
> > > struct fpga_device ts_rng;
> > > + struct fpga_device ts_irqc;
> > > };
> > >
> > > struct ts78xx_fpga_data {
> > > diff --git a/arch/arm/mach-orion5x/ts78xx-setup.c b/arch/arm/mach-
> > > orion5x/ts78xx-setup.c
> > > index af810e7ccd79..d319a68b7160 100644
> > > --- a/arch/arm/mach-orion5x/ts78xx-setup.c
> > > +++ b/arch/arm/mach-orion5x/ts78xx-setup.c
> > > @@ -322,6 +322,49 @@ static void ts78xx_ts_rng_unload(void)
> > > platform_device_del(&ts78xx_ts_rng_device);
> > > }
> > >
> > > +/*****************************************************************
> > > ************
> > > + * fpga irq controller
> > > +
> > > *******************************************************************
> > > *********/
> >
> > [...]
> >
> > Sorry, but I have to ask: what part of "we're not taking any
> > additional non-DT changes to these obsolete setups" did I fail to
> > accurately communicate?
> >
> > Until this board is entirely converted to DT, I'm not taking any
> > irqchip changes other than the most obvious bug fixes.
> as long as this board is present in the kernel in its current legacy
> form, this is a valid patch!
No. There is a cut of point. Code that we would taken 10 years ago
isn't necessarily valid anymore. We want to improve the kernel as a
whole, and not keep it in the past.
>
> >
> > [...]
> >
> > > +static void ts7800_irq_mask(struct irq_data *d)
> > > +{
> > > + struct ts7800_irq_data *data =
> > > irq_data_get_irq_chip_data(d);
> > > + u32 fpga_mask_reg = readl(data->base + IRQ_MASK_REG);
> > > + u32 mask_bits = 1 << d->hwirq;
> > > + unsigned long flags;
> > > +
> > > + raw_spin_lock_irqsave(&data->lock, flags);
> > > + writel(fpga_mask_reg & ~mask_bits, data->base +
> > > IRQ_MASK_REG);
> > > + writel(fpga_mask_reg & ~mask_bits, data->bridge +
> > > IRQ_MASK_REG);
> > > + raw_spin_unlock_irqrestore(&data->lock, flags);
> >
> > OMFG. What do you expect this protects? Same question applies to all
> > the instances where you take this pointless lock.
> don't jump now
> the locks added as per your previous comment, quoting:
> "I know your HW is UP, but seeing these RMW sequences without a lock
> makes me jump."
> On this single CPU based arch TS78xx, locks are waste of cpu cycles,
> also note that IRQs/preemption are/is already off in this context
>
> maybe you meant adding locks as to promote general correct coding ?
Let me spell it out for you: RMW means Read-Modify-Write. Putting a
lock around a *write only* serves zero purpose. It is just overhead,
and it is incorrect.
>
> or maybe it was like this previous nonsense comment, quoting :
> "We don't remove interrupt controllers. What happens if someone still
> had a mapping?"
And I stand by it.
>
>
> >
> > [...]
> >
> > > +static void ts7800_ic_chained_handle_irq(struct irq_desc *desc)
> > > +{
> > > + struct ts7800_irq_data *data =
> > > irq_desc_get_handler_data(desc);
> > > + struct irq_chip *chip = irq_desc_get_chip(desc);
> > > + u32 mask_bits = readl(data->base + IRQ_MASK_REG);
> > > + u32 status = readl(data->bridge);
> > > +
> > > + chained_irq_enter(chip, desc);
> > > +
> > > + if (unlikely(status == 0)) {
> > > + handle_bad_irq(desc);
> > > + goto out;
> > > + }
> > > +
> > > + do {
> > > + unsigned int bit = __ffs(status);
> > > + int irq = irq_find_mapping(data->domain, bit);
> > > +
> > > + if (irq && (mask_bits & BIT(bit)))
> > > + generic_handle_irq(irq);
> >
> > Again, this is not appropriate. I've pointed you to the right API in
> > my previous review of this patch.
> 'generic_handle_domain_irq' causing some issues
Which issue?
> >
> > [...]
> >
> > > +static struct platform_driver ts7800_ic_driver = {
> > > + .probe = ts7800_ic_probe,
> > > + .remove = ts7800_ic_remove,
> > > + .id_table = ts7800v1_ic_ids,
> > > + .driver = {
> > > + .name = DRIVER_NAME,
> > > + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> > > + },
> > > +};
> > > +builtin_platform_driver(ts7800_ic_driver);
> >
> > Again, this isn't appropriate either, and I pointed it out last
> > time. We have specific helpers for irqchip, and using them isn't
> > optional. But of course, you'll need to move to DT for that.
> >
> > Anyway, this is the last time I review this patch until you convert
> > the corresponding platform to DT.
> no problems, though have to note this is not constructive!
I've pointed out a bunch of issues, and provided advise on how you can
fix them. That's constructive. A non constructive approach would be to
just ignore your patch. If that's what you prefer, please say so.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] irqchip: add TS7800v1 fpga based controller driver
2022-10-21 17:18 ` Marc Zyngier
@ 2022-10-21 18:14 ` firas ashkar
2022-10-22 10:04 ` Marc Zyngier
0 siblings, 1 reply; 7+ messages in thread
From: firas ashkar @ 2022-10-21 18:14 UTC (permalink / raw)
To: Marc Zyngier; +Cc: alex, tglx, linux-kernel, linux-arm-kernel
On Fri, 2022-10-21 at 18:18 +0100, Marc Zyngier wrote:
> On Fri, 21 Oct 2022 17:19:02 +0100,
> firas ashkar <firas.ashkar@savoirfairelinux.com> wrote:
> >
> > On Fri, 2022-10-21 at 10:22 +0100, Marc Zyngier wrote:
> > > On Thu, 20 Oct 2022 15:13:51 +0100,
> > > Firas Ashkar <firas.ashkar@savoirfairelinux.com> wrote:
> > > >
> > > > 1. add TS-7800v1 fpga based irq controller driver, and
> > > > 2. add related memory and irq resources
> > > >
> > > > By default only mapped FPGA interrupts will be
> > > > chained/multiplexed,
> > > > these
> > > > chained interrupts will then be available to other device
> > > > drivers
> > > > to
> > > > request via the corresponding Linux IRQs.
> > > >
> > > > $ cat /proc/cpuinfo
> > > > processor : 0
> > > > model name : Feroceon rev 0 (v5l)
> > > > BogoMIPS : 333.33
> > > > Features : swp half thumb fastmult edsp
> > > > CPU implementer : 0x41
> > > > CPU architecture: 5TEJ
> > > > CPU variant : 0x0
> > > > CPU part : 0x926
> > > > CPU revision : 0
> > > >
> > > > Hardware : Technologic Systems TS-78xx SBC
> > > > Revision : 0000
> > > > Serial : 0000000000000000
> > > > $
> > > >
> > > > $ cat /proc/interrupts
> > > > CPU0
> > > > 1: 902 orion_irq Level orion_tick
> > > > 4: 795 orion_irq Level ttyS0
> > > > 13: 0 orion_irq Level ehci_hcd:usb2
> > > > 18: 0 orion_irq Level ehci_hcd:usb1
> > > > 22: 69 orion_irq Level eth0
> > > > 23: 171 orion_irq Level orion-mdio
> > > > 29: 0 orion_irq Level mv_crypto
> > > > 31: 2 orion_irq Level mv_xor.0
> > > > 65: 1 ts7800-irqc 0 Edge ts-dmac-cpupci
> > > > Err: 0
> > > > $
> > > >
> > > > $ uname -a
> > > > Linux ts-7800 6.1.0-rc1 #2 PREEMPT Mon Oct 17 11:19:12 EDT 2022
> > > > armv5tel
> > > > GNU/Linux
> > > > $
> > > >
> > > > Signed-off-by: Firas Ashkar <firas.ashkar@savoirfairelinux.com>
> > > > ---
> > > >
> > > > Notes:
> > > > Changes in V2
> > > > * limit the commit message
> > >
> > > Well, there is still a lot of work to do. Most of this commit
> > > message
> > > could be reduced to a single paragraph:
> > >
> > > "Add support for FooBar IRQ controller usually found on Zorglub
> > > platform."
> > >
> > > The rest is plain obvious.
> > nack, commit message is fine as is!
>
> Allow me to be the judge of that.
>
> > >
> > > > * format comments in source code
> > > > * use raw spin locks to protect mask/unmask ops
> > > > * use 'handle_edge_irq' and 'irq_ack' logic
> > > > * remove 'irq_domain_xlate_onecell'
> > > > * remove unnecessary status flags
> > > > * use 'builtin_platform_driver' helper routine
> > > >
> > > > :100644 100644 2f4fe3ca5c1a ed8378893208 M arch/arm/mach-
> > > > orion5x/ts78xx-fpga.h
> > > > :100644 100644 af810e7ccd79 d319a68b7160 M arch/arm/mach-
> > > > orion5x/ts78xx-setup.c
> > > > :100644 100644 7ef9f5e696d3 d184fb435c5d
> > > > M drivers/irqchip/Kconfig
> > > > :100644 100644 87b49a10962c b022eece2042
> > > > M drivers/irqchip/Makefile
> > > > :000000 100644 000000000000 e975607fa4d5
> > > > A drivers/irqchip/irq-ts7800.c
> > > > arch/arm/mach-orion5x/ts78xx-fpga.h | 1 +
> > > > arch/arm/mach-orion5x/ts78xx-setup.c | 55 +++++
> > > > drivers/irqchip/Kconfig | 12 +
> > > > drivers/irqchip/Makefile | 1 +
> > > > drivers/irqchip/irq-ts7800.c | 347
> > > > +++++++++++++++++++++++++++
> > > > 5 files changed, 416 insertions(+)
> > > >
> > > > diff --git a/arch/arm/mach-orion5x/ts78xx-fpga.h
> > > > b/arch/arm/mach-
> > > > orion5x/ts78xx-fpga.h
> > > > index 2f4fe3ca5c1a..ed8378893208 100644
> > > > --- a/arch/arm/mach-orion5x/ts78xx-fpga.h
> > > > +++ b/arch/arm/mach-orion5x/ts78xx-fpga.h
> > > > @@ -32,6 +32,7 @@ struct fpga_devices {
> > > > struct fpga_device ts_rtc;
> > > > struct fpga_device ts_nand;
> > > > struct fpga_device ts_rng;
> > > > + struct fpga_device ts_irqc;
> > > > };
> > > >
> > > > struct ts78xx_fpga_data {
> > > > diff --git a/arch/arm/mach-orion5x/ts78xx-setup.c
> > > > b/arch/arm/mach-
> > > > orion5x/ts78xx-setup.c
> > > > index af810e7ccd79..d319a68b7160 100644
> > > > --- a/arch/arm/mach-orion5x/ts78xx-setup.c
> > > > +++ b/arch/arm/mach-orion5x/ts78xx-setup.c
> > > > @@ -322,6 +322,49 @@ static void ts78xx_ts_rng_unload(void)
> > > > platform_device_del(&ts78xx_ts_rng_device);
> > > > }
> > > >
> > > > +/*************************************************************
> > > > ****
> > > > ************
> > > > + * fpga irq controller
> > > > +
> > > > ***************************************************************
> > > > ****
> > > > *********/
> > >
> > > [...]
> > >
> > > Sorry, but I have to ask: what part of "we're not taking any
> > > additional non-DT changes to these obsolete setups" did I fail to
> > > accurately communicate?
> > >
> > > Until this board is entirely converted to DT, I'm not taking any
> > > irqchip changes other than the most obvious bug fixes.
> > as long as this board is present in the kernel in its current
> > legacy
> > form, this is a valid patch!
>
> No. There is a cut of point. Code that we would taken 10 years ago
> isn't necessarily valid anymore. We want to improve the kernel as a
> whole, and not keep it in the past.
>
> >
> > >
> > > [...]
> > >
> > > > +static void ts7800_irq_mask(struct irq_data *d)
> > > > +{
> > > > + struct ts7800_irq_data *data =
> > > > irq_data_get_irq_chip_data(d);
> > > > + u32 fpga_mask_reg = readl(data->base + IRQ_MASK_REG);
> > > > + u32 mask_bits = 1 << d->hwirq;
> > > > + unsigned long flags;
> > > > +
> > > > + raw_spin_lock_irqsave(&data->lock, flags);
> > > > + writel(fpga_mask_reg & ~mask_bits, data->base +
> > > > IRQ_MASK_REG);
> > > > + writel(fpga_mask_reg & ~mask_bits, data->bridge +
> > > > IRQ_MASK_REG);
> > > > + raw_spin_unlock_irqrestore(&data->lock, flags);
> > >
> > > OMFG. What do you expect this protects? Same question applies to
> > > all
> > > the instances where you take this pointless lock.
> > don't jump now
> > the locks added as per your previous comment, quoting:
> > "I know your HW is UP, but seeing these RMW sequences without a
> > lock
> > makes me jump."
> > On this single CPU based arch TS78xx, locks are waste of cpu
> > cycles,
> > also note that IRQs/preemption are/is already off in this context
> >
> > maybe you meant adding locks as to promote general correct coding ?
>
> Let me spell it out for you: RMW means Read-Modify-Write. Putting a
> lock around a *write only* serves zero purpose. It is just overhead,
> and it is incorrect.
>
> >
> > or maybe it was like this previous nonsense comment, quoting :
> > "We don't remove interrupt controllers. What happens if someone
> > still
> > had a mapping?"
>
> And I stand by it.
>
> >
> >
> > >
> > > [...]
> > >
> > > > +static void ts7800_ic_chained_handle_irq(struct irq_desc
> > > > *desc)
> > > > +{
> > > > + struct ts7800_irq_data *data =
> > > > irq_desc_get_handler_data(desc);
> > > > + struct irq_chip *chip = irq_desc_get_chip(desc);
> > > > + u32 mask_bits = readl(data->base + IRQ_MASK_REG);
> > > > + u32 status = readl(data->bridge);
> > > > +
> > > > + chained_irq_enter(chip, desc);
> > > > +
> > > > + if (unlikely(status == 0)) {
> > > > + handle_bad_irq(desc);
> > > > + goto out;
> > > > + }
> > > > +
> > > > + do {
> > > > + unsigned int bit = __ffs(status);
> > > > + int irq = irq_find_mapping(data->domain, bit);
> > > > +
> > > > + if (irq && (mask_bits & BIT(bit)))
> > > > + generic_handle_irq(irq);
> > >
> > > Again, this is not appropriate. I've pointed you to the right API
> > > in
> > > my previous review of this patch.
> > 'generic_handle_domain_irq' causing some issues
>
> Which issue?
# insmod /tmp/virt-dma.ko
# insmod /tmp/ts-dmac.ko
generic_handle_domain_irq failed, error -22
irq 2, desc: a285c39f, depth: 0, count: 0, unhandled: 0
->handle_irq(): e7046ee9, ts7800_ic_chained_handle_irq+0x0/0x1ec
[irq_ts7800]
->irq_data.chip(): 70d81143, 0xc14db44c
->action(): f799c8dd
->action->handler(): dcf07981, bad_chained_irq+0x0/0x64
IRQ_LEVEL set
IRQ_NOPROBE set
IRQ_NOREQUEST set
IRQ_NOTHREAD set
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
irq 2, desc: a285c39f, depth: 0, count: 0, unhandled: 0
->handle_irq(): e7046ee9, ts7800_ic_chained_handle_irq+0x0/0x1ec
[irq_ts7800]
->irq_data.chip(): 70d81143, 0xc14db44c
->action(): f799c8dd
->action->handler(): dcf07981, bad_chained_irq+0x0/0x64
IRQ_LEVEL set
IRQ_NOPROBE set
IRQ_NOREQUEST set
IRQ_NOTHREAD set
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
irq 2, desc: a285c39f, depth: 0, count: 0, unhandled: 0
->handle_irq(): e7046ee9, ts7800_ic_chained_handle_irq+0x0/0x1ec
[irq_ts7800]
->irq_data.chip(): 70d81143, 0xc14db44c
->action(): f799c8dd
->action->handler(): dcf07981, bad_chained_irq+0x0/0x64
IRQ_LEVEL set
IRQ_NOPROBE set
IRQ_NOREQUEST set
IRQ_NOTHREAD set
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
irq 2, desc: a285c39f, depth: 0, count: 0, unhandled: 0
->handle_irq(): e7046ee9, ts7800_ic_chained_handle_irq+0x0/0x1ec
[irq_ts7800]
->irq_data.chip(): 70d81143, 0xc14db44c
->action(): f799c8dd
->action->handler(): dcf07981, bad_chained_irq+0x0/0x64
IRQ_LEVEL set
IRQ_NOPROBE set
IRQ_NOREQUEST set
IRQ_NOTHREAD set
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
irq 2, desc: a285c39f, depth: 0, count: 0, unhandled: 0
->handle_irq(): e7046ee9, ts7800_ic_chained_handle_irq+0x0/0x1ec
[irq_ts7800]
->irq_data.chip(): 70d81143, 0xc14db44c
->action(): f799c8dd
->action->handler(): dcf07981, bad_chained_irq+0x0/0x64
IRQ_LEVEL set
IRQ_NOPROBE set
IRQ_NOREQUEST set
IRQ_NOTHREAD set
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
unexpected IRQ trap at vector 02
generic_handle_domain_irq failed, error -22
unexpected IRQ trap at vector 02
...
>
> > >
> > > [...]
> > >
> > > > +static struct platform_driver ts7800_ic_driver = {
> > > > + .probe = ts7800_ic_probe,
> > > > + .remove = ts7800_ic_remove,
> > > > + .id_table = ts7800v1_ic_ids,
> > > > + .driver = {
> > > > + .name = DRIVER_NAME,
> > > > + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> > > > + },
> > > > +};
> > > > +builtin_platform_driver(ts7800_ic_driver);
> > >
> > > Again, this isn't appropriate either, and I pointed it out last
> > > time. We have specific helpers for irqchip, and using them isn't
> > > optional. But of course, you'll need to move to DT for that.
> > >
> > > Anyway, this is the last time I review this patch until you
> > > convert
> > > the corresponding platform to DT.
> > no problems, though have to note this is not constructive!
>
> I've pointed out a bunch of issues, and provided advise on how you
> can
> fix them. That's constructive. A non constructive approach would be
> to
> just ignore your patch. If that's what you prefer, please say so.
>
> Thanks,
>
> M.
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] irqchip: add TS7800v1 fpga based controller driver
2022-10-21 18:14 ` firas ashkar
@ 2022-10-22 10:04 ` Marc Zyngier
2022-10-24 20:08 ` firas ashkar
0 siblings, 1 reply; 7+ messages in thread
From: Marc Zyngier @ 2022-10-22 10:04 UTC (permalink / raw)
To: firas ashkar; +Cc: alex, tglx, linux-kernel, linux-arm-kernel
On Fri, 21 Oct 2022 19:14:48 +0100,
firas ashkar <firas.ashkar@savoirfairelinux.com> wrote:
> > > > > +static void ts7800_ic_chained_handle_irq(struct irq_desc
> > > > > *desc)
> > > > > +{
> > > > > + struct ts7800_irq_data *data =
> > > > > irq_desc_get_handler_data(desc);
> > > > > + struct irq_chip *chip = irq_desc_get_chip(desc);
> > > > > + u32 mask_bits = readl(data->base + IRQ_MASK_REG);
> > > > > + u32 status = readl(data->bridge);
> > > > > +
> > > > > + chained_irq_enter(chip, desc);
> > > > > +
> > > > > + if (unlikely(status == 0)) {
> > > > > + handle_bad_irq(desc);
> > > > > + goto out;
> > > > > + }
> > > > > +
> > > > > + do {
> > > > > + unsigned int bit = __ffs(status);
> > > > > + int irq = irq_find_mapping(data->domain, bit);
> > > > > +
> > > > > + if (irq && (mask_bits & BIT(bit)))
> > > > > + generic_handle_irq(irq);
> > > >
> > > > Again, this is not appropriate. I've pointed you to the right API
> > > > in
> > > > my previous review of this patch.
> > > 'generic_handle_domain_irq' causing some issues
> >
> > Which issue?
> # insmod /tmp/virt-dma.ko
> # insmod /tmp/ts-dmac.ko
> generic_handle_domain_irq failed, error -22
> irq 2, desc: a285c39f, depth: 0, count: 0, unhandled: 0
> ->handle_irq(): e7046ee9, ts7800_ic_chained_handle_irq+0x0/0x1ec
> [irq_ts7800]
> ->irq_data.chip(): 70d81143, 0xc14db44c
> ->action(): f799c8dd
> ->action->handler(): dcf07981, bad_chained_irq+0x0/0x64
> IRQ_LEVEL set
> IRQ_NOPROBE set
> IRQ_NOREQUEST set
> IRQ_NOTHREAD set
> unexpected IRQ trap at vector 02
> generic_handle_domain_irq failed, error -22
[...]
News flash, your code is buggy. How about you debug it? The domain
name you use definitely indicates that you "know how" to do it...
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] irqchip: add TS7800v1 fpga based controller driver
2022-10-22 10:04 ` Marc Zyngier
@ 2022-10-24 20:08 ` firas ashkar
0 siblings, 0 replies; 7+ messages in thread
From: firas ashkar @ 2022-10-24 20:08 UTC (permalink / raw)
To: Marc Zyngier; +Cc: alex, tglx, linux-kernel, linux-arm-kernel
--
Firas Ashkar
Developpeur Système Embarqué
savoirfairelinux.com | Montréal, Québec
Tél.: +1 514 276 5468 ext. 118
On Sat, 2022-10-22 at 11:04 +0100, Marc Zyngier wrote:
> On Fri, 21 Oct 2022 19:14:48 +0100,
> firas ashkar <firas.ashkar@savoirfairelinux.com> wrote:
> > > > > > +static void ts7800_ic_chained_handle_irq(struct irq_desc
> > > > > > *desc)
> > > > > > +{
> > > > > > + struct ts7800_irq_data *data =
> > > > > > irq_desc_get_handler_data(desc);
> > > > > > + struct irq_chip *chip = irq_desc_get_chip(desc);
> > > > > > + u32 mask_bits = readl(data->base + IRQ_MASK_REG);
> > > > > > + u32 status = readl(data->bridge);
> > > > > > +
> > > > > > + chained_irq_enter(chip, desc);
> > > > > > +
> > > > > > + if (unlikely(status == 0)) {
> > > > > > + handle_bad_irq(desc);
> > > > > > + goto out;
> > > > > > + }
> > > > > > +
> > > > > > + do {
> > > > > > + unsigned int bit = __ffs(status);
> > > > > > + int irq = irq_find_mapping(data->domain,
> > > > > > bit);
> > > > > > +
> > > > > > + if (irq && (mask_bits & BIT(bit)))
> > > > > > + generic_handle_irq(irq);
> > > > >
> > > > > Again, this is not appropriate. I've pointed you to the right
> > > > > API
> > > > > in
> > > > > my previous review of this patch.
> > > > 'generic_handle_domain_irq' causing some issues
> > >
> > > Which issue?
> > # insmod /tmp/virt-dma.ko
> > # insmod /tmp/ts-dmac.ko
> > generic_handle_domain_irq failed, error -22
> > irq 2, desc: a285c39f, depth: 0, count: 0, unhandled: 0
> > ->handle_irq(): e7046ee9, ts7800_ic_chained_handle_irq+0x0/0x1ec
> > [irq_ts7800]
> > ->irq_data.chip(): 70d81143, 0xc14db44c
> > ->action(): f799c8dd
> > ->action->handler(): dcf07981, bad_chained_irq+0x0/0x64
> > IRQ_LEVEL set
> > IRQ_NOPROBE set
> > IRQ_NOREQUEST set
> > IRQ_NOTHREAD set
> > unexpected IRQ trap at vector 02
> > generic_handle_domain_irq failed, error -22
>
> [...]
>
> News flash, your code is buggy. How about you debug it? The domain
> name you use definitely indicates that you "know how" to do it...
Ack
1. using domain hwirq as in
'generic_handle_domain_irq(data->domain, domain_hwirq);', and
2. changing the irq ack callback a bit
fixed the issue
# cat /proc/interrupts
CPU0
1: 7730781 orion_irq Level orion_tick
4: 2706 orion_irq Level ttyS0
13: 0 orion_irq Level ehci_hcd:usb2
18: 0 orion_irq Level ehci_hcd:usb1
22: 101488 orion_irq Level eth0
23: 5075 orion_irq Level orion-mdio
29: 0 orion_irq Level mv_crypto
31: 2 orion_irq Level mv_xor.0
65: 292763 ts7800-irqc 0 Edge ts-dmac-cpupci
66: 292760 ts7800-irqc 1 Edge ts-dmac-fpga
Err: 0
# uname -a
Linux ts-7800 6.1.0-rc1 #6 PREEMPT Fri Oct 21 11:25:55 EDT 2022
armv5tel GNU/Linux
# lsmod
Module Size Used by Not tainted
ts7800v1_sdmmc 36893 0
ts_dmac 17970 1
virt_dma 3716 1 ts_dmac
irq_ts7800 4035 0 [permanent]
#
ty
>
> M.
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-10-24 20:10 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-20 14:13 [PATCH v2] irqchip: add TS7800v1 fpga based controller driver Firas Ashkar
2022-10-21 9:22 ` Marc Zyngier
2022-10-21 16:19 ` firas ashkar
2022-10-21 17:18 ` Marc Zyngier
2022-10-21 18:14 ` firas ashkar
2022-10-22 10:04 ` Marc Zyngier
2022-10-24 20:08 ` firas ashkar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).