| Message ID | 20181006072812.15814-1-lokeshvutla@ti.com |
|---|---|
| Headers | show |
| Series | irqchip: ti-sci-intr: Add support for K3 Interrupt Router | expand |
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.
Hi Marc, On 10/6/2018 3:25 PM, Marc Zyngier wrote: > 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? Yes, that's 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... There are 2 reasons why I made it tristate: - Not all interrupts go through this irqchip(At least in the AM6 SoC using this). Most of the legacy peripherals still are directly connected to GIC - TI_SCI_PROTOCOL is defined as tristate. If you still feel I should not make it user-selectable, I can drop it. > >> + >> 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 okay. > >> + >> +/** >> + * 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: okay. > > *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. All existing TISCI users follow the same convention, so I did not bother adding any such wrapper. Will update TISCI with these wrappers and see what firmware maintainer says. > >> + 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. hmm..you are right, these 3 fields can be dropped completely. > > 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). will fix it in v2. > >> + >> + 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. I tried to consolidate sci resource part under devm_ti_sci_get_of_resource() api but dst-id is something that is used by irqchip driver. So couldn't consolidate it and had to get it from dt in the driver probe. Thanks and regards, Lokesh
Hi Lokesh, On 08/10/18 10:48, Lokesh Vutla wrote: > Hi Marc, > > On 10/6/2018 3:25 PM, Marc Zyngier wrote: >> 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? > > Yes, that's 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... > > There are 2 reasons why I made it tristate: > - Not all interrupts go through this irqchip(At least in the AM6 SoC > using this). Most of the legacy peripherals still are directly connected > to GIC > - TI_SCI_PROTOCOL is defined as tristate. But as you said, these are "legacy" interrupts, and most of the interesting stuff is routed through the system controller. We also try not to have core interrupt controllers as modules. As for having the firmware interface as a module, I wonder what the use-case is. > If you still feel I should not make it user-selectable, I can drop it. I really wonder what the added value is for the user. > >> >>> + >>> 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 > > okay. > >> >>> + >>> +/** >>> + * 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: > > okay. > >> >> *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. > > All existing TISCI users follow the same convention, so I did not bother adding > any such wrapper. Will update TISCI with these wrappers and see what firmware > maintainer says. Frankly, exposing all kind of data structures to the world is a pretty poor form of abstraction, which is what the firmware is supposed to provide. I'd strongly suggest that include/linux/soc/ti/ti_sci_protocol.h gets cleaned up, and that the whole ti_sci_ops disappears from the that file. Nobody outside of the firmware *implementation* needs to know about its, and it would be much better served by a set of helpers. Finally, please make the TISCI interrupt management part of this series, so that I can review it as part of the code that uses it. Thanks, M.
On Monday 08 October 2018 06:30 PM, Marc Zyngier wrote: > Hi Lokesh, > > On 08/10/18 10:48, Lokesh Vutla wrote: >> Hi Marc, >> >> On 10/6/2018 3:25 PM, Marc Zyngier wrote: >>> 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? >> >> Yes, that's 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... >> >> There are 2 reasons why I made it tristate: >> - Not all interrupts go through this irqchip(At least in the AM6 SoC >> using this). Most of the legacy peripherals still are directly connected >> to GIC >> - TI_SCI_PROTOCOL is defined as tristate. > > But as you said, these are "legacy" interrupts, and most of the > interesting stuff is routed through the system controller. We also try > not to have core interrupt controllers as modules. As for having the > firmware interface as a module, I wonder what the use-case is. > >> If you still feel I should not make it user-selectable, I can drop it. > > I really wonder what the added value is for the user. okay, will not make it use configurable in v2. > >> >>> >>>> + >>>> 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 >> >> okay. >> >>> >>>> + >>>> +/** >>>> + * 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: >> >> okay. >> >>> >>> *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. >> >> All existing TISCI users follow the same convention, so I did not bother adding >> any such wrapper. Will update TISCI with these wrappers and see what firmware >> maintainer says. > > Frankly, exposing all kind of data structures to the world is a pretty > poor form of abstraction, which is what the firmware is supposed to > provide. > > I'd strongly suggest that include/linux/soc/ti/ti_sci_protocol.h gets > cleaned up, and that the whole ti_sci_ops disappears from the that file. > Nobody outside of the firmware *implementation* needs to know about its, > and it would be much better served by a set of helpers. > > Finally, please make the TISCI interrupt management part of this series, > so that I can review it as part of the code that uses it. Sure, my next version will include TISCI interrupt management as well. Thanks and regards, Lokesh