Skip to contentSkip to conversation

clk: Update comment for clk_round_rate()

PATCH New viresh kumar 4 participants 6 messages Apr 4, 2012 – Apr 5, 2012

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 clk: Update comment for clk_round_rate()

From: Viresh Kumar <[email protected]>

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.

---

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(-)
drivers/clk/clk.c +0 -0
index 9cf6f59..2f58455 100644
@@ -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)
{

include/linux/clk.h +2 -1
index b025272..19d31ef 100644
@@ -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
  *
Signature · 1 line
1.7.9
#1 lore raw

PATCH 1/1 clk: Update comment for clk_round_rate()

Quoted 7 lines · viresh kumar

From: Viresh Kumar <[email protected]>

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.

Quoted 7 lines

---

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

Quoted 44 lines

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

drivers/clk/clk.c +0 -0
index 9cf6f59..2f58455 100644
@@ -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)
?{

include/linux/clk.h +0 -0
index b025272..19d31ef 100644
@@ -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

#2 lore raw

PATCH 1/1 clk: Update comment for clk_round_rate()

Quoted 4 lines · Mike

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.

Quoted 6 lines

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.

Signature · 1 line
viresh
#3 lore raw

PATCH 1/1 clk: Update comment for clk_round_rate()

Hello,

Quoted 6 lines · Viresh Kumar
Quoted 4 lines · Mike

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).

Quoted 8 lines

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

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
#4 lore raw

PATCH 1/1 clk: Update comment for clk_round_rate()

Quoted 3 lines · Uwe Kleine-K?nig

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.

Signature · 1 line
viresh
#5 lore raw

PATCH 1/1 clk: Update comment for clk_round_rate()

Quoted 9 lines · Viresh Kumar
Quoted 3 lines · Uwe Kleine-K?nig

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

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