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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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, &timestamp);
> > > +	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


[Index of Archives]     [Automotive Discussions]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]     [CAN Bus]

  Powered by Linux