Hi Marc, thanks for another review from you. I'll merge the responses for all three mails from you, so I don't spam the mailing list too much. On Mon, Oct 24, 2022 at 10:02:38PM +0200, Marc Kleine-Budde wrote: > 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? > No, it doesn't explicitly. Unless something changed and I don't know about it, all the drivers using can_put_echo_skb() (includes ctucanfd) now report software (hardware if available) tx timestamps thanks to Vincent's patch. https://git.kernel.org/pub/scm/linux/kernel/git/mkl/linux-can-next.git/commit/?id=12a18d79dc14c80b358dbd26461614b97f2ea4a6 > > 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 Thanks for pointing this out, I'll fix this. > > > 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. Yes, will do. > > > > 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 > I don't really see the benefit at the moment (I have to include clocksource.h anyway due to the clocks_calc_mult_shift()), but sure, I'll use cyclecounter_cyc2ns(). Fun fact :-D I might look into the cpts.c > > + 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 | Mail 2: >Regarding the timestamp_clk handling: > >If you prepare_enable the timestamp_clk during probe_common() and don't >disable_unprepare it, it stays on the whole lifetime of the driver. So >there's no need/reason for the runtime suspend/resume functions. > >So either keep the clock powered and remove the suspend/resume functions >or shut down the clock after probe. > >If you want to make things 1000% clean, you can get the timestamp's >clock rate during open() and re-calculate the mult and shift. The >background is that the clock rate might change if the clock is not >enabled (at least that's not guaranteed by the common clock framework). >Actual HW implementations might differ. Hmm, I thought that pm_runtime_put() will eventually run runtime suspend callback, but now I see that it will run only the idle callback (which I haven't defined). I'll remove the runtime suspend/resume callbacks. Best regards, Matej
