Re: [PATCH 1/3] clk: ti: add 'ti,round-rate' flag
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 at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] clk: ti: add 'ti,round-rate' flag
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] clk: ti: add 'ti,round-rate' flag
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 signature.asc Description: OpenPGP digital signature
Re: [PATCH 1/3] clk: ti: add 'ti,round-rate' flag
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] clk: ti: add 'ti,round-rate' flag
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 signature.asc Description: OpenPGP digital signature
Re: [PATCH 1/3] clk: ti: add 'ti,round-rate' flag
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 -- To unsubscribe from this list: send the line "unsubscribe linux-o
Re: [PATCH 1/3] clk: ti: add 'ti,round-rate' flag
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. We can always see how it goes and fix it up afterwards when everything explodes. Regards, Mike > > Tomi > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] clk: ti: add 'ti,round-rate' flag
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. Tomi signature.asc Description: OpenPGP digital signature
Re: [PATCH 1/3] clk: ti: add 'ti,round-rate' flag
On Thu, May 15, 2014 at 1:08 AM, 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? > > For those users of the omapconf tool I enjoy doing something like the > following: > > > omapconf dump prcm > old > > > omapconf dump prcm > new > > diff -u old new > > This should reveal any deltas, assuming the board boots and doesn't let > magic smoke out. > it would probably help for omap4,5,dra7, but AM335x, OMAP3 are not supported on omapconf - yet. just a side note. omapconf: https://github.com/omapconf/omapconf Regards, Nishanth Menon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] clk: ti: add 'ti,round-rate' flag
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? For those users of the omapconf tool I enjoy doing something like the following: omapconf dump prcm > old omapconf dump prcm > new diff -u old new This should reveal any deltas, assuming the board boots and doesn't let magic smoke out. Regards, Mike > > 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 > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] clk: ti: add 'ti,round-rate' flag
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. 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 signature.asc Description: OpenPGP digital signature
Re: [PATCH 1/3] clk: ti: add 'ti,round-rate' flag
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,
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.
-Tero
Signed-off-by: Tomi Valkeinen
---
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(+)
diff --git a/Documentation/devicetree/bindings/clock/ti/dpll.txt
b/Documentation/devicetree/bindings/clock/ti/dpll.txt
index 30bfdb7c9f18..7912d876e858 100644
--- a/Documentation/devicetree/bindings/clock/ti/dpll.txt
+++ b/Documentation/devicetree/bindings/clock/ti/dpll.txt
@@ -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 {
diff --git a/drivers/clk/ti/dpll.c b/drivers/clk/ti/dpll.c
index 7e498a44f97d..c5858c46b58c 100644
--- a/drivers/clk/ti/dpll.c
+++ b/drivers/clk/ti/dpll.c
@@ -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;
diff --git a/include/linux/clk/ti.h b/include/linux/clk/ti.h
index 4a21a872dbbd..79e8a7876457 100644
--- a/include/linux/clk/ti.h
+++ b/include/linux/clk/ti.h
@@ -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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] 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
---
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(+)
diff --git a/Documentation/devicetree/bindings/clock/ti/dpll.txt
b/Documentation/devicetree/bindings/clock/ti/dpll.txt
index 30bfdb7c9f18..7912d876e858 100644
--- a/Documentation/devicetree/bindings/clock/ti/dpll.txt
+++ b/Documentation/devicetree/bindings/clock/ti/dpll.txt
@@ -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 {
diff --git a/drivers/clk/ti/dpll.c b/drivers/clk/ti/dpll.c
index 7e498a44f97d..c5858c46b58c 100644
--- a/drivers/clk/ti/dpll.c
+++ b/drivers/clk/ti/dpll.c
@@ -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;
diff --git a/include/linux/clk/ti.h b/include/linux/clk/ti.h
index 4a21a872dbbd..79e8a7876457 100644
--- a/include/linux/clk/ti.h
+++ b/include/linux/clk/ti.h
@@ -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_STOP0x1
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
