Re: [PATCH v5 2/4] can: ctucanfd: add HW timestamps to RX and error CAN frames

[Pavel Pisa <pisa@cmp.felk.cvut.cz>]

Thanks for the work

On Wednesday 12 of October 2022 08:25:56 Matej Vasilevski wrote:
> This patch adds support for retrieving hardware timestamps to RX and
> error CAN frames. It uses timecounter and cyclecounter structures,
> because the timestamping counter width depends on the IP core integration
> (it might not always be 64-bit).
> For platform devices, you should specify "ts" clock in device tree.
> For PCI devices, the timestamping frequency is assumed to be the same
> as bus frequency.
>
> Signed-off-by: Matej Vasilevski <matej.vasilevski@seznam.cz>

Acked-by: Pave Pisa <pisa@cmp.felk.cvut.cz>

It would be great if the code gets in as a basic level for CTU CAN FD
timestamping which we need for CAN latency test project.

In the longer term, it could be usesfull to discuss if rx_filter == HWTSTAMP_FILTER_ALL
and cfg.tx_type == HWTSTAMP_TX_ON should be divided to allow separate timestamping
enable and disable for transmit and receive. Our actual focus is to receive
and Tx is implemented by reading the timestamping counter in the message transmit
done interrupt. There is option (for newer core version) to loop Tx frames
into Rx loop which could allow to enhance precision of Tx timestamps
to 10 ns. But that requires newer IP core and I wait even for some minor changes
to allow identification of looped Tx frames into Rx queue.
Switch to such processing mode will have some overhead etc... So it should
stay configurable and used only when precise Tx timestamp are really required...

When the current timestamping patch is accepted I plan to discuss
use of clk_prepare_enable for the main IP core clocks.
These clocks are AXI bus ones on our FPGA integration so they
has to be up anyway and clk_prepare_enable etc.. does not change
behavior, but I want to make that correct in long term.
I hope/expect that it is not problem to call clk_prepare_enable twice
on same reference when the clocks are the same. As I read the code the
state is counted. If it is a problem then some if has to be put there
when the core and timestamp clock are the same.

Thanks for work and reviews,

                Pavel
-- 
                Pavel Pisa
    phone:      +420 603531357
    e-mail:     pisa@cmp.felk.cvut.cz
    Department of Control Engineering FEE CVUT
    Karlovo namesti 13, 121 35, Prague 2
    university: http://control.fel.cvut.cz/
    personal:   http://cmp.felk.cvut.cz/~pisa
    projects:   https://www.openhub.net/accounts/ppisa
    CAN related:http://canbus.pages.fel.cvut.cz/
    RISC-V education: https://comparch.edu.cvut.cz/
    Open Technologies Research Education and Exchange Services
    https://gitlab.fel.cvut.cz/otrees/org/-/wikis/home