| Message ID | 20181227181319.31095-15-paul@crapouillou.net |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Ingenic TCU patchset v9 | expand |
On Thu, Dec 27, 2018 at 07:13:06PM +0100, Paul Cercueil wrote: > The previous algorithm hardcoded details about how the TCU clocks work. > The new algorithm will use clk_round_rate to find the perfect clock rate > for the PWM channel. > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > > Notes: > v9: New patch > > drivers/pwm/pwm-jz4740.c | 26 +++++++++++++++----------- > 1 file changed, 15 insertions(+), 11 deletions(-) > > diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c > index c6136bd4434b..dd80a2cf6528 100644 > --- a/drivers/pwm/pwm-jz4740.c > +++ b/drivers/pwm/pwm-jz4740.c > @@ -110,23 +110,27 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip); > struct clk *clk = jz4740->clks[pwm->hwpwm], > *parent_clk = clk_get_parent(clk); > - unsigned long rate, period, duty; > + unsigned long rate, new_rate, period, duty; > unsigned long long tmp; > - unsigned int prescaler = 0; > > rate = clk_get_rate(parent_clk); > - tmp = (unsigned long long)rate * state->period; > - do_div(tmp, 1000000000); > - period = tmp; > > - while (period > 0xffff && prescaler < 6) { > - period >>= 2; > - rate >>= 2; > - ++prescaler; > + for (;;) { > + tmp = (unsigned long long)rate * state->period; > + do_div(tmp, 1000000000); NSEC_PER_SEC? > + > + if (tmp <= 0xffff) > + break; > + > + new_rate = clk_round_rate(clk, rate - 1); > + > + if (new_rate < rate) > + rate = new_rate; > + else > + return -EINVAL; You are assuming stuff here about the parent clk which isn't guaranteed (AFAICT) by the clk framework: If you call clk_round_rate(clk, rate - 1) this might well return rate even if the clock could run slower than rate. Wouldn't it make sense to start iterating with rate = 0xffff * 1e9 / period? Otherwise you get bad configurations if rate is considerable slower than necessary. Best regards Uwe
Hi, On Sat, Jan 5, 2019 at 4:57 PM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Thu, Dec 27, 2018 at 07:13:06PM +0100, Paul Cercueil wrote: >> The previous algorithm hardcoded details about how the TCU clocks >> work. >> The new algorithm will use clk_round_rate to find the perfect clock >> rate >> for the PWM channel. >> >> Signed-off-by: Paul Cercueil <paul@crapouillou.net> >> --- >> >> Notes: >> v9: New patch >> >> drivers/pwm/pwm-jz4740.c | 26 +++++++++++++++----------- >> 1 file changed, 15 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c >> index c6136bd4434b..dd80a2cf6528 100644 >> --- a/drivers/pwm/pwm-jz4740.c >> +++ b/drivers/pwm/pwm-jz4740.c >> @@ -110,23 +110,27 @@ static int jz4740_pwm_apply(struct pwm_chip >> *chip, struct pwm_device *pwm, >> struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip); >> struct clk *clk = jz4740->clks[pwm->hwpwm], >> *parent_clk = clk_get_parent(clk); >> - unsigned long rate, period, duty; >> + unsigned long rate, new_rate, period, duty; >> unsigned long long tmp; >> - unsigned int prescaler = 0; >> >> rate = clk_get_rate(parent_clk); >> - tmp = (unsigned long long)rate * state->period; >> - do_div(tmp, 1000000000); >> - period = tmp; >> >> - while (period > 0xffff && prescaler < 6) { >> - period >>= 2; >> - rate >>= 2; >> - ++prescaler; >> + for (;;) { >> + tmp = (unsigned long long)rate * state->period; >> + do_div(tmp, 1000000000); > > NSEC_PER_SEC? Ok, didn't know about it. >> + >> + if (tmp <= 0xffff) >> + break; >> + >> + new_rate = clk_round_rate(clk, rate - 1); >> + >> + if (new_rate < rate) >> + rate = new_rate; >> + else >> + return -EINVAL; > > You are assuming stuff here about the parent clk which isn't > guaranteed > (AFAICT) by the clk framework: If you call clk_round_rate(clk, rate - > 1) > this might well return rate even if the clock could run slower than > rate. It may not be guaranteed by the clock framework itself, but it is guaranteed to behave like that on this family of SoCs. > Wouldn't it make sense to start iterating with rate = 0xffff * 1e9 / > period? Otherwise you get bad configurations if rate is considerable > slower than necessary. The algorithm will start with 'rate' being the parent clock's rate, which will always be the highest rate that the child clock will support. > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König > | > Industrial Linux Solutions | > http://www.pengutronix.de/ |
Hello Paul, On Sat, Jan 05, 2019 at 06:05:38PM -0300, Paul Cercueil wrote: > On Sat, Jan 5, 2019 at 4:57 PM, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > You are assuming stuff here about the parent clk which isn't guaranteed > > (AFAICT) by the clk framework: If you call clk_round_rate(clk, rate - 1) > > this might well return rate even if the clock could run slower than > > rate. > > It may not be guaranteed by the clock framework itself, but it is guaranteed > to behave like that on this family of SoCs. You shouldn't rely on that. Experience shows that people will start copying code to machines where this is not guaranteed. Even if they don't copy and only learn from reading this is bad. Also how do you guarantee that this won't change in the future making the pwm code break without noticing? If you use an API better don't assume more things given than are guaranteed by the API. Having said that I would consider it sensible to introduce something like clk_roundup_rate() and clk_rounddown_rate() which would allow calculations like that. > > Wouldn't it make sense to start iterating with rate = 0xffff * 1e9 / > > period? Otherwise you get bad configurations if rate is considerable > > slower than necessary. > > The algorithm will start with 'rate' being the parent clock's rate, which > will always be the highest rate that the child clock will support. Ah right, I missed that bit. Best regards Uwe
Adding Stephen to the discussion. Adding Stephen to the discussion. On Sat, Jan 5, 2019 at 6:27 PM, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > Hello Paul, > > On Sat, Jan 05, 2019 at 06:05:38PM -0300, Paul Cercueil wrote: >> On Sat, Jan 5, 2019 at 4:57 PM, Uwe Kleine-König >> <u.kleine-koenig@pengutronix.de> wrote: >> > You are assuming stuff here about the parent clk which isn't >> guaranteed >> > (AFAICT) by the clk framework: If you call clk_round_rate(clk, >> rate - 1) >> > this might well return rate even if the clock could run slower >> than >> > rate. >> >> It may not be guaranteed by the clock framework itself, but it is >> guaranteed >> to behave like that on this family of SoCs. > > You shouldn't rely on that. Experience shows that people will start > copying code to machines where this is not guaranteed. Even if they > don't copy and only learn from reading this is bad. Also how do you > guarantee that this won't change in the future making the pwm code > break > without noticing? > > If you use an API better don't assume more things given than are > guaranteed by the API. > > Having said that I would consider it sensible to introduce something > like clk_roundup_rate() and clk_rounddown_rate() which would allow > calculations like that. @Stephen: Some context: my algorithm makes use of clk_round_rate(clk, rate - 1) to get the next (smaller) clock rate that a clock support. Is it something safe to assume? If not is there a better way? >> > Wouldn't it make sense to start iterating with rate = 0xffff * >> 1e9 / >> > period? Otherwise you get bad configurations if rate is >> considerable >> > slower than necessary. >> >> The algorithm will start with 'rate' being the parent clock's rate, >> which >> will always be the highest rate that the child clock will support. > > Ah right, I missed that bit. Thanks, -Paul
Hi, Le jeu. 10 janv. 2019 à 11:04, Paul Cercueil <paul@crapouillou.net> a écrit : > Adding Stephen to the discussion. > Adding Stephen to the discussion. > > On Sat, Jan 5, 2019 at 6:27 PM, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: >> Hello Paul, >> >> On Sat, Jan 05, 2019 at 06:05:38PM -0300, Paul Cercueil wrote: >>> On Sat, Jan 5, 2019 at 4:57 PM, Uwe Kleine-König >>> <u.kleine-koenig@pengutronix.de> wrote: >>> > You are assuming stuff here about the parent clk which isn't >>> guaranteed >>> > (AFAICT) by the clk framework: If you call clk_round_rate(clk, >>> rate - 1) >>> > this might well return rate even if the clock could run slower >>> than >>> > rate. >>> >>> It may not be guaranteed by the clock framework itself, but it is >>> guaranteed >>> to behave like that on this family of SoCs. >> >> You shouldn't rely on that. Experience shows that people will start >> copying code to machines where this is not guaranteed. Even if they >> don't copy and only learn from reading this is bad. Also how do you >> guarantee that this won't change in the future making the pwm code >> break >> without noticing? >> >> If you use an API better don't assume more things given than are >> guaranteed by the API. >> >> Having said that I would consider it sensible to introduce something >> like clk_roundup_rate() and clk_rounddown_rate() which would allow >> calculations like that. > > @Stephen: > Some context: my algorithm makes use of clk_round_rate(clk, rate - 1) > to get the > next (smaller) clock rate that a clock support. > > Is it something safe to assume? If not is there a better way? Bump. What should I do here? >>> > Wouldn't it make sense to start iterating with rate = 0xffff * >>> 1e9 / >>> > period? Otherwise you get bad configurations if rate is >>> considerable >>> > slower than necessary. >>> >>> The algorithm will start with 'rate' being the parent clock's >>> rate, which >>> will always be the highest rate that the child clock will support. >> >> Ah right, I missed that bit. Thanks, -Paul
Quoting Paul Cercueil (2019-02-22 17:17:58) > Bump. > > What should I do here? > I thought I replied to the list but maybe it got rejected because my MUA is currently failing hard at sending 8-bit mails without using quoted printable. Let me remove all non-ascii characters from this mail! If someone has the mail please rebounce it to the list. Otherwise, I think I basically said this is OK because clk_round_rate() semantics are specifically vague here to allow the implementation to decide how rates are rounded (up, down, closest, etc). As long as the whole rate space is searched with a +1 or a -1 style of search it should be work. We're not going to add round_up() or round_down() APIs as far as I'm concerned, and you can look on the list to see previous proposals on that topic to get some background on why they aren't liked.
diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c index c6136bd4434b..dd80a2cf6528 100644 --- a/drivers/pwm/pwm-jz4740.c +++ b/drivers/pwm/pwm-jz4740.c @@ -110,23 +110,27 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip); struct clk *clk = jz4740->clks[pwm->hwpwm], *parent_clk = clk_get_parent(clk); - unsigned long rate, period, duty; + unsigned long rate, new_rate, period, duty; unsigned long long tmp; - unsigned int prescaler = 0; rate = clk_get_rate(parent_clk); - tmp = (unsigned long long)rate * state->period; - do_div(tmp, 1000000000); - period = tmp; - while (period > 0xffff && prescaler < 6) { - period >>= 2; - rate >>= 2; - ++prescaler; + for (;;) { + tmp = (unsigned long long)rate * state->period; + do_div(tmp, 1000000000); + + if (tmp <= 0xffff) + break; + + new_rate = clk_round_rate(clk, rate - 1); + + if (new_rate < rate) + rate = new_rate; + else + return -EINVAL; } - if (prescaler == 6) - return -EINVAL; + period = tmp; tmp = (unsigned long long)period * state->duty_cycle; do_div(tmp, state->period);
The previous algorithm hardcoded details about how the TCU clocks work. The new algorithm will use clk_round_rate to find the perfect clock rate for the PWM channel. Signed-off-by: Paul Cercueil <paul@crapouillou.net> --- Notes: v9: New patch drivers/pwm/pwm-jz4740.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-)