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

[PATCH v6 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:
 - https://lore.kernel.org/all/20240124103838.32478-1-shradha.t@samsung.com/
 - 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
v5:
 - https://lore.kernel.org/lkml/20240213132751.46813-1-shradha.t@samsung.com/
 - Rephrased comments for better readability
v6:
 - Removed extra new line

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                     | 22 ++++++++++
 3 files changed, 66 insertions(+), 50 deletions(-)

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v6 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>
Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Shradha Todi <shradha.t@samsung.com>
---
 drivers/clk/clk-devres.c | 40 ++++++++++++++++++++++++++++++++++++++++
 include/linux/clk.h      | 22 ++++++++++++++++++++++
 2 files changed, 62 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..a8e6b045b848 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -438,6 +438,22 @@ 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 - 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 all clocks of the
+ * consumer and enables 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 +976,12 @@ 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Stephen Boyd]

Quoting Shradha Todi (2024-02-20 00:40:45)
> 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>
> Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Signed-off-by: Shradha Todi <shradha.t@samsung.com>
> ---

Applied to clk-next

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Dan Carpenter]

On Tue, Feb 20, 2024 at 02:10:45PM +0530, Shradha Todi wrote:
> +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) {

I feel like this should be >= instead of >.  There aren't any callers
of this function yet so we can't see what's in *clks at the start but
it's easy to imagine a situation where it's bad data.

> +		*clks = devres->clks;
> +		devres->num_clks = ret;
> +	} else {
> +		devres_free(devres);
> +		return ret;

When clk_bulk_get_all() returns zero then we return success here.

regards,
dan carpenter

> +	}
> +
> +	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);
> +


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Shradha Todi]

> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@linaro.org>
> Sent: 05 March 2024 14:20
> 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; manivannan.sadhasivam@linaro.org;
> pankaj.dubey@samsung.com; gost.dev@samsung.com
> Subject: Re: [PATCH v6 1/2] clk: Provide managed helper to get and enable bulk
> clocks
> 
> On Tue, Feb 20, 2024 at 02:10:45PM +0530, Shradha Todi wrote:
> > +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) {
> 
> I feel like this should be >= instead of >.  There aren't any callers of this
function
> yet so we can't see what's in *clks at the start but it's easy to imagine a
situation
> where it's bad data.
> 

Reference for this piece of code has been taken from devm_clk_bulk_get_all()
which
has multiple callers, so it's safe. If we make this >=, it will hold on to the
devres node
even though there are no clocks.

> > +		*clks = devres->clks;
> > +		devres->num_clks = ret;
> > +	} else {
> > +		devres_free(devres);
> > +		return ret;
> 
> When clk_bulk_get_all() returns zero then we return success here.
> 

Yes, we are returning success in case there are no clocks as well. In case there
are no
clocks defined in the DT-node, then it is assumed that the driver does not need
any
clock manipulation for driver operation. So the intention here is to continue
without
throwing error.

> regards,
> dan carpenter
> 
> > +	}
> > +
> > +	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);
> > +



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Stephen Boyd]

Quoting Shradha Todi (2024-03-06 04:13:03)
> > 
> > When clk_bulk_get_all() returns zero then we return success here.
> > 
> 
> Yes, we are returning success in case there are no clocks as well. In case there
> are no
> clocks defined in the DT-node, then it is assumed that the driver does not need
> any
> clock manipulation for driver operation. So the intention here is to continue
> without
> throwing error.

Maybe we shouldn't even return the clks to the caller. Do you have any
use for the clk pointers?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Shradha Todi]

> -----Original Message-----
> From: Stephen Boyd <sboyd@kernel.org>
> Sent: 09 March 2024 06:21
> To: 'Dan Carpenter' <dan.carpenter@linaro.org>; 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; 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;
> gost.dev@samsung.com
> Subject: RE: [PATCH v6 1/2] clk: Provide managed helper to get and enable bulk
> clocks
> 
> Quoting Shradha Todi (2024-03-06 04:13:03)
> > >
> > > When clk_bulk_get_all() returns zero then we return success here.
> > >
> >
> > Yes, we are returning success in case there are no clocks as well. In
> > case there are no clocks defined in the DT-node, then it is assumed
> > that the driver does not need any clock manipulation for driver
> > operation. So the intention here is to continue without throwing
> > error.
> 
> Maybe we shouldn't even return the clks to the caller. Do you have any use for
> the clk pointers?

The intention to return the clk pointers was in the case where caller wants to
manipulate a particular clock in certain conditions. They can obtain the clock pointer
and use clk_set_parent, clk_set_rate on those particular clocks.
But I understand that in that case users can use existing clk_bulk_get_all() API.
So, should I go ahead and send v7?


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Stephen Boyd]

Quoting Shradha Todi (2024-03-15 04:34:44)
> > 
> > Quoting Shradha Todi (2024-03-06 04:13:03)
> > > >
> > > > When clk_bulk_get_all() returns zero then we return success here.
> > > >
> > >
> > > Yes, we are returning success in case there are no clocks as well. In
> > > case there are no clocks defined in the DT-node, then it is assumed
> > > that the driver does not need any clock manipulation for driver
> > > operation. So the intention here is to continue without throwing
> > > error.
> > 
> > Maybe we shouldn't even return the clks to the caller. Do you have any use for
> > the clk pointers?
> 
> The intention to return the clk pointers was in the case where caller wants to
> manipulate a particular clock in certain conditions. They can obtain the clock pointer
> and use clk_set_parent, clk_set_rate on those particular clocks.
> But I understand that in that case users can use existing clk_bulk_get_all() API.
> So, should I go ahead and send v7?
> 

No, I think this is fine.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Krzysztof Wilczyński]

Hello,

> 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.

Applied to controller/exynos, thank you!

[1/1] PCI: exynos: Adapt to use bulk clock APIs
      https://git.kernel.org/pci/pci/c/358e579a9da2

	Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel