diff mbox series

[v6,03/19] clk: Unconditionally recursively en-/dis-able clocks

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

Commit Message

Sean Anderson March 5, 2020, 6:12 p.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 v5:
- Clear enable_count on request

Changes in v4:
- Lint

Changes in v3:
- New

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

Comments

Rick Chen March 10, 2020, 6:29 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 v5:
> - Clear enable_count on request
>
> Changes in v4:
> - Lint
>
> Changes in v3:
> - New
>
>  drivers/clk/clk-uclass.c | 60 ++++++++++++++++++----------------------
>  1 file changed, 27 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> index d497737298..3b711395b8 100644
> --- a/drivers/clk/clk-uclass.c
> +++ b/drivers/clk/clk-uclass.c
> @@ -410,6 +410,7 @@ int clk_request(struct udevice *dev, struct clk *clk)
>         ops = clk_dev_ops(dev);
>
>         clk->dev = dev;
> +       clk->enable_count = 0;

Lukasz has taged Acked-by in v3.
Re: [PATCH v3 03/12] clk: Unconditionally recursively en-/dis-able clocks

This patch shall be fixed and freeze.
But you add a new modification (Clear enable_count on request)in v5 and v6.
Although it seem to look fine and nothing big deal.
I just highlight it here, and if Lukasz have no other comment further.
I will pull it via riscv tree later.

Thanks,
Rick



>
>         if (!ops->request)
>                 return 0;
> @@ -522,7 +523,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);
> @@ -531,32 +531,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;
> @@ -582,7 +579,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);
> @@ -591,29 +587,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
>
Rick Chen March 10, 2020, 6:51 a.m. UTC | #2 
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 v5:
> > - Clear enable_count on request
> >
> > Changes in v4:
> > - Lint
> >
> > Changes in v3:
> > - New
> >
> >  drivers/clk/clk-uclass.c | 60 ++++++++++++++++++----------------------
> >  1 file changed, 27 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> > index d497737298..3b711395b8 100644
> > --- a/drivers/clk/clk-uclass.c
> > +++ b/drivers/clk/clk-uclass.c
> > @@ -410,6 +410,7 @@ int clk_request(struct udevice *dev, struct clk *clk)
> >         ops = clk_dev_ops(dev);
> >
> >         clk->dev = dev;
> > +       clk->enable_count = 0;
>
> Lukasz has taged Acked-by in v3.
> Re: [PATCH v3 03/12] clk: Unconditionally recursively en-/dis-able clocks
>
> This patch shall be fixed and freeze.
> But you add a new modification (Clear enable_count on request)in v5 and v6.
> Although it seem to look fine and nothing big deal.
> I just highlight it here, and if Lukasz have no other comment further.
> I will pull it via riscv tree later.
>

I have told you in v5, that this patch have conflict with
u-boot/master please rebase it.
But it still conflict in v6.

https://patchwork.ozlabs.org/patch/1246836/

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:522
error: drivers/clk/clk-uclass.c: patch does not apply
Patch failed at 0003 clk: Unconditionally recursively en-/dis-able clocks
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


> Thanks,
> Rick
>
>
>
> >
> >         if (!ops->request)
> >                 return 0;
> > @@ -522,7 +523,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);
> > @@ -531,32 +531,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;
> > @@ -582,7 +579,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);
> > @@ -591,29 +587,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 March 10, 2020, 1:14 p.m. UTC | #3 
On 3/10/20 2:51 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 v5:
>>> - Clear enable_count on request
>>>
>>> Changes in v4:
>>> - Lint
>>>
>>> Changes in v3:
>>> - New
>>>
>>>  drivers/clk/clk-uclass.c | 60 ++++++++++++++++++----------------------
>>>  1 file changed, 27 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
>>> index d497737298..3b711395b8 100644
>>> --- a/drivers/clk/clk-uclass.c
>>> +++ b/drivers/clk/clk-uclass.c
>>> @@ -410,6 +410,7 @@ int clk_request(struct udevice *dev, struct clk *clk)
>>>         ops = clk_dev_ops(dev);
>>>
>>>         clk->dev = dev;
>>> +       clk->enable_count = 0;
>>
>> Lukasz has taged Acked-by in v3.
>> Re: [PATCH v3 03/12] clk: Unconditionally recursively en-/dis-able clocks
>>
>> This patch shall be fixed and freeze.
>> But you add a new modification (Clear enable_count on request)in v5 and v6.
>> Although it seem to look fine and nothing big deal.
>> I just highlight it here, and if Lukasz have no other comment further.
>> I will pull it via riscv tree later.
>>
> 
> I have told you in v5, that this patch have conflict with
> u-boot/master please rebase it.
> But it still conflict in v6.
> 
> https://patchwork.ozlabs.org/patch/1246836/
> 
> 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:522
> error: drivers/clk/clk-uclass.c: patch does not apply
> Patch failed at 0003 clk: Unconditionally recursively en-/dis-able clocks
> The copy of the patch that failed is found in: .git/rebase-apply/patch
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
> 
> 

This was just rebased against u-boot/master. Should I be rebasing
against a different branch? The commit I am working off is

$ git show upstream/master
commit 1efb9796f80e1394f080be1b4f3173ff108ad1f2 (upstream/master, upstream/WIP/03Mar2020, upstream/HEAD)
Merge: 8aad16916d 9aa886cc0b
Author: Tom Rini <trini at konsulko.com>
Date:   Tue Mar 3 21:48:49 2020 -0500

    Merge tag 'dm-pull-3mar20' of git://git.denx.de/u-boot-dm
    
    Fixes for power domain on device removal

>> Thanks,
>> Rick
>>
>>
>>
>>>
>>>         if (!ops->request)
>>>                 return 0;
>>> @@ -522,7 +523,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);
>>> @@ -531,32 +531,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;
>>> @@ -582,7 +579,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);
>>> @@ -591,29 +587,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
>>>
Rick Chen March 11, 2020, 2:09 a.m. UTC | #4 
Hi Sean


> On 3/10/20 2:51 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 v5:
> >>> - Clear enable_count on request
> >>>
> >>> Changes in v4:
> >>> - Lint
> >>>
> >>> Changes in v3:
> >>> - New
> >>>
> >>>  drivers/clk/clk-uclass.c | 60 ++++++++++++++++++----------------------
> >>>  1 file changed, 27 insertions(+), 33 deletions(-)
> >>>
> >>> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> >>> index d497737298..3b711395b8 100644
> >>> --- a/drivers/clk/clk-uclass.c
> >>> +++ b/drivers/clk/clk-uclass.c
> >>> @@ -410,6 +410,7 @@ int clk_request(struct udevice *dev, struct clk *clk)
> >>>         ops = clk_dev_ops(dev);
> >>>
> >>>         clk->dev = dev;
> >>> +       clk->enable_count = 0;
> >>
> >> Lukasz has taged Acked-by in v3.
> >> Re: [PATCH v3 03/12] clk: Unconditionally recursively en-/dis-able clocks
> >>
> >> This patch shall be fixed and freeze.
> >> But you add a new modification (Clear enable_count on request)in v5 and v6.
> >> Although it seem to look fine and nothing big deal.
> >> I just highlight it here, and if Lukasz have no other comment further.
> >> I will pull it via riscv tree later.
> >>
> >
> > I have told you in v5, that this patch have conflict with
> > u-boot/master please rebase it.
> > But it still conflict in v6.
> >
> > https://patchwork.ozlabs.org/patch/1246836/
> >
> > 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:522
> > error: drivers/clk/clk-uclass.c: patch does not apply
> > Patch failed at 0003 clk: Unconditionally recursively en-/dis-able clocks
> > The copy of the patch that failed is found in: .git/rebase-apply/patch
> > When you have resolved this problem, run "git am --continue".
> > If you prefer to skip this patch, run "git am --skip" instead.
> > To restore the original branch and stop patching, run "git am --abort".
> >
> >
>
> This was just rebased against u-boot/master. Should I be rebasing
> against a different branch? The commit I am working off is

It is just fine to sync with u-boot/master.
Before I send a PR, I shall sync to u-boot/master too.

Thanks,
Rick

>
> $ git show upstream/master
> commit 1efb9796f80e1394f080be1b4f3173ff108ad1f2 (upstream/master, upstream/WIP/03Mar2020, upstream/HEAD)
> Merge: 8aad16916d 9aa886cc0b
> Author: Tom Rini <trini at konsulko.com>
> Date:   Tue Mar 3 21:48:49 2020 -0500
>
>     Merge tag 'dm-pull-3mar20' of git://git.denx.de/u-boot-dm
>
>     Fixes for power domain on device removal
>
> >> Thanks,
> >> Rick
> >>
> >>
> >>
> >>>
> >>>         if (!ops->request)
> >>>                 return 0;
> >>> @@ -522,7 +523,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);
> >>> @@ -531,32 +531,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;
> >>> @@ -582,7 +579,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);
> >>> @@ -591,29 +587,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 March 12, 2020, 6:27 p.m. UTC | #5 
On 3/10/20 10:09 PM, Rick Chen wrote:
> Hi Sean
>> On 3/10/20 2:51 AM, Rick Chen wrote:
>>> Hi Sean
>>> I have told you in v5, that this patch have conflict with
>>> u-boot/master please rebase it.
>>> But it still conflict in v6.
>>>
>>> https://patchwork.ozlabs.org/patch/1246836/
>>>
>>> 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:522
>>> error: drivers/clk/clk-uclass.c: patch does not apply
>>> Patch failed at 0003 clk: Unconditionally recursively en-/dis-able clocks
>>> The copy of the patch that failed is found in: .git/rebase-apply/patch
>>> When you have resolved this problem, run "git am --continue".
>>> If you prefer to skip this patch, run "git am --skip" instead.
>>> To restore the original branch and stop patching, run "git am --abort".
>>>
>>>
>>
>> This was just rebased against u-boot/master. Should I be rebasing
>> against a different branch? The commit I am working off is
> 
> It is just fine to sync with u-boot/master.
> Before I send a PR, I shall sync to u-boot/master too.
> 
> Thanks,
> Rick

I think I found the issue. I made a commit to add some more log messages
to that file, and it seems to have changed the line numbers. Next patch
I will rebase without that commit.

--Sean
diff mbox series

Patch

diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
index d497737298..3b711395b8 100644
--- a/drivers/clk/clk-uclass.c
+++ b/drivers/clk/clk-uclass.c
@@ -410,6 +410,7 @@  int clk_request(struct udevice *dev, struct clk *clk)
 	ops = clk_dev_ops(dev);
 
 	clk->dev = dev;
+	clk->enable_count = 0;
 
 	if (!ops->request)
 		return 0;
@@ -522,7 +523,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);
@@ -531,32 +531,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;
@@ -582,7 +579,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);
@@ -591,29 +587,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;
 			}
 		}