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

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

[Stefan Roese]

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(+)

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

[RESEND PATCH v2 2/4] PCI: Add pci_check_platform_service_irqs

[Stefan Roese]

From: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>

Adding method pci_check_platform_service_irqs to check if platform
has registered method to proivde dedicated IRQ lines for PCIe services
like AER.

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 | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 291eadade811..d6812d596ecc 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2420,6 +2420,24 @@ static inline bool pci_ari_enabled(struct pci_bus *bus)
 	return bus->self && bus->self->ari_enabled;
 }
 
+/**
+ * pci_check_platform_service_irqs - check platform service irq's
+ * @pdev: PCI Express device to check
+ * @irqs: Array of irqs to populate
+ * @mask: Bitmask of capabilities
+ */
+static inline void pci_check_platform_service_irqs(struct pci_dev *dev,
+						   int *irqs, int mask)
+{
+	struct pci_host_bridge *bridge;
+
+	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
+		bridge = pci_find_host_bridge(dev->bus);
+		if (bridge && bridge->setup_platform_service_irq)
+			bridge->setup_platform_service_irq(bridge, irqs, mask);
+	}
+}
+
 /**
  * pci_is_thunderbolt_attached - whether device is on a Thunderbolt daisy chain
  * @pdev: PCI device to check
-- 
2.34.1

[RESEND PATCH v2 3/4] PCI/portdrv: Check platform supported service IRQ's

[Stefan Roese]

From: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>

Platforms may have dedicated IRQ lines for PCIe services like
AER/PME etc., check for such IRQ lines.
Check if platform has any dedicated IRQ lines for PCIe
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>
---
 drivers/pci/pcie/portdrv_core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index bda630889f95..70dd45671ed8 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -358,6 +358,14 @@ int pcie_port_device_register(struct pci_dev *dev)
 		}
 	}
 
+	/*
+	 * Some platforms have dedicated interrupt line from root complex to
+	 * interrupt controller for PCIe services like AER/PME etc., check
+	 * if platform registered with any such IRQ.
+	 */
+	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
+		pci_check_platform_service_irqs(dev, irqs, capabilities);
+
 	/* Allocate child services if any */
 	status = -ENODEV;
 	nr_service = 0;
-- 
2.34.1

[RESEND PATCH v2 4/4] PCI: xilinx-nwl: Add method to setup_platform_service_irq hook

[Stefan Roese]

From: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>

Add nwl_setup_service_irqs hook to setup_platform_service_irq to
register platform provided IRQ number to kernel AER service.

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>
---
 drivers/pci/controller/pcie-xilinx-nwl.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/pci/controller/pcie-xilinx-nwl.c b/drivers/pci/controller/pcie-xilinx-nwl.c
index 414b679175b3..4969f35db7e5 100644
--- a/drivers/pci/controller/pcie-xilinx-nwl.c
+++ b/drivers/pci/controller/pcie-xilinx-nwl.c
@@ -24,6 +24,7 @@
 #include <linux/irqchip/chained_irq.h>
 
 #include "../pci.h"
+#include "../pcie/portdrv.h"
 
 /* Bridge core config registers */
 #define BRCFG_PCIE_RX0			0x00000000
@@ -806,6 +807,16 @@ static int nwl_pcie_parse_dt(struct nwl_pcie *pcie,
 	return 0;
 }
 
+static void nwl_setup_service_irqs(struct pci_host_bridge *bridge, int *irqs,
+				   int plat_mask)
+{
+	struct nwl_pcie *pcie;
+
+	pcie = pci_host_bridge_priv(bridge);
+	if (plat_mask & PCIE_PORT_SERVICE_AER)
+		irqs[PCIE_PORT_SERVICE_AER_SHIFT] = pcie->irq_misc;
+}
+
 static const struct of_device_id nwl_pcie_of_match[] = {
 	{ .compatible = "xlnx,nwl-pcie-2.11", },
 	{}
@@ -857,6 +868,7 @@ static int nwl_pcie_probe(struct platform_device *pdev)
 
 	bridge->sysdata = pcie;
 	bridge->ops = &nwl_pcie_ops;
+	bridge->setup_platform_service_irq = nwl_setup_service_irqs;
 
 	if (IS_ENABLED(CONFIG_PCI_MSI)) {
 		err = nwl_pcie_enable_msi(pcie);
-- 
2.34.1

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

[Pali Rohár]

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
> 

Re: [RESEND PATCH v2 3/4] PCI/portdrv: Check platform supported service IRQ's

[Pali Rohár]

On Wednesday 12 January 2022 10:42:50 Stefan Roese wrote:
> From: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
> 
> Platforms may have dedicated IRQ lines for PCIe services like
> AER/PME etc., check for such IRQ lines.
> Check if platform has any dedicated IRQ lines for PCIe
> 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>
> ---
>  drivers/pci/pcie/portdrv_core.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index bda630889f95..70dd45671ed8 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -358,6 +358,14 @@ int pcie_port_device_register(struct pci_dev *dev)
>  		}
>  	}
>  
> +	/*
> +	 * Some platforms have dedicated interrupt line from root complex to
> +	 * interrupt controller for PCIe services like AER/PME etc., check
> +	 * if platform registered with any such IRQ.
> +	 */
> +	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
> +		pci_check_platform_service_irqs(dev, irqs, capabilities);
> +

In my opinion calling this hook at this stage is too late. Few lines
above is following code:

	if (irq_services) {
		/*
		 * Initialize service IRQs. Don't use service devices that
		 * require interrupts if there is no way to generate them.
		 * However, some drivers may have a polling mode (e.g.
		 * pciehp_poll_mode) that can be used in the absence of IRQs.
		 * Allow them to determine if that is to be used.
		 */
		status = pcie_init_service_irqs(dev, irqs, irq_services);
		if (status) {
			irq_services &= PCIE_PORT_SERVICE_HP;
			if (!irq_services)
				goto error_disable;
		}
	}

Function pcie_init_service_irqs() fails if "dev" does not have any
suitable interrupt. Which happens for devices / root ports without
support for standard interrupts (INTx / MSI).

I think that this new hook should have preference over
pcie_init_service_irqs() and after this new hook should be
pcie_init_service_irqs() called only for irq_services which new hook has
not filled. And if at least new hook or pcie_init_service_irqs() passes
then "error_disable" path should not be called.

>  	/* Allocate child services if any */
>  	status = -ENODEV;
>  	nr_service = 0;
> -- 
> 2.34.1
> 

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

[Bjorn Helgaas]

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
> 

Re: [RESEND PATCH v2 3/4] PCI/portdrv: Check platform supported service IRQ's

[Bjorn Helgaas]

On Wed, Jan 12, 2022 at 11:34:02AM +0100, Pali Rohár wrote:
> On Wednesday 12 January 2022 10:42:50 Stefan Roese wrote:
> > From: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
> > 
> > Platforms may have dedicated IRQ lines for PCIe services like
> > AER/PME etc., check for such IRQ lines.
> > Check if platform has any dedicated IRQ lines for PCIe
> > services.

Use the terminology from the spec again ("platform-specific System
Errors").

> > 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>
> > ---
> >  drivers/pci/pcie/portdrv_core.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> > index bda630889f95..70dd45671ed8 100644
> > --- a/drivers/pci/pcie/portdrv_core.c
> > +++ b/drivers/pci/pcie/portdrv_core.c
> > @@ -358,6 +358,14 @@ int pcie_port_device_register(struct pci_dev *dev)
> >  		}
> >  	}
> >  
> > +	/*
> > +	 * Some platforms have dedicated interrupt line from root complex to
> > +	 * interrupt controller for PCIe services like AER/PME etc., check
> > +	 * if platform registered with any such IRQ.
> > +	 */
> > +	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
> > +		pci_check_platform_service_irqs(dev, irqs, capabilities);
> > +
> 
> In my opinion calling this hook at this stage is too late. Few lines
> above is following code:
> 
> 	if (irq_services) {
> 		/*
> 		 * Initialize service IRQs. Don't use service devices that
> 		 * require interrupts if there is no way to generate them.
> 		 * However, some drivers may have a polling mode (e.g.
> 		 * pciehp_poll_mode) that can be used in the absence of IRQs.
> 		 * Allow them to determine if that is to be used.
> 		 */
> 		status = pcie_init_service_irqs(dev, irqs, irq_services);
> 		if (status) {
> 			irq_services &= PCIE_PORT_SERVICE_HP;
> 			if (!irq_services)
> 				goto error_disable;
> 		}
> 	}
> 
> Function pcie_init_service_irqs() fails if "dev" does not have any
> suitable interrupt. Which happens for devices / root ports without
> support for standard interrupts (INTx / MSI).
> 
> I think that this new hook should have preference over
> pcie_init_service_irqs() and after this new hook should be
> pcie_init_service_irqs() called only for irq_services which new hook has
> not filled. And if at least new hook or pcie_init_service_irqs() passes
> then "error_disable" path should not be called.

I tend to agree.  I expect that a host bridge will only use this new
mechanism if the standard INTx/MSI interrupts don't work.  

I guess it's possible a device could use platform-specific errors for
some services and standard INTx/MSI for others, but unless we have an
example of that, I'm not sure it's worth trying to support that.

For now, I think it will be simpler to skip pcie_init_service_irqs()
completely if the platform-specific hook is successful.

Bjorn

Re: [RESEND PATCH v2 2/4] PCI: Add pci_check_platform_service_irqs

[Bjorn Helgaas]

On Wed, Jan 12, 2022 at 10:42:49AM +0100, Stefan Roese wrote:
> From: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
> 
> Adding method pci_check_platform_service_irqs to check if platform
> has registered method to proivde dedicated IRQ lines for PCIe services
> like AER.
> 
> 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 | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 291eadade811..d6812d596ecc 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -2420,6 +2420,24 @@ static inline bool pci_ari_enabled(struct pci_bus *bus)
>  	return bus->self && bus->self->ari_enabled;
>  }
>  
> +/**
> + * pci_check_platform_service_irqs - check platform service irq's
> + * @pdev: PCI Express device to check
> + * @irqs: Array of irqs to populate
> + * @mask: Bitmask of capabilities
> + */
> +static inline void pci_check_platform_service_irqs(struct pci_dev *dev,
> +						   int *irqs, int mask)
> +{
> +	struct pci_host_bridge *bridge;
> +
> +	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
> +		bridge = pci_find_host_bridge(dev->bus);
> +		if (bridge && bridge->setup_platform_service_irq)
> +			bridge->setup_platform_service_irq(bridge, irqs, mask);
> +	}
> +}

I don't think this needs to be in include/linux/pci.h; I think it
should be in drivers/pci/pcie/portdrv_core.c where it is called.

The name and signature should be parallel to pcie_init_service_irqs()
since it is basically doing the same thing for platform-specific
interrupts.

These patches are split up a little bit *too* much.  Each one should
add some piece of functionality.  Currently they add declarations that
are not used, functions that are not called, etc.  That makes it hard
to read one patch and get any idea of what it's for.

Bjorn

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

[Stefan Roese]

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

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

[Stefan Roese]

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

Re: [RESEND PATCH v2 3/4] PCI/portdrv: Check platform supported service IRQ's

[Stefan Roese]

On 1/12/22 17:36, Bjorn Helgaas wrote:
> On Wed, Jan 12, 2022 at 11:34:02AM +0100, Pali Rohár wrote:
>> On Wednesday 12 January 2022 10:42:50 Stefan Roese wrote:
>>> From: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
>>>
>>> Platforms may have dedicated IRQ lines for PCIe services like
>>> AER/PME etc., check for such IRQ lines.
>>> Check if platform has any dedicated IRQ lines for PCIe
>>> services.
> 
> Use the terminology from the spec again ("platform-specific System
> Errors").

Ok.

>>> 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>
>>> ---
>>>   drivers/pci/pcie/portdrv_core.c | 8 ++++++++
>>>   1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
>>> index bda630889f95..70dd45671ed8 100644
>>> --- a/drivers/pci/pcie/portdrv_core.c
>>> +++ b/drivers/pci/pcie/portdrv_core.c
>>> @@ -358,6 +358,14 @@ int pcie_port_device_register(struct pci_dev *dev)
>>>   		}
>>>   	}
>>>   
>>> +	/*
>>> +	 * Some platforms have dedicated interrupt line from root complex to
>>> +	 * interrupt controller for PCIe services like AER/PME etc., check
>>> +	 * if platform registered with any such IRQ.
>>> +	 */
>>> +	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
>>> +		pci_check_platform_service_irqs(dev, irqs, capabilities);
>>> +
>>
>> In my opinion calling this hook at this stage is too late. Few lines
>> above is following code:
>>
>> 	if (irq_services) {
>> 		/*
>> 		 * Initialize service IRQs. Don't use service devices that
>> 		 * require interrupts if there is no way to generate them.
>> 		 * However, some drivers may have a polling mode (e.g.
>> 		 * pciehp_poll_mode) that can be used in the absence of IRQs.
>> 		 * Allow them to determine if that is to be used.
>> 		 */
>> 		status = pcie_init_service_irqs(dev, irqs, irq_services);
>> 		if (status) {
>> 			irq_services &= PCIE_PORT_SERVICE_HP;
>> 			if (!irq_services)
>> 				goto error_disable;
>> 		}
>> 	}
>>
>> Function pcie_init_service_irqs() fails if "dev" does not have any
>> suitable interrupt. Which happens for devices / root ports without
>> support for standard interrupts (INTx / MSI).
>>
>> I think that this new hook should have preference over
>> pcie_init_service_irqs() and after this new hook should be
>> pcie_init_service_irqs() called only for irq_services which new hook has
>> not filled. And if at least new hook or pcie_init_service_irqs() passes
>> then "error_disable" path should not be called.
> 
> I tend to agree.  I expect that a host bridge will only use this new
> mechanism if the standard INTx/MSI interrupts don't work.
> 
> I guess it's possible a device could use platform-specific errors for
> some services and standard INTx/MSI for others, but unless we have an
> example of that, I'm not sure it's worth trying to support that.
> 
> For now, I think it will be simpler to skip pcie_init_service_irqs()
> completely if the platform-specific hook is successful.

Understood. I'll make the necessary changes in v3.

Thanks,
Stefan

Re: [RESEND PATCH v2 2/4] PCI: Add pci_check_platform_service_irqs

[Stefan Roese]

On 1/12/22 17:42, Bjorn Helgaas wrote:
> On Wed, Jan 12, 2022 at 10:42:49AM +0100, Stefan Roese wrote:
>> From: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
>>
>> Adding method pci_check_platform_service_irqs to check if platform
>> has registered method to proivde dedicated IRQ lines for PCIe services
>> like AER.
>>
>> 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 | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 291eadade811..d6812d596ecc 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -2420,6 +2420,24 @@ static inline bool pci_ari_enabled(struct pci_bus *bus)
>>   	return bus->self && bus->self->ari_enabled;
>>   }
>>   
>> +/**
>> + * pci_check_platform_service_irqs - check platform service irq's
>> + * @pdev: PCI Express device to check
>> + * @irqs: Array of irqs to populate
>> + * @mask: Bitmask of capabilities
>> + */
>> +static inline void pci_check_platform_service_irqs(struct pci_dev *dev,
>> +						   int *irqs, int mask)
>> +{
>> +	struct pci_host_bridge *bridge;
>> +
>> +	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
>> +		bridge = pci_find_host_bridge(dev->bus);
>> +		if (bridge && bridge->setup_platform_service_irq)
>> +			bridge->setup_platform_service_irq(bridge, irqs, mask);
>> +	}
>> +}
> 
> I don't think this needs to be in include/linux/pci.h; I think it
> should be in drivers/pci/pcie/portdrv_core.c where it is called.

Agreed.

> The name and signature should be parallel to pcie_init_service_irqs()
> since it is basically doing the same thing for platform-specific
> interrupts.

I've changed it now to pcie_init_platform_service_irqs(). Just let me
know if you had some other naming in mind.

> These patches are split up a little bit *too* much.  Each one should
> add some piece of functionality.  Currently they add declarations that
> are not used, functions that are not called, etc.  That makes it hard
> to read one patch and get any idea of what it's for.

Agreed. Let me see, how this is best done. Most likely 2 patches, one
adding the new infrastructure to the PCIe subsystem and one adding the
"user" to the Xilinx PCIe controller driver.

Thanks,
Stefan