[PATCH] pci: rockchip: Release resources on failing probe

[Kever Yang]

On 2023/7/12 07:13, Jonas Karlman wrote:
> The PCIe driver for RK3399 is affected by a similar issue that was fixed
> for RK35xx in the commit e04b67a7f4c1 ("pci: pcie_dw_rockchip: release
> resources on failing probe").
>
> Resources are not released on failing probe, e.g. regulators may be left
> enabled and the ep-gpio may be left in a requested state.
>
> Change to use regulator_set_enable_if_allowed and disable regulators
> after failure to keep regulator enable count balanced, ep-gpio is also
> released on regulator failure.
>
> Also add support for the vpcie12v-supply, remove unused include and
> check return value from dev_read_addr_name.
>
> Signed-off-by: Jonas Karlman <jonas at kwiboo.se>
Reviewed-by: Kever Yang <kever.yang at rock-chips.com>

Thanks,
- Kever
> ---
>   drivers/pci/pcie_rockchip.c | 108 +++++++++++++++++++-----------------
>   1 file changed, 57 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/pci/pcie_rockchip.c b/drivers/pci/pcie_rockchip.c
> index 72b41398f27b..624841e9d8b8 100644
> --- a/drivers/pci/pcie_rockchip.c
> +++ b/drivers/pci/pcie_rockchip.c
> @@ -12,23 +12,15 @@
>    */
>   
>   #include <common.h>
> -#include <clk.h>
>   #include <dm.h>
> -#include <asm/global_data.h>
>   #include <dm/device_compat.h>
>   #include <generic-phy.h>
>   #include <pci.h>
> -#include <power-domain.h>
>   #include <power/regulator.h>
>   #include <reset.h>
> -#include <syscon.h>
> -#include <asm/io.h>
>   #include <asm-generic/gpio.h>
> -#include <asm/arch-rockchip/clock.h>
>   #include <linux/iopoll.h>
>   
> -DECLARE_GLOBAL_DATA_PTR;
> -
>   #define HIWORD_UPDATE(mask, val)        (((mask) << 16) | (val))
>   #define HIWORD_UPDATE_BIT(val)          HIWORD_UPDATE(val, val)
>   
> @@ -383,41 +375,38 @@ static int rockchip_pcie_set_vpcie(struct udevice *dev)
>   	struct rockchip_pcie *priv = dev_get_priv(dev);
>   	int ret;
>   
> -	if (priv->vpcie3v3) {
> -		ret = regulator_set_enable(priv->vpcie3v3, true);
> -		if (ret) {
> -			dev_err(dev, "failed to enable vpcie3v3 (ret=%d)\n",
> -				ret);
> -			return ret;
> -		}
> +	ret = regulator_set_enable_if_allowed(priv->vpcie12v, true);
> +	if (ret && ret != -ENOSYS) {
> +		dev_err(dev, "failed to enable vpcie12v (ret=%d)\n", ret);
> +		return ret;
>   	}
>   
> -	if (priv->vpcie1v8) {
> -		ret = regulator_set_enable(priv->vpcie1v8, true);
> -		if (ret) {
> -			dev_err(dev, "failed to enable vpcie1v8 (ret=%d)\n",
> -				ret);
> -			goto err_disable_3v3;
> -		}
> +	ret = regulator_set_enable_if_allowed(priv->vpcie3v3, true);
> +	if (ret && ret != -ENOSYS) {
> +		dev_err(dev, "failed to enable vpcie3v3 (ret=%d)\n", ret);
> +		goto err_disable_12v;
>   	}
>   
> -	if (priv->vpcie0v9) {
> -		ret = regulator_set_enable(priv->vpcie0v9, true);
> -		if (ret) {
> -			dev_err(dev, "failed to enable vpcie0v9 (ret=%d)\n",
> -				ret);
> -			goto err_disable_1v8;
> -		}
> +	ret = regulator_set_enable_if_allowed(priv->vpcie1v8, true);
> +	if (ret && ret != -ENOSYS) {
> +		dev_err(dev, "failed to enable vpcie1v8 (ret=%d)\n", ret);
> +		goto err_disable_3v3;
> +	}
> +
> +	ret = regulator_set_enable_if_allowed(priv->vpcie0v9, true);
> +	if (ret && ret != -ENOSYS) {
> +		dev_err(dev, "failed to enable vpcie0v9 (ret=%d)\n", ret);
> +		goto err_disable_1v8;
>   	}
>   
>   	return 0;
>   
>   err_disable_1v8:
> -	if (priv->vpcie1v8)
> -		regulator_set_enable(priv->vpcie1v8, false);
> +	regulator_set_enable_if_allowed(priv->vpcie1v8, false);
>   err_disable_3v3:
> -	if (priv->vpcie3v3)
> -		regulator_set_enable(priv->vpcie3v3, false);
> +	regulator_set_enable_if_allowed(priv->vpcie3v3, false);
> +err_disable_12v:
> +	regulator_set_enable_if_allowed(priv->vpcie12v, false);
>   	return ret;
>   }
>   
> @@ -427,19 +416,12 @@ static int rockchip_pcie_parse_dt(struct udevice *dev)
>   	int ret;
>   
>   	priv->axi_base = dev_read_addr_name(dev, "axi-base");
> -	if (!priv->axi_base)
> -		return -ENODEV;
> +	if (priv->axi_base == FDT_ADDR_T_NONE)
> +		return -EINVAL;
>   
>   	priv->apb_base = dev_read_addr_name(dev, "apb-base");
> -	if (!priv->axi_base)
> -		return -ENODEV;
> -
> -	ret = gpio_request_by_name(dev, "ep-gpios", 0,
> -				   &priv->ep_gpio, GPIOD_IS_OUT);
> -	if (ret) {
> -		dev_err(dev, "failed to find ep-gpios property\n");
> -		return ret;
> -	}
> +	if (priv->apb_base == FDT_ADDR_T_NONE)
> +		return -EINVAL;
>   
>   	ret = reset_get_by_name(dev, "core", &priv->core_rst);
>   	if (ret) {
> @@ -483,6 +465,13 @@ static int rockchip_pcie_parse_dt(struct udevice *dev)
>   		return ret;
>   	}
>   
> +	ret = device_get_supply_regulator(dev, "vpcie12v-supply",
> +					  &priv->vpcie12v);
> +	if (ret && ret != -ENOENT) {
> +		dev_err(dev, "failed to get vpcie12v supply (ret=%d)\n", ret);
> +		return ret;
> +	}
> +
>   	ret = device_get_supply_regulator(dev, "vpcie3v3-supply",
>   					  &priv->vpcie3v3);
>   	if (ret && ret != -ENOENT) {
> @@ -510,6 +499,13 @@ static int rockchip_pcie_parse_dt(struct udevice *dev)
>   		return ret;
>   	}
>   
> +	ret = gpio_request_by_name(dev, "ep-gpios", 0,
> +				   &priv->ep_gpio, GPIOD_IS_OUT);
> +	if (ret) {
> +		dev_err(dev, "failed to find ep-gpios property\n");
> +		return ret;
> +	}
> +
>   	return 0;
>   }
>   
> @@ -529,16 +525,26 @@ static int rockchip_pcie_probe(struct udevice *dev)
>   
>   	ret = rockchip_pcie_set_vpcie(dev);
>   	if (ret)
> -		return ret;
> +		goto err_gpio_free;
>   
>   	ret = rockchip_pcie_init_port(dev);
>   	if (ret)
> -		return ret;
> +		goto err_disable_vpcie;
>   
>   	dev_info(dev, "PCIE-%d: Link up (Bus%d)\n",
>   		 dev_seq(dev), hose->first_busno);
>   
>   	return 0;
> +
> +err_disable_vpcie:
> +	regulator_set_enable_if_allowed(priv->vpcie0v9, false);
> +	regulator_set_enable_if_allowed(priv->vpcie1v8, false);
> +	regulator_set_enable_if_allowed(priv->vpcie3v3, false);
> +	regulator_set_enable_if_allowed(priv->vpcie12v, false);
> +err_gpio_free:
> +	if (dm_gpio_is_valid(&priv->ep_gpio))
> +		dm_gpio_free(dev, &priv->ep_gpio);
> +	return ret;
>   }
>   
>   static const struct dm_pci_ops rockchip_pcie_ops = {
> @@ -552,10 +558,10 @@ static const struct udevice_id rockchip_pcie_ids[] = {
>   };
>   
>   U_BOOT_DRIVER(rockchip_pcie) = {
> -	.name			= "rockchip_pcie",
> -	.id			= UCLASS_PCI,
> -	.of_match		= rockchip_pcie_ids,
> -	.ops			= &rockchip_pcie_ops,
> -	.probe			= rockchip_pcie_probe,
> +	.name		= "rockchip_pcie",
> +	.id		= UCLASS_PCI,
> +	.of_match	= rockchip_pcie_ids,
> +	.ops		= &rockchip_pcie_ops,
> +	.probe		= rockchip_pcie_probe,
>   	.priv_auto	= sizeof(struct rockchip_pcie),
>   };