linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] clk: Update comment for clk_round_rate()
@ 2012-04-04 17:27 viresh kumar
  2012-04-04 18:07 ` Turquette, Mike
  0 siblings, 1 reply; 6+ messages in thread
From: viresh kumar @ 2012-04-04 17:27 UTC (permalink / raw)
  To: linux-arm-kernel

From: Viresh Kumar <viresh.kumar@st.com>

clk_round_rate() must not provide rate greater than requested rate, as hardware
may not work at it. This wasn't clear from documentation of clk_round_rate().

This patch updates documentation for this.

Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
---

Mike,

I don't know if this holds true for common clock framework or not?
But it looks more logical to me and this is how it was implemented for SPEAr.

 drivers/clk/clk.c   |    6 +++---
 include/linux/clk.h |    3 ++-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 9cf6f59..2f58455 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -603,9 +603,9 @@ unsigned long __clk_round_rate(struct clk *clk,
unsigned long rate)
  * @clk: the clk for which we are rounding a rate
  * @rate: the rate which is to be rounded
  *
- * Takes in a rate as input and rounds it to a rate that the clk can actually
- * use which is then returned.  If clk doesn't support round_rate operation
- * then the parent rate is returned.
+ * Takes in a rate as input and rounds it to a rate <= input rate that the clk
+ * can actually use which is then returned.  If clk doesn't support round_rate
+ * operation then the parent rate is returned.
  */
 long clk_round_rate(struct clk *clk, unsigned long rate)
 {
diff --git a/include/linux/clk.h b/include/linux/clk.h
index b025272..19d31ef 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -213,7 +213,8 @@ void clk_put(struct clk *clk);


 /**
- * clk_round_rate - adjust a rate to the exact rate a clock can provide
+ * clk_round_rate - adjust a rate to the exact rate a clock can provide. Output
+ *		    clock rate should be <= requested rate
  * @clk: clock source
  * @rate: desired clock rate in Hz
  *
-- 
1.7.9

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH] clk: Update comment for clk_round_rate()
  2012-04-04 17:27 [PATCH] clk: Update comment for clk_round_rate() viresh kumar
@ 2012-04-04 18:07 ` Turquette, Mike
  2012-04-05  5:11   ` Viresh Kumar
  0 siblings, 1 reply; 6+ messages in thread
From: Turquette, Mike @ 2012-04-04 18:07 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Apr 4, 2012 at 10:27 AM, viresh kumar <viresh.linux@gmail.com> wrote:
> From: Viresh Kumar <viresh.kumar@st.com>
>
> clk_round_rate() must not provide rate greater than requested rate, as hardware
> may not work at it. This wasn't clear from documentation of clk_round_rate().
>
> This patch updates documentation for this.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@st.com>
> ---
>
> Mike,
>
> I don't know if this holds true for common clock framework or not?
> But it looks more logical to me and this is how it was implemented for SPEAr.

The common clk framework doesn't enforce any policy like this, nor do
I think it should.  The clk framework is far from complete and I
wouldn't be surprised if we see folks who want their rounded rate to
represent a minimum value (instead of a maximum as your patch states).
 This might be achieved later on by a platform-specific or
framework-wide flag, similar to how CPUfreq does it today
(CPUFREQ_RELATION_L and CPUFREQ_RELATION_H).

Thank you for submitting the patch, but for now it's a NACK.

Regards,
Mike

>
> ?drivers/clk/clk.c ? | ? ?6 +++---
> ?include/linux/clk.h | ? ?3 ++-
> ?2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 9cf6f59..2f58455 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -603,9 +603,9 @@ unsigned long __clk_round_rate(struct clk *clk,
> unsigned long rate)
> ?* @clk: the clk for which we are rounding a rate
> ?* @rate: the rate which is to be rounded
> ?*
> - * Takes in a rate as input and rounds it to a rate that the clk can actually
> - * use which is then returned. ?If clk doesn't support round_rate operation
> - * then the parent rate is returned.
> + * Takes in a rate as input and rounds it to a rate <= input rate that the clk
> + * can actually use which is then returned. ?If clk doesn't support round_rate
> + * operation then the parent rate is returned.
> ?*/
> ?long clk_round_rate(struct clk *clk, unsigned long rate)
> ?{
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index b025272..19d31ef 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -213,7 +213,8 @@ void clk_put(struct clk *clk);
>
>
> ?/**
> - * clk_round_rate - adjust a rate to the exact rate a clock can provide
> + * clk_round_rate - adjust a rate to the exact rate a clock can provide. Output
> + * ? ? ? ? ? ? ? ? clock rate should be <= requested rate
> ?* @clk: clock source
> ?* @rate: desired clock rate in Hz
> ?*
> --
> 1.7.9
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH] clk: Update comment for clk_round_rate()
  2012-04-04 18:07 ` Turquette, Mike
@ 2012-04-05  5:11   ` Viresh Kumar
  2012-04-05  7:03     ` Uwe Kleine-König
  0 siblings, 1 reply; 6+ messages in thread
From: Viresh Kumar @ 2012-04-05  5:11 UTC (permalink / raw)
  To: linux-arm-kernel

On 4/4/2012 11:37 PM, Turquette, Mike wrote:
> The common clk framework doesn't enforce any policy like this, nor do
> I think it should.  The clk framework is far from complete and I
> wouldn't be surprised if we see folks who want their rounded rate to
> represent a minimum value (instead of a maximum as your patch states).

Ok. Just for example, suppose foo_clk can have following rates:
100, 110, 120, 130, 140.

And we call round_rate with a value of 126.

Now i can't visualize why would anybody want it to return 130 (above the
limits requested)? I agree both 100 and 120 can be returned, based on
your below logic.

>  This might be achieved later on by a platform-specific or
> framework-wide flag, similar to how CPUfreq does it today
> (CPUFREQ_RELATION_L and CPUFREQ_RELATION_H).
> 

> Thank you for submitting the patch, but for now it's a NACK.

No Probs.

-- 
viresh

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH] clk: Update comment for clk_round_rate()
  2012-04-05  5:11   ` Viresh Kumar
@ 2012-04-05  7:03     ` Uwe Kleine-König
  2012-04-05  7:07       ` Viresh Kumar
  0 siblings, 1 reply; 6+ messages in thread
From: Uwe Kleine-König @ 2012-04-05  7:03 UTC (permalink / raw)
  To: linux-arm-kernel

Hello,

On Thu, Apr 05, 2012 at 10:41:58AM +0530, Viresh Kumar wrote:
> On 4/4/2012 11:37 PM, Turquette, Mike wrote:
> > The common clk framework doesn't enforce any policy like this, nor do
> > I think it should.  The clk framework is far from complete and I
> > wouldn't be surprised if we see folks who want their rounded rate to
> > represent a minimum value (instead of a maximum as your patch states).
> 
> Ok. Just for example, suppose foo_clk can have following rates:
> 100, 110, 120, 130, 140.
> 
> And we call round_rate with a value of 126.
> 
> Now i can't visualize why would anybody want it to return 130 (above the
> limits requested)? I agree both 100 and 120 can be returned, based on
> your below logic.
It depends on your scenario. E.g. for an UART clock it might be better
to choose 130. Here choosing the frequency is not about a fixed maximum
but to match the sample rate of the device connect to your UART.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH] clk: Update comment for clk_round_rate()
  2012-04-05  7:03     ` Uwe Kleine-König
@ 2012-04-05  7:07       ` Viresh Kumar
  2012-04-05  7:26         ` Uwe Kleine-König
  0 siblings, 1 reply; 6+ messages in thread
From: Viresh Kumar @ 2012-04-05  7:07 UTC (permalink / raw)
  To: linux-arm-kernel

On 4/5/2012 12:33 PM, Uwe Kleine-K?nig wrote:
> It depends on your scenario. E.g. for an UART clock it might be better
> to choose 130. Here choosing the frequency is not about a fixed maximum
> but to match the sample rate of the device connect to your UART.

My point was: we can almost always guarantee that device will still
be operational at any frequency below the one requested. But it may
not work at any freq above the requested one. So, giving 130 here may
break it.

-- 
viresh

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH] clk: Update comment for clk_round_rate()
  2012-04-05  7:07       ` Viresh Kumar
@ 2012-04-05  7:26         ` Uwe Kleine-König
  0 siblings, 0 replies; 6+ messages in thread
From: Uwe Kleine-König @ 2012-04-05  7:26 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Apr 05, 2012 at 12:37:24PM +0530, Viresh Kumar wrote:
> On 4/5/2012 12:33 PM, Uwe Kleine-K?nig wrote:
> > It depends on your scenario. E.g. for an UART clock it might be better
> > to choose 130. Here choosing the frequency is not about a fixed maximum
> > but to match the sample rate of the device connect to your UART.
> 
> My point was: we can almost always guarantee that device will still
> be operational at any frequency below the one requested. But it may
> not work at any freq above the requested one. So, giving 130 here may
> break it.
It depends. If for satisfying a request for 115200 Bd you have the choice
between 115202 Bd and 57601 Bd, I'd recommend the former.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2012-04-05  7:26 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-04-04 17:27 [PATCH] clk: Update comment for clk_round_rate() viresh kumar
2012-04-04 18:07 ` Turquette, Mike
2012-04-05  5:11   ` Viresh Kumar
2012-04-05  7:03     ` Uwe Kleine-König
2012-04-05  7:07       ` Viresh Kumar
2012-04-05  7:26         ` Uwe Kleine-König

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).