lkml.org 
[lkml]   [2018]   [Oct]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/2] irqchip: ti-sci-intr: Add support for Interrupt Router driver
Hi Lokesh,

On Sat, 06 Oct 2018 08:28:12 +0100,
Lokesh Vutla <lokeshvutla@ti.com> wrote:
>
> Texas Instruments' K3 generation SoCs has an IP Interrupt Router
> that does allows for multiplexing of input interrupts to host
> interrupt controller. Interrupt Router inputs are either from a
> peripheral or from an Interrupt Aggregator which is another
> interrupt controller.
>
> Configuration of the interrupt router registers can only be done by
> a system co-processor and the driver needs to send a message to this
> co processor over TISCI protocol.

I assume that this co-processor only deals with the routing itself,
and doesn't need to be talked to during interrupt processing, right?

>
> Add support for Interrupt Router driver over TISCI protocol.
>
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> ---
> MAINTAINERS | 1 +
> drivers/irqchip/Kconfig | 11 +
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-ti-sci-intr.c | 325 ++++++++++++++++++++++++++++++
> 4 files changed, 338 insertions(+)
> create mode 100644 drivers/irqchip/irq-ti-sci-intr.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a23778b68d74..cf3c834f8cee 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14626,6 +14626,7 @@ F: Documentation/devicetree/bindings/clock/ti,sci-clk.txt
> F: drivers/clk/keystone/sci-clk.c
> F: drivers/reset/reset-ti-sci.c
> F: Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt
> +F: drivers/irqchip/irq-ti-sci-intr.c
>
> THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER
> M: Hans Verkuil <hverkuil@xs4all.nl>
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 96451b581452..9a965fe22043 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -374,6 +374,17 @@ config QCOM_PDC
> Power Domain Controller driver to manage and configure wakeup
> IRQs for Qualcomm Technologies Inc (QTI) mobile chips.
>
> +config TI_SCI_INTR_IRQCHIP
> + tristate "TISCI based Interrupt Router irqchip driver"
> + depends on TI_SCI_PROTOCOL && ARCH_K3
> + select IRQ_DOMAIN
> + select IRQ_DOMAIN_HIERARCHY
> + help
> + This enables the irqchip driver support for K3 Interrupt router
> + over TI System Control Interface available on some new TI's SoCs.
> + If you wish to use interrupt router irq resources managed by the
> + TI System Controller, say Y here. Otherwise, say N.

I don't really see the point of making this user-selectable. If you're
compiling support for a given platform, this platform configuration
fragment should itself select the necessary dependencies for the
system to work as expected. Here, you are leaving the choice to the
user, with a 50% chance of getting a system that doesn't boot...

> +
> endmenu
>
> config SIFIVE_PLIC
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index b822199445ff..44bf65606d60 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -89,3 +89,4 @@ obj-$(CONFIG_GOLDFISH_PIC) += irq-goldfish-pic.o
> obj-$(CONFIG_NDS32) += irq-ativic32.o
> obj-$(CONFIG_QCOM_PDC) += qcom-pdc.o
> obj-$(CONFIG_SIFIVE_PLIC) += irq-sifive-plic.o
> +obj-$(CONFIG_TI_SCI_INTR_IRQCHIP) += irq-ti-sci-intr.o
> diff --git a/drivers/irqchip/irq-ti-sci-intr.c b/drivers/irqchip/irq-ti-sci-intr.c
> new file mode 100644
> index 000000000000..f04fe6da1b09
> --- /dev/null
> +++ b/drivers/irqchip/irq-ti-sci-intr.c
> @@ -0,0 +1,325 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Texas Instruments' K3 Interrupt Router irqchip driver
> + *
> + * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
> + * Lokesh Vutla <lokeshvutla@ti.com>
> + */
> +
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/irqchip.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/irqdomain.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 TI_SCI_IS_EVENT_IRQ BIT(31)
> +
> +#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))

nit: s/(HWIRQ)/(hwirq)/g

> +
> +/**
> + * 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 destination irq controller.
> + * @dst_id: TISCI device ID of the destination irq controller.
> + */
> +struct ti_sci_intr_irq_domain {
> + const struct ti_sci_handle *sci;
> + struct ti_sci_resource *dst_irq;
> + u16 dst_id;
> +};
> +
> +/**
> + * struct ti_sci_intr_irq_desc - Description of an Interrupt Router IRQ
> + * @src_id: TISCI device ID of the IRQ source
> + * @src_index: IRQ source index within the device.
> + * @dst_irq: Destination host IRQ.
> + */
> +struct ti_sci_intr_irq_desc {
> + u16 src_id;
> + u16 src_index;
> + u16 dst_irq;
> +};

Oh great. So this is reinventing the GICv3 ITS, only for SPIs. :-(

Now, this structure seems completely useless, see below.

> +
> +static struct irq_chip ti_sci_intr_irq_chip = {
> + .name = "INTR",
> + .irq_eoi = irq_chip_eoi_parent,
> + .irq_mask = irq_chip_mask_parent,
> + .irq_unmask = irq_chip_unmask_parent,
> + .irq_retrigger = irq_chip_retrigger_hierarchy,
> + .irq_set_type = irq_chip_set_type_parent,
> + .irq_set_affinity = irq_chip_set_affinity_parent,
> +};
> +
> +/**
> + * ti_sci_intr_irq_domain_translate() - Retrieve hwirq and type from
> + * IRQ firmware specific handler.
> + * @domain: Pointer to IRQ domain
> + * @fwspec: Pointer to IRQ specific firmware structure
> + * @hwirq: IRQ number identified by hardware
> + * @type: IRQ type
> + *
> + * Return 0 if all went ok else appropriate error.
> + */
> +static int ti_sci_intr_irq_domain_translate(struct irq_domain *domain,
> + struct irq_fwspec *fwspec,
> + unsigned long *hwirq,
> + unsigned int *type)
> +{
> + if (is_of_node(fwspec->fwnode)) {
> + if (fwspec->param_count != 3)
> + return -EINVAL;
> +
> + *hwirq = ((fwspec->param[0] & TI_SCI_DEV_ID_MASK) <<
> + TI_SCI_DEV_ID_SHIFT) |
> + (fwspec->param[1] & TI_SCI_IRQ_ID_MASK);

Maybe it would make sense to have a macro that hides this:

*hwirq = FWSPEC_TO_HWIRQ(fwspec);

> + *type = fwspec->param[2];
> +
> + return 0;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static inline void ti_sci_intr_delete_desc(struct ti_sci_intr_irq_domain *intr,
> + struct ti_sci_intr_irq_desc *desc)
> +{
> + intr->sci->ops.rm_irq_ops.free_direct_irq(intr->sci, desc->src_id,
> + desc->src_index,
> + intr->dst_id, desc->dst_irq);

This looks horrible. Why doesn't your firmware interface have a helper
functions that hides this? Something like:

ti_sci_free_direct_irq(intr, src_id, src_index, dst_irq);

and you could even add some error checking.

> + pr_debug("%s: IRQ deleted from src = %d, src_index = %d, to dst = %d, dst_irq = %d\n",
> + __func__, desc->src_id, desc->src_index, intr->dst_id,
> + desc->dst_irq);

And put this where it belongs (in the helper function).

> +}
> +
> +/**
> + * ti_sci_intr_irq_domain_free() - Free the specified IRQs from the domain.
> + * @domain: Domain to which the irqs belong
> + * @virq: Linux virtual IRQ to be freed.
> + * @nr_irqs: Number of continuous irqs to be freed
> + */
> +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 ti_sci_intr_irq_desc *desc;
> + struct irq_data *data;
> + int i;
> +
> + intr = domain->host_data;
> +
> + for (i = 0; i < nr_irqs; i++) {
> + data = irq_domain_get_irq_data(domain, virq + i);
> +
> + desc = irq_data_get_irq_chip_data(data);
> +
> + ti_sci_intr_delete_desc(intr, desc);
> + irq_domain_free_irqs_parent(domain, virq, 1);
> + irq_domain_reset_irq_data(data);
> + ti_sci_release_resource(intr->dst_irq, desc->dst_irq);
> + kfree(desc);
> + }
> +}
> +
> +/**
> + * allocate_gic_irq() - Allocate GIC specific IRQ
> + * @domain: Point to the interrupt router IRQ domain
> + * @dev: TISCI device IRQ generating the IRQ
> + * @irq: IRQ offset within the device
> + * @flags: Corresponding flags to the IRQ
> + *
> + * Returns pointer to irq descriptor if all went well else appropriate
> + * error pointer.
> + */
> +static struct ti_sci_intr_irq_desc *allocate_gic_irq(struct irq_domain *domain,
> + unsigned int virq,
> + u16 dev, u16 irq,
> + u32 flags)
> +{
> + struct ti_sci_intr_irq_domain *intr = domain->host_data;
> + struct ti_sci_intr_irq_desc *desc;
> + struct irq_fwspec fwspec;
> + int err;
> +
> + if (!irq_domain_get_of_node(domain->parent))
> + return ERR_PTR(-EINVAL);
> +
> + desc = kzalloc(sizeof(*desc), GFP_KERNEL);
> + if (!desc)
> + return ERR_PTR(-ENOMEM);
> +
> + desc->src_id = dev;
> + desc->src_index = irq;
> + desc->dst_irq = ti_sci_get_free_resource(intr->dst_irq);

I don't think this structure serves any purpose. src_id and src_index
are just a decomposition of hwirq. dst_irq is the GIC interrupt, which
is stored... by the GIC driver. Also, it is worth realising that
you're allocating per-interrupt data, but none of the per-interrupt
callbacks are using it. In my book, that's a sure sign that this
structure is pointless.

Am I missing anything here?

> +
> + fwspec.fwnode = domain->parent->fwnode;
> + fwspec.param_count = 3;
> + fwspec.param[0] = 0; /* SPI */
> + fwspec.param[1] = desc->dst_irq - 32; /* SPI offset */
> + fwspec.param[2] = flags & IRQ_TYPE_SENSE_MASK;
> +
> + err = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec);
> + if (err)
> + goto err_irqs;
> +
> + pr_debug("%s: IRQ requested from src = %d, src_index = %d, to dst = %d, dst_irq = %d\n",
> + __func__, desc->src_id, desc->src_index, intr->dst_id,
> + desc->dst_irq);
> +
> + err = intr->sci->ops.rm_irq_ops.set_direct_irq(intr->sci, desc->src_id,
> + desc->src_index,
> + intr->dst_id,
> + desc->dst_irq);

Same remarks about the horrible interface.

> + if (err) {
> + pr_err("%s: IRQ allocation failed from src = %d, src_index = %d to dst_id = %d, dst_irq = %d",
> + __func__, desc->src_id, desc->src_index, intr->dst_id,
> + desc->dst_irq);
> + goto err_msg;
> + }
> +
> + return desc;
> +
> +err_msg:
> + irq_domain_free_irqs_parent(domain, virq, 1);
> +err_irqs:
> + ti_sci_release_resource(intr->dst_irq, desc->dst_irq);
> + kfree(desc);
> + return ERR_PTR(err);
> +}
> +
> +/**
> + * ti_sci_intr_irq_domain_alloc() - Allocate Interrupt router IRQs
> + * @domain: Point to the interrupt router IRQ domain
> + * @virq: Corresponding Linux virtual IRQ number
> + * @nr_irqs: Continuous irqs to be allocated
> + * @data: Pointer to firmware specifier
> + *
> + * Return 0 if all went well else appropriate error value.
> + */
> +static int ti_sci_intr_irq_domain_alloc(struct irq_domain *domain,
> + unsigned int virq, unsigned int nr_irqs,
> + void *data)
> +{
> + struct irq_fwspec *fwspec = data;
> + struct ti_sci_intr_irq_desc *desc;
> + unsigned long hwirq;
> + u16 src_id, src_index;
> + int i, err;
> + u32 type;
> +
> + err = ti_sci_intr_irq_domain_translate(domain, fwspec, &hwirq, &type);
> + if (err)
> + return err;
> +
> + src_id = HWIRQ_TO_DEVID(hwirq);
> + src_index = HWIRQ_TO_IRQID(hwirq);
> +
> + for (i = 0; i < nr_irqs; i++) {
> + desc = allocate_gic_irq(domain, virq + i, src_id, src_index + i,
> + type);
> + if (IS_ERR(desc))
> + /* ToDO: Clean already allocated IRQs */
> + return PTR_ERR(desc);

Please address this. But it also worth realising that this code will
never be called with nr_irqs!=1 (that's only for things like PCI
Multi-MSI).

> +
> + err = irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> + &ti_sci_intr_irq_chip,
> + desc);
> + if (err)
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +static const struct irq_domain_ops ti_sci_intr_irq_domain_ops = {
> + .alloc = ti_sci_intr_irq_domain_alloc,
> + .free = ti_sci_intr_irq_domain_free,
> + .translate = ti_sci_intr_irq_domain_translate,
> +};
> +
> +static int ti_sci_intr_irq_domain_probe(struct platform_device *pdev)
> +{
> + struct irq_domain *parent_domain, *domain;
> + struct ti_sci_intr_irq_domain *intr;
> + struct device_node *parent_node;
> + struct device *dev = &pdev->dev;
> + int ret;
> +
> + parent_node = of_irq_find_parent(dev_of_node(dev));
> + if (!parent_node) {
> + dev_err(dev, "Failed to get IRQ parent node\n");
> + return -ENODEV;
> + }
> +
> + parent_domain = irq_find_host(parent_node);
> + if (!parent_domain) {
> + dev_err(dev, "Failed to find IRQ parent domain\n");
> + return -ENODEV;
> + }
> +
> + intr = devm_kzalloc(dev, sizeof(*intr), GFP_KERNEL);
> + if (!intr)
> + return -ENOMEM;
> +
> + intr->sci = devm_ti_sci_get_by_phandle(dev, "ti,sci");
> + if (IS_ERR(intr->sci)) {
> + ret = PTR_ERR(intr->sci);
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "ti,sci read fail %d\n", ret);
> + intr->sci = NULL;
> + return ret;
> + }
> +
> + intr->dst_irq = devm_ti_sci_get_of_resource(intr->sci, dev,
> + "ti,sci-rm-range-girq");
> + if (IS_ERR(intr->dst_irq)) {
> + dev_err(dev, "Destination irq resource allocation failed\n");
> + return PTR_ERR(intr->dst_irq);
> + }
> +
> + ret = of_property_read_u32(dev_of_node(dev), "ti,sci-dst-id",
> + (u32 *)&intr->dst_id);
> + if (ret) {
> + dev_err(dev, "missing 'ti,sci-dst-id' property\n");
> + return -EINVAL;
> + }

Do you expect other drivers to require similar resource request? If
so, It might be worth getting the firmware interface to do that
work. Specially the "give me my SCI" part.

> +
> + domain = irq_domain_add_hierarchy(parent_domain, 0, 0, dev_of_node(dev),
> + &ti_sci_intr_irq_domain_ops, intr);
> + if (!domain) {
> + dev_err(dev, "Failed to allocate IRQ domain\n");
> + return -ENOMEM;
> + }
> +
> + return 0;
> +}
> +
> +static const struct of_device_id ti_sci_intr_irq_domain_of_match[] = {
> + { .compatible = "ti,sci-intr", },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, ti_sci_intr_irq_domain_of_match);
> +
> +static struct platform_driver ti_sci_intr_irq_domain_driver = {
> + .probe = ti_sci_intr_irq_domain_probe,
> + .driver = {
> + .name = "ti-sci-intr",
> + .of_match_table = ti_sci_intr_irq_domain_of_match,
> + },
> +};
> +module_platform_driver(ti_sci_intr_irq_domain_driver);
> +
> +MODULE_AUTHOR("Lokesh Vutla <lokeshvutla@ticom>");
> +MODULE_DESCRIPTION("K3 Interrupt Router driver over TI SCI protocol");
> +MODULE_LICENSE("GPL v2");
> --
> 2.19.0
>

Thanks,

M.

--
Jazz is not dead, it just smell funny.

\
 
 \ /
  Last update: 2018-10-06 11:56    [W:0.086 / U:0.156 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site