diff mbox series

[v4,03/17] clk: Unconditionally recursively en-/dis-able clocks

Message ID 20200211060425.1619471-4-seanga2@gmail.com
State Superseded
Headers show
Series riscv: Add Sipeed Maix support | expand

Commit Message

Sean Anderson Feb. 11, 2020, 6:04 a.m. UTC 
For clocks not in the CCF, their parents will not have UCLASS_CLK, so we
just enable them as normal. The enable count is local to the struct clk,
but this will never result in the actual en-/dis-able op being called
(unless the same struct clk is enabled twice).

For clocks in the CCF, we always traverse up the tree when enabling.
Previously, CCF clocks without id set would be skipped, stopping the
traversal too early.

Signed-off-by: Sean Anderson <seanga2 at gmail.com>
Acked-by: Lukasz Majewski <lukma at denx.de>
---

Changes in v4:
- Lint

Changes in v3:
- New

 drivers/clk/clk-uclass.c | 59 ++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 33 deletions(-)

Comments

Rick Chen Feb. 18, 2020, 7:20 a.m. UTC | #1 
Hi Sean

> For clocks not in the CCF, their parents will not have UCLASS_CLK, so we
> just enable them as normal. The enable count is local to the struct clk,
> but this will never result in the actual en-/dis-able op being called
> (unless the same struct clk is enabled twice).
>
> For clocks in the CCF, we always traverse up the tree when enabling.
> Previously, CCF clocks without id set would be skipped, stopping the
> traversal too early.
>
> Signed-off-by: Sean Anderson <seanga2 at gmail.com>
> Acked-by: Lukasz Majewski <lukma at denx.de>
> ---
>
> Changes in v4:
> - Lint
>
> Changes in v3:
> - New
>
>  drivers/clk/clk-uclass.c | 59 ++++++++++++++++++----------------------
>  1 file changed, 26 insertions(+), 33 deletions(-)
>

When I tried to apply this patch-set, there occur a conflict as below:

Applying: clk: Always use the supplied struct clk
Applying: clk: Check that ops of composite clock components exist before calling
Applying: clk: Unconditionally recursively en-/dis-able clocks
error: patch failed: drivers/clk/clk-uclass.c:491
error: drivers/clk/clk-uclass.c: patch does not apply
Patch failed at 0003 clk: Unconditionally recursively en-/dis-able clocks

Thanks
Rick

> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> index 246b9c0ab8..c5f87fee72 100644
> --- a/drivers/clk/clk-uclass.c
> +++ b/drivers/clk/clk-uclass.c
> @@ -491,7 +491,6 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
>  int clk_enable(struct clk *clk)
>  {
>         const struct clk_ops *ops;
> -       struct clk *clkp = NULL;
>         int ret;
>
>         debug("%s(clk=%p \"%s\")\n", __func__, clk, clk->dev->name);
> @@ -500,32 +499,29 @@ int clk_enable(struct clk *clk)
>         ops = clk_dev_ops(clk->dev);
>
>         if (CONFIG_IS_ENABLED(CLK_CCF)) {
> -               /* Take id 0 as a non-valid clk, such as dummy */
> -               if (clk->id && !clk_get_by_id(clk->id, &clkp)) {
> -                       if (clkp->enable_count) {
> -                               clkp->enable_count++;
> -                               return 0;
> -                       }
> -                       if (clkp->dev->parent &&
> -                           device_get_uclass_id(clkp->dev) == UCLASS_CLK) {
> -                               ret = clk_enable(dev_get_clk_ptr(clkp->dev->parent));
> -                               if (ret) {
> -                                       printf("Enable %s failed\n",
> -                                              clkp->dev->parent->name);
> -                                       return ret;
> -                               }
> +               if (clk->enable_count) {
> +                       clk->enable_count++;
> +                       return 0;
> +               }
> +               if (clk->dev->parent &&
> +                   device_get_uclass_id(clk->dev->parent) == UCLASS_CLK) {
> +                       ret = clk_enable(dev_get_clk_ptr(clk->dev->parent));
> +                       if (ret) {
> +                               printf("Enable %s failed\n",
> +                                      clk->dev->parent->name);
> +                               return ret;
>                         }
>                 }
>
>                 if (ops->enable) {
>                         ret = ops->enable(clk);
>                         if (ret) {
> -                               printf("Enable %s failed\n", clk->dev->name);
> +                               printf("Enable %s failed (error %d)\n",
> +                                      clk->dev->name, ret);
>                                 return ret;
>                         }
>                 }
> -               if (clkp)
> -                       clkp->enable_count++;
> +               clk->enable_count++;
>         } else {
>                 if (!ops->enable)
>                         return -ENOSYS;
> @@ -551,7 +547,6 @@ int clk_enable_bulk(struct clk_bulk *bulk)
>  int clk_disable(struct clk *clk)
>  {
>         const struct clk_ops *ops;
> -       struct clk *clkp = NULL;
>         int ret;
>
>         debug("%s(clk=%p)\n", __func__, clk);
> @@ -560,29 +555,27 @@ int clk_disable(struct clk *clk)
>         ops = clk_dev_ops(clk->dev);
>
>         if (CONFIG_IS_ENABLED(CLK_CCF)) {
> -               if (clk->id && !clk_get_by_id(clk->id, &clkp)) {
> -                       if (clkp->enable_count == 0) {
> -                               printf("clk %s already disabled\n",
> -                                      clkp->dev->name);
> -                               return 0;
> -                       }
> -
> -                       if (--clkp->enable_count > 0)
> -                               return 0;
> +               if (clk->enable_count == 0) {
> +                       printf("clk %s already disabled\n",
> +                              clk->dev->name);
> +                       return 0;
>                 }
>
> +               if (--clk->enable_count > 0)
> +                       return 0;
> +
>                 if (ops->disable) {
>                         ret = ops->disable(clk);
>                         if (ret)
>                                 return ret;
>                 }
>
> -               if (clkp && clkp->dev->parent &&
> -                   device_get_uclass_id(clkp->dev) == UCLASS_CLK) {
> -                       ret = clk_disable(dev_get_clk_ptr(clkp->dev->parent));
> +               if (clk->dev->parent &&
> +                   device_get_uclass_id(clk->dev) == UCLASS_CLK) {
> +                       ret = clk_disable(dev_get_clk_ptr(clk->dev->parent));
>                         if (ret) {
> -                               printf("Disable %s failed\n",
> -                                      clkp->dev->parent->name);
> +                               printf("Disable %s failed (error %d)\n",
> +                                      clk->dev->parent->name, ret);
>                                 return ret;
>                         }
>                 }
> --
> 2.25.0
>
Sean Anderson Feb. 18, 2020, 2:11 p.m. UTC | #2 
On 2/18/20 2:20 AM, Rick Chen wrote:
> Hi Sean
> 
>> For clocks not in the CCF, their parents will not have UCLASS_CLK, so we
>> just enable them as normal. The enable count is local to the struct clk,
>> but this will never result in the actual en-/dis-able op being called
>> (unless the same struct clk is enabled twice).
>>
>> For clocks in the CCF, we always traverse up the tree when enabling.
>> Previously, CCF clocks without id set would be skipped, stopping the
>> traversal too early.
>>
>> Signed-off-by: Sean Anderson <seanga2 at gmail.com>
>> Acked-by: Lukasz Majewski <lukma at denx.de>
>> ---
>>
>> Changes in v4:
>> - Lint
>>
>> Changes in v3:
>> - New
>>
>>  drivers/clk/clk-uclass.c | 59 ++++++++++++++++++----------------------
>>  1 file changed, 26 insertions(+), 33 deletions(-)
>>
> 
> When I tried to apply this patch-set, there occur a conflict as below:
> 
> Applying: clk: Always use the supplied struct clk
> Applying: clk: Check that ops of composite clock components exist before calling
> Applying: clk: Unconditionally recursively en-/dis-able clocks
> error: patch failed: drivers/clk/clk-uclass.c:491
> error: drivers/clk/clk-uclass.c: patch does not apply
> Patch failed at 0003 clk: Unconditionally recursively en-/dis-able clocks
> 
> Thanks
> Rick

Hm, perhaps you are applying it on a different commit. My series is
based off of

c00bd81ae0 Merge branch 'next' of https://gitlab.denx.de/u-boot/custodians/u-boot-mpc83xx

What are you applying to?

--Sean
Rick Chen Feb. 19, 2020, 1:43 a.m. UTC | #3 
Hi Sean

> On 2/18/20 2:20 AM, Rick Chen wrote:
> > Hi Sean
> >
> >> For clocks not in the CCF, their parents will not have UCLASS_CLK, so we
> >> just enable them as normal. The enable count is local to the struct clk,
> >> but this will never result in the actual en-/dis-able op being called
> >> (unless the same struct clk is enabled twice).
> >>
> >> For clocks in the CCF, we always traverse up the tree when enabling.
> >> Previously, CCF clocks without id set would be skipped, stopping the
> >> traversal too early.
> >>
> >> Signed-off-by: Sean Anderson <seanga2 at gmail.com>
> >> Acked-by: Lukasz Majewski <lukma at denx.de>
> >> ---
> >>
> >> Changes in v4:
> >> - Lint
> >>
> >> Changes in v3:
> >> - New
> >>
> >>  drivers/clk/clk-uclass.c | 59 ++++++++++++++++++----------------------
> >>  1 file changed, 26 insertions(+), 33 deletions(-)
> >>
> >
> > When I tried to apply this patch-set, there occur a conflict as below:
> >
> > Applying: clk: Always use the supplied struct clk
> > Applying: clk: Check that ops of composite clock components exist before calling
> > Applying: clk: Unconditionally recursively en-/dis-able clocks
> > error: patch failed: drivers/clk/clk-uclass.c:491
> > error: drivers/clk/clk-uclass.c: patch does not apply
> > Patch failed at 0003 clk: Unconditionally recursively en-/dis-able clocks
> >
> > Thanks
> > Rick
>
> Hm, perhaps you are applying it on a different commit. My series is
> based off of
>
> c00bd81ae0 Merge branch 'next' of https://gitlab.denx.de/u-boot/custodians/u-boot-mpc83xx
>
> What are you applying to?

I applied to u-boot/master

Thanks
Rick

>
> --Sean
diff mbox series

Patch

diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
index 246b9c0ab8..c5f87fee72 100644
--- a/drivers/clk/clk-uclass.c
+++ b/drivers/clk/clk-uclass.c
@@ -491,7 +491,6 @@  int clk_set_parent(struct clk *clk, struct clk *parent)
 int clk_enable(struct clk *clk)
 {
 	const struct clk_ops *ops;
-	struct clk *clkp = NULL;
 	int ret;
 
 	debug("%s(clk=%p \"%s\")\n", __func__, clk, clk->dev->name);
@@ -500,32 +499,29 @@  int clk_enable(struct clk *clk)
 	ops = clk_dev_ops(clk->dev);
 
 	if (CONFIG_IS_ENABLED(CLK_CCF)) {
-		/* Take id 0 as a non-valid clk, such as dummy */
-		if (clk->id && !clk_get_by_id(clk->id, &clkp)) {
-			if (clkp->enable_count) {
-				clkp->enable_count++;
-				return 0;
-			}
-			if (clkp->dev->parent &&
-			    device_get_uclass_id(clkp->dev) == UCLASS_CLK) {
-				ret = clk_enable(dev_get_clk_ptr(clkp->dev->parent));
-				if (ret) {
-					printf("Enable %s failed\n",
-					       clkp->dev->parent->name);
-					return ret;
-				}
+		if (clk->enable_count) {
+			clk->enable_count++;
+			return 0;
+		}
+		if (clk->dev->parent &&
+		    device_get_uclass_id(clk->dev->parent) == UCLASS_CLK) {
+			ret = clk_enable(dev_get_clk_ptr(clk->dev->parent));
+			if (ret) {
+				printf("Enable %s failed\n",
+				       clk->dev->parent->name);
+				return ret;
 			}
 		}
 
 		if (ops->enable) {
 			ret = ops->enable(clk);
 			if (ret) {
-				printf("Enable %s failed\n", clk->dev->name);
+				printf("Enable %s failed (error %d)\n",
+				       clk->dev->name, ret);
 				return ret;
 			}
 		}
-		if (clkp)
-			clkp->enable_count++;
+		clk->enable_count++;
 	} else {
 		if (!ops->enable)
 			return -ENOSYS;
@@ -551,7 +547,6 @@  int clk_enable_bulk(struct clk_bulk *bulk)
 int clk_disable(struct clk *clk)
 {
 	const struct clk_ops *ops;
-	struct clk *clkp = NULL;
 	int ret;
 
 	debug("%s(clk=%p)\n", __func__, clk);
@@ -560,29 +555,27 @@  int clk_disable(struct clk *clk)
 	ops = clk_dev_ops(clk->dev);
 
 	if (CONFIG_IS_ENABLED(CLK_CCF)) {
-		if (clk->id && !clk_get_by_id(clk->id, &clkp)) {
-			if (clkp->enable_count == 0) {
-				printf("clk %s already disabled\n",
-				       clkp->dev->name);
-				return 0;
-			}
-
-			if (--clkp->enable_count > 0)
-				return 0;
+		if (clk->enable_count == 0) {
+			printf("clk %s already disabled\n",
+			       clk->dev->name);
+			return 0;
 		}
 
+		if (--clk->enable_count > 0)
+			return 0;
+
 		if (ops->disable) {
 			ret = ops->disable(clk);
 			if (ret)
 				return ret;
 		}
 
-		if (clkp && clkp->dev->parent &&
-		    device_get_uclass_id(clkp->dev) == UCLASS_CLK) {
-			ret = clk_disable(dev_get_clk_ptr(clkp->dev->parent));
+		if (clk->dev->parent &&
+		    device_get_uclass_id(clk->dev) == UCLASS_CLK) {
+			ret = clk_disable(dev_get_clk_ptr(clk->dev->parent));
 			if (ret) {
-				printf("Disable %s failed\n",
-				       clkp->dev->parent->name);
+				printf("Disable %s failed (error %d)\n",
+				       clk->dev->parent->name, ret);
 				return ret;
 			}
 		}