Message-Id: <202205132102.58109.pisa@cmp.felk.cvut.cz>
Date:   Fri, 13 May 2022 21:02:58 +0200
From:   Pavel Pisa <pisa@....felk.cvut.cz>
To:     "Marc Kleine-Budde" <mkl@...gutronix.de>,
        Matej Vasilevski <matej.vasilevski@...nam.cz>,
        ondrej.ille@...il.com, Jiri Novak <jnovak@....cvut.cz>
Cc:     linux-can@...r.kernel.org, devicetree@...r.kernel.org,
        netdev@...r.kernel.org, martin.jerabek01@...il.com
Subject: Re: [RFC PATCH 1/3] can: ctucanfd: add HW timestamps to RX and error CAN frames

Hello Marc,

thanks for the fast feedback.

On Friday 13 of May 2022 13:41:35 Marc Kleine-Budde wrote:
> On 13.05.2022 01:27:05, Matej Vasilevski wrote:
> > This patch adds support for retrieving hardware timestamps to RX and
> > error CAN frames for platform devices. It uses timecounter and
> > cyclecounter structures, because the timestamping counter width depends
> > on the IP core implementation (it might not always be 64-bit).
> > To enable HW timestamps, you have to enable it in the kernel config
> > and provide the following properties in device tree:
>
> Please no Kconfig option. There is a proper interface to enable/disable
> time stamps form the user space. IIRC it's an ioctl. But I think the
> overhead is neglectable here.

thanks for suggestion

> > - ts-used-bits
>
> A property with "width"

agree

> in the name seems to be more common. You 
> probably have to add the "ctu" vendor prefix. BTW: the bindings document
> update should come before changing the driver.

this is RFC and not a final.

In general and long term, I vote and prefer to have number of the
most significant active timestamp bit to be encoded in some
CTU CAN FD IP core info register same as for the number of the Tx
buffers. We will discuss that internally. The the solution is the
same for platform as well as for PCI. But the possible second clock
frequency same as the bitrate clock source should stay to be provided
from platform and some table based on vendor and device ID in the PCI
case. Or at least it is my feeling about the situation.

> > - add second clock phandle to 'clocks' property
> > - create 'clock-names' property and name the second clock 'ts_clk'
> >
> > Alternatively, you can set property 'ts-frequency' directly with
> > the timestamping frequency, instead of setting second clock.
>
> For now, please use a clock property only. If you need ACPI bindings add
> them later.

I would be happy if I would never need to think about ACPI...
or if somebody else does it for us...

> > Signed-off-by: Matej Vasilevski <matej.vasilevski@...nam.cz>
> > ---
> >  drivers/net/can/ctucanfd/Kconfig              |  10 ++
> >  drivers/net/can/ctucanfd/Makefile             |   2 +-
> >  drivers/net/can/ctucanfd/ctucanfd.h           |  25 ++++
> >  drivers/net/can/ctucanfd/ctucanfd_base.c      | 123 +++++++++++++++++-
> >  drivers/net/can/ctucanfd/ctucanfd_timestamp.c | 113 ++++++++++++++++
> >  5 files changed, 267 insertions(+), 6 deletions(-)
> >  create mode 100644 drivers/net/can/ctucanfd/ctucanfd_timestamp.c
> >
> > diff --git a/drivers/net/can/ctucanfd/Kconfig
> > b/drivers/net/can/ctucanfd/Kconfig index 48963efc7f19..d75931525ce7
> > 100644
> > --- a/drivers/net/can/ctucanfd/Kconfig
> > +++ b/drivers/net/can/ctucanfd/Kconfig
> > @@ -32,3 +32,13 @@ config CAN_CTUCANFD_PLATFORM
> >  	  company. FPGA design
> > https://gitlab.fel.cvut.cz/canbus/zynq/zynq-can-sja1000-top. The kit
> > description at the Computer Architectures course pages
> > https://cw.fel.cvut.cz/wiki/courses/b35apo/documentation/mz_apo/start . +
> > +config CAN_CTUCANFD_PLATFORM_ENABLE_HW_TIMESTAMPS
> > +	bool "CTU CAN-FD IP core platform device hardware timestamps"
> > +	depends on CAN_CTUCANFD_PLATFORM
> > +	default n
> > +	help
> > +	  Enables reading hardware timestamps from the IP core for platform
> > +	  devices by default. You will have to provide ts-bit-size and
> > +	  ts-frequency/timestaping clock in device tree for CTU CAN-FD IP
> > cores, +	  see device tree bindings for more details.
>
> Please no Kconfig option, see above.

It is only my feeling, but I would keep driver for one or two releases
with timestamps code really disabled by default and make option visible
only when CONFIG_EXPERIMENTAL is set. This would could allow possible
incompatible changes and settle of the situation on IP core side...
Other options is to keep feature for while out of the tree. But review
by community is really important and I am open to suggestions...

> > diff --git a/drivers/net/can/ctucanfd/Makefile
> > b/drivers/net/can/ctucanfd/Makefile index 8078f1f2c30f..78b7d9830098
> > 100644
> > --- a/drivers/net/can/ctucanfd/Makefile
> > +++ b/drivers/net/can/ctucanfd/Makefile
> > --- /dev/null
> > +++ b/drivers/net/can/ctucanfd/ctucanfd_timestamp.c
> > @@ -0,0 +1,113 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/***********************************************************************
> >******** + *
> > + * CTU CAN FD IP Core
> > + *
> > + * Copyright (C) 2022 Matej Vasilevski <matej.vasilevski@...nam.cz> FEE
> > CTU + *
> > + * Project advisors:
> > + *     Jiri Novak <jnovak@....cvut.cz>
> > + *     Pavel Pisa <pisa@....felk.cvut.cz>
> > + *
> > + * Department of Measurement         (http://meas.fel.cvut.cz/)
> > + * Faculty of Electrical Engineering (http://www.fel.cvut.cz)
> > + * Czech Technical University        (http://www.cvut.cz/)
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
>
> With the SPDX-License-Identifier you can skip this.

OK, Matej Vasilevski started his work on out of the tree code.

Please, model header according to actual net-next CTU CAN FD
files header.


> > +int ctucan_timestamp_init(struct ctucan_priv *priv)
> > +{
> > +	struct cyclecounter *cc = &priv->cc;
> > +
> > +	cc->read = ctucan_timestamp_read;
> > +	cc->mask = CYCLECOUNTER_MASK(priv->timestamp_bit_size);
> > +	cc->shift = 10;
> > +	cc->mult = clocksource_hz2mult(priv->timestamp_freq, cc->shift);
>
> If you frequency and width is not known, it's probably better not to
>
> hard code the shift and use clocks_calc_mult_shift() instead:
> | https://elixir.bootlin.com/linux/v5.17.7/source/kernel/time/clocksource.c
> |#L47

Thanks for the pointer. I have suggested dynamic shift approach used actually
in calculate_and_set_work_delay. May it be it can be replaced by some 
cloksource function as well.

> There's no need to do the above init on open(), especially in your case.
> I know the mcp251xfd does it this way....In your case, as you parse data
> from DT, it's better to do the parsing in probe and directly do all
> needed calculations and fill the struct cyclecounter there.

OK

Best wishes and thanks Matej Vasilevski for the great work and Marc
for the help to get it into the shape,

                Pavel
-- 
                Pavel Pisa
    e-mail:     pisa@....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/
    Open Technologies Research Education and Exchange Services
    https://gitlab.fel.cvut.cz/otrees/org/-/wikis/home

Powered by blists - more mailing lists