diff mbox series

[v3,5/6] clk: samsung: Extract parent clock enabling to common function

Message ID 20230223041938.22732-6-semen.protsenko@linaro.org (mailing list archive)
State Superseded, archived
Headers show
Series clk: samsung: Add PM support for ARM64 Exynos chips | expand

Commit Message

Sam Protsenko Feb. 23, 2023, 4:19 a.m. UTC 
Extract parent clock enabling from exynos_arm64_register_cmu() to
dedicated function.

Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
---
Changes in v3:
  - Rebased on top of latest soc/for-next tree
  - Added Marek's Acked-by tag

Changes in v2:
  - Rebased on top of latest soc/for-next tree
  - Improved English in kernel doc comment
  - Added clk_prepare_enable() return value check
  - Added exynos_arm64_enable_bus_clk() check in
    exynos_arm64_register_cmu()
  - Changed the commit message to reflect code changes

 drivers/clk/samsung/clk-exynos-arm64.c | 51 ++++++++++++++++++--------
 1 file changed, 35 insertions(+), 16 deletions(-)

Comments

Krzysztof Kozlowski March 6, 2023, 2:35 p.m. UTC | #1 
On 23/02/2023 05:19, Sam Protsenko wrote:
> Extract parent clock enabling from exynos_arm64_register_cmu() to
> dedicated function.
> 
> Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> ---
> Changes in v3:
>   - Rebased on top of latest soc/for-next tree
>   - Added Marek's Acked-by tag
> 
> Changes in v2:
>   - Rebased on top of latest soc/for-next tree
>   - Improved English in kernel doc comment
>   - Added clk_prepare_enable() return value check
>   - Added exynos_arm64_enable_bus_clk() check in
>     exynos_arm64_register_cmu()
>   - Changed the commit message to reflect code changes
> 
>  drivers/clk/samsung/clk-exynos-arm64.c | 51 ++++++++++++++++++--------
>  1 file changed, 35 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/clk/samsung/clk-exynos-arm64.c b/drivers/clk/samsung/clk-exynos-arm64.c
> index b921b9a1134a..2aa3f0a5644e 100644
> --- a/drivers/clk/samsung/clk-exynos-arm64.c
> +++ b/drivers/clk/samsung/clk-exynos-arm64.c
> @@ -56,6 +56,37 @@ static void __init exynos_arm64_init_clocks(struct device_node *np,
>  	iounmap(reg_base);
>  }
>  
> +/**
> + * exynos_arm64_enable_bus_clk - Enable parent clock of specified CMU
> + *
> + * @dev:	Device object; may be NULL if this function is not being
> + *		called from platform driver probe function
> + * @np:		CMU device tree node
> + * @cmu:	CMU data
> + *
> + * Keep CMU parent clock running (needed for CMU registers access).
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +static int __init exynos_arm64_enable_bus_clk(struct device *dev,
> +		struct device_node *np, const struct samsung_cmu_info *cmu)
> +{
> +	struct clk *parent_clk;
> +
> +	if (!cmu->clk_name)
> +		return 0;
> +
> +	if (dev)
> +		parent_clk = clk_get(dev, cmu->clk_name);
> +	else
> +		parent_clk = of_clk_get_by_name(np, cmu->clk_name);
> +
> +	if (IS_ERR(parent_clk))
> +		return PTR_ERR(parent_clk);
> +
> +	return clk_prepare_enable(parent_clk);
> +}
> +
>  /**
>   * exynos_arm64_register_cmu - Register specified Exynos CMU domain
>   * @dev:	Device object; may be NULL if this function is not being
> @@ -72,23 +103,11 @@ static void __init exynos_arm64_init_clocks(struct device_node *np,
>  void __init exynos_arm64_register_cmu(struct device *dev,
>  		struct device_node *np, const struct samsung_cmu_info *cmu)
>  {
> -	/* Keep CMU parent clock running (needed for CMU registers access) */
> -	if (cmu->clk_name) {
> -		struct clk *parent_clk;
> -
> -		if (dev)
> -			parent_clk = clk_get(dev, cmu->clk_name);
> -		else
> -			parent_clk = of_clk_get_by_name(np, cmu->clk_name);
> -
> -		if (IS_ERR(parent_clk)) {
> -			pr_err("%s: could not find bus clock %s; err = %ld\n",
> -			       __func__, cmu->clk_name, PTR_ERR(parent_clk));
> -		} else {
> -			clk_prepare_enable(parent_clk);
> -		}
> -	}
> +	int err;
>  
> +	err = exynos_arm64_enable_bus_clk(dev, np, cmu);
> +	if (err)
> +		panic("%s: could not enable bus clock\n", __func__);

The error handling is changed and not equivalent. I would say that we
could still try to boot even if this failed, so kernel should not panic.
Maybe the parent clock is enabled by bootloader.

Best regards,
Krzysztof
Sam Protsenko March 6, 2023, 11:47 p.m. UTC | #2 
On Mon, 6 Mar 2023 at 08:35, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 23/02/2023 05:19, Sam Protsenko wrote:
> > Extract parent clock enabling from exynos_arm64_register_cmu() to
> > dedicated function.
> >
> > Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org>
> > ---
> > Changes in v3:
> >   - Rebased on top of latest soc/for-next tree
> >   - Added Marek's Acked-by tag
> >
> > Changes in v2:
> >   - Rebased on top of latest soc/for-next tree
> >   - Improved English in kernel doc comment
> >   - Added clk_prepare_enable() return value check
> >   - Added exynos_arm64_enable_bus_clk() check in
> >     exynos_arm64_register_cmu()
> >   - Changed the commit message to reflect code changes
> >
> >  drivers/clk/samsung/clk-exynos-arm64.c | 51 ++++++++++++++++++--------
> >  1 file changed, 35 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/clk/samsung/clk-exynos-arm64.c b/drivers/clk/samsung/clk-exynos-arm64.c
> > index b921b9a1134a..2aa3f0a5644e 100644
> > --- a/drivers/clk/samsung/clk-exynos-arm64.c
> > +++ b/drivers/clk/samsung/clk-exynos-arm64.c
> > @@ -56,6 +56,37 @@ static void __init exynos_arm64_init_clocks(struct device_node *np,
> >       iounmap(reg_base);
> >  }
> >
> > +/**
> > + * exynos_arm64_enable_bus_clk - Enable parent clock of specified CMU
> > + *
> > + * @dev:     Device object; may be NULL if this function is not being
> > + *           called from platform driver probe function
> > + * @np:              CMU device tree node
> > + * @cmu:     CMU data
> > + *
> > + * Keep CMU parent clock running (needed for CMU registers access).
> > + *
> > + * Return: 0 on success or a negative error code on failure.
> > + */
> > +static int __init exynos_arm64_enable_bus_clk(struct device *dev,
> > +             struct device_node *np, const struct samsung_cmu_info *cmu)
> > +{
> > +     struct clk *parent_clk;
> > +
> > +     if (!cmu->clk_name)
> > +             return 0;
> > +
> > +     if (dev)
> > +             parent_clk = clk_get(dev, cmu->clk_name);
> > +     else
> > +             parent_clk = of_clk_get_by_name(np, cmu->clk_name);
> > +
> > +     if (IS_ERR(parent_clk))
> > +             return PTR_ERR(parent_clk);
> > +
> > +     return clk_prepare_enable(parent_clk);
> > +}
> > +
> >  /**
> >   * exynos_arm64_register_cmu - Register specified Exynos CMU domain
> >   * @dev:     Device object; may be NULL if this function is not being
> > @@ -72,23 +103,11 @@ static void __init exynos_arm64_init_clocks(struct device_node *np,
> >  void __init exynos_arm64_register_cmu(struct device *dev,
> >               struct device_node *np, const struct samsung_cmu_info *cmu)
> >  {
> > -     /* Keep CMU parent clock running (needed for CMU registers access) */
> > -     if (cmu->clk_name) {
> > -             struct clk *parent_clk;
> > -
> > -             if (dev)
> > -                     parent_clk = clk_get(dev, cmu->clk_name);
> > -             else
> > -                     parent_clk = of_clk_get_by_name(np, cmu->clk_name);
> > -
> > -             if (IS_ERR(parent_clk)) {
> > -                     pr_err("%s: could not find bus clock %s; err = %ld\n",
> > -                            __func__, cmu->clk_name, PTR_ERR(parent_clk));
> > -             } else {
> > -                     clk_prepare_enable(parent_clk);
> > -             }
> > -     }
> > +     int err;
> >
> > +     err = exynos_arm64_enable_bus_clk(dev, np, cmu);
> > +     if (err)
> > +             panic("%s: could not enable bus clock\n", __func__);
>
> The error handling is changed and not equivalent. I would say that we
> could still try to boot even if this failed, so kernel should not panic.
> Maybe the parent clock is enabled by bootloader.
>

Agreed, I've probably overlooked that one when making all the
refactoring. The same stands for the patch #6. Will rework and send
out those two separately soon, as the rest of patches you already
applied.

Thanks!

> Best regards,
> Krzysztof
>
diff mbox series

Patch

diff --git a/drivers/clk/samsung/clk-exynos-arm64.c b/drivers/clk/samsung/clk-exynos-arm64.c
index b921b9a1134a..2aa3f0a5644e 100644
--- a/drivers/clk/samsung/clk-exynos-arm64.c
+++ b/drivers/clk/samsung/clk-exynos-arm64.c
@@ -56,6 +56,37 @@  static void __init exynos_arm64_init_clocks(struct device_node *np,
 	iounmap(reg_base);
 }
 
+/**
+ * exynos_arm64_enable_bus_clk - Enable parent clock of specified CMU
+ *
+ * @dev:	Device object; may be NULL if this function is not being
+ *		called from platform driver probe function
+ * @np:		CMU device tree node
+ * @cmu:	CMU data
+ *
+ * Keep CMU parent clock running (needed for CMU registers access).
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+static int __init exynos_arm64_enable_bus_clk(struct device *dev,
+		struct device_node *np, const struct samsung_cmu_info *cmu)
+{
+	struct clk *parent_clk;
+
+	if (!cmu->clk_name)
+		return 0;
+
+	if (dev)
+		parent_clk = clk_get(dev, cmu->clk_name);
+	else
+		parent_clk = of_clk_get_by_name(np, cmu->clk_name);
+
+	if (IS_ERR(parent_clk))
+		return PTR_ERR(parent_clk);
+
+	return clk_prepare_enable(parent_clk);
+}
+
 /**
  * exynos_arm64_register_cmu - Register specified Exynos CMU domain
  * @dev:	Device object; may be NULL if this function is not being
@@ -72,23 +103,11 @@  static void __init exynos_arm64_init_clocks(struct device_node *np,
 void __init exynos_arm64_register_cmu(struct device *dev,
 		struct device_node *np, const struct samsung_cmu_info *cmu)
 {
-	/* Keep CMU parent clock running (needed for CMU registers access) */
-	if (cmu->clk_name) {
-		struct clk *parent_clk;
-
-		if (dev)
-			parent_clk = clk_get(dev, cmu->clk_name);
-		else
-			parent_clk = of_clk_get_by_name(np, cmu->clk_name);
-
-		if (IS_ERR(parent_clk)) {
-			pr_err("%s: could not find bus clock %s; err = %ld\n",
-			       __func__, cmu->clk_name, PTR_ERR(parent_clk));
-		} else {
-			clk_prepare_enable(parent_clk);
-		}
-	}
+	int err;
 
+	err = exynos_arm64_enable_bus_clk(dev, np, cmu);
+	if (err)
+		panic("%s: could not enable bus clock\n", __func__);
 	exynos_arm64_init_clocks(np, cmu->clk_regs, cmu->nr_clk_regs);
 	samsung_cmu_register_one(np, cmu);
 }