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@xxxxxxxxxxxxxx/
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
