| Message ID | 20190809123031.24219-5-paul@crapouillou.net |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | pwm: jz4740: Driver update | expand |
On Fri, Aug 09, 2019 at 02:30:28PM +0200, 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> > Tested-by: Mathieu Malaterre <malat@debian.org> > Tested-by: Artur Rojek <contact@artur-rojek.eu> > --- > drivers/pwm/pwm-jz4740.c | 60 +++++++++++++++++++++++++++++----------- > 1 file changed, 44 insertions(+), 16 deletions(-) > > diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c > index 6ec8794d3b99..f20dc2e19240 100644 > --- a/drivers/pwm/pwm-jz4740.c > +++ b/drivers/pwm/pwm-jz4740.c > @@ -110,24 +110,56 @@ 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 = pwm_get_chip_data(pwm), > *parent_clk = clk_get_parent(clk); > - unsigned long rate, period, duty; > + unsigned long rate, parent_rate, period, duty; > unsigned long long tmp; > - unsigned int prescaler = 0; > + int ret; > > - rate = clk_get_rate(parent_clk); > - tmp = (unsigned long long)rate * state->period; > - do_div(tmp, 1000000000); > - period = tmp; > + parent_rate = clk_get_rate(parent_clk); > + > + jz4740_pwm_disable(chip, pwm); > > - while (period > 0xffff && prescaler < 6) { > - period >>= 2; > - rate >>= 2; > - ++prescaler; > + /* Reset the clock to the maximum rate, and we'll reduce it if needed */ > + ret = clk_set_max_rate(clk, parent_rate); What is the purpose of this call? IIUC this limits the allowed range of rates for clk. I assume the idea is to prevent other consumers to change the rate in a way that makes it unsuitable for this pwm. But this only makes sense if you had a notifier for clk changes, doesn't it? I'm confused. I think this doesn't match the commit log, you didn't even introduced a call to clk_round_rate(). > + if (ret) { > + dev_err(chip->dev, "Unable to set max rate: %d\n", ret); > + return ret; > } > > - if (prescaler == 6) > - return -EINVAL; > + ret = clk_set_rate(clk, parent_rate); > + if (ret) { > + dev_err(chip->dev, "Unable to reset to parent rate (%lu Hz)", > + parent_rate); > + return ret; > + } > + > + /* > + * Limit the clock to a maximum rate that still gives us a period value > + * which fits in 16 bits. > + */ > + tmp = 0xffffull * NSEC_PER_SEC; > + do_div(tmp, state->period); > > + ret = clk_set_max_rate(clk, tmp); And now you change the maximal rate again? > + if (ret) { > + dev_err(chip->dev, "Unable to set max rate: %d\n", ret); > + return ret; > + } > + > + /* > + * Read back the clock rate, as it may have been modified by > + * clk_set_max_rate() > + */ > + rate = clk_get_rate(clk); > + > + if (rate != parent_rate) > + dev_dbg(chip->dev, "PWM clock updated to %lu Hz\n", rate); > + > + /* Calculate period value */ > + tmp = (unsigned long long)rate * state->period; > + do_div(tmp, NSEC_PER_SEC); > + period = (unsigned long)tmp; > + > + /* Calculate duty value */ > tmp = (unsigned long long)period * state->duty_cycle; > do_div(tmp, state->period); > duty = period - tmp; > @@ -135,14 +167,10 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > if (duty >= period) > duty = period - 1; > > - jz4740_pwm_disable(chip, pwm); > - > /* Set abrupt shutdown */ > regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm), > TCU_TCSR_PWM_SD, TCU_TCSR_PWM_SD); > > - clk_set_rate(clk, rate); > - It's not obvious to me why removing these two lines belong in the current patch. Best regards Uwe
Le ven. 9 août 2019 à 19:05, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= <u.kleine-koenig@pengutronix.de> a écrit : > On Fri, Aug 09, 2019 at 02:30:28PM +0200, 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> >> Tested-by: Mathieu Malaterre <malat@debian.org> >> Tested-by: Artur Rojek <contact@artur-rojek.eu> >> --- >> drivers/pwm/pwm-jz4740.c | 60 >> +++++++++++++++++++++++++++++----------- >> 1 file changed, 44 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c >> index 6ec8794d3b99..f20dc2e19240 100644 >> --- a/drivers/pwm/pwm-jz4740.c >> +++ b/drivers/pwm/pwm-jz4740.c >> @@ -110,24 +110,56 @@ 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 = pwm_get_chip_data(pwm), >> *parent_clk = clk_get_parent(clk); >> - unsigned long rate, period, duty; >> + unsigned long rate, parent_rate, period, duty; >> unsigned long long tmp; >> - unsigned int prescaler = 0; >> + int ret; >> >> - rate = clk_get_rate(parent_clk); >> - tmp = (unsigned long long)rate * state->period; >> - do_div(tmp, 1000000000); >> - period = tmp; >> + parent_rate = clk_get_rate(parent_clk); >> + >> + jz4740_pwm_disable(chip, pwm); >> >> - while (period > 0xffff && prescaler < 6) { >> - period >>= 2; >> - rate >>= 2; >> - ++prescaler; >> + /* Reset the clock to the maximum rate, and we'll reduce it if >> needed */ >> + ret = clk_set_max_rate(clk, parent_rate); > > What is the purpose of this call? IIUC this limits the allowed range > of > rates for clk. I assume the idea is to prevent other consumers to > change > the rate in a way that makes it unsuitable for this pwm. But this only > makes sense if you had a notifier for clk changes, doesn't it? I'm > confused. Nothing like that. The second call to clk_set_max_rate() might have set a maximum clock rate that's lower than the parent's rate, and we want to undo that. > I think this doesn't match the commit log, you didn't even introduced > a > call to clk_round_rate(). Right, I'll edit the commit message. >> + if (ret) { >> + dev_err(chip->dev, "Unable to set max rate: %d\n", ret); >> + return ret; >> } >> >> - if (prescaler == 6) >> - return -EINVAL; >> + ret = clk_set_rate(clk, parent_rate); >> + if (ret) { >> + dev_err(chip->dev, "Unable to reset to parent rate (%lu Hz)", >> + parent_rate); >> + return ret; >> + } >> + >> + /* >> + * Limit the clock to a maximum rate that still gives us a period >> value >> + * which fits in 16 bits. >> + */ >> + tmp = 0xffffull * NSEC_PER_SEC; >> + do_div(tmp, state->period); >> >> + ret = clk_set_max_rate(clk, tmp); > > And now you change the maximal rate again? Basically, we start from the maximum clock rate we can get for that PWM - which is the rate of the parent clk - and from that compute the maximum clock rate that we can support that still gives us < 16-bits hardware values for the period and duty. We then pass that computed maximum clock rate to clk_set_max_rate(), which may or may not update the current PWM clock's rate to match the new limits. Finally we read back the PWM clock's rate and compute the period and duty from that. >> + if (ret) { >> + dev_err(chip->dev, "Unable to set max rate: %d\n", ret); >> + return ret; >> + } >> + >> + /* >> + * Read back the clock rate, as it may have been modified by >> + * clk_set_max_rate() >> + */ >> + rate = clk_get_rate(clk); >> + >> + if (rate != parent_rate) >> + dev_dbg(chip->dev, "PWM clock updated to %lu Hz\n", rate); >> + >> + /* Calculate period value */ >> + tmp = (unsigned long long)rate * state->period; >> + do_div(tmp, NSEC_PER_SEC); >> + period = (unsigned long)tmp; >> + >> + /* Calculate duty value */ >> tmp = (unsigned long long)period * state->duty_cycle; >> do_div(tmp, state->period); >> duty = period - tmp; >> @@ -135,14 +167,10 @@ static int jz4740_pwm_apply(struct pwm_chip >> *chip, struct pwm_device *pwm, >> if (duty >= period) >> duty = period - 1; >> >> - jz4740_pwm_disable(chip, pwm); >> - >> /* Set abrupt shutdown */ >> regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm), >> TCU_TCSR_PWM_SD, TCU_TCSR_PWM_SD); >> >> - clk_set_rate(clk, rate); >> - > > It's not obvious to me why removing these two lines belong in the > current patch. They're not removed, they're both moved up in the function. > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König > | > Industrial Linux Solutions | > http://www.pengutronix.de/ |
Hello Paul, On Fri, Aug 09, 2019 at 07:14:45PM +0200, Paul Cercueil wrote: > Le ven. 9 août 2019 à 19:05, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= > <u.kleine-koenig@pengutronix.de> a écrit : > > On Fri, Aug 09, 2019 at 02:30:28PM +0200, 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> > > > Tested-by: Mathieu Malaterre <malat@debian.org> > > > Tested-by: Artur Rojek <contact@artur-rojek.eu> > > > --- > > > drivers/pwm/pwm-jz4740.c | 60 > > > +++++++++++++++++++++++++++++----------- > > > 1 file changed, 44 insertions(+), 16 deletions(-) > > > > > > diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c > > > index 6ec8794d3b99..f20dc2e19240 100644 > > > --- a/drivers/pwm/pwm-jz4740.c > > > +++ b/drivers/pwm/pwm-jz4740.c > > > @@ -110,24 +110,56 @@ 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 = pwm_get_chip_data(pwm), > > > *parent_clk = clk_get_parent(clk); > > > - unsigned long rate, period, duty; > > > + unsigned long rate, parent_rate, period, duty; > > > unsigned long long tmp; > > > - unsigned int prescaler = 0; > > > + int ret; > > > > > > - rate = clk_get_rate(parent_clk); > > > - tmp = (unsigned long long)rate * state->period; > > > - do_div(tmp, 1000000000); > > > - period = tmp; > > > + parent_rate = clk_get_rate(parent_clk); > > > + > > > + jz4740_pwm_disable(chip, pwm); > > > > > > - while (period > 0xffff && prescaler < 6) { > > > - period >>= 2; > > > - rate >>= 2; > > > - ++prescaler; > > > + /* Reset the clock to the maximum rate, and we'll reduce it if needed */ > > > + ret = clk_set_max_rate(clk, parent_rate); > > > > What is the purpose of this call? IIUC this limits the allowed range of > > rates for clk. I assume the idea is to prevent other consumers to change > > the rate in a way that makes it unsuitable for this pwm. But this only > > makes sense if you had a notifier for clk changes, doesn't it? I'm > > confused. > > Nothing like that. The second call to clk_set_max_rate() might have set > a maximum clock rate that's lower than the parent's rate, and we want to > undo that. I still don't get the purpose of this call. Why do you limit the clock rate at all? > > I think this doesn't match the commit log, you didn't even introduced a > > call to clk_round_rate(). > > Right, I'll edit the commit message. > > > > > + if (ret) { > > > + dev_err(chip->dev, "Unable to set max rate: %d\n", ret); > > > + return ret; > > > } > > > > > > - if (prescaler == 6) > > > - return -EINVAL; > > > + ret = clk_set_rate(clk, parent_rate); > > > + if (ret) { > > > + dev_err(chip->dev, "Unable to reset to parent rate (%lu Hz)", > > > + parent_rate); > > > + return ret; > > > + } > > > + > > > + /* > > > + * Limit the clock to a maximum rate that still gives us a period value > > > + * which fits in 16 bits. > > > + */ > > > + tmp = 0xffffull * NSEC_PER_SEC; > > > + do_div(tmp, state->period); > > > > > > + ret = clk_set_max_rate(clk, tmp); > > > > And now you change the maximal rate again? > > Basically, we start from the maximum clock rate we can get for that PWM > - which is the rate of the parent clk - and from that compute the maximum > clock rate that we can support that still gives us < 16-bits hardware > values for the period and duty. > > We then pass that computed maximum clock rate to clk_set_max_rate(), which > may or may not update the current PWM clock's rate to match the new limits. > Finally we read back the PWM clock's rate and compute the period and duty > from that. If you change the clk rate, is this externally visible on the PWM output? Does this affect other PWM instances? > > > + if (ret) { > > > + dev_err(chip->dev, "Unable to set max rate: %d\n", ret); > > > + return ret; > > > + } > > > + > > > + /* > > > + * Read back the clock rate, as it may have been modified by > > > + * clk_set_max_rate() > > > + */ > > > + rate = clk_get_rate(clk); > > > + > > > + if (rate != parent_rate) > > > + dev_dbg(chip->dev, "PWM clock updated to %lu Hz\n", rate); > > > + > > > + /* Calculate period value */ > > > + tmp = (unsigned long long)rate * state->period; > > > + do_div(tmp, NSEC_PER_SEC); > > > + period = (unsigned long)tmp; > > > + > > > + /* Calculate duty value */ > > > tmp = (unsigned long long)period * state->duty_cycle; > > > do_div(tmp, state->period); > > > duty = period - tmp; > > > @@ -135,14 +167,10 @@ static int jz4740_pwm_apply(struct pwm_chip > > > *chip, struct pwm_device *pwm, > > > if (duty >= period) > > > duty = period - 1; > > > > > > - jz4740_pwm_disable(chip, pwm); > > > - > > > /* Set abrupt shutdown */ > > > regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm), > > > TCU_TCSR_PWM_SD, TCU_TCSR_PWM_SD); > > > > > > - clk_set_rate(clk, rate); > > > - > > > > It's not obvious to me why removing these two lines belong in the > > current patch. > > They're not removed, they're both moved up in the function. OK, will look closer in the next iteration. Best regards Uwe
Le lun. 12 août 2019 à 8:15, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= <u.kleine-koenig@pengutronix.de> a écrit : > Hello Paul, > > On Fri, Aug 09, 2019 at 07:14:45PM +0200, Paul Cercueil wrote: >> Le ven. 9 août 2019 à 19:05, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= >> <u.kleine-koenig@pengutronix.de> a écrit : >> > On Fri, Aug 09, 2019 at 02:30:28PM +0200, 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> >> > > Tested-by: Mathieu Malaterre <malat@debian.org> >> > > Tested-by: Artur Rojek <contact@artur-rojek.eu> >> > > --- >> > > drivers/pwm/pwm-jz4740.c | 60 >> > > +++++++++++++++++++++++++++++----------- >> > > 1 file changed, 44 insertions(+), 16 deletions(-) >> > > >> > > diff --git a/drivers/pwm/pwm-jz4740.c >> b/drivers/pwm/pwm-jz4740.c >> > > index 6ec8794d3b99..f20dc2e19240 100644 >> > > --- a/drivers/pwm/pwm-jz4740.c >> > > +++ b/drivers/pwm/pwm-jz4740.c >> > > @@ -110,24 +110,56 @@ 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 = pwm_get_chip_data(pwm), >> > > *parent_clk = clk_get_parent(clk); >> > > - unsigned long rate, period, duty; >> > > + unsigned long rate, parent_rate, period, duty; >> > > unsigned long long tmp; >> > > - unsigned int prescaler = 0; >> > > + int ret; >> > > >> > > - rate = clk_get_rate(parent_clk); >> > > - tmp = (unsigned long long)rate * state->period; >> > > - do_div(tmp, 1000000000); >> > > - period = tmp; >> > > + parent_rate = clk_get_rate(parent_clk); >> > > + >> > > + jz4740_pwm_disable(chip, pwm); >> > > >> > > - while (period > 0xffff && prescaler < 6) { >> > > - period >>= 2; >> > > - rate >>= 2; >> > > - ++prescaler; >> > > + /* Reset the clock to the maximum rate, and we'll reduce it >> if needed */ >> > > + ret = clk_set_max_rate(clk, parent_rate); >> > >> > What is the purpose of this call? IIUC this limits the allowed >> range of >> > rates for clk. I assume the idea is to prevent other consumers to >> change >> > the rate in a way that makes it unsuitable for this pwm. But this >> only >> > makes sense if you had a notifier for clk changes, doesn't it? I'm >> > confused. >> >> Nothing like that. The second call to clk_set_max_rate() might have >> set >> a maximum clock rate that's lower than the parent's rate, and we >> want to >> undo that. > > I still don't get the purpose of this call. Why do you limit the clock > rate at all? As it says below, we "limit the clock to a maximum rate that still gives us a period value which fits in 16 bits". So that the computed hardware values won't overflow. E.g. if at a rate of 12 MHz your computed hardware value for the period is 0xf000, then at a rate of 24 MHz it won't fit in 16 bits. So the clock rate must be reduced to the highest possible that will still give you a < 16-bit value. We always want the highest possible clock rate that works, for the sake of precision. >> > I think this doesn't match the commit log, you didn't even >> introduced a >> > call to clk_round_rate(). >> >> Right, I'll edit the commit message. >> >> >> > > + if (ret) { >> > > + dev_err(chip->dev, "Unable to set max rate: %d\n", ret); >> > > + return ret; >> > > } >> > > >> > > - if (prescaler == 6) >> > > - return -EINVAL; >> > > + ret = clk_set_rate(clk, parent_rate); >> > > + if (ret) { >> > > + dev_err(chip->dev, "Unable to reset to parent rate (%lu >> Hz)", >> > > + parent_rate); >> > > + return ret; >> > > + } >> > > + >> > > + /* >> > > + * Limit the clock to a maximum rate that still gives us a >> period value >> > > + * which fits in 16 bits. >> > > + */ >> > > + tmp = 0xffffull * NSEC_PER_SEC; >> > > + do_div(tmp, state->period); >> > > >> > > + ret = clk_set_max_rate(clk, tmp); >> > >> > And now you change the maximal rate again? >> >> Basically, we start from the maximum clock rate we can get for that >> PWM >> - which is the rate of the parent clk - and from that compute the >> maximum >> clock rate that we can support that still gives us < 16-bits >> hardware >> values for the period and duty. >> >> We then pass that computed maximum clock rate to >> clk_set_max_rate(), which >> may or may not update the current PWM clock's rate to match the new >> limits. >> Finally we read back the PWM clock's rate and compute the period >> and duty >> from that. > > If you change the clk rate, is this externally visible on the PWM > output? Does this affect other PWM instances? The clock rate doesn't change the PWM output because the hardware values for the period and duty are adapted accordingly to reflect the change. >> > > + if (ret) { >> > > + dev_err(chip->dev, "Unable to set max rate: %d\n", ret); >> > > + return ret; >> > > + } >> > > + >> > > + /* >> > > + * Read back the clock rate, as it may have been modified by >> > > + * clk_set_max_rate() >> > > + */ >> > > + rate = clk_get_rate(clk); >> > > + >> > > + if (rate != parent_rate) >> > > + dev_dbg(chip->dev, "PWM clock updated to %lu Hz\n", rate); >> > > + >> > > + /* Calculate period value */ >> > > + tmp = (unsigned long long)rate * state->period; >> > > + do_div(tmp, NSEC_PER_SEC); >> > > + period = (unsigned long)tmp; >> > > + >> > > + /* Calculate duty value */ >> > > tmp = (unsigned long long)period * state->duty_cycle; >> > > do_div(tmp, state->period); >> > > duty = period - tmp; >> > > @@ -135,14 +167,10 @@ static int jz4740_pwm_apply(struct >> pwm_chip >> > > *chip, struct pwm_device *pwm, >> > > if (duty >= period) >> > > duty = period - 1; >> > > >> > > - jz4740_pwm_disable(chip, pwm); >> > > - >> > > /* Set abrupt shutdown */ >> > > regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm), >> > > TCU_TCSR_PWM_SD, TCU_TCSR_PWM_SD); >> > > >> > > - clk_set_rate(clk, rate); >> > > - >> > >> > It's not obvious to me why removing these two lines belong in the >> > current patch. >> >> They're not removed, they're both moved up in the function. > > OK, will look closer in the next iteration. > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König > | > Industrial Linux Solutions | > http://www.pengutronix.de/ |
Hello Paul, On Mon, Aug 12, 2019 at 10:43:10PM +0200, Paul Cercueil wrote: > Le lun. 12 août 2019 à 8:15, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= > <u.kleine-koenig@pengutronix.de> a écrit : > > On Fri, Aug 09, 2019 at 07:14:45PM +0200, Paul Cercueil wrote: > > > Le ven. 9 août 2019 à 19:05, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= > > > <u.kleine-koenig@pengutronix.de> a écrit : > > > > On Fri, Aug 09, 2019 at 02:30:28PM +0200, Paul Cercueil wrote: > > > > > [...] > > > > > + /* Reset the clock to the maximum rate, and we'll reduce it if needed */ > > > > > + ret = clk_set_max_rate(clk, parent_rate); > > > > > > > > What is the purpose of this call? IIUC this limits the allowed range of > > > > rates for clk. I assume the idea is to prevent other consumers to change > > > > the rate in a way that makes it unsuitable for this pwm. But this only > > > > makes sense if you had a notifier for clk changes, doesn't it? I'm > > > > confused. > > > > > > Nothing like that. The second call to clk_set_max_rate() might have set > > > a maximum clock rate that's lower than the parent's rate, and we want to > > > undo that. > > > > I still don't get the purpose of this call. Why do you limit the clock > > rate at all? > > As it says below, we "limit the clock to a maximum rate that still gives > us a period value which fits in 16 bits". So that the computed hardware > values won't overflow. But why not just using clk_set_rate? You want to have the clock running at a certain rate, not any rate below that certain rate, don't you? > E.g. if at a rate of 12 MHz your computed hardware value for the period > is 0xf000, then at a rate of 24 MHz it won't fit in 16 bits. So the clock > rate must be reduced to the highest possible that will still give you a > < 16-bit value. > > We always want the highest possible clock rate that works, for the sake of > precision. This is dubious; but ok to keep the driver simple. (Consider a PWM that can run at i MHz for i in [1, .. 30]. If a period of 120 ns and a duty cycle of 40 ns is requested you can get an exact match with 25 MHz, but not with 30 MHz.) > > > Basically, we start from the maximum clock rate we can get for that PWM > > > - which is the rate of the parent clk - and from that compute the maximum > > > clock rate that we can support that still gives us < 16-bits hardware > > > values for the period and duty. > > > > > > We then pass that computed maximum clock rate to clk_set_max_rate(), which > > > may or may not update the current PWM clock's rate to match the new limits. > > > Finally we read back the PWM clock's rate and compute the period and duty > > > from that. > > > > If you change the clk rate, is this externally visible on the PWM > > output? Does this affect other PWM instances? > > The clock rate doesn't change the PWM output because the hardware values for > the period and duty are adapted accordingly to reflect the change. It doesn't change it in the end. But in the (short) time frame between the call to change the clock and the update of the PWM registers there is a glitch, right? You didn't answer to the question about other PWM instances. Does that mean others are not affected? Best regards Uwe PS: It would be great if you could fix your mailer to not damage the quoted mail. Also it doesn't seem to understand how my name is encoded in the From line. I fixed up the quotes in my reply.
[Re-send my message in plain text, as it was bounced by the lists - sorry about that] Le lun. 12 août 2019 à 23:48, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= <u.kleine-koenig@pengutronix.de> a écrit : > Hello Paul, > > On Mon, Aug 12, 2019 at 10:43:10PM +0200, Paul Cercueil wrote: >> Le lun. 12 août 2019 à 8:15, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= >> <u.kleine-koenig@pengutronix.de> a écrit : >> > On Fri, Aug 09, 2019 at 07:14:45PM +0200, Paul Cercueil wrote: >> > > Le ven. 9 août 2019 à 19:05, Uwe >> =?iso-8859-1?q?Kleine-K=F6nig?= >> > > <u.kleine-koenig@pengutronix.de> a écrit : >> > > > On Fri, Aug 09, 2019 at 02:30:28PM +0200, Paul Cercueil >> wrote: >> > > > > [...] >> > > > > + /* Reset the clock to the maximum rate, and we'll >> reduce it if needed */ >> > > > > + ret = clk_set_max_rate(clk, parent_rate); >> > > > >> > > > What is the purpose of this call? IIUC this limits the >> allowed range of >> > > > rates for clk. I assume the idea is to prevent other >> consumers to change >> > > > the rate in a way that makes it unsuitable for this pwm. But >> this only >> > > > makes sense if you had a notifier for clk changes, doesn't >> it? I'm >> > > > confused. >> > > >> > > Nothing like that. The second call to clk_set_max_rate() might >> have set >> > > a maximum clock rate that's lower than the parent's rate, and >> we want to >> > > undo that. >> > >> > I still don't get the purpose of this call. Why do you limit the >> clock >> > rate at all? >> >> As it says below, we "limit the clock to a maximum rate that still >> gives >> us a period value which fits in 16 bits". So that the computed >> hardware >> values won't overflow. > > But why not just using clk_set_rate? You want to have the clock > running > at a certain rate, not any rate below that certain rate, don't you? I'll let yourself answer yourself: https://patchwork.ozlabs.org/patch/1018969/ It's enough to run it below a certain rate, yes. The actual rate doesn't actually matter that much. > >> E.g. if at a rate of 12 MHz your computed hardware value for the >> period >> is 0xf000, then at a rate of 24 MHz it won't fit in 16 bits. So the >> clock >> rate must be reduced to the highest possible that will still give >> you a >> < 16-bit value. >> >> We always want the highest possible clock rate that works, for the >> sake of >> precision. > > This is dubious; but ok to keep the driver simple. (Consider a PWM > that > can run at i MHz for i in [1, .. 30]. If a period of 120 ns and a duty > cycle of 40 ns is requested you can get an exact match with 25 MHz, > but > not with 30 MHz.) The clock rate is actually (parent_rate >> (2 * x) ) for x = 0, 1, 2, ... So if your parent_rate is 30 MHz the next valid one is 7.5 MHz, and the next one is 1.875 MHz. It'd be very unlikely that you get a better match at a lower clock. >> > > Basically, we start from the maximum clock rate we can get for >> that PWM >> > > - which is the rate of the parent clk - and from that compute >> the maximum >> > > clock rate that we can support that still gives us < 16-bits >> hardware >> > > values for the period and duty. >> > > >> > > We then pass that computed maximum clock rate to >> clk_set_max_rate(), which >> > > may or may not update the current PWM clock's rate to match >> the new limits. >> > > Finally we read back the PWM clock's rate and compute the >> period and duty >> > > from that. >> > >> > If you change the clk rate, is this externally visible on the PWM >> > output? Does this affect other PWM instances? >> >> The clock rate doesn't change the PWM output because the hardware >> values for >> the period and duty are adapted accordingly to reflect the change. > > It doesn't change it in the end. But in the (short) time frame between > the call to change the clock and the update of the PWM registers there > is a glitch, right? The PWM is disabled, so the line is in inactive state, and will be in that state until the PWM is enabled again. No glitch to fear. > You didn't answer to the question about other PWM instances. Does that > mean others are not affected? Sorry. Yes, they are not affected - all PWM channels are independent. > Best regards > Uwe > > PS: It would be great if you could fix your mailer to not damage the > quoted mail. Also it doesn't seem to understand how my name is encoded > in the From line. I fixed up the quotes in my reply. I guess I'll submit a bug report to Geary then. > > -- > Pengutronix e.K. | Uwe Kleine-König > | > Industrial Linux Solutions | > http://www.pengutronix.de/ |
Hello Paul, [adding Stephen Boyd to Cc] On Tue, Aug 13, 2019 at 12:16:23AM +0200, Paul Cercueil wrote: > Le lun. 12 août 2019 à 23:48, Uwe Kleine-König a écrit : > > On Mon, Aug 12, 2019 at 10:43:10PM +0200, Paul Cercueil wrote: > > > Le lun. 12 août 2019 à 8:15, Uwe Kleine-König a écrit : > > > > On Fri, Aug 09, 2019 at 07:14:45PM +0200, Paul Cercueil wrote: > > > > > Le ven. 9 août 2019 à 19:05, Uwe Kleine-König a écrit : > > > > > > On Fri, Aug 09, 2019 at 02:30:28PM +0200, Paul Cercueil wrote: > > > > > > > [...] > > > > > > > + /* Reset the clock to the maximum rate, and we'll reduce it if needed */ > > > > > > > + ret = clk_set_max_rate(clk, parent_rate); > > > > > > > > > > > > What is the purpose of this call? IIUC this limits the allowed range of > > > > > > rates for clk. I assume the idea is to prevent other consumers to change > > > > > > the rate in a way that makes it unsuitable for this pwm. But this only > > > > > > makes sense if you had a notifier for clk changes, doesn't it? I'm > > > > > > confused. > > > > > > > > > > Nothing like that. The second call to clk_set_max_rate() might have set > > > > > a maximum clock rate that's lower than the parent's rate, and we want to > > > > > undo that. > > > > > > > > I still don't get the purpose of this call. Why do you limit the clock > > > > rate at all? > > > > > > As it says below, we "limit the clock to a maximum rate that still gives > > > us a period value which fits in 16 bits". So that the computed hardware > > > values won't overflow. > > > > But why not just using clk_set_rate? You want to have the clock running > > at a certain rate, not any rate below that certain rate, don't you? > > I'll let yourself answer yourself: > https://patchwork.ozlabs.org/patch/1018969/ In that thread I claimed that you used clk_round_rate wrongly, not that you should use clk_set_max_rate(). (The claim was somewhat weakend by Stephen, but still I think that clk_round_rate is the right approach.) The upside of clk_round_rate is that it allows you to test for the capabilities of the clock without actually changing it before you found a setting you consider to be good. > It's enough to run it below a certain rate, yes. The actual rate doesn't > actually matter that much. 1 Hz would be fine? I doubt it. > > > E.g. if at a rate of 12 MHz your computed hardware value for the period > > > is 0xf000, then at a rate of 24 MHz it won't fit in 16 bits. So the clock > > > rate must be reduced to the highest possible that will still give you a > > > < 16-bit value. > > > > > > We always want the highest possible clock rate that works, for the sake of > > > precision. > > > > This is dubious; but ok to keep the driver simple. (Consider a PWM that > > can run at i MHz for i in [1, .. 30]. If a period of 120 ns and a duty > > cycle of 40 ns is requested you can get an exact match with 25 MHz, but > > not with 30 MHz.) > > The clock rate is actually (parent_rate >> (2 * x) ) > for x = 0, 1, 2, ... > > So if your parent_rate is 30 MHz the next valid one is 7.5 MHz, and the > next one is 1.875 MHz. It'd be very unlikely that you get a better match at > a lower clock. If the smaller freqs are all dividers of the fastest that's fine. Please note in a code comment that you're assuming this. > > > > > Basically, we start from the maximum clock rate we can get for that PWM > > > > > - which is the rate of the parent clk - and from that compute the maximum > > > > > clock rate that we can support that still gives us < 16-bits hardware > > > > > values for the period and duty. > > > > > > > > > > We then pass that computed maximum clock rate to clk_set_max_rate(), which > > > > > may or may not update the current PWM clock's rate to match the new limits. > > > > > Finally we read back the PWM clock's rate and compute the period and duty > > > > > from that. > > > > > > > > If you change the clk rate, is this externally visible on the PWM > > > > output? Does this affect other PWM instances? > > > > > > The clock rate doesn't change the PWM output because the hardware values for > > > the period and duty are adapted accordingly to reflect the change. > > > > It doesn't change it in the end. But in the (short) time frame between > > the call to change the clock and the update of the PWM registers there > > is a glitch, right? > > The PWM is disabled, so the line is in inactive state, and will be in that state > until the PWM is enabled again. No glitch to fear. ok, please note in the commit log that the reordering doesn't affect the output because the PWM is off and are done to make it more obvious what happens. > > You didn't answer to the question about other PWM instances. Does that > > mean others are not affected? > > Sorry. Yes, they are not affected - all PWM channels are independent. ok. > > PS: It would be great if you could fix your mailer to not damage the > > quoted mail. Also it doesn't seem to understand how my name is encoded > > in the From line. I fixed up the quotes in my reply. > > I switched Geary to "rich text". Is that better? No. It looks exactly like the copy you bounced to the list. See https://patchwork.ozlabs.org/comment/2236355/ for how it looks. Best regards Uwe
Le mar. 13 août 2019 à 7:27, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= <u.kleine-koenig@pengutronix.de> a écrit : > Hello Paul, > > [adding Stephen Boyd to Cc] > > On Tue, Aug 13, 2019 at 12:16:23AM +0200, Paul Cercueil wrote: >> Le lun. 12 août 2019 à 23:48, Uwe Kleine-König a écrit : >> > On Mon, Aug 12, 2019 at 10:43:10PM +0200, Paul Cercueil wrote: >> > > Le lun. 12 août 2019 à 8:15, Uwe Kleine-König a écrit : >> > > > On Fri, Aug 09, 2019 at 07:14:45PM +0200, Paul Cercueil wrote: >> > > > > Le ven. 9 août 2019 à 19:05, Uwe Kleine-König a écrit : >> > > > > > On Fri, Aug 09, 2019 at 02:30:28PM +0200, Paul Cercueil >> wrote: >> > > > > > > [...] >> > > > > > > + /* Reset the clock to the maximum rate, and we'll >> reduce it if needed */ >> > > > > > > + ret = clk_set_max_rate(clk, parent_rate); >> > > > > > >> > > > > > What is the purpose of this call? IIUC this limits the >> allowed range of >> > > > > > rates for clk. I assume the idea is to prevent other >> consumers to change >> > > > > > the rate in a way that makes it unsuitable for this pwm. >> But this only >> > > > > > makes sense if you had a notifier for clk changes, >> doesn't it? I'm >> > > > > > confused. >> > > > > >> > > > > Nothing like that. The second call to clk_set_max_rate() >> might have set >> > > > > a maximum clock rate that's lower than the parent's rate, >> and we want to >> > > > > undo that. >> > > > >> > > > I still don't get the purpose of this call. Why do you limit >> the clock >> > > > rate at all? >> > > >> > > As it says below, we "limit the clock to a maximum rate that >> still gives >> > > us a period value which fits in 16 bits". So that the computed >> hardware >> > > values won't overflow. >> > >> > But why not just using clk_set_rate? You want to have the clock >> running >> > at a certain rate, not any rate below that certain rate, don't >> you? >> >> I'll let yourself answer yourself: >> https://patchwork.ozlabs.org/patch/1018969/ > > In that thread I claimed that you used clk_round_rate wrongly, not > that > you should use clk_set_max_rate(). (The claim was somewhat weakend by > Stephen, but still I think that clk_round_rate is the right approach.) Well, you said that I shouln't rely on the fact that clk_round_rate() will round down. That completely defeats the previous algorithm. So please tell me how to use it correctly, because I don't see it. I came up with a much smarter alternative, that doesn't rely on the rounding method of clk_round_rate, and which is better overall (no loop needed). It sounds to me like you're bashing the code without making the effort to understand what it does. Thierry called it a "neat trick" (https://patchwork.kernel.org/patch/10836879/) so it cannot be as bad as you say. > > The upside of clk_round_rate is that it allows you to test for the > capabilities of the clock without actually changing it before you > found > a setting you consider to be good. I know what clk_round_rate() is for. But here we don't do trial-and-error to find the first highest clock rate that works, we compute the maximum clock we can use and limit the clock rate to that. > >> It's enough to run it below a certain rate, yes. The actual rate >> doesn't >> actually matter that much. > > 1 Hz would be fine? I doubt it. We use the highest possible clock rate. We wouldn't use 1 Hz unless it's the highest clock rate available. > >> > > E.g. if at a rate of 12 MHz your computed hardware value for >> the period >> > > is 0xf000, then at a rate of 24 MHz it won't fit in 16 bits. >> So the clock >> > > rate must be reduced to the highest possible that will still >> give you a >> > > < 16-bit value. >> > > >> > > We always want the highest possible clock rate that works, for >> the sake of >> > > precision. >> > >> > This is dubious; but ok to keep the driver simple. (Consider a >> PWM that >> > can run at i MHz for i in [1, .. 30]. If a period of 120 ns and a >> duty >> > cycle of 40 ns is requested you can get an exact match with 25 >> MHz, but >> > not with 30 MHz.) >> >> The clock rate is actually (parent_rate >> (2 * x) ) >> for x = 0, 1, 2, ... >> >> So if your parent_rate is 30 MHz the next valid one is 7.5 MHz, and >> the >> next one is 1.875 MHz. It'd be very unlikely that you get a better >> match at >> a lower clock. > > If the smaller freqs are all dividers of the fastest that's fine. > Please > note in a code comment that you're assuming this. No, I am not assuming this. The current driver just picks the highest clock rate that works. We're not changing the behaviour here. > >> > > > > Basically, we start from the maximum clock rate we can >> get for that PWM >> > > > > - which is the rate of the parent clk - and from that >> compute the maximum >> > > > > clock rate that we can support that still gives us < >> 16-bits hardware >> > > > > values for the period and duty. >> > > > > >> > > > > We then pass that computed maximum clock rate to >> clk_set_max_rate(), which >> > > > > may or may not update the current PWM clock's rate to >> match the new limits. >> > > > > Finally we read back the PWM clock's rate and compute the >> period and duty >> > > > > from that. >> > > > >> > > > If you change the clk rate, is this externally visible on >> the PWM >> > > > output? Does this affect other PWM instances? >> > > >> > > The clock rate doesn't change the PWM output because the >> hardware values for >> > > the period and duty are adapted accordingly to reflect the >> change. >> > >> > It doesn't change it in the end. But in the (short) time frame >> between >> > the call to change the clock and the update of the PWM registers >> there >> > is a glitch, right? >> >> The PWM is disabled, so the line is in inactive state, and will be >> in that state >> until the PWM is enabled again. No glitch to fear. > > ok, please note in the commit log that the reordering doesn't affect > the > output because the PWM is off and are done to make it more obvious > what > happens. > >> > You didn't answer to the question about other PWM instances. Does >> that >> > mean others are not affected? >> >> Sorry. Yes, they are not affected - all PWM channels are >> independent. > > ok. > >> > PS: It would be great if you could fix your mailer to not damage >> the >> > quoted mail. Also it doesn't seem to understand how my name is >> encoded >> > in the From line. I fixed up the quotes in my reply. >> >> I switched Geary to "rich text". Is that better? > > No. It looks exactly like the copy you bounced to the list. See > https://patchwork.ozlabs.org/comment/2236355/ for how it looks. > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König > | > Industrial Linux Solutions | > http://www.pengutronix.de/ |
Hello Paul, On Tue, Aug 13, 2019 at 01:01:06PM +0200, Paul Cercueil wrote: > Le mar. 13 août 2019 à 7:27, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= > <u.kleine-koenig@pengutronix.de> a écrit : > > [adding Stephen Boyd to Cc] > > > > On Tue, Aug 13, 2019 at 12:16:23AM +0200, Paul Cercueil wrote: > > > Le lun. 12 août 2019 à 23:48, Uwe Kleine-König a écrit : > > > > On Mon, Aug 12, 2019 at 10:43:10PM +0200, Paul Cercueil wrote: > > > > > Le lun. 12 août 2019 à 8:15, Uwe Kleine-König a écrit : > > > > > > On Fri, Aug 09, 2019 at 07:14:45PM +0200, Paul Cercueil wrote: > > > > > > > Le ven. 9 août 2019 à 19:05, Uwe Kleine-König a écrit : > > > > > > > > On Fri, Aug 09, 2019 at 02:30:28PM +0200, Paul Cercueil wrote: > > > > > > > > > [...] > > > > > > > > > + /* Reset the clock to the maximum rate, and we'll reduce it if needed */ > > > > > > > > > + ret = clk_set_max_rate(clk, parent_rate); > > > > > > > > > > > > > > > > What is the purpose of this call? IIUC this limits the allowed range of > > > > > > > > rates for clk. I assume the idea is to prevent other consumers to change > > > > > > > > the rate in a way that makes it unsuitable for this pwm. But this only > > > > > > > > makes sense if you had a notifier for clk changes, doesn't it? I'm > > > > > > > > confused. > > > > > > > > > > > > > > Nothing like that. The second call to clk_set_max_rate() might have set > > > > > > > a maximum clock rate that's lower than the parent's rate, and we want to > > > > > > > undo that. > > > > > > > > > > > > I still don't get the purpose of this call. Why do you limit the clock > > > > > > rate at all? > > > > > > > > > > As it says below, we "limit the clock to a maximum rate that still gives > > > > > us a period value which fits in 16 bits". So that the computed hardware > > > > > values won't overflow. > > > > > > > > But why not just using clk_set_rate? You want to have the clock running > > > > at a certain rate, not any rate below that certain rate, don't you? > > > > > > I'll let yourself answer yourself: > > > https://patchwork.ozlabs.org/patch/1018969/ > > > > In that thread I claimed that you used clk_round_rate wrongly, not that > > you should use clk_set_max_rate(). (The claim was somewhat weakend by > > Stephen, but still I think that clk_round_rate is the right approach.) > > Well, you said that I shouln't rely on the fact that clk_round_rate() will > round down. That completely defeats the previous algorithm. So please tell > me how to use it correctly, because I don't see it. Using clk_round_rate correctly without additional knowledge is hard. If you assume at least some sane behaviour you'd still have to call it multiple times. Assuming maxrate is the maximal rate you can handle without overflowing your PWM registers you have to do: rate = maxrate; rounded_rate = clk_round_rate(clk, rate); while (rounded_rate > rate) { if (rate < rounded_rate - rate) { /* * clk doesn't support a rate smaller than * maxrate (or the round_rate callback doesn't * round consistently). */ return -ESOMETHING; } rate = rate - (rounded_rate - rate) rounded_rate = clk_round_rate(clk, rate); } return rate; Probably it would be sensible to put that in a function provided by the clk framework (maybe call it clk_round_rate_down and maybe with additional checks). > I came up with a much smarter alternative, that doesn't rely on the rounding > method of clk_round_rate, and which is better overall (no loop needed). It > sounds to me like you're bashing the code without making the effort to > understand what it does. > > Thierry called it a "neat trick" > (https://patchwork.kernel.org/patch/10836879/) so it cannot be as bad as you > say. Either that or Thierry failed to see the downside. The obvious downside is that once you set the period to something long (and so the clk was limited to a small frequency) you never make the clock any faster afterwards. Also I wonder how clk_set_max_rate() is supposed to be used like that or if instead some work should be invested to make it easier for clk consumers to use clk_round_rate() (e.g. by providing helper functions like the above). Stephen, can you shed some light into this? > > The upside of clk_round_rate is that it allows you to test for the > > capabilities of the clock without actually changing it before you found > > a setting you consider to be good. > > I know what clk_round_rate() is for. But here we don't do trial-and-error to > find the first highest clock rate that works, we compute the maximum clock > we can use and limit the clock rate to that. > > > > > > It's enough to run it below a certain rate, yes. The actual rate > > > doesn't > > > actually matter that much. > > > > 1 Hz would be fine? I doubt it. > > We use the highest possible clock rate. We wouldn't use 1 Hz unless it's the > highest clock rate available. That's wrong. If the clk already runs at 1 Hz and you call clk_set_max_rate(rate, somethingincrediblehigh); it still runs at 1 Hz afterwards. (Unless I missed something.) > > > > > E.g. if at a rate of 12 MHz your computed hardware value for the period > > > > > is 0xf000, then at a rate of 24 MHz it won't fit in 16 bits. So the clock > > > > > rate must be reduced to the highest possible that will still give you a > > > > > < 16-bit value. > > > > > > > > > > We always want the highest possible clock rate that works, for the sake of > > > > > precision. > > > > > > > > This is dubious; but ok to keep the driver simple. (Consider a PWM that > > > > can run at i MHz for i in [1, .. 30]. If a period of 120 ns and a duty > > > > cycle of 40 ns is requested you can get an exact match with 25 MHz, but > > > > not with 30 MHz.) > > > > > > The clock rate is actually (parent_rate >> (2 * x) ) > > > for x = 0, 1, 2, ... > > > > > > So if your parent_rate is 30 MHz the next valid one is 7.5 MHz, and the > > > next one is 1.875 MHz. It'd be very unlikely that you get a better match at > > > a lower clock. > > > > If the smaller freqs are all dividers of the fastest that's fine. Please > > note in a code comment that you're assuming this. > > No, I am not assuming this. The current driver just picks the highest clock > rate that works. We're not changing the behaviour here. But you hide it behind clk API functions that don't guarantee this behaviour. And even if it works for you it might not for the next person who copies your code to support another hardware. Best regards Uwe
Le mar. 13 août 2019 à 14:33, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= <u.kleine-koenig@pengutronix.de> a écrit : > Hello Paul, > > On Tue, Aug 13, 2019 at 01:01:06PM +0200, Paul Cercueil wrote: >> Le mar. 13 août 2019 à 7:27, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= >> <u.kleine-koenig@pengutronix.de> a écrit : >> > [adding Stephen Boyd to Cc] >> > >> > On Tue, Aug 13, 2019 at 12:16:23AM +0200, Paul Cercueil wrote: >> > > Le lun. 12 août 2019 à 23:48, Uwe Kleine-König a écrit : >> > > > On Mon, Aug 12, 2019 at 10:43:10PM +0200, Paul Cercueil wrote: >> > > > > Le lun. 12 août 2019 à 8:15, Uwe Kleine-König a écrit : >> > > > > > On Fri, Aug 09, 2019 at 07:14:45PM +0200, Paul Cercueil >> wrote: >> > > > > > > Le ven. 9 août 2019 à 19:05, Uwe Kleine-König a >> écrit : >> > > > > > > > On Fri, Aug 09, 2019 at 02:30:28PM +0200, Paul >> Cercueil wrote: >> > > > > > > > > [...] >> > > > > > > > > + /* Reset the clock to the maximum rate, and >> we'll reduce it if needed */ >> > > > > > > > > + ret = clk_set_max_rate(clk, parent_rate); >> > > > > > > > >> > > > > > > > What is the purpose of this call? IIUC this limits >> the allowed range of >> > > > > > > > rates for clk. I assume the idea is to prevent other >> consumers to change >> > > > > > > > the rate in a way that makes it unsuitable for this >> pwm. But this only >> > > > > > > > makes sense if you had a notifier for clk changes, >> doesn't it? I'm >> > > > > > > > confused. >> > > > > > > >> > > > > > > Nothing like that. The second call to >> clk_set_max_rate() might have set >> > > > > > > a maximum clock rate that's lower than the parent's >> rate, and we want to >> > > > > > > undo that. >> > > > > > >> > > > > > I still don't get the purpose of this call. Why do you >> limit the clock >> > > > > > rate at all? >> > > > > >> > > > > As it says below, we "limit the clock to a maximum rate >> that still gives >> > > > > us a period value which fits in 16 bits". So that the >> computed hardware >> > > > > values won't overflow. >> > > > >> > > > But why not just using clk_set_rate? You want to have the >> clock running >> > > > at a certain rate, not any rate below that certain rate, >> don't you? >> > > >> > > I'll let yourself answer yourself: >> > > https://patchwork.ozlabs.org/patch/1018969/ >> > >> > In that thread I claimed that you used clk_round_rate wrongly, >> not that >> > you should use clk_set_max_rate(). (The claim was somewhat >> weakend by >> > Stephen, but still I think that clk_round_rate is the right >> approach.) >> >> Well, you said that I shouln't rely on the fact that >> clk_round_rate() will >> round down. That completely defeats the previous algorithm. So >> please tell >> me how to use it correctly, because I don't see it. > > Using clk_round_rate correctly without additional knowledge is hard. > If > you assume at least some sane behaviour you'd still have to call it > multiple times. Assuming maxrate is the maximal rate you can handle > without overflowing your PWM registers you have to do: > > rate = maxrate; > rounded_rate = clk_round_rate(clk, rate); > while (rounded_rate > rate) { > if (rate < rounded_rate - rate) { > /* > * clk doesn't support a rate smaller than > * maxrate (or the round_rate callback doesn't > * round consistently). > */ > return -ESOMETHING; > } > rate = rate - (rounded_rate - rate) > rounded_rate = clk_round_rate(clk, rate); > } > > return rate; > > Probably it would be sensible to put that in a function provided by > the > clk framework (maybe call it clk_round_rate_down and maybe with > additional checks). clk_round_rate_down() has been refused multiple times in the past for reasons that Stephen can explain. > >> I came up with a much smarter alternative, that doesn't rely on the >> rounding >> method of clk_round_rate, and which is better overall (no loop >> needed). It >> sounds to me like you're bashing the code without making the effort >> to >> understand what it does. >> >> Thierry called it a "neat trick" >> (https://patchwork.kernel.org/patch/10836879/) so it cannot be as >> bad as you >> say. > > Either that or Thierry failed to see the downside. The obvious > downside > is that once you set the period to something long (and so the clk was > limited to a small frequency) you never make the clock any faster > afterwards. Read the algorithm again. > > Also I wonder how clk_set_max_rate() is supposed to be used like that > or > if instead some work should be invested to make it easier for clk > consumers to use clk_round_rate() (e.g. by providing helper functions > like the above). Stephen, can you shed some light into this? > >> > The upside of clk_round_rate is that it allows you to test for the >> > capabilities of the clock without actually changing it before you >> found >> > a setting you consider to be good. >> >> I know what clk_round_rate() is for. But here we don't do >> trial-and-error to >> find the first highest clock rate that works, we compute the >> maximum clock >> we can use and limit the clock rate to that. >> >> > >> > > It's enough to run it below a certain rate, yes. The actual >> rate >> > > doesn't >> > > actually matter that much. >> > >> > 1 Hz would be fine? I doubt it. >> >> We use the highest possible clock rate. We wouldn't use 1 Hz unless >> it's the >> highest clock rate available. > > That's wrong. If the clk already runs at 1 Hz and you call > clk_set_max_rate(rate, somethingincrediblehigh); it still runs at 1 Hz > afterwards. (Unless I missed something.) You missed something. I reset the max rate to the parent clock's rate at the beginning of the algorithm. It works just fine. > >> > > > > E.g. if at a rate of 12 MHz your computed hardware value >> for the period >> > > > > is 0xf000, then at a rate of 24 MHz it won't fit in 16 >> bits. So the clock >> > > > > rate must be reduced to the highest possible that will >> still give you a >> > > > > < 16-bit value. >> > > > > >> > > > > We always want the highest possible clock rate that works, >> for the sake of >> > > > > precision. >> > > > >> > > > This is dubious; but ok to keep the driver simple. (Consider >> a PWM that >> > > > can run at i MHz for i in [1, .. 30]. If a period of 120 ns >> and a duty >> > > > cycle of 40 ns is requested you can get an exact match with >> 25 MHz, but >> > > > not with 30 MHz.) >> > > >> > > The clock rate is actually (parent_rate >> (2 * x) ) >> > > for x = 0, 1, 2, ... >> > > >> > > So if your parent_rate is 30 MHz the next valid one is 7.5 MHz, >> and the >> > > next one is 1.875 MHz. It'd be very unlikely that you get a >> better match at >> > > a lower clock. >> > >> > If the smaller freqs are all dividers of the fastest that's fine. >> Please >> > note in a code comment that you're assuming this. >> >> No, I am not assuming this. The current driver just picks the >> highest clock >> rate that works. We're not changing the behaviour here. > > But you hide it behind clk API functions that don't guarantee this > behaviour. And even if it works for you it might not for the next > person > who copies your code to support another hardware. Again, I'm not *trying* to guarantee this behaviour. > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König > | > Industrial Linux Solutions | > http://www.pengutronix.de/ |
Hello Paul, On Tue, Aug 13, 2019 at 02:47:28PM +0200, Paul Cercueil wrote: > Le mar. 13 août 2019 à 14:33, Uwe Kleine-König a écrit : > > On Tue, Aug 13, 2019 at 01:01:06PM +0200, Paul Cercueil wrote: > > > Well, you said that I shouln't rely on the fact that clk_round_rate() will > > > round down. That completely defeats the previous algorithm. So please tell > > > me how to use it correctly, because I don't see it. > > > > Using clk_round_rate correctly without additional knowledge is hard. If > > you assume at least some sane behaviour you'd still have to call it > > multiple times. Assuming maxrate is the maximal rate you can handle > > without overflowing your PWM registers you have to do: > > > > rate = maxrate; > > rounded_rate = clk_round_rate(clk, rate); > > while (rounded_rate > rate) { > > if (rate < rounded_rate - rate) { > > /* > > * clk doesn't support a rate smaller than > > * maxrate (or the round_rate callback doesn't > > * round consistently). > > */ > > return -ESOMETHING; > > } > > rate = rate - (rounded_rate - rate) > > rounded_rate = clk_round_rate(clk, rate); > > } > > > > return rate; > > > > Probably it would be sensible to put that in a function provided by the > > clk framework (maybe call it clk_round_rate_down and maybe with > > additional checks). > > clk_round_rate_down() has been refused multiple times in the past for > reasons that Stephen can explain. I'd be really interested in these reasons as I think the clk framework should make it easy to solve common tasks related to clocks. And finding out the biggest supported rate not bigger than a given maxrate is something I consider such a common task. The first hit I found when searching was https://lkml.org/lkml/2010/7/14/260 . In there Stephen suggested that clk_round_rate with the current semantic is hardly useful and suggested clk_round_rate_up() and clk_round_rate_down() himself. > > > I came up with a much smarter alternative, that doesn't rely on the rounding > > > method of clk_round_rate, and which is better overall (no loop needed). It > > > sounds to me like you're bashing the code without making the effort to > > > understand what it does. > > > > > > Thierry called it a "neat trick" > > > (https://patchwork.kernel.org/patch/10836879/) so it cannot be as bad as you > > > say. > > > > Either that or Thierry failed to see the downside. The obvious downside > > is that once you set the period to something long (and so the clk was > > limited to a small frequency) you never make the clock any faster > > afterwards. > > Read the algorithm again. I indeed missed a call to clk_set_rate(clk, parent_rate). I thought I grepped for clk_set_rate before claiming the code was broken. Sorry. So I think the code works indeed, but it feels like abusing clk_set_max_rate. So I'd like to see some words from Stephen about this procedure. Also I think this is kind of inelegant to set the maximal rate twice. At least call clk_set_max_rate only once please. > > > > > > > E.g. if at a rate of 12 MHz your computed hardware value for the period > > > > > > > is 0xf000, then at a rate of 24 MHz it won't fit in 16 bits. So the clock > > > > > > > rate must be reduced to the highest possible that will still give you a > > > > > > > < 16-bit value. > > > > > > > > > > > > > > We always want the highest possible clock rate that works, for the sake of > > > > > > > precision. > > > > > > > > > > > > This is dubious; but ok to keep the driver simple. (Consider a PWM that > > > > > > can run at i MHz for i in [1, .. 30]. If a period of 120 ns and a duty > > > > > > cycle of 40 ns is requested you can get an exact match with 25 MHz, but > > > > > > not with 30 MHz.) > > > > > > > > > > The clock rate is actually (parent_rate >> (2 * x) ) > > > > > for x = 0, 1, 2, ... > > > > > > > > > > So if your parent_rate is 30 MHz the next valid one is 7.5 MHz, and the > > > > > next one is 1.875 MHz. It'd be very unlikely that you get a better match at > > > > > a lower clock. > > > > > > > > If the smaller freqs are all dividers of the fastest that's fine. Please > > > > note in a code comment that you're assuming this. > > > > > > No, I am not assuming this. The current driver just picks the highest clock > > > rate that works. We're not changing the behaviour here. > > > > But you hide it behind clk API functions that don't guarantee this > > behaviour. And even if it works for you it might not for the next person > > who copies your code to support another hardware. > > Again, I'm not *trying* to guarantee this behaviour. I didn't request you should guarantee this behaviour. I want you to make it obvious for readers of your code that you rely on something that isn't guaranteed. That your code works today isn't a good enough excuse. There are various examples like these. If you want a few: - printf("string: %s\n", NULL); works fine with glibc, but segfaults on other libcs. - setenv("MYVAR", NULL) used to work (and was equivalent to setenv("MYVAR", "")) but that was never guaranteed. Then at some point of time it started to segfault. - Look into commits like a4435febd4c0f14b25159dca249ecf91301c7c76. This used to work fine until compilers were changed to optimize more aggressively. Now if you use a clk and know that all rates smaller than the requested one are divisors of the fast one and your code only works (here: is optimal) when this condition is given, you're walking on thin ice just because this fact it's not guaranteed. The least you can do is to add a code comment to make people aware who debug the breakage or copy your code. I admit this wasn't optimal already before, but at least the logic was in the same code and not hidden behind the clk API. Please do people who review or copy your code the favour to document the assumptions you're relying on. And if it's only to save some time for someone who stumbles over your code who knows the clk API and starts thinking about improving the driver. Best regards Uwe
Hi Uwe, Le mar. 13 août 2019 à 16:09, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= <u.kleine-koenig@pengutronix.de> a écrit : > Hello Paul, > > On Tue, Aug 13, 2019 at 02:47:28PM +0200, Paul Cercueil wrote: >> Le mar. 13 août 2019 à 14:33, Uwe Kleine-König a écrit : >> > On Tue, Aug 13, 2019 at 01:01:06PM +0200, Paul Cercueil wrote: >> > > Well, you said that I shouln't rely on the fact that >> clk_round_rate() will >> > > round down. That completely defeats the previous algorithm. So >> please tell >> > > me how to use it correctly, because I don't see it. >> > >> > Using clk_round_rate correctly without additional knowledge is >> hard. If >> > you assume at least some sane behaviour you'd still have to call >> it >> > multiple times. Assuming maxrate is the maximal rate you can >> handle >> > without overflowing your PWM registers you have to do: >> > >> > rate = maxrate; >> > rounded_rate = clk_round_rate(clk, rate); >> > while (rounded_rate > rate) { >> > if (rate < rounded_rate - rate) { >> > /* >> > * clk doesn't support a rate smaller than >> > * maxrate (or the round_rate callback doesn't >> > * round consistently). >> > */ >> > return -ESOMETHING; >> > } >> > rate = rate - (rounded_rate - rate) >> > rounded_rate = clk_round_rate(clk, rate); >> > } >> > >> > return rate; >> > >> > Probably it would be sensible to put that in a function provided >> by the >> > clk framework (maybe call it clk_round_rate_down and maybe with >> > additional checks). >> >> clk_round_rate_down() has been refused multiple times in the past >> for >> reasons that Stephen can explain. > > I'd be really interested in these reasons as I think the clk framework > should make it easy to solve common tasks related to clocks. And > finding > out the biggest supported rate not bigger than a given maxrate is > something I consider such a common task. > > The first hit I found when searching was > https://lkml.org/lkml/2010/7/14/260 . In there Stephen suggested that > clk_round_rate with the current semantic is hardly useful and > suggested > clk_round_rate_up() and clk_round_rate_down() himself. That's from 2010, though. I agree that clk_round_rate_up() and clk_round_rate_down() should exist. Even if they return -ENOSYS if it's not implemented for a given clock controller. > >> > > I came up with a much smarter alternative, that doesn't rely >> on the rounding >> > > method of clk_round_rate, and which is better overall (no loop >> needed). It >> > > sounds to me like you're bashing the code without making the >> effort to >> > > understand what it does. >> > > >> > > Thierry called it a "neat trick" >> > > (https://patchwork.kernel.org/patch/10836879/) so it cannot be >> as bad as you >> > > say. >> > >> > Either that or Thierry failed to see the downside. The obvious >> downside >> > is that once you set the period to something long (and so the clk >> was >> > limited to a small frequency) you never make the clock any faster >> > afterwards. >> >> Read the algorithm again. > > I indeed missed a call to clk_set_rate(clk, parent_rate). I thought I > grepped for clk_set_rate before claiming the code was broken. Sorry. > > So I think the code works indeed, but it feels like abusing > clk_set_max_rate. So I'd like to see some words from Stephen about > this > procedure. > > Also I think this is kind of inelegant to set the maximal rate twice. > At > least call clk_set_max_rate only once please. Ok. I can do that. > >> > > > > > > E.g. if at a rate of 12 MHz your computed hardware >> value for the period >> > > > > > > is 0xf000, then at a rate of 24 MHz it won't fit in 16 >> bits. So the clock >> > > > > > > rate must be reduced to the highest possible that will >> still give you a >> > > > > > > < 16-bit value. >> > > > > > > >> > > > > > > We always want the highest possible clock rate that >> works, for the sake of >> > > > > > > precision. >> > > > > > >> > > > > > This is dubious; but ok to keep the driver simple. >> (Consider a PWM that >> > > > > > can run at i MHz for i in [1, .. 30]. If a period of 120 >> ns and a duty >> > > > > > cycle of 40 ns is requested you can get an exact match >> with 25 MHz, but >> > > > > > not with 30 MHz.) >> > > > > >> > > > > The clock rate is actually (parent_rate >> (2 * x) ) >> > > > > for x = 0, 1, 2, ... >> > > > > >> > > > > So if your parent_rate is 30 MHz the next valid one is 7.5 >> MHz, and the >> > > > > next one is 1.875 MHz. It'd be very unlikely that you get a >> better match at >> > > > > a lower clock. >> > > > >> > > > If the smaller freqs are all dividers of the fastest that's >> fine. Please >> > > > note in a code comment that you're assuming this. >> > > >> > > No, I am not assuming this. The current driver just picks the >> highest clock >> > > rate that works. We're not changing the behaviour here. >> > >> > But you hide it behind clk API functions that don't guarantee this >> > behaviour. And even if it works for you it might not for the next >> person >> > who copies your code to support another hardware. >> >> Again, I'm not *trying* to guarantee this behaviour. > > I didn't request you should guarantee this behaviour. I want you to > make > it obvious for readers of your code that you rely on something that > isn't guaranteed. That your code works today isn't a good enough > excuse. > There are various examples like these. If you want a few: > > - printf("string: %s\n", NULL); works fine with glibc, but segfaults > on > other libcs. > - setenv("MYVAR", NULL) used to work (and was equivalent to > setenv("MYVAR", "")) but that was never guaranteed. Then at some > point of time it started to segfault. > - Look into commits like a4435febd4c0f14b25159dca249ecf91301c7c76. > This > used to work fine until compilers were changed to optimize more > aggressively. > > Now if you use a clk and know that all rates smaller than the > requested > one are divisors of the fast one and your code only works (here: is > optimal) when this condition is given, you're walking on thin ice just > because this fact it's not guaranteed. > The least you can do is to add a code comment to make people aware who > debug the breakage or copy your code. If I was assuming something, it's not that the requested clock rates are always integer dividers of the parent rate - but rather that the difference in precision between two possible clock rates (even non-integer-dividers) is so tiny that we just don't care. > > I admit this wasn't optimal already before, but at least the logic was > in the same code and not hidden behind the clk API. > > Please do people who review or copy your code the favour to document > the > assumptions you're relying on. And if it's only to save some time for > someone who stumbles over your code who knows the clk API and starts > thinking about improving the driver. Ok. I can add a comment. > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König > | > Industrial Linux Solutions | > http://www.pengutronix.de/ |
Hello Paul, On Wed, Aug 14, 2019 at 06:10:35PM +0200, Paul Cercueil wrote: > Le mar. 13 août 2019 à 16:09, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= a écrit : > > On Tue, Aug 13, 2019 at 02:47:28PM +0200, Paul Cercueil wrote: > > > Le mar. 13 août 2019 à 14:33, Uwe Kleine-König a écrit : > > > > On Tue, Aug 13, 2019 at 01:01:06PM +0200, Paul Cercueil wrote: > > > > > Well, you said that I shouln't rely on the fact that clk_round_rate() will > > > > > round down. That completely defeats the previous algorithm. So please tell > > > > > me how to use it correctly, because I don't see it. > > > > > > > > Using clk_round_rate correctly without additional knowledge is hard. If > > > > you assume at least some sane behaviour you'd still have to call it > > > > multiple times. Assuming maxrate is the maximal rate you can handle > > > > without overflowing your PWM registers you have to do: > > > > > > > > rate = maxrate; > > > > rounded_rate = clk_round_rate(clk, rate); > > > > while (rounded_rate > rate) { > > > > if (rate < rounded_rate - rate) { > > > > /* > > > > * clk doesn't support a rate smaller than > > > > * maxrate (or the round_rate callback doesn't > > > > * round consistently). > > > > */ > > > > return -ESOMETHING; > > > > } > > > > rate = rate - (rounded_rate - rate) > > > > rounded_rate = clk_round_rate(clk, rate); > > > > } > > > > > > > > return rate; > > > > > > > > Probably it would be sensible to put that in a function provided by the > > > > clk framework (maybe call it clk_round_rate_down and maybe with > > > > additional checks). > > > > > > clk_round_rate_down() has been refused multiple times in the past for > > > reasons that Stephen can explain. > > > > I'd be really interested in these reasons as I think the clk framework > > should make it easy to solve common tasks related to clocks. And finding > > out the biggest supported rate not bigger than a given maxrate is > > something I consider such a common task. > > > > The first hit I found when searching was > > https://lkml.org/lkml/2010/7/14/260 . In there Stephen suggested that > > clk_round_rate with the current semantic is hardly useful and suggested > > clk_round_rate_up() and clk_round_rate_down() himself. > > That's from 2010, though. If you have a better link please tell me. > I agree that clk_round_rate_up() and clk_round_rate_down() should exist. > Even if they return -ENOSYS if it's not implemented for a given clock > controller. ack. > > > > > I came up with a much smarter alternative, that doesn't rely on the rounding > > > > > method of clk_round_rate, and which is better overall (no loop needed). It > > > > > sounds to me like you're bashing the code without making the effort to > > > > > understand what it does. > > > > > > > > > > Thierry called it a "neat trick" > > > > > (https://patchwork.kernel.org/patch/10836879/) so it cannot be as bad as you > > > > > say. > > > > > > > > Either that or Thierry failed to see the downside. The obvious downside > > > > is that once you set the period to something long (and so the clk was > > > > limited to a small frequency) you never make the clock any faster > > > > afterwards. > > > > > > Read the algorithm again. > > > > I indeed missed a call to clk_set_rate(clk, parent_rate). I thought I > > grepped for clk_set_rate before claiming the code was broken. Sorry. > > > > So I think the code works indeed, but it feels like abusing > > clk_set_max_rate. So I'd like to see some words from Stephen about this > > procedure. > > > > Also I think this is kind of inelegant to set the maximal rate twice. At > > least call clk_set_max_rate only once please. > > Ok. I can do that. I would still prefer to hear from Stephen about this approach. It seems wrong to have two different ways to achieve the same goal and my impression is that clk_round_rate is the function designed for this use case. > > > > > > > > > E.g. if at a rate of 12 MHz your computed hardware value for the period > > > > > > > > > is 0xf000, then at a rate of 24 MHz it won't fit in 16 bits. So the clock > > > > > > > > > rate must be reduced to the highest possible that will still give you a > > > > > > > > > < 16-bit value. > > > > > > > > > > > > > > > > > > We always want the highest possible clock rate that works, for the sake of > > > > > > > > > precision. > > > > > > > > > > > > > > > > This is dubious; but ok to keep the driver simple.> (Consider a PWM that > > > > > > > > can run at i MHz for i in [1, .. 30]. If a period of 120 ns and a duty > > > > > > > > cycle of 40 ns is requested you can get an exact match with 25 MHz, but > > > > > > > > not with 30 MHz.) > > > > > > > > > > > > > > The clock rate is actually (parent_rate >> (2 * x) ) > > > > > > > for x = 0, 1, 2, ... > > > > > > > > > > > > > > So if your parent_rate is 30 MHz the next valid one is 7.5 MHz, and the > > > > > > > next one is 1.875 MHz. It'd be very unlikely that you get a better match at > > > > > > > a lower clock. > > > > > > > > > > > > If the smaller freqs are all dividers of the fastest that's fine. Please > > > > > > note in a code comment that you're assuming this. > > > > > > > > > > No, I am not assuming this. The current driver just picks the highest clock > > > > > rate that works. We're not changing the behaviour here. > > > > > > > > But you hide it behind clk API functions that don't guarantee this > > > > behaviour. And even if it works for you it might not for the next person > > > > who copies your code to support another hardware. > > > > > > Again, I'm not *trying* to guarantee this behaviour. > > > > I didn't request you should guarantee this behaviour. I want you to make > > it obvious for readers of your code that you rely on something that > > isn't guaranteed. That your code works today isn't a good enough excuse. > > There are various examples like these. If you want a few: > > > > - printf("string: %s\n", NULL); works fine with glibc, but segfaults on > > other libcs. > > - setenv("MYVAR", NULL) used to work (and was equivalent to > > setenv("MYVAR", "")) but that was never guaranteed. Then at some > > point of time it started to segfault. > > - Look into commits like a4435febd4c0f14b25159dca249ecf91301c7c76. This > > used to work fine until compilers were changed to optimize more > > aggressively. > > > > Now if you use a clk and know that all rates smaller than the requested > > one are divisors of the fast one and your code only works (here: is > > optimal) when this condition is given, you're walking on thin ice just > > because this fact it's not guaranteed. > > The least you can do is to add a code comment to make people aware who > > debug the breakage or copy your code. > > If I was assuming something, it's not that the requested clock rates are > always integer dividers of the parent rate - but rather that the difference > in precision between two possible clock rates (even non-integer-dividers) is > so tiny that we just don't care. I'm more exacting here. If you are asked for X and can provide X - 2 you shouldn't provide X - 12. Depending on the use case the consumer is happy about every bit of accuracy they can get. So if you deliberately provide X - 12 because it is easier to do and good enough for you, at least document this laziness to not waste other people's time more than necessary. Best regards Uwe
Hi, Le mer., août 14, 2019 at 19:32, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> a écrit : > Hello Paul, > > On Wed, Aug 14, 2019 at 06:10:35PM +0200, Paul Cercueil wrote: >> Le mar. 13 août 2019 à 16:09, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= >> a écrit : >> > On Tue, Aug 13, 2019 at 02:47:28PM +0200, Paul Cercueil wrote: >> > > Le mar. 13 août 2019 à 14:33, Uwe Kleine-König a écrit : >> > > > On Tue, Aug 13, 2019 at 01:01:06PM +0200, Paul Cercueil wrote: >> > > > > Well, you said that I shouln't rely on the fact that >> clk_round_rate() will >> > > > > round down. That completely defeats the previous algorithm. >> So please tell >> > > > > me how to use it correctly, because I don't see it. >> > > > >> > > > Using clk_round_rate correctly without additional knowledge >> is hard. If >> > > > you assume at least some sane behaviour you'd still have to >> call it >> > > > multiple times. Assuming maxrate is the maximal rate you can >> handle >> > > > without overflowing your PWM registers you have to do: >> > > > >> > > > rate = maxrate; >> > > > rounded_rate = clk_round_rate(clk, rate); >> > > > while (rounded_rate > rate) { >> > > > if (rate < rounded_rate - rate) { >> > > > /* >> > > > * clk doesn't support a rate smaller than >> > > > * maxrate (or the round_rate callback doesn't >> > > > * round consistently). >> > > > */ >> > > > return -ESOMETHING; >> > > > } >> > > > rate = rate - (rounded_rate - rate) >> > > > rounded_rate = clk_round_rate(clk, rate); >> > > > } >> > > > >> > > > return rate; >> > > > >> > > > Probably it would be sensible to put that in a function >> provided by the >> > > > clk framework (maybe call it clk_round_rate_down and maybe >> with >> > > > additional checks). >> > > >> > > clk_round_rate_down() has been refused multiple times in the >> past for >> > > reasons that Stephen can explain. >> > >> > I'd be really interested in these reasons as I think the clk >> framework >> > should make it easy to solve common tasks related to clocks. And >> finding >> > out the biggest supported rate not bigger than a given maxrate is >> > something I consider such a common task. >> > >> > The first hit I found when searching was >> > https://lkml.org/lkml/2010/7/14/260 . In there Stephen suggested >> that >> > clk_round_rate with the current semantic is hardly useful and >> suggested >> > clk_round_rate_up() and clk_round_rate_down() himself. >> >> That's from 2010, though. > > If you have a better link please tell me. > >> I agree that clk_round_rate_up() and clk_round_rate_down() should >> exist. >> Even if they return -ENOSYS if it's not implemented for a given >> clock >> controller. > > ack. > >> > > > > I came up with a much smarter alternative, that doesn't >> rely on the rounding >> > > > > method of clk_round_rate, and which is better overall (no >> loop needed). It >> > > > > sounds to me like you're bashing the code without making >> the effort to >> > > > > understand what it does. >> > > > > >> > > > > Thierry called it a "neat trick" >> > > > > (https://patchwork.kernel.org/patch/10836879/) so it cannot >> be as bad as you >> > > > > say. >> > > > >> > > > Either that or Thierry failed to see the downside. The >> obvious downside >> > > > is that once you set the period to something long (and so the >> clk was >> > > > limited to a small frequency) you never make the clock any >> faster >> > > > afterwards. >> > > >> > > Read the algorithm again. >> > >> > I indeed missed a call to clk_set_rate(clk, parent_rate). I >> thought I >> > grepped for clk_set_rate before claiming the code was broken. >> Sorry. >> > >> > So I think the code works indeed, but it feels like abusing >> > clk_set_max_rate. So I'd like to see some words from Stephen >> about this >> > procedure. >> > >> > Also I think this is kind of inelegant to set the maximal rate >> twice. At >> > least call clk_set_max_rate only once please. >> >> Ok. I can do that. > > I would still prefer to hear from Stephen about this approach. It > seems > wrong to have two different ways to achieve the same goal and my > impression is that clk_round_rate is the function designed for this > use > case. Stephen, any feedback? I'm still stuck here. >> > > > > > > > > E.g. if at a rate of 12 MHz your computed hardware >> value for the period >> > > > > > > > > is 0xf000, then at a rate of 24 MHz it won't fit in >> 16 bits. So the clock >> > > > > > > > > rate must be reduced to the highest possible that >> will still give you a >> > > > > > > > > < 16-bit value. >> > > > > > > > > >> > > > > > > > > We always want the highest possible clock rate that >> works, for the sake of >> > > > > > > > > precision. >> > > > > > > > >> > > > > > > > This is dubious; but ok to keep the driver simple.> >> (Consider a PWM that >> > > > > > > > can run at i MHz for i in [1, .. 30]. If a period of >> 120 ns and a duty >> > > > > > > > cycle of 40 ns is requested you can get an exact >> match with 25 MHz, but >> > > > > > > > not with 30 MHz.) >> > > > > > > >> > > > > > > The clock rate is actually (parent_rate >> (2 * x) ) >> > > > > > > for x = 0, 1, 2, ... >> > > > > > > >> > > > > > > So if your parent_rate is 30 MHz the next valid one is >> 7.5 MHz, and the >> > > > > > > next one is 1.875 MHz. It'd be very unlikely that you >> get a better match at >> > > > > > > a lower clock. >> > > > > > >> > > > > > If the smaller freqs are all dividers of the fastest >> that's fine. Please >> > > > > > note in a code comment that you're assuming this. >> > > > > >> > > > > No, I am not assuming this. The current driver just picks >> the highest clock >> > > > > rate that works. We're not changing the behaviour here. >> > > > >> > > > But you hide it behind clk API functions that don't guarantee >> this >> > > > behaviour. And even if it works for you it might not for the >> next person >> > > > who copies your code to support another hardware. >> > > >> > > Again, I'm not *trying* to guarantee this behaviour. >> > >> > I didn't request you should guarantee this behaviour. I want you >> to make >> > it obvious for readers of your code that you rely on something >> that >> > isn't guaranteed. That your code works today isn't a good enough >> excuse. >> > There are various examples like these. If you want a few: >> > >> > - printf("string: %s\n", NULL); works fine with glibc, but >> segfaults on >> > other libcs. >> > - setenv("MYVAR", NULL) used to work (and was equivalent to >> > setenv("MYVAR", "")) but that was never guaranteed. Then at >> some >> > point of time it started to segfault. >> > - Look into commits like >> a4435febd4c0f14b25159dca249ecf91301c7c76. This >> > used to work fine until compilers were changed to optimize more >> > aggressively. >> > >> > Now if you use a clk and know that all rates smaller than the >> requested >> > one are divisors of the fast one and your code only works (here: >> is >> > optimal) when this condition is given, you're walking on thin ice >> just >> > because this fact it's not guaranteed. >> > The least you can do is to add a code comment to make people >> aware who >> > debug the breakage or copy your code. >> >> If I was assuming something, it's not that the requested clock >> rates are >> always integer dividers of the parent rate - but rather that the >> difference >> in precision between two possible clock rates (even >> non-integer-dividers) is >> so tiny that we just don't care. > > I'm more exacting here. If you are asked for X and can provide X - 2 > you > shouldn't provide X - 12. Depending on the use case the consumer is > happy > about every bit of accuracy they can get. So if you deliberately > provide > X - 12 because it is easier to do and good enough for you, at least > document this laziness to not waste other people's time more than > necessary. > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König > | > Industrial Linux Solutions | > http://www.pengutronix.de/ |
Hello Stephen, hello Michael, first some words about the context for the newcomers in this thread (or those who already got the earlier mails some time ago and obliterated the details): The task at hand is to set the frequency of a parent clock to be able to setup a PWM to yield a certain period and duty cycle. For that there is an upper limit of the frequency and otherwise we want the clock to run as fast as possible[1]. On Mon, Oct 21, 2019 at 02:47:57PM +0200, Paul Cercueil wrote: > Le mer., août 14, 2019 at 19:32, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> a écrit : > > On Wed, Aug 14, 2019 at 06:10:35PM +0200, Paul Cercueil wrote: > > > Le mar. 13 août 2019 à 16:09, Uwe =?iso-8859-1?q?Kleine-K=F6nig?= a écrit : > > > > On Tue, Aug 13, 2019 at 02:47:28PM +0200, Paul Cercueil wrote: > > > > > Le mar. 13 août 2019 à 14:33, Uwe Kleine-König a écrit : > > > > > > Using clk_round_rate correctly without additional knowledge is hard. If > > > > > > you assume at least some sane behaviour you'd still have to call it > > > > > > multiple times. Assuming maxrate is the maximal rate you can handle > > > > > > without overflowing your PWM registers you have to do: > > > > > > > > > > > > rate = maxrate; > > > > > > rounded_rate = clk_round_rate(clk, rate); > > > > > > while (rounded_rate > rate) { > > > > > > if (rate < rounded_rate - rate) { > > > > > > /* > > > > > > * clk doesn't support a rate smaller than > > > > > > * maxrate (or the round_rate callback doesn't > > > > > > * round consistently). > > > > > > */ > > > > > > return -ESOMETHING; > > > > > > } > > > > > > rate = rate - (rounded_rate - rate) > > > > > > rounded_rate = clk_round_rate(clk, rate); > > > > > > } > > > > > > > > > > > > return rate; > > > > > > > > > > > > Probably it would be sensible to put that in a function provided by the > > > > > > clk framework (maybe call it clk_round_rate_down and maybe with > > > > > > additional checks). > > > > > > > > > > clk_round_rate_down() has been refused multiple times in the past for > > > > > reasons that Stephen can explain. > > > > > > > > I'd be really interested in these reasons as I think the clk framework > > > > should make it easy to solve common tasks related to clocks. And finding > > > > out the biggest supported rate not bigger than a given maxrate is > > > > something I consider such a common task. > > > > > > > > The first hit I found when searching was > > > > https://lkml.org/lkml/2010/7/14/260 . In there Stephen suggested that > > > > clk_round_rate with the current semantic is hardly useful and suggested > > > > clk_round_rate_up() and clk_round_rate_down() himself. > > > > > > That's from 2010, though. > > > > If you have a better link please tell me. > > > > > I agree that clk_round_rate_up() and clk_round_rate_down() should exist. > > > Even if they return -ENOSYS if it's not implemented for a given clock > > > controller. > > > > ack. Can you please explain what is the reason why clk_round_rate_up/down() is a bad idea? Would it help to create a patch that introduces these functions to get the discussion going? > > > > > > > I came up with a much smarter alternative, that doesn't rely on the rounding > > > > > > > method of clk_round_rate, and which is better overall (no loop needed). It > > > > > > > sounds to me like you're bashing the code without making the effort to > > > > > > > understand what it does. > > > > > > > > > > > > > > Thierry called it a "neat trick" > > > > > > > (https://patchwork.kernel.org/patch/10836879/) so it cannot be as bad as you > > > > > > > say. > > > > > > [...] > > > > > [...] > > > > > > > > So I think the code works indeed, but it feels like abusing > > > > clk_set_max_rate. So I'd like to see some words from Stephen about this > > > > procedure. The approach here was as follows: clk_set_rate(clk, parentrate); clk_set_max_rate(clk, maxfreq); I don't know what the exact purpose of clk_set_max_rate() is, but it seems questionable to me if it is supposed to be used like that. (As a side note: According to the FIXME in clk_set_rate_range() it doesn't even guarantee that the rate of clk is <= maxfreq after the call. clk_round_rate_down() would help here, too ...) Best regards Uwe [1] This isn't necessarily the best clk freq, but a reasonable to work with.
Hello Stephen, hello Michael, On Wed, Feb 12, 2020 at 08:29:11AM +0100, Uwe Kleine-König wrote: > Can you please explain what is the reason why clk_round_rate_up/down() > is a bad idea? Would it help to create a patch that introduces these > functions to get the discussion going? I didn't get any feedback on my mail. Are you to busy working on more important stuff? Is the answer so obvious that you don't consider it worth your time to answer? Looking a bit through the code I see there are two callbacks hwclks can provide to implement rounding (determine_rate and round_rate). The docs for both use the term "return the closes rate actually supported". Does that mean "round-closest" is already the official policy and other strategies in lowlevel drivers are a bug? Best regards Uwe
Hello, On Tue, Apr 14, 2020 at 11:24:12AM +0200, Uwe Kleine-König wrote: > Hello Stephen, hello Michael, > > On Wed, Feb 12, 2020 at 08:29:11AM +0100, Uwe Kleine-König wrote: > > Can you please explain what is the reason why clk_round_rate_up/down() > > is a bad idea? Would it help to create a patch that introduces these > > functions to get the discussion going? > > I didn't get any feedback on my mail. Are you to busy working on more > important stuff? Is the answer so obvious that you don't consider it > worth your time to answer? > > Looking a bit through the code I see there are two callbacks hwclks can > provide to implement rounding (determine_rate and round_rate). The docs > for both use the term "return the closes rate actually supported". Does > that mean "round-closest" is already the official policy and other > strategies in lowlevel drivers are a bug? Feedback here would be really appreciated. I intend to unify the rounding behaviour of PWMs to always round down. If there was a similar constraint for clks, some corner cases might be a bit simpler. Looking forward to read about your thoughts, Uwe
diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c index 6ec8794d3b99..f20dc2e19240 100644 --- a/drivers/pwm/pwm-jz4740.c +++ b/drivers/pwm/pwm-jz4740.c @@ -110,24 +110,56 @@ 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 = pwm_get_chip_data(pwm), *parent_clk = clk_get_parent(clk); - unsigned long rate, period, duty; + unsigned long rate, parent_rate, period, duty; unsigned long long tmp; - unsigned int prescaler = 0; + int ret; - rate = clk_get_rate(parent_clk); - tmp = (unsigned long long)rate * state->period; - do_div(tmp, 1000000000); - period = tmp; + parent_rate = clk_get_rate(parent_clk); + + jz4740_pwm_disable(chip, pwm); - while (period > 0xffff && prescaler < 6) { - period >>= 2; - rate >>= 2; - ++prescaler; + /* Reset the clock to the maximum rate, and we'll reduce it if needed */ + ret = clk_set_max_rate(clk, parent_rate); + if (ret) { + dev_err(chip->dev, "Unable to set max rate: %d\n", ret); + return ret; } - if (prescaler == 6) - return -EINVAL; + ret = clk_set_rate(clk, parent_rate); + if (ret) { + dev_err(chip->dev, "Unable to reset to parent rate (%lu Hz)", + parent_rate); + return ret; + } + + /* + * Limit the clock to a maximum rate that still gives us a period value + * which fits in 16 bits. + */ + tmp = 0xffffull * NSEC_PER_SEC; + do_div(tmp, state->period); + ret = clk_set_max_rate(clk, tmp); + if (ret) { + dev_err(chip->dev, "Unable to set max rate: %d\n", ret); + return ret; + } + + /* + * Read back the clock rate, as it may have been modified by + * clk_set_max_rate() + */ + rate = clk_get_rate(clk); + + if (rate != parent_rate) + dev_dbg(chip->dev, "PWM clock updated to %lu Hz\n", rate); + + /* Calculate period value */ + tmp = (unsigned long long)rate * state->period; + do_div(tmp, NSEC_PER_SEC); + period = (unsigned long)tmp; + + /* Calculate duty value */ tmp = (unsigned long long)period * state->duty_cycle; do_div(tmp, state->period); duty = period - tmp; @@ -135,14 +167,10 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, if (duty >= period) duty = period - 1; - jz4740_pwm_disable(chip, pwm); - /* Set abrupt shutdown */ regmap_update_bits(jz4740->map, TCU_REG_TCSRc(pwm->hwpwm), TCU_TCSR_PWM_SD, TCU_TCSR_PWM_SD); - clk_set_rate(clk, rate); - /* Reset counter to 0 */ regmap_write(jz4740->map, TCU_REG_TCNTc(pwm->hwpwm), 0);