[PATCH 0/4] PCI: qcom: use clk_bulk API

[PATCH 0/4] PCI: qcom: use clk_bulk API

[Dmitry Baryshkov]

Replace open-coded clk iterations with clk_bulk_ API usage.

Dmitry Baryshkov (4):
  PCI: qcom: Move 2_1_0 defines close to the struct definition
  PCI: qcom: Use clk_bulk_ API for 1.0.0 clocks handling
  PCI: qcom: Use clk_bulk_ API for 2.3.2 clocks handling
  PCI: qcom: Use clk_bulk_ API for 2.3.3 clocks handling

 drivers/pci/controller/dwc/pcie-qcom.c | 230 +++++--------------------
 1 file changed, 45 insertions(+), 185 deletions(-)

-- 
2.35.1

[PATCH 1/4] PCI: qcom: Move 2_1_0 defines close to the struct definition

[Dmitry Baryshkov]

Move the QCOM_PCIE_2_1_0_MAX_* just before the struct
qcom_pcie_resources_2_1_0 to follow the example of other structs.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index f711acacaeaf..939f19241356 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -117,11 +117,10 @@
 
 #define DEVICE_TYPE_RC				0x4
 
-#define QCOM_PCIE_2_1_0_MAX_SUPPLY	3
-#define QCOM_PCIE_2_1_0_MAX_CLOCKS	5
-
 #define QCOM_PCIE_CRC8_POLYNOMIAL (BIT(2) | BIT(1) | BIT(0))
 
+#define QCOM_PCIE_2_1_0_MAX_SUPPLY	3
+#define QCOM_PCIE_2_1_0_MAX_CLOCKS	5
 struct qcom_pcie_resources_2_1_0 {
 	struct clk_bulk_data clks[QCOM_PCIE_2_1_0_MAX_CLOCKS];
 	struct reset_control *pci_reset;
-- 
2.35.1

[PATCH 2/4] PCI: qcom: Use clk_bulk_ API for 1.0.0 clocks handling

[Dmitry Baryshkov]

Change hand-coded implementation of bulk clocks to use the existing
clk_bulk_* API.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 67 ++++++--------------------
 1 file changed, 16 insertions(+), 51 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 939f19241356..74588438db07 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -133,10 +133,7 @@ struct qcom_pcie_resources_2_1_0 {
 };
 
 struct qcom_pcie_resources_1_0_0 {
-	struct clk *iface;
-	struct clk *aux;
-	struct clk *master_bus;
-	struct clk *slave_bus;
+	struct clk_bulk_data clks[4];
 	struct reset_control *core;
 	struct regulator *vdda;
 };
@@ -472,26 +469,20 @@ static int qcom_pcie_get_resources_1_0_0(struct qcom_pcie *pcie)
 	struct qcom_pcie_resources_1_0_0 *res = &pcie->res.v1_0_0;
 	struct dw_pcie *pci = pcie->pci;
 	struct device *dev = pci->dev;
+	int ret;
 
 	res->vdda = devm_regulator_get(dev, "vdda");
 	if (IS_ERR(res->vdda))
 		return PTR_ERR(res->vdda);
 
-	res->iface = devm_clk_get(dev, "iface");
-	if (IS_ERR(res->iface))
-		return PTR_ERR(res->iface);
-
-	res->aux = devm_clk_get(dev, "aux");
-	if (IS_ERR(res->aux))
-		return PTR_ERR(res->aux);
-
-	res->master_bus = devm_clk_get(dev, "master_bus");
-	if (IS_ERR(res->master_bus))
-		return PTR_ERR(res->master_bus);
+	res->clks[0].id = "aux";
+	res->clks[1].id = "iface";
+	res->clks[2].id = "master_bus";
+	res->clks[3].id = "slave_bus";
 
-	res->slave_bus = devm_clk_get(dev, "slave_bus");
-	if (IS_ERR(res->slave_bus))
-		return PTR_ERR(res->slave_bus);
+	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
+	if (ret < 0)
+		return ret;
 
 	res->core = devm_reset_control_get_exclusive(dev, "core");
 	return PTR_ERR_OR_ZERO(res->core);
@@ -502,10 +493,7 @@ static void qcom_pcie_deinit_1_0_0(struct qcom_pcie *pcie)
 	struct qcom_pcie_resources_1_0_0 *res = &pcie->res.v1_0_0;
 
 	reset_control_assert(res->core);
-	clk_disable_unprepare(res->slave_bus);
-	clk_disable_unprepare(res->master_bus);
-	clk_disable_unprepare(res->iface);
-	clk_disable_unprepare(res->aux);
+	clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks);
 	regulator_disable(res->vdda);
 }
 
@@ -522,45 +510,22 @@ static int qcom_pcie_init_1_0_0(struct qcom_pcie *pcie)
 		return ret;
 	}
 
-	ret = clk_prepare_enable(res->aux);
+	ret = clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
 	if (ret) {
-		dev_err(dev, "cannot prepare/enable aux clock\n");
+		dev_err(dev, "cannot prepare/enable clocks\n");
 		goto err_res;
 	}
 
-	ret = clk_prepare_enable(res->iface);
-	if (ret) {
-		dev_err(dev, "cannot prepare/enable iface clock\n");
-		goto err_aux;
-	}
-
-	ret = clk_prepare_enable(res->master_bus);
-	if (ret) {
-		dev_err(dev, "cannot prepare/enable master_bus clock\n");
-		goto err_iface;
-	}
-
-	ret = clk_prepare_enable(res->slave_bus);
-	if (ret) {
-		dev_err(dev, "cannot prepare/enable slave_bus clock\n");
-		goto err_master;
-	}
-
 	ret = regulator_enable(res->vdda);
 	if (ret) {
 		dev_err(dev, "cannot enable vdda regulator\n");
-		goto err_slave;
+		goto err_clocks;
 	}
 
 	return 0;
-err_slave:
-	clk_disable_unprepare(res->slave_bus);
-err_master:
-	clk_disable_unprepare(res->master_bus);
-err_iface:
-	clk_disable_unprepare(res->iface);
-err_aux:
-	clk_disable_unprepare(res->aux);
+
+err_clocks:
+	clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks);
 err_res:
 	reset_control_assert(res->core);
 
-- 
2.35.1

[PATCH 3/4] PCI: qcom: Use clk_bulk_ API for 2.3.2 clocks handling

[Dmitry Baryshkov]

Change hand-coded implementation of bulk clocks to use the existing
clk_bulk_* API.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 68 ++++++--------------------
 1 file changed, 15 insertions(+), 53 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 74588438db07..eee4d2179e90 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -139,11 +139,9 @@ struct qcom_pcie_resources_1_0_0 {
 };
 
 #define QCOM_PCIE_2_3_2_MAX_SUPPLY	2
+#define QCOM_PCIE_2_3_2_MAX_CLOCKS	4
 struct qcom_pcie_resources_2_3_2 {
-	struct clk *aux_clk;
-	struct clk *master_clk;
-	struct clk *slave_clk;
-	struct clk *cfg_clk;
+	struct clk_bulk_data clks[QCOM_PCIE_2_3_2_MAX_CLOCKS];
 	struct regulator_bulk_data supplies[QCOM_PCIE_2_3_2_MAX_SUPPLY];
 };
 
@@ -571,21 +569,14 @@ static int qcom_pcie_get_resources_2_3_2(struct qcom_pcie *pcie)
 	if (ret)
 		return ret;
 
-	res->aux_clk = devm_clk_get(dev, "aux");
-	if (IS_ERR(res->aux_clk))
-		return PTR_ERR(res->aux_clk);
-
-	res->cfg_clk = devm_clk_get(dev, "cfg");
-	if (IS_ERR(res->cfg_clk))
-		return PTR_ERR(res->cfg_clk);
-
-	res->master_clk = devm_clk_get(dev, "bus_master");
-	if (IS_ERR(res->master_clk))
-		return PTR_ERR(res->master_clk);
+	res->clks[0].id = "aux";
+	res->clks[1].id = "cfg";
+	res->clks[2].id = "master";
+	res->clks[3].id = "slave";
 
-	res->slave_clk = devm_clk_get(dev, "bus_slave");
-	if (IS_ERR(res->slave_clk))
-		return PTR_ERR(res->slave_clk);
+	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
+	if (ret < 0)
+		return ret;
 
 	return 0;
 }
@@ -594,11 +585,7 @@ static void qcom_pcie_deinit_2_3_2(struct qcom_pcie *pcie)
 {
 	struct qcom_pcie_resources_2_3_2 *res = &pcie->res.v2_3_2;
 
-	clk_disable_unprepare(res->slave_clk);
-	clk_disable_unprepare(res->master_clk);
-	clk_disable_unprepare(res->cfg_clk);
-	clk_disable_unprepare(res->aux_clk);
-
+	clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks);
 	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
 }
 
@@ -615,40 +602,15 @@ static int qcom_pcie_init_2_3_2(struct qcom_pcie *pcie)
 		return ret;
 	}
 
-	ret = clk_prepare_enable(res->aux_clk);
-	if (ret) {
-		dev_err(dev, "cannot prepare/enable aux clock\n");
-		goto err_aux_clk;
-	}
-
-	ret = clk_prepare_enable(res->cfg_clk);
-	if (ret) {
-		dev_err(dev, "cannot prepare/enable cfg clock\n");
-		goto err_cfg_clk;
-	}
-
-	ret = clk_prepare_enable(res->master_clk);
-	if (ret) {
-		dev_err(dev, "cannot prepare/enable master clock\n");
-		goto err_master_clk;
-	}
-
-	ret = clk_prepare_enable(res->slave_clk);
-	if (ret) {
-		dev_err(dev, "cannot prepare/enable slave clock\n");
-		goto err_slave_clk;
+	ret = clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
+	if (ret < 0) {
+		dev_err(dev, "cannot prepare/enable clocks\n");
+		goto err_clks;
 	}
 
 	return 0;
 
-err_slave_clk:
-	clk_disable_unprepare(res->master_clk);
-err_master_clk:
-	clk_disable_unprepare(res->cfg_clk);
-err_cfg_clk:
-	clk_disable_unprepare(res->aux_clk);
-
-err_aux_clk:
+err_clks:
 	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
 
 	return ret;
-- 
2.35.1

[PATCH 4/4] PCI: qcom: Use clk_bulk_ API for 2.3.3 clocks handling

[Dmitry Baryshkov]

Change hand-coded implementation of bulk clocks to use the existing
clk_bulk_* API.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 90 ++++----------------------
 1 file changed, 12 insertions(+), 78 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index eee4d2179e90..e64e504e531e 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -164,11 +164,7 @@ struct qcom_pcie_resources_2_4_0 {
 };
 
 struct qcom_pcie_resources_2_3_3 {
-	struct clk *iface;
-	struct clk *axi_m_clk;
-	struct clk *axi_s_clk;
-	struct clk *ahb_clk;
-	struct clk *aux_clk;
+	struct clk_bulk_data clks[5];
 	struct reset_control *rst[7];
 };
 
@@ -929,29 +925,19 @@ static int qcom_pcie_get_resources_2_3_3(struct qcom_pcie *pcie)
 	struct dw_pcie *pci = pcie->pci;
 	struct device *dev = pci->dev;
 	int i;
+	int ret;
 	const char *rst_names[] = { "axi_m", "axi_s", "pipe",
 				    "axi_m_sticky", "sticky",
 				    "ahb", "sleep", };
 
-	res->iface = devm_clk_get(dev, "iface");
-	if (IS_ERR(res->iface))
-		return PTR_ERR(res->iface);
-
-	res->axi_m_clk = devm_clk_get(dev, "axi_m");
-	if (IS_ERR(res->axi_m_clk))
-		return PTR_ERR(res->axi_m_clk);
-
-	res->axi_s_clk = devm_clk_get(dev, "axi_s");
-	if (IS_ERR(res->axi_s_clk))
-		return PTR_ERR(res->axi_s_clk);
-
-	res->ahb_clk = devm_clk_get(dev, "ahb");
-	if (IS_ERR(res->ahb_clk))
-		return PTR_ERR(res->ahb_clk);
-
-	res->aux_clk = devm_clk_get(dev, "aux");
-	if (IS_ERR(res->aux_clk))
-		return PTR_ERR(res->aux_clk);
+	res->clks[0].id = "iface";
+	res->clks[1].id = "axi_m";
+	res->clks[2].id = "axi_s";
+	res->clks[3].id = "ahb";
+	res->clks[4].id = "aux";
+	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
+	if (ret < 0)
+		return ret;
 
 	for (i = 0; i < ARRAY_SIZE(rst_names); i++) {
 		res->rst[i] = devm_reset_control_get(dev, rst_names[i]);
@@ -966,11 +952,7 @@ static void qcom_pcie_deinit_2_3_3(struct qcom_pcie *pcie)
 {
 	struct qcom_pcie_resources_2_3_3 *res = &pcie->res.v2_3_3;
 
-	clk_disable_unprepare(res->iface);
-	clk_disable_unprepare(res->axi_m_clk);
-	clk_disable_unprepare(res->axi_s_clk);
-	clk_disable_unprepare(res->ahb_clk);
-	clk_disable_unprepare(res->aux_clk);
+	clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks);
 }
 
 static int qcom_pcie_init_2_3_3(struct qcom_pcie *pcie)
@@ -1005,55 +987,7 @@ static int qcom_pcie_init_2_3_3(struct qcom_pcie *pcie)
 	 */
 	usleep_range(2000, 2500);
 
-	ret = clk_prepare_enable(res->iface);
-	if (ret) {
-		dev_err(dev, "cannot prepare/enable core clock\n");
-		goto err_clk_iface;
-	}
-
-	ret = clk_prepare_enable(res->axi_m_clk);
-	if (ret) {
-		dev_err(dev, "cannot prepare/enable core clock\n");
-		goto err_clk_axi_m;
-	}
-
-	ret = clk_prepare_enable(res->axi_s_clk);
-	if (ret) {
-		dev_err(dev, "cannot prepare/enable axi slave clock\n");
-		goto err_clk_axi_s;
-	}
-
-	ret = clk_prepare_enable(res->ahb_clk);
-	if (ret) {
-		dev_err(dev, "cannot prepare/enable ahb clock\n");
-		goto err_clk_ahb;
-	}
-
-	ret = clk_prepare_enable(res->aux_clk);
-	if (ret) {
-		dev_err(dev, "cannot prepare/enable aux clock\n");
-		goto err_clk_aux;
-	}
-
-	return 0;
-
-err_clk_aux:
-	clk_disable_unprepare(res->ahb_clk);
-err_clk_ahb:
-	clk_disable_unprepare(res->axi_s_clk);
-err_clk_axi_s:
-	clk_disable_unprepare(res->axi_m_clk);
-err_clk_axi_m:
-	clk_disable_unprepare(res->iface);
-err_clk_iface:
-	/*
-	 * Not checking for failure, will anyway return
-	 * the original failure in 'ret'.
-	 */
-	for (i = 0; i < ARRAY_SIZE(res->rst); i++)
-		reset_control_assert(res->rst[i]);
-
-	return ret;
+	return clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
 }
 
 static int qcom_pcie_post_init_2_3_3(struct qcom_pcie *pcie)
-- 
2.35.1

Re: [PATCH 2/4] PCI: qcom: Use clk_bulk_ API for 1.0.0 clocks handling

[Johan Hovold]

On Thu, Oct 20, 2022 at 01:31:18PM +0300, Dmitry Baryshkov wrote:
> Change hand-coded implementation of bulk clocks to use the existing

Let's hope everything is "hand-coded" at least for a few years still
(job security). ;)

Perhaps rephrase using "open-coded"?

> clk_bulk_* API.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 67 ++++++--------------------
>  1 file changed, 16 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 939f19241356..74588438db07 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -133,10 +133,7 @@ struct qcom_pcie_resources_2_1_0 {
>  };
>  
>  struct qcom_pcie_resources_1_0_0 {
> -	struct clk *iface;
> -	struct clk *aux;
> -	struct clk *master_bus;
> -	struct clk *slave_bus;
> +	struct clk_bulk_data clks[4];
>  	struct reset_control *core;
>  	struct regulator *vdda;
>  };
> @@ -472,26 +469,20 @@ static int qcom_pcie_get_resources_1_0_0(struct qcom_pcie *pcie)
>  	struct qcom_pcie_resources_1_0_0 *res = &pcie->res.v1_0_0;
>  	struct dw_pcie *pci = pcie->pci;
>  	struct device *dev = pci->dev;
> +	int ret;
>  
>  	res->vdda = devm_regulator_get(dev, "vdda");
>  	if (IS_ERR(res->vdda))
>  		return PTR_ERR(res->vdda);
>  
> -	res->iface = devm_clk_get(dev, "iface");
> -	if (IS_ERR(res->iface))
> -		return PTR_ERR(res->iface);
> -
> -	res->aux = devm_clk_get(dev, "aux");
> -	if (IS_ERR(res->aux))
> -		return PTR_ERR(res->aux);
> -
> -	res->master_bus = devm_clk_get(dev, "master_bus");
> -	if (IS_ERR(res->master_bus))
> -		return PTR_ERR(res->master_bus);
> +	res->clks[0].id = "aux";
> +	res->clks[1].id = "iface";
> +	res->clks[2].id = "master_bus";
> +	res->clks[3].id = "slave_bus";
>  
> -	res->slave_bus = devm_clk_get(dev, "slave_bus");
> -	if (IS_ERR(res->slave_bus))
> -		return PTR_ERR(res->slave_bus);
> +	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
> +	if (ret < 0)
> +		return ret;

Are you sure there are no dependencies between these clocks and that
they can be enabled and disabled in any order?

Are you also convinced that they will always be enabled and disabled
together (e.g. not controlled individually during suspend)?

> -	ret = clk_prepare_enable(res->aux);
> +	ret = clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
>  	if (ret) {
> -		dev_err(dev, "cannot prepare/enable aux clock\n");
> +		dev_err(dev, "cannot prepare/enable clocks\n");

The bulk API already logs an error so you can drop the dev_err().

These comments apply also to the other patches.

Johan

Re: [PATCH 2/4] PCI: qcom: Use clk_bulk_ API for 1.0.0 clocks handling

[Dmitry Baryshkov]

On 20/10/2022 14:08, Johan Hovold wrote:
> On Thu, Oct 20, 2022 at 01:31:18PM +0300, Dmitry Baryshkov wrote:
>> Change hand-coded implementation of bulk clocks to use the existing
> 
> Let's hope everything is "hand-coded" at least for a few years still
> (job security). ;)
> 
> Perhaps rephrase using "open-coded"?

Yes, thank you.

> 
>> clk_bulk_* API.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/pci/controller/dwc/pcie-qcom.c | 67 ++++++--------------------
>>   1 file changed, 16 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>> index 939f19241356..74588438db07 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>> @@ -133,10 +133,7 @@ struct qcom_pcie_resources_2_1_0 {
>>   };
>>   
>>   struct qcom_pcie_resources_1_0_0 {
>> -	struct clk *iface;
>> -	struct clk *aux;
>> -	struct clk *master_bus;
>> -	struct clk *slave_bus;
>> +	struct clk_bulk_data clks[4];
>>   	struct reset_control *core;
>>   	struct regulator *vdda;
>>   };
>> @@ -472,26 +469,20 @@ static int qcom_pcie_get_resources_1_0_0(struct qcom_pcie *pcie)
>>   	struct qcom_pcie_resources_1_0_0 *res = &pcie->res.v1_0_0;
>>   	struct dw_pcie *pci = pcie->pci;
>>   	struct device *dev = pci->dev;
>> +	int ret;
>>   
>>   	res->vdda = devm_regulator_get(dev, "vdda");
>>   	if (IS_ERR(res->vdda))
>>   		return PTR_ERR(res->vdda);
>>   
>> -	res->iface = devm_clk_get(dev, "iface");
>> -	if (IS_ERR(res->iface))
>> -		return PTR_ERR(res->iface);
>> -
>> -	res->aux = devm_clk_get(dev, "aux");
>> -	if (IS_ERR(res->aux))
>> -		return PTR_ERR(res->aux);
>> -
>> -	res->master_bus = devm_clk_get(dev, "master_bus");
>> -	if (IS_ERR(res->master_bus))
>> -		return PTR_ERR(res->master_bus);
>> +	res->clks[0].id = "aux";
>> +	res->clks[1].id = "iface";
>> +	res->clks[2].id = "master_bus";
>> +	res->clks[3].id = "slave_bus";
>>   
>> -	res->slave_bus = devm_clk_get(dev, "slave_bus");
>> -	if (IS_ERR(res->slave_bus))
>> -		return PTR_ERR(res->slave_bus);
>> +	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
>> +	if (ret < 0)
>> +		return ret;
> 
> Are you sure there are no dependencies between these clocks and that
> they can be enabled and disabled in any order?

The order is enforced by the bulk API. Forward to enable, backward to 
disable.

> 
> Are you also convinced that they will always be enabled and disabled
> together (e.g. not controlled individually during suspend)?

 From what I see downstream, yes. They separate host and pipe clocks, 
but for each of these groups all clocks are disabled and enabled in 
sequence.

For the newer platforms the only exceptions are refgen (handled by the 
PHY in our kernels) and ddrss_sf_tbu (only on some platforms), which is 
not touched by these patches.

> 
>> -	ret = clk_prepare_enable(res->aux);
>> +	ret = clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
>>   	if (ret) {
>> -		dev_err(dev, "cannot prepare/enable aux clock\n");
>> +		dev_err(dev, "cannot prepare/enable clocks\n");
> 
> The bulk API already logs an error so you can drop the dev_err().

Ack.

> 
> These comments apply also to the other patches.
> 
> Johan

-- 
With best wishes
Dmitry

Re: [PATCH 2/4] PCI: qcom: Use clk_bulk_ API for 1.0.0 clocks handling

[Johan Hovold]

On Thu, Oct 20, 2022 at 02:22:47PM +0300, Dmitry Baryshkov wrote:
> On 20/10/2022 14:08, Johan Hovold wrote:
> > On Thu, Oct 20, 2022 at 01:31:18PM +0300, Dmitry Baryshkov wrote:

> >> +	res->clks[0].id = "aux";
> >> +	res->clks[1].id = "iface";
> >> +	res->clks[2].id = "master_bus";
> >> +	res->clks[3].id = "slave_bus";
> >>   
> >> -	res->slave_bus = devm_clk_get(dev, "slave_bus");
> >> -	if (IS_ERR(res->slave_bus))
> >> -		return PTR_ERR(res->slave_bus);
> >> +	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
> >> +	if (ret < 0)
> >> +		return ret;
> > 
> > Are you sure there are no dependencies between these clocks and that
> > they can be enabled and disabled in any order?
> 
> The order is enforced by the bulk API. Forward to enable, backward to 
> disable.

Right you are. (I had it mixed up with a different API which had no such
guarantees and now I can't seem to remember which it was, maybe I dreamt
it.)

> > Are you also convinced that they will always be enabled and disabled
> > together (e.g. not controlled individually during suspend)?
> 
>  From what I see downstream, yes. They separate host and pipe clocks, 
> but for each of these groups all clocks are disabled and enabled in 
> sequence.
> 
> For the newer platforms the only exceptions are refgen (handled by the 
> PHY in our kernels) and ddrss_sf_tbu (only on some platforms), which is 
> not touched by these patches.

Sounds good.

Johan

Re: [PATCH 2/4] PCI: qcom: Use clk_bulk_ API for 1.0.0 clocks handling

[Dmitry Baryshkov]

On 20/10/2022 14:32, Johan Hovold wrote:
> On Thu, Oct 20, 2022 at 02:22:47PM +0300, Dmitry Baryshkov wrote:
>> On 20/10/2022 14:08, Johan Hovold wrote:
>>> On Thu, Oct 20, 2022 at 01:31:18PM +0300, Dmitry Baryshkov wrote:
> 
>>>> +	res->clks[0].id = "aux";
>>>> +	res->clks[1].id = "iface";
>>>> +	res->clks[2].id = "master_bus";
>>>> +	res->clks[3].id = "slave_bus";
>>>>    
>>>> -	res->slave_bus = devm_clk_get(dev, "slave_bus");
>>>> -	if (IS_ERR(res->slave_bus))
>>>> -		return PTR_ERR(res->slave_bus);
>>>> +	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>
>>> Are you sure there are no dependencies between these clocks and that
>>> they can be enabled and disabled in any order?
>>
>> The order is enforced by the bulk API. Forward to enable, backward to
>> disable.
> 
> Right you are. (I had it mixed up with a different API which had no such
> guarantees and now I can't seem to remember which it was, maybe I dreamt
> it.)

Most probably you were thinking about regulators, which are a separate 
crazy beast. The regulator_bulk_enable() enables all the regulators in 
parallel using async calls.

-- 
With best wishes
Dmitry

Re: [PATCH 3/4] PCI: qcom: Use clk_bulk_ API for 2.3.2 clocks handling

[Han Jingoo]

On Thu, Oct 20, 2022 Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
>
> Change hand-coded implementation of bulk clocks to use the existing
> clk_bulk_* API.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 68 ++++++--------------------
>  1 file changed, 15 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 74588438db07..eee4d2179e90 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -139,11 +139,9 @@ struct qcom_pcie_resources_1_0_0 {
>  };
>
>  #define QCOM_PCIE_2_3_2_MAX_SUPPLY     2
> +#define QCOM_PCIE_2_3_2_MAX_CLOCKS     4
>  struct qcom_pcie_resources_2_3_2 {
> -       struct clk *aux_clk;
> -       struct clk *master_clk;
> -       struct clk *slave_clk;
> -       struct clk *cfg_clk;
> +       struct clk_bulk_data clks[QCOM_PCIE_2_3_2_MAX_CLOCKS];
>         struct regulator_bulk_data supplies[QCOM_PCIE_2_3_2_MAX_SUPPLY];
>  };
>
> @@ -571,21 +569,14 @@ static int qcom_pcie_get_resources_2_3_2(struct qcom_pcie *pcie)
>         if (ret)
>                 return ret;
>
> -       res->aux_clk = devm_clk_get(dev, "aux");
> -       if (IS_ERR(res->aux_clk))
> -               return PTR_ERR(res->aux_clk);
> -
> -       res->cfg_clk = devm_clk_get(dev, "cfg");
> -       if (IS_ERR(res->cfg_clk))
> -               return PTR_ERR(res->cfg_clk);
> -
> -       res->master_clk = devm_clk_get(dev, "bus_master");
> -       if (IS_ERR(res->master_clk))
> -               return PTR_ERR(res->master_clk);
> +       res->clks[0].id = "aux";
> +       res->clks[1].id = "cfg";
> +       res->clks[2].id = "master";

Hi Dmitry,

I just have a simple question on clock names. The original clock name
is 'bus_master', while your patch uses just 'master' as the clock name.
As far as I know, the clock names are defined by clock side, not device
driver side. Is it intentional? If so, would you please explain why it is ok?

> +       res->clks[3].id = "slave";

Ditto.

Thank you.

Best regards,
Jingoo Han

>
> -       res->slave_clk = devm_clk_get(dev, "bus_slave");
> -       if (IS_ERR(res->slave_clk))
> -               return PTR_ERR(res->slave_clk);
> +       ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
> +       if (ret < 0)
> +               return ret;
>
>         return 0;
>  }
> @@ -594,11 +585,7 @@ static void qcom_pcie_deinit_2_3_2(struct qcom_pcie *pcie)
>  {
>         struct qcom_pcie_resources_2_3_2 *res = &pcie->res.v2_3_2;
>
> -       clk_disable_unprepare(res->slave_clk);
> -       clk_disable_unprepare(res->master_clk);
> -       clk_disable_unprepare(res->cfg_clk);
> -       clk_disable_unprepare(res->aux_clk);
> -
> +       clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks);
>         regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
>  }
>
> @@ -615,40 +602,15 @@ static int qcom_pcie_init_2_3_2(struct qcom_pcie *pcie)
>                 return ret;
>         }
>
> -       ret = clk_prepare_enable(res->aux_clk);
> -       if (ret) {
> -               dev_err(dev, "cannot prepare/enable aux clock\n");
> -               goto err_aux_clk;
> -       }
> -
> -       ret = clk_prepare_enable(res->cfg_clk);
> -       if (ret) {
> -               dev_err(dev, "cannot prepare/enable cfg clock\n");
> -               goto err_cfg_clk;
> -       }
> -
> -       ret = clk_prepare_enable(res->master_clk);
> -       if (ret) {
> -               dev_err(dev, "cannot prepare/enable master clock\n");
> -               goto err_master_clk;
> -       }
> -
> -       ret = clk_prepare_enable(res->slave_clk);
> -       if (ret) {
> -               dev_err(dev, "cannot prepare/enable slave clock\n");
> -               goto err_slave_clk;
> +       ret = clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
> +       if (ret < 0) {
> +               dev_err(dev, "cannot prepare/enable clocks\n");
> +               goto err_clks;
>         }
>
>         return 0;
>
> -err_slave_clk:
> -       clk_disable_unprepare(res->master_clk);
> -err_master_clk:
> -       clk_disable_unprepare(res->cfg_clk);
> -err_cfg_clk:
> -       clk_disable_unprepare(res->aux_clk);
> -
> -err_aux_clk:
> +err_clks:
>         regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
>
>         return ret;
> --
> 2.35.1
>

Re: [PATCH 2/4] PCI: qcom: Use clk_bulk_ API for 1.0.0 clocks handling

[Lorenzo Pieralisi]

On Thu, Oct 20, 2022 at 02:22:47PM +0300, Dmitry Baryshkov wrote:
> On 20/10/2022 14:08, Johan Hovold wrote:
> > On Thu, Oct 20, 2022 at 01:31:18PM +0300, Dmitry Baryshkov wrote:
> > > Change hand-coded implementation of bulk clocks to use the existing
> > 
> > Let's hope everything is "hand-coded" at least for a few years still
> > (job security). ;)
> > 
> > Perhaps rephrase using "open-coded"?
> 
> Yes, thank you.

If that's the only change required I can fix it up when merging the
series.

Please let me know.

Thanks,
Lorenzo

> > 
> > > clk_bulk_* API.
> > > 
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > >   drivers/pci/controller/dwc/pcie-qcom.c | 67 ++++++--------------------
> > >   1 file changed, 16 insertions(+), 51 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > index 939f19241356..74588438db07 100644
> > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > @@ -133,10 +133,7 @@ struct qcom_pcie_resources_2_1_0 {
> > >   };
> > >   struct qcom_pcie_resources_1_0_0 {
> > > -	struct clk *iface;
> > > -	struct clk *aux;
> > > -	struct clk *master_bus;
> > > -	struct clk *slave_bus;
> > > +	struct clk_bulk_data clks[4];
> > >   	struct reset_control *core;
> > >   	struct regulator *vdda;
> > >   };
> > > @@ -472,26 +469,20 @@ static int qcom_pcie_get_resources_1_0_0(struct qcom_pcie *pcie)
> > >   	struct qcom_pcie_resources_1_0_0 *res = &pcie->res.v1_0_0;
> > >   	struct dw_pcie *pci = pcie->pci;
> > >   	struct device *dev = pci->dev;
> > > +	int ret;
> > >   	res->vdda = devm_regulator_get(dev, "vdda");
> > >   	if (IS_ERR(res->vdda))
> > >   		return PTR_ERR(res->vdda);
> > > -	res->iface = devm_clk_get(dev, "iface");
> > > -	if (IS_ERR(res->iface))
> > > -		return PTR_ERR(res->iface);
> > > -
> > > -	res->aux = devm_clk_get(dev, "aux");
> > > -	if (IS_ERR(res->aux))
> > > -		return PTR_ERR(res->aux);
> > > -
> > > -	res->master_bus = devm_clk_get(dev, "master_bus");
> > > -	if (IS_ERR(res->master_bus))
> > > -		return PTR_ERR(res->master_bus);
> > > +	res->clks[0].id = "aux";
> > > +	res->clks[1].id = "iface";
> > > +	res->clks[2].id = "master_bus";
> > > +	res->clks[3].id = "slave_bus";
> > > -	res->slave_bus = devm_clk_get(dev, "slave_bus");
> > > -	if (IS_ERR(res->slave_bus))
> > > -		return PTR_ERR(res->slave_bus);
> > > +	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(res->clks), res->clks);
> > > +	if (ret < 0)
> > > +		return ret;
> > 
> > Are you sure there are no dependencies between these clocks and that
> > they can be enabled and disabled in any order?
> 
> The order is enforced by the bulk API. Forward to enable, backward to
> disable.
> 
> > 
> > Are you also convinced that they will always be enabled and disabled
> > together (e.g. not controlled individually during suspend)?
> 
> From what I see downstream, yes. They separate host and pipe clocks, but for
> each of these groups all clocks are disabled and enabled in sequence.
> 
> For the newer platforms the only exceptions are refgen (handled by the PHY
> in our kernels) and ddrss_sf_tbu (only on some platforms), which is not
> touched by these patches.
> 
> > 
> > > -	ret = clk_prepare_enable(res->aux);
> > > +	ret = clk_bulk_prepare_enable(ARRAY_SIZE(res->clks), res->clks);
> > >   	if (ret) {
> > > -		dev_err(dev, "cannot prepare/enable aux clock\n");
> > > +		dev_err(dev, "cannot prepare/enable clocks\n");
> > 
> > The bulk API already logs an error so you can drop the dev_err().
> 
> Ack.
> 
> > 
> > These comments apply also to the other patches.
> > 
> > Johan
> 
> -- 
> With best wishes
> Dmitry
> 

Re: [PATCH 2/4] PCI: qcom: Use clk_bulk_ API for 1.0.0 clocks handling

[Johan Hovold]

On Fri, Jan 13, 2023 at 04:54:25PM +0100, Lorenzo Pieralisi wrote:
> On Thu, Oct 20, 2022 at 02:22:47PM +0300, Dmitry Baryshkov wrote:
> > On 20/10/2022 14:08, Johan Hovold wrote:
> > > On Thu, Oct 20, 2022 at 01:31:18PM +0300, Dmitry Baryshkov wrote:
> > > > Change hand-coded implementation of bulk clocks to use the existing
> > > 
> > > Let's hope everything is "hand-coded" at least for a few years still
> > > (job security). ;)
> > > 
> > > Perhaps rephrase using "open-coded"?
> > 
> > Yes, thank you.
> 
> If that's the only change required I can fix it up when merging the
> series.

I believe there was also a couple of bugs in patch 3/4 which was spotted
by Jingoo Han and that would need to be fixed:

	https://lore.kernel.org/all/CAPOBaE5Zg+r0F35MvKWAozFa9x4xvym1LbA_UHvUSmnLbTpqzA@mail.gmail.com/

Johan