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

[Uwe Kleine-König]

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.

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

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

> > > +	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.

Best regards
Uwe

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

Attachment: signature.asc
Description: PGP signature