[2/2] pci/layerscape: change the default error response behavior

Message ID 20170922072522.36306-3-Zhiqiang.Hou@nxp.com
State Not Applicable
Headers show
Series
  • PCI: layerscape: add fixes for layerscape-pcie errata
Related show

Commit Message

Zhiqiang Hou Sept. 22, 2017, 7:25 a.m.
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(+)

Comments

Bjorn Helgaas Oct. 11, 2017, 7:41 p.m. | #1
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
>
Zhiqiang Hou Oct. 12, 2017, 3:33 a.m. | #2
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

Patch

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