Re: [PATCH v2 1/3] can: ctucanfd: add HW timestamps to RX and error CAN frames

[Matej Vasilevski <matej.vasilevski@seznam.cz>]

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@pengutronix.de/


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