Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration
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/ | signature.asc Description: PGP signature
Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration
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
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, );
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
Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration
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 +, 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 +, 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 +, 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
Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration
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 +, 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 +, 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 +, 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
Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration
Hello Sean, On Mon, Dec 07, 2020 at 09:43:20AM +, 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 +, 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 +, 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
Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration
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 +, 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 +, 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
On Sun, Dec 06, 2020 at 02:19:41PM +, 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 +, 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/ | signature.asc Description: PGP signature
Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration
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 +, 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
Hello Sean,
On Sat, Dec 05, 2020 at 05:34:44PM +, 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 +, 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 +, 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
Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration
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 +, 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 +, 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
>
Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration
Hello Sean,
On Fri, Dec 04, 2020 at 11:38:46AM +, 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 +, 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
Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration
Hi, On 04.12.20 at 12:21, Uwe Kleine-König wrote: > > I'd make value an unsigned long long and check for > 0x 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 0x we should use that. Thanks, Lino
Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration
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
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 +, 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 > 0x 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 0x. 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
Hello Sean, On Fri, Dec 04, 2020 at 11:40:36AM +, 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 > 0x 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/ | signature.asc Description: PGP signature
Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration
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 > 0x 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
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 +, 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, .. }, )
>
> 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
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 > 0x 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/ | signature.asc Description: PGP signature
Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration
Hello,
On Fri, Dec 04, 2020 at 08:44:17AM +, 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, .. }, )
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/ |
signature.asc
Description: PGP signature
Re: [PATCH v2] pwm: bcm2835: Support apply function for atomic configuration
On Fri, Dec 04, 2020 at 08:44:17AM +, 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
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
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
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
> ---
> 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/ |
signature.asc
Description: PGP signature
[PATCH v2] pwm: bcm2835: Support apply function for atomic configuration
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
---
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
