diff mbox series

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

Message ID 20220112094251.1271531-2-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>

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

Comments

Bjorn Helgaas Jan. 12, 2022, 4:42 p.m. UTC | #1
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
Stefan Roese Jan. 13, 2022, 9:08 a.m. UTC | #2
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
diff mbox series

Patch

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