Re: [PATCH v2] PCI: designware: add host_init error handling

[Jaehoon Chung]

Hi Srinivas,


On 12/07/2016 07:32 PM, Srinivas Kandagatla wrote:
> This patch add support to return value from host_init() callback from drivers,
> so that the designware libary can handle or pass it to proper place. Issue 
> with
> void return type is that errors or error handling within host_init() callback
> are never know to designware code, which could go ahead and access registers
> even in error cases.
> 
> Typical case in qcom controller driver is to turn off clks in case of errors,
> if designware code continues to read/write register when clocks are turned off
> the board would reboot/lockup.

Added the comment for minor thing.
I agreed this approach.

> 
> Signed-off-by: Srinivas Kandagatla 
> ---
> Currently designware code does not have a way return errors generated
> as part of host_init() callback in controller drivers. This is an issue
> with controller drivers like qcom which turns off the clocks in error
> handling path. As the dw core is un aware of this would continue to
> access registers which faults resulting in board reboots/hangs.
> 
> There are two ways to solve this issue,
> one is remove error handling in the qcom controller host_init() function
> other is to handle error and pass back to dw core code which would then
> pass back to controller driver as part of dw_pcie_host_init() return value.
> 
> Second option seems more sensible and correct way to fix the issue,
> this patch does the same.
> 
> As part of this change to host_init() return type I had to patch other
> ihost controller drivers which use dw core. Most of the changes to other 
> drivers
> are to return proper error codes to upper layer.
> Only compile tested drivers.
> 
> Changes since RFC:
>   - Add error handling to other drivers as suggested by Joao Pinto
> 
>  drivers/pci/host/pci-dra7xx.c   | 10 --
>  drivers/pci/host/pci-exynos.c   | 10 --
>  drivers/pci/host/pci-imx6.c | 10 --
>  drivers/pci/host/pci-keystone.c | 10 --
>  drivers/pci/host/pci-layerscape.c   | 22 +-
>  drivers/pci/host/pcie-armada8k.c|  4 +++-
>  drivers/pci/host/pcie-designware-plat.c | 10 --
>  drivers/pci/host/pcie-designware.c  |  4 +++-
>  drivers/pci/host/pcie-designware.h  |  2 +-
>  drivers/pci/host/pcie-qcom.c|  5 +++--
>  drivers/pci/host/pcie-spear13xx.c   | 10 --
>  11 files changed, 71 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c
> index 9595fad..811f0f9 100644
> --- a/drivers/pci/host/pci-dra7xx.c
> +++ b/drivers/pci/host/pci-dra7xx.c
> @@ -127,9 +127,10 @@ static void dra7xx_pcie_enable_interrupts(struct 
> dra7xx_pcie *dra7xx)
>  LEG_EP_INTERRUPTS);
>  }
>  
> -static void dra7xx_pcie_host_init(struct pcie_port *pp)
> +static int dra7xx_pcie_host_init(struct pcie_port *pp)
>  {
>   struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pp);
> + int ret;
>  
>   pp->io_base &= DRA7XX_CPU_TO_BUS_ADDR;
>   pp->mem_base &= DRA7XX_CPU_TO_BUS_ADDR;
> @@ -138,10 +139,15 @@ static void dra7xx_pcie_host_init(struct pcie_port *pp)
>  
>   dw_pcie_setup_rc(pp);
>  
> - dra7xx_pcie_establish_link(dra7xx);
> + ret = dra7xx_pcie_establish_link(dra7xx);
> + if (ret < 0)
> + return ret;
> +
>   if (IS_ENABLED(CONFIG_PCI_MSI))
>   dw_pcie_msi_init(pp);
>   dra7xx_pcie_enable_interrupts(dra7xx);
> +
> + return 0;
>  }
>  
>  static struct pcie_host_ops dra7xx_pcie_host_ops = {
> diff --git a/drivers/pci/host/pci-exynos.c b/drivers/pci/host/pci-exynos.c
> index f1c544b..c116fd9 100644
> --- a/drivers/pci/host/pci-exynos.c
> +++ b/drivers/pci/host/pci-exynos.c
> @@ -458,12 +458,18 @@ static int exynos_pcie_link_up(struct pcie_port *pp)
>   return 0;
>  }
>  
> -static void exynos_pcie_host_init(struct pcie_port *pp)
> +static int exynos_pcie_host_init(struct pcie_port *pp)
>  {
>   struct exynos_pcie *exynos_pcie = to_exynos_pcie(pp);
> + int ret;
> +
> + ret = exynos_pcie_establish_link(exynos_pcie);
> + if (ret < 0)
> + return ret;
>  
> - exynos_pcie_establish_link(exynos_pcie);
>   exynos_pcie_enable_interrupts(exynos_pcie);
> +
> + return 0;
>  }
>  
>  static struct pcie_host_ops exynos_pcie_host_ops = {
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index c8cefb0..1251e92 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -550,18 +550,24 @@ static int imx6_pcie_establish_link(struct imx6_pcie 
> *imx6_pcie)
>   return ret;
>  }
>  
> -static void imx6_pcie_host_init(struct pcie_port *pp)
> +static int imx6_pcie_host_init(struct pcie_port *pp)
>  {
>   struct imx6_pcie *imx6_pcie = to_imx6_pcie(pp);
> + int ret;
>  
>   imx6_pcie_assert_core_reset(imx6_pcie);
>   

Re: [PATCH v2] PCI: designware: add host_init error handling

[Jisheng Zhang]

Hi Srinivas,

On Wed,  7 Dec 2016 10:32:49 + Srinivas Kandagatla wrote:

> This patch add support to return value from host_init() callback from drivers,
> so that the designware libary can handle or pass it to proper place. Issue 
> with
> void return type is that errors or error handling within host_init() callback
> are never know to designware code, which could go ahead and access registers
> even in error cases.
> 
> Typical case in qcom controller driver is to turn off clks in case of errors,

I have similar idea last year

https://patchwork.kernel.org/patch/7602421/

But finally I dropped the patch. The reason is PCIe supports hotplug. I
noticed that most *errors* are due to the link can't be established, for
example, the exynos, dra7xx in your below patch. I think FAIL_TO_ESTABLISH_LINK
is a normal case if we didn't plug in the PCIe DP, then in this case I think
it can't be considered as *error* and we still need to continue. What about
your opinion?

Thanks,
Jisheng

> if designware code continues to read/write register when clocks are turned off
> the board would reboot/lockup.
> 
> Signed-off-by: Srinivas Kandagatla 
> ---
> Currently designware code does not have a way return errors generated
> as part of host_init() callback in controller drivers. This is an issue
> with controller drivers like qcom which turns off the clocks in error
> handling path. As the dw core is un aware of this would continue to
> access registers which faults resulting in board reboots/hangs.
> 
> There are two ways to solve this issue,
> one is remove error handling in the qcom controller host_init() function
> other is to handle error and pass back to dw core code which would then
> pass back to controller driver as part of dw_pcie_host_init() return value.
> 
> Second option seems more sensible and correct way to fix the issue,
> this patch does the same.
> 
> As part of this change to host_init() return type I had to patch other
> ihost controller drivers which use dw core. Most of the changes to other 
> drivers
> are to return proper error codes to upper layer.
> Only compile tested drivers.
> 
> Changes since RFC:
>   - Add error handling to other drivers as suggested by Joao Pinto
> 
>  drivers/pci/host/pci-dra7xx.c   | 10 --
>  drivers/pci/host/pci-exynos.c   | 10 --
>  drivers/pci/host/pci-imx6.c | 10 --
>  drivers/pci/host/pci-keystone.c | 10 --
>  drivers/pci/host/pci-layerscape.c   | 22 +-
>  drivers/pci/host/pcie-armada8k.c|  4 +++-
>  drivers/pci/host/pcie-designware-plat.c | 10 --
>  drivers/pci/host/pcie-designware.c  |  4 +++-
>  drivers/pci/host/pcie-designware.h  |  2 +-
>  drivers/pci/host/pcie-qcom.c|  5 +++--
>  drivers/pci/host/pcie-spear13xx.c   | 10 --
>  11 files changed, 71 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c
> index 9595fad..811f0f9 100644
> --- a/drivers/pci/host/pci-dra7xx.c
> +++ b/drivers/pci/host/pci-dra7xx.c
> @@ -127,9 +127,10 @@ static void dra7xx_pcie_enable_interrupts(struct 
> dra7xx_pcie *dra7xx)
>  LEG_EP_INTERRUPTS);
>  }
>  
> -static void dra7xx_pcie_host_init(struct pcie_port *pp)
> +static int dra7xx_pcie_host_init(struct pcie_port *pp)
>  {
>   struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pp);
> + int ret;
>  
>   pp->io_base &= DRA7XX_CPU_TO_BUS_ADDR;
>   pp->mem_base &= DRA7XX_CPU_TO_BUS_ADDR;
> @@ -138,10 +139,15 @@ static void dra7xx_pcie_host_init(struct pcie_port *pp)
>  
>   dw_pcie_setup_rc(pp);
>  
> - dra7xx_pcie_establish_link(dra7xx);
> + ret = dra7xx_pcie_establish_link(dra7xx);
> + if (ret < 0)
> + return ret;
> +
>   if (IS_ENABLED(CONFIG_PCI_MSI))
>   dw_pcie_msi_init(pp);
>   dra7xx_pcie_enable_interrupts(dra7xx);
> +
> + return 0;
>  }
>  
>  static struct pcie_host_ops dra7xx_pcie_host_ops = {
> diff --git a/drivers/pci/host/pci-exynos.c b/drivers/pci/host/pci-exynos.c
> index f1c544b..c116fd9 100644
> --- a/drivers/pci/host/pci-exynos.c
> +++ b/drivers/pci/host/pci-exynos.c
> @@ -458,12 +458,18 @@ static int exynos_pcie_link_up(struct pcie_port *pp)
>   return 0;
>  }
>  
> -static void exynos_pcie_host_init(struct pcie_port *pp)
> +static int exynos_pcie_host_init(struct pcie_port *pp)
>  {
>   struct exynos_pcie *exynos_pcie = to_exynos_pcie(pp);
> + int ret;
> +
> + ret = exynos_pcie_establish_link(exynos_pcie);
> + if (ret < 0)
> + return ret;
>  
> - exynos_pcie_establish_link(exynos_pcie);
>   exynos_pcie_enable_interrupts(exynos_pcie);
> +
> + return 0;
>  }
>  
>  static struct pcie_host_ops exynos_pcie_host_ops = {
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index c8cefb0..1251e92 100644
> --- 

Re: [PATCH v2] PCI: designware: add host_init error handling

[Joao Pinto]

Hi Srinivas!

Thanks for the update!

Acked-By: Joao Pinto 

Às 10:32 AM de 12/7/2016, Srinivas Kandagatla escreveu:
> This patch add support to return value from host_init() callback from drivers,
> so that the designware libary can handle or pass it to proper place. Issue 
> with
> void return type is that errors or error handling within host_init() callback
> are never know to designware code, which could go ahead and access registers
> even in error cases.
> 
> Typical case in qcom controller driver is to turn off clks in case of errors,
> if designware code continues to read/write register when clocks are turned off
> the board would reboot/lockup.
> 
> Signed-off-by: Srinivas Kandagatla 
> ---
> Currently designware code does not have a way return errors generated
> as part of host_init() callback in controller drivers. This is an issue
> with controller drivers like qcom which turns off the clocks in error
> handling path. As the dw core is un aware of this would continue to
> access registers which faults resulting in board reboots/hangs.
> 
> There are two ways to solve this issue,
> one is remove error handling in the qcom controller host_init() function
> other is to handle error and pass back to dw core code which would then
> pass back to controller driver as part of dw_pcie_host_init() return value.
> 
> Second option seems more sensible and correct way to fix the issue,
> this patch does the same.
> 
> As part of this change to host_init() return type I had to patch other
> ihost controller drivers which use dw core. Most of the changes to other 
> drivers
> are to return proper error codes to upper layer.
> Only compile tested drivers.
> 
> Changes since RFC:
>   - Add error handling to other drivers as suggested by Joao Pinto
> 
>  drivers/pci/host/pci-dra7xx.c   | 10 --
>  drivers/pci/host/pci-exynos.c   | 10 --
>  drivers/pci/host/pci-imx6.c | 10 --
>  drivers/pci/host/pci-keystone.c | 10 --
>  drivers/pci/host/pci-layerscape.c   | 22 +-
>  drivers/pci/host/pcie-armada8k.c|  4 +++-
>  drivers/pci/host/pcie-designware-plat.c | 10 --
>  drivers/pci/host/pcie-designware.c  |  4 +++-
>  drivers/pci/host/pcie-designware.h  |  2 +-
>  drivers/pci/host/pcie-qcom.c|  5 +++--
>  drivers/pci/host/pcie-spear13xx.c   | 10 --
>  11 files changed, 71 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c
> index 9595fad..811f0f9 100644
> --- a/drivers/pci/host/pci-dra7xx.c
> +++ b/drivers/pci/host/pci-dra7xx.c
> @@ -127,9 +127,10 @@ static void dra7xx_pcie_enable_interrupts(struct 
> dra7xx_pcie *dra7xx)
>  LEG_EP_INTERRUPTS);
>  }
>  
> -static void dra7xx_pcie_host_init(struct pcie_port *pp)
> +static int dra7xx_pcie_host_init(struct pcie_port *pp)
>  {
>   struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pp);
> + int ret;
>  
>   pp->io_base &= DRA7XX_CPU_TO_BUS_ADDR;
>   pp->mem_base &= DRA7XX_CPU_TO_BUS_ADDR;
> @@ -138,10 +139,15 @@ static void dra7xx_pcie_host_init(struct pcie_port *pp)
>  
>   dw_pcie_setup_rc(pp);
>  
> - dra7xx_pcie_establish_link(dra7xx);
> + ret = dra7xx_pcie_establish_link(dra7xx);
> + if (ret < 0)
> + return ret;
> +
>   if (IS_ENABLED(CONFIG_PCI_MSI))
>   dw_pcie_msi_init(pp);
>   dra7xx_pcie_enable_interrupts(dra7xx);
> +
> + return 0;
>  }
>  
>  static struct pcie_host_ops dra7xx_pcie_host_ops = {
> diff --git a/drivers/pci/host/pci-exynos.c b/drivers/pci/host/pci-exynos.c
> index f1c544b..c116fd9 100644
> --- a/drivers/pci/host/pci-exynos.c
> +++ b/drivers/pci/host/pci-exynos.c
> @@ -458,12 +458,18 @@ static int exynos_pcie_link_up(struct pcie_port *pp)
>   return 0;
>  }
>  
> -static void exynos_pcie_host_init(struct pcie_port *pp)
> +static int exynos_pcie_host_init(struct pcie_port *pp)
>  {
>   struct exynos_pcie *exynos_pcie = to_exynos_pcie(pp);
> + int ret;
> +
> + ret = exynos_pcie_establish_link(exynos_pcie);
> + if (ret < 0)
> + return ret;
>  
> - exynos_pcie_establish_link(exynos_pcie);
>   exynos_pcie_enable_interrupts(exynos_pcie);
> +
> + return 0;
>  }
>  
>  static struct pcie_host_ops exynos_pcie_host_ops = {
> diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
> index c8cefb0..1251e92 100644
> --- a/drivers/pci/host/pci-imx6.c
> +++ b/drivers/pci/host/pci-imx6.c
> @@ -550,18 +550,24 @@ static int imx6_pcie_establish_link(struct imx6_pcie 
> *imx6_pcie)
>   return ret;
>  }
>  
> -static void imx6_pcie_host_init(struct pcie_port *pp)
> +static int imx6_pcie_host_init(struct pcie_port *pp)
>  {
>   struct imx6_pcie *imx6_pcie = to_imx6_pcie(pp);
> + int ret;
>  
>   imx6_pcie_assert_core_reset(imx6_pcie);
>   

[PATCH v2] PCI: designware: add host_init error handling

[Srinivas Kandagatla]

This patch add support to return value from host_init() callback from drivers,
so that the designware libary can handle or pass it to proper place. Issue with
void return type is that errors or error handling within host_init() callback
are never know to designware code, which could go ahead and access registers
even in error cases.

Typical case in qcom controller driver is to turn off clks in case of errors,
if designware code continues to read/write register when clocks are turned off
the board would reboot/lockup.

Signed-off-by: Srinivas Kandagatla 
---
Currently designware code does not have a way return errors generated
as part of host_init() callback in controller drivers. This is an issue
with controller drivers like qcom which turns off the clocks in error
handling path. As the dw core is un aware of this would continue to
access registers which faults resulting in board reboots/hangs.

There are two ways to solve this issue,
one is remove error handling in the qcom controller host_init() function
other is to handle error and pass back to dw core code which would then
pass back to controller driver as part of dw_pcie_host_init() return value.

Second option seems more sensible and correct way to fix the issue,
this patch does the same.

As part of this change to host_init() return type I had to patch other
ihost controller drivers which use dw core. Most of the changes to other drivers
are to return proper error codes to upper layer.
Only compile tested drivers.

Changes since RFC:
- Add error handling to other drivers as suggested by Joao Pinto

 drivers/pci/host/pci-dra7xx.c   | 10 --
 drivers/pci/host/pci-exynos.c   | 10 --
 drivers/pci/host/pci-imx6.c | 10 --
 drivers/pci/host/pci-keystone.c | 10 --
 drivers/pci/host/pci-layerscape.c   | 22 +-
 drivers/pci/host/pcie-armada8k.c|  4 +++-
 drivers/pci/host/pcie-designware-plat.c | 10 --
 drivers/pci/host/pcie-designware.c  |  4 +++-
 drivers/pci/host/pcie-designware.h  |  2 +-
 drivers/pci/host/pcie-qcom.c|  5 +++--
 drivers/pci/host/pcie-spear13xx.c   | 10 --
 11 files changed, 71 insertions(+), 26 deletions(-)

diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c
index 9595fad..811f0f9 100644
--- a/drivers/pci/host/pci-dra7xx.c
+++ b/drivers/pci/host/pci-dra7xx.c
@@ -127,9 +127,10 @@ static void dra7xx_pcie_enable_interrupts(struct 
dra7xx_pcie *dra7xx)
   LEG_EP_INTERRUPTS);
 }
 
-static void dra7xx_pcie_host_init(struct pcie_port *pp)
+static int dra7xx_pcie_host_init(struct pcie_port *pp)
 {
struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pp);
+   int ret;
 
pp->io_base &= DRA7XX_CPU_TO_BUS_ADDR;
pp->mem_base &= DRA7XX_CPU_TO_BUS_ADDR;
@@ -138,10 +139,15 @@ static void dra7xx_pcie_host_init(struct pcie_port *pp)
 
dw_pcie_setup_rc(pp);
 
-   dra7xx_pcie_establish_link(dra7xx);
+   ret = dra7xx_pcie_establish_link(dra7xx);
+   if (ret < 0)
+   return ret;
+
if (IS_ENABLED(CONFIG_PCI_MSI))
dw_pcie_msi_init(pp);
dra7xx_pcie_enable_interrupts(dra7xx);
+
+   return 0;
 }
 
 static struct pcie_host_ops dra7xx_pcie_host_ops = {
diff --git a/drivers/pci/host/pci-exynos.c b/drivers/pci/host/pci-exynos.c
index f1c544b..c116fd9 100644
--- a/drivers/pci/host/pci-exynos.c
+++ b/drivers/pci/host/pci-exynos.c
@@ -458,12 +458,18 @@ static int exynos_pcie_link_up(struct pcie_port *pp)
return 0;
 }
 
-static void exynos_pcie_host_init(struct pcie_port *pp)
+static int exynos_pcie_host_init(struct pcie_port *pp)
 {
struct exynos_pcie *exynos_pcie = to_exynos_pcie(pp);
+   int ret;
+
+   ret = exynos_pcie_establish_link(exynos_pcie);
+   if (ret < 0)
+   return ret;
 
-   exynos_pcie_establish_link(exynos_pcie);
exynos_pcie_enable_interrupts(exynos_pcie);
+
+   return 0;
 }
 
 static struct pcie_host_ops exynos_pcie_host_ops = {
diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index c8cefb0..1251e92 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -550,18 +550,24 @@ static int imx6_pcie_establish_link(struct imx6_pcie 
*imx6_pcie)
return ret;
 }
 
-static void imx6_pcie_host_init(struct pcie_port *pp)
+static int imx6_pcie_host_init(struct pcie_port *pp)
 {
struct imx6_pcie *imx6_pcie = to_imx6_pcie(pp);
+   int ret;
 
imx6_pcie_assert_core_reset(imx6_pcie);
imx6_pcie_init_phy(imx6_pcie);
imx6_pcie_deassert_core_reset(imx6_pcie);
dw_pcie_setup_rc(pp);
-   imx6_pcie_establish_link(imx6_pcie);
+   ret = imx6_pcie_establish_link(imx6_pcie);
+
+   if (ret < 0)
+   return ret;
 
if (IS_ENABLED(CONFIG_PCI_MSI))
dw_pcie_msi_init(pp);
+
+