| Message ID | 20170922072522.36306-3-Zhiqiang.Hou@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Fri, Sep 22, 2017 at 03:25:22PM +0800, Zhiqiang Hou wrote: > From: Minghuan Lian <Minghuan.Lian@nxp.com> > > By default, when the PCIe controller experiences an erroneous > completion from an external completer for its outbound non-posted > request, it always sends an OKAY response to the device's internal > AXI slave system interface. However, such default system error > response behavior cannot be used for other types of outbound > non-posted requests. For example, the outbound memory read > transaction requires an actual ERROR response, like UR completion > or completion timeout. The patch is to fix it by forwarding > the error response of the non-posted request. > > Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com> > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > --- > drivers/pci/dwc/pci-layerscape.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/drivers/pci/dwc/pci-layerscape.c b/drivers/pci/dwc/pci-layerscape.c > index 3b01e309a55e..a647090c140e 100644 > --- a/drivers/pci/dwc/pci-layerscape.c > +++ b/drivers/pci/dwc/pci-layerscape.c > @@ -33,6 +33,8 @@ > > /* PEX Internal Configuration Registers */ > #define PCIE_STRFMR1 0x71c /* Symbol Timer & Filter Mask Register1 */ > +#define PCIE_ABSERR 0x8d0 /* Bridge Slave Error Response Register */ > +#define PCIE_ABSERR_SETTING 0x9401 /* Forward error of non-posted request */ > > #define PCIE_IATU_NUM 6 > > @@ -54,6 +56,19 @@ struct ls_pcie { > > #define to_ls_pcie(x) dev_get_drvdata((x)->dev) > > +static int err_response_flag = 1; > + > +static int __init ls_pcie_param(char *p) > +{ > + if (p && strncmp(p, "no-err-response", 15) == 0) > + err_response_flag = 0; > + else > + err_response_flag = 1; > + > + return 0; > +} > +early_param("ls_pcie", ls_pcie_param); What's the point of this parameter? If it's for debugging, it's not clear that we need it upstream. If it's for debugging and we *do* need it upstream, there should be some sort of comment to that effect. I assume you never expect an end user to need this parameter. > static bool ls_pcie_is_bridge(struct ls_pcie *pcie) > { > struct dw_pcie *pci = pcie->pci; > @@ -124,6 +139,14 @@ static int ls_pcie_link_up(struct dw_pcie *pci) > return 1; > } > > +/* Forward error response of outbound non-posted requests */ > +static void ls_pcie_fix_error_response(struct ls_pcie *pcie) > +{ > + struct dw_pcie *pci = pcie->pci; > + > + iowrite32(PCIE_ABSERR_SETTING, pci->dbi_base + PCIE_ABSERR); > +} > + > static int ls_pcie_host_init(struct pcie_port *pp) > { > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > @@ -135,6 +158,8 @@ static int ls_pcie_host_init(struct pcie_port *pp) > * dw_pcie_setup_rc() will reconfigure the outbound windows. > */ > ls_pcie_disable_outbound_atus(pcie); > + if (err_response_flag) > + ls_pcie_fix_error_response(pcie); > > dw_pcie_dbi_ro_wr_en(pci); > ls_pcie_clear_multifunction(pcie); > -- > 2.14.1 >
Hi Bjorn, Thanks a lot for your review! > -----Original Message----- > From: Bjorn Helgaas [mailto:helgaas@kernel.org] > Sent: 2017年10月12日 3:41 > To: Z.q. Hou <zhiqiang.hou@nxp.com> > Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > linux-pci@vger.kernel.org; bhelgaas@google.com; Roy Zang > <roy.zang@nxp.com>; Mingkai Hu <mingkai.hu@nxp.com>; M.h. Lian > <minghuan.lian@nxp.com> > Subject: Re: [PATCH 2/2] pci/layerscape: change the default error response > behavior > > On Fri, Sep 22, 2017 at 03:25:22PM +0800, Zhiqiang Hou wrote: > > From: Minghuan Lian <Minghuan.Lian@nxp.com> > > > > By default, when the PCIe controller experiences an erroneous > > completion from an external completer for its outbound non-posted > > request, it always sends an OKAY response to the device's internal AXI > > slave system interface. However, such default system error response > > behavior cannot be used for other types of outbound non-posted > > requests. For example, the outbound memory read transaction requires > > an actual ERROR response, like UR completion or completion timeout. > > The patch is to fix it by forwarding the error response of the > > non-posted request. > > > > Signed-off-by: Minghuan Lian <Minghuan.Lian@nxp.com> > > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > > --- > > drivers/pci/dwc/pci-layerscape.c | 25 +++++++++++++++++++++++++ > > 1 file changed, 25 insertions(+) > > > > diff --git a/drivers/pci/dwc/pci-layerscape.c > > b/drivers/pci/dwc/pci-layerscape.c > > index 3b01e309a55e..a647090c140e 100644 > > --- a/drivers/pci/dwc/pci-layerscape.c > > +++ b/drivers/pci/dwc/pci-layerscape.c > > @@ -33,6 +33,8 @@ > > > > /* PEX Internal Configuration Registers */ > > #define PCIE_STRFMR1 0x71c /* Symbol Timer & Filter Mask > Register1 */ > > +#define PCIE_ABSERR 0x8d0 /* Bridge Slave Error Response > Register */ > > +#define PCIE_ABSERR_SETTING 0x9401 /* Forward error of non-posted > request */ > > > > #define PCIE_IATU_NUM 6 > > > > @@ -54,6 +56,19 @@ struct ls_pcie { > > > > #define to_ls_pcie(x) dev_get_drvdata((x)->dev) > > > > +static int err_response_flag = 1; > > + > > +static int __init ls_pcie_param(char *p) { > > + if (p && strncmp(p, "no-err-response", 15) == 0) > > + err_response_flag = 0; > > + else > > + err_response_flag = 1; > > + > > + return 0; > > +} > > +early_param("ls_pcie", ls_pcie_param); > > What's the point of this parameter? If it's for debugging, it's not clear that > we need it upstream. If it's for debugging and we *do* need it upstream, > there should be some sort of comment to that effect. > > I assume you never expect an end user to need this parameter. It is for debugging, will drop this parameter next version. > > > static bool ls_pcie_is_bridge(struct ls_pcie *pcie) { > > struct dw_pcie *pci = pcie->pci; > > @@ -124,6 +139,14 @@ static int ls_pcie_link_up(struct dw_pcie *pci) > > return 1; > > } > > > > +/* Forward error response of outbound non-posted requests */ static > > +void ls_pcie_fix_error_response(struct ls_pcie *pcie) { > > + struct dw_pcie *pci = pcie->pci; > > + > > + iowrite32(PCIE_ABSERR_SETTING, pci->dbi_base + PCIE_ABSERR); } > > + > > static int ls_pcie_host_init(struct pcie_port *pp) { > > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); @@ -135,6 +158,8 @@ > > static int ls_pcie_host_init(struct pcie_port *pp) > > * dw_pcie_setup_rc() will reconfigure the outbound windows. > > */ > > ls_pcie_disable_outbound_atus(pcie); > > + if (err_response_flag) > > + ls_pcie_fix_error_response(pcie); > > > > dw_pcie_dbi_ro_wr_en(pci); > > ls_pcie_clear_multifunction(pcie); > > -- > > 2.14.1 > > Thanks, Zhiqiang
diff --git a/drivers/pci/dwc/pci-layerscape.c b/drivers/pci/dwc/pci-layerscape.c index 3b01e309a55e..a647090c140e 100644 --- a/drivers/pci/dwc/pci-layerscape.c +++ b/drivers/pci/dwc/pci-layerscape.c @@ -33,6 +33,8 @@ /* PEX Internal Configuration Registers */ #define PCIE_STRFMR1 0x71c /* Symbol Timer & Filter Mask Register1 */ +#define PCIE_ABSERR 0x8d0 /* Bridge Slave Error Response Register */ +#define PCIE_ABSERR_SETTING 0x9401 /* Forward error of non-posted request */ #define PCIE_IATU_NUM 6 @@ -54,6 +56,19 @@ struct ls_pcie { #define to_ls_pcie(x) dev_get_drvdata((x)->dev) +static int err_response_flag = 1; + +static int __init ls_pcie_param(char *p) +{ + if (p && strncmp(p, "no-err-response", 15) == 0) + err_response_flag = 0; + else + err_response_flag = 1; + + return 0; +} +early_param("ls_pcie", ls_pcie_param); + static bool ls_pcie_is_bridge(struct ls_pcie *pcie) { struct dw_pcie *pci = pcie->pci; @@ -124,6 +139,14 @@ static int ls_pcie_link_up(struct dw_pcie *pci) return 1; } +/* Forward error response of outbound non-posted requests */ +static void ls_pcie_fix_error_response(struct ls_pcie *pcie) +{ + struct dw_pcie *pci = pcie->pci; + + iowrite32(PCIE_ABSERR_SETTING, pci->dbi_base + PCIE_ABSERR); +} + static int ls_pcie_host_init(struct pcie_port *pp) { struct dw_pcie *pci = to_dw_pcie_from_pp(pp); @@ -135,6 +158,8 @@ static int ls_pcie_host_init(struct pcie_port *pp) * dw_pcie_setup_rc() will reconfigure the outbound windows. */ ls_pcie_disable_outbound_atus(pcie); + if (err_response_flag) + ls_pcie_fix_error_response(pcie); dw_pcie_dbi_ro_wr_en(pci); ls_pcie_clear_multifunction(pcie);