clk: ti: add 'ti,round-rate' flag

The current DPLL code does not try to round the clock rate, and instead returns an error if the requested clock rate cannot be produced exactly by the DPLL.

It could be argued that this is a bug, but as the current drivers may depend on that behavior, a new flag 'ti,round-rate' is added which enables clock rate rounding.

Signed-off-by Tomi Valkeinen <[email protected]>

---
  Documentation/devicetree/bindings/clock/ti/dpll.txt | 2 ++
  drivers/clk/ti/dpll.c                               | 3 +++
  include/linux/clk/ti.h                              | 1 +
  3 files changed, 6 insertions(+)

Documentation/devicetree/bindings/clock/ti/dpll.txt +2 -0

index 30bfdb7c9f18..7912d876e858 100644
@@ -48,6 +48,8 @@ Optional properties:
  	- ti,low-power-stop : DPLL supports low power stop mode, gating output
  	- ti,low-power-bypass : DPLL output matches rate of parent bypass clock
  	- ti,lock : DPLL locks in programmed rate
+- ti,round-rate : if not set, the dpll will return an error if asked for a
+                  clock rate that it cannot produce exactly.
 
  Examples:
  	dpll_core_ck: dpll_core_ck@44e00490 {

drivers/clk/ti/dpll.c +3 -0

index 7e498a44f97d..c5858c46b58c 100644
@@ -265,6 +265,9 @@ static void __init of_ti_dpll_setup(struct device_node *node,
  	if (dpll_mode)
  		dd->modes = dpll_mode;
 
+	if (of_property_read_bool(node, "ti,round-rate"))
+		clk_hw->flags |= DPLL_USE_ROUNDED_RATE;
+
  	ti_clk_register_dpll(&clk_hw->hw, node);
  	return;
 

include/linux/clk/ti.h +1 -0

index 4a21a872dbbd..79e8a7876457 100644
@@ -155,6 +155,7 @@ struct clk_hw_omap {
  #define INVERT_ENABLE		(1 << 4)	/* 0 enables, 1 disables */
  #define CLOCK_CLKOUTX2		(1 << 5)
  #define MEMMAP_ADDRESSING	(1 << 6)
+#define DPLL_USE_ROUNDED_RATE	(1 << 7)	/* dpll's round_rate() returns rounded rate */
 
  /* CM_CLKEN_PLL*.EN* bit values - not all are available for every DPLL */
  #define DPLL_LOW_POWER_STOP	0x1

Quoted 7 lines · Tomi Valkeinen

The current DPLL code does not try to round the clock rate, and instead returns an error if the requested clock rate cannot be produced exactly by the DPLL.

It could be argued that this is a bug, but as the current drivers may depend on that behavior, a new flag 'ti,round-rate' is added which enables clock rate rounding.

Someone could probably argue that this flag is not a hardware feature,

but instead is used to describe linux-kernel behavior, and would probably be frowned upon by the DT enthusiasts. Othen than that, I like this approach better than a global setting, but would like second opinions here.

Quoted 10 lines · Tomi Valkeinen

Quoted 7 lines

The current DPLL code does not try to round the clock rate, and instead returns an error if the requested clock rate cannot be produced exactly by the DPLL.

It could be argued that this is a bug, but as the current drivers may depend on that behavior, a new flag 'ti,round-rate' is added which enables clock rate rounding.

Someone could probably argue that this flag is not a hardware feature,

I fully agree.

Quoted 4 lines

but instead is used to describe linux-kernel behavior, and would probably be frowned upon by the DT enthusiasts. Othen than that, I like this approach better than a global setting, but would like second opinions here.

I think the dpll code should always do rounding. That's what round_rate() is supposed to do, afaik. The current behavior of not rounding and returning an error is a bug in my opinion.

From include/linux/clk.h:

So, if you ask me, the whole flag is just for the purpose of keeping the current drivers working, which could depend on the broken behavior. But I think we cannot have such drivers (functional, at least) in any case, as the clk-divider driver is broken and doesn't handle the errors the dpll code currently returns for non-exact rates.

Tomi

Quoting Tomi Valkeinen (2014-05-12 05:13:51)

Quoted 14 lines · Tero Kristo

Quoted 10 lines · Tomi Valkeinen

Quoted 7 lines

The current DPLL code does not try to round the clock rate, and instead returns an error if the requested clock rate cannot be produced exactly by the DPLL.

It could be argued that this is a bug, but as the current drivers may depend on that behavior, a new flag 'ti,round-rate' is added which enables clock rate rounding.

Someone could probably argue that this flag is not a hardware feature,

I fully agree.

Quoted 8 lines

Quoted 4 lines

but instead is used to describe linux-kernel behavior, and would probably be frowned upon by the DT enthusiasts. Othen than that, I like this approach better than a global setting, but would like second opinions here.

I think the dpll code should always do rounding. That's what round_rate() is supposed to do, afaik. The current behavior of not rounding and returning an error is a bug in my opinion.

From include/linux/clk.h:

/**
* clk_round_rate - adjust a rate to the exact rate a clock can provide
* @clk: clock source
* @rate: desired clock rate in Hz
*
* Returns rounded clock rate in Hz, or negative errno.
*/
long clk_round_rate(struct clk *clk, unsigned long rate);

Definitely not rounding the rate is a bug, with respect to the API definition. Has anyone tried making the new flag as the default behavior and seeing if anything breaks?

For those users of the omapconf tool I enjoy doing something like the following:

<boot current, buggy behavior>
omapconf dump prcm > old

<boot with Tomi's flag enabled for all DPLLs by default> omapconf dump prcm > new

diff -u old new

This should reveal any deltas, assuming the board boots and doesn't let magic smoke out.

Quoting Tomi Valkeinen (2014-05-12 05:13:51)

Quoted 14 lines · Tero Kristo

Quoted 10 lines · Tomi Valkeinen

Quoted 7 lines

The current DPLL code does not try to round the clock rate, and instead returns an error if the requested clock rate cannot be produced exactly by the DPLL.

It could be argued that this is a bug, but as the current drivers may depend on that behavior, a new flag 'ti,round-rate' is added which enables clock rate rounding.

Someone could probably argue that this flag is not a hardware feature,

I fully agree.

Quoted 8 lines

Quoted 4 lines

but instead is used to describe linux-kernel behavior, and would probably be frowned upon by the DT enthusiasts. Othen than that, I like this approach better than a global setting, but would like second opinions here.

I think the dpll code should always do rounding. That's what round_rate() is supposed to do, afaik. The current behavior of not rounding and returning an error is a bug in my opinion.

From include/linux/clk.h:

/**
* clk_round_rate - adjust a rate to the exact rate a clock can provide
* @clk: clock source
* @rate: desired clock rate in Hz
*
* Returns rounded clock rate in Hz, or negative errno.
*/
long clk_round_rate(struct clk *clk, unsigned long rate);

Definitely not rounding the rate is a bug, with respect to the API definition. Has anyone tried making the new flag as the default behavior and seeing if anything breaks?

Quoted 35 lines

Quoting Tomi Valkeinen (2014-05-12 05:13:51)

Quoted 14 lines · Tero Kristo

Quoted 10 lines · Tomi Valkeinen

Quoted 7 lines

The current DPLL code does not try to round the clock rate, and instead returns an error if the requested clock rate cannot be produced exactly by the DPLL.

It could be argued that this is a bug, but as the current drivers may depend on that behavior, a new flag 'ti,round-rate' is added which enables clock rate rounding.

Someone could probably argue that this flag is not a hardware feature,

I fully agree.

Quoted 8 lines

Quoted 4 lines

but instead is used to describe linux-kernel behavior, and would probably be frowned upon by the DT enthusiasts. Othen than that, I like this approach better than a global setting, but would like second opinions here.

I think the dpll code should always do rounding. That's what round_rate() is supposed to do, afaik. The current behavior of not rounding and returning an error is a bug in my opinion.

From include/linux/clk.h:

/**
* clk_round_rate - adjust a rate to the exact rate a clock can provide
* @clk: clock source
* @rate: desired clock rate in Hz
*
* Returns rounded clock rate in Hz, or negative errno.
*/
long clk_round_rate(struct clk *clk, unsigned long rate);

Quoted 3 lines

Definitely not rounding the rate is a bug, with respect to the API definition. Has anyone tried making the new flag as the default behavior and seeing if anything breaks?

The v1 of the patch fixed the rounding unconditionally:

http://article.gmane.org/gmane.linux.ports.arm.kernel/295077

Paul wanted it optional so that existing drivers would not break. No one knows if there is such a driver, or what would the driver's code look like that would cause an issue.

And, as I've pointed out in the above thread, as clk-divider driver doesn't an error code from the dpll driver, my opinion is that such drivers would not work even now.

I like v1 more.

In any case, I hope we'd get something merged ASAP so that we fix the display AM3xxx boards and we'd still have time to possibly find out if some other driver breaks.

Tomi

Quoting Tomi Valkeinen (2014-05-15 05:25:37)

Quoted 54 lines · Mike Turquette

Quoted 35 lines

Quoting Tomi Valkeinen (2014-05-12 05:13:51)

Quoted 14 lines · Tero Kristo

Quoted 10 lines · Tomi Valkeinen

Quoted 7 lines

The current DPLL code does not try to round the clock rate, and instead returns an error if the requested clock rate cannot be produced exactly by the DPLL.

It could be argued that this is a bug, but as the current drivers may depend on that behavior, a new flag 'ti,round-rate' is added which enables clock rate rounding.

Someone could probably argue that this flag is not a hardware feature,

I fully agree.

Quoted 8 lines

Quoted 4 lines

but instead is used to describe linux-kernel behavior, and would probably be frowned upon by the DT enthusiasts. Othen than that, I like this approach better than a global setting, but would like second opinions here.

I think the dpll code should always do rounding. That's what round_rate() is supposed to do, afaik. The current behavior of not rounding and returning an error is a bug in my opinion.

From include/linux/clk.h:

/**
* clk_round_rate - adjust a rate to the exact rate a clock can provide
* @clk: clock source
* @rate: desired clock rate in Hz
*
* Returns rounded clock rate in Hz, or negative errno.
*/
long clk_round_rate(struct clk *clk, unsigned long rate);

Quoted 3 lines

Definitely not rounding the rate is a bug, with respect to the API definition. Has anyone tried making the new flag as the default behavior and seeing if anything breaks?

The v1 of the patch fixed the rounding unconditionally:

http://article.gmane.org/gmane.linux.ports.arm.kernel/295077

Paul wanted it optional so that existing drivers would not break. No one knows if there is such a driver, or what would the driver's code look like that would cause an issue.

And, as I've pointed out in the above thread, as clk-divider driver doesn't an error code from the dpll driver, my opinion is that such drivers would not work even now.

I like v1 more.

Quoted 3 lines

In any case, I hope we'd get something merged ASAP so that we fix the display AM3xxx boards and we'd still have time to possibly find out if some other driver breaks.

Resurrecting this thread. Can we reach a consensus? I prefer V1 as well for the reasons stated above, and also since ti,round-rate isn't really describing the hardware at all in DT.

We can always see how it goes and fix it up afterwards when everything explodes.

What's really needed is better control over rounding in the clock code.

Both the driver API should be improved; and, to the extent that clock sources share the same underlying code implementation, probably some clock source configuration data enhancement is needed (DT or whatever).

Furthermore, clk_set_rate() should never silently round a requested rate - it should just return an error if it can't satisfy the requested rate precisely. Silently rounding a rate is racy, and, if we care about drivers behaving consistently across different integration environments, prone to behavior that the driver may not expect.

Frankly, a DT "ti,round-rate" property is risible. I certainly understand why you don't like it and I don't know why that specific property was proposed. The issue has never been whether clk_round_rate() should round

rates. Of course it should. The more pertinent question is, *how* should it round the rate? A more reasonable DT property approach would be to specify how the clock's rate should be rounded: to the closest rate, to the closest rate equal to or less than the desired rate, to the closest rate greater than or equal to the desired rate; what the tolerance of the "closest rate" should be, etc. But drivers, e.g. many drivers that control external I/O, should always be able to state that they want an exact rate.

...

In terms of the short-term thing to do, I'm currently thinking that the thing to do is to modify the PLL set_rate() code in mach-omap2/ to not round the rate at all, and then switch the PLL rate rounding code to equal-or-closest-lesser-rate, with something like the oldhardcoded rate tolerances. That should push the responsibility out to the drivers to control how they want the rounding to happen. Then the drivers should be able to probe available rates with repeated calls to clk_round_rate() if they want, and if people get unhappy with that, then it might drive rounding improvements in the clock API.

But all this won't happen in -rc8; this is 3.16 material..

Quoted 2 lines

We can always see how it goes and fix it up afterwards when everything explodes.

No thanks... that's what leads to these kinds of messes :-(

Quoted 1 line · Paul Walmsley

What's really needed is better control over rounding in the clock code.

Well, if you ask me, what's really needed _now_ is to fix the bug that makes am3xxx (and am4xxx when it's merged) display not to work. I don't need a fix that solves all the clock set_rate/round issues once and for all, I just want to get the display working.

Quoted 10 lines

Both the driver API should be improved; and, to the extent that clock sources share the same underlying code implementation, probably some clock source configuration data enhancement is needed (DT or whatever).

Furthermore, clk_set_rate() should never silently round a requested rate - it should just return an error if it can't satisfy the requested rate precisely. Silently rounding a rate is racy, and, if we care about drivers behaving consistently across different integration environments, prone to behavior that the driver may not expect.

Quoted 3 lines

Frankly, a DT "ti,round-rate" property is risible. I certainly understand why you don't like it and I don't know why that specific property was proposed. The issue has never been whether clk_round_rate() should round

I created the property for the v2 because (if I recall right) you were worried that fixing the rounding bug unconditionally could break some drivers, and suggested that it should be used only for DSS.

Quoted 7 lines

But all this won't happen in -rc8; this is 3.16 material..

Quoted 2 lines

We can always see how it goes and fix it up afterwards when everything explodes.

No thanks... that's what leads to these kinds of messes :-(

I don't understand how this fix would lead to a mess.

1. AM3xxx/AM4xxx display is broken
2. The PLL round function is broken, it doesn't round
3. The fix to omap2_dpll_round_rate has been in TI's kernel for a long time, no problems observed
4. We've ran test runs with linux-next + the fix, no problems observed

Whatever we do to the (common, not DSS-specific) clock code to fix this issue, it has to make sense independently of your specific DSS needs.

This is why I asked you for a DSS-specific change, since it would effectively avoid this basic principle.

Right now, in my view, it does not make sense to have a PLL clk_set_rate() function that unconditionally rounds to the "closest" rate for all users. As I've written before, callers could end up with a clock rate that is different than what's needed for a synchronous I/O user that expects an exact rate. I am concerned about both rounding to a lower rate and rounding to a higher rate in this case, although the higher rate is marginally more of a concern to me since the resulting rate may be higher than the device is characterized for at a given voltage.

Furthermore, as a general interface principle, allowing clk_set_rate() to silently round rates could lead to unexpected behavior, since the set of rates that clk_set_rate() can round to may change between the call to clk_round_rate() and the call to clk_set_rate().

So again I think that the right way to deal with this is to:

1. avoid silently rounding rates in clk_set_rate() and to return an error if calling clk_set_rate() would result in a rate other than the one desired; and to

2. modify clk_round_rate() such that it returns the closest lowest-or-equal rate match to the target rate requested.

Furthermore, as a general interface principle, allowing clk_set_rate() to silently round rates could lead to unexpected behavior, since the set of rates that clk_set_rate() can round to may change between the call to clk_round_rate() and the call to clk_set_rate().

So again I think that the right way to deal with this is to:

1. avoid silently rounding rates in clk_set_rate() and to return an error if calling clk_set_rate() would result in a rate other than the one desired; and to

2. modify clk_round_rate() such that it returns the closest lowest-or-equal rate match to the target rate requested.

- Paul

Let's get consensus on my above question before we solidify the API. It's worth noting that the current behavior of rounding the rate within clk_set_rate is actually what Russell had in mind back in 2010. See this thread[1] for details.