diff mbox series

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

Message ID 1533915580-31805-5-git-send-email-bharat.kumar.gogada@xilinx.com
State Changes Requested
Delegated to: Bjorn Helgaas
Headers show
Series Add support to register platform service IRQ | expand

Commit Message

Bharat Kumar Gogada Aug. 10, 2018, 3:39 p.m. UTC 
Add nwl_setup_service_irqs hook to setup_platform_service_irq IRQs to
register platform provided IRQ number to kernel AER service.

Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
---
 drivers/pci/controller/pcie-xilinx-nwl.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

Comments

kernel test robot Aug. 13, 2018, 9:09 a.m. UTC | #1 
Hi Bharat,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on pci/next]
[also build test WARNING on v4.18 next-20180810]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Bharat-Kumar-Gogada/Add-support-to-register-platform-service-IRQ/20180813-144216
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/pci/controller/pcie-xilinx-nwl.c:823:5: sparse: symbol 'nwl_setup_service_irqs' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Bharat Kumar Gogada Aug. 14, 2018, 3:55 p.m. UTC | #2 
Agreed, will Fix it in next version.

Regards,
Bharat
> -----Original Message-----
> From: kbuild test robot [mailto:lkp@intel.com]
> Sent: Monday, August 13, 2018 2:39 PM
> To: Bharat Kumar Gogada <bharatku@xilinx.com>
> Cc: kbuild-all@01.org; linux-pci@vger.kernel.org; linux-
> kernel@vger.kernel.org; bhelgaas@google.com; Ravikiran Gummaluri
> <rgummal@xilinx.com>; Bharat Kumar Gogada <bharatku@xilinx.com>
> Subject: Re: [PATCH 4/4] PCI: xilinx-nwl: Add method to
> setup_platform_service_irq hook
> 
> Hi Bharat,
> 
> Thank you for the patch! Perhaps something to improve:
> 
> [auto build test WARNING on pci/next]
> [also build test WARNING on v4.18 next-20180810] [if your patch is applied to
> the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Bharat-Kumar-
> Gogada/Add-support-to-register-platform-service-IRQ/20180813-144216
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
> reproduce:
>         # apt-get install sparse
>         make ARCH=x86_64 allmodconfig
>         make C=1 CF=-D__CHECK_ENDIAN__
> 
> 
> sparse warnings: (new ones prefixed by >>)
> 
> >> drivers/pci/controller/pcie-xilinx-nwl.c:823:5: sparse: symbol
> 'nwl_setup_service_irqs' was not declared. Should it be static?
> 
> Please review and possibly fold the followup patch.
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Bjorn Helgaas Sept. 4, 2018, 1:48 p.m. UTC | #3 
On Fri, Aug 10, 2018 at 09:09:40PM +0530, Bharat Kumar Gogada wrote:
> Add nwl_setup_service_irqs hook to setup_platform_service_irq IRQs to
> register platform provided IRQ number to kernel AER service.
> 
> Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
> ---
>  drivers/pci/controller/pcie-xilinx-nwl.c |   16 ++++++++++++++++
>  1 files changed, 16 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-xilinx-nwl.c b/drivers/pci/controller/pcie-xilinx-nwl.c
> index fb32840..285647b 100644
> --- a/drivers/pci/controller/pcie-xilinx-nwl.c
> +++ b/drivers/pci/controller/pcie-xilinx-nwl.c
> @@ -22,6 +22,7 @@
>  #include <linux/irqchip/chained_irq.h>
>  
>  #include "../pci.h"
> +#include "../pcie/portdrv.h"
>  
>  /* Bridge core config registers */
>  #define BRCFG_PCIE_RX0			0x00000000
> @@ -819,6 +820,20 @@ static int nwl_pcie_parse_dt(struct nwl_pcie *pcie,
>  	return 0;
>  }
>  
> +int 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;
> +		plat_mask &= ~(1 << PCIE_PORT_SERVICE_AER_SHIFT);
> +	}

If I understand correctly, this ultimately results in pcie->irq_misc
being hooked up to aer_irq() via the aer_probe() path.  We already
have pcie->irq_misc being hooked up to nwl_pcie_misc_handler() via
nwl_pcie_bridge_init().

We can't rely on the ordering of the two handlers.  Is it safe if
nwl_pcie_misc_handler() runs first, followed by aer_irq()?  It looks
like nwl_pcie_misc_handler() might log messages and clear AER-related
errors.  If that's the case aer_irq() might not find anything to do.

> +
> +	return plat_mask;
> +}
> +
>  static const struct of_device_id nwl_pcie_of_match[] = {
>  	{ .compatible = "xlnx,nwl-pcie-2.11", },
>  	{}
> @@ -880,6 +895,7 @@ static int nwl_pcie_probe(struct platform_device *pdev)
>  	bridge->ops = &nwl_pcie_ops;
>  	bridge->map_irq = of_irq_parse_and_map_pci;
>  	bridge->swizzle_irq = pci_common_swizzle;
> +	bridge->setup_platform_service_irq = nwl_setup_service_irqs;
>  
>  	if (IS_ENABLED(CONFIG_PCI_MSI)) {
>  		err = nwl_pcie_enable_msi(pcie);
> -- 
> 1.7.1
>
Bharat Kumar Gogada Sept. 6, 2018, 3:27 p.m. UTC | #4 
> Subject: Re: [PATCH 4/4] PCI: xilinx-nwl: Add method to
> setup_platform_service_irq hook
> 
> On Fri, Aug 10, 2018 at 09:09:40PM +0530, Bharat Kumar Gogada wrote:
> > Add nwl_setup_service_irqs hook to setup_platform_service_irq IRQs to
> > register platform provided IRQ number to kernel AER service.
> >
> > Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
> > ---
> >  drivers/pci/controller/pcie-xilinx-nwl.c |   16 ++++++++++++++++
> >  1 files changed, 16 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-xilinx-nwl.c
> > b/drivers/pci/controller/pcie-xilinx-nwl.c
> > index fb32840..285647b 100644
> > --- a/drivers/pci/controller/pcie-xilinx-nwl.c
> > +++ b/drivers/pci/controller/pcie-xilinx-nwl.c
> > @@ -22,6 +22,7 @@
> >  #include <linux/irqchip/chained_irq.h>
> >
> >  #include "../pci.h"
> > +#include "../pcie/portdrv.h"
> >
> >  /* Bridge core config registers */
> >  #define BRCFG_PCIE_RX0			0x00000000
> > @@ -819,6 +820,20 @@ static int nwl_pcie_parse_dt(struct nwl_pcie *pcie,
> >  	return 0;
> >  }
> >
> > +int 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;
> > +		plat_mask &= ~(1 << PCIE_PORT_SERVICE_AER_SHIFT);
> > +	}
> 
> If I understand correctly, this ultimately results in pcie->irq_misc being
> hooked up to aer_irq() via the aer_probe() path.  We already have pcie-
> >irq_misc being hooked up to nwl_pcie_misc_handler() via
> nwl_pcie_bridge_init().
> 
> We can't rely on the ordering of the two handlers.  Is it safe if
> nwl_pcie_misc_handler() runs first, followed by aer_irq()?  It looks like
> nwl_pcie_misc_handler() might log messages and clear AER-related errors.  If
> that's the case aer_irq() might not find anything to do.
> 
Hi Bjorn, the nwl_pcie_misc_handler() prints all pcie errors along with AER and then clears 
controller register (MSGF_MISC_STATUS) in which these status bits are set. 
But clearing this controller register will not effect any bits in AER capabilities.
So in our case we need  to clear controller register and also handle AER as per spec.
This will not cause any ordering issue as both paths are accessing different set of registers.

Thanks,
Bharat
diff mbox series

Patch

diff --git a/drivers/pci/controller/pcie-xilinx-nwl.c b/drivers/pci/controller/pcie-xilinx-nwl.c
index fb32840..285647b 100644
--- a/drivers/pci/controller/pcie-xilinx-nwl.c
+++ b/drivers/pci/controller/pcie-xilinx-nwl.c
@@ -22,6 +22,7 @@ 
 #include <linux/irqchip/chained_irq.h>
 
 #include "../pci.h"
+#include "../pcie/portdrv.h"
 
 /* Bridge core config registers */
 #define BRCFG_PCIE_RX0			0x00000000
@@ -819,6 +820,20 @@  static int nwl_pcie_parse_dt(struct nwl_pcie *pcie,
 	return 0;
 }
 
+int 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;
+		plat_mask &= ~(1 << PCIE_PORT_SERVICE_AER_SHIFT);
+	}
+
+	return plat_mask;
+}
+
 static const struct of_device_id nwl_pcie_of_match[] = {
 	{ .compatible = "xlnx,nwl-pcie-2.11", },
 	{}
@@ -880,6 +895,7 @@  static int nwl_pcie_probe(struct platform_device *pdev)
 	bridge->ops = &nwl_pcie_ops;
 	bridge->map_irq = of_irq_parse_and_map_pci;
 	bridge->swizzle_irq = pci_common_swizzle;
+	bridge->setup_platform_service_irq = nwl_setup_service_irqs;
 
 	if (IS_ENABLED(CONFIG_PCI_MSI)) {
 		err = nwl_pcie_enable_msi(pcie);