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 <sean...@gmail.com>
> >>> Acked-by: Lukasz Majewski <lu...@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 <tr...@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
> >>>
>

Reply via email to