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

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

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);
>> +}


-- 
With best wishes
Dmitry