[v4,3/9] PCI: keystone: Convert to using hierarchy domain for legacy interrupts
diff mbox series

Message ID 20190221101518.22604-4-kishon@ti.com
State Superseded
Headers show
Series
  • PCI: DWC/Keystone: MSI configuration cleanup
Related show

Commit Message

Kishon Vijay Abraham I Feb. 21, 2019, 10:15 a.m. UTC
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(-)

Comments

Lorenzo Pieralisi Feb. 21, 2019, 4:24 p.m. UTC | #1
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.

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;
> +	}
> +
> +	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];
> +
> +	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 (fwspec->param_count != 2)
> +			return -EINVAL;
> +
> +		if (fwspec->param[0] >= PCI_NUM_INTX)
> +			return -EINVAL;
> +
> +		*hwirq = fwspec->param[0];
> +		*type = fwspec->param[1];
> +
> +		return 0;
> +	}
> +
> +	return -EINVAL;
>  }
>  
>  static const struct irq_domain_ops ks_pcie_legacy_irq_domain_ops = {
> -	.map = ks_pcie_init_legacy_irq_map,
> -	.xlate = irq_domain_xlate_onetwocell,
> +	.alloc		= ks_pcie_legacy_irq_domain_alloc,
> +	.free		= irq_domain_free_irqs_common,
> +	.translate	= ks_pcie_irq_domain_translate,
>  };
>  
>  /**
> @@ -572,35 +629,6 @@ static void ks_pcie_msi_irq_handler(struct irq_desc *desc)
>  	chained_irq_exit(chip, desc);
>  }
>  
> -/**
> - * ks_pcie_legacy_irq_handler() - Handle legacy interrupt
> - * @irq: IRQ line for legacy interrupts
> - * @desc: Pointer to irq descriptor
> - *
> - * Traverse through pending legacy interrupts and invoke handler for each. Also
> - * takes care of interrupt controller level mask/ack operation.
> - */
> -static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
> -{
> -	unsigned int irq = irq_desc_get_irq(desc);
> -	struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc);
> -	struct dw_pcie *pci = ks_pcie->pci;
> -	struct device *dev = pci->dev;
> -	u32 irq_offset = irq - ks_pcie->legacy_host_irqs[0];
> -	struct irq_chip *chip = irq_desc_get_chip(desc);
> -
> -	dev_dbg(dev, ": Handling legacy irq %d\n", irq);
> -
> -	/*
> -	 * The chained irq handler installation would have replaced normal
> -	 * interrupt driver handler so we need to take care of mask/unmask and
> -	 * ack operation.
> -	 */
> -	chained_irq_enter(chip, desc);
> -	ks_pcie_handle_legacy_irq(ks_pcie, irq_offset);
> -	chained_irq_exit(chip, desc);
> -}
> -
>  static int ks_pcie_config_msi_irq(struct keystone_pcie *ks_pcie)
>  {
>  	struct device *dev = ks_pcie->pci->dev;
> @@ -655,14 +683,33 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
>  	struct device *dev = ks_pcie->pci->dev;
>  	struct irq_domain *legacy_irq_domain;
>  	struct device_node *np = ks_pcie->np;
> +	struct irq_domain *parent_domain;
> +	struct device_node *parent_node;
>  	struct device_node *intc_np;
> -	int irq_count, irq, ret = 0, i;
> +	int irq_count, ret = 0;
>  
>  	intc_np = of_get_child_by_name(np, "legacy-interrupt-controller");
>  	if (!intc_np) {
>  		dev_warn(dev, "legacy-interrupt-controller node is absent\n");
>  		return -EINVAL;
>  	}
> +	ks_pcie->legacy_intc_np = intc_np;
> +
> +	parent_node = of_irq_find_parent(intc_np);
> +	if (!parent_node) {
> +		dev_err(dev, "unable to obtain parent node\n");
> +		ret = -ENXIO;
> +		goto err;
> +	}
> +
> +	parent_domain = irq_find_host(parent_node);
> +	if (!parent_domain) {
> +		dev_err(dev, "unable to obtain parent domain\n");
> +		ret = -ENXIO;
> +		goto err;
> +	}
> +
> +	of_node_put(parent_node);
>  
>  	irq_count = of_irq_count(intc_np);
>  	if (!irq_count) {
> @@ -671,31 +718,15 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
>  		goto err;
>  	}
>  
> -	for (i = 0; i < irq_count; i++) {
> -		irq = irq_of_parse_and_map(intc_np, i);
> -		if (!irq) {
> -			ret = -EINVAL;
> -			goto err;
> -		}
> -		ks_pcie->legacy_host_irqs[i] = irq;
> -
> -		irq_set_chained_handler_and_data(irq,
> -						 ks_pcie_legacy_irq_handler,
> -						 ks_pcie);
> -	}
> -
>  	legacy_irq_domain =
> -		irq_domain_add_linear(intc_np, PCI_NUM_INTX,
> -				      &ks_pcie_legacy_irq_domain_ops, NULL);
> +		irq_domain_add_hierarchy(parent_domain, 0, PCI_NUM_INTX,
> +					 intc_np,
> +					 &ks_pcie_legacy_irq_domain_ops,
> +					 ks_pcie);
>  	if (!legacy_irq_domain) {
>  		dev_err(dev, "Failed to add irq domain for legacy irqs\n");
>  		ret = -EINVAL;
> -		goto err;
>  	}
> -	ks_pcie->legacy_irq_domain = legacy_irq_domain;
> -
> -	for (i = 0; i < PCI_NUM_INTX; i++)
> -		ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET(i), INTx_EN);
>  
>  err:
>  	of_node_put(intc_np);
> -- 
> 2.17.1
>
Marc Zyngier Feb. 23, 2019, 12:11 p.m. UTC | #2
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.

> > +
> > +	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.

> > +
> > +	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.

> > +		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?

> > +
> > +		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?

> > +
> > +		*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).

> > +
> > +		return 0;
> > +	}
> > +
> > +	return -EINVAL;
> >  }
> >  
> >  static const struct irq_domain_ops ks_pcie_legacy_irq_domain_ops = {
> > -	.map = ks_pcie_init_legacy_irq_map,
> > -	.xlate = irq_domain_xlate_onetwocell,
> > +	.alloc		= ks_pcie_legacy_irq_domain_alloc,
> > +	.free		= irq_domain_free_irqs_common,
> > +	.translate	= ks_pcie_irq_domain_translate,
> >  };
> >  
> >  /**
> > @@ -572,35 +629,6 @@ static void ks_pcie_msi_irq_handler(struct irq_desc *desc)
> >  	chained_irq_exit(chip, desc);
> >  }
> >  
> > -/**
> > - * ks_pcie_legacy_irq_handler() - Handle legacy interrupt
> > - * @irq: IRQ line for legacy interrupts
> > - * @desc: Pointer to irq descriptor
> > - *
> > - * Traverse through pending legacy interrupts and invoke handler for each. Also
> > - * takes care of interrupt controller level mask/ack operation.
> > - */
> > -static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
> > -{
> > -	unsigned int irq = irq_desc_get_irq(desc);
> > -	struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc);
> > -	struct dw_pcie *pci = ks_pcie->pci;
> > -	struct device *dev = pci->dev;
> > -	u32 irq_offset = irq - ks_pcie->legacy_host_irqs[0];
> > -	struct irq_chip *chip = irq_desc_get_chip(desc);
> > -
> > -	dev_dbg(dev, ": Handling legacy irq %d\n", irq);
> > -
> > -	/*
> > -	 * The chained irq handler installation would have replaced normal
> > -	 * interrupt driver handler so we need to take care of mask/unmask and
> > -	 * ack operation.
> > -	 */
> > -	chained_irq_enter(chip, desc);
> > -	ks_pcie_handle_legacy_irq(ks_pcie, irq_offset);
> > -	chained_irq_exit(chip, desc);
> > -}
> > -
> >  static int ks_pcie_config_msi_irq(struct keystone_pcie *ks_pcie)
> >  {
> >  	struct device *dev = ks_pcie->pci->dev;
> > @@ -655,14 +683,33 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
> >  	struct device *dev = ks_pcie->pci->dev;
> >  	struct irq_domain *legacy_irq_domain;
> >  	struct device_node *np = ks_pcie->np;
> > +	struct irq_domain *parent_domain;
> > +	struct device_node *parent_node;
> >  	struct device_node *intc_np;
> > -	int irq_count, irq, ret = 0, i;
> > +	int irq_count, ret = 0;
> >  
> >  	intc_np = of_get_child_by_name(np, "legacy-interrupt-controller");
> >  	if (!intc_np) {
> >  		dev_warn(dev, "legacy-interrupt-controller node is absent\n");
> >  		return -EINVAL;
> >  	}
> > +	ks_pcie->legacy_intc_np = intc_np;
> > +
> > +	parent_node = of_irq_find_parent(intc_np);
> > +	if (!parent_node) {
> > +		dev_err(dev, "unable to obtain parent node\n");
> > +		ret = -ENXIO;
> > +		goto err;
> > +	}
> > +
> > +	parent_domain = irq_find_host(parent_node);
> > +	if (!parent_domain) {
> > +		dev_err(dev, "unable to obtain parent domain\n");
> > +		ret = -ENXIO;
> > +		goto err;
> > +	}
> > +
> > +	of_node_put(parent_node);
> >  
> >  	irq_count = of_irq_count(intc_np);
> >  	if (!irq_count) {
> > @@ -671,31 +718,15 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
> >  		goto err;
> >  	}
> >  
> > -	for (i = 0; i < irq_count; i++) {
> > -		irq = irq_of_parse_and_map(intc_np, i);
> > -		if (!irq) {
> > -			ret = -EINVAL;
> > -			goto err;
> > -		}
> > -		ks_pcie->legacy_host_irqs[i] = irq;
> > -
> > -		irq_set_chained_handler_and_data(irq,
> > -						 ks_pcie_legacy_irq_handler,
> > -						 ks_pcie);
> > -	}
> > -
> >  	legacy_irq_domain =
> > -		irq_domain_add_linear(intc_np, PCI_NUM_INTX,
> > -				      &ks_pcie_legacy_irq_domain_ops, NULL);
> > +		irq_domain_add_hierarchy(parent_domain, 0, PCI_NUM_INTX,
> > +					 intc_np,
> > +					 &ks_pcie_legacy_irq_domain_ops,
> > +					 ks_pcie);
> >  	if (!legacy_irq_domain) {
> >  		dev_err(dev, "Failed to add irq domain for legacy irqs\n");
> >  		ret = -EINVAL;
> > -		goto err;
> >  	}
> > -	ks_pcie->legacy_irq_domain = legacy_irq_domain;
> > -
> > -	for (i = 0; i < PCI_NUM_INTX; i++)
> > -		ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET(i), INTx_EN);
> >  
> >  err:
> >  	of_node_put(intc_np);
> > -- 
> > 2.17.1
> >   

The structure of the code otherwise looks good. I trust you'd be able
to shed some light on the questions I have.

Thanks,

	M.
Kishon Vijay Abraham I March 7, 2019, 9:12 a.m. UTC | #3
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
Marc Zyngier March 7, 2019, 12:02 p.m. UTC | #4
On Thu, 07 Mar 2019 09:12:30 +0000,
Kishon Vijay Abraham I <kishon@ti.com> wrote:
> 
> 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?

Up to you. I don't mind either way, but it is the lack of
documentation that annoys me the most here.

The more worrying part is that you are using an EDGE signalling in
these bindings, and that's wrong. PCI legacy interrupts are level,
always.

[...]

> > 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.

As I said above, this is wrong. Legacy interrupts are always level, by
definition. Why are you making them edge-triggered?

> > 
> >>> +
> >>> +		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).

I'm not disputing this. It is more that the DT code does number the
pin from 1, if memory serves well. Can you please verify this (see
of_irq_parse_pci).

> >
> >>> +
> >>> +		*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.

This looks completely wrong. MSIs are always edge, and legacy always
level, end of story. If your HW is any different, then it doesn't seem
to be compliant with PCI.

Thanks,

	M.
Kishon Vijay Abraham I March 18, 2019, 5:33 a.m. UTC | #5
Hi Marc,

On 07/03/19 5:32 PM, Marc Zyngier wrote:
> On Thu, 07 Mar 2019 09:12:30 +0000,
> Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>
>> 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?
> 
> Up to you. I don't mind either way, but it is the lack of
> documentation that annoys me the most here.

Okay, I'll add a comment before invoking of_irq_parse_one.
> 
> The more worrying part is that you are using an EDGE signalling in
> these bindings, and that's wrong. PCI legacy interrupts are level,
> always.

Agreed. But the HW uses pulse signal whenever it sees an Assert_Intx message.
> 
> [...]
> 
>>> 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.
> 
> As I said above, this is wrong. Legacy interrupts are always level, by
> definition. Why are you making them edge-triggered
> 
>>>
>>>>> +
>>>>> +		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).
> 
> I'm not disputing this. It is more that the DT code does number the
> pin from 1, if memory serves well. Can you please verify this (see
> of_irq_parse_pci).

Yes it does number from '1'. However I use interrupt-map property to map from
legacy interrupt pin to a interrupt number of the legacy interrupt controller
which starts from '0'.
> 
>>>
>>>>> +
>>>>> +		*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.
> 
> This looks completely wrong. MSIs are always edge, and legacy always
> level, end of story. If your HW is any different, then it doesn't seem
> to be compliant with PCI.

Agree that Legacy interrupts ought to be level-triggered. I checked this with
HW team and I've copy-pasted the response.

"The legacy interrupts from the PCIe controller are still level. The PCIe
controller will hold the legacy interrupt lines high until a de-assert message
is sent back. The pulse interrupt is only to send the interrupt from the PCIe
sub-system to the SoC interrupt controller. This should not impact the
operation of the legacy interrupt on the PCIe side."

Looks like the interrupts are propagated multiple levels and only the last
level to the interrupt controller is pulse.

Thanks
Kishon
Marc Zyngier March 18, 2019, 11:16 a.m. UTC | #6
On Mon, 18 Mar 2019 11:03:07 +0530
Kishon Vijay Abraham I <kishon@ti.com> wrote:

> Hi Marc,
> 
> On 07/03/19 5:32 PM, Marc Zyngier wrote:
> > On Thu, 07 Mar 2019 09:12:30 +0000,
> > Kishon Vijay Abraham I <kishon@ti.com> wrote:  
> >>
> >> 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?  
> > 
> > Up to you. I don't mind either way, but it is the lack of
> > documentation that annoys me the most here.  
> 
> Okay, I'll add a comment before invoking of_irq_parse_one.
> > 
> > The more worrying part is that you are using an EDGE signalling in
> > these bindings, and that's wrong. PCI legacy interrupts are level,
> > always.  
> 
> Agreed. But the HW uses pulse signal whenever it sees an Assert_Intx message.

I'm not convinced... See below.

> > 
> > [...]
> >   
> >>> 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.  
> > 
> > As I said above, this is wrong. Legacy interrupts are always level, by
> > definition. Why are you making them edge-triggered
> >   
> >>>  
> >>>>> +
> >>>>> +		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).  
> > 
> > I'm not disputing this. It is more that the DT code does number the
> > pin from 1, if memory serves well. Can you please verify this (see
> > of_irq_parse_pci).  
> 
> Yes it does number from '1'. However I use interrupt-map property to map from
> legacy interrupt pin to a interrupt number of the legacy interrupt controller
> which starts from '0'.

If you've verified that it actually works, fine bby me.

> >   
> >>>  
> >>>>> +
> >>>>> +		*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.  
> > 
> > This looks completely wrong. MSIs are always edge, and legacy always
> > level, end of story. If your HW is any different, then it doesn't seem
> > to be compliant with PCI.  
> 
> Agree that Legacy interrupts ought to be level-triggered. I checked this with
> HW team and I've copy-pasted the response.
> 
> "The legacy interrupts from the PCIe controller are still level. The PCIe
> controller will hold the legacy interrupt lines high until a de-assert message
> is sent back. The pulse interrupt is only to send the interrupt from the PCIe
> sub-system to the SoC interrupt controller. This should not impact the
> operation of the legacy interrupt on the PCIe side."
> 
> Looks like the interrupts are propagated multiple levels and only the last
> level to the interrupt controller is pulse.

Let's take an example: My device (let's assume a network controller of
some sort) generates an interrupt. Level is high, and you observe an
edge. The driver handles the interrupt, but due to a race, the line
stays high (the device has received a new packet).

How is a *new* edge generated? The line never transited to low, and we
have more packet to process. With a level triggered interrupt, you just
take the interrupt again. With an edge interrupt, you need to actively
resample the level and retrigger the interrupt. How is this done?

Similar issues exist with interrupt sharing, which is a fairly common
thing.

Thanks,

	M.
Kishon Vijay Abraham I March 19, 2019, 10:52 a.m. UTC | #7
Hi Marc,

On 18/03/19 4:46 PM, Marc Zyngier wrote:
> On Mon, 18 Mar 2019 11:03:07 +0530
> Kishon Vijay Abraham I <kishon@ti.com> wrote:
> 
>> Hi Marc,
>>
>> On 07/03/19 5:32 PM, Marc Zyngier wrote:
>>> On Thu, 07 Mar 2019 09:12:30 +0000,
>>> Kishon Vijay Abraham I <kishon@ti.com> wrote:  
>>>>
>>>> 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?  
>>>
>>> Up to you. I don't mind either way, but it is the lack of
>>> documentation that annoys me the most here.  
>>
>> Okay, I'll add a comment before invoking of_irq_parse_one.
>>>
>>> The more worrying part is that you are using an EDGE signalling in
>>> these bindings, and that's wrong. PCI legacy interrupts are level,
>>> always.  
>>
>> Agreed. But the HW uses pulse signal whenever it sees an Assert_Intx message.
> 
> I'm not convinced... See below.
> 
>>>
>>> [...]
>>>   
>>>>> 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.  
>>>
>>> As I said above, this is wrong. Legacy interrupts are always level, by
>>> definition. Why are you making them edge-triggered
>>>   
>>>>>  
>>>>>>> +
>>>>>>> +		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).  
>>>
>>> I'm not disputing this. It is more that the DT code does number the
>>> pin from 1, if memory serves well. Can you please verify this (see
>>> of_irq_parse_pci).  
>>
>> Yes it does number from '1'. However I use interrupt-map property to map from
>> legacy interrupt pin to a interrupt number of the legacy interrupt controller
>> which starts from '0'.
> 
> If you've verified that it actually works, fine bby me.
> 
>>>   
>>>>>  
>>>>>>> +
>>>>>>> +		*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.  
>>>
>>> This looks completely wrong. MSIs are always edge, and legacy always
>>> level, end of story. If your HW is any different, then it doesn't seem
>>> to be compliant with PCI.  
>>
>> Agree that Legacy interrupts ought to be level-triggered. I checked this with
>> HW team and I've copy-pasted the response.
>>
>> "The legacy interrupts from the PCIe controller are still level. The PCIe
>> controller will hold the legacy interrupt lines high until a de-assert message
>> is sent back. The pulse interrupt is only to send the interrupt from the PCIe
>> sub-system to the SoC interrupt controller. This should not impact the
>> operation of the legacy interrupt on the PCIe side."
>>
>> Looks like the interrupts are propagated multiple levels and only the last
>> level to the interrupt controller is pulse.
> 
> Let's take an example: My device (let's assume a network controller of
> some sort) generates an interrupt. Level is high, and you observe an
> edge. The driver handles the interrupt, but due to a race, the line
> stays high (the device has received a new packet).
> 
> How is a *new* edge generated? The line never transited to low, and we
> have more packet to process. With a level triggered interrupt, you just
> take the interrupt again. With an edge interrupt, you need to actively
> resample the level and retrigger the interrupt. How is this done?

There might be an actual issue in the HW while converting from level to edge
where new edge interrupt might not be generated in the case you just mentioned.
I am checking this with HW folks.

Since this might take time, I'll remove this patch and resend only the MSI
cleanup in order to make progress.

Thanks
Kishon
Marc Zyngier March 19, 2019, 11:35 a.m. UTC | #8
On Tue, 19 Mar 2019 16:22:39 +0530
Kishon Vijay Abraham I <kishon@ti.com> wrote:

Hi Kishon,

> Hi Marc,
> 
> On 18/03/19 4:46 PM, Marc Zyngier wrote:
> > On Mon, 18 Mar 2019 11:03:07 +0530
> > Kishon Vijay Abraham I <kishon@ti.com> wrote:
> >   
> >> Hi Marc,
> >>
> >> On 07/03/19 5:32 PM, Marc Zyngier wrote:  
> >>> On Thu, 07 Mar 2019 09:12:30 +0000,
> >>> Kishon Vijay Abraham I <kishon@ti.com> wrote:    


[...]

> >>>> 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.    
> >>>
> >>> This looks completely wrong. MSIs are always edge, and legacy always
> >>> level, end of story. If your HW is any different, then it doesn't seem
> >>> to be compliant with PCI.    
> >>
> >> Agree that Legacy interrupts ought to be level-triggered. I checked this with
> >> HW team and I've copy-pasted the response.
> >>
> >> "The legacy interrupts from the PCIe controller are still level. The PCIe
> >> controller will hold the legacy interrupt lines high until a de-assert message
> >> is sent back. The pulse interrupt is only to send the interrupt from the PCIe
> >> sub-system to the SoC interrupt controller. This should not impact the
> >> operation of the legacy interrupt on the PCIe side."
> >>
> >> Looks like the interrupts are propagated multiple levels and only the last
> >> level to the interrupt controller is pulse.  
> > 
> > Let's take an example: My device (let's assume a network controller of
> > some sort) generates an interrupt. Level is high, and you observe an
> > edge. The driver handles the interrupt, but due to a race, the line
> > stays high (the device has received a new packet).
> > 
> > How is a *new* edge generated? The line never transited to low, and we
> > have more packet to process. With a level triggered interrupt, you just
> > take the interrupt again. With an edge interrupt, you need to actively
> > resample the level and retrigger the interrupt. How is this done?  
> 
> There might be an actual issue in the HW while converting from level to edge
> where new edge interrupt might not be generated in the case you just mentioned.
> I am checking this with HW folks.

There is (non-PCI) HW that does that. On EOI, they force the interrupt
controller to resample the level, which results in a new edge to be
generated if the level is high. This of course require some HW, usually
a register taking the ID of the line that needs resampling. I'd be
surprised if there wasn't something along those lines in your HW, at
least as a debug mechanism.

> Since this might take time, I'll remove this patch and resend only the MSI
> cleanup in order to make progress.

OK.

Thanks,

	M.

Patch
diff mbox series

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;
+	}
+
+	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];
+
+	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 (fwspec->param_count != 2)
+			return -EINVAL;
+
+		if (fwspec->param[0] >= PCI_NUM_INTX)
+			return -EINVAL;
+
+		*hwirq = fwspec->param[0];
+		*type = fwspec->param[1];
+
+		return 0;
+	}
+
+	return -EINVAL;
 }
 
 static const struct irq_domain_ops ks_pcie_legacy_irq_domain_ops = {
-	.map = ks_pcie_init_legacy_irq_map,
-	.xlate = irq_domain_xlate_onetwocell,
+	.alloc		= ks_pcie_legacy_irq_domain_alloc,
+	.free		= irq_domain_free_irqs_common,
+	.translate	= ks_pcie_irq_domain_translate,
 };
 
 /**
@@ -572,35 +629,6 @@  static void ks_pcie_msi_irq_handler(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
-/**
- * ks_pcie_legacy_irq_handler() - Handle legacy interrupt
- * @irq: IRQ line for legacy interrupts
- * @desc: Pointer to irq descriptor
- *
- * Traverse through pending legacy interrupts and invoke handler for each. Also
- * takes care of interrupt controller level mask/ack operation.
- */
-static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
-{
-	unsigned int irq = irq_desc_get_irq(desc);
-	struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc);
-	struct dw_pcie *pci = ks_pcie->pci;
-	struct device *dev = pci->dev;
-	u32 irq_offset = irq - ks_pcie->legacy_host_irqs[0];
-	struct irq_chip *chip = irq_desc_get_chip(desc);
-
-	dev_dbg(dev, ": Handling legacy irq %d\n", irq);
-
-	/*
-	 * The chained irq handler installation would have replaced normal
-	 * interrupt driver handler so we need to take care of mask/unmask and
-	 * ack operation.
-	 */
-	chained_irq_enter(chip, desc);
-	ks_pcie_handle_legacy_irq(ks_pcie, irq_offset);
-	chained_irq_exit(chip, desc);
-}
-
 static int ks_pcie_config_msi_irq(struct keystone_pcie *ks_pcie)
 {
 	struct device *dev = ks_pcie->pci->dev;
@@ -655,14 +683,33 @@  static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
 	struct device *dev = ks_pcie->pci->dev;
 	struct irq_domain *legacy_irq_domain;
 	struct device_node *np = ks_pcie->np;
+	struct irq_domain *parent_domain;
+	struct device_node *parent_node;
 	struct device_node *intc_np;
-	int irq_count, irq, ret = 0, i;
+	int irq_count, ret = 0;
 
 	intc_np = of_get_child_by_name(np, "legacy-interrupt-controller");
 	if (!intc_np) {
 		dev_warn(dev, "legacy-interrupt-controller node is absent\n");
 		return -EINVAL;
 	}
+	ks_pcie->legacy_intc_np = intc_np;
+
+	parent_node = of_irq_find_parent(intc_np);
+	if (!parent_node) {
+		dev_err(dev, "unable to obtain parent node\n");
+		ret = -ENXIO;
+		goto err;
+	}
+
+	parent_domain = irq_find_host(parent_node);
+	if (!parent_domain) {
+		dev_err(dev, "unable to obtain parent domain\n");
+		ret = -ENXIO;
+		goto err;
+	}
+
+	of_node_put(parent_node);
 
 	irq_count = of_irq_count(intc_np);
 	if (!irq_count) {
@@ -671,31 +718,15 @@  static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
 		goto err;
 	}
 
-	for (i = 0; i < irq_count; i++) {
-		irq = irq_of_parse_and_map(intc_np, i);
-		if (!irq) {
-			ret = -EINVAL;
-			goto err;
-		}
-		ks_pcie->legacy_host_irqs[i] = irq;
-
-		irq_set_chained_handler_and_data(irq,
-						 ks_pcie_legacy_irq_handler,
-						 ks_pcie);
-	}
-
 	legacy_irq_domain =
-		irq_domain_add_linear(intc_np, PCI_NUM_INTX,
-				      &ks_pcie_legacy_irq_domain_ops, NULL);
+		irq_domain_add_hierarchy(parent_domain, 0, PCI_NUM_INTX,
+					 intc_np,
+					 &ks_pcie_legacy_irq_domain_ops,
+					 ks_pcie);
 	if (!legacy_irq_domain) {
 		dev_err(dev, "Failed to add irq domain for legacy irqs\n");
 		ret = -EINVAL;
-		goto err;
 	}
-	ks_pcie->legacy_irq_domain = legacy_irq_domain;
-
-	for (i = 0; i < PCI_NUM_INTX; i++)
-		ks_pcie_app_writel(ks_pcie, IRQ_ENABLE_SET(i), INTx_EN);
 
 err:
 	of_node_put(intc_np);