On Thu, Aug 18, 2022 at 11:24:35AM +0200, Marc Kleine-Budde wrote:
> 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'm afraid creating a patch for the export would open another can of worms. I'll
take a barebones version of the function: only the _cyc2ns(), and the max_cycles
computation to avoid overflows for 64-bit mask. It should fit in 3 rows of code.
> > 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:
Yes, I'll have to add PM to Kconfig. Currently the driver defines suspend
and resume sleep callbacks, but PM isn't in KConfig.
I would support only runtime PM, but Pavel Pisa knows more and might disagree.
In such case this write up will be very helpful, thank you.
> | 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?
Fortunately the kernel was configured with CONFIG_PM. But I didn't have
runtime_suspend and runtime_resume callbacks defined, only the "system
sleep" suspend and resume (I wasn't aware of the difference).
After I defined some runtime suspend/resume callbacks, they were executed
as expected.
>
> > 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 |
Thanks, regards
Matej
