Re: [PATCH v3 6/9] irqchip/ti-sci-intr: Add support for INTR being a parent to INTR

[Lokesh Vutla <lokeshvutla@ti.com>]

On 25/07/20 7:36 pm, Marc Zyngier wrote:
> On Fri, 24 Jul 2020 15:18:34 +0100,
> Lokesh Vutla <lokeshvutla@ti.com> wrote:
>>
>> Driver assumes that Interrupt parent to Interrupt router is always GIC.
>> This is not true always and an Interrupt Router can be a parent to
>> Interrupt Router. Update the driver to detect the parent and request the
>> parent irqs accordingly.
>>
>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>> ---
>>  drivers/irqchip/irq-ti-sci-intr.c | 150 ++++++++++++++++++------------
>>  1 file changed, 91 insertions(+), 59 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-ti-sci-intr.c b/drivers/irqchip/irq-ti-sci-intr.c
>> index 59d51a20bbd8..0b73816e77fc 100644
>> --- a/drivers/irqchip/irq-ti-sci-intr.c
>> +++ b/drivers/irqchip/irq-ti-sci-intr.c
>> @@ -17,29 +17,18 @@
>>  #include <linux/of_irq.h>
>>  #include <linux/soc/ti/ti_sci_protocol.h>
>>  
>> -#define TI_SCI_DEV_ID_MASK	0xffff
>> -#define TI_SCI_DEV_ID_SHIFT	16
>> -#define TI_SCI_IRQ_ID_MASK	0xffff
>> -#define TI_SCI_IRQ_ID_SHIFT	0
>> -#define HWIRQ_TO_DEVID(hwirq)	(((hwirq) >> (TI_SCI_DEV_ID_SHIFT)) & \
>> -				 (TI_SCI_DEV_ID_MASK))
>> -#define HWIRQ_TO_IRQID(hwirq)	((hwirq) & (TI_SCI_IRQ_ID_MASK))
>> -#define TO_HWIRQ(dev, index)	((((dev) & TI_SCI_DEV_ID_MASK) << \
>> -				 TI_SCI_DEV_ID_SHIFT) | \
>> -				((index) & TI_SCI_IRQ_ID_MASK))
>> -
>>  /**
>>   * struct ti_sci_intr_irq_domain - Structure representing a TISCI based
>>   *				   Interrupt Router IRQ domain.
>>   * @sci:	Pointer to TISCI handle
>> - * @dst_irq:	TISCI resource pointer representing GIC irq controller.
>> - * @dst_id:	TISCI device ID of the GIC irq controller.
>> + * @out_irqs:	TISCI resource pointer representing INTR irqs.
>> + * @pdev:	Pointer to platform device.
>>   * @type:	Specifies the trigger type supported by this Interrupt Router
>>   */
>>  struct ti_sci_intr_irq_domain {
>>  	const struct ti_sci_handle *sci;
>> -	struct ti_sci_resource *dst_irq;
>> -	u32 dst_id;
>> +	struct ti_sci_resource *out_irqs;
>> +	struct platform_device *pdev;
>>  	u32 type;
>>  };
>>  
>> @@ -70,15 +59,44 @@ static int ti_sci_intr_irq_domain_translate(struct irq_domain *domain,
>>  {
>>  	struct ti_sci_intr_irq_domain *intr = domain->host_data;
>>  
>> -	if (fwspec->param_count != 2)
>> +	if (fwspec->param_count != 1)
>>  		return -EINVAL;
>>  
>> -	*hwirq = TO_HWIRQ(fwspec->param[0], fwspec->param[1]);
>> +	*hwirq = fwspec->param[0];
>>  	*type = intr->type;
>>  
>>  	return 0;
>>  }
>>  
>> +/**
>> + * ti_sci_intr_xlate_irq() - Translate hwirq to parent's hwirq.
>> + * @intr:	IRQ domain corresponding to Interrupt Router
>> + * @irq:	Hardware irq corresponding to the above irq domain
>> + *
>> + * Return parent irq number if translation is available else -ENOENT.
>> + */
>> +static int ti_sci_intr_xlate_irq(struct ti_sci_intr_irq_domain *intr, u32 irq)
>> +{
>> +	struct device_node *np = dev_of_node(&intr->pdev->dev);
>> +	u32 base, pbase, size, len;
>> +	const __be32 *range;
>> +
>> +	range = of_get_property(np, "ti,interrupt-ranges", &len);
>> +	if (!range)
>> +		return irq;
>> +
>> +	for (len /= sizeof(*range); len >= 3; len -= 3) {
>> +		base = be32_to_cpu(*range++);
>> +		pbase = be32_to_cpu(*range++);
>> +		size = be32_to_cpu(*range++);
>> +
>> +		if (base <= irq && irq < base + size)
>> +			return irq - base + pbase;
>> +	}
>> +
>> +	return -ENOENT;
>> +}
>> +
>>  /**
>>   * ti_sci_intr_irq_domain_free() - Free the specified IRQs from the domain.
>>   * @domain:	Domain to which the irqs belong
>> @@ -89,66 +107,77 @@ static void ti_sci_intr_irq_domain_free(struct irq_domain *domain,
>>  					unsigned int virq, unsigned int nr_irqs)
>>  {
>>  	struct ti_sci_intr_irq_domain *intr = domain->host_data;
>> -	struct irq_data *data, *parent_data;
>> -	u16 dev_id, irq_index;
>> +	struct irq_data *data;
>> +	int out_irq;
>>  
>> -	parent_data = irq_domain_get_irq_data(domain->parent, virq);
>>  	data = irq_domain_get_irq_data(domain, virq);
>> -	irq_index = HWIRQ_TO_IRQID(data->hwirq);
>> -	dev_id = HWIRQ_TO_DEVID(data->hwirq);
>> +	out_irq = (uintptr_t)data->chip_data;
>>  
>> -	intr->sci->ops.rm_irq_ops.free_irq(intr->sci, dev_id, irq_index,
>> -					   intr->dst_id, parent_data->hwirq);
>> -	ti_sci_release_resource(intr->dst_irq, parent_data->hwirq);
>> +	intr->sci->ops.rm_irq_ops.free_irq(intr->sci,
>> +					   intr->pdev->id, data->hwirq,
>> +					   intr->pdev->id, out_irq);
>> +	ti_sci_release_resource(intr->out_irqs, out_irq);
>>  	irq_domain_free_irqs_parent(domain, virq, 1);
>>  	irq_domain_reset_irq_data(data);
>>  }
>>  
>>  /**
>> - * ti_sci_intr_alloc_gic_irq() - Allocate GIC specific IRQ
>> + * ti_sci_intr_alloc_parent_irq() - Allocate parent IRQ
>>   * @domain:	Pointer to the interrupt router IRQ domain
>>   * @virq:	Corresponding Linux virtual IRQ number
>>   * @hwirq:	Corresponding hwirq for the IRQ within this IRQ domain
>>   *
>> - * Returns 0 if all went well else appropriate error pointer.
>> + * Returns parent irq if all went well else appropriate error pointer.
>>   */
>> -static int ti_sci_intr_alloc_gic_irq(struct irq_domain *domain,
>> -				     unsigned int virq, u32 hwirq)
>> +static int ti_sci_intr_alloc_parent_irq(struct irq_domain *domain,
>> +					unsigned int virq, u32 hwirq)
>>  {
>>  	struct ti_sci_intr_irq_domain *intr = domain->host_data;
>> +	struct device_node *parent_node;
>>  	struct irq_fwspec fwspec;
>> -	u16 dev_id, irq_index;
>> -	u16 dst_irq;
>> -	int err;
>> -
>> -	dev_id = HWIRQ_TO_DEVID(hwirq);
>> -	irq_index = HWIRQ_TO_IRQID(hwirq);
>> +	u16 out_irq, p_hwirq;
>> +	int err = 0;
>>  
>> -	dst_irq = ti_sci_get_free_resource(intr->dst_irq);
>> -	if (dst_irq == TI_SCI_RESOURCE_NULL)
>> +	out_irq = ti_sci_get_free_resource(intr->out_irqs);
>> +	if (out_irq == TI_SCI_RESOURCE_NULL)
>>  		return -EINVAL;
>>  
>> -	fwspec.fwnode = domain->parent->fwnode;
>> -	fwspec.param_count = 3;
>> -	fwspec.param[0] = 0;	/* SPI */
>> -	fwspec.param[1] = dst_irq - 32; /* SPI offset */
>> -	fwspec.param[2] = intr->type;
>> +	p_hwirq = ti_sci_intr_xlate_irq(intr, out_irq);
>> +	if (p_hwirq < 0)
>> +		goto err_irqs;
>> +
>> +	parent_node = of_irq_find_parent(dev_of_node(&intr->pdev->dev));
>> +	fwspec.fwnode = of_node_to_fwnode(parent_node);
>> +	if (of_property_read_u32(parent_node, "#interrupt-cells",
>> +				 &fwspec.param_count))
>> +		goto err_irqs;
>> +
>> +	if (of_device_is_compatible(parent_node, "arm,gic-v3")) {
>> +		/* Parent is GIC */
>> +		fwspec.param[0] = 0;	/* SPI */
>> +		fwspec.param[1] = p_hwirq - 32; /* SPI offset */
>> +		fwspec.param[2] = intr->type;
> 
> So you pick up param_count from the parent node, but:
> - you don't check that you have indeed at least 3 cells (unlikely to
>   fail, but still)
> - you leave the rest of the param array uninitialised if param_count
>   was > 3, which is allowed by the GICv3 binding.
> 
> This definitely needs fixing.


Initially I was using count for determining the parent, which was wrong then I
moved to compatible and kept the param_count from parent as is. I could just
assign fwspec.count = 3 directly here. WIll fix in v4.

> 
>> +	} else {
>> +		/* Parent is Interrupt Router */
>> +		fwspec.param[0] = p_hwirq;

and count = 1 here.

>> +	}
>>  
>>  	err = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec);
>>  	if (err)
>>  		goto err_irqs;
>>  
>> -	err = intr->sci->ops.rm_irq_ops.set_irq(intr->sci, dev_id, irq_index,
>> -						intr->dst_id, dst_irq);
>> +	err = intr->sci->ops.rm_irq_ops.set_irq(intr->sci,
>> +						intr->pdev->id, hwirq,
>> +						intr->pdev->id, out_irq);
>>  	if (err)
>>  		goto err_msg;
>>  
>> -	return 0;
>> +	return p_hwirq;
>>  
>>  err_msg:
>>  	irq_domain_free_irqs_parent(domain, virq, 1);
>>  err_irqs:
>> -	ti_sci_release_resource(intr->dst_irq, dst_irq);
>> +	ti_sci_release_resource(intr->out_irqs, out_irq);
>>  	return err;
>>  }
>>  
>> @@ -168,18 +197,19 @@ static int ti_sci_intr_irq_domain_alloc(struct irq_domain *domain,
>>  	struct irq_fwspec *fwspec = data;
>>  	unsigned long hwirq;
>>  	unsigned int flags;
>> -	int err;
>> +	int err, p_hwirq;
>>  
>>  	err = ti_sci_intr_irq_domain_translate(domain, fwspec, &hwirq, &flags);
>>  	if (err)
>>  		return err;
>>  
>> -	err = ti_sci_intr_alloc_gic_irq(domain, virq, hwirq);
>> -	if (err)
>> -		return err;
>> +	p_hwirq = ti_sci_intr_alloc_parent_irq(domain, virq, hwirq);
>> +	if (p_hwirq < 0)
>> +		return p_hwirq;
>>  
>>  	irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
>> -				      &ti_sci_intr_irq_chip, NULL);
>> +				      &ti_sci_intr_irq_chip,
>> +				      (void *)(uintptr_t)p_hwirq);
>>  
>>  	return 0;
>>  }
>> @@ -214,6 +244,7 @@ static int ti_sci_intr_irq_domain_probe(struct platform_device *pdev)
>>  	if (!intr)
>>  		return -ENOMEM;
>>  
>> +	intr->pdev = pdev;
>>  	ret = of_property_read_u32(dev_of_node(dev), "ti,intr-trigger-type",
>>  				   &intr->type);
>>  	if (ret) {
>> @@ -230,19 +261,18 @@ static int ti_sci_intr_irq_domain_probe(struct platform_device *pdev)
>>  		return ret;
>>  	}
>>  
>> -	ret = of_property_read_u32(dev_of_node(dev), "ti,sci-dst-id",
>> -				   &intr->dst_id);
>> +	ret = of_property_read_u32(dev_of_node(dev), "ti,sci-dev-id",
>> +				   &pdev->id);
> 
> This feels very dodgy. You are hijacking a random field in the
> platform device data structure, which shouldn't be any of your

pdev->id is pretty much unused field today. I was kind of hoping that in future
it can be utilized to populated field with TISCI device IDs in some generic
manner. So thought of using it now.

> business. What was wrong with having a separate field for something
> that is obviously platform specific?

No reason. I could just add a new filed in the intr structure.

Thanks and regards,
Lokesh