[V4,4/6] PCI: tegra: Continue unconfig sequence even if parts fail

Currently the driver checks for error value of different APIs during the
uninitialization sequence. It just returns from there if there is any error
observed for one of those calls. Comparatively it is better to continue the
uninitialization sequence irrespective of whether some of them are
returning error. That way, it is more closer to complete uninitialization.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
V4:
* None

V3:
* Modified subject as per Bjorn's suggestion
* Removed tegra_pcie_init_controller()'s error checking part and pushed
  a separate patch for it

V2:
* None

 drivers/pci/controller/dwc/pcie-tegra194.c | 39 +++++++++-------------
 1 file changed, 15 insertions(+), 24 deletions(-)

On Mon, Nov 09, 2020 at 10:49:35PM +0530, Vidya Sagar wrote:
> Currently the driver checks for error value of different APIs during the
> uninitialization sequence. It just returns from there if there is any error
> observed for one of those calls. Comparatively it is better to continue the
> uninitialization sequence irrespective of whether some of them are
> returning error. That way, it is more closer to complete uninitialization.
> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
> V4:
> * None
> 
> V3:
> * Modified subject as per Bjorn's suggestion
> * Removed tegra_pcie_init_controller()'s error checking part and pushed
>   a separate patch for it
> 
> V2:
> * None
> 
>  drivers/pci/controller/dwc/pcie-tegra194.c | 39 +++++++++-------------
>  1 file changed, 15 insertions(+), 24 deletions(-)

Tested-by: Thierry Reding <treding@nvidia.com>
Acked-by: Thierry Reding <treding@nvidia.com>

On Mon, Nov 09, 2020 at 10:49:35PM +0530, Vidya Sagar wrote:
> Currently the driver checks for error value of different APIs during the
> uninitialization sequence. It just returns from there if there is any error
> observed for one of those calls. Comparatively it is better to continue the
> uninitialization sequence irrespective of whether some of them are
> returning error. That way, it is more closer to complete uninitialization.

Hi Vidya, Thierry,

I can apply this series (dropping patches as suggested by Thierry),
before though I wanted to ask you if this patch is really an
improvement, it is hard to understand why skipping some error
codes is OK for device correct operations to continue, maybe it
is worth describing why some of those failures aren't really
fatal.

Please let me know, thanks.

Lorenzo

> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
> V4:
> * None
> 
> V3:
> * Modified subject as per Bjorn's suggestion
> * Removed tegra_pcie_init_controller()'s error checking part and pushed
>   a separate patch for it
> 
> V2:
> * None
> 
>  drivers/pci/controller/dwc/pcie-tegra194.c | 39 +++++++++-------------
>  1 file changed, 15 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> index 253d91033bc3..9be10c8953df 100644
> --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> @@ -1422,43 +1422,32 @@ static int tegra_pcie_config_controller(struct tegra_pcie_dw *pcie,
>  	return ret;
>  }
>  
> -static int __deinit_controller(struct tegra_pcie_dw *pcie)
> +static void tegra_pcie_unconfig_controller(struct tegra_pcie_dw *pcie)
>  {
>  	int ret;
>  
>  	ret = reset_control_assert(pcie->core_rst);
> -	if (ret) {
> -		dev_err(pcie->dev, "Failed to assert \"core\" reset: %d\n",
> -			ret);
> -		return ret;
> -	}
> +	if (ret)
> +		dev_err(pcie->dev, "Failed to assert \"core\" reset: %d\n", ret);
>  
>  	tegra_pcie_disable_phy(pcie);
>  
>  	ret = reset_control_assert(pcie->core_apb_rst);
> -	if (ret) {
> +	if (ret)
>  		dev_err(pcie->dev, "Failed to assert APB reset: %d\n", ret);
> -		return ret;
> -	}
>  
>  	clk_disable_unprepare(pcie->core_clk);
>  
>  	ret = regulator_disable(pcie->pex_ctl_supply);
> -	if (ret) {
> +	if (ret)
>  		dev_err(pcie->dev, "Failed to disable regulator: %d\n", ret);
> -		return ret;
> -	}
>  
>  	tegra_pcie_disable_slot_regulators(pcie);
>  
>  	ret = tegra_pcie_bpmp_set_ctrl_state(pcie, false);
> -	if (ret) {
> +	if (ret)
>  		dev_err(pcie->dev, "Failed to disable controller %d: %d\n",
>  			pcie->cid, ret);
> -		return ret;
> -	}
> -
> -	return ret;
>  }
>  
>  static int tegra_pcie_init_controller(struct tegra_pcie_dw *pcie)
> @@ -1482,7 +1471,8 @@ static int tegra_pcie_init_controller(struct tegra_pcie_dw *pcie)
>  	return 0;
>  
>  fail_host_init:
> -	return __deinit_controller(pcie);
> +	tegra_pcie_unconfig_controller(pcie);
> +	return ret;
>  }
>  
>  static int tegra_pcie_try_link_l2(struct tegra_pcie_dw *pcie)
> @@ -1551,13 +1541,12 @@ static void tegra_pcie_dw_pme_turnoff(struct tegra_pcie_dw *pcie)
>  	appl_writel(pcie, data, APPL_PINMUX);
>  }
>  
> -static int tegra_pcie_deinit_controller(struct tegra_pcie_dw *pcie)
> +static void tegra_pcie_deinit_controller(struct tegra_pcie_dw *pcie)
>  {
>  	tegra_pcie_downstream_dev_to_D0(pcie);
>  	dw_pcie_host_deinit(&pcie->pci.pp);
>  	tegra_pcie_dw_pme_turnoff(pcie);
> -
> -	return __deinit_controller(pcie);
> +	tegra_pcie_unconfig_controller(pcie);
>  }
>  
>  static int tegra_pcie_config_rp(struct tegra_pcie_dw *pcie)
> @@ -2238,8 +2227,9 @@ static int tegra_pcie_dw_suspend_noirq(struct device *dev)
>  					       PORT_LOGIC_MSI_CTRL_INT_0_EN);
>  	tegra_pcie_downstream_dev_to_D0(pcie);
>  	tegra_pcie_dw_pme_turnoff(pcie);
> +	tegra_pcie_unconfig_controller(pcie);
>  
> -	return __deinit_controller(pcie);
> +	return 0;
>  }
>  
>  static int tegra_pcie_dw_resume_noirq(struct device *dev)
> @@ -2267,7 +2257,8 @@ static int tegra_pcie_dw_resume_noirq(struct device *dev)
>  	return 0;
>  
>  fail_host_init:
> -	return __deinit_controller(pcie);
> +	tegra_pcie_unconfig_controller(pcie);
> +	return ret;
>  }
>  
>  static int tegra_pcie_dw_resume_early(struct device *dev)
> @@ -2305,7 +2296,7 @@ static void tegra_pcie_dw_shutdown(struct platform_device *pdev)
>  		disable_irq(pcie->pci.pp.msi_irq);
>  
>  	tegra_pcie_dw_pme_turnoff(pcie);
> -	__deinit_controller(pcie);
> +	tegra_pcie_unconfig_controller(pcie);
>  }
>  
>  static const struct tegra_pcie_dw_of_data tegra_pcie_dw_rc_of_data = {
> -- 
> 2.17.1
>

On Mon, Nov 30, 2020 at 12:10:07PM +0000, Lorenzo Pieralisi wrote:
> On Mon, Nov 09, 2020 at 10:49:35PM +0530, Vidya Sagar wrote:
> > Currently the driver checks for error value of different APIs during the
> > uninitialization sequence. It just returns from there if there is any error
> > observed for one of those calls. Comparatively it is better to continue the
> > uninitialization sequence irrespective of whether some of them are
> > returning error. That way, it is more closer to complete uninitialization.
> 
> Hi Vidya, Thierry,
> 
> I can apply this series (dropping patches as suggested by Thierry),
> before though I wanted to ask you if this patch is really an
> improvement, it is hard to understand why skipping some error
> codes is OK for device correct operations to continue, maybe it
> is worth describing why some of those failures aren't really
> fatal.
> 
> Please let me know, thanks.

As explained in the commit message, the idea is to continue tearing
down even if things fail somewhere in the middle, because that ensures
that the hardware gets as close to an "uninitialized" state as possible.
If for example the first reset assert were to fail, then none of the
PHYs get disabled, the regulator stays on and the clocks stays on, all
of which can continue draining power after the controller has already
been torn down.

So yes, I think this is an improvement. It's unclear to me what you're
asking for, though. Would you rather have a comment somewhere near the
tegra_pcie_unconfig_controller() function that states the same thing as
the commit message?

Thierry

On Tue, Dec 01, 2020 at 03:24:24PM +0100, Thierry Reding wrote:
> On Mon, Nov 30, 2020 at 12:10:07PM +0000, Lorenzo Pieralisi wrote:
> > On Mon, Nov 09, 2020 at 10:49:35PM +0530, Vidya Sagar wrote:
> > > Currently the driver checks for error value of different APIs during the
> > > uninitialization sequence. It just returns from there if there is any error
> > > observed for one of those calls. Comparatively it is better to continue the
> > > uninitialization sequence irrespective of whether some of them are
> > > returning error. That way, it is more closer to complete uninitialization.
> > 
> > Hi Vidya, Thierry,
> > 
> > I can apply this series (dropping patches as suggested by Thierry),
> > before though I wanted to ask you if this patch is really an
> > improvement, it is hard to understand why skipping some error
> > codes is OK for device correct operations to continue, maybe it
> > is worth describing why some of those failures aren't really
> > fatal.
> > 
> > Please let me know, thanks.
> 
> As explained in the commit message, the idea is to continue tearing
> down even if things fail somewhere in the middle, because that ensures
> that the hardware gets as close to an "uninitialized" state as possible.
> If for example the first reset assert were to fail, then none of the
> PHYs get disabled, the regulator stays on and the clocks stays on, all
> of which can continue draining power after the controller has already
> been torn down.

Understood. By reading the code it looked weird that eg a reset failure
was tolerable - I thought an error would be fatal (I don't know what are
the consequences for instance on a subsequent resume) but it looks like
it actually is not, that's the only point I raised.

> So yes, I think this is an improvement. It's unclear to me what you're
> asking for, though. Would you rather have a comment somewhere near the
> tegra_pcie_unconfig_controller() function that states the same thing as
> the commit message?

It could be useful but it is up to you, I will merge the series as-is
or I can add it myself, as you wish.

Thanks,
Lorenzo