| Message ID | 20190420100950.7997-1-lokeshvutla@ti.com |
|---|---|
| Headers | show |
| Series | Add support for TISCI Interrupt controller drivers | expand |
Hi Marc, On 20/04/19 3:39 PM, Lokesh Vutla wrote: > Texas Instruments' K3 generation SoCs has an IP Interrupt Aggregator > which is an interrupt controller that does the following: > - Converts events to interrupts that can be understood by > an interrupt router. > - Allows for multiplexing of events to interrupts. > > Configuration of the interrupt aggregator 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. This patch adds support for Interrupt > Aggregator irqdomain. > > Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> > --- > Changes since v6: > - Updated commit message. > - Arranged header files in alphabetical order > - Included vint_bit in struct ti_sci_inta_event_desc > - With the above change now the chip_data is event_desc instead of vint_desc > - No loops are used in atomic contexts. > - Fixed locking issue while freeing parent virq > - Fixed few other cosmetic changes. > > MAINTAINERS | 1 + > drivers/irqchip/Kconfig | 11 + > drivers/irqchip/Makefile | 1 + > drivers/irqchip/irq-ti-sci-inta.c | 589 ++++++++++++++++++++++++++++++ > 4 files changed, 602 insertions(+) > create mode 100644 drivers/irqchip/irq-ti-sci-inta.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 461db0a8233f..055467fb2019 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -15352,6 +15352,7 @@ F: drivers/reset/reset-ti-sci.c > F: Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt > F: Documentation/devicetree/bindings/interrupt-controller/ti,sci-inta.txt > F: drivers/irqchip/irq-ti-sci-intr.c > +F: drivers/irqchip/irq-ti-sci-inta.c > > Texas Instruments ASoC drivers > M: Peter Ujfalusi <peter.ujfalusi@ti.com> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > index 4f84e7902626..7c84a71bcd88 100644 > --- a/drivers/irqchip/Kconfig > +++ b/drivers/irqchip/Kconfig > @@ -436,6 +436,17 @@ config TI_SCI_INTR_IRQCHIP > If you wish to use interrupt router irq resources managed by the > TI System Controller, say Y here. Otherwise, say N. > > +config TI_SCI_INTA_IRQCHIP > + bool > + depends on TI_SCI_PROTOCOL > + select IRQ_DOMAIN > + select IRQ_DOMAIN_HIERARCHY > + help > + This enables the irqchip driver support for K3 Interrupt aggregator > + over TI System Control Interface available on some new TI's SoCs. > + If you wish to use interrupt aggregator irq resources managed by the > + TI System Controller, say Y here. Otherwise, say N. > + > endmenu > > config SIFIVE_PLIC > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > index fa5c865788b5..8a33013da953 100644 > --- a/drivers/irqchip/Makefile > +++ b/drivers/irqchip/Makefile > @@ -98,3 +98,4 @@ obj-$(CONFIG_IMX_IRQSTEER) += irq-imx-irqsteer.o > obj-$(CONFIG_MADERA_IRQ) += irq-madera.o > obj-$(CONFIG_LS1X_IRQ) += irq-ls1x.o > obj-$(CONFIG_TI_SCI_INTR_IRQCHIP) += irq-ti-sci-intr.o > +obj-$(CONFIG_TI_SCI_INTA_IRQCHIP) += irq-ti-sci-inta.o > diff --git a/drivers/irqchip/irq-ti-sci-inta.c b/drivers/irqchip/irq-ti-sci-inta.c > new file mode 100644 > index 000000000000..71e5b45ab4ce > --- /dev/null > +++ b/drivers/irqchip/irq-ti-sci-inta.c > @@ -0,0 +1,589 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Texas Instruments' K3 Interrupt Aggregator irqchip driver > + * > + * Copyright (C) 2018-2019 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/irqdomain.h> > +#include <linux/interrupt.h> > +#include <linux/msi.h> > +#include <linux/module.h> > +#include <linux/moduleparam.h> > +#include <linux/of_address.h> > +#include <linux/of_irq.h> > +#include <linux/of_platform.h> > +#include <linux/irqchip/chained_irq.h> > +#include <linux/soc/ti/ti_sci_protocol.h> > +#include <asm-generic/msi.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 MAX_EVENTS_PER_VINT 64 > +#define VINT_ENABLE_SET_OFFSET 0x0 > +#define VINT_ENABLE_CLR_OFFSET 0x8 > +#define VINT_STATUS_OFFSET 0x18 > + > +/** > + * struct ti_sci_inta_event_desc - Description of an event coming to > + * Interrupt Aggregator. This serves > + * as a mapping table for global event, > + * hwirq and vint bit. > + * @global_event: Global event number corresponding to this event > + * @hwirq: Hwirq of the incoming interrupt > + * @vint_bit: Corresponding vint bit to which this event is attached. > + */ > +struct ti_sci_inta_event_desc { > + u16 global_event; > + u32 hwirq; > + u8 vint_bit; > +}; > + > +/** > + * struct ti_sci_inta_vint_desc - Description of a virtual interrupt coming out > + * of Interrupt Aggregator. > + * @domain: Pointer to IRQ domain to which this vint belongs. > + * @list: List entry for the vint list > + * @event_map: Bitmap to manage the allocation of events to vint. > + * @event_mutex: Mutex to protect allocation of events. > + * @events: Array of event descriptors assigned to this vint. > + * @parent_virq: Linux IRQ number that gets attached to parent > + * @vint_id: TISCI vint ID > + */ > +struct ti_sci_inta_vint_desc { > + struct irq_domain *domain; > + struct list_head list; > + DECLARE_BITMAP(event_map, MAX_EVENTS_PER_VINT); > + /* Mutex to protect allocation of events */ > + struct mutex event_mutex; > + struct ti_sci_inta_event_desc events[MAX_EVENTS_PER_VINT]; > + unsigned int parent_virq; > + u16 vint_id; > +}; > + > +/** > + * struct ti_sci_inta_irq_domain - Structure representing a TISCI based > + * Interrupt Aggregator IRQ domain. > + * @sci: Pointer to TISCI handle > + * @vint: TISCI resource pointer representing IA inerrupts. > + * @global_event: TISCI resource pointer representing global events. > + * @vint_list: List of the vints active in the system > + * @vint_mutex: Mutex to protect vint_list > + * @base: Base address of the memory mapped IO registers > + * @pdev: Pointer to platform device. > + */ > +struct ti_sci_inta_irq_domain { > + const struct ti_sci_handle *sci; > + struct ti_sci_resource *vint; > + struct ti_sci_resource *global_event; > + struct list_head vint_list; > + /* Mutex to protect vint list */ > + struct mutex vint_mutex; > + void __iomem *base; > + struct platform_device *pdev; > +}; > + > +#define to_vint_desc(e, i) container_of(e, struct ti_sci_inta_vint_desc, \ > + events[i]) > + > +/** > + * ti_sci_inta_irq_handler() - Chained IRQ handler for the vint irqs > + * @desc: Pointer to irq_desc corresponding to the irq > + */ > +static void ti_sci_inta_irq_handler(struct irq_desc *desc) > +{ > + struct ti_sci_inta_vint_desc *vint_desc; > + struct ti_sci_inta_irq_domain *inta; > + struct irq_domain *domain; > + unsigned int virq, bit; > + unsigned long val; > + > + vint_desc = irq_desc_get_handler_data(desc); > + domain = vint_desc->domain; > + inta = domain->host_data; > + > + chained_irq_enter(irq_desc_get_chip(desc), desc); > + > + val = readq_relaxed(inta->base + vint_desc->vint_id * 0x1000 + > + VINT_STATUS_OFFSET); > + > + for_each_set_bit(bit, &val, MAX_EVENTS_PER_VINT) { > + virq = irq_find_mapping(domain, vint_desc->events[bit].hwirq); > + if (virq) > + generic_handle_irq(virq); > + } > + > + chained_irq_exit(irq_desc_get_chip(desc), desc); > +} > + > +/** > + * ti_sci_inta_alloc_parent_irq() - Allocate parent irq to Interrupt aggregator > + * @domain: IRQ domain corresponding to Interrupt Aggregator > + * > + * Return 0 if all went well else corresponding error value. > + */ > +static struct ti_sci_inta_vint_desc *ti_sci_inta_alloc_parent_irq(struct irq_domain *domain) > +{ > + struct ti_sci_inta_irq_domain *inta = domain->host_data; > + struct ti_sci_inta_vint_desc *vint_desc; > + struct irq_fwspec parent_fwspec; > + unsigned int parent_virq; > + u16 vint_id; > + > + vint_id = ti_sci_get_free_resource(inta->vint); > + if (vint_id == TI_SCI_RESOURCE_NULL) > + return ERR_PTR(-EINVAL); > + > + vint_desc = kzalloc(sizeof(*vint_desc), GFP_KERNEL); > + if (!vint_desc) > + return ERR_PTR(-ENOMEM); > + > + mutex_init(&vint_desc->event_mutex); > + > + vint_desc->domain = domain; > + vint_desc->vint_id = vint_id; > + INIT_LIST_HEAD(&vint_desc->list); > + > + parent_fwspec.fwnode = of_node_to_fwnode(of_irq_find_parent(dev_of_node(&inta->pdev->dev))); > + parent_fwspec.param_count = 3; > + parent_fwspec.param[0] = inta->pdev->id; > + parent_fwspec.param[1] = vint_desc->vint_id; > + parent_fwspec.param[2] = 1; > + > + parent_virq = irq_create_fwspec_mapping(&parent_fwspec); > + if (parent_virq <= 0) { > + kfree(vint_desc); > + return ERR_PTR(parent_virq); > + } > + vint_desc->parent_virq = parent_virq; > + > + mutex_lock(&inta->vint_mutex); > + list_add_tail(&vint_desc->list, &inta->vint_list); > + mutex_unlock(&inta->vint_mutex); > + > + irq_set_chained_handler_and_data(vint_desc->parent_virq, > + ti_sci_inta_irq_handler, vint_desc); > + > + return vint_desc; > +} > + > +/** > + * ti_sci_inta_alloc_event() - Attach an event to a IA vint. > + * @vint_desc: Pointer to vint_desc to which the event gets attached > + * @free_bit: Bit inside vint to which event gets attached > + * @hwirq: hwirq of the input event > + * > + * Return event_desc pointer if all went ok else appropriate error value. > + */ > +static struct ti_sci_inta_event_desc *ti_sci_inta_alloc_event(struct ti_sci_inta_vint_desc *vint_desc, > + u16 free_bit, > + u32 hwirq) > +{ > + struct ti_sci_inta_irq_domain *inta = vint_desc->domain->host_data; > + struct ti_sci_inta_event_desc *event_desc; > + u16 dev_id, dev_index; > + int err; > + > + dev_id = HWIRQ_TO_DEVID(hwirq); > + dev_index = HWIRQ_TO_IRQID(hwirq); > + > + mutex_lock(&vint_desc->event_mutex); > + event_desc = &vint_desc->events[free_bit]; > + event_desc->hwirq = hwirq; > + event_desc->vint_bit = free_bit; > + event_desc->global_event = ti_sci_get_free_resource(inta->global_event); > + if (event_desc->global_event == TI_SCI_RESOURCE_NULL) { > + err = -EINVAL; > + goto free_vint_bit; > + } > + > + err = inta->sci->ops.rm_irq_ops.set_event_map(inta->sci, > + dev_id, dev_index, > + inta->pdev->id, > + vint_desc->vint_id, > + event_desc->global_event, > + free_bit); > + if (err) > + goto free_global_event; > + > + mutex_unlock(&vint_desc->event_mutex); > + return event_desc; > +free_global_event: > + ti_sci_release_resource(inta->global_event, event_desc->global_event); > +free_vint_bit: > + clear_bit(free_bit, vint_desc->event_map); > + mutex_unlock(&vint_desc->event_mutex); > + return ERR_PTR(err); > +} > + > +/** > + * ti_sci_inta_alloc_irq() - Allocate an irq within INTA domain > + * @domain: irq_domain pointer corresponding to INTA > + * @hwirq: hwirq of the input event > + * > + * Note: Allocation happens in the following manner: > + * - Find a free bit available in any of the vints available in the list. > + * - If not found, allocate a vint from the vint pool > + * - Attach the free bit to input hwirq. > + * Return event_desc if all went ok else appropriate error value. > + */ > +static struct ti_sci_inta_event_desc *ti_sci_inta_alloc_irq(struct irq_domain *domain, > + u32 hwirq) > +{ > + struct ti_sci_inta_irq_domain *inta = domain->host_data; > + struct ti_sci_inta_vint_desc *vint_desc = NULL; > + u16 free_bit; > + > + mutex_lock(&inta->vint_mutex); > + list_for_each_entry(vint_desc, &inta->vint_list, list) { > + mutex_lock(&vint_desc->event_mutex); > + free_bit = find_first_zero_bit(vint_desc->event_map, > + MAX_EVENTS_PER_VINT); > + if (free_bit != MAX_EVENTS_PER_VINT) { > + set_bit(free_bit, vint_desc->event_map); > + mutex_unlock(&vint_desc->event_mutex); > + mutex_unlock(&inta->vint_mutex); > + goto alloc_event; > + } > + mutex_unlock(&vint_desc->event_mutex); > + } > + mutex_unlock(&inta->vint_mutex); > + > + /* No free bits available. Allocate a new vint */ > + vint_desc = ti_sci_inta_alloc_parent_irq(domain); > + if (IS_ERR(vint_desc)) > + return ERR_PTR(PTR_ERR(vint_desc)); > + > + mutex_lock(&vint_desc->event_mutex); > + free_bit = find_first_zero_bit(vint_desc->event_map, > + MAX_EVENTS_PER_VINT); > + set_bit(free_bit, vint_desc->event_map); > + mutex_unlock(&vint_desc->event_mutex); > + > +alloc_event: > + return ti_sci_inta_alloc_event(vint_desc, free_bit, hwirq); > +} > + > +/** > + * ti_sci_inta_free_parent_irq() - Free a parent irq to INTA > + * @inta: Pointer to inta domain. > + * @vint_desc: Pointer to vint_desc that needs to be freed. > + */ > +static void ti_sci_inta_free_parent_irq(struct ti_sci_inta_irq_domain *inta, > + struct ti_sci_inta_vint_desc *vint_desc) > +{ > + mutex_lock(&inta->vint_mutex); > + mutex_lock(&vint_desc->event_mutex); > + if (find_first_bit(vint_desc->event_map, MAX_EVENTS_PER_VINT) == MAX_EVENTS_PER_VINT) { > + list_del(&vint_desc->list); > + mutex_unlock(&vint_desc->event_mutex); > + mutex_unlock(&inta->vint_mutex); > + ti_sci_release_resource(inta->vint, vint_desc->vint_id); > + irq_dispose_mapping(vint_desc->parent_virq); > + kfree(vint_desc); > + return; > + } > + mutex_unlock(&vint_desc->event_mutex); > + mutex_unlock(&inta->vint_mutex); > +} > + > +/** > + * ti_sci_inta_free_irq() - Free an IRQ within INTA domain > + * @event_desc: Pointer to event_desc that needs to be freed. > + * @hwirq: Hwirq number within INTA domain that needs to be freed > + */ > +static void ti_sci_inta_free_irq(struct ti_sci_inta_event_desc *event_desc, > + u32 hwirq) > +{ > + struct ti_sci_inta_vint_desc *vint_desc; > + struct ti_sci_inta_irq_domain *inta; > + > + vint_desc = to_vint_desc(event_desc, event_desc->vint_bit); > + inta = vint_desc->domain->host_data; > + /* free event irq */ > + mutex_lock(&vint_desc->event_mutex); > + inta->sci->ops.rm_irq_ops.free_event_map(inta->sci, > + HWIRQ_TO_DEVID(hwirq), > + HWIRQ_TO_IRQID(hwirq), > + inta->pdev->id, > + vint_desc->vint_id, > + event_desc->global_event, > + event_desc->vint_bit); > + > + clear_bit(event_desc->vint_bit, vint_desc->event_map); > + ti_sci_release_resource(inta->global_event, event_desc->global_event); > + event_desc->global_event = TI_SCI_RESOURCE_NULL; > + event_desc->hwirq = 0; > + mutex_unlock(&vint_desc->event_mutex); > + > + ti_sci_inta_free_parent_irq(inta, vint_desc); > +} > + > +/** > + * ti_sci_inta_request_resources() - Allocate resources for input irq > + * @data: Pointer to corresponding irq_data > + * > + * Note: This is the core api where the actual allocation happens for input > + * hwirq. This allocation involves creating a parent irq for vint. > + * If this is done in irq_domain_ops.alloc() then a deadlock is reached > + * for allocation. So this allocation is being done in request_resources() > + * > + * Return: 0 if all went well else corresponding error. > + */ > +static int ti_sci_inta_request_resources(struct irq_data *data) > +{ > + struct ti_sci_inta_event_desc *event_desc; > + > + event_desc = ti_sci_inta_alloc_irq(data->domain, data->hwirq); > + if (IS_ERR(event_desc)) > + return PTR_ERR(event_desc); > + > + data->chip_data = event_desc; > + > + return 0; > +} > + > +/** > + * ti_sci_inta_release_resources - Release resources for input irq > + * @data: Pointer to corresponding irq_data > + * > + * Note: Corresponding to request_resources(), all the unmapping and deletion > + * of parent vint irqs happens in this api. > + */ > +static void ti_sci_inta_release_resources(struct irq_data *data) > +{ > + struct ti_sci_inta_event_desc *event_desc; > + > + event_desc = irq_data_get_irq_chip_data(data); > + ti_sci_inta_free_irq(event_desc, data->hwirq); > +} > + > +/** > + * ti_sci_inta_manage_event() - Control the event based on the offset > + * @data: Pointer to corresponding irq_data > + * @offset: register offset using which event is controlled. > + */ > +static void ti_sci_inta_manage_event(struct irq_data *data, u32 offset) > +{ > + struct ti_sci_inta_event_desc *event_desc; > + struct ti_sci_inta_vint_desc *vint_desc; > + struct ti_sci_inta_irq_domain *inta; > + > + event_desc = irq_data_get_irq_chip_data(data); > + vint_desc = to_vint_desc(event_desc, event_desc->vint_bit); > + inta = data->domain->host_data; > + > + writeq_relaxed(BIT(event_desc->vint_bit), > + inta->base + vint_desc->vint_id * 0x1000 + offset); > +} > + > +/** > + * ti_sci_inta_mask_irq() - Mask an event > + * @data: Pointer to corresponding irq_data > + */ > +static void ti_sci_inta_mask_irq(struct irq_data *data) > +{ > + ti_sci_inta_manage_event(data, VINT_ENABLE_CLR_OFFSET); > +} > + > +/** > + * ti_sci_inta_unmask_irq() - Unmask an event > + * @data: Pointer to corresponding irq_data > + */ > +static void ti_sci_inta_unmask_irq(struct irq_data *data) > +{ > + ti_sci_inta_manage_event(data, VINT_ENABLE_SET_OFFSET); > +} > + > +/** > + * ti_sci_inta_ack_irq() - Ack an event > + * @data: Pointer to corresponding irq_data > + */ > +static void ti_sci_inta_ack_irq(struct irq_data *data) > +{ > + ti_sci_inta_manage_event(data, VINT_STATUS_OFFSET); > +} > + > +static int ti_sci_inta_set_affinity(struct irq_data *d, > + const struct cpumask *mask_val, bool force) > +{ > + return -EINVAL; > +} > + > +/** > + * ti_sci_inta_set_type() - Update the trigger type of the irq. > + * @data: Pointer to corresponding irq_data > + * @type: Trigger type as specified by user > + * > + * Note: This updates the handle_irq callback for level msi. > + * > + * Return 0 if all went well else appropriate error. > + */ > +static int ti_sci_inta_set_type(struct irq_data *data, unsigned int type) > +{ > + struct irq_desc *desc = irq_to_desc(data->irq); > + > + /* > + * .alloc default sets handle_edge_irq. But if the user specifies > + * that IRQ is level MSI, then update the handle to handle_level_irq > + */ > + if (type & IRQF_TRIGGER_HIGH) > + desc->handle_irq = handle_level_irq; > + > + return 0; Returning error value is causing request_irq to fail, so still returning 0. Do you suggest any other method to handle this? Thanks and regards, Lokesh
On Sat, Apr 20, 2019 at 12:11 PM Lokesh Vutla <lokeshvutla@ti.com> wrote: > thunderx_gpio_irq_{request,release}_resources apis are trying to > {request,release} resources on parent interrupt. There are default > apis doing the same. Use the default parent apis instead of writing > the same code snippet. > > Cc: linux-gpio@vger.kernel.org > Cc: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> Acked-by: Linus Walleij <linus.walleij@linaro.org> It's fine to apply this to the irqchip tree once the irq maintainers are happy. Yours, Linus Walleij
On 23/04/2019 11:00, Lokesh Vutla wrote: > Hi Marc, [...] >> +/** >> + * ti_sci_inta_set_type() - Update the trigger type of the irq. >> + * @data: Pointer to corresponding irq_data >> + * @type: Trigger type as specified by user >> + * >> + * Note: This updates the handle_irq callback for level msi. >> + * >> + * Return 0 if all went well else appropriate error. >> + */ >> +static int ti_sci_inta_set_type(struct irq_data *data, unsigned int type) >> +{ >> + struct irq_desc *desc = irq_to_desc(data->irq); >> + >> + /* >> + * .alloc default sets handle_edge_irq. But if the user specifies >> + * that IRQ is level MSI, then update the handle to handle_level_irq >> + */ >> + if (type & IRQF_TRIGGER_HIGH) >> + desc->handle_irq = handle_level_irq; >> + >> + return 0; > > > Returning error value is causing request_irq to fail, so still returning 0. Do > you suggest any other method to handle this? But that is the very point, isn't it? If you pass the wrong triggering type to request_irq, it *must* fail. What you should have is something like: switch (type & IRQ_TYPE_SENSE_MASK) { case IRQF_TRIGGER_HIGH: desc->handle_irq = handle_level_irq; return 0; case IRQ_TYPE_EDGE_RISING: return 0; default: return -EINVAL; } (adjust as necessary). What's wrong with this? Thanks, M.
On 29/04/19 2:17 PM, Marc Zyngier wrote: > On 23/04/2019 11:00, Lokesh Vutla wrote: >> Hi Marc, > > [...] > >>> +/** >>> + * ti_sci_inta_set_type() - Update the trigger type of the irq. >>> + * @data: Pointer to corresponding irq_data >>> + * @type: Trigger type as specified by user >>> + * >>> + * Note: This updates the handle_irq callback for level msi. >>> + * >>> + * Return 0 if all went well else appropriate error. >>> + */ >>> +static int ti_sci_inta_set_type(struct irq_data *data, unsigned int type) >>> +{ >>> + struct irq_desc *desc = irq_to_desc(data->irq); >>> + >>> + /* >>> + * .alloc default sets handle_edge_irq. But if the user specifies >>> + * that IRQ is level MSI, then update the handle to handle_level_irq >>> + */ >>> + if (type & IRQF_TRIGGER_HIGH) >>> + desc->handle_irq = handle_level_irq; >>> + >>> + return 0; >> >> >> Returning error value is causing request_irq to fail, so still returning 0. Do >> you suggest any other method to handle this? > > But that is the very point, isn't it? If you pass the wrong triggering > type to request_irq, it *must* fail. What you should have is something like: > > switch (type & IRQ_TYPE_SENSE_MASK) { > case IRQF_TRIGGER_HIGH: > desc->handle_irq = handle_level_irq; > return 0; > case IRQ_TYPE_EDGE_RISING: > return 0; > default: > return -EINVAL; > } > > (adjust as necessary). > > What's wrong with this? I get it. Will fix it in next version. I also got the firmware update as well. If you are okay with rest of the series, I want to post the next version with the firmware update. Thanks and regards, Lokesh
On 29/04/2019 09:59, Lokesh Vutla wrote: > > > On 29/04/19 2:17 PM, Marc Zyngier wrote: >> On 23/04/2019 11:00, Lokesh Vutla wrote: >>> Hi Marc, >> >> [...] >> >>>> +/** >>>> + * ti_sci_inta_set_type() - Update the trigger type of the irq. >>>> + * @data: Pointer to corresponding irq_data >>>> + * @type: Trigger type as specified by user >>>> + * >>>> + * Note: This updates the handle_irq callback for level msi. >>>> + * >>>> + * Return 0 if all went well else appropriate error. >>>> + */ >>>> +static int ti_sci_inta_set_type(struct irq_data *data, unsigned int type) >>>> +{ >>>> + struct irq_desc *desc = irq_to_desc(data->irq); >>>> + >>>> + /* >>>> + * .alloc default sets handle_edge_irq. But if the user specifies >>>> + * that IRQ is level MSI, then update the handle to handle_level_irq >>>> + */ >>>> + if (type & IRQF_TRIGGER_HIGH) >>>> + desc->handle_irq = handle_level_irq; >>>> + >>>> + return 0; >>> >>> >>> Returning error value is causing request_irq to fail, so still returning 0. Do >>> you suggest any other method to handle this? >> >> But that is the very point, isn't it? If you pass the wrong triggering >> type to request_irq, it *must* fail. What you should have is something like: >> >> switch (type & IRQ_TYPE_SENSE_MASK) { >> case IRQF_TRIGGER_HIGH: >> desc->handle_irq = handle_level_irq; >> return 0; >> case IRQ_TYPE_EDGE_RISING: >> return 0; >> default: >> return -EINVAL; >> } >> >> (adjust as necessary). >> >> What's wrong with this? > > I get it. Will fix it in next version. I also got the firmware update as well. > If you are okay with rest of the series, I want to post the next version with > the firmware update. Then post it now, and I'll review that. I'd rather look at the latest than providing feedback on something that has already changed. Thanks, M.
On 20/04/2019 11:09, Lokesh Vutla wrote: > Texas Instruments' K3 generation SoCs has an IP Interrupt Aggregator > which is an interrupt controller that does the following: > - Converts events to interrupts that can be understood by > an interrupt router. > - Allows for multiplexing of events to interrupts. > > Configuration of the interrupt aggregator 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. This patch adds support for Interrupt > Aggregator irqdomain. > > Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> > --- > Changes since v6: > - Updated commit message. > - Arranged header files in alphabetical order > - Included vint_bit in struct ti_sci_inta_event_desc > - With the above change now the chip_data is event_desc instead of vint_desc > - No loops are used in atomic contexts. > - Fixed locking issue while freeing parent virq > - Fixed few other cosmetic changes. > > MAINTAINERS | 1 + > drivers/irqchip/Kconfig | 11 + > drivers/irqchip/Makefile | 1 + > drivers/irqchip/irq-ti-sci-inta.c | 589 ++++++++++++++++++++++++++++++ > 4 files changed, 602 insertions(+) > create mode 100644 drivers/irqchip/irq-ti-sci-inta.c > [...] > +/** > + * ti_sci_inta_alloc_irq() - Allocate an irq within INTA domain > + * @domain: irq_domain pointer corresponding to INTA > + * @hwirq: hwirq of the input event > + * > + * Note: Allocation happens in the following manner: > + * - Find a free bit available in any of the vints available in the list. > + * - If not found, allocate a vint from the vint pool > + * - Attach the free bit to input hwirq. > + * Return event_desc if all went ok else appropriate error value. > + */ > +static struct ti_sci_inta_event_desc *ti_sci_inta_alloc_irq(struct irq_domain *domain, > + u32 hwirq) > +{ > + struct ti_sci_inta_irq_domain *inta = domain->host_data; > + struct ti_sci_inta_vint_desc *vint_desc = NULL; > + u16 free_bit; > + > + mutex_lock(&inta->vint_mutex); > + list_for_each_entry(vint_desc, &inta->vint_list, list) { > + mutex_lock(&vint_desc->event_mutex); > + free_bit = find_first_zero_bit(vint_desc->event_map, > + MAX_EVENTS_PER_VINT); > + if (free_bit != MAX_EVENTS_PER_VINT) { > + set_bit(free_bit, vint_desc->event_map); > + mutex_unlock(&vint_desc->event_mutex); > + mutex_unlock(&inta->vint_mutex); > + goto alloc_event; > + } > + mutex_unlock(&vint_desc->event_mutex); > + } > + mutex_unlock(&inta->vint_mutex); > + > + /* No free bits available. Allocate a new vint */ > + vint_desc = ti_sci_inta_alloc_parent_irq(domain); > + if (IS_ERR(vint_desc)) > + return ERR_PTR(PTR_ERR(vint_desc)); > + > + mutex_lock(&vint_desc->event_mutex); > + free_bit = find_first_zero_bit(vint_desc->event_map, > + MAX_EVENTS_PER_VINT); > + set_bit(free_bit, vint_desc->event_map); > + mutex_unlock(&vint_desc->event_mutex); This code is still quite racy: you can have two parallel allocations failing to get a free bit in any of the already allocated vint_desc, and then both allocating a new vint_desc. If there was only one left, one of the allocation will fail despite having at least 63 free interrupts. M.
On 29/04/19 6:41 PM, Marc Zyngier wrote: > On 20/04/2019 11:09, Lokesh Vutla wrote: >> Texas Instruments' K3 generation SoCs has an IP Interrupt Aggregator >> which is an interrupt controller that does the following: >> - Converts events to interrupts that can be understood by >> an interrupt router. >> - Allows for multiplexing of events to interrupts. >> >> Configuration of the interrupt aggregator 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. This patch adds support for Interrupt >> Aggregator irqdomain. >> >> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> >> --- >> Changes since v6: >> - Updated commit message. >> - Arranged header files in alphabetical order >> - Included vint_bit in struct ti_sci_inta_event_desc >> - With the above change now the chip_data is event_desc instead of vint_desc >> - No loops are used in atomic contexts. >> - Fixed locking issue while freeing parent virq >> - Fixed few other cosmetic changes. >> >> MAINTAINERS | 1 + >> drivers/irqchip/Kconfig | 11 + >> drivers/irqchip/Makefile | 1 + >> drivers/irqchip/irq-ti-sci-inta.c | 589 ++++++++++++++++++++++++++++++ >> 4 files changed, 602 insertions(+) >> create mode 100644 drivers/irqchip/irq-ti-sci-inta.c >> > > [...] > >> +/** >> + * ti_sci_inta_alloc_irq() - Allocate an irq within INTA domain >> + * @domain: irq_domain pointer corresponding to INTA >> + * @hwirq: hwirq of the input event >> + * >> + * Note: Allocation happens in the following manner: >> + * - Find a free bit available in any of the vints available in the list. >> + * - If not found, allocate a vint from the vint pool >> + * - Attach the free bit to input hwirq. >> + * Return event_desc if all went ok else appropriate error value. >> + */ >> +static struct ti_sci_inta_event_desc *ti_sci_inta_alloc_irq(struct irq_domain *domain, >> + u32 hwirq) >> +{ >> + struct ti_sci_inta_irq_domain *inta = domain->host_data; >> + struct ti_sci_inta_vint_desc *vint_desc = NULL; >> + u16 free_bit; >> + >> + mutex_lock(&inta->vint_mutex); >> + list_for_each_entry(vint_desc, &inta->vint_list, list) { >> + mutex_lock(&vint_desc->event_mutex); >> + free_bit = find_first_zero_bit(vint_desc->event_map, >> + MAX_EVENTS_PER_VINT); >> + if (free_bit != MAX_EVENTS_PER_VINT) { >> + set_bit(free_bit, vint_desc->event_map); >> + mutex_unlock(&vint_desc->event_mutex); >> + mutex_unlock(&inta->vint_mutex); >> + goto alloc_event; >> + } >> + mutex_unlock(&vint_desc->event_mutex); >> + } >> + mutex_unlock(&inta->vint_mutex); >> + >> + /* No free bits available. Allocate a new vint */ >> + vint_desc = ti_sci_inta_alloc_parent_irq(domain); >> + if (IS_ERR(vint_desc)) >> + return ERR_PTR(PTR_ERR(vint_desc)); >> + >> + mutex_lock(&vint_desc->event_mutex); >> + free_bit = find_first_zero_bit(vint_desc->event_map, >> + MAX_EVENTS_PER_VINT); >> + set_bit(free_bit, vint_desc->event_map); >> + mutex_unlock(&vint_desc->event_mutex); > > This code is still quite racy: you can have two parallel allocations > failing to get a free bit in any of the already allocated vint_desc, and > then both allocating a new vint_desc. If there was only one left, one of > the allocation will fail despite having at least 63 free interrupts. Good point. After thinking a bit more, I saw similar issue when two parallel frees happens on a vint with only 2 bits allocated. First free when freeing parent_irq might see all the bits cleared and does kfree(vint). Then second free will crash when freeing parent irq. Ill guard the entire allocation and freeing with vint_mutex and drop the event_mutex altogether. Thanks and regards, Lokesh > > M. >
TI AM65x SoC based on K3 architecture introduced support for Events which are message based interrupts with minimal latency. These events are not compatible with regular interrupts and are valid only through an event transport lane. An Interrupt Aggregator(INTA) is introduced to convert these events to interrupts. INTA can also group 64 events into a single interrupt. Now the SoC has many peripherals and a large number of event sources (time sync or DMA), the use of events is completely dependent on a user's specific application, which drives a need for maximum flexibility in which event sources are used in the system. It is also completely up to software control as to how the events are serviced. Because of the huge flexibility there are certain standard peripherals (like GPIO etc)where all interrupts cannot be directly corrected to host interrupt controller. For this purpose, Interrupt Router(INTR) is introduced in the SoC. INTR just does a classic interrupt redirection. So the SoC has 3 types of interrupt controllers: - GIC500 - Interrupt Router - Interrupt Aggregator Below is a diagrammatic view of how SoC integration of these interrupt controllers:(https://pastebin.ubuntu.com/p/9ngV3jdGj2/) Device Index-x Device Index-y | | | | .... \ / \ / \ (global events) / +---------------------------+ +---------+ | | | | | INTA | | GPIO | | | | | +---------------------------+ +---------+ | (vint) | | | \|/ | +---------------------------+ | | |<-------+ | INTR | | | +---------------------------+ | | \|/ (gic irq) +---------------------------+ | | | GIC | | | +---------------------------+ While at it, TISCI abstracts the handling of all above IRQ routes where interrupt sources are not directly connected to host interrupt controller. That would be configuration of Interrupt Aggregator and Interrupt Router. This series adds support for: - TISCI commands needed for IRQ configuration - Interrupt Router(INTR) driver. - Interrupt Aggregator(INTA) driver. - Interrupt Aggregator MSI bus layer. Changes since v6: - Each patch has respective changes mentioned. Grygorii Strashko (1): firmware: ti_sci: Add support to get TISCI handle using of_phandle Lokesh Vutla (12): firmware: ti_sci: Add support for RM core ops firmware: ti_sci: Add support for IRQ management firmware: ti_sci: Add helper apis to manage resources genirq: Introduce irq_chip_{request,release}_resource_parent() apis gpio: thunderx: Use the default parent apis for {request,release}_resources dt-bindings: irqchip: Introduce TISCI Interrupt router bindings irqchip: ti-sci-intr: Add support for Interrupt Router driver dt-bindings: irqchip: Introduce TISCI Interrupt Aggregator bindings irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver soc: ti: Add MSI domain bus support for Interrupt Aggregator irqchip: ti-sci-inta: Add msi domain support arm64: arch_k3: Enable interrupt controller drivers Peter Ujfalusi (1): firmware: ti_sci: Add RM mapping table for am654 .../bindings/arm/keystone/ti,sci.txt | 3 +- .../interrupt-controller/ti,sci-inta.txt | 66 ++ .../interrupt-controller/ti,sci-intr.txt | 84 +++ MAINTAINERS | 6 + arch/arm64/Kconfig.platforms | 5 + drivers/firmware/ti_sci.c | 666 ++++++++++++++++++ drivers/firmware/ti_sci.h | 102 +++ drivers/gpio/gpio-thunderx.c | 16 +- drivers/irqchip/Kconfig | 23 + drivers/irqchip/Makefile | 2 + drivers/irqchip/irq-ti-sci-inta.c | 627 +++++++++++++++++ drivers/irqchip/irq-ti-sci-intr.c | 285 ++++++++ drivers/soc/ti/Kconfig | 6 + drivers/soc/ti/Makefile | 1 + drivers/soc/ti/ti_sci_inta_msi.c | 146 ++++ include/linux/irq.h | 2 + include/linux/irqdomain.h | 1 + include/linux/msi.h | 10 + include/linux/soc/ti/ti_sci_inta_msi.h | 23 + include/linux/soc/ti/ti_sci_protocol.h | 126 ++++ kernel/irq/chip.c | 27 + 21 files changed, 2214 insertions(+), 13 deletions(-) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ti,sci-inta.txt create mode 100644 Documentation/devicetree/bindings/interrupt-controller/ti,sci-intr.txt create mode 100644 drivers/irqchip/irq-ti-sci-inta.c create mode 100644 drivers/irqchip/irq-ti-sci-intr.c create mode 100644 drivers/soc/ti/ti_sci_inta_msi.c create mode 100644 include/linux/soc/ti/ti_sci_inta_msi.h