Hi Lokesh, On Sat, 06 Oct 2018 08:28:12 +0100, Lokesh Vutla <lokeshvutla@xxxxxx> 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@xxxxxx> > --- > 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@xxxxxxxxx> > 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@xxxxxx> > + */ > + > +#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.
