diff mbox series

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

Message ID 20220112094251.1271531-3-sr@denx.de (mailing list archive)
State Changes Requested
Delegated to: Bjorn Helgaas
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>

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

Comments

Pali Rohár Jan. 12, 2022, 10:34 a.m. UTC | #1 
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
>
Bjorn Helgaas Jan. 12, 2022, 4:36 p.m. UTC | #2 
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
Stefan Roese Jan. 13, 2022, 9:04 a.m. UTC | #3 
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
diff mbox series

Patch

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;