[v5,5/7] PCI: qcom: Handle MSI IRQs properly

On some of Qualcomm platforms each group of 32 MSI vectors is routed to the
separate GIC interrupt. Thus to receive higher MSI vectors properly,
add separate msi_host_init()/msi_host_deinit() handling additional host
IRQs.

Note, that if DT doesn't list extra MSI interrupts, the driver will limit
the amount of supported MSI vectors accordingly (to 32).

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 137 ++++++++++++++++++++++++-
 1 file changed, 136 insertions(+), 1 deletion(-)

In subject, "Handle MSI IRQs properly" really doesn't tell us anything
useful.  The existing MSI support handles some MSI IRQs "properly," so
we should say something specific about the improvements here, like
"Handle multiple MSI groups" or "Handle MSIs routed to multiple GIC
interrupts" or "Handle split MSI IRQs" or similar.

On Sat, Apr 30, 2022 at 12:42:48AM +0300, Dmitry Baryshkov wrote:
> On some of Qualcomm platforms each group of 32 MSI vectors is routed to the
> separate GIC interrupt. Thus to receive higher MSI vectors properly,
> add separate msi_host_init()/msi_host_deinit() handling additional host
> IRQs.

> +static void qcom_chained_msi_isr(struct irq_desc *desc)
> +{
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	int irq = irq_desc_get_irq(desc);
> +	struct pcie_port *pp;
> +	int idx, pos;
> +	unsigned long val;
> +	u32 status, num_ctrls;
> +	struct dw_pcie *pci;
> +	struct qcom_pcie *pcie;
> +
> +	chained_irq_enter(chip, desc);
> +
> +	pp = irq_desc_get_handler_data(desc);
> +	pci = to_dw_pcie_from_pp(pp);
> +	pcie = to_qcom_pcie(pci);
> +
> +	/*
> +	 * Unlike generic dw_handle_msi_irq we can determine, which group of
> +	 * MSIs triggered the IRQ, so process just single group.

Parens and punctuation touch-up:

  Unlike generic dw_handle_msi_irq(), we can determine which group of
  MSIs triggered the IRQ, so process just that group.

> +	 */
> +	num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL;
> +
> +	for (idx = 0; idx < num_ctrls; idx++) {
> +		if (pcie->msi_irqs[idx] == irq)
> +			break;
> +	}

Since this is basically an enhanced clone of dw_handle_msi_irq(), it
would be nice to use the same variable names ("i" instead of "idx")
so it's not gratuitously different.

Actually, I wonder if you could enhance dw_handle_msi_irq() slightly
so you could use it directly, e.g.,

    struct dw_pcie_host_ops {
      ...
      void (*msi_host_deinit)(struct pcie_port *pp);
 +    bool (*msi_in_block)(struct pcie_port *pp, int irq, int i);
    };

    dw_handle_msi_irq(...)
    {
      ...

      for (i = 0; i < num_ctrls; i++) {
 +      if (pp->ops->msi_in_block && !pp->ops->msi_in_block(pp, irq, i))
 +        continue;

	status = dw_pcie_readl_dbi(pci, PCIE_MSI_INTR0_STATUS ...);
	...

 +  bool qcom_pcie_msi_in_block(struct pcie_port *pp, int irq, int i)
 +  {
 +    ...
 +    pci = to_dw_pcie_from_pp(pp);
 +    pcie = to_qcom_pcie(pci);
 +
 +    if (pcie->msi_irqs[i] == irq)
 +      return true;
 +
 +    return false;
 +  }

Maybe that's more complicated than it's worth.

> +
> +	if (WARN_ON_ONCE(unlikely(idx == num_ctrls)))
> +		goto out;
> +
> +	status = dw_pcie_readl_dbi(pci, PCIE_MSI_INTR0_STATUS +
> +				   (idx * MSI_REG_CTRL_BLOCK_SIZE));
> +	if (!status)
> +		goto out;
> +
> +	val = status;
> +	pos = 0;
> +	while ((pos = find_next_bit(&val, MAX_MSI_IRQS_PER_CTRL,
> +				    pos)) != MAX_MSI_IRQS_PER_CTRL) {
> +		generic_handle_domain_irq(pp->irq_domain,
> +					  (idx * MAX_MSI_IRQS_PER_CTRL) +
> +					  pos);
> +		pos++;
> +	}
> +
> +out:
> +	chained_irq_exit(chip, desc);
> +}

On 30/04/2022 01:47, Bjorn Helgaas wrote:
> In subject, "Handle MSI IRQs properly" really doesn't tell us anything
> useful.  The existing MSI support handles some MSI IRQs "properly," so
> we should say something specific about the improvements here, like
> "Handle multiple MSI groups" or "Handle MSIs routed to multiple GIC
> interrupts" or "Handle split MSI IRQs" or similar.
> 
> On Sat, Apr 30, 2022 at 12:42:48AM +0300, Dmitry Baryshkov wrote:
>> On some of Qualcomm platforms each group of 32 MSI vectors is routed to the
>> separate GIC interrupt. Thus to receive higher MSI vectors properly,
>> add separate msi_host_init()/msi_host_deinit() handling additional host
>> IRQs.
> 
>> +static void qcom_chained_msi_isr(struct irq_desc *desc)
>> +{
>> +	struct irq_chip *chip = irq_desc_get_chip(desc);
>> +	int irq = irq_desc_get_irq(desc);
>> +	struct pcie_port *pp;
>> +	int idx, pos;
>> +	unsigned long val;
>> +	u32 status, num_ctrls;
>> +	struct dw_pcie *pci;
>> +	struct qcom_pcie *pcie;
>> +
>> +	chained_irq_enter(chip, desc);
>> +
>> +	pp = irq_desc_get_handler_data(desc);
>> +	pci = to_dw_pcie_from_pp(pp);
>> +	pcie = to_qcom_pcie(pci);
>> +
>> +	/*
>> +	 * Unlike generic dw_handle_msi_irq we can determine, which group of
>> +	 * MSIs triggered the IRQ, so process just single group.
> 
> Parens and punctuation touch-up:
> 
>    Unlike generic dw_handle_msi_irq(), we can determine which group of
>    MSIs triggered the IRQ, so process just that group.
> 
>> +	 */
>> +	num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL;
>> +
>> +	for (idx = 0; idx < num_ctrls; idx++) {
>> +		if (pcie->msi_irqs[idx] == irq)
>> +			break;
>> +	}
> 
> Since this is basically an enhanced clone of dw_handle_msi_irq(), it
> would be nice to use the same variable names ("i" instead of "idx")
> so it's not gratuitously different.
> 
> Actually, I wonder if you could enhance dw_handle_msi_irq() slightly
> so you could use it directly, e.g.,
> 
>      struct dw_pcie_host_ops {
>        ...
>        void (*msi_host_deinit)(struct pcie_port *pp);
>   +    bool (*msi_in_block)(struct pcie_port *pp, int irq, int i);
>      };
> 
>      dw_handle_msi_irq(...)
>      {
>        ...
> 
>        for (i = 0; i < num_ctrls; i++) {
>   +      if (pp->ops->msi_in_block && !pp->ops->msi_in_block(pp, irq, i))
>   +        continue;
> 
> 	status = dw_pcie_readl_dbi(pci, PCIE_MSI_INTR0_STATUS ...);
> 	...
> 
>   +  bool qcom_pcie_msi_in_block(struct pcie_port *pp, int irq, int i)
>   +  {
>   +    ...
>   +    pci = to_dw_pcie_from_pp(pp);
>   +    pcie = to_qcom_pcie(pci);
>   +
>   +    if (pcie->msi_irqs[i] == irq)
>   +      return true;
>   +
>   +    return false;
>   +  }
> 
> Maybe that's more complicated than it's worth.

I think it will complicate the IRQ handler unnecessary. Just using a 
separate handler seems simpler.

> 
>> +
>> +	if (WARN_ON_ONCE(unlikely(idx == num_ctrls)))
>> +		goto out;
>> +
>> +	status = dw_pcie_readl_dbi(pci, PCIE_MSI_INTR0_STATUS +
>> +				   (idx * MSI_REG_CTRL_BLOCK_SIZE));
>> +	if (!status)
>> +		goto out;
>> +
>> +	val = status;
>> +	pos = 0;
>> +	while ((pos = find_next_bit(&val, MAX_MSI_IRQS_PER_CTRL,
>> +				    pos)) != MAX_MSI_IRQS_PER_CTRL) {
>> +		generic_handle_domain_irq(pp->irq_domain,
>> +					  (idx * MAX_MSI_IRQS_PER_CTRL) +
>> +					  pos);
>> +		pos++;
>> +	}
>> +
>> +out:
>> +	chained_irq_exit(chip, desc);
>> +}