From mboxrd@z Thu Jan 1 00:00:00 1970 From: Richard Cochran Subject: Re: [PATCH net-next 6/8] net: eth: altera: tse: add support for ptp and timestamping Date: Wed, 14 Nov 2018 19:24:11 -0800 Message-ID: <20181115032411.bh6dxqh44iqap4vt@localhost> References: <20181115005047.28464-1-dwesterg@gmail.com> <20181115005047.28464-7-dwesterg@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: netdev@vger.kernel.org, dinguyen@kernel.org, thor.thayer@linux.intel.com, Dalon Westergreen To: Dalon Westergreen Return-path: Received: from mail-pf1-f195.google.com ([209.85.210.195]:43439 "EHLO mail-pf1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726807AbeKONaS (ORCPT ); Thu, 15 Nov 2018 08:30:18 -0500 Received: by mail-pf1-f195.google.com with SMTP id g7-v6so8950992pfo.10 for ; Wed, 14 Nov 2018 19:24:14 -0800 (PST) Content-Disposition: inline In-Reply-To: <20181115005047.28464-7-dwesterg@gmail.com> Sender: netdev-owner@vger.kernel.org List-ID: On Wed, Nov 14, 2018 at 04:50:45PM -0800, Dalon Westergreen wrote: > From: Dalon Westergreen > > 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 > --- > 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