Re: [PATCH v2 1/1] can: esd: add support for esd GmbH PCIe/402 CAN interface family

[Vincent MAILHOL <mailhol.vincent@wanadoo.fr>]

Hi Stefan,

On Tue. 17 Aug 2021 at 07:04, Stefan Mätje <Stefan.Maetje@esd.eu> wrote:
> Am Freitag, den 06.08.2021, 15:31 +0200 schrieb Marc Kleine-Budde:
> > On 30.07.2021 19:38:05, Stefan Mätje wrote:
...
> > This device supports HW timestamping. Please don't roll your own
> > conversion functions. Please make use of the timecounter/cyclecounter
> > API, have a look at the mcp251xfd driver for example:
> >
> > https://elixir.bootlin.com/linux/v5.13/source/drivers/net/can/spi/mcp251xfd/mcp251xfd-timestamp.c#L52
> >
> > The idea is that there is a counter of a certain with (here 32 bit) that
> > has a certain frequency (here: priv->can.clock.freq).
> >
> > >     cc->read = mcp251xfd_timestamp_read;
> > >     cc->mask = CYCLECOUNTER_MASK(32);
> > >     cc->shift = 1;
> > >     cc->mult = clocksource_hz2mult(priv->can.clock.freq, cc->shift);
> >
> > The conversion from the register value to ns in done with:
> > > ns = ((reg & mask) * mult) >> shift;
> > In the above example I'm using a shift of "1" as 1ns is an integer
> > multiple of the used frequency (which is 20 or 40 MHz).
> >
> > To cope with overflows of the cycle counter, read the current timestamp
> > with timecounter_read() with at least the double frequency of the
> > overflows happening (plus some slack). The mcp251xfd driver sets up a
> > worker for this. The mcp251xfd drive does this every 45 seconds, with an
> > overflow happening every 107s.
>
> At the moment I can't see the real benefit of this API. This is because the
> device delivers the HW timestamp as a 64-bit value with a certain frequency
> (atm. 80MHz). This timestamp will wrap after(!) the the result in ns of
> ktime_t.
>
> The other devices with 64-bit native timestamps (like etas_58x, peak_canfd.c
> and kvaser_pciefd.c) also do simple multiplication / division operations on
> the 64-bit HW timestamp
>
> Using the struct cyclecounter to hold the multiplier and divisor in the
> struct acc_ov (instead of the members ts2ns_numerator and ts2ns_denominator)
> would result in such an initialization for a struct cyclecounter cc:
>
> struct cyclecounter cc = {
>         .read = NULL,
>         .mask = CYCLECOUNTER_MASK(64),
>         .shift = 1,
>         .mult = clocksource_hz2mult(ov->timestamp_frequency, cc->shift),/* 25 */
> }
>
> Then in acc_ts2ktime() the function cyclecounter_cyc2ns() could be used like this:
>
> static ktime_t acc_ts2ktime(struct acc_ov *ov, u64 ts)
> {
>         u64 unused_frac;
>         u64 ns;
>
>         ns = cyclecounter_cyc2ns(ov->cc, ts, 0, &unused_frac);
>
>         return ns_to_ktime(ns);
> }
>
> One concluding question. Need the HW timestamps be only in ns (since powerup) or should they also be in relation to the kernel time
> of the startup like it is done in Vincent's etas_58x driver?

In a nutshell, I converted the hardware timestamps to kernel
time (UNIX format) because I like to be able to derive the date
and time from my timestamps. I explained it in more details in
below message:

https://lore.kernel.org/linux-can/CAMZ6RqL+n4tRy-B-W+fzW5B3QV6Bedrko57pU_0TE023Oxw_5w@mail.gmail.com/

Yours sincerely,
Vincent