Re: [net-next PATCH 29/29] fm10k: Add support for PTP

[Richard Cochran <richardcochran@gmail.com>]

On Thu, Sep 18, 2014 at 06:40:46PM -0400, Alexander Duyck wrote:

> +static s32 fm10k_1588_msg_vf(struct fm10k_hw *hw, u32 **results,
> +			     struct fm10k_mbx_info *mbx)
> +{
> +	struct fm10k_intfc *interface = container_of(hw,
> +						     struct fm10k_intfc,
> +						     hw);

This looks really funny to me here and in the other spot. Why not this?

	struct fm10k_intfc *interface = container_of(hw, struct fm10k_intfc, hw);

Its only one over the 80 km/h speed limit.

> +	u64 timestamp;
> +	s32 err;
> +
> +	err = fm10k_tlv_attr_get_u64(results[FM10K_1588_MSG_TIMESTAMP],
> +				     &timestamp);
> +	if (err)
> +		return err;
> +
> +	fm10k_ts_tx_hwtstamp(interface, 0, timestamp);
> +
> +	return 0;
> +}

> diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_ptp.c b/drivers/net/ethernet/intel/fm10k/fm10k_ptp.c
> new file mode 100644
> index 0000000..41da724
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/fm10k/fm10k_ptp.c

> +/* We use a 64b counter so overflow is extremely seldom.  Just
> + * to keep things sane we should check for overflow once per day
> + */

Hm...

> +void fm10k_ts_start_cc(struct fm10k_intfc *interface)
> +{
> +	struct fm10k_hw *hw = &interface->hw;
> +
> +	/* Initialize cycle counter */
> +	interface->cc.read = (hw->mac.type == fm10k_mac_pf) ? fm10k_cc_read_pf :
> +							      fm10k_cc_read_vf;
> +	interface->cc.mask = CLOCKSOURCE_MASK(64);
> +	interface->cc.mult = 1;

So shift = 0 and multi = 1. Your clock counts nanoseconds. Why not use
it directly? Then you won't need the timecounter stuff or the overflow
watchdog either.

> +
> +	/* Initialize lock protecting register access */
> +	rwlock_init(&interface->tsreg_lock);
> +
> +	/* Initialize skb queue for pending timestamp requests */
> +	skb_queue_head_init(&interface->ts_tx_skb_queue);
> +
> +	/* Initialize the clock */
> +	fm10k_ts_reset_cc(interface);
> +
> +	/* Initialize the overflow work */
> +	INIT_DELAYED_WORK(&interface->ts_overflow_work,
> +			  fm10k_ts_overflow_check);
> +	schedule_delayed_work(&interface->ts_overflow_work,
> +			      FM10K_SYSTIME_OVERFLOW_PERIOD);
> +}

> +static int fm10k_ptp_enable(struct ptp_clock_info *ptp,
> +			    struct ptp_clock_request *rq, int on)
> +{
> +	struct fm10k_intfc *interface =
> +		container_of(ptp, struct fm10k_intfc, ptp_caps);
> +	struct ptp_clock_time *t = &rq->perout.period;
> +	struct fm10k_hw *hw = &interface->hw;
> +	u64 period;
> +	u32 step;
> +
> +	/* we can only support periodic output */
> +	if (rq->type != PTP_CLK_REQ_PEROUT)
> +		return -EINVAL;
> +
> +	/* verify the requested channel is there */
> +	if (rq->perout.index >= ptp->n_per_out)
> +		return -EINVAL;
> +
> +	/* we simply cannot support the operation if we don't have BAR4 */
> +	if (!hw->sw_addr)
> +		return -ENOTSUPP;
> +
> +	/* we cannot enforce start time as there is no
> +	 * mechanism for that in the hardware, we can only control
> +	 * the period.
> +	 */

Is this because of the timecounter in the way? Another reason to use
the 64 bit nanosecond counter directly.

> diff --git a/drivers/net/ethernet/intel/fm10k/fm10k_type.h b/drivers/net/ethernet/intel/fm10k/fm10k_type.h
> index 5055bef..dac5b79 100644
> --- a/drivers/net/ethernet/intel/fm10k/fm10k_type.h
> +++ b/drivers/net/ethernet/intel/fm10k/fm10k_type.h
> @@ -225,11 +225,7 @@ struct fm10k_hw;
>  #define FM10K_STATS_NODESC_DROP		0x3807
>  
>  /* Timesync registers */
> -#define FM10K_RRTIME_CFG	0x3808
> -#define FM10K_RRTIME_LIMIT(_n)	((_n) + 0x380C)
> -#define FM10K_RRTIME_COUNT(_n)	((_n) + 0x3810)
>  #define FM10K_SYSTIME		0x3814
> -#define FM10K_SYSTIME0		0x3816
>  #define FM10K_SYSTIME_CFG	0x3818
>  #define FM10K_SYSTIME_CFG_STEP_MASK		0x0000000F
>  
> @@ -368,9 +364,6 @@ struct fm10k_hw;
>  #define FM10K_VFITR(_n)		((_n) + 0x00060)
>  
>  /* Registers contained in BAR 4 for Switch management */
> -#define FM10K_SW_SYSTIME_CFG	0x0224C
> -#define FM10K_SW_SYSTIME_CFG_STEP_SHIFT		4
> -#define FM10K_SW_SYSTIME_CFG_ADJUST_MASK	0xFF000000

You added these three lines in the previous patch.

>  #define FM10K_SW_SYSTIME_ADJUST	0x0224D
>  #define FM10K_SW_SYSTIME_ADJUST_MASK		0x3FFFFFFF
>  #define FM10K_SW_SYSTIME_ADJUST_DIR_NEGATIVE	0x80000000
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thanks,
Richard