Re: [RESEND PATCH v2 1/4] PCI: Add setup_platform_service_irq hook to struct pci_host_bridge

Stefan Roese
2022-01-13T08:42:21Z

On 1/12/22 11:23, Pali Rohár wrote:
> Hello!
>
> On Wednesday 12 January 2022 10:42:48 Stefan Roese wrote:
>> From: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
>>
>> As per section 6.2.4.1.2, 6.2.6 in PCIe r4.0 error interrupts can
>> be delivered with platform specific interrupt lines.
>
> My understanding of these sections is that they still have to be
> deliverable via standard interrupts (INTx / MSI) if root port supports
> standard interrupts:
>
> "If a Root Port or Root Complex Event Collector is enabled for
> level-triggered interrupt signaling using the INTx messages, the virtual
> INTx wire must be asserted..."
>
> "If a Root Port or Root Complex Event Collector is enabled for
> edge-triggered interrupt signaling using MSI or MSI-X, an interrupt
> message must be sent every time..."
>
>> Add setup_platform_service_irq hook to struct pci_host_bridge.
>> Some platforms have dedicated interrupt line from root complex to
>> interrupt controller for PCIe services like AER.
>> This hook is to register platform IRQ's to PCIe port services.
>>
>> Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
>> Signed-off-by: Stefan Roese <sr@denx.de>
>> Tested-by: Stefan Roese <sr@denx.de>
>> Cc: Bjorn Helgaas <helgaas@kernel.org>
>> Cc: Pali Rohár <pali@kernel.org>
>> Cc: Michal Simek <michal.simek@xilinx.com>
>> ---
>> include/linux/pci.h | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 18a75c8e615c..291eadade811 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -554,6 +554,8 @@ struct pci_host_bridge {
>> u8 (*swizzle_irq)(struct pci_dev *, u8 *); /* Platform IRQ swizzler */
>> int (*map_irq)(const struct pci_dev *, u8, u8);
>> void (*release_fn)(struct pci_host_bridge *);
>> + void (*setup_platform_service_irq)(struct pci_host_bridge *, int *,
>> + int);
>
> This callback is used only for root port. So I would suggest to put
> _root port_ into the name of the callback to indicate it. To distinguish
> for which devices is callback designed because other callbacks (e.g.
> map_irq) are used for any device.

I see your point, but I also don't want to make this function name too
long, making the code harder to read IMHO. In the next patchset version
I've changed this name to init_platform_service_irqs (added 's') to
better reflect the input parameters and its related function in
portdrv_core.c, which I now changed according to Bjorn's suggestion to
pcie_init_platform_service_irqs().

Please feel free to suggest a better matching name that is not too long
if one comes to your mind.

> This callback is root port specific and therefore struct pci_dev *
> pointer should be passed as callback argument. Host bridge may have
> multiple root ports, so passing only host bridge is not enough.
>
> Maybe it would be better to pass struct pci_dev * instead of struct
> pci_host_bridge * As from pci_dev can be easily derived host bridge.

Good idea. I'll integrate this in the next version.

> Anyway, this callback looks to be very useful, I would like to use it in
> pci-aardvark.c and pci-mvebu.c drivers for better mapping of PME, AER
> and HP interrupts. And pci-mvebu.c is multi root port driver, so needs
> pci_dev*.

Sounds like a plan.

> And my guess is that this callback can be useful for adding AER support
> also for pcie-uniphier.c driver, as replacement for this (rather ugly)
> patches:
> https://lore.kernel.org/linux-pci/1619111097-10232-1-git-send-email-hayashi.kunihiko@socionext.com/
>
> So I would be happy to see it!

Stay tuned...

Thanks,
Stefan