[PATCH 1/3] clk: ti: add 'ti,round-rate' flag

[PATCH 1/3] clk: ti: add 'ti,round-rate' flag

[Paul Walmsley]

On Fri, 30 May 2014, Mike Turquette wrote:

> Quoting Tomi Valkeinen (2014-05-15 05:25:37)
> > On 15/05/14 09:08, Mike Turquette wrote:
> > > Quoting Tomi Valkeinen (2014-05-12 05:13:51)
> > >> On 12/05/14 15:02, Tero Kristo wrote:
> > >>> On 05/08/2014 12:06 PM, Tomi Valkeinen wrote:
> > >>>> 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.
> > >>
> > >>> 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?
> > 
> > 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.
> 
> 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.

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


> 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 :-( 


- Paul

[PATCH 1/3] clk: ti: add 'ti,round-rate' flag

[Tomi Valkeinen]

On 03/06/14 22:35, Paul Walmsley wrote:

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

> 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 

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.

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

I agree, the current clock rounding is rather undefined, and I need to
do my own calculations in omapdss to "probe" the clock rates.

> But all this won't happen in -rc8; this is 3.16 material..
> 
> 
>> 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

I agree that there's a possibility that some driver could break with the
fix. I think it's a theoretical possibility, because clk-divider.c (I
think there's always a divider after the pll in omaps) doesn't handle
the error from the dpll code, so I don't really see how a driver could
manage to handle the error as the error is not conveyed to the driver.

In any case, if a driver breaks because of the fix, the driver's clock
handling is totally broken in any case, and it should be fixed.

Do we want to keep an important subsystem totally broken because there's
a theoretical possibility that the fix could break something else?

The fix was first posted in January. And we're still discussing about
it. And I still don't understand why the fix could cause problems. So
if the patch (v1 preferably) cannot be applied to 3.15, I'd very much
like to hear how it makes the situation worse, either for the current
kernel or for any future changes to the drivers/clock code.

 Tomi


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140604/ff93366d/attachment.sig>

[PATCH 1/3] clk: ti: add 'ti,round-rate' flag

[Paul Walmsley]

On Wed, 4 Jun 2014, Tomi Valkeinen wrote:

> On 03/06/14 22:35, Paul Walmsley wrote:
> 
> > 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.

Yep, I understand; it's not a great situation.

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

Here's the summary from my perspective: I don't want to merge a patch that 
results in a behavior change for all PLLs just to fix one user of one 
single PLL.  That's why I haven't merged your v1 patches.

So that's why I asked you for patches that limit the impact of the change 
to the PLL that you're trying to change.  You (graciously) respun those 
patches, but with a DT flag that doesn't really make sense, and those 
patches were NACK'ed by others.  So that's why the v2 patches haven't gone 
anywhere.

> > But all this won't happen in -rc8; this is 3.16 material..
> > 
> > 
> >> 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.


- Paul

[PATCH 1/3] clk: ti: add 'ti,round-rate' flag

[Tomi Valkeinen]

On 13/06/14 22:53, Paul Walmsley wrote:

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

I agree. I think the fix (v1) makes sense for all users of the pll.
Correct me if I'm wrong, but the current state is:

- The pll's round_rate does not round, but instead returns an error if
it cannot produce the exact rate that was requested.

- All OMAP PLL's have dividers after them, handled with clk-divider driver.

- clk-divider driver does not handle errors from round_rate, but instead
goes on and the resulting clock rate is more or less garbage.

- If a driver requests a clock rate that cannot be produced exactly,
it'll instead get a garbage clock rate, leading to undefined behavior.

So surely fixing the round_rate so that the clock code behaves sanely,
if not optimally, is much better than the current undefined behavior?

And again, currently a driver needs to request an exact clock rate, as
otherwise it'll get a garbage clock rate, and I'm quite sure it won't
work correctly. So all the current drivers request an exact clock rate,
and they are not affected by the fix, or the drivers are totally broken
already.

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

Yes, a DSS specific change would be marginally safer, but I think the
DSS specific options get rather hacky or complex. One option was the DT
flag, which was not accepted. Another would be adding a list of accepted
clock rates to DSS driver, and the DSS driver would "round" internally.
Quite hacky, I wouldn't like to go there.

And as I don't see the generic fix causing any problems, I don't see why
we would want to make big hacks somewhere else, just to avoid the
generic fix.

I'm open to ideas how to make a relatively clean DSS specific fix for
this, which can be merged for the next -rc.

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

Maybe, but, correct me if I'm wrong, that's how the clock set_rate has
worked (always?). The exact behavior for set_rate and round_rate isn't
defined anywhere I've seen, which to me means the behavior is
implementation specific.

However, I think it's clear that round_rate _should_ round, which it
currently does not. With this fix, both set_rate and round_rate work
inside the "spec".

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

I see that as a separate thing. What you're talking about is improving
the clock API. What I'm talking about is fixing a major (at least to the
owners of AM3xxx boards) bug in the mainline kernel, with as minimal
changes as possible.

The fix doesn't need to solve all the possible issues around clock rate.
It just needs to fix the bug we have, without causing any new bugs.
Afaik, the fix does not introduce any new problems. The behavior of
set_rate and round_rate can be improved later.

If the fix would be merged asap, we would get as long testing time as
possible before the 3.16 is released. If we find any drivers broken by
this fix, we can fix the drivers or in the worst case revert the fix.

 Tomi


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140616/3e80eca0/attachment-0001.sig>

[PATCH 1/3] clk: ti: add 'ti,round-rate' flag

[Mike Turquette]

Quoting Paul Walmsley (2014-06-13 12:53:06)
> 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().

Rounding the rate will always happen in a call to clk_set_rate. We need
to know what rate that clock node can actually support. The real
question is what do we do with that rounded rate. There are two options:

1) abort and return an error code if the rounded rate does not exactly
match the requested rate

2) use the rounded rate even if it does not match the requested rate

#2 has some variations worth considering:

a) allow a rounded rate within a specified tolerance (e.g. 10% of the
requested rate)
b) allow a rounded rate so long as it is rounded down from the requested
rate
c) same as #b, but rounded up, etc.

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

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.

Also note that this opens up a can of worms regarding *intended rates*.
For example, if you always abort clk_set_rate if the rounded rate does
not match the requested rate, then what does that mean for a child clock
that will be changed by some parent clock higher up the tree? If that
child gets put to a rate that would never be returned by clk_round_rate
then is the framework responsible for walking down the tree, checking
every child and aborting when the first one is off by a few hertz?
That's going to be messy.

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

I agree that the behavior of clk_round_rate needs to be defined once and
for all. I also think that clk_round_ceil, clk_round_floor and
clk_round_exact aren't terrible ideas either.

I'll kick off a thread on this topic shortly, and we can hopefully gain
some consensus.

Regards,
Mike

[1] https://lkml.org/lkml/2010/7/14/330

> 
> 
> - Paul

[PATCH 1/3] clk: ti: add 'ti,round-rate' flag

[Russell King - ARM Linux]

On Tue, Jul 01, 2014 at 02:40:17PM -0700, Mike Turquette wrote:
> 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.

Yes, because the problem is that doing anything else is racy.

Consider the sequence (which some folk who don't understand this point
have coded all over the place):

	rate = clk_round_rate(clk, my_rate);

	clk_set_rate(clk, rate);

The problem here is that between the two calls, it is possible that (as
you say) the parent clocks could be reconfigured - there are no locks
held at any point to protect against that happening.

So, what the above sequence means is that:

	rate = clk_round_rate(clk, my_rate);

would return the rounded rate that the clock could provide for my_rate.
Then the clk's parent changes.

We now do the clk_set_rate().  This re-rounds the rate, and we get a
different rate to the one which was passed.  This could well be end
result from the rate you would get if you simply did:

	clk_set_rate(clk, my_rate);

Let's say that we did augment the interface with a load of clk_round_xxx()
method functions, and made clk_set_rate() error out of the passed rate
is not possible.  On the face of it, this looks like a sensible way
forward - but wait!  What if we re-read the above with these new
conditions and that race occurs:

	rate = clk_round_nearest(clk, my_rate);
	/* clk is reparented */
	err = clk_set_rate(clk, rate);

Now, err indicates an error.  So now we need to audit every driver using
this that they do something like this:

	for (try = 0; try < 10; try++) {
		rate = clk_round_nearest(clk, my_rate);

		err = clk_set_rate(clk, rate);
		if (!err)
			continue;
	}

	if (err)
		dev_err(dev, "failed to set clock rate to %lu: %d\n",
			my_rate, err);

and this is still racy and unreliable (who says ten loops is enough?)

You could introduce a per-clock lock around this, but that also gets
messy, and makes the API harder to use (and more error-prone for the
driver author to use.)

What /may/ be a better idea is to pass a function pointer to a new
clk_round_rate() or clk_set_rate() pair of functions - both accept
the same function pointer.  This function pointer points at the
rounding method that is desired, which would be called by the
underlying implementation with the appropriate locks.

Yes, it still doesn't guarantee that this sequence:

	rate = clk_round_new_rate(clk, my_rate, clk_round_nearest);

	clk_set_new_rate(clk, my_rate, clk_round_nearest);

will end up with the same clock, but it will help ensure that there
is a consistent manner in which both of these functions operate, and
that there won't be any doubling up of rounding errors caused by
serially calling clk_round_rate followed by clk_set_rate.

-- 
FTTC broadband for 0.8mile line: now@9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.