[PATCH v2] pwm: bcm2835: Support apply function for atomic configuration

[PATCH v2] pwm: bcm2835: Support apply function for atomic configuration

[Lino Sanfilippo]

Use the newer apply function of pwm_ops instead of config, enable, disable
and set_polarity.

This guarantees atomic changes of the pwm controller configuration. It also
reduces the size of the driver.

This has been tested on a Raspberry PI 4.

v2: Fixed compiler error

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 drivers/pwm/pwm-bcm2835.c | 64 ++++++++++++++++-------------------------------
 1 file changed, 21 insertions(+), 43 deletions(-)

diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c
index 6841dcf..dad7443 100644
--- a/drivers/pwm/pwm-bcm2835.c
+++ b/drivers/pwm/pwm-bcm2835.c
@@ -58,13 +58,14 @@ static void bcm2835_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 	writel(value, pc->base + PWM_CONTROL);
 }
 
-static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
-			      int duty_ns, int period_ns)
+static int bcm2835_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			     const struct pwm_state *state)
 {
+
 	struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
 	unsigned long rate = clk_get_rate(pc->clk);
 	unsigned long scaler;
-	u32 period;
+	u32 value;
 
 	if (!rate) {
 		dev_err(pc->dev, "failed to get clock rate\n");
@@ -72,65 +73,42 @@ static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	}
 
 	scaler = DIV_ROUND_CLOSEST(NSEC_PER_SEC, rate);
-	period = DIV_ROUND_CLOSEST(period_ns, scaler);
+	/* set period */
+	value = DIV_ROUND_CLOSEST_ULL(state->period, scaler);
 
-	if (period < PERIOD_MIN)
+	if (value < PERIOD_MIN)
 		return -EINVAL;
 
-	writel(DIV_ROUND_CLOSEST(duty_ns, scaler),
-	       pc->base + DUTY(pwm->hwpwm));
-	writel(period, pc->base + PERIOD(pwm->hwpwm));
-
-	return 0;
-}
+	writel(value, pc->base + PERIOD(pwm->hwpwm));
 
-static int bcm2835_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
-	u32 value;
+	/* set duty cycle */
+	value = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, scaler);
+	writel(value, pc->base + DUTY(pwm->hwpwm));
 
+	/* set polarity */
 	value = readl(pc->base + PWM_CONTROL);
-	value |= PWM_ENABLE << PWM_CONTROL_SHIFT(pwm->hwpwm);
-	writel(value, pc->base + PWM_CONTROL);
-
-	return 0;
-}
 
-static void bcm2835_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
-	u32 value;
-
-	value = readl(pc->base + PWM_CONTROL);
-	value &= ~(PWM_ENABLE << PWM_CONTROL_SHIFT(pwm->hwpwm));
-	writel(value, pc->base + PWM_CONTROL);
-}
-
-static int bcm2835_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
-				enum pwm_polarity polarity)
-{
-	struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
-	u32 value;
-
-	value = readl(pc->base + PWM_CONTROL);
-
-	if (polarity == PWM_POLARITY_NORMAL)
+	if (state->polarity == PWM_POLARITY_NORMAL)
 		value &= ~(PWM_POLARITY << PWM_CONTROL_SHIFT(pwm->hwpwm));
 	else
 		value |= PWM_POLARITY << PWM_CONTROL_SHIFT(pwm->hwpwm);
 
+	/* enable/disable */
+	if (state->enabled)
+		value |= PWM_ENABLE << PWM_CONTROL_SHIFT(pwm->hwpwm);
+	else
+		value &= ~(PWM_ENABLE << PWM_CONTROL_SHIFT(pwm->hwpwm));
+
 	writel(value, pc->base + PWM_CONTROL);
 
 	return 0;
 }
 
+
 static const struct pwm_ops bcm2835_pwm_ops = {
 	.request = bcm2835_pwm_request,
 	.free = bcm2835_pwm_free,
-	.config = bcm2835_pwm_config,
-	.enable = bcm2835_pwm_enable,
-	.disable = bcm2835_pwm_disable,
-	.set_polarity = bcm2835_set_polarity,
+	.apply = bcm2835_pwm_apply,
 	.owner = THIS_MODULE,
 };
 
-- 
2.7.4

Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2308 bytes --]

On Sat, Nov 28, 2020 at 01:02:06PM +0100, Lino Sanfilippo wrote:
> Use the newer apply function of pwm_ops instead of config, enable, disable
> and set_polarity.
> 
> This guarantees atomic changes of the pwm controller configuration. It also
> reduces the size of the driver.
> 
> This has been tested on a Raspberry PI 4.
> 
> v2: Fixed compiler error

Changelog between review rounds go to below the tripple-dash below.

> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
> ---
>  drivers/pwm/pwm-bcm2835.c | 64 ++++++++++++++++-------------------------------
>  1 file changed, 21 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c
> index 6841dcf..dad7443 100644
> --- a/drivers/pwm/pwm-bcm2835.c
> +++ b/drivers/pwm/pwm-bcm2835.c
> @@ -58,13 +58,14 @@ static void bcm2835_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
>  	writel(value, pc->base + PWM_CONTROL);
>  }
>  
> -static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> -			      int duty_ns, int period_ns)
> +static int bcm2835_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			     const struct pwm_state *state)
>  {
> +
>  	struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
>  	unsigned long rate = clk_get_rate(pc->clk);
>  	unsigned long scaler;
> -	u32 period;
> +	u32 value;
>  
>  	if (!rate) {
>  		dev_err(pc->dev, "failed to get clock rate\n");
> @@ -72,65 +73,42 @@ static int bcm2835_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	}
>  
>  	scaler = DIV_ROUND_CLOSEST(NSEC_PER_SEC, rate);
> -	period = DIV_ROUND_CLOSEST(period_ns, scaler);
> +	/* set period */
> +	value = DIV_ROUND_CLOSEST_ULL(state->period, scaler);

You're storing an unsigned long long (i.e. 64 bits) in an u32. If you
are sure that this won't discard relevant bits, please explain this in a
comment for the cursory reader.

Also note that round_closed is probably wrong, as .apply() is supposed
to round down the period to the next achievable period. (But fixing this
has to do done in a separate patch.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration

[Lino Sanfilippo]

Hi Uwe,

First off, thanks for the review!

On 29.11.20 at 19:10, Uwe Kleine-König wrote:

>
> Changelog between review rounds go to below the tripple-dash below.
>

Right, will do so in the next patch version.

>
> You're storing an unsigned long long (i.e. 64 bits) in an u32. If
> you are sure that this won't discard relevant bits, please explain
> this in a comment for the cursory reader.

What about an extra check then to make sure that the period has not been truncated,
e.g:

	value = DIV_ROUND_CLOSEST_ULL(state->period, scaler);

	/* dont accept a period that is too small or has been truncated */
	if ((value < PERIOD_MIN) ||
	    (value != DIV_ROUND_CLOSEST_ULL(state->period, scaler)))
		return -EINVAL;


> Also note that round_closed is probably wrong, as .apply() is
> supposed to round down the period to the next achievable period. (But
> fixing this has to do done in a separate patch.)

According to commit 11fc4edc4 rounding to the closest integer has been introduced
to improve precision in case that the pwm controller is used by the pwm-ir-tx driver.
I dont know how strong the requirement is to round down the period in apply(), but I
can imagine that this may be a good reason to deviate from this rule.
(CCing Sean Young who introduced DIV_ROUND_CLOSEST)

Regards,
Lino

Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration

[Sean Young]

Hi,

On Fri, Dec 04, 2020 at 12:42:15AM +0100, Lino Sanfilippo wrote:
> > You're storing an unsigned long long (i.e. 64 bits) in an u32. If
> > you are sure that this won't discard relevant bits, please explain
> > this in a comment for the cursory reader.
> 
> What about an extra check then to make sure that the period has not been truncated,
> e.g:
> 
> 	value = DIV_ROUND_CLOSEST_ULL(state->period, scaler);
> 
> 	/* dont accept a period that is too small or has been truncated */
> 	if ((value < PERIOD_MIN) ||
> 	    (value != DIV_ROUND_CLOSEST_ULL(state->period, scaler)))
> 		return -EINVAL;

Rather than doing another 64 bit division which is expensive (esp on 32 bit
kernels), you could assign to u64 and check:

	if (value < PERIOD || value > U32_MAX)
		return -EINVAL;

> > Also note that round_closed is probably wrong, as .apply() is
> > supposed to round down the period to the next achievable period. (But
> > fixing this has to do done in a separate patch.)
> 
> According to commit 11fc4edc4 rounding to the closest integer has been introduced
> to improve precision in case that the pwm controller is used by the pwm-ir-tx driver.
> I dont know how strong the requirement is to round down the period in apply(), but I
> can imagine that this may be a good reason to deviate from this rule.
> (CCing Sean Young who introduced DIV_ROUND_CLOSEST)

There was a problem where the carrier is incorrect for some IR hardware
which uses a carrier of 455kHz. With periods that small, rounding errors
do really matter and rounding down might cause problems.

A policy of rounding down the carrier is not the right thing to do
for pwm-ir-tx, and such a change will probably break pwm-ir-tx in some
edge cases.

Thanks

Sean

Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration

[Sean Young]

On Fri, Dec 04, 2020 at 08:44:17AM +0000, Sean Young wrote:
> On Fri, Dec 04, 2020 at 12:42:15AM +0100, Lino Sanfilippo wrote:
> > According to commit 11fc4edc4 rounding to the closest integer has been introduced
> > to improve precision in case that the pwm controller is used by the pwm-ir-tx driver.
> > I dont know how strong the requirement is to round down the period in apply(), but I
> > can imagine that this may be a good reason to deviate from this rule.
> > (CCing Sean Young who introduced DIV_ROUND_CLOSEST)
> 
> There was a problem where the carrier is incorrect for some IR hardware
> which uses a carrier of 455kHz. With periods that small, rounding errors
> do really matter and rounding down might cause problems.
> 
> A policy of rounding down the carrier is not the right thing to do
> for pwm-ir-tx, and such a change will probably break pwm-ir-tx in some
> edge cases.

Let me rephrase that.

Changing the division to rounding down will exactly revert the fix I made
in commit 11fc4edc483bea8bf0efa0cc726886d2342f6fa6.

So in the case described in that commit, the requested frequency was 455kHz,
but rounding down resulted in a frequency of 476kHz.

That's totally broken and a bad idea.


Sean

Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2916 bytes --]

Hello,

On Fri, Dec 04, 2020 at 08:44:17AM +0000, Sean Young wrote:
> On Fri, Dec 04, 2020 at 12:42:15AM +0100, Lino Sanfilippo wrote:
> > > You're storing an unsigned long long (i.e. 64 bits) in an u32. If
> > > you are sure that this won't discard relevant bits, please explain
> > > this in a comment for the cursory reader.
> > 
> > What about an extra check then to make sure that the period has not been truncated,
> > e.g:
> > 
> > 	value = DIV_ROUND_CLOSEST_ULL(state->period, scaler);
> > 
> > 	/* dont accept a period that is too small or has been truncated */
> > 	if ((value < PERIOD_MIN) ||
> > 	    (value != DIV_ROUND_CLOSEST_ULL(state->period, scaler)))
> > 		return -EINVAL;
> 
> Rather than doing another 64 bit division which is expensive (esp on 32 bit
> kernels), you could assign to u64 and check:
> 
> 	if (value < PERIOD || value > U32_MAX)
> 		return -EINVAL;

Given that value is a u32, value > U32_MAX will never trigger.

Maybe checking period before doing the division is more sensible.

> > > Also note that round_closed is probably wrong, as .apply() is
> > > supposed to round down the period to the next achievable period. (But
> > > fixing this has to do done in a separate patch.)
> > 
> > According to commit 11fc4edc4 rounding to the closest integer has been introduced
> > to improve precision in case that the pwm controller is used by the pwm-ir-tx driver.
> > I dont know how strong the requirement is to round down the period in apply(), but I
> > can imagine that this may be a good reason to deviate from this rule.
> > (CCing Sean Young who introduced DIV_ROUND_CLOSEST)
> 
> There was a problem where the carrier is incorrect for some IR hardware
> which uses a carrier of 455kHz. With periods that small, rounding errors
> do really matter and rounding down might cause problems.
> 
> A policy of rounding down the carrier is not the right thing to do
> for pwm-ir-tx, and such a change will probably break pwm-ir-tx in some
> edge cases.

IMO it's not an option to say: pwm-driver A is used for IR, so A's
.apply uses round-nearest and pwm-driver B is used for $somethingelse,
so B's .apply uses round-down. To be a sensible API pwm_apply_state
should have a fixed behaviour. I consider round-down the sensible
choice (because it is easier to implmement the other options with this)
and for consumers like the IR stuff we need to provide some more
functions to allow it selecting a better suited state. Something like:

	pwm_round_state_nearest(pwm, { .period = 2198, .. }, &state)

which queries the hardwares capabilities and then assigns state.period =
2200 instead of 2100.

Where can I find the affected (consumer) driver?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1050 bytes --]

Hello Lino,

On Fri, Dec 04, 2020 at 12:42:15AM +0100, Lino Sanfilippo wrote:
> On 29.11.20 at 19:10, Uwe Kleine-König wrote:
> > You're storing an unsigned long long (i.e. 64 bits) in an u32. If
> > you are sure that this won't discard relevant bits, please explain
> > this in a comment for the cursory reader.
> 
> What about an extra check then to make sure that the period has not been truncated,
> e.g:
> 
> 	value = DIV_ROUND_CLOSEST_ULL(state->period, scaler);
> 
> 	/* dont accept a period that is too small or has been truncated */
> 	if ((value < PERIOD_MIN) ||
> 	    (value != DIV_ROUND_CLOSEST_ULL(state->period, scaler)))
> 		return -EINVAL;

I'd make value an unsigned long long and check for > 0xffffffff instead
of repeating the (expensive) division. (Hmm, maybe the compiler is smart
enough to not actually repeat it, but still.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration

[Sean Young]

Hi,

On Fri, Dec 04, 2020 at 12:13:26PM +0100, Uwe Kleine-König wrote:
> On Fri, Dec 04, 2020 at 08:44:17AM +0000, Sean Young wrote:
> > On Fri, Dec 04, 2020 at 12:42:15AM +0100, Lino Sanfilippo wrote:
> > > > You're storing an unsigned long long (i.e. 64 bits) in an u32. If
> > > > you are sure that this won't discard relevant bits, please explain
> > > > this in a comment for the cursory reader.
> > > 
> > > What about an extra check then to make sure that the period has not been truncated,
> > > e.g:
> > > 
> > > 	value = DIV_ROUND_CLOSEST_ULL(state->period, scaler);
> > > 
> > > 	/* dont accept a period that is too small or has been truncated */
> > > 	if ((value < PERIOD_MIN) ||
> > > 	    (value != DIV_ROUND_CLOSEST_ULL(state->period, scaler)))
> > > 		return -EINVAL;
> > 
> > Rather than doing another 64 bit division which is expensive (esp on 32 bit
> > kernels), you could assign to u64 and check:
> > 
> > 	if (value < PERIOD_MIN || value > U32_MAX)
> > 		return -EINVAL;
> 
> Given that value is a u32, value > U32_MAX will never trigger.

I meant that value is declared u64 as well ("assign to u64").

> Maybe checking period before doing the division is more sensible.

That could introduce rounding errors, exactly why PERIOD_MIN was introduced.

> > > > Also note that round_closed is probably wrong, as .apply() is
> > > > supposed to round down the period to the next achievable period. (But
> > > > fixing this has to do done in a separate patch.)
> > > 
> > > According to commit 11fc4edc4 rounding to the closest integer has been introduced
> > > to improve precision in case that the pwm controller is used by the pwm-ir-tx driver.
> > > I dont know how strong the requirement is to round down the period in apply(), but I
> > > can imagine that this may be a good reason to deviate from this rule.
> > > (CCing Sean Young who introduced DIV_ROUND_CLOSEST)
> > 
> > There was a problem where the carrier is incorrect for some IR hardware
> > which uses a carrier of 455kHz. With periods that small, rounding errors
> > do really matter and rounding down might cause problems.
> > 
> > A policy of rounding down the carrier is not the right thing to do
> > for pwm-ir-tx, and such a change will probably break pwm-ir-tx in some
> > edge cases.
> 
> IMO it's not an option to say: pwm-driver A is used for IR, so A's
> .apply uses round-nearest and pwm-driver B is used for $somethingelse,
> so B's .apply uses round-down.

I'm not saying that one driver should have one it one way and another driver
another way.

> To be a sensible API pwm_apply_state
> should have a fixed behaviour. I consider round-down the sensible
> choice (because it is easier to implmement the other options with this)

It's not sensible when it's wrong about half the time.

Why is is easier to implement?

> and for consumers like the IR stuff we need to provide some more
> functions to allow it selecting a better suited state. Something like:
> 
> 	pwm_round_state_nearest(pwm, { .period = 2198, .. }, &state)
> 
> which queries the hardwares capabilities and then assigns state.period =
> 2200 instead of 2100.

This is very elaborate and surely not "easier to implement". Why not just
do the right thing in the first place and round-closest?

> Where can I find the affected (consumer) driver?

So there is the pwm-ir-tx driver. The infrared led is directly connected
to the pwm output pin, so that's all there is.

Thanks,

Sean

Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration

[Sean Young]

On Fri, Dec 04, 2020 at 12:21:15PM +0100, Uwe Kleine-König wrote:
> Hello Lino,
> 
> On Fri, Dec 04, 2020 at 12:42:15AM +0100, Lino Sanfilippo wrote:
> > On 29.11.20 at 19:10, Uwe Kleine-König wrote:
> > > You're storing an unsigned long long (i.e. 64 bits) in an u32. If
> > > you are sure that this won't discard relevant bits, please explain
> > > this in a comment for the cursory reader.
> > 
> > What about an extra check then to make sure that the period has not been truncated,
> > e.g:
> > 
> > 	value = DIV_ROUND_CLOSEST_ULL(state->period, scaler);
> > 
> > 	/* dont accept a period that is too small or has been truncated */
> > 	if ((value < PERIOD_MIN) ||
> > 	    (value != DIV_ROUND_CLOSEST_ULL(state->period, scaler)))
> > 		return -EINVAL;
> 
> I'd make value an unsigned long long and check for > 0xffffffff instead
> of repeating the (expensive) division. (Hmm, maybe the compiler is smart
> enough to not actually repeat it, but still.)

I wonder where you got that idea from.


Sean

Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1502 bytes --]

Hello Sean,

On Fri, Dec 04, 2020 at 11:40:36AM +0000, Sean Young wrote:
> On Fri, Dec 04, 2020 at 12:21:15PM +0100, Uwe Kleine-König wrote:
> > On Fri, Dec 04, 2020 at 12:42:15AM +0100, Lino Sanfilippo wrote:
> > > On 29.11.20 at 19:10, Uwe Kleine-König wrote:
> > > > You're storing an unsigned long long (i.e. 64 bits) in an u32. If
> > > > you are sure that this won't discard relevant bits, please explain
> > > > this in a comment for the cursory reader.
> > > 
> > > What about an extra check then to make sure that the period has not been truncated,
> > > e.g:
> > > 
> > > 	value = DIV_ROUND_CLOSEST_ULL(state->period, scaler);
> > > 
> > > 	/* dont accept a period that is too small or has been truncated */
> > > 	if ((value < PERIOD_MIN) ||
> > > 	    (value != DIV_ROUND_CLOSEST_ULL(state->period, scaler)))
> > > 		return -EINVAL;
> > 
> > I'd make value an unsigned long long and check for > 0xffffffff instead
> > of repeating the (expensive) division. (Hmm, maybe the compiler is smart
> > enough to not actually repeat it, but still.)
> 
> I wonder where you got that idea from.

I don't know how to honestly answer your question.
Which idea do you mean? That divisions are expensive? Or that compilers
might be smart? And do you consider it a good idea? Or do you disagree?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration

[Sean Young]

Hi Uwe,

On Fri, Dec 04, 2020 at 10:55:25PM +0100, Uwe Kleine-König wrote:
> On Fri, Dec 04, 2020 at 11:40:36AM +0000, Sean Young wrote:
> > On Fri, Dec 04, 2020 at 12:21:15PM +0100, Uwe Kleine-König wrote:
> > > On Fri, Dec 04, 2020 at 12:42:15AM +0100, Lino Sanfilippo wrote:
> > > > On 29.11.20 at 19:10, Uwe Kleine-König wrote:
> > > > > You're storing an unsigned long long (i.e. 64 bits) in an u32. If
> > > > > you are sure that this won't discard relevant bits, please explain
> > > > > this in a comment for the cursory reader.
> > > > 
> > > > What about an extra check then to make sure that the period has not been truncated,
> > > > e.g:
> > > > 
> > > > 	value = DIV_ROUND_CLOSEST_ULL(state->period, scaler);
> > > > 
> > > > 	/* dont accept a period that is too small or has been truncated */
> > > > 	if ((value < PERIOD_MIN) ||
> > > > 	    (value != DIV_ROUND_CLOSEST_ULL(state->period, scaler)))
> > > > 		return -EINVAL;
> > > 
> > > I'd make value an unsigned long long and check for > 0xffffffff instead
> > > of repeating the (expensive) division. (Hmm, maybe the compiler is smart
> > > enough to not actually repeat it, but still.)
> > 
> > I wonder where you got that idea from.
> 
> I don't know how to honestly answer your question.
> Which idea do you mean? That divisions are expensive? Or that compilers
> might be smart? And do you consider it a good idea? Or do you disagree?

I had already made this exact suggestion -- and you had replied to my
email making that suggestion -- before you emailed this. Granted, I said
u64 and U32_MAX rather than unsigned long long and 0xffffffff.

However, I should not have sent that snotty email. It's irrelevant.

My apologies.


Sean

Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration

[Lino Sanfilippo]

Hi Sean,

On 04.12.20 at 09:44, Sean Young wrote:

>> What about an extra check then to make sure that the period has not been truncated,
>> e.g:
>>
>> 	value = DIV_ROUND_CLOSEST_ULL(state->period, scaler);
>>
>> 	/* dont accept a period that is too small or has been truncated */
>> 	if ((value < PERIOD_MIN) ||
>> 	    (value != DIV_ROUND_CLOSEST_ULL(state->period, scaler)))
>> 		return -EINVAL;
>
> Rather than doing another 64 bit division which is expensive (esp on 32 bit
> kernels), you could assign to u64 and check:
>
> 	if (value < PERIOD || value > U32_MAX)
> 		return -EINVAL;
>

Sound reasonable, I will adjust this.

>
> There was a problem where the carrier is incorrect for some IR hardware
> which uses a carrier of 455kHz. With periods that small, rounding errors
> do really matter and rounding down might cause problems.
>
> A policy of rounding down the carrier is not the right thing to do
> for pwm-ir-tx, and such a change will probably break pwm-ir-tx in some
> edge cases.
>


Thanks for this background information.

Regards,
Lino

Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration

[Lino Sanfilippo]

Hi,

On 04.12.20 at 12:21, Uwe Kleine-König wrote:

>
> I'd make value an unsigned long long and check for > 0xffffffff instead
> of repeating the (expensive) division. (Hmm, maybe the compiler is smart
> enough to not actually repeat it, but still.)
I also prefer unsigned long long over u64 since thats what DIV_ROUND_CLOSEST_ULL returns.
However since we have the constant U32_MAX for 0xffffffff we should use that.

Thanks,
Lino

Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 7317 bytes --]

Hello Sean,

On Fri, Dec 04, 2020 at 11:38:46AM +0000, Sean Young wrote:
> On Fri, Dec 04, 2020 at 12:13:26PM +0100, Uwe Kleine-König wrote:
> > On Fri, Dec 04, 2020 at 08:44:17AM +0000, Sean Young wrote:
> > > On Fri, Dec 04, 2020 at 12:42:15AM +0100, Lino Sanfilippo wrote:
> > > > > You're storing an unsigned long long (i.e. 64 bits) in an u32. If
> > > > > you are sure that this won't discard relevant bits, please explain
> > > > > this in a comment for the cursory reader.
> > > > 
> > > > What about an extra check then to make sure that the period has not been truncated,
> > > > e.g:
> > > > 
> > > > 	value = DIV_ROUND_CLOSEST_ULL(state->period, scaler);
> > > > 
> > > > 	/* dont accept a period that is too small or has been truncated */
> > > > 	if ((value < PERIOD_MIN) ||
> > > > 	    (value != DIV_ROUND_CLOSEST_ULL(state->period, scaler)))
> > > > 		return -EINVAL;
> > > 
> > > Rather than doing another 64 bit division which is expensive (esp on 32 bit
> > > kernels), you could assign to u64 and check:
> > > 
> > > 	if (value < PERIOD_MIN || value > U32_MAX)
> > > 		return -EINVAL;
> > 
> > Given that value is a u32, value > U32_MAX will never trigger.
> 
> I meant that value is declared u64 as well ("assign to u64").
> 
> > Maybe checking period before doing the division is more sensible.
> 
> That could introduce rounding errors, exactly why PERIOD_MIN was introduced.

If done correctly it doesn't introduce rounding errors.

> > > > > Also note that round_closed is probably wrong, as .apply() is
> > > > > supposed to round down the period to the next achievable period. (But
> > > > > fixing this has to do done in a separate patch.)
> > > > 
> > > > According to commit 11fc4edc4 rounding to the closest integer has been introduced
> > > > to improve precision in case that the pwm controller is used by the pwm-ir-tx driver.
> > > > I dont know how strong the requirement is to round down the period in apply(), but I
> > > > can imagine that this may be a good reason to deviate from this rule.
> > > > (CCing Sean Young who introduced DIV_ROUND_CLOSEST)
> > > 
> > > There was a problem where the carrier is incorrect for some IR hardware
> > > which uses a carrier of 455kHz. With periods that small, rounding errors
> > > do really matter and rounding down might cause problems.
> > > 
> > > A policy of rounding down the carrier is not the right thing to do
> > > for pwm-ir-tx, and such a change will probably break pwm-ir-tx in some
> > > edge cases.
> > 
> > IMO it's not an option to say: pwm-driver A is used for IR, so A's
> > .apply uses round-nearest and pwm-driver B is used for $somethingelse,
> > so B's .apply uses round-down.
> 
> I'm not saying that one driver should have one it one way and another driver
> another way.

I read between your lines that you think that round-nearest is the
single best strategy, is that right?

> > To be a sensible API pwm_apply_state
> > should have a fixed behaviour. I consider round-down the sensible
> > choice (because it is easier to implmement the other options with this)
> 
> It's not sensible when it's wrong about half the time.

So round-nearest which is wrong about the other half is better?
If you have two consumer drivers and one requires round-nearest and the
other requires round-down, how would you suggest to implement these two?
Always adapting the low-level driver depending on which consumer is in
use sounds wrong. So I conclude that the expectation about the
implemented rounding behaviour should be the same for all drivers. And
if your consumer happens to require a different strategy you're either
out of luck (bad), or we need to expand the PWM API to make this
possible, probably by implementing a round_state callback that tells the
caller the resulting state if the given state is applied.

> Why is is easier to implement?

If pwm_apply_state (and so pwm_round_state) rounds down, you can achieve
round-nearest (simplified: Ignoring polarity, just looking for period) using:

	lower_state = pwm_round_state(pwm, target_state);
	upper_state = {
		.period = 2 * target_state.period - lower_state.period,
		...
	}
	tmp = pwm_round_state(pwm, upper)

	if tmp.period < target_state.period:
		# tmp == lower_state
		return lower_state

	else while tmp.period > target_state.period:
		upper = tmp;
		tmp.period -= 1
		tmp = pwm_round_state(pwm, tmp)

I admit it is not pretty. But please try to implement it the other way
around (i.e. pwm_round_state rounding to nearest and search for a
setting that yields the biggest period not above target.period without
just trying all steps). I spend a few brain cycles and the corner cases
are harder. (But maybe I'm not smart enough, so please convince me.)

Note that with round-nearest there is another complication: Assume a PWM
that can implement period = 500 µs and period = 1000 µs (and nothing
inbetween). That corresponds to the frequencies 2000 Hz and 1000 Hz.
round_nearest for state with period = 700 µs (corresponding to 1428.5714
Hz) would then pick 500 µs (corresponding to 2000 Hz), right? So is
round-nearest really what you prefer?

> > and for consumers like the IR stuff we need to provide some more
> > functions to allow it selecting a better suited state. Something like:
> > 
> > 	pwm_round_state_nearest(pwm, { .period = 2198, .. }, &state)
> > 
> > which queries the hardwares capabilities and then assigns state.period =
> > 2200 instead of 2100.
> 
> This is very elaborate and surely not "easier to implement". Why not just
> do the right thing in the first place and round-closest?

I looked through the history of drivers/pwm for commits changing the
rounding behaviour. I found:

 - 11fc4edc483 which changes bcm2835 from round-down to round-closest
   (I didn't check but given that the driver divides by the result of a
   division the rounding might not always be round-closest.)
 - 12f9ce4a519 which changes pwm-rockchip from round-down to
   round-closest
   (The motivation described in the commit log is wrong today as
   pwm_get_state() gives the last set value, not the result of the
   lowlevel driver's .get_state callback. Also this problem can be fixed
   with drivers implementing round-down by just letting .get_state round
   up. (Which by the way is how I recommend how to implement it when
   reviewing new drivers.))

Did I miss something?

For a quick (and maybe unreliable) overview:

	$ git grep -l _CLOSEST drivers/pwm/ | wc -l
	15

so we might have 15 drivers that round to nearest and the remaining 40
round down. (I checked a few and didn't find a false diagnose.)

For me this isn't a clear indication that round-nearest is
unconditionally better. What is the fact that convinces you that
round-nearest is better in general?

> > Where can I find the affected (consumer) driver?
> 
> So there is the pwm-ir-tx driver. The infrared led is directly connected
> to the pwm output pin, so that's all there is.

Ah, found it, drivers/media/rc/pwm-ir-tx.c, thanks.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration

[Sean Young]

Hello Uwe,

On Sat, Dec 05, 2020 at 12:28:34AM +0100, Uwe Kleine-König wrote:
> Hello Sean,
> 
> On Fri, Dec 04, 2020 at 11:38:46AM +0000, Sean Young wrote:
> > On Fri, Dec 04, 2020 at 12:13:26PM +0100, Uwe Kleine-König wrote:
> > > On Fri, Dec 04, 2020 at 08:44:17AM +0000, Sean Young wrote:
> > > > On Fri, Dec 04, 2020 at 12:42:15AM +0100, Lino Sanfilippo wrote:
> > > > > > You're storing an unsigned long long (i.e. 64 bits) in an u32. If
> > > > > > you are sure that this won't discard relevant bits, please explain
> > > > > > this in a comment for the cursory reader.
> > > > > 
> > > > > > Also note that round_closed is probably wrong, as .apply() is
> > > > > > supposed to round down the period to the next achievable period. (But
> > > > > > fixing this has to do done in a separate patch.)
> > > > > 
> > > > > According to commit 11fc4edc4 rounding to the closest integer has been introduced
> > > > > to improve precision in case that the pwm controller is used by the pwm-ir-tx driver.
> > > > > I dont know how strong the requirement is to round down the period in apply(), but I
> > > > > can imagine that this may be a good reason to deviate from this rule.
> > > > > (CCing Sean Young who introduced DIV_ROUND_CLOSEST)
> > > > 
> > > > There was a problem where the carrier is incorrect for some IR hardware
> > > > which uses a carrier of 455kHz. With periods that small, rounding errors
> > > > do really matter and rounding down might cause problems.
> > > > 
> > > > A policy of rounding down the carrier is not the right thing to do
> > > > for pwm-ir-tx, and such a change will probably break pwm-ir-tx in some
> > > > edge cases.
> > > 
> > > IMO it's not an option to say: pwm-driver A is used for IR, so A's
> > > .apply uses round-nearest and pwm-driver B is used for $somethingelse,
> > > so B's .apply uses round-down.
> > 
> > I'm not saying that one driver should have one it one way and another driver
> > another way.
> 
> I read between your lines that you think that round-nearest is the
> single best strategy, is that right?

Certain the default strategy. When setting a pwm of period X, I would
expect it set it to the closest period it can match to X. Doing anything
else by default is a surprising API.

What real life uses-cases are there for round down? If you want to round
down, is there any need for round up?

Hypothetically you may want e.g. nearest to 100kHz but absolutely no less
than 100kHz. I don't know when this comes up, it would be interesting to
hear where this is needed.

In fact, I am not sure you can guarantee this; when programming the hardware 
there is some division arithmetic which may do some rounding and you'll end
up with slightly more than 100kHz.

Equally, you way want e.g. nearest 1MHz but absolutely no more than 1MHz.
This would require round-up for the period.

> If you have two consumer drivers and one requires round-nearest and the
> other requires round-down, how would you suggest to implement these two?

So when does really happen?

> Always adapting the low-level driver depending on which consumer is in
> use sounds wrong. So I conclude that the expectation about the
> implemented rounding behaviour should be the same for all drivers.

Agreed.

> And
> if your consumer happens to require a different strategy you're either
> out of luck (bad), or we need to expand the PWM API to make this
> possible, probably by implementing a round_state callback that tells the
> caller the resulting state if the given state is applied.

Agreed.

> > Why is is easier to implement?
> 
> If pwm_apply_state (and so pwm_round_state) rounds down, you can achieve
> round-nearest (simplified: Ignoring polarity, just looking for period) using:
> 
> 	lower_state = pwm_round_state(pwm, target_state);
> 	upper_state = {
> 		.period = 2 * target_state.period - lower_state.period,
> 		...
> 	}
> 	tmp = pwm_round_state(pwm, upper)
> 
> 	if tmp.period < target_state.period:
> 		# tmp == lower_state
> 		return lower_state
> 
> 	else while tmp.period > target_state.period:
> 		upper = tmp;
> 		tmp.period -= 1
> 		tmp = pwm_round_state(pwm, tmp)
> 
> I admit it is not pretty. But please try to implement it the other way
> around (i.e. pwm_round_state rounding to nearest and search for a
> setting that yields the biggest period not above target.period without
> just trying all steps). I spend a few brain cycles and the corner cases
> are harder. (But maybe I'm not smart enough, so please convince me.)

Ok. Does pwm hardware always work on a linear scale?

> Note that with round-nearest there is another complication: Assume a PWM
> that can implement period = 500 µs and period = 1000 µs (and nothing
> inbetween). That corresponds to the frequencies 2000 Hz and 1000 Hz.
> round_nearest for state with period = 700 µs (corresponding to 1428.5714
> Hz) would then pick 500 µs (corresponding to 2000 Hz), right? So is
> round-nearest really what you prefer?

That is an interesting point. So, I guess the question is: do you want the
nearest period or the nearest frequency.

> > > and for consumers like the IR stuff we need to provide some more
> > > functions to allow it selecting a better suited state. Something like:
> > > 
> > > 	pwm_round_state_nearest(pwm, { .period = 2198, .. }, &state)
> > > 
> > > which queries the hardwares capabilities and then assigns state.period =
> > > 2200 instead of 2100.
> > 
> > This is very elaborate and surely not "easier to implement". Why not just
> > do the right thing in the first place and round-closest?
> 
> I looked through the history of drivers/pwm for commits changing the
> rounding behaviour. I found:
> 
>  - 11fc4edc483 which changes bcm2835 from round-down to round-closest
>    (I didn't check but given that the driver divides by the result of a
>    division the rounding might not always be round-closest.)
>  - 12f9ce4a519 which changes pwm-rockchip from round-down to
>    round-closest
>    (The motivation described in the commit log is wrong today as
>    pwm_get_state() gives the last set value, not the result of the
>    lowlevel driver's .get_state callback. Also this problem can be fixed
>    with drivers implementing round-down by just letting .get_state round
>    up. (Which by the way is how I recommend how to implement it when
>    reviewing new drivers.))
> 
> Did I miss something?
> 
> For a quick (and maybe unreliable) overview:
> 
> 	$ git grep -l _CLOSEST drivers/pwm/ | wc -l
> 	15
> 
> so we might have 15 drivers that round to nearest and the remaining 40
> round down. (I checked a few and didn't find a false diagnose.)
> 
> For me this isn't a clear indication that round-nearest is
> unconditionally better.

Just because some drivers don't use DIV_ROUND_CLOSEST() doesn't mean
it was considered by the driver author.

I think some drivers use DIV_ROUND_UP, e.g. pwm-sl28cpld.c.

So there is no concensus between the pwm drivers as to what should be the
default.

> What is the fact that convinces you that
> round-nearest is better in general?

Surely the general use-case is match frequency (or period!) as closely
as possible. What is the use-case for round-period-down and how common
is this? What about round-period-up? Why would you want to do round up/down
at all?


Thanks

Sean

Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 7986 bytes --]

Hello Sean,

On Sat, Dec 05, 2020 at 05:34:44PM +0000, Sean Young wrote:
> On Sat, Dec 05, 2020 at 12:28:34AM +0100, Uwe Kleine-König wrote:
> > On Fri, Dec 04, 2020 at 11:38:46AM +0000, Sean Young wrote:
> > > On Fri, Dec 04, 2020 at 12:13:26PM +0100, Uwe Kleine-König wrote:
> > > > On Fri, Dec 04, 2020 at 08:44:17AM +0000, Sean Young wrote:
> > > > > On Fri, Dec 04, 2020 at 12:42:15AM +0100, Lino Sanfilippo wrote:
> > > > > > > You're storing an unsigned long long (i.e. 64 bits) in an u32. If
> > > > > > > you are sure that this won't discard relevant bits, please explain
> > > > > > > this in a comment for the cursory reader.
> > > > > > 
> > > > > > > Also note that round_closed is probably wrong, as .apply() is
> > > > > > > supposed to round down the period to the next achievable period. (But
> > > > > > > fixing this has to do done in a separate patch.)
> > > > > > 
> > > > > > According to commit 11fc4edc4 rounding to the closest integer has been introduced
> > > > > > to improve precision in case that the pwm controller is used by the pwm-ir-tx driver.
> > > > > > I dont know how strong the requirement is to round down the period in apply(), but I
> > > > > > can imagine that this may be a good reason to deviate from this rule.
> > > > > > (CCing Sean Young who introduced DIV_ROUND_CLOSEST)
> > > > > 
> > > > > There was a problem where the carrier is incorrect for some IR hardware
> > > > > which uses a carrier of 455kHz. With periods that small, rounding errors
> > > > > do really matter and rounding down might cause problems.
> > > > > 
> > > > > A policy of rounding down the carrier is not the right thing to do
> > > > > for pwm-ir-tx, and such a change will probably break pwm-ir-tx in some
> > > > > edge cases.
> > > > 
> > > > IMO it's not an option to say: pwm-driver A is used for IR, so A's
> > > > .apply uses round-nearest and pwm-driver B is used for $somethingelse,
> > > > so B's .apply uses round-down.
> > > 
> > > I'm not saying that one driver should have one it one way and another driver
> > > another way.
> > 
> > I read between your lines that you think that round-nearest is the
> > single best strategy, is that right?
> 
> Certain the default strategy. When setting a pwm of period X, I would
> expect it set it to the closest period it can match to X. Doing anything
> else by default is a surprising API.

This reminds me of a similar discussion about rounding in the clk
framework which is an unsolved problem, too. It's unspecified how
clk_set_rate and clk_round_rate behave. If you want to operate an IP
block you usually have a fixed upper limit for the clock frequency and
you want the clk as fast as possible. If you operate an UART you want
the nearest match (for some definition of near, see below) to match the
baud rate.

> What real life uses-cases are there for round down? If you want to round
> down, is there any need for round up?

The scenario I have in mind is for driving a motor. I have to admit
however that usually the period doesn't matter much and it's the
duty_cycle that defines the motor's speed. So for this case the
conservative behaviour is round-down to not make the motor run faster
than expected.

For other usecases (fan, backlight, LED) exactness typically doesn't
matter that much.

So we could find a compromise: round period to nearest and duty_cycle
down. But I'm convinced this is a bad compromise because it's quite
unintuitive.

> Hypothetically you may want e.g. nearest to 100kHz but absolutely no less
> than 100kHz. I don't know when this comes up, it would be interesting to
> hear where this is needed.

ack.

> > > Why is is easier to implement?
> > 
> > If pwm_apply_state (and so pwm_round_state) rounds down, you can achieve
> > round-nearest (simplified: Ignoring polarity, just looking for period) using:
> > 
> > 	lower_state = pwm_round_state(pwm, target_state);
> > 	upper_state = {
> > 		.period = 2 * target_state.period - lower_state.period,
> > 		...
> > 	}
> > 	tmp = pwm_round_state(pwm, upper)
> > 
> > 	if tmp.period < target_state.period:
> > 		# tmp == lower_state
> > 		return lower_state
> > 
> > 	else while tmp.period > target_state.period:
> > 		upper = tmp;
> > 		tmp.period -= 1
> > 		tmp = pwm_round_state(pwm, tmp)
> > 
> > I admit it is not pretty. But please try to implement it the other way
> > around (i.e. pwm_round_state rounding to nearest and search for a
> > setting that yields the biggest period not above target.period without
> > just trying all steps). I spend a few brain cycles and the corner cases
> > are harder. (But maybe I'm not smart enough, so please convince me.)
> 
> Ok. Does pwm hardware always work on a linear scale?

No. A quite usual setup is that the PWM hardware has a built-in divider.
The details here vary heavily (range of the divider, some can only
divide by powers of two, or by little integer multiples of powers of two
...)

> > Note that with round-nearest there is another complication: Assume a PWM
> > that can implement period = 500 µs and period = 1000 µs (and nothing
> > inbetween). That corresponds to the frequencies 2000 Hz and 1000 Hz.
> > round_nearest for state with period = 700 µs (corresponding to 1428.5714
> > Hz) would then pick 500 µs (corresponding to 2000 Hz), right? So is
> > round-nearest really what you prefer?
> 
> That is an interesting point. So, I guess the question is: do you want the
> nearest period or the nearest frequency.

I think to match a carrier frequency you want to minimize the deviation
in period, not frequency. (That is, if you want to match 1000 Hz, 950 Hz
is worse than 1050 Hz because with 950 Hz it takes little more than 19
periods ((1/1000) / abs(1/950 - 1/1000)) until you have more than one
period difference compared to 1000 Hz while with 1050 Hz it takes nearly
21 periods ((1/1000) / abs(1/1050 - 1/1000)). (So this was a bit of a
trick question because yes, you should prefer round-nearest, but it
nicely shows the complexity of the topic.)

> > For a quick (and maybe unreliable) overview:
> > 
> > 	$ git grep -l _CLOSEST drivers/pwm/ | wc -l
> > 	15
> > 
> > so we might have 15 drivers that round to nearest and the remaining 40
> > round down. (I checked a few and didn't find a false diagnose.)
> > 
> > For me this isn't a clear indication that round-nearest is
> > unconditionally better.
> 
> Just because some drivers don't use DIV_ROUND_CLOSEST() doesn't mean
> it was considered by the driver author.
> 
> I think some drivers use DIV_ROUND_UP, e.g. pwm-sl28cpld.c.

Yes, still the intention there (see the comment above DIV_ROUND_UP) is
to round down the period. And rounding up the prescaler is right because
a bigger divisor yields a lower period.
 
> So there is no concensus between the pwm drivers as to what should be the
> default.

yes. And that's mostly because for a long time nobody cared for
uniformity. Since some time I ensure for new drivers that they implement
round-down, but touching older drivers is difficult because often there
is no contact to someone who can test it, and even if there is someone,
this doesn't mean others don't depend on the current behaviour.

So this is kind of a chicken-and-egg problem. We should provide the
option to consumers to choose their preferred rounding, but adding an
API function with having lowlevel drivers implementing different
behaviour is quite hard.

> > What is the fact that convinces you that
> > round-nearest is better in general?
> 
> Surely the general use-case is match frequency (or period!) as closely
> as possible.

Sounds tempting, but I'm not convinced enough to think this to be
universally right.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration

[Sean Young]

Hello Uwe,

On Sat, Dec 05, 2020 at 08:25:10PM +0100, Uwe Kleine-König wrote:
> On Sat, Dec 05, 2020 at 05:34:44PM +0000, Sean Young wrote:
> > What real life uses-cases are there for round down? If you want to round
> > down, is there any need for round up?
> 
> The scenario I have in mind is for driving a motor. I have to admit
> however that usually the period doesn't matter much and it's the
> duty_cycle that defines the motor's speed. So for this case the
> conservative behaviour is round-down to not make the motor run faster
> than expected.

I am reading here that for driving motors, only the duty cycle matters,
not the period.

> For other usecases (fan, backlight, LED) exactness typically doesn't
> matter that much.

So, the use-cases you have are driving motor, fan, backlight, and led.
And in all these cases the exact Hz does not matter.

The only uses case where the exact Hz does matter is pwm-ir-tx. 

So, I gather there are no use-cases for round-down. Yes, should round-down
be needed, then this is more difficult to implement if the driver always
does a round-closest. But, since there is no reason to have round-down,
this is all academic.

Your policy of forcing new pwm drivers to use round-down is breaking
pwm-ir-tx.

Thanks,

Sean

Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4011 bytes --]

On Sun, Dec 06, 2020 at 02:19:41PM +0000, Sean Young wrote:
> Hello Uwe,
> 
> On Sat, Dec 05, 2020 at 08:25:10PM +0100, Uwe Kleine-König wrote:
> > On Sat, Dec 05, 2020 at 05:34:44PM +0000, Sean Young wrote:
> > > What real life uses-cases are there for round down? If you want to round
> > > down, is there any need for round up?
> > 
> > The scenario I have in mind is for driving a motor. I have to admit
> > however that usually the period doesn't matter much and it's the
> > duty_cycle that defines the motor's speed. So for this case the
> > conservative behaviour is round-down to not make the motor run faster
> > than expected.
> 
> I am reading here that for driving motors, only the duty cycle matters,
> not the period.

There is an upper limit (usually around 1 ms) for the period, but if you
choose 0.1 ms or 0.001 ms doesn't matter much AFAICT.

@Thierry: Do you have further use cases in mind?

> > For other usecases (fan, backlight, LED) exactness typically doesn't
> > matter that much.
> 
> So, the use-cases you have are driving motor, fan, backlight, and led.
> And in all these cases the exact Hz does not matter.
> 
> The only uses case where the exact Hz does matter is pwm-ir-tx. 
> 
> So, I gather there are no use-cases for round-down. Yes, should round-down
> be needed, then this is more difficult to implement if the driver always
> does a round-closest. But, since there is no reason to have round-down,
> this is all academic.
> 
> Your policy of forcing new pwm drivers to use round-down is breaking
> pwm-ir-tx.

So you're indeed suggesting that the "right" rounding strategy for
lowlevel drivers should be:

 - Use the period length closest to the requested period (in doubt round
   down?)
 - With the chosen period length use the biggest duty_cycle not bigger
   than the requested duty_cycle.

While this seems technically fine I think for maintenance this is a
nightmare.

My preference would be to stick to the rounding strategy we used so far
(i.e.:

 - Use the biggest period length not bigger than the requested period
 - With the chosen period length use the biggest duty_cycle not bigger
   than the requested duty_cycle.

) and for pwm-ir-tx add support to the PWM API to still make it possible
(and easy) to select the best setting.

The reasons why I think that this rounding-down strategy is the best
are (in order of importance):

 - It is easier to implement correctly [1]
 - Same rounding method for period and duty cycle
 - most drivers already do this (I think)

The (IMHO nice) result would then mean:

 - All consumers can get the setting they want; and
 - Code in lowlevel drivers is simple and the complexity is in common
   code and so a single place.

And it would also allow the pwm-ir-tx driver to notice if the PWM to be
used can for example only support frequencies under 400 kHz.

Best regards
Uwe

[1] Consider a PWM with a parent frequency of 66 MHz, to select the
    period you can pick an integer divider "div" resulting in the period
    4096 / (pclk * d). So the obvious implementation for round-nearest
    would be:

    	pclk = clk_get_rate(myclk);
	div = DIV_ROUND_CLOSEST(NSEC_PER_SEC * 4096, targetperiod * pclk);

    , right?

    With targetperiod = 2641 ns this picks div = 23 and so a period of
    2698.2872200263505 ns (delta = 57.2872200263505 ns).
    The optimal divider however is div = 24. (implemented period =
    2585.8585858585857 ns, delta = 55.14141414141448 ns)

    For round-down the correct implementation is:

    	pclk = clk_get_rate(myclk);
	div = DIV_ROUND_UP(NSEC_PER_SEC * 4096, targetperiod * pclk);

    Exercise for the reader: Come up with a correct implementation for
    "round-nearest" and compare its complexity to the round-down code.

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration

[Sean Young]

Hello Uwe,

Thank you for taking the time to explain your thinking.

On Mon, Dec 07, 2020 at 09:16:28AM +0100, Uwe Kleine-König wrote:
> On Sun, Dec 06, 2020 at 02:19:41PM +0000, Sean Young wrote:
> > On Sat, Dec 05, 2020 at 08:25:10PM +0100, Uwe Kleine-König wrote:
> > > On Sat, Dec 05, 2020 at 05:34:44PM +0000, Sean Young wrote:
> > > > What real life uses-cases are there for round down? If you want to round
> > > > down, is there any need for round up?
> > > 
> > > The scenario I have in mind is for driving a motor. I have to admit
> > > however that usually the period doesn't matter much and it's the
> > > duty_cycle that defines the motor's speed. So for this case the
> > > conservative behaviour is round-down to not make the motor run faster
> > > than expected.
> > 
> > I am reading here that for driving motors, only the duty cycle matters,
> > not the period.
> 
> There is an upper limit (usually around 1 ms) for the period, but if you
> choose 0.1 ms or 0.001 ms doesn't matter much AFAICT.
> 
> @Thierry: Do you have further use cases in mind?
> 
> > > For other usecases (fan, backlight, LED) exactness typically doesn't
> > > matter that much.
> > 
> > So, the use-cases you have are driving motor, fan, backlight, and led.
> > And in all these cases the exact Hz does not matter.
> > 
> > The only uses case where the exact Hz does matter is pwm-ir-tx. 
> > 
> > So, I gather there are no use-cases for round-down. Yes, should round-down
> > be needed, then this is more difficult to implement if the driver always
> > does a round-closest. But, since there is no reason to have round-down,
> > this is all academic.
> > 
> > Your policy of forcing new pwm drivers to use round-down is breaking
> > pwm-ir-tx.
> 
> So you're indeed suggesting that the "right" rounding strategy for
> lowlevel drivers should be:
> 
>  - Use the period length closest to the requested period (in doubt round
>    down?)
>  - With the chosen period length use the biggest duty_cycle not bigger
>    than the requested duty_cycle.
> 
> While this seems technically fine I think for maintenance this is a
> nightmare.
> 
> My preference would be to stick to the rounding strategy we used so far
> (i.e.:
> 
>  - Use the biggest period length not bigger than the requested period
>  - With the chosen period length use the biggest duty_cycle not bigger
>    than the requested duty_cycle.
> 
> ) and for pwm-ir-tx add support to the PWM API to still make it possible
> (and easy) to select the best setting.
> 
> The reasons why I think that this rounding-down strategy is the best
> are (in order of importance):
> 
>  - It is easier to implement correctly [1]

Yes, you are right. You have given a great example where a simple
DIV_ROUND_CLOSEST() does not give the result you want.

>  - Same rounding method for period and duty cycle
>  - most drivers already do this (I think)
> 
> The (IMHO nice) result would then mean:
> 
>  - All consumers can get the setting they want; and

Once there is a nice pwm api for selecting round-nearest, then yes.

For the uses cases you've given, fan, backlight, and led a round-nearest
is the rounding they would want, I would expect.

>  - Code in lowlevel drivers is simple and the complexity is in common
>    code and so a single place.
> 
> And it would also allow the pwm-ir-tx driver to notice if the PWM to be
> used can for example only support frequencies under 400 kHz.

I doubt pwm-ir-tx cares about this, however it is a nice-to-have. It would
also be nice if the rounding could be used with atomic configuration
as well.

Please let me know when/if this new API exists for pwm so that pwm-ir-tx
can select the right rounding.

> [1] Consider a PWM with a parent frequency of 66 MHz, to select the
>     period you can pick an integer divider "div" resulting in the period
>     4096 / (pclk * d). So the obvious implementation for round-nearest
>     would be:
> 
>     	pclk = clk_get_rate(myclk);
> 	div = DIV_ROUND_CLOSEST(NSEC_PER_SEC * 4096, targetperiod * pclk);

Note NSEC_PER_SEC * 4096 >> 2^32 so this would need to be
DIV_ROUND_CLOSEST_ULL.

>     , right?
> 
>     With targetperiod = 2641 ns this picks div = 23 and so a period of
>     2698.2872200263505 ns (delta = 57.2872200263505 ns).
>     The optimal divider however is div = 24. (implemented period =
>     2585.8585858585857 ns, delta = 55.14141414141448 ns)
> 
>     For round-down the correct implementation is:
> 
>     	pclk = clk_get_rate(myclk);
> 	div = DIV_ROUND_UP(NSEC_PER_SEC * 4096, targetperiod * pclk);
> 
>     Exercise for the reader: Come up with a correct implementation for
>     "round-nearest" and compare its complexity to the round-down code.

To be fair, I haven't been been able to come up with a solution without
control flow.

Thank you for an interesting conversation about this.

 
Sean

Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 5925 bytes --]

Hello Sean,

On Mon, Dec 07, 2020 at 09:43:20AM +0000, Sean Young wrote:
> Thank you for taking the time to explain your thinking.

I'm happy you have an open ear for it. With this I really enjoy spending
the time to find the right arguments and examples.

> On Mon, Dec 07, 2020 at 09:16:28AM +0100, Uwe Kleine-König wrote:
> > On Sun, Dec 06, 2020 at 02:19:41PM +0000, Sean Young wrote:
> > > On Sat, Dec 05, 2020 at 08:25:10PM +0100, Uwe Kleine-König wrote:
> > > > On Sat, Dec 05, 2020 at 05:34:44PM +0000, Sean Young wrote:
> > > > > What real life uses-cases are there for round down? If you want to round
> > > > > down, is there any need for round up?
> > > > 
> > > > The scenario I have in mind is for driving a motor. I have to admit
> > > > however that usually the period doesn't matter much and it's the
> > > > duty_cycle that defines the motor's speed. So for this case the
> > > > conservative behaviour is round-down to not make the motor run faster
> > > > than expected.
> > > 
> > > I am reading here that for driving motors, only the duty cycle matters,
> > > not the period.
> > 
> > There is an upper limit (usually around 1 ms) for the period, but if you
> > choose 0.1 ms or 0.001 ms doesn't matter much AFAICT.
> > 
> > @Thierry: Do you have further use cases in mind?

I asked in the hardware department of the company I work for and they
had another usecase: Motors where for example a 1 ms pulse means "move
forwards" and 2 ms means "move backwards". They had the same idea as I
had: You want to know beforehand the result of a given
pwm_apply_state().

> > > > For other usecases (fan, backlight, LED) exactness typically doesn't
> > > > matter that much.
> > > 
> > > So, the use-cases you have are driving motor, fan, backlight, and led.
> > > And in all these cases the exact Hz does not matter.
> > > 
> > > The only uses case where the exact Hz does matter is pwm-ir-tx. 
> > > 
> > > So, I gather there are no use-cases for round-down. Yes, should round-down
> > > be needed, then this is more difficult to implement if the driver always
> > > does a round-closest. But, since there is no reason to have round-down,
> > > this is all academic.
> > > 
> > > Your policy of forcing new pwm drivers to use round-down is breaking
> > > pwm-ir-tx.
> > 
> > So you're indeed suggesting that the "right" rounding strategy for
> > lowlevel drivers should be:
> > 
> >  - Use the period length closest to the requested period (in doubt round
> >    down?)
> >  - With the chosen period length use the biggest duty_cycle not bigger
> >    than the requested duty_cycle.
> > 
> > While this seems technically fine I think for maintenance this is a
> > nightmare.
> > 
> > My preference would be to stick to the rounding strategy we used so far
> > (i.e.:
> > 
> >  - Use the biggest period length not bigger than the requested period
> >  - With the chosen period length use the biggest duty_cycle not bigger
> >    than the requested duty_cycle.
> > 
> > ) and for pwm-ir-tx add support to the PWM API to still make it possible
> > (and easy) to select the best setting.
> > 
> > The reasons why I think that this rounding-down strategy is the best
> > are (in order of importance):
> > 
> >  - It is easier to implement correctly [1]
> 
> Yes, you are right. You have given a great example where a simple
> DIV_ROUND_CLOSEST() does not give the result you want.
> 
> >  - Same rounding method for period and duty cycle
> >  - most drivers already do this (I think)
> > 
> > The (IMHO nice) result would then mean:
> > 
> >  - All consumers can get the setting they want; and
> 
> Once there is a nice pwm api for selecting round-nearest, then yes.
> 
> For the uses cases you've given, fan, backlight, and led a round-nearest
> is the rounding they would want, I would expect.

maybe, yes. Maybe it is also not important enough to spend the extra
cycles getting round nearest and so sticking to round-down is good
enough.

> >  - Code in lowlevel drivers is simple and the complexity is in common
> >    code and so a single place.
> > 
> > And it would also allow the pwm-ir-tx driver to notice if the PWM to be
> > used can for example only support frequencies under 400 kHz.
> 
> I doubt pwm-ir-tx cares about this, however it is a nice-to-have. It would
> also be nice if the rounding could be used with atomic configuration
> as well.

I cannot follow, you created 11fc4edc483bea8bf0efa0cc726886d2342f6fa6
because 476.2 Mhz was too bad. So you seem to be interested in
deviations and part of the problem is that you don't get feedback about
how your request is fulfilled.

> Please let me know when/if this new API exists for pwm so that pwm-ir-tx
> can select the right rounding.

Given that the bcm2835 driver is quite trivial I would be happy to
create a series that "fixes" the driver to round down and provide a
prototype for pwm_round_nearest for you to test on pwm-ir-tx. A willing
tester and a real use-case were the single two things that stopped me
investing time here.

> > [1] Consider a PWM with a parent frequency of 66 MHz, to select the
> >     period you can pick an integer divider "div" resulting in the period
> >     4096 / (pclk * d). So the obvious implementation for round-nearest
> >     would be:
> > 
> >     	pclk = clk_get_rate(myclk);
> > 	div = DIV_ROUND_CLOSEST(NSEC_PER_SEC * 4096, targetperiod * pclk);
> 
> Note NSEC_PER_SEC * 4096 >> 2^32 so this would need to be
> DIV_ROUND_CLOSEST_ULL.

Yeah, I ignored all these nasty little details like ranges of integers
and the valid range for div etc. for the sake of simplicity.
 
> >     , right?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 8398 bytes --]

On Mon, Dec 07, 2020 at 02:52:09PM +0100, Uwe Kleine-König wrote:
> Hello Sean,
> 
> On Mon, Dec 07, 2020 at 09:43:20AM +0000, Sean Young wrote:
> > Thank you for taking the time to explain your thinking.
> 
> I'm happy you have an open ear for it. With this I really enjoy spending
> the time to find the right arguments and examples.
> 
> > On Mon, Dec 07, 2020 at 09:16:28AM +0100, Uwe Kleine-König wrote:
> > > On Sun, Dec 06, 2020 at 02:19:41PM +0000, Sean Young wrote:
> > > > On Sat, Dec 05, 2020 at 08:25:10PM +0100, Uwe Kleine-König wrote:
> > > > > On Sat, Dec 05, 2020 at 05:34:44PM +0000, Sean Young wrote:
> > > > > > What real life uses-cases are there for round down? If you want to round
> > > > > > down, is there any need for round up?
> > > > > 
> > > > > The scenario I have in mind is for driving a motor. I have to admit
> > > > > however that usually the period doesn't matter much and it's the
> > > > > duty_cycle that defines the motor's speed. So for this case the
> > > > > conservative behaviour is round-down to not make the motor run faster
> > > > > than expected.
> > > > 
> > > > I am reading here that for driving motors, only the duty cycle matters,
> > > > not the period.
> > > 
> > > There is an upper limit (usually around 1 ms) for the period, but if you
> > > choose 0.1 ms or 0.001 ms doesn't matter much AFAICT.
> > > 
> > > @Thierry: Do you have further use cases in mind?
> 
> I asked in the hardware department of the company I work for and they
> had another usecase: Motors where for example a 1 ms pulse means "move
> forwards" and 2 ms means "move backwards". They had the same idea as I
> had: You want to know beforehand the result of a given
> pwm_apply_state().

I've occasionally considered the idea of adding a pwm_check_state() API
that would allow you to pass in a struct pwm_state and get a result as
to whether it can be applied or not. It's never really made much sense
because pwm_apply_state() can already return failure if it can't apply
the state.

However, if we need some way for consumers to be more clever about state
changes, then something like pwm_check_state() might be more useful if,
in addition to just checking the validity/applicability of the state we
can also return the state that would be applied after all the hardware-
specific rounding.

That way the consumer can use it to perform some checks on the resulting
state and submit it if satisfied with the outcome. Alternatively, if it
determines that the state is not viable, it can retry with different
values.

I'm not sure how useful that really is because it makes the usage really
difficult on the consumer side. Perhaps there's no need for this anymore
if the consumer is able to specify the rounding, so perhaps we should
concentrate on that API first.

> > > > > For other usecases (fan, backlight, LED) exactness typically doesn't
> > > > > matter that much.
> > > > 
> > > > So, the use-cases you have are driving motor, fan, backlight, and led.
> > > > And in all these cases the exact Hz does not matter.
> > > > 
> > > > The only uses case where the exact Hz does matter is pwm-ir-tx. 
> > > > 
> > > > So, I gather there are no use-cases for round-down. Yes, should round-down
> > > > be needed, then this is more difficult to implement if the driver always
> > > > does a round-closest. But, since there is no reason to have round-down,
> > > > this is all academic.
> > > > 
> > > > Your policy of forcing new pwm drivers to use round-down is breaking
> > > > pwm-ir-tx.
> > > 
> > > So you're indeed suggesting that the "right" rounding strategy for
> > > lowlevel drivers should be:
> > > 
> > >  - Use the period length closest to the requested period (in doubt round
> > >    down?)
> > >  - With the chosen period length use the biggest duty_cycle not bigger
> > >    than the requested duty_cycle.
> > > 
> > > While this seems technically fine I think for maintenance this is a
> > > nightmare.
> > > 
> > > My preference would be to stick to the rounding strategy we used so far
> > > (i.e.:
> > > 
> > >  - Use the biggest period length not bigger than the requested period
> > >  - With the chosen period length use the biggest duty_cycle not bigger
> > >    than the requested duty_cycle.
> > > 
> > > ) and for pwm-ir-tx add support to the PWM API to still make it possible
> > > (and easy) to select the best setting.
> > > 
> > > The reasons why I think that this rounding-down strategy is the best
> > > are (in order of importance):
> > > 
> > >  - It is easier to implement correctly [1]
> > 
> > Yes, you are right. You have given a great example where a simple
> > DIV_ROUND_CLOSEST() does not give the result you want.
> > 
> > >  - Same rounding method for period and duty cycle
> > >  - most drivers already do this (I think)
> > > 
> > > The (IMHO nice) result would then mean:
> > > 
> > >  - All consumers can get the setting they want; and
> > 
> > Once there is a nice pwm api for selecting round-nearest, then yes.
> > 
> > For the uses cases you've given, fan, backlight, and led a round-nearest
> > is the rounding they would want, I would expect.
> 
> maybe, yes. Maybe it is also not important enough to spend the extra
> cycles getting round nearest and so sticking to round-down is good
> enough.

Yeah, I think in most cases currently the consumer just doesn't care
enough and things happen to just work. Maybe they're not perfect, but
good enough.

One of the reasons I was reluctant to introduce a "default" rounding
behaviour is precisely because it's not clear cut, so in some cases the
default may not be what we really want, such as in the pwm-ir-tx case
here.

> > >  - Code in lowlevel drivers is simple and the complexity is in common
> > >    code and so a single place.
> > > 
> > > And it would also allow the pwm-ir-tx driver to notice if the PWM to be
> > > used can for example only support frequencies under 400 kHz.
> > 
> > I doubt pwm-ir-tx cares about this, however it is a nice-to-have. It would
> > also be nice if the rounding could be used with atomic configuration
> > as well.
> 
> I cannot follow, you created 11fc4edc483bea8bf0efa0cc726886d2342f6fa6
> because 476.2 Mhz was too bad. So you seem to be interested in
> deviations and part of the problem is that you don't get feedback about
> how your request is fulfilled.
> 
> > Please let me know when/if this new API exists for pwm so that pwm-ir-tx
> > can select the right rounding.
> 
> Given that the bcm2835 driver is quite trivial I would be happy to
> create a series that "fixes" the driver to round down and provide a
> prototype for pwm_round_nearest for you to test on pwm-ir-tx. A willing
> tester and a real use-case were the single two things that stopped me
> investing time here.

I'd like to avoid adding a new function for this functionality and
instead add a rounding type field to the PWM state. Also, in doing so we
should be able to keep the status quo for everyone by making the default
rounding behaviour "don't care", which is what basically everyone right
now uses. In specific cases like pwm-ir-tx we can adjust the rounding to
become "nearest".

That said, the rounding behaviour is not something that the API can
guarantee, because if we start rejecting "nearest" requests, we might
end up breaking a bunch of setups that want "nearest" but where the
controller doesn't support it. At the same time I don't want to make it
a prerequisite that all drivers implement all possible rounding
behaviours because it puts a very high burden on the driver writer that
may not need (or have a way of testing) anything other than "nearest",
or "round down", or whatever.

So I think from an API perspective the rounding behaviour would always
have to be a sort of "hint" to the driver to specify what the consumer
wants to use, but it should never fail to apply a state purely on this
rounding behaviour, so returning some state that's the best the driver
can do is better than failing if it doesn't know some mode.

This also ensures that existing drivers will be able to continue to work
the same way they always have, and the new mechanism is merely something
to improve the use-cases where we need more precise control.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration

[Sean Young]

Hi Uwe,

On Mon, Dec 07, 2020 at 02:52:09PM +0100, Uwe Kleine-König wrote:
> On Mon, Dec 07, 2020 at 09:43:20AM +0000, Sean Young wrote:
> > On Mon, Dec 07, 2020 at 09:16:28AM +0100, Uwe Kleine-König wrote:
> > > On Sun, Dec 06, 2020 at 02:19:41PM +0000, Sean Young wrote:
> > > > On Sat, Dec 05, 2020 at 08:25:10PM +0100, Uwe Kleine-König wrote:
> > > > > On Sat, Dec 05, 2020 at 05:34:44PM +0000, Sean Young wrote:
> > > > > > What real life uses-cases are there for round down? If you want to round
> > > > > > down, is there any need for round up?
> > > > > 
> > > > > The scenario I have in mind is for driving a motor. I have to admit
> > > > > however that usually the period doesn't matter much and it's the
> > > > > duty_cycle that defines the motor's speed. So for this case the
> > > > > conservative behaviour is round-down to not make the motor run faster
> > > > > than expected.
> > > > 
> > > > I am reading here that for driving motors, only the duty cycle matters,
> > > > not the period.
> > > 
> > > There is an upper limit (usually around 1 ms) for the period, but if you
> > > choose 0.1 ms or 0.001 ms doesn't matter much AFAICT.
> > > 
> > > @Thierry: Do you have further use cases in mind?
> 
> I asked in the hardware department of the company I work for and they
> had another usecase: Motors where for example a 1 ms pulse means "move
> forwards" and 2 ms means "move backwards". They had the same idea as I
> had: You want to know beforehand the result of a given
> pwm_apply_state().

That sounds good, that would be nice.

> > > > > For other usecases (fan, backlight, LED) exactness typically doesn't
> > > > > matter that much.
> > > > 
> > > > So, the use-cases you have are driving motor, fan, backlight, and led.
> > > > And in all these cases the exact Hz does not matter.
> > > > 
> > > > The only uses case where the exact Hz does matter is pwm-ir-tx. 
> > > > 
> > > > So, I gather there are no use-cases for round-down. Yes, should round-down
> > > > be needed, then this is more difficult to implement if the driver always
> > > > does a round-closest. But, since there is no reason to have round-down,
> > > > this is all academic.
> > > > 
> > > > Your policy of forcing new pwm drivers to use round-down is breaking
> > > > pwm-ir-tx.
> > > 
> > > So you're indeed suggesting that the "right" rounding strategy for
> > > lowlevel drivers should be:
> > > 
> > >  - Use the period length closest to the requested period (in doubt round
> > >    down?)
> > >  - With the chosen period length use the biggest duty_cycle not bigger
> > >    than the requested duty_cycle.
> > > 
> > > While this seems technically fine I think for maintenance this is a
> > > nightmare.
> > > 
> > > My preference would be to stick to the rounding strategy we used so far
> > > (i.e.:
> > > 
> > >  - Use the biggest period length not bigger than the requested period
> > >  - With the chosen period length use the biggest duty_cycle not bigger
> > >    than the requested duty_cycle.
> > > 
> > > ) and for pwm-ir-tx add support to the PWM API to still make it possible
> > > (and easy) to select the best setting.
> > > 
> > > The reasons why I think that this rounding-down strategy is the best
> > > are (in order of importance):
> > > 
> > >  - It is easier to implement correctly [1]
> > 
> > Yes, you are right. You have given a great example where a simple
> > DIV_ROUND_CLOSEST() does not give the result you want.
> > 
> > >  - Same rounding method for period and duty cycle
> > >  - most drivers already do this (I think)
> > > 
> > > The (IMHO nice) result would then mean:
> > > 
> > >  - All consumers can get the setting they want; and
> > 
> > Once there is a nice pwm api for selecting round-nearest, then yes.
> > 
> > For the uses cases you've given, fan, backlight, and led a round-nearest
> > is the rounding they would want, I would expect.
> 
> maybe, yes. Maybe it is also not important enough to spend the extra
> cycles getting round nearest and so sticking to round-down is good
> enough.
> 
> > >  - Code in lowlevel drivers is simple and the complexity is in common
> > >    code and so a single place.
> > > 
> > > And it would also allow the pwm-ir-tx driver to notice if the PWM to be
> > > used can for example only support frequencies under 400 kHz.
> > 
> > I doubt pwm-ir-tx cares about this, however it is a nice-to-have. It would
> > also be nice if the rounding could be used with atomic configuration
> > as well.
> 
> I cannot follow, you created 11fc4edc483bea8bf0efa0cc726886d2342f6fa6
> because 476.2 Mhz was too bad. So you seem to be interested in
> deviations and part of the problem is that you don't get feedback about
> how your request is fulfilled.

Right, that's true.

> > Please let me know when/if this new API exists for pwm so that pwm-ir-tx
> > can select the right rounding.
> 
> Given that the bcm2835 driver is quite trivial I would be happy to
> create a series that "fixes" the driver to round down and provide a
> prototype for pwm_round_nearest for you to test on pwm-ir-tx. A willing
> tester and a real use-case were the single two things that stopped me
> investing time here.

pwm-ir-tx does not just use the bcm2845 driver/rpi. There is the
Firefly ROC-RK3308-CC board which uses pwm-ir-tx with a different pwm
dirver.

Also all you need is a infrared led, and a resistor to stop the led from
burning out, to create your own infrared emitter. So, users can easily
add pwm-ir-tx to their systems.

Having said that I'm happy to test the rpi. I would attach a logic analyser
and check the period.


Sean

Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 7530 bytes --]

Hello Thierry,

On Mon, Dec 07, 2020 at 04:29:36PM +0100, Thierry Reding wrote:
> On Mon, Dec 07, 2020 at 02:52:09PM +0100, Uwe Kleine-König wrote:
> > I asked in the hardware department of the company I work for and they
> > had another usecase: Motors where for example a 1 ms pulse means "move
> > forwards" and 2 ms means "move backwards". They had the same idea as I
> > had: You want to know beforehand the result of a given
> > pwm_apply_state().
> 
> I've occasionally considered the idea of adding a pwm_check_state() API
> that would allow you to pass in a struct pwm_state and get a result as
> to whether it can be applied or not. It's never really made much sense
> because pwm_apply_state() can already return failure if it can't apply
> the state.
> 
> However, if we need some way for consumers to be more clever about state
> changes, then something like pwm_check_state() might be more useful if,
> in addition to just checking the validity/applicability of the state we
> can also return the state that would be applied after all the hardware-
> specific rounding.

You describe exactly the function I had in mind when talking about
pwm_round_state. In my eyes pwm_round_state is the better name, because
it makes it obvious that it modifies the passed pwm_state. The clk
framework also has clk_round_rate with similar semantics.

> I'm not sure how useful that really is because it makes the usage really
> difficult on the consumer side. Perhaps there's no need for this anymore
> if the consumer is able to specify the rounding, so perhaps we should
> concentrate on that API first.

Yeah, I think it will not be very useful to be used directly by
consumers in most cases. The driver's callback can however be used in
helper functions provided by the framework. The pwm-ir-tx driver would
then do:

	struct pwm_state state = {
		.period = carrier_period,
		.duty_cycle = 0,
		...
	};

	ret = pwm_round_nearest(mypwn, &state);
	if (!ret)
		... error handling

and then inspect state to judge if it is good enough and use that.

> One of the reasons I was reluctant to introduce a "default" rounding
> behaviour is precisely because it's not clear cut, so in some cases the
> default may not be what we really want, such as in the pwm-ir-tx case
> here.

I think we can agree that with consumers having different needs we
should be able to give all of them what they need. Preferably in a way
that lowlevel drivers must do only something simple and the main
complexity lives in common framework code.

> > Given that the bcm2835 driver is quite trivial I would be happy to
> > create a series that "fixes" the driver to round down and provide a
> > prototype for pwm_round_nearest for you to test on pwm-ir-tx. A willing
> > tester and a real use-case were the single two things that stopped me
> > investing time here.
> 
> I'd like to avoid adding a new function for this functionality and
> instead add a rounding type field to the PWM state. Also, in doing so we
> should be able to keep the status quo for everyone by making the default
> rounding behaviour "don't care", which is what basically everyone right
> now uses. In specific cases like pwm-ir-tx we can adjust the rounding to
> become "nearest".

And you want to adapt all drivers (maybe on a on-demand base) to
implement "round-down", "round-period-to-nearest-but-duty-down" and all
other demands that my come up in the future? Did you notice how
difficult "round-nearest" is even in the simple example with a single
divider in my mail to Sean earlier today? I don't want this in several
drivers. And note this isn't even workable, consider the servo motor
example where a 1ms pulse means move forward and 2ms pulse means move
backwards. In this case you really want to know before applying the
state that the resulting pulses will be longer than (say) 1.8 ms or
shorter than 1.2 ms. And note that adding a "rounding" member to state
doesn't prevent us touching all drivers. If I request a certain state
with round-nearest and the driver is not aware of rounding it might use
round-down and even applies this. Also the PWM driver should not be free
to say "Ohh, the consumer requested 2ms and rounding up, but 1ms is the
best I can do, so that's what they get". So I might drive my vehicle
into a house and won't even notice before something bad happens.

This convinces me that it's impossible to provide the needed features to
consumers without adding a new callback that queries the HW capabilities
without modifying the output.

> That said, the rounding behaviour is not something that the API can
> guarantee, because if we start rejecting "nearest" requests, we might
> end up breaking a bunch of setups that want "nearest" but where the
> controller doesn't support it.

I cannot follow you. Why do you want to reject nearest requests? There
should always be a single state that is nearest to a given request. If
you request a period length of 2 years the actually returned state might
use a considerably shorter period, but there should be no need to reject
this. (It might only be good if this can be noticed before the state
with the shorter period is put into action.)

For round-down some states are impossible, but round-nearest should be
fine.

> At the same time I don't want to make it
> a prerequisite that all drivers implement all possible rounding
> behaviours because it puts a very high burden on the driver writer that
> may not need (or have a way of testing) anything other than "nearest",
> or "round down", or whatever.

That's why I think all drivers should just implement "round down" and
framework logic can implement the necessary logic to still provide
consumers a "round nearest" or any other necessary rounding strategy.

> So I think from an API perspective the rounding behaviour would always
> have to be a sort of "hint" to the driver to specify what the consumer
> wants to use, but it should never fail to apply a state purely on this
> rounding behaviour, so returning some state that's the best the driver
> can do is better than failing if it doesn't know some mode.

I don't agree. If I want my motor to move forward, I don't want the PWM
driver in use be lax enough to result in a backwards move.

> This also ensures that existing drivers will be able to continue to work
> the same way they always have, and the new mechanism is merely something
> to improve the use-cases where we need more precise control.

My thought to go forward is:

- Introduce a new callback "round_state" or "round_down_state" to make
  it more obvious which behaviour is expected.
- For all drivers implementing this callback, enforce that
  .apply(.round_down_state(somestate)) behaves identically to
  .apply(somestate) and that .round_down_state indeed rounds down.
- Implement a pwm_round_nearest_state() function that only works for
  lowlevel drivers implementing .round_down_state. This way it can rely
  on the above item and so can be implemented without too much hassle.

Also all drivers can just stay as they are and as soon as someone
implements the round_down_state callback the driver becomes more useful.
Until this is the case they just continue to work as they do today, too.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration

[Lino Sanfilippo]

Hi,

On 07.12.20 at 14:52, Uwe Kleine-König wrote:

>
> Given that the bcm2835 driver is quite trivial I would be happy to
> create a series that "fixes" the driver to round down and provide a
> prototype for pwm_round_nearest for you to test on pwm-ir-tx. A willing
> tester and a real use-case were the single two things that stopped me
> investing time here.
>

Should I send a v3 of the .apply() support for the bcm2835 driver before you start
such a rework? The v3 would contain the check against truncation of the period but
keep the round-closest strategy as it is.

Regards,
Lino

Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 909 bytes --]

Hi Lino,

On Tue, Dec 08, 2020 at 01:00:02AM +0100, Lino Sanfilippo wrote:
> On 07.12.20 at 14:52, Uwe Kleine-König wrote:
> > Given that the bcm2835 driver is quite trivial I would be happy to
> > create a series that "fixes" the driver to round down and provide a
> > prototype for pwm_round_nearest for you to test on pwm-ir-tx. A willing
> > tester and a real use-case were the single two things that stopped me
> > investing time here.
> >
> 
> Should I send a v3 of the .apply() support for the bcm2835 driver before you start
> such a rework? The v3 would contain the check against truncation of the period but
> keep the round-closest strategy as it is.

Yes, I had asking for that on my plate for today.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]