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