| Message ID | 20230602103211.2199283-1-gnstark@sberdevices.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pwm: meson: compute cnt register value in proper way | expand |
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@sberdevices.ru> > Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru> > --- > This patch is based on currently unmerged patch by Heiner Kallweit > https://lore.kernel.org/linux-amlogic/23fe625e-dc23-4db8-3dce-83167cd3b206@gmail.com > --- > 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--; 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. And IIRC this should not happen. > } > > return 0; > @@ -340,7 +345,8 @@ > channel->lo = FIELD_GET(PWM_LOW_MASK, value); > channel->hi = FIELD_GET(PWM_HIGH_MASK, value); > > - state->period = meson_pwm_cnt_to_ns(chip, pwm, channel->lo + channel->hi); > + state->period = meson_pwm_cnt_to_ns(chip, pwm, > + channel->lo + 1 + channel->hi + 1); > state->duty_cycle = meson_pwm_cnt_to_ns(chip, pwm, channel->hi); > Doesn't channel->hi have to be incremented here too? > return 0;
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@sberdevices.ru> >> Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru> >> --- >> This patch is based on currently unmerged patch by Heiner Kallweit >> https://lore.kernel.org/linux-amlogic/23fe625e-dc23-4db8-3dce-83167cd3b206@gmail.com >> --- >> 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 | 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 >> } >> >> return 0; >> @@ -340,7 +345,8 @@ >> channel->lo = FIELD_GET(PWM_LOW_MASK, value); >> channel->hi = FIELD_GET(PWM_HIGH_MASK, value); >> >> - state->period = meson_pwm_cnt_to_ns(chip, pwm, channel->lo + channel->hi); >> + state->period = meson_pwm_cnt_to_ns(chip, pwm, >> + channel->lo + 1 + channel->hi + 1); >> state->duty_cycle = meson_pwm_cnt_to_ns(chip, pwm, channel->hi); >> > Doesn't channel->hi have to be incremented here too? Yes, lost the line. I'll fix it Best regards George >> return 0; >
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@sberdevices.ru> >>> Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru> >>> --- >>> This patch is based on currently unmerged patch by Heiner Kallweit >>> https://lore.kernel.org/linux-amlogic/23fe625e-dc23-4db8-3dce-83167cd3b206@gmail.com >>> --- >>> 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. > | 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); >>> } >>> return 0; >>> @@ -340,7 +345,8 @@ >>> channel->lo = FIELD_GET(PWM_LOW_MASK, value); >>> channel->hi = FIELD_GET(PWM_HIGH_MASK, value); >>> - state->period = meson_pwm_cnt_to_ns(chip, pwm, channel->lo + channel->hi); >>> + state->period = meson_pwm_cnt_to_ns(chip, pwm, >>> + channel->lo + 1 + channel->hi + 1); >>> state->duty_cycle = meson_pwm_cnt_to_ns(chip, pwm, channel->hi); >>> >> Doesn't channel->hi have to be incremented here too? > Yes, lost the line. I'll fix it > > Best regards > George >>> return 0; >> >
Hello, just a small note: This patch wasn't sent to the pwm mailing list and so is missing in our patchwork instance. I didn't follow the discussion, but please (and in your own interest) make sure to Cc: linux-pwm@vger.kernel.org for the next revision (or resend if the conclusion of the discussion is that the patch is fine). Best regards Uwe
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@sberdevices.ru> > >>> Signed-off-by: Dmitry Rokosov <ddrokosov@sberdevices.ru> > >>> --- > >>> This patch is based on currently unmerged patch by Heiner Kallweit > >>> https://lore.kernel.org/linux-amlogic/23fe625e-dc23-4db8-3dce-83167cd3b206@gmail.com > >>> --- > >>> 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
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--; } return 0; @@ -340,7 +345,8 @@ channel->lo = FIELD_GET(PWM_LOW_MASK, value); channel->hi = FIELD_GET(PWM_HIGH_MASK, value); - state->period = meson_pwm_cnt_to_ns(chip, pwm, channel->lo + channel->hi); + state->period = meson_pwm_cnt_to_ns(chip, pwm, + channel->lo + 1 + channel->hi + 1); state->duty_cycle = meson_pwm_cnt_to_ns(chip, pwm, channel->hi); return 0;