| Message ID | 20250205160119.136639-2-c-vankar@ti.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | Add support for Timesync Interrupt Router | expand |
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; | ^~~~~~~~~~~
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
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 > >
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
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
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
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
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
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
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");
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