Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 9 Jul 2020 04:31:26 -0700
From:   Richard Cochran <richardcochran@gmail.com>
To:     "Ooi, Joyce" <joyce.ooi@intel.com>
Cc:     Thor Thayer <thor.thayer@linux.intel.com>,
        "David S . Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Dalon Westergreen <dalon.westergreen@linux.intel.com>,
        Tan Ley Foon <ley.foon.tan@intel.com>,
        See Chin Liang <chin.liang.see@intel.com>,
        Dinh Nguyen <dinh.nguyen@intel.com>,
        Dalon Westergreen <dalon.westergreen@intel.com>
Subject: Re: [PATCH v4 08/10] net: eth: altera: add support for ptp and
 timestamping
Message-ID: <20200709113126.GA776@hoboy>
References: <20200708072401.169150-1-joyce.ooi@intel.com>
 <20200708072401.169150-9-joyce.ooi@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200708072401.169150-9-joyce.ooi@intel.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Jul 08, 2020 at 03:23:59PM +0800, Ooi, Joyce wrote:

> @@ -222,6 +223,32 @@ static void tse_get_regs(struct net_device *dev, struct ethtool_regs *regs,
>  		buf[i] = csrrd32(priv->mac_dev, i * 4);
>  }
>  
> +static int tse_get_ts_info(struct net_device *dev,
> +			   struct ethtool_ts_info *info)
> +{
> +	struct altera_tse_private *priv = netdev_priv(dev);
> +
> +	if (priv->ptp_enable) {
> +		if (priv->ptp_priv.ptp_clock)
> +			info->phc_index =
> +				ptp_clock_index(priv->ptp_priv.ptp_clock);

Need to handle case where priv->ptp_priv.ptp_clock == NULL.

> +		info->so_timestamping = SOF_TIMESTAMPING_TX_HARDWARE |
> +					SOF_TIMESTAMPING_RX_HARDWARE |
> +					SOF_TIMESTAMPING_RAW_HARDWARE;
> +
> +		info->tx_types = (1 << HWTSTAMP_TX_OFF) |
> +						 (1 << HWTSTAMP_TX_ON);

No need to break statement.  This fits nicely on one line.

> +
> +		info->rx_filters = (1 << HWTSTAMP_FILTER_NONE) |
> +						   (1 << HWTSTAMP_FILTER_ALL);
> +
> +		return 0;
> +	} else {

No need for else block.

> +		return ethtool_op_get_ts_info(dev, info);
> +	}
> +}
> +
>  static const struct ethtool_ops tse_ethtool_ops = {
>  	.get_drvinfo = tse_get_drvinfo,
>  	.get_regs_len = tse_reglen,


> @@ -1309,6 +1324,83 @@ static int tse_shutdown(struct net_device *dev)
>  	return 0;
>  }
>  
> +/* ioctl to configure timestamping */
> +static int tse_do_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> +{
> +	struct altera_tse_private *priv = netdev_priv(dev);
> +	struct hwtstamp_config config;

Need to check here for phy_has_hwtstamp() and pass through to PHY
layer if true.

> +
> +	if (!netif_running(dev))
> +		return -EINVAL;
> +
> +	if (!priv->ptp_enable)	{
> +		netdev_alert(priv->dev, "Timestamping not supported");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (cmd == SIOCSHWTSTAMP) {
> +		if (copy_from_user(&config, ifr->ifr_data,
> +				   sizeof(struct hwtstamp_config)))
> +			return -EFAULT;
> +
> +		if (config.flags)
> +			return -EINVAL;
> +
> +		switch (config.tx_type) {
> +		case HWTSTAMP_TX_OFF:
> +			priv->hwts_tx_en = 0;
> +			break;
> +		case HWTSTAMP_TX_ON:
> +			priv->hwts_tx_en = 1;
> +			break;
> +		default:
> +			return -ERANGE;
> +		}
> +
> +		switch (config.rx_filter) {
> +		case HWTSTAMP_FILTER_NONE:
> +			priv->hwts_rx_en = 0;
> +			config.rx_filter = HWTSTAMP_FILTER_NONE;
> +			break;
> +		default:
> +			priv->hwts_rx_en = 1;
> +			config.rx_filter = HWTSTAMP_FILTER_ALL;
> +			break;
> +		}
> +
> +		if (copy_to_user(ifr->ifr_data, &config,
> +				 sizeof(struct hwtstamp_config)))
> +			return -EFAULT;
> +		else
> +			return 0;
> +	}
> +
> +	if (cmd == SIOCGHWTSTAMP) {
> +		config.flags = 0;
> +
> +		if (priv->hwts_tx_en)
> +			config.tx_type = HWTSTAMP_TX_ON;
> +		else
> +			config.tx_type = HWTSTAMP_TX_OFF;
> +
> +		if (priv->hwts_rx_en)
> +			config.rx_filter = HWTSTAMP_FILTER_ALL;
> +		else
> +			config.rx_filter = HWTSTAMP_FILTER_NONE;
> +
> +		if (copy_to_user(ifr->ifr_data, &config,
> +				 sizeof(struct hwtstamp_config)))
> +			return -EFAULT;
> +		else
> +			return 0;
> +	}
> +
> +	if (!dev->phydev)
> +		return -EINVAL;
> +
> +	return phy_mii_ioctl(dev->phydev, ifr, cmd);
> +}
> +
>  static struct net_device_ops altera_tse_netdev_ops = {
>  	.ndo_open		= tse_open,
>  	.ndo_stop		= tse_shutdown,


> @@ -1568,6 +1661,27 @@ static int altera_tse_probe(struct platform_device *pdev)
>  		netdev_err(ndev, "Cannot attach to PHY (error: %d)\n", ret);
>  		goto err_init_phy;
>  	}
> +
> +	priv->ptp_enable = of_property_read_bool(pdev->dev.of_node,
> +						 "altr,has-ptp");

The name "ptp_enable" is a poor choice.  It sounds like something that
can be enabled at run time.  Suggest "has_ptp" instead.

> +	dev_info(&pdev->dev, "PTP Enable: %d\n", priv->ptp_enable);
> +
> +	if (priv->ptp_enable) {
> +		/* MAP PTP */
> +		ret = intel_fpga_tod_probe(pdev, &priv->ptp_priv);
> +		if (ret) {
> +			dev_err(&pdev->dev, "cannot map PTP\n");
> +			goto err_init_phy;
> +		}
> +		ret = intel_fpga_tod_register(&priv->ptp_priv,
> +					      priv->device);
> +		if (ret) {
> +			dev_err(&pdev->dev, "Failed to register PTP clock\n");
> +			ret = -ENXIO;
> +			goto err_init_phy;
> +		}
> +	}
> +
>  	return 0;
>  
>  err_init_phy:


> +/* Initialize PTP control block registers */
> +int intel_fpga_tod_init(struct intel_fpga_tod_private *priv)
> +{
> +	struct timespec64 now;
> +	int ret = 0;
> +
> +	ret = intel_fpga_tod_adjust_fine(&priv->ptp_clock_ops, 0l);

Why clobber a learned frequency offset here?  If user space closes
then re-opens, then it expects the old frequency to be preserved.

It is fine to set this to zero when the driver loads, but not after.

> +	if (ret != 0)
> +		goto out;
> +
> +	/* Initialize the hardware clock to the system time */
> +	ktime_get_real_ts64(&now);

Please initialize to zero instead, as some people prefer it that way.

(But only the first time when the driver loads!)

> +	intel_fpga_tod_set_time(&priv->ptp_clock_ops, &now);
> +
> +	spin_lock_init(&priv->tod_lock);
> +
> +out:
> +	return ret;
> +}

Thanks,
Richard