On 26.10.2022 00:22:37, Matej Vasilevski wrote: > On Mon, Oct 24, 2022 at 10:02:38PM +0200, Marc Kleine-Budde wrote: > > On 12.10.2022 08:25:56, Matej Vasilevski wrote: > > > This patch adds support for retrieving hardware timestamps to RX and > > > > Later in the code you set struct ethtool_ts_info::tx_types but the > > driver doesn't set TX timestamps, does it? > > No, it doesn't explicitly. Unless something changed and I don't know about it, > all the drivers using can_put_echo_skb() (includes ctucanfd) now report > software (hardware if available) tx timestamps thanks to Vincent's patch. > > https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/commit/?id=12a18d79dc14c80b358dbd26461614b97f2ea4a6 Yes, since that patch, drivers using can_put_echo_skb() support software TX timestamps. But you have to set the HW timestamp on the TX'ed CAN frame prior to the can_rx_offload_get_echo_skb() call for HW TX timestamps, e.g.: | https://elixir.bootlin.com/linux/v6.0.3/source/drivers/net/can/spi/mcp251xfd/mcp251xfd-tef.c#L112 [...] > > > + /* Setup conversion constants and work delay */ > > > + if (priv->timestamp_possible) { > > > + u64 max_cycles; > > > + u64 work_delay_ns; > > > + u32 maxsec; > > > + > > > + priv->cc.read = ctucan_read_timestamp_cc_wrapper; > > > + priv->cc.mask = CYCLECOUNTER_MASK(timestamp_bit_size); > > > + maxsec = min_t(u32, CTUCANFD_MAX_WORK_DELAY_SEC, > > > + div_u64(priv->cc.mask, timestamp_clk_rate)); > > > + clocks_calc_mult_shift(&priv->cc.mult, &priv->cc.shift, > > > + timestamp_clk_rate, NSEC_PER_SEC, maxsec); > > > + > > > + /* shortened copy of clocks_calc_max_nsecs() */ > > > + max_cycles = div_u64(ULLONG_MAX, priv->cc.mult); > > > + max_cycles = min(max_cycles, priv->cc.mask); > > > + work_delay_ns = clocksource_cyc2ns(max_cycles, priv->cc.mult, > > > + priv->cc.shift) >> 2; > > > > I think we can use cyclecounter_cyc2ns() for this, see: > > > > | https://elixir.bootlin.com/linux/v6.0.3/source/drivers/net/ethernet/ti/cpts.c#L642 > > > > BTW: This is the only networking driver using clocks_calc_mult_shift() > > (so far) :D > > > > I don't really see the benefit at the moment (I have to include > clocksource.h anyway due to the clocks_calc_mult_shift()), but sure, > I'll use cyclecounter_cyc2ns(). > > Fun fact :-D I might look into the cpts.c The benefit is less variance in the kernel tree, use the same pattern to calculate the delay if both register width and frequency are unknown at compile time. [...] > >Regarding the timestamp_clk handling: > > > >If you prepare_enable the timestamp_clk during probe_common() and don't > >disable_unprepare it, it stays on the whole lifetime of the driver. So > >there's no need/reason for the runtime suspend/resume functions. > > > >So either keep the clock powered and remove the suspend/resume functions > >or shut down the clock after probe. > > > >If you want to make things 1000% clean, you can get the timestamp's > >clock rate during open() and re-calculate the mult and shift. The > >background is that the clock rate might change if the clock is not > >enabled (at least that's not guaranteed by the common clock framework). > >Actual HW implementations might differ. > > Hmm, I thought that pm_runtime_put() will eventually run runtime suspend > callback, but now I see that it will run only the idle callback (which > I haven't defined). > I'll remove the runtime suspend/resume callbacks. If your clock stays enabled the whole driver lifetime you can use devm_clk_get_enabled(), devm will take care of the disable & unprepare. regards, 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
