Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932587AbbBBRHy (ORCPT ); Mon, 2 Feb 2015 12:07:54 -0500 Received: from smtp3.ore.mailhop.org ([54.149.88.251]:59353 "EHLO smtp3.ore.mailhop.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754539AbbBBRHu (ORCPT ); Mon, 2 Feb 2015 12:07:50 -0500 X-Mail-Handler: DuoCircle Outbound SMTP X-Originating-IP: 104.193.169.186 X-Report-Abuse-To: abuse@duocircle.com (see https://support.duocircle.com/support/solutions/articles/5000540958-duocircle-standard-smtp-abuse-information for abuse reporting information) X-MHO-User: U2FsdGVkX19yAnIw9zUX2ehzdgo+60vy Date: Mon, 2 Feb 2015 09:04:59 -0800 From: Tony Lindgren To: Mike Turquette Cc: Tomeu Vizoso , linux-kernel@vger.kernel.org, Stephen Boyd , Paul Walmsley , linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org, t-kristo@ti.com Subject: Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances Message-ID: <20150202170458.GB9418@atomide.com> References: <1422011024-32283-1-git-send-email-tomeu.vizoso@collabora.com> <1422011024-32283-4-git-send-email-tomeu.vizoso@collabora.com> <20150201212432.22722.70917@quantum> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150201212432.22722.70917@quantum> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3768 Lines: 85 * Mike Turquette [150201 13:27]: > Quoting Tomeu Vizoso (2015-01-23 03:03:30) > > Moves clock state to struct clk_core, but takes care to change as little API as > > possible. > > > > struct clk_hw still has a pointer to a struct clk, which is the > > implementation's per-user clk instance, for backwards compatibility. > > > > The struct clk that clk_get_parent() returns isn't owned by the caller, but by > > the clock implementation, so the former shouldn't call clk_put() on it. > > > > Because some boards in mach-omap2 still register clocks statically, their clock > > registration had to be updated to take into account that the clock information > > is stored in struct clk_core now. > > Tero, Paul & Tony, > > Tomeu's patch unveils a problem with omap3_noncore_dpll_enable and > struct dpll_data, namely this snippet from > arch/arm/mach-omap2/dpll3xxx.c: > > parent = __clk_get_parent(hw->clk); > > if (__clk_get_rate(hw->clk) == __clk_get_rate(dd->clk_bypass)) { > WARN(parent != dd->clk_bypass, > "here0, parent name is %s, bypass name is %s\n", > __clk_get_name(parent), __clk_get_name(dd->clk_bypass)); > r = _omap3_noncore_dpll_bypass(clk); > } else { > WARN(parent != dd->clk_ref, > "here1, parent name is %s, ref name is %s\n", > __clk_get_name(parent), __clk_get_name(dd->clk_ref)); > r = _omap3_noncore_dpll_lock(clk); > } > > struct dpll_data has members clk_ref and clk_bypass which are struct clk > pointers. This was always a bit of a violation of the clk.h contract > since drivers are not supposed to deref struct clk pointers. Now that we > generate unique pointers for each call to clk_get (clk_ref & clk_bypass > are populated by of_clk_get in ti_clk_register_dpll) then the pointer > comparisons above will never be equal (even if they resolve down to the > same struct clk_core). I added the verbose traces to the WARNs above to > illustrate the point: the names are always the same but the pointers > differ. > > AFAICT this doesn't break anything, but booting on OMAP3+ results in > noisy WARNs. Also on at least omap4 like I posted. So sounds like the check for WARN is wrong but harmless. Paul & Tero, what do you want to do about that? > I think the correct fix is to replace clk_bypass and clk_ref pointers > with a simple integer parent_index. In fact we already have this index. > See how the pointers are populated in ti_clk_register_dpll: > > > dd->clk_ref = of_clk_get(node, 0); > dd->clk_bypass = of_clk_get(node, 1); > > Tony, the same problem affects the FAPLL code which copy/pastes some of > the DPLL code. > > Thoughts? Not seeing these warnings with dm186x as fapll.c does not use dpll3xxx.c. This is because of the way the PLL's child synthesizers need to also access the PLL registers for power and bypass mode. Not related to the $subject bug, but to me it seems that we could possibly have Linux generic PLL code if we add support for parent_in_bypass_mode in addition to the parent_rate. This is because the PLL can in theory generate the same rate both in bypass mode and regular mode so parent_rate is not enough to tell it to the child synthesizers. Not sure how the PLL registers enabling and disabling it's children should be handled, maybe regmap would work there. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/