Skip to contentSkip to conversation

pch_gbe: ptp bug fixes

PATCH In review Richard Cochran 2 participants 13 messages Apr 20, 2012 – Apr 21, 2012

Subsystems (3)
lore
Share & tools
lore

Richard Cochran

author maintainer
#0 lore raw

cover pch_gbe: ptp bug fixes

The content in this patch series originally was posted to netdev by Takahiro on March 23, 2012, as a loose group of five patches. These patches addressed problems pointed out in review, both on and off list.

I have taken his work and rebased, squashed, and split it into the present form, and I also wrote commit messages that more fully explain the changes. Also, I added the last two patches myself.

Richard Cochran (2):
pch_gbe: run the ptp bpf just once per packet
pch_gbe: remove suspicious comment

Takahiro Shimizu (7):
pch_gbe: scale time stamps to nanoseconds
pch_gbe: simplify transmit time stamping flag test
pch_gbe: reprogram multicast address register on reset
pch_gbe: export a method to set the receive match address
pch_gbe: improve coding style
pch_gbe: do not set the channel control register
pch_gbe: correct receive time stamp filtering

 drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h    |    1 +
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   |  107 +++++++++++++++-----
 drivers/ptp/Kconfig                                |   10 ++-
 drivers/ptp/ptp_pch.c                              |    7 +-
 4 files changed, 95 insertions(+), 30 deletions(-)
Signature · 1 line
1.7.2.5
author maintainer
#1 lore raw

PATCH 1/9 pch_gbe: scale time stamps to nanoseconds

From: Takahiro Shimizu <[email protected]>

This patch fixes the helper functions that give the transmit and receive time stamps to return nanoseconds, instead of arbitrary clock ticks.

[ RC - Rebased Takahiro's changes and wrote a commit message
explaining the changes. ]

---
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   |    4 ----
 drivers/ptp/ptp_pch.c                              |    2 ++
 2 files changed, 2 insertions(+), 4 deletions(-)
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c +0 -4
index 8035e5f..7c2dabb 100644
@@ -101,8 +101,6 @@ const char pch_driver_version[] = DRV_VERSION;
 
 #ifdef CONFIG_PCH_PTP
 /* Macros for ieee1588 */
-#define TICKS_NS_SHIFT  5
-
 /* 0x40 Time Synchronization Channel Control Register Bits */
 #define MASTER_MODE   (1<<0)
 #define SLAVE_MODE    (0<<0)
@@ -183,7 +181,6 @@ static void pch_rx_timestamp(
 		goto out;
 
 	ns = pch_rx_snap_read(pdev);
-	ns <<= TICKS_NS_SHIFT;
 
 	shhwtstamps = skb_hwtstamps(skb);
 	memset(shhwtstamps, 0, sizeof(*shhwtstamps));
@@ -226,7 +223,6 @@ static void pch_tx_timestamp(
 	}
 
 	ns = pch_tx_snap_read(pdev);
-	ns <<= TICKS_NS_SHIFT;
 
 	memset(&shhwtstamps, 0, sizeof(shhwtstamps));
 	shhwtstamps.hwtstamp = ns_to_ktime(ns);
drivers/ptp/ptp_pch.c +2 -0
index 375eb04..847a3c3 100644
@@ -261,6 +261,7 @@ u64 pch_rx_snap_read(struct pci_dev *pdev)
 
 	ns = ((u64) hi) << 32;
 	ns |= lo;
+	ns <<= TICKS_NS_SHIFT;
 
 	return ns;
 }
@@ -277,6 +278,7 @@ u64 pch_tx_snap_read(struct pci_dev *pdev)
 
 	ns = ((u64) hi) << 32;
 	ns |= lo;
+	ns <<= TICKS_NS_SHIFT;
 
 	return ns;
 }
Signature · 1 line
1.7.2.5
author maintainer
#2 lore raw

PATCH 2/9 pch_gbe: simplify transmit time stamping flag test

In reply to Richard Cochran

From: Takahiro Shimizu <[email protected]>

This patch makes logic surrounding the test of the transmit time stamping flag more readable.

[ RC - Rebased Takahiro's changes and wrote a commit message
explaining the changes. ]

---
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c +3 -3
index 7c2dabb..6a9a63b 100644
@@ -199,11 +199,11 @@ static void pch_tx_timestamp(
 	u32 cnt, val;
 
 	shtx = skb_shinfo(skb);
-	if (unlikely(shtx->tx_flags & SKBTX_HW_TSTAMP && adapter->hwts_tx_en))
-		shtx->tx_flags |= SKBTX_IN_PROGRESS;
-	else
+	if (likely(!(shtx->tx_flags & SKBTX_HW_TSTAMP && adapter->hwts_tx_en)))
 		return;
 
+	shtx->tx_flags |= SKBTX_IN_PROGRESS;
+
 	/* Get ieee1588's dev information */
 	pdev = adapter->ptp_pdev;
 
Signature · 1 line
1.7.2.5
author maintainer
#3 lore raw

PATCH 3/9 pch_gbe: reprogram multicast address register on reset

In reply to Richard Cochran

From: Takahiro Shimizu <[email protected]>

The reset logic after a Rx FIFO overrun will clear the programmed multicast addresses. This patch fixes the issue by reprogramming the registers after the reset.

[ RC - Rebased Takahiro's changes and wrote a commit message
explaining the changes. ]

---
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   |   60 +++++++++++++++++++-
 1 files changed, 57 insertions(+), 3 deletions(-)
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c +57 -3
index 6a9a63b..dc15e93 100644
@@ -383,31 +383,85 @@ static void pch_gbe_mac_mar_set(struct pch_gbe_hw *hw, u8 * addr, u32 index)
 }
 
 /**
+ * pch_gbe_mac_save_mac_addr_regs - Save MAC addresse registers
+ * @hw:		Pointer to the HW structure
+ * @addr:	Pointer to the MAC address
+ * @index:  MAC address array register
+ */
+static void
+pch_gbe_mac_save_mac_addr_regs(struct pch_gbe_hw *hw,
+			struct pch_gbe_regs_mac_adr *mac_adr, u32 index)
+{
+	mac_adr->high = ioread32(&hw->reg->mac_adr[index].high);
+	mac_adr->low = ioread32(&hw->reg->mac_adr[index].low);
+}
+
+/**
+ * pch_gbe_mac_store_mac_addr_regs - Store MAC addresse registers
+ * @hw:		Pointer to the HW structure
+ * @addr:	Pointer to the MAC address
+ * @index:  MAC address array register
+ */
+static void
+pch_gbe_mac_store_mac_addr_regs(struct pch_gbe_hw *hw,
+			struct pch_gbe_regs_mac_adr *mac_adr, u32 index)
+{
+	u32 adrmask;
+
+	adrmask = ioread32(&hw->reg->ADDR_MASK);
+	iowrite32((adrmask | (0x0001 << index)), &hw->reg->ADDR_MASK);
+	/* wait busy */
+	pch_gbe_wait_clr_bit(&hw->reg->ADDR_MASK, PCH_GBE_BUSY);
+	/* Set the MAC address to the MAC address xA/xB register */
+	iowrite32(mac_adr->high, &hw->reg->mac_adr[index].high);
+	iowrite32(mac_adr->low, &hw->reg->mac_adr[index].low);
+	iowrite32((adrmask & ~(0x0001 << index)), &hw->reg->ADDR_MASK);
+	/* wait busy */
+	pch_gbe_wait_clr_bit(&hw->reg->ADDR_MASK, PCH_GBE_BUSY);
+}
+
+#define MAC_ADDR_LIST_NUM 16
+/**
  * pch_gbe_mac_reset_hw - Reset hardware
  * @hw:	Pointer to the HW structure
  */
 static void pch_gbe_mac_reset_hw(struct pch_gbe_hw *hw)
 {
+	struct pch_gbe_regs_mac_adr mac_addr_list[MAC_ADDR_LIST_NUM];
+	int i;
+
 	/* Read the MAC address. and store to the private data */
 	pch_gbe_mac_read_mac_addr(hw);
+	/* Read other MAC addresses */
+	for (i = 1; i < MAC_ADDR_LIST_NUM; i++)
+		pch_gbe_mac_save_mac_addr_regs(hw, &mac_addr_list[i], i);
 	iowrite32(PCH_GBE_ALL_RST, &hw->reg->RESET);
 #ifdef PCH_GBE_MAC_IFOP_RGMII
 	iowrite32(PCH_GBE_MODE_GMII_ETHER, &hw->reg->MODE);
 #endif
 	pch_gbe_wait_clr_bit(&hw->reg->RESET, PCH_GBE_ALL_RST);
-	/* Setup the receive address */
+	/* Setup the receive addresses */
 	pch_gbe_mac_mar_set(hw, hw->mac.addr, 0);
+	for (i = 1; i < MAC_ADDR_LIST_NUM; i++)
+		pch_gbe_mac_store_mac_addr_regs(hw, &mac_addr_list[i], i);
 	return;
 }
 
 static void pch_gbe_mac_reset_rx(struct pch_gbe_hw *hw)
 {
-	/* Read the MAC address. and store to the private data */
+	struct pch_gbe_regs_mac_adr mac_addr_list[MAC_ADDR_LIST_NUM];
+	int i;
+
+	/* Read the MAC addresses. and store to the private data */
 	pch_gbe_mac_read_mac_addr(hw);
+	for (i = 1; i < MAC_ADDR_LIST_NUM; i++)
+		pch_gbe_mac_save_mac_addr_regs(hw, &mac_addr_list[i], i);
 	iowrite32(PCH_GBE_RX_RST, &hw->reg->RESET);
 	pch_gbe_wait_clr_bit_irq(&hw->reg->RESET, PCH_GBE_RX_RST);
-	/* Setup the MAC address */
+	/* Setup the MAC addresses */
 	pch_gbe_mac_mar_set(hw, hw->mac.addr, 0);
+	for (i = 1; i < MAC_ADDR_LIST_NUM; i++)
+		pch_gbe_mac_store_mac_addr_regs(hw, &mac_addr_list[i], i);
 	return;
 }
 
Signature · 1 line
1.7.2.5
author maintainer
#4 lore raw

PATCH 4/9 pch_gbe: export a method to set the receive match address

In reply to Richard Cochran

From: Takahiro Shimizu <[email protected]>

The code in phc_gbe_main will need to call this method in order to set the station address register according to the receive time stamping filter.

[ RC - Rebased Takahiro's changes and wrote a commit message
explaining the changes. ]

---
 drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h |    1 +
 drivers/ptp/ptp_pch.c                           |    3 ++-
 2 files changed, 3 insertions(+), 1 deletions(-)
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe.h +1 -0
index dd14915..9f3dbc4 100644
@@ -660,6 +660,7 @@ extern u32 pch_src_uuid_lo_read(struct pci_dev *pdev);
 extern u32 pch_src_uuid_hi_read(struct pci_dev *pdev);
 extern u64 pch_rx_snap_read(struct pci_dev *pdev);
 extern u64 pch_tx_snap_read(struct pci_dev *pdev);
+extern int pch_set_station_address(u8 *addr, struct pci_dev *pdev);
 #endif
 
 /* pch_gbe_param.c */
drivers/ptp/ptp_pch.c +2 -1
index 847a3c3..9e397fe 100644
@@ -308,7 +308,7 @@ static void pch_reset(struct pch_dev *chip)
  *				    traffic on the  ethernet interface
  * @addr:	dress which contain the column separated address to be used.
  */
-static int pch_set_station_address(u8 *addr, struct pci_dev *pdev)
+int pch_set_station_address(u8 *addr, struct pci_dev *pdev)
 {
 	s32 i;
 	struct pch_dev *chip = pci_get_drvdata(pdev);
@@ -352,6 +352,7 @@ static int pch_set_station_address(u8 *addr, struct pci_dev *pdev)
 	}
 	return 0;
 }
+EXPORT_SYMBOL(pch_set_station_address);
 
 /*
  * Interrupt service routine
Signature · 1 line
1.7.2.5
author maintainer
#5 lore raw

PATCH 5/9 pch_gbe: improve coding style

In reply to Richard Cochran

From: Takahiro Shimizu <[email protected]>

This patch clears up a few coding style issues:

- Makes two function definitions a bit nicer looking.
- Remove unneeded parentheses.
- Simplify macros for register bits.

[ RC - Rebased Takahiro's changes and wrote a commit message
explaining the changes. ]

---
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c +9 -9
index dc15e93..799a85a 100644
@@ -103,9 +103,9 @@ const char pch_driver_version[] = DRV_VERSION;
 /* Macros for ieee1588 */
 /* 0x40 Time Synchronization Channel Control Register Bits */
 #define MASTER_MODE   (1<<0)
-#define SLAVE_MODE    (0<<0)
+#define SLAVE_MODE    (0)
 #define V2_MODE       (1<<31)
-#define CAP_MODE0     (0<<16)
+#define CAP_MODE0     (0)
 #define CAP_MODE2     (1<<17)
 
 /* 0x44 Time Synchronization Channel Event Register Bits */
@@ -151,8 +151,8 @@ static int pch_ptp_match(struct sk_buff *skb, u16 uid_hi, u32 uid_lo, u16 seqid)
 		seqid  == *id);
 }
 
-static void pch_rx_timestamp(
-			struct pch_gbe_adapter *adapter, struct sk_buff *skb)
+static void
+pch_rx_timestamp(struct pch_gbe_adapter *adapter, struct sk_buff *skb)
 {
 	struct skb_shared_hwtstamps *shhwtstamps;
 	struct pci_dev *pdev;
@@ -189,8 +189,8 @@ out:
 	pch_ch_event_write(pdev, RX_SNAPSHOT_LOCKED);
 }
 
-static void pch_tx_timestamp(
-			struct pch_gbe_adapter *adapter, struct sk_buff *skb)
+static void
+pch_tx_timestamp(struct pch_gbe_adapter *adapter, struct sk_buff *skb)
 {
 	struct skb_shared_hwtstamps shhwtstamps;
 	struct pci_dev *pdev;
@@ -263,15 +263,15 @@ static int hwtstamp_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
 		break;
 	case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
 		adapter->hwts_rx_en = 0;
-		pch_ch_control_write(pdev, (SLAVE_MODE | CAP_MODE0));
+		pch_ch_control_write(pdev, SLAVE_MODE | CAP_MODE0);
 		break;
 	case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
 		adapter->hwts_rx_en = 1;
-		pch_ch_control_write(pdev, (MASTER_MODE | CAP_MODE0));
+		pch_ch_control_write(pdev, MASTER_MODE | CAP_MODE0);
 		break;
 	case HWTSTAMP_FILTER_PTP_V2_EVENT:
 		adapter->hwts_rx_en = 1;
-		pch_ch_control_write(pdev, (V2_MODE | CAP_MODE2));
+		pch_ch_control_write(pdev, V2_MODE | CAP_MODE2);
 		break;
 	default:
 		return -ERANGE;
Signature · 1 line
1.7.2.5
author maintainer
#6 lore raw

PATCH 6/9 pch_gbe: do not set the channel control register

In reply to Richard Cochran

From: Takahiro Shimizu <[email protected]>

We will let the pch_gbe code do that according to the receive time stamp filter.

[ RC - Rebased Takahiro's changes and wrote a commit message
explaining the changes. ]

---
 drivers/ptp/ptp_pch.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)
drivers/ptp/ptp_pch.c +0 -2
index 9e397fe..08c3311 100644
@@ -652,8 +652,6 @@ pch_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	iowrite32(1, &chip->regs->trgt_lo);
 	iowrite32(0, &chip->regs->trgt_hi);
 	iowrite32(PCH_TSE_TTIPEND, &chip->regs->event);
-	/* Version: IEEE1588 v1 and IEEE1588-2008,  Mode: All Evwnt, Locked  */
-	iowrite32(0x80020000, &chip->regs->ch_control);
 
 	pch_eth_enable_set(chip);
 
Signature · 1 line
1.7.2.5
author maintainer
#7 lore raw

PATCH 7/9 pch_gbe: correct receive time stamp filtering

In reply to Richard Cochran

From: Takahiro Shimizu <[email protected]>

This patch fixes the driver so that multicast PTP event messages can be recognized by the hardware time stamping unit. The station address register must be set according to the desired transport type.

[ RC - Rebased Takahiro's changes and wrote a commit message
explaining the changes. ]

---
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   |   14 +++++++++++++-
 drivers/ptp/Kconfig                                |   10 +++++++---
 2 files changed, 20 insertions(+), 4 deletions(-)
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c +13 -1
index 799a85a..53ac2fb 100644
@@ -111,6 +111,9 @@ const char pch_driver_version[] = DRV_VERSION;
 /* 0x44 Time Synchronization Channel Event Register Bits */
 #define TX_SNAPSHOT_LOCKED (1<<0)
 #define RX_SNAPSHOT_LOCKED (1<<1)
+
+#define PTP_L4_MULTICAST_SA "01:00:5e:00:01:81"
+#define PTP_L2_MULTICAST_SA "01:1b:19:00:00:00"
 #endif
 
 static unsigned int copybreak __read_mostly = PCH_GBE_COPYBREAK_DEFAULT;
@@ -236,6 +239,7 @@ static int hwtstamp_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
 	struct hwtstamp_config cfg;
 	struct pch_gbe_adapter *adapter = netdev_priv(netdev);
 	struct pci_dev *pdev;
+	u8 station[20];
 
 	if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg)))
 		return -EFAULT;
@@ -269,9 +273,17 @@ static int hwtstamp_ioctl(struct net_device *netdev, struct ifreq *ifr, int cmd)
 		adapter->hwts_rx_en = 1;
 		pch_ch_control_write(pdev, MASTER_MODE | CAP_MODE0);
 		break;
-	case HWTSTAMP_FILTER_PTP_V2_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
+		adapter->hwts_rx_en = 1;
+		pch_ch_control_write(pdev, V2_MODE | CAP_MODE2);
+		strcpy(station, PTP_L4_MULTICAST_SA);
+		pch_set_station_address(station, pdev);
+		break;
+	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
 		adapter->hwts_rx_en = 1;
 		pch_ch_control_write(pdev, V2_MODE | CAP_MODE2);
+		strcpy(station, PTP_L2_MULTICAST_SA);
+		pch_set_station_address(station, pdev);
 		break;
 	default:
 		return -ERANGE;
drivers/ptp/Kconfig +7 -3
index cd9bc3b..5648dad 100644
@@ -78,9 +78,13 @@ config PTP_1588_CLOCK_PCH
 	depends on PCH_GBE
 	help
 	  This driver adds support for using the PCH EG20T as a PTP
-	  clock. This clock is only useful if your PTP programs are
-	  getting hardware time stamps on the PTP Ethernet packets
-	  using the SO_TIMESTAMPING API.
+	  clock. The hardware supports time stamping of PTP packets
+	  when using the end-to-end delay (E2E) mechansim. The peer
+	  delay mechansim (P2P) is not supported.
+
+	  This clock is only useful if your PTP programs are getting
+	  hardware time stamps on the PTP Ethernet packets using the
+	  SO_TIMESTAMPING API.
 
 	  To compile this driver as a module, choose M here: the module
 	  will be called ptp_pch.
Signature · 1 line
1.7.2.5
author maintainer
#8 lore raw

PATCH 8/9 pch_gbe: run the ptp bpf just once per packet

In reply to Richard Cochran

This patch fixes code which needlessly ran the BPF twice per packet. Instead, we just run the classifier once and test whether the packet is any kind of PTP event message.

---
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c +1 -3
index 53ac2fb..e9b785e 100644
@@ -134,10 +134,8 @@ static int pch_ptp_match(struct sk_buff *skb, u16 uid_hi, u32 uid_lo, u16 seqid)
 	u16 *hi, *id;
 	u32 lo;
 
-	if ((sk_run_filter(skb, ptp_filter) != PTP_CLASS_V2_IPV4) &&
-		(sk_run_filter(skb, ptp_filter) != PTP_CLASS_V1_IPV4)) {
+	if (sk_run_filter(skb, ptp_filter) == PTP_CLASS_NONE)
 		return 0;
-	}
 
 	offset = ETH_HLEN + IPV4_HLEN(data) + UDP_HLEN;
 
Signature · 1 line
1.7.2.5
author maintainer
#9 lore raw

PATCH 9/9 pch_gbe: remove suspicious comment

In reply to Richard Cochran

The time stamping code in this driver appears to have been copied from the ixp4xx_eth.c driver, including this timing comment. I had actually measured the time stamp delay on an IXP425, but I really doubt that this value also applies here.

---
 .../net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c   |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)
drivers/net/ethernet/oki-semi/pch_gbe/pch_gbe_main.c +0 -1
index e9b785e..89c6bcf 100644
@@ -210,7 +210,6 @@ pch_tx_timestamp(struct pch_gbe_adapter *adapter, struct sk_buff *skb)
 
 	/*
 	 * This really stinks, but we have to poll for the Tx time stamp.
-	 * Usually, the time stamp is ready after 4 to 6 microseconds.
 	 */
 	for (cnt = 0; cnt < 100; cnt++) {
 		val = pch_ch_event_read(pdev);
Signature · 1 line
1.7.2.5
maintainer
#10 lore raw
In reply to Richard Cochran

From: Richard Cochran <[email protected]>
Date: Fri, 20 Apr 2012 22:09:19 +0200

Quoted 8 lines

The content in this patch series originally was posted to netdev by Takahiro on March 23, 2012, as a loose group of five patches. These patches addressed problems pointed out in review, both on and off list.

I have taken his work and rebased, squashed, and split it into the present form, and I also wrote commit messages that more fully explain the changes. Also, I added the last two patches myself.

3.4-rcX is not the appropriate tree to base this work on, whereas net-next is.

Thanks.

author maintainer
#11 lore raw
Quoted 3 lines · David Miller

3.4-rcX is not the appropriate tree to base this work on, whereas net-next is.

Okay, posted again, branched off of net-next.
Content is the same, I think.

Thanks,
Richard

maintainer
#12 lore raw

From: Richard Cochran <[email protected]>
Date: Sat, 21 Apr 2012 06:59:49 +0200

Quoted 7 lines · David Miller
Quoted 3 lines

3.4-rcX is not the appropriate tree to base this work on, whereas net-next is.

Okay, posted again, branched off of net-next.
Content is the same, I think.

Cool, thanks.