From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jon Hunter Subject: Re: [PATCH v2 1/2] clk: change clk_ops' ->round_rate() prototype Date: Fri, 5 Jun 2015 09:46:09 +0100 Message-ID: <557161D1.3040107@nvidia.com> References: <1430407809-31147-1-git-send-email-boris.brezillon@free-electrons.com> <1430407809-31147-2-git-send-email-boris.brezillon@free-electrons.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: linux-media-owner@vger.kernel.org To: Paul Walmsley , Boris Brezillon Cc: Mike Turquette , Stephen Boyd , linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org, Jonathan Corbet , Shawn Guo , ascha Hauer , David Brown , Daniel Walker , Bryan Huntsman , Tony Lindgren , Liviu Dudau , Sudeep Holla , Lorenzo Pieralisi , Ralf Baechle , Max Filippov , Heiko Stuebner , Sylwester Nawrocki , Tomasz Figa , Barry Song , Viresh Kumar , =?UTF-8?B?RW1pbGlvIEzDs3Bleg==?= , Maxime Ripard List-Id: linux-arm-msm@vger.kernel.org On 05/06/15 00:02, Paul Walmsley wrote: > Hi folks > > just a brief comment on this one: > > On Thu, 30 Apr 2015, Boris Brezillon wrote: > >> Clock rates are stored in an unsigned long field, but ->round_rate() >> (which returns a rounded rate from a requested one) returns a long >> value (errors are reported using negative error codes), which can lead >> to long overflow if the clock rate exceed 2Ghz. >> >> Change ->round_rate() prototype to return 0 or an error code, and pass the >> requested rate as a pointer so that it can be adjusted depending on >> hardware capabilities. > > ... > >> diff --git a/Documentation/clk.txt b/Documentation/clk.txt >> index 0e4f90a..fca8b7a 100644 >> --- a/Documentation/clk.txt >> +++ b/Documentation/clk.txt >> @@ -68,8 +68,8 @@ the operations defined in clk.h: >> int (*is_enabled)(struct clk_hw *hw); >> unsigned long (*recalc_rate)(struct clk_hw *hw, >> unsigned long parent_rate); >> - long (*round_rate)(struct clk_hw *hw, >> - unsigned long rate, >> + int (*round_rate)(struct clk_hw *hw, >> + unsigned long *rate, >> unsigned long *parent_rate); >> long (*determine_rate)(struct clk_hw *hw, >> unsigned long rate, > > I'd suggest that we should probably go straight to 64-bit rates. There > are already plenty of clock sources that can generate rates higher than > 4GiHz. An alternative would be to introduce to a frequency "base" the default could be Hz (for backwards compatibility), but for CPUs we probably only care about MHz (or may be kHz) and so 32-bits would still suffice. Even if CPUs cared about Hz they could still use Hz, but in that case they probably don't care about GHz. Obviously, we don't want to break DT compatibility but may be the frequency base could be defined in DT and if it is missing then Hz is assumed. Just a thought ... Jon From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Message-ID: <557161D1.3040107@nvidia.com> Date: Fri, 5 Jun 2015 09:46:09 +0100 From: Jon Hunter MIME-Version: 1.0 To: Paul Walmsley , Boris Brezillon CC: Mike Turquette , Stephen Boyd , , , Jonathan Corbet , Shawn Guo , ascha Hauer , David Brown , Daniel Walker , Bryan Huntsman , Tony Lindgren , Liviu Dudau , Sudeep Holla , Lorenzo Pieralisi , Ralf Baechle , Max Filippov , Heiko Stuebner , Sylwester Nawrocki , Tomasz Figa , Barry Song , Viresh Kumar , =?UTF-8?B?RW1pbGlvIEzDs3Bleg==?= , Maxime Ripard , Peter De Schrijver , Prashant Gaikwad , Stephen Warren , Thierry Reding , Alexandre Courbot , Tero Kristo , Ulf Hansson , Michal Simek , Philipp Zabel , , , , , , , , , , , , , Subject: Re: [PATCH v2 1/2] clk: change clk_ops' ->round_rate() prototype References: <1430407809-31147-1-git-send-email-boris.brezillon@free-electrons.com> <1430407809-31147-2-git-send-email-boris.brezillon@free-electrons.com> In-Reply-To: Return-Path: jonathanh@nvidia.com Content-Type: text/plain; charset="utf-8" List-ID: On 05/06/15 00:02, Paul Walmsley wrote: > Hi folks > > just a brief comment on this one: > > On Thu, 30 Apr 2015, Boris Brezillon wrote: > >> Clock rates are stored in an unsigned long field, but ->round_rate() >> (which returns a rounded rate from a requested one) returns a long >> value (errors are reported using negative error codes), which can lead >> to long overflow if the clock rate exceed 2Ghz. >> >> Change ->round_rate() prototype to return 0 or an error code, and pass the >> requested rate as a pointer so that it can be adjusted depending on >> hardware capabilities. > > ... > >> diff --git a/Documentation/clk.txt b/Documentation/clk.txt >> index 0e4f90a..fca8b7a 100644 >> --- a/Documentation/clk.txt >> +++ b/Documentation/clk.txt >> @@ -68,8 +68,8 @@ the operations defined in clk.h: >> int (*is_enabled)(struct clk_hw *hw); >> unsigned long (*recalc_rate)(struct clk_hw *hw, >> unsigned long parent_rate); >> - long (*round_rate)(struct clk_hw *hw, >> - unsigned long rate, >> + int (*round_rate)(struct clk_hw *hw, >> + unsigned long *rate, >> unsigned long *parent_rate); >> long (*determine_rate)(struct clk_hw *hw, >> unsigned long rate, > > I'd suggest that we should probably go straight to 64-bit rates. There > are already plenty of clock sources that can generate rates higher than > 4GiHz. An alternative would be to introduce to a frequency "base" the default could be Hz (for backwards compatibility), but for CPUs we probably only care about MHz (or may be kHz) and so 32-bits would still suffice. Even if CPUs cared about Hz they could still use Hz, but in that case they probably don't care about GHz. Obviously, we don't want to break DT compatibility but may be the frequency base could be defined in DT and if it is missing then Hz is assumed. Just a thought ... Jon From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from hqemgate14.nvidia.com (hqemgate14.nvidia.com. [216.228.121.143]) by gmr-mx.google.com with ESMTPS id fl5si703669pad.0.2015.06.05.01.46.20 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Fri, 05 Jun 2015 01:46:20 -0700 (PDT) Message-ID: <557161D1.3040107@nvidia.com> Date: Fri, 5 Jun 2015 09:46:09 +0100 From: Jon Hunter MIME-Version: 1.0 To: Paul Walmsley , Boris Brezillon CC: Mike Turquette , Stephen Boyd , , , Jonathan Corbet , Shawn Guo , ascha Hauer , David Brown , Daniel Walker , Bryan Huntsman , Tony Lindgren , Liviu Dudau , Sudeep Holla , Lorenzo Pieralisi , Ralf Baechle , Max Filippov , Heiko Stuebner , Sylwester Nawrocki , Tomasz Figa , Barry Song , Viresh Kumar , =?UTF-8?B?RW1pbGlvIEzDs3Bleg==?= , Maxime Ripard , Peter De Schrijver , Prashant Gaikwad , Stephen Warren , Thierry Reding , Alexandre Courbot , Tero Kristo , Ulf Hansson , Michal Simek , Philipp Zabel , , , , , , , , , , , , , Subject: [rtc-linux] Re: [PATCH v2 1/2] clk: change clk_ops' ->round_rate() prototype References: <1430407809-31147-1-git-send-email-boris.brezillon@free-electrons.com> <1430407809-31147-2-git-send-email-boris.brezillon@free-electrons.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Reply-To: rtc-linux@googlegroups.com List-ID: List-Post: , List-Help: , List-Archive: , List-Unsubscribe: , On 05/06/15 00:02, Paul Walmsley wrote: > Hi folks > > just a brief comment on this one: > > On Thu, 30 Apr 2015, Boris Brezillon wrote: > >> Clock rates are stored in an unsigned long field, but ->round_rate() >> (which returns a rounded rate from a requested one) returns a long >> value (errors are reported using negative error codes), which can lead >> to long overflow if the clock rate exceed 2Ghz. >> >> Change ->round_rate() prototype to return 0 or an error code, and pass the >> requested rate as a pointer so that it can be adjusted depending on >> hardware capabilities. > > ... > >> diff --git a/Documentation/clk.txt b/Documentation/clk.txt >> index 0e4f90a..fca8b7a 100644 >> --- a/Documentation/clk.txt >> +++ b/Documentation/clk.txt >> @@ -68,8 +68,8 @@ the operations defined in clk.h: >> int (*is_enabled)(struct clk_hw *hw); >> unsigned long (*recalc_rate)(struct clk_hw *hw, >> unsigned long parent_rate); >> - long (*round_rate)(struct clk_hw *hw, >> - unsigned long rate, >> + int (*round_rate)(struct clk_hw *hw, >> + unsigned long *rate, >> unsigned long *parent_rate); >> long (*determine_rate)(struct clk_hw *hw, >> unsigned long rate, > > I'd suggest that we should probably go straight to 64-bit rates. There > are already plenty of clock sources that can generate rates higher than > 4GiHz. An alternative would be to introduce to a frequency "base" the default could be Hz (for backwards compatibility), but for CPUs we probably only care about MHz (or may be kHz) and so 32-bits would still suffice. Even if CPUs cared about Hz they could still use Hz, but in that case they probably don't care about GHz. Obviously, we don't want to break DT compatibility but may be the frequency base could be defined in DT and if it is missing then Hz is assumed. Just a thought ... Jon -- -- You received this message because you are subscribed to "rtc-linux". Membership options at http://groups.google.com/group/rtc-linux . Please read http://groups.google.com/group/rtc-linux/web/checklist before submitting a driver. --- You received this message because you are subscribed to the Google Groups "rtc-linux" group. To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com. For more options, visit https://groups.google.com/d/optout. From mboxrd@z Thu Jan 1 00:00:00 1970 From: jonathanh@nvidia.com (Jon Hunter) Date: Fri, 5 Jun 2015 09:46:09 +0100 Subject: [PATCH v2 1/2] clk: change clk_ops' ->round_rate() prototype In-Reply-To: References: <1430407809-31147-1-git-send-email-boris.brezillon@free-electrons.com> <1430407809-31147-2-git-send-email-boris.brezillon@free-electrons.com> Message-ID: <557161D1.3040107@nvidia.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 05/06/15 00:02, Paul Walmsley wrote: > Hi folks > > just a brief comment on this one: > > On Thu, 30 Apr 2015, Boris Brezillon wrote: > >> Clock rates are stored in an unsigned long field, but ->round_rate() >> (which returns a rounded rate from a requested one) returns a long >> value (errors are reported using negative error codes), which can lead >> to long overflow if the clock rate exceed 2Ghz. >> >> Change ->round_rate() prototype to return 0 or an error code, and pass the >> requested rate as a pointer so that it can be adjusted depending on >> hardware capabilities. > > ... > >> diff --git a/Documentation/clk.txt b/Documentation/clk.txt >> index 0e4f90a..fca8b7a 100644 >> --- a/Documentation/clk.txt >> +++ b/Documentation/clk.txt >> @@ -68,8 +68,8 @@ the operations defined in clk.h: >> int (*is_enabled)(struct clk_hw *hw); >> unsigned long (*recalc_rate)(struct clk_hw *hw, >> unsigned long parent_rate); >> - long (*round_rate)(struct clk_hw *hw, >> - unsigned long rate, >> + int (*round_rate)(struct clk_hw *hw, >> + unsigned long *rate, >> unsigned long *parent_rate); >> long (*determine_rate)(struct clk_hw *hw, >> unsigned long rate, > > I'd suggest that we should probably go straight to 64-bit rates. There > are already plenty of clock sources that can generate rates higher than > 4GiHz. An alternative would be to introduce to a frequency "base" the default could be Hz (for backwards compatibility), but for CPUs we probably only care about MHz (or may be kHz) and so 32-bits would still suffice. Even if CPUs cared about Hz they could still use Hz, but in that case they probably don't care about GHz. Obviously, we don't want to break DT compatibility but may be the frequency base could be defined in DT and if it is missing then Hz is assumed. Just a thought ... Jon