LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH RESEND] pwm: berlin: Don't use broken prescaler values
@ 2018-06-04 18:32 Thomas Hebb
  2018-06-05  9:10 ` Thierry Reding
  0 siblings, 1 reply; 4+ messages in thread
From: Thomas Hebb @ 2018-06-04 18:32 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: Antoine Tenart, sebastian.hesselbarth, jszhang, Thomas Hebb,
	Thierry Reding, open list:PWM SUBSYSTEM, open list

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@gmail.com>
---
 drivers/pwm/pwm-berlin.c | 45 ++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/drivers/pwm/pwm-berlin.c b/drivers/pwm/pwm-berlin.c
index 771859aca4be..7c8d6a168ceb 100644
--- a/drivers/pwm/pwm-berlin.c
+++ b/drivers/pwm/pwm-berlin.c
@@ -21,8 +21,18 @@
 #define BERLIN_PWM_EN			0x0
 #define  BERLIN_PWM_ENABLE		BIT(0)
 #define BERLIN_PWM_CONTROL		0x4
-#define  BERLIN_PWM_PRESCALE_MASK	0x7
-#define  BERLIN_PWM_PRESCALE_MAX	4096
+/*
+ * The prescaler claims to support 8 different moduli, configured using the
+ * low three bits of PWM_CONTROL. (Sequentially, they are 1, 4, 8, 16, 64,
+ * 256, 1024, and 4096.)  However, the moduli from 4 to 1024 appear to be
+ * implemented by internally shifting TCNT left without adding additional
+ * bits. So, the max TCNT that actually works for a modulus of 4 is 0x3fff;
+ * for 8, 0x1fff; and so on. This means that those moduli are entirely
+ * useless, as we could just do the shift ourselves. The 4096 modulus is
+ * implemented with a real prescaler, so we do use that, but we treat it
+ * as a flag instead of pretending the modulus is actually configurable.
+ */
+#define  BERLIN_PWM_PRESCALE_4096	0x7
 #define  BERLIN_PWM_INVERT_POLARITY	BIT(3)
 #define BERLIN_PWM_DUTY			0x8
 #define BERLIN_PWM_TCNT			0xc
@@ -46,10 +56,6 @@ static inline struct berlin_pwm_chip *to_berlin_pwm_chip(struct pwm_chip *chip)
 	return container_of(chip, struct berlin_pwm_chip, chip);
 }
 
-static const u32 prescaler_table[] = {
-	1, 4, 8, 16, 64, 256, 1024, 4096
-};
-
 static inline u32 berlin_pwm_readl(struct berlin_pwm_chip *chip,
 				   unsigned int channel, unsigned long offset)
 {
@@ -86,33 +92,32 @@ static int berlin_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm_dev,
 			     int duty_ns, int period_ns)
 {
 	struct berlin_pwm_chip *pwm = to_berlin_pwm_chip(chip);
-	unsigned int prescale;
+	bool prescale_4096 = false;
 	u32 value, duty, period;
-	u64 cycles, tmp;
+	u64 cycles;
 
 	cycles = clk_get_rate(pwm->clk);
 	cycles *= period_ns;
 	do_div(cycles, NSEC_PER_SEC);
 
-	for (prescale = 0; prescale < ARRAY_SIZE(prescaler_table); prescale++) {
-		tmp = cycles;
-		do_div(tmp, prescaler_table[prescale]);
+	if (cycles > BERLIN_PWM_MAX_TCNT) {
+		prescale_4096 = true;
+		cycles >>= 12; // Prescaled by 4096
 
-		if (tmp <= BERLIN_PWM_MAX_TCNT)
-			break;
+		if (cycles > BERLIN_PWM_MAX_TCNT)
+			return -ERANGE;
 	}
 
-	if (tmp > BERLIN_PWM_MAX_TCNT)
-		return -ERANGE;
-
-	period = tmp;
-	cycles = tmp * duty_ns;
+	period = cycles;
+	cycles *= duty_ns;
 	do_div(cycles, period_ns);
 	duty = cycles;
 
 	value = berlin_pwm_readl(pwm, pwm_dev->hwpwm, BERLIN_PWM_CONTROL);
-	value &= ~BERLIN_PWM_PRESCALE_MASK;
-	value |= prescale;
+	if (prescale_4096)
+		value |= BERLIN_PWM_PRESCALE_4096;
+	else
+		value &= ~BERLIN_PWM_PRESCALE_4096;
 	berlin_pwm_writel(pwm, pwm_dev->hwpwm, value, BERLIN_PWM_CONTROL);
 
 	berlin_pwm_writel(pwm, pwm_dev->hwpwm, duty, BERLIN_PWM_DUTY);
-- 
2.17.0

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH RESEND] pwm: berlin: Don't use broken prescaler values
  2018-06-04 18:32 [PATCH RESEND] pwm: berlin: Don't use broken prescaler values Thomas Hebb
@ 2018-06-05  9:10 ` Thierry Reding
  2018-06-05 16:48   ` Tom Hebb
  0 siblings, 1 reply; 4+ messages in thread
From: Thierry Reding @ 2018-06-05  9:10 UTC (permalink / raw)
  To: Thomas Hebb
  Cc: linux-arm-kernel, Antoine Tenart, sebastian.hesselbarth, jszhang,
	open list:PWM SUBSYSTEM, open list

[-- Attachment #1: Type: text/plain, Size: 853 bytes --]

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@gmail.com>
> ---
>  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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH RESEND] pwm: berlin: Don't use broken prescaler values
  2018-06-05  9:10 ` Thierry Reding
@ 2018-06-05 16:48   ` Tom Hebb
  2018-06-06  9:44     ` Thierry Reding
  0 siblings, 1 reply; 4+ messages in thread
From: Tom Hebb @ 2018-06-05 16:48 UTC (permalink / raw)
  To: Thierry Reding
  Cc: linux-arm-kernel, Antoine Tenart, sebastian.hesselbarth, jszhang,
	open list:PWM SUBSYSTEM, open list

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@gmail.com>
>> ---
>>  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.

-Tom

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH RESEND] pwm: berlin: Don't use broken prescaler values
  2018-06-05 16:48   ` Tom Hebb
@ 2018-06-06  9:44     ` Thierry Reding
  0 siblings, 0 replies; 4+ messages in thread
From: Thierry Reding @ 2018-06-06  9:44 UTC (permalink / raw)
  To: Tom Hebb
  Cc: linux-arm-kernel, Antoine Tenart, sebastian.hesselbarth, jszhang,
	open list:PWM SUBSYSTEM, open list

[-- Attachment #1: Type: text/plain, Size: 2840 bytes --]

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@gmail.com>
> >> ---
> >>  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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2018-06-06  9:44 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-04 18:32 [PATCH RESEND] pwm: berlin: Don't use broken prescaler values Thomas Hebb
2018-06-05  9:10 ` Thierry Reding
2018-06-05 16:48   ` Tom Hebb
2018-06-06  9:44     ` Thierry Reding

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).