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
