Date:   Thu, 18 Aug 2022 18:03:44 +0200
From:   Matej Vasilevski <matej.vasilevski@...nam.cz>
To:     Marc Kleine-Budde <mkl@...gutronix.de>
Cc:     Pavel Pisa <pisa@....felk.cvut.cz>,
        Ondrej Ille <ondrej.ille@...il.com>,
        Wolfgang Grandegger <wg@...ndegger.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-can@...r.kernel.org, netdev@...r.kernel.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCH v2 1/3] can: ctucanfd: add HW timestamps to RX and error
 CAN frames

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@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'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

Powered by blists - more mailing lists