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@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?
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:
signature.asc
Description: PGP signature
