| Message ID | 1606564926-19555-1-git-send-email-LinoSanfilippo@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] pwm: bcm2835: Support apply function for atomic configuration | expand |
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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.
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
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
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
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
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
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
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
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, };
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(-)