[PATCH v1 0/2] HiSilicon STB PCIe host use bulk API for clock

[PATCH v1 0/2] HiSilicon STB PCIe host use bulk API for clock

[Anand Moon]

Simplify the code using the clk_bulk*() and reset_control_bulk() APIs

Thanks
-Anand

Anand Moon (2):
  PCI: dwc: histb: Simplify clock handling by using clk_bulk*()
    functions
  PCI: dwc: histb: Simplify reset control handling by using
    reset_control_bulk*() function

 drivers/pci/controller/dwc/pcie-histb.c | 119 +++++++-----------------
 1 file changed, 35 insertions(+), 84 deletions(-)


base-commit: fab1beda7597fac1cecc01707d55eadb6bbe773c
-- 
2.50.1

[PATCH v1 1/2] PCI: dwc: histb: Simplify clock handling by using clk_bulk*() functions

[Anand Moon]

Currently, the driver acquires clocks and prepare/enable/disable/unprepare
the clocks individually thereby making the driver complex to read.

The driver can be simplified by using the clk_bulk*() APIs.

Use:
  - devm_clk_bulk_get_all() API to acquire all the clocks
  - clk_bulk_prepare_enable() to prepare/enable clocks
  - clk_bulk_disable_unprepare() APIs to disable/unprepare them in bulk

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 drivers/pci/controller/dwc/pcie-histb.c | 70 ++++---------------------
 1 file changed, 11 insertions(+), 59 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-histb.c b/drivers/pci/controller/dwc/pcie-histb.c
index a52071589377..4022349e85d2 100644
--- a/drivers/pci/controller/dwc/pcie-histb.c
+++ b/drivers/pci/controller/dwc/pcie-histb.c
@@ -51,10 +51,8 @@
 
 struct histb_pcie {
 	struct dw_pcie *pci;
-	struct clk *aux_clk;
-	struct clk *pipe_clk;
-	struct clk *sys_clk;
-	struct clk *bus_clk;
+	struct  clk_bulk_data *clks;
+	int     num_clks;
 	struct phy *phy;
 	struct reset_control *soft_reset;
 	struct reset_control *sys_reset;
@@ -204,10 +202,7 @@ static void histb_pcie_host_disable(struct histb_pcie *hipcie)
 	reset_control_assert(hipcie->sys_reset);
 	reset_control_assert(hipcie->bus_reset);
 
-	clk_disable_unprepare(hipcie->aux_clk);
-	clk_disable_unprepare(hipcie->pipe_clk);
-	clk_disable_unprepare(hipcie->sys_clk);
-	clk_disable_unprepare(hipcie->bus_clk);
+	clk_bulk_disable_unprepare(hipcie->num_clks, hipcie->clks);
 
 	if (hipcie->reset_gpio)
 		gpiod_set_value_cansleep(hipcie->reset_gpio, 1);
@@ -235,28 +230,10 @@ static int histb_pcie_host_enable(struct dw_pcie_rp *pp)
 	if (hipcie->reset_gpio)
 		gpiod_set_value_cansleep(hipcie->reset_gpio, 0);
 
-	ret = clk_prepare_enable(hipcie->bus_clk);
+	ret = clk_bulk_prepare_enable(hipcie->num_clks, hipcie->clks);
 	if (ret) {
-		dev_err(dev, "cannot prepare/enable bus clk\n");
-		goto err_bus_clk;
-	}
-
-	ret = clk_prepare_enable(hipcie->sys_clk);
-	if (ret) {
-		dev_err(dev, "cannot prepare/enable sys clk\n");
-		goto err_sys_clk;
-	}
-
-	ret = clk_prepare_enable(hipcie->pipe_clk);
-	if (ret) {
-		dev_err(dev, "cannot prepare/enable pipe clk\n");
-		goto err_pipe_clk;
-	}
-
-	ret = clk_prepare_enable(hipcie->aux_clk);
-	if (ret) {
-		dev_err(dev, "cannot prepare/enable aux clk\n");
-		goto err_aux_clk;
+		ret = dev_err_probe(dev, ret, "failed to enable clocks\n");
+		goto reg_dis;
 	}
 
 	reset_control_assert(hipcie->soft_reset);
@@ -270,13 +247,7 @@ static int histb_pcie_host_enable(struct dw_pcie_rp *pp)
 
 	return 0;
 
-err_aux_clk:
-	clk_disable_unprepare(hipcie->pipe_clk);
-err_pipe_clk:
-	clk_disable_unprepare(hipcie->sys_clk);
-err_sys_clk:
-	clk_disable_unprepare(hipcie->bus_clk);
-err_bus_clk:
+reg_dis:
 	if (hipcie->vpcie)
 		regulator_disable(hipcie->vpcie);
 
@@ -345,29 +316,10 @@ static int histb_pcie_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	hipcie->aux_clk = devm_clk_get(dev, "aux");
-	if (IS_ERR(hipcie->aux_clk)) {
-		dev_err(dev, "Failed to get PCIe aux clk\n");
-		return PTR_ERR(hipcie->aux_clk);
-	}
-
-	hipcie->pipe_clk = devm_clk_get(dev, "pipe");
-	if (IS_ERR(hipcie->pipe_clk)) {
-		dev_err(dev, "Failed to get PCIe pipe clk\n");
-		return PTR_ERR(hipcie->pipe_clk);
-	}
-
-	hipcie->sys_clk = devm_clk_get(dev, "sys");
-	if (IS_ERR(hipcie->sys_clk)) {
-		dev_err(dev, "Failed to get PCIEe sys clk\n");
-		return PTR_ERR(hipcie->sys_clk);
-	}
-
-	hipcie->bus_clk = devm_clk_get(dev, "bus");
-	if (IS_ERR(hipcie->bus_clk)) {
-		dev_err(dev, "Failed to get PCIe bus clk\n");
-		return PTR_ERR(hipcie->bus_clk);
-	}
+	hipcie->num_clks = devm_clk_bulk_get_all(dev, &hipcie->clks);
+	if (hipcie->num_clks < 0)
+		return dev_err_probe(dev, hipcie->num_clks,
+				     "failed to get clocks\n");
 
 	hipcie->soft_reset = devm_reset_control_get(dev, "soft");
 	if (IS_ERR(hipcie->soft_reset)) {
-- 
2.50.1

Re: [PATCH v1 1/2] PCI: dwc: histb: Simplify clock handling by using clk_bulk*() functions

[Bjorn Helgaas]

In subject, remove "dwc: " to follow historical convention.  (See
"git log --oneline")

On Tue, Aug 26, 2025 at 05:12:40PM +0530, Anand Moon wrote:
> Currently, the driver acquires clocks and prepare/enable/disable/unprepare
> the clocks individually thereby making the driver complex to read.
> 
> The driver can be simplified by using the clk_bulk*() APIs.
> 
> Use:
>   - devm_clk_bulk_get_all() API to acquire all the clocks
>   - clk_bulk_prepare_enable() to prepare/enable clocks
>   - clk_bulk_disable_unprepare() APIs to disable/unprepare them in bulk

I assume this means the order in which we prepare/enable and
disable/unprepare will now depend on the order the clocks appear in
the device tree instead of the order in the code?  If so, please
mention that here and verify that all upstream device trees have the
correct order.

> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>  drivers/pci/controller/dwc/pcie-histb.c | 70 ++++---------------------
>  1 file changed, 11 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-histb.c b/drivers/pci/controller/dwc/pcie-histb.c
> index a52071589377..4022349e85d2 100644
> --- a/drivers/pci/controller/dwc/pcie-histb.c
> +++ b/drivers/pci/controller/dwc/pcie-histb.c
> @@ -51,10 +51,8 @@
>  
>  struct histb_pcie {
>  	struct dw_pcie *pci;
> -	struct clk *aux_clk;
> -	struct clk *pipe_clk;
> -	struct clk *sys_clk;
> -	struct clk *bus_clk;
> +	struct  clk_bulk_data *clks;
> +	int     num_clks;
>  	struct phy *phy;
>  	struct reset_control *soft_reset;
>  	struct reset_control *sys_reset;
> @@ -204,10 +202,7 @@ static void histb_pcie_host_disable(struct histb_pcie *hipcie)
>  	reset_control_assert(hipcie->sys_reset);
>  	reset_control_assert(hipcie->bus_reset);
>  
> -	clk_disable_unprepare(hipcie->aux_clk);
> -	clk_disable_unprepare(hipcie->pipe_clk);
> -	clk_disable_unprepare(hipcie->sys_clk);
> -	clk_disable_unprepare(hipcie->bus_clk);
> +	clk_bulk_disable_unprepare(hipcie->num_clks, hipcie->clks);
>  
>  	if (hipcie->reset_gpio)
>  		gpiod_set_value_cansleep(hipcie->reset_gpio, 1);
> @@ -235,28 +230,10 @@ static int histb_pcie_host_enable(struct dw_pcie_rp *pp)
>  	if (hipcie->reset_gpio)
>  		gpiod_set_value_cansleep(hipcie->reset_gpio, 0);
>  
> -	ret = clk_prepare_enable(hipcie->bus_clk);
> +	ret = clk_bulk_prepare_enable(hipcie->num_clks, hipcie->clks);
>  	if (ret) {
> -		dev_err(dev, "cannot prepare/enable bus clk\n");
> -		goto err_bus_clk;
> -	}
> -
> -	ret = clk_prepare_enable(hipcie->sys_clk);
> -	if (ret) {
> -		dev_err(dev, "cannot prepare/enable sys clk\n");
> -		goto err_sys_clk;
> -	}
> -
> -	ret = clk_prepare_enable(hipcie->pipe_clk);
> -	if (ret) {
> -		dev_err(dev, "cannot prepare/enable pipe clk\n");
> -		goto err_pipe_clk;
> -	}
> -
> -	ret = clk_prepare_enable(hipcie->aux_clk);
> -	if (ret) {
> -		dev_err(dev, "cannot prepare/enable aux clk\n");
> -		goto err_aux_clk;
> +		ret = dev_err_probe(dev, ret, "failed to enable clocks\n");
> +		goto reg_dis;
>  	}
>  
>  	reset_control_assert(hipcie->soft_reset);
> @@ -270,13 +247,7 @@ static int histb_pcie_host_enable(struct dw_pcie_rp *pp)
>  
>  	return 0;
>  
> -err_aux_clk:
> -	clk_disable_unprepare(hipcie->pipe_clk);
> -err_pipe_clk:
> -	clk_disable_unprepare(hipcie->sys_clk);
> -err_sys_clk:
> -	clk_disable_unprepare(hipcie->bus_clk);
> -err_bus_clk:
> +reg_dis:
>  	if (hipcie->vpcie)
>  		regulator_disable(hipcie->vpcie);
>  
> @@ -345,29 +316,10 @@ static int histb_pcie_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	hipcie->aux_clk = devm_clk_get(dev, "aux");
> -	if (IS_ERR(hipcie->aux_clk)) {
> -		dev_err(dev, "Failed to get PCIe aux clk\n");
> -		return PTR_ERR(hipcie->aux_clk);
> -	}
> -
> -	hipcie->pipe_clk = devm_clk_get(dev, "pipe");
> -	if (IS_ERR(hipcie->pipe_clk)) {
> -		dev_err(dev, "Failed to get PCIe pipe clk\n");
> -		return PTR_ERR(hipcie->pipe_clk);
> -	}
> -
> -	hipcie->sys_clk = devm_clk_get(dev, "sys");
> -	if (IS_ERR(hipcie->sys_clk)) {
> -		dev_err(dev, "Failed to get PCIEe sys clk\n");
> -		return PTR_ERR(hipcie->sys_clk);
> -	}
> -
> -	hipcie->bus_clk = devm_clk_get(dev, "bus");
> -	if (IS_ERR(hipcie->bus_clk)) {
> -		dev_err(dev, "Failed to get PCIe bus clk\n");
> -		return PTR_ERR(hipcie->bus_clk);
> -	}
> +	hipcie->num_clks = devm_clk_bulk_get_all(dev, &hipcie->clks);
> +	if (hipcie->num_clks < 0)
> +		return dev_err_probe(dev, hipcie->num_clks,
> +				     "failed to get clocks\n");
>  
>  	hipcie->soft_reset = devm_reset_control_get(dev, "soft");
>  	if (IS_ERR(hipcie->soft_reset)) {
> -- 
> 2.50.1
> 

Re: [PATCH v1 1/2] PCI: dwc: histb: Simplify clock handling by using clk_bulk*() functions

[Anand Moon]

Hi Bjorn,

Thanks for your review comments.
On Tue, 26 Aug 2025 at 21:55, Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> In subject, remove "dwc: " to follow historical convention.  (See
> "git log --oneline")
>
Ok I will keep it as per the git history.

> On Tue, Aug 26, 2025 at 05:12:40PM +0530, Anand Moon wrote:
> > Currently, the driver acquires clocks and prepare/enable/disable/unprepare
> > the clocks individually thereby making the driver complex to read.
> >
> > The driver can be simplified by using the clk_bulk*() APIs.
> >
> > Use:
> >   - devm_clk_bulk_get_all() API to acquire all the clocks
> >   - clk_bulk_prepare_enable() to prepare/enable clocks
> >   - clk_bulk_disable_unprepare() APIs to disable/unprepare them in bulk
>
> I assume this means the order in which we prepare/enable and
> disable/unprepare will now depend on the order the clocks appear in
> the device tree instead of the order in the code?  If so, please
> mention that here and verify that all upstream device trees have the
> correct order.
>
Following is the order in the device tree

       clocks = <&crg HISTB_PCIE_AUX_CLK>,
                                 <&crg HISTB_PCIE_PIPE_CLK>,
                                 <&crg HISTB_PCIE_SYS_CLK>,
                                 <&crg HISTB_PCIE_BUS_CLK>;
                        clock-names = "aux", "pipe", "sys", "bus";

Ok I will update this in the commit message.

Thanks
-Anand

Re: [PATCH v1 1/2] PCI: dwc: histb: Simplify clock handling by using clk_bulk*() functions

[Bjorn Helgaas]

On Tue, Aug 26, 2025 at 11:32:34PM +0530, Anand Moon wrote:
> Hi Bjorn,
> 
> Thanks for your review comments.
> On Tue, 26 Aug 2025 at 21:55, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > In subject, remove "dwc: " to follow historical convention.  (See
> > "git log --oneline")
> >
> Ok I will keep it as per the git history.
> 
> > On Tue, Aug 26, 2025 at 05:12:40PM +0530, Anand Moon wrote:
> > > Currently, the driver acquires clocks and prepare/enable/disable/unprepare
> > > the clocks individually thereby making the driver complex to read.
> > >
> > > The driver can be simplified by using the clk_bulk*() APIs.
> > >
> > > Use:
> > >   - devm_clk_bulk_get_all() API to acquire all the clocks
> > >   - clk_bulk_prepare_enable() to prepare/enable clocks
> > >   - clk_bulk_disable_unprepare() APIs to disable/unprepare them in bulk
> >
> > I assume this means the order in which we prepare/enable and
> > disable/unprepare will now depend on the order the clocks appear in
> > the device tree instead of the order in the code?  If so, please
> > mention that here and verify that all upstream device trees have the
> > correct order.
> >
> Following is the order in the device tree
> 
>        clocks = <&crg HISTB_PCIE_AUX_CLK>,
>                                  <&crg HISTB_PCIE_PIPE_CLK>,
>                                  <&crg HISTB_PCIE_SYS_CLK>,
>                                  <&crg HISTB_PCIE_BUS_CLK>;
>                         clock-names = "aux", "pipe", "sys", "bus";

The current order in the code is:

  histb_pcie_host_enable
    clk_prepare_enable(hipcie->bus_clk);
    clk_prepare_enable(hipcie->sys_clk);
    clk_prepare_enable(hipcie->pipe_clk);
    clk_prepare_enable(hipcie->aux_clk);

  histb_pcie_host_disable
    clk_disable_unprepare(hipcie->aux_clk);
    clk_disable_unprepare(hipcie->pipe_clk);
    clk_disable_unprepare(hipcie->sys_clk);
    clk_disable_unprepare(hipcie->bus_clk);

After this patch, it looks like we'll have:

  histb_pcie_host_enable
    clk_bulk_prepare_enable
      aux
      pipe
      sys
      bus

  histb_pcie_host_disable
    clk_bulk_disable_unprepare
      bus
      sys
      pipe
      aux

So it looks like this patch will reverse the ordering both for
enabling and disabling, right?  I'm totally fine with this as long as
it works.

Bjorn

Re: [PATCH v1 1/2] PCI: dwc: histb: Simplify clock handling by using clk_bulk*() functions

[Anand Moon]

Hi Bjorn,

On Wed, 27 Aug 2025 at 00:32, Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Tue, Aug 26, 2025 at 11:32:34PM +0530, Anand Moon wrote:
> > Hi Bjorn,
> >
> > Thanks for your review comments.
> > On Tue, 26 Aug 2025 at 21:55, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > >
> > > In subject, remove "dwc: " to follow historical convention.  (See
> > > "git log --oneline")
> > >
> > Ok I will keep it as per the git history.
> >
> > > On Tue, Aug 26, 2025 at 05:12:40PM +0530, Anand Moon wrote:
> > > > Currently, the driver acquires clocks and prepare/enable/disable/unprepare
> > > > the clocks individually thereby making the driver complex to read.
> > > >
> > > > The driver can be simplified by using the clk_bulk*() APIs.
> > > >
> > > > Use:
> > > >   - devm_clk_bulk_get_all() API to acquire all the clocks
> > > >   - clk_bulk_prepare_enable() to prepare/enable clocks
> > > >   - clk_bulk_disable_unprepare() APIs to disable/unprepare them in bulk
> > >
> > > I assume this means the order in which we prepare/enable and
> > > disable/unprepare will now depend on the order the clocks appear in
> > > the device tree instead of the order in the code?  If so, please
> > > mention that here and verify that all upstream device trees have the
> > > correct order.
> > >
> > Following is the order in the device tree
> >
> >        clocks = <&crg HISTB_PCIE_AUX_CLK>,
> >                                  <&crg HISTB_PCIE_PIPE_CLK>,
> >                                  <&crg HISTB_PCIE_SYS_CLK>,
> >                                  <&crg HISTB_PCIE_BUS_CLK>;
> >                         clock-names = "aux", "pipe", "sys", "bus";
>
> The current order in the code is:
>
>   histb_pcie_host_enable
>     clk_prepare_enable(hipcie->bus_clk);
>     clk_prepare_enable(hipcie->sys_clk);
>     clk_prepare_enable(hipcie->pipe_clk);
>     clk_prepare_enable(hipcie->aux_clk);
>
>   histb_pcie_host_disable
>     clk_disable_unprepare(hipcie->aux_clk);
>     clk_disable_unprepare(hipcie->pipe_clk);
>     clk_disable_unprepare(hipcie->sys_clk);
>     clk_disable_unprepare(hipcie->bus_clk);
>
> After this patch, it looks like we'll have:
>
>   histb_pcie_host_enable
>     clk_bulk_prepare_enable
>       aux
>       pipe
>       sys
>       bus
>
>   histb_pcie_host_disable
>     clk_bulk_disable_unprepare
>       bus
>       sys
>       pipe
>       aux
>
> So it looks like this patch will reverse the ordering both for
> enabling and disabling, right?  I'm totally fine with this as long as
> it works.
>
Thank you for the input. Let's wait for additional feedback regarding
the changes.
> Bjorn
Thanks
-Anand

[PATCH v1 2/2] PCI: dwc: histb: Simplify reset control handling by using reset_control_bulk*() function

[Anand Moon]

Currently, the driver acquires and asserts/deasserts the resets
individually thereby making the driver complex to read.

This can be simplified by using the reset_control_bulk() APIs.

Use devm_reset_control_bulk_get_exclusive() API to acquire all the resets
and use reset_control_bulk_{assert/deassert}() APIs to assert/deassert them
in bulk.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 drivers/pci/controller/dwc/pcie-histb.c | 57 ++++++++++++-------------
 1 file changed, 28 insertions(+), 29 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-histb.c b/drivers/pci/controller/dwc/pcie-histb.c
index 4022349e85d2..4ba5c9af63a0 100644
--- a/drivers/pci/controller/dwc/pcie-histb.c
+++ b/drivers/pci/controller/dwc/pcie-histb.c
@@ -49,14 +49,20 @@
 #define PCIE_LTSSM_STATE_MASK		GENMASK(5, 0)
 #define PCIE_LTSSM_STATE_ACTIVE		0x11
 
+#define PCIE_HISTB_NUM_RESETS   ARRAY_SIZE(histb_pci_rsts)
+
+static const char * const histb_pci_rsts[] = {
+	"soft",
+	"sys",
+	"bus",
+};
+
 struct histb_pcie {
 	struct dw_pcie *pci;
 	struct  clk_bulk_data *clks;
 	int     num_clks;
 	struct phy *phy;
-	struct reset_control *soft_reset;
-	struct reset_control *sys_reset;
-	struct reset_control *bus_reset;
+	struct  reset_control_bulk_data reset[PCIE_HISTB_NUM_RESETS];
 	void __iomem *ctrl;
 	struct gpio_desc *reset_gpio;
 	struct regulator *vpcie;
@@ -198,9 +204,8 @@ static const struct dw_pcie_host_ops histb_pcie_host_ops = {
 
 static void histb_pcie_host_disable(struct histb_pcie *hipcie)
 {
-	reset_control_assert(hipcie->soft_reset);
-	reset_control_assert(hipcie->sys_reset);
-	reset_control_assert(hipcie->bus_reset);
+	reset_control_bulk_assert(PCIE_HISTB_NUM_RESETS,
+				  hipcie->reset);
 
 	clk_bulk_disable_unprepare(hipcie->num_clks, hipcie->clks);
 
@@ -236,14 +241,19 @@ static int histb_pcie_host_enable(struct dw_pcie_rp *pp)
 		goto reg_dis;
 	}
 
-	reset_control_assert(hipcie->soft_reset);
-	reset_control_deassert(hipcie->soft_reset);
-
-	reset_control_assert(hipcie->sys_reset);
-	reset_control_deassert(hipcie->sys_reset);
+	ret = reset_control_bulk_assert(PCIE_HISTB_NUM_RESETS,
+					hipcie->reset);
+	if (ret) {
+		dev_err(dev, "Couldn't assert reset %d\n", ret);
+		goto reg_dis;
+	}
 
-	reset_control_assert(hipcie->bus_reset);
-	reset_control_deassert(hipcie->bus_reset);
+	ret = reset_control_bulk_deassert(PCIE_HISTB_NUM_RESETS,
+					  hipcie->reset);
+	if (ret) {
+		dev_err(dev, "Couldn't dessert reset %d\n", ret);
+		goto reg_dis;
+	}
 
 	return 0;
 
@@ -321,23 +331,12 @@ static int histb_pcie_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, hipcie->num_clks,
 				     "failed to get clocks\n");
 
-	hipcie->soft_reset = devm_reset_control_get(dev, "soft");
-	if (IS_ERR(hipcie->soft_reset)) {
-		dev_err(dev, "couldn't get soft reset\n");
-		return PTR_ERR(hipcie->soft_reset);
-	}
+	ret = devm_reset_control_bulk_get_exclusive(dev,
+						    PCIE_HISTB_NUM_RESETS,
+						    hipcie->reset);
+	if (ret)
+		return dev_err_probe(dev, ret, "Cannot get the Core resets\n");
 
-	hipcie->sys_reset = devm_reset_control_get(dev, "sys");
-	if (IS_ERR(hipcie->sys_reset)) {
-		dev_err(dev, "couldn't get sys reset\n");
-		return PTR_ERR(hipcie->sys_reset);
-	}
-
-	hipcie->bus_reset = devm_reset_control_get(dev, "bus");
-	if (IS_ERR(hipcie->bus_reset)) {
-		dev_err(dev, "couldn't get bus reset\n");
-		return PTR_ERR(hipcie->bus_reset);
-	}
 
 	hipcie->phy = devm_phy_get(dev, "phy");
 	if (IS_ERR(hipcie->phy)) {
-- 
2.50.1

Re: [PATCH v1 2/2] PCI: dwc: histb: Simplify reset control handling by using reset_control_bulk*() function

[Philipp Zabel]

On Di, 2025-08-26 at 17:12 +0530, Anand Moon wrote:
> Currently, the driver acquires and asserts/deasserts the resets
> individually thereby making the driver complex to read.
> 
> This can be simplified by using the reset_control_bulk() APIs.
> 
> Use devm_reset_control_bulk_get_exclusive() API to acquire all the resets
> and use reset_control_bulk_{assert/deassert}() APIs to assert/deassert them
> in bulk.
> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>  drivers/pci/controller/dwc/pcie-histb.c | 57 ++++++++++++-------------
>  1 file changed, 28 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-histb.c b/drivers/pci/controller/dwc/pcie-histb.c
> index 4022349e85d2..4ba5c9af63a0 100644
> --- a/drivers/pci/controller/dwc/pcie-histb.c
> +++ b/drivers/pci/controller/dwc/pcie-histb.c
> @@ -49,14 +49,20 @@
>  #define PCIE_LTSSM_STATE_MASK		GENMASK(5, 0)
>  #define PCIE_LTSSM_STATE_ACTIVE		0x11
>  
> +#define PCIE_HISTB_NUM_RESETS   ARRAY_SIZE(histb_pci_rsts)
> +
> +static const char * const histb_pci_rsts[] = {
> +	"soft",
> +	"sys",
> +	"bus",
> +};
> +
[...]
> @@ -236,14 +241,19 @@ static int histb_pcie_host_enable(struct dw_pcie_rp *pp)
>  		goto reg_dis;
>  	}
>  
> -	reset_control_assert(hipcie->soft_reset);
> -	reset_control_deassert(hipcie->soft_reset);
> -
> -	reset_control_assert(hipcie->sys_reset);
> -	reset_control_deassert(hipcie->sys_reset);
> +	ret = reset_control_bulk_assert(PCIE_HISTB_NUM_RESETS,
> +					hipcie->reset);
> +	if (ret) {
> +		dev_err(dev, "Couldn't assert reset %d\n", ret);
> +		goto reg_dis;
> +	}
>  
> -	reset_control_assert(hipcie->bus_reset);
> -	reset_control_deassert(hipcie->bus_reset);
> +	ret = reset_control_bulk_deassert(PCIE_HISTB_NUM_RESETS,

Note that this changes the order of assertion/deassertion, not only
because resets lines are now switched in bulk, but also because
reset_control_bulk_deassert() deasserts the reset lines in reverse
order. So this does

If the three resets are independent and order doesn't matter,

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

Re: [PATCH v1 2/2] PCI: dwc: histb: Simplify reset control handling by using reset_control_bulk*() function

[Anand Moon]

Hi Philipp,

Thanks for your review comments.
On Tue, 26 Aug 2025 at 18:16, Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> On Di, 2025-08-26 at 17:12 +0530, Anand Moon wrote:
> > Currently, the driver acquires and asserts/deasserts the resets
> > individually thereby making the driver complex to read.
> >
> > This can be simplified by using the reset_control_bulk() APIs.
> >
> > Use devm_reset_control_bulk_get_exclusive() API to acquire all the resets
> > and use reset_control_bulk_{assert/deassert}() APIs to assert/deassert them
> > in bulk.
> >
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-histb.c | 57 ++++++++++++-------------
> >  1 file changed, 28 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-histb.c b/drivers/pci/controller/dwc/pcie-histb.c
> > index 4022349e85d2..4ba5c9af63a0 100644
> > --- a/drivers/pci/controller/dwc/pcie-histb.c
> > +++ b/drivers/pci/controller/dwc/pcie-histb.c
> > @@ -49,14 +49,20 @@
> >  #define PCIE_LTSSM_STATE_MASK                GENMASK(5, 0)
> >  #define PCIE_LTSSM_STATE_ACTIVE              0x11
> >
> > +#define PCIE_HISTB_NUM_RESETS   ARRAY_SIZE(histb_pci_rsts)
> > +
> > +static const char * const histb_pci_rsts[] = {
> > +     "soft",
> > +     "sys",
> > +     "bus",
> > +};
> > +
> [...]
> > @@ -236,14 +241,19 @@ static int histb_pcie_host_enable(struct dw_pcie_rp *pp)
> >               goto reg_dis;
> >       }
> >
> > -     reset_control_assert(hipcie->soft_reset);
> > -     reset_control_deassert(hipcie->soft_reset);
> > -
> > -     reset_control_assert(hipcie->sys_reset);
> > -     reset_control_deassert(hipcie->sys_reset);
> > +     ret = reset_control_bulk_assert(PCIE_HISTB_NUM_RESETS,
> > +                                     hipcie->reset);
> > +     if (ret) {
> > +             dev_err(dev, "Couldn't assert reset %d\n", ret);
> > +             goto reg_dis;
> > +     }
> >
> > -     reset_control_assert(hipcie->bus_reset);
> > -     reset_control_deassert(hipcie->bus_reset);
> > +     ret = reset_control_bulk_deassert(PCIE_HISTB_NUM_RESETS,
>
> Note that this changes the order of assertion/deassertion, not only
> because resets lines are now switched in bulk, but also because
> reset_control_bulk_deassert() deasserts the reset lines in reverse
> order. So this does
>
> If the three resets are independent and order doesn't matter,
>
I followed the expected reset flow as part of the initialization probe process.
> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
>
> regards
> Philipp
Thanks
-Anand

Re: [PATCH v1 2/2] PCI: dwc: histb: Simplify reset control handling by using reset_control_bulk*() function

[Bjorn Helgaas]

In subject, remove "dwc: " to follow historical convention.

On Tue, Aug 26, 2025 at 05:12:41PM +0530, Anand Moon wrote:
> Currently, the driver acquires and asserts/deasserts the resets
> individually thereby making the driver complex to read.
> 
> This can be simplified by using the reset_control_bulk() APIs.
> 
> Use devm_reset_control_bulk_get_exclusive() API to acquire all the resets
> and use reset_control_bulk_{assert/deassert}() APIs to assert/deassert them
> in bulk.

Please include a note that this changes the order of reset assert and
deassert and explain why this is safe.

> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
>  drivers/pci/controller/dwc/pcie-histb.c | 57 ++++++++++++-------------
>  1 file changed, 28 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-histb.c b/drivers/pci/controller/dwc/pcie-histb.c
> index 4022349e85d2..4ba5c9af63a0 100644
> --- a/drivers/pci/controller/dwc/pcie-histb.c
> +++ b/drivers/pci/controller/dwc/pcie-histb.c
> @@ -49,14 +49,20 @@
>  #define PCIE_LTSSM_STATE_MASK		GENMASK(5, 0)
>  #define PCIE_LTSSM_STATE_ACTIVE		0x11
>  
> +#define PCIE_HISTB_NUM_RESETS   ARRAY_SIZE(histb_pci_rsts)
> +
> +static const char * const histb_pci_rsts[] = {
> +	"soft",
> +	"sys",
> +	"bus",
> +};
> +
>  struct histb_pcie {
>  	struct dw_pcie *pci;
>  	struct  clk_bulk_data *clks;
>  	int     num_clks;
>  	struct phy *phy;
> -	struct reset_control *soft_reset;
> -	struct reset_control *sys_reset;
> -	struct reset_control *bus_reset;
> +	struct  reset_control_bulk_data reset[PCIE_HISTB_NUM_RESETS];
>  	void __iomem *ctrl;
>  	struct gpio_desc *reset_gpio;
>  	struct regulator *vpcie;
> @@ -198,9 +204,8 @@ static const struct dw_pcie_host_ops histb_pcie_host_ops = {
>  
>  static void histb_pcie_host_disable(struct histb_pcie *hipcie)
>  {
> -	reset_control_assert(hipcie->soft_reset);
> -	reset_control_assert(hipcie->sys_reset);
> -	reset_control_assert(hipcie->bus_reset);
> +	reset_control_bulk_assert(PCIE_HISTB_NUM_RESETS,
> +				  hipcie->reset);
>  
>  	clk_bulk_disable_unprepare(hipcie->num_clks, hipcie->clks);
>  
> @@ -236,14 +241,19 @@ static int histb_pcie_host_enable(struct dw_pcie_rp *pp)
>  		goto reg_dis;
>  	}
>  
> -	reset_control_assert(hipcie->soft_reset);
> -	reset_control_deassert(hipcie->soft_reset);
> -
> -	reset_control_assert(hipcie->sys_reset);
> -	reset_control_deassert(hipcie->sys_reset);
> +	ret = reset_control_bulk_assert(PCIE_HISTB_NUM_RESETS,
> +					hipcie->reset);
> +	if (ret) {
> +		dev_err(dev, "Couldn't assert reset %d\n", ret);
> +		goto reg_dis;
> +	}
>  
> -	reset_control_assert(hipcie->bus_reset);
> -	reset_control_deassert(hipcie->bus_reset);
> +	ret = reset_control_bulk_deassert(PCIE_HISTB_NUM_RESETS,
> +					  hipcie->reset);
> +	if (ret) {
> +		dev_err(dev, "Couldn't dessert reset %d\n", ret);

s/dessert/deassert/

> +		goto reg_dis;
> +	}
>  
>  	return 0;
>  
> @@ -321,23 +331,12 @@ static int histb_pcie_probe(struct platform_device *pdev)
>  		return dev_err_probe(dev, hipcie->num_clks,
>  				     "failed to get clocks\n");
>  
> -	hipcie->soft_reset = devm_reset_control_get(dev, "soft");
> -	if (IS_ERR(hipcie->soft_reset)) {
> -		dev_err(dev, "couldn't get soft reset\n");
> -		return PTR_ERR(hipcie->soft_reset);
> -	}
> +	ret = devm_reset_control_bulk_get_exclusive(dev,
> +						    PCIE_HISTB_NUM_RESETS,
> +						    hipcie->reset);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Cannot get the Core resets\n");
>  
> -	hipcie->sys_reset = devm_reset_control_get(dev, "sys");
> -	if (IS_ERR(hipcie->sys_reset)) {
> -		dev_err(dev, "couldn't get sys reset\n");
> -		return PTR_ERR(hipcie->sys_reset);
> -	}
> -
> -	hipcie->bus_reset = devm_reset_control_get(dev, "bus");
> -	if (IS_ERR(hipcie->bus_reset)) {
> -		dev_err(dev, "couldn't get bus reset\n");
> -		return PTR_ERR(hipcie->bus_reset);
> -	}
>  
>  	hipcie->phy = devm_phy_get(dev, "phy");
>  	if (IS_ERR(hipcie->phy)) {
> -- 
> 2.50.1
> 

Re: [PATCH v1 2/2] PCI: dwc: histb: Simplify reset control handling by using reset_control_bulk*() function

[Anand Moon]

Hi Bjorn,

Thanks for your review comments.

On Tue, 26 Aug 2025 at 21:55, Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> In subject, remove "dwc: " to follow historical convention.
>
Ok
> On Tue, Aug 26, 2025 at 05:12:41PM +0530, Anand Moon wrote:
> > Currently, the driver acquires and asserts/deasserts the resets
> > individually thereby making the driver complex to read.
> >
> > This can be simplified by using the reset_control_bulk() APIs.
> >
> > Use devm_reset_control_bulk_get_exclusive() API to acquire all the resets
> > and use reset_control_bulk_{assert/deassert}() APIs to assert/deassert them
> > in bulk.
>
> Please include a note that this changes the order of reset assert and
> deassert and explain why this is safe.
>
I feel the device tree follows the same order as defined in the array.

     resets = <&crg 0x18c 6>, <&crg 0x18c 5>, <&crg 0x18c 4>;
     reset-names = "soft", "sys", "bus";

Ok I will update this in the commit message.

Thanks
-Anand

Re: [PATCH v1 2/2] PCI: dwc: histb: Simplify reset control handling by using reset_control_bulk*() function

[Bjorn Helgaas]

[cc->to: Shawn]

On Tue, Aug 26, 2025 at 11:33:17PM +0530, Anand Moon wrote:
> On Tue, 26 Aug 2025 at 21:55, Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Tue, Aug 26, 2025 at 05:12:41PM +0530, Anand Moon wrote:
> > > Currently, the driver acquires and asserts/deasserts the resets
> > > individually thereby making the driver complex to read.
> > >
> > > This can be simplified by using the reset_control_bulk() APIs.
> > >
> > > Use devm_reset_control_bulk_get_exclusive() API to acquire all the resets
> > > and use reset_control_bulk_{assert/deassert}() APIs to assert/deassert them
> > > in bulk.
> >
> > Please include a note that this changes the order of reset assert and
> > deassert and explain why this is safe.
> >
> I feel the device tree follows the same order as defined in the array.
> 
>      resets = <&crg 0x18c 6>, <&crg 0x18c 5>, <&crg 0x18c 4>;
>      reset-names = "soft", "sys", "bus";
> 
> Ok I will update this in the commit message.

Following the device tree order doesn't necessarily mean that this is
safe.  We know the current order in the code works.  We don't know
that a different order works until that's tested and verified.

These will both need acks from Shawn (pcie-histb.c maintainer).

Re: [PATCH v1 0/2] HiSilicon STB PCIe host use bulk API for clock

[Manivannan Sadhasivam]

On Tue, Aug 26, 2025 at 05:12:39PM GMT, Anand Moon wrote:
> Simplify the code using the clk_bulk*() and reset_control_bulk() APIs
> 

We need either an Ack from Shawn or the Tested-by tag from anyone having access
to the HiSTB boards.

Also, as others explained, you need to mention the potential order reversing
while switching to these APIs as they may break the driver.

- Mani

> Thanks
> -Anand
> 
> Anand Moon (2):
>   PCI: dwc: histb: Simplify clock handling by using clk_bulk*()
>     functions
>   PCI: dwc: histb: Simplify reset control handling by using
>     reset_control_bulk*() function
> 
>  drivers/pci/controller/dwc/pcie-histb.c | 119 +++++++-----------------
>  1 file changed, 35 insertions(+), 84 deletions(-)
> 
> 
> base-commit: fab1beda7597fac1cecc01707d55eadb6bbe773c
> -- 
> 2.50.1
> 

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