| Message ID | 1472553558-27215-1-git-send-email-bharatku@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Hi Bharat, On Tue, Aug 30, 2016 at 04:09:16PM +0530, Bharat Kumar Gogada wrote: > The current driver prints pcie core error, for all core events. > Instead of just printing PCIe core error, now adding prints to show > individual core events occurred. > > Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com> > --- > drivers/pci/host/pcie-xilinx-nwl.c | 48 +++++++++++++++++++++++++++++++------- > 1 file changed, 40 insertions(+), 8 deletions(-) > > diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c > index 3479d30..86c1834 100644 > --- a/drivers/pci/host/pcie-xilinx-nwl.c > +++ b/drivers/pci/host/pcie-xilinx-nwl.c > @@ -85,10 +85,15 @@ > #define MSGF_MISC_SR_MASTER_ERR BIT(5) > #define MSGF_MISC_SR_I_ADDR_ERR BIT(6) > #define MSGF_MISC_SR_E_ADDR_ERR BIT(7) > -#define MSGF_MISC_SR_UR_DETECT BIT(20) > - > -#define MSGF_MISC_SR_PCIE_CORE GENMASK(18, 16) > -#define MSGF_MISC_SR_PCIE_CORE_ERR GENMASK(31, 22) > +#define MSGF_MISC_SR_FATAL_AER BIT(16) > +#define MSGF_MISC_SR_NON_FATAL_AER BIT(17) > +#define MSGF_MISC_SR_CORR_AER BIT(18) > +#define MSGF_MISC_SR_UR_DETECT BIT(20) > +#define MSGF_MISC_SR_NON_FATAL_DEV BIT(22) > +#define MSGF_MISC_SR_FATAL_DEV BIT(23) > +#define MSGF_MISC_SR_LINK_DOWN BIT(24) > +#define MSGF_MSIC_SR_LINK_AUTO_BWIDTH BIT(25) > +#define MSGF_MSIC_SR_LINK_BWIDTH BIT(26) > > #define MSGF_MISC_SR_MASKALL (MSGF_MISC_SR_RXMSG_AVAIL | \ > MSGF_MISC_SR_RXMSG_OVER | \ > @@ -96,9 +101,15 @@ > MSGF_MISC_SR_MASTER_ERR | \ > MSGF_MISC_SR_I_ADDR_ERR | \ > MSGF_MISC_SR_E_ADDR_ERR | \ > + MSGF_MISC_SR_FATAL_AER | \ > + MSGF_MISC_SR_NON_FATAL_AER | \ > + MSGF_MISC_SR_CORR_AER | \ > MSGF_MISC_SR_UR_DETECT | \ > - MSGF_MISC_SR_PCIE_CORE | \ > - MSGF_MISC_SR_PCIE_CORE_ERR) > + MSGF_MISC_SR_NON_FATAL_DEV | \ > + MSGF_MISC_SR_FATAL_DEV | \ > + MSGF_MISC_SR_LINK_DOWN | \ > + MSGF_MSIC_SR_LINK_AUTO_BWIDTH | \ > + MSGF_MSIC_SR_LINK_BWIDTH) > > /* Legacy interrupt status mask bits */ > #define MSGF_LEG_SR_INTA BIT(0) > @@ -291,8 +302,29 @@ static irqreturn_t nwl_pcie_misc_handler(int irq, void *data) > dev_err(pcie->dev, > "In Misc Egress address translation error\n"); > > - if (misc_stat & MSGF_MISC_SR_PCIE_CORE_ERR) > - dev_err(pcie->dev, "PCIe Core error\n"); > + if (misc_stat & MSGF_MISC_SR_FATAL_AER) > + dev_err(pcie->dev, "Fatal Error in AER Capability\n"); > + > + if (misc_stat & MSGF_MISC_SR_NON_FATAL_AER) > + dev_err(pcie->dev, "Non-Fatal Error in AER Capability\n"); > + > + if (misc_stat & MSGF_MISC_SR_CORR_AER) > + dev_err(pcie->dev, "Correctable Error in AER Capability\n"); > + > + if (misc_stat & MSGF_MISC_SR_UR_DETECT) > + dev_err(pcie->dev, "Unsupported request Detected\n"); > + > + if (misc_stat & MSGF_MISC_SR_NON_FATAL_DEV) > + dev_err(pcie->dev, "Non-Fatal Error Detected\n"); > + > + if (misc_stat & MSGF_MISC_SR_FATAL_DEV) > + dev_err(pcie->dev, "Fatal Error Detected\n"); > + > + if (misc_stat & MSGF_MSIC_SR_LINK_AUTO_BWIDTH) > + dev_info(pcie->dev, "Link Autonomous Bandwidth Management Status bit set\n"); > + > + if (misc_stat & MSGF_MSIC_SR_LINK_BWIDTH) > + dev_info(pcie->dev, "Link Bandwidth Management Status bit set\n"); > > /* Clear misc interrupt status */ > nwl_bridge_writel(pcie, misc_stat, MSGF_MISC_STATUS); This patch looks fine, but looking at the code as a whole, I have a question. You basically have this: misc_stat = nwl_bridge_readl(pcie, MSGF_MISC_STATUS) & MSGF_MISC_SR_MASKALL; if (!misc_stat) return IRQ_NONE; ... nwl_bridge_writel(pcie, misc_stat, MSGF_MISC_STATUS); The masking with MSGF_MISC_SR_MASKALL seems wrong. Let's say MSGF_MISC_STATUS had some other bit set, e.g., BIT(31), indicating some yet-unsupported interrupt cause. BIT(31) is not in MSGF_MISC_SR_MASKALL, so we return IRQ_NONE without clearing the interrupt bit in MSGF_MISC_STATUS. Won't that cause an interrupt storm where the interrupt is continually re-asserted because we never clear it? It seems like it would make more sense to do this: misc_stat = nwl_bridge_readl(pcie, MSGF_MISC_STATUS); if (!misc_stat) return IRQ_NONE; ... if (misc_stat != (misc_stat & MSGF_MISC_SR_MASKALL)) dev_err(dev, "unexpected IRQ, MSGF_MISC_STATUS %#010x\n", misc_stat); nwl_bridge_writel(pcie, misc_stat, MSGF_MISC_STATUS); -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 30, 2016 at 04:09:16PM +0530, Bharat Kumar Gogada wrote: > The current driver prints pcie core error, for all core events. > Instead of just printing PCIe core error, now adding prints to show > individual core events occurred. > > Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com> I applied the first two patches to pci/host-xilinx for v4.9, thanks! I'd like to work on the third one a little more, as I mentioned in my response to it. > --- > drivers/pci/host/pcie-xilinx-nwl.c | 48 +++++++++++++++++++++++++++++++------- > 1 file changed, 40 insertions(+), 8 deletions(-) > > diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c > index 3479d30..86c1834 100644 > --- a/drivers/pci/host/pcie-xilinx-nwl.c > +++ b/drivers/pci/host/pcie-xilinx-nwl.c > @@ -85,10 +85,15 @@ > #define MSGF_MISC_SR_MASTER_ERR BIT(5) > #define MSGF_MISC_SR_I_ADDR_ERR BIT(6) > #define MSGF_MISC_SR_E_ADDR_ERR BIT(7) > -#define MSGF_MISC_SR_UR_DETECT BIT(20) > - > -#define MSGF_MISC_SR_PCIE_CORE GENMASK(18, 16) > -#define MSGF_MISC_SR_PCIE_CORE_ERR GENMASK(31, 22) > +#define MSGF_MISC_SR_FATAL_AER BIT(16) > +#define MSGF_MISC_SR_NON_FATAL_AER BIT(17) > +#define MSGF_MISC_SR_CORR_AER BIT(18) > +#define MSGF_MISC_SR_UR_DETECT BIT(20) > +#define MSGF_MISC_SR_NON_FATAL_DEV BIT(22) > +#define MSGF_MISC_SR_FATAL_DEV BIT(23) > +#define MSGF_MISC_SR_LINK_DOWN BIT(24) > +#define MSGF_MSIC_SR_LINK_AUTO_BWIDTH BIT(25) > +#define MSGF_MSIC_SR_LINK_BWIDTH BIT(26) > > #define MSGF_MISC_SR_MASKALL (MSGF_MISC_SR_RXMSG_AVAIL | \ > MSGF_MISC_SR_RXMSG_OVER | \ > @@ -96,9 +101,15 @@ > MSGF_MISC_SR_MASTER_ERR | \ > MSGF_MISC_SR_I_ADDR_ERR | \ > MSGF_MISC_SR_E_ADDR_ERR | \ > + MSGF_MISC_SR_FATAL_AER | \ > + MSGF_MISC_SR_NON_FATAL_AER | \ > + MSGF_MISC_SR_CORR_AER | \ > MSGF_MISC_SR_UR_DETECT | \ > - MSGF_MISC_SR_PCIE_CORE | \ > - MSGF_MISC_SR_PCIE_CORE_ERR) > + MSGF_MISC_SR_NON_FATAL_DEV | \ > + MSGF_MISC_SR_FATAL_DEV | \ > + MSGF_MISC_SR_LINK_DOWN | \ > + MSGF_MSIC_SR_LINK_AUTO_BWIDTH | \ > + MSGF_MSIC_SR_LINK_BWIDTH) > > /* Legacy interrupt status mask bits */ > #define MSGF_LEG_SR_INTA BIT(0) > @@ -291,8 +302,29 @@ static irqreturn_t nwl_pcie_misc_handler(int irq, void *data) > dev_err(pcie->dev, > "In Misc Egress address translation error\n"); > > - if (misc_stat & MSGF_MISC_SR_PCIE_CORE_ERR) > - dev_err(pcie->dev, "PCIe Core error\n"); > + if (misc_stat & MSGF_MISC_SR_FATAL_AER) > + dev_err(pcie->dev, "Fatal Error in AER Capability\n"); > + > + if (misc_stat & MSGF_MISC_SR_NON_FATAL_AER) > + dev_err(pcie->dev, "Non-Fatal Error in AER Capability\n"); > + > + if (misc_stat & MSGF_MISC_SR_CORR_AER) > + dev_err(pcie->dev, "Correctable Error in AER Capability\n"); > + > + if (misc_stat & MSGF_MISC_SR_UR_DETECT) > + dev_err(pcie->dev, "Unsupported request Detected\n"); > + > + if (misc_stat & MSGF_MISC_SR_NON_FATAL_DEV) > + dev_err(pcie->dev, "Non-Fatal Error Detected\n"); > + > + if (misc_stat & MSGF_MISC_SR_FATAL_DEV) > + dev_err(pcie->dev, "Fatal Error Detected\n"); > + > + if (misc_stat & MSGF_MSIC_SR_LINK_AUTO_BWIDTH) > + dev_info(pcie->dev, "Link Autonomous Bandwidth Management Status bit set\n"); > + > + if (misc_stat & MSGF_MSIC_SR_LINK_BWIDTH) > + dev_info(pcie->dev, "Link Bandwidth Management Status bit set\n"); > > /* Clear misc interrupt status */ > nwl_bridge_writel(pcie, misc_stat, MSGF_MISC_STATUS); > -- > 2.1.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Hi Bharat, > > On Tue, Aug 30, 2016 at 04:09:16PM +0530, Bharat Kumar Gogada wrote: > > The current driver prints pcie core error, for all core events. > > Instead of just printing PCIe core error, now adding prints to show > > individual core events occurred. > > > > Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com> > > --- > > drivers/pci/host/pcie-xilinx-nwl.c | 48 > > +++++++++++++++++++++++++++++++------- > > 1 file changed, 40 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/pci/host/pcie-xilinx-nwl.c > > b/drivers/pci/host/pcie-xilinx-nwl.c > > index 3479d30..86c1834 100644 > > --- a/drivers/pci/host/pcie-xilinx-nwl.c > > +++ b/drivers/pci/host/pcie-xilinx-nwl.c > > @@ -85,10 +85,15 @@ > > #define MSGF_MISC_SR_MASTER_ERR BIT(5) > > #define MSGF_MISC_SR_I_ADDR_ERR BIT(6) > > #define MSGF_MISC_SR_E_ADDR_ERR BIT(7) > > -#define MSGF_MISC_SR_UR_DETECT BIT(20) > > - > > -#define MSGF_MISC_SR_PCIE_CORE GENMASK(18, 16) > > -#define MSGF_MISC_SR_PCIE_CORE_ERR GENMASK(31, 22) > > +#define MSGF_MISC_SR_FATAL_AER BIT(16) > > +#define MSGF_MISC_SR_NON_FATAL_AER BIT(17) > > +#define MSGF_MISC_SR_CORR_AER BIT(18) > > +#define MSGF_MISC_SR_UR_DETECT BIT(20) > > +#define MSGF_MISC_SR_NON_FATAL_DEV BIT(22) > > +#define MSGF_MISC_SR_FATAL_DEV BIT(23) > > +#define MSGF_MISC_SR_LINK_DOWN BIT(24) > > +#define MSGF_MSIC_SR_LINK_AUTO_BWIDTH BIT(25) > > +#define MSGF_MSIC_SR_LINK_BWIDTH BIT(26) > > > > #define MSGF_MISC_SR_MASKALL > (MSGF_MISC_SR_RXMSG_AVAIL | \ > > MSGF_MISC_SR_RXMSG_OVER | \ > > @@ -96,9 +101,15 @@ > > MSGF_MISC_SR_MASTER_ERR | \ > > MSGF_MISC_SR_I_ADDR_ERR | \ > > MSGF_MISC_SR_E_ADDR_ERR | \ > > + MSGF_MISC_SR_FATAL_AER | \ > > + MSGF_MISC_SR_NON_FATAL_AER | \ > > + MSGF_MISC_SR_CORR_AER | \ > > MSGF_MISC_SR_UR_DETECT | \ > > - MSGF_MISC_SR_PCIE_CORE | \ > > - MSGF_MISC_SR_PCIE_CORE_ERR) > > + MSGF_MISC_SR_NON_FATAL_DEV | \ > > + MSGF_MISC_SR_FATAL_DEV | \ > > + MSGF_MISC_SR_LINK_DOWN | \ > > + MSGF_MSIC_SR_LINK_AUTO_BWIDTH > | \ > > + MSGF_MSIC_SR_LINK_BWIDTH) > > > > /* Legacy interrupt status mask bits */ > > #define MSGF_LEG_SR_INTA BIT(0) > > @@ -291,8 +302,29 @@ static irqreturn_t nwl_pcie_misc_handler(int irq, void > *data) > > dev_err(pcie->dev, > > "In Misc Egress address translation error\n"); > > > > - if (misc_stat & MSGF_MISC_SR_PCIE_CORE_ERR) > > - dev_err(pcie->dev, "PCIe Core error\n"); > > + if (misc_stat & MSGF_MISC_SR_FATAL_AER) > > + dev_err(pcie->dev, "Fatal Error in AER Capability\n"); > > + > > + if (misc_stat & MSGF_MISC_SR_NON_FATAL_AER) > > + dev_err(pcie->dev, "Non-Fatal Error in AER Capability\n"); > > + > > + if (misc_stat & MSGF_MISC_SR_CORR_AER) > > + dev_err(pcie->dev, "Correctable Error in AER Capability\n"); > > + > > + if (misc_stat & MSGF_MISC_SR_UR_DETECT) > > + dev_err(pcie->dev, "Unsupported request Detected\n"); > > + > > + if (misc_stat & MSGF_MISC_SR_NON_FATAL_DEV) > > + dev_err(pcie->dev, "Non-Fatal Error Detected\n"); > > + > > + if (misc_stat & MSGF_MISC_SR_FATAL_DEV) > > + dev_err(pcie->dev, "Fatal Error Detected\n"); > > + > > + if (misc_stat & MSGF_MSIC_SR_LINK_AUTO_BWIDTH) > > + dev_info(pcie->dev, "Link Autonomous Bandwidth Management > Status > > +bit set\n"); > > + > > + if (misc_stat & MSGF_MSIC_SR_LINK_BWIDTH) > > + dev_info(pcie->dev, "Link Bandwidth Management Status bit > set\n"); > > > > /* Clear misc interrupt status */ > > nwl_bridge_writel(pcie, misc_stat, MSGF_MISC_STATUS); > > This patch looks fine, but looking at the code as a whole, I have a question. You > basically have this: > > misc_stat = nwl_bridge_readl(pcie, MSGF_MISC_STATUS) & > MSGF_MISC_SR_MASKALL; > if (!misc_stat) > return IRQ_NONE; > ... > nwl_bridge_writel(pcie, misc_stat, MSGF_MISC_STATUS); > > The masking with MSGF_MISC_SR_MASKALL seems wrong. Let's say > MSGF_MISC_STATUS had some other bit set, e.g., BIT(31), indicating some yet- > unsupported interrupt cause. BIT(31) is not in MSGF_MISC_SR_MASKALL, so we > return IRQ_NONE without clearing the interrupt bit in MSGF_MISC_STATUS. > Won't that cause an interrupt storm where the interrupt is continually re- > asserted because we never clear it? > > It seems like it would make more sense to do this: > > misc_stat = nwl_bridge_readl(pcie, MSGF_MISC_STATUS); > if (!misc_stat) > return IRQ_NONE; > ... > if (misc_stat != (misc_stat & MSGF_MISC_SR_MASKALL)) > dev_err(dev, "unexpected IRQ, MSGF_MISC_STATUS %#010x\n", misc_stat); > nwl_bridge_writel(pcie, misc_stat, MSGF_MISC_STATUS); Hi Bjorn, Thanks for the suggestion. The above mentioned bits are reserved and they will never go high. We haven't seen such kind of issue until now. So I feel the current implementation is fine. Thanks & Regards, Bharat -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Tue, Aug 30, 2016 at 04:09:16PM +0530, Bharat Kumar Gogada wrote: > > The current driver prints pcie core error, for all core events. > > Instead of just printing PCIe core error, now adding prints to show > > individual core events occurred. > > > > Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com> > > I applied the first two patches to pci/host-xilinx for v4.9, thanks! > > I'd like to work on the third one a little more, as I mentioned in my response to > it. > Thanks Bjorn, yes I agree that third one needs better way of handling. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/host/pcie-xilinx-nwl.c b/drivers/pci/host/pcie-xilinx-nwl.c index 3479d30..86c1834 100644 --- a/drivers/pci/host/pcie-xilinx-nwl.c +++ b/drivers/pci/host/pcie-xilinx-nwl.c @@ -85,10 +85,15 @@ #define MSGF_MISC_SR_MASTER_ERR BIT(5) #define MSGF_MISC_SR_I_ADDR_ERR BIT(6) #define MSGF_MISC_SR_E_ADDR_ERR BIT(7) -#define MSGF_MISC_SR_UR_DETECT BIT(20) - -#define MSGF_MISC_SR_PCIE_CORE GENMASK(18, 16) -#define MSGF_MISC_SR_PCIE_CORE_ERR GENMASK(31, 22) +#define MSGF_MISC_SR_FATAL_AER BIT(16) +#define MSGF_MISC_SR_NON_FATAL_AER BIT(17) +#define MSGF_MISC_SR_CORR_AER BIT(18) +#define MSGF_MISC_SR_UR_DETECT BIT(20) +#define MSGF_MISC_SR_NON_FATAL_DEV BIT(22) +#define MSGF_MISC_SR_FATAL_DEV BIT(23) +#define MSGF_MISC_SR_LINK_DOWN BIT(24) +#define MSGF_MSIC_SR_LINK_AUTO_BWIDTH BIT(25) +#define MSGF_MSIC_SR_LINK_BWIDTH BIT(26) #define MSGF_MISC_SR_MASKALL (MSGF_MISC_SR_RXMSG_AVAIL | \ MSGF_MISC_SR_RXMSG_OVER | \ @@ -96,9 +101,15 @@ MSGF_MISC_SR_MASTER_ERR | \ MSGF_MISC_SR_I_ADDR_ERR | \ MSGF_MISC_SR_E_ADDR_ERR | \ + MSGF_MISC_SR_FATAL_AER | \ + MSGF_MISC_SR_NON_FATAL_AER | \ + MSGF_MISC_SR_CORR_AER | \ MSGF_MISC_SR_UR_DETECT | \ - MSGF_MISC_SR_PCIE_CORE | \ - MSGF_MISC_SR_PCIE_CORE_ERR) + MSGF_MISC_SR_NON_FATAL_DEV | \ + MSGF_MISC_SR_FATAL_DEV | \ + MSGF_MISC_SR_LINK_DOWN | \ + MSGF_MSIC_SR_LINK_AUTO_BWIDTH | \ + MSGF_MSIC_SR_LINK_BWIDTH) /* Legacy interrupt status mask bits */ #define MSGF_LEG_SR_INTA BIT(0) @@ -291,8 +302,29 @@ static irqreturn_t nwl_pcie_misc_handler(int irq, void *data) dev_err(pcie->dev, "In Misc Egress address translation error\n"); - if (misc_stat & MSGF_MISC_SR_PCIE_CORE_ERR) - dev_err(pcie->dev, "PCIe Core error\n"); + if (misc_stat & MSGF_MISC_SR_FATAL_AER) + dev_err(pcie->dev, "Fatal Error in AER Capability\n"); + + if (misc_stat & MSGF_MISC_SR_NON_FATAL_AER) + dev_err(pcie->dev, "Non-Fatal Error in AER Capability\n"); + + if (misc_stat & MSGF_MISC_SR_CORR_AER) + dev_err(pcie->dev, "Correctable Error in AER Capability\n"); + + if (misc_stat & MSGF_MISC_SR_UR_DETECT) + dev_err(pcie->dev, "Unsupported request Detected\n"); + + if (misc_stat & MSGF_MISC_SR_NON_FATAL_DEV) + dev_err(pcie->dev, "Non-Fatal Error Detected\n"); + + if (misc_stat & MSGF_MISC_SR_FATAL_DEV) + dev_err(pcie->dev, "Fatal Error Detected\n"); + + if (misc_stat & MSGF_MSIC_SR_LINK_AUTO_BWIDTH) + dev_info(pcie->dev, "Link Autonomous Bandwidth Management Status bit set\n"); + + if (misc_stat & MSGF_MSIC_SR_LINK_BWIDTH) + dev_info(pcie->dev, "Link Bandwidth Management Status bit set\n"); /* Clear misc interrupt status */ nwl_bridge_writel(pcie, misc_stat, MSGF_MISC_STATUS);
The current driver prints pcie core error, for all core events. Instead of just printing PCIe core error, now adding prints to show individual core events occurred. Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com> --- drivers/pci/host/pcie-xilinx-nwl.c | 48 +++++++++++++++++++++++++++++++------- 1 file changed, 40 insertions(+), 8 deletions(-)