Re: [PATCH v4 2/3] PCI: rockchip: Simplify reset control handling by using reset_control_bulk*() function

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Tue, Jun 25, 2024 at 04:10:33PM +0530, Anand Moon wrote:
> Refactor the reset control handling in the Rockchip PCIe driver,
> introducing a more robust and efficient method for assert and
> deassert reset controller using reset_control_bulk*() API. Using the
> reset_control_bulk APIs, the reset handling for the core clocks reset
> unit becomes much simpler.
> 
> As per rockchip rk3399 TRM SOFTRST_CON8 soft reset controller
> have clock reset unit value set to 0x1 for example "pcie_pipe",
> "pcie_mgmt_sticky", "pcie_mgmt" and "pci_core", hence group then under
> one reset bulk controller.
> 
> Where as "pcie_pm", "presetn_pcie", "aresetn_pcie" have reset value
> set to 0x0, hence group them under reset control bulk controller.
> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
> v4: use dev_err_probe in error path.
> v3: Fix typo in commit message, dropped reported by.
> v2: Fix compilation error reported by Intel test robot
>     fixed checkpatch warning
> ---
>  drivers/pci/controller/pcie-rockchip.c | 149 +++++--------------------
>  drivers/pci/controller/pcie-rockchip.h |  25 +++--
>  2 files changed, 47 insertions(+), 127 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-rockchip.c b/drivers/pci/controller/pcie-rockchip.c
> index 804135511528..024308bb6ac8 100644
> --- a/drivers/pci/controller/pcie-rockchip.c
> +++ b/drivers/pci/controller/pcie-rockchip.c
> @@ -69,55 +69,23 @@ int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
>  	if (rockchip->link_gen < 0 || rockchip->link_gen > 2)
>  		rockchip->link_gen = 2;
>  
> -	rockchip->core_rst = devm_reset_control_get_exclusive(dev, "core");
> -	if (IS_ERR(rockchip->core_rst)) {
> -		if (PTR_ERR(rockchip->core_rst) != -EPROBE_DEFER)
> -			dev_err(dev, "missing core reset property in node\n");
> -		return PTR_ERR(rockchip->core_rst);
> -	}
> -
> -	rockchip->mgmt_rst = devm_reset_control_get_exclusive(dev, "mgmt");
> -	if (IS_ERR(rockchip->mgmt_rst)) {
> -		if (PTR_ERR(rockchip->mgmt_rst) != -EPROBE_DEFER)
> -			dev_err(dev, "missing mgmt reset property in node\n");
> -		return PTR_ERR(rockchip->mgmt_rst);
> -	}
> -
> -	rockchip->mgmt_sticky_rst = devm_reset_control_get_exclusive(dev,
> -								"mgmt-sticky");
> -	if (IS_ERR(rockchip->mgmt_sticky_rst)) {
> -		if (PTR_ERR(rockchip->mgmt_sticky_rst) != -EPROBE_DEFER)
> -			dev_err(dev, "missing mgmt-sticky reset property in node\n");
> -		return PTR_ERR(rockchip->mgmt_sticky_rst);
> -	}
> -
> -	rockchip->pipe_rst = devm_reset_control_get_exclusive(dev, "pipe");
> -	if (IS_ERR(rockchip->pipe_rst)) {
> -		if (PTR_ERR(rockchip->pipe_rst) != -EPROBE_DEFER)
> -			dev_err(dev, "missing pipe reset property in node\n");
> -		return PTR_ERR(rockchip->pipe_rst);
> -	}
> +	for (i = 0; i < ROCKCHIP_NUM_PM_RSTS; i++)
> +		rockchip->pm_rsts[i].id = rockchip_pci_pm_rsts[i];
>  
> -	rockchip->pm_rst = devm_reset_control_get_exclusive(dev, "pm");
> -	if (IS_ERR(rockchip->pm_rst)) {
> -		if (PTR_ERR(rockchip->pm_rst) != -EPROBE_DEFER)
> -			dev_err(dev, "missing pm reset property in node\n");
> -		return PTR_ERR(rockchip->pm_rst);
> -	}
> +	err = devm_reset_control_bulk_get_optional_exclusive(dev,
> +							     ROCKCHIP_NUM_PM_RSTS,
> +							     rockchip->pm_rsts);
> +	if (err)
> +		return dev_err_probe(dev, err, "cannot get the reset control\n");
>  
> -	rockchip->pclk_rst = devm_reset_control_get_exclusive(dev, "pclk");
> -	if (IS_ERR(rockchip->pclk_rst)) {
> -		if (PTR_ERR(rockchip->pclk_rst) != -EPROBE_DEFER)
> -			dev_err(dev, "missing pclk reset property in node\n");
> -		return PTR_ERR(rockchip->pclk_rst);
> -	}
> +	for (i = 0; i < ROCKCHIP_NUM_CORE_RSTS; i++)
> +		rockchip->core_rsts[i].id = rockchip_pci_core_rsts[i];
>  
> -	rockchip->aclk_rst = devm_reset_control_get_exclusive(dev, "aclk");
> -	if (IS_ERR(rockchip->aclk_rst)) {
> -		if (PTR_ERR(rockchip->aclk_rst) != -EPROBE_DEFER)
> -			dev_err(dev, "missing aclk reset property in node\n");
> -		return PTR_ERR(rockchip->aclk_rst);
> -	}
> +	err = devm_reset_control_bulk_get_optional_exclusive(dev,
> +							     ROCKCHIP_NUM_CORE_RSTS,
> +							     rockchip->core_rsts);
> +	if (err)
> +		return dev_err_probe(dev, err, "cannot get the reset control\n");
>  
>  	if (rockchip->is_rc) {
>  		rockchip->ep_gpio = devm_gpiod_get_optional(dev, "ep",
> @@ -150,23 +118,10 @@ int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
>  	int err, i;
>  	u32 regs;
>  
> -	err = reset_control_assert(rockchip->aclk_rst);
> -	if (err) {
> -		dev_err(dev, "assert aclk_rst err %d\n", err);
> -		return err;
> -	}
> -
> -	err = reset_control_assert(rockchip->pclk_rst);
> -	if (err) {
> -		dev_err(dev, "assert pclk_rst err %d\n", err);
> -		return err;
> -	}
> -
> -	err = reset_control_assert(rockchip->pm_rst);
> -	if (err) {
> -		dev_err(dev, "assert pm_rst err %d\n", err);
> -		return err;
> -	}
> +	err = reset_control_bulk_assert(ROCKCHIP_NUM_PM_RSTS,
> +					rockchip->pm_rsts);
> +	if (err)
> +		return dev_err_probe(dev, err, "reset bulk assert pm reset\n");
>  
>  	for (i = 0; i < MAX_LANE_NUM; i++) {
>  		err = phy_init(rockchip->phys[i]);
> @@ -176,47 +131,17 @@ int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
>  		}
>  	}
>  
> -	err = reset_control_assert(rockchip->core_rst);
> -	if (err) {
> -		dev_err(dev, "assert core_rst err %d\n", err);
> -		goto err_exit_phy;
> -	}
> -
> -	err = reset_control_assert(rockchip->mgmt_rst);
> -	if (err) {
> -		dev_err(dev, "assert mgmt_rst err %d\n", err);
> -		goto err_exit_phy;
> -	}
> -
> -	err = reset_control_assert(rockchip->mgmt_sticky_rst);
> -	if (err) {
> -		dev_err(dev, "assert mgmt_sticky_rst err %d\n", err);
> -		goto err_exit_phy;
> -	}
> -
> -	err = reset_control_assert(rockchip->pipe_rst);
> -	if (err) {
> -		dev_err(dev, "assert pipe_rst err %d\n", err);
> -		goto err_exit_phy;
> -	}
> +	err = reset_control_bulk_assert(ROCKCHIP_NUM_CORE_RSTS,
> +					rockchip->core_rsts);
> +	if (err)
> +		return dev_err_probe(dev, err, "reset bulk assert core reset\n");
>  
>  	udelay(10);
>  
> -	err = reset_control_deassert(rockchip->pm_rst);
> -	if (err) {
> -		dev_err(dev, "deassert pm_rst err %d\n", err);
> -		goto err_exit_phy;
> -	}
> -
> -	err = reset_control_deassert(rockchip->aclk_rst);
> -	if (err) {
> -		dev_err(dev, "deassert aclk_rst err %d\n", err);
> -		goto err_exit_phy;
> -	}
> -
> -	err = reset_control_deassert(rockchip->pclk_rst);
> +	err = reset_control_bulk_deassert(ROCKCHIP_NUM_PM_RSTS,
> +					  rockchip->pm_rsts);
>  	if (err) {
> -		dev_err(dev, "deassert pclk_rst err %d\n", err);
> +		dev_err(dev, "reset bulk deassert pm err %d\n", err);
>  		goto err_exit_phy;
>  	}
>  
> @@ -259,31 +184,15 @@ int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
>  	 * Please don't reorder the deassert sequence of the following
>  	 * four reset pins.
>  	 */

The comment above says that the resets should not be reordered. But you have
reordered the resets.

- Mani

> -	err = reset_control_deassert(rockchip->mgmt_sticky_rst);
> -	if (err) {
> -		dev_err(dev, "deassert mgmt_sticky_rst err %d\n", err);
> -		goto err_power_off_phy;
> -	}
> -
> -	err = reset_control_deassert(rockchip->core_rst);
> +	err = reset_control_bulk_deassert(ROCKCHIP_NUM_CORE_RSTS,
> +					  rockchip->core_rsts);
>  	if (err) {
> -		dev_err(dev, "deassert core_rst err %d\n", err);
> -		goto err_power_off_phy;
> -	}
> -
> -	err = reset_control_deassert(rockchip->mgmt_rst);
> -	if (err) {
> -		dev_err(dev, "deassert mgmt_rst err %d\n", err);
> -		goto err_power_off_phy;
> -	}
> -
> -	err = reset_control_deassert(rockchip->pipe_rst);
> -	if (err) {
> -		dev_err(dev, "deassert pipe_rst err %d\n", err);
> +		dev_err(dev, "reset bulk deassert core err %d\n", err);
>  		goto err_power_off_phy;
>  	}
>  
>  	return 0;
> +
>  err_power_off_phy:
>  	while (i--)
>  		phy_power_off(rockchip->phys[i]);
> diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/controller/pcie-rockchip.h
> index 72346e17e45e..27e951b41b80 100644
> --- a/drivers/pci/controller/pcie-rockchip.h
> +++ b/drivers/pci/controller/pcie-rockchip.h
> @@ -15,6 +15,7 @@
>  #include <linux/kernel.h>
>  #include <linux/pci.h>
>  #include <linux/pci-ecam.h>
> +#include <linux/reset.h>
>  
>  /*
>   * The upper 16 bits of PCIE_CLIENT_CONFIG are a write mask for the lower 16
> @@ -289,6 +290,8 @@
>  		 ROCKCHIP_PCIE_CORE_EP_FUNC_BAR_CFG_BAR_CTRL_MASK(b))
>  
>  #define ROCKCHIP_NUM_CLKS	ARRAY_SIZE(rockchip_pci_clks)
> +#define ROCKCHIP_NUM_PM_RSTS	ARRAY_SIZE(rockchip_pci_pm_rsts)
> +#define ROCKCHIP_NUM_CORE_RSTS	ARRAY_SIZE(rockchip_pci_core_rsts)
>  
>  static const char * const rockchip_pci_clks[] = {
>  	"aclk",
> @@ -297,18 +300,26 @@ static const char * const rockchip_pci_clks[] = {
>  	"pm",
>  };
>  
> +static const char * const rockchip_pci_pm_rsts[] = {
> +	"pm",
> +	"pclk",
> +	"aclk",
> +};
> +
> +static const char * const rockchip_pci_core_rsts[] = {
> +	"core",
> +	"mgmt",
> +	"mgmt-sticky",
> +	"pipe",
> +};
> +
>  struct rockchip_pcie {
>  	void	__iomem *reg_base;		/* DT axi-base */
>  	void	__iomem *apb_base;		/* DT apb-base */
>  	bool    legacy_phy;
>  	struct  phy *phys[MAX_LANE_NUM];
> -	struct	reset_control *core_rst;
> -	struct	reset_control *mgmt_rst;
> -	struct	reset_control *mgmt_sticky_rst;
> -	struct	reset_control *pipe_rst;
> -	struct	reset_control *pm_rst;
> -	struct	reset_control *aclk_rst;
> -	struct	reset_control *pclk_rst;
> +	struct  reset_control_bulk_data pm_rsts[ROCKCHIP_NUM_PM_RSTS];
> +	struct  reset_control_bulk_data core_rsts[ROCKCHIP_NUM_CORE_RSTS];
>  	struct  clk_bulk_data clks[ROCKCHIP_NUM_CLKS];
>  	struct	regulator *vpcie12v; /* 12V power supply */
>  	struct	regulator *vpcie3v3; /* 3.3V power supply */
> -- 
> 2.44.0
> 
> 

-- 
மணிவண்ணன் சதாசிவம்