LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Lokesh Vutla <lokeshvutla@ti.com>
To: Marc Zyngier <marc.zyngier@arm.com>,
	Tony Lindgren <tony@atomide.com>, Nishanth Menon <nm@ti.com>,
	Santosh Shilimkar <ssantosh@kernel.org>,
	Rob Herring <robh+dt@kernel.org>, <jason@lakedaemon.net>
Cc: Linux ARM Mailing List <linux-arm-kernel@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>,
	Device Tree Mailing List <devicetree@vger.kernel.org>,
	Sekhar Nori <nsekhar@ti.com>, Tero Kristo <t-kristo@ti.com>,
	Peter Ujfalusi <peter.ujfalusi@ti.com>,
	Grygorii Strashko <grygorii.strashko@ti.com>
Subject: Re: [PATCH v6 10/12] soc: ti: Add MSI domain bus support for Interrupt Aggregator
Date: Wed, 17 Apr 2019 22:29:06 +0530
Message-ID: <a660c601-33f2-243d-7d83-7622e2c0f1b9@ti.com> (raw)
In-Reply-To: <8691f4f1-741c-c15d-62d6-3c2f7157cb51@arm.com>



On 17/04/19 10:04 PM, Marc Zyngier wrote:
> On 10/04/2019 05:13, Lokesh Vutla wrote:
>> With the system coprocessor managing the range allocation of the
>> inputs to Interrupt Aggregator, it is difficult to represent
>> the device IRQs from DT.
>>
>> The suggestion is to use MSI in such cases where devices wants
>> to allocate and group interrupts dynamically.
>>
>> Create a MSI domain bus layer that allocates and frees MSIs for
>> a device.
>>
>> APIs that are implemented:
>> - ti_sci_inta_msi_create_irq_domain() that creates a MSI domain
>> - ti_sci_inta_msi_domain_alloc_irqs() that creates MSIs for the
>>   specified device and resource.
>> - ti_sci_inta_msi_domain_free_irqs() frees the irqs attached to the device.
>> - ti_sci_inta_msi_get_virq() for getting the virq attached to a specific event.
>>
>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
>> ---
>> Changes since v5:
>> - Updated the input parametes to all apis
>> - Updated the default chip ops.
>> - Prefixed all the apis with ti_sci_inta_
>>
>> Marc,
>> 	Right now ti_sci_resource is being passed for irq allocatons.
>> I couldn't get to use resources attached to platform_device. Because
>> platform_device resources are allocated in of_device_alloc() and number
>> of resources are fixed in it. In order to update the resources, driver
>> has to do a krealloc(pdev->resources) and update the num of resources.
>> Is it allowed to update the pdev->resources during probe time? If yes,
>> Ill be happy to update the patch to use platform_dev resources.
> 
> My suggestion was for you to define your own bus, device type and co
> (much like the fsl-mc stuff), and not reuse platform devices at all.
> 
>>
>>
>>  MAINTAINERS                            |   2 +
>>  drivers/soc/ti/Kconfig                 |   6 +
>>  drivers/soc/ti/Makefile                |   1 +
>>  drivers/soc/ti/ti_sci_inta_msi.c       | 167 +++++++++++++++++++++++++
>>  include/linux/irqdomain.h              |   1 +
>>  include/linux/msi.h                    |   6 +
>>  include/linux/soc/ti/ti_sci_inta_msi.h |  23 ++++
>>  7 files changed, 206 insertions(+)
>>  create mode 100644 drivers/soc/ti/ti_sci_inta_msi.c
>>  create mode 100644 include/linux/soc/ti/ti_sci_inta_msi.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index ba88b3033fe4..dd31d7cb2fc6 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -15353,6 +15353,8 @@ 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
>> +F:	include/linux/soc/ti/ti_sci_inta_msi.h
>> +F:	drivers/soc/ti/ti_sci_inta_msi.c
>>  
>>  Texas Instruments ASoC drivers
>>  M:	Peter Ujfalusi <peter.ujfalusi@ti.com>
>> diff --git a/drivers/soc/ti/Kconfig b/drivers/soc/ti/Kconfig
>> index be4570baad96..82f110fe4953 100644
>> --- a/drivers/soc/ti/Kconfig
>> +++ b/drivers/soc/ti/Kconfig
>> @@ -73,4 +73,10 @@ config TI_SCI_PM_DOMAINS
>>  	  called ti_sci_pm_domains. Note this is needed early in boot before
>>  	  rootfs may be available.
>>  
>> +config TI_SCI_INTA_MSI_DOMAIN
>> +	bool
>> +	select GENERIC_MSI_IRQ_DOMAIN
>> +	help
>> +	  Driver to enable Interrupt Aggregator specific MSI Domain.
>> +
>>  endif # SOC_TI
>> diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile
>> index a22edc0b258a..b3868d392d4f 100644
>> --- a/drivers/soc/ti/Makefile
>> +++ b/drivers/soc/ti/Makefile
>> @@ -8,3 +8,4 @@ obj-$(CONFIG_KEYSTONE_NAVIGATOR_DMA)	+= knav_dma.o
>>  obj-$(CONFIG_AMX3_PM)			+= pm33xx.o
>>  obj-$(CONFIG_WKUP_M3_IPC)		+= wkup_m3_ipc.o
>>  obj-$(CONFIG_TI_SCI_PM_DOMAINS)		+= ti_sci_pm_domains.o
>> +obj-$(CONFIG_TI_SCI_INTA_MSI_DOMAIN)	+= ti_sci_inta_msi.o
>> diff --git a/drivers/soc/ti/ti_sci_inta_msi.c b/drivers/soc/ti/ti_sci_inta_msi.c
>> new file mode 100644
>> index 000000000000..247a5e5f216b
>> --- /dev/null
>> +++ b/drivers/soc/ti/ti_sci_inta_msi.c
>> @@ -0,0 +1,167 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Texas Instruments' K3 Interrupt Aggregator MSI bus
>> + *
>> + * Copyright (C) 2018-2019 Texas Instruments Incorporated - http://www.ti.com/
>> + *	Lokesh Vutla <lokeshvutla@ti.com>
>> + */
>> +
>> +#include <linux/of_device.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/irq.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/msi.h>
> 
> Alphabetical ordering, please.

Sure.

> 
>> +#include <linux/soc/ti/ti_sci_inta_msi.h>
>> +#include <linux/soc/ti/ti_sci_protocol.h>
>> +
>> +static void ti_sci_inta_msi_write_msg(struct irq_data *data,
>> +				      struct msi_msg *msg)
>> +{
>> +	/* Nothing to do */
>> +}
>> +
>> +static void ti_sci_inta_msi_compose_msi_msg(struct irq_data *data,
>> +					    struct msi_msg *msg)
>> +{
>> +	/* Nothing to do */
>> +}
>> +
>> +static int ti_sci_inta_msi_request_resources(struct irq_data *data)
>> +{
>> +	data = data->parent_data;
>> +
>> +	return data->chip->irq_request_resources(data);
>> +}
>> +
>> +static void ti_sci_inta_msi_release_resources(struct irq_data *data)
>> +{
>> +	data = data->parent_data;
>> +	data->chip->irq_release_resources(data);
>> +}
> 
> The two functions above are an implementation of
> irq_chip_{request,release}_resource_parent(). Please make them generic
> functions, use them and fix drivers/gpio/gpio-thunderx.c to use them too.

okay, will create  irq_chip_{request,release}_resource_parent() apis and use them.

> 
>> +
>> +static void ti_sci_inta_msi_update_chip_ops(struct msi_domain_info *info)
>> +{
>> +	struct irq_chip *chip = info->chip;
>> +
>> +	WARN_ON(!chip);
> 
> Just doing that isn't going to help, as you'll crash on the following
> line...

Checkpatch is scribbling about it. Will use BUG_ON() in next version.

> 
>> +	if (!chip->irq_mask)
>> +		chip->irq_mask = irq_chip_mask_parent;
>> +	if (!chip->irq_unmask)
>> +		chip->irq_unmask = irq_chip_unmask_parent;
>> +	if (!chip->irq_ack)
>> +		chip->irq_ack = irq_chip_ack_parent;
>> +	if (!chip->irq_set_type)
>> +		chip->irq_set_type = irq_chip_set_type_parent;
>> +	if (!chip->irq_write_msi_msg)
>> +		chip->irq_write_msi_msg = ti_sci_inta_msi_write_msg;
>> +	if (!chip->irq_compose_msi_msg)
>> +		chip->irq_compose_msi_msg = ti_sci_inta_msi_compose_msi_msg;
>> +	if (!chip->irq_request_resources)
>> +		chip->irq_request_resources = ti_sci_inta_msi_request_resources;
>> +	if (!chip->irq_release_resources)
>> +		chip->irq_release_resources = ti_sci_inta_msi_release_resources;
> 
> Is there any case where a client driver wouldn't use the default all the
> time?

I don't think so.

> 
>> +}
>> +
>> +struct irq_domain
>> +*ti_sci_inta_msi_create_irq_domain(struct fwnode_handle *fwnode,
>> +				   struct msi_domain_info *info,
>> +				   struct irq_domain *parent)
>> +{
>> +	struct irq_domain *domain;
>> +
>> +	if (info->flags & MSI_FLAG_USE_DEF_CHIP_OPS)
>> +		ti_sci_inta_msi_update_chip_ops(info);
> 
> If the answer above is "no", then you can happily ignore this flag and
> always populate the callbacks.

Okay, will ignore the flag and populate apis.

> 
>> +
>> +	domain = msi_create_irq_domain(fwnode, info, parent);
>> +	if (domain)
>> +		irq_domain_update_bus_token(domain, DOMAIN_BUS_TI_SCI_INTA_MSI);
>> +
>> +	return domain;
>> +}
>> +EXPORT_SYMBOL_GPL(ti_sci_inta_msi_create_irq_domain);
>> +
>> +static void ti_sci_inta_msi_free_descs(struct device *dev)
>> +{
>> +	struct msi_desc *desc, *tmp;
>> +
>> +	list_for_each_entry_safe(desc, tmp, dev_to_msi_list(dev), list) {
>> +		list_del(&desc->list);
>> +		free_msi_entry(desc);
>> +	}
>> +}
>> +
>> +static int ti_sci_inta_msi_alloc_descs(struct device *dev, u32 dev_id,
>> +				       struct ti_sci_resource *res)
>> +{
>> +	struct msi_desc *msi_desc;
>> +	int set, i, count = 0;
>> +
>> +	for (set = 0; set < res->sets; set++) {
>> +		for (i = 0; i < res->desc[set].num; i++) {
>> +			msi_desc = alloc_msi_entry(dev, 1, NULL);
>> +			if (!msi_desc) {
>> +				ti_sci_inta_msi_free_descs(dev);
>> +				return -ENOMEM;
>> +			}
>> +
>> +			msi_desc->inta.index = res->desc[set].start + i;
>> +			msi_desc->inta.dev_id = dev_id;
> 
> I'm highly suspiscious of this. See further down.

I need to pass dev_id and dev_index to my irqchip driver so that hwirq gets created.

> 
>> +			INIT_LIST_HEAD(&msi_desc->list);
>> +			list_add_tail(&msi_desc->list, dev_to_msi_list(dev));
>> +			count++;
>> +		}
>> +	}
>> +
>> +	return count;
>> +}
>> +
>> +int ti_sci_inta_msi_domain_alloc_irqs(struct platform_device *pdev,
>> +				      struct ti_sci_resource *res)
>> +{
>> +	struct irq_domain *msi_domain;
>> +	int ret, nvec;
>> +
>> +	msi_domain = dev_get_msi_domain(&pdev->dev);
>> +	if (!msi_domain)
>> +		return -EINVAL;
>> +
>> +	if (pdev->id < 0)
>> +		return -ENODEV;
>> +
>> +	nvec = ti_sci_inta_msi_alloc_descs(&pdev->dev, pdev->id, res);
>> +	if (nvec <= 0)
>> +		return nvec;
>> +
>> +	ret = msi_domain_alloc_irqs(msi_domain, &pdev->dev, nvec);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "Failed to allocate IRQs %d\n", ret);
>> +		goto cleanup;
>> +	}
>> +
>> +	return 0;
>> +
>> +cleanup:
>> +	ti_sci_inta_msi_free_descs(&pdev->dev);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(ti_sci_inta_msi_domain_alloc_irqs);
>> +
>> +void ti_sci_inta_msi_domain_free_irqs(struct device *dev)
>> +{
>> +	msi_domain_free_irqs(dev->msi_domain, dev);
>> +	ti_sci_inta_msi_free_descs(dev);
>> +}
>> +EXPORT_SYMBOL_GPL(ti_sci_inta_msi_domain_free_irqs);
>> +
>> +unsigned int ti_sci_inta_msi_get_virq(struct platform_device *pdev, u32 index)
>> +{
>> +	struct msi_desc *desc;
>> +
>> +	for_each_msi_entry(desc, &pdev->dev)
>> +		if (desc->inta.index == index && desc->inta.dev_id == pdev->id)
> 
> What is this "index"? Why isn't the right entry the index-th element in
> the msi_desc list?  Worse, the dev_id check. The whole point of having a
> per-device MSI list is that it is, well, per device.

Might be wrong choice of word here. As you know, dev_index need not be
contiguous. ti_sci_resource will have the range of dev_index allocated to the
linux host. using this dev_index irqs gets configured. Even the client drivers
only track this dev_index. Isn't it correct to use this dev_index to translate
to virq?


Thanks and regards,
Lokesh

> 
>> +			return desc->irq;
>> +
>> +	return -ENODEV;
>> +}
>> +EXPORT_SYMBOL_GPL(ti_sci_inta_msi_get_virq);
>> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
>> index 61706b430907..07ec8b390161 100644
>> --- a/include/linux/irqdomain.h
>> +++ b/include/linux/irqdomain.h
>> @@ -82,6 +82,7 @@ enum irq_domain_bus_token {
>>  	DOMAIN_BUS_NEXUS,
>>  	DOMAIN_BUS_IPI,
>>  	DOMAIN_BUS_FSL_MC_MSI,
>> +	DOMAIN_BUS_TI_SCI_INTA_MSI,
>>  };
>>  
>>  /**
>> diff --git a/include/linux/msi.h b/include/linux/msi.h
>> index 7e9b81c3b50d..7e12dfc4cb39 100644
>> --- a/include/linux/msi.h
>> +++ b/include/linux/msi.h
>> @@ -47,6 +47,11 @@ struct fsl_mc_msi_desc {
>>  	u16				msi_index;
>>  };
>>  
>> +struct ti_sci_inta_msi_desc {
>> +	u16	dev_id;
>> +	u16	index;
>> +};
>> +
>>  /**
>>   * struct msi_desc - Descriptor structure for MSI based interrupts
>>   * @list:	List head for management
>> @@ -106,6 +111,7 @@ struct msi_desc {
>>  		 */
>>  		struct platform_msi_desc platform;
>>  		struct fsl_mc_msi_desc fsl_mc;
>> +		struct ti_sci_inta_msi_desc inta;
>>  	};
>>  };
>>  
>> diff --git a/include/linux/soc/ti/ti_sci_inta_msi.h b/include/linux/soc/ti/ti_sci_inta_msi.h
>> new file mode 100644
>> index 000000000000..b0ca20ab3f49
>> --- /dev/null
>> +++ b/include/linux/soc/ti/ti_sci_inta_msi.h
>> @@ -0,0 +1,23 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Texas Instruments' K3 TI SCI INTA MSI helper
>> + *
>> + * Copyright (C) 2018-2019 Texas Instruments Incorporated - http://www.ti.com/
>> + *	Lokesh Vutla <lokeshvutla@ti.com>
>> + */
>> +
>> +#ifndef __INCLUDE_LINUX_TI_SCI_INTA_MSI_H
>> +#define __INCLUDE_LINUX_TI_SCI_INTA_MSI_H
>> +
>> +#include <linux/msi.h>
>> +#include <linux/soc/ti/ti_sci_protocol.h>
>> +
>> +struct irq_domain
>> +*ti_sci_inta_msi_create_irq_domain(struct fwnode_handle *fwnode,
>> +				   struct msi_domain_info *info,
>> +				   struct irq_domain *parent);
>> +int ti_sci_inta_msi_domain_alloc_irqs(struct platform_device *pdev,
>> +				      struct ti_sci_resource *res);
>> +unsigned int ti_sci_inta_msi_get_virq(struct platform_device *dev, u32 index);
>> +void ti_sci_inta_msi_domain_free_irqs(struct device *dev);
>> +#endif /* __INCLUDE_LINUX_IRQCHIP_TI_SCI_INTA_H */
>>
> 
> Thanks,
> 
> 	M.
> 

  reply index

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-10  4:13 [PATCH v6 00/12] Add support for TISCI Interrupt controller drivers Lokesh Vutla
2019-04-10  4:13 ` [PATCH v6 01/12] firmware: ti_sci: Add support to get TISCI handle using of_phandle Lokesh Vutla
2019-04-10  4:13 ` [PATCH v6 02/12] firmware: ti_sci: Add support for RM core ops Lokesh Vutla
2019-04-10  4:13 ` [PATCH v6 03/12] firmware: ti_sci: Add support for IRQ management Lokesh Vutla
2019-04-10  4:13 ` [PATCH v6 04/12] firmware: ti_sci: Add RM mapping table for am654 Lokesh Vutla
2019-04-11 14:54   ` Tony Lindgren
2019-04-12  4:08     ` Lokesh Vutla
2019-04-12 15:20       ` Tony Lindgren
2019-04-10  4:13 ` [PATCH v6 05/12] firmware: ti_sci: Add helper apis to manage resources Lokesh Vutla
2019-04-10  4:13 ` [PATCH v6 06/12] dt-bindings: irqchip: Introduce TISCI Interrupt router bindings Lokesh Vutla
2019-04-11 15:00   ` Tony Lindgren
2019-04-12  4:24     ` Lokesh Vutla
2019-04-12  8:42       ` Tero Kristo
2019-04-12 15:33         ` Tony Lindgren
2019-04-10  4:13 ` [PATCH v6 07/12] irqchip: ti-sci-intr: Add support for Interrupt Router driver Lokesh Vutla
2019-04-10  4:13 ` [PATCH v6 08/12] dt-bindings: irqchip: Introduce TISCI Interrupt Aggregator bindings Lokesh Vutla
2019-04-10  4:13 ` [PATCH v6 09/12] irqchip: ti-sci-inta: Add support for Interrupt Aggregator driver Lokesh Vutla
2019-04-17 11:16   ` Lokesh Vutla
2019-04-17 14:14   ` Marc Zyngier
2019-04-17 16:42     ` Lokesh Vutla
2019-04-10  4:13 ` [PATCH v6 10/12] soc: ti: Add MSI domain bus support for Interrupt Aggregator Lokesh Vutla
2019-04-17 16:34   ` Marc Zyngier
2019-04-17 16:59     ` Lokesh Vutla [this message]
2019-04-17 17:06       ` Marc Zyngier
2019-04-17 17:08         ` Lokesh Vutla
2019-04-10  4:13 ` [PATCH v6 11/12] irqchip: ti-sci-inta: Add msi domain support Lokesh Vutla
2019-04-10  4:13 ` [PATCH v6 12/12] soc: ti: am6: Enable interrupt controller driver Lokesh Vutla

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a660c601-33f2-243d-7d83-7622e2c0f1b9@ti.com \
    --to=lokeshvutla@ti.com \
    --cc=devicetree@vger.kernel.org \
    --cc=grygorii.strashko@ti.com \
    --cc=jason@lakedaemon.net \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=marc.zyngier@arm.com \
    --cc=nm@ti.com \
    --cc=nsekhar@ti.com \
    --cc=peter.ujfalusi@ti.com \
    --cc=robh+dt@kernel.org \
    --cc=ssantosh@kernel.org \
    --cc=t-kristo@ti.com \
    --cc=tony@atomide.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox