Hello Marc,
I have two questions before I send the next patch version, please
bear with me.
On Wed, Aug 03, 2022 at 10:53:03AM +0200, Marc Kleine-Budde wrote:
[...]
> > > > + if (priv->timestamp_possible) {
> > > > + clocks_calc_mult_shift(&priv->cc.mult, &priv->cc.shift, timestamp_freq,
> > > > + NSEC_PER_SEC, CTUCANFD_MAX_WORK_DELAY_SEC);
> > > > + priv->work_delay_jiffies =
> > > > + ctucan_calculate_work_delay(timestamp_bit_size, timestamp_freq);
> > > > + if (priv->work_delay_jiffies == 0)
> > > > + priv->timestamp_possible = false;
> > >
> > > You'll get a higher precision if you take the mask into account, at
> > > least if the counter overflows before CTUCANFD_MAX_WORK_DELAY_SEC:
> > >
> > > maxsec = min(CTUCANFD_MAX_WORK_DELAY_SEC, priv->cc.mask / timestamp_freq);
> > >
> > > clocks_calc_mult_shift(&priv->cc.mult, &priv->cc.shift, timestamp_freq, NSEC_PER_SEC, maxsec);
> > > work_delay_in_ns = clocks_calc_max_nsecs(&priv->cc.mult, &priv->cc.shift, 0, &priv->cc.mask, NULL);
> > >
> > > You can use clocks_calc_max_nsecs() to calculate the work delay.
> >
> > This is a good point, thanks. I'll incorporate it into the patch.
>
> And do this calculation after a clk_prepare_enable(), see other mail to
> Pavel
> | https://lore.kernel.org/all/20220803083718.7bh2edmsorwuv4vu@xxxxxxxxxxxxxx/
1) I can't use clocks_calc_max_nsecs(), because it isn't exported
symbol (and I get modpost error during linking). Is that simply an
oversight on your end or I'm doing something incorrectly?
I've also listed all the exported symbols from /kernel/time, and nothing
really stood out to me as super useful for this patch. So I would
continue using ctucan_calculate_work_delay().
2) Instead of using clk_prepare_enable() manually in probe, I've added
the prepare_enable and disable_unprepare(ts_clk) calls into pm_runtime
suspend and resume callbacks. And I call clk_get_rate(ts_clk) only after
the pm_runtime_enable() and pm_runtime_get_sync() are called. This
seemed nicer to me, because the core clock prepare/unprepare will go
into the pm_runtime callbacks too.
Is that a correct approach, or should I really use the clk_prepare_enable()
and clk_disable_unprepare() "manually" in ctucan_common_probe()/ctucan_timestamp_stop()?
On my Zynq board I don't see the ctucan_resume() callback executed during probe
(after pm_runtime_enable() and pm_runtime_get_sync() are called in _probe()),
but in theory it seems like the correct approach. Xilinx_can driver does this too.
Other drivers (e.g. flexcan, mpc251xfd, rcar) call clk_get_rate() right after
devm_clk_get() in probe, but maybe the situation there is different, I don't
know too much about clocks and pm_runtime yet.
Thanks and best regards,
Matej
