On Mon, Jun 05, 2023 at 11:01:40PM +0200, Heiner Kallweit wrote: > On 05.06.2023 09:11, George Stark wrote: > > On 6/2/23 23:52, Heiner Kallweit wrote: > >> On 02.06.2023 12:32, George Stark wrote: > >>> According to the datasheet, the PWM high and low clock count values > >>> should be set to at least one. Therefore, setting the clock count > >>> register to 0 actually means 1 clock count. > >>> > >>> Signed-off-by: George Stark <GNStark@xxxxxxxxxxxxxx> > >>> Signed-off-by: Dmitry Rokosov <ddrokosov@xxxxxxxxxxxxxx> > >>> --- > >>> This patch is based on currently unmerged patch by Heiner Kallweit > >>> https://lore.kernel.org/linux-amlogic/23fe625e-dc23-4db8-3dce-83167cd3b206@xxxxxxxxx > >>> --- > >>> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c > >>> index 834acd7..57e7d9c 100644 > >>> --- a/drivers/pwm/pwm-meson.c > >>> +++ b/drivers/pwm/pwm-meson.c > >>> @@ -206,6 +206,11 @@ > >>> channel->pre_div = pre_div; > >>> channel->hi = duty_cnt; > >>> channel->lo = cnt - duty_cnt; > >>> + > >>> + if (channel->hi) > >>> + channel->hi--; > >>> + if (channel->lo) > >>> + channel->lo--; > > Hello Heiner > > > > Thanks for review > >> I'm not sure whether we should do this. duty_cnt and cnt are results > >> of an integer division and therefore potentially rounded down. > >> The chip-internal increment may help to compensate such rounding > >> errors, so to say. With the proposed change we may end up with the > >> effective period being shorter than the requested one. > > Although chip-internal increment sometimes may help accidentally > > there are cases when the increment ruins precise calculation in unexpected way. > > > > Here's our experience on meson a113l (meson-a1) with pwm driver based on ccf: > > we need to get pwm period as close as possible to 32768hz. > > config pwm to period 1/32768 = 30517ns, duty 15258n > > How driver calculates hi\lo regs: > > rate = NSEC_PER_SEC * 0xffff / 30517 = ~2147Mhz > > rate = clk_round_rate(rate) clk_round_rate selects fastest parent clock which is 64Mhz in our case then calculating hi\lo at last: period= mul_u64_u64_div_u64(rate, state->period, NSEC_PER_SEC); // 1953 > > duty= mul_u64_u64_div_u64(rate, state->duty_cycle, NSEC_PER_SEC); // 976 > > channel->hi= duty; > > channel->lo= period- duty; > > with the internal increment we'll have real output (1953-976 + 1 + 976 + 1) * 1 / 64Mhz = 32736.57Hz but we should have (1953-976 + 976) * 1 / 64Mhz = 32770.09Hz > > Supposedly, depending on the prior rounding errors, something incrementing, > and sometimes not incrementing may provide the more precise result. > Another source of error is shown your example, the duty cycle isn't 50% > due to the rounding. > Not sure however where there's any use case where such small deviations > would cause problems. Therefore I don't have a strong opinion. My strong opinion here is: .apply should implement the the best round-down setting. That is it should pick the maximal period that isn't bigger than the requested one, and for that period pick the maximal duty_cycle that isn't bigger than the requested one. I have some implementation ideas to allow consumers to want different rounding, but for that to work the .apply functions should all round the same way. > > | And IIRC this should not happen. > > Could you please explain why or point out doc/description where it's stated? > > If so we can add explicit check to prevent such a case > > I think I got this wrong. When checking where I got this information from > I found the following in pwm_apply_state_debug(): > > if (state->enabled && state->period < s2.period) > dev_warn(chip->dev, > ".apply is supposed to round down period (requested: %llu, applied: %llu)\n", > state->period, s2.period); pwm_apply_state_debug() also checks the above sketched rule. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
