* Re: [PATCH v2 1/3] can: ctucanfd: add HW timestamps to RX and error CAN frames
2022-08-01 18:46 ` [PATCH v2 1/3] can: ctucanfd: add HW timestamps to RX and error CAN frames Matej Vasilevski
@ 2022-08-01 20:42 ` Pavel Pisa
2022-08-02 3:43 ` Vincent Mailhol
2022-08-02 9:29 ` Marc Kleine-Budde
2 siblings, 0 replies; 30+ messages in thread
From: Pavel Pisa @ 2022-08-01 20:42 UTC (permalink / raw)
To: Matej Vasilevski
Cc: Ondrej Ille, Wolfgang Grandegger, Marc Kleine-Budde,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, linux-can, netdev, devicetree,
Jiri Novak
Dear Matej Vasilevski,
thanks much for the work. It is important for our plan to provide
solution to run our of box continuous integration and latency
testing tool for linux kernel CAN performance and RT state
reporting for arbitrarily CAN controller by drivers vendors inhouse.
We have discussed on Embedded World option to integrate service
into OSADL.org QA Real-Time farm and work is ongoing.
I have sot two places for minor clean up of the patch.
Sorry, I have overlooked it during internal review unfortunately.
We will be happy if maintainer or other focus their eyeballs
to code to catch our possible other omissions.
I have test code against actual QEMU PCI emulation (it is without
timestamping for now), I try to find time to test against PCIe
CTU CAN FD IP core integration card later. Zynq is tested
by Matej Vasilevski and it is our actual main target for latency
tester system.
On Monday 01 of August 2022 20:46:54 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_clk" 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>
> ---
> drivers/net/can/ctucanfd/Makefile | 2 +-
> drivers/net/can/ctucanfd/ctucanfd.h | 20 ++
> drivers/net/can/ctucanfd/ctucanfd_base.c | 214 +++++++++++++++++-
> drivers/net/can/ctucanfd/ctucanfd_timestamp.c | 87 +++++++
> 4 files changed, 315 insertions(+), 8 deletions(-)
> create mode 100644 drivers/net/can/ctucanfd/ctucanfd_timestamp.c
>
> diff --git a/drivers/net/can/ctucanfd/Makefile
> b/drivers/net/can/ctucanfd/Makefile index 8078f1f2c30f..a36e66f2cea7 100644
> --- a/drivers/net/can/ctucanfd/Makefile
> +++ b/drivers/net/can/ctucanfd/Makefile
> @@ -4,7 +4,7 @@
> #
>
> obj-$(CONFIG_CAN_CTUCANFD) := ctucanfd.o
> -ctucanfd-y := ctucanfd_base.o
> +ctucanfd-y := ctucanfd_base.o ctucanfd_timestamp.o
>
> obj-$(CONFIG_CAN_CTUCANFD_PCI) += ctucanfd_pci.o
> obj-$(CONFIG_CAN_CTUCANFD_PLATFORM) += ctucanfd_platform.o
> diff --git a/drivers/net/can/ctucanfd/ctucanfd.h
> b/drivers/net/can/ctucanfd/ctucanfd.h index 0e9904f6a05d..43d9c73ce244
> 100644
> --- a/drivers/net/can/ctucanfd/ctucanfd.h
> +++ b/drivers/net/can/ctucanfd/ctucanfd.h
> @@ -23,6 +23,10 @@
> #include <linux/netdevice.h>
> #include <linux/can/dev.h>
> #include <linux/list.h>
> +#include <linux/timecounter.h>
> +#include <linux/workqueue.h>
> +
> +#define CTUCANFD_MAX_WORK_DELAY_SEC 86400U /* one day == 24 * 3600 seconds
> */
>
> enum ctu_can_fd_can_registers;
>
> @@ -51,6 +55,15 @@ struct ctucan_priv {
> u32 rxfrm_first_word;
>
> struct list_head peers_on_pdev;
> +
> + struct cyclecounter cc;
> + struct timecounter tc;
> + struct delayed_work timestamp;
> +
> + struct clk *timestamp_clk;
> + u32 work_delay_jiffies;
> + bool timestamp_enabled;
> + bool timestamp_possible;
> };
>
> /**
> @@ -79,4 +92,11 @@ int ctucan_probe_common(struct device *dev, void __iomem
> *addr, int ctucan_suspend(struct device *dev) __maybe_unused;
> int ctucan_resume(struct device *dev) __maybe_unused;
>
> +u64 ctucan_read_timestamp_cc_wrapper(const struct cyclecounter *cc);
> +u64 ctucan_read_timestamp_counter(struct ctucan_priv *priv);
> +u32 ctucan_calculate_work_delay(const u32 timestamp_bit_size, const u32
> timestamp_freq); +void ctucan_skb_set_timestamp(const struct ctucan_priv
> *priv, struct sk_buff *skb, + u64 timestamp);
> +void ctucan_timestamp_init(struct ctucan_priv *priv);
> +void ctucan_timestamp_stop(struct ctucan_priv *priv);
> #endif /*__CTUCANFD__*/
> diff --git a/drivers/net/can/ctucanfd/ctucanfd_base.c
> b/drivers/net/can/ctucanfd/ctucanfd_base.c index 3c18d028bd8c..35b37de51811
> 100644
> --- a/drivers/net/can/ctucanfd/ctucanfd_base.c
> +++ b/drivers/net/can/ctucanfd/ctucanfd_base.c
> @@ -18,6 +18,7 @@
>
> ***************************************************************************
>***/
>
> #include <linux/clk.h>
> +#include <linux/clocksource.h>
> #include <linux/errno.h>
> #include <linux/ethtool.h>
> #include <linux/init.h>
> @@ -148,6 +149,27 @@ static void ctucan_write_txt_buf(struct ctucan_priv
> *priv, enum ctu_can_fd_can_r priv->write_reg(priv, buf_base + offset, val);
> }
>
> +static u64 concatenate_two_u32(u32 high, u32 low)
> +{
> + return ((u64)high << 32) | ((u64)low);
> +}
> +
> +u64 ctucan_read_timestamp_counter(struct ctucan_priv *priv)
> +{
> + u32 ts_low;
> + u32 ts_high;
> + u32 ts_high2;
> +
> + ts_high = ctucan_read32(priv, CTUCANFD_TIMESTAMP_HIGH);
> + ts_low = ctucan_read32(priv, CTUCANFD_TIMESTAMP_LOW);
> + ts_high2 = ctucan_read32(priv, CTUCANFD_TIMESTAMP_HIGH);
> +
> + if (ts_high2 != ts_high)
> + ts_low = priv->read_reg(priv, CTUCANFD_TIMESTAMP_LOW);
> +
> + return concatenate_two_u32(ts_high2, ts_low) & priv->cc.mask;
> +}
> +
> #define CTU_CAN_FD_TXTNF(priv) (!!FIELD_GET(REG_STATUS_TXNF,
> ctucan_read32(priv, CTUCANFD_STATUS))) #define CTU_CAN_FD_ENABLED(priv)
> (!!FIELD_GET(REG_MODE_ENA, ctucan_read32(priv, CTUCANFD_MODE)))
>
> @@ -640,12 +662,16 @@ static netdev_tx_t ctucan_start_xmit(struct sk_buff
> *skb, struct net_device *nde * @priv: Pointer to CTU CAN FD's private data
> * @cf: Pointer to CAN frame struct
> * @ffw: Previously read frame format word
> + * @skb: Pointer to buffer to store timestamp
> *
> * Note: Frame format word must be read separately and provided in 'ffw'.
> */
> -static void ctucan_read_rx_frame(struct ctucan_priv *priv, struct
> canfd_frame *cf, u32 ffw) +static void ctucan_read_rx_frame(struct
> ctucan_priv *priv, struct canfd_frame *cf, + u32 ffw, u64 *timestamp)
> {
> u32 idw;
> + u32 tstamp_high;
> + u32 tstamp_low;
> unsigned int i;
> unsigned int wc;
> unsigned int len;
> @@ -682,9 +708,10 @@ static void ctucan_read_rx_frame(struct ctucan_priv
> *priv, struct canfd_frame *c if (unlikely(len > wc * 4))
> len = wc * 4;
>
> - /* Timestamp - Read and throw away */
> - ctucan_read32(priv, CTUCANFD_RX_DATA);
> - ctucan_read32(priv, CTUCANFD_RX_DATA);
> + /* Timestamp */
> + tstamp_low = ctucan_read32(priv, CTUCANFD_RX_DATA);
> + tstamp_high = ctucan_read32(priv, CTUCANFD_RX_DATA);
> + *timestamp = concatenate_two_u32(tstamp_high, tstamp_low) &
> priv->cc.mask;
>
> /* Data */
> for (i = 0; i < len; i += 4) {
> @@ -713,6 +740,7 @@ static int ctucan_rx(struct net_device *ndev)
> struct net_device_stats *stats = &ndev->stats;
> struct canfd_frame *cf;
> struct sk_buff *skb;
> + u64 timestamp;
> u32 ffw;
>
> if (test_bit(CTUCANFD_FLAG_RX_FFW_BUFFERED, &priv->drv_flags)) {
> @@ -736,7 +764,9 @@ static int ctucan_rx(struct net_device *ndev)
> return 0;
> }
>
> - ctucan_read_rx_frame(priv, cf, ffw);
> + ctucan_read_rx_frame(priv, cf, ffw, ×tamp);
> + if (priv->timestamp_enabled)
> + ctucan_skb_set_timestamp(priv, skb, timestamp);
>
> stats->rx_bytes += cf->len;
> stats->rx_packets++;
> @@ -906,6 +936,11 @@ static void ctucan_err_interrupt(struct net_device
> *ndev, u32 isr) if (skb) {
> stats->rx_packets++;
> stats->rx_bytes += cf->can_dlc;
> + if (priv->timestamp_enabled) {
> + u64 tstamp = ctucan_read_timestamp_counter(priv);
> +
> + ctucan_skb_set_timestamp(priv, skb, tstamp);
> + }
> netif_rx(skb);
> }
> }
> @@ -951,6 +986,11 @@ static int ctucan_rx_poll(struct napi_struct *napi,
> int quota) cf->data[1] |= CAN_ERR_CRTL_RX_OVERFLOW;
> stats->rx_packets++;
> stats->rx_bytes += cf->can_dlc;
> + if (priv->timestamp_enabled) {
> + u64 tstamp = ctucan_read_timestamp_counter(priv);
> +
> + ctucan_skb_set_timestamp(priv, skb, tstamp);
> + }
> netif_rx(skb);
> }
>
> @@ -1231,6 +1271,9 @@ static int ctucan_open(struct net_device *ndev)
> goto err_chip_start;
> }
>
> + if (priv->timestamp_possible)
> + ctucan_timestamp_init(priv);
> +
> netdev_info(ndev, "ctu_can_fd device registered\n");
> napi_enable(&priv->napi);
> netif_start_queue(ndev);
> @@ -1263,6 +1306,9 @@ static int ctucan_close(struct net_device *ndev)
> ctucan_chip_stop(ndev);
> free_irq(ndev->irq, ndev);
> close_candev(ndev);
> + if (priv->timestamp_possible)
> + ctucan_timestamp_stop(priv);
> +
>
> pm_runtime_put(priv->dev);
>
> @@ -1295,15 +1341,117 @@ static int ctucan_get_berr_counter(const struct
> net_device *ndev, struct can_ber return 0;
> }
>
> +static int ctucan_hwtstamp_set(struct net_device *dev, struct ifreq *ifr)
> +{
> + struct ctucan_priv *priv = netdev_priv(dev);
> + struct hwtstamp_config cfg;
> +
> + if (!priv->timestamp_possible)
> + return -EOPNOTSUPP;
> +
> + if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
> + return -EFAULT;
> +
> + if (cfg.flags)
> + return -EINVAL;
> +
> + if (cfg.tx_type != HWTSTAMP_TX_OFF)
> + return -ERANGE;
> +
> + switch (cfg.rx_filter) {
> + case HWTSTAMP_FILTER_NONE:
> + priv->timestamp_enabled = false;
> + break;
> + case HWTSTAMP_FILTER_ALL:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V2_EVENT:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V2_SYNC:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
> + priv->timestamp_enabled = true;
> + cfg.rx_filter = HWTSTAMP_FILTER_ALL;
> + break;
> + default:
> + return -ERANGE;
> + }
> +
> + return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
> +}
> +
> +static int ctucan_hwtstamp_get(struct net_device *dev, struct ifreq *ifr)
> +{
> + struct ctucan_priv *priv = netdev_priv(dev);
> + struct hwtstamp_config cfg;
> +
> + if (!priv->timestamp_possible)
> + return -EOPNOTSUPP;
> +
> + cfg.flags = 0;
> + cfg.tx_type = HWTSTAMP_TX_OFF;
> + cfg.rx_filter = priv->timestamp_enabled ? HWTSTAMP_FILTER_ALL :
> HWTSTAMP_FILTER_NONE; +
> + return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
> +}
> +
> +static int ctucan_ioctl(struct net_device *dev, struct ifreq *ifr, int
> cmd) +{
> + switch (cmd) {
> + case SIOCSHWTSTAMP:
> + return ctucan_hwtstamp_set(dev, ifr);
> + case SIOCGHWTSTAMP:
> + return ctucan_hwtstamp_get(dev, ifr);
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static int ctucan_ethtool_get_ts_info(struct net_device *ndev, struct
> ethtool_ts_info *info) +{
> + struct ctucan_priv *priv = netdev_priv(ndev);
> +
> + ethtool_op_get_ts_info(ndev, info);
> +
> + if (!priv->timestamp_possible)
> + return 0;
> +
> + info->so_timestamping |= SOF_TIMESTAMPING_RX_HARDWARE |
> + SOF_TIMESTAMPING_RAW_HARDWARE;
> + info->tx_types = BIT(HWTSTAMP_TX_OFF);
> + info->rx_filters = BIT(HWTSTAMP_FILTER_NONE) |
> + BIT(HWTSTAMP_FILTER_ALL);
> +
> + return 0;
> +}
> +
> static const struct net_device_ops ctucan_netdev_ops = {
> .ndo_open = ctucan_open,
> .ndo_stop = ctucan_close,
> .ndo_start_xmit = ctucan_start_xmit,
> .ndo_change_mtu = can_change_mtu,
> + .ndo_eth_ioctl = ctucan_ioctl,
> };
>
> static const struct ethtool_ops ctucan_ethtool_ops = {
> - .get_ts_info = ethtool_op_get_ts_info,
> + .get_ts_info = ctucan_ethtool_get_ts_info,
> };
>
> int ctucan_suspend(struct device *dev)
> @@ -1345,6 +1493,8 @@ int ctucan_probe_common(struct device *dev, void
> __iomem *addr, int irq, unsigne struct ctucan_priv *priv;
> struct net_device *ndev;
> int ret;
> + u32 timestamp_freq = 0;
> + u32 timestamp_bit_size = 0;
>
> /* Create a CAN device instance */
> ndev = alloc_candev(sizeof(struct ctucan_priv), ntxbufs);
> @@ -1386,7 +1536,9 @@ int ctucan_probe_common(struct device *dev, void
> __iomem *addr, int irq, unsigne
>
> /* Getting the can_clk info */
> if (!can_clk_rate) {
> - priv->can_clk = devm_clk_get(dev, NULL);
> + priv->can_clk = devm_clk_get_optional(dev, "core-clk");
> + if (!priv->can_clk)
> + priv->can_clk = devm_clk_get(dev, NULL);
> if (IS_ERR(priv->can_clk)) {
> dev_err(dev, "Device clock not found.\n");
> ret = PTR_ERR(priv->can_clk);
> @@ -1425,6 +1577,54 @@ int ctucan_probe_common(struct device *dev, void
> __iomem *addr, int irq, unsigne
>
> priv->can.clock.freq = can_clk_rate;
>
> + priv->timestamp_enabled = false;
> + priv->timestamp_possible = true;
> + priv->timestamp_clk = NULL;
> +
> + /* Obtain timestamping frequency */
> + if (pm_enable_call) {
> + /* Plaftorm device: get tstamp clock from device tree */
> + priv->timestamp_clk = devm_clk_get(dev, "ts-clk");
> + if (IS_ERR(priv->timestamp_clk)) {
> + /* Take the core clock frequency instead */
> + timestamp_freq = can_clk_rate;
> + } else {
> + timestamp_freq = clk_get_rate(priv->timestamp_clk);
> + }
> + } else {
> + /* PCI device: assume tstamp freq is equal to bus clk rate */
> + timestamp_freq = can_clk_rate;
> + }
> +
> + /* Obtain timestamping counter bit size */
> + timestamp_bit_size = (ctucan_read32(priv, CTUCANFD_ERR_CAPT) &
> REG_ERR_CAPT_TS_BITS) >> 24; + timestamp_bit_size += 1; /* the register
> value was bit_size - 1 */ +
- timestamp_bit_size = (ctucan_read32(priv, CTUCANFD_ERR_CAPT) & REG_ERR_CAPT_TS_BITS) >> 24;
+ timestamp_bit_size = FIELD_GET(REG_ERR_CAPT_TS_BITS, ctucan_read32(priv, CTUCANFD_ERR_CAPT));
> + /* For 2.x versions of the IP core, we will assume 64-bit counter
> + * if there was a 0 in the register.
> + */
> + if (timestamp_bit_size == 1) {
> + u32 version_reg = ctucan_read32(priv, CTUCANFD_DEVICE_ID);
> + u32 major = (version_reg & REG_DEVICE_ID_VER_MAJOR) >> 24;
- u32 major = (version_reg & REG_DEVICE_ID_VER_MAJOR) >> 24;
+ u32 major = FIELD_GET(REG_DEVICE_ID_VER_MAJOR, version_reg);
> +
> + if (major == 2)
> + timestamp_bit_size = 64;
> + else
> + priv->timestamp_possible = false;
> + }
> +
> + /* Setup conversion constants and work delay */
> + priv->cc.read = ctucan_read_timestamp_cc_wrapper;
> + priv->cc.mask = CYCLECOUNTER_MASK(timestamp_bit_size);
> + if (priv->timestamp_possible) {
> + clocks_calc_mult_shift(&priv->cc.mult, &priv->cc.shift, timestamp_freq,
> + NSEC_PER_SEC, CTUCANFD_MAX_WORK_DELAY_SEC);
> + priv->work_delay_jiffies =
> + ctucan_calculate_work_delay(timestamp_bit_size, timestamp_freq);
> + if (priv->work_delay_jiffies == 0)
> + priv->timestamp_possible = false;
> + }
> +
> netif_napi_add(ndev, &priv->napi, ctucan_rx_poll, NAPI_POLL_WEIGHT);
>
> ret = register_candev(ndev);
> diff --git a/drivers/net/can/ctucanfd/ctucanfd_timestamp.c
> b/drivers/net/can/ctucanfd/ctucanfd_timestamp.c new file mode 100644
> index 000000000000..c802123bbfbb
> --- /dev/null
> +++ b/drivers/net/can/ctucanfd/ctucanfd_timestamp.c
> @@ -0,0 +1,87 @@
> +// 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/)
> +
> ***************************************************************************
>***/ +
> +#include "vdso/time64.h"
> +#include <linux/bitops.h>
> +#include <linux/clocksource.h>
> +#include <linux/math64.h>
> +#include <linux/timecounter.h>
> +#include <linux/workqueue.h>
> +
> +#include "ctucanfd.h"
> +#include "ctucanfd_kregs.h"
> +
> +u64 ctucan_read_timestamp_cc_wrapper(const struct cyclecounter *cc)
> +{
> + struct ctucan_priv *priv;
> +
> + priv = container_of(cc, struct ctucan_priv, cc);
> + return ctucan_read_timestamp_counter(priv);
> +}
> +
> +static void ctucan_timestamp_work(struct work_struct *work)
> +{
> + struct delayed_work *delayed_work = to_delayed_work(work);
> + struct ctucan_priv *priv;
> +
> + priv = container_of(delayed_work, struct ctucan_priv, timestamp);
> + timecounter_read(&priv->tc);
> + schedule_delayed_work(&priv->timestamp, priv->work_delay_jiffies);
> +}
> +
> +u32 ctucan_calculate_work_delay(const u32 timestamp_bit_size, const u32
> timestamp_freq) +{
> + u32 jiffies_order = fls(HZ);
> + u32 max_shift_left = 63 - jiffies_order;
> + s32 final_shift = (timestamp_bit_size - 1) - max_shift_left;
> + u64 work_delay_jiffies;
> +
> + /* The formula is work_delay_jiffies = 2**(bit_size - 1) / ts_frequency *
> HZ + * using (bit_size - 1) instead of full bit_size to read the counter
> + * roughly twice per period
> + */
> + work_delay_jiffies = div_u64((u64)HZ << max_shift_left, timestamp_freq);
> +
> + if (final_shift > 0)
> + work_delay_jiffies = work_delay_jiffies << final_shift;
> + else
> + work_delay_jiffies = work_delay_jiffies >> -final_shift;
> +
> + work_delay_jiffies = min(work_delay_jiffies,
> + (unsigned long long)CTUCANFD_MAX_WORK_DELAY_SEC * HZ);
> + return (u32)work_delay_jiffies;
> +}
> +
> +void ctucan_skb_set_timestamp(const struct ctucan_priv *priv, struct
> sk_buff *skb, u64 timestamp) +{
> + struct skb_shared_hwtstamps *hwtstamps = skb_hwtstamps(skb);
> + u64 ns;
> +
> + ns = timecounter_cyc2time(&priv->tc, timestamp);
> + hwtstamps->hwtstamp = ns_to_ktime(ns);
> +}
> +
> +void ctucan_timestamp_init(struct ctucan_priv *priv)
> +{
> + timecounter_init(&priv->tc, &priv->cc, ktime_get_real_ns());
> + INIT_DELAYED_WORK(&priv->timestamp, ctucan_timestamp_work);
> + schedule_delayed_work(&priv->timestamp, priv->work_delay_jiffies);
> +}
> +
> +void ctucan_timestamp_stop(struct ctucan_priv *priv)
> +{
> + cancel_delayed_work_sync(&priv->timestamp);
> +}
--
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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v2 1/3] can: ctucanfd: add HW timestamps to RX and error CAN frames
2022-08-01 18:46 ` [PATCH v2 1/3] can: ctucanfd: add HW timestamps to RX and error CAN frames Matej Vasilevski
2022-08-01 20:42 ` Pavel Pisa
@ 2022-08-02 3:43 ` Vincent Mailhol
2022-08-02 6:42 ` Matej Vasilevski
2022-08-02 7:37 ` Pavel Pisa
2022-08-02 9:29 ` Marc Kleine-Budde
2 siblings, 2 replies; 30+ messages in thread
From: Vincent Mailhol @ 2022-08-02 3:43 UTC (permalink / raw)
To: Matej Vasilevski
Cc: Pavel Pisa, Ondrej Ille, Wolfgang Grandegger, Marc Kleine-Budde,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, linux-can, netdev, devicetree
Hi Matej,
I just send a series last week which a significant amount of changes
for CAN timestamping tree-wide:
https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/commit/?id=12a18d79dc14c80b358dbd26461614b97f2ea4a6
I suggest you have a look at this series and harmonize it with the new
features (e.g. Hardware TX timestamp).
On Tue. 2 Aug. 2022 at 03:52, Matej Vasilevski
<matej.vasilevski@seznam.cz> 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_clk" 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>
> ---
> drivers/net/can/ctucanfd/Makefile | 2 +-
> drivers/net/can/ctucanfd/ctucanfd.h | 20 ++
> drivers/net/can/ctucanfd/ctucanfd_base.c | 214 +++++++++++++++++-
> drivers/net/can/ctucanfd/ctucanfd_timestamp.c | 87 +++++++
> 4 files changed, 315 insertions(+), 8 deletions(-)
> create mode 100644 drivers/net/can/ctucanfd/ctucanfd_timestamp.c
>
> diff --git a/drivers/net/can/ctucanfd/Makefile b/drivers/net/can/ctucanfd/Makefile
> index 8078f1f2c30f..a36e66f2cea7 100644
> --- a/drivers/net/can/ctucanfd/Makefile
> +++ b/drivers/net/can/ctucanfd/Makefile
> @@ -4,7 +4,7 @@
> #
>
> obj-$(CONFIG_CAN_CTUCANFD) := ctucanfd.o
> -ctucanfd-y := ctucanfd_base.o
> +ctucanfd-y := ctucanfd_base.o ctucanfd_timestamp.o
>
> obj-$(CONFIG_CAN_CTUCANFD_PCI) += ctucanfd_pci.o
> obj-$(CONFIG_CAN_CTUCANFD_PLATFORM) += ctucanfd_platform.o
> diff --git a/drivers/net/can/ctucanfd/ctucanfd.h b/drivers/net/can/ctucanfd/ctucanfd.h
> index 0e9904f6a05d..43d9c73ce244 100644
> --- a/drivers/net/can/ctucanfd/ctucanfd.h
> +++ b/drivers/net/can/ctucanfd/ctucanfd.h
> @@ -23,6 +23,10 @@
> #include <linux/netdevice.h>
> #include <linux/can/dev.h>
> #include <linux/list.h>
> +#include <linux/timecounter.h>
> +#include <linux/workqueue.h>
> +
> +#define CTUCANFD_MAX_WORK_DELAY_SEC 86400U /* one day == 24 * 3600 seconds */
>
> enum ctu_can_fd_can_registers;
>
> @@ -51,6 +55,15 @@ struct ctucan_priv {
> u32 rxfrm_first_word;
>
> struct list_head peers_on_pdev;
> +
> + struct cyclecounter cc;
> + struct timecounter tc;
> + struct delayed_work timestamp;
> +
> + struct clk *timestamp_clk;
> + u32 work_delay_jiffies;
> + bool timestamp_enabled;
> + bool timestamp_possible;
> };
>
> /**
> @@ -79,4 +92,11 @@ int ctucan_probe_common(struct device *dev, void __iomem *addr,
> int ctucan_suspend(struct device *dev) __maybe_unused;
> int ctucan_resume(struct device *dev) __maybe_unused;
>
> +u64 ctucan_read_timestamp_cc_wrapper(const struct cyclecounter *cc);
> +u64 ctucan_read_timestamp_counter(struct ctucan_priv *priv);
> +u32 ctucan_calculate_work_delay(const u32 timestamp_bit_size, const u32 timestamp_freq);
> +void ctucan_skb_set_timestamp(const struct ctucan_priv *priv, struct sk_buff *skb,
> + u64 timestamp);
> +void ctucan_timestamp_init(struct ctucan_priv *priv);
> +void ctucan_timestamp_stop(struct ctucan_priv *priv);
> #endif /*__CTUCANFD__*/
> diff --git a/drivers/net/can/ctucanfd/ctucanfd_base.c b/drivers/net/can/ctucanfd/ctucanfd_base.c
> index 3c18d028bd8c..35b37de51811 100644
> --- a/drivers/net/can/ctucanfd/ctucanfd_base.c
> +++ b/drivers/net/can/ctucanfd/ctucanfd_base.c
> @@ -18,6 +18,7 @@
> ******************************************************************************/
>
> #include <linux/clk.h>
> +#include <linux/clocksource.h>
> #include <linux/errno.h>
> #include <linux/ethtool.h>
> #include <linux/init.h>
> @@ -148,6 +149,27 @@ static void ctucan_write_txt_buf(struct ctucan_priv *priv, enum ctu_can_fd_can_r
> priv->write_reg(priv, buf_base + offset, val);
> }
>
> +static u64 concatenate_two_u32(u32 high, u32 low)
Might be good to add the "namespace" prefix. I suggest:
static u64 ctucan_concat_tstamp(u32 high, u32 low)
Because, so far, the function is to be used exclusively with timestamps.
Also, I was surprised that no helper functions in include/linux/
headers already do that. But this is another story.
> +{
> + return ((u64)high << 32) | ((u64)low);
> +}
> +
> +u64 ctucan_read_timestamp_counter(struct ctucan_priv *priv)
> +{
> + u32 ts_low;
> + u32 ts_high;
> + u32 ts_high2;
> +
> + ts_high = ctucan_read32(priv, CTUCANFD_TIMESTAMP_HIGH);
> + ts_low = ctucan_read32(priv, CTUCANFD_TIMESTAMP_LOW);
> + ts_high2 = ctucan_read32(priv, CTUCANFD_TIMESTAMP_HIGH);
> +
> + if (ts_high2 != ts_high)
> + ts_low = priv->read_reg(priv, CTUCANFD_TIMESTAMP_LOW);
> +
> + return concatenate_two_u32(ts_high2, ts_low) & priv->cc.mask;
> +}
> +
> #define CTU_CAN_FD_TXTNF(priv) (!!FIELD_GET(REG_STATUS_TXNF, ctucan_read32(priv, CTUCANFD_STATUS)))
> #define CTU_CAN_FD_ENABLED(priv) (!!FIELD_GET(REG_MODE_ENA, ctucan_read32(priv, CTUCANFD_MODE)))
#define CTU_CAN_FD_TXTNF(priv) \
(!!FIELD_GET(REG_STATUS_TXNF, ctucan_read32(priv, CTUCANFD_STATUS)))
#define CTU_CAN_FD_ENABLED(priv) \
(!!FIELD_GET(REG_MODE_ENA, ctucan_read32(priv, CTUCANFD_MODE)))
Even if the rule is now more relaxed, the soft limit remains 80
characters per line:
https://www.kernel.org/doc/html/latest/process/coding-style.html#breaking-long-lines-and-strings
> @@ -640,12 +662,16 @@ static netdev_tx_t ctucan_start_xmit(struct sk_buff *skb, struct net_device *nde
> * @priv: Pointer to CTU CAN FD's private data
> * @cf: Pointer to CAN frame struct
> * @ffw: Previously read frame format word
> + * @skb: Pointer to buffer to store timestamp
> *
> * Note: Frame format word must be read separately and provided in 'ffw'.
> */
> -static void ctucan_read_rx_frame(struct ctucan_priv *priv, struct canfd_frame *cf, u32 ffw)
> +static void ctucan_read_rx_frame(struct ctucan_priv *priv, struct canfd_frame *cf,
> + u32 ffw, u64 *timestamp)
> {
> u32 idw;
> + u32 tstamp_high;
> + u32 tstamp_low;
> unsigned int i;
> unsigned int wc;
> unsigned int len;
> @@ -682,9 +708,10 @@ static void ctucan_read_rx_frame(struct ctucan_priv *priv, struct canfd_frame *c
> if (unlikely(len > wc * 4))
> len = wc * 4;
>
> - /* Timestamp - Read and throw away */
> - ctucan_read32(priv, CTUCANFD_RX_DATA);
> - ctucan_read32(priv, CTUCANFD_RX_DATA);
> + /* Timestamp */
> + tstamp_low = ctucan_read32(priv, CTUCANFD_RX_DATA);
> + tstamp_high = ctucan_read32(priv, CTUCANFD_RX_DATA);
> + *timestamp = concatenate_two_u32(tstamp_high, tstamp_low) & priv->cc.mask;
>
> /* Data */
> for (i = 0; i < len; i += 4) {
> @@ -713,6 +740,7 @@ static int ctucan_rx(struct net_device *ndev)
> struct net_device_stats *stats = &ndev->stats;
> struct canfd_frame *cf;
> struct sk_buff *skb;
> + u64 timestamp;
> u32 ffw;
>
> if (test_bit(CTUCANFD_FLAG_RX_FFW_BUFFERED, &priv->drv_flags)) {
> @@ -736,7 +764,9 @@ static int ctucan_rx(struct net_device *ndev)
> return 0;
> }
>
> - ctucan_read_rx_frame(priv, cf, ffw);
> + ctucan_read_rx_frame(priv, cf, ffw, ×tamp);
> + if (priv->timestamp_enabled)
> + ctucan_skb_set_timestamp(priv, skb, timestamp);
>
> stats->rx_bytes += cf->len;
> stats->rx_packets++;
> @@ -906,6 +936,11 @@ static void ctucan_err_interrupt(struct net_device *ndev, u32 isr)
> if (skb) {
> stats->rx_packets++;
> stats->rx_bytes += cf->can_dlc;
> + if (priv->timestamp_enabled) {
> + u64 tstamp = ctucan_read_timestamp_counter(priv);
> +
> + ctucan_skb_set_timestamp(priv, skb, tstamp);
> + }
> netif_rx(skb);
> }
> }
> @@ -951,6 +986,11 @@ static int ctucan_rx_poll(struct napi_struct *napi, int quota)
> cf->data[1] |= CAN_ERR_CRTL_RX_OVERFLOW;
> stats->rx_packets++;
> stats->rx_bytes += cf->can_dlc;
> + if (priv->timestamp_enabled) {
> + u64 tstamp = ctucan_read_timestamp_counter(priv);
> +
> + ctucan_skb_set_timestamp(priv, skb, tstamp);
> + }
> netif_rx(skb);
> }
>
> @@ -1231,6 +1271,9 @@ static int ctucan_open(struct net_device *ndev)
> goto err_chip_start;
> }
>
> + if (priv->timestamp_possible)
> + ctucan_timestamp_init(priv);
> +
> netdev_info(ndev, "ctu_can_fd device registered\n");
> napi_enable(&priv->napi);
> netif_start_queue(ndev);
> @@ -1263,6 +1306,9 @@ static int ctucan_close(struct net_device *ndev)
> ctucan_chip_stop(ndev);
> free_irq(ndev->irq, ndev);
> close_candev(ndev);
> + if (priv->timestamp_possible)
> + ctucan_timestamp_stop(priv);
> +
>
> pm_runtime_put(priv->dev);
>
> @@ -1295,15 +1341,117 @@ static int ctucan_get_berr_counter(const struct net_device *ndev, struct can_ber
> return 0;
> }
>
> +static int ctucan_hwtstamp_set(struct net_device *dev, struct ifreq *ifr)
> +{
> + struct ctucan_priv *priv = netdev_priv(dev);
> + struct hwtstamp_config cfg;
> +
> + if (!priv->timestamp_possible)
> + return -EOPNOTSUPP;
> +
> + if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
> + return -EFAULT;
> +
> + if (cfg.flags)
> + return -EINVAL;
> +
> + if (cfg.tx_type != HWTSTAMP_TX_OFF)
> + return -ERANGE;
I have a great news: your driver now also support hardware TX timestamps:
https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/commit/?id=8bdd1112edcd3edce2843e03826204a84a61042d
> +
> + switch (cfg.rx_filter) {
> + case HWTSTAMP_FILTER_NONE:
> + priv->timestamp_enabled = false;
> + break;
> + case HWTSTAMP_FILTER_ALL:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V2_EVENT:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V2_SYNC:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
> + priv->timestamp_enabled = true;
> + cfg.rx_filter = HWTSTAMP_FILTER_ALL;
> + break;
All those HWTSTAMP_FILTER_PTP_V2_* filters are for UDP, Ethernet or AS1:
https://elixir.bootlin.com/linux/v5.4.5/source/include/uapi/linux/net_tstamp.h#L106
Because those layers do not exist in CAN, I suggest treating them all
as not supported.
Please have a look at this patch:
https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/commit/?id=90f942c5a6d775bad1be33ba214755314105da4a
> + default:
> + return -ERANGE;
> + }
> +
> + return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
> +}
> +
> +static int ctucan_hwtstamp_get(struct net_device *dev, struct ifreq *ifr)
> +{
> + struct ctucan_priv *priv = netdev_priv(dev);
> + struct hwtstamp_config cfg;
> +
> + if (!priv->timestamp_possible)
> + return -EOPNOTSUPP;
> +
> + cfg.flags = 0;
> + cfg.tx_type = HWTSTAMP_TX_OFF;
Hardware TX timestamps are now supported (c.f. supra).
> + cfg.rx_filter = priv->timestamp_enabled ? HWTSTAMP_FILTER_ALL : HWTSTAMP_FILTER_NONE;
> + return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
> +}
> +
> +static int ctucan_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
Please consider using the generic function can_eth_ioctl_hwts()
https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/commit/?id=90f942c5a6d775bad1be33ba214755314105da4a
> +{
> + switch (cmd) {
> + case SIOCSHWTSTAMP:
> + return ctucan_hwtstamp_set(dev, ifr);
> + case SIOCGHWTSTAMP:
> + return ctucan_hwtstamp_get(dev, ifr);
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
>
> +static int ctucan_ethtool_get_ts_info(struct net_device *ndev, struct ethtool_ts_info *info)
Please break the line to meet the 80 columns soft limit.
Please consider using the generic function can_ethtool_op_get_ts_info_hwts():
https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/commit/?id=7fb48d25b5ce3bc488dbb019bf1736248181de9a
Something like that:
static int ctucan_ethtool_get_ts_info(struct net_device *ndev,
struct ethtool_ts_info *inf
{
struct ctucan_priv *priv = netdev_priv(ndev);
if (!priv->timestamp_possible)
ethtool_op_get_ts_info(ndev, info);
return can_ethtool_op_get_ts_info_hwts(ndev, info);
}
> +{
> + struct ctucan_priv *priv = netdev_priv(ndev);
> +
> + ethtool_op_get_ts_info(ndev, info);
> +
> + if (!priv->timestamp_possible)
> + return 0;
> +
> + info->so_timestamping |= SOF_TIMESTAMPING_RX_HARDWARE |
> + SOF_TIMESTAMPING_RAW_HARDWARE;
> + info->tx_types = BIT(HWTSTAMP_TX_OFF);
Hardware TX timestamps are now supported (c.f. supra).
> + info->rx_filters = BIT(HWTSTAMP_FILTER_NONE) |
> + BIT(HWTSTAMP_FILTER_ALL);
> +
> + return 0;
> +}
> +
> static const struct net_device_ops ctucan_netdev_ops = {
> .ndo_open = ctucan_open,
> .ndo_stop = ctucan_close,
> .ndo_start_xmit = ctucan_start_xmit,
> .ndo_change_mtu = can_change_mtu,
> + .ndo_eth_ioctl = ctucan_ioctl,
> };
>
> static const struct ethtool_ops ctucan_ethtool_ops = {
> - .get_ts_info = ethtool_op_get_ts_info,
> + .get_ts_info = ctucan_ethtool_get_ts_info,
> };
>
> int ctucan_suspend(struct device *dev)
> @@ -1345,6 +1493,8 @@ int ctucan_probe_common(struct device *dev, void __iomem *addr, int irq, unsigne
> struct ctucan_priv *priv;
> struct net_device *ndev;
> int ret;
> + u32 timestamp_freq = 0;
> + u32 timestamp_bit_size = 0;
>
> /* Create a CAN device instance */
> ndev = alloc_candev(sizeof(struct ctucan_priv), ntxbufs);
> @@ -1386,7 +1536,9 @@ int ctucan_probe_common(struct device *dev, void __iomem *addr, int irq, unsigne
>
> /* Getting the can_clk info */
> if (!can_clk_rate) {
> - priv->can_clk = devm_clk_get(dev, NULL);
> + priv->can_clk = devm_clk_get_optional(dev, "core-clk");
> + if (!priv->can_clk)
> + priv->can_clk = devm_clk_get(dev, NULL);
> if (IS_ERR(priv->can_clk)) {
> dev_err(dev, "Device clock not found.\n");
Just a suggestion, but you may want to print the mnemotechnic of the error code:
dev_err(dev, "Device clock not found: %pe.\n", priv->can_clk);
> ret = PTR_ERR(priv->can_clk);
> @@ -1425,6 +1577,54 @@ int ctucan_probe_common(struct device *dev, void __iomem *addr, int irq, unsigne
>
> priv->can.clock.freq = can_clk_rate;
>
> + priv->timestamp_enabled = false;
> + priv->timestamp_possible = true;
> + priv->timestamp_clk = NULL;
> +
> + /* Obtain timestamping frequency */
> + if (pm_enable_call) {
> + /* Plaftorm device: get tstamp clock from device tree */
> + priv->timestamp_clk = devm_clk_get(dev, "ts-clk");
> + if (IS_ERR(priv->timestamp_clk)) {
> + /* Take the core clock frequency instead */
> + timestamp_freq = can_clk_rate;
> + } else {
> + timestamp_freq = clk_get_rate(priv->timestamp_clk);
> + }
> + } else {
> + /* PCI device: assume tstamp freq is equal to bus clk rate */
> + timestamp_freq = can_clk_rate;
> + }
> +
> + /* Obtain timestamping counter bit size */
> + timestamp_bit_size = (ctucan_read32(priv, CTUCANFD_ERR_CAPT) & REG_ERR_CAPT_TS_BITS) >> 24;
> + timestamp_bit_size += 1; /* the register value was bit_size - 1 */
> +
> + /* For 2.x versions of the IP core, we will assume 64-bit counter
> + * if there was a 0 in the register.
> + */
> + if (timestamp_bit_size == 1) {
> + u32 version_reg = ctucan_read32(priv, CTUCANFD_DEVICE_ID);
> + u32 major = (version_reg & REG_DEVICE_ID_VER_MAJOR) >> 24;
> +
> + if (major == 2)
> + timestamp_bit_size = 64;
> + else
> + priv->timestamp_possible = false;
> + }
> +
> + /* Setup conversion constants and work delay */
> + priv->cc.read = ctucan_read_timestamp_cc_wrapper;
> + priv->cc.mask = CYCLECOUNTER_MASK(timestamp_bit_size);
> + if (priv->timestamp_possible) {
> + clocks_calc_mult_shift(&priv->cc.mult, &priv->cc.shift, timestamp_freq,
> + NSEC_PER_SEC, CTUCANFD_MAX_WORK_DELAY_SEC);
> + priv->work_delay_jiffies =
> + ctucan_calculate_work_delay(timestamp_bit_size, timestamp_freq);
> + if (priv->work_delay_jiffies == 0)
> + priv->timestamp_possible = false;
> + }
> +
> netif_napi_add(ndev, &priv->napi, ctucan_rx_poll, NAPI_POLL_WEIGHT);
>
> ret = register_candev(ndev);
> diff --git a/drivers/net/can/ctucanfd/ctucanfd_timestamp.c b/drivers/net/can/ctucanfd/ctucanfd_timestamp.c
> new file mode 100644
> index 000000000000..c802123bbfbb
> --- /dev/null
> +++ b/drivers/net/can/ctucanfd/ctucanfd_timestamp.c
> @@ -0,0 +1,87 @@
> +// 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/)
> + ******************************************************************************/
> +
> +#include "vdso/time64.h"
> +#include <linux/bitops.h>
> +#include <linux/clocksource.h>
> +#include <linux/math64.h>
> +#include <linux/timecounter.h>
> +#include <linux/workqueue.h>
> +
> +#include "ctucanfd.h"
> +#include "ctucanfd_kregs.h"
> +
> +u64 ctucan_read_timestamp_cc_wrapper(const struct cyclecounter *cc)
> +{
> + struct ctucan_priv *priv;
> +
> + priv = container_of(cc, struct ctucan_priv, cc);
> + return ctucan_read_timestamp_counter(priv);
> +}
> +
> +static void ctucan_timestamp_work(struct work_struct *work)
> +{
> + struct delayed_work *delayed_work = to_delayed_work(work);
> + struct ctucan_priv *priv;
> +
> + priv = container_of(delayed_work, struct ctucan_priv, timestamp);
> + timecounter_read(&priv->tc);
> + schedule_delayed_work(&priv->timestamp, priv->work_delay_jiffies);
> +}
> +
> +u32 ctucan_calculate_work_delay(const u32 timestamp_bit_size, const u32 timestamp_freq)
> +{
> + u32 jiffies_order = fls(HZ);
> + u32 max_shift_left = 63 - jiffies_order;
> + s32 final_shift = (timestamp_bit_size - 1) - max_shift_left;
> + u64 work_delay_jiffies;
> +
> + /* The formula is work_delay_jiffies = 2**(bit_size - 1) / ts_frequency * HZ
> + * using (bit_size - 1) instead of full bit_size to read the counter
> + * roughly twice per period
> + */
> + work_delay_jiffies = div_u64((u64)HZ << max_shift_left, timestamp_freq);
> +
> + if (final_shift > 0)
> + work_delay_jiffies = work_delay_jiffies << final_shift;
> + else
> + work_delay_jiffies = work_delay_jiffies >> -final_shift;
> +
> + work_delay_jiffies = min(work_delay_jiffies,
> + (unsigned long long)CTUCANFD_MAX_WORK_DELAY_SEC * HZ);
> + return (u32)work_delay_jiffies;
> +}
> +
> +void ctucan_skb_set_timestamp(const struct ctucan_priv *priv, struct sk_buff *skb, u64 timestamp)
> +{
> + struct skb_shared_hwtstamps *hwtstamps = skb_hwtstamps(skb);
> + u64 ns;
> +
> + ns = timecounter_cyc2time(&priv->tc, timestamp);
> + hwtstamps->hwtstamp = ns_to_ktime(ns);
> +}
> +
> +void ctucan_timestamp_init(struct ctucan_priv *priv)
> +{
> + timecounter_init(&priv->tc, &priv->cc, ktime_get_real_ns());
> + INIT_DELAYED_WORK(&priv->timestamp, ctucan_timestamp_work);
> + schedule_delayed_work(&priv->timestamp, priv->work_delay_jiffies);
> +}
> +
> +void ctucan_timestamp_stop(struct ctucan_priv *priv)
> +{
> + cancel_delayed_work_sync(&priv->timestamp);
> +}
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v2 1/3] can: ctucanfd: add HW timestamps to RX and error CAN frames
2022-08-02 3:43 ` Vincent Mailhol
@ 2022-08-02 6:42 ` Matej Vasilevski
2022-08-02 7:37 ` Pavel Pisa
1 sibling, 0 replies; 30+ messages in thread
From: Matej Vasilevski @ 2022-08-02 6:42 UTC (permalink / raw)
To: Vincent Mailhol
Cc: Pavel Pisa, Ondrej Ille, Wolfgang Grandegger, Marc Kleine-Budde,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, linux-can, netdev, devicetree
On Tue, Aug 02, 2022 at 12:43:38PM +0900, Vincent Mailhol wrote:
> Hi Matej,
>
> I just send a series last week which a significant amount of changes
> for CAN timestamping tree-wide:
> https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/commit/?id=12a18d79dc14c80b358dbd26461614b97f2ea4a6
>
> I suggest you have a look at this series and harmonize it with the new
> features (e.g. Hardware TX timestamp).
Hi Vincent,
thanks for your review! I saw your patch series, but I've only skimmed
through it. I'll read it fully in the evening.
> > @@ -148,6 +149,27 @@ static void ctucan_write_txt_buf(struct ctucan_priv *priv, enum ctu_can_fd_can_r
> > priv->write_reg(priv, buf_base + offset, val);
> > }
> >
> > +static u64 concatenate_two_u32(u32 high, u32 low)
>
> Might be good to add the "namespace" prefix. I suggest:
>
> static u64 ctucan_concat_tstamp(u32 high, u32 low)
>
> Because, so far, the function is to be used exclusively with timestamps.
>
> Also, I was surprised that no helper functions in include/linux/
> headers already do that. But this is another story.
I agree, it is more specific, thanks for the suggestion.
> > +{
> > + return ((u64)high << 32) | ((u64)low);
> > +}
> > +
> > +u64 ctucan_read_timestamp_counter(struct ctucan_priv *priv)
> > +{
> > + u32 ts_low;
> > + u32 ts_high;
> > + u32 ts_high2;
> > +
> > + ts_high = ctucan_read32(priv, CTUCANFD_TIMESTAMP_HIGH);
> > + ts_low = ctucan_read32(priv, CTUCANFD_TIMESTAMP_LOW);
> > + ts_high2 = ctucan_read32(priv, CTUCANFD_TIMESTAMP_HIGH);
> > +
> > + if (ts_high2 != ts_high)
> > + ts_low = priv->read_reg(priv, CTUCANFD_TIMESTAMP_LOW);
> > +
> > + return concatenate_two_u32(ts_high2, ts_low) & priv->cc.mask;
> > +}
> > +
> > #define CTU_CAN_FD_TXTNF(priv) (!!FIELD_GET(REG_STATUS_TXNF, ctucan_read32(priv, CTUCANFD_STATUS)))
> > #define CTU_CAN_FD_ENABLED(priv) (!!FIELD_GET(REG_MODE_ENA, ctucan_read32(priv, CTUCANFD_MODE)))
>
>
> #define CTU_CAN_FD_TXTNF(priv) \
> (!!FIELD_GET(REG_STATUS_TXNF, ctucan_read32(priv, CTUCANFD_STATUS)))
>
> #define CTU_CAN_FD_ENABLED(priv) \
> (!!FIELD_GET(REG_MODE_ENA, ctucan_read32(priv, CTUCANFD_MODE)))
>
> Even if the rule is now more relaxed, the soft limit remains 80
> characters per line:
>
> https://www.kernel.org/doc/html/latest/process/coding-style.html#breaking-long-lines-and-strings
Not from my patch but no problem, I'll fix it in the next version.
Thanks for spotting this.
> > @@ -1295,15 +1341,117 @@ static int ctucan_get_berr_counter(const struct net_device *ndev, struct can_ber
> > return 0;
> > }
> >
> > +static int ctucan_hwtstamp_set(struct net_device *dev, struct ifreq *ifr)
> > +{
> > + struct ctucan_priv *priv = netdev_priv(dev);
> > + struct hwtstamp_config cfg;
> > +
> > + if (!priv->timestamp_possible)
> > + return -EOPNOTSUPP;
> > +
> > + if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
> > + return -EFAULT;
> > +
> > + if (cfg.flags)
> > + return -EINVAL;
> > +
> > + if (cfg.tx_type != HWTSTAMP_TX_OFF)
> > + return -ERANGE;
>
> I have a great news: your driver now also support hardware TX timestamps:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/commit/?id=8bdd1112edcd3edce2843e03826204a84a61042d
Yes, I'll read your patch series and update accordingly.
> > +
> > + switch (cfg.rx_filter) {
> > + case HWTSTAMP_FILTER_NONE:
> > + priv->timestamp_enabled = false;
> > + break;
> > + case HWTSTAMP_FILTER_ALL:
> > + fallthrough;
> > + case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
> > + fallthrough;
> > + case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
> > + fallthrough;
> > + case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
> > + fallthrough;
> > + case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
> > + fallthrough;
> > + case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
> > + fallthrough;
> > + case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
> > + fallthrough;
> > + case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
> > + fallthrough;
> > + case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
> > + fallthrough;
> > + case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
> > + fallthrough;
> > + case HWTSTAMP_FILTER_PTP_V2_EVENT:
> > + fallthrough;
> > + case HWTSTAMP_FILTER_PTP_V2_SYNC:
> > + fallthrough;
> > + case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
> > + priv->timestamp_enabled = true;
> > + cfg.rx_filter = HWTSTAMP_FILTER_ALL;
> > + break;
>
> All those HWTSTAMP_FILTER_PTP_V2_* filters are for UDP, Ethernet or AS1:
> https://elixir.bootlin.com/linux/v5.4.5/source/include/uapi/linux/net_tstamp.h#L106
>
> Because those layers do not exist in CAN, I suggest treating them all
> as not supported.
>
> Please have a look at this patch:
> https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/commit/?id=90f942c5a6d775bad1be33ba214755314105da4a
I followed the Doc/networking/timestamping.txt, and section 3.1 says
"Drivers are free to use a more permissive configuration than the requested
configuration."
So I've added in all the _PTP filters etc. If the consensus is that
_NONE and _ALL filters are enough, I'll gladly remove the dozen of
unnecessary cases.
> > + default:
> > + return -ERANGE;
> > + }
> > +
> > + return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
> > +}
> > +
> > +static int ctucan_hwtstamp_get(struct net_device *dev, struct ifreq *ifr)
> > +{
> > + struct ctucan_priv *priv = netdev_priv(dev);
> > + struct hwtstamp_config cfg;
> > +
> > + if (!priv->timestamp_possible)
> > + return -EOPNOTSUPP;
> > +
> > + cfg.flags = 0;
> > + cfg.tx_type = HWTSTAMP_TX_OFF;
>
> Hardware TX timestamps are now supported (c.f. supra).
ACK
>
> > + cfg.rx_filter = priv->timestamp_enabled ? HWTSTAMP_FILTER_ALL : HWTSTAMP_FILTER_NONE;
> > + return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
> > +}
> > +
> > +static int ctucan_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
>
> Please consider using the generic function can_eth_ioctl_hwts()
> https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/commit/?id=90f942c5a6d775bad1be33ba214755314105da4a
I've seen it, but I have to use a custom ioctl function, if I want to
toggle rx timestamps enabled/disabled.
> > +{
> > + switch (cmd) {
> > + case SIOCSHWTSTAMP:
> > + return ctucan_hwtstamp_set(dev, ifr);
> > + case SIOCGHWTSTAMP:
> > + return ctucan_hwtstamp_get(dev, ifr);
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +}
> >
> > +static int ctucan_ethtool_get_ts_info(struct net_device *ndev, struct ethtool_ts_info *info)
>
> Please break the line to meet the 80 columns soft limit.
>
> Please consider using the generic function can_ethtool_op_get_ts_info_hwts():
> https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/commit/?id=7fb48d25b5ce3bc488dbb019bf1736248181de9a
>
> Something like that:
> static int ctucan_ethtool_get_ts_info(struct net_device *ndev,
> struct ethtool_ts_info *inf
> {
> struct ctucan_priv *priv = netdev_priv(ndev);
>
> if (!priv->timestamp_possible)
> ethtool_op_get_ts_info(ndev, info);
>
> return can_ethtool_op_get_ts_info_hwts(ndev, info);
> }
Sure, this is better, I'll include it in v3. Thank you.
> > +{
> > + struct ctucan_priv *priv = netdev_priv(ndev);
> > +
> > + ethtool_op_get_ts_info(ndev, info);
> > +
> > + if (!priv->timestamp_possible)
> > + return 0;
> > +
> > + info->so_timestamping |= SOF_TIMESTAMPING_RX_HARDWARE |
> > + SOF_TIMESTAMPING_RAW_HARDWARE;
> > + info->tx_types = BIT(HWTSTAMP_TX_OFF);
>
> Hardware TX timestamps are now supported (c.f. supra).
ACK
> > + info->rx_filters = BIT(HWTSTAMP_FILTER_NONE) |
> > + BIT(HWTSTAMP_FILTER_ALL);
> > +
> > + return 0;
> > +}
> > +
> > @@ -1386,7 +1536,9 @@ int ctucan_probe_common(struct device *dev, void __iomem *addr, int irq, unsigne
> >
> > /* Getting the can_clk info */
> > if (!can_clk_rate) {
> > - priv->can_clk = devm_clk_get(dev, NULL);
> > + priv->can_clk = devm_clk_get_optional(dev, "core-clk");
> > + if (!priv->can_clk)
> > + priv->can_clk = devm_clk_get(dev, NULL);
> > if (IS_ERR(priv->can_clk)) {
> > dev_err(dev, "Device clock not found.\n");
>
> Just a suggestion, but you may want to print the mnemotechnic of the error code:
> dev_err(dev, "Device clock not found: %pe.\n", priv->can_clk);
Yes the error print might need some tweaking, I'll think about it.
> > ret = PTR_ERR(priv->can_clk);
> > @@ -1425,6 +1577,54 @@ int ctucan_probe_common(struct device *dev, void __iomem *addr, int irq, unsigne
> >
> > priv->can.clock.freq = can_clk_rate;
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v2 1/3] can: ctucanfd: add HW timestamps to RX and error CAN frames
2022-08-02 3:43 ` Vincent Mailhol
2022-08-02 6:42 ` Matej Vasilevski
@ 2022-08-02 7:37 ` Pavel Pisa
2022-08-03 9:04 ` Marc Kleine-Budde
2022-08-12 14:35 ` Vincent Mailhol
1 sibling, 2 replies; 30+ messages in thread
From: Pavel Pisa @ 2022-08-02 7:37 UTC (permalink / raw)
To: Vincent Mailhol
Cc: Matej Vasilevski, Ondrej Ille, Wolfgang Grandegger,
Marc Kleine-Budde, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, linux-can, netdev,
devicetree, Jiri Novak, Oliver Hartkopp
Hello Vincent,
thanks much for review. I am adding some notices to Tx timestamps
after your comments
On Tuesday 02 of August 2022 05:43:38 Vincent Mailhol wrote:
> I just send a series last week which a significant amount of changes
> for CAN timestamping tree-wide:
> https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/comm
>it/?id=12a18d79dc14c80b358dbd26461614b97f2ea4a6
>
> I suggest you have a look at this series and harmonize it with the new
> features (e.g. Hardware TX timestamp).
>
> On Tue. 2 Aug. 2022 at 03:52, Matej Vasilevski
...
> > +static int ctucan_hwtstamp_set(struct net_device *dev, struct ifreq
> > *ifr) +{
> > + struct ctucan_priv *priv = netdev_priv(dev);
> > + struct hwtstamp_config cfg;
> > +
> > + if (!priv->timestamp_possible)
> > + return -EOPNOTSUPP;
> > +
> > + if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
> > + return -EFAULT;
> > +
> > + if (cfg.flags)
> > + return -EINVAL;
> > +
> > + if (cfg.tx_type != HWTSTAMP_TX_OFF)
> > + return -ERANGE;
>
> I have a great news: your driver now also support hardware TX timestamps:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/comm
>it/?id=8bdd1112edcd3edce2843e03826204a84a61042d
>
> > +
> > + switch (cfg.rx_filter) {
> > + case HWTSTAMP_FILTER_NONE:
> > + priv->timestamp_enabled = false;
...
> > +
> > + cfg.flags = 0;
> > + cfg.tx_type = HWTSTAMP_TX_OFF;
>
> Hardware TX timestamps are now supported (c.f. supra).
>
> > + cfg.rx_filter = priv->timestamp_enabled ? HWTSTAMP_FILTER_ALL :
> > HWTSTAMP_FILTER_NONE; + return copy_to_user(ifr->ifr_data, &cfg,
> > sizeof(cfg)) ? -EFAULT : 0; +}
> > +
> > +static int ctucan_ioctl(struct net_device *dev, struct ifreq *ifr, int
> > cmd)
>
> Please consider using the generic function can_eth_ioctl_hwts()
> https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/comm
>it/?id=90f942c5a6d775bad1be33ba214755314105da4a
>
> > +{
...
> > + info->so_timestamping |= SOF_TIMESTAMPING_RX_HARDWARE |
> > + SOF_TIMESTAMPING_RAW_HARDWARE;
> > + info->tx_types = BIT(HWTSTAMP_TX_OFF);
>
> Hardware TX timestamps are now supported (c.f. supra).
>
> > + info->rx_filters = BIT(HWTSTAMP_FILTER_NONE) |
> > + BIT(HWTSTAMP_FILTER_ALL);
I am not sure if it is good idea to report support for hardware
TX timestamps by all drivers. Precise hardware Tx timestamps
are important for some CAN applications but they require to be
exactly/properly aligned with Rx timestamps.
Only some CAN (FD) controllers really support that feature.
For M-CAN and some others it is realized as another event
FIFO in addition to Tx and Rx FIFOs.
For CTU CAN FD, we have decided that we do not complicate design
and driver by separate events channel. We have configurable
and possibly large Rx FIFO depth which is logical to use for
analyzer mode and we can use loopback to receive own messages
timestamped same way as external received ones.
See 2.14.1 Loopback mode
SETTINGS[ILBP]=1.
in the datasheet
http://canbus.pages.fel.cvut.cz/ctucanfd_ip_core/doc/Datasheet.pdf
There is still missing information which frames are received
locally and from which buffer they are in the Rx message format,
but we plan to add that into VHDL design.
In such case, we can switch driver mode and release Tx buffers
only after corresponding message is read from Rx FIFO and
fill exact finegrain (10 ns in our current design) timestamps
to the echo skb. The order of received messages will be seen
exactly mathing the wire order for both transmitted and received
messages then. Which I consider as proper solution for the
most applications including CAN bus analyzers.
So I consider to report HW Tx timestamps for cases where exact,
precise timestamping is not available for loopback messages
as problematic because you cannot distinguish if you talk
with driver and HW with real/precise timestamps support
or only dummy implementation to make some tools happy.
Best wishes and thanks for consideration about altrenatives,
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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v2 1/3] can: ctucanfd: add HW timestamps to RX and error CAN frames
2022-08-02 7:37 ` Pavel Pisa
@ 2022-08-03 9:04 ` Marc Kleine-Budde
2022-08-04 8:08 ` Pavel Pisa
2022-08-12 14:35 ` Vincent Mailhol
1 sibling, 1 reply; 30+ messages in thread
From: Marc Kleine-Budde @ 2022-08-03 9:04 UTC (permalink / raw)
To: Pavel Pisa
Cc: Vincent Mailhol, Matej Vasilevski, Ondrej Ille,
Wolfgang Grandegger, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski,
linux-can, netdev, devicetree, Jiri Novak, Oliver Hartkopp
[-- Attachment #1: Type: text/plain, Size: 2715 bytes --]
On 02.08.2022 09:37:54, Pavel Pisa wrote:
> > Hardware TX timestamps are now supported (c.f. supra).
> >
> > > + info->rx_filters = BIT(HWTSTAMP_FILTER_NONE) |
> > > + BIT(HWTSTAMP_FILTER_ALL);
>
> I am not sure if it is good idea to report support for hardware
> TX timestamps by all drivers. Precise hardware Tx timestamps
> are important for some CAN applications but they require to be
> exactly/properly aligned with Rx timestamps.
>
> Only some CAN (FD) controllers really support that feature.
> For M-CAN and some others it is realized as another event
> FIFO in addition to Tx and Rx FIFOs.
The mcp251xfd uses the event FIFO to signal TX completion. Timestamps
are optional, but always enabled in the mcp251xfd driver.
> For CTU CAN FD, we have decided that we do not complicate design
> and driver by separate events channel. We have configurable
> and possibly large Rx FIFO depth which is logical to use for
> analyzer mode and we can use loopback to receive own messages
> timestamped same way as external received ones.
>
> See 2.14.1 Loopback mode
> SETTINGS[ILBP]=1.
>
> in the datasheet
>
> http://canbus.pages.fel.cvut.cz/ctucanfd_ip_core/doc/Datasheet.pdf
BTW: the datasheet says:
| 3.1.36 RX_DATA
|
| ... this register must be read by 32 bit access.
While there is a section that uses 8-bit accessed on that register:
| 2.10.10 Sample code 2 - Frame reception in manual mode (8-bit access)
> There is still missing information which frames are received
> locally and from which buffer they are in the Rx message format,
> but we plan to add that into VHDL design.
>
> In such case, we can switch driver mode and release Tx buffers
> only after corresponding message is read from Rx FIFO and
> fill exact finegrain (10 ns in our current design) timestamps
> to the echo skb. The order of received messages will be seen
> exactly mathing the wire order for both transmitted and received
> messages then. Which I consider as proper solution for the
> most applications including CAN bus analyzers.
>
> So I consider to report HW Tx timestamps for cases where exact,
> precise timestamping is not available for loopback messages
> as problematic because you cannot distinguish if you talk
> with driver and HW with real/precise timestamps support
> or only dummy implementation to make some tools happy.
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 --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 1/3] can: ctucanfd: add HW timestamps to RX and error CAN frames
2022-08-03 9:04 ` Marc Kleine-Budde
@ 2022-08-04 8:08 ` Pavel Pisa
0 siblings, 0 replies; 30+ messages in thread
From: Pavel Pisa @ 2022-08-04 8:08 UTC (permalink / raw)
To: Marc Kleine-Budde, Matej Vasilevski, Ondrej Ille
Cc: Vincent Mailhol, Wolfgang Grandegger, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
Krzysztof Kozlowski, linux-can, netdev, devicetree, Jiri Novak,
Oliver Hartkopp
Hello Marc,
On Wednesday 03 of August 2022 11:04:36 Marc Kleine-Budde wrote:
> On 02.08.2022 09:37:54, Pavel Pisa wrote:
> > See 2.14.1 Loopback mode
> > SETTINGS[ILBP]=1.
> >
> > in the datasheet
> >
> > http://canbus.pages.fel.cvut.cz/ctucanfd_ip_core/doc/Datasheet.pdf
>
> BTW: the datasheet says:
> | 3.1.36 RX_DATA
> |
> | ... this register must be read by 32 bit access.
>
> While there is a section that uses 8-bit accessed on that register:
Congratulation to watchfull reading.
The FPGA design is done so that 32, 16 and 8-bits read writes
are supported. But when you read only part of the RX data
register then you lose rest because FIFO advances to the next
32-bit word. That is reasonable solution for 32-bit systems.
There has been added option in 3.0 version to switch into mode
where FIFO doe not advance when RX_DATA is read, then you can
read it by 8 or 16 bits. If MODES[RXBAM]=0 is set. Then advance
to the next world is requested by COMMAND[RXRPMV] bit.
Ondrej Ille considered that mode for for some embedded
use of the core connected to some small MCU. But this
mode causes great overhead in the driver, multiple reads
of RX_DATA followed by write to to COMMAND. I would consider
only default 32-bit mode for Linux driver.
In the fact, to connect to 16-bit systems, my preference would
be to add option into design to select if Rx FIFO advances
by read of LSB or MSB of the RX_DATA register.
Anyway, command register needs at least 16-bit accesses
for now to correctly command operation, for 8-bit it would
require catch one written byte into latch and combine
it with another one written to the other part and this
would require locking around each command write...
For priority register we need at least 16-bit single write
cycle access when 4 TX buffers are used in FIFO configuration.
I do not intend to complicate driver for all these edge cases
when our target on regular Linux systems is utilize 32-bit
busses...
Actual driver uses only 32-bit access exclusively...
Thanks for the checks and suggestions,
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 1/3] can: ctucanfd: add HW timestamps to RX and error CAN frames
2022-08-02 7:37 ` Pavel Pisa
2022-08-03 9:04 ` Marc Kleine-Budde
@ 2022-08-12 14:35 ` Vincent Mailhol
2022-08-12 15:19 ` Pavel Pisa
1 sibling, 1 reply; 30+ messages in thread
From: Vincent Mailhol @ 2022-08-12 14:35 UTC (permalink / raw)
To: Pavel Pisa
Cc: Matej Vasilevski, Ondrej Ille, Wolfgang Grandegger,
Marc Kleine-Budde, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, linux-can, netdev,
devicetree, Jiri Novak, Oliver Hartkopp
Hi Pavel,
On Tue. 2 Aug. 2022 at 16:38, Pavel Pisa <pisa@cmp.felk.cvut.cz> wrote:
> Hello Vincent,
>
> thanks much for review. I am adding some notices to Tx timestamps
> after your comments
>
> On Tuesday 02 of August 2022 05:43:38 Vincent Mailhol wrote:
> > I just send a series last week which a significant amount of changes
> > for CAN timestamping tree-wide:
> > https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/comm
> >it/?id=12a18d79dc14c80b358dbd26461614b97f2ea4a6
> >
> > I suggest you have a look at this series and harmonize it with the new
> > features (e.g. Hardware TX timestamp).
> >
> > On Tue. 2 Aug. 2022 at 03:52, Matej Vasilevski
> ...
> > > +static int ctucan_hwtstamp_set(struct net_device *dev, struct ifreq
> > > *ifr) +{
> > > + struct ctucan_priv *priv = netdev_priv(dev);
> > > + struct hwtstamp_config cfg;
> > > +
> > > + if (!priv->timestamp_possible)
> > > + return -EOPNOTSUPP;
> > > +
> > > + if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
> > > + return -EFAULT;
> > > +
> > > + if (cfg.flags)
> > > + return -EINVAL;
> > > +
> > > + if (cfg.tx_type != HWTSTAMP_TX_OFF)
> > > + return -ERANGE;
> >
> > I have a great news: your driver now also support hardware TX timestamps:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/comm
> >it/?id=8bdd1112edcd3edce2843e03826204a84a61042d
> >
> > > +
> > > + switch (cfg.rx_filter) {
> > > + case HWTSTAMP_FILTER_NONE:
> > > + priv->timestamp_enabled = false;
> ...
> > > +
> > > + cfg.flags = 0;
> > > + cfg.tx_type = HWTSTAMP_TX_OFF;
> >
> > Hardware TX timestamps are now supported (c.f. supra).
> >
> > > + cfg.rx_filter = priv->timestamp_enabled ? HWTSTAMP_FILTER_ALL :
> > > HWTSTAMP_FILTER_NONE; + return copy_to_user(ifr->ifr_data, &cfg,
> > > sizeof(cfg)) ? -EFAULT : 0; +}
> > > +
> > > +static int ctucan_ioctl(struct net_device *dev, struct ifreq *ifr, int
> > > cmd)
> >
> > Please consider using the generic function can_eth_ioctl_hwts()
> > https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/comm
> >it/?id=90f942c5a6d775bad1be33ba214755314105da4a
> >
> > > +{
> ...
> > > + info->so_timestamping |= SOF_TIMESTAMPING_RX_HARDWARE |
> > > + SOF_TIMESTAMPING_RAW_HARDWARE;
> > > + info->tx_types = BIT(HWTSTAMP_TX_OFF);
> >
> > Hardware TX timestamps are now supported (c.f. supra).
> >
> > > + info->rx_filters = BIT(HWTSTAMP_FILTER_NONE) |
> > > + BIT(HWTSTAMP_FILTER_ALL);
>
>
> I am not sure if it is good idea to report support for hardware
> TX timestamps by all drivers. Precise hardware Tx timestamps
> are important for some CAN applications but they require to be
> exactly/properly aligned with Rx timestamps.
>
> Only some CAN (FD) controllers really support that feature.
> For M-CAN and some others it is realized as another event
> FIFO in addition to Tx and Rx FIFOs.
>
> For CTU CAN FD, we have decided that we do not complicate design
> and driver by separate events channel. We have configurable
> and possibly large Rx FIFO depth which is logical to use for
> analyzer mode and we can use loopback to receive own messages
> timestamped same way as external received ones.
>
> See 2.14.1 Loopback mode
> SETTINGS[ILBP]=1.
>
> in the datasheet
>
> http://canbus.pages.fel.cvut.cz/ctucanfd_ip_core/doc/Datasheet.pdf
>
> There is still missing information which frames are received
> locally and from which buffer they are in the Rx message format,
> but we plan to add that into VHDL design.
>
> In such case, we can switch driver mode and release Tx buffers
> only after corresponding message is read from Rx FIFO and
> fill exact finegrain (10 ns in our current design) timestamps
> to the echo skb. The order of received messages will be seen
> exactly mathing the wire order for both transmitted and received
> messages then. Which I consider as proper solution for the
> most applications including CAN bus analyzers.
>
> So I consider to report HW Tx timestamps for cases where exact,
> precise timestamping is not available for loopback messages
> as problematic because you cannot distinguish if you talk
> with driver and HW with real/precise timestamps support
> or only dummy implementation to make some tools happy.
Thank you for the explanation. I did not know about the nuance about
those different hardware timestamps.
So if I understand correctly, your hardware can deliver two types of
hardware timestamps:
- The "real" one: fine grained with 10 ns precision when the frame
is actually sent
- The "dummy" one: less precise timestamp generated when there is an
event on the device’s Rx or Tx FIFO.
Is this correct?
Are the "real" and the "dummy" timestamps synced on the same quartz?
What is the precision of the "dummy" timestamp? If it in the order of
magnitude of 10µs? For many use cases, this is enough. 10µs represents
roughly a dozen of time quata (more or less depending on the bitrate
and its prescaler).
Actually, I never saw hardware with a timestamp precision below 1µs
(not saying those don't exist, just that I never encountered them).
I am not against what you propose. But my suggestion would be rather
to report both TX and RX timestamps and just document the precision
(i.e. TX has precision with an order of magnitude of 10µs and RX has
precision of 10 ns).
At the end, I let you decide what works the best for you. Just keep in
mind that the micro second precision is already a great achievement
and not many people need the 10 nano second (especially for CAN).
P.S.: I am on holiday, my answers might be delayed :)
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v2 1/3] can: ctucanfd: add HW timestamps to RX and error CAN frames
2022-08-12 14:35 ` Vincent Mailhol
@ 2022-08-12 15:19 ` Pavel Pisa
2022-08-26 22:26 ` Vincent Mailhol
0 siblings, 1 reply; 30+ messages in thread
From: Pavel Pisa @ 2022-08-12 15:19 UTC (permalink / raw)
To: Vincent Mailhol, Matej Vasilevski, Pavel Hronek
Cc: Ondrej Ille, Wolfgang Grandegger, Marc Kleine-Budde,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, linux-can, netdev, devicetree,
Jiri Novak, Oliver Hartkopp
Hello Vincent,
On Friday 12 of August 2022 16:35:30 Vincent Mailhol wrote:
> Hi Pavel,
>
> On Tue. 2 Aug. 2022 at 16:38, Pavel Pisa <pisa@cmp.felk.cvut.cz> wrote:
> > Hello Vincent,
> >
> > thanks much for review. I am adding some notices to Tx timestamps
> > after your comments
> >
> > On Tuesday 02 of August 2022 05:43:38 Vincent Mailhol wrote:
> > > I just send a series last week which a significant amount of changes
> > > for CAN timestamping tree-wide:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/
> > >comm it/?id=12a18d79dc14c80b358dbd26461614b97f2ea4a6
> > >
> > > I suggest you have a look at this series and harmonize it with the new
> > > features (e.g. Hardware TX timestamp).
> > >
> > > On Tue. 2 Aug. 2022 at 03:52, Matej Vasilevski
> >
> > ...
> >
> > > > +static int ctucan_hwtstamp_set(struct net_device *dev, struct ifreq
> > > > *ifr) +{
> > > > + struct ctucan_priv *priv = netdev_priv(dev);
> > > > + struct hwtstamp_config cfg;
> > > > +
> > > > + if (!priv->timestamp_possible)
> > > > + return -EOPNOTSUPP;
> > > > +
> > > > + if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
> > > > + return -EFAULT;
> > > > +
> > > > + if (cfg.flags)
> > > > + return -EINVAL;
> > > > +
> > > > + if (cfg.tx_type != HWTSTAMP_TX_OFF)
> > > > + return -ERANGE;
> > >
> > > I have a great news: your driver now also support hardware TX
> > > timestamps:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/
> > >comm it/?id=8bdd1112edcd3edce2843e03826204a84a61042d
> > >
> > > > +
> > > > + switch (cfg.rx_filter) {
> > > > + case HWTSTAMP_FILTER_NONE:
> > > > + priv->timestamp_enabled = false;
> >
> > ...
> >
> > > > +
> > > > + cfg.flags = 0;
> > > > + cfg.tx_type = HWTSTAMP_TX_OFF;
> > >
> > > Hardware TX timestamps are now supported (c.f. supra).
> > >
> > > > + cfg.rx_filter = priv->timestamp_enabled ? HWTSTAMP_FILTER_ALL
> > > > : HWTSTAMP_FILTER_NONE; + return copy_to_user(ifr->ifr_data,
> > > > &cfg, sizeof(cfg)) ? -EFAULT : 0; +}
> > > > +
> > > > +static int ctucan_ioctl(struct net_device *dev, struct ifreq *ifr,
> > > > int cmd)
> > >
> > > Please consider using the generic function can_eth_ioctl_hwts()
> > > https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/
> > >comm it/?id=90f942c5a6d775bad1be33ba214755314105da4a
> > >
> > > > +{
> >
> > ...
> >
> > > > + info->so_timestamping |= SOF_TIMESTAMPING_RX_HARDWARE |
> > > > + SOF_TIMESTAMPING_RAW_HARDWARE;
> > > > + info->tx_types = BIT(HWTSTAMP_TX_OFF);
> > >
> > > Hardware TX timestamps are now supported (c.f. supra).
> > >
> > > > + info->rx_filters = BIT(HWTSTAMP_FILTER_NONE) |
> > > > + BIT(HWTSTAMP_FILTER_ALL);
> >
> > I am not sure if it is good idea to report support for hardware
> > TX timestamps by all drivers. Precise hardware Tx timestamps
> > are important for some CAN applications but they require to be
> > exactly/properly aligned with Rx timestamps.
> >
> > Only some CAN (FD) controllers really support that feature.
> > For M-CAN and some others it is realized as another event
> > FIFO in addition to Tx and Rx FIFOs.
> >
> > For CTU CAN FD, we have decided that we do not complicate design
> > and driver by separate events channel. We have configurable
> > and possibly large Rx FIFO depth which is logical to use for
> > analyzer mode and we can use loopback to receive own messages
> > timestamped same way as external received ones.
> >
> > See 2.14.1 Loopback mode
> > SETTINGS[ILBP]=1.
> >
> > in the datasheet
> >
> > http://canbus.pages.fel.cvut.cz/ctucanfd_ip_core/doc/Datasheet.pdf
> >
> > There is still missing information which frames are received
> > locally and from which buffer they are in the Rx message format,
> > but we plan to add that into VHDL design.
> >
> > In such case, we can switch driver mode and release Tx buffers
> > only after corresponding message is read from Rx FIFO and
> > fill exact finegrain (10 ns in our current design) timestamps
> > to the echo skb. The order of received messages will be seen
> > exactly mathing the wire order for both transmitted and received
> > messages then. Which I consider as proper solution for the
> > most applications including CAN bus analyzers.
> >
> > So I consider to report HW Tx timestamps for cases where exact,
> > precise timestamping is not available for loopback messages
> > as problematic because you cannot distinguish if you talk
> > with driver and HW with real/precise timestamps support
> > or only dummy implementation to make some tools happy.
>
> Thank you for the explanation. I did not know about the nuance about
> those different hardware timestamps.
>
> So if I understand correctly, your hardware can deliver two types of
> hardware timestamps:
>
> - The "real" one: fine grained with 10 ns precision when the frame
> is actually sent
>
> - The "dummy" one: less precise timestamp generated when there is an
> event on the device’s Rx or Tx FIFO.
>
> Is this correct?
>
> Are the "real" and the "dummy" timestamps synced on the same quartz?
>
> What is the precision of the "dummy" timestamp? If it in the order of
> magnitude of 10µs? For many use cases, this is enough. 10µs represents
> roughly a dozen of time quata (more or less depending on the bitrate
> and its prescaler).
> Actually, I never saw hardware with a timestamp precision below 1µs
> (not saying those don't exist, just that I never encountered them).
>
> I am not against what you propose. But my suggestion would be rather
> to report both TX and RX timestamps and just document the precision
> (i.e. TX has precision with an order of magnitude of 10µs and RX has
> precision of 10 ns).
>
> At the end, I let you decide what works the best for you. Just keep in
> mind that the micro second precision is already a great achievement
> and not many people need the 10 nano second (especially for CAN).
>
> P.S.: I am on holiday, my answers might be delayed :)
I am leaving off the Internet for next week as well now...
My discussion has been reaction to your information about your
CTU CAN FD change, but may it be I have lost the track.
> > On Tuesday 02 of August 2022 05:43:38 Vincent Mailhol wrote:
> > > I have a great news: your driver now also support hardware TX
> > > timestamps:
Our actual/mainline driver actually does not support neither Rx nor Tx
timestamps. Matej Vasilevski has prepared and sent to review patches
adding Rx timestamping (10 ns resolution for our actual designs).
He has rebased his changes above yours... CTU CAN FD hardware
supports such timestamping for many years... probably preceding 2.0
IP core version.
But even when this patch is clean up and accepted into mainline,
CTU CAN FD driver will not support hardware Tx timestams, may it
be software ones are implemented in generic CAN echo code, not checked
now... So if your change add report of HW Tx stamps then it would be
problem to distinguish situation when we implement hardware Tx timestamps.
The rest of the previous text is how to implement precise Tx timestamps
on other and our controller design. We do not have separate queue
to report Tx timestamps for successfully sent frames. But we can
enable copy of sent Tx frames to Rx FIFO so there is a way how
to achieve that. But there is still minor design detail that
we need to mark such frames as echo of local Tx in Rx FIFO queue
and ideally add there even number of the Tx buffer or even some
user provided information from some Tx buffer filed to distinguish
that such frames should be reported through echo and ensure that
they are not reported to that client who has sent them etc...
But there are our implementation details...
But what worries me, is your statement that HW Tx timestamps
are already reported as available on CTU CAN FD by your patch,
if I understood your reply well.
Best wishes,
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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v2 1/3] can: ctucanfd: add HW timestamps to RX and error CAN frames
2022-08-12 15:19 ` Pavel Pisa
@ 2022-08-26 22:26 ` Vincent Mailhol
0 siblings, 0 replies; 30+ messages in thread
From: Vincent Mailhol @ 2022-08-26 22:26 UTC (permalink / raw)
To: Pavel Pisa
Cc: Matej Vasilevski, Pavel Hronek, Ondrej Ille, Wolfgang Grandegger,
Marc Kleine-Budde, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, linux-can, netdev,
devicetree, Jiri Novak, Oliver Hartkopp
On Mon. 13 Aug. 2022 at 00:20, Pavel Pisa <pisa@cmp.felk.cvut.cz> wrote:
> Hello Vincent,
>
> On Friday 12 of August 2022 16:35:30 Vincent Mailhol wrote:
> > Hi Pavel,
> >
> > On Tue. 2 Aug. 2022 at 16:38, Pavel Pisa <pisa@cmp.felk.cvut.cz> wrote:
> > > Hello Vincent,
> > >
> > > thanks much for review. I am adding some notices to Tx timestamps
> > > after your comments
> > >
> > > On Tuesday 02 of August 2022 05:43:38 Vincent Mailhol wrote:
> > > > I just send a series last week which a significant amount of changes
> > > > for CAN timestamping tree-wide:
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/
> > > >comm it/?id=12a18d79dc14c80b358dbd26461614b97f2ea4a6
> > > >
> > > > I suggest you have a look at this series and harmonize it with the new
> > > > features (e.g. Hardware TX timestamp).
> > > >
> > > > On Tue. 2 Aug. 2022 at 03:52, Matej Vasilevski
> > >
> > > ...
> > >
> > > > > +static int ctucan_hwtstamp_set(struct net_device *dev, struct ifreq
> > > > > *ifr) +{
> > > > > + struct ctucan_priv *priv = netdev_priv(dev);
> > > > > + struct hwtstamp_config cfg;
> > > > > +
> > > > > + if (!priv->timestamp_possible)
> > > > > + return -EOPNOTSUPP;
> > > > > +
> > > > > + if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
> > > > > + return -EFAULT;
> > > > > +
> > > > > + if (cfg.flags)
> > > > > + return -EINVAL;
> > > > > +
> > > > > + if (cfg.tx_type != HWTSTAMP_TX_OFF)
> > > > > + return -ERANGE;
> > > >
> > > > I have a great news: your driver now also support hardware TX
> > > > timestamps:
> > > >
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/
> > > >comm it/?id=8bdd1112edcd3edce2843e03826204a84a61042d
> > > >
> > > > > +
> > > > > + switch (cfg.rx_filter) {
> > > > > + case HWTSTAMP_FILTER_NONE:
> > > > > + priv->timestamp_enabled = false;
> > >
> > > ...
> > >
> > > > > +
> > > > > + cfg.flags = 0;
> > > > > + cfg.tx_type = HWTSTAMP_TX_OFF;
> > > >
> > > > Hardware TX timestamps are now supported (c.f. supra).
> > > >
> > > > > + cfg.rx_filter = priv->timestamp_enabled ? HWTSTAMP_FILTER_ALL
> > > > > : HWTSTAMP_FILTER_NONE; + return copy_to_user(ifr->ifr_data,
> > > > > &cfg, sizeof(cfg)) ? -EFAULT : 0; +}
> > > > > +
> > > > > +static int ctucan_ioctl(struct net_device *dev, struct ifreq *ifr,
> > > > > int cmd)
> > > >
> > > > Please consider using the generic function can_eth_ioctl_hwts()
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/
> > > >comm it/?id=90f942c5a6d775bad1be33ba214755314105da4a
> > > >
> > > > > +{
> > >
> > > ...
> > >
> > > > > + info->so_timestamping |= SOF_TIMESTAMPING_RX_HARDWARE |
> > > > > + SOF_TIMESTAMPING_RAW_HARDWARE;
> > > > > + info->tx_types = BIT(HWTSTAMP_TX_OFF);
> > > >
> > > > Hardware TX timestamps are now supported (c.f. supra).
> > > >
> > > > > + info->rx_filters = BIT(HWTSTAMP_FILTER_NONE) |
> > > > > + BIT(HWTSTAMP_FILTER_ALL);
> > >
> > > I am not sure if it is good idea to report support for hardware
> > > TX timestamps by all drivers. Precise hardware Tx timestamps
> > > are important for some CAN applications but they require to be
> > > exactly/properly aligned with Rx timestamps.
> > >
> > > Only some CAN (FD) controllers really support that feature.
> > > For M-CAN and some others it is realized as another event
> > > FIFO in addition to Tx and Rx FIFOs.
> > >
> > > For CTU CAN FD, we have decided that we do not complicate design
> > > and driver by separate events channel. We have configurable
> > > and possibly large Rx FIFO depth which is logical to use for
> > > analyzer mode and we can use loopback to receive own messages
> > > timestamped same way as external received ones.
> > >
> > > See 2.14.1 Loopback mode
> > > SETTINGS[ILBP]=1.
> > >
> > > in the datasheet
> > >
> > > http://canbus.pages.fel.cvut.cz/ctucanfd_ip_core/doc/Datasheet.pdf
> > >
> > > There is still missing information which frames are received
> > > locally and from which buffer they are in the Rx message format,
> > > but we plan to add that into VHDL design.
> > >
> > > In such case, we can switch driver mode and release Tx buffers
> > > only after corresponding message is read from Rx FIFO and
> > > fill exact finegrain (10 ns in our current design) timestamps
> > > to the echo skb. The order of received messages will be seen
> > > exactly mathing the wire order for both transmitted and received
> > > messages then. Which I consider as proper solution for the
> > > most applications including CAN bus analyzers.
> > >
> > > So I consider to report HW Tx timestamps for cases where exact,
> > > precise timestamping is not available for loopback messages
> > > as problematic because you cannot distinguish if you talk
> > > with driver and HW with real/precise timestamps support
> > > or only dummy implementation to make some tools happy.
> >
> > Thank you for the explanation. I did not know about the nuance about
> > those different hardware timestamps.
> >
> > So if I understand correctly, your hardware can deliver two types of
> > hardware timestamps:
> >
> > - The "real" one: fine grained with 10 ns precision when the frame
> > is actually sent
> >
> > - The "dummy" one: less precise timestamp generated when there is an
> > event on the device’s Rx or Tx FIFO.
> >
> > Is this correct?
> >
> > Are the "real" and the "dummy" timestamps synced on the same quartz?
> >
> > What is the precision of the "dummy" timestamp? If it in the order of
> > magnitude of 10µs? For many use cases, this is enough. 10µs represents
> > roughly a dozen of time quata (more or less depending on the bitrate
> > and its prescaler).
> > Actually, I never saw hardware with a timestamp precision below 1µs
> > (not saying those don't exist, just that I never encountered them).
> >
> > I am not against what you propose. But my suggestion would be rather
> > to report both TX and RX timestamps and just document the precision
> > (i.e. TX has precision with an order of magnitude of 10µs and RX has
> > precision of 10 ns).
> >
> > At the end, I let you decide what works the best for you. Just keep in
> > mind that the micro second precision is already a great achievement
> > and not many people need the 10 nano second (especially for CAN).
> >
> > P.S.: I am on holiday, my answers might be delayed :)
>
> I am leaving off the Internet for next week as well now...
>
> My discussion has been reaction to your information about your
> CTU CAN FD change, but may it be I have lost the track.
>
> > > On Tuesday 02 of August 2022 05:43:38 Vincent Mailhol wrote:
> > > > I have a great news: your driver now also support hardware TX
> > > > timestamps:
>
> Our actual/mainline driver actually does not support neither Rx nor Tx
> timestamps. Matej Vasilevski has prepared and sent to review patches
> adding Rx timestamping (10 ns resolution for our actual designs).
> He has rebased his changes above yours... CTU CAN FD hardware
> supports such timestamping for many years... probably preceding 2.0
> IP core version.
>
> But even when this patch is clean up and accepted into mainline,
> CTU CAN FD driver will not support hardware Tx timestams, may it
> be software ones are implemented in generic CAN echo code, not checked
> now... So if your change add report of HW Tx stamps then it would be
> problem to distinguish situation when we implement hardware Tx timestamps.
>
> The rest of the previous text is how to implement precise Tx timestamps
> on other and our controller design. We do not have separate queue
> to report Tx timestamps for successfully sent frames. But we can
> enable copy of sent Tx frames to Rx FIFO so there is a way how
> to achieve that. But there is still minor design detail that
> we need to mark such frames as echo of local Tx in Rx FIFO queue
> and ideally add there even number of the Tx buffer or even some
> user provided information from some Tx buffer filed to distinguish
> that such frames should be reported through echo and ensure that
> they are not reported to that client who has sent them etc...
> But there are our implementation details...
>
> But what worries me, is your statement that HW Tx timestamps
> are already reported as available on CTU CAN FD by your patch,
> if I understood your reply well.
I read again the full exchange, and you were right from the beginning.
Please forget my comments on the hardware TX timestamps, I just
misread you.
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 1/3] can: ctucanfd: add HW timestamps to RX and error CAN frames
2022-08-01 18:46 ` [PATCH v2 1/3] can: ctucanfd: add HW timestamps to RX and error CAN frames Matej Vasilevski
2022-08-01 20:42 ` Pavel Pisa
2022-08-02 3:43 ` Vincent Mailhol
@ 2022-08-02 9:29 ` Marc Kleine-Budde
2022-08-02 10:26 ` Marc Kleine-Budde
` (2 more replies)
2 siblings, 3 replies; 30+ messages in thread
From: Marc Kleine-Budde @ 2022-08-02 9:29 UTC (permalink / raw)
To: Matej Vasilevski
Cc: Pavel Pisa, Ondrej Ille, Wolfgang Grandegger, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
Krzysztof Kozlowski, linux-can, netdev, devicetree
[-- Attachment #1: Type: text/plain, Size: 15896 bytes --]
On 01.08.2022 20:46:54, 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_clk" 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>
> ---
> drivers/net/can/ctucanfd/Makefile | 2 +-
> drivers/net/can/ctucanfd/ctucanfd.h | 20 ++
> drivers/net/can/ctucanfd/ctucanfd_base.c | 214 +++++++++++++++++-
> drivers/net/can/ctucanfd/ctucanfd_timestamp.c | 87 +++++++
> 4 files changed, 315 insertions(+), 8 deletions(-)
> create mode 100644 drivers/net/can/ctucanfd/ctucanfd_timestamp.c
>
> diff --git a/drivers/net/can/ctucanfd/Makefile b/drivers/net/can/ctucanfd/Makefile
> index 8078f1f2c30f..a36e66f2cea7 100644
> --- a/drivers/net/can/ctucanfd/Makefile
> +++ b/drivers/net/can/ctucanfd/Makefile
> @@ -4,7 +4,7 @@
> #
>
> obj-$(CONFIG_CAN_CTUCANFD) := ctucanfd.o
> -ctucanfd-y := ctucanfd_base.o
> +ctucanfd-y := ctucanfd_base.o ctucanfd_timestamp.o
>
> obj-$(CONFIG_CAN_CTUCANFD_PCI) += ctucanfd_pci.o
> obj-$(CONFIG_CAN_CTUCANFD_PLATFORM) += ctucanfd_platform.o
> diff --git a/drivers/net/can/ctucanfd/ctucanfd.h b/drivers/net/can/ctucanfd/ctucanfd.h
> index 0e9904f6a05d..43d9c73ce244 100644
> --- a/drivers/net/can/ctucanfd/ctucanfd.h
> +++ b/drivers/net/can/ctucanfd/ctucanfd.h
> @@ -23,6 +23,10 @@
> #include <linux/netdevice.h>
> #include <linux/can/dev.h>
> #include <linux/list.h>
> +#include <linux/timecounter.h>
> +#include <linux/workqueue.h>
> +
> +#define CTUCANFD_MAX_WORK_DELAY_SEC 86400U /* one day == 24 * 3600 seconds */
For higher precision we can limit this to 3600s, as the sched_clock does
it
:
| https://elixir.bootlin.com/linux/v5.19/source/kernel/time/sched_clock.c#L169
> enum ctu_can_fd_can_registers;
>
> @@ -51,6 +55,15 @@ struct ctucan_priv {
> u32 rxfrm_first_word;
>
> struct list_head peers_on_pdev;
> +
> + struct cyclecounter cc;
> + struct timecounter tc;
> + struct delayed_work timestamp;
> +
> + struct clk *timestamp_clk;
> + u32 work_delay_jiffies;
schedule_delayed_work() takes an "unsigned long" not a u32.
> + bool timestamp_enabled;
> + bool timestamp_possible;
> };
>
> /**
> @@ -79,4 +92,11 @@ int ctucan_probe_common(struct device *dev, void __iomem *addr,
> int ctucan_suspend(struct device *dev) __maybe_unused;
> int ctucan_resume(struct device *dev) __maybe_unused;
>
> +u64 ctucan_read_timestamp_cc_wrapper(const struct cyclecounter *cc);
> +u64 ctucan_read_timestamp_counter(struct ctucan_priv *priv);
> +u32 ctucan_calculate_work_delay(const u32 timestamp_bit_size, const u32 timestamp_freq);
> +void ctucan_skb_set_timestamp(const struct ctucan_priv *priv, struct sk_buff *skb,
> + u64 timestamp);
> +void ctucan_timestamp_init(struct ctucan_priv *priv);
> +void ctucan_timestamp_stop(struct ctucan_priv *priv);
> #endif /*__CTUCANFD__*/
> diff --git a/drivers/net/can/ctucanfd/ctucanfd_base.c b/drivers/net/can/ctucanfd/ctucanfd_base.c
> index 3c18d028bd8c..35b37de51811 100644
> --- a/drivers/net/can/ctucanfd/ctucanfd_base.c
> +++ b/drivers/net/can/ctucanfd/ctucanfd_base.c
> @@ -18,6 +18,7 @@
> ******************************************************************************/
>
> #include <linux/clk.h>
> +#include <linux/clocksource.h>
> #include <linux/errno.h>
> #include <linux/ethtool.h>
> #include <linux/init.h>
> @@ -148,6 +149,27 @@ static void ctucan_write_txt_buf(struct ctucan_priv *priv, enum ctu_can_fd_can_r
> priv->write_reg(priv, buf_base + offset, val);
> }
>
> +static u64 concatenate_two_u32(u32 high, u32 low)
static inline?
> +{
> + return ((u64)high << 32) | ((u64)low);
> +}
> +
> +u64 ctucan_read_timestamp_counter(struct ctucan_priv *priv)
> +{
> + u32 ts_low;
> + u32 ts_high;
> + u32 ts_high2;
> +
> + ts_high = ctucan_read32(priv, CTUCANFD_TIMESTAMP_HIGH);
> + ts_low = ctucan_read32(priv, CTUCANFD_TIMESTAMP_LOW);
> + ts_high2 = ctucan_read32(priv, CTUCANFD_TIMESTAMP_HIGH);
> +
> + if (ts_high2 != ts_high)
> + ts_low = priv->read_reg(priv, CTUCANFD_TIMESTAMP_LOW);
> +
> + return concatenate_two_u32(ts_high2, ts_low) & priv->cc.mask;
> +}
> +
> #define CTU_CAN_FD_TXTNF(priv) (!!FIELD_GET(REG_STATUS_TXNF, ctucan_read32(priv, CTUCANFD_STATUS)))
> #define CTU_CAN_FD_ENABLED(priv) (!!FIELD_GET(REG_MODE_ENA, ctucan_read32(priv, CTUCANFD_MODE)))
please make these static inline bool functions.
>
> @@ -640,12 +662,16 @@ static netdev_tx_t ctucan_start_xmit(struct sk_buff *skb, struct net_device *nde
> * @priv: Pointer to CTU CAN FD's private data
> * @cf: Pointer to CAN frame struct
> * @ffw: Previously read frame format word
> + * @skb: Pointer to buffer to store timestamp
> *
> * Note: Frame format word must be read separately and provided in 'ffw'.
> */
> -static void ctucan_read_rx_frame(struct ctucan_priv *priv, struct canfd_frame *cf, u32 ffw)
> +static void ctucan_read_rx_frame(struct ctucan_priv *priv, struct canfd_frame *cf,
> + u32 ffw, u64 *timestamp)
> {
> u32 idw;
> + u32 tstamp_high;
> + u32 tstamp_low;
> unsigned int i;
> unsigned int wc;
> unsigned int len;
> @@ -682,9 +708,10 @@ static void ctucan_read_rx_frame(struct ctucan_priv *priv, struct canfd_frame *c
> if (unlikely(len > wc * 4))
> len = wc * 4;
>
> - /* Timestamp - Read and throw away */
> - ctucan_read32(priv, CTUCANFD_RX_DATA);
> - ctucan_read32(priv, CTUCANFD_RX_DATA);
> + /* Timestamp */
> + tstamp_low = ctucan_read32(priv, CTUCANFD_RX_DATA);
> + tstamp_high = ctucan_read32(priv, CTUCANFD_RX_DATA);
> + *timestamp = concatenate_two_u32(tstamp_high, tstamp_low) & priv->cc.mask;
>
> /* Data */
> for (i = 0; i < len; i += 4) {
> @@ -713,6 +740,7 @@ static int ctucan_rx(struct net_device *ndev)
> struct net_device_stats *stats = &ndev->stats;
> struct canfd_frame *cf;
> struct sk_buff *skb;
> + u64 timestamp;
> u32 ffw;
>
> if (test_bit(CTUCANFD_FLAG_RX_FFW_BUFFERED, &priv->drv_flags)) {
> @@ -736,7 +764,9 @@ static int ctucan_rx(struct net_device *ndev)
> return 0;
> }
>
> - ctucan_read_rx_frame(priv, cf, ffw);
> + ctucan_read_rx_frame(priv, cf, ffw, ×tamp);
> + if (priv->timestamp_enabled)
> + ctucan_skb_set_timestamp(priv, skb, timestamp);
Can the ctucan_skb_set_timestamp() and ctucan_read_timestamp_counter()
happen concurrently? AFAICS they are all called from ctucan_rx_poll(),
right?
>
> stats->rx_bytes += cf->len;
> stats->rx_packets++;
> @@ -906,6 +936,11 @@ static void ctucan_err_interrupt(struct net_device *ndev, u32 isr)
> if (skb) {
> stats->rx_packets++;
> stats->rx_bytes += cf->can_dlc;
> + if (priv->timestamp_enabled) {
> + u64 tstamp = ctucan_read_timestamp_counter(priv);
> +
> + ctucan_skb_set_timestamp(priv, skb, tstamp);
> + }
> netif_rx(skb);
> }
> }
> @@ -951,6 +986,11 @@ static int ctucan_rx_poll(struct napi_struct *napi, int quota)
> cf->data[1] |= CAN_ERR_CRTL_RX_OVERFLOW;
> stats->rx_packets++;
> stats->rx_bytes += cf->can_dlc;
> + if (priv->timestamp_enabled) {
> + u64 tstamp = ctucan_read_timestamp_counter(priv);
> +
> + ctucan_skb_set_timestamp(priv, skb, tstamp);
> + }
> netif_rx(skb);
> }
>
> @@ -1231,6 +1271,9 @@ static int ctucan_open(struct net_device *ndev)
> goto err_chip_start;
> }
>
> + if (priv->timestamp_possible)
> + ctucan_timestamp_init(priv);
> +
> netdev_info(ndev, "ctu_can_fd device registered\n");
> napi_enable(&priv->napi);
> netif_start_queue(ndev);
> @@ -1263,6 +1306,9 @@ static int ctucan_close(struct net_device *ndev)
> ctucan_chip_stop(ndev);
> free_irq(ndev->irq, ndev);
> close_candev(ndev);
> + if (priv->timestamp_possible)
> + ctucan_timestamp_stop(priv);
> +
Nitpick: Don't add an extra newline here.
>
> pm_runtime_put(priv->dev);
>
> @@ -1295,15 +1341,117 @@ static int ctucan_get_berr_counter(const struct net_device *ndev, struct can_ber
> return 0;
> }
>
> +static int ctucan_hwtstamp_set(struct net_device *dev, struct ifreq *ifr)
> +{
> + struct ctucan_priv *priv = netdev_priv(dev);
> + struct hwtstamp_config cfg;
> +
> + if (!priv->timestamp_possible)
> + return -EOPNOTSUPP;
> +
> + if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
> + return -EFAULT;
> +
> + if (cfg.flags)
> + return -EINVAL;
> +
> + if (cfg.tx_type != HWTSTAMP_TX_OFF)
> + return -ERANGE;
> +
> + switch (cfg.rx_filter) {
> + case HWTSTAMP_FILTER_NONE:
> + priv->timestamp_enabled = false;
> + break;
> + case HWTSTAMP_FILTER_ALL:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V1_L4_EVENT:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V2_EVENT:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V2_SYNC:
> + fallthrough;
> + case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
> + priv->timestamp_enabled = true;
> + cfg.rx_filter = HWTSTAMP_FILTER_ALL;
> + break;
> + default:
> + return -ERANGE;
> + }
> +
> + return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
> +}
> +
> +static int ctucan_hwtstamp_get(struct net_device *dev, struct ifreq *ifr)
> +{
> + struct ctucan_priv *priv = netdev_priv(dev);
> + struct hwtstamp_config cfg;
> +
> + if (!priv->timestamp_possible)
> + return -EOPNOTSUPP;
> +
> + cfg.flags = 0;
> + cfg.tx_type = HWTSTAMP_TX_OFF;
> + cfg.rx_filter = priv->timestamp_enabled ? HWTSTAMP_FILTER_ALL : HWTSTAMP_FILTER_NONE;
> +
> + return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0;
> +}
> +
> +static int ctucan_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> +{
> + switch (cmd) {
> + case SIOCSHWTSTAMP:
> + return ctucan_hwtstamp_set(dev, ifr);
> + case SIOCGHWTSTAMP:
> + return ctucan_hwtstamp_get(dev, ifr);
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static int ctucan_ethtool_get_ts_info(struct net_device *ndev, struct ethtool_ts_info *info)
> +{
> + struct ctucan_priv *priv = netdev_priv(ndev);
> +
> + ethtool_op_get_ts_info(ndev, info);
> +
> + if (!priv->timestamp_possible)
> + return 0;
> +
> + info->so_timestamping |= SOF_TIMESTAMPING_RX_HARDWARE |
> + SOF_TIMESTAMPING_RAW_HARDWARE;
> + info->tx_types = BIT(HWTSTAMP_TX_OFF);
> + info->rx_filters = BIT(HWTSTAMP_FILTER_NONE) |
> + BIT(HWTSTAMP_FILTER_ALL);
> +
> + return 0;
> +}
> +
> static const struct net_device_ops ctucan_netdev_ops = {
> .ndo_open = ctucan_open,
> .ndo_stop = ctucan_close,
> .ndo_start_xmit = ctucan_start_xmit,
> .ndo_change_mtu = can_change_mtu,
> + .ndo_eth_ioctl = ctucan_ioctl,
> };
>
> static const struct ethtool_ops ctucan_ethtool_ops = {
> - .get_ts_info = ethtool_op_get_ts_info,
> + .get_ts_info = ctucan_ethtool_get_ts_info,
> };
>
> int ctucan_suspend(struct device *dev)
> @@ -1345,6 +1493,8 @@ int ctucan_probe_common(struct device *dev, void __iomem *addr, int irq, unsigne
> struct ctucan_priv *priv;
> struct net_device *ndev;
> int ret;
> + u32 timestamp_freq = 0;
> + u32 timestamp_bit_size = 0;
Nitpick: please move the u32 between the struct and the int.
>
> /* Create a CAN device instance */
> ndev = alloc_candev(sizeof(struct ctucan_priv), ntxbufs);
> @@ -1386,7 +1536,9 @@ int ctucan_probe_common(struct device *dev, void __iomem *addr, int irq, unsigne
>
> /* Getting the can_clk info */
> if (!can_clk_rate) {
> - priv->can_clk = devm_clk_get(dev, NULL);
> + priv->can_clk = devm_clk_get_optional(dev, "core-clk");
> + if (!priv->can_clk)
> + priv->can_clk = devm_clk_get(dev, NULL);
Please add a comment here, that the NULL clock is for compatibility with
older DTs.
> if (IS_ERR(priv->can_clk)) {
> dev_err(dev, "Device clock not found.\n");
> ret = PTR_ERR(priv->can_clk);
> @@ -1425,6 +1577,54 @@ int ctucan_probe_common(struct device *dev, void __iomem *addr, int irq, unsigne
>
> priv->can.clock.freq = can_clk_rate;
>
> + priv->timestamp_enabled = false;
> + priv->timestamp_possible = true;
> + priv->timestamp_clk = NULL;
priv is allocated and initialized with 0
> +
> + /* Obtain timestamping frequency */
> + if (pm_enable_call) {
> + /* Plaftorm device: get tstamp clock from device tree */
> + priv->timestamp_clk = devm_clk_get(dev, "ts-clk");
> + if (IS_ERR(priv->timestamp_clk)) {
> + /* Take the core clock frequency instead */
> + timestamp_freq = can_clk_rate;
> + } else {
> + timestamp_freq = clk_get_rate(priv->timestamp_clk);
> + }
Who prepares/enabled the timestamp clock? clk_get_rate() is only valid if
the clock is enabled. I know, we violate this for the CAN clock. :/
> + } else {
> + /* PCI device: assume tstamp freq is equal to bus clk rate */
> + timestamp_freq = can_clk_rate;
> + }
> +
> + /* Obtain timestamping counter bit size */
> + timestamp_bit_size = (ctucan_read32(priv, CTUCANFD_ERR_CAPT) & REG_ERR_CAPT_TS_BITS) >> 24;
> + timestamp_bit_size += 1; /* the register value was bit_size - 1 */
Please move the +1 into the else of the following if() which results in:
| if (timestamp_bit_size)
which is IMHO easier to read.
> +
> + /* For 2.x versions of the IP core, we will assume 64-bit counter
> + * if there was a 0 in the register.
> + */
> + if (timestamp_bit_size == 1) {
> + u32 version_reg = ctucan_read32(priv, CTUCANFD_DEVICE_ID);
> + u32 major = (version_reg & REG_DEVICE_ID_VER_MAJOR) >> 24;
> +
> + if (major == 2)
> + timestamp_bit_size = 64;
> + else
> + priv->timestamp_possible = false;
> + }
> +
> + /* Setup conversion constants and work delay */
> + priv->cc.read = ctucan_read_timestamp_cc_wrapper;
> + priv->cc.mask = CYCLECOUNTER_MASK(timestamp_bit_size);
Does the driver use these 2 if timestamping is not possible?
> + if (priv->timestamp_possible) {
> + clocks_calc_mult_shift(&priv->cc.mult, &priv->cc.shift, timestamp_freq,
> + NSEC_PER_SEC, CTUCANFD_MAX_WORK_DELAY_SEC);
> + priv->work_delay_jiffies =
> + ctucan_calculate_work_delay(timestamp_bit_size, timestamp_freq);
> + if (priv->work_delay_jiffies == 0)
> + priv->timestamp_possible = false;
You'll get a higher precision if you take the mask into account, at
least if the counter overflows before CTUCANFD_MAX_WORK_DELAY_SEC:
maxsec = min(CTUCANFD_MAX_WORK_DELAY_SEC, priv->cc.mask / timestamp_freq);
clocks_calc_mult_shift(&priv->cc.mult, &priv->cc.shift, timestamp_freq, NSEC_PER_SEC, maxsec);
work_delay_in_ns = clocks_calc_max_nsecs(&priv->cc.mult, &priv->cc.shift, 0, &priv->cc.mask, NULL);
You can use clocks_calc_max_nsecs() to calculate the work delay.
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 --]
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v2 1/3] can: ctucanfd: add HW timestamps to RX and error CAN frames
2022-08-02 9:29 ` Marc Kleine-Budde
@ 2022-08-02 10:26 ` Marc Kleine-Budde
2022-08-02 16:20 ` Pavel Pisa
2022-08-03 0:09 ` Matej Vasilevski
2 siblings, 0 replies; 30+ messages in thread
From: Marc Kleine-Budde @ 2022-08-02 10:26 UTC (permalink / raw)
To: Matej Vasilevski
Cc: Pavel Pisa, Ondrej Ille, Wolfgang Grandegger, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
Krzysztof Kozlowski, linux-can, netdev, devicetree
[-- Attachment #1: Type: text/plain, Size: 662 bytes --]
On 02.08.2022 11:29:07, Marc Kleine-Budde wrote:
> > #define CTU_CAN_FD_TXTNF(priv) (!!FIELD_GET(REG_STATUS_TXNF, ctucan_read32(priv, CTUCANFD_STATUS)))
> > #define CTU_CAN_FD_ENABLED(priv) (!!FIELD_GET(REG_MODE_ENA, ctucan_read32(priv, CTUCANFD_MODE)))
>
> please make these static inline bool functions.
Ignore this comment - these were already in the driver.
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 --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 1/3] can: ctucanfd: add HW timestamps to RX and error CAN frames
2022-08-02 9:29 ` Marc Kleine-Budde
2022-08-02 10:26 ` Marc Kleine-Budde
@ 2022-08-02 16:20 ` Pavel Pisa
2022-08-03 8:37 ` Marc Kleine-Budde
2022-08-03 0:09 ` Matej Vasilevski
2 siblings, 1 reply; 30+ messages in thread
From: Pavel Pisa @ 2022-08-02 16:20 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: Matej Vasilevski, Ondrej Ille, Wolfgang Grandegger,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, linux-can, netdev, devicetree
Hello Marc,
thanks for feedback.
On Tuesday 02 of August 2022 11:29:07 Marc Kleine-Budde wrote:
> On 01.08.2022 20:46:54, 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_clk" 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>
> > ---
> > drivers/net/can/ctucanfd/Makefile | 2 +-
> > drivers/net/can/ctucanfd/ctucanfd.h | 20 ++
> > drivers/net/can/ctucanfd/ctucanfd_base.c | 214 +++++++++++++++++-
> > drivers/net/can/ctucanfd/ctucanfd_timestamp.c | 87 +++++++
> > 4 files changed, 315 insertions(+), 8 deletions(-)
> > create mode 100644 drivers/net/can/ctucanfd/ctucanfd_timestamp.c
...
> > + if (ts_high2 != ts_high)
> > + ts_low = priv->read_reg(priv, CTUCANFD_TIMESTAMP_LOW);
> > +
> > + return concatenate_two_u32(ts_high2, ts_low) & priv->cc.mask;
> > +}
> > +
> > #define CTU_CAN_FD_TXTNF(priv) (!!FIELD_GET(REG_STATUS_TXNF,
> > ctucan_read32(priv, CTUCANFD_STATUS))) #define CTU_CAN_FD_ENABLED(priv)
> > (!!FIELD_GET(REG_MODE_ENA, ctucan_read32(priv, CTUCANFD_MODE)))
>
> please make these static inline bool functions.
We put that to TODO list. But I prefer to prepare separate followup
patch later.
> > @@ -736,7 +764,9 @@ static int ctucan_rx(struct net_device *ndev)
> > return 0;
> > }
> >
> > - ctucan_read_rx_frame(priv, cf, ffw);
> > + ctucan_read_rx_frame(priv, cf, ffw, ×tamp);
> > + if (priv->timestamp_enabled)
> > + ctucan_skb_set_timestamp(priv, skb, timestamp);
>
> Can the ctucan_skb_set_timestamp() and ctucan_read_timestamp_counter()
> happen concurrently? AFAICS they are all called from ctucan_rx_poll(),
> right?
I am not sure about which possible problem do you think.
But ctucan_read_timestamp_counter() is fully reentrant
and has no side effect on the core. So there is no
problem.
> >
> > /* Getting the can_clk info */
> > if (!can_clk_rate) {
> > - priv->can_clk = devm_clk_get(dev, NULL);
> > + priv->can_clk = devm_clk_get_optional(dev, "core-clk");
> > + if (!priv->can_clk)
> > + priv->can_clk = devm_clk_get(dev, NULL);
>
> Please add a comment here, that the NULL clock is for compatibility with
> older DTs.
yes we need that for compatability with older devicetree builds
and I even consider even to keep option of simple DTS with single
clock without specific name even for future.
> > if (IS_ERR(priv->can_clk)) {
> > dev_err(dev, "Device clock not found.\n");
> > ret = PTR_ERR(priv->can_clk);
> > @@ -1425,6 +1577,54 @@ int ctucan_probe_common(struct device *dev, void
> > __iomem *addr, int irq, unsigne
> >
> > priv->can.clock.freq = can_clk_rate;
> >
> > + priv->timestamp_enabled = false;
> > + priv->timestamp_possible = true;
> > + priv->timestamp_clk = NULL;
>
> priv is allocated and initialized with 0
My personal low weight/priority opinion is to have this
visualized, reminded in the code. But I understand that
this add some unnecessary instructions...
> > +
> > + /* Obtain timestamping frequency */
> > + if (pm_enable_call) {
> > + /* Plaftorm device: get tstamp clock from device tree */
> > + priv->timestamp_clk = devm_clk_get(dev, "ts-clk");
> > + if (IS_ERR(priv->timestamp_clk)) {
> > + /* Take the core clock frequency instead */
> > + timestamp_freq = can_clk_rate;
> > + } else {
> > + timestamp_freq = clk_get_rate(priv->timestamp_clk);
> > + }
>
> Who prepares/enabled the timestamp clock? clk_get_rate() is only valid if
> the clock is enabled. I know, we violate this for the CAN clock. :/
Yes, I have noticed that we miss clk_prepare_enable() in the
ctucan_probe_common() and clk_disable_unprepare() in ctucan_platform_remove().
The need for clock running should be released in ctucan_suspend()
and regained in ctucan_resume(). I see that the most CAN drivers
use there clk_disable_unprepare/clk_prepare_enable but I am not
sure, if this is right. Ma be plain clk_disable/clk_enable should
be used for suspend and resume because as I understand, the clock
frequency can be recomputed and reset during clk_prepare which
would require to recompute bitrate. Do you have some advice
what is a right option there?
Actual omission is no problem on our systems, be the clock are used
in whole FPGA system with AXI connection and has to running already
and we use same for timestamping.
I would prefer to allow timestamping patch as it is without clock enable
and then correct clock enable, disable by another patch for both ts and core
clocks.
> > + } else {
> > + /* PCI device: assume tstamp freq is equal to bus clk rate */
> > + timestamp_freq = can_clk_rate;
> > + }
> > +
> > + /* Obtain timestamping counter bit size */
> > + timestamp_bit_size = (ctucan_read32(priv, CTUCANFD_ERR_CAPT) &
> > REG_ERR_CAPT_TS_BITS) >> 24; + timestamp_bit_size += 1; /* the register
> > value was bit_size - 1 */
>
> Please move the +1 into the else of the following if() which results in:
> | if (timestamp_bit_size)
>
> which is IMHO easier to read.
OK
> > +
> > + /* For 2.x versions of the IP core, we will assume 64-bit counter
> > + * if there was a 0 in the register.
> > + */
> > + if (timestamp_bit_size == 1) {
> > + u32 version_reg = ctucan_read32(priv, CTUCANFD_DEVICE_ID);
> > + u32 major = (version_reg & REG_DEVICE_ID_VER_MAJOR) >> 24;
> > +
> > + if (major == 2)
> > + timestamp_bit_size = 64;
> > + else
> > + priv->timestamp_possible = false;
> > + }
> > +
> > + /* Setup conversion constants and work delay */
> > + priv->cc.read = ctucan_read_timestamp_cc_wrapper;
> > + priv->cc.mask = CYCLECOUNTER_MASK(timestamp_bit_size);
>
> Does the driver use these 2 if timestamping is not possible?
We have timestamping included in all previous and actual FPGA
designs so we can assume it unconditional for version 2.
It is missing in QEMU emulation for now but result is that registers
are read as zero. So you get incorrect timestamps but no fatal
error occurs. I plan to update QEMU to provide at least some
timestamp approximation values but that has low priority for now.
> > + if (priv->timestamp_possible) {
> > + clocks_calc_mult_shift(&priv->cc.mult, &priv->cc.shift,
> > timestamp_freq, + NSEC_PER_SEC, CTUCANFD_MAX_WORK_DELAY_SEC);
> > + priv->work_delay_jiffies =
> > + ctucan_calculate_work_delay(timestamp_bit_size, timestamp_freq);
> > + if (priv->work_delay_jiffies == 0)
> > + priv->timestamp_possible = false;
>
> You'll get a higher precision if you take the mask into account, at
> least if the counter overflows before CTUCANFD_MAX_WORK_DELAY_SEC:
>
> maxsec = min(CTUCANFD_MAX_WORK_DELAY_SEC, priv->cc.mask /
> timestamp_freq);
>
> clocks_calc_mult_shift(&priv->cc.mult, &priv->cc.shift,
> timestamp_freq, NSEC_PER_SEC, maxsec); work_delay_in_ns =
> clocks_calc_max_nsecs(&priv->cc.mult, &priv->cc.shift, 0, &priv->cc.mask,
> NULL);
>
> You can use clocks_calc_max_nsecs() to calculate the work delay.
>
> regards,
> Marc
Thanks the review and support,
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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v2 1/3] can: ctucanfd: add HW timestamps to RX and error CAN frames
2022-08-02 16:20 ` Pavel Pisa
@ 2022-08-03 8:37 ` Marc Kleine-Budde
2022-08-04 8:08 ` Pavel Pisa
0 siblings, 1 reply; 30+ messages in thread
From: Marc Kleine-Budde @ 2022-08-03 8:37 UTC (permalink / raw)
To: Pavel Pisa
Cc: Matej Vasilevski, Ondrej Ille, Wolfgang Grandegger,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, linux-can, netdev, devicetree
[-- Attachment #1: Type: text/plain, Size: 6013 bytes --]
On 02.08.2022 18:20:17, Pavel Pisa wrote:
> Hello Marc,
>
> thanks for feedback.
>
> On Tuesday 02 of August 2022 11:29:07 Marc Kleine-Budde wrote:
> > On 01.08.2022 20:46:54, 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_clk" 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>
> > > ---
> > > drivers/net/can/ctucanfd/Makefile | 2 +-
> > > drivers/net/can/ctucanfd/ctucanfd.h | 20 ++
> > > drivers/net/can/ctucanfd/ctucanfd_base.c | 214 +++++++++++++++++-
> > > drivers/net/can/ctucanfd/ctucanfd_timestamp.c | 87 +++++++
> > > 4 files changed, 315 insertions(+), 8 deletions(-)
> > > create mode 100644 drivers/net/can/ctucanfd/ctucanfd_timestamp.c
> ...
> > > + if (ts_high2 != ts_high)
> > > + ts_low = priv->read_reg(priv, CTUCANFD_TIMESTAMP_LOW);
> > > +
> > > + return concatenate_two_u32(ts_high2, ts_low) & priv->cc.mask;
> > > +}
> > > +
> > > #define CTU_CAN_FD_TXTNF(priv) (!!FIELD_GET(REG_STATUS_TXNF,
> > > ctucan_read32(priv, CTUCANFD_STATUS))) #define CTU_CAN_FD_ENABLED(priv)
> > > (!!FIELD_GET(REG_MODE_ENA, ctucan_read32(priv, CTUCANFD_MODE)))
> >
> > please make these static inline bool functions.
>
> We put that to TODO list. But I prefer to prepare separate followup
> patch later.
ACK. I noticed later that these were not modified by this patch. Sorry
for the noise
>
> > > @@ -736,7 +764,9 @@ static int ctucan_rx(struct net_device *ndev)
> > > return 0;
> > > }
> > >
> > > - ctucan_read_rx_frame(priv, cf, ffw);
> > > + ctucan_read_rx_frame(priv, cf, ffw, ×tamp);
> > > + if (priv->timestamp_enabled)
> > > + ctucan_skb_set_timestamp(priv, skb, timestamp);
> >
> > Can the ctucan_skb_set_timestamp() and ctucan_read_timestamp_counter()
> > happen concurrently? AFAICS they are all called from ctucan_rx_poll(),
> > right?
>
> I am not sure about which possible problem do you think.
> But ctucan_read_timestamp_counter() is fully reentrant
> and has no side effect on the core. So there is no
> problem.
ctucan_read_timestamp_counter() is reentrant, but on 32 bit systems the
update of tc->cycle_last isn't.
[...]
> > > +
> > > + /* Obtain timestamping frequency */
> > > + if (pm_enable_call) {
> > > + /* Plaftorm device: get tstamp clock from device tree */
> > > + priv->timestamp_clk = devm_clk_get(dev, "ts-clk");
> > > + if (IS_ERR(priv->timestamp_clk)) {
> > > + /* Take the core clock frequency instead */
> > > + timestamp_freq = can_clk_rate;
> > > + } else {
> > > + timestamp_freq = clk_get_rate(priv->timestamp_clk);
> > > + }
> >
> > Who prepares/enabled the timestamp clock? clk_get_rate() is only valid if
> > the clock is enabled. I know, we violate this for the CAN clock. :/
>
> Yes, I have noticed that we miss clk_prepare_enable() in the
> ctucan_probe_common() and clk_disable_unprepare() in ctucan_platform_remove().
Oh, I missed the fact that the CAN clock is not enabled at all. That
should be fixed, too, in a separate patch.
So let's focus on the ts_clk here. On DT systems if there's no ts-clk,
you can assign the normal clk pointer to the priv->timestamp_clk, too.
Move the calculation of mult, shift and the delays into
ctucan_timestamp_init(). If ctucan_timestamp_init is not NULL, add a
clk_prepare_enable() and clk_get_rate(), otherwise use the can_clk_rate.
Add the corresponding clk_disable_unprepare() to ctucan_timestamp_stop().
> The need for clock running should be released in ctucan_suspend()
> and regained in ctucan_resume(). I see that the most CAN drivers
> use there clk_disable_unprepare/clk_prepare_enable but I am not
> sure, if this is right. Ma be plain clk_disable/clk_enable should
> be used for suspend and resume because as I understand, the clock
> frequency can be recomputed and reset during clk_prepare which
> would require to recompute bitrate. Do you have some advice
> what is a right option there?
For the CAN clock, add a prepare_enable to ndo_open, corresponding
function to ndo_stop. Or better, add these time runtime_pm.
Has system suspend/resume been tested? I think the IP core might be
powered off during system suspend, so the driver has to restore the
state of the chip. The easiest would be to run through
chip_start()/chip_stop().
For the possible change of clock rate between probe and ifup, we should
add a CAN driver framework wide function to re-calculate the bitrates
with the current clock rate after the prepare_enable.
BTW: In an early version of the stm32mp1 device tree some graphics clock
and the CAN clock shared the same parent clock. The configuration of the
display (which happened after the probe of the CAN driver ) caused a
different rate in the CAN clock, resulting in broken bit timings.
> Actual omission is no problem on our systems, be the clock are used
> in whole FPGA system with AXI connection and has to running already
> and we use same for timestamping.
>
> I would prefer to allow timestamping patch as it is without clock enable
> and then correct clock enable, disable by another patch for both ts and core
> clocks.
NACK - if the time stamping clock is added, please with proper handling.
The core clock can be fixed in a later patch.
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 --]
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v2 1/3] can: ctucanfd: add HW timestamps to RX and error CAN frames
2022-08-03 8:37 ` Marc Kleine-Budde
@ 2022-08-04 8:08 ` Pavel Pisa
2022-08-04 9:11 ` Marc Kleine-Budde
0 siblings, 1 reply; 30+ messages in thread
From: Pavel Pisa @ 2022-08-04 8:08 UTC (permalink / raw)
To: Marc Kleine-Budde, Matej Vasilevski
Cc: Ondrej Ille, Wolfgang Grandegger, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski,
linux-can, netdev, devicetree
Hello Marc,
On Wednesday 03 of August 2022 10:37:18 Marc Kleine-Budde wrote:
> On 02.08.2022 18:20:17, Pavel Pisa wrote:
> > Hello Marc,
> >
> > thanks for feedback.
> >
> > On Tuesday 02 of August 2022 11:29:07 Marc Kleine-Budde wrote:
> > > On 01.08.2022 20:46:54, 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_clk" 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>
> > > > ---
> > > > drivers/net/can/ctucanfd/Makefile | 2 +-
> > > > drivers/net/can/ctucanfd/ctucanfd.h | 20 ++
> > > > drivers/net/can/ctucanfd/ctucanfd_base.c | 214
> > > > +++++++++++++++++- drivers/net/can/ctucanfd/ctucanfd_timestamp.c |
> > > > 87 +++++++ 4 files changed, 315 insertions(+), 8 deletions(-)
> > > > create mode 100644 drivers/net/can/ctucanfd/ctucanfd_timestamp.c
> >
> > ...
> >
> > > > + if (ts_high2 != ts_high)
> > > > + ts_low = priv->read_reg(priv, CTUCANFD_TIMESTAMP_LOW);
> > > > +
> > > > + return concatenate_two_u32(ts_high2, ts_low) & priv->cc.mask;
> > > > +}
> > > > +
> > > > #define CTU_CAN_FD_TXTNF(priv) (!!FIELD_GET(REG_STATUS_TXNF,
> > > > ctucan_read32(priv, CTUCANFD_STATUS))) #define
> > > > CTU_CAN_FD_ENABLED(priv) (!!FIELD_GET(REG_MODE_ENA,
> > > > ctucan_read32(priv, CTUCANFD_MODE)))
> > >
> > > please make these static inline bool functions.
> >
> > We put that to TODO list. But I prefer to prepare separate followup
> > patch later.
>
> ACK. I noticed later that these were not modified by this patch. Sorry
> for the noise
OK
> > > > @@ -736,7 +764,9 @@ static int ctucan_rx(struct net_device *ndev)
> > > > return 0;
> > > > }
> > > >
> > > > - ctucan_read_rx_frame(priv, cf, ffw);
> > > > + ctucan_read_rx_frame(priv, cf, ffw, ×tamp);
> > > > + if (priv->timestamp_enabled)
> > > > + ctucan_skb_set_timestamp(priv, skb, timestamp);
> > >
> > > Can the ctucan_skb_set_timestamp() and ctucan_read_timestamp_counter()
> > > happen concurrently? AFAICS they are all called from ctucan_rx_poll(),
> > > right?
> >
> > I am not sure about which possible problem do you think.
> > But ctucan_read_timestamp_counter() is fully reentrant
> > and has no side effect on the core. So there is no
> > problem.
>
> ctucan_read_timestamp_counter() is reentrant, but on 32 bit systems the
> update of tc->cycle_last isn't.
>
> [...]
Good catch, so we probably should use atomic there or we need to add
spinlock, but I think that atomic is optimal solution there.
> > > > +
> > > > + /* Obtain timestamping frequency */
> > > > + if (pm_enable_call) {
> > > > + /* Plaftorm device: get tstamp clock from device tree */
> > > > + priv->timestamp_clk = devm_clk_get(dev, "ts-clk");
> > > > + if (IS_ERR(priv->timestamp_clk)) {
> > > > + /* Take the core clock frequency instead */
> > > > + timestamp_freq = can_clk_rate;
> > > > + } else {
> > > > + timestamp_freq = clk_get_rate(priv->timestamp_clk);
> > > > + }
> > >
> > > Who prepares/enabled the timestamp clock? clk_get_rate() is only valid
> > > if the clock is enabled. I know, we violate this for the CAN clock. :/
> >
> > Yes, I have noticed that we miss clk_prepare_enable() in the
> > ctucan_probe_common() and clk_disable_unprepare() in
> > ctucan_platform_remove().
>
> Oh, I missed the fact that the CAN clock is not enabled at all. That
> should be fixed, too, in a separate patch.
>
> So let's focus on the ts_clk here. On DT systems if there's no ts-clk,
> you can assign the normal clk pointer to the priv->timestamp_clk, too.
> Move the calculation of mult, shift and the delays into
> ctucan_timestamp_init(). If ctucan_timestamp_init is not NULL, add a
> clk_prepare_enable() and clk_get_rate(), otherwise use the can_clk_rate.
> Add the corresponding clk_disable_unprepare() to ctucan_timestamp_stop().
OK
> > The need for clock running should be released in ctucan_suspend()
> > and regained in ctucan_resume(). I see that the most CAN drivers
> > use there clk_disable_unprepare/clk_prepare_enable but I am not
> > sure, if this is right. Ma be plain clk_disable/clk_enable should
> > be used for suspend and resume because as I understand, the clock
> > frequency can be recomputed and reset during clk_prepare which
> > would require to recompute bitrate. Do you have some advice
> > what is a right option there?
>
> For the CAN clock, add a prepare_enable to ndo_open, corresponding
> function to ndo_stop. Or better, add these time runtime_pm.
Hmm, there is problem that we have single clock for whole design,
so if we try to touch AXI/APB slave registers without clock setup
then system blocks. So I think that we need to prepare and enable
clocks in probe to allow registers access later. We need it at least
for core bus endian probe and version validation/quirks. May it be
we can disable clocks and reenable them in open.... But it is
a little risky play and needs to ensure that no other path
in the closed driver can attempt to access registers.
Due to use of AXI bridges and other peripherals in Zynq Programmable
Logic (FPGA) we keep forcibly clock enabled. In the fact, this
should be solved some way on level of APB (now renamed in Zynq DST
to AXI) bus mapping.
> Has system suspend/resume been tested? I think the IP core might be
> powered off during system suspend, so the driver has to restore the
> state of the chip. The easiest would be to run through
> chip_start()/chip_stop().
Hmm, if we do not reconfigure FPGA then it keeps state and if we
lose configuration then whole cycle with DTS overlay is required.
So basically for runtime power down wee need to unload overlay
which removes driver instances and then reload overlay and instances
again... If you speak about suspend to disk, then I do not expect
to run this way on any platform bus based system in near future.
With PCIe card on PC it is possible... So it is other area to invest
work in future...
> For the possible change of clock rate between probe and ifup, we should
> add a CAN driver framework wide function to re-calculate the bitrates
> with the current clock rate after the prepare_enable.
ACK
> BTW: In an early version of the stm32mp1 device tree some graphics clock
> and the CAN clock shared the same parent clock. The configuration of the
> display (which happened after the probe of the CAN driver ) caused a
> different rate in the CAN clock, resulting in broken bit timings.
So in the fact each CAN device should listen to the clock rate
change notifier...
> > Actual omission is no problem on our systems, be the clock are used
> > in whole FPGA system with AXI connection and has to running already
> > and we use same for timestamping.
> >
> > I would prefer to allow timestamping patch as it is without clock enable
> > and then correct clock enable, disable by another patch for both ts and
> > core clocks.
>
> NACK - if the time stamping clock is added, please with proper handling.
> The core clock can be fixed in a later patch.
OK, how is it with your timing plans. I have already stated to Matej
Vasilevski that slip of the patch sending after 5.19 release means
that we would not fit in 5.20 probably. If it is so and you, then I
expect that postpone of discussions, replies and thoughts about our
work after 5.20 preparation calms down is preferred on your side.
We focus on preparation of proper series for early 5.21/6.0 cycle.
Thanks for your time
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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v2 1/3] can: ctucanfd: add HW timestamps to RX and error CAN frames
2022-08-04 8:08 ` Pavel Pisa
@ 2022-08-04 9:11 ` Marc Kleine-Budde
0 siblings, 0 replies; 30+ messages in thread
From: Marc Kleine-Budde @ 2022-08-04 9:11 UTC (permalink / raw)
To: Pavel Pisa
Cc: Matej Vasilevski, Ondrej Ille, Wolfgang Grandegger,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, linux-can, netdev, devicetree
[-- Attachment #1: Type: text/plain, Size: 6447 bytes --]
On 04.08.2022 10:08:15, Pavel Pisa wrote:
> > > > Can the ctucan_skb_set_timestamp() and ctucan_read_timestamp_counter()
> > > > happen concurrently? AFAICS they are all called from ctucan_rx_poll(),
> > > > right?
> > >
> > > I am not sure about which possible problem do you think.
> > > But ctucan_read_timestamp_counter() is fully reentrant
> > > and has no side effect on the core. So there is no
> > > problem.
> >
> > ctucan_read_timestamp_counter() is reentrant, but on 32 bit systems the
> > update of tc->cycle_last isn't.
> >
> > [...]
>
> Good catch, so we probably should use atomic there or we need to add
> spinlock, but I think that atomic is optimal solution there.
Atomic look like a good solution, but not scope of this patch, as the
timercounter needs to be modified. So use spinlocks for now.
> > > The need for clock running should be released in ctucan_suspend()
> > > and regained in ctucan_resume(). I see that the most CAN drivers
> > > use there clk_disable_unprepare/clk_prepare_enable but I am not
> > > sure, if this is right. Ma be plain clk_disable/clk_enable should
> > > be used for suspend and resume because as I understand, the clock
> > > frequency can be recomputed and reset during clk_prepare which
> > > would require to recompute bitrate. Do you have some advice
> > > what is a right option there?
> >
> > For the CAN clock, add a prepare_enable to ndo_open, corresponding
> > function to ndo_stop. Or better, add these time runtime_pm.
>
> Hmm, there is problem that we have single clock for whole design,
> so if we try to touch AXI/APB slave registers without clock setup
> then system blocks. So I think that we need to prepare and enable
> clocks in probe to allow registers access later.
ACK, enable clocks during probe, too. There are already pm_runtime calls
in the driver, but no runtime pm callback that handle the clocks.
BTW: that pm_enable_call argument to ctucan_probe_common() looks a bit
strange.
| int ctucan_probe_common(struct device *dev, void __iomem *addr, int irq, unsigned int ntxbufs,
| unsigned long can_clk_rate, int pm_enable_call,
| void (*set_drvdata_fnc)(struct device *dev, struct net_device *ndev))
I think the caller should setup the pm stuff so that the
ctucan_probe_common() can do a pm_runtime_get_sync(dev) call.
> We need it at least
> for core bus endian probe and version validation/quirks. May it be
> we can disable clocks and reenable them in open.... But it is
> a little risky play and needs to ensure that no other path
> in the closed driver can attempt to access registers.
Sure. There are basically 3 parts in the base driver to consider:
1) ctucan_probe_common()
2) ndo_open()...ndo_stop()
3) do_get_berr_counter()
> Due to use of AXI bridges and other peripherals in Zynq Programmable
> Logic (FPGA) we keep forcibly clock enabled. In the fact, this
> should be solved some way on level of APB (now renamed in Zynq DST
> to AXI) bus mapping.
>
> > Has system suspend/resume been tested? I think the IP core might be
> > powered off during system suspend, so the driver has to restore the
> > state of the chip. The easiest would be to run through
> > chip_start()/chip_stop().
>
> Hmm, if we do not reconfigure FPGA then it keeps state and if we
> lose configuration then whole cycle with DTS overlay is required.
Hardware IP cores might be powered down during system suspend and lose
their configuration. So you have to configure the CAN IP core again,
i.e. setup priorities, queues, bit timing, etc... Things that the
ctucan_chip_start() does. So I think ctucan_resume() must call to
ctucan_chip_start() if the interface was up while the system was
suspended.
> So basically for runtime power down wee need to unload overlay
> which removes driver instances and then reload overlay and instances
> again...
I've never played with softcores before, but from the theory I don't
think you have to unload overlays etc...every driver needs to implement
proper suspend/resume functions.
> If you speak about suspend to disk, then I do not expect
> to run this way on any platform bus based system in near future.
> With PCIe card on PC it is possible... So it is other area to invest
> work in future...
:)
> > For the possible change of clock rate between probe and ifup, we should
> > add a CAN driver framework wide function to re-calculate the bitrates
> > with the current clock rate after the prepare_enable.
>
> ACK
>
> > BTW: In an early version of the stm32mp1 device tree some graphics clock
> > and the CAN clock shared the same parent clock. The configuration of the
> > display (which happened after the probe of the CAN driver ) caused a
> > different rate in the CAN clock, resulting in broken bit timings.
>
> So in the fact each CAN device should listen to the clock rate
> change notifier...
ACK - and strictly speaking clk_get_rate() of a clock that's not enabled
is not valid. But that's not enforced by the common clock framework.
> > > Actual omission is no problem on our systems, be the clock are used
> > > in whole FPGA system with AXI connection and has to running already
> > > and we use same for timestamping.
> > >
> > > I would prefer to allow timestamping patch as it is without clock enable
> > > and then correct clock enable, disable by another patch for both ts and
> > > core clocks.
> >
> > NACK - if the time stamping clock is added, please with proper handling.
> > The core clock can be fixed in a later patch.
>
> OK, how is it with your timing plans. I have already stated to Matej
> Vasilevski that slip of the patch sending after 5.19 release means
> that we would not fit in 5.20 probably.
net-next for 5.20 is closed.
> If it is so and you, then I
> expect that postpone of discussions, replies and thoughts about our
> work after 5.20 preparation calms down is preferred on your side.
> We focus on preparation of proper series for early 5.21/6.0 cycle.
I don't mind discussing things for 5.21 now. Everything for 5.20 is now
bug fixes only.
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 --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v2 1/3] can: ctucanfd: add HW timestamps to RX and error CAN frames
2022-08-02 9:29 ` Marc Kleine-Budde
2022-08-02 10:26 ` Marc Kleine-Budde
2022-08-02 16:20 ` Pavel Pisa
@ 2022-08-03 0:09 ` Matej Vasilevski
2022-08-03 6:11 ` Pavel Pisa
2022-08-03 8:53 ` Marc Kleine-Budde
2 siblings, 2 replies; 30+ messages in thread
From: Matej Vasilevski @ 2022-08-03 0:09 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: Pavel Pisa, Ondrej Ille, Wolfgang Grandegger, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
Krzysztof Kozlowski, linux-can, netdev, devicetree
Hello Marc,
thanks for your review. I see I forgot to put dt bindings as the first
commit, I've reordered it locally so I won't forget again.
On Tue, Aug 02, 2022 at 11:29:07AM +0200, Marc Kleine-Budde wrote:
> > diff --git a/drivers/net/can/ctucanfd/ctucanfd.h b/drivers/net/can/ctucanfd/ctucanfd.h
> > index 0e9904f6a05d..43d9c73ce244 100644
> > --- a/drivers/net/can/ctucanfd/ctucanfd.h
> > +++ b/drivers/net/can/ctucanfd/ctucanfd.h
> > @@ -23,6 +23,10 @@
> > #include <linux/netdevice.h>
> > #include <linux/can/dev.h>
> > #include <linux/list.h>
> > +#include <linux/timecounter.h>
> > +#include <linux/workqueue.h>
> > +
> > +#define CTUCANFD_MAX_WORK_DELAY_SEC 86400U /* one day == 24 * 3600 seconds */
>
> For higher precision we can limit this to 3600s, as the sched_clock does
> it
> :
> | https://elixir.bootlin.com/linux/v5.19/source/kernel/time/sched_clock.c#L169
Sure, the one day limit was rather arbitrary, I wanted to keep the load
on the system minimal.
> > enum ctu_can_fd_can_registers;
> >
> > @@ -51,6 +55,15 @@ struct ctucan_priv {
> > u32 rxfrm_first_word;
> >
> > struct list_head peers_on_pdev;
> > +
> > + struct cyclecounter cc;
> > + struct timecounter tc;
> > + struct delayed_work timestamp;
> > +
> > + struct clk *timestamp_clk;
> > + u32 work_delay_jiffies;
>
> schedule_delayed_work() takes an "unsigned long" not a u32.
>
Ok I'll cast the u32 to unsigned long, as 'long' is guaranteed to be at
least 32 bits.
> > @@ -148,6 +149,27 @@ static void ctucan_write_txt_buf(struct ctucan_priv *priv, enum ctu_can_fd_can_r
> > priv->write_reg(priv, buf_base + offset, val);
> > }
> >
> > +static u64 concatenate_two_u32(u32 high, u32 low)
>
> static inline?
>
Sure, inline seems reasonable here.
> > +{
> > + return ((u64)high << 32) | ((u64)low);
> > +}
> > +
> > @@ -682,9 +708,10 @@ static void ctucan_read_rx_frame(struct ctucan_priv *priv, struct canfd_frame *c
> > if (unlikely(len > wc * 4))
> > len = wc * 4;
> >
> > - /* Timestamp - Read and throw away */
> > - ctucan_read32(priv, CTUCANFD_RX_DATA);
> > - ctucan_read32(priv, CTUCANFD_RX_DATA);
> > + /* Timestamp */
> > + tstamp_low = ctucan_read32(priv, CTUCANFD_RX_DATA);
> > + tstamp_high = ctucan_read32(priv, CTUCANFD_RX_DATA);
> > + *timestamp = concatenate_two_u32(tstamp_high, tstamp_low) & priv->cc.mask;
> >
> > /* Data */
> > for (i = 0; i < len; i += 4) {
> > @@ -713,6 +740,7 @@ static int ctucan_rx(struct net_device *ndev)
> > struct net_device_stats *stats = &ndev->stats;
> > struct canfd_frame *cf;
> > struct sk_buff *skb;
> > + u64 timestamp;
> > u32 ffw;
> >
> > if (test_bit(CTUCANFD_FLAG_RX_FFW_BUFFERED, &priv->drv_flags)) {
> > @@ -736,7 +764,9 @@ static int ctucan_rx(struct net_device *ndev)
> > return 0;
> > }
> >
> > - ctucan_read_rx_frame(priv, cf, ffw);
> > + ctucan_read_rx_frame(priv, cf, ffw, ×tamp);
> > + if (priv->timestamp_enabled)
> > + ctucan_skb_set_timestamp(priv, skb, timestamp);
>
> Can the ctucan_skb_set_timestamp() and ctucan_read_timestamp_counter()
> happen concurrently? AFAICS they are all called from ctucan_rx_poll(),
> right?
Yes, I see no problem when two ctucan_read_timestamp_counter run
concurrently, same goes for two ctucan_skb_set_timestamp and
ctucan_skb_set_timestamp concurrently with
ctucan_read_timestamp_counter.
The _counter() function only reads from the core's registers and returns
a new timestamp. The _set_timestamp() only writes to the skb, but the
skb will be allocated new in every _rx_poll() call.
The only concurrency issue I can remotely see is when the periodic worker
updates timecounter->cycle_last, right when the value is used in
timecounter_cyc2time (from _set_timestamp()). But I don't think this is
worth using some synchronization primitive.
>
> >
> > stats->rx_bytes += cf->len;
> > stats->rx_packets++;
> > @@ -1263,6 +1306,9 @@ static int ctucan_close(struct net_device *ndev)
> > ctucan_chip_stop(ndev);
> > free_irq(ndev->irq, ndev);
> > close_candev(ndev);
> > + if (priv->timestamp_possible)
> > + ctucan_timestamp_stop(priv);
> > +
>
> Nitpick: Don't add an extra newline here.
>
Ok, I've deleted the extra newline (one is still here below btw - seems
to me you're pointing out that the code isn't in one continuous block;
double newline wouldn't be a nitpick for me).
> >
> > pm_runtime_put(priv->dev);
> >
> > @@ -1345,6 +1493,8 @@ int ctucan_probe_common(struct device *dev, void __iomem *addr, int irq, unsigne
> > struct ctucan_priv *priv;
> > struct net_device *ndev;
> > int ret;
> > + u32 timestamp_freq = 0;
> > + u32 timestamp_bit_size = 0;
>
> Nitpick: please move the u32 between the struct and the int.
Ack.
>
> >
> > /* Create a CAN device instance */
> > ndev = alloc_candev(sizeof(struct ctucan_priv), ntxbufs);
> > @@ -1386,7 +1536,9 @@ int ctucan_probe_common(struct device *dev, void __iomem *addr, int irq, unsigne
> >
> > /* Getting the can_clk info */
> > if (!can_clk_rate) {
> > - priv->can_clk = devm_clk_get(dev, NULL);
> > + priv->can_clk = devm_clk_get_optional(dev, "core-clk");
> > + if (!priv->can_clk)
> > + priv->can_clk = devm_clk_get(dev, NULL);
>
> Please add a comment here, that the NULL clock is for compatibility with
> older DTs.
Even in this patch the clock-names isn't a required property in the DT.
But I can add a comment explaining the situation.
>
> > if (IS_ERR(priv->can_clk)) {
> > dev_err(dev, "Device clock not found.\n");
> > ret = PTR_ERR(priv->can_clk);
> > @@ -1425,6 +1577,54 @@ int ctucan_probe_common(struct device *dev, void __iomem *addr, int irq, unsigne
> >
> > priv->can.clock.freq = can_clk_rate;
> >
> > + priv->timestamp_enabled = false;
> > + priv->timestamp_possible = true;
> > + priv->timestamp_clk = NULL;
>
> priv is allocated and initialized with 0
Ok, false and NULL deleted.
>
> > +
> > + /* Obtain timestamping frequency */
> > + if (pm_enable_call) {
> > + /* Plaftorm device: get tstamp clock from device tree */
> > + priv->timestamp_clk = devm_clk_get(dev, "ts-clk");
> > + if (IS_ERR(priv->timestamp_clk)) {
> > + /* Take the core clock frequency instead */
> > + timestamp_freq = can_clk_rate;
> > + } else {
> > + timestamp_freq = clk_get_rate(priv->timestamp_clk);
> > + }
>
> Who prepares/enabled the timestamp clock? clk_get_rate() is only valid if
> the clock is enabled. I know, we violate this for the CAN clock. :/
> > + } else {
> > + /* PCI device: assume tstamp freq is equal to bus clk rate */
> > + timestamp_freq = can_clk_rate;
> > + }
> > +
> > + /* Obtain timestamping counter bit size */
> > + timestamp_bit_size = (ctucan_read32(priv, CTUCANFD_ERR_CAPT) & REG_ERR_CAPT_TS_BITS) >> 24;
> > + timestamp_bit_size += 1; /* the register value was bit_size - 1 */
>
> Please move the +1 into the else of the following if() which results in:
>
> | if (timestamp_bit_size)
>
> which is IMHO easier to read.
Sure I'll move it (into the 'if' branch).
> > +
> > + /* For 2.x versions of the IP core, we will assume 64-bit counter
> > + * if there was a 0 in the register.
> > + */
> > + if (timestamp_bit_size == 1) {
> > + u32 version_reg = ctucan_read32(priv, CTUCANFD_DEVICE_ID);
> > + u32 major = (version_reg & REG_DEVICE_ID_VER_MAJOR) >> 24;
> > +
> > + if (major == 2)
> > + timestamp_bit_size = 64;
> > + else
> > + priv->timestamp_possible = false;
> > + }
> > +
> > + /* Setup conversion constants and work delay */
> > + priv->cc.read = ctucan_read_timestamp_cc_wrapper;
> > + priv->cc.mask = CYCLECOUNTER_MASK(timestamp_bit_size);
>
> Does the driver use these 2 if timestamping is not possible?
Cc.mask is always used in ctucan_read_rx_frame(), cc.read isn't used
when timestamps aren't possible. I can move cc.read inside the 'if' for
maximal efficiency.
>
> > + if (priv->timestamp_possible) {
> > + clocks_calc_mult_shift(&priv->cc.mult, &priv->cc.shift, timestamp_freq,
> > + NSEC_PER_SEC, CTUCANFD_MAX_WORK_DELAY_SEC);
> > + priv->work_delay_jiffies =
> > + ctucan_calculate_work_delay(timestamp_bit_size, timestamp_freq);
> > + if (priv->work_delay_jiffies == 0)
> > + priv->timestamp_possible = false;
>
> You'll get a higher precision if you take the mask into account, at
> least if the counter overflows before CTUCANFD_MAX_WORK_DELAY_SEC:
>
> maxsec = min(CTUCANFD_MAX_WORK_DELAY_SEC, priv->cc.mask / timestamp_freq);
>
> clocks_calc_mult_shift(&priv->cc.mult, &priv->cc.shift, timestamp_freq, NSEC_PER_SEC, maxsec);
> work_delay_in_ns = clocks_calc_max_nsecs(&priv->cc.mult, &priv->cc.shift, 0, &priv->cc.mask, NULL);
>
> You can use clocks_calc_max_nsecs() to calculate the work delay.
This is a good point, thanks. I'll incorporate it into the patch.
Best regards,
Matej
> 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 |
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v2 1/3] can: ctucanfd: add HW timestamps to RX and error CAN frames
2022-08-03 0:09 ` Matej Vasilevski
@ 2022-08-03 6:11 ` Pavel Pisa
2022-08-03 8:53 ` Marc Kleine-Budde
1 sibling, 0 replies; 30+ messages in thread
From: Pavel Pisa @ 2022-08-03 6:11 UTC (permalink / raw)
To: Matej Vasilevski, Marc Kleine-Budde
Cc: Ondrej Ille, Wolfgang Grandegger, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Rob Herring, Krzysztof Kozlowski,
linux-can, netdev, devicetree
Minor remark to clocks
On Wednesday 03 of August 2022 02:09:03 Matej Vasilevski wrote:
> On Tue, Aug 02, 2022 at 11:29:07AM +0200, Marc Kleine-Budde wrote:
> > > diff --git a/drivers/net/can/ctucanfd/ctucanfd.h
> > > b/drivers/net/can/ctucanfd/ctucanfd.h index 0e9904f6a05d..43d9c73ce244
....
> > > @@ -1386,7 +1536,9 @@ int ctucan_probe_common(struct device *dev, void
> > > __iomem *addr, int irq, unsigne
> > >
> > > /* Getting the can_clk info */
> > > if (!can_clk_rate) {
> > > - priv->can_clk = devm_clk_get(dev, NULL);
> > > + priv->can_clk = devm_clk_get_optional(dev, "core-clk");
> > > + if (!priv->can_clk)
> > > + priv->can_clk = devm_clk_get(dev, NULL);
> >
> > Please add a comment here, that the NULL clock is for compatibility with
> > older DTs.
>
> Even in this patch the clock-names isn't a required property in the DT.
> But I can add a comment explaining the situation.
In the fact, actual FPGA design is intended for single clock domain
and if timestamp counter is running from other non synchronized
clocks then cross domain synchronization is necessary
which in a 64-bit parallel case is relatively complex.
Sampling of some slower clocks signal on input to CTU CAN FD
core domain would be easier...
There can appear optimization for some constrained designs
in future where clock counter is narrowed to less bits
and clock is divided from main clocks by some ratio...
So option to use separate ts-clock is reserve for future.
I personally, do not even insist on including the second
clock to the driver now because all our actual and currently
planed designs are single clock domain. But may it be Ondrej
Ille can comment even further foresee...
Thanks for the work and review,
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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v2 1/3] can: ctucanfd: add HW timestamps to RX and error CAN frames
2022-08-03 0:09 ` Matej Vasilevski
2022-08-03 6:11 ` Pavel Pisa
@ 2022-08-03 8:53 ` Marc Kleine-Budde
2022-08-17 23:14 ` Matej Vasilevski
1 sibling, 1 reply; 30+ messages in thread
From: Marc Kleine-Budde @ 2022-08-03 8:53 UTC (permalink / raw)
To: Matej Vasilevski
Cc: Pavel Pisa, Ondrej Ille, Wolfgang Grandegger, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
Krzysztof Kozlowski, linux-can, netdev, devicetree
[-- Attachment #1: Type: text/plain, Size: 4095 bytes --]
On 03.08.2022 02:09:03, Matej Vasilevski wrote:
[...]
> > > @@ -682,9 +708,10 @@ static void ctucan_read_rx_frame(struct ctucan_priv *priv, struct canfd_frame *c
> > > if (unlikely(len > wc * 4))
> > > len = wc * 4;
> > >
> > > - /* Timestamp - Read and throw away */
> > > - ctucan_read32(priv, CTUCANFD_RX_DATA);
> > > - ctucan_read32(priv, CTUCANFD_RX_DATA);
> > > + /* Timestamp */
> > > + tstamp_low = ctucan_read32(priv, CTUCANFD_RX_DATA);
> > > + tstamp_high = ctucan_read32(priv, CTUCANFD_RX_DATA);
> > > + *timestamp = concatenate_two_u32(tstamp_high, tstamp_low) & priv->cc.mask;
> > >
> > > /* Data */
> > > for (i = 0; i < len; i += 4) {
> > > @@ -713,6 +740,7 @@ static int ctucan_rx(struct net_device *ndev)
> > > struct net_device_stats *stats = &ndev->stats;
> > > struct canfd_frame *cf;
> > > struct sk_buff *skb;
> > > + u64 timestamp;
> > > u32 ffw;
> > >
> > > if (test_bit(CTUCANFD_FLAG_RX_FFW_BUFFERED, &priv->drv_flags)) {
> > > @@ -736,7 +764,9 @@ static int ctucan_rx(struct net_device *ndev)
> > > return 0;
> > > }
> > >
> > > - ctucan_read_rx_frame(priv, cf, ffw);
> > > + ctucan_read_rx_frame(priv, cf, ffw, ×tamp);
> > > + if (priv->timestamp_enabled)
> > > + ctucan_skb_set_timestamp(priv, skb, timestamp);
> >
> > Can the ctucan_skb_set_timestamp() and ctucan_read_timestamp_counter()
> > happen concurrently? AFAICS they are all called from ctucan_rx_poll(),
> > right?
>
> Yes, I see no problem when two ctucan_read_timestamp_counter run
> concurrently, same goes for two ctucan_skb_set_timestamp and
> ctucan_skb_set_timestamp concurrently with
> ctucan_read_timestamp_counter.
Right!
> The _counter() function only reads from the core's registers and returns
> a new timestamp. The _set_timestamp() only writes to the skb, but the
> skb will be allocated new in every _rx_poll() call.
>
> The only concurrency issue I can remotely see is when the periodic worker
> updates timecounter->cycle_last, right when the value is used in
> timecounter_cyc2time (from _set_timestamp()). But I don't think this is
> worth using some synchronization primitive.
Yes, I'm worried about the cycle_last on 32 bit systems.
[...]
> > > + priv->cc.read = ctucan_read_timestamp_cc_wrapper;
> > > + priv->cc.mask = CYCLECOUNTER_MASK(timestamp_bit_size);
> >
> > Does the driver use these 2 if timestamping is not possible?
>
> Cc.mask is always used in ctucan_read_rx_frame(), cc.read isn't used
> when timestamps aren't possible. I can move cc.read inside the 'if' for
> maximal efficiency.
Ok
> > > + if (priv->timestamp_possible) {
> > > + clocks_calc_mult_shift(&priv->cc.mult, &priv->cc.shift, timestamp_freq,
> > > + NSEC_PER_SEC, CTUCANFD_MAX_WORK_DELAY_SEC);
> > > + priv->work_delay_jiffies =
> > > + ctucan_calculate_work_delay(timestamp_bit_size, timestamp_freq);
> > > + if (priv->work_delay_jiffies == 0)
> > > + priv->timestamp_possible = false;
> >
> > You'll get a higher precision if you take the mask into account, at
> > least if the counter overflows before CTUCANFD_MAX_WORK_DELAY_SEC:
> >
> > maxsec = min(CTUCANFD_MAX_WORK_DELAY_SEC, priv->cc.mask / timestamp_freq);
> >
> > clocks_calc_mult_shift(&priv->cc.mult, &priv->cc.shift, timestamp_freq, NSEC_PER_SEC, maxsec);
> > work_delay_in_ns = clocks_calc_max_nsecs(&priv->cc.mult, &priv->cc.shift, 0, &priv->cc.mask, NULL);
> >
> > You can use clocks_calc_max_nsecs() to calculate the work delay.
>
> This is a good point, thanks. I'll incorporate it into the patch.
And do this calculation after a clk_prepare_enable(), see other mail to
Pavel
| https://lore.kernel.org/all/20220803083718.7bh2edmsorwuv4vu@pengutronix.de/
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 --]
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v2 1/3] can: ctucanfd: add HW timestamps to RX and error CAN frames
2022-08-03 8:53 ` Marc Kleine-Budde
@ 2022-08-17 23:14 ` Matej Vasilevski
2022-08-18 9:24 ` Marc Kleine-Budde
0 siblings, 1 reply; 30+ messages in thread
From: Matej Vasilevski @ 2022-08-17 23:14 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: Pavel Pisa, Ondrej Ille, Wolfgang Grandegger, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
Krzysztof Kozlowski, linux-can, netdev, devicetree
Hello Marc,
I have two questions before I send the next patch version, please
bear with me.
On Wed, Aug 03, 2022 at 10:53:03AM +0200, Marc Kleine-Budde wrote:
[...]
> > > > + if (priv->timestamp_possible) {
> > > > + clocks_calc_mult_shift(&priv->cc.mult, &priv->cc.shift, timestamp_freq,
> > > > + NSEC_PER_SEC, CTUCANFD_MAX_WORK_DELAY_SEC);
> > > > + priv->work_delay_jiffies =
> > > > + ctucan_calculate_work_delay(timestamp_bit_size, timestamp_freq);
> > > > + if (priv->work_delay_jiffies == 0)
> > > > + priv->timestamp_possible = false;
> > >
> > > You'll get a higher precision if you take the mask into account, at
> > > least if the counter overflows before CTUCANFD_MAX_WORK_DELAY_SEC:
> > >
> > > maxsec = min(CTUCANFD_MAX_WORK_DELAY_SEC, priv->cc.mask / timestamp_freq);
> > >
> > > clocks_calc_mult_shift(&priv->cc.mult, &priv->cc.shift, timestamp_freq, NSEC_PER_SEC, maxsec);
> > > work_delay_in_ns = clocks_calc_max_nsecs(&priv->cc.mult, &priv->cc.shift, 0, &priv->cc.mask, NULL);
> > >
> > > You can use clocks_calc_max_nsecs() to calculate the work delay.
> >
> > This is a good point, thanks. I'll incorporate it into the patch.
>
> And do this calculation after a clk_prepare_enable(), see other mail to
> Pavel
> | https://lore.kernel.org/all/20220803083718.7bh2edmsorwuv4vu@pengutronix.de/
1) I can't use clocks_calc_max_nsecs(), because it isn't exported
symbol (and I get modpost error during linking). Is that simply an
oversight on your end or I'm doing something incorrectly?
I've also listed all the exported symbols from /kernel/time, and nothing
really stood out to me as super useful for this patch. So I would
continue using ctucan_calculate_work_delay().
2) Instead of using clk_prepare_enable() manually in probe, I've added
the prepare_enable and disable_unprepare(ts_clk) calls into pm_runtime
suspend and resume callbacks. And I call clk_get_rate(ts_clk) only after
the pm_runtime_enable() and pm_runtime_get_sync() are called. This
seemed nicer to me, because the core clock prepare/unprepare will go
into the pm_runtime callbacks too.
Is that a correct approach, or should I really use the clk_prepare_enable()
and clk_disable_unprepare() "manually" in ctucan_common_probe()/ctucan_timestamp_stop()?
On my Zynq board I don't see the ctucan_resume() callback executed during probe
(after pm_runtime_enable() and pm_runtime_get_sync() are called in _probe()),
but in theory it seems like the correct approach. Xilinx_can driver does this too.
Other drivers (e.g. flexcan, mpc251xfd, rcar) call clk_get_rate() right after
devm_clk_get() in probe, but maybe the situation there is different, I don't
know too much about clocks and pm_runtime yet.
Thanks and best regards,
Matej
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v2 1/3] can: ctucanfd: add HW timestamps to RX and error CAN frames
2022-08-17 23:14 ` Matej Vasilevski
@ 2022-08-18 9:24 ` Marc Kleine-Budde
2022-08-18 16:03 ` Matej Vasilevski
0 siblings, 1 reply; 30+ messages in thread
From: Marc Kleine-Budde @ 2022-08-18 9:24 UTC (permalink / raw)
To: Matej Vasilevski
Cc: Pavel Pisa, Ondrej Ille, Wolfgang Grandegger, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
Krzysztof Kozlowski, linux-can, netdev, devicetree
[-- Attachment #1: Type: text/plain, Size: 4884 bytes --]
On 18.08.2022 01:14:34, Matej Vasilevski wrote:
> Hello Marc,
>
> I have two questions before I send the next patch version, please
> bear with me.
>
> On Wed, Aug 03, 2022 at 10:53:03AM +0200, Marc Kleine-Budde wrote:
>
> [...]
>
> > > > > + if (priv->timestamp_possible) {
> > > > > + clocks_calc_mult_shift(&priv->cc.mult, &priv->cc.shift, timestamp_freq,
> > > > > + NSEC_PER_SEC, CTUCANFD_MAX_WORK_DELAY_SEC);
> > > > > + priv->work_delay_jiffies =
> > > > > + ctucan_calculate_work_delay(timestamp_bit_size, timestamp_freq);
> > > > > + if (priv->work_delay_jiffies == 0)
> > > > > + priv->timestamp_possible = false;
> > > >
> > > > You'll get a higher precision if you take the mask into account, at
> > > > least if the counter overflows before CTUCANFD_MAX_WORK_DELAY_SEC:
> > > >
> > > > maxsec = min(CTUCANFD_MAX_WORK_DELAY_SEC, priv->cc.mask / timestamp_freq);
> > > >
> > > > clocks_calc_mult_shift(&priv->cc.mult, &priv->cc.shift, timestamp_freq, NSEC_PER_SEC, maxsec);
> > > > work_delay_in_ns = clocks_calc_max_nsecs(&priv->cc.mult, &priv->cc.shift, 0, &priv->cc.mask, NULL);
> > > >
> > > > You can use clocks_calc_max_nsecs() to calculate the work delay.
> > >
> > > This is a good point, thanks. I'll incorporate it into the patch.
> >
> > And do this calculation after a clk_prepare_enable(), see other mail to
> > Pavel
> > | https://lore.kernel.org/all/20220803083718.7bh2edmsorwuv4vu@pengutronix.de/
>
>
> 1) I can't use clocks_calc_max_nsecs(), because it isn't exported
> symbol (and I get modpost error during linking). Is that simply an
> oversight on your end or I'm doing something incorrectly?
Oh, I haven't checked if clocks_calc_max_nsecs() is exported. You can
either create a patch to export it, or "open code" its functionality. I
think this should be more or less equivalent:
| work_delay_in_ns = clocksource_cyc2ns(mask, mult, shift) >> 1;
> I've also listed all the exported symbols from /kernel/time, and nothing
> really stood out to me as super useful for this patch. So I would
> continue using ctucan_calculate_work_delay().
>
> 2) Instead of using clk_prepare_enable() manually in probe, I've added
> the prepare_enable and disable_unprepare(ts_clk) calls into pm_runtime
> suspend and resume callbacks. And I call clk_get_rate(ts_clk) only after
> the pm_runtime_enable() and pm_runtime_get_sync() are called.
Use pm_runtime_resume_and_get(), see:
| https://elixir.bootlin.com/linux/v5.19/source/include/linux/pm_runtime.h#L419
> This
> seemed nicer to me, because the core clock prepare/unprepare will go
> into the pm_runtime callbacks too.
Sound good. If you rely on the runtime PM, please add a "depends on PM"
to the Kconfig. If you want/need to support configurations without
runtime PM, you have to do some extra work:
| https://elixir.bootlin.com/linux/v5.19/source/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c#L1860
In the mcp251xfd driver without runtime PM I enable the clocks and VDD
during probe() and keep them running until remove(). The idea is:
1) call clock_prepare_enable() manually
2) call pm_runtime_get_noresume(), which equal to
pm_runtime_resume_and_get() but doesn't call the resume function
3) pm_runtime_enable()
4) pm_runtime_put()
will call suspend with runtime PM enabled,
will do nothing otherwise
Then use pm_runtime_resume_and_get() during open() and pm_runtime_put()
during stop(). Use both between accessing regs in do_get_berr_counter().
During remove it's a bit simpler:
| https://elixir.bootlin.com/linux/v5.19/source/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c#L1932
> Is that a correct approach, or should I really use the clk_prepare_enable()
> and clk_disable_unprepare() "manually" in ctucan_common_probe()/ctucan_timestamp_stop()?
>
> On my Zynq board I don't see the ctucan_resume() callback executed during probe
> (after pm_runtime_enable() and pm_runtime_get_sync() are called in _probe()),
Is this a kernel without CONFIG_PM?
> but in theory it seems like the correct approach. Xilinx_can driver does this too.
> Other drivers (e.g. flexcan, mpc251xfd, rcar) call clk_get_rate() right after
> devm_clk_get() in probe, but maybe the situation there is different, I don't
> know too much about clocks and pm_runtime yet.
The API says the clock must be enabled during clk_get_rate() (but that's
not enforced). And another problem is that the clock rate might change,
but let's ignore the clock rate change problem for now.
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 --]
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH v2 1/3] can: ctucanfd: add HW timestamps to RX and error CAN frames
2022-08-18 9:24 ` Marc Kleine-Budde
@ 2022-08-18 16:03 ` Matej Vasilevski
0 siblings, 0 replies; 30+ messages in thread
From: Matej Vasilevski @ 2022-08-18 16:03 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: Pavel Pisa, Ondrej Ille, Wolfgang Grandegger, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Rob Herring,
Krzysztof Kozlowski, linux-can, netdev, devicetree
On Thu, Aug 18, 2022 at 11:24:35AM +0200, Marc Kleine-Budde wrote:
> On 18.08.2022 01:14:34, Matej Vasilevski wrote:
> > Hello Marc,
> >
> > I have two questions before I send the next patch version, please
> > bear with me.
> >
> > On Wed, Aug 03, 2022 at 10:53:03AM +0200, Marc Kleine-Budde wrote:
> >
> > [...]
> >
> > > > > > + if (priv->timestamp_possible) {
> > > > > > + clocks_calc_mult_shift(&priv->cc.mult, &priv->cc.shift, timestamp_freq,
> > > > > > + NSEC_PER_SEC, CTUCANFD_MAX_WORK_DELAY_SEC);
> > > > > > + priv->work_delay_jiffies =
> > > > > > + ctucan_calculate_work_delay(timestamp_bit_size, timestamp_freq);
> > > > > > + if (priv->work_delay_jiffies == 0)
> > > > > > + priv->timestamp_possible = false;
> > > > >
> > > > > You'll get a higher precision if you take the mask into account, at
> > > > > least if the counter overflows before CTUCANFD_MAX_WORK_DELAY_SEC:
> > > > >
> > > > > maxsec = min(CTUCANFD_MAX_WORK_DELAY_SEC, priv->cc.mask / timestamp_freq);
> > > > >
> > > > > clocks_calc_mult_shift(&priv->cc.mult, &priv->cc.shift, timestamp_freq, NSEC_PER_SEC, maxsec);
> > > > > work_delay_in_ns = clocks_calc_max_nsecs(&priv->cc.mult, &priv->cc.shift, 0, &priv->cc.mask, NULL);
> > > > >
> > > > > You can use clocks_calc_max_nsecs() to calculate the work delay.
> > > >
> > > > This is a good point, thanks. I'll incorporate it into the patch.
> > >
> > > And do this calculation after a clk_prepare_enable(), see other mail to
> > > Pavel
> > > | https://lore.kernel.org/all/20220803083718.7bh2edmsorwuv4vu@pengutronix.de/
> >
> >
> > 1) I can't use clocks_calc_max_nsecs(), because it isn't exported
> > symbol (and I get modpost error during linking). Is that simply an
> > oversight on your end or I'm doing something incorrectly?
>
> Oh, I haven't checked if clocks_calc_max_nsecs() is exported. You can
> either create a patch to export it, or "open code" its functionality. I
> think this should be more or less equivalent:
>
> | work_delay_in_ns = clocksource_cyc2ns(mask, mult, shift) >> 1;
I'm afraid creating a patch for the export would open another can of worms. I'll
take a barebones version of the function: only the _cyc2ns(), and the max_cycles
computation to avoid overflows for 64-bit mask. It should fit in 3 rows of code.
> > I've also listed all the exported symbols from /kernel/time, and nothing
> > really stood out to me as super useful for this patch. So I would
> > continue using ctucan_calculate_work_delay().
> >
> > 2) Instead of using clk_prepare_enable() manually in probe, I've added
> > the prepare_enable and disable_unprepare(ts_clk) calls into pm_runtime
> > suspend and resume callbacks. And I call clk_get_rate(ts_clk) only after
> > the pm_runtime_enable() and pm_runtime_get_sync() are called.
>
> Use pm_runtime_resume_and_get(), see:
>
> | https://elixir.bootlin.com/linux/v5.19/source/include/linux/pm_runtime.h#L419
>
> > This
> > seemed nicer to me, because the core clock prepare/unprepare will go
> > into the pm_runtime callbacks too.
>
> Sound good. If you rely on the runtime PM, please add a "depends on PM"
> to the Kconfig. If you want/need to support configurations without
> runtime PM, you have to do some extra work:
Yes, I'll have to add PM to Kconfig. Currently the driver defines suspend
and resume sleep callbacks, but PM isn't in KConfig.
I would support only runtime PM, but Pavel Pisa knows more and might disagree.
In such case this write up will be very helpful, thank you.
> | https://elixir.bootlin.com/linux/v5.19/source/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c#L1860
>
> In the mcp251xfd driver without runtime PM I enable the clocks and VDD
> during probe() and keep them running until remove(). The idea is:
>
> 1) call clock_prepare_enable() manually
> 2) call pm_runtime_get_noresume(), which equal to
> pm_runtime_resume_and_get() but doesn't call the resume function
> 3) pm_runtime_enable()
> 4) pm_runtime_put()
> will call suspend with runtime PM enabled,
> will do nothing otherwise
>
> Then use pm_runtime_resume_and_get() during open() and pm_runtime_put()
> during stop(). Use both between accessing regs in do_get_berr_counter().
>
> During remove it's a bit simpler:
>
> | https://elixir.bootlin.com/linux/v5.19/source/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c#L1932
>
> > Is that a correct approach, or should I really use the clk_prepare_enable()
> > and clk_disable_unprepare() "manually" in ctucan_common_probe()/ctucan_timestamp_stop()?
> >
> > On my Zynq board I don't see the ctucan_resume() callback executed during probe
> > (after pm_runtime_enable() and pm_runtime_get_sync() are called in _probe()),
>
> Is this a kernel without CONFIG_PM?
Fortunately the kernel was configured with CONFIG_PM. But I didn't have
runtime_suspend and runtime_resume callbacks defined, only the "system
sleep" suspend and resume (I wasn't aware of the difference).
After I defined some runtime suspend/resume callbacks, they were executed
as expected.
>
> > but in theory it seems like the correct approach. Xilinx_can driver does this too.
> > Other drivers (e.g. flexcan, mpc251xfd, rcar) call clk_get_rate() right after
> > devm_clk_get() in probe, but maybe the situation there is different, I don't
> > know too much about clocks and pm_runtime yet.
>
> The API says the clock must be enabled during clk_get_rate() (but that's
> not enforced). And another problem is that the clock rate might change,
> but let's ignore the clock rate change problem for now.
>
> 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 |
Thanks, regards
Matej
^ permalink raw reply [flat|nested] 30+ messages in thread