Re: [PATCH v2 1/2] clk: change clk_ops' -round_rate() prototype

2015-06-08 Thread Jon Hunter
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-media 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

2015-06-05 Thread Jon Hunter

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-media 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

2015-06-05 Thread Boris Brezillon
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-media 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

2015-06-05 Thread Boris Brezillon
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-media 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

2015-06-04 Thread Paul Walmsley
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-media 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

2015-05-19 Thread Stephen Boyd
On 05/16, Mikko Perttunen wrote:
 On 05/15/2015 06:40 PM, Boris Brezillon wrote:
 Hi Stephen,
 
 Adding Mikko in the loop (after all, he was the one complaining about
 this signed long limitation in the first place, and I forgot to add
 him in the Cc list :-/).
 
 I think I got it through linux-tegra anyway, but thanks :)
 
 
 Mikko, are you okay with the approach proposed by Stephen (adding a
 new method) ?
 
 Yes, sounds good to me. If a driver uses the existing methods with
 too large frequencies, the issue is pretty discoverable anyway. I
 think adjust_rate sounds a bit too much like it sets the clock's
 rate, though; perhaps adjust_rate_request or something like that?
 

Well I'm also OK with changing the determine_rate API one more
time, but we'll have to be careful. Invariably someone will push
a new clock driver through some non-clk tree path and we'll get
build breakage. They shouldn't be doing that, so either we do the
change now and push it to -next and see what breaks, or we do
this after -rc1 comes out and make sure everyone has lots of
warning.

-- 
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-media 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

2015-05-16 Thread Mikko Perttunen

On 05/15/2015 06:40 PM, Boris Brezillon wrote:

Hi Stephen,

Adding Mikko in the loop (after all, he was the one complaining about
this signed long limitation in the first place, and I forgot to add
him in the Cc list :-/).


I think I got it through linux-tegra anyway, but thanks :)



Mikko, are you okay with the approach proposed by Stephen (adding a
new method) ?


Yes, sounds good to me. If a driver uses the existing methods with too 
large frequencies, the issue is pretty discoverable anyway. I think 
adjust_rate sounds a bit too much like it sets the clock's rate, 
though; perhaps adjust_rate_request or something like that?


Thanks,
Mikko



On Thu, 7 May 2015 09:37:02 +0200
Boris Brezillon boris.brezil...@free-electrons.com wrote:


Hi Stephen,

On Wed, 6 May 2015 23:39:53 -0700
Stephen Boyd sb...@codeaurora.org wrote:


On 04/30, 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.

Signed-off-by: Boris Brezillon boris.brezil...@free-electrons.com
Tested-by: Heiko Stuebner he...@sntech.de
Tested-by: Mikko Perttunen mikko.perttu...@kapsi.fi
Reviewed-by: Heiko Stuebner he...@sntech.de


This patch is fairly invasive, and it probably doesn't even
matter for most of these clock providers to be able to round a
rate above 2GHz.


Fair enough.


I've been trying to remove the .round_rate op
from the framework by encouraging new features via the
.determine_rate op.


Oh, I wasn't aware of that (BTW, that's a good thing).
Maybe this should be clearly stated (both in the struct clk_ops
kerneldoc header and in Documentation/clk.txt).


Sadly, we still have to do a flag day and
change all the .determine_rate ops when we want to add things.


Yes, but the number of clk drivers implementing -determine_rate() is
still quite limited compared to those implementing -round_rate().



What if we changed determine_rate ops to take a struct
clk_determine_info (or some better named structure) instead of
the current list of arguments that it currently takes? Then when
we want to make these sorts of framework wide changes we can just
throw a new member into that structure and be done.


I really like this idea, especially since I was wondering if we could
pass other 'clk rate requirements' like the rounding policy (down,
closest, up), or the maximum clk inaccuracy.



It doesn't solve the unsigned long to int return value problem
though. We can solve that by gradually introducing a new op and
handling another case in the rounding path. If we can come up
with some good name for that new op like .decide_rate or
something then it makes things nicer in the long run. I like the
name .determine_rate though :/


Okay, if you want a new method, how about this one:

struct clk_adjust_rate_req {
/* fields filled by the caller */
unsigned long rate; /* rate is updated by the clk driver */
unsigned long min;
unsigned long max;

/* fields filled by the clk driver */
struct clk_hw *best_parent;
unsigned long best_parent_rate;

/*
 * new fields I'd like to add at some point:
 * unsigned long max_inaccuracy;
 * something about the power consumption constraints :-)
 */
};

int (*adjust_rate)(struct clk_hw *hw, struct clk_adjust_rate_req *req);



Why not changing the -determine_rate() prototype. As said above, the
number of clk drivers implementing this function is still quite
limited, and I guess we can have an ack for all of them.



The benefit of all this is that we don't have to worry about
finding the random clk providers that get added into other
subsystems and fixing them up. If drivers actually care about
this problem then they'll be fixed to use the proper op. FYI,
last time we updated the function signature of .determine_rate we
broke a couple drivers along the way.



Hm, IMHO, adding a new op is not a good thing. I agree that it eases
the transition, but ITOH you'll have to live with old/deprecated ops in
your clk_ops structure with people introducing new drivers still using
the old ops (see the number of clk drivers implementing -round_rate()
instead of -determine_rate()).

Best Regards,

Boris







--
To unsubscribe from this list: send the line unsubscribe linux-media 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

2015-05-15 Thread Boris Brezillon
Hi Stephen,

Adding Mikko in the loop (after all, he was the one complaining about
this signed long limitation in the first place, and I forgot to add
him in the Cc list :-/).

Mikko, are you okay with the approach proposed by Stephen (adding a
new method) ?

On Thu, 7 May 2015 09:37:02 +0200
Boris Brezillon boris.brezil...@free-electrons.com wrote:

 Hi Stephen,
 
 On Wed, 6 May 2015 23:39:53 -0700
 Stephen Boyd sb...@codeaurora.org wrote:
 
  On 04/30, 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.
   
   Signed-off-by: Boris Brezillon boris.brezil...@free-electrons.com
   Tested-by: Heiko Stuebner he...@sntech.de
   Tested-by: Mikko Perttunen mikko.perttu...@kapsi.fi
   Reviewed-by: Heiko Stuebner he...@sntech.de
  
  This patch is fairly invasive, and it probably doesn't even
  matter for most of these clock providers to be able to round a
  rate above 2GHz.
 
 Fair enough.
 
  I've been trying to remove the .round_rate op
  from the framework by encouraging new features via the
  .determine_rate op.
 
 Oh, I wasn't aware of that (BTW, that's a good thing).
 Maybe this should be clearly stated (both in the struct clk_ops
 kerneldoc header and in Documentation/clk.txt).
 
  Sadly, we still have to do a flag day and
  change all the .determine_rate ops when we want to add things.
 
 Yes, but the number of clk drivers implementing -determine_rate() is
 still quite limited compared to those implementing -round_rate().
 
  
  What if we changed determine_rate ops to take a struct
  clk_determine_info (or some better named structure) instead of
  the current list of arguments that it currently takes? Then when
  we want to make these sorts of framework wide changes we can just
  throw a new member into that structure and be done.
 
 I really like this idea, especially since I was wondering if we could
 pass other 'clk rate requirements' like the rounding policy (down,
 closest, up), or the maximum clk inaccuracy.
 
  
  It doesn't solve the unsigned long to int return value problem
  though. We can solve that by gradually introducing a new op and
  handling another case in the rounding path. If we can come up
  with some good name for that new op like .decide_rate or
  something then it makes things nicer in the long run. I like the
  name .determine_rate though :/

Okay, if you want a new method, how about this one:

struct clk_adjust_rate_req {
/* fields filled by the caller */
unsigned long rate; /* rate is updated by the clk driver */
unsigned long min;
unsigned long max;

/* fields filled by the clk driver */
struct clk_hw *best_parent;
unsigned long best_parent_rate;

/*
 * new fields I'd like to add at some point:
 * unsigned long max_inaccuracy;
 * something about the power consumption constraints :-)
 */
};

int (*adjust_rate)(struct clk_hw *hw, struct clk_adjust_rate_req *req);

 
 Why not changing the -determine_rate() prototype. As said above, the
 number of clk drivers implementing this function is still quite
 limited, and I guess we can have an ack for all of them.
 
  
  The benefit of all this is that we don't have to worry about
  finding the random clk providers that get added into other
  subsystems and fixing them up. If drivers actually care about
  this problem then they'll be fixed to use the proper op. FYI,
  last time we updated the function signature of .determine_rate we
  broke a couple drivers along the way.
  
 
 Hm, IMHO, adding a new op is not a good thing. I agree that it eases
 the transition, but ITOH you'll have to live with old/deprecated ops in
 your clk_ops structure with people introducing new drivers still using
 the old ops (see the number of clk drivers implementing -round_rate()
 instead of -determine_rate()).
 
 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-media 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

2015-05-07 Thread Stephen Boyd
On 04/30, 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.
 
 Signed-off-by: Boris Brezillon boris.brezil...@free-electrons.com
 Tested-by: Heiko Stuebner he...@sntech.de
 Tested-by: Mikko Perttunen mikko.perttu...@kapsi.fi
 Reviewed-by: Heiko Stuebner he...@sntech.de

This patch is fairly invasive, and it probably doesn't even
matter for most of these clock providers to be able to round a
rate above 2GHz. I've been trying to remove the .round_rate op
from the framework by encouraging new features via the
.determine_rate op. Sadly, we still have to do a flag day and
change all the .determine_rate ops when we want to add things.

What if we changed determine_rate ops to take a struct
clk_determine_info (or some better named structure) instead of
the current list of arguments that it currently takes? Then when
we want to make these sorts of framework wide changes we can just
throw a new member into that structure and be done.

It doesn't solve the unsigned long to int return value problem
though. We can solve that by gradually introducing a new op and
handling another case in the rounding path. If we can come up
with some good name for that new op like .decide_rate or
something then it makes things nicer in the long run. I like the
name .determine_rate though :/

The benefit of all this is that we don't have to worry about
finding the random clk providers that get added into other
subsystems and fixing them up. If drivers actually care about
this problem then they'll be fixed to use the proper op. FYI,
last time we updated the function signature of .determine_rate we
broke a couple drivers along the way.

-- 
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-media 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

2015-05-07 Thread Boris Brezillon
Hi Stephen,

On Wed, 6 May 2015 23:39:53 -0700
Stephen Boyd sb...@codeaurora.org wrote:

 On 04/30, 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.
  
  Signed-off-by: Boris Brezillon boris.brezil...@free-electrons.com
  Tested-by: Heiko Stuebner he...@sntech.de
  Tested-by: Mikko Perttunen mikko.perttu...@kapsi.fi
  Reviewed-by: Heiko Stuebner he...@sntech.de
 
 This patch is fairly invasive, and it probably doesn't even
 matter for most of these clock providers to be able to round a
 rate above 2GHz.

Fair enough.

 I've been trying to remove the .round_rate op
 from the framework by encouraging new features via the
 .determine_rate op.

Oh, I wasn't aware of that (BTW, that's a good thing).
Maybe this should be clearly stated (both in the struct clk_ops
kerneldoc header and in Documentation/clk.txt).

 Sadly, we still have to do a flag day and
 change all the .determine_rate ops when we want to add things.

Yes, but the number of clk drivers implementing -determine_rate() is
still quite limited compared to those implementing -round_rate().

 
 What if we changed determine_rate ops to take a struct
 clk_determine_info (or some better named structure) instead of
 the current list of arguments that it currently takes? Then when
 we want to make these sorts of framework wide changes we can just
 throw a new member into that structure and be done.

I really like this idea, especially since I was wondering if we could
pass other 'clk rate requirements' like the rounding policy (down,
closest, up), or the maximum clk inaccuracy.

 
 It doesn't solve the unsigned long to int return value problem
 though. We can solve that by gradually introducing a new op and
 handling another case in the rounding path. If we can come up
 with some good name for that new op like .decide_rate or
 something then it makes things nicer in the long run. I like the
 name .determine_rate though :/

Why not changing the -determine_rate() prototype. As said above, the
number of clk drivers implementing this function is still quite
limited, and I guess we can have an ack for all of them.

 
 The benefit of all this is that we don't have to worry about
 finding the random clk providers that get added into other
 subsystems and fixing them up. If drivers actually care about
 this problem then they'll be fixed to use the proper op. FYI,
 last time we updated the function signature of .determine_rate we
 broke a couple drivers along the way.
 

Hm, IMHO, adding a new op is not a good thing. I agree that it eases
the transition, but ITOH you'll have to live with old/deprecated ops in
your clk_ops structure with people introducing new drivers still using
the old ops (see the number of clk drivers implementing -round_rate()
instead of -determine_rate()).

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-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH v2 1/2] clk: change clk_ops' -round_rate() prototype

2015-04-30 Thread Boris Brezillon
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.

Signed-off-by: Boris Brezillon boris.brezil...@free-electrons.com
Tested-by: Heiko Stuebner he...@sntech.de
Tested-by: Mikko Perttunen mikko.perttu...@kapsi.fi
Reviewed-by: Heiko Stuebner he...@sntech.de
---
CC: Jonathan Corbet cor...@lwn.net
CC: Shawn Guo shawn@linaro.org
CC: ascha Hauer ker...@pengutronix.de
CC: David Brown dav...@codeaurora.org
CC: Daniel Walker dwal...@fifo99.com
CC: Bryan Huntsman bry...@codeaurora.org
CC: Tony Lindgren t...@atomide.com
CC: Paul Walmsley p...@pwsan.com
CC: Liviu Dudau liviu.du...@arm.com
CC: Sudeep Holla sudeep.ho...@arm.com
CC: Lorenzo Pieralisi lorenzo.pieral...@arm.com
CC: Ralf Baechle r...@linux-mips.org
CC: Max Filippov jcmvb...@gmail.com
CC: Heiko Stuebner he...@sntech.de
CC: Sylwester Nawrocki s.nawro...@samsung.com 
CC: Tomasz Figa tomasz.f...@gmail.com
CC: Barry Song bao...@kernel.org
CC: Viresh Kumar viresh.li...@gmail.com
CC: Emilio L??pez emi...@elopez.com.ar
CC: Maxime Ripard maxime.rip...@free-electrons.com
CC: Peter De Schrijver pdeschrij...@nvidia.com
CC: Prashant Gaikwad pgaik...@nvidia.com
CC: Stephen Warren swar...@wwwdotorg.org
CC: Thierry Reding thierry.red...@gmail.com
CC: Alexandre Courbot gnu...@gmail.com
CC: Tero Kristo t-kri...@ti.com
CC: Ulf Hansson ulf.hans...@linaro.org
CC: Michal Simek michal.si...@xilinx.com
CC: Philipp Zabel p.za...@pengutronix.de
CC: linux-...@vger.kernel.org
CC: linux-ker...@vger.kernel.org
CC: linux-arm-ker...@lists.infradead.org
CC: linux-arm-...@vger.kernel.org
CC: linux-o...@vger.kernel.org
CC: linux-m...@linux-mips.org
CC: patc...@opensource.wolfsonmicro.com
CC: linux-rockc...@lists.infradead.org
CC: linux-samsung-...@vger.kernel.org
CC: spear-de...@list.st.com
CC: linux-te...@vger.kernel.org
CC: dri-de...@lists.freedesktop.org
CC: linux-media@vger.kernel.org
CC: rtc-li...@googlegroups.com

 Documentation/clk.txt|  4 +-
 arch/arm/mach-imx/clk-busy.c |  2 +-
 arch/arm/mach-imx/clk-cpu.c  | 12 +++-
 arch/arm/mach-imx/clk-fixup-div.c|  2 +-
 arch/arm/mach-imx/clk-pfd.c  | 11 ++--
 arch/arm/mach-imx/clk-pllv2.c|  8 ++-
 arch/arm/mach-imx/clk-pllv3.c| 46 +++--
 arch/arm/mach-msm/clock-pcom.c   |  4 +-
 arch/arm/mach-omap2/clkt2xxx_virt_prcm_set.c | 13 ++--
 arch/arm/mach-omap2/clkt_clksel.c|  6 +-
 arch/arm/mach-omap2/clkt_dpll.c  | 21 +++---
 arch/arm/mach-omap2/clock.h  |  4 +-
 arch/arm/mach-omap2/dpll3xxx.c   | 27 +---
 arch/arm/mach-omap2/dpll44xx.c   | 26 +---
 arch/arm/mach-vexpress/spc.c | 11 +++-
 arch/mips/alchemy/common/clock.c | 13 ++--
 drivers/clk/at91/clk-h32mx.c | 24 ---
 drivers/clk/at91/clk-peripheral.c| 31 +
 drivers/clk/at91/clk-pll.c   | 14 ++--
 drivers/clk/at91/clk-plldiv.c| 22 ---
 drivers/clk/at91/clk-smd.c   | 24 ---
 drivers/clk/at91/clk-usb.c   | 16 +++--
 drivers/clk/clk-axi-clkgen.c |  5 +-
 drivers/clk/clk-cdce706.c| 46 ++---
 drivers/clk/clk-composite.c  | 23 ---
 drivers/clk/clk-divider.c| 16 +++--
 drivers/clk/clk-fixed-factor.c   |  7 +-
 drivers/clk/clk-fractional-divider.c | 16 +++--
 drivers/clk/clk-highbank.c   | 18 +++---
 drivers/clk/clk-si5351.c | 96 ++--
 drivers/clk/clk-si570.c  | 14 ++--
 drivers/clk/clk-u300.c   | 65 ++-
 drivers/clk/clk-vt8500.c | 27 
 drivers/clk/clk-wm831x.c | 11 ++--
 drivers/clk/clk-xgene.c  | 11 ++--
 drivers/clk/clk.c| 14 ++--
 drivers/clk/mmp/clk-frac.c   | 14 ++--
 drivers/clk/mvebu/clk-corediv.c  |  7 +-
 drivers/clk/mvebu/clk-cpu.c  |  9 +--
 drivers/clk/mxs/clk-div.c|  4 +-
 drivers/clk/mxs/clk-frac.c   | 11 ++--
 drivers/clk/mxs/clk-ref.c| 11 ++--
 drivers/clk/qcom/clk-regmap-divider.c|  4 +-
 drivers/clk/rockchip/clk-pll.c   | 13 ++--
 drivers/clk/samsung/clk-pll.c| 13 ++--
 drivers/clk/shmobile/clk-div6.c  |  7 +-
 drivers/clk/shmobile/clk-rcar-gen2.c |  9 +--
 drivers/clk/sirf/clk-common.c| 18 +++---