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