RE: [PATCH 4/4] PCI: xilinx-nwl: Add method to setup_platform_service_irq hook
> 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
> > ---
> > 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
> >
> > #include "../pci.h"
> > +#include "../pcie/portdrv.h"
> >
> > /* Bridge core config registers */
> > #define BRCFG_PCIE_RX0 0x
> > @@ -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
RE: [PATCH 4/4] PCI: xilinx-nwl: Add method to setup_platform_service_irq hook
> 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
> > ---
> > 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
> >
> > #include "../pci.h"
> > +#include "../pcie/portdrv.h"
> >
> > /* Bridge core config registers */
> > #define BRCFG_PCIE_RX0 0x
> > @@ -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
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
> ---
> 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
>
> #include "../pci.h"
> +#include "../pcie/portdrv.h"
>
> /* Bridge core config registers */
> #define BRCFG_PCIE_RX0 0x
> @@ -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 = _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
>
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
> ---
> 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
>
> #include "../pci.h"
> +#include "../pcie/portdrv.h"
>
> /* Bridge core config registers */
> #define BRCFG_PCIE_RX0 0x
> @@ -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 = _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
>
RE: [PATCH 4/4] PCI: xilinx-nwl: Add method to setup_platform_service_irq hook
Agreed, will Fix it in next version. Regards, Bharat > -Original Message- > From: kbuild test robot [mailto:l...@intel.com] > Sent: Monday, August 13, 2018 2:39 PM > To: Bharat Kumar Gogada > Cc: kbuild-...@01.org; linux-...@vger.kernel.org; linux- > ker...@vger.kernel.org; bhelg...@google.com; Ravikiran Gummaluri > ; Bharat Kumar Gogada > 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 infrastructureOpen Source Technology Center > https://lists.01.org/pipermail/kbuild-all Intel Corporation
RE: [PATCH 4/4] PCI: xilinx-nwl: Add method to setup_platform_service_irq hook
Agreed, will Fix it in next version. Regards, Bharat > -Original Message- > From: kbuild test robot [mailto:l...@intel.com] > Sent: Monday, August 13, 2018 2:39 PM > To: Bharat Kumar Gogada > Cc: kbuild-...@01.org; linux-...@vger.kernel.org; linux- > ker...@vger.kernel.org; bhelg...@google.com; Ravikiran Gummaluri > ; Bharat Kumar Gogada > 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 infrastructureOpen Source Technology Center > https://lists.01.org/pipermail/kbuild-all Intel Corporation
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 infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
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 infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
[PATCH 4/4] PCI: xilinx-nwl: Add method to setup_platform_service_irq hook
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
---
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
#include "../pci.h"
+#include "../pcie/portdrv.h"
/* Bridge core config registers */
#define BRCFG_PCIE_RX0 0x
@@ -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 = _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
[PATCH 4/4] PCI: xilinx-nwl: Add method to setup_platform_service_irq hook
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
---
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
#include "../pci.h"
+#include "../pcie/portdrv.h"
/* Bridge core config registers */
#define BRCFG_PCIE_RX0 0x
@@ -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 = _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
