Re: [PATCH V3 4/5] clk: core: add CLK_OPS_PARENT_ON flags to support clocks require parent on

[Dong Aisheng]

On Thu, Aug 13, 2015 at 06:01:09PM -0700, Stephen Boyd wrote:
> On 07/28, Dong Aisheng wrote:
> > On Freescale i.MX7D platform, all clocks operations, including
> > enable/disable, rate change and re-parent, requires its parent
> > clock on. Current clock core can not support it well.
> > This patch introduce a new flag CLK_OPS_PARENT_ON to handle this
> > special case in clock core that enable its parent clock firstly for
> > each operation and disable it later after operation complete.
> > 
> > This patch fixes disaling clocks while its parent is off.
> > This is a special case that is caused by a state mis-align between
> > HW and SW in clock tree during booting.
> > Usually in uboot, we may enable all clocks in HW by default.
> > And during kernel booting time, the parent clock could be disabled in its
> > driver probe due to calling clk_prepare_enable and clk_disable_unprepare.
> > Because it's child clock is only enabled in HW while its SW usecount
> > in clock tree is still 0, so clk_disable of parent clock will gate
> > the parent clock in both HW and SW usecount ultimately.
> > Then there will be a clock is on in HW while its parent is disabled.
> > 
> > Later when clock core does clk_disable_unused, this clock disable
> > will cause system hang due to the limitation of operation requiring
> > its parent clock on.
> > 
> > Cc: Mike Turquette <mturquette@xxxxxxxxxx>
> > Cc: Stephen Boyd <sboyd@xxxxxxxxxxxxxx>
> > Signed-off-by: Dong Aisheng <aisheng.dong@xxxxxxxxxxxxx>
> > ---
> 
> Sorry, I still don't agree with this patch. There's no reason we
> should be turning on clocks during late init so that we can turn
> off clocks.

Can't the reason be that it's fairly possible one clock is enabled
in HW while it's parent is disabled in initial clock tree state
and to enable parent clocks to disable itself is required by its special
HW characteristic?

Dosen't it quite clear from HW point of view?

> If there's some sort of problem in doing that we
> should figure it out and make it so that during late init we turn
> off clocks from the leaves of the tree to the root.
> 

Turning off clocks from the leaves to root probably may require clock core
to provide a way to keep the parent clock enabled once finding its child
is still on in HW (clk_core_is_enabled() returns true) but enable_count
is zero before late init.

One possible solution may be leaving parent clocks on in HW during disable
once finding its child is on in HW and only decrease the parent's refcount,
and then replying on the later clk_disable_unused() to disable both child
and parent from leave to root.
e.g. clock A: parent, clock B: child of A
Initial state: clock B is enabled in HW while refcount is zero
Step1: Driver A enable clock A during probe
 A: refcount becomes 1 HW state: enabled
Step2: Driver A disable clock A after probe
 A: refcount becomes 0 HW state: enabled (only decrease refcount)

Then Clock A will be the same state as B, HW enabled while refcount is zero(
means no users), the later clk_disable_unusersd() will disable them all
from leave to root.

This is a workable solution but seems much more complicated than the exist
one in this patch which is only 5 lines of code changes.

And the question is:
since we already have the support of CLK_OPS_PARENT_ON (required by
clock set_rate/re-parent), why we still need invent another complicated
mechanism to support avoiding enable parent clock only for clk_disable_unused()?
Is that really worthy?
And it's also less power efficiency than the one in the patch.

> I agree that there's a problem here where we don't properly
> handle keeping children clocks on if a driver comes in and turns
> off a clock in the middle of the tree before late init. That's a
> real bug, and we should fix it.

Sorry, i still can't understand it's a bug.
Can you help explain more?

It looks to me like reasonable.
Enable/disable clock in driver is just one case, the initial clock tree may
also have such cases.
(Here i took the 'children clocks' you said as the one who's child clock is on
in HW while refcount is zero, fix me if wrong)

And it seems not so quite make sense to not physically disable the clock
when there's already no child users(refcount becomes zero) and i don't
think the child clock's default enablement state in HW means a valid user
since it's just caused by misalignment between HW and SW clock tree during
kernel booting phase which is meaningless.
And that seems is why the clk_disable_unused() function exist for fixing
this state misalignment issue.

> Mike Turquette has been doing
> some work to have a way to indicate that certain clocks should be
> kept on until client drivers grab them. 

Sorry i can't see how this help on my issue.

> I think it will also make
> sure to up refcounts on parent clocks in the middle of the tree
> when it figures out that a child clock is enabled. Would that be
> all that we need to do to fix this problem?
> 

Then when will we down the refcounts on parent clocks and when to disable it?
The current clk_disable_unused() only handle HW clk enable/disable, no
refcount operations.
Not sure how this is going to fix my issues.

And again, as i said above, i don't think it makes much sense to not disable
parent only if child is enabled in HW, unless there's more strong reasons.

> Also, the subject of this patch and patch 5 are the same. Why?
> 

Sorry, mainly because the full feature of CLK_OPS_PARENT_ON is divided into
two patches for better review, their commit message is different.
patch 4 is adding support for clk_disable_unused() while patch 5 is for
clk_setrate/clk_reparent.
I could reform the subject if needed.

Regards
Dong Aisheng

> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/