lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 12 Jul 2018 20:31:35 -0700
From:   Richard Cochran <richardcochran@...il.com>
To:     Bryan Whitehead <Bryan.Whitehead@...rochip.com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org,
        UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH v2 net-next 9/9] lan743x: Add PTP support

On Thu, Jul 12, 2018 at 03:05:06PM -0400, Bryan Whitehead wrote:
> +static int lan743x_ethtool_get_ts_info(struct net_device *netdev,
> +				       struct ethtool_ts_info *ts_info)
> +{
> +	struct lan743x_adapter *adapter = netdev_priv(netdev);
> +
> +	ts_info->so_timestamping = SOF_TIMESTAMPING_TX_SOFTWARE |
> +				   SOF_TIMESTAMPING_RX_SOFTWARE |
> +				   SOF_TIMESTAMPING_SOFTWARE |
> +				   SOF_TIMESTAMPING_TX_HARDWARE |
> +				   SOF_TIMESTAMPING_RX_HARDWARE |
> +				   SOF_TIMESTAMPING_RAW_HARDWARE;
> +#ifdef CONFIG_PTP_1588_CLOCK

No need for this ifdeferry - ptp_clock_index() already returns -1 in
that case.

> +	if (adapter->ptp.ptp_clock)
> +		ts_info->phc_index = ptp_clock_index(adapter->ptp.ptp_clock);
> +	else
> +		ts_info->phc_index = -1;
> +#else
> +	ts_info->phc_index = -1;
> +#endif
> +	ts_info->tx_types = BIT(HWTSTAMP_TX_OFF) |
> +			    BIT(HWTSTAMP_TX_ON);
> +	ts_info->rx_filters = BIT(HWTSTAMP_FILTER_NONE) |
> +			      BIT(HWTSTAMP_FILTER_ALL);
> +	return 0;
> +}
> +

> @@ -690,6 +717,7 @@ const struct ethtool_ops lan743x_ethtool_ops = {
>  	.get_rxfh_indir_size = lan743x_ethtool_get_rxfh_indir_size,
>  	.get_rxfh = lan743x_ethtool_get_rxfh,
>  	.set_rxfh = lan743x_ethtool_set_rxfh,
> +	.get_ts_info = lan743x_ethtool_get_ts_info,
>  	.get_eee = lan743x_ethtool_get_eee,
>  	.set_eee = lan743x_ethtool_set_eee,
>  	.get_link_ksettings = phy_ethtool_get_link_ksettings,
> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
> index 953b581..ca9ae49 100644
> --- a/drivers/net/ethernet/microchip/lan743x_main.c
> +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> @@ -267,6 +267,10 @@ static void lan743x_intr_shared_isr(void *context, u32 int_sts, u32 flags)
>  			lan743x_intr_software_isr(adapter);
>  			int_sts &= ~INT_BIT_SW_GP_;
>  		}
> +		if (int_sts & INT_BIT_1588_) {
> +			lan743x_ptp_isr(adapter);
> +			int_sts &= ~INT_BIT_1588_;
> +		}
>  	}
>  	if (int_sts)
>  		lan743x_csr_write(adapter, INT_EN_CLR, int_sts);
> @@ -976,6 +980,7 @@ static void lan743x_phy_link_status_change(struct net_device *netdev)
>  					       ksettings.base.duplex,
>  					       local_advertisement,
>  					       remote_advertisement);
> +		lan743x_ptp_update_latency(adapter, ksettings.base.speed);
>  	}
>  }
>  
> @@ -1256,11 +1261,29 @@ static void lan743x_tx_release_desc(struct lan743x_tx *tx,
>  		buffer_info->dma_ptr = 0;
>  		buffer_info->buffer_length = 0;
>  	}
> -	if (buffer_info->skb) {
> +	if (!buffer_info->skb)
> +		goto clear_active;
> +
> +	if (!(buffer_info->flags &
> +		TX_BUFFER_INFO_FLAG_TIMESTAMP_REQUESTED)) {

Bad line break.

>  		dev_kfree_skb(buffer_info->skb);
> -		buffer_info->skb = NULL;
> +		goto clear_skb;
>  	}
>  
> +	if (cleanup) {
> +		lan743x_ptp_unrequest_tx_timestamp(tx->adapter);
> +		dev_kfree_skb(buffer_info->skb);
> +	} else {
> +		lan743x_ptp_tx_timestamp_skb(tx->adapter,
> +					     buffer_info->skb,
> +					     (buffer_info->flags &
> +					     TX_BUFFER_INFO_FLAG_IGNORE_SYNC)
> +					     != 0);

This is poor coding style.  Please find a better way.

> +	}
> +
> +clear_skb:
> +	buffer_info->skb = NULL;
> +
>  clear_active:
>  	buffer_info->flags &= ~TX_BUFFER_INFO_FLAG_ACTIVE;
>  
> @@ -1321,10 +1344,25 @@ static int lan743x_tx_get_avail_desc(struct lan743x_tx *tx)
>  		return last_head - last_tail - 1;
>  }
>  
> +void lan743x_tx_set_timestamping_mode(struct lan743x_tx *tx,
> +				      bool enable_timestamping,
> +				      bool enable_onestep_sync)
> +{
> +	if (enable_timestamping)
> +		tx->ts_flags |= TX_TS_FLAG_TIMESTAMPING_ENABLED;
> +	else
> +		tx->ts_flags &= ~TX_TS_FLAG_TIMESTAMPING_ENABLED;
> +	if (enable_onestep_sync)
> +		tx->ts_flags |= TX_TS_FLAG_ONE_STEP_SYNC;
> +	else
> +		tx->ts_flags &= ~TX_TS_FLAG_ONE_STEP_SYNC;
> +}
> +
>  static int lan743x_tx_frame_start(struct lan743x_tx *tx,
>  				  unsigned char *first_buffer,
>  				  unsigned int first_buffer_length,
>  				  unsigned int frame_length,
> +				  bool time_stamp,
>  				  bool check_sum)
>  {
>  	/* called only from within lan743x_tx_xmit_frame.
> @@ -1362,6 +1400,8 @@ static int lan743x_tx_frame_start(struct lan743x_tx *tx,
>  		TX_DESC_DATA0_DTYPE_DATA_ |
>  		TX_DESC_DATA0_FS_ |
>  		TX_DESC_DATA0_FCS_;
> +	if (time_stamp)
> +		tx->frame_data0 |= TX_DESC_DATA0_TSE_;
>  
>  	if (check_sum)
>  		tx->frame_data0 |= TX_DESC_DATA0_ICE_ |
> @@ -1475,6 +1515,7 @@ static int lan743x_tx_frame_add_fragment(struct lan743x_tx *tx,
>  
>  static void lan743x_tx_frame_end(struct lan743x_tx *tx,
>  				 struct sk_buff *skb,
> +				 bool time_stamp,
>  				 bool ignore_sync)
>  {
>  	/* called only from within lan743x_tx_xmit_frame
> @@ -1492,6 +1533,8 @@ static void lan743x_tx_frame_end(struct lan743x_tx *tx,
>  	tx_descriptor = &tx->ring_cpu_ptr[tx->frame_tail];
>  	buffer_info = &tx->buffer_info[tx->frame_tail];
>  	buffer_info->skb = skb;
> +	if (time_stamp)
> +		buffer_info->flags |= TX_BUFFER_INFO_FLAG_TIMESTAMP_REQUESTED;
>  	if (ignore_sync)
>  		buffer_info->flags |= TX_BUFFER_INFO_FLAG_IGNORE_SYNC;
>  
> @@ -1520,6 +1563,7 @@ static netdev_tx_t lan743x_tx_xmit_frame(struct lan743x_tx *tx,
>  	unsigned int frame_length = 0;
>  	unsigned int head_length = 0;
>  	unsigned long irq_flags = 0;
> +	bool do_timestamp = false;
>  	bool ignore_sync = false;
>  	int nr_frags = 0;
>  	bool gso = false;
> @@ -1541,6 +1585,16 @@ static netdev_tx_t lan743x_tx_xmit_frame(struct lan743x_tx *tx,
>  	}
>  
>  	/* space available, transmit skb  */
> +	if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) {
> +		if (tx->ts_flags & TX_TS_FLAG_TIMESTAMPING_ENABLED) {
> +			if (lan743x_ptp_request_tx_timestamp(tx->adapter)) {

Why not use && instead of three nested tests?

> +				skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> +				do_timestamp = true;
> +				if (tx->ts_flags & TX_TS_FLAG_ONE_STEP_SYNC)
> +					ignore_sync = true;
> +			}
> +		}
> +	}
>  	head_length = skb_headlen(skb);
>  	frame_length = skb_pagelen(skb);
>  	nr_frags = skb_shinfo(skb)->nr_frags;
> @@ -1554,6 +1608,7 @@ static netdev_tx_t lan743x_tx_xmit_frame(struct lan743x_tx *tx,
>  	if (lan743x_tx_frame_start(tx,
>  				   skb->data, head_length,
>  				   start_frame_length,
> +				   do_timestamp,
>  				   skb->ip_summed == CHECKSUM_PARTIAL)) {
>  		dev_kfree_skb(skb);
>  		goto unlock;
> @@ -1581,7 +1636,7 @@ static netdev_tx_t lan743x_tx_xmit_frame(struct lan743x_tx *tx,
>  	}
>  
>  finish:
> -	lan743x_tx_frame_end(tx, skb, ignore_sync);
> +	lan743x_tx_frame_end(tx, skb, do_timestamp, ignore_sync);
>  
>  unlock:
>  	spin_unlock_irqrestore(&tx->ring_lock, irq_flags);
> @@ -2410,6 +2465,8 @@ static int lan743x_netdev_close(struct net_device *netdev)
>  	for (index = 0; index < LAN743X_USED_RX_CHANNELS; index++)
>  		lan743x_rx_close(&adapter->rx[index]);
>  
> +	lan743x_ptp_close(adapter);
> +
>  	lan743x_phy_close(adapter);
>  
>  	lan743x_mac_close(adapter);
> @@ -2437,6 +2494,10 @@ static int lan743x_netdev_open(struct net_device *netdev)
>  	if (ret)
>  		goto close_mac;
>  
> +	ret = lan743x_ptp_open(adapter);
> +	if (ret)
> +		goto close_phy;
> +
>  	lan743x_rfe_open(adapter);
>  
>  	for (index = 0; index < LAN743X_USED_RX_CHANNELS; index++) {
> @@ -2456,6 +2517,9 @@ static int lan743x_netdev_open(struct net_device *netdev)
>  		if (adapter->rx[index].ring_cpu_ptr)
>  			lan743x_rx_close(&adapter->rx[index]);
>  	}
> +	lan743x_ptp_close(adapter);
> +
> +close_phy:
>  	lan743x_phy_close(adapter);
>  
>  close_mac:
> @@ -2483,6 +2547,8 @@ static int lan743x_netdev_ioctl(struct net_device *netdev,
>  {
>  	if (!netif_running(netdev))
>  		return -EINVAL;
> +	if (cmd == SIOCSHWTSTAMP)
> +		return lan743x_ptp_ioctl(netdev, ifr, cmd);
>  	return phy_mii_ioctl(netdev->phydev, ifr, cmd);
>  }
>  
> @@ -2607,6 +2673,11 @@ static int lan743x_hardware_init(struct lan743x_adapter *adapter,
>  	adapter->intr.irq = adapter->pdev->irq;
>  	lan743x_csr_write(adapter, INT_EN_CLR, 0xFFFFFFFF);
>  	mutex_init(&adapter->dp_lock);
> +
> +	ret = lan743x_gpio_init(adapter);
> +	if (ret)
> +		return ret;
> +
>  	ret = lan743x_mac_init(adapter);
>  	if (ret)
>  		return ret;
> @@ -2615,6 +2686,10 @@ static int lan743x_hardware_init(struct lan743x_adapter *adapter,
>  	if (ret)
>  		return ret;
>  
> +	ret = lan743x_ptp_init(adapter);
> +	if (ret)
> +		return ret;
> +
>  	lan743x_rfe_update_mac_address(adapter);
>  
>  	ret = lan743x_dmac_init(adapter);

...

> diff --git a/drivers/net/ethernet/microchip/lan743x_ptp.c b/drivers/net/ethernet/microchip/lan743x_ptp.c
> new file mode 100644
> index 0000000..f14565b
> --- /dev/null
> +++ b/drivers/net/ethernet/microchip/lan743x_ptp.c
> @@ -0,0 +1,1194 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/* Copyright (C) 2018 Microchip Technology Inc. */
> +
> +#include <linux/netdevice.h>
> +#include "lan743x_main.h"
> +
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/netdevice.h>
> +#include <linux/net_tstamp.h>
> +
> +#include "lan743x_ptp.h"
> +
> +/* GPIO */
> +#define LAN743X_NUMBER_OF_GPIO          (12)
> +
> +int lan743x_gpio_init(struct lan743x_adapter *adapter)
> +{
> +	struct lan743x_gpio *gpio = &adapter->gpio;
> +
> +	spin_lock_init(&gpio->gpio_lock);
> +
> +	gpio->gpio_cfg0 = 0; /* set all direction to input, data = 0 */
> +	gpio->gpio_cfg1 = 0x0FFF0000;/* disable all gpio, set to open drain */
> +	gpio->gpio_cfg2 = 0;/* set all to 1588 low polarity level */
> +	gpio->gpio_cfg3 = 0;/* disable all 1588 output */
> +	lan743x_csr_write(adapter, GPIO_CFG0, gpio->gpio_cfg0);
> +	lan743x_csr_write(adapter, GPIO_CFG1, gpio->gpio_cfg1);
> +	lan743x_csr_write(adapter, GPIO_CFG2, gpio->gpio_cfg2);
> +	lan743x_csr_write(adapter, GPIO_CFG3, gpio->gpio_cfg3);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PTP_1588_CLOCK
> +static int lan743x_gpio_reserve_ptp_output(struct lan743x_adapter *adapter,
> +					   int bit, int ptp_channel)
> +{
> +	struct lan743x_gpio *gpio = &adapter->gpio;
> +	unsigned long irq_flags = 0;
> +	int bit_mask = BIT(bit);
> +	int ret = -EBUSY;
> +
> +	spin_lock_irqsave(&gpio->gpio_lock, irq_flags);
> +
> +	if (!(gpio->used_bits & bit_mask)) {
> +		gpio->used_bits |= bit_mask;
> +		gpio->output_bits |= bit_mask;
> +		gpio->ptp_bits |= bit_mask;
> +
> +		/* set as output, and zero initial value */
> +		gpio->gpio_cfg0 |= GPIO_CFG0_GPIO_DIR_BIT_(bit);
> +		gpio->gpio_cfg0 &= ~GPIO_CFG0_GPIO_DATA_BIT_(bit);
> +		lan743x_csr_write(adapter, GPIO_CFG0, gpio->gpio_cfg0);
> +
> +		/* enable gpio , and set buffer type to push pull */
> +		gpio->gpio_cfg1 &= ~GPIO_CFG1_GPIOEN_BIT_(bit);
> +		gpio->gpio_cfg1 |= GPIO_CFG1_GPIOBUF_BIT_(bit);
> +		lan743x_csr_write(adapter, GPIO_CFG1, gpio->gpio_cfg1);
> +
> +		/* set 1588 polarity to high */
> +		gpio->gpio_cfg2 |= GPIO_CFG2_1588_POL_BIT_(bit);
> +		lan743x_csr_write(adapter, GPIO_CFG2, gpio->gpio_cfg2);
> +
> +		if (!ptp_channel) {
> +			/* use channel A */
> +			gpio->gpio_cfg3 &= ~GPIO_CFG3_1588_CH_SEL_BIT_(bit);
> +		} else {
> +			/* use channel B */
> +			gpio->gpio_cfg3 |= GPIO_CFG3_1588_CH_SEL_BIT_(bit);
> +		}
> +		gpio->gpio_cfg3 |= GPIO_CFG3_1588_OE_BIT_(bit);
> +		lan743x_csr_write(adapter, GPIO_CFG3, gpio->gpio_cfg3);
> +
> +		ret = bit;
> +	}
> +	spin_unlock_irqrestore(&gpio->gpio_lock, irq_flags);
> +	return ret;
> +}
> +#endif /* CONFIG_PTP_1588_CLOCK */
> +
> +#ifdef CONFIG_PTP_1588_CLOCK
> +static void lan743x_gpio_release(struct lan743x_adapter *adapter, int bit)
> +{
> +	struct lan743x_gpio *gpio = &adapter->gpio;
> +	unsigned long irq_flags = 0;
> +	int bit_mask = BIT(bit);
> +
> +	spin_lock_irqsave(&gpio->gpio_lock, irq_flags);
> +	if (gpio->used_bits & bit_mask) {
> +		gpio->used_bits &= ~bit_mask;
> +		if (gpio->output_bits & bit_mask) {
> +			gpio->output_bits &= ~bit_mask;
> +
> +			if (gpio->ptp_bits & bit_mask) {
> +				gpio->ptp_bits &= ~bit_mask;
> +				/* disable ptp output */
> +				gpio->gpio_cfg3 &= ~GPIO_CFG3_1588_OE_BIT_(bit);
> +				lan743x_csr_write(adapter, GPIO_CFG3,
> +						  gpio->gpio_cfg3);
> +			}
> +			/* release gpio output */
> +
> +			/* disable gpio */
> +			gpio->gpio_cfg1 |= GPIO_CFG1_GPIOEN_BIT_(bit);
> +			gpio->gpio_cfg1 &= ~GPIO_CFG1_GPIOBUF_BIT_(bit);
> +			lan743x_csr_write(adapter, GPIO_CFG1, gpio->gpio_cfg1);
> +
> +			/* reset back to input */
> +			gpio->gpio_cfg0 &= ~GPIO_CFG0_GPIO_DIR_BIT_(bit);
> +			gpio->gpio_cfg0 &= ~GPIO_CFG0_GPIO_DATA_BIT_(bit);
> +			lan743x_csr_write(adapter, GPIO_CFG0, gpio->gpio_cfg0);
> +		}
> +	}
> +	spin_unlock_irqrestore(&gpio->gpio_lock, irq_flags);
> +}
> +#endif /* CONFIG_PTP_1588_CLOCK */
> +
> +/* PTP */
> +#define LAN743X_PTP_MAX_FREQ_ADJ_IN_PPB (31249999)
> +
> +#ifdef CONFIG_PTP_1588_CLOCK
> +static int lan743x_ptp_reserve_event_ch(struct lan743x_adapter *adapter);
> +static void lan743x_ptp_release_event_ch(struct lan743x_adapter *adapter,
> +					 int event_channel);
> +#endif
> +
> +static bool lan743x_ptp_is_enabled(struct lan743x_adapter *adapter);
> +static void lan743x_ptp_enable(struct lan743x_adapter *adapter);
> +static void lan743x_ptp_disable(struct lan743x_adapter *adapter);
> +static void lan743x_ptp_reset(struct lan743x_adapter *adapter);
> +
> +#ifdef CONFIG_PTP_1588_CLOCK
> +static void lan743x_ptp_clock_get(struct lan743x_adapter *adapter,
> +				  u32 *seconds, u32 *nano_seconds,
> +				  u32 *sub_nano_seconds);
> +static int lan743x_ptp_enable_pps(struct lan743x_adapter *adapter);
> +static void lan743x_ptp_disable_pps(struct lan743x_adapter *adapter);
> +#endif /* CONFIG_PTP_1588_CLOCK */
> +
> +#ifdef CONFIG_PTP_1588_CLOCK
> +static void lan743x_ptp_clock_step(struct lan743x_adapter *adapter,
> +				   s64 time_step_ns);
> +#endif /* CONFIG_PTP_1588_CLOCK */

The constant ifdef CONFIG_PTP_1588_CLOCK is poor style and
unnecessary.  Just group this code together under one ifdef, or better
yet put it into its own file.

> +static void lan743x_ptp_clock_set(struct lan743x_adapter *adapter,
> +				  u32 seconds, u32 nano_seconds,
> +				  u32 sub_nano_seconds);
> +
> +#ifdef CONFIG_PTP_1588_CLOCK
> +static int lan743x_ptpci_adjfreq(struct ptp_clock_info *ptpci, s32 delta_ppb)

Please implement adjfine().

 * @adjfine:  Adjusts the frequency of the hardware clock.
 *            parameter scaled_ppm: Desired frequency offset from
 *            nominal frequency in parts per million, but with a
 *            16 bit binary fractional field.
 *
 * @adjfreq:  Adjusts the frequency of the hardware clock.
 *            This method is deprecated.  New drivers should implement
 *            the @adjfine method instead.
 *            parameter delta: Desired frequency offset from nominal frequency
 *            in parts per billion

> +{
> +	struct lan743x_ptp *ptp = container_of(ptpci, struct lan743x_ptp,
> +					       ptp_clock_info);
> +	struct lan743x_adapter *adapter = container_of(ptp,
> +						       struct lan743x_adapter,
> +						       ptp);

Coding style:
	struct lan743x_adapter *adapter =
		container_of(ptp, struct lan743x_adapter, ptp);

> +	u32 lan743x_rate_adj = 0;
> +	bool positive = true;
> +	u32 u32_delta = 0;
> +	u64 u64_delta = 0;
> +
> +	if ((delta_ppb < (-LAN743X_PTP_MAX_FREQ_ADJ_IN_PPB)) ||
> +	    delta_ppb > LAN743X_PTP_MAX_FREQ_ADJ_IN_PPB) {
> +		return -EINVAL;
> +	}
> +	if (delta_ppb > 0) {
> +		u32_delta = (u32)delta_ppb;
> +		positive = true;
> +	} else {
> +		u32_delta = (u32)(-delta_ppb);
> +		positive = false;
> +	}
> +	u64_delta = (((u64)u32_delta) * 0x800000000ULL);
> +	lan743x_rate_adj = (u32)(u64_delta / 1000000000);

You need to use the div_u64() macro here.

> +
> +	if (positive)
> +		lan743x_rate_adj |= PTP_CLOCK_RATE_ADJ_DIR_;
> +
> +	lan743x_csr_write(adapter, PTP_CLOCK_RATE_ADJ,
> +			  lan743x_rate_adj);
> +
> +	netif_info(adapter, drv, adapter->netdev,
> +		   "adjfreq, delta_ppb = %d, lan743x_rate_adj = 0x%08X\n",
> +		   delta_ppb, lan743x_rate_adj);

This definitely should be at the debug level, or just delete it altogether.

> +	return 0;
> +}
> +#endif /*CONFIG_PTP_1588_CLOCK */
> +
> +#ifdef CONFIG_PTP_1588_CLOCK
> +static int lan743x_ptpci_adjtime(struct ptp_clock_info *ptpci, s64 delta)
> +{
> +	struct lan743x_ptp *ptp = container_of(ptpci, struct lan743x_ptp,
> +					       ptp_clock_info);
> +	struct lan743x_adapter *adapter = container_of(ptp,
> +						       struct lan743x_adapter,
> +						       ptp);

Coding style.

> +	bool enable_pps = false;
> +
> +	if (ptp->pps_event_ch >= 0) {
> +		lan743x_ptp_disable_pps(adapter);
> +		enable_pps = true;
> +	}
> +
> +	lan743x_ptp_clock_step(adapter, delta);
> +	netif_info(adapter, drv, adapter->netdev,
> +		   "adjtime, delta = %lld\n", delta);

Again, debug or delete.

> +
> +	if (enable_pps)
> +		lan743x_ptp_enable_pps(adapter);
> +
> +	return 0;
> +}
> +#endif /*CONFIG_PTP_1588_CLOCK */
> +
> +#ifdef CONFIG_PTP_1588_CLOCK
> +static int lan743x_ptpci_gettime64(struct ptp_clock_info *ptpci,
> +				   struct timespec64 *ts)
> +{
> +	struct lan743x_ptp *ptp = container_of(ptpci, struct lan743x_ptp,
> +					       ptp_clock_info);
> +	struct lan743x_adapter *adapter = container_of(ptp,
> +						       struct lan743x_adapter,
> +						       ptp);

Style.

> +
> +	if (ts) {
> +		u32 seconds = 0;
> +		u32 nano_seconds = 0;

Please declare stack variables at the top of the function.

> +
> +		lan743x_ptp_clock_get(adapter, &seconds, &nano_seconds, NULL);
> +		ts->tv_sec = seconds;
> +		ts->tv_nsec = nano_seconds;
> +		netif_info(adapter, drv, adapter->netdev,
> +			   "gettime = %u.%09u\n", seconds, nano_seconds);

Debug/delete

> +	} else {
> +		netif_warn(adapter, drv, adapter->netdev, "ts == NULL\n");
> +		return -EINVAL;

No need to test for 'ts'. The caller must supply a valid pointer.

> +	}
> +	return 0;
> +}
> +#endif /*CONFIG_PTP_1588_CLOCK */
> +
> +#ifdef CONFIG_PTP_1588_CLOCK
> +static int lan743x_ptpci_settime64(struct ptp_clock_info *ptpci,
> +				   const struct timespec64 *ts)
> +{
> +	struct lan743x_ptp *ptp = container_of(ptpci, struct lan743x_ptp,
> +					       ptp_clock_info);
> +	struct lan743x_adapter *adapter = container_of(ptp,
> +						       struct lan743x_adapter,
> +						       ptp);
> +	bool enable_pps = false;
> +
> +	if (ptp->pps_event_ch >= 0) {
> +		lan743x_ptp_disable_pps(adapter);
> +		enable_pps = true;
> +	}
> +
> +	if (ts) {
> +		u32 seconds = 0;
> +		u32 nano_seconds = 0;
> +
> +		if (ts->tv_sec > 0xFFFFFFFFLL ||
> +		    ts->tv_sec < 0) {

Actually seconds will exceed four bytes sooner than you think.  If
your HW has a restriction, then simply keep the seconds offset in SW
in the driver, adding it in where needed (like when reading the clock
or providing time stamps).

> +			netif_warn(adapter, drv, adapter->netdev,
> +				   "ts->tv_sec out of range, %lld\n",
> +				   ts->tv_sec);
> +			return -EINVAL;
> +		}
> +		if (ts->tv_nsec >= 1000000000L ||
> +		    ts->tv_nsec < 0) {
> +			netif_warn(adapter, drv, adapter->netdev,
> +				   "ts->tv_nsec out of range, %ld\n",
> +				   ts->tv_nsec);
> +			return -EINVAL;
> +		}
> +		seconds = ts->tv_sec;
> +		nano_seconds = ts->tv_nsec;
> +		netif_info(adapter, drv, adapter->netdev,
> +			   "settime = %u.%09u\n", seconds, nano_seconds);
> +		lan743x_ptp_clock_set(adapter, seconds, nano_seconds, 0);
> +	} else {
> +		netif_warn(adapter, drv, adapter->netdev, "ts == NULL\n");
> +		return -EINVAL;
> +	}
> +
> +	if (enable_pps)
> +		lan743x_ptp_enable_pps(adapter);
> +
> +	return 0;
> +}
> +#endif /*CONFIG_PTP_1588_CLOCK */
> +
> +#ifdef CONFIG_PTP_1588_CLOCK
> +static int lan743x_ptp_enable_pps(struct lan743x_adapter *adapter)
> +{
> +	struct lan743x_ptp *ptp = &adapter->ptp;
> +	u32 current_seconds = 0;
> +	u32 target_seconds = 0;
> +	u32 general_config = 0;
> +	int result = -ENODEV;
> +	int pps_bit = 0;

So this function is really *not* implementing the PTP_CLK_REQ_PPS
feature but rather the PTP_CLK_REQ_PEROUT with a period of once per
second.

PTP_CLK_REQ_PPS means placing a PPS event into the kernel's "hardpps"
subsystem by calling ptp_clock_event().

I'm sorry this isn't really documented.  I should fix that.

If you HW can output arbitrary signals, then you should implement
PTP_CLK_REQ_PEROUT.  In any case, you shouldn't advertise the
ptp_clock_info.pps capability.

> +	if (ptp->pps_event_ch >= 0) {
> +		result = 0;
> +		goto done;
> +	}
> +
> +	ptp->pps_event_ch = lan743x_ptp_reserve_event_ch(adapter);
> +	if (ptp->pps_event_ch < 0) {
> +		netif_warn(adapter, drv, adapter->netdev,
> +			   "Failed to reserve event channel for PPS\n");
> +		goto done;
> +	}
> +
> +	switch(adapter->csr.id_rev & ID_REV_ID_MASK_) {
> +	case ID_REV_ID_LAN7430_:
> +		pps_bit = 2;/* GPIO 2 is preferred on EVB LAN7430 */
> +		break;
> +	case ID_REV_ID_LAN7431_:
> +		pps_bit = 4;/* GPIO 4 is preferred on EVB LAN7431 */
> +		break;
> +	}
> +
> +	ptp->pps_gpio_bit = lan743x_gpio_reserve_ptp_output(adapter, pps_bit,
> +							    ptp->pps_event_ch);
> +
> +	if (ptp->pps_gpio_bit < 0) {
> +		netif_warn(adapter, drv, adapter->netdev,
> +			   "Failed to reserve gpio 0 for PPS\n");
> +		goto done;
> +	}
> +
> +	lan743x_ptp_clock_get(adapter, &current_seconds, NULL, NULL);
> +
> +	/* set the first target ahead by 2 seconds
> +	 *	to make sure its not missed
> +	 */
> +	target_seconds = current_seconds + 2;
> +
> +	/* set the new target */
> +	lan743x_csr_write(adapter,
> +			  PTP_CLOCK_TARGET_SEC_X(ptp->pps_event_ch),
> +			  0xFFFF0000);
> +	lan743x_csr_write(adapter,
> +			  PTP_CLOCK_TARGET_NS_X(ptp->pps_event_ch), 0);
> +
> +	general_config = lan743x_csr_read(adapter, PTP_GENERAL_CONFIG);
> +
> +	general_config &= ~(PTP_GENERAL_CONFIG_CLOCK_EVENT_X_MASK_
> +			  (ptp->pps_event_ch));
> +	general_config |= PTP_GENERAL_CONFIG_CLOCK_EVENT_X_SET_
> +			  (ptp->pps_event_ch,
> +			   PTP_GENERAL_CONFIG_CLOCK_EVENT_100US_);
> +	general_config &= ~PTP_GENERAL_CONFIG_RELOAD_ADD_X_
> +			  (ptp->pps_event_ch);
> +	lan743x_csr_write(adapter, PTP_GENERAL_CONFIG, general_config);
> +
> +	/* set the reload to one second steps */
> +	lan743x_csr_write(adapter,
> +			  PTP_CLOCK_TARGET_RELOAD_SEC_X(ptp->pps_event_ch),
> +			  1);
> +	lan743x_csr_write(adapter,
> +			  PTP_CLOCK_TARGET_RELOAD_NS_X(ptp->pps_event_ch),
> +			  0);
> +
> +	/* set the new target */
> +	lan743x_csr_write(adapter,
> +			  PTP_CLOCK_TARGET_SEC_X(ptp->pps_event_ch),
> +			  target_seconds);
> +	lan743x_csr_write(adapter,
> +			  PTP_CLOCK_TARGET_NS_X(ptp->pps_event_ch),
> +			  0);
> +	return 0;
> +
> +done:
> +	if (ptp->pps_gpio_bit >= 0) {
> +		lan743x_gpio_release(adapter, ptp->pps_gpio_bit);
> +		ptp->pps_gpio_bit = -1;
> +	}
> +	if (ptp->pps_event_ch >= 0) {
> +		lan743x_ptp_release_event_ch(adapter,
> +					     ptp->pps_event_ch);
> +		ptp->pps_event_ch = -1;
> +	}
> +	return result;
> +}
> +#endif /* CONFIG_PTP_1588_CLOCK */
> +
> +#ifdef CONFIG_PTP_1588_CLOCK
> +static void lan743x_ptp_disable_pps(struct lan743x_adapter *adapter)
> +{
> +	struct lan743x_ptp *ptp = &adapter->ptp;
> +
> +	if (ptp->pps_gpio_bit >= 0) {
> +		lan743x_gpio_release(adapter, ptp->pps_gpio_bit);
> +		ptp->pps_gpio_bit = -1;
> +	}
> +
> +	if (ptp->pps_event_ch >= 0) {
> +		u32 general_config = 0;
> +
> +		/* set target to far in the future, effectively disabling it */
> +		lan743x_csr_write(adapter,
> +				  PTP_CLOCK_TARGET_SEC_X(ptp->pps_event_ch),
> +				  0xFFFF0000);
> +		lan743x_csr_write(adapter,
> +				  PTP_CLOCK_TARGET_NS_X(ptp->pps_event_ch), 0);
> +
> +		general_config = lan743x_csr_read(adapter, PTP_GENERAL_CONFIG);
> +		general_config |= PTP_GENERAL_CONFIG_RELOAD_ADD_X_
> +				  (ptp->pps_event_ch);
> +		lan743x_csr_write(adapter, PTP_GENERAL_CONFIG, general_config);
> +		lan743x_ptp_release_event_ch(adapter, ptp->pps_event_ch);
> +		ptp->pps_event_ch = -1;
> +	}
> +}
> +#endif /* CONFIG_PTP_1588_CLOCK */
> +
> +#ifdef CONFIG_PTP_1588_CLOCK
> +static int lan743x_ptpci_enable(struct ptp_clock_info *ptpci,
> +				struct ptp_clock_request *request, int on)
> +{
> +	struct lan743x_ptp *ptp = container_of(ptpci, struct lan743x_ptp,
> +					       ptp_clock_info);
> +	struct lan743x_adapter *adapter = container_of(ptp,
> +						       struct lan743x_adapter,
> +						       ptp);
> +
> +	if (request) {
> +		switch (request->type) {
> +		case PTP_CLK_REQ_EXTTS:
> +			return -EINVAL;
> +		case PTP_CLK_REQ_PEROUT:
> +			return -EINVAL;
> +		case PTP_CLK_REQ_PPS:
> +			if (on) {
> +				if (lan743x_ptp_enable_pps(adapter) >= 0)
> +					netif_info(adapter, drv,
> +						   adapter->netdev,
> +						   "PPS is ON\n");
> +				else
> +					netif_warn(adapter, drv,
> +						   adapter->netdev,
> +						   "Error starting PPS\n");
> +			} else {
> +				lan743x_ptp_disable_pps(adapter);
> +				netif_info(adapter, drv, adapter->netdev,
> +					   "PPS is OFF\n");
> +			}
> +			break;
> +		default:
> +			netif_err(adapter, drv, adapter->netdev,
> +				  "request->type == %d, Unknown\n",
> +				  request->type);
> +			break;
> +		}
> +	} else {
> +		netif_err(adapter, drv, adapter->netdev, "request == NULL\n");
> +	}
> +	return 0;
> +}
> +#endif /* CONFIG_PTP_1588_CLOCK */
> +
> +void lan743x_ptp_isr(void *context)
> +{
> +	struct lan743x_adapter *adapter = (struct lan743x_adapter *)context;
> +	struct lan743x_ptp *ptp = NULL;
> +	int enable_flag = 1;
> +	u32 ptp_int_sts = 0;
> +
> +	ptp = &adapter->ptp;
> +
> +	lan743x_csr_write(adapter, INT_EN_CLR, INT_BIT_1588_);
> +
> +	ptp_int_sts = lan743x_csr_read(adapter, PTP_INT_STS);
> +	ptp_int_sts &= lan743x_csr_read(adapter, PTP_INT_EN_SET);
> +
> +	if (ptp_int_sts & PTP_INT_BIT_TX_TS_) {
> +		tasklet_schedule(&ptp->ptp_isr_bottom_half);

Please no new tasklets.  Instead use a work queue.  If you need lower
latency, consider using ptp_schedule_worker().

> +		enable_flag = 0;/* tasklet will re-enable later */
> +	}
> +	if (ptp_int_sts & PTP_INT_BIT_TX_SWTS_ERR_) {
> +		netif_err(adapter, drv, adapter->netdev,
> +			  "PTP TX Software Timestamp Error\n");
> +		/* clear int status bit */
> +		lan743x_csr_write(adapter, PTP_INT_STS,
> +				  PTP_INT_BIT_TX_SWTS_ERR_);
> +	}
> +	if (ptp_int_sts & PTP_INT_BIT_TIMER_B_) {
> +		netif_info(adapter, drv, adapter->netdev,
> +			   "PTP TIMER B Interrupt\n");

Don't print like this from an ISR.  Or is this an error, since you
don't enable this bit?

> +		/* clear int status bit */
> +		lan743x_csr_write(adapter, PTP_INT_STS,
> +				  PTP_INT_BIT_TIMER_B_);
> +	}
> +	if (ptp_int_sts & PTP_INT_BIT_TIMER_A_) {
> +		netif_info(adapter, drv, adapter->netdev,
> +			   "PTP TIMER A Interrupt\n");
> +		/* clear int status bit */
> +		lan743x_csr_write(adapter, PTP_INT_STS,
> +				  PTP_INT_BIT_TIMER_A_);
> +	}
> +
> +	if (enable_flag) {
> +		/* re-enable isr */
> +		lan743x_csr_write(adapter, INT_EN_SET, INT_BIT_1588_);
> +	}
> +}
> +
> +static void lan743x_ptp_tx_ts_complete(struct lan743x_adapter *adapter)
> +{
> +	struct lan743x_ptp *ptp = &adapter->ptp;
> +	int i;
> +	int c;

Put same types on one line:
	int c, i;

> +
> +	spin_lock_bh(&ptp->tx_ts_lock);
> +	c = ptp->tx_ts_skb_queue_size;
> +
> +	if (c > ptp->tx_ts_queue_size)
> +		c = ptp->tx_ts_queue_size;
> +	if (c <= 0)
> +		goto done;
> +
> +	for (i = 0; i < c; i++) {
> +		bool ignore_sync = ((ptp->tx_ts_ignore_sync_queue &
> +				    BIT(i)) != 0);
> +		struct sk_buff *skb = ptp->tx_ts_skb_queue[i];
> +		u32 nseconds = ptp->tx_ts_nseconds_queue[i];
> +		u32 seconds = ptp->tx_ts_seconds_queue[i];
> +		u32 header = ptp->tx_ts_header_queue[i];
> +		struct skb_shared_hwtstamps tstamps;

Locals to top of function please.

> +		memset(&tstamps, 0, sizeof(tstamps));
> +		tstamps.hwtstamp = ktime_set(seconds, nseconds);
> +		if (!ignore_sync ||
> +		    ((header & PTP_TX_MSG_HEADER_MSG_TYPE_) !=
> +		    PTP_TX_MSG_HEADER_MSG_TYPE_SYNC_))
> +			skb_tstamp_tx(skb, &tstamps);
> +
> +		dev_kfree_skb(skb);
> +
> +		ptp->tx_ts_skb_queue[i] = NULL;
> +		ptp->tx_ts_seconds_queue[i] = 0;
> +		ptp->tx_ts_nseconds_queue[i] = 0;
> +		ptp->tx_ts_header_queue[i] = 0;
> +	}
> +
> +	/* shift queue */
> +	ptp->tx_ts_ignore_sync_queue >>= c;
> +	for (i = c; i < LAN743X_PTP_NUMBER_OF_TX_TIMESTAMPS; i++) {
> +		ptp->tx_ts_skb_queue[i - c] = ptp->tx_ts_skb_queue[i];
> +		ptp->tx_ts_seconds_queue[i - c] = ptp->tx_ts_seconds_queue[i];
> +		ptp->tx_ts_nseconds_queue[i - c] = ptp->tx_ts_nseconds_queue[i];
> +		ptp->tx_ts_header_queue[i - c] = ptp->tx_ts_header_queue[i];
> +
> +		ptp->tx_ts_skb_queue[i] = NULL;
> +		ptp->tx_ts_seconds_queue[i] = 0;
> +		ptp->tx_ts_nseconds_queue[i] = 0;
> +		ptp->tx_ts_header_queue[i] = 0;
> +	}
> +	ptp->tx_ts_skb_queue_size -= c;
> +	ptp->tx_ts_queue_size -= c;
> +done:
> +	ptp->pending_tx_timestamps -= c;
> +	spin_unlock_bh(&ptp->tx_ts_lock);
> +}
> +
> +static void lan743x_ptp_tx_ts_enqueue_skb(struct lan743x_adapter *adapter,
> +					  struct sk_buff *skb, bool ignore_sync)
> +{
> +	struct lan743x_ptp *ptp = &adapter->ptp;
> +
> +	spin_lock_bh(&ptp->tx_ts_lock);
> +	if (ptp->tx_ts_skb_queue_size < LAN743X_PTP_NUMBER_OF_TX_TIMESTAMPS) {
> +		ptp->tx_ts_skb_queue[ptp->tx_ts_skb_queue_size] = skb;
> +		if (ignore_sync)
> +			ptp->tx_ts_ignore_sync_queue |=
> +				BIT(ptp->tx_ts_skb_queue_size);
> +		ptp->tx_ts_skb_queue_size++;
> +	} else {
> +		/* this should never happen, so long as the tx channel
> +		 * calls and honors the result from
> +		 * lan743x_ptp_request_tx_timestamp
> +		 */
> +		netif_err(adapter, drv, adapter->netdev,
> +			  "tx ts skb queue overflow\n");
> +		dev_kfree_skb(skb);
> +	}
> +	spin_unlock_bh(&ptp->tx_ts_lock);
> +}
> +
> +static void lan743x_ptp_tx_ts_enqueue_ts(struct lan743x_adapter *adapter,
> +					 u32 seconds, u32 nano_seconds,
> +					 u32 header)
> +{
> +	struct lan743x_ptp *ptp = &adapter->ptp;
> +
> +	spin_lock_bh(&ptp->tx_ts_lock);
> +	if (ptp->tx_ts_queue_size < LAN743X_PTP_NUMBER_OF_TX_TIMESTAMPS) {
> +		ptp->tx_ts_seconds_queue[ptp->tx_ts_queue_size] = seconds;
> +		ptp->tx_ts_nseconds_queue[ptp->tx_ts_queue_size] = nano_seconds;
> +		ptp->tx_ts_header_queue[ptp->tx_ts_queue_size] = header;
> +		ptp->tx_ts_queue_size++;
> +	} else {
> +		netif_err(adapter, drv, adapter->netdev,
> +			  "tx ts queue overflow\n");
> +	}
> +	spin_unlock_bh(&ptp->tx_ts_lock);
> +}
> +
> +static void lan743x_ptp_isr_bottom_half(unsigned long param)
> +{
> +	struct lan743x_adapter *adapter = (struct lan743x_adapter *)param;
> +	bool new_timestamp_available = false;
> +
> +	while (lan743x_csr_read(adapter, PTP_INT_STS) & PTP_INT_BIT_TX_TS_) {

As a sanity check, you should break this loop using a counter.

> +		u32 cap_info = lan743x_csr_read(adapter, PTP_CAP_INFO);
> +
> +		if (PTP_CAP_INFO_TX_TS_CNT_GET_(cap_info) > 0) {
> +			u32 seconds = lan743x_csr_read(adapter,
> +				PTP_TX_EGRESS_SEC);
> +			u32 nsec = lan743x_csr_read(adapter, PTP_TX_EGRESS_NS);
> +			u32 cause = (nsec &
> +				     PTP_TX_EGRESS_NS_CAPTURE_CAUSE_MASK_);
> +			u32 header = lan743x_csr_read(adapter,
> +						      PTP_TX_MSG_HEADER);
> +
> +			if (cause == PTP_TX_EGRESS_NS_CAPTURE_CAUSE_SW_) {
> +				nsec &= PTP_TX_EGRESS_NS_TS_NS_MASK_;
> +				lan743x_ptp_tx_ts_enqueue_ts(adapter,
> +							     seconds, nsec,
> +							     header);
> +				new_timestamp_available = true;
> +			} else if (cause ==
> +				PTP_TX_EGRESS_NS_CAPTURE_CAUSE_AUTO_) {
> +				netif_err(adapter, drv, adapter->netdev,
> +					  "Auto capture cause not supported\n");
> +			} else {
> +				netif_warn(adapter, drv, adapter->netdev,
> +					   "unknown tx timestamp capture cause\n");
> +			}
> +		} else {
> +			netif_warn(adapter, drv, adapter->netdev,
> +				   "TX TS INT but no TX TS CNT\n");
> +		}
> +		lan743x_csr_write(adapter, PTP_INT_STS, PTP_INT_BIT_TX_TS_);
> +	}
> +
> +	if (new_timestamp_available)
> +		lan743x_ptp_tx_ts_complete(adapter);
> +
> +	lan743x_csr_write(adapter, INT_EN_SET, INT_BIT_1588_);
> +}
> +
> +static void lan743x_ptp_sync_to_system_clock(struct lan743x_adapter *adapter)
> +{
> +	struct timeval tv;
> +
> +	memset(&tv, 0, sizeof(tv));
> +	do_gettimeofday(&tv);

Use the TAI clock instead.

> +	lan743x_ptp_clock_set(adapter, tv.tv_sec, tv.tv_usec * 1000, 0);
> +}
> +
> +void lan743x_ptp_update_latency(struct lan743x_adapter *adapter,
> +				u32 link_speed)
> +{
> +	switch (link_speed) {
> +	case 10:
> +		lan743x_csr_write(adapter, PTP_LATENCY,
> +				  PTP_LATENCY_TX_SET_(0) |
> +				  PTP_LATENCY_RX_SET_(0));
> +		break;
> +	case 100:
> +		lan743x_csr_write(adapter, PTP_LATENCY,
> +				  PTP_LATENCY_TX_SET_(181) |
> +				  PTP_LATENCY_RX_SET_(594));
> +		break;
> +	case 1000:
> +		lan743x_csr_write(adapter, PTP_LATENCY,
> +				  PTP_LATENCY_TX_SET_(30) |
> +				  PTP_LATENCY_RX_SET_(525));
> +		break;
> +	}
> +}
> +
> +int lan743x_ptp_init(struct lan743x_adapter *adapter)
> +{
> +	struct lan743x_ptp *ptp = &adapter->ptp;
> +
> +	mutex_init(&ptp->command_lock);
> +	spin_lock_init(&ptp->tx_ts_lock);
> +	tasklet_init(&ptp->ptp_isr_bottom_half,
> +		     lan743x_ptp_isr_bottom_half, (unsigned long)adapter);
> +	tasklet_disable(&ptp->ptp_isr_bottom_half);
> +	ptp->used_event_ch = 0;
> +	ptp->pps_event_ch = -1;
> +	ptp->pps_gpio_bit = -1;
> +	return 0;
> +}
> +
> +int lan743x_ptp_open(struct lan743x_adapter *adapter)
> +{
> +	struct lan743x_ptp *ptp = &adapter->ptp;
> +	int ret = -ENODEV;
> +	u32 temp;
> +
> +	lan743x_ptp_reset(adapter);
> +	lan743x_ptp_sync_to_system_clock(adapter);
> +	temp = lan743x_csr_read(adapter, PTP_TX_MOD2);
> +	temp |= PTP_TX_MOD2_TX_PTP_CLR_UDPV4_CHKSUM_;
> +	lan743x_csr_write(adapter, PTP_TX_MOD2, temp);
> +	lan743x_ptp_enable(adapter);
> +	tasklet_enable(&ptp->ptp_isr_bottom_half);
> +	lan743x_csr_write(adapter, INT_EN_SET, INT_BIT_1588_);
> +	lan743x_csr_write(adapter, PTP_INT_EN_SET,
> +			  PTP_INT_BIT_TX_SWTS_ERR_ | PTP_INT_BIT_TX_TS_);
> +	ptp->flags |= PTP_FLAG_ISR_ENABLED;
> +
> +#ifdef CONFIG_PTP_1588_CLOCK
> +	snprintf(ptp->pin_config[0].name, 32, "lan743x_ptp_pin_0");
> +	ptp->pin_config[0].index = 0;
> +	ptp->pin_config[0].func = PTP_PF_PEROUT;
> +	ptp->pin_config[0].chan = 0;
> +
> +	ptp->ptp_clock_info.owner = THIS_MODULE;
> +	snprintf(ptp->ptp_clock_info.name, 16, "%pm",
> +		 adapter->netdev->dev_addr);
> +	ptp->ptp_clock_info.max_adj = LAN743X_PTP_MAX_FREQ_ADJ_IN_PPB;
> +	ptp->ptp_clock_info.n_alarm = 0;
> +	ptp->ptp_clock_info.n_ext_ts = 0;
> +	ptp->ptp_clock_info.n_per_out = 0;
> +	ptp->ptp_clock_info.n_pins = 0;
> +	ptp->ptp_clock_info.pps = 1;
> +	ptp->ptp_clock_info.pin_config = NULL;
> +	ptp->ptp_clock_info.adjfreq = lan743x_ptpci_adjfreq;
> +	ptp->ptp_clock_info.adjtime = lan743x_ptpci_adjtime;
> +	ptp->ptp_clock_info.gettime64 = lan743x_ptpci_gettime64;
> +	ptp->ptp_clock_info.getcrosststamp = NULL;
> +	ptp->ptp_clock_info.settime64 = lan743x_ptpci_settime64;
> +	ptp->ptp_clock_info.enable = lan743x_ptpci_enable;
> +	ptp->ptp_clock_info.verify = NULL;
> +
> +	ptp->ptp_clock = ptp_clock_register(&ptp->ptp_clock_info,
> +					    &adapter->pdev->dev);
> +
> +	if (IS_ERR(ptp->ptp_clock)) {
> +		netif_err(adapter, ifup, adapter->netdev,
> +			  "ptp_clock_register failed\n");
> +		goto done;
> +	}
> +	ptp->flags |= PTP_FLAG_PTP_CLOCK_REGISTERED;
> +	netif_info(adapter, ifup, adapter->netdev,
> +		   "successfully registered ptp clock\n");
> +#endif
> +
> +	return 0;
> +
> +#ifdef CONFIG_PTP_1588_CLOCK
> +done:
> +	lan743x_ptp_close(adapter);
> +	return ret;
> +#endif
> +}
> +
> +void lan743x_ptp_close(struct lan743x_adapter *adapter)
> +{
> +	struct lan743x_ptp *ptp = &adapter->ptp;
> +	int index;
> +
> +#ifdef CONFIG_PTP_1588_CLOCK
> +	if (ptp->flags & PTP_FLAG_PTP_CLOCK_REGISTERED) {
> +		ptp_clock_unregister(ptp->ptp_clock);
> +		ptp->ptp_clock = NULL;
> +		ptp->flags &= ~PTP_FLAG_PTP_CLOCK_REGISTERED;
> +		netif_info(adapter, drv, adapter->netdev,
> +			   "ptp clock unregister\n");
> +	}
> +#endif
> +
> +	if (ptp->flags & PTP_FLAG_ISR_ENABLED) {
> +		lan743x_csr_write(adapter, PTP_INT_EN_CLR,
> +				  PTP_INT_BIT_TX_SWTS_ERR_ |
> +				  PTP_INT_BIT_TX_TS_);
> +		lan743x_csr_write(adapter, INT_EN_CLR, INT_BIT_1588_);
> +		tasklet_disable(&ptp->ptp_isr_bottom_half);
> +		ptp->flags &= ~PTP_FLAG_ISR_ENABLED;
> +	}
> +
> +	/* clean up pending timestamp requests */
> +	lan743x_ptp_tx_ts_complete(adapter);
> +	spin_lock_bh(&ptp->tx_ts_lock);
> +	for (index = 0;
> +		index < LAN743X_PTP_NUMBER_OF_TX_TIMESTAMPS;
> +		index++) {
> +		struct sk_buff *skb = ptp->tx_ts_skb_queue[index];
> +
> +		if (skb)
> +			dev_kfree_skb(skb);
> +		ptp->tx_ts_skb_queue[index] = NULL;
> +		ptp->tx_ts_seconds_queue[index] = 0;
> +		ptp->tx_ts_nseconds_queue[index] = 0;
> +	}
> +	ptp->tx_ts_skb_queue_size = 0;
> +	ptp->tx_ts_queue_size = 0;
> +	ptp->pending_tx_timestamps = 0;
> +	spin_unlock_bh(&ptp->tx_ts_lock);
> +
> +	lan743x_ptp_disable(adapter);
> +}
> +
> +void lan743x_ptp_set_sync_ts_insert(struct lan743x_adapter *adapter,
> +				    bool ts_insert_enable)
> +{
> +	u32 ptp_tx_mod = lan743x_csr_read(adapter, PTP_TX_MOD);
> +
> +	if (ts_insert_enable)
> +		ptp_tx_mod |= PTP_TX_MOD_TX_PTP_SYNC_TS_INSERT_;
> +	else
> +		ptp_tx_mod &= ~PTP_TX_MOD_TX_PTP_SYNC_TS_INSERT_;
> +
> +	lan743x_csr_write(adapter, PTP_TX_MOD, ptp_tx_mod);
> +}
> +
> +static bool lan743x_ptp_is_enabled(struct lan743x_adapter *adapter)
> +{
> +	if (lan743x_csr_read(adapter, PTP_CMD_CTL) & PTP_CMD_CTL_PTP_ENABLE_)
> +		return true;
> +	return false;
> +}
> +
> +static void lan743x_ptp_wait_till_cmd_done(struct lan743x_adapter *adapter,
> +					   u32 bit_mask)
> +{
> +	int timeout = 1000;
> +	u32 data = 0;
> +
> +	while (timeout &&
> +	       (data = (lan743x_csr_read(adapter, PTP_CMD_CTL) &
> +	       bit_mask))) {
> +		usleep_range(1000, 20000);
> +		timeout--;
> +	}
> +	if (data) {
> +		netif_err(adapter, drv, adapter->netdev,
> +			  "timeout waiting for cmd to be done, cmd = 0x%08X\n",
> +			  bit_mask);
> +	}
> +}
> +
> +static void lan743x_ptp_enable(struct lan743x_adapter *adapter)
> +{
> +	struct lan743x_ptp *ptp = &adapter->ptp;
> +
> +	mutex_lock(&ptp->command_lock);
> +
> +	if (lan743x_ptp_is_enabled(adapter)) {
> +		netif_warn(adapter, drv, adapter->netdev,
> +			   "PTP already enabled\n");
> +		goto done;
> +	}
> +	lan743x_csr_write(adapter, PTP_CMD_CTL, PTP_CMD_CTL_PTP_ENABLE_);
> +done:
> +	mutex_unlock(&ptp->command_lock);
> +}
> +
> +static void lan743x_ptp_disable(struct lan743x_adapter *adapter)
> +{
> +	struct lan743x_ptp *ptp = &adapter->ptp;
> +
> +	mutex_lock(&ptp->command_lock);
> +	if (!lan743x_ptp_is_enabled(adapter)) {
> +		netif_warn(adapter, drv, adapter->netdev,
> +			   "PTP already disabled\n");
> +		goto done;
> +	}
> +	lan743x_csr_write(adapter, PTP_CMD_CTL, PTP_CMD_CTL_PTP_DISABLE_);
> +	lan743x_ptp_wait_till_cmd_done(adapter, PTP_CMD_CTL_PTP_ENABLE_);
> +done:
> +	mutex_unlock(&ptp->command_lock);
> +}
> +
> +static void lan743x_ptp_reset(struct lan743x_adapter *adapter)
> +{
> +	struct lan743x_ptp *ptp = &adapter->ptp;
> +
> +	mutex_lock(&ptp->command_lock);
> +
> +	if (lan743x_ptp_is_enabled(adapter)) {
> +		netif_err(adapter, drv, adapter->netdev,
> +			  "Attempting reset while enabled\n");
> +		goto done;
> +	}
> +
> +	lan743x_csr_write(adapter, PTP_CMD_CTL, PTP_CMD_CTL_PTP_RESET_);
> +	lan743x_ptp_wait_till_cmd_done(adapter, PTP_CMD_CTL_PTP_RESET_);
> +done:
> +	mutex_unlock(&ptp->command_lock);
> +}
> +
> +#ifdef CONFIG_PTP_1588_CLOCK
> +static int lan743x_ptp_reserve_event_ch(struct lan743x_adapter *adapter)
> +{
> +	struct lan743x_ptp *ptp = &adapter->ptp;
> +	int result = -ENODEV;
> +	int index = 0;
> +
> +	mutex_lock(&ptp->command_lock);
> +	for (index = 0; index < LAN743X_PTP_NUMBER_OF_EVENT_CHANNELS; index++) {
> +		if (!(test_bit(index, &ptp->used_event_ch))) {
> +			ptp->used_event_ch |= BIT(index);
> +			result = index;
> +			break;
> +		}
> +	}
> +	mutex_unlock(&ptp->command_lock);
> +	return result;
> +}
> +#endif /* CONFIG_PTP_1588_CLOCK */
> +
> +#ifdef CONFIG_PTP_1588_CLOCK
> +static void lan743x_ptp_release_event_ch(struct lan743x_adapter *adapter,
> +					 int event_channel)
> +{
> +	struct lan743x_ptp *ptp = &adapter->ptp;
> +
> +	mutex_lock(&ptp->command_lock);
> +	if (test_bit(event_channel, &ptp->used_event_ch)) {
> +		ptp->used_event_ch &= ~BIT(event_channel);
> +	} else {
> +		netif_warn(adapter, drv, adapter->netdev,
> +			   "attempted release on a not used event_channel = %d\n",
> +			   event_channel);
> +	}
> +	mutex_unlock(&ptp->command_lock);
> +}
> +#endif /* CONFIG_PTP_1588_CLOCK */
> +
> +#ifdef CONFIG_PTP_1588_CLOCK
> +static void lan743x_ptp_clock_get(struct lan743x_adapter *adapter,
> +				  u32 *seconds, u32 *nano_seconds,
> +				  u32 *sub_nano_seconds)
> +{
> +	struct lan743x_ptp *ptp = &adapter->ptp;
> +
> +	mutex_lock(&ptp->command_lock);
> +
> +	lan743x_csr_write(adapter, PTP_CMD_CTL, PTP_CMD_CTL_PTP_CLOCK_READ_);
> +	lan743x_ptp_wait_till_cmd_done(adapter, PTP_CMD_CTL_PTP_CLOCK_READ_);
> +
> +	if (seconds)
> +		(*seconds) = lan743x_csr_read(adapter, PTP_CLOCK_SEC);
> +
> +	if (nano_seconds)
> +		(*nano_seconds) = lan743x_csr_read(adapter, PTP_CLOCK_NS);
> +
> +	if (sub_nano_seconds)
> +		(*sub_nano_seconds) =
> +		lan743x_csr_read(adapter, PTP_CLOCK_SUBNS);
> +
> +	mutex_unlock(&ptp->command_lock);
> +}
> +#endif /* CONFIG_PTP_1588_CLOCK */
> +
> +static void lan743x_ptp_clock_set(struct lan743x_adapter *adapter,
> +				  u32 seconds, u32 nano_seconds,
> +				  u32 sub_nano_seconds)
> +{
> +	struct lan743x_ptp *ptp = &adapter->ptp;
> +
> +	mutex_lock(&ptp->command_lock);
> +
> +	lan743x_csr_write(adapter, PTP_CLOCK_SEC, seconds);
> +	lan743x_csr_write(adapter, PTP_CLOCK_NS, nano_seconds);
> +	lan743x_csr_write(adapter, PTP_CLOCK_SUBNS, sub_nano_seconds);
> +
> +	lan743x_csr_write(adapter, PTP_CMD_CTL, PTP_CMD_CTL_PTP_CLOCK_LOAD_);
> +	lan743x_ptp_wait_till_cmd_done(adapter, PTP_CMD_CTL_PTP_CLOCK_LOAD_);
> +	mutex_unlock(&ptp->command_lock);
> +}
> +
> +#ifdef CONFIG_PTP_1588_CLOCK
> +static void lan743x_ptp_clock_step(struct lan743x_adapter *adapter,
> +				   s64 time_step_ns)
> +{
> +	struct lan743x_ptp *ptp = &adapter->ptp;
> +	u64 abs_time_step_ns = 0;
> +	u32 nano_seconds = 0;
> +	s32 seconds = 0;
> +
> +	if (time_step_ns >  15000000000LL) {
> +		/* convert to clock set */
> +		u32 nano_seconds = 0;
> +		u32 seconds = 0;
> +
> +		lan743x_ptp_clock_get(adapter, &seconds, &nano_seconds, NULL);
> +		seconds += (time_step_ns / 1000000000LL);

Use the macro.

> +		nano_seconds += (time_step_ns % 1000000000LL);

Actually, use div_u64_rem() to avoid the % operator.

> +		if (nano_seconds >= 1000000000) {

How can this test be true?

> +			seconds++;
> +			nano_seconds -= 1000000000;
> +		}
> +		lan743x_ptp_clock_set(adapter, seconds, nano_seconds, 0);
> +		return;
> +	} else if (time_step_ns < -15000000000LL) {
> +		/* convert to clock set */
> +		u32 nano_seconds_step = 0;
> +		u32 nano_seconds = 0;
> +		u32 seconds = 0;

Ugh.  Now you have these defined twice.  Move them to the top, please.

> +		time_step_ns = -time_step_ns;
> +
> +		lan743x_ptp_clock_get(adapter, &seconds, &nano_seconds, NULL);
> +		seconds -= (time_step_ns / 1000000000LL);
> +		nano_seconds_step = (time_step_ns % 1000000000LL);

Use division macro for 64 bit.

> +		if (nano_seconds < nano_seconds_step) {
> +			seconds--;
> +			nano_seconds += 1000000000;
> +		}
> +		nano_seconds -= nano_seconds_step;
> +		lan743x_ptp_clock_set(adapter, seconds, nano_seconds, 0);
> +		return;
> +	}
> +
> +	/* do clock step */
> +
> +	if (time_step_ns >= 0) {
> +		abs_time_step_ns = (u64)(time_step_ns);
> +		seconds = (s32)(abs_time_step_ns / 1000000000);
> +		nano_seconds = (u32)(abs_time_step_ns % 1000000000);
> +	} else {
> +		abs_time_step_ns = (u64)(-time_step_ns);
> +		seconds = -((s32)(abs_time_step_ns / 1000000000));
> +		nano_seconds = (u32)(abs_time_step_ns % 1000000000);
> +		if (nano_seconds > 0) {
> +			/* subtracting nano seconds is not allowed
> +			 * convert to subtracting from seconds,
> +			 * and adding to nanoseconds
> +			 */
> +			seconds--;
> +			nano_seconds = (1000000000 - nano_seconds);
> +		}
> +	}
> +
> +	if (nano_seconds > 0) {
> +		/* add 8 ns to cover the likely normal increment */
> +		nano_seconds += 8;
> +	}
> +
> +	if (nano_seconds >= 1000000000) {
> +		/* carry into seconds */
> +		seconds++;
> +		nano_seconds -= 1000000000;
> +	}
> +
> +	while (seconds) {
> +		mutex_lock(&ptp->command_lock);
> +		if (seconds > 0) {
> +			u32 adjustment_value = (u32)seconds;
> +
> +			if (adjustment_value > 0xF)
> +				adjustment_value = 0xF;
> +			lan743x_csr_write(adapter, PTP_CLOCK_STEP_ADJ,
> +					  PTP_CLOCK_STEP_ADJ_DIR_ |
> +					  adjustment_value);
> +			seconds -= ((s32)adjustment_value);
> +		} else {
> +			u32 adjustment_value = (u32)(-seconds);
> +
> +			if (adjustment_value > 0xF)
> +				adjustment_value = 0xF;
> +			lan743x_csr_write(adapter, PTP_CLOCK_STEP_ADJ,
> +					  adjustment_value);
> +			seconds += ((s32)adjustment_value);
> +		}
> +		lan743x_csr_write(adapter, PTP_CMD_CTL,
> +				  PTP_CMD_CTL_PTP_CLOCK_STEP_SEC_);
> +		lan743x_ptp_wait_till_cmd_done(adapter,
> +					       PTP_CMD_CTL_PTP_CLOCK_STEP_SEC_);
> +		mutex_unlock(&ptp->command_lock);
> +	}
> +	if (nano_seconds) {
> +		mutex_lock(&ptp->command_lock);
> +		lan743x_csr_write(adapter, PTP_CLOCK_STEP_ADJ,
> +				  PTP_CLOCK_STEP_ADJ_DIR_ |
> +				  (nano_seconds &
> +				  PTP_CLOCK_STEP_ADJ_VALUE_MASK_));
> +		lan743x_csr_write(adapter, PTP_CMD_CTL,
> +				  PTP_CMD_CTL_PTP_CLK_STP_NSEC_);
> +		lan743x_ptp_wait_till_cmd_done(adapter,
> +					       PTP_CMD_CTL_PTP_CLK_STP_NSEC_);
> +		mutex_unlock(&ptp->command_lock);
> +	}
> +}
> +#endif /* CONFIG_PTP_1588_CLOCK */
> +
> +bool lan743x_ptp_request_tx_timestamp(struct lan743x_adapter *adapter)
> +{
> +	struct lan743x_ptp *ptp = &adapter->ptp;
> +	bool result = false;
> +
> +	spin_lock_bh(&ptp->tx_ts_lock);
> +	if (ptp->pending_tx_timestamps < LAN743X_PTP_NUMBER_OF_TX_TIMESTAMPS) {
> +		ptp->pending_tx_timestamps++;
> +		result = true;/* request granted */

Avoid tail comments please.

> +	}
> +	spin_unlock_bh(&ptp->tx_ts_lock);
> +	return result;
> +}

Thanks,
Richard

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ