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

Hi Stefan,

On Tue. 17 Aug 2021 at 07:04, Stefan Mätje [off-list ref] wrote:

Am Freitag, den 06.08.2021, 15:31 +0200 schrieb Marc Kleine-Budde:

quoted

On 30.07.2021 19:38:05, Stefan Mätje wrote:

...

quoted

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).

quoted

    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:

quoted

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/ (local)

Yours sincerely,
Vincent