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

[Sayyed, Mubin]

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%).

> 
> > > Another possible glitch here.
> > [Mubin]: Can you please elaborate.
> 
> If you switch polarity (say from normal to inverted) and duty/period you do
> (simplified)
> 
> 	ttc_pwm_disable(priv, pwm); // might yield a falling edge
> 	set polarity                // might yield a raising edge
> 	ttc_pwm_enable(priv, pwm);  // might yield a falling edge
> 	... some calculations
> 	ttc_pwm_disable(priv, pwm); // might yield a raising edge
> 	setup counters
> 	ttc_pwm_enable(priv, pwm);  // might yield a falling edge
> 
> so during apply() the output might toggle several times at a high(?)
> frequency. Depending on what you have connected to the PWM this is bad.
> (A LED might blink, a backlight might flicker, a motor might stutter and enter
> an error state or accelerate for a moment.)

[Mubin]: Agreed, will modify logic to avoid toggling
> 
> > > > +	clksel = ttc_pwm_readl(priv, TTC_CLK_CNTRL_OFFSET);
> > >
> > > TTC_CLK_CNTRL_OFFSET is as parameter for ttc_pwm_ch_readl and
> > > ttc_pwm_readl, is this correct?
> > [Mubin]: Here TTC_CLK_CNTRL_OFFSET is being read only for 0th channel,
> so using ttc_pwm_readl does not impact offsets.
> 
> So which clk is selected (for all channels?) depends on how channel 0's clock
> setting is setup by the bootloader (or bootrom)? Sounds strange.
[Mubin]: I confirmed that each channel can have separate clk source. I will update it for all channels and modify ttc_pwm_priv structure accordingly.

Thanks,
Mubin
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |