diff mbox series

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

Message ID 20220112094251.1271531-1-sr@denx.de
State New
Headers show
Series [RESEND,v2,1/4] PCI: Add setup_platform_service_irq hook to struct pci_host_bridge | expand

Commit Message

Stefan Roese Jan. 12, 2022, 9:42 a.m. UTC 
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.
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(+)

Comments

Pali Rohár Jan. 12, 2022, 10:23 a.m. UTC | #1 
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.

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.

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*.

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!

>  	void		*release_data;
>  	unsigned int	ignore_reset_delay:1;	/* For entire hierarchy */
>  	unsigned int	no_ext_tags:1;		/* No Extended Tags */
> -- 
> 2.34.1
>
Bjorn Helgaas Jan. 12, 2022, 4:20 p.m. UTC | #2 
On Wed, Jan 12, 2022 at 10:42:48AM +0100, 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.
> 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>

It would be nice if this included a cover letter like Bharat's
original posting did [1].  That makes it easier to keep organized.

The PCIe r6.0 spec just came out, so let's update the spec references
to that.  Conveniently, the section numbers stayed the same as they
were in r4.0.

Update the language here to use the spec terminology, i.e.,
"platform-specific System Errors"  That helps find the related bits,
e.g., "System Error on Correctable Error Enable" in the Root Control
register.

Rewrap this into paragraphs separated by blank lines.

[1] https://lore.kernel.org/all/1542206878-24587-1-git-send-email-bharat.kumar.gogada@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);
>  	void		*release_data;
>  	unsigned int	ignore_reset_delay:1;	/* For entire hierarchy */
>  	unsigned int	no_ext_tags:1;		/* No Extended Tags */
> -- 
> 2.34.1
>
Stefan Roese Jan. 13, 2022, 8:42 a.m. UTC | #3 
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
Stefan Roese Jan. 13, 2022, 8:46 a.m. UTC | #4 
On 1/12/22 17:20, Bjorn Helgaas wrote:
> On Wed, Jan 12, 2022 at 10:42:48AM +0100, 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.
>> 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>
> 
> It would be nice if this included a cover letter like Bharat's
> original posting did [1].  That makes it easier to keep organized.

I wanted to do this, but somehow forgot it when sending the patches.
Will do in the next version for sure.

> The PCIe r6.0 spec just came out, so let's update the spec references
> to that.  Conveniently, the section numbers stayed the same as they
> were in r4.0.
> 
> Update the language here to use the spec terminology, i.e.,
> "platform-specific System Errors"  That helps find the related bits,
> e.g., "System Error on Correctable Error Enable" in the Root Control
> register.
> 
> Rewrap this into paragraphs separated by blank lines.
> 
> [1] https://lore.kernel.org/all/1542206878-24587-1-git-send-email-bharat.kumar.gogada@xilinx.com/

Okay, I'll try to update these descriptions accordingly in v3.

Thanks,
Stefan

>> ---
>>   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);
>>   	void		*release_data;
>>   	unsigned int	ignore_reset_delay:1;	/* For entire hierarchy */
>>   	unsigned int	no_ext_tags:1;		/* No Extended Tags */
>> -- 
>> 2.34.1
>>
diff mbox series

Patch

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);
 	void		*release_data;
 	unsigned int	ignore_reset_delay:1;	/* For entire hierarchy */
 	unsigned int	no_ext_tags:1;		/* No Extended Tags */