On Mon, Aug 17, 2015 at 08:45:18PM +0800, Dong Aisheng wrote: > 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 <mturque...@linaro.org> > > > Cc: Stephen Boyd <sb...@codeaurora.org> > > > Signed-off-by: Dong Aisheng <aisheng.d...@freescale.com> > > > --- > > > > 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 >
Hi Mike & Stephen, Any comments about this? 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
