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

[Sean Young]

Hello Uwe,

Thank you for taking the time to explain your thinking.

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

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

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

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

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

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

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

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

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

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

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

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

Thank you for an interesting conversation about this.

 
Sean