Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
From: Kishon Vijay Abraham I <kishon@ti.com>
To: Marc Zyngier <marc.zyngier@arm.com>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Murali Karicheri <m-karicheri2@ti.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Jingoo Han <jingoohan1@gmail.com>,
	Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
	<linux-pci@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v4 3/9] PCI: keystone: Convert to using hierarchy domain for legacy interrupts
Date: Thu, 7 Mar 2019 14:42:30 +0530
Message-ID: <4c55487d-213f-f5f5-6d67-93a417d2a0d7@ti.com> (raw)
In-Reply-To: <20190223121143.14c1f150@why.wild-wind.fr.eu.org>

Hi,

On 23/02/19 5:41 PM, Marc Zyngier wrote:
> On Thu, 21 Feb 2019 16:24:14 +0000
> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> 
>> On Thu, Feb 21, 2019 at 03:45:12PM +0530, Kishon Vijay Abraham I wrote:
>>> K2G provides separate IRQ lines for each of the four legacy interrupts.
>>> Model this using hierarchy domain instead of linear domain with chained
>>> IRQ handler.
>>>
>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>> ---
>>>  drivers/pci/controller/dwc/pci-keystone.c | 205 +++++++++++++---------
>>>  1 file changed, 118 insertions(+), 87 deletions(-)  
>>
>> Hi Kishon,
>>
>> I CC'ed Marc because you are actually re-writing an interrupt controller
>> driver so I would be happier to merge this refactoring if Marc can have
>> a look and he is satisfied with it - more so because most of the code can
>> be reused by other host bridge drivers with similar behaviour.
> 
> Cheers Lorenzo.
> 
> It doesn't look too bad, but there is a couple of points I'd like to see
> clarified. Comments below.
> 
>>
>> I will have a look too, unfortunately it is becoming a bit tight for
>> v5.1 but let's see how it goes.
>>
>> Thanks,
>> Lorenzo
>>
>>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
>>> index 47f0dcf638f2..7f1648453f54 100644
>>> --- a/drivers/pci/controller/dwc/pci-keystone.c
>>> +++ b/drivers/pci/controller/dwc/pci-keystone.c
>>> @@ -61,6 +61,7 @@
>>>  
>>>  #define IRQ_STATUS(n)			(0x184 + ((n) << 4))
>>>  #define IRQ_ENABLE_SET(n)		(0x188 + ((n) << 4))
>>> +#define IRQ_ENABLE_CLR(n)		(0x18C + ((n) << 4))
>>>  #define INTx_EN				BIT(0)
>>>  
>>>  #define ERR_IRQ_STATUS			0x1c4
>>> @@ -87,7 +88,6 @@ struct keystone_pcie {
>>>  	struct dw_pcie		*pci;
>>>  	/* PCI Device ID */
>>>  	u32			device_id;
>>> -	int			legacy_host_irqs[PCI_NUM_INTX];
>>>  	struct			device_node *legacy_intc_np;
>>>  
>>>  	int			msi_host_irqs[MAX_MSI_HOST_IRQS];
>>> @@ -96,7 +96,6 @@ struct keystone_pcie {
>>>  	struct phy		**phy;
>>>  	struct device_link	**link;
>>>  	struct			device_node *msi_intc_np;
>>> -	struct irq_domain	*legacy_irq_domain;
>>>  	struct device_node	*np;
>>>  
>>>  	int error_irq;
>>> @@ -199,26 +198,6 @@ static int ks_pcie_msi_host_init(struct pcie_port *pp)
>>>  	return dw_pcie_allocate_domains(pp);
>>>  }
>>>  
>>> -static void ks_pcie_handle_legacy_irq(struct keystone_pcie *ks_pcie,
>>> -				      int offset)
>>> -{
>>> -	struct dw_pcie *pci = ks_pcie->pci;
>>> -	struct device *dev = pci->dev;
>>> -	u32 pending;
>>> -	int virq;
>>> -
>>> -	pending = ks_pcie_app_readl(ks_pcie, IRQ_STATUS(offset));
>>> -
>>> -	if (BIT(0) & pending) {
>>> -		virq = irq_linear_revmap(ks_pcie->legacy_irq_domain, offset);
>>> -		dev_dbg(dev, ": irq: irq_offset %d, virq %d\n", offset, virq);
>>> -		generic_handle_irq(virq);
>>> -	}
>>> -
>>> -	/* EOI the INTx interrupt */
>>> -	ks_pcie_app_writel(ks_pcie, IRQ_EOI, offset);
>>> -}
>>> -
>>>  static void ks_pcie_enable_error_irq(struct keystone_pcie *ks_pcie)
>>>  {
>>>  	ks_pcie_app_writel(ks_pcie, ERR_IRQ_ENABLE_SET, ERR_IRQ_ALL);
>>> @@ -256,39 +235,117 @@ static irqreturn_t ks_pcie_handle_error_irq(struct keystone_pcie *ks_pcie)
>>>  	return IRQ_HANDLED;
>>>  }
>>>  
>>> -static void ks_pcie_ack_legacy_irq(struct irq_data *d)
>>> +void ks_pcie_irq_eoi(struct irq_data *data)
>>>  {
>>> +	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
>>> +	irq_hw_number_t hwirq = data->hwirq;
>>> +
>>> +	ks_pcie_app_writel(ks_pcie, IRQ_EOI, hwirq);
>>> +	irq_chip_eoi_parent(data);
>>>  }
>>>  
>>> -static void ks_pcie_mask_legacy_irq(struct irq_data *d)
>>> +void ks_pcie_irq_enable(struct irq_data *data)
>>>  {
>>> +	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
>>> +	irq_hw_number_t hwirq = data->hwirq;
>>> +
>>> +	ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET(hwirq), INTx_EN);
>>> +	irq_chip_enable_parent(data);
>>>  }
>>>  
>>> -static void ks_pcie_unmask_legacy_irq(struct irq_data *d)
>>> +void ks_pcie_irq_disable(struct irq_data *data)
>>>  {
>>> +	struct keystone_pcie *ks_pcie = irq_data_get_irq_chip_data(data);
>>> +	irq_hw_number_t hwirq = data->hwirq;
>>> +
>>> +	ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_CLR(hwirq), INTx_EN);
>>> +	irq_chip_disable_parent(data);
>>>  }
>>>  
>>>  static struct irq_chip ks_pcie_legacy_irq_chip = {
>>> -	.name = "Keystone-PCI-Legacy-IRQ",
>>> -	.irq_ack = ks_pcie_ack_legacy_irq,
>>> -	.irq_mask = ks_pcie_mask_legacy_irq,
>>> -	.irq_unmask = ks_pcie_unmask_legacy_irq,
>>> +	.name			= "Keystone-PCI-Legacy-IRQ",
>>> +	.irq_enable		= ks_pcie_irq_enable,
>>> +	.irq_disable		= ks_pcie_irq_disable,
>>> +	.irq_eoi		= ks_pcie_irq_eoi,
>>> +	.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,
>>>  };
>>>  
>>> -static int ks_pcie_init_legacy_irq_map(struct irq_domain *d,
>>> -				       unsigned int irq,
>>> -				       irq_hw_number_t hw_irq)
>>> +static int ks_pcie_legacy_irq_domain_alloc(struct irq_domain *domain,
>>> +					   unsigned int virq,
>>> +					   unsigned int nr_irqs, void *data)
>>>  {
>>> -	irq_set_chip_and_handler(irq, &ks_pcie_legacy_irq_chip,
>>> -				 handle_level_irq);
>>> -	irq_set_chip_data(irq, d->host_data);
>>> +	struct keystone_pcie *ks_pcie = domain->host_data;
>>> +	struct device_node *np = ks_pcie->legacy_intc_np;
>>> +	struct irq_fwspec parent_fwspec, *fwspec = data;
>>> +	struct of_phandle_args out_irq;
>>> +	int ret, i;
>>> +
>>> +	if (nr_irqs != 1)
>>> +		return -EINVAL;
>>> +
>>> +	ret = of_irq_parse_one(np, fwspec->param[0], &out_irq);
>>> +	if (ret < 0) {
>>> +		pr_err("Failed to parse interrupt node\n");
>>> +		return ret;
>>> +	}
> 
> What it this trying to do? Fishing out the interrupts from DT based on
> the legacy pin? This looks at best obscure. I wonder why you don't do
> that at probe time instead. Anyway, this requires documenting.

The device-tree of PCIe node looks something like below.

interrupt-map = <0 0 0 1 &pcie_intc0 0 IRQ_TYPE_EDGE_RISING>, /* INT A */
		<0 0 0 2 &pcie_intc0 1 IRQ_TYPE_EDGE_RISING>, /* INT B */
		<0 0 0 3 &pcie_intc0 2 IRQ_TYPE_EDGE_RISING>, /* INT C */
		<0 0 0 4 &pcie_intc0 3 IRQ_TYPE_EDGE_RISING>; /* INT D */
pcie_intc0: legacy-interrupt-controller {
	interrupt-controller;
	#interrupt-cells = <2>;
	interrupt-parent = <&gic>;
	interrupts = <GIC_SPI 48 IRQ_TYPE_EDGE_RISING>,
		     <GIC_SPI 49 IRQ_TYPE_EDGE_RISING>,
		     <GIC_SPI 50 IRQ_TYPE_EDGE_RISING>,
		     <GIC_SPI 51 IRQ_TYPE_EDGE_RISING>;
};

INTA corresponds to HWIRQ '0' of the hierarchy irq domain we create in this
driver which in turn corresponds to GIC_SPI '48' of GIC.

We could create an array of parent_fwspec for each of the four interrupt lines
in legacy-interrupt-controller during probe and use it directly here while
invoking irq_domain_alloc_irqs_parent. I think you want me to do that instead
of using of_irq_parse_one here?
> 
>>> +
>>> +	parent_fwspec.fwnode = &out_irq.np->fwnode;
>>> +	parent_fwspec.param_count = out_irq.args_count;
>>> +
>>> +	for (i = 0; i < out_irq.args_count; i++)
>>> +		parent_fwspec.param[i] = out_irq.args[i];
> 
> This feels like a duplicate of of_phandle_args_to_fwspec(). If you need
> such a helper, please export it from the irqdomain code.

sure.
> 
>>> +
>>> +	ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec);
>>> +	if (ret < 0) {
>>> +		pr_err("Failed to allocate parent irq %u: %d\n",
>>> +		       parent_fwspec.param[0], ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	ret = irq_domain_set_hwirq_and_chip(domain, virq, fwspec->param[0],
>>> +					    &ks_pcie_legacy_irq_chip, ks_pcie);
>>> +	if (ret < 0) {
>>> +		pr_err("Failed to set hwirq and chip\n");
>>> +		goto err_set_hwirq_and_chip;
>>> +	}
>>>  
>>>  	return 0;
>>> +
>>> +err_set_hwirq_and_chip:
>>> +	irq_domain_free_irqs_parent(domain, virq, 1);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static int ks_pcie_irq_domain_translate(struct irq_domain *domain,
>>> +					struct irq_fwspec *fwspec,
>>> +					unsigned long *hwirq,
>>> +					unsigned int *type)
>>> +{
>>> +	if (is_of_node(fwspec->fwnode)) {
> 
> If you don't plan to support ACPI, you can drop this.

okay.
> 
>>> +		if (fwspec->param_count != 2)
>>> +			return -EINVAL;
> 
> From the DT binding:
> 
> pcie_intc: Interrupt controller device node for Legacy IRQ chip
>         interrupt-cells: should be set to 1
> 
> So why do we have 2 cells here?

With '1' cells, it's not possible to specify irq trigger type. DT binding has
to be fixed. I'll fix this in the next revision of the patch series.
> 
>>> +
>>> +		if (fwspec->param[0] >= PCI_NUM_INTX)
>>> +			return -EINVAL;
> 
> Most of the OF code assumes that the pin number describing the legacy
> interrupt is 1-based, while you obviously treat it as 0-based. How does
> it work?

INTA corresponds to '0' of the hierarchy interrupt domain (using interrupt-map).
> 
>>> +
>>> +		*hwirq = fwspec->param[0];
>>> +		*type = fwspec->param[1];
> 
> As far as I remember, PCI legacy interrupts are level triggered, so
> there should be no need to advertise the trigger (which is consistent
> with the way the binding is written).

It is pulse triggered at subsystem level. Quoting the TRM
"The interrupt request signal at the PCIe SS boundary is a pulse signal that is
triggered each time an assert interrupt message is received." The PCIe
subsystem also has a level signal (interrupt pending signal) but the interrupt
request signal is the one that is connected to GIC.

Thanks
Kishon

  reply index

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-21 10:15 [PATCH v4 0/9] PCI: DWC/Keystone: MSI configuration cleanup Kishon Vijay Abraham I
2019-02-21 10:15 ` [PATCH v4 1/9] PCI: keystone: Cleanup interrupt related macros Kishon Vijay Abraham I
2019-02-21 10:15 ` [PATCH v4 2/9] PCI: keystone: Add separate functions for configuring MSI and legacy interrupt Kishon Vijay Abraham I
2019-02-21 10:15 ` [PATCH v4 3/9] PCI: keystone: Convert to using hierarchy domain for legacy interrupts Kishon Vijay Abraham I
2019-02-21 16:24   ` Lorenzo Pieralisi
2019-02-23 12:11     ` Marc Zyngier
2019-03-07  9:12       ` Kishon Vijay Abraham I [this message]
2019-03-07 12:02         ` Marc Zyngier
2019-03-18  5:33           ` Kishon Vijay Abraham I
2019-03-18 11:16             ` Marc Zyngier
2019-03-19 10:52               ` Kishon Vijay Abraham I
2019-03-19 11:35                 ` Marc Zyngier
2019-02-21 10:15 ` [PATCH v4 4/9] PCI: keystone: Use hwirq to get the MSI IRQ number offset Kishon Vijay Abraham I
2019-02-21 10:15 ` [PATCH v4 5/9] PCI: keystone: Cleanup ks_pcie_msi_irq_handler Kishon Vijay Abraham I
2019-02-21 10:15 ` [PATCH v4 6/9] PCI: dwc: Add support to use non default msi_irq_chip Kishon Vijay Abraham I
2019-02-21 10:15 ` [PATCH v4 7/9] PCI: keystone: Use Keystone specific msi_irq_chip Kishon Vijay Abraham I
2019-02-21 10:15 ` [PATCH v4 8/9] PCI: dwc: Remove Keystone specific dw_pcie_host_ops Kishon Vijay Abraham I
2019-02-21 10:15 ` [PATCH v4 9/9] PCI: dwc: Do not write to MSI control registers if the platform doesn't use it Kishon Vijay Abraham I

Reply instructions:

You may reply publicly 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=4c55487d-213f-f5f5-6d67-93a417d2a0d7@ti.com \
    --to=kishon@ti.com \
    --cc=bhelgaas@google.com \
    --cc=gustavo.pimentel@synopsys.com \
    --cc=jingoohan1@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=m-karicheri2@ti.com \
    --cc=marc.zyngier@arm.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

Linux-PCI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git

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

Example config snippet for mirrors

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


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