From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754228AbbBAVYj (ORCPT ); Sun, 1 Feb 2015 16:24:39 -0500 Received: from mail-ig0-f172.google.com ([209.85.213.172]:52279 "EHLO mail-ig0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751703AbbBAVYh convert rfc822-to-8bit (ORCPT ); Sun, 1 Feb 2015 16:24:37 -0500 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8BIT To: Tomeu Vizoso , linux-kernel@vger.kernel.org, "Stephen Boyd" From: Mike Turquette In-Reply-To: <1422011024-32283-4-git-send-email-tomeu.vizoso@collabora.com> Cc: "Tomeu Vizoso" , "Paul Walmsley" , "Tony Lindgren" , linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org, t-kristo@ti.com References: <1422011024-32283-1-git-send-email-tomeu.vizoso@collabora.com> <1422011024-32283-4-git-send-email-tomeu.vizoso@collabora.com> Message-ID: <20150201212432.22722.70917@quantum> User-Agent: alot/0.3.5 Subject: Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances Date: Sun, 01 Feb 2015 13:24:32 -0800 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. 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? Regards, Mike From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mike Turquette Subject: Re: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances Date: Sun, 01 Feb 2015 13:24:32 -0800 Message-ID: <20150201212432.22722.70917@quantum> References: <1422011024-32283-1-git-send-email-tomeu.vizoso@collabora.com> <1422011024-32283-4-git-send-email-tomeu.vizoso@collabora.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8BIT Return-path: In-Reply-To: <1422011024-32283-4-git-send-email-tomeu.vizoso@collabora.com> Sender: linux-kernel-owner@vger.kernel.org To: linux-kernel@vger.kernel.org, Stephen Boyd Cc: Tomeu Vizoso , Paul Walmsley , Tony Lindgren , linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org, t-kristo@ti.com List-Id: linux-omap@vger.kernel.org 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. 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? Regards, Mike From mboxrd@z Thu Jan 1 00:00:00 1970 From: mturquette@linaro.org (Mike Turquette) Date: Sun, 01 Feb 2015 13:24:32 -0800 Subject: [PATCH v13 3/6] clk: Make clk API return per-user struct clk instances In-Reply-To: <1422011024-32283-4-git-send-email-tomeu.vizoso@collabora.com> References: <1422011024-32283-1-git-send-email-tomeu.vizoso@collabora.com> <1422011024-32283-4-git-send-email-tomeu.vizoso@collabora.com> Message-ID: <20150201212432.22722.70917@quantum> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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. 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? Regards, Mike