Re: [LINUX PATCH 3/3] pwm: pwm-cadence: Add support for TTC PWM

[Uwe Kleine-König]

On Tue, Jan 17, 2023 at 12:55:06PM +0000, Sayyed, Mubin wrote:
> Hi Uwe,
> 
> > -----Original Message-----
> > From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> > Sent: Tuesday, January 17, 2023 4:57 PM
> > To: Sayyed, Mubin <mubin.sayyed@xxxxxxx>
> > Cc: robh+dt@xxxxxxxxxx; treding@xxxxxxxxxx; linux-pwm@xxxxxxxxxxxxxxx;
> > devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> > kernel@xxxxxxxxxxxxxxx; git (AMD-Xilinx) <git@xxxxxxx>; Simek, Michal
> > <michal.simek@xxxxxxx>; Paladugu, Siva Durga Prasad
> > <siva.durga.prasad.paladugu@xxxxxxx>; mubin10@xxxxxxxxx;
> > krzk@xxxxxxxxxx
> > Subject: Re: [LINUX PATCH 3/3] pwm: pwm-cadence: Add support for TTC
> > PWM
> > 
> > Hello Mubin,
> > 
> > On Tue, Jan 17, 2023 at 09:58:10AM +0000, Sayyed, Mubin wrote:
> > > > On Thu, Jan 12, 2023 at 12:45:26PM +0530, Mubin Sayyed wrote:
> > > > Is there a public manual for the hardware? If yes, please mention a link
> > here.
> > > [Mubin]: I did not find any public manual from cadence. However, details
> > can be found in Zynq-7000/ Zynq UltraScale/Versal ACAP TRM available
> > publicly.
> > 
> > Thenk please add a link to that one.
> > 
> > > > how does the output pin behave between the writes in this function
> > > > (and the others in .apply())?
> > > [Mubin]:  ttc_pwm_apply  is disabling counters before calling this
> > > function, and enabling it back immediately after it.  So, output pin
> > > would be low during configuration.
> > 
> > Please document this behaviour (i.e. "A disabled PWM emits a zero") in a
> > paragraph at the top of the driver in the format that e.g.
> > drivers/pwm/pwm-pxa.c is using. Also please test if it emits a zero or the
> > inactive level, i.e. if the output depends on the polarity setting.
> 
> [Mubin]: will confirm behavior on hardware and document it.
> > 
> > > > > +		rate = clk_get_rate(priv->clk);
> > > > > +
> > > > > +		/* Prevent overflow by limiting to the maximum possible
> > period */
> > > > > +		period_cycles = min_t(u64, state->period, ULONG_MAX *
> > NSEC_PER_SEC);
> > > > > +		period_cycles = DIV_ROUND_CLOSEST(period_cycles * rate,
> > > > > +NSEC_PER_SEC);
> > > >
> > > > DIV_ROUND_CLOSEST isn't suiteable to work with u64. (That's also the
> > > > reason for the build bot report.)
> > > >
> > > > The usual alternative trick here is to do:
> > > >
> > > > 	if (rate > NSEC_PER_SEC)
> > > > 		return -EINVAL;
> > > >
> > > > 	period_cycles = mul_u64_u64_div_u64(state->period, rate,
> > > > NSEC_PER_SEC);
> > > [Mubin]: Initially I tried mul_u64_u64_div_u64, it was impacting
> > > accuracy while generating PWM signal of high frequency(output
> > > frequency above 5 MHZ,  input 100 MHZ ). Usage of DIV_ROUND_CLOSEST
> > > improved accuracy. Can you please suggest better alternative for
> > > improving accuracy.
> > 
> > Unless I'm missing something, you have to adjust your definition of accuracy
> > :-)
> > 
> > If you request (say) a period of 149 ns and the nearest actual values your
> > hardware can provide left and right of that is 140 ns and 150 ns, 140ns is the
> > one to select. That is pick the biggest possible period not bigger than the
> > requested period. And with that pick the biggest possible duty_cycle not
> > bigger than the requested duty_cycle. (i.e. it's a bug to emit period=150ns if
> > 149 was requested.)
> > 
> > There are ideas to implement something like
> > 
> > 	pwm_apply_nearest(...);
> > 
> > but that's still in the idea stage (and will depend on the lowlevel drivers
> > implementing the strategy described above).
> [Mubin]: Thanks for explaining, will switch to mul_u64_u64_div_u64,
> though percentage of deviation would be more for PWM signal of high
> frequency. For example, requested period is 50 ns,  requested duty
> cycle is 49 ns(98%), actual duty cycle set by driver would be 40ns
> (80%).

Note it's a trade-off to let drivers round down and not nearest. The
motivating reasons are:

 - division rounding down is the default in C, so it's a tad easier
 - there are less corner cases
   (There are two strange effects that I'm aware of:
    - Consider for example a hardware that can implement periods of
      length 49.2 ns, 49.4 ns and 50.7 ns (and nothing in-between). If
      you request 50 ns, you get 49.4. .get_state would then tell you
      that the hardware has configured 49 ns. But if you request 49
      ns, you don't get 49.4 ns.
    - Rounding to the nearest time is different to rounding to the
      nearest frequency:
      Consider a hardware than can configure a period of 200 ns (freq:
      5 MHz) and 300 ns (freq: 3.33333 MHz). If you request 249 ns
      (freq: 4.016 MHz) you'd get 200 ns if you round to the nearest
      time, but 300 if you round to the nearest frequency.
   Both problems don't happen with rounding down.)
 - .get_state which is supposed to be the inverse of .apply() needs one
   more distinction of cases.
   (That is, if the nearest integer above or below the actual value
   should be returned. And additionally another arbitraty choice what to
   return for 50.5 ns)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature