Re: [PATCH v11 3/7] PCI: dwc: Handle MSIs routed to multiple GIC interrupts

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

On 23/05/2022 10:53, Johan Hovold wrote:
> On Fri, May 20, 2022 at 09:31:10PM +0300, Dmitry Baryshkov wrote:
>> On some of Qualcomm platforms each group of 32 MSI vectors is routed to the
>> separate GIC interrupt. Implement support for such configurations by
>> parsing "msi0" ... "msiN" interrupts and attaching them to the chained
>> handler.
>>
>> Note, that if DT doesn't list an array of MSI interrupts and uses single
>> "msi" IRQ, the driver will limit the amount of supported MSI vectors
>> accordingly (to 32).
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   .../pci/controller/dwc/pcie-designware-host.c | 58 +++++++++++++++++--
>>   1 file changed, 54 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>> index a076abe6611c..381bc24d5715 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>> @@ -288,6 +288,43 @@ static void dw_pcie_msi_init(struct pcie_port *pp)
>>   	dw_pcie_writel_dbi(pci, PCIE_MSI_ADDR_HI, upper_32_bits(msi_target));
>>   }
>>   
>> +static const char * const split_msi_names[] = {
>> +	"msi0", "msi1", "msi2", "msi3",
>> +	"msi4", "msi5", "msi6", "msi7",
>> +};
>> +
>> +static int dw_pcie_parse_split_msi_irq(struct pcie_port *pp)
>> +{
>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> +	struct device *dev = pci->dev;
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	int irq;
>> +	u32 ctrl;
>> +
>> +	irq = platform_get_irq_byname_optional(pdev, split_msi_names[0]);
>> +	if (irq == -ENXIO)
>> +		return -ENXIO;
> 
> You still need to check for other errors and -EPROBE_DEFER here.

I think even the if (irq < 0) return irq; will work here.

> 
>> +
>> +	pp->msi_irq[0] = irq;
>> +
>> +	/* Parse as many IRQs as described in the DTS. */
> 
> s/DTS/devicetree/
> 
>> +	for (ctrl = 1; ctrl < MAX_MSI_CTRLS; ctrl++) {
>> +		irq = platform_get_irq_byname_optional(pdev, split_msi_names[ctrl]);
>> +		if (irq == -ENXIO)
>> +			break;
>> +		if (irq < 0)
>> +			return dev_err_probe(dev, irq,
>> +					     "Failed to parse MSI IRQ '%s'\n",
>> +					     split_msi_names[ctrl]);
>> +
>> +		pp->msi_irq[ctrl] = irq;
>> +	}
>> +
>> +	pp->num_vectors = ctrl * MAX_MSI_IRQS_PER_CTRL;
>> +
>> +	return 0;
>> +}
>> +
>>   static int dw_pcie_msi_host_init(struct pcie_port *pp)
>>   {
>>   	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> @@ -295,22 +332,34 @@ static int dw_pcie_msi_host_init(struct pcie_port *pp)
>>   	struct platform_device *pdev = to_platform_device(dev);
>>   	int ret;
>>   	u32 ctrl, num_ctrls;
>> +	bool has_split_msi_irq = false;
> 
> This one should go in the follow-on patch that starts using it.
> 
>> -	num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL;
>> -	for (ctrl = 0; ctrl < num_ctrls; ctrl++)
>> +	for (ctrl = 0; ctrl < MAX_MSI_CTRLS; ctrl++)
>>   		pp->irq_mask[ctrl] = ~0;
>>   
>> +	if (!pp->msi_irq[0]) {
>> +		ret = dw_pcie_parse_split_msi_irq(pp);
>> +		if (ret < 0 && ret != -ENXIO)
>> +			return ret;
>> +	}
>> +
>> +	if (!pp->num_vectors)
>> +		pp->num_vectors = MSI_DEF_NUM_VECTORS;
> 
> This works, but now you override num_vectors unconditionally when using
> split msis (and not just when num_vectors is set to zero) >
> Is it work allowing to use num_vectors as a maximum as in previous
> versions (if only for consistency)?

Let me take a look.

> 
>> +	num_ctrls = pp->num_vectors / MAX_MSI_IRQS_PER_CTRL;
>> +
>>   	if (!pp->msi_irq[0]) {
>>   		int irq = platform_get_irq_byname_optional(pdev, "msi");
>>   
>>   		if (irq < 0) {
>>   			irq = platform_get_irq(pdev, 0);
>>   			if (irq < 0)
>> -				return irq;
>> +				return dev_err_probe(dev, irq, "Failed to parse MSI irq\n");
>>   		}
>>   		pp->msi_irq[0] = irq;
>>   	}
>>   
>> +	dev_dbg(dev, "Using %d MSI vectors\n", pp->num_vectors);
>> +
>>   	pp->msi_irq_chip = &dw_pci_msi_bottom_irq_chip;
>>   
>>   	ret = dw_pcie_allocate_domains(pp);
>> @@ -407,7 +456,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
>>   				     of_property_read_bool(np, "msi-parent") ||
>>   				     of_property_read_bool(np, "msi-map"));
>>   
>> -		if (!pp->num_vectors) {
>> +		/* for the has_msi_ctrl the default assignment is handled inside dw_pcie_msi_host_init() */
>> +		if (!pp->has_msi_ctrl && !pp->num_vectors) {
>>   			pp->num_vectors = MSI_DEF_NUM_VECTORS;
>>   		} else if (pp->num_vectors > MAX_MSI_IRQS) {
>>   			dev_err(dev, "Invalid number of vectors\n");
> 
> Johan


-- 
With best wishes
Dmitry