RE: [PATCH net-next v3 1/4] net: wangxun: Add support for PTP clock

["Jiawen Wu" <jiawenwu@trustnetic.com>]

> > @@ -1501,12 +1535,19 @@ static netdev_tx_t wx_xmit_frame_ring(struct sk_buff *skb,
> >   	if (test_bit(WX_FLAG_FDIR_CAPABLE, wx->flags) && tx_ring->atr_sample_rate)
> >   		wx->atr(tx_ring, first, ptype);
> >
> > -	wx_tx_map(tx_ring, first, hdr_len);
> > +	if (wx_tx_map(tx_ring, first, hdr_len))
> > +		goto cleanup_tx_tstamp;
> >
> >   	return NETDEV_TX_OK;
> >   out_drop:
> >   	dev_kfree_skb_any(first->skb);
> >   	first->skb = NULL;
> > +cleanup_tx_tstamp:
> > +	if (unlikely(tx_flags & WX_TX_FLAGS_TSTAMP)) {
> > +		dev_kfree_skb_any(wx->ptp_tx_skb);
> > +		wx->ptp_tx_skb = NULL;
> > +		clear_bit_unlock(WX_STATE_PTP_TX_IN_PROGRESS, wx->state);
> > +	}
> 
> This is error path of dma mapping, means TX timestamp will be missing
> because the packet was not sent. But the error/missing counter is not
> bumped. I think it needs to be indicated.

I'll count it as 'err' in ethtool_ts_stats.

> > +static int wx_ptp_set_timestamp_mode(struct wx *wx,
> > +				     struct kernel_hwtstamp_config *config)
> > +{
> > +	u32 tsync_tx_ctl = WX_TSC_1588_CTL_ENABLED;
> > +	u32 tsync_rx_ctl = WX_PSR_1588_CTL_ENABLED;
> > +	DECLARE_BITMAP(flags, WX_PF_FLAGS_NBITS);
> > +	u32 tsync_rx_mtrl = PTP_EV_PORT << 16;
> > +	bool is_l2 = false;
> > +	u32 regval;
> > +
> > +	memcpy(flags, wx->flags, sizeof(wx->flags));
> > +
> > +	switch (config->tx_type) {
> > +	case HWTSTAMP_TX_OFF:
> > +		tsync_tx_ctl = 0;
> > +		break;
> > +	case HWTSTAMP_TX_ON:
> > +		break;
> > +	default:
> > +		return -ERANGE;
> > +	}
> > +
> > +	switch (config->rx_filter) {
> > +	case HWTSTAMP_FILTER_NONE:
> > +		tsync_rx_ctl = 0;
> > +		tsync_rx_mtrl = 0;
> > +		clear_bit(WX_FLAG_RX_HWTSTAMP_ENABLED, flags);
> > +		clear_bit(WX_FLAG_RX_HWTSTAMP_IN_REGISTER, flags);
> > +		break;
> > +	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
> > +		tsync_rx_ctl |= WX_PSR_1588_CTL_TYPE_L4_V1;
> > +		tsync_rx_mtrl |= WX_PSR_1588_MSG_V1_SYNC;
> > +		set_bit(WX_FLAG_RX_HWTSTAMP_ENABLED, flags);
> > +		set_bit(WX_FLAG_RX_HWTSTAMP_IN_REGISTER, flags);
> > +		break;
> > +	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
> > +		tsync_rx_ctl |= WX_PSR_1588_CTL_TYPE_L4_V1;
> > +		tsync_rx_mtrl |= WX_PSR_1588_MSG_V1_DELAY_REQ;
> > +		set_bit(WX_FLAG_RX_HWTSTAMP_ENABLED, flags);
> > +		set_bit(WX_FLAG_RX_HWTSTAMP_IN_REGISTER, flags);
> > +		break;
> > +	case HWTSTAMP_FILTER_PTP_V2_EVENT:
> > +	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
> > +	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
> > +	case HWTSTAMP_FILTER_PTP_V2_SYNC:
> > +	case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
> > +	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
> > +	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
> > +	case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
> > +	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
> > +		tsync_rx_ctl |= WX_PSR_1588_CTL_TYPE_EVENT_V2;
> > +		is_l2 = true;
> > +		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
> > +		set_bit(WX_FLAG_RX_HWTSTAMP_ENABLED, flags);
> > +		set_bit(WX_FLAG_RX_HWTSTAMP_IN_REGISTER, flags);
> > +		break;
> > +	default:
> > +		/* register RXMTRL must be set in order to do V1 packets,
> > +		 * therefore it is not possible to time stamp both V1 Sync and
> > +		 * Delay_Req messages unless hardware supports timestamping all
> > +		 * packets => return error
> > +		 */
> > +		clear_bit(WX_FLAG_RX_HWTSTAMP_ENABLED, wx->flags);
> > +		clear_bit(WX_FLAG_RX_HWTSTAMP_IN_REGISTER, wx->flags);
> > +		config->rx_filter = HWTSTAMP_FILTER_NONE;
> > +		return -ERANGE;
> 
> looks like this code is a bit tricky and leads to out-of-sync
> configuration. Imagine the situation when HWTSTAMP_FILTER_PTP_V2_EVENT
> was configured first, the hardware was properly set up and timestamps
> are coming. wx->flags will have bits WX_FLAG_RX_HWTSTAMP_ENABLED and
> WX_FLAG_RX_HWTSTAMP_IN_REGISTER set. Then the user asks to enable
> HWTSTAMP_FILTER_ALL, which is not supported. wx->flags will have bits
> mentioned above cleared, but the hardware will still continue to
> timestamp some packets.

You are right. I'll remove the bit clears in the default case.

> > +void wx_ptp_reset(struct wx *wx)
> > +{
> > +	unsigned long flags;
> > +
> > +	/* reset the hardware timestamping mode */
> > +	wx_ptp_set_timestamp_mode(wx, &wx->tstamp_config);
> > +	wx_ptp_reset_cyclecounter(wx);
> > +
> > +	wr32ptp(wx, WX_TSC_1588_SYSTIML, 0);
> > +	wr32ptp(wx, WX_TSC_1588_SYSTIMH, 0);
> > +	WX_WRITE_FLUSH(wx);
> 
> writes to WX_TSC_1588_SYSTIML/WX_TSC_1588_SYSTIMH are not protected by
> tmreg_lock, while reads are protected in wx_ptp_read() and in
> wx_ptp_gettimex64()

No need to protect it. See below.

> > @@ -1133,6 +1168,21 @@ struct wx {
> >   	void (*atr)(struct wx_ring *ring, struct wx_tx_buffer *first, u8 ptype);
> >   	void (*configure_fdir)(struct wx *wx);
> >   	void (*do_reset)(struct net_device *netdev);
> > +
> > +	u32 base_incval;
> > +	u32 tx_hwtstamp_pkts;
> > +	u32 tx_hwtstamp_timeouts;
> > +	u32 tx_hwtstamp_skipped;
> > +	u32 rx_hwtstamp_cleared;
> > +	unsigned long ptp_tx_start;
> > +	spinlock_t tmreg_lock; /* spinlock for ptp */
> 
> Could you please explain what this lock protects exactly? According to
> the name, it should serialize access to tm(?) registers, but there is
> a mix of locked and unlocked accesses in the code ...
> If this lock protects cyclecounter/timecounter then it might be better
> to use another name, like hw_cc_lock. And in this case it's even better
> to use seqlock_t with reader/writer accessors according to the code path.

It is for struct timecounter. The registers are read only to update the cycle
counter. I think  it's better to  name it hw_tc_lock.