[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <DM4PR12MB5938774A495A246EE5557BEF9DC69@DM4PR12MB5938.namprd12.prod.outlook.com>
Date: Tue, 17 Jan 2023 12:55:06 +0000
From: "Sayyed, Mubin" <mubin.sayyed@....com>
To: Uwe Kleine-König
<u.kleine-koenig@...gutronix.de>
CC: "robh+dt@...nel.org" <robh+dt@...nel.org>,
"treding@...dia.com" <treding@...dia.com>,
"linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"git (AMD-Xilinx)" <git@....com>,
"Simek, Michal" <michal.simek@....com>,
"Paladugu, Siva Durga Prasad" <siva.durga.prasad.paladugu@....com>,
"mubin10@...il.com" <mubin10@...il.com>,
"krzk@...nel.org" <krzk@...nel.org>
Subject: RE: [LINUX PATCH 3/3] pwm: pwm-cadence: Add support for TTC PWM
Hi Uwe,
> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
> Sent: Tuesday, January 17, 2023 4:57 PM
> To: Sayyed, Mubin <mubin.sayyed@....com>
> Cc: robh+dt@...nel.org; treding@...dia.com; linux-pwm@...r.kernel.org;
> devicetree@...r.kernel.org; linux-arm-kernel@...ts.infradead.org; linux-
> kernel@...r.kernel.org; git (AMD-Xilinx) <git@....com>; Simek, Michal
> <michal.simek@....com>; Paladugu, Siva Durga Prasad
> <siva.durga.prasad.paladugu@....com>; mubin10@...il.com;
> krzk@...nel.org
> 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/ |
Powered by blists - more mailing lists