[RFC PATCH 0/2] Add support for Timesync Interrupt Router

[RFC PATCH 0/2] Add support for Timesync Interrupt Router

[Chintan Vankar]

This series introduces the driver support for Timesync Interrupt Router,
I will appreciate feedback on the driver implementation.

Timesync Interrupt Router is an instantiation of the generic interrupt
router module. It provides a mechanism to mux M interrupt inputs to N
interrupt outputs, where all M inputs are selectable to be driven as per N
output. More details about interrupt router and timesync interrupt router
can be found in sections 9.3 and 11.3.2 of TRM:
https://www.ti.com/lit/ug/spruiu1d/spruiu1d.pdf

Timesync Interrupt Router's inputs are either from peripherals or from
Device sync events. This series adds support on how we can map output
of the Timesync Interrupt Router corresponding to the input received
from peripherals.

As an instance, one of the input for Timesync Interrupt Router is,
Generator function, which is an output from CPTS module. The CPTS hardware
doesn't support PPS signal generation. Using the GenFx (periodic signal
generator) function, it is possible to model a PPS signal followed by
routing it via the Timesync Interrupt router to the CPTS_HWy_TS_PUSH
(hardware time stamp) input, in order to generate timestamps at 1 second
intervals.

To provide PPS support to am65-cpts driver we need to configure timesync
interrupt router to route input received as GenFx output from CPTS module
back to HWy_TS_PUSH input of CPTS module.

AM62x is one of the SoCs which has Timesync Router and mapping for all its
output corresponding to its input can be found on SDK documentation of
TISCI at:
https://software-dl.ti.com/tisci/esd/latest/5_soc_doc/am62x/interrupt_cfg.html#timesync-event-router0-interrupt-router-output-destinations

This series is based on linux-next tagged next-20250205.


Chintan Vankar (2):
  irqchip: ti-tsir: Add support for Timesync Interrupt Router
  net: ethernet: ti: am65-cpts: Add support to configure GenF signal for
    CPTS

 drivers/irqchip/Kconfig             |   9 +++
 drivers/irqchip/Makefile            |   1 +
 drivers/irqchip/ti-timesync-intr.c  | 109 ++++++++++++++++++++++++++++
 drivers/net/ethernet/ti/am65-cpts.c |  21 ++++++
 4 files changed, 140 insertions(+)
 create mode 100644 drivers/irqchip/ti-timesync-intr.c

-- 
2.34.1

[RFC PATCH 1/2] irqchip: ti-tsir: Add support for Timesync Interrupt Router

[Chintan Vankar]

Timesync Interrupt Router is an instantiation of generic interrupt router
module. It provides a mechanism to mux M interrupt inputs to N interrupt
outputs, where all M inputs are selectable to be driven as per N output.
Timesync Interrupt Router's inputs are either from peripherals or from
Device sync Events.

Add support for Timesync Interrupt Router driver to map input received
from peripherals with the corresponding output.

Signed-off-by: Chintan Vankar <c-vankar@ti.com>
---
 drivers/irqchip/Kconfig            |   9 +++
 drivers/irqchip/Makefile           |   1 +
 drivers/irqchip/ti-timesync-intr.c | 109 +++++++++++++++++++++++++++++
 3 files changed, 119 insertions(+)
 create mode 100644 drivers/irqchip/ti-timesync-intr.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index c11b9965c4ad..48b9d907be0f 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -557,6 +557,15 @@ config TI_SCI_INTA_IRQCHIP
 	  If you wish to use interrupt aggregator irq resources managed by the
 	  TI System Controller, say Y here. Otherwise, say N.
 
+config TI_TS_INTR
+	bool
+	select IRQ_DOMAIN_HIERARCHY
+	help
+	  This enables the irqchip driver support for K3 Timesync Interrupt
+	  router available on TI's SoCs.
+	  To enable Timesync Interrupt Router's mapping, say Y here. Otherwise
+	  say N.
+
 config TI_PRUSS_INTC
 	tristate
 	depends on TI_PRUSS
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 25e9ad29b8c4..00c49f6d492a 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -129,3 +129,4 @@ obj-$(CONFIG_IRQ_IDT3243X)		+= irq-idt3243x.o
 obj-$(CONFIG_APPLE_AIC)			+= irq-apple-aic.o
 obj-$(CONFIG_MCHP_EIC)			+= irq-mchp-eic.o
 obj-$(CONFIG_SUNPLUS_SP7021_INTC)	+= irq-sp7021-intc.o
+obj-$(CONFIG_TS_INTR)			+= ti-timesync-intr.o
diff --git a/drivers/irqchip/ti-timesync-intr.c b/drivers/irqchip/ti-timesync-intr.c
new file mode 100644
index 000000000000..11f26ca649d2
--- /dev/null
+++ b/drivers/irqchip/ti-timesync-intr.c
@@ -0,0 +1,109 @@
+// SPDX-License-Identifier: GPL
+/*
+ * Texas Instruments K3 Timesync Interrupt Router driver
+ *
+ * Copyright (C) 2025 Texas Instruments Incorporated - https://www.ti.com/
+ *	Chintan Vankar <c-vankar@ti.com>
+ */
+
+#include <linux/irqchip.h>
+#include <linux/irqdomain.h>
+#include <linux/of_address.h>
+#include <linux/list.h>
+
+#define DRIVER_NAME	"ti-tsir"
+
+#define TIMESYNC_INTRTR_ENABLE			GENMASK(5, 0)
+#define TIMESYNC_INTRTR_INT_ENABLE		BIT(16)
+#define TIMESYNC_INTRTR_MAX_OUTPUT_LINES	48
+
+struct tsr_chip_data {
+	void __iomem		*tsr_base;
+	struct irq_domain	*domain;
+	u64			flags;
+};
+
+static struct irq_chip ts_intr_irq_chip = {
+	.name			= "TIMESYNC_INTRTR",
+};
+
+static u32 output_line_to_virq[TIMESYNC_INTRTR_MAX_OUTPUT_LINES];
+static struct tsr_chip_data tsr_data;
+
+static int ts_intr_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
+				    unsigned int nr_irqs, void *arg)
+{
+	unsigned int output_line, input_line, output_line_offset;
+	struct irq_fwspec *fwspec = (struct irq_fwspec *)arg;
+	int ret;
+
+	irq_domain_set_hwirq_and_chip(domain, virq, output_line,
+				      &ts_intr_irq_chip,
+				      NULL);
+
+	/* Check for two input parameters: output line and corresponding input line */
+	if (fwspec->param_count != 2)
+		return -EINVAL;
+
+	output_line = fwspec->param[0];
+
+	/* Timesync Interrupt Router's mux-controller register starts at offset 4 from base
+	 * address and each output line are at offset in multiple of 4s in Timesync INTR's
+	 * register space, calculate the register offset from provided output line.
+	 */
+	output_line_offset = 4 * output_line + 0x4;
+	output_line_to_virq[output_line] = virq;
+	input_line = fwspec->param[1] & TIMESYNC_INTRTR_ENABLE;
+
+	/* Map output line corresponding to input line */
+	writel(input_line, tsr_data.tsr_base + output_line_offset);
+
+	/* When interrupt enable bit is set for Timesync Interrupt Router it maps the output
+	 * line with the existing input line, hence enable interrupt line after we set bits for
+	 * output line.
+	 */
+	input_line |= TIMESYNC_INTRTR_INT_ENABLE;
+	writel(input_line, tsr_data.tsr_base + output_line_offset);
+
+	return 0;
+}
+
+static void ts_intr_irq_domain_free(struct irq_domain *domain, unsigned int virq,
+				    unsigned int nr_irqs)
+{
+	struct output_line_to_virq *node, *n;
+	unsigned int output_line_offset;
+	int i;
+
+	for (i = 0; i < TIMESYNC_INTRTR_MAX_OUTPUT_LINES; i++) {
+		if (output_line_to_virq[i] == virq) {
+			/* Calculate the register offset value from provided output line */
+			output_line_offset = 4 * i + 0x4;
+			writel(~TIMESYNC_INTRTR_INT_ENABLE, tsr_data.tsr_base + output_line_offset);
+		}
+	}
+}
+
+static const struct irq_domain_ops ts_intr_irq_domain_ops = {
+	.alloc		= ts_intr_irq_domain_alloc,
+	.free		= ts_intr_irq_domain_free,
+};
+
+static int tsr_init(struct device_node *node)
+{
+	tsr_data.tsr_base = of_iomap(node, 0);
+	if (IS_ERR(tsr_data.tsr_base)) {
+		pr_err("Unable to get reg\n");
+		return PTR_ERR(tsr_data.tsr_base);
+	}
+
+	tsr_data.domain = irq_domain_create_tree(&node->fwnode, &ts_intr_irq_domain_ops, &tsr_data);
+
+	return 0;
+}
+
+IRQCHIP_DECLARE(ts_intr, "ti,ts-intr", tsr_init);
+
+MODULE_AUTHOR("Chintan Vankar <c-vankar@ti.com>");
+MODULE_DESCRIPTION("Driver to configure Timesync Interrupt Router");
+MODULE_LICENSE("GPL");
-- 
2.34.1

[RFC PATCH 2/2] net: ethernet: ti: am65-cpts: Add support to configure GenF signal for CPTS

[Chintan Vankar]

Add support to configure GenFx (periodic signal generator) function which
can be used by Timesync Interrupt Router to map it with CPTS_HWy_TS_PUSH,
in order to generate timestamps at 1 second intervals. This configuration
is optional for CPTS module.

Signed-off-by: Chintan Vankar <c-vankar@ti.com>
---
 drivers/net/ethernet/ti/am65-cpts.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/net/ethernet/ti/am65-cpts.c b/drivers/net/ethernet/ti/am65-cpts.c
index 59d6ab989c55..c32e6c60561d 100644
--- a/drivers/net/ethernet/ti/am65-cpts.c
+++ b/drivers/net/ethernet/ti/am65-cpts.c
@@ -171,6 +171,7 @@ struct am65_cpts {
 	u32 genf_num;
 	u32 ts_add_val;
 	int irq;
+	int genf_irq;
 	struct mutex ptp_clk_lock; /* PHC access sync */
 	u64 timestamp;
 	u32 genf_enable;
@@ -409,6 +410,11 @@ static irqreturn_t am65_cpts_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static irqreturn_t am65_cpts_genf_interrupt(int irq, void *dev_id)
+{
+	return IRQ_HANDLED;
+}
+
 /* PTP clock operations */
 static int am65_cpts_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
 {
@@ -1216,6 +1222,21 @@ struct am65_cpts *am65_cpts_create(struct device *dev, void __iomem *regs,
 		goto reset_ptpclk;
 	}
 
+	/*
+	 * This API is used by Timesync Router's driver code to map
+	 * GenFx output of CPTS module with HWx_TS_PUSH input to generate PPS
+	 * signal.
+	 */
+	cpts->genf_irq = of_irq_get_byname(node, "genf");
+	if (cpts->genf_irq > 0) {
+		ret = devm_request_threaded_irq(dev, cpts->genf_irq, NULL,
+						am65_cpts_genf_interrupt,
+						0, dev_name(dev), cpts);
+		if (ret < 0)
+			dev_dbg(cpts->dev, "GENF output will not be routed via Time Sync Interrupt Router
+				%d\n", ret);
+	}
+
 	dev_info(dev, "CPTS ver 0x%08x, freq:%u, add_val:%u pps:%d\n",
 		 am65_cpts_read32(cpts, idver),
 		 cpts->refclk_freq, cpts->ts_add_val, cpts->pps_present);
-- 
2.34.1

Re: [RFC PATCH 0/2] Add support for Timesync Interrupt Router

[Vignesh Raghavendra]

On 05/02/25 21:31, Chintan Vankar wrote:
> Chintan Vankar (2):
>   irqchip: ti-tsir: Add support for Timesync Interrupt Router
>   net: ethernet: ti: am65-cpts: Add support to configure GenF signal for
>     CPTS
> 
>  drivers/irqchip/Kconfig             |   9 +++
>  drivers/irqchip/Makefile            |   1 +
>  drivers/irqchip/ti-timesync-intr.c  | 109 ++++++++++++++++++++++++++++
>  drivers/net/ethernet/ti/am65-cpts.c |  21 ++++++
>  4 files changed, 140 insertions(+)
>  create mode 100644 drivers/irqchip/ti-timesync-intr.c

Where are the device-tree binding updates that need to go with
individual driver changes?

-- 
Regards
Vignesh
https://ti.com/opensource

Re: [RFC PATCH 0/2] Add support for Timesync Interrupt Router

[Chintan Vankar]

On 06/02/25 10:43, Vignesh Raghavendra wrote:
> 
> 
> On 05/02/25 21:31, Chintan Vankar wrote:
>> Chintan Vankar (2):
>>    irqchip: ti-tsir: Add support for Timesync Interrupt Router
>>    net: ethernet: ti: am65-cpts: Add support to configure GenF signal for
>>      CPTS
>>
>>   drivers/irqchip/Kconfig             |   9 +++
>>   drivers/irqchip/Makefile            |   1 +
>>   drivers/irqchip/ti-timesync-intr.c  | 109 ++++++++++++++++++++++++++++
>>   drivers/net/ethernet/ti/am65-cpts.c |  21 ++++++
>>   4 files changed, 140 insertions(+)
>>   create mode 100644 drivers/irqchip/ti-timesync-intr.c
> 
> Where are the device-tree binding updates that need to go with
> individual driver changes?
> 


Hello Vignesh, This series is not specific to any use-case that Timesync
Interrupt Router is implementing. Through this RFC series I am expecting
a feedback on driver implementation so that later on we can make use of
this driver to implement certain functionality.

Regards,
Chintan.

Re: [RFC PATCH 1/2] irqchip: ti-tsir: Add support for Timesync Interrupt Router

[Simon Horman]

On Wed, Feb 05, 2025 at 09:31:18PM +0530, Chintan Vankar wrote:
> Timesync Interrupt Router is an instantiation of generic interrupt router
> module. It provides a mechanism to mux M interrupt inputs to N interrupt
> outputs, where all M inputs are selectable to be driven as per N output.
> Timesync Interrupt Router's inputs are either from peripherals or from
> Device sync Events.
> 
> Add support for Timesync Interrupt Router driver to map input received
> from peripherals with the corresponding output.
> 
> Signed-off-by: Chintan Vankar <c-vankar@ti.com>

...

> diff --git a/drivers/irqchip/ti-timesync-intr.c b/drivers/irqchip/ti-timesync-intr.c
> new file mode 100644
> index 000000000000..11f26ca649d2
> --- /dev/null
> +++ b/drivers/irqchip/ti-timesync-intr.c
> @@ -0,0 +1,109 @@
> +// SPDX-License-Identifier: GPL

Hi Chintan,

I think you need to be mores specific here wrt the version of the GPL.

Link: https://www.kernel.org/doc/html/v6.13-rc6/process/license-rules.html

Flagged by ./scripts/spdxcheck.py

Also, compiling this file with GCC 14.2.0 for allmodconfig with W=1
generates a significant number of warnings. You may wish to look into that.

drivers/irqchip/ti-timesync-intr.c: In function ‘ts_intr_irq_domain_alloc’:
drivers/irqchip/ti-timesync-intr.c:38:13: error: unused variable ‘ret’ [-Werror=unused-variable]
   38 |         int ret;
      |             ^~~
drivers/irqchip/ti-timesync-intr.c: In function ‘ts_intr_irq_domain_free’:
drivers/irqchip/ti-timesync-intr.c:82:32: error: conversion from ‘long unsigned int’ to ‘unsigned int’ changes value from ‘18446744073709486079’ to ‘4294901759’ [-Werror=overflow]
   82 |                         writel(~TIMESYNC_INTRTR_INT_ENABLE, tsr_data.tsr_base + output_line_offset);
drivers/irqchip/ti-timesync-intr.c:74:44: error: unused variable ‘n’ [-Werror=unused-variable]
   74 |         struct output_line_to_virq *node, *n;
      |                                            ^
drivers/irqchip/ti-timesync-intr.c:74:37: error: unused variable ‘node’ [-Werror=unused-variable]
   74 |         struct output_line_to_virq *node, *n;
      |                                     ^~~~
In file included from ./include/linux/irqchip.h:16,
                 from drivers/irqchip/ti-timesync-intr.c:9:
drivers/irqchip/ti-timesync-intr.c: At top level:
./include/linux/minmax.h:24:35: error: comparison of distinct pointer types lacks a cast [-Werror=compare-distinct-pointer-types]
   24 |         (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
      |                                   ^~
./include/linux/of.h:1525:31: note: in definition of macro ‘_OF_DECLARE’
 1525 |                      .data = (fn == (fn_type)NULL) ? fn : fn  }
      |                               ^~
./include/linux/irqchip.h:37:9: note: in expansion of macro ‘OF_DECLARE_2’
   37 |         OF_DECLARE_2(irqchip, name, compat, typecheck_irq_init_cb(fn))
      |         ^~~~~~~~~~~~
./include/linux/irqchip.h:24:10: note: in expansion of macro ‘__typecheck’
   24 |         (__typecheck(typecheck_irq_init_cb, &fn) ? fn : fn)
      |          ^~~~~~~~~~~
./include/linux/irqchip.h:37:45: note: in expansion of macro ‘typecheck_irq_init_cb’
   37 |         OF_DECLARE_2(irqchip, name, compat, typecheck_irq_init_cb(fn))
      |                                             ^~~~~~~~~~~~~~~~~~~~~
drivers/irqchip/ti-timesync-intr.c:105:1: note: in expansion of macro ‘IRQCHIP_DECLARE’
  105 | IRQCHIP_DECLARE(ts_intr, "ti,ts-intr", tsr_init);
      | ^~~~~~~~~~~~~~~
./include/linux/of.h:1525:34: error: comparison of distinct pointer types lacks a cast [-Werror=compare-distinct-pointer-types]
 1525 |                      .data = (fn == (fn_type)NULL) ? fn : fn  }
      |                                  ^~
./include/linux/of.h:1540:17: note: in expansion of macro ‘_OF_DECLARE’
 1540 |                 _OF_DECLARE(table, name, compat, fn, of_init_fn_2)
      |                 ^~~~~~~~~~~
./include/linux/irqchip.h:37:9: note: in expansion of macro ‘OF_DECLARE_2’
   37 |         OF_DECLARE_2(irqchip, name, compat, typecheck_irq_init_cb(fn))
      |         ^~~~~~~~~~~~
drivers/irqchip/ti-timesync-intr.c:105:1: note: in expansion of macro ‘IRQCHIP_DECLARE’
  105 | IRQCHIP_DECLARE(ts_intr, "ti,ts-intr", tsr_init);
      | ^~~~~~~~~~~~~~~
./include/linux/minmax.h:24:35: error: comparison of distinct pointer types lacks a cast [-Werror=compare-distinct-pointer-types]
   24 |         (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
      |                                   ^~
./include/linux/of.h:1525:54: note: in definition of macro ‘_OF_DECLARE’
 1525 |                      .data = (fn == (fn_type)NULL) ? fn : fn  }
      |                                                      ^~
./include/linux/irqchip.h:37:9: note: in expansion of macro ‘OF_DECLARE_2’
   37 |         OF_DECLARE_2(irqchip, name, compat, typecheck_irq_init_cb(fn))
      |         ^~~~~~~~~~~~
./include/linux/irqchip.h:24:10: note: in expansion of macro ‘__typecheck’
   24 |         (__typecheck(typecheck_irq_init_cb, &fn) ? fn : fn)
      |          ^~~~~~~~~~~
./include/linux/irqchip.h:37:45: note: in expansion of macro ‘typecheck_irq_init_cb’
   37 |         OF_DECLARE_2(irqchip, name, compat, typecheck_irq_init_cb(fn))
      |                                             ^~~~~~~~~~~~~~~~~~~~~
drivers/irqchip/ti-timesync-intr.c:105:1: note: in expansion of macro ‘IRQCHIP_DECLARE’
  105 | IRQCHIP_DECLARE(ts_intr, "ti,ts-intr", tsr_init);
      | ^~~~~~~~~~~~~~~
./include/linux/minmax.h:24:35: error: comparison of distinct pointer types lacks a cast [-Werror=compare-distinct-pointer-types]
   24 |         (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
      |                                   ^~
./include/linux/of.h:1525:59: note: in definition of macro ‘_OF_DECLARE’
 1525 |                      .data = (fn == (fn_type)NULL) ? fn : fn  }
      |                                                           ^~
./include/linux/irqchip.h:37:9: note: in expansion of macro ‘OF_DECLARE_2’
   37 |         OF_DECLARE_2(irqchip, name, compat, typecheck_irq_init_cb(fn))
      |         ^~~~~~~~~~~~
./include/linux/irqchip.h:24:10: note: in expansion of macro ‘__typecheck’
   24 |         (__typecheck(typecheck_irq_init_cb, &fn) ? fn : fn)
      |          ^~~~~~~~~~~
./include/linux/irqchip.h:37:45: note: in expansion of macro ‘typecheck_irq_init_cb’
   37 |         OF_DECLARE_2(irqchip, name, compat, typecheck_irq_init_cb(fn))
      |                                             ^~~~~~~~~~~~~~~~~~~~~
drivers/irqchip/ti-timesync-intr.c:105:1: note: in expansion of macro ‘IRQCHIP_DECLARE’
  105 | IRQCHIP_DECLARE(ts_intr, "ti,ts-intr", tsr_init);
      | ^~~~~~~~~~~~~~~
drivers/irqchip/ti-timesync-intr.c: In function ‘ts_intr_irq_domain_alloc’:
drivers/irqchip/ti-timesync-intr.c:40:9: error: ‘output_line’ is used uninitialized [-Werror=uninitialized]
   40 |         irq_domain_set_hwirq_and_chip(domain, virq, output_line,
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   41 |                                       &ts_intr_irq_chip,
      |                                       ~~~~~~~~~~~~~~~~~~
   42 |                                       NULL);
      |                                       ~~~~~
drivers/irqchip/ti-timesync-intr.c:36:22: note: ‘output_line’ was declared here
   36 |         unsigned int output_line, input_line, output_line_offset;
      |                      ^~~~~~~~~~~

Re: [RFC PATCH 1/2] irqchip: ti-tsir: Add support for Timesync Interrupt Router

[Thomas Gleixner]

On Wed, Feb 05 2025 at 21:31, Chintan Vankar wrote:
> +++ b/drivers/irqchip/ti-timesync-intr.c
> @@ -0,0 +1,109 @@
> +// SPDX-License-Identifier: GPL

That's not a valid license identifier

> +static struct irq_chip ts_intr_irq_chip = {
> +	.name			= "TIMESYNC_INTRTR",
> +};

How is this interrupt chip supposed to work? All it implements is a
name.

> +static int ts_intr_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> +				    unsigned int nr_irqs, void *arg)
> +{
> +	unsigned int output_line, input_line, output_line_offset;
> +	struct irq_fwspec *fwspec = (struct irq_fwspec *)arg;
> +	int ret;
> +
> +	irq_domain_set_hwirq_and_chip(domain, virq, output_line,
> +				      &ts_intr_irq_chip,
> +				      NULL);

You set the interrupt chip and data before validating that the input
argument is valid. That does not make any sense.

> +	/* Check for two input parameters: output line and corresponding input line */
> +	if (fwspec->param_count != 2)
> +		return -EINVAL;
> +
> +	output_line = fwspec->param[0];
> +
> +	/* Timesync Interrupt Router's mux-controller register starts at offset 4 from base
> +	 * address and each output line are at offset in multiple of 4s in Timesync INTR's
> +	 * register space, calculate the register offset from provided output line.
> +	 */

Please use proper kernel comment style as documented:

  https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#comment-style

This is not networking code.

> +	output_line_offset = 4 * output_line + 0x4;

Magic hardcoded numbers '4' and '0x4' without any explanation of the logic.

> +	output_line_to_virq[output_line] = virq;
> +	input_line = fwspec->param[1] & TIMESYNC_INTRTR_ENABLE;
> +
> +	/* Map output line corresponding to input line */
> +	writel(input_line, tsr_data.tsr_base + output_line_offset);
> +
> +	/* When interrupt enable bit is set for Timesync Interrupt Router it maps the output
> +	 * line with the existing input line, hence enable interrupt line after we set bits for
> +	 * output line.

I have no idea what this comment is trying to tell me.

> +	 */
> +	input_line |= TIMESYNC_INTRTR_INT_ENABLE;
> +	writel(input_line, tsr_data.tsr_base + output_line_offset);
> +
> +	return 0;
> +}
> +
> +static void ts_intr_irq_domain_free(struct irq_domain *domain, unsigned int virq,
> +				    unsigned int nr_irqs)
> +{
> +	struct output_line_to_virq *node, *n;
> +	unsigned int output_line_offset;
> +	int i;
> +
> +	for (i = 0; i < TIMESYNC_INTRTR_MAX_OUTPUT_LINES; i++) {
> +		if (output_line_to_virq[i] == virq) {
> +			/* Calculate the register offset value from provided output line */

Can you please implement a properly commented helper function which
explains how this offset calculation is supposed to work?

> +			output_line_offset = 4 * i + 0x4;
> +			writel(~TIMESYNC_INTRTR_INT_ENABLE, tsr_data.tsr_base + output_line_offset);
> +		}
> +	}
> +}
> +
> +static const struct irq_domain_ops ts_intr_irq_domain_ops = {
> +	.alloc		= ts_intr_irq_domain_alloc,
> +	.free		= ts_intr_irq_domain_free,
> +};
> +
> +static int tsr_init(struct device_node *node)
> +{
> +	tsr_data.tsr_base = of_iomap(node, 0);
> +	if (IS_ERR(tsr_data.tsr_base)) {
> +		pr_err("Unable to get reg\n");
> +		return PTR_ERR(tsr_data.tsr_base);
> +	}
> +
> +	tsr_data.domain = irq_domain_create_tree(&node->fwnode, &ts_intr_irq_domain_ops, &tsr_data);

So this instantiates a interrupt domain which is completely disconnected
from the rest of the world.

How is the output side of this supposed to handle an interrupt which is
routed to it?

Thanks,

        tglx

Re: [RFC PATCH 1/2] irqchip: ti-tsir: Add support for Timesync Interrupt Router

[Vankar, Chintan]

Hello Thomas,

Thank you for the reply, I will address your comments to modify license
identifier, comments, helper function for offset calculation. I have
prioritized explaining Timesync interrupt router's functionality below.


On 2/7/2025 2:58 AM, Thomas Gleixner wrote:
> On Wed, Feb 05 2025 at 21:31, Chintan Vankar wrote:
>> +++ b/drivers/irqchip/ti-timesync-intr.c
>> @@ -0,0 +1,109 @@
>> +// SPDX-License-Identifier: GPL
> 
> That's not a valid license identifier
> 
>> +static struct irq_chip ts_intr_irq_chip = {
>> +	.name			= "TIMESYNC_INTRTR",
>> +};
> 
> How is this interrupt chip supposed to work? All it implements is a
> name.
> 

Timesync INTR can be used to map input sources with the corresponding
output, so that we can configure specific functionality for the device
that is using this output sources either as an interrupt source or to
synchronize the time.

To implement above Timesync INTR's functionality, I have implemented
ts_intr_irq_domain_alloc() and ts_intr_irq_domain_free() ops which are
sufficient. Let me know if they are fine.

>> +static int ts_intr_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
>> +				    unsigned int nr_irqs, void *arg)
>> +{
>> +	unsigned int output_line, input_line, output_line_offset;
>> +	struct irq_fwspec *fwspec = (struct irq_fwspec *)arg;
>> +	int ret;
>> +
>> +	irq_domain_set_hwirq_and_chip(domain, virq, output_line,
>> +				      &ts_intr_irq_chip,
>> +				      NULL);
> 
> You set the interrupt chip and data before validating that the input
> argument is valid. That does not make any sense.
> 
>> +	/* Check for two input parameters: output line and corresponding input line */
>> +	if (fwspec->param_count != 2)
>> +		return -EINVAL;
>> +
>> +	output_line = fwspec->param[0];
>> +
>> +	/* Timesync Interrupt Router's mux-controller register starts at offset 4 from base
>> +	 * address and each output line are at offset in multiple of 4s in Timesync INTR's
>> +	 * register space, calculate the register offset from provided output line.
>> +	 */
> 
> Please use proper kernel comment style as documented:
> 
>    https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#comment-style
> 
> This is not networking code.
> 
>> +	output_line_offset = 4 * output_line + 0x4;
> 
> Magic hardcoded numbers '4' and '0x4' without any explanation of the logic.
> 
>> +	output_line_to_virq[output_line] = virq;
>> +	input_line = fwspec->param[1] & TIMESYNC_INTRTR_ENABLE;
>> +
>> +	/* Map output line corresponding to input line */
>> +	writel(input_line, tsr_data.tsr_base + output_line_offset);
>> +
>> +	/* When interrupt enable bit is set for Timesync Interrupt Router it maps the output
>> +	 * line with the existing input line, hence enable interrupt line after we set bits for
>> +	 * output line.
> 
> I have no idea what this comment is trying to tell me.
> 
>> +	 */
>> +	input_line |= TIMESYNC_INTRTR_INT_ENABLE;
>> +	writel(input_line, tsr_data.tsr_base + output_line_offset);
>> +
>> +	return 0;
>> +}
>> +
>> +static void ts_intr_irq_domain_free(struct irq_domain *domain, unsigned int virq,
>> +				    unsigned int nr_irqs)
>> +{
>> +	struct output_line_to_virq *node, *n;
>> +	unsigned int output_line_offset;
>> +	int i;
>> +
>> +	for (i = 0; i < TIMESYNC_INTRTR_MAX_OUTPUT_LINES; i++) {
>> +		if (output_line_to_virq[i] == virq) {
>> +			/* Calculate the register offset value from provided output line */
> 
> Can you please implement a properly commented helper function which
> explains how this offset calculation is supposed to work?
> 
>> +			output_line_offset = 4 * i + 0x4;
>> +			writel(~TIMESYNC_INTRTR_INT_ENABLE, tsr_data.tsr_base + output_line_offset);
>> +		}
>> +	}
>> +}
>> +
>> +static const struct irq_domain_ops ts_intr_irq_domain_ops = {
>> +	.alloc		= ts_intr_irq_domain_alloc,
>> +	.free		= ts_intr_irq_domain_free,
>> +};
>> +
>> +static int tsr_init(struct device_node *node)
>> +{
>> +	tsr_data.tsr_base = of_iomap(node, 0);
>> +	if (IS_ERR(tsr_data.tsr_base)) {
>> +		pr_err("Unable to get reg\n");
>> +		return PTR_ERR(tsr_data.tsr_base);
>> +	}
>> +
>> +	tsr_data.domain = irq_domain_create_tree(&node->fwnode, &ts_intr_irq_domain_ops, &tsr_data);
> 
> So this instantiates a interrupt domain which is completely disconnected
> from the rest of the world.
>  > How is the output side of this supposed to handle an interrupt which is
> routed to it?
> 

                         ________________________
                        |    Timesync INTR       +---->dma_local_events
                        |                        |
Device sync events----->                        +---->pcie_cpts_hw_push
                        |                        |
          cpts_genf----->                        +---->cpts_hw_push
                        |________________________|


No it is connected, it is being used to configure the output for
Timesync INTR as mentioned above.

As seen in the diagram, Timesync INTR has multiple output interfaces and
we can configure those to map them with the corresponding input as
required by peripherals which receives the signal. In context of this
series, CPTS module is utilizing the output signal of cpts_genf as
Hardware timestamp push event to generate timestamps at 1 seconds.

Let me know if you need more details regarding any of the above points
that I have mentioned.


Regards,
Chintan.

> Thanks,
> 
>          tglx
> 
> 

Re: [RFC PATCH 1/2] irqchip: ti-tsir: Add support for Timesync Interrupt Router

[Thomas Gleixner]

Chintan!

On Sun, Feb 09 2025 at 14:06, Vankar, Chintan wrote:
> On 2/7/2025 2:58 AM, Thomas Gleixner wrote:
>> On Wed, Feb 05 2025 at 21:31, Chintan Vankar wrote:
>>> +static struct irq_chip ts_intr_irq_chip = {
>>> +	.name			= "TIMESYNC_INTRTR",
>>> +};
>> 
>> How is this interrupt chip supposed to work? All it implements is a
>> name.
>> 
>
> Timesync INTR can be used to map input sources with the corresponding
> output, so that we can configure specific functionality for the device
> that is using this output sources either as an interrupt source or to
> synchronize the time.
>
> To implement above Timesync INTR's functionality, I have implemented
> ts_intr_irq_domain_alloc() and ts_intr_irq_domain_free() ops which are
> sufficient. Let me know if they are fine.
>>> +
>>> +	tsr_data.domain = irq_domain_create_tree(&node->fwnode, &ts_intr_irq_domain_ops, &tsr_data);
>> 
>> So this instantiates a interrupt domain which is completely disconnected
>> from the rest of the world.
>>  > How is the output side of this supposed to handle an interrupt which is
>> routed to it?
>> 
>
>                          ________________________
>                         |    Timesync INTR       +---->dma_local_events
>                         |                        |
> Device sync events----->                        +---->pcie_cpts_hw_push
>                         |                        |
>           cpts_genf----->                        +---->cpts_hw_push
>                         |________________________|
>
>
> No it is connected, it is being used to configure the output for
> Timesync INTR as mentioned above.
>
> As seen in the diagram, Timesync INTR has multiple output interfaces and
> we can configure those to map them with the corresponding input as
> required by peripherals which receives the signal. In context of this
> series, CPTS module is utilizing the output signal of cpts_genf as
> Hardware timestamp push event to generate timestamps at 1 seconds

If I understand this correctly, then the interrupt number you need to
allocate for this is never going to be requested. If it would be
requested it just would do nothing and the handler would never be
invoked, right?

The allocation just establishes the routing of a signal between two
arbitrary IP blocks in the SoC.

So the question is what has this to do with interrupts in the first
place?

Thanks,

        tglx

Re: [RFC PATCH 1/2] irqchip: ti-tsir: Add support for Timesync Interrupt Router

[Vankar, Chintan]

On 2/11/2025 1:33 AM, Thomas Gleixner wrote:
> Chintan!
> 
> On Sun, Feb 09 2025 at 14:06, Vankar, Chintan wrote:
>> On 2/7/2025 2:58 AM, Thomas Gleixner wrote:
>>> On Wed, Feb 05 2025 at 21:31, Chintan Vankar wrote:
>>>> +static struct irq_chip ts_intr_irq_chip = {
>>>> +	.name			= "TIMESYNC_INTRTR",
>>>> +};
>>>
>>> How is this interrupt chip supposed to work? All it implements is a
>>> name.
>>>
>>
>> Timesync INTR can be used to map input sources with the corresponding
>> output, so that we can configure specific functionality for the device
>> that is using this output sources either as an interrupt source or to
>> synchronize the time.
>>
>> To implement above Timesync INTR's functionality, I have implemented
>> ts_intr_irq_domain_alloc() and ts_intr_irq_domain_free() ops which are
>> sufficient. Let me know if they are fine.
>>>> +
>>>> +	tsr_data.domain = irq_domain_create_tree(&node->fwnode, &ts_intr_irq_domain_ops, &tsr_data);
>>>
>>> So this instantiates a interrupt domain which is completely disconnected
>>> from the rest of the world.
>>>   > How is the output side of this supposed to handle an interrupt which is
>>> routed to it?
>>>
>>
>>                           ________________________
>>                          |    Timesync INTR       +---->dma_local_events
>>                          |                        |
>> Device sync events----->                        +---->pcie_cpts_hw_push
>>                          |                        |
>>            cpts_genf----->                        +---->cpts_hw_push
>>                          |________________________|
>>
>>
>> No it is connected, it is being used to configure the output for
>> Timesync INTR as mentioned above.
>>
>> As seen in the diagram, Timesync INTR has multiple output interfaces and
>> we can configure those to map them with the corresponding input as
>> required by peripherals which receives the signal. In context of this
>> series, CPTS module is utilizing the output signal of cpts_genf as
>> Hardware timestamp push event to generate timestamps at 1 seconds
> 
> If I understand this correctly, then the interrupt number you need to
> allocate for this is never going to be requested. If it would be
> requested it just would do nothing and the handler would never be
> invoked, right?
> 
> The allocation just establishes the routing of a signal between two
> arbitrary IP blocks in the SoC.
> 
> So the question is what has this to do with interrupts in the first
> place?
> 

Hello Thomas,

Your understanding is correct about the Timesync INTR. As I mentioned
Timesync INTR is an instance of Interrupt Router which has multiple
output and not all the output lines are acting as interrupt lines unlike
other Interrupt Routers. Timesync INTR can have devices on both the
sides, we can provide input to Timesync INTR that can be consumed by
some other device from the output line. As an instance, One of the
input of Timesync INTR is an output from the CPTS module which can be
consumed by other device and that does not need to handle/allocate Linux
irq number.

Let me know if implementing this driver for this specific use-case would
be feasible.

> Thanks,
> 
>          tglx

Re: [RFC PATCH 1/2] irqchip: ti-tsir: Add support for Timesync Interrupt Router

[Thomas Gleixner]

Chintan!

On Fri, Feb 14 2025 at 00:15, Vankar, Chintan wrote:
> On 2/11/2025 1:33 AM, Thomas Gleixner wrote:
>> On Sun, Feb 09 2025 at 14:06, Vankar, Chintan wrote:
>>> On 2/7/2025 2:58 AM, Thomas Gleixner wrote:
>> If I understand this correctly, then the interrupt number you need to
>> allocate for this is never going to be requested. If it would be
>> requested it just would do nothing and the handler would never be
>> invoked, right?
>> 
>> The allocation just establishes the routing of a signal between two
>> arbitrary IP blocks in the SoC.
>> 
>> So the question is what has this to do with interrupts in the first
>> place?
>
> Your understanding is correct about the Timesync INTR. As I mentioned
> Timesync INTR is an instance of Interrupt Router which has multiple
> output and not all the output lines are acting as interrupt lines unlike
> other Interrupt Routers. Timesync INTR can have devices on both the
> sides, we can provide input to Timesync INTR that can be consumed by
> some other device from the output line. As an instance, One of the
> input of Timesync INTR is an output from the CPTS module which can be
> consumed by other device and that does not need to handle/allocate Linux
> irq number.

Two questions:

 1) For the case where no interrupt is involved, how is the routing
    configured?

 2) For the case where it routes an input line to an interupt, then how
    is this interrupt going to be handled by this interrupt domain which
    is not connected to anything and implements an empty disfunctional
    interrupt chip?

Thanks

        tglx

Re: [RFC PATCH 1/2] irqchip: ti-tsir: Add support for Timesync Interrupt Router

[Chintan Vankar]

Hello Thomas,

On 14/02/25 04:13, Thomas Gleixner wrote:
> Chintan!
> 
> On Fri, Feb 14 2025 at 00:15, Vankar, Chintan wrote:
>> On 2/11/2025 1:33 AM, Thomas Gleixner wrote:
>>> On Sun, Feb 09 2025 at 14:06, Vankar, Chintan wrote:
>>>> On 2/7/2025 2:58 AM, Thomas Gleixner wrote:
>>> If I understand this correctly, then the interrupt number you need to
>>> allocate for this is never going to be requested. If it would be
>>> requested it just would do nothing and the handler would never be
>>> invoked, right?
>>>
>>> The allocation just establishes the routing of a signal between two
>>> arbitrary IP blocks in the SoC.
>>>
>>> So the question is what has this to do with interrupts in the first
>>> place?
>>
>> Your understanding is correct about the Timesync INTR. As I mentioned
>> Timesync INTR is an instance of Interrupt Router which has multiple
>> output and not all the output lines are acting as interrupt lines unlike
>> other Interrupt Routers. Timesync INTR can have devices on both the
>> sides, we can provide input to Timesync INTR that can be consumed by
>> some other device from the output line. As an instance, One of the
>> input of Timesync INTR is an output from the CPTS module which can be
>> consumed by other device and that does not need to handle/allocate Linux
>> irq number.
> 
> Two questions:
> 
>   1) For the case where no interrupt is involved, how is the routing
>      configured?
> 
>   2) For the case where it routes an input line to an interupt, then how
>      is this interrupt going to be handled by this interrupt domain which
>      is not connected to anything and implements an empty disfunctional
>      interrupt chip?
> 

For both the cases above the job of Timesync INTR is to map the output
register with the corresponding input.

As described in section 11.3.2.1 in the TRM at:
https://www.ti.com/lit/ug/spruiu1d/spruiu1d.pdf,
the job of the Timesync INTR is to provide a configuration of the
"output registers which controls the selection". Hence we just have to
provide configuration APIs in the Timesync INTR which programs output
registers of the Timesync INTR. About the handling of the interrupts,
the device which receives an interrupt needs to handle the interrupt.

Could you please explain why we consider these two cases to be
different?


Regards,
Chintan.

> Thanks
> 
>          tglx

Re: [RFC PATCH 1/2] irqchip: ti-tsir: Add support for Timesync Interrupt Router

[Thomas Gleixner]

Chintan!

On Sat, Feb 15 2025 at 17:19, Chintan Vankar wrote:
> On 14/02/25 04:13, Thomas Gleixner wrote:
>> Two questions:
>> 
>>   1) For the case where no interrupt is involved, how is the routing
>>      configured?
>> 
>>   2) For the case where it routes an input line to an interupt, then how
>>      is this interrupt going to be handled by this interrupt domain which
>>      is not connected to anything and implements an empty disfunctional
>>      interrupt chip?
>> 
>
> For both the cases above the job of Timesync INTR is to map the output
> register with the corresponding input.
>
> As described in section 11.3.2.1 in the TRM at:
> https://www.ti.com/lit/ug/spruiu1d/spruiu1d.pdf,
> the job of the Timesync INTR is to provide a configuration of the
> "output registers which controls the selection". Hence we just have to
> provide configuration APIs in the Timesync INTR which programs output
> registers of the Timesync INTR. About the handling of the interrupts,
> the device which receives an interrupt needs to handle the interrupt.
>
> Could you please explain why we consider these two cases to be
> different?

They are different as

  #1 Routes the signal from one IP block to another IP block

     So there is no notion of an actual interrupt, but still you use the
     interrupt domain mechanism, which requires to allocate a Linux
     interrupt number just to configure that router.

     What's the purpose of this interrupt number and the allocated
     resources behind it?

  #2 Routes the signal from an IP block to an actual interrupt "input"

     Again, this requires to allocate a Linux interrupt number which is
     disfunctional as it is not connected in the interrupt domain
     hierarchy and just provides an interrupt chip with a name and no
     actual functionality behind it.

     So the resulting real interrupt needs yet another interrupt number
     which then maps to something which actually can handle interrupts.

So in some aspect they are not that different because both have nothing
to do with the actual concept of interrupt management in the Linux
kernel.

From the kernel's interrupt handling POV this is a completely
transparent piece of hardware, which is not associated to any interrupt
handling mechanism. Just because the manual mentions INTR in the name of
the IP block does not make it part of the actual kernel interrupt
handling.

I have no idea into which subsystem such a SoC configuration belongs to,
but Greg might have an idea.

Thanks,

        tglx

Re: [RFC PATCH 1/2] irqchip: ti-tsir: Add support for Timesync Interrupt Router

[Vankar, Chintan]

Hello Thomas/Greg,

On 2/18/2025 1:47 AM, Thomas Gleixner wrote:
> Chintan!
> 
> On Sat, Feb 15 2025 at 17:19, Chintan Vankar wrote:
>> On 14/02/25 04:13, Thomas Gleixner wrote:
>>> Two questions:
>>>
>>>    1) For the case where no interrupt is involved, how is the routing
>>>       configured?
>>>
>>>    2) For the case where it routes an input line to an interupt, then how
>>>       is this interrupt going to be handled by this interrupt domain which
>>>       is not connected to anything and implements an empty disfunctional
>>>       interrupt chip?
>>>
>>
>> For both the cases above the job of Timesync INTR is to map the output
>> register with the corresponding input.
>>
>> As described in section 11.3.2.1 in the TRM at:
>> https://www.ti.com/lit/ug/spruiu1d/spruiu1d.pdf,
>> the job of the Timesync INTR is to provide a configuration of the
>> "output registers which controls the selection". Hence we just have to
>> provide configuration APIs in the Timesync INTR which programs output
>> registers of the Timesync INTR. About the handling of the interrupts,
>> the device which receives an interrupt needs to handle the interrupt.
>>
>> Could you please explain why we consider these two cases to be
>> different?
> 
> They are different as
> 
>    #1 Routes the signal from one IP block to another IP block
> 
>       So there is no notion of an actual interrupt, but still you use the
>       interrupt domain mechanism, which requires to allocate a Linux
>       interrupt number just to configure that router.
> 
>       What's the purpose of this interrupt number and the allocated
>       resources behind it?
> 
>    #2 Routes the signal from an IP block to an actual interrupt "input"
> 
>       Again, this requires to allocate a Linux interrupt number which is
>       disfunctional as it is not connected in the interrupt domain
>       hierarchy and just provides an interrupt chip with a name and no
>       actual functionality behind it.
> 
>       So the resulting real interrupt needs yet another interrupt number
>       which then maps to something which actually can handle interrupts.
> 
> So in some aspect they are not that different because both have nothing
> to do with the actual concept of interrupt management in the Linux
> kernel.
> 
>  From the kernel's interrupt handling POV this is a completely
> transparent piece of hardware, which is not associated to any interrupt
> handling mechanism. Just because the manual mentions INTR in the name of
> the IP block does not make it part of the actual kernel interrupt
> handling.
> 
> I have no idea into which subsystem such a SoC configuration belongs to,
> but Greg might have an idea.
> 

Thanks for the reviewing the patch. Since you suggest to implement it
with a different subsystem, I want your and Greg's suggestion for that.

As we discussed and also from the documentation, Timesync INTR should be
configured by programming it's output registers to control the selection
corresponding to the input. Mux-controller subsystem also works on the
similar kind of principle, to program the output by selectively choosing
from multiple input sources, I am trying to relate Timesync INTR with
that subsystem.

Could you please suggest if the implementation can be achieved using the
mux-subsystem ?


Regards,
Chintan.

> Thanks,
> 
>          tglx