Message-ID: <541C76B8.6040708@intel.com>
Date:	Fri, 19 Sep 2014 11:32:24 -0700
From:	Alexander Duyck <alexander.h.duyck@...el.com>
To:	Richard Cochran <richardcochran@...il.com>
CC:	davem@...emloft.net, nhorman@...hat.com, netdev@...r.kernel.org,
	john.fastabend@...il.com, matthew.vick@...el.com,
	jeffrey.t.kirsher@...el.com, sassmann@...hat.com
Subject: Re: [net-next PATCH 29/29] fm10k: Add support for PTP

On 09/19/2014 10:35 AM, Richard Cochran wrote:
> 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);
> 

Because doing it that way it extends over 80 characters.

> 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.

Because the value cannot be adjusted directly.  The timesource for the
switch is shared by all ports and host interfaces.  As such we have to
maintain a software offset per host to avoid corrupting the other clocks.

>> +
>> +	/* 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.

The problem is we cannot modify the counter as it is in use by multiple
clocks.  So if one adjusted the value it would mess up all of the others.

Also in our case the hardware design doesn't give us a register we can
plug the start time into.  We can only control the frequency of the pulse.

>> 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
>>

Yeah, I noticed that after you asked your original question.  When I had
gone through to clean up the defines that weren't used I cleaned these
up in the wrong patch.

If/when I submit a v2 I will pull them out of the previous patch in the
sequence since that is where they are added.  I already have this
resolved in my local copy as of an hour or so ago.

Thanks,

Alex
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists