Message-ID: <20181115032411.bh6dxqh44iqap4vt@localhost>
Date:   Wed, 14 Nov 2018 19:24:11 -0800
From:   Richard Cochran <richardcochran@...il.com>
To:     Dalon Westergreen <dwesterg@...il.com>
Cc:     netdev@...r.kernel.org, dinguyen@...nel.org,
        thor.thayer@...ux.intel.com,
        Dalon Westergreen <dalon.westergreen@...el.com>
Subject: Re: [PATCH net-next 6/8] net: eth: altera: tse: add support for ptp
 and timestamping

On Wed, Nov 14, 2018 at 04:50:45PM -0800, Dalon Westergreen wrote:
> From: Dalon Westergreen <dalon.westergreen@...el.com>
> 
> Add support for the ptp clock used with the tse, and update
> the driver to support timestamping when enabled.  We also
> enable debugfs entries for the ptp clock to allow some user
> control and interaction with the ptp clock.
> 
> Signed-off-by: Dalon Westergreen <dalon.westergreen@...el.com>
> ---
>  drivers/net/ethernet/altera/Kconfig           |   1 +
>  drivers/net/ethernet/altera/Makefile          |   3 +-
>  drivers/net/ethernet/altera/altera_ptp.c      | 473 ++++++++++++++++++
>  drivers/net/ethernet/altera/altera_ptp.h      |  77 +++

Wouldn't it be nice if the PTP maintainer were included on CC for new
PTP drivers?  One can always wish...

> diff --git a/drivers/net/ethernet/altera/Kconfig b/drivers/net/ethernet/altera/Kconfig
> index fdddba51473e..36aee0fc0b51 100644
> --- a/drivers/net/ethernet/altera/Kconfig
> +++ b/drivers/net/ethernet/altera/Kconfig
> @@ -2,6 +2,7 @@ config ALTERA_TSE
>  	tristate "Altera Triple-Speed Ethernet MAC support"
>  	depends on HAS_DMA
>  	select PHYLIB
> +	select PTP_1588_CLOCK

We now use "imply" instead of "select".

> +/* A fine ToD HW clock offset adjustment.
> + * To perform the fine offset adjustment the AdjustPeriod register is used
> + * to replace the Period register for AdjustCount clock cycles in hardware.
> + */
> +static int fine_adjust_tod_clock(struct altera_ptp_private *priv,
> +				 u32 adjust_period, u32 adjust_count)

The function naming is really poor throughout this driver.  You should
use a unique prefix that relates to your driver or HW.

> +static int adjust_fine(struct ptp_clock_info *ptp, long scaled_ppm)
> +{

This is named like a global function.  Needs prefix.

> +	struct altera_ptp_private *priv =
> +		container_of(ptp, struct altera_ptp_private, ptp_clock_ops);
> +	unsigned long flags;
> +	int ret = 0;
> +	u64 ppb;
> +	u32 tod_period;
> +	u32 tod_rem;
> +	u32 tod_drift_adjust_fns;
> +	u32 tod_drift_adjust_rate;
> +	unsigned long rate;

Please put all like types together on one line, and use reverse
Christmas tree style.

> +
> +	priv->scaled_ppm = scaled_ppm;
> +
> +	/* only unlock if it is currently locked */
> +	if (mutex_is_locked(&priv->ppm_mutex))
> +		mutex_unlock(&priv->ppm_mutex);
> +
> +	if (!priv->ptp_correct_freq)
> +		goto out;
> +
> +	rate = clk_get_rate(priv->tod_clk);
> +
> +	/* From scaled_ppm_to_ppb */
> +	ppb = 1 + scaled_ppm;
> +	ppb *= 125;
> +	ppb >>= 13;
> +
> +	ppb += NOMINAL_PPB;
> +
> +	tod_period = div_u64_rem(ppb << 16, rate, &tod_rem);
> +	if (tod_period > TOD_PERIOD_MAX) {
> +		ret = -ERANGE;
> +		goto out;
> +	}
> +
> +	/* The drift of ToD adjusted periodically by adding a drift_adjust_fns
> +	 * correction value every drift_adjust_rate count of clock cycles.
> +	 */
> +	tod_drift_adjust_fns = tod_rem / gcd(tod_rem, rate);
> +	tod_drift_adjust_rate = rate / gcd(tod_rem, rate);
> +
> +	while ((tod_drift_adjust_fns > TOD_DRIFT_ADJUST_FNS_MAX) |
> +		(tod_drift_adjust_rate > TOD_DRIFT_ADJUST_RATE_MAX)) {
> +		tod_drift_adjust_fns = tod_drift_adjust_fns >> 1;
> +		tod_drift_adjust_rate = tod_drift_adjust_rate >> 1;
> +	}
> +
> +	if (tod_drift_adjust_fns == 0)
> +		tod_drift_adjust_rate = 0;
> +
> +	spin_lock_irqsave(&priv->tod_lock, flags);
> +	csrwr32(tod_period, priv->tod_ctrl, tod_csroffs(period));
> +	csrwr32(0, priv->tod_ctrl, tod_csroffs(adjust_period));
> +	csrwr32(0, priv->tod_ctrl, tod_csroffs(adjust_count));
> +	csrwr32(tod_drift_adjust_fns, priv->tod_ctrl,
> +		tod_csroffs(drift_adjust));
> +	csrwr32(tod_drift_adjust_rate, priv->tod_ctrl,
> +		tod_csroffs(drift_adjust_rate));
> +	spin_unlock_irqrestore(&priv->tod_lock, flags);
> +
> +out:
> +	return ret;
> +}
> +
> +static int adjust_time(struct ptp_clock_info *ptp, s64 delta)
> +{
> +	struct altera_ptp_private *priv =
> +		container_of(ptp, struct altera_ptp_private, ptp_clock_ops);
> +	int ret = 0;
> +	u64 abs_delta;
> +	unsigned long flags;
> +	u32 period;
> +	u32 diff;
> +	u64 count;
> +	u32 rem;

Same here.

> +	if (!priv->ptp_correct_offs)
> +		goto out;
> +
> +	if (delta < 0)
> +		abs_delta = -delta;
> +	else
> +		abs_delta = delta;
> +
> +	spin_lock_irqsave(&priv->tod_lock, flags);
> +
> +	/* Get the maximum possible value of the Period register offset
> +	 * adjustment in nanoseconds scale. This depends on the current
> +	 * Period register settings and the maximum and minimum possible
> +	 * values of the Period register.
> +	 */
> +	period = csrrd32(priv->tod_ctrl, tod_csroffs(period));
> +
> +	if (delta < 0)
> +		diff = (period - TOD_PERIOD_MIN) >> 16;
> +	else
> +		diff = (TOD_PERIOD_MAX - period) >> 16;
> +
> +	/* Find out the least number of cycles needed for the Period register
> +	 * adjustment by delta nanoseconds.
> +	 */
> +	while (diff) {

Ugh.  How many 64 divisions are you going to do?  Is there no better
way?

> +		count = div_u64_rem(abs_delta, diff, &rem);
> +		if (rem == 0)
> +			break;
> +		diff--;
> +	}
> +
> +	if (diff && count && count < TOD_ADJUST_COUNT_MAX) {
> +		/* Perform the fine time offset adjustment */
> +		if (delta < 0)
> +			period -= (diff << 16);
> +		else
> +			period += (diff << 16);
> +
> +		ret = fine_adjust_tod_clock(priv, period, count);
> +
> +	} else {
> +		/* Perform the coarse time offset adjustment */
> +		ret = coarse_adjust_tod_clock(priv, delta);
> +	}
> +
> +	spin_unlock_irqrestore(&priv->tod_lock, flags);
> +
> +out:
> +	return ret;
> +}
> +
> +static int get_time(struct ptp_clock_info *ptp, struct timespec64 *ts)
> +{
> +	struct altera_ptp_private *priv =
> +		container_of(ptp, struct altera_ptp_private, ptp_clock_ops);
> +	unsigned long flags;
> +	u64 seconds;
> +	u32 seconds_msb;
> +	u32 seconds_lsb;
> +	u32 nanosec;
> +
> +	spin_lock_irqsave(&priv->tod_lock, flags);
> +	nanosec = csrrd32(priv->tod_ctrl, tod_csroffs(nanosec));
> +	seconds_lsb = csrrd32(priv->tod_ctrl, tod_csroffs(seconds_lsb));
> +	seconds_msb = csrrd32(priv->tod_ctrl, tod_csroffs(seconds_msb));
> +	spin_unlock_irqrestore(&priv->tod_lock, flags);
> +
> +	seconds = (((u64)(seconds_msb & 0x0000ffff)) << 32) | seconds_lsb;
> +
> +	ts->tv_nsec = nanosec;
> +	ts->tv_sec = (__kernel_time_t)seconds;
> +
> +	return 0;
> +}
> +
> +static int set_time(struct ptp_clock_info *ptp, const struct timespec64 *ts)
> +{
> +	struct altera_ptp_private *priv =
> +		container_of(ptp, struct altera_ptp_private, ptp_clock_ops);
> +	unsigned long flags;
> +	u32 seconds_msb = upper_32_bits(ts->tv_sec) & 0x0000ffff;
> +	u32 seconds_lsb = lower_32_bits(ts->tv_sec);
> +	u32 nanosec = lower_32_bits(ts->tv_nsec);
> +
> +	spin_lock_irqsave(&priv->tod_lock, flags);
> +	csrwr32(seconds_msb, priv->tod_ctrl, tod_csroffs(seconds_msb));
> +	csrwr32(seconds_lsb, priv->tod_ctrl, tod_csroffs(seconds_lsb));
> +	csrwr32(nanosec, priv->tod_ctrl, tod_csroffs(nanosec));
> +	spin_unlock_irqrestore(&priv->tod_lock, flags);
> +
> +	return 0;
> +}
> +
> +static int enable_feature(struct ptp_clock_info *ptp,
> +			  struct ptp_clock_request *request, int on)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static struct ptp_clock_info altera_ptp_clock_ops = {
> +	.owner = THIS_MODULE,
> +	.name = "altera_ptp",
> +	.max_adj = 500000000,
> +	.n_alarm = 0,
> +	.n_ext_ts = 0,
> +	.n_per_out = 0,
> +	.pps = 0,
> +	.adjfine = adjust_fine,
> +	.adjtime = adjust_time,
> +	.gettime64 = get_time,
> +	.settime64 = set_time,
> +	.enable = enable_feature,

Use driver-specific naming, please.

> +};
> +
> +/* ptp debugfs */
> +static ssize_t ptp_tod_read(struct file *filp, char __user *buffer,
> +			    size_t count, loff_t *ppos)
> +{

Please drop this debugfs stuff.  We can read the clock with clock_gettime().

> +	struct altera_ptp_private *priv = filp->private_data;
> +	unsigned long flags;
> +	u32 seconds_msb;
> +	u32 seconds_lsb;
> +	u32 nanosec;
> +
> +	if (*ppos != 0)
> +		return 0;
> +
> +	if (count < (sizeof(u32) * 3))
> +		return -ENOSPC;
> +
> +	spin_lock_irqsave(&priv->tod_lock, flags);
> +	nanosec = csrrd32(priv->tod_ctrl, tod_csroffs(nanosec));
> +	seconds_lsb = csrrd32(priv->tod_ctrl, tod_csroffs(seconds_lsb));
> +	seconds_msb = csrrd32(priv->tod_ctrl, tod_csroffs(seconds_msb));
> +	spin_unlock_irqrestore(&priv->tod_lock, flags);
> +
> +	if (copy_to_user(buffer, &seconds_msb, sizeof(u32)))
> +		return -EFAULT;
> +	buffer += sizeof(u32);
> +
> +	if (copy_to_user(buffer, &seconds_lsb, sizeof(u32)))
> +		return -EFAULT;
> +	buffer += sizeof(u32);
> +
> +	if (copy_to_user(buffer, &nanosec, sizeof(u32)))
> +		return -EFAULT;
> +
> +	return (sizeof(u32) * 3);
> +}
> +
> +static const struct file_operations ptp_dbg_tod_ops = {
> +	.owner = THIS_MODULE,
> +	.open = simple_open,
> +	.read = ptp_tod_read,
> +	.write = NULL,
> +};
> +
> +static ssize_t ptp_ppm_read(struct file *filp, char __user *buffer,
> +			    size_t count, loff_t *ppos)
> +{
> +	struct altera_ptp_private *priv = filp->private_data;
> +
> +	if (*ppos != 0)
> +		return 0;
> +
> +	if (count < sizeof(long))
> +		return -ENOSPC;
> +
> +	if (mutex_lock_interruptible(&priv->ppm_mutex))
> +		return -EAGAIN;
> +
> +	if (copy_to_user(buffer, &priv->scaled_ppm, sizeof(long)))
> +		return -EFAULT;
> +
> +	return sizeof(long);
> +}
> +
> +static const struct file_operations ptp_dbg_ppm_ops = {
> +	.owner = THIS_MODULE,
> +	.open = simple_open,
> +	.read = ptp_ppm_read,
> +	.write = NULL,
> +};
> +
> +int altera_ptp_dbg_init(struct altera_ptp_private *priv,
> +			struct dentry *dfs_dir_root)
> +{
> +	struct dentry *dfs_dir;
> +
> +	dfs_dir = debugfs_create_dir("ptp", dfs_dir_root);
> +
> +	/* Use u32 instead of bool */
> +	if (!debugfs_create_u32("correct_offset", 0600, dfs_dir,
> +				&priv->ptp_correct_offs))
> +		return -ENOMEM;
> +
> +	/* Use u32 instead of bool */
> +	if (!debugfs_create_u32("correct_frequency", 0600, dfs_dir,
> +				&priv->ptp_correct_freq))
> +		return -ENOMEM;
> +
> +	if (!debugfs_create_file("tod", 0400, dfs_dir,
> +				 (void *)priv, &ptp_dbg_tod_ops))
> +		return -ENOMEM;
> +
> +	if (!debugfs_create_file("scaled_ppm", 0400, dfs_dir,
> +				 (void *)priv, &ptp_dbg_ppm_ops))
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
> +/* Initialize PTP control block registers */
> +int altera_init_ptp(struct altera_ptp_private *priv)
> +{

So here we have a prefix, but "altera" is a poor choice.  That is (or
was) a company name, not the name of a specific device.  Imagine if a
MAC driver used "intel_init_mac()"!

Maybe "altera_tse_" ?

Thanks,
Richard

Powered by blists - more mailing lists