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/ |
