[4/7] pwm: jz4740: Improve algorithm of clock calculation
diff mbox series

Message ID 20190809123031.24219-5-paul@crapouillou.net
State New
Headers show
Series
  • pwm: jz4740: Driver update
Related show

Commit Message

Paul Cercueil Aug. 9, 2019, 12:30 p.m. UTC
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(-)

Comments

Uwe Kleine-König Aug. 9, 2019, 5:05 p.m. UTC | #1
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
Paul Cercueil Aug. 9, 2019, 5:14 p.m. UTC | #2
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/  |
Uwe Kleine-König Aug. 12, 2019, 6:15 a.m. UTC | #3
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
Paul Cercueil Aug. 12, 2019, 8:43 p.m. UTC | #4
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/  |
Uwe Kleine-König Aug. 12, 2019, 9:48 p.m. UTC | #5
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.
Paul Cercueil Aug. 12, 2019, 10:25 p.m. UTC | #6
[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/  |
Uwe Kleine-König Aug. 13, 2019, 5:27 a.m. UTC | #7
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
Paul Cercueil Aug. 13, 2019, 11:01 a.m. UTC | #8
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/  |
Uwe Kleine-König Aug. 13, 2019, 12:33 p.m. UTC | #9
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
Paul Cercueil Aug. 13, 2019, 12:47 p.m. UTC | #10
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/  |
Uwe Kleine-König Aug. 13, 2019, 2:09 p.m. UTC | #11
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
Paul Cercueil Aug. 14, 2019, 4:10 p.m. UTC | #12
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/  |
Uwe Kleine-König Aug. 14, 2019, 5:32 p.m. UTC | #13
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

Patch
diff mbox series

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