On Wed, Jul 24, 2024 at 11:23:04AM -0500, Bjorn Helgaas wrote:
Subject should say something about why this change is needed, not just translate the C code to English.
My intent was to summarize the change to make it easy for anyone to find out what's being done. The commit message below explains in detail as to why they are set to NULL. As an alternative, I could change the $subject to: PCI: j721e: Disable INTx mapping and swizzling where the "Disable" is equivalent to NULL. Kindly let me know if this is acceptable or needs to be improved further.
On Wed, Jul 24, 2024 at 12:20:48PM +0530, Siddharth Vadapalli wrote:
Since the configuration of Legacy Interrupts (INTx) is not supported, set
I assume you mean J721E doesn't support INTx?
Yes, the driver support doesn't exist and the device-tree also doesn't have the required nodes. Kishon had posted a series to enable it for J721E, J7200 and AM64 on the 4th of August 2021: https://lore.kernel.org/r/20210804132912.30685-1-kishon@ti.com/ and based on the discussion on the series, an Errata was discovered for J721E: https://www.ti.com/lit/er/sprz455d/sprz455d.pdf i2094: PCIe: End of Interrupt (EOI) Not Enabled for PCIe Legacy Interrupts
Thus, there is no support for Legacy Interrupts (INTx) in the pci-j721e.c driver and as a consequence, no device-tree nodes either.
the .map_irq and .swizzle_irq callbacks to NULL. This fixes the error: of_irq_parse_pci: failed with rc=-22
I guess this happens because devm_of_pci_bridge_init() initializes .map_irq and .swizzle_irq unconditionally?
Yes. The PCIe Controller on J721E and other SoCs is the Cadence PCIe Controller. In the commit which added driver support for the Cadence PCIe Controller's Host functionality: https://github.com/torvalds/linux/commit/1b79c5284439 'map_irq' and 'swizzle_irq' were set to the same functions: 'of_irq_parse_and_map_pci()' and 'pci_common_swizzle()' respectively. Commit: https://github.com/torvalds/linux/commit/b64aa11eb2dd extracted out the shared defaults from multiple Host Controller drivers and moved them into the common 'devm_of_pci_bridge_init()' function.
While 'map_irq' and 'swizzle_irq' are unconditionally set to the defaults in 'devm_of_pci_bridge_init()', those Host Controller driver which haven't been using the shared defaults do have the choice of overriding them in the driver (which is already being done).
Similarly, for pci-j721e.c, since the shared defaults inherited from the Cadence PCIe Controller are not applicable (due to the Errata), the pci-j721e.c driver should override them to NULL as done in this patch.
I'm not sure the default assumption should be that a host bridge supports INTx.
Maybe .map_irq and .swizzle_irq should only be set when we discover some information about INTx routing, e.g., an ACPI _PRT or the corresponding DT properties.
I believe that the defaults were set since most of the Host drivers were using them in their respective drivers as indicated in the commit: https://github.com/torvalds/linux/commit/b64aa11eb2dd
[...]
drivers/pci/controller/cadence/pci-j721e.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/pci/controller/cadence/pci-j721e.c b/drivers/pci/controller/cadence/pci-j721e.c index 85718246016b..5372218849a8 100644 --- a/drivers/pci/controller/cadence/pci-j721e.c +++ b/drivers/pci/controller/cadence/pci-j721e.c @@ -417,6 +417,10 @@ static int j721e_pcie_probe(struct platform_device *pdev) if (!bridge) return -ENOMEM;
/* Legacy interrupts are not supported */Say "INTx" explicitly here instead of assuming "legacy" == "INTx".
Sure. I will update it in the v2 patch.
Thank you for reviewing this patch and sharing your feedback.
Regards, Siddharth.