Re: [PATCH v2 1/2] clk: change clk_ops' -round_rate() prototype
Hi Boris, On 05/06/15 12:39, Boris Brezillon wrote: Hi Jon, On Fri, 5 Jun 2015 09:46:09 +0100 Jon Hunter jonath...@nvidia.com wrote: 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 ... Yes, but is it really worth the additional complexity. You'll have to add the unit information anyway, so using an unsigned long for the value and another field for the unit (an enum ?) is just like using a 64 bit integer. For a storage perspective, yes it would be the same. However, there are probably a lot of devices that would not need the extra range, but would now have to deal with 64-bit types. I have no idea how much overhead that would be in reality. If the overhead is negligible then a 64-bit type is definitely the way to go, as I agree it is simpler and cleaner. Cheers Jon -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] clk: change clk_ops' -round_rate() prototype
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 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] clk: change clk_ops' -round_rate() prototype
Hi Jon, On Fri, 5 Jun 2015 09:46:09 +0100 Jon Hunter jonath...@nvidia.com wrote: 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 ... Yes, but is it really worth the additional complexity. You'll have to add the unit information anyway, so using an unsigned long for the value and another field for the unit (an enum ?) is just like using a 64 bit integer. Best Regards, Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] clk: change clk_ops' -round_rate() prototype
Hi Paul, On Thu, 4 Jun 2015 23:02:25 + (UTC) Paul Walmsley p...@pwsan.com 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. Yep, that was something I was considering too. If Stephen agrees I'll change that in the next version. BTW, you're referring to the second version of this patch, but things have changed a bit: Stephen recommended to only modify the -determine_rate() prototype and pass a structure instead of a list of arguments. Here is the last version of this series [1]. Best Regards, Boris [1]http://patchwork.linux-mips.org/patch/10092/ -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] clk: change clk_ops' -round_rate() prototype
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. - Paul -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
