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