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

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 4884 bytes --]

On 18.08.2022 01:14:34, Matej Vasilevski wrote:
> 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?

Oh, I haven't checked if clocks_calc_max_nsecs() is exported. You can
either create a patch to export it, or "open code" its functionality. I
think this should be more or less equivalent:

| work_delay_in_ns = clocksource_cyc2ns(mask, mult, shift) >> 1;

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

Use pm_runtime_resume_and_get(), see:

| https://elixir.bootlin.com/linux/v5.19/source/include/linux/pm_runtime.h#L419

> This
> seemed nicer to me, because the core clock prepare/unprepare will go
> into the pm_runtime callbacks too.

Sound good. If you rely on the runtime PM, please add a "depends on PM"
to the Kconfig. If you want/need to support configurations without
runtime PM, you have to do some extra work:

| https://elixir.bootlin.com/linux/v5.19/source/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c#L1860

In the mcp251xfd driver without runtime PM I enable the clocks and VDD
during probe() and keep them running until remove(). The idea is:

1) call clock_prepare_enable() manually
2) call pm_runtime_get_noresume(), which equal to
   pm_runtime_resume_and_get() but doesn't call the resume function
3) pm_runtime_enable()
4) pm_runtime_put()
   will call suspend with runtime PM enabled,
   will do nothing otherwise

Then use pm_runtime_resume_and_get() during open() and pm_runtime_put()
during stop(). Use both between accessing regs in do_get_berr_counter().

During remove it's a bit simpler:

| https://elixir.bootlin.com/linux/v5.19/source/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c#L1932

> 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()),

Is this a kernel without CONFIG_PM?

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

The API says the clock must be enabled during clk_get_rate() (but that's
not enforced). And another problem is that the clock rate might change,
but let's ignore the clock rate change problem for now.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]