Re: [RFC PATCH 1/3] can: ctucanfd: add HW timestamps to RX and error CAN frames

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 7556 bytes --]

On 13.05.2022 21:02:58, Pavel Pisa wrote:
[...]
> > 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.

+1

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

Ack, this is the most straight forward option. ACPI being more
complicated - tough I've never touched it.

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

I see no reason for ACPI at the moment.

> > > Signed-off-by: Matej Vasilevski <matej.vasilevski@seznam.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...

The current Kconfig option only sets if timestamping is enabled by
default or not.

If we now add the TS support including the DT bits, we have to support
the DT bindings, even after the info registers have been added. Once you
have a HW with the info registers and boot a system with TS related DT
information you (or rather the driver) has to decide which information
to use.

> > > 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@seznam.cz> FEE
> > > CTU + *
> > > + * Project advisors:
> > > + *     Jiri Novak <jnovak@fel.cvut.cz>
> > > + *     Pavel Pisa <pisa@cmp.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.

The function clocks_calc_mult_shift() actually calculated the mult and
shift values. It takes frequency and a maxsec argument:

| The @maxsec conversion range argument controls the time frame in
| seconds which must be covered by the runtime conversion with the
| calculated mult and shift factors. This guarantees that no 64bit
| overflow happens when the input value of the conversion is multiplied
| with the calculated mult factor. Larger ranges may reduce the
| conversion accuracy by choosing smaller mult and shift factors.

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

You're welcome. I'm looking forward to use this IP core and driver some
day.

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]