On 12.10.2022 08:25:56, Matej Vasilevski wrote:
> This patch adds support for retrieving hardware timestamps to RX and
Later in the code you set struct ethtool_ts_info::tx_types but the
driver doesn't set TX timestamps, does it?
> error CAN frames. It uses timecounter and cyclecounter structures,
> because the timestamping counter width depends on the IP core integration
> (it might not always be 64-bit).
> For platform devices, you should specify "ts" clock in device tree.
> For PCI devices, the timestamping frequency is assumed to be the same
> as bus frequency.
>
> Signed-off-by: Matej Vasilevski <matej.vasilevski@xxxxxxxxx>
[...]
> diff --git a/drivers/net/can/ctucanfd/ctucanfd_base.c b/drivers/net/can/ctucanfd/ctucanfd_base.c
> index b8da15ea6ad9..079819d53e23 100644
> --- a/drivers/net/can/ctucanfd/ctucanfd_base.c
> +++ b/drivers/net/can/ctucanfd/ctucanfd_base.c
[...]
> @@ -950,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);
> }
>
> @@ -1230,6 +1271,9 @@ static int ctucan_open(struct net_device *ndev)
> goto err_chip_start;
> }
>
> + if (priv->timestamp_possible)
> + ctucan_timestamp_init(priv);
> +
This is racy. You have to init the timestamping before the start of the
chip, i.e. enabling the IRQs. I had the same problem with the gs_usb
driver:
| https://lore.kernel.org/all/20220921081329.385509-1-mkl@xxxxxxxxxxxxxx
> netdev_info(ndev, "ctu_can_fd device registered\n");
> napi_enable(&priv->napi);
> netif_start_queue(ndev);
> @@ -1262,6 +1306,8 @@ 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);
Can you make this symmetric with respect to the ctucan_open() function.
>
> pm_runtime_put(priv->dev);
>
> @@ -1294,15 +1340,88 @@ static int ctucan_get_berr_counter(const struct net_device *ndev, struct can_ber
> return 0;
> }
[...]
> @@ -1385,15 +1534,29 @@ 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");
> + if (!priv->can_clk)
> + /* For compatibility with (older) device trees without clock-names */
> + priv->can_clk = devm_clk_get(dev, NULL);
> if (IS_ERR(priv->can_clk)) {
> - dev_err(dev, "Device clock not found.\n");
> + dev_err(dev, "Device clock not found: %pe.\n", priv->can_clk);
> ret = PTR_ERR(priv->can_clk);
> goto err_free;
> }
> can_clk_rate = clk_get_rate(priv->can_clk);
> }
>
> + if (!timestamp_clk_rate) {
> + priv->timestamp_clk = devm_clk_get(dev, "ts");
> + if (IS_ERR(priv->timestamp_clk)) {
> + /* Take the core clock instead */
> + dev_info(dev, "Failed to get ts clk\n");
> + priv->timestamp_clk = priv->can_clk;
> + }
> + clk_prepare_enable(priv->timestamp_clk);
> + timestamp_clk_rate = clk_get_rate(priv->timestamp_clk);
> + }
> +
> priv->write_reg = ctucan_write32_le;
> priv->read_reg = ctucan_read32_le;
>
> @@ -1424,6 +1587,50 @@ int ctucan_probe_common(struct device *dev, void __iomem *addr, int irq, unsigne
>
> priv->can.clock.freq = can_clk_rate;
>
> + /* Obtain timestamping counter bit size */
> + timestamp_bit_size = FIELD_GET(REG_ERR_CAPT_TS_BITS,
> + ctucan_read32(priv, CTUCANFD_ERR_CAPT));
> +
> + /* The register value is actually bit_size - 1 */
> + if (timestamp_bit_size) {
> + timestamp_bit_size += 1;
> + } else {
> + /* For 2.x versions of the IP core, we will assume 64-bit counter
> + * if there was a 0 in the register.
> + */
> + u32 version_reg = ctucan_read32(priv, CTUCANFD_DEVICE_ID);
> + 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 */
> + if (priv->timestamp_possible) {
> + u64 max_cycles;
> + u64 work_delay_ns;
> + u32 maxsec;
> +
> + priv->cc.read = ctucan_read_timestamp_cc_wrapper;
> + priv->cc.mask = CYCLECOUNTER_MASK(timestamp_bit_size);
> + maxsec = min_t(u32, CTUCANFD_MAX_WORK_DELAY_SEC,
> + div_u64(priv->cc.mask, timestamp_clk_rate));
> + clocks_calc_mult_shift(&priv->cc.mult, &priv->cc.shift,
> + timestamp_clk_rate, NSEC_PER_SEC, maxsec);
> +
> + /* shortened copy of clocks_calc_max_nsecs() */
> + max_cycles = div_u64(ULLONG_MAX, priv->cc.mult);
> + max_cycles = min(max_cycles, priv->cc.mask);
> + work_delay_ns = clocksource_cyc2ns(max_cycles, priv->cc.mult,
> + priv->cc.shift) >> 2;
I think we can use cyclecounter_cyc2ns() for this, see:
| https://elixir.bootlin.com/linux/v6.0.3/source/drivers/net/ethernet/ti/cpts.c#L642
BTW: This is the only networking driver using clocks_calc_mult_shift()
(so far) :D
> + priv->work_delay_jiffies = nsecs_to_jiffies(work_delay_ns);
> +
> + if (priv->work_delay_jiffies == 0)
> + priv->timestamp_possible = false;
> + }
> +
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:
signature.asc
Description: PGP signature
