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

[PATCH v3 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.
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

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                | 41 ++++++++++++++++++
 drivers/pci/controller/dwc/pci-exynos.c | 55 +++----------------------
 include/linux/clk.h                     | 25 +++++++++++
 3 files changed, 71 insertions(+), 50 deletions(-)

-- 
2.17.1

[PATCH v3 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 | 41 ++++++++++++++++++++++++++++++++++++++++
 include/linux/clk.h      | 25 ++++++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index 4fb4fd4b06bd..05b0ff4bc1d4 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -102,6 +102,7 @@ EXPORT_SYMBOL_GPL(devm_clk_get_optional_enabled);
 struct clk_bulk_devres {
 	struct clk_bulk_data *clks;
 	int num_clks;
+	void (*exit)(int num_clks, const struct clk_bulk_data *clks);
 };
 
 static void devm_clk_bulk_release(struct device *dev, void *res)
@@ -182,6 +183,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_enabled(struct device *dev, void *res)
+{
+	struct clk_bulk_devres *devres = res;
+
+	if (devres->exit)
+		devres->exit(devres->num_clks, devres->clks);
+
+	clk_bulk_put_all(devres->num_clks, devres->clks);
+}
+
+int __must_check devm_clk_bulk_get_all_enabled(struct device *dev,
+				  struct clk_bulk_data **clks, int *num_clks)
+{
+	struct clk_bulk_devres *devres;
+	int ret;
+
+	devres = devres_alloc(devm_clk_bulk_release_all_enabled,
+			      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;
+		*num_clks = ret;
+		devres_add(dev, devres);
+	} else {
+		devres_free(devres);
+		return ret;
+	}
+
+	ret = clk_bulk_prepare_enable(devres->num_clks, *clks);
+	if (!ret)
+		devres->exit = clk_bulk_disable_unprepare;
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all_enabled);
+
 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..bf3e9bee5754 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -438,6 +438,24 @@ 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_enabled - managed get multiple clk consumers and
+ *					enable all clk
+ * @dev: device for clock "consumer"
+ * @clks: pointer to the clk_bulk_data table of consumer
+ * @num_clks: out parameter to store the number of clk_bulk_data
+ *
+ * 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_enabled(struct device *dev,
+				struct clk_bulk_data **clks, int *num_clks);
+
 /**
  * devm_clk_get - lookup and obtain a managed reference to a clock producer.
  * @dev: device for clock "consumer"
@@ -960,6 +978,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_enabled(struct device *dev,
+				struct clk_bulk_data **clks, int *num_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 v3 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 10, 2024 4:31 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; Shradha
> Todi <shradha.t@samsung.com>
> Subject: [PATCH v3 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>
> ---
>  drivers/clk/clk-devres.c | 41
> ++++++++++++++++++++++++++++++++++++++++
>  include/linux/clk.h      | 25 ++++++++++++++++++++++++
>  2 files changed, 66 insertions(+)
> 
> diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c index
> 4fb4fd4b06bd..05b0ff4bc1d4 100644
> --- a/drivers/clk/clk-devres.c
> +++ b/drivers/clk/clk-devres.c
> @@ -102,6 +102,7 @@
> EXPORT_SYMBOL_GPL(devm_clk_get_optional_enabled);
>  struct clk_bulk_devres {
>  	struct clk_bulk_data *clks;
>  	int num_clks;
> +	void (*exit)(int num_clks, const struct clk_bulk_data *clks);
>  };
> 
>  static void devm_clk_bulk_release(struct device *dev, void *res) @@ -182,6
> +183,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_enabled(struct device *dev, void
May be devm_clk_bulk_release_all_disable()

Also this is similar to already existing devm_clk_bulk_release_all(), may be you can reuse this function
And add the exit() callback in devm_clk_bulk_release_all()

> +*res) {
> +	struct clk_bulk_devres *devres = res;
> +
> +	if (devres->exit)
> +		devres->exit(devres->num_clks, devres->clks);
> +
> +	clk_bulk_put_all(devres->num_clks, devres->clks); }
> +
> +int __must_check devm_clk_bulk_get_all_enabled(struct device *dev,

May be devm_clk_bulk_get_all_enable() is more suitable

> +				  struct clk_bulk_data **clks, int *num_clks) {
> +	struct clk_bulk_devres *devres;
> +	int ret;
> +
> +	devres = devres_alloc(devm_clk_bulk_release_all_enabled,
> +			      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;
> +		*num_clks = ret;
> +		devres_add(dev, devres);
> +	} else {
> +		devres_free(devres);
> +		return ret;
> +	}
> +
> +	ret = clk_bulk_prepare_enable(devres->num_clks, *clks);
> +	if (!ret)
> +		devres->exit = clk_bulk_disable_unprepare;
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all_enabled);
> +
>  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..bf3e9bee5754 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -438,6 +438,24 @@ 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_enabled - managed get multiple clk consumers
> and
> + *					enable all clk
> + * @dev: device for clock "consumer"
> + * @clks: pointer to the clk_bulk_data table of consumer
> + * @num_clks: out parameter to store the number of clk_bulk_data
> + *
> + * 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_enabled(struct device *dev,
> +				struct clk_bulk_data **clks, int *num_clks);
> +
>  /**
>   * devm_clk_get - lookup and obtain a managed reference to a clock
> producer.
>   * @dev: device for clock "consumer"
> @@ -960,6 +978,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_enabled(struct
> device *dev,
> +				struct clk_bulk_data **clks, int *num_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 v3 1/2] clk: Provide managed helper to get and enable bulk clocks

[Shradha Todi]

> -----Original Message-----
> From: Alim Akhtar <alim.akhtar@samsung.com>
> Sent: 10 January 2024 18:01
> To: 'Shradha Todi' <shradha.t@samsung.com>; 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; linux@armlinux.org.uk;
> m.szyprowski@samsung.com; manivannan.sadhasivam@linaro.org;
> alim.akhtar@samsung.com
> Subject: RE: [PATCH v3 1/2] clk: Provide managed helper to get and enable bulk
> clocks
> 
> Hi Shradha,
> 
> > -----Original Message-----
> > From: Shradha Todi <shradha.t@samsung.com>
> > Sent: Wednesday, January 10, 2024 4:31 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; Shradha
> > Todi <shradha.t@samsung.com>
> > Subject: [PATCH v3 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>
> > ---
> >  drivers/clk/clk-devres.c | 41
> > ++++++++++++++++++++++++++++++++++++++++
> >  include/linux/clk.h      | 25 ++++++++++++++++++++++++
> >  2 files changed, 66 insertions(+)
> >
> > diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c index
> > 4fb4fd4b06bd..05b0ff4bc1d4 100644
> > --- a/drivers/clk/clk-devres.c
> > +++ b/drivers/clk/clk-devres.c
> > @@ -102,6 +102,7 @@
> > EXPORT_SYMBOL_GPL(devm_clk_get_optional_enabled);
> >  struct clk_bulk_devres {
> >  	struct clk_bulk_data *clks;
> >  	int num_clks;
> > +	void (*exit)(int num_clks, const struct clk_bulk_data *clks);
> >  };
> >
> >  static void devm_clk_bulk_release(struct device *dev, void *res) @@
> > -182,6
> > +183,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_enabled(struct device *dev,
> > +void
> May be devm_clk_bulk_release_all_disable()
> 

Will change this in the next patchset

> Also this is similar to already existing devm_clk_bulk_release_all(), may be you
> can reuse this function And add the exit() callback in devm_clk_bulk_release_all()
> 

Since I'm planning to remove the exit callback in the next version as suggested by Manivannan, I will have to
go with a new release function

> > +*res) {
> > +	struct clk_bulk_devres *devres = res;
> > +
> > +	if (devres->exit)
> > +		devres->exit(devres->num_clks, devres->clks);
> > +
> > +	clk_bulk_put_all(devres->num_clks, devres->clks); }
> > +
> > +int __must_check devm_clk_bulk_get_all_enabled(struct device *dev,
> 
> May be devm_clk_bulk_get_all_enable() is more suitable
> 

Will take this in the next patchset! Thanks for the review

> > +				  struct clk_bulk_data **clks, int *num_clks) {
> > +	struct clk_bulk_devres *devres;
> > +	int ret;
> > +
> > +	devres = devres_alloc(devm_clk_bulk_release_all_enabled,
> > +			      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;
> > +		*num_clks = ret;
> > +		devres_add(dev, devres);
> > +	} else {
> > +		devres_free(devres);
> > +		return ret;
> > +	}
> > +
> > +	ret = clk_bulk_prepare_enable(devres->num_clks, *clks);
> > +	if (!ret)
> > +		devres->exit = clk_bulk_disable_unprepare;
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all_enabled);
> > +
> >  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..bf3e9bee5754 100644
> > --- a/include/linux/clk.h
> > +++ b/include/linux/clk.h
> > @@ -438,6 +438,24 @@ 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_enabled - managed get multiple clk consumers
> > and
> > + *					enable all clk
> > + * @dev: device for clock "consumer"
> > + * @clks: pointer to the clk_bulk_data table of consumer
> > + * @num_clks: out parameter to store the number of clk_bulk_data
> > + *
> > + * 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_enabled(struct device *dev,
> > +				struct clk_bulk_data **clks, int *num_clks);
> > +
> >  /**
> >   * devm_clk_get - lookup and obtain a managed reference to a clock
> > producer.
> >   * @dev: device for clock "consumer"
> > @@ -960,6 +978,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_enabled(struct
> > device *dev,
> > +				struct clk_bulk_data **clks, int *num_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 v3 1/2] clk: Provide managed helper to get and enable bulk clocks

[Manivannan Sadhasivam]

On Wed, Jan 10, 2024 at 04:31:14PM +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 | 41 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/clk.h      | 25 ++++++++++++++++++++++++
>  2 files changed, 66 insertions(+)
> 
> diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
> index 4fb4fd4b06bd..05b0ff4bc1d4 100644
> --- a/drivers/clk/clk-devres.c
> +++ b/drivers/clk/clk-devres.c
> @@ -102,6 +102,7 @@ EXPORT_SYMBOL_GPL(devm_clk_get_optional_enabled);
>  struct clk_bulk_devres {
>  	struct clk_bulk_data *clks;
>  	int num_clks;
> +	void (*exit)(int num_clks, const struct clk_bulk_data *clks);
>  };
>  
>  static void devm_clk_bulk_release(struct device *dev, void *res)
> @@ -182,6 +183,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_enabled(struct device *dev, void *res)
> +{
> +	struct clk_bulk_devres *devres = res;
> +
> +	if (devres->exit)
> +		devres->exit(devres->num_clks, devres->clks);
> +
> +	clk_bulk_put_all(devres->num_clks, devres->clks);
> +}
> +
> +int __must_check devm_clk_bulk_get_all_enabled(struct device *dev,
> +				  struct clk_bulk_data **clks, int *num_clks)

What is the user supposed to do with "num_clks" when you are already handling
the enable part?

> +{
> +	struct clk_bulk_devres *devres;
> +	int ret;
> +
> +	devres = devres_alloc(devm_clk_bulk_release_all_enabled,
> +			      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;
> +		*num_clks = ret;
> +		devres_add(dev, devres);

If you move the statements inside this condition to the end of this function,
you could get rid of the exit() callback and directly use
clk_bulk_disable_unprepare() in devm_clk_bulk_release_all_enabled().

> +	} else {
> +		devres_free(devres);
> +		return ret;
> +	}
> +
> +	ret = clk_bulk_prepare_enable(devres->num_clks, *clks);
> +	if (!ret)
> +		devres->exit = clk_bulk_disable_unprepare;

Here you can just do clk_bulk_put_all() and devres_free() directly because you
know that the driver won't proceed after this error.

- Mani

> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all_enabled);
> +
>  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..bf3e9bee5754 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -438,6 +438,24 @@ 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_enabled - managed get multiple clk consumers and
> + *					enable all clk
> + * @dev: device for clock "consumer"
> + * @clks: pointer to the clk_bulk_data table of consumer
> + * @num_clks: out parameter to store the number of clk_bulk_data
> + *
> + * 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_enabled(struct device *dev,
> +				struct clk_bulk_data **clks, int *num_clks);
> +
>  /**
>   * devm_clk_get - lookup and obtain a managed reference to a clock producer.
>   * @dev: device for clock "consumer"
> @@ -960,6 +978,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_enabled(struct device *dev,
> +				struct clk_bulk_data **clks, int *num_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 v3 1/2] clk: Provide managed helper to get and enable bulk clocks

[Shradha Todi]

> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Sent: 20 January 2024 20:33
> 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
> Subject: Re: [PATCH v3 1/2] clk: Provide managed helper to get and enable bulk
> clocks
> 
> On Wed, Jan 10, 2024 at 04:31:14PM +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 | 41
> ++++++++++++++++++++++++++++++++++++++++
> >  include/linux/clk.h      | 25 ++++++++++++++++++++++++
> >  2 files changed, 66 insertions(+)
> >
> > diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c index
> > 4fb4fd4b06bd..05b0ff4bc1d4 100644
> > --- a/drivers/clk/clk-devres.c
> > +++ b/drivers/clk/clk-devres.c
> > @@ -102,6 +102,7 @@
> EXPORT_SYMBOL_GPL(devm_clk_get_optional_enabled);
> >  struct clk_bulk_devres {
> >  	struct clk_bulk_data *clks;
> >  	int num_clks;
> > +	void (*exit)(int num_clks, const struct clk_bulk_data *clks);
> >  };
> >
> >  static void devm_clk_bulk_release(struct device *dev, void *res) @@
> > -182,6 +183,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_enabled(struct device *dev,
> > +void *res) {
> > +	struct clk_bulk_devres *devres = res;
> > +
> > +	if (devres->exit)
> > +		devres->exit(devres->num_clks, devres->clks);
> > +
> > +	clk_bulk_put_all(devres->num_clks, devres->clks); }
> > +
> > +int __must_check devm_clk_bulk_get_all_enabled(struct device *dev,
> > +				  struct clk_bulk_data **clks, int *num_clks)
> 
> What is the user supposed to do with "num_clks" when you are already handling
> the enable part?
> 

Since the initial clk_bulk_get_all was returning the num_clks value, the thought was to maintain this
as the user might want to have a check in their driver whether x number of clocks were successfully
retrieved and enabled.

> > +{
> > +	struct clk_bulk_devres *devres;
> > +	int ret;
> > +
> > +	devres = devres_alloc(devm_clk_bulk_release_all_enabled,
> > +			      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;
> > +		*num_clks = ret;
> > +		devres_add(dev, devres);
> 
> If you move the statements inside this condition to the end of this function, you
> could get rid of the exit() callback and directly use
> clk_bulk_disable_unprepare() in devm_clk_bulk_release_all_enabled().
> 

Okay! I will change this in the next patchset as suggested

> > +	} else {
> > +		devres_free(devres);
> > +		return ret;
> > +	}
> > +
> > +	ret = clk_bulk_prepare_enable(devres->num_clks, *clks);
> > +	if (!ret)
> > +		devres->exit = clk_bulk_disable_unprepare;
> 
> Here you can just do clk_bulk_put_all() and devres_free() directly because you
> know that the driver won't proceed after this error.
> 
> - Mani
> 

Thanks for the review, Mani. Will change this as suggested!

> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all_enabled);
> > +
> >  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..bf3e9bee5754 100644
> > --- a/include/linux/clk.h
> > +++ b/include/linux/clk.h
> > @@ -438,6 +438,24 @@ 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_enabled - managed get multiple clk consumers and
> > + *					enable all clk
> > + * @dev: device for clock "consumer"
> > + * @clks: pointer to the clk_bulk_data table of consumer
> > + * @num_clks: out parameter to store the number of clk_bulk_data
> > + *
> > + * 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_enabled(struct device *dev,
> > +				struct clk_bulk_data **clks, int *num_clks);
> > +
> >  /**
> >   * devm_clk_get - lookup and obtain a managed reference to a clock producer.
> >   * @dev: device for clock "consumer"
> > @@ -960,6 +978,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_enabled(struct device
> *dev,
> > +				struct clk_bulk_data **clks, int *num_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 v3 1/2] clk: Provide managed helper to get and enable bulk clocks

['Manivannan Sadhasivam']

On Wed, Jan 24, 2024 at 12:23:15PM +0530, Shradha Todi wrote:
> 
> 
> > -----Original Message-----
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Sent: 20 January 2024 20:33
> > 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
> > Subject: Re: [PATCH v3 1/2] clk: Provide managed helper to get and enable bulk
> > clocks
> > 
> > On Wed, Jan 10, 2024 at 04:31:14PM +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 | 41
> > ++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/clk.h      | 25 ++++++++++++++++++++++++
> > >  2 files changed, 66 insertions(+)
> > >
> > > diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c index
> > > 4fb4fd4b06bd..05b0ff4bc1d4 100644
> > > --- a/drivers/clk/clk-devres.c
> > > +++ b/drivers/clk/clk-devres.c
> > > @@ -102,6 +102,7 @@
> > EXPORT_SYMBOL_GPL(devm_clk_get_optional_enabled);
> > >  struct clk_bulk_devres {
> > >  	struct clk_bulk_data *clks;
> > >  	int num_clks;
> > > +	void (*exit)(int num_clks, const struct clk_bulk_data *clks);
> > >  };
> > >
> > >  static void devm_clk_bulk_release(struct device *dev, void *res) @@
> > > -182,6 +183,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_enabled(struct device *dev,
> > > +void *res) {
> > > +	struct clk_bulk_devres *devres = res;
> > > +
> > > +	if (devres->exit)
> > > +		devres->exit(devres->num_clks, devres->clks);
> > > +
> > > +	clk_bulk_put_all(devres->num_clks, devres->clks); }
> > > +
> > > +int __must_check devm_clk_bulk_get_all_enabled(struct device *dev,
> > > +				  struct clk_bulk_data **clks, int *num_clks)
> > 
> > What is the user supposed to do with "num_clks" when you are already handling
> > the enable part?
> > 
> 
> Since the initial clk_bulk_get_all was returning the num_clks value, the thought was to maintain this
> as the user might want to have a check in their driver whether x number of clocks were successfully
> retrieved and enabled.
> 

IIUC, the returned clock count is only used for enabling or disabling the
clocks later. Since you are already handling it inside this API, I do not see a
necessity to return the count.

- Mani

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

[PATCH v3 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. The total number of clocks supplied is
obtained and enabled using the devm_clk_bulk_get_all_enabled() API.

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

diff --git a/drivers/pci/controller/dwc/pci-exynos.c b/drivers/pci/controller/dwc/pci-exynos.c
index ec5611005566..3c0bc2505602 100644
--- a/drivers/pci/controller/dwc/pci-exynos.c
+++ b/drivers/pci/controller/dwc/pci-exynos.c
@@ -54,43 +54,12 @@
 struct exynos_pcie {
 	struct dw_pcie			pci;
 	void __iomem			*elbi_base;
-	struct clk			*clk;
-	struct clk			*bus_clk;
+	struct clk_bulk_data		*clks;
+	int				clk_cnt;
 	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 +301,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_enabled(dev, &ep->clks, &ep->clk_cnt);
+	if (ret < 0)
+		return ret;
 
 	ep->supplies[0].supply = "vdd18";
 	ep->supplies[1].supply = "vdd10";
@@ -351,10 +312,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 +326,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 +339,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