[PATCH v7] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller

[Marc Zyngier]

On 04/11/15 06:38, Bharat Kumar Gogada wrote:
>>> +static struct msi_domain_info nwl_msi_domain_info = {
>>> +	.flags = (MSI_FLAG_USE_DEF_DOM_OPS |
>> MSI_FLAG_USE_DEF_CHIP_OPS |
>>> +		  MSI_FLAG_MULTI_PCI_MSI),
>>
>> If you're supporting multi-MSI, how do you ensure that all hwirqs are
>> contiguous as required by the spec? Clearly, your allocator doesn't provide
>> this guarantee.
>>
> Oh ok, then how can we know if one EP is requesting for multiple irq's, because in pci_enable_msi_range,
>  it does do while loop according to msi_capability_init return value 
> which in turn requests for multiple irq's. Is there any way to find out when single EP requesting for multiple MSI?

Please read what I've written: hwirqs *must* be contiguous for
multi-MSI. If the device requests 4 MSIs, they *must* be in the [x,x+3]
range. Your allocator only allocates one interrupt at time (ignoring the
nr_irqs parameter).

You shouldn't try to find what the device does, that's irrelevant at
that level.

>> Also, you still lack support for MSI-X (which would come for free...).
> 
> We don't support MSI-X in root port mode.

I don't believe you. If you support single MSI, you support MSI-X
(because that's mostly a property of the endpoint).

>>> +	.chip = &nwl_msi_irq_chip,
>>> +};
>>> +#endif
>>> +
>>> +	irq_domain_remove(pcie->legacy_irq_domain);
>>> +
>>> +#ifdef CONFIG_PCI_MSI
>>> +	irq_set_chained_handler_and_data(msi->irq_msi0, NULL, NULL);
>>> +	irq_set_chained_handler_and_data(msi->irq_msi1, NULL, NULL);
>>> +
>>> +	irq_domain_remove(msi->msi_domain);
>>> +	irq_domain_remove(msi->dev_domain);
>>> +#endif
>>> +
>>> +}
>>
>> Remove these #ifdefs. You can test the validity of these fields before calling
>> the various functions. You can also move this to a separate function.
> 
> Without #ifdefs if we compile driver for legacy, MSI structures will not be available and we get compile time error.

Legacy? Legacy interrupts? The msi structure is still there, you've
embedded it in your pcie structure. Let me spell it out for you:

static void nwl_msi_free_domain(struct nwl_pcie *pcie)
{
	struct nwl_msi *msi = &pcie->msi;

	if (msi->irq_msi0)
		irq_set_chained_handler_and_data(msi->irq_msi0, NULL, NULL);
	if (msi->irq_msi1)
		irq_set_chained_handler_and_data(msi->irq_msi1, NULL, NULL);

	if (msi->msi_domain)
		irq_domain_remove(msi->msi_domain);
	if (msi->dev_domain)
		irq_domain_remove(msi->dev_domain);
}

static void nwl_pcie_free_irq_domain(struct nwl_pcie *pcie)
{
	int i;
	u32 irq;

	for (i = 0; i < 4; i++) {
		irq = irq_find_mapping(pcie->legacy_irq_domain, i + 1);
		if (irq > 0)
			irq_dispose_mapping(irq);
	}

	irq_domain_remove(pcie->legacy_irq_domain);

	nwl_msi_free_domain(pcie);
}

>>> +
>>> +	/* setup AFI/FPCI range */
>>> +	msi->pages = __get_free_pages(GFP_KERNEL, 0);
>>> +	base = virt_to_phys((void *)msi->pages);
>>> +	nwl_bridge_writel(pcie, lower_32_bits(base), I_MSII_BASE_LO);
>>> +	nwl_bridge_writel(pcie, upper_32_bits(base), I_MSII_BASE_HI);
>>
>> I just read this, and I'm puzzled. Actually, puzzled is an understatement. Why
>> on Earth do you need to give RAM to your MSI HW?
>> This should be a device, not RAM. By the look of it, this could be absolutely
>> anything. Are you sure you have to supply RAM here?
>>
> This is required in our hardware, so that bridge identifies incoming MWr as MSI.

I'm asking why this has to be RAM. What is the actual requirement?

	M.
-- 
Jazz is not dead. It just smells funny...