On 25/07/20 7:36 pm, Marc Zyngier wrote:
> On Fri, 24 Jul 2020 15:18:34 +0100,
> Lokesh Vutla <lokeshvutla@xxxxxx> 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@xxxxxx>
>> ---
>> 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
