Re: [PATCH RESEND] pwm: berlin: Don't use broken prescaler values
From: Thierry Reding
Date: Wed Jun 06 2018 - 05:44:35 EST
On Tue, Jun 05, 2018 at 12:48:51PM -0400, Tom Hebb wrote:
> On 06/05/2018 05:10 AM, Thierry Reding wrote:
> > On Mon, Jun 04, 2018 at 02:32:41PM -0400, Thomas Hebb wrote:
> >> Six of the eight prescaler values available for Berlin PWM are not true
> >> prescalers but rather internal shifts that throw away the high bits of
> >> TCNT. Currently, we attempt to use those high bits, leading to erratic
> >> behavior. Restrict the prescaler configurations we select to only the
> >> two that respect the full range of TCNT.
> >>
> >> Tested on BG2CD.
> >>
> >> Signed-off-by: Thomas Hebb <tommyhebb@xxxxxxxxx>
> >> ---
> >> drivers/pwm/pwm-berlin.c | 45 ++++++++++++++++++++++------------------
> >> 1 file changed, 25 insertions(+), 20 deletions(-)
> >
> > Antoine, Jisheng,
> >
> > can you guys review this patch? I'm personally on the fence about this,
> > even if we can technically do the shift in software, I don't necessarily
> > see a reason why we can't "offload" to the hardware.
> >
> > Thierry
>
> Sorry if my commit message was unclear: this patch doesn't just
> arbitrarily change the hw/sw division of responsibility. The driver in
> its current state is broken (at least on BG2CD), and this patch
> implements a fix.
>
> The reason the middle six prescaler values are useless is because they
> do not actually slow down the clock. Instead, they emulate slowing down
> the clock by internally multiplying TCNT.
>
> This would be a fine trick, if not for the fact that the internal TCNT
> value has no extra bits beyond the 16 already exposed to software by the
> register. What this means is that, for a prescaler of 4, the software
> must ensure that the top two bits of TCNT are not set, because hardware
> will chop them off; for a prescaler of 8, the top three bits must not be
> set, and so forth. Software does not currently ensure this, resulting in
> a TCNT several orders of magnitude lower than intended any time one of
> those six prescalers are selected.
>
> Because hardware chops off the high bits in its internal shift, the
> middle six prescalers don't actually allow *anything* that the first
> doesn't. In fact, they are strictly worse than the first, since the
> internal shift of TCNT prevents software from setting the low bits,
> decreasing the resolution, without providing any extra high bits.
>
> By skipping the useless prescalers entirely, this patch actually
> increases the driver's performance, since, when the 4096 prescaler is
> selected, it now does only a single shift rather than the seven
> successive divisions it did before.
>
> Let me know if any of this is still unclear, or if you'd like me to
> revise the commit message.
Perfect, the above, with slight adaptations, would make a great commit
message. =)
Thierry
Attachment:
signature.asc
Description: PGP signature