Message-ID: <1c2f2610-2be4-c145-4786-eabf3a07bc47@ti.com>
Date:   Wed, 20 Feb 2019 18:47:16 +0530
From:   Lokesh Vutla <lokeshvutla@...com>
To:     Marc Zyngier <marc.zyngier@....com>
CC:     Tony Lindgren <tony@...mide.com>, Nishanth Menon <nm@...com>,
        Santosh Shilimkar <ssantosh@...nel.org>,
        Rob Herring <robh+dt@...nel.org>, <tglx@...utronix.de>,
        <jason@...edaemon.net>,
        Linux ARM Mailing List <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>,
        Device Tree Mailing List <devicetree@...r.kernel.org>,
        Sekhar Nori <nsekhar@...com>, Tero Kristo <t-kristo@...com>,
        Peter Ujfalusi <peter.ujfalusi@...com>
Subject: Re: [PATCH v5 06/10] irqchip: ti-sci-intr: Add support for Interrupt
 Router driver

Hi Marc,

On 18/02/19 9:22 PM, Marc Zyngier wrote:
> On Tue, 12 Feb 2019 13:12:33 +0530
> Lokesh Vutla <lokeshvutla@...com> wrote:
> 
>> Texas Instruments' K3 generation SoCs has an IP Interrupt Router
>> that does allows for redirection 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.
>>
>> Add support for Interrupt Router driver over TISCI protocol.
>>
>> Signed-off-by: Lokesh Vutla <lokeshvutla@...com>
>> ---
>> Changes since v4:
>> - Moved the ti_sci irq resource handling to ti-sci-common.c from ti_sci.c.
>>   Firmware maintainer rejected the idea of having this in firmware
>> driver as the resource handling is specific to the client.
>> - Obtain the number of peripheral interrupts attached to INTR by
>> parsing DT. Using this information store the max irqs that can be
>> allocated to INTA. This is done for pre allocating the INTA
>> irqs(vints) during inta driver probe.
>>   This will not work for cases where there are more that 1 INTAs
>> attached to INTR, but wanted to show this approach as suggested by
>> Marc.
> 
> Or not.
> 
>>
>>  MAINTAINERS                         |   3 +
>>  drivers/irqchip/Kconfig             |  11 +
>>  drivers/irqchip/Makefile            |   1 +
>>  drivers/irqchip/irq-ti-sci-common.c | 131 ++++++++++++
>>  drivers/irqchip/irq-ti-sci-common.h |  59 ++++++
>>  drivers/irqchip/irq-ti-sci-intr.c   | 315
>> ++++++++++++++++++++++++++++ 6 files changed, 520 insertions(+)
>>  create mode 100644 drivers/irqchip/irq-ti-sci-common.c
>>  create mode 100644 drivers/irqchip/irq-ti-sci-common.h
>>  create mode 100644 drivers/irqchip/irq-ti-sci-intr.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index c918d9b2ee18..823eeb672cf0 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -15065,6 +15065,9 @@ 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 +F:
>> drivers/irqchip/irq-ti-sci-common.c +F:
>> drivers/irqchip/irq-ti-sci-common.h 
>>  Texas Instruments ASoC drivers
>>  M:	Peter Ujfalusi <peter.ujfalusi@...com>
>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>> index 3d1e60779078..a8d9bed0254b 100644
>> --- a/drivers/irqchip/Kconfig
>> +++ b/drivers/irqchip/Kconfig
>> @@ -406,6 +406,17 @@ config IMX_IRQSTEER
>>  	help
>>  	  Support for the i.MX IRQSTEER interrupt
>> multiplexer/remapper. 
>> +config TI_SCI_INTR_IRQCHIP
>> +	bool
>> +	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.
>> +
>>  endmenu
>>  
>>  config SIFIVE_PLIC
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index c93713d24b86..0499fae148a9 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -94,3 +94,4 @@ obj-$(CONFIG_CSKY_APB_INTC)		+=
>> irq-csky-apb-intc.o obj-$(CONFIG_SIFIVE_PLIC)		+=
>> irq-sifive-plic.o obj-$(CONFIG_IMX_IRQSTEER)		+=
>> irq-imx-irqsteer.o obj-$(CONFIG_MADERA_IRQ)		+=
>> irq-madera.o +obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+=
>> irq-ti-sci-intr.o irq-ti-sci-common.o diff --git
>> a/drivers/irqchip/irq-ti-sci-common.c
>> b/drivers/irqchip/irq-ti-sci-common.c new file mode 100644 index
>> 000000000000..79d9c4e8ea14 --- /dev/null
>> +++ b/drivers/irqchip/irq-ti-sci-common.c
>> @@ -0,0 +1,131 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Common Code for TISCI IRQCHIP drivers
>> + *
>> + * Copyright (C) 2019 Texas Instruments Incorporated -
>> http://www.ti.com/
>> + *	Lokesh Vutla <lokeshvutla@...com>
>> + */
>> +
>> +#include <linux/of.h>
>> +#include <linux/bitmap.h>
>> +#include <linux/device.h>
>> +#include <linux/soc/ti/ti_sci_protocol.h>
>> +#include "irq-ti-sci-common.h"
>> +
>> +/**
>> + * ti_sci_get_free_resource() - Get a free resource from TISCI
>> resource.
>> + * @res:	Pointer to the TISCI resource
>> + *
>> + * Return: resource num if all went ok else TI_SCI_RESOURCE_NULL.
>> + */
>> +u16 ti_sci_get_free_resource(struct ti_sci_resource *res)
>> +{
>> +	u16 set, free_bit;
>> +
>> +	mutex_lock(&res->request_mutex);
>> +	for (set = 0; set < res->sets; set++) {
>> +		free_bit =
>> find_first_zero_bit(res->desc[set].res_map,
>> +					       res->desc[set].num);
>> +		if (free_bit != res->desc[set].num) {
>> +			set_bit(free_bit, res->desc[set].res_map);
>> +			mutex_unlock(&res->request_mutex);
>> +			return res->desc[set].start + free_bit;
>> +		}
>> +	}
>> +	mutex_unlock(&res->request_mutex);
>> +
>> +	return TI_SCI_RESOURCE_NULL;
>> +}
>> +
>> +/**
>> + * ti_sci_release_resource() - Release a resource from TISCI
>> resource.
>> + * @res:	Pointer to the TISCI resource
>> + * @id:		Resource id to be released.
>> + */
>> +void ti_sci_release_resource(struct ti_sci_resource *res, u16 id)
>> +{
>> +	u16 set;
>> +
>> +	mutex_lock(&res->request_mutex);
>> +	for (set = 0; set < res->sets; set++) {
>> +		if (res->desc[set].start <= id &&
>> +		    (res->desc[set].num + res->desc[set].start) > id)
>> +			clear_bit(id - res->desc[set].start,
>> +				  res->desc[set].res_map);
>> +	}
>> +	mutex_unlock(&res->request_mutex);
>> +}
>> +
>> +u32 ti_sci_get_num_resources(struct ti_sci_resource *res)
>> +{
>> +	u32 count = 0, set;
>> +
>> +	for (set = 0; set < res->sets; set++)
>> +		count += res->desc[set].num;
>> +
>> +	return count;
>> +}
>> +
>> +/**
>> + * devm_ti_sci_get_of_resource() - Get a TISCI resource assigned to
>> a device
>> + * @handle:	TISCI handle
>> + * @dev:	Device pointer to which the resource is assigned
>> + * @of_prop:	property name by which the resource are
>> represented
>> + *
>> + * Return: Pointer to ti_sci_resource if all went well else
>> appropriate
>> + *	   error pointer.
>> + */
>> +struct ti_sci_resource *
>> +devm_ti_sci_get_of_resource(const struct ti_sci_handle *handle,
>> +			    struct device *dev, u32 dev_id, char
>> *of_prop) +{
>> +	struct ti_sci_resource *res;
>> +	u32 resource_subtype;
>> +	int i, ret;
>> +
>> +	res = devm_kzalloc(dev, sizeof(*res), GFP_KERNEL);
>> +	if (!res)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	res->sets =
>> of_property_count_elems_of_size(dev_of_node(dev), of_prop,
>> +						    sizeof(u32));
>> +	if (res->sets < 0) {
>> +		dev_err(dev, "%s resource type ids not available\n",
>> of_prop);
>> +		return ERR_PTR(res->sets);
>> +	}
>> +
>> +	res->desc = devm_kcalloc(dev, res->sets, sizeof(*res->desc),
>> +				 GFP_KERNEL);
>> +	if (!res->desc)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	for (i = 0; i < res->sets; i++) {
>> +		ret = of_property_read_u32_index(dev_of_node(dev),
>> of_prop, i,
>> +						 &resource_subtype);
>> +		if (ret)
>> +			return ERR_PTR(-EINVAL);
>> +
>> +		ret = handle->ops.rm_core_ops.get_range(handle,
>> dev_id,
>> +
>> resource_subtype,
>> +
>> &res->desc[i].start,
>> +
>> &res->desc[i].num);
>> +		if (ret) {
>> +			dev_err(dev, "dev = %d subtype %d not
>> allocated for this host\n",
>> +				dev_id, resource_subtype);
>> +			return ERR_PTR(ret);
>> +		}
>> +
>> +		dev_dbg(dev, "dev = %d, subtype = %d, start = %d,
>> num = %d\n",
>> +			dev_id, resource_subtype, res->desc[i].start,
>> +			res->desc[i].num);
>> +
>> +		res->desc[i].res_map =
>> +			devm_kzalloc(dev,
>> BITS_TO_LONGS(res->desc[i].num) *
>> +				     sizeof(*res->desc[i].res_map),
>> GFP_KERNEL);
>> +		if (!res->desc[i].res_map)
>> +			return ERR_PTR(-ENOMEM);
>> +	}
>> +	mutex_init(&res->request_mutex);
>> +
>> +	return res;
>> +}
>> diff --git a/drivers/irqchip/irq-ti-sci-common.h
>> b/drivers/irqchip/irq-ti-sci-common.h new file mode 100644
>> index 000000000000..60c4b28bebab
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-ti-sci-common.h
>> @@ -0,0 +1,59 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Common header file for TISCI IRQCHIP drivers
>> + *
>> + * Copyright (C) 2019 Texas Instruments Incorporated -
>> http://www.ti.com/
>> + *	Lokesh Vutla <lokeshvutla@...com>
>> + */
>> +
>> +#ifndef __TI_SCI_COMMON_IRQCHIP_H
>> +#define __TI_SCI_COMMON_IRQCHIP_H
>> +
>> +#include <linux/mutex.h>
>> +
>> +#define TI_SCI_RESOURCE_NULL	0xffff
>> +#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_VINT_IRQ		BIT(0)
>> +#define HWIRQ_TO_DEVID(hwirq)	(((hwirq) >>
>> (TI_SCI_DEV_ID_SHIFT)) & \
>> +				 (TI_SCI_DEV_ID_MASK))
>> +#define HWIRQ_TO_IRQID(hwirq)	((hwirq) & (TI_SCI_IRQ_ID_MASK))
>> +#define TO_HWIRQ(dev, index)	((((dev) & TI_SCI_DEV_ID_MASK)
>> << \
>> +				 TI_SCI_DEV_ID_SHIFT) | \
>> +				((index) & TI_SCI_IRQ_ID_MASK))
>> +
>> +/**
>> + * struct ti_sci_resource_desc - Description of TI SCI resource
>> instance range.
>> + * @start:	Start index of the resource.
>> + * @num:	Number of resources.
>> + * @res_map:	Bitmap to manage the allocation of these
>> resources.
>> + */
>> +struct ti_sci_resource_desc {
>> +	u16 start;
>> +	u16 num;
>> +	unsigned long *res_map;
>> +};
>> +
>> +/**
>> + * struct ti_sci_resource - Structure representing a resource
>> assigned
>> + *			    to a device.
>> + * @sets:		Number of sets available from this resource
>> type
>> + * @request_mutex:	mutex to protect request and release of
>> resources.
>> + * @desc:		Array of resource descriptors.
>> + */
>> +struct ti_sci_resource {
>> +	u16 sets;
>> +	/* Mutex to protect request and release of resources */
>> +	struct mutex request_mutex;
>> +	struct ti_sci_resource_desc *desc;
>> +};
>> +
>> +u16 ti_sci_get_free_resource(struct ti_sci_resource *res);
>> +void ti_sci_release_resource(struct ti_sci_resource *res, u16 id);
>> +u32 ti_sci_get_num_resources(struct ti_sci_resource *res);
>> +struct ti_sci_resource *
>> +devm_ti_sci_get_of_resource(const struct ti_sci_handle *handle,
>> +			    struct device *dev, u32 dev_id, char
>> *of_prop); +#endif /*__TI_SCI_COMMON_IRQCHIP_H */
> 
> Most the above "common" should be a separate patch, really. Also, why

ok will move this into a separate patch.

> isn't the whole resource management part of the firmware interface? I
> see that "Firmware maintainer rejected the idea", but I don't really
> buy the rational. I don't see anything irq specific to the whole
> devm_ti_sci_get_of_resource and co, to be honest.

I guess the concern is mostly about the way ids are represented from DT. Using
this every tisci client should mandate to have separate DT property for each of
its resource.

> 
>> diff --git a/drivers/irqchip/irq-ti-sci-intr.c b/drivers/irqchip/irq-ti-sci-intr.c
>> new file mode 100644
>> index 000000000000..7e224552a735
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-ti-sci-intr.c
>> @@ -0,0 +1,315 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Texas Instruments' K3 Interrupt Router irqchip driver
>> + *
>> + * Copyright (C) 2018-2019 Texas Instruments Incorporated - http://www.ti.com/
>> + *	Lokesh Vutla <lokeshvutla@...com>
>> + */

[..snip..]

>> +	err = ti_sci_intr_irq_domain_translate(domain, fwspec, &hwirq, &type);
>> +	if (err)
>> +		return err;
>> +
>> +	vint_irq = fwspec->param[3] & TI_SCI_VINT_IRQ;
>> +
>> +	err = ti_sci_intr_alloc_gic_irq(domain, virq, hwirq, type, vint_irq);
>> +	if (err)
>> +		return err;
>> +
>> +	err = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
>> +					    &ti_sci_intr_irq_chip,
>> +					    (void *)vint_irq);
> 
> I think I see what you're trying to achieve. I suggest you look a
> uintptr_t instead of playing with unrelated data types whose semantic
> really doesn't apply here.

As you commented on the dt-bindings patch to differentiate vints using the INTA
ids, this will not be needed anymore. Will completely drop this.

> 
>> +	if (err) {
>> +		ti_sci_intr_irq_domain_free(domain, virq, 1);
>> +		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 device_node *parent_node, *dn;
>> +	struct ti_sci_intr_irq_domain *intr;
>> +	struct device *dev = &pdev->dev;
>> +	struct of_phandle_iterator it;
>> +	int ret, count, intsize;
>> +
>> +	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;
>> +	}
>> +
>> +	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;
>> +	}
>> +
>> +	intr->dst_irq = devm_ti_sci_get_of_resource(intr->sci, dev,
>> +						    intr->dst_id,
>> +						    "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);
>> +	}
>> +
>> +	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;
>> +	}
>> +
>> +	if (of_property_read_u32(dev_of_node(dev), "#interrupt-cells",
>> +				 &intsize))
>> +		return -EINVAL;
>> +
>> +	count = 0;
>> +	for_each_node_with_property(dn, "interrupts") {
>> +		if (of_irq_find_parent(dn) != dev_of_node(dev))
>> +			continue;
>> +		count += of_property_count_elems_of_size(dn, "interrupts",
>> +							 sizeof(u32) * intsize);
>> +	}
>> +
>> +	for_each_node_with_property(dn, "interrupts-extended") {
>> +		of_for_each_phandle(&it, ret, dn, "interrupts-extended",
>> +				    "#interrupt-cells", 0) {
>> +			if (it.node == dev_of_node(dev))
>> +				count++;
>> +		}
>> +	}
> 
> What is this trying to do? Counting the number of interrupt descriptors
> in the whole system? What does this even mean?

I am just trying for max utilization of GIC interrupts available for this. Since
inta vints are not represented in DT, total gic available - whatever is in DT
gives the max interrupts that can be used for the child INTA.

> 
> What I suggested was to allocate the required number of interrupts
> for a given device, based on the requirements of that device. I also

okay, then Ill get the number of interrupts needed for INTA from DT.

> suggested to investigate the x86 two phase allocation mechanism for the
> INTA driver.

Sorry, Ill try to explore this path. Any pointers to the doc/code will be really
helpful :)

Thanks and regards,
Lokesh

> 
> I never suggested that you'd parse the DT to guess how many interrupts
> you'd need to allocate...
> 
> 	M.
> 
>> +
>> +	intr->vint_irqs = ti_sci_get_num_resources(intr->dst_irq) - count;
>> +
>> +	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@...om>");
>> +MODULE_DESCRIPTION("K3 Interrupt Router driver over TI SCI protocol");
>> +MODULE_LICENSE("GPL v2");
> 
> 
> 

Powered by blists - more mailing lists