[PATCH v4 0/2] Add helper function to get and enable all bulk clocks

[PATCH v4 0/2] Add helper function to get and enable all bulk clocks

[Shradha Todi]

Create a managed API wrapper to get all the bulk clocks and enable them
as it is a very common practice in many drivers. The second patch uses
this API to adapt to clk_bulk_* APIs in the exynos driver.
v1:
 - https://lore.kernel.org/lkml/20231009062216.6729-1-shradha.t@samsung.com/
v2:
 - https://lore.kernel.org/lkml/20231115065621.27014-1-shradha.t@samsung.com/
 - Addressed Manivannan's comments to improve patch
v3:
 - https://lore.kernel.org/all/20240110110115.56270-1-shradha.t@samsung.com/
 - Took Marek's suggestion to make a common bulk clk wrapper and use it in
   the exynos driver
v4:
 - Addressed Alim and Manivannan's comments
 - Changed enabled->enable and disabled->disable in function name
 - Remove num_clks out parameter as it is not required by user
 - Removed exit callback and used function name directly in release

Shradha Todi (2):
  clk: Provide managed helper to get and enable bulk clocks
  PCI: exynos: Adapt to clk_bulk_* APIs

 drivers/clk/clk-devres.c                | 40 ++++++++++++++++++
 drivers/pci/controller/dwc/pci-exynos.c | 54 ++-----------------------
 include/linux/clk.h                     | 24 +++++++++++
 3 files changed, 68 insertions(+), 50 deletions(-)

-- 
2.17.1

[PATCH v4 1/2] clk: Provide managed helper to get and enable bulk clocks

[Shradha Todi]

Provide a managed devm_clk_bulk* wrapper to get and enable all
bulk clocks in order to simplify drivers that keeps all clocks
enabled for the time of driver operation.

Suggested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Shradha Todi <shradha.t@samsung.com>
---
 drivers/clk/clk-devres.c | 40 ++++++++++++++++++++++++++++++++++++++++
 include/linux/clk.h      | 24 ++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index 4fb4fd4b06bd..cbbd2cc339c3 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -182,6 +182,46 @@ int __must_check devm_clk_bulk_get_all(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all);
 
+static void devm_clk_bulk_release_all_enable(struct device *dev, void *res)
+{
+	struct clk_bulk_devres *devres = res;
+
+	clk_bulk_disable_unprepare(devres->num_clks, devres->clks);
+	clk_bulk_put_all(devres->num_clks, devres->clks);
+}
+
+int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
+					      struct clk_bulk_data **clks)
+{
+	struct clk_bulk_devres *devres;
+	int ret;
+
+	devres = devres_alloc(devm_clk_bulk_release_all_enable,
+			      sizeof(*devres), GFP_KERNEL);
+	if (!devres)
+		return -ENOMEM;
+
+	ret = clk_bulk_get_all(dev, &devres->clks);
+	if (ret > 0) {
+		*clks = devres->clks;
+		devres->num_clks = ret;
+	} else {
+		devres_free(devres);
+		return ret;
+	}
+
+	ret = clk_bulk_prepare_enable(devres->num_clks, *clks);
+	if (!ret) {
+		devres_add(dev, devres);
+	} else {
+		clk_bulk_put_all(devres->num_clks, devres->clks);
+		devres_free(devres);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all_enable);
+
 static int devm_clk_match(struct device *dev, void *res, void *data)
 {
 	struct clk **c = res;
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 1ef013324237..a005e709b7bd 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -438,6 +438,23 @@ int __must_check devm_clk_bulk_get_optional(struct device *dev, int num_clks,
 int __must_check devm_clk_bulk_get_all(struct device *dev,
 				       struct clk_bulk_data **clks);
 
+/**
+ * devm_clk_bulk_get_all_enable - managed get multiple clk consumers and
+ *				  enable all clks
+ * @dev: device for clock "consumer"
+ * @clks: pointer to the clk_bulk_data table of consumer
+ *
+ * Returns success (0) or negative errno.
+ *
+ * This helper function allows drivers to get several clk
+ * consumers and enable all of them in one operation with management.
+ * The clks will automatically be disabled and freed when the device
+ * is unbound.
+ */
+
+int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
+					      struct clk_bulk_data **clks);
+
 /**
  * devm_clk_get - lookup and obtain a managed reference to a clock producer.
  * @dev: device for clock "consumer"
@@ -960,6 +977,13 @@ static inline int __must_check devm_clk_bulk_get_all(struct device *dev,
 	return 0;
 }
 
+static inline int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
+						struct clk_bulk_data **clks)
+{
+
+	return 0;
+}
+
 static inline struct clk *devm_get_clk_from_child(struct device *dev,
 				struct device_node *np, const char *con_id)
 {
-- 
2.17.1

[PATCH v4 2/2] PCI: exynos: Adapt to clk_bulk_* APIs

[Shradha Todi]

There is no need to hardcode the clock info in the driver as driver can
rely on the devicetree to supply the clocks required for the functioning
of the peripheral. Get rid of the static clock info and obtain the
platform supplied clocks. All the clocks supplied is obtained and enabled
using the devm_clk_bulk_get_all_enable() API.

Signed-off-by: Shradha Todi <shradha.t@samsung.com>
---
 drivers/pci/controller/dwc/pci-exynos.c | 54 ++-----------------------
 1 file changed, 4 insertions(+), 50 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
index ec5611005566..3234eb5be1fb 100644
--- a/drivers/pci/controller/dwc/pci-exynos.c
+++ b/drivers/pci/controller/dwc/pci-exynos.c
@@ -54,43 +54,11 @@
 struct exynos_pcie {
 	struct dw_pcie			pci;
 	void __iomem			*elbi_base;
-	struct clk			*clk;
-	struct clk			*bus_clk;
+	struct clk_bulk_data		*clks;
 	struct phy			*phy;
 	struct regulator_bulk_data	supplies[2];
 };
 
-static int exynos_pcie_init_clk_resources(struct exynos_pcie *ep)
-{
-	struct device *dev = ep->pci.dev;
-	int ret;
-
-	ret = clk_prepare_enable(ep->clk);
-	if (ret) {
-		dev_err(dev, "cannot enable pcie rc clock");
-		return ret;
-	}
-
-	ret = clk_prepare_enable(ep->bus_clk);
-	if (ret) {
-		dev_err(dev, "cannot enable pcie bus clock");
-		goto err_bus_clk;
-	}
-
-	return 0;
-
-err_bus_clk:
-	clk_disable_unprepare(ep->clk);
-
-	return ret;
-}
-
-static void exynos_pcie_deinit_clk_resources(struct exynos_pcie *ep)
-{
-	clk_disable_unprepare(ep->bus_clk);
-	clk_disable_unprepare(ep->clk);
-}
-
 static void exynos_pcie_writel(void __iomem *base, u32 val, u32 reg)
 {
 	writel(val, base + reg);
@@ -332,17 +300,9 @@ static int exynos_pcie_probe(struct platform_device *pdev)
 	if (IS_ERR(ep->elbi_base))
 		return PTR_ERR(ep->elbi_base);
 
-	ep->clk = devm_clk_get(dev, "pcie");
-	if (IS_ERR(ep->clk)) {
-		dev_err(dev, "Failed to get pcie rc clock\n");
-		return PTR_ERR(ep->clk);
-	}
-
-	ep->bus_clk = devm_clk_get(dev, "pcie_bus");
-	if (IS_ERR(ep->bus_clk)) {
-		dev_err(dev, "Failed to get pcie bus clock\n");
-		return PTR_ERR(ep->bus_clk);
-	}
+	ret = devm_clk_bulk_get_all_enable(dev, &ep->clks);
+	if (ret < 0)
+		return ret;
 
 	ep->supplies[0].supply = "vdd18";
 	ep->supplies[1].supply = "vdd10";
@@ -351,10 +311,6 @@ static int exynos_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	ret = exynos_pcie_init_clk_resources(ep);
-	if (ret)
-		return ret;
-
 	ret = regulator_bulk_enable(ARRAY_SIZE(ep->supplies), ep->supplies);
 	if (ret)
 		return ret;
@@ -369,7 +325,6 @@ static int exynos_pcie_probe(struct platform_device *pdev)
 
 fail_probe:
 	phy_exit(ep->phy);
-	exynos_pcie_deinit_clk_resources(ep);
 	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
 
 	return ret;
@@ -383,7 +338,6 @@ static int __exit exynos_pcie_remove(struct platform_device *pdev)
 	exynos_pcie_assert_core_reset(ep);
 	phy_power_off(ep->phy);
 	phy_exit(ep->phy);
-	exynos_pcie_deinit_clk_resources(ep);
 	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
 
 	return 0;
-- 
2.17.1

Re: [PATCH v4 1/2] clk: Provide managed helper to get and enable bulk clocks

[Manivannan Sadhasivam]

On Wed, Jan 24, 2024 at 04:08:37PM +0530, Shradha Todi wrote:
> Provide a managed devm_clk_bulk* wrapper to get and enable all
> bulk clocks in order to simplify drivers that keeps all clocks
> enabled for the time of driver operation.
> 
> Suggested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Shradha Todi <shradha.t@samsung.com>
> ---
>  drivers/clk/clk-devres.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/clk.h      | 24 ++++++++++++++++++++++++
>  2 files changed, 64 insertions(+)
> 
> diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
> index 4fb4fd4b06bd..cbbd2cc339c3 100644
> --- a/drivers/clk/clk-devres.c
> +++ b/drivers/clk/clk-devres.c
> @@ -182,6 +182,46 @@ int __must_check devm_clk_bulk_get_all(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all);
>  
> +static void devm_clk_bulk_release_all_enable(struct device *dev, void *res)
> +{
> +	struct clk_bulk_devres *devres = res;
> +
> +	clk_bulk_disable_unprepare(devres->num_clks, devres->clks);
> +	clk_bulk_put_all(devres->num_clks, devres->clks);
> +}
> +
> +int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
> +					      struct clk_bulk_data **clks)
> +{
> +	struct clk_bulk_devres *devres;
> +	int ret;
> +
> +	devres = devres_alloc(devm_clk_bulk_release_all_enable,
> +			      sizeof(*devres), GFP_KERNEL);
> +	if (!devres)
> +		return -ENOMEM;
> +
> +	ret = clk_bulk_get_all(dev, &devres->clks);
> +	if (ret > 0) {
> +		*clks = devres->clks;
> +		devres->num_clks = ret;
> +	} else {
> +		devres_free(devres);
> +		return ret;
> +	}

How about:

	ret = clk_bulk_get_all(dev, &devres->clks);
	if (ret <= 0) {
		devres_free(devres);
		return ret;
	}

	*clks = devres->clks;
	devres->num_clks = ret;

Even though this patch follows the pattern used by the rest of the APIs in the
driver, IMO above makes it more readable.

> +
> +	ret = clk_bulk_prepare_enable(devres->num_clks, *clks);
> +	if (!ret) {
> +		devres_add(dev, devres);
> +	} else {
> +		clk_bulk_put_all(devres->num_clks, devres->clks);
> +		devres_free(devres);
> +	}
> +

Same as above:

	ret = clk_bulk_prepare_enable(devres->num_clks, *clks);
	if (ret) {
		clk_bulk_put_all(devres->num_clks, devres->clks);
		devres_free(devres);
		return ret;
	}

	devres_add(dev, devres);

> +	return ret;

	return 0;

> +}
> +EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all_enable);
> +
>  static int devm_clk_match(struct device *dev, void *res, void *data)
>  {
>  	struct clk **c = res;
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 1ef013324237..a005e709b7bd 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -438,6 +438,23 @@ int __must_check devm_clk_bulk_get_optional(struct device *dev, int num_clks,
>  int __must_check devm_clk_bulk_get_all(struct device *dev,
>  				       struct clk_bulk_data **clks);
>  
> +/**
> + * devm_clk_bulk_get_all_enable - managed get multiple clk consumers and
> + *				  enable all clks

"Get and enable all clocks of the consumer (managed)"

> + * @dev: device for clock "consumer"
> + * @clks: pointer to the clk_bulk_data table of consumer
> + *
> + * Returns success (0) or negative errno.
> + *
> + * This helper function allows drivers to get several clk

"This helper function allows drivers to get all clocks of the consumer and
enables them..."

- Mani

> + * consumers and enable all of them in one operation with management.
> + * The clks will automatically be disabled and freed when the device
> + * is unbound.
> + */
> +
> +int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
> +					      struct clk_bulk_data **clks);
> +
>  /**
>   * devm_clk_get - lookup and obtain a managed reference to a clock producer.
>   * @dev: device for clock "consumer"
> @@ -960,6 +977,13 @@ static inline int __must_check devm_clk_bulk_get_all(struct device *dev,
>  	return 0;
>  }
>  
> +static inline int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
> +						struct clk_bulk_data **clks)
> +{
> +
> +	return 0;
> +}
> +
>  static inline struct clk *devm_get_clk_from_child(struct device *dev,
>  				struct device_node *np, const char *con_id)
>  {
> -- 
> 2.17.1
> 

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

Re: [PATCH v4 2/2] PCI: exynos: Adapt to clk_bulk_* APIs

[Manivannan Sadhasivam]

On Wed, Jan 24, 2024 at 04:08:38PM +0530, Shradha Todi wrote:
> There is no need to hardcode the clock info in the driver as driver can
> rely on the devicetree to supply the clocks required for the functioning
> of the peripheral. Get rid of the static clock info and obtain the
> platform supplied clocks. All the clocks supplied is obtained and enabled
> using the devm_clk_bulk_get_all_enable() API.
> 
> Signed-off-by: Shradha Todi <shradha.t@samsung.com>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  drivers/pci/controller/dwc/pci-exynos.c | 54 ++-----------------------
>  1 file changed, 4 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
> index ec5611005566..3234eb5be1fb 100644
> --- a/drivers/pci/controller/dwc/pci-exynos.c
> +++ b/drivers/pci/controller/dwc/pci-exynos.c
> @@ -54,43 +54,11 @@
>  struct exynos_pcie {
>  	struct dw_pcie			pci;
>  	void __iomem			*elbi_base;
> -	struct clk			*clk;
> -	struct clk			*bus_clk;
> +	struct clk_bulk_data		*clks;
>  	struct phy			*phy;
>  	struct regulator_bulk_data	supplies[2];
>  };
>  
> -static int exynos_pcie_init_clk_resources(struct exynos_pcie *ep)
> -{
> -	struct device *dev = ep->pci.dev;
> -	int ret;
> -
> -	ret = clk_prepare_enable(ep->clk);
> -	if (ret) {
> -		dev_err(dev, "cannot enable pcie rc clock");
> -		return ret;
> -	}
> -
> -	ret = clk_prepare_enable(ep->bus_clk);
> -	if (ret) {
> -		dev_err(dev, "cannot enable pcie bus clock");
> -		goto err_bus_clk;
> -	}
> -
> -	return 0;
> -
> -err_bus_clk:
> -	clk_disable_unprepare(ep->clk);
> -
> -	return ret;
> -}
> -
> -static void exynos_pcie_deinit_clk_resources(struct exynos_pcie *ep)
> -{
> -	clk_disable_unprepare(ep->bus_clk);
> -	clk_disable_unprepare(ep->clk);
> -}
> -
>  static void exynos_pcie_writel(void __iomem *base, u32 val, u32 reg)
>  {
>  	writel(val, base + reg);
> @@ -332,17 +300,9 @@ static int exynos_pcie_probe(struct platform_device *pdev)
>  	if (IS_ERR(ep->elbi_base))
>  		return PTR_ERR(ep->elbi_base);
>  
> -	ep->clk = devm_clk_get(dev, "pcie");
> -	if (IS_ERR(ep->clk)) {
> -		dev_err(dev, "Failed to get pcie rc clock\n");
> -		return PTR_ERR(ep->clk);
> -	}
> -
> -	ep->bus_clk = devm_clk_get(dev, "pcie_bus");
> -	if (IS_ERR(ep->bus_clk)) {
> -		dev_err(dev, "Failed to get pcie bus clock\n");
> -		return PTR_ERR(ep->bus_clk);
> -	}
> +	ret = devm_clk_bulk_get_all_enable(dev, &ep->clks);
> +	if (ret < 0)
> +		return ret;
>  
>  	ep->supplies[0].supply = "vdd18";
>  	ep->supplies[1].supply = "vdd10";
> @@ -351,10 +311,6 @@ static int exynos_pcie_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	ret = exynos_pcie_init_clk_resources(ep);
> -	if (ret)
> -		return ret;
> -
>  	ret = regulator_bulk_enable(ARRAY_SIZE(ep->supplies), ep->supplies);
>  	if (ret)
>  		return ret;
> @@ -369,7 +325,6 @@ static int exynos_pcie_probe(struct platform_device *pdev)
>  
>  fail_probe:
>  	phy_exit(ep->phy);
> -	exynos_pcie_deinit_clk_resources(ep);
>  	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
>  
>  	return ret;
> @@ -383,7 +338,6 @@ static int __exit exynos_pcie_remove(struct platform_device *pdev)
>  	exynos_pcie_assert_core_reset(ep);
>  	phy_power_off(ep->phy);
>  	phy_exit(ep->phy);
> -	exynos_pcie_deinit_clk_resources(ep);
>  	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
>  
>  	return 0;
> -- 
> 2.17.1
> 

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

RE: [PATCH v4 1/2] clk: Provide managed helper to get and enable bulk clocks

[Alim Akhtar]

Hi Shradha

> -----Original Message-----
> From: Shradha Todi <shradha.t@samsung.com>
> Sent: Wednesday, January 24, 2024 4:09 PM
> To: linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-samsung-
> soc@vger.kernel.org
> Cc: mturquette@baylibre.com; sboyd@kernel.org; jingoohan1@gmail.com;
> lpieralisi@kernel.org; kw@linux.com; robh@kernel.org;
> bhelgaas@google.com; krzysztof.kozlowski@linaro.org;
> alim.akhtar@samsung.com; linux@armlinux.org.uk;
> m.szyprowski@samsung.com; manivannan.sadhasivam@linaro.org;
> pankaj.dubey@samsung.com; Shradha Todi <shradha.t@samsung.com>
> Subject: [PATCH v4 1/2] clk: Provide managed helper to get and enable bulk
> clocks
> 
> Provide a managed devm_clk_bulk* wrapper to get and enable all bulk clocks
> in order to simplify drivers that keeps all clocks enabled for the time of driver
> operation.
> 
> Suggested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Shradha Todi <shradha.t@samsung.com>
> ---

Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>

>  drivers/clk/clk-devres.c | 40
> ++++++++++++++++++++++++++++++++++++++++
>  include/linux/clk.h      | 24 ++++++++++++++++++++++++
>  2 files changed, 64 insertions(+)
> 
> diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c index
> 4fb4fd4b06bd..cbbd2cc339c3 100644
> --- a/drivers/clk/clk-devres.c
> +++ b/drivers/clk/clk-devres.c
> @@ -182,6 +182,46 @@ int __must_check devm_clk_bulk_get_all(struct
> device *dev,  }  EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all);
> 
> +static void devm_clk_bulk_release_all_enable(struct device *dev, void
> +*res) {
> +	struct clk_bulk_devres *devres = res;
> +
> +	clk_bulk_disable_unprepare(devres->num_clks, devres->clks);
> +	clk_bulk_put_all(devres->num_clks, devres->clks); }
> +
> +int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
> +					      struct clk_bulk_data **clks) {
> +	struct clk_bulk_devres *devres;
> +	int ret;
> +
> +	devres = devres_alloc(devm_clk_bulk_release_all_enable,
> +			      sizeof(*devres), GFP_KERNEL);
> +	if (!devres)
> +		return -ENOMEM;
> +
> +	ret = clk_bulk_get_all(dev, &devres->clks);
> +	if (ret > 0) {
> +		*clks = devres->clks;
> +		devres->num_clks = ret;
> +	} else {
> +		devres_free(devres);
> +		return ret;
> +	}
> +
> +	ret = clk_bulk_prepare_enable(devres->num_clks, *clks);
> +	if (!ret) {
> +		devres_add(dev, devres);
> +	} else {
> +		clk_bulk_put_all(devres->num_clks, devres->clks);
> +		devres_free(devres);
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all_enable);
> +
>  static int devm_clk_match(struct device *dev, void *res, void *data)  {
>  	struct clk **c = res;
> diff --git a/include/linux/clk.h b/include/linux/clk.h index
> 1ef013324237..a005e709b7bd 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -438,6 +438,23 @@ int __must_check
> devm_clk_bulk_get_optional(struct device *dev, int num_clks,  int
> __must_check devm_clk_bulk_get_all(struct device *dev,
>  				       struct clk_bulk_data **clks);
> 
> +/**
> + * devm_clk_bulk_get_all_enable - managed get multiple clk consumers
> and
> + *				  enable all clks
> + * @dev: device for clock "consumer"
> + * @clks: pointer to the clk_bulk_data table of consumer
> + *
> + * Returns success (0) or negative errno.
> + *
> + * This helper function allows drivers to get several clk
> + * consumers and enable all of them in one operation with management.
> + * The clks will automatically be disabled and freed when the device
> + * is unbound.
> + */
> +
> +int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
> +					      struct clk_bulk_data **clks);
> +
>  /**
>   * devm_clk_get - lookup and obtain a managed reference to a clock
> producer.
>   * @dev: device for clock "consumer"
> @@ -960,6 +977,13 @@ static inline int __must_check
> devm_clk_bulk_get_all(struct device *dev,
>  	return 0;
>  }
> 
> +static inline int __must_check devm_clk_bulk_get_all_enable(struct device
> *dev,
> +						struct clk_bulk_data **clks)
> +{
> +
> +	return 0;
> +}
> +
>  static inline struct clk *devm_get_clk_from_child(struct device *dev,
>  				struct device_node *np, const char *con_id)
> {
> --
> 2.17.1

RE: [PATCH v4 2/2] PCI: exynos: Adapt to clk_bulk_* APIs

[Alim Akhtar]

Hello Shradha

> -----Original Message-----
> From: Shradha Todi <shradha.t@samsung.com>
> Sent: Wednesday, January 24, 2024 4:09 PM
> To: linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-samsung-
> soc@vger.kernel.org
> Cc: mturquette@baylibre.com; sboyd@kernel.org; jingoohan1@gmail.com;
> lpieralisi@kernel.org; kw@linux.com; robh@kernel.org;
> bhelgaas@google.com; krzysztof.kozlowski@linaro.org;
> alim.akhtar@samsung.com; linux@armlinux.org.uk;
> m.szyprowski@samsung.com; manivannan.sadhasivam@linaro.org;
> pankaj.dubey@samsung.com; Shradha Todi <shradha.t@samsung.com>
> Subject: [PATCH v4 2/2] PCI: exynos: Adapt to clk_bulk_* APIs
> 
> There is no need to hardcode the clock info in the driver as driver can rely on
> the devicetree to supply the clocks required for the functioning of the
> peripheral. Get rid of the static clock info and obtain the platform supplied
> clocks. All the clocks supplied is obtained and enabled using the
> devm_clk_bulk_get_all_enable() API.
> 
> Signed-off-by: Shradha Todi <shradha.t@samsung.com>
> ---
Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>

>  drivers/pci/controller/dwc/pci-exynos.c | 54 ++-----------------------
>  1 file changed, 4 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-exynos.c
> b/drivers/pci/controller/dwc/pci-exynos.c
> index ec5611005566..3234eb5be1fb 100644
> --- a/drivers/pci/controller/dwc/pci-exynos.c
> +++ b/drivers/pci/controller/dwc/pci-exynos.c
> @@ -54,43 +54,11 @@
>  struct exynos_pcie {
>  	struct dw_pcie			pci;
>  	void __iomem			*elbi_base;
> -	struct clk			*clk;
> -	struct clk			*bus_clk;
> +	struct clk_bulk_data		*clks;
>  	struct phy			*phy;
>  	struct regulator_bulk_data	supplies[2];
>  };
> 
> -static int exynos_pcie_init_clk_resources(struct exynos_pcie *ep) -{
> -	struct device *dev = ep->pci.dev;
> -	int ret;
> -
> -	ret = clk_prepare_enable(ep->clk);
> -	if (ret) {
> -		dev_err(dev, "cannot enable pcie rc clock");
> -		return ret;
> -	}
> -
> -	ret = clk_prepare_enable(ep->bus_clk);
> -	if (ret) {
> -		dev_err(dev, "cannot enable pcie bus clock");
> -		goto err_bus_clk;
> -	}
> -
> -	return 0;
> -
> -err_bus_clk:
> -	clk_disable_unprepare(ep->clk);
> -
> -	return ret;
> -}
> -
> -static void exynos_pcie_deinit_clk_resources(struct exynos_pcie *ep) -{
> -	clk_disable_unprepare(ep->bus_clk);
> -	clk_disable_unprepare(ep->clk);
> -}
> -
>  static void exynos_pcie_writel(void __iomem *base, u32 val, u32 reg)  {
>  	writel(val, base + reg);
> @@ -332,17 +300,9 @@ static int exynos_pcie_probe(struct platform_device
> *pdev)
>  	if (IS_ERR(ep->elbi_base))
>  		return PTR_ERR(ep->elbi_base);
> 
> -	ep->clk = devm_clk_get(dev, "pcie");
> -	if (IS_ERR(ep->clk)) {
> -		dev_err(dev, "Failed to get pcie rc clock\n");
> -		return PTR_ERR(ep->clk);
> -	}
> -
> -	ep->bus_clk = devm_clk_get(dev, "pcie_bus");
> -	if (IS_ERR(ep->bus_clk)) {
> -		dev_err(dev, "Failed to get pcie bus clock\n");
> -		return PTR_ERR(ep->bus_clk);
> -	}
> +	ret = devm_clk_bulk_get_all_enable(dev, &ep->clks);
> +	if (ret < 0)
> +		return ret;
> 
>  	ep->supplies[0].supply = "vdd18";
>  	ep->supplies[1].supply = "vdd10";
> @@ -351,10 +311,6 @@ static int exynos_pcie_probe(struct platform_device
> *pdev)
>  	if (ret)
>  		return ret;
> 
> -	ret = exynos_pcie_init_clk_resources(ep);
> -	if (ret)
> -		return ret;
> -
>  	ret = regulator_bulk_enable(ARRAY_SIZE(ep->supplies), ep-
> >supplies);
>  	if (ret)
>  		return ret;
> @@ -369,7 +325,6 @@ static int exynos_pcie_probe(struct platform_device
> *pdev)
> 
>  fail_probe:
>  	phy_exit(ep->phy);
> -	exynos_pcie_deinit_clk_resources(ep);
>  	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
> 
>  	return ret;
> @@ -383,7 +338,6 @@ static int __exit exynos_pcie_remove(struct
> platform_device *pdev)
>  	exynos_pcie_assert_core_reset(ep);
>  	phy_power_off(ep->phy);
>  	phy_exit(ep->phy);
> -	exynos_pcie_deinit_clk_resources(ep);
>  	regulator_bulk_disable(ARRAY_SIZE(ep->supplies), ep->supplies);
> 
>  	return 0;
> --
> 2.17.1

RE: [PATCH v4 1/2] clk: Provide managed helper to get and enable bulk clocks

[Shradha Todi]

> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Sent: 29 January 2024 12:25
> To: Shradha Todi <shradha.t@samsung.com>
> Cc: linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-samsung-
> soc@vger.kernel.org; mturquette@baylibre.com; sboyd@kernel.org;
> jingoohan1@gmail.com; lpieralisi@kernel.org; kw@linux.com; robh@kernel.org;
> bhelgaas@google.com; krzysztof.kozlowski@linaro.org;
> alim.akhtar@samsung.com; linux@armlinux.org.uk;
> m.szyprowski@samsung.com; pankaj.dubey@samsung.com
> Subject: Re: [PATCH v4 1/2] clk: Provide managed helper to get and enable bulk
> clocks
> 
> On Wed, Jan 24, 2024 at 04:08:37PM +0530, Shradha Todi wrote:
> > Provide a managed devm_clk_bulk* wrapper to get and enable all bulk
> > clocks in order to simplify drivers that keeps all clocks enabled for
> > the time of driver operation.
> >
> > Suggested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > Signed-off-by: Shradha Todi <shradha.t@samsung.com>
> > ---
> >  drivers/clk/clk-devres.c | 40
> ++++++++++++++++++++++++++++++++++++++++
> >  include/linux/clk.h      | 24 ++++++++++++++++++++++++
> >  2 files changed, 64 insertions(+)
> >
> > diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c index
> > 4fb4fd4b06bd..cbbd2cc339c3 100644
> > --- a/drivers/clk/clk-devres.c
> > +++ b/drivers/clk/clk-devres.c
> > @@ -182,6 +182,46 @@ int __must_check devm_clk_bulk_get_all(struct
> > device *dev,  }  EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all);
> >
> > +static void devm_clk_bulk_release_all_enable(struct device *dev, void
> > +*res) {
> > +	struct clk_bulk_devres *devres = res;
> > +
> > +	clk_bulk_disable_unprepare(devres->num_clks, devres->clks);
> > +	clk_bulk_put_all(devres->num_clks, devres->clks); }
> > +
> > +int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
> > +					      struct clk_bulk_data **clks) {
> > +	struct clk_bulk_devres *devres;
> > +	int ret;
> > +
> > +	devres = devres_alloc(devm_clk_bulk_release_all_enable,
> > +			      sizeof(*devres), GFP_KERNEL);
> > +	if (!devres)
> > +		return -ENOMEM;
> > +
> > +	ret = clk_bulk_get_all(dev, &devres->clks);
> > +	if (ret > 0) {
> > +		*clks = devres->clks;
> > +		devres->num_clks = ret;
> > +	} else {
> > +		devres_free(devres);
> > +		return ret;
> > +	}
> 
> How about:
> 
> 	ret = clk_bulk_get_all(dev, &devres->clks);
> 	if (ret <= 0) {
> 		devres_free(devres);
> 		return ret;
> 	}
> 
> 	*clks = devres->clks;
> 	devres->num_clks = ret;
> 
> Even though this patch follows the pattern used by the rest of the APIs in the
> driver, IMO above makes it more readable.
> 

Since I have usually seen that maintainers suggest to maintain the coding style of the file, I followed the same.
If you have a stronger reason to change this, please let me know
Marek, Michael, Stephen please let us know what do you think about this?

> > +
> > +	ret = clk_bulk_prepare_enable(devres->num_clks, *clks);
> > +	if (!ret) {
> > +		devres_add(dev, devres);
> > +	} else {
> > +		clk_bulk_put_all(devres->num_clks, devres->clks);
> > +		devres_free(devres);
> > +	}
> > +
> 
> Same as above:
> 
> 	ret = clk_bulk_prepare_enable(devres->num_clks, *clks);
> 	if (ret) {
> 		clk_bulk_put_all(devres->num_clks, devres->clks);
> 		devres_free(devres);
> 		return ret;
> 	}
> 
> 	devres_add(dev, devres);
> 
> > +	return ret;
> 
> 	return 0;
> 

Same as above

> > +}
> > +EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all_enable);
> > +
> >  static int devm_clk_match(struct device *dev, void *res, void *data)
> > {
> >  	struct clk **c = res;
> > diff --git a/include/linux/clk.h b/include/linux/clk.h index
> > 1ef013324237..a005e709b7bd 100644
> > --- a/include/linux/clk.h
> > +++ b/include/linux/clk.h
> > @@ -438,6 +438,23 @@ int __must_check
> > devm_clk_bulk_get_optional(struct device *dev, int num_clks,  int
> __must_check devm_clk_bulk_get_all(struct device *dev,
> >  				       struct clk_bulk_data **clks);
> >
> > +/**
> > + * devm_clk_bulk_get_all_enable - managed get multiple clk consumers and
> > + *				  enable all clks
> 
> "Get and enable all clocks of the consumer (managed)"
> 

Will take this up in the next patchset

> > + * @dev: device for clock "consumer"
> > + * @clks: pointer to the clk_bulk_data table of consumer
> > + *
> > + * Returns success (0) or negative errno.
> > + *
> > + * This helper function allows drivers to get several clk
> 
> "This helper function allows drivers to get all clocks of the consumer and enables
> them..."
> 
> - Mani
> 

Will take this up. Thanks for your review Mani!

> > + * consumers and enable all of them in one operation with management.
> > + * The clks will automatically be disabled and freed when the device
> > + * is unbound.
> > + */
> > +
> > +int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
> > +					      struct clk_bulk_data **clks);
> > +
> >  /**
> >   * devm_clk_get - lookup and obtain a managed reference to a clock producer.
> >   * @dev: device for clock "consumer"
> > @@ -960,6 +977,13 @@ static inline int __must_check
> devm_clk_bulk_get_all(struct device *dev,
> >  	return 0;
> >  }
> >
> > +static inline int __must_check devm_clk_bulk_get_all_enable(struct device
> *dev,
> > +						struct clk_bulk_data **clks)
> > +{
> > +
> > +	return 0;
> > +}
> > +
> >  static inline struct clk *devm_get_clk_from_child(struct device *dev,
> >  				struct device_node *np, const char *con_id)  {
> > --
> > 2.17.1
> >
> 
> --
> மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 1/2] clk: Provide managed helper to get and enable bulk clocks

[Marek Szyprowski]

On 02.02.2024 12:59, Shradha Todi wrote:
>> -----Original Message-----
>> From: Manivannan Sadhasivam<manivannan.sadhasivam@linaro.org>
>> Sent: 29 January 2024 12:25
>> To: Shradha Todi<shradha.t@samsung.com>
>> Subject: Re: [PATCH v4 1/2] clk: Provide managed helper to get and enable bulk
>> clocks
>>
>> On Wed, Jan 24, 2024 at 04:08:37PM +0530, Shradha Todi wrote:
>>> Provide a managed devm_clk_bulk* wrapper to get and enable all bulk
>>> clocks in order to simplify drivers that keeps all clocks enabled for
>>> the time of driver operation.
>>>
>>> Suggested-by: Marek Szyprowski<m.szyprowski@samsung.com>
>>> Signed-off-by: Shradha Todi<shradha.t@samsung.com>
>>> ---
>>>   drivers/clk/clk-devres.c | 40
>> ++++++++++++++++++++++++++++++++++++++++
>>>   include/linux/clk.h      | 24 ++++++++++++++++++++++++
>>>   2 files changed, 64 insertions(+)
>>>
>>> diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c index
>>> 4fb4fd4b06bd..cbbd2cc339c3 100644
>>> --- a/drivers/clk/clk-devres.c
>>> +++ b/drivers/clk/clk-devres.c
>>> @@ -182,6 +182,46 @@ int __must_check devm_clk_bulk_get_all(struct
>>> device *dev,  }  EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all);
>>>
>>> +static void devm_clk_bulk_release_all_enable(struct device *dev, void
>>> +*res) {
>>> +	struct clk_bulk_devres *devres = res;
>>> +
>>> +	clk_bulk_disable_unprepare(devres->num_clks, devres->clks);
>>> +	clk_bulk_put_all(devres->num_clks, devres->clks); }
>>> +
>>> +int __must_check devm_clk_bulk_get_all_enable(struct device *dev,
>>> +					      struct clk_bulk_data **clks) {
>>> +	struct clk_bulk_devres *devres;
>>> +	int ret;
>>> +
>>> +	devres = devres_alloc(devm_clk_bulk_release_all_enable,
>>> +			      sizeof(*devres), GFP_KERNEL);
>>> +	if (!devres)
>>> +		return -ENOMEM;
>>> +
>>> +	ret = clk_bulk_get_all(dev, &devres->clks);
>>> +	if (ret > 0) {
>>> +		*clks = devres->clks;
>>> +		devres->num_clks = ret;
>>> +	} else {
>>> +		devres_free(devres);
>>> +		return ret;
>>> +	}
>> How about:
>>
>> 	ret = clk_bulk_get_all(dev, &devres->clks);
>> 	if (ret <= 0) {
>> 		devres_free(devres);
>> 		return ret;
>> 	}
>>
>> 	*clks = devres->clks;
>> 	devres->num_clks = ret;
>>
>> Even though this patch follows the pattern used by the rest of the APIs in the
>> driver, IMO above makes it more readable.
>>
> Since I have usually seen that maintainers suggest to maintain the coding style of the file, I followed the same.
> If you have a stronger reason to change this, please let me know
> Marek, Michael, Stephen please let us know what do you think about this?

I suggest to keep the same style as is used in the modified file (if it 
doesn't conflict with the rules enforced by checkpatch and kernel's 
coding style).

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland