linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x
@ 2020-11-18 20:30 Christian Eggers
  2020-11-18 20:30 ` [PATCH net-next v3 01/12] dt-bindings: net: dsa: convert ksz bindings document to yaml Christian Eggers
                   ` (12 more replies)
  0 siblings, 13 replies; 32+ messages in thread
From: Christian Eggers @ 2020-11-18 20:30 UTC (permalink / raw)
  To: Vladimir Oltean, Jakub Kicinski, Andrew Lunn, Richard Cochran,
	Rob Herring
  Cc: Vivien Didelot, David S . Miller, Kurt Kanzenbach,
	George McCollister, Marek Vasut, Helmut Grohne, Paul Barker,
	Codrin Ciubotariu, Tristram Ha, Woojung Huh,
	Microchip Linux Driver Support, Christian Eggers, netdev,
	devicetree, linux-kernel

This series adds support for PTP to the KSZ956x and KSZ9477 devices.

There is only little documentation for PTP available on the data sheet
[1] (more or less only the register reference). Questions to the
Microchip support were seldom answered comprehensively or in reasonable
time. So this is more or less the result of reverse engineering.

[1]
http://ww1.microchip.com/downloads/en/DeviceDoc/KSZ9563R-Data-Sheet-DS00002419D.pdf

Changes from v2  --> v3
------------------------
Applied all changes requested by Vladimir Oltean. v3 depends on my other
netdev patches from 2020-11-18:
- net: ptp: introduce common defines for PTP message types
- net: dsa: avoid potential use-after-free error

[1/11]-->[1/12]  - dts: remove " OR BSD-2-Clause" from SPDX-License-Identifier
                 - dts: add "additionalProperties"
                 - dts: remove quotes
[2/11]-->[2/12]  - Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
[3/11]           - [Patch removed] (split ksz_common.h)
[4/11]-->[3/12]  - Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
                 - fixed summary
[5/11]-->[4/12]  - Use "interrupts-extended" syntax
[6/11]-->[5+6/12] - Split up patch
                 - style fixes
                 - use GENMASK()
                 - IRQF_ONESHOT|IRQF_SHARED
[7/11]-->[7/12]  - Remove "default n" from Kconfig
                 - use mutex in adjfine()
                 - style fixes
[8/11]-->[8/12]  - Be more verbose in commit message
                 - Rename helper
                 - provide correction instead of t2
                 - simplify location of UDP header
[9/11]-->[9+10/12] - Split up patch
                 - Update commmit messages
                 - don't use OR operator on irqreturn_t
                 - spin_lock_irqsave() --> spin_lock_bh()
                 - style fixes
                 - remove rx_filter cases for DELAY_REQ
                 - use new PTP_MSGTYPE_* defines
                 - inline ksz9477_ptp_should_tstamp()
                 - ksz9477_tstamp_to_clock() --> ksz9477_tstamp_reconstruct()
                 - use shared data in include/linux/net/dsa/ksz_common.h
                 - wait for tx time stamp (within sleepable context)
                 - use boolean for tx time stamp enable
                 - move t2 from correction to tail tag (again)
                 - ...

Changes from RFC --> v2
------------------------
I think that all open questions regarding the RFC version could be solved.
dts: referenced to dsa.yaml
dts: changed node name to "switch" in example
dts: changed "ports" subnode to "ethernet-ports"
ksz_common: support "ethernet-ports" subnode
tag_ksz: fix usage of correction field (32 bit ns + 16 bit sub-ns)
tag_ksz: use cached PTP header from device's .port_txtstamp function
tag_ksz: refactored ksz9477_tstamp_to_clock()
tag_ksz: pdelay_req: only subtract 2 bit seconds from the correction field
tag_ksz: pdelay_resp: don't move (negative) correction to the egress tail tag
ptp_classify: add ptp_onestep_p2p_move_t2_to_correction helper
ksz9477_ptp: removed E2E support (as suggested by Vladimir)
ksz9477_ptp: removed master/slave sysfs attributes (nacked by Richard)
ksz9477_ptp: refactored ksz9477_ptp_port_txtstamp
ksz9477_ptp: removed "pulse" attribute
kconfig: depend on PTP_1588_CLOCK (instead of "imply")





^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH net-next v3 01/12] dt-bindings: net: dsa: convert ksz bindings document to yaml
  2020-11-18 20:30 [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x Christian Eggers
@ 2020-11-18 20:30 ` Christian Eggers
  2020-11-19 13:42   ` Rob Herring
  2020-11-19 13:48   ` Rob Herring
  2020-11-18 20:30 ` [PATCH net-next v3 02/12] net: dsa: microchip: support for "ethernet-ports" node Christian Eggers
                   ` (11 subsequent siblings)
  12 siblings, 2 replies; 32+ messages in thread
From: Christian Eggers @ 2020-11-18 20:30 UTC (permalink / raw)
  To: Vladimir Oltean, Jakub Kicinski, Andrew Lunn, Richard Cochran,
	Rob Herring
  Cc: Vivien Didelot, David S . Miller, Kurt Kanzenbach,
	George McCollister, Marek Vasut, Helmut Grohne, Paul Barker,
	Codrin Ciubotariu, Tristram Ha, Woojung Huh,
	Microchip Linux Driver Support, Christian Eggers, netdev,
	devicetree, linux-kernel

Convert the bindings document for Microchip KSZ Series Ethernet switches
from txt to yaml.

Signed-off-by: Christian Eggers <ceggers@arri.de>
---
 .../devicetree/bindings/net/dsa/ksz.txt       | 125 --------------
 .../bindings/net/dsa/microchip,ksz.yaml       | 152 ++++++++++++++++++
 MAINTAINERS                                   |   2 +-
 3 files changed, 153 insertions(+), 126 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/net/dsa/ksz.txt
 create mode 100644 Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml

diff --git a/Documentation/devicetree/bindings/net/dsa/ksz.txt b/Documentation/devicetree/bindings/net/dsa/ksz.txt
deleted file mode 100644
index 95e91e84151c..000000000000
--- a/Documentation/devicetree/bindings/net/dsa/ksz.txt
+++ /dev/null
@@ -1,125 +0,0 @@
-Microchip KSZ Series Ethernet switches
-==================================
-
-Required properties:
-
-- compatible: For external switch chips, compatible string must be exactly one
-  of the following:
-  - "microchip,ksz8765"
-  - "microchip,ksz8794"
-  - "microchip,ksz8795"
-  - "microchip,ksz9477"
-  - "microchip,ksz9897"
-  - "microchip,ksz9896"
-  - "microchip,ksz9567"
-  - "microchip,ksz8565"
-  - "microchip,ksz9893"
-  - "microchip,ksz9563"
-  - "microchip,ksz8563"
-
-Optional properties:
-
-- reset-gpios		: Should be a gpio specifier for a reset line
-- microchip,synclko-125 : Set if the output SYNCLKO frequency should be set to
-			  125MHz instead of 25MHz.
-
-See Documentation/devicetree/bindings/net/dsa/dsa.txt for a list of additional
-required and optional properties.
-
-Examples:
-
-Ethernet switch connected via SPI to the host, CPU port wired to eth0:
-
-	eth0: ethernet@10001000 {
-		fixed-link {
-			speed = <1000>;
-			full-duplex;
-		};
-	};
-
-	spi1: spi@f8008000 {
-		pinctrl-0 = <&pinctrl_spi_ksz>;
-		cs-gpios = <&pioC 25 0>;
-		id = <1>;
-
-		ksz9477: ksz9477@0 {
-			compatible = "microchip,ksz9477";
-			reg = <0>;
-
-			spi-max-frequency = <44000000>;
-			spi-cpha;
-			spi-cpol;
-
-			ports {
-				#address-cells = <1>;
-				#size-cells = <0>;
-				port@0 {
-					reg = <0>;
-					label = "lan1";
-				};
-				port@1 {
-					reg = <1>;
-					label = "lan2";
-				};
-				port@2 {
-					reg = <2>;
-					label = "lan3";
-				};
-				port@3 {
-					reg = <3>;
-					label = "lan4";
-				};
-				port@4 {
-					reg = <4>;
-					label = "lan5";
-				};
-				port@5 {
-					reg = <5>;
-					label = "cpu";
-					ethernet = <&eth0>;
-					fixed-link {
-						speed = <1000>;
-						full-duplex;
-					};
-				};
-			};
-		};
-		ksz8565: ksz8565@0 {
-			compatible = "microchip,ksz8565";
-			reg = <0>;
-
-			spi-max-frequency = <44000000>;
-			spi-cpha;
-			spi-cpol;
-
-			ports {
-				#address-cells = <1>;
-				#size-cells = <0>;
-				port@0 {
-					reg = <0>;
-					label = "lan1";
-				};
-				port@1 {
-					reg = <1>;
-					label = "lan2";
-				};
-				port@2 {
-					reg = <2>;
-					label = "lan3";
-				};
-				port@3 {
-					reg = <3>;
-					label = "lan4";
-				};
-				port@6 {
-					reg = <6>;
-					label = "cpu";
-					ethernet = <&eth0>;
-					fixed-link {
-						speed = <1000>;
-						full-duplex;
-					};
-				};
-			};
-		};
-	};
diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
new file mode 100644
index 000000000000..010adb09a68f
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
@@ -0,0 +1,152 @@
+# SPDX-License-Identifier: GPL-2.0-only
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/dsa/microchip,ksz.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip KSZ Series Ethernet switches
+
+allOf:
+  - $ref: dsa.yaml#
+
+maintainers:
+  - Marek Vasut <marex@denx.de>
+  - Woojung Huh <Woojung.Huh@microchip.com>
+
+properties:
+  # See Documentation/devicetree/bindings/net/dsa/dsa.yaml for a list of additional
+  # required and optional properties.
+  compatible:
+    enum:
+      - microchip,ksz8765
+      - microchip,ksz8794
+      - microchip,ksz8795
+      - microchip,ksz9477
+      - microchip,ksz9897
+      - microchip,ksz9896
+      - microchip,ksz9567
+      - microchip,ksz8565
+      - microchip,ksz9893
+      - microchip,ksz9563
+      - microchip,ksz8563
+
+  reset-gpios:
+    description:
+      Should be a gpio specifier for a reset line.
+    maxItems: 1
+
+  microchip,synclko-125:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      Set if the output SYNCLKO frequency should be set to 125MHz instead of 25MHz.
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    // Ethernet switch connected via SPI to the host, CPU port wired to eth0:
+    eth0 {
+        fixed-link {
+            speed = <1000>;
+            full-duplex;
+        };
+    };
+
+    spi0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        pinctrl-0 = <&pinctrl_spi_ksz>;
+        cs-gpios = <&pioC 25 0>;
+        id = <1>;
+
+        ksz9477: switch@0 {
+            compatible = "microchip,ksz9477";
+            reg = <0>;
+            reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
+
+            spi-max-frequency = <44000000>;
+            spi-cpha;
+            spi-cpol;
+
+            ethernet-ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+                port@0 {
+                    reg = <0>;
+                    label = "lan1";
+                };
+                port@1 {
+                    reg = <1>;
+                    label = "lan2";
+                };
+                port@2 {
+                    reg = <2>;
+                    label = "lan3";
+                };
+                port@3 {
+                    reg = <3>;
+                    label = "lan4";
+                };
+                port@4 {
+                    reg = <4>;
+                    label = "lan5";
+                };
+                port@5 {
+                    reg = <5>;
+                    label = "cpu";
+                    ethernet = <&eth0>;
+                    fixed-link {
+                        speed = <1000>;
+                        full-duplex;
+                    };
+                };
+            };
+        };
+
+        ksz8565: switch@1 {
+            compatible = "microchip,ksz8565";
+            reg = <1>;
+
+            spi-max-frequency = <44000000>;
+            spi-cpha;
+            spi-cpol;
+
+            ethernet-ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+                port@0 {
+                    reg = <0>;
+                    label = "lan1";
+                };
+                port@1 {
+                    reg = <1>;
+                    label = "lan2";
+                };
+                port@2 {
+                    reg = <2>;
+                    label = "lan3";
+                };
+                port@3 {
+                    reg = <3>;
+                    label = "lan4";
+                };
+                port@6 {
+                    reg = <6>;
+                    label = "cpu";
+                    ethernet = <&eth0>;
+                    fixed-link {
+                        speed = <1000>;
+                        full-duplex;
+                    };
+                };
+            };
+        };
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 18b5b7896af8..d1003033412f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11508,7 +11508,7 @@ M:	Woojung Huh <woojung.huh@microchip.com>
 M:	Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>
 L:	netdev@vger.kernel.org
 S:	Maintained
-F:	Documentation/devicetree/bindings/net/dsa/ksz.txt
+F:	Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
 F:	drivers/net/dsa/microchip/*
 F:	include/linux/platform_data/microchip-ksz.h
 F:	net/dsa/tag_ksz.c
-- 
Christian Eggers
Embedded software developer

Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH net-next v3 02/12] net: dsa: microchip: support for "ethernet-ports" node
  2020-11-18 20:30 [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x Christian Eggers
  2020-11-18 20:30 ` [PATCH net-next v3 01/12] dt-bindings: net: dsa: convert ksz bindings document to yaml Christian Eggers
@ 2020-11-18 20:30 ` Christian Eggers
  2020-11-18 20:30 ` [PATCH net-next v3 03/12] net: dsa: microchip: rename ksz9477.c to ksz9477_main.c Christian Eggers
                   ` (10 subsequent siblings)
  12 siblings, 0 replies; 32+ messages in thread
From: Christian Eggers @ 2020-11-18 20:30 UTC (permalink / raw)
  To: Vladimir Oltean, Jakub Kicinski, Andrew Lunn, Richard Cochran,
	Rob Herring
  Cc: Vivien Didelot, David S . Miller, Kurt Kanzenbach,
	George McCollister, Marek Vasut, Helmut Grohne, Paul Barker,
	Codrin Ciubotariu, Tristram Ha, Woojung Huh,
	Microchip Linux Driver Support, Christian Eggers, netdev,
	devicetree, linux-kernel

The dsa.yaml device tree binding allows "ethernet-ports" (preferred) and
"ports".

Signed-off-by: Christian Eggers <ceggers@arri.de>
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/net/dsa/microchip/ksz_common.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 71cd1828e25d..a135fd5a9264 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -427,7 +427,9 @@ int ksz_switch_register(struct ksz_device *dev,
 		ret = of_get_phy_mode(dev->dev->of_node, &interface);
 		if (ret == 0)
 			dev->compat_interface = interface;
-		ports = of_get_child_by_name(dev->dev->of_node, "ports");
+		ports = of_get_child_by_name(dev->dev->of_node, "ethernet-ports");
+		if (!ports)
+			ports = of_get_child_by_name(dev->dev->of_node, "ports");
 		if (ports)
 			for_each_available_child_of_node(ports, port) {
 				if (of_property_read_u32(port, "reg",
-- 
Christian Eggers
Embedded software developer

Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH net-next v3 03/12] net: dsa: microchip: rename ksz9477.c to ksz9477_main.c
  2020-11-18 20:30 [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x Christian Eggers
  2020-11-18 20:30 ` [PATCH net-next v3 01/12] dt-bindings: net: dsa: convert ksz bindings document to yaml Christian Eggers
  2020-11-18 20:30 ` [PATCH net-next v3 02/12] net: dsa: microchip: support for "ethernet-ports" node Christian Eggers
@ 2020-11-18 20:30 ` Christian Eggers
  2020-11-18 20:30 ` [PATCH net-next v3 04/12] dt-bindings: net: dsa: microchip,ksz: add interrupt property Christian Eggers
                   ` (9 subsequent siblings)
  12 siblings, 0 replies; 32+ messages in thread
From: Christian Eggers @ 2020-11-18 20:30 UTC (permalink / raw)
  To: Vladimir Oltean, Jakub Kicinski, Andrew Lunn, Richard Cochran,
	Rob Herring
  Cc: Vivien Didelot, David S . Miller, Kurt Kanzenbach,
	George McCollister, Marek Vasut, Helmut Grohne, Paul Barker,
	Codrin Ciubotariu, Tristram Ha, Woojung Huh,
	Microchip Linux Driver Support, Christian Eggers, netdev,
	devicetree, linux-kernel

PTP functionality will be built into a separate source file
(ksz9477_ptp.c).

Signed-off-by: Christian Eggers <ceggers@arri.de>
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/net/dsa/microchip/Makefile                      | 1 +
 drivers/net/dsa/microchip/{ksz9477.c => ksz9477_main.c} | 0
 2 files changed, 1 insertion(+)
 rename drivers/net/dsa/microchip/{ksz9477.c => ksz9477_main.c} (100%)

diff --git a/drivers/net/dsa/microchip/Makefile b/drivers/net/dsa/microchip/Makefile
index 929caa81e782..c5cc1d5dea06 100644
--- a/drivers/net/dsa/microchip/Makefile
+++ b/drivers/net/dsa/microchip/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ_COMMON)	+= ksz_common.o
 obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ9477)		+= ksz9477.o
+ksz9477-objs := ksz9477_main.o
 obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ9477_I2C)	+= ksz9477_i2c.o
 obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ9477_SPI)	+= ksz9477_spi.o
 obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ8795)		+= ksz8795.o
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477_main.c
similarity index 100%
rename from drivers/net/dsa/microchip/ksz9477.c
rename to drivers/net/dsa/microchip/ksz9477_main.c
-- 
Christian Eggers
Embedded software developer

Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH net-next v3 04/12] dt-bindings: net: dsa: microchip,ksz: add interrupt property
  2020-11-18 20:30 [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x Christian Eggers
                   ` (2 preceding siblings ...)
  2020-11-18 20:30 ` [PATCH net-next v3 03/12] net: dsa: microchip: rename ksz9477.c to ksz9477_main.c Christian Eggers
@ 2020-11-18 20:30 ` Christian Eggers
  2020-11-18 20:30 ` [PATCH net-next v3 05/12] net: dsa: microchip: ksz9477: move chip reset to ksz9477_switch_init() Christian Eggers
                   ` (8 subsequent siblings)
  12 siblings, 0 replies; 32+ messages in thread
From: Christian Eggers @ 2020-11-18 20:30 UTC (permalink / raw)
  To: Vladimir Oltean, Jakub Kicinski, Andrew Lunn, Richard Cochran,
	Rob Herring
  Cc: Vivien Didelot, David S . Miller, Kurt Kanzenbach,
	George McCollister, Marek Vasut, Helmut Grohne, Paul Barker,
	Codrin Ciubotariu, Tristram Ha, Woojung Huh,
	Microchip Linux Driver Support, Christian Eggers, netdev,
	devicetree, linux-kernel

The devices have an optional interrupt line.

Signed-off-by: Christian Eggers <ceggers@arri.de>
---
 .../devicetree/bindings/net/dsa/microchip,ksz.yaml         | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
index 010adb09a68f..74d42dee310f 100644
--- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
@@ -35,6 +35,11 @@ properties:
       Should be a gpio specifier for a reset line.
     maxItems: 1
 
+  interrupts:
+    description:
+      Interrupt specifier for the INTRP_N line from the device.
+    maxItems: 1
+
   microchip,synclko-125:
     $ref: /schemas/types.yaml#/definitions/flag
     description:
@@ -49,6 +54,7 @@ additionalProperties: false
 examples:
   - |
     #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
 
     // Ethernet switch connected via SPI to the host, CPU port wired to eth0:
     eth0 {
@@ -70,6 +76,7 @@ examples:
             compatible = "microchip,ksz9477";
             reg = <0>;
             reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
+            interrupts-extended = <&gpio5 1 IRQ_TYPE_LEVEL_LOW>;  /* INTRP_N line */
 
             spi-max-frequency = <44000000>;
             spi-cpha;
-- 
Christian Eggers
Embedded software developer

Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH net-next v3 05/12] net: dsa: microchip: ksz9477: move chip reset to ksz9477_switch_init()
  2020-11-18 20:30 [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x Christian Eggers
                   ` (3 preceding siblings ...)
  2020-11-18 20:30 ` [PATCH net-next v3 04/12] dt-bindings: net: dsa: microchip,ksz: add interrupt property Christian Eggers
@ 2020-11-18 20:30 ` Christian Eggers
  2020-11-20 22:49   ` Vladimir Oltean
  2020-11-18 20:30 ` [PATCH net-next v3 06/12] net: dsa: microchip: ksz9477: basic interrupt support Christian Eggers
                   ` (7 subsequent siblings)
  12 siblings, 1 reply; 32+ messages in thread
From: Christian Eggers @ 2020-11-18 20:30 UTC (permalink / raw)
  To: Vladimir Oltean, Jakub Kicinski, Andrew Lunn, Richard Cochran,
	Rob Herring
  Cc: Vivien Didelot, David S . Miller, Kurt Kanzenbach,
	George McCollister, Marek Vasut, Helmut Grohne, Paul Barker,
	Codrin Ciubotariu, Tristram Ha, Woojung Huh,
	Microchip Linux Driver Support, Christian Eggers, netdev,
	devicetree, linux-kernel

The next patch will add basic interrupt support. Chip reset must be
performed before requesting the IRQ, so move this from ksz9477_setup()
to ksz9477_init().

Signed-off-by: Christian Eggers <ceggers@arri.de>
---
 drivers/net/dsa/microchip/ksz9477_main.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477_main.c b/drivers/net/dsa/microchip/ksz9477_main.c
index abfd3802bb51..c0b4cf66c904 100644
--- a/drivers/net/dsa/microchip/ksz9477_main.c
+++ b/drivers/net/dsa/microchip/ksz9477_main.c
@@ -1345,19 +1345,12 @@ static void ksz9477_config_cpu_port(struct dsa_switch *ds)
 static int ksz9477_setup(struct dsa_switch *ds)
 {
 	struct ksz_device *dev = ds->priv;
-	int ret = 0;
 
 	dev->vlan_cache = devm_kcalloc(dev->dev, sizeof(struct vlan_table),
 				       dev->num_vlans, GFP_KERNEL);
 	if (!dev->vlan_cache)
 		return -ENOMEM;
 
-	ret = ksz9477_reset_switch(dev);
-	if (ret) {
-		dev_err(ds->dev, "failed to reset switch\n");
-		return ret;
-	}
-
 	/* Required for port partitioning. */
 	ksz9477_cfg32(dev, REG_SW_QM_CTRL__4, UNICAST_VLAN_BOUNDARY,
 		      true);
@@ -1537,10 +1530,16 @@ static const struct ksz_chip_data ksz9477_switch_chips[] = {
 
 static int ksz9477_switch_init(struct ksz_device *dev)
 {
-	int i;
+	int i, ret;
 
 	dev->ds->ops = &ksz9477_switch_ops;
 
+	ret = ksz9477_reset_switch(dev);
+	if (ret) {
+		dev_err(dev->dev, "failed to reset switch\n");
+		return ret;
+	}
+
 	for (i = 0; i < ARRAY_SIZE(ksz9477_switch_chips); i++) {
 		const struct ksz_chip_data *chip = &ksz9477_switch_chips[i];
 
-- 
Christian Eggers
Embedded software developer

Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH net-next v3 06/12] net: dsa: microchip: ksz9477: basic interrupt support
  2020-11-18 20:30 [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x Christian Eggers
                   ` (4 preceding siblings ...)
  2020-11-18 20:30 ` [PATCH net-next v3 05/12] net: dsa: microchip: ksz9477: move chip reset to ksz9477_switch_init() Christian Eggers
@ 2020-11-18 20:30 ` Christian Eggers
  2020-11-20 23:03   ` Vladimir Oltean
  2020-11-18 20:30 ` [PATCH net-next v3 07/12] net: dsa: microchip: ksz9477: add Posix clock support for chip PTP clock Christian Eggers
                   ` (6 subsequent siblings)
  12 siblings, 1 reply; 32+ messages in thread
From: Christian Eggers @ 2020-11-18 20:30 UTC (permalink / raw)
  To: Vladimir Oltean, Jakub Kicinski, Andrew Lunn, Richard Cochran,
	Rob Herring
  Cc: Vivien Didelot, David S . Miller, Kurt Kanzenbach,
	George McCollister, Marek Vasut, Helmut Grohne, Paul Barker,
	Codrin Ciubotariu, Tristram Ha, Woojung Huh,
	Microchip Linux Driver Support, Christian Eggers, netdev,
	devicetree, linux-kernel

Interrupts are required for TX time stamping. Probably they could also
be used for PHY connection status.

This patch only adds the basic infrastructure for interrupts, no
interrupts are finally enabled nor handled.

Signed-off-by: Christian Eggers <ceggers@arri.de>
---
 drivers/net/dsa/microchip/ksz9477_i2c.c  |  2 +
 drivers/net/dsa/microchip/ksz9477_main.c | 68 ++++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz9477_spi.c  |  2 +
 drivers/net/dsa/microchip/ksz_common.h   |  1 +
 4 files changed, 73 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz9477_i2c.c b/drivers/net/dsa/microchip/ksz9477_i2c.c
index 4e053a25d077..4ed1f503044a 100644
--- a/drivers/net/dsa/microchip/ksz9477_i2c.c
+++ b/drivers/net/dsa/microchip/ksz9477_i2c.c
@@ -41,6 +41,8 @@ static int ksz9477_i2c_probe(struct i2c_client *i2c,
 	if (i2c->dev.platform_data)
 		dev->pdata = i2c->dev.platform_data;
 
+	dev->irq = i2c->irq;
+
 	ret = ksz9477_switch_register(dev);
 
 	/* Main DSA driver may not be started yet. */
diff --git a/drivers/net/dsa/microchip/ksz9477_main.c b/drivers/net/dsa/microchip/ksz9477_main.c
index c0b4cf66c904..34964f6bf354 100644
--- a/drivers/net/dsa/microchip/ksz9477_main.c
+++ b/drivers/net/dsa/microchip/ksz9477_main.c
@@ -5,9 +5,12 @@
  * Copyright (C) 2017-2019 Microchip Technology Inc.
  */
 
+#include <linux/bits.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/interrupt.h>
 #include <linux/iopoll.h>
+#include <linux/irq.h>
 #include <linux/platform_data/microchip-ksz.h>
 #include <linux/phy.h>
 #include <linux/if_bridge.h>
@@ -1528,6 +1531,54 @@ static const struct ksz_chip_data ksz9477_switch_chips[] = {
 	},
 };
 
+static irqreturn_t ksz9477_switch_irq_thread(int irq, void *dev_id)
+{
+	struct ksz_device *dev = dev_id;
+	u32 data;
+	int port;
+	int ret;
+
+	/* Read global port interrupt status register */
+	ret = ksz_read32(dev, REG_SW_PORT_INT_STATUS__4, &data);
+	if (ret)
+		return IRQ_NONE;
+
+	for (port = 0; port < dev->port_cnt; port++) {
+		u8 data8;
+
+		if (!(data & BIT(port)))
+			continue;
+
+		/* Read port interrupt status register */
+		ret = ksz_read8(dev, PORT_CTRL_ADDR(port, REG_PORT_INT_STATUS),
+				&data8);
+		if (ret)
+			return IRQ_NONE;
+
+		/* ToDo: Add specific handling of port interrupts */
+	}
+
+	return IRQ_NONE;
+}
+
+static int ksz9477_enable_port_interrupts(struct ksz_device *dev, bool enable)
+{
+	u32 data, mask = GENMASK(dev->port_cnt - 1, 0);
+	int ret;
+
+	ret = ksz_read32(dev, REG_SW_PORT_INT_MASK__4, &data);
+	if (ret)
+		return ret;
+
+	/* bits in REG_SW_PORT_INT_MASK__4 are low active */
+	if (enable)
+		data &= ~mask;
+	else
+		data |= mask;
+
+	return ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data);
+}
+
 static int ksz9477_switch_init(struct ksz_device *dev)
 {
 	int i, ret;
@@ -1583,12 +1634,29 @@ static int ksz9477_switch_init(struct ksz_device *dev)
 
 	/* set the real number of ports */
 	dev->ds->num_ports = dev->port_cnt;
+	if (dev->irq > 0) {
+		ret = devm_request_threaded_irq(dev->dev, dev->irq, NULL,
+						ksz9477_switch_irq_thread,
+						IRQF_ONESHOT | IRQF_SHARED,
+						dev_name(dev->dev),
+						dev);
+		if (ret) {
+			dev_err(dev->dev, "failed to request IRQ.\n");
+			return ret;
+		}
+
+		ret = ksz9477_enable_port_interrupts(dev, true);
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }
 
 static void ksz9477_switch_exit(struct ksz_device *dev)
 {
+	if (dev->irq > 0)
+		ksz9477_enable_port_interrupts(dev, false);
 	ksz9477_reset_switch(dev);
 }
 
diff --git a/drivers/net/dsa/microchip/ksz9477_spi.c b/drivers/net/dsa/microchip/ksz9477_spi.c
index 1142768969c2..d2eea9596e53 100644
--- a/drivers/net/dsa/microchip/ksz9477_spi.c
+++ b/drivers/net/dsa/microchip/ksz9477_spi.c
@@ -48,6 +48,8 @@ static int ksz9477_spi_probe(struct spi_device *spi)
 	if (spi->dev.platform_data)
 		dev->pdata = spi->dev.platform_data;
 
+	dev->irq = spi->irq;
+
 	ret = ksz9477_switch_register(dev);
 
 	/* Main DSA driver may not be started yet. */
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index cf866e48ff66..5df4a7f9df02 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -55,6 +55,7 @@ struct ksz_device {
 
 	struct device *dev;
 	struct regmap *regmap[3];
+	int irq;
 
 	void *priv;
 
-- 
Christian Eggers
Embedded software developer

Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH net-next v3 07/12] net: dsa: microchip: ksz9477: add Posix clock support for chip PTP clock
  2020-11-18 20:30 [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x Christian Eggers
                   ` (5 preceding siblings ...)
  2020-11-18 20:30 ` [PATCH net-next v3 06/12] net: dsa: microchip: ksz9477: basic interrupt support Christian Eggers
@ 2020-11-18 20:30 ` Christian Eggers
  2020-11-20 23:14   ` Vladimir Oltean
  2020-11-18 20:30 ` [PATCH net-next v3 08/12] net: ptp: add helper for one-step P2P clocks Christian Eggers
                   ` (5 subsequent siblings)
  12 siblings, 1 reply; 32+ messages in thread
From: Christian Eggers @ 2020-11-18 20:30 UTC (permalink / raw)
  To: Vladimir Oltean, Jakub Kicinski, Andrew Lunn, Richard Cochran,
	Rob Herring
  Cc: Vivien Didelot, David S . Miller, Kurt Kanzenbach,
	George McCollister, Marek Vasut, Helmut Grohne, Paul Barker,
	Codrin Ciubotariu, Tristram Ha, Woojung Huh,
	Microchip Linux Driver Support, Christian Eggers, netdev,
	devicetree, linux-kernel

Implement routines (adjfine, adjtime, gettime and settime) for
manipulating the chip's PTP clock.

Signed-off-by: Christian Eggers <ceggers@arri.de>
---
 drivers/net/dsa/microchip/Kconfig        |   8 +
 drivers/net/dsa/microchip/Makefile       |   1 +
 drivers/net/dsa/microchip/ksz9477_i2c.c  |   2 +-
 drivers/net/dsa/microchip/ksz9477_main.c |  17 ++
 drivers/net/dsa/microchip/ksz9477_ptp.c  | 308 +++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz9477_ptp.h  |  27 ++
 drivers/net/dsa/microchip/ksz9477_spi.c  |   2 +-
 drivers/net/dsa/microchip/ksz_common.h   |   8 +
 8 files changed, 371 insertions(+), 2 deletions(-)
 create mode 100644 drivers/net/dsa/microchip/ksz9477_ptp.c
 create mode 100644 drivers/net/dsa/microchip/ksz9477_ptp.h

diff --git a/drivers/net/dsa/microchip/Kconfig b/drivers/net/dsa/microchip/Kconfig
index 4ec6a47b7f72..7a4e06bab238 100644
--- a/drivers/net/dsa/microchip/Kconfig
+++ b/drivers/net/dsa/microchip/Kconfig
@@ -24,6 +24,14 @@ config NET_DSA_MICROCHIP_KSZ9477_SPI
 	help
 	  Select to enable support for registering switches configured through SPI.
 
+config NET_DSA_MICROCHIP_KSZ9477_PTP
+	bool "PTP support for Microchip KSZ9477 series"
+	depends on NET_DSA_MICROCHIP_KSZ9477
+	depends on PTP_1588_CLOCK
+	help
+	  Say Y to enable PTP hardware timestamping on Microchip KSZ switch
+	  chips that support it.
+
 menuconfig NET_DSA_MICROCHIP_KSZ8795
 	tristate "Microchip KSZ8795 series switch support"
 	depends on NET_DSA
diff --git a/drivers/net/dsa/microchip/Makefile b/drivers/net/dsa/microchip/Makefile
index c5cc1d5dea06..35c4356bad65 100644
--- a/drivers/net/dsa/microchip/Makefile
+++ b/drivers/net/dsa/microchip/Makefile
@@ -2,6 +2,7 @@
 obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ_COMMON)	+= ksz_common.o
 obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ9477)		+= ksz9477.o
 ksz9477-objs := ksz9477_main.o
+ksz9477-$(CONFIG_NET_DSA_MICROCHIP_KSZ9477_PTP)	+= ksz9477_ptp.o
 obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ9477_I2C)	+= ksz9477_i2c.o
 obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ9477_SPI)	+= ksz9477_spi.o
 obj-$(CONFIG_NET_DSA_MICROCHIP_KSZ8795)		+= ksz8795.o
diff --git a/drivers/net/dsa/microchip/ksz9477_i2c.c b/drivers/net/dsa/microchip/ksz9477_i2c.c
index 4ed1f503044a..315eb24c444d 100644
--- a/drivers/net/dsa/microchip/ksz9477_i2c.c
+++ b/drivers/net/dsa/microchip/ksz9477_i2c.c
@@ -58,7 +58,7 @@ static int ksz9477_i2c_remove(struct i2c_client *i2c)
 {
 	struct ksz_device *dev = i2c_get_clientdata(i2c);
 
-	ksz_switch_remove(dev);
+	ksz9477_switch_remove(dev);
 
 	return 0;
 }
diff --git a/drivers/net/dsa/microchip/ksz9477_main.c b/drivers/net/dsa/microchip/ksz9477_main.c
index 34964f6bf354..d1a2ad4a34f1 100644
--- a/drivers/net/dsa/microchip/ksz9477_main.c
+++ b/drivers/net/dsa/microchip/ksz9477_main.c
@@ -19,6 +19,7 @@
 
 #include "ksz9477_reg.h"
 #include "ksz_common.h"
+#include "ksz9477_ptp.h"
 
 /* Used with variable features to indicate capabilities. */
 #define GBIT_SUPPORT			BIT(0)
@@ -1699,10 +1700,26 @@ int ksz9477_switch_register(struct ksz_device *dev)
 			phy_remove_link_mode(phydev,
 					     ETHTOOL_LINK_MODE_1000baseT_Full_BIT);
 	}
+
+	ret = ksz9477_ptp_init(dev);
+	if (ret)
+		goto error_switch_unregister;
+
+	return 0;
+
+error_switch_unregister:
+	ksz_switch_remove(dev);
 	return ret;
 }
 EXPORT_SYMBOL(ksz9477_switch_register);
 
+void ksz9477_switch_remove(struct ksz_device *dev)
+{
+	ksz9477_ptp_deinit(dev);
+	ksz_switch_remove(dev);
+}
+EXPORT_SYMBOL(ksz9477_switch_remove);
+
 MODULE_AUTHOR("Woojung Huh <Woojung.Huh@microchip.com>");
 MODULE_DESCRIPTION("Microchip KSZ9477 Series Switch DSA Driver");
 MODULE_LICENSE("GPL");
diff --git a/drivers/net/dsa/microchip/ksz9477_ptp.c b/drivers/net/dsa/microchip/ksz9477_ptp.c
new file mode 100644
index 000000000000..0ffc4504a290
--- /dev/null
+++ b/drivers/net/dsa/microchip/ksz9477_ptp.c
@@ -0,0 +1,308 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Microchip KSZ9477 switch driver PTP routines
+ *
+ * Author: Christian Eggers <ceggers@arri.de>
+ *
+ * Copyright (c) 2020 ARRI Lighting
+ */
+
+#include <linux/ptp_clock_kernel.h>
+
+#include "ksz_common.h"
+#include "ksz9477_reg.h"
+
+#include "ksz9477_ptp.h"
+
+#define KSZ_PTP_INC_NS 40  /* HW clock is incremented every 40 ns (by 40) */
+#define KSZ_PTP_SUBNS_BITS 32  /* Number of bits in sub-nanoseconds counter */
+
+/* Posix clock support */
+
+static int ksz9477_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
+{
+	struct ksz_device *dev = container_of(ptp, struct ksz_device, ptp_caps);
+	u16 data16;
+	int ret;
+
+	mutex_lock(&dev->ptp_mutex);
+
+	if (scaled_ppm) {
+		s64 ppb, adj;
+		u32 data32;
+
+		/* basic calculation:
+		 * s32 ppb = scaled_ppm_to_ppb(scaled_ppm);
+		 * s64 adj = div_s64(((s64)ppb * KSZ_PTP_INC_NS) << KSZ_PTP_SUBNS_BITS,
+		 *                   NSEC_PER_SEC);
+		 */
+
+		/* More precise calculation (avoids shifting out precision).
+		 * See scaled_ppm_to_ppb() in ptp_clock.c for details.
+		 */
+		ppb = 1 + scaled_ppm;
+		ppb *= 125;
+		ppb *= KSZ_PTP_INC_NS;
+		ppb <<= KSZ_PTP_SUBNS_BITS - 13;
+		adj = div_s64(ppb, NSEC_PER_SEC);
+
+		data32 = abs(adj);
+		data32 &= BIT_MASK(30) - 1;
+		if (adj >= 0)
+			data32 |= PTP_RATE_DIR;
+
+		ret = ksz_write32(dev, REG_PTP_SUBNANOSEC_RATE, data32);
+		if (ret)
+			goto error_return;
+	}
+
+	ret = ksz_read16(dev, REG_PTP_CLK_CTRL, &data16);
+	if (ret)
+		goto error_return;
+
+	if (scaled_ppm)
+		data16 |= PTP_CLK_ADJ_ENABLE;
+	else
+		data16 &= ~PTP_CLK_ADJ_ENABLE;
+
+	ret = ksz_write16(dev, REG_PTP_CLK_CTRL, data16);
+	if (ret)
+		goto error_return;
+
+error_return:
+	mutex_unlock(&dev->ptp_mutex);
+	return ret;
+}
+
+static int ksz9477_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
+{
+	struct ksz_device *dev = container_of(ptp, struct ksz_device, ptp_caps);
+	s32 sec, nsec;
+	u16 data16;
+	int ret;
+
+	mutex_lock(&dev->ptp_mutex);
+
+	/* Do not use ns_to_timespec64(), both sec and nsec are subtracted by
+	 * hardware.
+	 */
+	sec = div_s64_rem(delta, NSEC_PER_SEC, &nsec);
+
+	ret = ksz_write32(dev, REG_PTP_RTC_NANOSEC, abs(nsec));
+	if (ret)
+		goto error_return;
+
+	/* Contradictory to the data sheet, seconds are also considered.  */
+	ret = ksz_write32(dev, REG_PTP_RTC_SEC, abs(sec));
+	if (ret)
+		goto error_return;
+
+	ret = ksz_read16(dev, REG_PTP_CLK_CTRL, &data16);
+	if (ret)
+		goto error_return;
+
+	data16 |= PTP_STEP_ADJ;
+	if (delta < 0)
+		data16 &= ~PTP_STEP_DIR;  /* 0: subtract */
+	else
+		data16 |= PTP_STEP_DIR;   /* 1: add */
+
+	ret = ksz_write16(dev, REG_PTP_CLK_CTRL, data16);
+	if (ret)
+		goto error_return;
+
+error_return:
+	mutex_unlock(&dev->ptp_mutex);
+	return ret;
+}
+
+static int _ksz9477_ptp_gettime(struct ksz_device *dev, struct timespec64 *ts)
+{
+	u32 nanoseconds;
+	u32 seconds;
+	u16 data16;
+	u8 phase;
+	int ret;
+
+	/* Copy current PTP clock into shadow registers */
+	ret = ksz_read16(dev, REG_PTP_CLK_CTRL, &data16);
+	if (ret)
+		return ret;
+
+	data16 |= PTP_READ_TIME;
+
+	ret = ksz_write16(dev, REG_PTP_CLK_CTRL, data16);
+	if (ret)
+		return ret;
+
+	/* Read from shadow registers */
+	ret = ksz_read8(dev, REG_PTP_RTC_SUB_NANOSEC__2, &phase);
+	if (ret)
+		return ret;
+	ret = ksz_read32(dev, REG_PTP_RTC_NANOSEC, &nanoseconds);
+	if (ret)
+		return ret;
+	ret = ksz_read32(dev, REG_PTP_RTC_SEC, &seconds);
+	if (ret)
+		return ret;
+
+	ts->tv_sec = seconds;
+	ts->tv_nsec = nanoseconds + phase * 8;
+
+	return 0;
+}
+
+static int ksz9477_ptp_gettime(struct ptp_clock_info *ptp,
+			       struct timespec64 *ts)
+{
+	struct ksz_device *dev = container_of(ptp, struct ksz_device, ptp_caps);
+	int ret;
+
+	mutex_lock(&dev->ptp_mutex);
+	ret = _ksz9477_ptp_gettime(dev, ts);
+	mutex_unlock(&dev->ptp_mutex);
+
+	return ret;
+}
+
+static int ksz9477_ptp_settime(struct ptp_clock_info *ptp,
+			       struct timespec64 const *ts)
+{
+	struct ksz_device *dev = container_of(ptp, struct ksz_device, ptp_caps);
+	u16 data16;
+	int ret;
+
+	mutex_lock(&dev->ptp_mutex);
+
+	/* Write to shadow registers */
+
+	/* clock phase */
+	ret = ksz_read16(dev, REG_PTP_RTC_SUB_NANOSEC__2, &data16);
+	if (ret)
+		goto error_return;
+
+	data16 &= ~PTP_RTC_SUB_NANOSEC_M;
+
+	ret = ksz_write16(dev, REG_PTP_RTC_SUB_NANOSEC__2, data16);
+	if (ret)
+		goto error_return;
+
+	/* nanoseconds */
+	ret = ksz_write32(dev, REG_PTP_RTC_NANOSEC, ts->tv_nsec);
+	if (ret)
+		goto error_return;
+
+	/* seconds */
+	ret = ksz_write32(dev, REG_PTP_RTC_SEC, ts->tv_sec);
+	if (ret)
+		goto error_return;
+
+	/* Load PTP clock from shadow registers */
+	ret = ksz_read16(dev, REG_PTP_CLK_CTRL, &data16);
+	if (ret)
+		goto error_return;
+
+	data16 |= PTP_LOAD_TIME;
+
+	ret = ksz_write16(dev, REG_PTP_CLK_CTRL, data16);
+	if (ret)
+		goto error_return;
+
+error_return:
+	mutex_unlock(&dev->ptp_mutex);
+	return ret;
+}
+
+static int ksz9477_ptp_enable(struct ptp_clock_info *ptp,
+			      struct ptp_clock_request *req, int on)
+{
+	return -EOPNOTSUPP;
+}
+
+static int ksz9477_ptp_start_clock(struct ksz_device *dev)
+{
+	u16 data;
+	int ret;
+
+	ret = ksz_read16(dev, REG_PTP_CLK_CTRL, &data);
+	if (ret)
+		return ret;
+
+	/* Perform PTP clock reset */
+	data |= PTP_CLK_RESET;
+	ret = ksz_write16(dev, REG_PTP_CLK_CTRL, data);
+	if (ret)
+		return ret;
+	data &= ~PTP_CLK_RESET;
+
+	/* Enable PTP clock */
+	data |= PTP_CLK_ENABLE;
+	ret = ksz_write16(dev, REG_PTP_CLK_CTRL, data);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ksz9477_ptp_stop_clock(struct ksz_device *dev)
+{
+	u16 data;
+	int ret;
+
+	ret = ksz_read16(dev, REG_PTP_CLK_CTRL, &data);
+	if (ret)
+		return ret;
+
+	/* Disable PTP clock */
+	data &= ~PTP_CLK_ENABLE;
+	return ksz_write16(dev, REG_PTP_CLK_CTRL, data);
+}
+
+int ksz9477_ptp_init(struct ksz_device *dev)
+{
+	int ret;
+
+	mutex_init(&dev->ptp_mutex);
+
+	/* PTP clock properties */
+
+	dev->ptp_caps.owner = THIS_MODULE;
+	snprintf(dev->ptp_caps.name, sizeof(dev->ptp_caps.name),
+		 dev_name(dev->dev));
+
+	/* Sub-nanoseconds-adj,max * sub-nanoseconds / 40ns * 1ns
+	 * = (2^30-1) * (2 ^ 32) / 40 ns * 1 ns = 6249999
+	 */
+	dev->ptp_caps.max_adj     = 6249999;
+	dev->ptp_caps.n_alarm     = 0;
+	dev->ptp_caps.n_ext_ts    = 0;  /* currently not implemented */
+	dev->ptp_caps.n_per_out   = 0;
+	dev->ptp_caps.pps         = 0;
+	dev->ptp_caps.adjfine     = ksz9477_ptp_adjfine;
+	dev->ptp_caps.adjtime     = ksz9477_ptp_adjtime;
+	dev->ptp_caps.gettime64   = ksz9477_ptp_gettime;
+	dev->ptp_caps.settime64   = ksz9477_ptp_settime;
+	dev->ptp_caps.enable      = ksz9477_ptp_enable;
+
+	/* Start hardware counter (will overflow after 136 years) */
+	ret = ksz9477_ptp_start_clock(dev);
+	if (ret)
+		return ret;
+
+	dev->ptp_clock = ptp_clock_register(&dev->ptp_caps, dev->dev);
+	if (IS_ERR(dev->ptp_clock)) {
+		ret = PTR_ERR(dev->ptp_clock);
+		goto error_stop_clock;
+	}
+
+	return 0;
+
+error_stop_clock:
+	ksz9477_ptp_stop_clock(dev);
+	return ret;
+}
+
+void ksz9477_ptp_deinit(struct ksz_device *dev)
+{
+	ptp_clock_unregister(dev->ptp_clock);
+	ksz9477_ptp_stop_clock(dev);
+}
diff --git a/drivers/net/dsa/microchip/ksz9477_ptp.h b/drivers/net/dsa/microchip/ksz9477_ptp.h
new file mode 100644
index 000000000000..0076538419fa
--- /dev/null
+++ b/drivers/net/dsa/microchip/ksz9477_ptp.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * Microchip KSZ9477 switch driver PTP routines
+ *
+ * Author: Christian Eggers <ceggers@arri.de>
+ *
+ * Copyright (c) 2020 ARRI Lighting
+ */
+
+#ifndef DRIVERS_NET_DSA_MICROCHIP_KSZ9477_PTP_H_
+#define DRIVERS_NET_DSA_MICROCHIP_KSZ9477_PTP_H_
+
+#include "ksz_common.h"
+
+#if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ9477_PTP)
+
+int ksz9477_ptp_init(struct ksz_device *dev);
+void ksz9477_ptp_deinit(struct ksz_device *dev);
+
+#else
+
+static inline int ksz9477_ptp_init(struct ksz_device *dev) { return 0; }
+static inline void ksz9477_ptp_deinit(struct ksz_device *dev) {}
+
+#endif
+
+#endif /* DRIVERS_NET_DSA_MICROCHIP_KSZ9477_PTP_H_ */
diff --git a/drivers/net/dsa/microchip/ksz9477_spi.c b/drivers/net/dsa/microchip/ksz9477_spi.c
index d2eea9596e53..e49d581547ac 100644
--- a/drivers/net/dsa/microchip/ksz9477_spi.c
+++ b/drivers/net/dsa/microchip/ksz9477_spi.c
@@ -66,7 +66,7 @@ static int ksz9477_spi_remove(struct spi_device *spi)
 	struct ksz_device *dev = spi_get_drvdata(spi);
 
 	if (dev)
-		ksz_switch_remove(dev);
+		ksz9477_switch_remove(dev);
 
 	return 0;
 }
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 5df4a7f9df02..43dd66009482 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -11,6 +11,7 @@
 #include <linux/kernel.h>
 #include <linux/mutex.h>
 #include <linux/phy.h>
+#include <linux/ptp_clock_kernel.h>
 #include <linux/regmap.h>
 #include <net/dsa.h>
 
@@ -92,6 +93,12 @@ struct ksz_device {
 	u32 overrides;			/* chip functions set by user */
 	u16 host_mask;
 	u16 port_mask;
+
+#if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ9477_PTP)
+	struct ptp_clock *ptp_clock;
+	struct ptp_clock_info ptp_caps;
+	struct mutex ptp_mutex;		/* protects PTP related hardware */
+#endif
 };
 
 struct alu_struct {
@@ -147,6 +154,7 @@ void ksz_switch_remove(struct ksz_device *dev);
 
 int ksz8795_switch_register(struct ksz_device *dev);
 int ksz9477_switch_register(struct ksz_device *dev);
+void ksz9477_switch_remove(struct ksz_device *dev);
 
 void ksz_update_port_member(struct ksz_device *dev, int port);
 void ksz_init_mib_timer(struct ksz_device *dev);
-- 
Christian Eggers
Embedded software developer

Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH net-next v3 08/12] net: ptp: add helper for one-step P2P clocks
  2020-11-18 20:30 [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x Christian Eggers
                   ` (6 preceding siblings ...)
  2020-11-18 20:30 ` [PATCH net-next v3 07/12] net: dsa: microchip: ksz9477: add Posix clock support for chip PTP clock Christian Eggers
@ 2020-11-18 20:30 ` Christian Eggers
  2020-11-18 20:30 ` [PATCH net-next v3 09/12] net: dsa: microchip: ksz9477: initial hardware time stamping support Christian Eggers
                   ` (4 subsequent siblings)
  12 siblings, 0 replies; 32+ messages in thread
From: Christian Eggers @ 2020-11-18 20:30 UTC (permalink / raw)
  To: Vladimir Oltean, Jakub Kicinski, Andrew Lunn, Richard Cochran,
	Rob Herring
  Cc: Vivien Didelot, David S . Miller, Kurt Kanzenbach,
	George McCollister, Marek Vasut, Helmut Grohne, Paul Barker,
	Codrin Ciubotariu, Tristram Ha, Woojung Huh,
	Microchip Linux Driver Support, Christian Eggers, netdev,
	devicetree, linux-kernel

For P2P delay measurement, the ingress time stamp of the PDelay_Req is
required for the correction field of the PDelay_Resp. The application
echoes back the correction field of the PDelay_Req when sending the
PDelay_Resp.

Some hardware (like the ZHAW InES PTP time stamping IP core) subtracts
the ingress timestamp autonomously from the correction field, so that
the hardware only needs to add the egress timestamp on tx. Other
hardware (like the Microchip KSZ9563) reports the ingress time stamp via
an interrupt and requires that the software provides this time stamp via
tail-tag on tx.

In order to avoid introducing a further application interface for this,
the driver can simply emulate the behavior of the InES device and
subtract the ingress time stamp in software from the correction field.

On egress, the correction field can either be kept as it is (and the
time stamp field in the tail-tag is set to zero) or move the value from
the correction field back to the tail-tag.

Changing the correction field requires updating the UDP checksum (if UDP
is used as transport).

Signed-off-by: Christian Eggers <ceggers@arri.de>
---
 include/linux/ptp_classify.h | 73 ++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/include/linux/ptp_classify.h b/include/linux/ptp_classify.h
index cc0da0b134a4..f19f2f6a9475 100644
--- a/include/linux/ptp_classify.h
+++ b/include/linux/ptp_classify.h
@@ -10,8 +10,12 @@
 #ifndef _PTP_CLASSIFY_H_
 #define _PTP_CLASSIFY_H_
 
+#include <asm/unaligned.h>
 #include <linux/ip.h>
+#include <linux/ktime.h>
 #include <linux/skbuff.h>
+#include <linux/udp.h>
+#include <net/checksum.h>
 
 #define PTP_CLASS_NONE  0x00 /* not a PTP event message */
 #define PTP_CLASS_V1    0x01 /* protocol version 1 */
@@ -123,6 +127,67 @@ static inline u8 ptp_get_msgtype(const struct ptp_header *hdr,
 	return msgtype;
 }
 
+/**
+ * ptp_check_diff8 - Computes new checksum (when altering a 64-bit field)
+ * @old: old field value
+ * @new: new field value
+ * @oldsum: previous checksum
+ *
+ * This function can be used to calculate a new checksum when only a single
+ * field is changed. Similar as ip_vs_check_diff*() in ip_vs.h.
+ *
+ * Return: Updated checksum
+ */
+static inline __wsum ptp_check_diff8(__be64 old, __be64 new, __wsum oldsum)
+{
+	__be64 diff[2] = { ~old, new };
+
+	return csum_partial(diff, sizeof(diff), oldsum);
+}
+
+/**
+ * ptp_header_update_correction - Update PTP header's correction field
+ * @skb: packet buffer
+ * @type: type of the packet (see ptp_classify_raw())
+ * @hdr: ptp header
+ * @correction: new correction value
+ *
+ * This updates the correction field of a PTP header and updates the UDP
+ * checksum (if UDP is used as transport). It is needed for hardware capable of
+ * one-step P2P that does not already modify the correction field of Pdelay_Req
+ * event messages on ingress.
+ */
+static inline
+void ptp_header_update_correction(struct sk_buff *skb, unsigned int type,
+				  struct ptp_header *hdr, s64 correction)
+{
+	__be64 correction_old;
+	struct udphdr *uhdr;
+
+	/* previous correction value is required for checksum update. */
+	memcpy(&correction_old,  &hdr->correction, sizeof(correction_old));
+
+	/* write new correction value */
+	put_unaligned_be64((u64)correction, &hdr->correction);
+
+	switch (type & PTP_CLASS_PMASK) {
+	case PTP_CLASS_IPV4:
+	case PTP_CLASS_IPV6:
+		/* locate udp header */
+		uhdr = (struct udphdr *)((char *)hdr - sizeof(struct udphdr));
+		break;
+	default:
+		return;
+	}
+
+	/* update checksum */
+	uhdr->check = csum_fold(ptp_check_diff8(correction_old,
+						hdr->correction,
+						~csum_unfold(uhdr->check)));
+	if (!uhdr->check)
+		uhdr->check = CSUM_MANGLED_0;
+}
+
 void __init ptp_classifier_init(void);
 #else
 static inline void ptp_classifier_init(void)
@@ -145,5 +210,13 @@ static inline u8 ptp_get_msgtype(const struct ptp_header *hdr,
 	 */
 	return PTP_MSGTYPE_SYNC;
 }
+
+static inline
+void ptp_onestep_p2p_move_t2_to_correction(struct sk_buff *skb,
+					   unsigned int type,
+					   struct ptp_header *hdr,
+					   ktime_t t2)
+{
+}
 #endif
 #endif /* _PTP_CLASSIFY_H_ */
-- 
Christian Eggers
Embedded software developer

Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH net-next v3 09/12] net: dsa: microchip: ksz9477: initial hardware time stamping support
  2020-11-18 20:30 [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x Christian Eggers
                   ` (7 preceding siblings ...)
  2020-11-18 20:30 ` [PATCH net-next v3 08/12] net: ptp: add helper for one-step P2P clocks Christian Eggers
@ 2020-11-18 20:30 ` Christian Eggers
  2020-11-20 23:27   ` Vladimir Oltean
  2020-11-18 20:30 ` [PATCH net-next v3 10/12] net: dsa: microchip: ksz9477: remaining " Christian Eggers
                   ` (3 subsequent siblings)
  12 siblings, 1 reply; 32+ messages in thread
From: Christian Eggers @ 2020-11-18 20:30 UTC (permalink / raw)
  To: Vladimir Oltean, Jakub Kicinski, Andrew Lunn, Richard Cochran,
	Rob Herring
  Cc: Vivien Didelot, David S . Miller, Kurt Kanzenbach,
	George McCollister, Marek Vasut, Helmut Grohne, Paul Barker,
	Codrin Ciubotariu, Tristram Ha, Woojung Huh,
	Microchip Linux Driver Support, Christian Eggers, netdev,
	devicetree, linux-kernel

Add control routines required for TX hardware time stamping.

The KSZ9563 only supports one step time stamping
(HWTSTAMP_TX_ONESTEP_P2P), which requires linuxptp-2.0 or later.

Currently, only P2P delay measurement is supported. See patchwork
discussion and comments in ksz9477_ptp_init() for details:
https://patchwork.ozlabs.org/project/netdev/patch/20201019172435.4416-8-ceggers@arri.de/

Signed-off-by: Christian Eggers <ceggers@arri.de>
---
 drivers/net/dsa/microchip/ksz9477_main.c |   6 +
 drivers/net/dsa/microchip/ksz9477_ptp.c  | 187 +++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz9477_ptp.h  |  22 +++
 drivers/net/dsa/microchip/ksz_common.h   |   4 +
 4 files changed, 219 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz9477_main.c b/drivers/net/dsa/microchip/ksz9477_main.c
index d1a2ad4a34f1..830efdaef9dc 100644
--- a/drivers/net/dsa/microchip/ksz9477_main.c
+++ b/drivers/net/dsa/microchip/ksz9477_main.c
@@ -1389,6 +1389,7 @@ static const struct dsa_switch_ops ksz9477_switch_ops = {
 	.phy_read		= ksz9477_phy_read16,
 	.phy_write		= ksz9477_phy_write16,
 	.phylink_mac_link_down	= ksz_mac_link_down,
+	.get_ts_info		= ksz9477_ptp_get_ts_info,
 	.port_enable		= ksz_enable_port,
 	.get_strings		= ksz9477_get_strings,
 	.get_ethtool_stats	= ksz_get_ethtool_stats,
@@ -1409,6 +1410,11 @@ static const struct dsa_switch_ops ksz9477_switch_ops = {
 	.port_mdb_del           = ksz9477_port_mdb_del,
 	.port_mirror_add	= ksz9477_port_mirror_add,
 	.port_mirror_del	= ksz9477_port_mirror_del,
+	.port_hwtstamp_get      = ksz9477_ptp_port_hwtstamp_get,
+	.port_hwtstamp_set      = ksz9477_ptp_port_hwtstamp_set,
+	.port_txtstamp          = NULL,
+	/* never defer rx delivery, tstamping is done via tail tagging */
+	.port_rxtstamp          = NULL,
 };
 
 static u32 ksz9477_get_port_addr(int port, int offset)
diff --git a/drivers/net/dsa/microchip/ksz9477_ptp.c b/drivers/net/dsa/microchip/ksz9477_ptp.c
index 0ffc4504a290..f411e5cb88a5 100644
--- a/drivers/net/dsa/microchip/ksz9477_ptp.c
+++ b/drivers/net/dsa/microchip/ksz9477_ptp.c
@@ -218,6 +218,18 @@ static int ksz9477_ptp_enable(struct ptp_clock_info *ptp,
 	return -EOPNOTSUPP;
 }
 
+static long ksz9477_ptp_do_aux_work(struct ptp_clock_info *ptp)
+{
+	struct ksz_device *dev = container_of(ptp, struct ksz_device, ptp_caps);
+	struct timespec64 ts;
+
+	mutex_lock(&dev->ptp_mutex);
+	_ksz9477_ptp_gettime(dev, &ts);
+	mutex_unlock(&dev->ptp_mutex);
+
+	return HZ;  /* reschedule in 1 second */
+}
+
 static int ksz9477_ptp_start_clock(struct ksz_device *dev)
 {
 	u16 data;
@@ -257,6 +269,54 @@ static int ksz9477_ptp_stop_clock(struct ksz_device *dev)
 	return ksz_write16(dev, REG_PTP_CLK_CTRL, data);
 }
 
+/* device attributes */
+
+enum ksz9477_ptp_tcmode {
+	KSZ9477_PTP_TCMODE_E2E,
+	KSZ9477_PTP_TCMODE_P2P,
+};
+
+static int ksz9477_ptp_tcmode_set(struct ksz_device *dev,
+				  enum ksz9477_ptp_tcmode tcmode)
+{
+	u16 data;
+	int ret;
+
+	ret = ksz_read16(dev, REG_PTP_MSG_CONF1, &data);
+	if (ret)
+		return ret;
+
+	if (tcmode == KSZ9477_PTP_TCMODE_P2P)
+		data |= PTP_TC_P2P;
+	else
+		data &= ~PTP_TC_P2P;
+
+	return ksz_write16(dev, REG_PTP_MSG_CONF1, data);
+}
+
+enum ksz9477_ptp_ocmode {
+	KSZ9477_PTP_OCMODE_SLAVE,
+	KSZ9477_PTP_OCMODE_MASTER,
+};
+
+static int ksz9477_ptp_ocmode_set(struct ksz_device *dev,
+				  enum ksz9477_ptp_ocmode ocmode)
+{
+	u16 data;
+	int ret;
+
+	ret = ksz_read16(dev, REG_PTP_MSG_CONF1, &data);
+	if (ret)
+		return ret;
+
+	if (ocmode == KSZ9477_PTP_OCMODE_MASTER)
+		data |= PTP_MASTER;
+	else
+		data &= ~PTP_MASTER;
+
+	return ksz_write16(dev, REG_PTP_MSG_CONF1, data);
+}
+
 int ksz9477_ptp_init(struct ksz_device *dev)
 {
 	int ret;
@@ -282,6 +342,7 @@ int ksz9477_ptp_init(struct ksz_device *dev)
 	dev->ptp_caps.gettime64   = ksz9477_ptp_gettime;
 	dev->ptp_caps.settime64   = ksz9477_ptp_settime;
 	dev->ptp_caps.enable      = ksz9477_ptp_enable;
+	dev->ptp_caps.do_aux_work = ksz9477_ptp_do_aux_work;
 
 	/* Start hardware counter (will overflow after 136 years) */
 	ret = ksz9477_ptp_start_clock(dev);
@@ -294,8 +355,31 @@ int ksz9477_ptp_init(struct ksz_device *dev)
 		goto error_stop_clock;
 	}
 
+	/* Currently, only P2P delay measurement is supported.  Setting ocmode
+	 * to slave will work independently of actually being master or slave.
+	 * For E2E delay measurement, switching between master and slave would
+	 * be required, as the KSZ devices filters out PTP messages depending on
+	 * the ocmode setting:
+	 * - in slave mode, DelayReq messages are filtered out
+	 * - in master mode, Sync messages are filtered out
+	 * Currently (and probably also in future) there is no interface in the
+	 * kernel which allows switching between master and slave mode.  For
+	 * this reason, E2E cannot be supported. See patchwork for full
+	 * discussion:
+	 * https://patchwork.ozlabs.org/project/netdev/patch/20201019172435.4416-8-ceggers@arri.de/
+	 */
+	ksz9477_ptp_tcmode_set(dev, KSZ9477_PTP_TCMODE_P2P);
+	ksz9477_ptp_ocmode_set(dev, KSZ9477_PTP_OCMODE_SLAVE);
+
+	/* Schedule cyclic call of ksz_ptp_do_aux_work() */
+	ret = ptp_schedule_worker(dev->ptp_clock, 0);
+	if (ret)
+		goto error_unregister_clock;
+
 	return 0;
 
+error_unregister_clock:
+	ptp_clock_unregister(dev->ptp_clock);
 error_stop_clock:
 	ksz9477_ptp_stop_clock(dev);
 	return ret;
@@ -306,3 +390,106 @@ void ksz9477_ptp_deinit(struct ksz_device *dev)
 	ptp_clock_unregister(dev->ptp_clock);
 	ksz9477_ptp_stop_clock(dev);
 }
+
+/* DSA PTP operations */
+
+int ksz9477_ptp_get_ts_info(struct dsa_switch *ds, int port,
+			    struct ethtool_ts_info *ts)
+{
+	struct ksz_device *dev = ds->priv;
+
+	ts->so_timestamping = SOF_TIMESTAMPING_TX_HARDWARE |
+			      SOF_TIMESTAMPING_RX_HARDWARE |
+			      SOF_TIMESTAMPING_RAW_HARDWARE;
+
+	ts->phc_index = ptp_clock_index(dev->ptp_clock);
+
+	ts->tx_types = BIT(HWTSTAMP_TX_OFF) |
+		       BIT(HWTSTAMP_TX_ONESTEP_P2P);
+
+	ts->rx_filters = BIT(HWTSTAMP_FILTER_NONE) |
+			 BIT(HWTSTAMP_FILTER_PTP_V2_L4_EVENT) |
+			 BIT(HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
+			 BIT(HWTSTAMP_FILTER_PTP_V2_EVENT);
+
+	return 0;
+}
+
+static int ksz9477_set_hwtstamp_config(struct ksz_device *dev, int port,
+				       struct hwtstamp_config *config)
+{
+	struct ksz_port *prt = &dev->ports[port];
+
+	/* reserved for future extensions */
+	if (config->flags)
+		return -EINVAL;
+
+	switch (config->tx_type) {
+	case HWTSTAMP_TX_OFF:
+		prt->hwts_tx_en = false;
+		break;
+	case HWTSTAMP_TX_ONESTEP_P2P:
+		prt->hwts_tx_en = true;
+		break;
+	default:
+		return -ERANGE;
+	}
+
+	switch (config->rx_filter) {
+	case HWTSTAMP_FILTER_NONE:
+		break;
+	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
+		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L4_EVENT;
+		break;
+	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
+		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
+		break;
+	case HWTSTAMP_FILTER_PTP_V2_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_SYNC:
+		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
+		break;
+	case HWTSTAMP_FILTER_ALL:
+	default:
+		config->rx_filter = HWTSTAMP_FILTER_NONE;
+		return -ERANGE;
+	}
+
+	return 0;
+}
+
+int ksz9477_ptp_port_hwtstamp_get(struct dsa_switch *ds, int port,
+				  struct ifreq *ifr)
+{
+	struct ksz_device *dev = ds->priv;
+	unsigned long bytes_copied;
+
+	bytes_copied = copy_to_user(ifr->ifr_data,
+				    &dev->ports[port].tstamp_config,
+				    sizeof(dev->ports[port].tstamp_config));
+
+	return bytes_copied ? -EFAULT : 0;
+}
+
+int ksz9477_ptp_port_hwtstamp_set(struct dsa_switch *ds, int port,
+				  struct ifreq *ifr)
+{
+	struct ksz_device *dev = ds->priv;
+	struct hwtstamp_config config;
+	unsigned long bytes_copied;
+	int err;
+
+	if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
+		return -EFAULT;
+
+	err = ksz9477_set_hwtstamp_config(dev, port, &config);
+	if (err)
+		return err;
+
+	/* Save the chosen configuration to be returned later. */
+	memcpy(&dev->ports[port].tstamp_config, &config, sizeof(config));
+	bytes_copied = copy_to_user(ifr->ifr_data, &config, sizeof(config));
+
+	return bytes_copied ? -EFAULT : 0;
+}
diff --git a/drivers/net/dsa/microchip/ksz9477_ptp.h b/drivers/net/dsa/microchip/ksz9477_ptp.h
index 0076538419fa..2fd58a981ec5 100644
--- a/drivers/net/dsa/microchip/ksz9477_ptp.h
+++ b/drivers/net/dsa/microchip/ksz9477_ptp.h
@@ -10,6 +10,8 @@
 #ifndef DRIVERS_NET_DSA_MICROCHIP_KSZ9477_PTP_H_
 #define DRIVERS_NET_DSA_MICROCHIP_KSZ9477_PTP_H_
 
+#include <linux/types.h>
+
 #include "ksz_common.h"
 
 #if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ9477_PTP)
@@ -17,11 +19,31 @@
 int ksz9477_ptp_init(struct ksz_device *dev);
 void ksz9477_ptp_deinit(struct ksz_device *dev);
 
+int ksz9477_ptp_get_ts_info(struct dsa_switch *ds, int port,
+			    struct ethtool_ts_info *ts);
+int ksz9477_ptp_port_hwtstamp_get(struct dsa_switch *ds, int port,
+				  struct ifreq *ifr);
+int ksz9477_ptp_port_hwtstamp_set(struct dsa_switch *ds, int port,
+				  struct ifreq *ifr);
+
 #else
 
 static inline int ksz9477_ptp_init(struct ksz_device *dev) { return 0; }
 static inline void ksz9477_ptp_deinit(struct ksz_device *dev) {}
 
+static inline int ksz9477_ptp_get_ts_info(struct dsa_switch *ds, int port,
+					  struct ethtool_ts_info *ts)
+{ return -EOPNOTSUPP; }
+
+static inline int ksz9477_ptp_port_hwtstamp_get(struct dsa_switch *ds, int port,
+						struct ifreq *ifr)
+{ return -EOPNOTSUPP; }
+
+static inline int ksz9477_ptp_port_hwtstamp_set(struct dsa_switch *ds, int port,
+						struct ifreq *ifr)
+{ return -EOPNOTSUPP; }
+
+
 #endif
 
 #endif /* DRIVERS_NET_DSA_MICROCHIP_KSZ9477_PTP_H_ */
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 43dd66009482..139e9b84290b 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -41,6 +41,10 @@ struct ksz_port {
 
 	struct ksz_port_mib mib;
 	phy_interface_t interface;
+#if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ9477_PTP)
+	struct hwtstamp_config tstamp_config;
+	bool hwts_tx_en;
+#endif
 };
 
 struct ksz_device {
-- 
Christian Eggers
Embedded software developer

Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH net-next v3 10/12] net: dsa: microchip: ksz9477: remaining hardware time stamping support
  2020-11-18 20:30 [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x Christian Eggers
                   ` (8 preceding siblings ...)
  2020-11-18 20:30 ` [PATCH net-next v3 09/12] net: dsa: microchip: ksz9477: initial hardware time stamping support Christian Eggers
@ 2020-11-18 20:30 ` Christian Eggers
  2020-11-18 20:30 ` [PATCH net-next v3 11/12] net: dsa: microchip: ksz9477: add Pulse Per Second (PPS) support Christian Eggers
                   ` (2 subsequent siblings)
  12 siblings, 0 replies; 32+ messages in thread
From: Christian Eggers @ 2020-11-18 20:30 UTC (permalink / raw)
  To: Vladimir Oltean, Jakub Kicinski, Andrew Lunn, Richard Cochran,
	Rob Herring
  Cc: Vivien Didelot, David S . Miller, Kurt Kanzenbach,
	George McCollister, Marek Vasut, Helmut Grohne, Paul Barker,
	Codrin Ciubotariu, Tristram Ha, Woojung Huh,
	Microchip Linux Driver Support, Christian Eggers, netdev,
	devicetree, linux-kernel

Add data path routines required for TX hardware time stamping.

PTP mode is permanently enabled (changes tail tag; depends on
CONFIG_NET_DSA_MICROCHIP_KSZ9477_PTP). TX time stamps are reported via
an interrupt / device registers whilst RX time stamps are reported via
an additional tail tag.

One step TX time stamping of PDelay_Resp requires the RX time stamp from
the associated PDelay_Req message. The user space PTP stack assumes that
the RX time stamp has already been subtracted from the PDelay_Req
correction field (as done by the ZHAW InES PTP time stamping core). It
will echo back the value of the correction field in the PDelay_Resp
message.

In order to be compatible to this already established interface, the
KSZ9563 code emulates this behavior. When processing the PDelay_Resp
message, the time stamp is moved back from the correction field to the
tail tag, as the hardware generates an invalid UDP checksum if this
field is negative.

Of course, the UDP checksums (if any) have to be corrected after this
(for both directions).

Everything has been tested on a Microchip KSZ9563 switch.

Signed-off-by: Christian Eggers <ceggers@arri.de>
---
 drivers/net/dsa/microchip/ksz9477_main.c |  12 +-
 drivers/net/dsa/microchip/ksz9477_ptp.c  | 343 ++++++++++++++++++++++-
 drivers/net/dsa/microchip/ksz9477_ptp.h  |  13 +
 drivers/net/dsa/microchip/ksz_common.h   |   8 +
 include/linux/dsa/ksz_common.h           |  69 +++++
 net/dsa/tag_ksz.c                        | 217 +++++++++++++-
 6 files changed, 648 insertions(+), 14 deletions(-)
 create mode 100644 include/linux/dsa/ksz_common.h

diff --git a/drivers/net/dsa/microchip/ksz9477_main.c b/drivers/net/dsa/microchip/ksz9477_main.c
index 830efdaef9dc..4d4d44fcec92 100644
--- a/drivers/net/dsa/microchip/ksz9477_main.c
+++ b/drivers/net/dsa/microchip/ksz9477_main.c
@@ -1412,7 +1412,7 @@ static const struct dsa_switch_ops ksz9477_switch_ops = {
 	.port_mirror_del	= ksz9477_port_mirror_del,
 	.port_hwtstamp_get      = ksz9477_ptp_port_hwtstamp_get,
 	.port_hwtstamp_set      = ksz9477_ptp_port_hwtstamp_set,
-	.port_txtstamp          = NULL,
+	.port_txtstamp          = ksz9477_ptp_port_txtstamp,
 	/* never defer rx delivery, tstamping is done via tail tagging */
 	.port_rxtstamp          = NULL,
 };
@@ -1541,6 +1541,7 @@ static const struct ksz_chip_data ksz9477_switch_chips[] = {
 static irqreturn_t ksz9477_switch_irq_thread(int irq, void *dev_id)
 {
 	struct ksz_device *dev = dev_id;
+	irqreturn_t result = IRQ_NONE;
 	u32 data;
 	int port;
 	int ret;
@@ -1560,12 +1561,15 @@ static irqreturn_t ksz9477_switch_irq_thread(int irq, void *dev_id)
 		ret = ksz_read8(dev, PORT_CTRL_ADDR(port, REG_PORT_INT_STATUS),
 				&data8);
 		if (ret)
-			return IRQ_NONE;
+			return result;
 
-		/* ToDo: Add specific handling of port interrupts */
+		if (data8 & PORT_PTP_INT) {
+			if (ksz9477_ptp_port_interrupt(dev, port) != IRQ_NONE)
+				result = IRQ_HANDLED;
+		}
 	}
 
-	return IRQ_NONE;
+	return result;
 }
 
 static int ksz9477_enable_port_interrupts(struct ksz_device *dev, bool enable)
diff --git a/drivers/net/dsa/microchip/ksz9477_ptp.c b/drivers/net/dsa/microchip/ksz9477_ptp.c
index f411e5cb88a5..f998eb5d8dc4 100644
--- a/drivers/net/dsa/microchip/ksz9477_ptp.c
+++ b/drivers/net/dsa/microchip/ksz9477_ptp.c
@@ -6,7 +6,10 @@
  * Copyright (c) 2020 ARRI Lighting
  */
 
+#include <linux/net_tstamp.h>
+#include <linux/ptp_classify.h>
 #include <linux/ptp_clock_kernel.h>
+#include <linux/sysfs.h>
 
 #include "ksz_common.h"
 #include "ksz9477_reg.h"
@@ -76,6 +79,8 @@ static int ksz9477_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
 static int ksz9477_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
 {
 	struct ksz_device *dev = container_of(ptp, struct ksz_device, ptp_caps);
+	struct ksz_device_ptp_shared *ptp_shared = &dev->ptp_shared;
+	struct timespec64 delta64 = ns_to_timespec64(delta);
 	s32 sec, nsec;
 	u16 data16;
 	int ret;
@@ -110,6 +115,11 @@ static int ksz9477_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
 	if (ret)
 		goto error_return;
 
+	spin_lock_bh(&ptp_shared->ptp_clock_lock);
+	ptp_shared->ptp_clock_time = timespec64_add(ptp_shared->ptp_clock_time,
+						    delta64);
+	spin_unlock_bh(&ptp_shared->ptp_clock_lock);
+
 error_return:
 	mutex_unlock(&dev->ptp_mutex);
 	return ret;
@@ -168,6 +178,7 @@ static int ksz9477_ptp_settime(struct ptp_clock_info *ptp,
 			       struct timespec64 const *ts)
 {
 	struct ksz_device *dev = container_of(ptp, struct ksz_device, ptp_caps);
+	struct ksz_device_ptp_shared *ptp_shared = &dev->ptp_shared;
 	u16 data16;
 	int ret;
 
@@ -207,6 +218,10 @@ static int ksz9477_ptp_settime(struct ptp_clock_info *ptp,
 	if (ret)
 		goto error_return;
 
+	spin_lock_bh(&ptp_shared->ptp_clock_lock);
+	ptp_shared->ptp_clock_time = *ts;
+	spin_unlock_bh(&ptp_shared->ptp_clock_lock);
+
 error_return:
 	mutex_unlock(&dev->ptp_mutex);
 	return ret;
@@ -221,17 +236,23 @@ static int ksz9477_ptp_enable(struct ptp_clock_info *ptp,
 static long ksz9477_ptp_do_aux_work(struct ptp_clock_info *ptp)
 {
 	struct ksz_device *dev = container_of(ptp, struct ksz_device, ptp_caps);
+	struct ksz_device_ptp_shared *ptp_shared = &dev->ptp_shared;
 	struct timespec64 ts;
 
 	mutex_lock(&dev->ptp_mutex);
 	_ksz9477_ptp_gettime(dev, &ts);
 	mutex_unlock(&dev->ptp_mutex);
 
+	spin_lock_bh(&ptp_shared->ptp_clock_lock);
+	ptp_shared->ptp_clock_time = ts;
+	spin_unlock_bh(&ptp_shared->ptp_clock_lock);
+
 	return HZ;  /* reschedule in 1 second */
 }
 
 static int ksz9477_ptp_start_clock(struct ksz_device *dev)
 {
+	struct ksz_device_ptp_shared *ptp_shared = &dev->ptp_shared;
 	u16 data;
 	int ret;
 
@@ -252,6 +273,11 @@ static int ksz9477_ptp_start_clock(struct ksz_device *dev)
 	if (ret)
 		return ret;
 
+	spin_lock_bh(&ptp_shared->ptp_clock_lock);
+	ptp_shared->ptp_clock_time.tv_sec = 0;
+	ptp_shared->ptp_clock_time.tv_nsec = 0;
+	spin_unlock_bh(&ptp_shared->ptp_clock_lock);
+
 	return 0;
 }
 
@@ -269,6 +295,215 @@ static int ksz9477_ptp_stop_clock(struct ksz_device *dev)
 	return ksz_write16(dev, REG_PTP_CLK_CTRL, data);
 }
 
+/* Time stamping support */
+
+static int ksz9477_ptp_enable_mode(struct ksz_device *dev, bool enable)
+{
+	u16 data;
+	int ret;
+
+	ret = ksz_read16(dev, REG_PTP_MSG_CONF1, &data);
+	if (ret)
+		return ret;
+
+	if (enable)
+		data |= PTP_ENABLE;
+	else
+		data &= ~PTP_ENABLE;
+
+	return ksz_write16(dev, REG_PTP_MSG_CONF1, data);
+}
+
+static int ksz9477_ptp_enable_port_ptp_interrupts(struct ksz_device *dev,
+						  int port, bool enable)
+{
+	u32 addr = PORT_CTRL_ADDR(port, REG_PORT_INT_MASK);
+	u8 data;
+	int ret;
+
+	ret = ksz_read8(dev, addr, &data);
+	if (ret)
+		return ret;
+
+	/* PORT_PTP_INT bit is low active */
+	if (enable)
+		data &= ~PORT_PTP_INT;
+	else
+		data |= PORT_PTP_INT;
+
+	return ksz_write8(dev, addr, data);
+}
+
+static int ksz9477_ptp_enable_port_egress_interrupts(struct ksz_device *dev,
+						     int port, bool enable)
+{
+	u32 addr = PORT_CTRL_ADDR(port, REG_PTP_PORT_TX_INT_ENABLE__2);
+	u16 data;
+	int ret;
+
+	ret = ksz_read16(dev, addr, &data);
+	if (ret)
+		return ret;
+
+	/* PTP_PORT_XDELAY_REQ_INT is high active */
+	if (enable)
+		data |= PTP_PORT_XDELAY_REQ_INT;
+	else
+		data &= PTP_PORT_XDELAY_REQ_INT;
+
+	return ksz_write16(dev, addr, data);
+}
+
+static void ksz9477_ptp_txtstamp_skb(struct ksz_device *dev,
+				     struct ksz_port *prt, struct sk_buff *skb)
+{
+	struct skb_shared_hwtstamps hwtstamps = {};
+	int ret;
+
+	skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
+
+	/* timeout must include tstamp latency, IRQ latency and time for
+	 * reading the time stamp via I2C.
+	 */
+	ret = wait_for_completion_timeout(&prt->tstamp_completion,
+					  msecs_to_jiffies(100));
+	if (!ret) {
+		dev_err(dev->dev, "timeout waiting for time stamp\n");
+		return;
+	}
+	hwtstamps.hwtstamp = prt->tstamp_xdelay;
+	skb_complete_tx_timestamp(skb, &hwtstamps);
+}
+
+#define work_to_port(work) \
+		container_of((work), struct ksz_port_ptp_shared, xmit_work)
+#define ptp_shared_to_ksz_port(t) \
+		container_of((t), struct ksz_port, ptp_shared)
+#define ptp_shared_to_ksz_device(t) \
+		container_of((t), struct ksz_device, ptp_shared)
+
+/* Deferred work is necessary for time stamped PDelay_Req messages. This cannot
+ * be done from atomic context as we have to wait for the hardware interrupt.
+ */
+static void ksz9477_port_deferred_xmit(struct kthread_work *work)
+{
+	struct ksz_port_ptp_shared *prt_ptp_shared = work_to_port(work);
+	struct ksz_port *prt = ptp_shared_to_ksz_port(prt_ptp_shared);
+	struct ksz_device_ptp_shared *ptp_shared = prt_ptp_shared->dev;
+	struct ksz_device *dev = ptp_shared_to_ksz_device(ptp_shared);
+	int port = prt - dev->ports;
+	struct sk_buff *skb;
+
+	while ((skb = skb_dequeue(&prt_ptp_shared->xmit_queue)) != NULL) {
+		struct sk_buff *clone = DSA_SKB_CB(skb)->clone;
+
+		reinit_completion(&prt->tstamp_completion);
+
+		/* Transfer skb to the host port. */
+		dsa_enqueue_skb(skb, dsa_to_port(dev->ds, port)->slave);
+
+		ksz9477_ptp_txtstamp_skb(dev, prt, clone);
+	}
+}
+
+static int ksz9477_ptp_port_init(struct ksz_device *dev, int port)
+{
+	struct ksz_port *prt = &dev->ports[port];
+	struct ksz_port_ptp_shared *ptp_shared = &prt->ptp_shared;
+	struct dsa_port *dp = dsa_to_port(dev->ds, port);
+	int ret;
+
+	/* Read rx and tx delay from port registers */
+	ret = ksz_read16(dev, PORT_CTRL_ADDR(port, REG_PTP_PORT_RX_DELAY__2),
+			 &ptp_shared->tstamp_rx_latency_ns);
+	if (ret)
+		return ret;
+
+	ret = ksz_read16(dev, PORT_CTRL_ADDR(port, REG_PTP_PORT_TX_DELAY__2),
+			 &prt->tstamp_tx_latency_ns);
+	if (ret)
+		return ret;
+
+	if (port == dev->cpu_port)
+		return 0;
+
+	ret = ksz9477_ptp_enable_port_ptp_interrupts(dev, port, true);
+	if (ret)
+		return ret;
+
+	ret = ksz9477_ptp_enable_port_egress_interrupts(dev, port, true);
+	if (ret)
+		goto error_disable_port_ptp_interrupts;
+
+	/* ksz_port::ptp_shared is used in tagging driver */
+	ptp_shared->dev = &dev->ptp_shared;
+	dp->priv = ptp_shared;
+
+	/* PDelay_Req messages require deferred transmit as the time
+	 * stamp unit provides no sequenceId or similar.  So we must
+	 * wait for the time stamp interrupt.
+	 */
+	init_completion(&prt->tstamp_completion);
+	kthread_init_work(&ptp_shared->xmit_work,
+			  ksz9477_port_deferred_xmit);
+	ptp_shared->xmit_worker = kthread_create_worker(0, "%s_xmit",
+							dp->slave->name);
+	if (IS_ERR(ptp_shared->xmit_worker)) {
+		ret = PTR_ERR(ptp_shared->xmit_worker);
+		dev_err(dev->dev,
+			"failed to create deferred xmit thread: %d\n", ret);
+		goto error_disable_port_egress_interrupts;
+	}
+	skb_queue_head_init(&ptp_shared->xmit_queue);
+
+	return 0;
+
+error_disable_port_egress_interrupts:
+	ksz9477_ptp_enable_port_egress_interrupts(dev, port, false);
+error_disable_port_ptp_interrupts:
+	ksz9477_ptp_enable_port_ptp_interrupts(dev, port, false);
+	return ret;
+}
+
+static void ksz9477_ptp_port_deinit(struct ksz_device *dev, int port)
+{
+	struct ksz_port_ptp_shared *ptp_shared = &dev->ports[port].ptp_shared;
+
+	if (port == dev->cpu_port)
+		return;
+
+	kthread_destroy_worker(ptp_shared->xmit_worker);
+	ksz9477_ptp_enable_port_egress_interrupts(dev, port, false);
+	ksz9477_ptp_enable_port_ptp_interrupts(dev, port, false);
+}
+
+static int ksz9477_ptp_ports_init(struct ksz_device *dev)
+{
+	int port;
+	int ret;
+
+	for (port = 0; port < dev->port_cnt; port++) {
+		ret = ksz9477_ptp_port_init(dev, port);
+		if (ret)
+			goto error_deinit;
+	}
+
+	return 0;
+
+error_deinit:
+	while (port-- > 0)
+		ksz9477_ptp_port_deinit(dev, port);
+	return ret;
+}
+
+static void ksz9477_ptp_ports_deinit(struct ksz_device *dev)
+{
+	int port;
+
+	for (port = 0; port < dev->port_cnt; port++)
+		ksz9477_ptp_port_deinit(dev, port);
+}
+
 /* device attributes */
 
 enum ksz9477_ptp_tcmode {
@@ -322,6 +557,7 @@ int ksz9477_ptp_init(struct ksz_device *dev)
 	int ret;
 
 	mutex_init(&dev->ptp_mutex);
+	spin_lock_init(&dev->ptp_shared.ptp_clock_lock);
 
 	/* PTP clock properties */
 
@@ -355,6 +591,16 @@ int ksz9477_ptp_init(struct ksz_device *dev)
 		goto error_stop_clock;
 	}
 
+	/* Enable PTP mode (will affect tail tagging format) */
+	ret = ksz9477_ptp_enable_mode(dev, true);
+	if (ret)
+		goto error_unregister_clock;
+
+	/* Init switch ports */
+	ret = ksz9477_ptp_ports_init(dev);
+	if (ret)
+		goto error_disable_mode;
+
 	/* Currently, only P2P delay measurement is supported.  Setting ocmode
 	 * to slave will work independently of actually being master or slave.
 	 * For E2E delay measurement, switching between master and slave would
@@ -374,10 +620,14 @@ int ksz9477_ptp_init(struct ksz_device *dev)
 	/* Schedule cyclic call of ksz_ptp_do_aux_work() */
 	ret = ptp_schedule_worker(dev->ptp_clock, 0);
 	if (ret)
-		goto error_unregister_clock;
+		goto error_ports_deinit;
 
 	return 0;
 
+error_ports_deinit:
+	ksz9477_ptp_ports_deinit(dev);
+error_disable_mode:
+	ksz9477_ptp_enable_mode(dev, false);
 error_unregister_clock:
 	ptp_clock_unregister(dev->ptp_clock);
 error_stop_clock:
@@ -387,10 +637,54 @@ int ksz9477_ptp_init(struct ksz_device *dev)
 
 void ksz9477_ptp_deinit(struct ksz_device *dev)
 {
+	ksz9477_ptp_ports_deinit(dev);
+	ksz9477_ptp_enable_mode(dev, false);
 	ptp_clock_unregister(dev->ptp_clock);
 	ksz9477_ptp_stop_clock(dev);
 }
 
+irqreturn_t ksz9477_ptp_port_interrupt(struct ksz_device *dev, int port)
+{
+	u32 addr = PORT_CTRL_ADDR(port, REG_PTP_PORT_TX_INT_STATUS__2);
+	struct ksz_port *prt = &dev->ports[port];
+	u16 data;
+	int ret;
+
+	ret = ksz_read16(dev, addr, &data);
+	if (ret)
+		return IRQ_NONE;
+
+	if (data & PTP_PORT_XDELAY_REQ_INT) {
+		/* Timestamp for Pdelay_Req / Delay_Req */
+		struct ksz_device_ptp_shared *ptp_shared = &dev->ptp_shared;
+		u32 tstamp_raw;
+		ktime_t tstamp;
+
+		/* In contrast to the KSZ9563R data sheet, the format of the
+		 * port time stamp registers is also 2 bit seconds + 30 bit
+		 * nanoseconds (same as in the tail tags).
+		 */
+		ret = ksz_read32(dev,
+				 PORT_CTRL_ADDR(port, REG_PTP_PORT_XDELAY_TS),
+				 &tstamp_raw);
+		if (ret)
+			return IRQ_NONE;
+
+		tstamp = ksz9477_decode_tstamp(tstamp_raw,
+					       prt->tstamp_tx_latency_ns);
+		prt->tstamp_xdelay = ksz9477_tstamp_reconstruct(ptp_shared,
+								tstamp);
+		complete(&prt->tstamp_completion);
+	}
+
+	/* Clear interrupt(s) (W1C) */
+	ret = ksz_write16(dev, addr, data);
+	if (ret)
+		return IRQ_NONE;
+
+	return IRQ_HANDLED;
+}
+
 /* DSA PTP operations */
 
 int ksz9477_ptp_get_ts_info(struct dsa_switch *ds, int port,
@@ -493,3 +787,50 @@ int ksz9477_ptp_port_hwtstamp_set(struct dsa_switch *ds, int port,
 
 	return bytes_copied ? -EFAULT : 0;
 }
+
+bool ksz9477_ptp_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *clone,
+			       unsigned int type)
+{
+	struct ksz_device *dev = ds->priv;
+	struct ksz_port *prt = &dev->ports[port];
+	struct ptp_header *hdr;
+	u8 ptp_msg_type;
+
+	/* Should already been tested in dsa_skb_tx_timestamp()? */
+	if (!(skb_shinfo(clone)->tx_flags & SKBTX_HW_TSTAMP))
+		return false;
+
+	if (!prt->hwts_tx_en)
+		return false;
+
+	hdr = ptp_parse_header(clone, type);
+	if (!hdr)
+		return false;
+
+	ptp_msg_type = ptp_get_msgtype(hdr, type);
+	switch (ptp_msg_type) {
+	/* As the KSZ9563 always performs one step time stamping, only the time
+	 * stamp for Pdelay_Req is reported to the application via socket error
+	 * queue.  Time stamps for Sync and Pdelay_resp will be applied directly
+	 * to the outgoing message (e.g. correction field), but will NOT be
+	 * reported to the socket.
+	 * Delay_Req is not time stamped as E2E is currently not supported by
+	 * this driver.  See ksz9477_ptp_init() for details.
+	 */
+	case PTP_MSGTYPE_PDELAY_REQ:
+	case PTP_MSGTYPE_PDELAY_RESP:
+		break;
+	default:
+		return false;
+	}
+
+	/* ptp_type will be reused in ksz9477_xmit_timestamp(). ptp_msg_type
+	 * will be reused in ksz9477_defer_xmit(). For PDelay_Resp, the cloned
+	 * skb will not be passed to skb_complete_tx_timestamp() and has to be
+	 * freed manually in ksz9477_defer_xmit().
+	 */
+	KSZ9477_SKB_CB(clone)->ptp_type = type;
+	KSZ9477_SKB_CB(clone)->ptp_msg_type = ptp_msg_type;
+
+	return true;  /* keep cloned skb */
+}
diff --git a/drivers/net/dsa/microchip/ksz9477_ptp.h b/drivers/net/dsa/microchip/ksz9477_ptp.h
index 2fd58a981ec5..2f7c4fa0753a 100644
--- a/drivers/net/dsa/microchip/ksz9477_ptp.h
+++ b/drivers/net/dsa/microchip/ksz9477_ptp.h
@@ -10,6 +10,7 @@
 #ifndef DRIVERS_NET_DSA_MICROCHIP_KSZ9477_PTP_H_
 #define DRIVERS_NET_DSA_MICROCHIP_KSZ9477_PTP_H_
 
+#include <linux/irqreturn.h>
 #include <linux/types.h>
 
 #include "ksz_common.h"
@@ -19,18 +20,26 @@
 int ksz9477_ptp_init(struct ksz_device *dev);
 void ksz9477_ptp_deinit(struct ksz_device *dev);
 
+irqreturn_t ksz9477_ptp_port_interrupt(struct ksz_device *dev, int port);
+
 int ksz9477_ptp_get_ts_info(struct dsa_switch *ds, int port,
 			    struct ethtool_ts_info *ts);
 int ksz9477_ptp_port_hwtstamp_get(struct dsa_switch *ds, int port,
 				  struct ifreq *ifr);
 int ksz9477_ptp_port_hwtstamp_set(struct dsa_switch *ds, int port,
 				  struct ifreq *ifr);
+bool ksz9477_ptp_port_txtstamp(struct dsa_switch *ds, int port,
+			       struct sk_buff *clone, unsigned int type);
 
 #else
 
 static inline int ksz9477_ptp_init(struct ksz_device *dev) { return 0; }
 static inline void ksz9477_ptp_deinit(struct ksz_device *dev) {}
 
+static inline irqreturn_t ksz9477_ptp_port_interrupt(struct ksz_device *dev,
+						     int port)
+{ return IRQ_NONE; }
+
 static inline int ksz9477_ptp_get_ts_info(struct dsa_switch *ds, int port,
 					  struct ethtool_ts_info *ts)
 { return -EOPNOTSUPP; }
@@ -43,6 +52,10 @@ static inline int ksz9477_ptp_port_hwtstamp_set(struct dsa_switch *ds, int port,
 						struct ifreq *ifr)
 { return -EOPNOTSUPP; }
 
+static inline bool ksz9477_ptp_port_txtstamp(struct dsa_switch *ds, int port,
+					     struct sk_buff *clone,
+					     unsigned int type)
+{ return false; }
 
 #endif
 
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 139e9b84290b..868d0cc9d84f 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -7,8 +7,11 @@
 #ifndef __KSZ_COMMON_H
 #define __KSZ_COMMON_H
 
+#include <linux/completion.h>
+#include <linux/dsa/ksz_common.h>
 #include <linux/etherdevice.h>
 #include <linux/kernel.h>
+#include <linux/ktime.h>
 #include <linux/mutex.h>
 #include <linux/phy.h>
 #include <linux/ptp_clock_kernel.h>
@@ -42,7 +45,11 @@ struct ksz_port {
 	struct ksz_port_mib mib;
 	phy_interface_t interface;
 #if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ9477_PTP)
+	struct ksz_port_ptp_shared ptp_shared;
+	u16 tstamp_tx_latency_ns;   /* tx delay from tstamp unit to wire */
 	struct hwtstamp_config tstamp_config;
+	ktime_t tstamp_xdelay;
+	struct completion tstamp_completion;
 	bool hwts_tx_en;
 #endif
 };
@@ -102,6 +109,7 @@ struct ksz_device {
 	struct ptp_clock *ptp_clock;
 	struct ptp_clock_info ptp_caps;
 	struct mutex ptp_mutex;		/* protects PTP related hardware */
+	struct ksz_device_ptp_shared ptp_shared;
 #endif
 };
 
diff --git a/include/linux/dsa/ksz_common.h b/include/linux/dsa/ksz_common.h
new file mode 100644
index 000000000000..16634ee3489e
--- /dev/null
+++ b/include/linux/dsa/ksz_common.h
@@ -0,0 +1,69 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Routines shared between drivers/net/dsa/microchip/ksz9477_ptp.h and
+ * net/dsa/tag_ksz.c
+ *
+ * Copyright (C) 2020 ARRI Lighting
+ */
+
+#ifndef _NET_DSA_KSZ_COMMON_H_
+#define _NET_DSA_KSZ_COMMON_H_
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/ktime.h>
+#include <linux/kthread.h>
+#include <linux/net_tstamp.h>
+#include <linux/ptp_classify.h>
+#include <linux/skbuff.h>
+#include <linux/spinlock.h>
+#include <linux/time64.h>
+#include <net/dsa.h>
+
+/* All time stamps from the KSZ consist of 2 bits for seconds and 30 bits for
+ * nanoseconds. This is NOT the same as 32 bits for nanoseconds.
+ */
+#define KSZ_TSTAMP_SEC_MASK  GENMASK(31, 30)
+#define KSZ_TSTAMP_NSEC_MASK GENMASK(29, 0)
+
+struct ksz_device_ptp_shared {
+	/* protects ptp_clock_time (user space (various syscalls)
+	 * vs. softirq in ksz9477_rcv_timestamp()).
+	 */
+	spinlock_t ptp_clock_lock;
+	/* approximated current time, read once per second from hardware */
+	struct timespec64 ptp_clock_time;
+};
+
+struct ksz_port_ptp_shared {
+	struct ksz_device_ptp_shared *dev;
+	u16 tstamp_rx_latency_ns;   /* rx delay from wire to tstamp unit */
+	struct kthread_worker *xmit_worker;
+	struct kthread_work xmit_work;
+	struct sk_buff_head xmit_queue;
+};
+
+/* net/dsa/tag_ksz.c */
+static inline ktime_t ksz9477_decode_tstamp(u32 tstamp, int offset_ns)
+{
+	u64 ns = FIELD_GET(KSZ_TSTAMP_SEC_MASK, tstamp) * NSEC_PER_SEC +
+		 FIELD_GET(KSZ_TSTAMP_NSEC_MASK, tstamp);
+
+	/* Add/remove excess delay between wire and time stamp unit */
+	return ns_to_ktime(ns + offset_ns);
+}
+
+ktime_t ksz9477_tstamp_reconstruct(struct ksz_device_ptp_shared *ksz,
+				   ktime_t tstamp);
+
+struct ksz9477_skb_cb {
+	unsigned int ptp_type;
+	/* Do not cache pointer to PTP header between ksz9477_ptp_port_txtstamp
+	 * and ksz9xxx_xmit() (will become invalid during dsa_realloc_skb()).
+	 */
+	u8 ptp_msg_type;
+};
+
+#define KSZ9477_SKB_CB(skb) \
+	((struct ksz9477_skb_cb *)DSA_SKB_CB_PRIV(skb))
+
+#endif /* _NET_DSA_KSZ_COMMON_H_ */
diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
index 4820dbcedfa2..aa87ceac8c56 100644
--- a/net/dsa/tag_ksz.c
+++ b/net/dsa/tag_ksz.c
@@ -4,10 +4,14 @@
  * Copyright (c) 2017 Microchip Technology
  */
 
+#include <asm/unaligned.h>
+#include <linux/dsa/ksz_common.h>
 #include <linux/etherdevice.h>
 #include <linux/list.h>
+#include <linux/ptp_classify.h>
 #include <linux/slab.h>
 #include <net/dsa.h>
+#include <net/checksum.h>
 #include "dsa_priv.h"
 
 /* Typically only one byte is used for tail tag. */
@@ -87,26 +91,217 @@ MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ8795);
 /*
  * For Ingress (Host -> KSZ9477), 2 bytes are added before FCS.
  * ---------------------------------------------------------------------------
- * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|tag0(1byte)|tag1(1byte)|FCS(4bytes)
+ * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|ts(4bytes)|tag0(1byte)|tag1(1byte)|FCS(4bytes)
  * ---------------------------------------------------------------------------
+ * ts   : time stamp (only present if PTP is enabled on the hardware).
  * tag0 : Prioritization (not used now)
  * tag1 : each bit represents port (eg, 0x01=port1, 0x02=port2, 0x10=port5)
  *
- * For Egress (KSZ9477 -> Host), 1 byte is added before FCS.
+ * For Egress (KSZ9477 -> Host), 1/4 bytes are added before FCS.
  * ---------------------------------------------------------------------------
- * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|tag0(1byte)|FCS(4bytes)
+ * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|ts(4bytes)|tag0(1byte)|FCS(4bytes)
  * ---------------------------------------------------------------------------
+ * ts   : time stamp (only present of bit 7 in tag0 is set
  * tag0 : zero-based value represents port
  *	  (eg, 0x00=port1, 0x02=port3, 0x06=port7)
  */
 
 #define KSZ9477_INGRESS_TAG_LEN		2
 #define KSZ9477_PTP_TAG_LEN		4
-#define KSZ9477_PTP_TAG_INDICATION	0x80
+#define KSZ9477_PTP_TAG_INDICATION	BIT(7)
 
 #define KSZ9477_TAIL_TAG_OVERRIDE	BIT(9)
 #define KSZ9477_TAIL_TAG_LOOKUP		BIT(10)
 
+#if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ9477_PTP)
+/* Time stamp tag is only inserted if PTP is enabled in hardware. */
+static void ksz9477_xmit_timestamp(struct sk_buff *skb)
+{
+	struct sk_buff *clone = DSA_SKB_CB(skb)->clone;
+	struct ptp_header *ptp_hdr;
+	unsigned int ptp_type;
+	u32 tstamp_raw = 0;
+	u8 ptp_msg_type;
+	s64 correction;
+
+	if (!clone)
+		goto out_put_tag;
+
+	/* Use cached PTP type from ksz9477_ptp_port_txtstamp().  */
+	ptp_type = KSZ9477_SKB_CB(clone)->ptp_type;
+	if (ptp_type == PTP_CLASS_NONE)
+		goto out_put_tag;
+
+	ptp_hdr = ptp_parse_header(skb, ptp_type);
+	if (!ptp_hdr)
+		goto out_put_tag;
+
+	ptp_msg_type = KSZ9477_SKB_CB(clone)->ptp_msg_type;
+	if (ptp_msg_type != PTP_MSGTYPE_PDELAY_RESP)
+		goto out_put_tag;
+
+	correction = (s64)get_unaligned_be64(&ptp_hdr->correction);
+
+	/* For PDelay_Resp messages we will likely have a negative value in the
+	 * correction field (see ksz9477_rcv()). The switch hardware cannot
+	 * correctly update such values (produces an off by one error in the UDP
+	 * checksum), so it must be moved to the time stamp field in the tail
+	 * tag.
+	 */
+	if (correction < 0) {
+		struct timespec64 ts;
+
+		/* Move ingress time stamp from PTP header's correction field to
+		 * tail tag. Format of the correction filed is 48 bit ns + 16
+		 * bit fractional ns.
+		 */
+		ts = ns_to_timespec64(-correction >> 16);
+		tstamp_raw = ((ts.tv_sec & 3) << 30) | ts.tv_nsec;
+
+		/* Set correction field to 0 and update UDP checksum.  */
+		ptp_header_update_correction(skb, ptp_type, ptp_hdr, 0);
+	}
+
+	/* For PDelay_Resp messages, the clone is not required in
+	 * skb_complete_tx_timestamp() and should be freed here.
+	 */
+	kfree_skb(clone);
+	DSA_SKB_CB(skb)->clone = NULL;
+
+out_put_tag:
+	put_unaligned_be32(tstamp_raw, skb_put(skb, KSZ9477_PTP_TAG_LEN));
+}
+
+/* Defer transmit if waiting for egress time stamp is required.  */
+static struct sk_buff *ksz9477_defer_xmit(struct dsa_port *dp,
+					  struct sk_buff *skb)
+{
+	struct ksz_port_ptp_shared *ptp_shared = dp->priv;
+	struct sk_buff *clone = DSA_SKB_CB(skb)->clone;
+	u8 ptp_msg_type;
+
+	if (!clone)
+		return skb;  /* no deferred xmit for this packet */
+
+	/* Use cached PTP msg type from ksz9477_ptp_port_txtstamp().  */
+	ptp_msg_type = KSZ9477_SKB_CB(clone)->ptp_msg_type;
+	if (ptp_msg_type != PTP_MSGTYPE_PDELAY_REQ)
+		goto out_free_clone;  /* only PDelay_Req is deferred */
+
+	/* Increase refcount so the kfree_skb in dsa_slave_xmit
+	 * won't really free the packet.
+	 */
+	skb_queue_tail(&ptp_shared->xmit_queue, skb_get(skb));
+	kthread_queue_work(ptp_shared->xmit_worker, &ptp_shared->xmit_work);
+
+	return NULL;
+
+out_free_clone:
+	kfree_skb(clone);
+	DSA_SKB_CB(skb)->clone = NULL;
+	return skb;
+}
+
+ktime_t ksz9477_tstamp_reconstruct(struct ksz_device_ptp_shared *ksz,
+				   ktime_t tstamp)
+{
+	struct timespec64 ts = ktime_to_timespec64(tstamp);
+	struct timespec64 ptp_clock_time;
+	struct timespec64 diff;
+
+	spin_lock_bh(&ksz->ptp_clock_lock);
+	ptp_clock_time = ksz->ptp_clock_time;
+	spin_unlock_bh(&ksz->ptp_clock_lock);
+
+	/* calculate full time from partial time stamp */
+	ts.tv_sec = (ptp_clock_time.tv_sec & ~3) | ts.tv_sec;
+
+	/* find nearest possible point in time */
+	diff = timespec64_sub(ts, ptp_clock_time);
+	if (diff.tv_sec > 2)
+		ts.tv_sec -= 4;
+	else if (diff.tv_sec < -2)
+		ts.tv_sec += 4;
+
+	return timespec64_to_ktime(ts);
+}
+EXPORT_SYMBOL(ksz9477_tstamp_reconstruct);
+
+static void ksz9477_rcv_timestamp(struct sk_buff *skb, u8 *tag,
+				  struct net_device *dev, unsigned int port)
+{
+	struct skb_shared_hwtstamps *hwtstamps = skb_hwtstamps(skb);
+	struct dsa_switch *ds = dev->dsa_ptr->ds;
+	struct ksz_port_ptp_shared *port_ptp_shared;
+	u8 *tstamp_raw = tag - KSZ9477_PTP_TAG_LEN;
+	struct ptp_header *ptp_hdr;
+	unsigned int ptp_type;
+	u8 ptp_msg_type;
+	ktime_t tstamp;
+	s64 correction;
+
+	port_ptp_shared = dsa_to_port(ds, port)->priv;
+	if (!port_ptp_shared)
+		return;
+
+	/* convert time stamp and write to skb */
+	tstamp = ksz9477_decode_tstamp(get_unaligned_be32(tstamp_raw),
+				       -port_ptp_shared->tstamp_rx_latency_ns);
+	memset(hwtstamps, 0, sizeof(*hwtstamps));
+	hwtstamps->hwtstamp = ksz9477_tstamp_reconstruct(port_ptp_shared->dev,
+							 tstamp);
+
+	/* For PDelay_Req messages, user space (ptp4l) expects that the hardware
+	 * subtracts the ingress time stamp from the correction field.  The
+	 * separate hw time stamp from the sk_buff struct will not be used in
+	 * this case.
+	 */
+
+	if (skb_headroom(skb) < ETH_HLEN)
+		return;
+
+	__skb_push(skb, ETH_HLEN);
+	ptp_type = ptp_classify_raw(skb);
+	__skb_pull(skb, ETH_HLEN);
+
+	if (ptp_type == PTP_CLASS_NONE)
+		return;
+
+	ptp_hdr = ptp_parse_header(skb, ptp_type);
+	if (!ptp_hdr)
+		return;
+
+	ptp_msg_type = ptp_get_msgtype(ptp_hdr, ptp_type);
+	if (ptp_msg_type != PTP_MSGTYPE_PDELAY_REQ)
+		return;
+
+	/* Only subtract the partial time stamp from the correction field.  When
+	 * the hardware adds the egress time stamp to the correction field of
+	 * the PDelay_Resp message on tx, also only the partial time stamp will
+	 * be added.
+	 */
+	correction = (s64)get_unaligned_be64(&ptp_hdr->correction);
+	correction -= ktime_to_ns(tstamp) << 16;
+
+	ptp_header_update_correction(skb, ptp_type, ptp_hdr, correction);
+}
+#else   /* IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ9477_PTP) */
+static void ksz9477_xmit_timestamp(struct sk_buff *skb)
+{
+}
+
+static struct sk_buff *ksz9477_defer_xmit(struct dsa_port *dp,
+					  struct sk_buff *skb)
+{
+	return skb;  /* no deferred xmit */
+}
+
+static void ksz9477_rcv_timestamp(struct sk_buff *skb, u8 *tag,
+				  struct net_device *dev, unsigned int port)
+{
+}
+#endif  /* IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ9477_PTP) */
+
 static struct sk_buff *ksz9477_xmit(struct sk_buff *skb,
 				    struct net_device *dev)
 {
@@ -116,6 +311,7 @@ static struct sk_buff *ksz9477_xmit(struct sk_buff *skb,
 	u16 val;
 
 	/* Tag encoding */
+	ksz9477_xmit_timestamp(skb);
 	tag = skb_put(skb, KSZ9477_INGRESS_TAG_LEN);
 	addr = skb_mac_header(skb);
 
@@ -126,7 +322,7 @@ static struct sk_buff *ksz9477_xmit(struct sk_buff *skb,
 
 	*tag = cpu_to_be16(val);
 
-	return skb;
+	return ksz9477_defer_xmit(dp, skb);
 }
 
 static struct sk_buff *ksz9477_rcv(struct sk_buff *skb, struct net_device *dev,
@@ -138,8 +334,10 @@ static struct sk_buff *ksz9477_rcv(struct sk_buff *skb, struct net_device *dev,
 	unsigned int len = KSZ_EGRESS_TAG_LEN;
 
 	/* Extra 4-bytes PTP timestamp */
-	if (tag[0] & KSZ9477_PTP_TAG_INDICATION)
+	if (tag[0] & KSZ9477_PTP_TAG_INDICATION) {
+		ksz9477_rcv_timestamp(skb, tag, dev, port);
 		len += KSZ9477_PTP_TAG_LEN;
+	}
 
 	return ksz_common_rcv(skb, dev, port, len);
 }
@@ -149,7 +347,7 @@ static const struct dsa_device_ops ksz9477_netdev_ops = {
 	.proto	= DSA_TAG_PROTO_KSZ9477,
 	.xmit	= ksz9477_xmit,
 	.rcv	= ksz9477_rcv,
-	.overhead = KSZ9477_INGRESS_TAG_LEN,
+	.overhead = KSZ9477_INGRESS_TAG_LEN + KSZ9477_PTP_TAG_LEN,
 	.tail_tag = true,
 };
 
@@ -167,6 +365,7 @@ static struct sk_buff *ksz9893_xmit(struct sk_buff *skb,
 	u8 *tag;
 
 	/* Tag encoding */
+	ksz9477_xmit_timestamp(skb);
 	tag = skb_put(skb, KSZ_INGRESS_TAG_LEN);
 	addr = skb_mac_header(skb);
 
@@ -175,7 +374,7 @@ static struct sk_buff *ksz9893_xmit(struct sk_buff *skb,
 	if (is_link_local_ether_addr(addr))
 		*tag |= KSZ9893_TAIL_TAG_OVERRIDE;
 
-	return skb;
+	return ksz9477_defer_xmit(dp, skb);
 }
 
 static const struct dsa_device_ops ksz9893_netdev_ops = {
@@ -183,7 +382,7 @@ static const struct dsa_device_ops ksz9893_netdev_ops = {
 	.proto	= DSA_TAG_PROTO_KSZ9893,
 	.xmit	= ksz9893_xmit,
 	.rcv	= ksz9477_rcv,
-	.overhead = KSZ_INGRESS_TAG_LEN,
+	.overhead = KSZ_INGRESS_TAG_LEN + KSZ9477_PTP_TAG_LEN,
 	.tail_tag = true,
 };
 
-- 
Christian Eggers
Embedded software developer

Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH net-next v3 11/12] net: dsa: microchip: ksz9477: add Pulse Per Second (PPS) support
  2020-11-18 20:30 [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x Christian Eggers
                   ` (9 preceding siblings ...)
  2020-11-18 20:30 ` [PATCH net-next v3 10/12] net: dsa: microchip: ksz9477: remaining " Christian Eggers
@ 2020-11-18 20:30 ` Christian Eggers
  2020-11-18 20:30 ` [PATCH net-next v3 12/12] net: dsa: microchip: ksz9477: add periodic output support Christian Eggers
  2020-11-18 23:40 ` [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x Vladimir Oltean
  12 siblings, 0 replies; 32+ messages in thread
From: Christian Eggers @ 2020-11-18 20:30 UTC (permalink / raw)
  To: Vladimir Oltean, Jakub Kicinski, Andrew Lunn, Richard Cochran,
	Rob Herring
  Cc: Vivien Didelot, David S . Miller, Kurt Kanzenbach,
	George McCollister, Marek Vasut, Helmut Grohne, Paul Barker,
	Codrin Ciubotariu, Tristram Ha, Woojung Huh,
	Microchip Linux Driver Support, Christian Eggers, netdev,
	devicetree, linux-kernel

The KSZ9563 has a Trigger Output Unit (TOU) which can be used to
generate periodic signals.

After adjusting the PTP clock time, the PPS signal has to be restarted.

Tested on a Microchip KSZ9563 switch.

Signed-off-by: Christian Eggers <ceggers@arri.de>
---
 drivers/net/dsa/microchip/ksz9477_ptp.c | 251 +++++++++++++++++++++++-
 drivers/net/dsa/microchip/ksz9477_ptp.h |   4 +
 drivers/net/dsa/microchip/ksz_common.h  |   6 +
 3 files changed, 259 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477_ptp.c b/drivers/net/dsa/microchip/ksz9477_ptp.c
index f998eb5d8dc4..ce3fdc9a1f9e 100644
--- a/drivers/net/dsa/microchip/ksz9477_ptp.c
+++ b/drivers/net/dsa/microchip/ksz9477_ptp.c
@@ -19,6 +19,126 @@
 #define KSZ_PTP_INC_NS 40  /* HW clock is incremented every 40 ns (by 40) */
 #define KSZ_PTP_SUBNS_BITS 32  /* Number of bits in sub-nanoseconds counter */
 
+/* Shared register access routines (Trigger Output Unit) */
+
+static int ksz9477_ptp_tou_reset(struct ksz_device *dev, unsigned int unit)
+{
+	u32 ctrl_stat, data;
+	int ret;
+
+	/* Reset trigger unit (clears TRIGGER_EN, but not GPIOSTATx) */
+	ret = ksz_read32(dev, REG_PTP_CTRL_STAT__4, &ctrl_stat);
+	if (ret)
+		return ret;
+
+	ctrl_stat |= TRIG_RESET;
+
+	ret = ksz_write32(dev, REG_PTP_CTRL_STAT__4, ctrl_stat);
+	if (ret)
+		return ret;
+
+	/* Clear DONE */
+	data = 1 << (unit + TRIG_DONE_S);
+	ret = ksz_write32(dev, REG_PTP_TRIG_STATUS__4, data);
+	if (ret)
+		return ret;
+
+	/* Clear IRQ */
+	data = 1 << (unit + TRIG_INT_S);
+	ret = ksz_write32(dev, REG_PTP_INT_STATUS__4, data);
+	if (ret)
+		return ret;
+
+	/* Clear reset and set GPIO direction */
+	ctrl_stat &= ~TRIG_ENABLE;  /* clear cached bit :-) */
+	ctrl_stat &= ~TRIG_RESET;
+
+	ret = ksz_write32(dev, REG_PTP_CTRL_STAT__4, ctrl_stat);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ksz9477_ptp_tou_cycle_width_set(struct ksz_device *dev, u32 width_ns)
+{
+	int ret;
+
+	ret = ksz_write32(dev, REG_TRIG_CYCLE_WIDTH, width_ns);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ksz9477_ptp_tou_cycle_count_set(struct ksz_device *dev, u16 count)
+{
+	u32 data;
+	int ret;
+
+	ret = ksz_read32(dev, REG_TRIG_CYCLE_CNT, &data);
+	if (ret)
+		return ret;
+
+	data &= ~(TRIG_CYCLE_CNT_M << TRIG_CYCLE_CNT_S);
+	data |= (count & TRIG_CYCLE_CNT_M) << TRIG_CYCLE_CNT_S;
+
+	ret = ksz_write32(dev, REG_TRIG_CYCLE_CNT, data);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ksz9477_ptp_tou_pulse_set(struct ksz_device *dev, u32 pulse_ns)
+{
+	u32 data;
+
+	data = (pulse_ns / 8);
+
+	return ksz_write32(dev, REG_TRIG_PULSE_WIDTH__4, data);
+}
+
+static int ksz9477_ptp_tou_target_time_set(struct ksz_device *dev, struct timespec64 const *ts)
+{
+	int ret;
+
+	/* Hardware has only 32 bit */
+	if ((ts->tv_sec & 0xffffffff) != ts->tv_sec)
+		return -EINVAL;
+
+	ret = ksz_write32(dev, REG_TRIG_TARGET_NANOSEC, ts->tv_nsec);
+	if (ret)
+		return ret;
+
+	ret = ksz_write32(dev, REG_TRIG_TARGET_SEC, ts->tv_sec);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ksz9477_ptp_tou_start(struct ksz_device *dev, u32 *ctrl_stat_)
+{
+	u32 ctrl_stat;
+	int ret;
+
+	ret = ksz_read32(dev, REG_PTP_CTRL_STAT__4, &ctrl_stat);
+	if (ret)
+		return ret;
+
+	ctrl_stat |= TRIG_ENABLE;
+
+	ret = ksz_write32(dev, REG_PTP_CTRL_STAT__4, ctrl_stat);
+	if (ret)
+		return ret;
+
+	if (ctrl_stat_)
+		*ctrl_stat_ = ctrl_stat;
+
+	return 0;
+}
+
 /* Posix clock support */
 
 static int ksz9477_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
@@ -76,6 +196,8 @@ static int ksz9477_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
 	return ret;
 }
 
+static int ksz9477_ptp_enable_pps(struct ksz_device *dev, int on);
+
 static int ksz9477_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
 {
 	struct ksz_device *dev = container_of(ptp, struct ksz_device, ptp_caps);
@@ -115,6 +237,20 @@ static int ksz9477_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
 	if (ret)
 		goto error_return;
 
+	switch (dev->ptp_tou_mode) {
+	case KSZ_PTP_TOU_IDLE:
+		break;
+
+	case KSZ_PTP_TOU_PPS:
+		dev_info(dev->dev, "Restarting PPS\n");
+
+		ret = ksz9477_ptp_enable_pps(dev, 1);
+		if (ret)
+			goto error_return;
+
+		break;
+	}
+
 	spin_lock_bh(&ptp_shared->ptp_clock_lock);
 	ptp_shared->ptp_clock_time = timespec64_add(ptp_shared->ptp_clock_time,
 						    delta64);
@@ -218,6 +354,20 @@ static int ksz9477_ptp_settime(struct ptp_clock_info *ptp,
 	if (ret)
 		goto error_return;
 
+	switch (dev->ptp_tou_mode) {
+	case KSZ_PTP_TOU_IDLE:
+		break;
+
+	case KSZ_PTP_TOU_PPS:
+		dev_info(dev->dev, "Restarting PPS\n");
+
+		ret = ksz9477_ptp_enable_pps(dev, 1);
+		if (ret)
+			goto error_return;
+
+		break;
+	}
+
 	spin_lock_bh(&ptp_shared->ptp_clock_lock);
 	ptp_shared->ptp_clock_time = *ts;
 	spin_unlock_bh(&ptp_shared->ptp_clock_lock);
@@ -227,10 +377,107 @@ static int ksz9477_ptp_settime(struct ptp_clock_info *ptp,
 	return ret;
 }
 
+#define KSZ9477_PPS_TOU 0   /* currently fixed to trigger output unit 0 */
+
+static int ksz9477_ptp_enable_pps(struct ksz_device *dev, int on)
+{
+	struct timespec64 now, pps_start, diff;
+	u32 gpio_stat0, trig_ctrl;
+	int ret;
+
+	if (dev->ptp_tou_mode != KSZ_PTP_TOU_PPS && dev->ptp_tou_mode != KSZ_PTP_TOU_IDLE)
+		return -EBUSY;
+
+	/* Reset trigger unit 0 */
+	ret = ksz9477_ptp_tou_reset(dev, KSZ9477_PPS_TOU);
+	if (ret)
+		return ret;
+
+	if (!on) {
+		dev->ptp_tou_mode = KSZ_PTP_TOU_IDLE;
+		return 0;  /* success */
+	}
+
+	/* Enable notify, set rising edge, set periodic pulse pattern */
+	trig_ctrl = TRIG_NOTIFY | (TRIG_POS_PERIOD << TRIG_PATTERN_S);
+
+	ret = ksz_write32(dev, REG_TRIG_CTRL__4, trig_ctrl);
+	if (ret)
+		return ret;
+
+	/* Set cycle width (1 s) */
+	ret = ksz9477_ptp_tou_cycle_width_set(dev, NSEC_PER_SEC);
+	if (ret)
+		return ret;
+
+	/* Set cycle count (infinite) */
+	ksz9477_ptp_tou_cycle_count_set(dev, 0);
+	if (ret)
+		return ret;
+
+	/* Set pulse with (125 ms / 8 ns) */
+	ret = ksz9477_ptp_tou_pulse_set(dev, 125000000);
+	if (ret)
+		return ret;
+
+	/* Read current time */
+	ret = _ksz9477_ptp_gettime(dev, &now);
+	if (ret)
+		return ret;
+
+	/* Determine and write start time of PPS */
+	pps_start.tv_sec = now.tv_sec + 1;
+	pps_start.tv_nsec = 0;
+	diff = timespec64_sub(pps_start, now);
+
+	/* Reserve at least 100 ms for programming and activating */
+	if (diff.tv_nsec < 100000000)
+		pps_start.tv_sec++;
+
+	ret = ksz9477_ptp_tou_target_time_set(dev, &pps_start);
+	if (ret)
+		return ret;
+
+	/* Activate trigger unit */
+	ret = ksz9477_ptp_tou_start(dev, NULL);
+	if (ret)
+		return ret;
+
+	/* Check error flag:
+	 * - the ACTIVE flag is NOT cleared an error!
+	 */
+	ret = ksz_read32(dev, REG_PTP_TRIG_STATUS__4, &gpio_stat0);
+	if (ret)
+		return ret;
+
+	if (gpio_stat0 & (1 << (KSZ9477_PPS_TOU + TRIG_ERROR_S))) {
+		dev_err(dev->dev, "%s: Trigger unit error!\n", __func__);
+		ret = -EIO;
+		/* Unit will be reset on next access */
+		return ret;
+	}
+
+	dev->ptp_tou_mode = KSZ_PTP_TOU_PPS;
+	return 0;
+}
+
 static int ksz9477_ptp_enable(struct ptp_clock_info *ptp,
 			      struct ptp_clock_request *req, int on)
 {
-	return -EOPNOTSUPP;
+	struct ksz_device *dev = container_of(ptp, struct ksz_device, ptp_caps);
+	int ret;
+
+	switch (req->type) {
+	case PTP_CLK_REQ_PPS:
+		mutex_lock(&dev->ptp_mutex);
+		ret = ksz9477_ptp_enable_pps(dev, on);
+		mutex_unlock(&dev->ptp_mutex);
+		return ret;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
 }
 
 static long ksz9477_ptp_do_aux_work(struct ptp_clock_info *ptp)
@@ -572,7 +819,7 @@ int ksz9477_ptp_init(struct ksz_device *dev)
 	dev->ptp_caps.n_alarm     = 0;
 	dev->ptp_caps.n_ext_ts    = 0;  /* currently not implemented */
 	dev->ptp_caps.n_per_out   = 0;
-	dev->ptp_caps.pps         = 0;
+	dev->ptp_caps.pps         = 1;
 	dev->ptp_caps.adjfine     = ksz9477_ptp_adjfine;
 	dev->ptp_caps.adjtime     = ksz9477_ptp_adjtime;
 	dev->ptp_caps.gettime64   = ksz9477_ptp_gettime;
diff --git a/drivers/net/dsa/microchip/ksz9477_ptp.h b/drivers/net/dsa/microchip/ksz9477_ptp.h
index 2f7c4fa0753a..4d20decf0ad7 100644
--- a/drivers/net/dsa/microchip/ksz9477_ptp.h
+++ b/drivers/net/dsa/microchip/ksz9477_ptp.h
@@ -20,6 +20,7 @@
 int ksz9477_ptp_init(struct ksz_device *dev);
 void ksz9477_ptp_deinit(struct ksz_device *dev);
 
+irqreturn_t ksz9477_ptp_interrupt(struct ksz_device *dev);
 irqreturn_t ksz9477_ptp_port_interrupt(struct ksz_device *dev, int port);
 
 int ksz9477_ptp_get_ts_info(struct dsa_switch *ds, int port,
@@ -36,6 +37,9 @@ bool ksz9477_ptp_port_txtstamp(struct dsa_switch *ds, int port,
 static inline int ksz9477_ptp_init(struct ksz_device *dev) { return 0; }
 static inline void ksz9477_ptp_deinit(struct ksz_device *dev) {}
 
+static inline irqreturn_t ksz9477_ptp_interrupt(struct ksz_device *dev)
+{ return IRQ_NONE; }
+
 static inline irqreturn_t ksz9477_ptp_port_interrupt(struct ksz_device *dev,
 						     int port)
 { return IRQ_NONE; }
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 868d0cc9d84f..3481477a62e0 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -54,6 +54,11 @@ struct ksz_port {
 #endif
 };
 
+enum ksz_ptp_tou_mode {
+	KSZ_PTP_TOU_IDLE,
+	KSZ_PTP_TOU_PPS,
+};
+
 struct ksz_device {
 	struct dsa_switch *ds;
 	struct ksz_platform_data *pdata;
@@ -110,6 +115,7 @@ struct ksz_device {
 	struct ptp_clock_info ptp_caps;
 	struct mutex ptp_mutex;		/* protects PTP related hardware */
 	struct ksz_device_ptp_shared ptp_shared;
+	enum ksz_ptp_tou_mode ptp_tou_mode;
 #endif
 };
 
-- 
Christian Eggers
Embedded software developer

Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH net-next v3 12/12] net: dsa: microchip: ksz9477: add periodic output support
  2020-11-18 20:30 [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x Christian Eggers
                   ` (10 preceding siblings ...)
  2020-11-18 20:30 ` [PATCH net-next v3 11/12] net: dsa: microchip: ksz9477: add Pulse Per Second (PPS) support Christian Eggers
@ 2020-11-18 20:30 ` Christian Eggers
  2020-11-20 23:48   ` Vladimir Oltean
  2020-11-18 23:40 ` [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x Vladimir Oltean
  12 siblings, 1 reply; 32+ messages in thread
From: Christian Eggers @ 2020-11-18 20:30 UTC (permalink / raw)
  To: Vladimir Oltean, Jakub Kicinski, Andrew Lunn, Richard Cochran,
	Rob Herring
  Cc: Vivien Didelot, David S . Miller, Kurt Kanzenbach,
	George McCollister, Marek Vasut, Helmut Grohne, Paul Barker,
	Codrin Ciubotariu, Tristram Ha, Woojung Huh,
	Microchip Linux Driver Support, Christian Eggers, netdev,
	devicetree, linux-kernel

The KSZ9563 has a Trigger Output Unit (TOU) which can be used to
generate periodic signals.

The pulse length can be altered via a device attribute.

Tested on a Microchip KSZ9563 switch.

Signed-off-by: Christian Eggers <ceggers@arri.de>
---
 drivers/net/dsa/microchip/ksz9477_ptp.c | 197 +++++++++++++++++++++++-
 drivers/net/dsa/microchip/ksz_common.h  |   5 +
 2 files changed, 201 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/microchip/ksz9477_ptp.c b/drivers/net/dsa/microchip/ksz9477_ptp.c
index ce3fdc9a1f9e..3174574d52f6 100644
--- a/drivers/net/dsa/microchip/ksz9477_ptp.c
+++ b/drivers/net/dsa/microchip/ksz9477_ptp.c
@@ -90,6 +90,20 @@ static int ksz9477_ptp_tou_cycle_count_set(struct ksz_device *dev, u16 count)
 	return 0;
 }
 
+static int ksz9477_ptp_tou_pulse_verify(u64 pulse_ns)
+{
+	u32 data;
+
+	if (pulse_ns & 0x3)
+		return -EINVAL;
+
+	data = (pulse_ns / 8);
+	if (data != (data & TRIG_PULSE_WIDTH_M))
+		return -ERANGE;
+
+	return 0;
+}
+
 static int ksz9477_ptp_tou_pulse_set(struct ksz_device *dev, u32 pulse_ns)
 {
 	u32 data;
@@ -196,6 +210,7 @@ static int ksz9477_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
 	return ret;
 }
 
+static int ksz9477_ptp_restart_perout(struct ksz_device *dev);
 static int ksz9477_ptp_enable_pps(struct ksz_device *dev, int on);
 
 static int ksz9477_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
@@ -241,6 +256,15 @@ static int ksz9477_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
 	case KSZ_PTP_TOU_IDLE:
 		break;
 
+	case KSZ_PTP_TOU_PEROUT:
+		dev_info(dev->dev, "Restarting periodic output signal\n");
+
+		ret = ksz9477_ptp_restart_perout(dev);
+		if (ret)
+			goto error_return;
+
+		break;
+
 	case KSZ_PTP_TOU_PPS:
 		dev_info(dev->dev, "Restarting PPS\n");
 
@@ -358,6 +382,15 @@ static int ksz9477_ptp_settime(struct ptp_clock_info *ptp,
 	case KSZ_PTP_TOU_IDLE:
 		break;
 
+	case KSZ_PTP_TOU_PEROUT:
+		dev_info(dev->dev, "Restarting periodic output signal\n");
+
+		ret = ksz9477_ptp_restart_perout(dev);
+		if (ret)
+			goto error_return;
+
+		break;
+
 	case KSZ_PTP_TOU_PPS:
 		dev_info(dev->dev, "Restarting PPS\n");
 
@@ -377,6 +410,159 @@ static int ksz9477_ptp_settime(struct ptp_clock_info *ptp,
 	return ret;
 }
 
+static int ksz9477_ptp_configure_perout(struct ksz_device *dev, u32 cycle_width_ns,
+					u16 cycle_count, u32 pulse_width_ns,
+					struct timespec64 const *target_time)
+{
+	int ret;
+	u32 trig_ctrl;
+
+	/* Enable notify, set rising edge, set periodic pattern */
+	trig_ctrl = TRIG_NOTIFY | (TRIG_POS_PERIOD << TRIG_PATTERN_S);
+	ret = ksz_write32(dev, REG_TRIG_CTRL__4, trig_ctrl);
+	if (ret)
+		return ret;
+
+	ret = ksz9477_ptp_tou_cycle_width_set(dev, cycle_width_ns);
+	if (ret)
+		return ret;
+
+	ksz9477_ptp_tou_cycle_count_set(dev,  cycle_count);
+	if (ret)
+		return ret;
+
+	ret = ksz9477_ptp_tou_pulse_set(dev, pulse_width_ns);
+	if (ret)
+		return ret;
+
+	ret = ksz9477_ptp_tou_target_time_set(dev, target_time);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ksz9477_ptp_enable_perout(struct ksz_device *dev,
+				     struct ptp_perout_request const *perout_request, int on)
+{
+	u32 gpio_stat0;
+	u64 cycle_width_ns;
+	int ret;
+
+	if (dev->ptp_tou_mode != KSZ_PTP_TOU_PEROUT && dev->ptp_tou_mode != KSZ_PTP_TOU_IDLE)
+		return -EBUSY;
+
+	ret = ksz9477_ptp_tou_reset(dev, 0);
+	if (ret)
+		return ret;
+
+	if (!on) {
+		dev->ptp_tou_mode = KSZ_PTP_TOU_IDLE;
+		return 0;  /* success */
+	}
+
+	dev->ptp_perout_target_time_first.tv_sec  = perout_request->start.sec;
+	dev->ptp_perout_target_time_first.tv_nsec = perout_request->start.nsec;
+
+	dev->ptp_perout_period.tv_sec = perout_request->period.sec;
+	dev->ptp_perout_period.tv_nsec = perout_request->period.nsec;
+
+	cycle_width_ns = timespec64_to_ns(&dev->ptp_perout_period);
+	if ((cycle_width_ns & GENMASK(31, 0)) != cycle_width_ns)
+		return -EINVAL;
+
+	if (perout_request->flags & PTP_PEROUT_DUTY_CYCLE) {
+		u64 value = perout_request->on.sec * NSEC_PER_SEC +
+			    perout_request->on.nsec;
+
+		ret = ksz9477_ptp_tou_pulse_verify(value);
+		if (ret)
+			return ret;
+
+		dev->ptp_perout_pulse_width_ns = value;
+	}
+
+	ret = ksz9477_ptp_configure_perout(dev, cycle_width_ns,
+					   dev->ptp_perout_cycle_count,
+					   dev->ptp_perout_pulse_width_ns,
+					   &dev->ptp_perout_target_time_first);
+	if (ret)
+		return ret;
+
+	/* Activate trigger unit */
+	ret = ksz9477_ptp_tou_start(dev, NULL);
+	if (ret)
+		return ret;
+
+	/* Check error flag:
+	 * - the ACTIVE flag is NOT cleared an error!
+	 */
+	ret = ksz_read32(dev, REG_PTP_TRIG_STATUS__4, &gpio_stat0);
+	if (ret)
+		return ret;
+
+	if (gpio_stat0 & (1 << (0 + TRIG_ERROR_S))) {
+		dev_err(dev->dev, "%s: Trigger unit0 error!\n", __func__);
+		ret = -EIO;
+		/* Unit will be reset on next access */
+		return ret;
+	}
+
+	dev->ptp_tou_mode = KSZ_PTP_TOU_PEROUT;
+	return 0;
+}
+
+static int ksz9477_ptp_restart_perout(struct ksz_device *dev)
+{
+	struct timespec64 now;
+	s64 now_ns, first_ns, period_ns, next_ns;
+	unsigned int count;
+	int ret;
+
+	ret = _ksz9477_ptp_gettime(dev, &now);
+	if (ret)
+		return ret;
+
+	now_ns = timespec64_to_ns(&now);
+	first_ns = timespec64_to_ns(&dev->ptp_perout_target_time_first);
+
+	/* Calculate next perout event based on start time and period */
+	period_ns = timespec64_to_ns(&dev->ptp_perout_period);
+
+	if (first_ns < now_ns) {
+		count = div_u64(now_ns - first_ns, period_ns);
+		next_ns = first_ns + count * period_ns;
+	} else {
+		next_ns = first_ns;
+	}
+
+	/* Ensure 100 ms guard time prior next event */
+	while (next_ns < now_ns + 100000000)
+		next_ns += period_ns;
+
+	/* Restart periodic output signal */
+	{
+		struct timespec64 next = ns_to_timespec64(next_ns);
+		struct ptp_perout_request perout_request = {
+			.start = {
+				.sec  = next.tv_sec,
+				.nsec = next.tv_nsec
+			},
+			.period = {
+				.sec  = dev->ptp_perout_period.tv_sec,
+				.nsec = dev->ptp_perout_period.tv_nsec
+			},
+			.index = 0,
+			.flags = 0,  /* keep current values */
+		};
+		ret = ksz9477_ptp_enable_perout(dev, &perout_request, 1);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 #define KSZ9477_PPS_TOU 0   /* currently fixed to trigger output unit 0 */
 
 static int ksz9477_ptp_enable_pps(struct ksz_device *dev, int on)
@@ -468,6 +654,15 @@ static int ksz9477_ptp_enable(struct ptp_clock_info *ptp,
 	int ret;
 
 	switch (req->type) {
+	case PTP_CLK_REQ_PEROUT:
+	{
+		struct ptp_perout_request const *perout_request = &req->perout;
+
+		mutex_lock(&dev->ptp_mutex);
+		ret = ksz9477_ptp_enable_perout(dev, perout_request, on);
+		mutex_unlock(&dev->ptp_mutex);
+		return ret;
+	}
 	case PTP_CLK_REQ_PPS:
 		mutex_lock(&dev->ptp_mutex);
 		ret = ksz9477_ptp_enable_pps(dev, on);
@@ -818,7 +1013,7 @@ int ksz9477_ptp_init(struct ksz_device *dev)
 	dev->ptp_caps.max_adj     = 6249999;
 	dev->ptp_caps.n_alarm     = 0;
 	dev->ptp_caps.n_ext_ts    = 0;  /* currently not implemented */
-	dev->ptp_caps.n_per_out   = 0;
+	dev->ptp_caps.n_per_out   = 1;
 	dev->ptp_caps.pps         = 1;
 	dev->ptp_caps.adjfine     = ksz9477_ptp_adjfine;
 	dev->ptp_caps.adjtime     = ksz9477_ptp_adjtime;
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 3481477a62e0..3b897a6c882d 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -56,6 +56,7 @@ struct ksz_port {
 
 enum ksz_ptp_tou_mode {
 	KSZ_PTP_TOU_IDLE,
+	KSZ_PTP_TOU_PEROUT,
 	KSZ_PTP_TOU_PPS,
 };
 
@@ -116,6 +117,10 @@ struct ksz_device {
 	struct mutex ptp_mutex;		/* protects PTP related hardware */
 	struct ksz_device_ptp_shared ptp_shared;
 	enum ksz_ptp_tou_mode ptp_tou_mode;
+	struct timespec64 ptp_perout_target_time_first;  /* start of first perout pulse */
+	struct timespec64 ptp_perout_period;
+	u32 ptp_perout_pulse_width_ns;
+	u16 ptp_perout_cycle_count;
 #endif
 };
 
-- 
Christian Eggers
Embedded software developer

Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x
  2020-11-18 20:30 [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x Christian Eggers
                   ` (11 preceding siblings ...)
  2020-11-18 20:30 ` [PATCH net-next v3 12/12] net: dsa: microchip: ksz9477: add periodic output support Christian Eggers
@ 2020-11-18 23:40 ` Vladimir Oltean
  2020-11-19  5:28   ` Christian Eggers
  12 siblings, 1 reply; 32+ messages in thread
From: Vladimir Oltean @ 2020-11-18 23:40 UTC (permalink / raw)
  To: Christian Eggers
  Cc: Jakub Kicinski, Andrew Lunn, Richard Cochran, Rob Herring,
	Vivien Didelot, David S . Miller, Kurt Kanzenbach,
	George McCollister, Marek Vasut, Helmut Grohne, Paul Barker,
	Codrin Ciubotariu, Tristram Ha, Woojung Huh,
	Microchip Linux Driver Support, netdev, devicetree, linux-kernel

On Wed, Nov 18, 2020 at 09:30:01PM +0100, Christian Eggers wrote:
> This series adds support for PTP to the KSZ956x and KSZ9477 devices.
> 
> There is only little documentation for PTP available on the data sheet
> [1] (more or less only the register reference). Questions to the
> Microchip support were seldom answered comprehensively or in reasonable
> time. So this is more or less the result of reverse engineering.

I will not have the time today, and probably not tomorrow, to review
this. I want to take some time to get more hands-on with the UDP
checksumming issues reported by Christian in the previous version (in
order to understand what the problem really is),
https://lore.kernel.org/netdev/1813904.kIZFssEuCH@n95hx1g2/
and I will probably only find time for that in the weekend. If anybody
feels like reviewing the series in the meantime, of course feel free to
do so.

One thing that should definitely not be part of this series though is
patch 11/12. Christian, given the conversation we had on your previous
patch:
https://lore.kernel.org/netdev/20201113025311.jpkplhmacjz6lkc5@skbuf/
as well as the documentation patch that was submitted in the meantime:
https://lore.kernel.org/netdev/20201117213826.18235-1-a.fatoum@pengutronix.de/
obviously you chose to completely disregard that. May we know why?
How are you even making use of the PTP_CLK_REQ_PPS feature?

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x
  2020-11-18 23:40 ` [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x Vladimir Oltean
@ 2020-11-19  5:28   ` Christian Eggers
  2020-11-19 18:51     ` Tristram.Ha
  0 siblings, 1 reply; 32+ messages in thread
From: Christian Eggers @ 2020-11-19  5:28 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: Jakub Kicinski, Andrew Lunn, Richard Cochran, Rob Herring,
	Vivien Didelot, David S . Miller, Kurt Kanzenbach,
	George McCollister, Marek Vasut, Helmut Grohne, Paul Barker,
	Codrin Ciubotariu, Tristram Ha, Woojung Huh,
	Microchip Linux Driver Support, netdev, devicetree, linux-kernel

Hi Vladimir,

On Thursday, 19 November 2020, 00:40:18 CET, Vladimir Oltean wrote:
> On Wed, Nov 18, 2020 at 09:30:01PM +0100, Christian Eggers wrote:
> > This series adds support for PTP to the KSZ956x and KSZ9477 devices.
> > 
> > There is only little documentation for PTP available on the data sheet
> > [1] (more or less only the register reference). Questions to the
> > Microchip support were seldom answered comprehensively or in reasonable
> > time. So this is more or less the result of reverse engineering.
> 
> [...]
> One thing that should definitely not be part of this series though is
> patch 11/12. Christian, given the conversation we had on your previous
> patch:
> https://lore.kernel.org/netdev/20201113025311.jpkplhmacjz6lkc5@skbuf/
sorry, I didn't read that carefully enough. Some of the other requested changes
were quite challenging for me. Additionally, finding the UDP checksum bug
needed some time for identifying because I didn't recognize that when it got
introduced.

> as well as the documentation patch that was submitted in the meantime:
> https://lore.kernel.org/netdev/20201117213826.18235-1-a.fatoum@pengutronix.de/ 
I am not subscribed to the list. 

> obviously you chose to completely disregard that. May we know why? How
> are you even making use of the PTP_CLK_REQ_PPS feature?
Of course I will drop that patch from the next series.

regards
Christian




^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH net-next v3 01/12] dt-bindings: net: dsa: convert ksz bindings document to yaml
  2020-11-18 20:30 ` [PATCH net-next v3 01/12] dt-bindings: net: dsa: convert ksz bindings document to yaml Christian Eggers
@ 2020-11-19 13:42   ` Rob Herring
  2020-11-19 13:48   ` Rob Herring
  1 sibling, 0 replies; 32+ messages in thread
From: Rob Herring @ 2020-11-19 13:42 UTC (permalink / raw)
  To: Christian Eggers
  Cc: Paul Barker, Microchip Linux Driver Support, Helmut Grohne,
	Marek Vasut, linux-kernel, Andrew Lunn, Rob Herring,
	Jakub Kicinski, Richard Cochran, netdev, Codrin Ciubotariu,
	Kurt Kanzenbach, Vivien Didelot, David S . Miller,
	George McCollister, Woojung Huh, devicetree, Tristram Ha,
	Vladimir Oltean

On Wed, 18 Nov 2020 21:30:02 +0100, Christian Eggers wrote:
> Convert the bindings document for Microchip KSZ Series Ethernet switches
> from txt to yaml.
> 
> Signed-off-by: Christian Eggers <ceggers@arri.de>
> ---
>  .../devicetree/bindings/net/dsa/ksz.txt       | 125 --------------
>  .../bindings/net/dsa/microchip,ksz.yaml       | 152 ++++++++++++++++++
>  MAINTAINERS                                   |   2 +-
>  3 files changed, 153 insertions(+), 126 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/net/dsa/ksz.txt
>  create mode 100644 Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> 


My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/dsa/microchip,ksz.example.dt.yaml: switch@0: 'ethernet-ports', 'reg', 'spi-cpha', 'spi-cpol', 'spi-max-frequency' do not match any of the regexes: 'pinctrl-[0-9]+'
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/dsa/microchip,ksz.example.dt.yaml: switch@1: 'ethernet-ports', 'reg', 'spi-cpha', 'spi-cpol', 'spi-max-frequency' do not match any of the regexes: 'pinctrl-[0-9]+'
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml


See https://patchwork.ozlabs.org/patch/1402525

The base for the patch is generally the last rc1. Any dependencies
should be noted.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH net-next v3 01/12] dt-bindings: net: dsa: convert ksz bindings document to yaml
  2020-11-18 20:30 ` [PATCH net-next v3 01/12] dt-bindings: net: dsa: convert ksz bindings document to yaml Christian Eggers
  2020-11-19 13:42   ` Rob Herring
@ 2020-11-19 13:48   ` Rob Herring
  2020-11-19 20:22     ` Christian Eggers
  1 sibling, 1 reply; 32+ messages in thread
From: Rob Herring @ 2020-11-19 13:48 UTC (permalink / raw)
  To: Christian Eggers
  Cc: Vladimir Oltean, Jakub Kicinski, Andrew Lunn, Richard Cochran,
	Vivien Didelot, David S . Miller, Kurt Kanzenbach,
	George McCollister, Marek Vasut, Helmut Grohne, Paul Barker,
	Codrin Ciubotariu, Tristram Ha, Woojung Huh,
	Microchip Linux Driver Support, netdev, devicetree, linux-kernel

On Wed, Nov 18, 2020 at 09:30:02PM +0100, Christian Eggers wrote:
> Convert the bindings document for Microchip KSZ Series Ethernet switches
> from txt to yaml.
> 
> Signed-off-by: Christian Eggers <ceggers@arri.de>
> ---
>  .../devicetree/bindings/net/dsa/ksz.txt       | 125 --------------
>  .../bindings/net/dsa/microchip,ksz.yaml       | 152 ++++++++++++++++++
>  MAINTAINERS                                   |   2 +-
>  3 files changed, 153 insertions(+), 126 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/net/dsa/ksz.txt
>  create mode 100644 Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/ksz.txt b/Documentation/devicetree/bindings/net/dsa/ksz.txt
> deleted file mode 100644
> index 95e91e84151c..000000000000
> --- a/Documentation/devicetree/bindings/net/dsa/ksz.txt
> +++ /dev/null
> @@ -1,125 +0,0 @@
> -Microchip KSZ Series Ethernet switches
> -==================================
> -
> -Required properties:
> -
> -- compatible: For external switch chips, compatible string must be exactly one
> -  of the following:
> -  - "microchip,ksz8765"
> -  - "microchip,ksz8794"
> -  - "microchip,ksz8795"
> -  - "microchip,ksz9477"
> -  - "microchip,ksz9897"
> -  - "microchip,ksz9896"
> -  - "microchip,ksz9567"
> -  - "microchip,ksz8565"
> -  - "microchip,ksz9893"
> -  - "microchip,ksz9563"
> -  - "microchip,ksz8563"
> -
> -Optional properties:
> -
> -- reset-gpios		: Should be a gpio specifier for a reset line
> -- microchip,synclko-125 : Set if the output SYNCLKO frequency should be set to
> -			  125MHz instead of 25MHz.
> -
> -See Documentation/devicetree/bindings/net/dsa/dsa.txt for a list of additional
> -required and optional properties.
> -
> -Examples:
> -
> -Ethernet switch connected via SPI to the host, CPU port wired to eth0:
> -
> -	eth0: ethernet@10001000 {
> -		fixed-link {
> -			speed = <1000>;
> -			full-duplex;
> -		};
> -	};
> -
> -	spi1: spi@f8008000 {
> -		pinctrl-0 = <&pinctrl_spi_ksz>;
> -		cs-gpios = <&pioC 25 0>;
> -		id = <1>;
> -
> -		ksz9477: ksz9477@0 {
> -			compatible = "microchip,ksz9477";
> -			reg = <0>;
> -
> -			spi-max-frequency = <44000000>;
> -			spi-cpha;
> -			spi-cpol;
> -
> -			ports {
> -				#address-cells = <1>;
> -				#size-cells = <0>;
> -				port@0 {
> -					reg = <0>;
> -					label = "lan1";
> -				};
> -				port@1 {
> -					reg = <1>;
> -					label = "lan2";
> -				};
> -				port@2 {
> -					reg = <2>;
> -					label = "lan3";
> -				};
> -				port@3 {
> -					reg = <3>;
> -					label = "lan4";
> -				};
> -				port@4 {
> -					reg = <4>;
> -					label = "lan5";
> -				};
> -				port@5 {
> -					reg = <5>;
> -					label = "cpu";
> -					ethernet = <&eth0>;
> -					fixed-link {
> -						speed = <1000>;
> -						full-duplex;
> -					};
> -				};
> -			};
> -		};
> -		ksz8565: ksz8565@0 {
> -			compatible = "microchip,ksz8565";
> -			reg = <0>;
> -
> -			spi-max-frequency = <44000000>;
> -			spi-cpha;
> -			spi-cpol;
> -
> -			ports {
> -				#address-cells = <1>;
> -				#size-cells = <0>;
> -				port@0 {
> -					reg = <0>;
> -					label = "lan1";
> -				};
> -				port@1 {
> -					reg = <1>;
> -					label = "lan2";
> -				};
> -				port@2 {
> -					reg = <2>;
> -					label = "lan3";
> -				};
> -				port@3 {
> -					reg = <3>;
> -					label = "lan4";
> -				};
> -				port@6 {
> -					reg = <6>;
> -					label = "cpu";
> -					ethernet = <&eth0>;
> -					fixed-link {
> -						speed = <1000>;
> -						full-duplex;
> -					};
> -				};
> -			};
> -		};
> -	};
> diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> new file mode 100644
> index 000000000000..010adb09a68f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> @@ -0,0 +1,152 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/dsa/microchip,ksz.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip KSZ Series Ethernet switches
> +
> +allOf:
> +  - $ref: dsa.yaml#

Move this after 'maintainers'.

> +
> +maintainers:
> +  - Marek Vasut <marex@denx.de>
> +  - Woojung Huh <Woojung.Huh@microchip.com>
> +
> +properties:
> +  # See Documentation/devicetree/bindings/net/dsa/dsa.yaml for a list of additional
> +  # required and optional properties.
> +  compatible:
> +    enum:
> +      - microchip,ksz8765
> +      - microchip,ksz8794
> +      - microchip,ksz8795
> +      - microchip,ksz9477
> +      - microchip,ksz9897
> +      - microchip,ksz9896
> +      - microchip,ksz9567
> +      - microchip,ksz8565
> +      - microchip,ksz9893
> +      - microchip,ksz9563
> +      - microchip,ksz8563
> +
> +  reset-gpios:
> +    description:
> +      Should be a gpio specifier for a reset line.
> +    maxItems: 1
> +
> +  microchip,synclko-125:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      Set if the output SYNCLKO frequency should be set to 125MHz instead of 25MHz.
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false

You need to use unevaluatedProperties instead.

> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    // Ethernet switch connected via SPI to the host, CPU port wired to eth0:
> +    eth0 {
> +        fixed-link {
> +            speed = <1000>;
> +            full-duplex;
> +        };
> +    };
> +
> +    spi0 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        pinctrl-0 = <&pinctrl_spi_ksz>;
> +        cs-gpios = <&pioC 25 0>;
> +        id = <1>;
> +
> +        ksz9477: switch@0 {
> +            compatible = "microchip,ksz9477";
> +            reg = <0>;
> +            reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
> +
> +            spi-max-frequency = <44000000>;
> +            spi-cpha;
> +            spi-cpol;

Are these 2 optional or required? Being optional is rare as most 
devices support 1 mode, but not unheard of. In general, you shouldn't 
need them as the driver should know how to configure the mode if the h/w 
is fixed.

> +
> +            ethernet-ports {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +                port@0 {
> +                    reg = <0>;
> +                    label = "lan1";
> +                };
> +                port@1 {
> +                    reg = <1>;
> +                    label = "lan2";
> +                };
> +                port@2 {
> +                    reg = <2>;
> +                    label = "lan3";
> +                };
> +                port@3 {
> +                    reg = <3>;
> +                    label = "lan4";
> +                };
> +                port@4 {
> +                    reg = <4>;
> +                    label = "lan5";
> +                };
> +                port@5 {
> +                    reg = <5>;
> +                    label = "cpu";
> +                    ethernet = <&eth0>;
> +                    fixed-link {
> +                        speed = <1000>;
> +                        full-duplex;
> +                    };
> +                };
> +            };
> +        };
> +
> +        ksz8565: switch@1 {
> +            compatible = "microchip,ksz8565";
> +            reg = <1>;
> +
> +            spi-max-frequency = <44000000>;
> +            spi-cpha;
> +            spi-cpol;
> +
> +            ethernet-ports {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +                port@0 {
> +                    reg = <0>;
> +                    label = "lan1";
> +                };
> +                port@1 {
> +                    reg = <1>;
> +                    label = "lan2";
> +                };
> +                port@2 {
> +                    reg = <2>;
> +                    label = "lan3";
> +                };
> +                port@3 {
> +                    reg = <3>;
> +                    label = "lan4";
> +                };
> +                port@6 {
> +                    reg = <6>;
> +                    label = "cpu";
> +                    ethernet = <&eth0>;
> +                    fixed-link {
> +                        speed = <1000>;
> +                        full-duplex;
> +                    };
> +                };
> +            };
> +        };
> +    };
> +...
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 18b5b7896af8..d1003033412f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11508,7 +11508,7 @@ M:	Woojung Huh <woojung.huh@microchip.com>
>  M:	Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>
>  L:	netdev@vger.kernel.org
>  S:	Maintained
> -F:	Documentation/devicetree/bindings/net/dsa/ksz.txt
> +F:	Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
>  F:	drivers/net/dsa/microchip/*
>  F:	include/linux/platform_data/microchip-ksz.h
>  F:	net/dsa/tag_ksz.c
> -- 
> Christian Eggers
> Embedded software developer
> 
> Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
> Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
> Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
> Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
> Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler
> 

^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x
  2020-11-19  5:28   ` Christian Eggers
@ 2020-11-19 18:51     ` Tristram.Ha
  2020-11-19 20:16       ` Christian Eggers
                         ` (2 more replies)
  0 siblings, 3 replies; 32+ messages in thread
From: Tristram.Ha @ 2020-11-19 18:51 UTC (permalink / raw)
  To: ceggers, olteanv
  Cc: kuba, andrew, richardcochran, robh+dt, vivien.didelot, davem,
	kurt.kanzenbach, george.mccollister, marex, helmut.grohne,
	pbarker, Codrin.Ciubotariu, Woojung.Huh, UNGLinuxDriver, netdev,
	devicetree, linux-kernel

> On Thursday, 19 November 2020, 00:40:18 CET, Vladimir Oltean wrote:
> > On Wed, Nov 18, 2020 at 09:30:01PM +0100, Christian Eggers wrote:
> > > This series adds support for PTP to the KSZ956x and KSZ9477 devices.
> > >
> > > There is only little documentation for PTP available on the data sheet
> > > [1] (more or less only the register reference). Questions to the
> > > Microchip support were seldom answered comprehensively or in
> reasonable
> > > time. So this is more or less the result of reverse engineering.
> >
> > [...]
> > One thing that should definitely not be part of this series though is
> > patch 11/12. Christian, given the conversation we had on your previous
> > patch:
> > https://lore.kernel.org/netdev/20201113025311.jpkplhmacjz6lkc5@skbuf/
> sorry, I didn't read that carefully enough. Some of the other requested
> changes
> were quite challenging for me. Additionally, finding the UDP checksum bug
> needed some time for identifying because I didn't recognize that when it got
> introduced.
> 
> > as well as the documentation patch that was submitted in the meantime:
> > https://lore.kernel.org/netdev/20201117213826.18235-1-
> a.fatoum@pengutronix.de/
> I am not subscribed to the list.
> 
> > obviously you chose to completely disregard that. May we know why? How
> > are you even making use of the PTP_CLK_REQ_PPS feature?
> Of course I will drop that patch from the next series.

These are general comments about this PTP patch.

The initial proposal in tag_ksz.c is for the switch driver to provide callback functions
to handle receiving and transmitting.  Then each switch driver can be added to
process the tail tag in its own driver and leave tag_ksz.c unchanged.

It was rejected because of wanting to keep tag_ksz.c code and switch driver code
separate and concern about performance.

Now tag_ksz.c is filled with PTP code that is not relevant for other switches and will
need to be changed again when another switch driver with PTP function is added.

Can we implement that callback mechanism?

One issue with transmission with PTP enabled is that the tail tag needs to contain 4
additional bytes.  When the PTP function is off the bytes are not added.  This should
be monitored all the time.

The extra 4 bytes are only used for 1-step Pdelay_Resp.  It should contain the receive
timestamp of previous Pdelay_Req with latency adjusted.  The correction field in
Pdelay_Resp should be zero.  It may be a hardware bug to have wrong UDP checksum
when the message is sent.

I think the right implementation is for the driver to remember this receive timestamp
of Pdelay_Req and puts it in the tail tag when it sees a 1-step Pdelay_Resp is sent.

There is one more requirement that is a little difficult to do.  The calculated peer delay
needs to be programmed in hardware register, but the regular PTP stack has no way to
send that command.  I think the driver has to do its own calculation by snooping on the
Pdelay_Req/Pdelay_Resp/Pdelay_Resp_Follow_Up messages.

The receive and transmit latencies are different for different connected speed.  So the
driver needs to change them when the link changes.  For that reason the PTP stack
should not use its own latency values as generally the application does not care about
the linked speed.
 

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x
  2020-11-19 18:51     ` Tristram.Ha
@ 2020-11-19 20:16       ` Christian Eggers
  2020-11-21  1:26       ` Vladimir Oltean
  2020-11-25 21:08       ` Christian Eggers
  2 siblings, 0 replies; 32+ messages in thread
From: Christian Eggers @ 2020-11-19 20:16 UTC (permalink / raw)
  To: Tristram.Ha
  Cc: olteanv, kuba, andrew, richardcochran, robh+dt, vivien.didelot,
	davem, kurt.kanzenbach, george.mccollister, marex, helmut.grohne,
	pbarker, Codrin.Ciubotariu, Woojung.Huh, UNGLinuxDriver, netdev,
	devicetree, linux-kernel

Hi Tristram,

thank you for joining this thread.

On Thursday, 19 November 2020, 19:51:15 CET, Tristram.Ha@microchip.com wrote:
> > On Thursday, 19 November 2020, 00:40:18 CET, Vladimir Oltean wrote:
> > > On Wed, Nov 18, 2020 at 09:30:01PM +0100, Christian Eggers wrote:
> > > [...]
> > [...]
> These are general comments about this PTP patch.
> 
> The initial proposal in tag_ksz.c is for the switch driver to provide
> callback functions to handle receiving and transmitting.  Then each switch
> driver can be added to process the tail tag in its own driver and leave
> tag_ksz.c unchanged. 
> It was rejected because of wanting to keep tag_ksz.c code and switch driver
> code separate and concern about performance.
> 
> Now tag_ksz.c is filled with PTP code that is not relevant for other
> switches and will need to be changed again when another switch driver with
> PTP function is added. 
> Can we implement that callback mechanism?
I didn't read the full history of the tagging driver. Vladimir already asked
whether I could put more stuff into the device driver. Lets wait for his
advice how to do this best.

> One issue with transmission with PTP enabled is that the tail tag needs to
> contain 4 additional bytes.  When the PTP function is off the bytes are
> not added.  This should be monitored all the time.
Currently, enabling the PTP function is only dependent on
CONFIG_NET_DSA_MICROCHIP_KSZ9477_PTP. The same condition is used for inserting
the additional 4 bytes.

> The extra 4 bytes are only used for 1-step Pdelay_Resp.  It should contain
> the receive timestamp of previous Pdelay_Req with latency adjusted.  The
> correction field in Pdelay_Resp should be zero.  It may be a hardware bug
> to have wrong UDP checksum when the message is sent.
Thanks for clarifying this.

> I think the right implementation is for the driver to remember this receive
> timestamp of Pdelay_Req and puts it in the tail tag when it sees a 1-step
> Pdelay_Resp is sent. 
I would like keep the current method ("time stamp to correction" on RX, 
"correction to tail tag" on TX). Otherwise the driver would have to keep a 
list
of rx time stamps which could grow if no corresponding PDelay_Resp is sent.
It was also discussed about creating a new interface for bringing the time
stamp to user space and then back into the kernel. But this has been rejected.

> There is one more requirement that is a little difficult to do.  The
> calculated peer delay needs to be programmed in hardware register, but the
> regular PTP stack has no way to send that command.
I already recognized that register. Can you please provide some more 
information what the switch does with this value? At least when I connect only
two boards, I get almost perfect synchronization (PPS output) without writing
anything to this register. Looks like this only affects forwarded messages,
right?

> I think the driver has to do its own calculation by snooping on the
> Pdelay_Req/Pdelay_Resp/Pdelay_Resp_Follow_Up messages.
As I already wrote, I am definitely not an expert for PTP. But if I remember
correctly, the delay values used by ptp4l are the result of filtering several 
delay measurements. I don't think that this algorithm should be duplicated in
the kernel. 

On the other hand, there is currently no interface for this. In my internal
tree, I have created sysfs entries for this, so that (a modified version of)
ptp4l could write the measured values. I also recognized, that ptp4l has some
kind of remote interface (I haven't really looked at it). Maybe it is possible
to do necessary management of the switch outside ptp4l in a separate process.

One other important question was about the internal "filter". Richard rejected
the idea of "manually" switching between the "master" and "slave" mode. Is
there any (undocumented) register bit for disabling filtering of Sync/
Delay_Req
messages entirely?

> The receive and transmit latencies are different for different connected
> speed.  So the driver needs to change them when the link changes.  For
> that reason the PTP stack should not use its own latency values as
> generally the application does not care about the linked speed.
Up to now, I didn't configure any latency values in ptp4l. I assume that the
power on default values are fine for 1000 MBit/s. Can you provide the latency
values for other links speeds? Would it be a major limitation if PTP 
functionality depend on 1000 MBit/s?

regards
Christian




^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH net-next v3 01/12] dt-bindings: net: dsa: convert ksz bindings document to yaml
  2020-11-19 13:48   ` Rob Herring
@ 2020-11-19 20:22     ` Christian Eggers
  0 siblings, 0 replies; 32+ messages in thread
From: Christian Eggers @ 2020-11-19 20:22 UTC (permalink / raw)
  To: Rob Herring
  Cc: Vladimir Oltean, Jakub Kicinski, Andrew Lunn, Richard Cochran,
	Vivien Didelot, David S . Miller, Kurt Kanzenbach,
	George McCollister, Marek Vasut, Helmut Grohne, Paul Barker,
	Codrin Ciubotariu, Tristram Ha, Woojung Huh,
	Microchip Linux Driver Support, netdev, devicetree, linux-kernel

On Thursday, 19 November 2020, 14:48:01 CET, Rob Herring wrote:
> On Wed, Nov 18, 2020 at 09:30:02PM +0100, Christian Eggers wrote:
> > Convert the bindings document for Microchip KSZ Series Ethernet switches
> > from txt to yaml.
> > 
> > Signed-off-by: Christian Eggers <ceggers@arri.de>
> > ---
> > 
> >  .../devicetree/bindings/net/dsa/ksz.txt       | 125 --------------
> >  .../bindings/net/dsa/microchip,ksz.yaml       | 152 ++++++++++++++++++
> >  MAINTAINERS                                   |   2 +-
> >  3 files changed, 153 insertions(+), 126 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/net/dsa/ksz.txt
> >  create mode 100644
> >  Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml> 
> > diff --git a/Documentation/devicetree/bindings/net/dsa/ksz.txt
> > b/Documentation/devicetree/bindings/net/dsa/ksz.txt deleted file mode
> > 100644
> > index 95e91e84151c..000000000000
> > --- a/Documentation/devicetree/bindings/net/dsa/ksz.txt
> > +++ /dev/null
>> [...]
> > diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml new file
> > mode 100644
> > index 000000000000..010adb09a68f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > @@ -0,0 +1,152 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/dsa/microchip,ksz.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Microchip KSZ Series Ethernet switches
> > +
> > +allOf:
> > +  - $ref: dsa.yaml#
> 
> Move this after 'maintainers'.
changed for v4

> 
> > +
> > +maintainers:
> > +  - Marek Vasut <marex@denx.de>
> > +  - Woojung Huh <Woojung.Huh@microchip.com>
> > +
> > +properties:
> > +  # See Documentation/devicetree/bindings/net/dsa/dsa.yaml for a list of
> > additional +  # required and optional properties.
> > +  compatible:
> > +    enum:
> > +      - microchip,ksz8765
> > +      - microchip,ksz8794
> > +      - microchip,ksz8795
> > +      - microchip,ksz9477
> > +      - microchip,ksz9897
> > +      - microchip,ksz9896
> > +      - microchip,ksz9567
> > +      - microchip,ksz8565
> > +      - microchip,ksz9893
> > +      - microchip,ksz9563
> > +      - microchip,ksz8563
> > +
> > +  reset-gpios:
> > +    description:
> > +      Should be a gpio specifier for a reset line.
> > +    maxItems: 1
> > +
> > +  microchip,synclko-125:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    description:
> > +      Set if the output SYNCLKO frequency should be set to 125MHz instead
> > of 25MHz. +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> 
> You need to use unevaluatedProperties instead.
dt_binding_check is happy now

> 
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +
> > +    // Ethernet switch connected via SPI to the host, CPU port wired to
> > eth0: +    eth0 {
> > +        fixed-link {
> > +            speed = <1000>;
> > +            full-duplex;
> > +        };
> > +    };
> > +
> > +    spi0 {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        pinctrl-0 = <&pinctrl_spi_ksz>;
> > +        cs-gpios = <&pioC 25 0>;
> > +        id = <1>;
> > +
> > +        ksz9477: switch@0 {
> > +            compatible = "microchip,ksz9477";
> > +            reg = <0>;
> > +            reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
> > +
> > +            spi-max-frequency = <44000000>;
> > +            spi-cpha;
> > +            spi-cpol;
> 
> Are these 2 optional or required? Being optional is rare as most
> devices support 1 mode, but not unheard of.
From the data sheet:
"Input data on SDI is latched on the rising edge of serial clock SCL. Output
data on SDO is clocked on the falling edge of SCL." Clock has inverted 
polarity in all diagrams.

> In general, you shouldn't
> need them as the driver should know how to configure the mode if the h/w
> is fixed.
I will check this in the driver. This also requires updating
at91-sama5d2_icp.dts. I personally use I2C instead of SPI on this chip.

> > [...]




^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH net-next v3 05/12] net: dsa: microchip: ksz9477: move chip reset to ksz9477_switch_init()
  2020-11-18 20:30 ` [PATCH net-next v3 05/12] net: dsa: microchip: ksz9477: move chip reset to ksz9477_switch_init() Christian Eggers
@ 2020-11-20 22:49   ` Vladimir Oltean
  0 siblings, 0 replies; 32+ messages in thread
From: Vladimir Oltean @ 2020-11-20 22:49 UTC (permalink / raw)
  To: Christian Eggers
  Cc: Jakub Kicinski, Andrew Lunn, Richard Cochran, Rob Herring,
	Vivien Didelot, David S . Miller, Kurt Kanzenbach,
	George McCollister, Marek Vasut, Helmut Grohne, Paul Barker,
	Codrin Ciubotariu, Tristram Ha, Woojung Huh,
	Microchip Linux Driver Support, netdev, devicetree, linux-kernel

On Wed, Nov 18, 2020 at 09:30:06PM +0100, Christian Eggers wrote:
> The next patch will add basic interrupt support. Chip reset must be
> performed before requesting the IRQ, so move this from ksz9477_setup()
> to ksz9477_init().
> 
> Signed-off-by: Christian Eggers <ceggers@arri.de>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH net-next v3 06/12] net: dsa: microchip: ksz9477: basic interrupt support
  2020-11-18 20:30 ` [PATCH net-next v3 06/12] net: dsa: microchip: ksz9477: basic interrupt support Christian Eggers
@ 2020-11-20 23:03   ` Vladimir Oltean
  0 siblings, 0 replies; 32+ messages in thread
From: Vladimir Oltean @ 2020-11-20 23:03 UTC (permalink / raw)
  To: Christian Eggers
  Cc: Jakub Kicinski, Andrew Lunn, Richard Cochran, Rob Herring,
	Vivien Didelot, David S . Miller, Kurt Kanzenbach,
	George McCollister, Marek Vasut, Helmut Grohne, Paul Barker,
	Codrin Ciubotariu, Tristram Ha, Woojung Huh,
	Microchip Linux Driver Support, netdev, devicetree, linux-kernel

On Wed, Nov 18, 2020 at 09:30:07PM +0100, Christian Eggers wrote:
> Interrupts are required for TX time stamping. Probably they could also
> be used for PHY connection status.
> 
> This patch only adds the basic infrastructure for interrupts, no
> interrupts are finally enabled nor handled.
> 
> Signed-off-by: Christian Eggers <ceggers@arri.de>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

> +static int ksz9477_enable_port_interrupts(struct ksz_device *dev, bool enable)
> +{
> +	u32 data, mask = GENMASK(dev->port_cnt - 1, 0);
> +	int ret;
> +
> +	ret = ksz_read32(dev, REG_SW_PORT_INT_MASK__4, &data);
> +	if (ret)
> +		return ret;
> +
> +	/* bits in REG_SW_PORT_INT_MASK__4 are low active */

s/low active/active low/

> +	if (enable)
> +		data &= ~mask;
> +	else
> +		data |= mask;
> +
> +	return ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data);
> +}

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH net-next v3 07/12] net: dsa: microchip: ksz9477: add Posix clock support for chip PTP clock
  2020-11-18 20:30 ` [PATCH net-next v3 07/12] net: dsa: microchip: ksz9477: add Posix clock support for chip PTP clock Christian Eggers
@ 2020-11-20 23:14   ` Vladimir Oltean
  0 siblings, 0 replies; 32+ messages in thread
From: Vladimir Oltean @ 2020-11-20 23:14 UTC (permalink / raw)
  To: Christian Eggers
  Cc: Jakub Kicinski, Andrew Lunn, Richard Cochran, Rob Herring,
	Vivien Didelot, David S . Miller, Kurt Kanzenbach,
	George McCollister, Marek Vasut, Helmut Grohne, Paul Barker,
	Codrin Ciubotariu, Tristram Ha, Woojung Huh,
	Microchip Linux Driver Support, netdev, devicetree, linux-kernel

On Wed, Nov 18, 2020 at 09:30:08PM +0100, Christian Eggers wrote:
> Implement routines (adjfine, adjtime, gettime and settime) for
> manipulating the chip's PTP clock.
> 
> Signed-off-by: Christian Eggers <ceggers@arri.de>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH net-next v3 09/12] net: dsa: microchip: ksz9477: initial hardware time stamping support
  2020-11-18 20:30 ` [PATCH net-next v3 09/12] net: dsa: microchip: ksz9477: initial hardware time stamping support Christian Eggers
@ 2020-11-20 23:27   ` Vladimir Oltean
  0 siblings, 0 replies; 32+ messages in thread
From: Vladimir Oltean @ 2020-11-20 23:27 UTC (permalink / raw)
  To: Christian Eggers
  Cc: Jakub Kicinski, Andrew Lunn, Richard Cochran, Rob Herring,
	Vivien Didelot, David S . Miller, Kurt Kanzenbach,
	George McCollister, Marek Vasut, Helmut Grohne, Paul Barker,
	Codrin Ciubotariu, Tristram Ha, Woojung Huh,
	Microchip Linux Driver Support, netdev, devicetree, linux-kernel

On Wed, Nov 18, 2020 at 09:30:10PM +0100, Christian Eggers wrote:
> Add control routines required for TX hardware time stamping.
> 
> The KSZ9563 only supports one step time stamping
> (HWTSTAMP_TX_ONESTEP_P2P), which requires linuxptp-2.0 or later.
> 
> Currently, only P2P delay measurement is supported. See patchwork
> discussion and comments in ksz9477_ptp_init() for details:
> https://patchwork.ozlabs.org/project/netdev/patch/20201019172435.4416-8-ceggers@arri.de/
> 
> Signed-off-by: Christian Eggers <ceggers@arri.de>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

> +static int ksz9477_set_hwtstamp_config(struct ksz_device *dev, int port,
> +				       struct hwtstamp_config *config)
> +{
> +	struct ksz_port *prt = &dev->ports[port];
> +
> +	/* reserved for future extensions */
> +	if (config->flags)
> +		return -EINVAL;
> +
> +	switch (config->tx_type) {
> +	case HWTSTAMP_TX_OFF:
> +		prt->hwts_tx_en = false;
> +		break;
> +	case HWTSTAMP_TX_ONESTEP_P2P:
> +		prt->hwts_tx_en = true;
> +		break;
> +	default:
> +		return -ERANGE;
> +	}
> +
> +	switch (config->rx_filter) {
> +	case HWTSTAMP_FILTER_NONE:
> +		break;
> +	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
> +	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
> +		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L4_EVENT;
> +		break;
> +	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
> +	case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
> +		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;
> +		break;
> +	case HWTSTAMP_FILTER_PTP_V2_EVENT:
> +	case HWTSTAMP_FILTER_PTP_V2_SYNC:
> +		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
> +		break;
> +	case HWTSTAMP_FILTER_ALL:

Putting anything in the same "case" statement as "default" is useless.

> +	default:
> +		config->rx_filter = HWTSTAMP_FILTER_NONE;
> +		return -ERANGE;
> +	}
> +
> +	return 0;
> +}

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH net-next v3 12/12] net: dsa: microchip: ksz9477: add periodic output support
  2020-11-18 20:30 ` [PATCH net-next v3 12/12] net: dsa: microchip: ksz9477: add periodic output support Christian Eggers
@ 2020-11-20 23:48   ` Vladimir Oltean
  0 siblings, 0 replies; 32+ messages in thread
From: Vladimir Oltean @ 2020-11-20 23:48 UTC (permalink / raw)
  To: Christian Eggers
  Cc: Jakub Kicinski, Andrew Lunn, Richard Cochran, Rob Herring,
	Vivien Didelot, David S . Miller, Kurt Kanzenbach,
	George McCollister, Marek Vasut, Helmut Grohne, Paul Barker,
	Codrin Ciubotariu, Tristram Ha, Woojung Huh,
	Microchip Linux Driver Support, netdev, devicetree, linux-kernel

On Wed, Nov 18, 2020 at 09:30:13PM +0100, Christian Eggers wrote:
> The KSZ9563 has a Trigger Output Unit (TOU) which can be used to
> generate periodic signals.
> 
> The pulse length can be altered via a device attribute.
> 
> Tested on a Microchip KSZ9563 switch.
> 
> Signed-off-by: Christian Eggers <ceggers@arri.de>
> ---
>  drivers/net/dsa/microchip/ksz9477_ptp.c | 197 +++++++++++++++++++++++-
>  drivers/net/dsa/microchip/ksz_common.h  |   5 +
>  2 files changed, 201 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz9477_ptp.c b/drivers/net/dsa/microchip/ksz9477_ptp.c
> index ce3fdc9a1f9e..3174574d52f6 100644
> --- a/drivers/net/dsa/microchip/ksz9477_ptp.c
> +++ b/drivers/net/dsa/microchip/ksz9477_ptp.c
> @@ -90,6 +90,20 @@ static int ksz9477_ptp_tou_cycle_count_set(struct ksz_device *dev, u16 count)
>  	return 0;
>  }
>  
> +static int ksz9477_ptp_tou_pulse_verify(u64 pulse_ns)
> +{
> +	u32 data;
> +
> +	if (pulse_ns & 0x3)
> +		return -EINVAL;
> +
> +	data = (pulse_ns / 8);
> +	if (data != (data & TRIG_PULSE_WIDTH_M))
> +		return -ERANGE;
> +
> +	return 0;
> +}
> +
>  static int ksz9477_ptp_tou_pulse_set(struct ksz_device *dev, u32 pulse_ns)
>  {
>  	u32 data;
> @@ -196,6 +210,7 @@ static int ksz9477_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
>  	return ret;
>  }
>  
> +static int ksz9477_ptp_restart_perout(struct ksz_device *dev);
>  static int ksz9477_ptp_enable_pps(struct ksz_device *dev, int on);
>  
>  static int ksz9477_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
> @@ -241,6 +256,15 @@ static int ksz9477_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
>  	case KSZ_PTP_TOU_IDLE:
>  		break;
>  
> +	case KSZ_PTP_TOU_PEROUT:
> +		dev_info(dev->dev, "Restarting periodic output signal\n");

Isn't this a bit too verbose, or is there something for the user to be
concerned about?

> +
> +		ret = ksz9477_ptp_restart_perout(dev);
> +		if (ret)
> +			goto error_return;
> +
> +		break;
> +
>  	case KSZ_PTP_TOU_PPS:
>  		dev_info(dev->dev, "Restarting PPS\n");
>  
> @@ -358,6 +382,15 @@ static int ksz9477_ptp_settime(struct ptp_clock_info *ptp,
>  	case KSZ_PTP_TOU_IDLE:
>  		break;
>  
> +	case KSZ_PTP_TOU_PEROUT:
> +		dev_info(dev->dev, "Restarting periodic output signal\n");
> +
> +		ret = ksz9477_ptp_restart_perout(dev);
> +		if (ret)
> +			goto error_return;
> +
> +		break;
> +
>  	case KSZ_PTP_TOU_PPS:
>  		dev_info(dev->dev, "Restarting PPS\n");
>  
> @@ -377,6 +410,159 @@ static int ksz9477_ptp_settime(struct ptp_clock_info *ptp,
>  	return ret;
>  }
>  
> +static int ksz9477_ptp_configure_perout(struct ksz_device *dev, u32 cycle_width_ns,

Watch out for 80 characters, please!

> +					u16 cycle_count, u32 pulse_width_ns,
> +					struct timespec64 const *target_time)
> +{
> +	int ret;
> +	u32 trig_ctrl;

Reverse Christmas tree.

> +
> +	/* Enable notify, set rising edge, set periodic pattern */
> +	trig_ctrl = TRIG_NOTIFY | (TRIG_POS_PERIOD << TRIG_PATTERN_S);
> +	ret = ksz_write32(dev, REG_TRIG_CTRL__4, trig_ctrl);
> +	if (ret)
> +		return ret;
> +
> +	ret = ksz9477_ptp_tou_cycle_width_set(dev, cycle_width_ns);
> +	if (ret)
> +		return ret;
> +
> +	ksz9477_ptp_tou_cycle_count_set(dev,  cycle_count);
> +	if (ret)
> +		return ret;
> +
> +	ret = ksz9477_ptp_tou_pulse_set(dev, pulse_width_ns);
> +	if (ret)
> +		return ret;
> +
> +	ret = ksz9477_ptp_tou_target_time_set(dev, target_time);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int ksz9477_ptp_enable_perout(struct ksz_device *dev,
> +				     struct ptp_perout_request const *perout_request, int on)
> +{
> +	u32 gpio_stat0;
> +	u64 cycle_width_ns;
> +	int ret;
> +
> +	if (dev->ptp_tou_mode != KSZ_PTP_TOU_PEROUT && dev->ptp_tou_mode != KSZ_PTP_TOU_IDLE)
> +		return -EBUSY;
> +
> +	ret = ksz9477_ptp_tou_reset(dev, 0);
> +	if (ret)
> +		return ret;
> +
> +	if (!on) {
> +		dev->ptp_tou_mode = KSZ_PTP_TOU_IDLE;
> +		return 0;  /* success */
> +	}
> +
> +	dev->ptp_perout_target_time_first.tv_sec  = perout_request->start.sec;
> +	dev->ptp_perout_target_time_first.tv_nsec = perout_request->start.nsec;
> +
> +	dev->ptp_perout_period.tv_sec = perout_request->period.sec;
> +	dev->ptp_perout_period.tv_nsec = perout_request->period.nsec;
> +
> +	cycle_width_ns = timespec64_to_ns(&dev->ptp_perout_period);
> +	if ((cycle_width_ns & GENMASK(31, 0)) != cycle_width_ns)
> +		return -EINVAL;
> +
> +	if (perout_request->flags & PTP_PEROUT_DUTY_CYCLE) {
> +		u64 value = perout_request->on.sec * NSEC_PER_SEC +
> +			    perout_request->on.nsec;
> +
> +		ret = ksz9477_ptp_tou_pulse_verify(value);
> +		if (ret)
> +			return ret;
> +
> +		dev->ptp_perout_pulse_width_ns = value;
> +	}

It is not guaranteed that user space will set this flag. Shouldn't you
assign a default value for the pulse width? I don't know, half the
period should be a good default.

Also, please reject PTP_PEROUT_ONE_SHOT and PTP_PEROUT_PHASE, since you
don't do anything with them, but user space might be led into believing
otherwise.

> +
> +	ret = ksz9477_ptp_configure_perout(dev, cycle_width_ns,
> +					   dev->ptp_perout_cycle_count,
> +					   dev->ptp_perout_pulse_width_ns,
> +					   &dev->ptp_perout_target_time_first);
> +	if (ret)
> +		return ret;
> +
> +	/* Activate trigger unit */
> +	ret = ksz9477_ptp_tou_start(dev, NULL);
> +	if (ret)
> +		return ret;
> +
> +	/* Check error flag:
> +	 * - the ACTIVE flag is NOT cleared an error!
> +	 */
> +	ret = ksz_read32(dev, REG_PTP_TRIG_STATUS__4, &gpio_stat0);
> +	if (ret)
> +		return ret;
> +
> +	if (gpio_stat0 & (1 << (0 + TRIG_ERROR_S))) {

What is the role of this "0 +" term here?

> +		dev_err(dev->dev, "%s: Trigger unit0 error!\n", __func__);
> +		ret = -EIO;
> +		/* Unit will be reset on next access */
> +		return ret;
> +	}
> +
> +	dev->ptp_tou_mode = KSZ_PTP_TOU_PEROUT;
> +	return 0;
> +}

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x
  2020-11-19 18:51     ` Tristram.Ha
  2020-11-19 20:16       ` Christian Eggers
@ 2020-11-21  1:26       ` Vladimir Oltean
  2020-11-22  1:36         ` Richard Cochran
  2020-11-22  1:53         ` Richard Cochran
  2020-11-25 21:08       ` Christian Eggers
  2 siblings, 2 replies; 32+ messages in thread
From: Vladimir Oltean @ 2020-11-21  1:26 UTC (permalink / raw)
  To: Tristram.Ha
  Cc: ceggers, kuba, andrew, richardcochran, robh+dt, vivien.didelot,
	davem, kurt.kanzenbach, george.mccollister, marex, helmut.grohne,
	pbarker, Codrin.Ciubotariu, Woojung.Huh, UNGLinuxDriver, netdev,
	devicetree, linux-kernel

On Thu, Nov 19, 2020 at 06:51:15PM +0000, Tristram.Ha@microchip.com wrote:
> The initial proposal in tag_ksz.c is for the switch driver to provide callback functions
> to handle receiving and transmitting.  Then each switch driver can be added to
> process the tail tag in its own driver and leave tag_ksz.c unchanged.
>
> It was rejected because of wanting to keep tag_ksz.c code and switch driver code
> separate and concern about performance.
>
> Now tag_ksz.c is filled with PTP code that is not relevant for other switches and will
> need to be changed again when another switch driver with PTP function is added.
>
> Can we implement that callback mechanism?

I, too, lack the context here. But it sounds like feedback that Andrew
would give.

If you don't like the #ifdef's, I am not in love with them either. But
maybe Christian is just optimizing too aggressively, and doesn't actually
need to put those #ifdef's there and provide stub implementations, but
could actually just leave the ksz9477_rcv_timestamp and ksz9477_xmit_timestamp
always compiled-in, and "dead at runtime" in the case there is no PTP.

If there is something else you don't like, what is it? If you know that
other KSZ switches don't implement timestamping in the same way, well,
we don't know that. I thought that it's generally up to the second
implementer to recognize which parts of the code are common and should
be reused, not for the first one to guess. I would not add function
pointers for a single implementation if they don't have a clear
justification.

> One issue with transmission with PTP enabled is that the tail tag needs to contain 4
> additional bytes.  When the PTP function is off the bytes are not added.  This should
> be monitored all the time.
>
> The extra 4 bytes are only used for 1-step Pdelay_Resp.  It should contain the receive
> timestamp of previous Pdelay_Req with latency adjusted.  The correction field in
> Pdelay_Resp should be zero.  It may be a hardware bug to have wrong UDP checksum
> when the message is sent.

It "may" be a hardware bug? Are you unsure or polite?
As for the phrase "the correction field in Pdelay_Resp should be zero".
Consider the case where there is an E2E TC switch attached to that port.
It will update the correctionField of the Pdelay_Req message. Then the
application stack running on this ksz9477 switch is forced by the
standard to copy the correctionField as-is from the Pdelay_Req into the
Pdelay_Resp message. So that correctionField is never guaranteed to be
zero, even if Christian doesn't fiddle with it within the driver. Are
you saying that for proper UDP checksum calculation, the driver should
be forcing the correctionField to zero and moving that value into the
tail tag?

> I think the right implementation is for the driver to remember this receive timestamp
> of Pdelay_Req and puts it in the tail tag when it sees a 1-step Pdelay_Resp is sent.

I have mixed feelings about this. IIUC, you're saying "let's implement a
fixed-size FIFO of RX timestamps of Pdelay_Req messages, and let's match
them on TX to Pdelay_Resp messages, by {sequenceId, domainNumber}."

But how deep should we make that FIFO? I.e. how many Pdelay_Req messages
should we expect before the user space will inject back a Pdelay_Resp
for transmission?

Again, consider the case of an E2E TC attached to a ksz9477 port. Even
if we run peer delay, it's not guaranteed that we only have one peer.
That E2E TC might connect us to a plethora of other peers. And the more
peers we are connected to, the higher the chance that the size of this
Pdelay_Req RX timestamp FIFO will not be adequately chosen.

> There is one more requirement that is a little difficult to do.  The calculated peer delay
> needs to be programmed in hardware register, but the regular PTP stack has no way to
> send that command.  I think the driver has to do its own calculation by snooping on the
> Pdelay_Req/Pdelay_Resp/Pdelay_Resp_Follow_Up messages.

What register, and what does the switch do with this peer delay information?

> The receive and transmit latencies are different for different connected speed.  So the
> driver needs to change them when the link changes.  For that reason the PTP stack
> should not use its own latency values as generally the application does not care about
> the linked speed.

The thing is, ptp4l already has ingressLatency and egressLatency
settings, and I would not be surprised if those config options would get
extended to cover values at multiple link speeds.

In the general case, the ksz9477 MAC could be attached to any external
PHY, having its own propagation delay characteristics, or any number of
other things that cause clock domain crossings. I'm not sure how feasible
it is for the kernel to abstract this away completely, and adjust
timestamps automatically based on any and all combinations of MAC and
PHY. Maybe this is just wishful thinking.

Oh, and by the way, Christian, I'm not even sure if you aren't in fact
just beating around the bush with these tstamp_rx_latency_ns and
tstamp_tx_latency_ns values? I mean, the switch adds the latency value
to the timestamps. And you, from the driver, read the value of the
register, so you can subtract the value from the timestamp, to
compensate for its correction. So, all in all, there is no net latency
compensation seen by the outside world?! If that is the case, can't you
just set the latency registers to zero, do your compensation from the
application stack and call it a day?

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x
  2020-11-21  1:26       ` Vladimir Oltean
@ 2020-11-22  1:36         ` Richard Cochran
  2020-11-22  1:53         ` Richard Cochran
  1 sibling, 0 replies; 32+ messages in thread
From: Richard Cochran @ 2020-11-22  1:36 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: Tristram.Ha, ceggers, kuba, andrew, robh+dt, vivien.didelot,
	davem, kurt.kanzenbach, george.mccollister, marex, helmut.grohne,
	pbarker, Codrin.Ciubotariu, Woojung.Huh, UNGLinuxDriver, netdev,
	devicetree, linux-kernel

On Sat, Nov 21, 2020 at 03:26:11AM +0200, Vladimir Oltean wrote:
> On Thu, Nov 19, 2020 at 06:51:15PM +0000, Tristram.Ha@microchip.com wrote:
> > The receive and transmit latencies are different for different connected speed.  So the
> > driver needs to change them when the link changes.  For that reason the PTP stack
> > should not use its own latency values as generally the application does not care about
> > the linked speed.
> 
> The thing is, ptp4l already has ingressLatency and egressLatency
> settings, and I would not be surprised if those config options would get
> extended to cover values at multiple link speeds.
> 
> In the general case, the ksz9477 MAC could be attached to any external
> PHY, having its own propagation delay characteristics, or any number of
> other things that cause clock domain crossings. I'm not sure how feasible
> it is for the kernel to abstract this away completely, and adjust
> timestamps automatically based on any and all combinations of MAC and
> PHY. Maybe this is just wishful thinking.

The idea that the driver will correctly adjust time stamps according
to link speed sounds nice in theory, but in practice it fails.  There
is a at least one other driver that attempted this, but, surprise,
surprise, the hard coded correction values turned out to be wrong.

I think the best way would be to let user space monitor the link speed
and apply the matching correction value.  That way, we avoid bogus,
hard coded values in kernel space.  (This isn't implemented in
linuxptp, but it certainly could be.)

Thanks,
Richard

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x
  2020-11-21  1:26       ` Vladimir Oltean
  2020-11-22  1:36         ` Richard Cochran
@ 2020-11-22  1:53         ` Richard Cochran
  1 sibling, 0 replies; 32+ messages in thread
From: Richard Cochran @ 2020-11-22  1:53 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: Tristram.Ha, ceggers, kuba, andrew, robh+dt, vivien.didelot,
	davem, kurt.kanzenbach, george.mccollister, marex, helmut.grohne,
	pbarker, Codrin.Ciubotariu, Woojung.Huh, UNGLinuxDriver, netdev,
	devicetree, linux-kernel

On Sat, Nov 21, 2020 at 03:26:11AM +0200, Vladimir Oltean wrote:
> On Thu, Nov 19, 2020 at 06:51:15PM +0000, Tristram.Ha@microchip.com wrote:

> > I think the right implementation is for the driver to remember this receive timestamp
> > of Pdelay_Req and puts it in the tail tag when it sees a 1-step Pdelay_Resp is sent.

As long as this is transparent to user space, it could work.  Remember
that user space simply copies the correction field from the Request
into the Response.  If the driver correctly accumulates the turnaround
time into the correction field of the response, then all is well.
 
> I have mixed feelings about this. IIUC, you're saying "let's implement a
> fixed-size FIFO of RX timestamps of Pdelay_Req messages, and let's match
> them on TX to Pdelay_Resp messages, by {sequenceId, domainNumber}."
> 
> But how deep should we make that FIFO? I.e. how many Pdelay_Req messages
> should we expect before the user space will inject back a Pdelay_Resp
> for transmission?

Good question.  Normally you would expect just one Request pending at
any one time, but nothing guarantees that, and so the driver would
have to match the Req/Resp exactly and deal with rogue/buggy requests
and responses.

Thanks,
Richard

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x
  2020-11-19 18:51     ` Tristram.Ha
  2020-11-19 20:16       ` Christian Eggers
  2020-11-21  1:26       ` Vladimir Oltean
@ 2020-11-25 21:08       ` Christian Eggers
  2020-11-26 16:53         ` Christian Eggers
  2 siblings, 1 reply; 32+ messages in thread
From: Christian Eggers @ 2020-11-25 21:08 UTC (permalink / raw)
  To: Tristram.Ha
  Cc: olteanv, kuba, andrew, richardcochran, robh+dt, vivien.didelot,
	davem, kurt.kanzenbach, george.mccollister, marex, helmut.grohne,
	pbarker, Codrin.Ciubotariu, Woojung.Huh, UNGLinuxDriver, netdev,
	devicetree, linux-kernel

I need some help from Microchip, please read below.

On Thursday, 19 November 2020, 19:51:15 CET, Tristram.Ha@microchip.com wrote:
> There is one more requirement that is a little difficult to do.  The calculated peer delay
> needs to be programmed in hardware register, but the regular PTP stack has no way to
> send that command.  I think the driver has to do its own calculation by snooping on the
> Pdelay_Req/Pdelay_Resp/Pdelay_Resp_Follow_Up messages.

In an (offline) discussion with Vladimir we discovered, that the KSZ switch
behaves different as ptp4l expects: 

The KSZ switch forwards PTP (e.g. SYNC) messages in hardware (with updating
the correction field). For this, the peer delays need be configured for each
port.

ptp4l in turn expects to do the forwarding in software (for the P2P_TC clock
configuration). For this, no hardware configuration of the peer delay is
necessary. But due to limitations of currently available hardware, this TC
forwarding is currently only supported for 2 step clocks, as a one-step clock
would probably fully replace the originTimestamp field (similar as a BC, but
not as a TC).

Vladimir suggested to configure an ACL in the KSZ switch to block forwarding
of PTP messages between the user ports and to run ptp4l as BC. My idea is to
simply block forwarding of UDP messages with destination ports 319+320 and
L2 messages with the PTP Ether-Type.

I installed the following ACL (for UDP) in the Port ACL Access registers 0-F:
|_0__1__2__3__4__5__6__7__8__9__A__B__C__D__E__F
| 00 39 01 40 01 3F 42 22 00 00 00 60 00 00 00 01
ACL index: 0

Match: 
- MD=11 (L4)
- ENB=10 (UDP ports)
- S/D=0 (dst)
- EQ=1 (equal)
- MAX_PORT=320
- MIN_PORT=319
- PC=01 (min or max)
- PRO=17 (UDP, don't care?)
- FME=0 (disabled)

Action:
- PM=0 (disabled)
- P=0 (don't care)
- RPE=0 (disabled)
- RP=0 (don't care)
- MM=11 (replace)
- PORT_FWD_MAP: all ports to 0

Processing entry:
- Ruleset=0x0001
- FRN=0

Unfortunately, with this configuration PTP messages are still forwarded from
port 1 to port 2. Although I was successful in blocking other communication
(e.g. by MAC address), the matching rules above seem not to work. Is there an
error in the ACL, or is forwarding of PTP traffic independent of configured
ACLs?

regards
Christian




^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x
  2020-11-25 21:08       ` Christian Eggers
@ 2020-11-26 16:53         ` Christian Eggers
  2020-11-30 21:01           ` Tristram.Ha
  0 siblings, 1 reply; 32+ messages in thread
From: Christian Eggers @ 2020-11-26 16:53 UTC (permalink / raw)
  To: Tristram.Ha
  Cc: olteanv, kuba, andrew, richardcochran, robh+dt, vivien.didelot,
	davem, kurt.kanzenbach, george.mccollister, marex, helmut.grohne,
	pbarker, Codrin.Ciubotariu, Woojung.Huh, UNGLinuxDriver, netdev,
	devicetree, linux-kernel

Hi Microchip,

as ACL based blocking of PTP traffic seems not to work, I tried to install MAC
based static lookup rules on the switch I successfully managed to block other
non-PTP traffic, but for PTP the lookup table entry (see below) seems not to
work. Incoming SYNC messages on port are still forwarded to port 2.

The table entry is based on the multicast MAC used for PTP. With PTP domains!=0
there could be 128 possible MAC addresses that needs to blocked (but the switch
has only 16 entries in the static table). Is there any way to block the whole
PTP multicast address range (01:00:5E:00:01:81-01:00:5E:00:01:ff)? The data sheet
mentions that the static address table can be used for multicast addresses,
so there should be a way.

Alternatively, is there a hidden "disable TC" setting which disables the
transparent clock entirely?

regards
Christian

        Look-up Tables
        ALU_STAT_CTL 00000001  TABLE_INDEX       0       START_FINISH   idle             TABLE_SELECT Static Address
                               ACTION     read

        Static Address Table
        ALU_VAL_A    80000000  VALID      valid            SRC_FILTER     disabled       DST_FILTER   disabled
                               PRIORITY             0      MSTP                     0
        ALU_VAL_B    80000000  OVERRIDE   enabled          USE_FID        disabled
                               PRT3_FWD   disabled         PRT2_FWD       disabled       PRT1_FWD     disabled
        ALU_VAL_C    00000100  FID                  0      MAC_0_1              01:00
        ALU_VAL_D    5E000181  MAC_2_5    5E:00:01:81


On Wednesday, 25 November 2020, 22:08:39 CET, Christian Eggers wrote:
> I need some help from Microchip, please read below.
> 
> On Thursday, 19 November 2020, 19:51:15 CET, Tristram.Ha@microchip.com wrote:
> > There is one more requirement that is a little difficult to do.  The calculated peer delay
> > needs to be programmed in hardware register, but the regular PTP stack has no way to
> > send that command.  I think the driver has to do its own calculation by snooping on the
> > Pdelay_Req/Pdelay_Resp/Pdelay_Resp_Follow_Up messages.
> 
> In an (offline) discussion with Vladimir we discovered, that the KSZ switch
> behaves different as ptp4l expects: 
> 
> The KSZ switch forwards PTP (e.g. SYNC) messages in hardware (with updating
> the correction field). For this, the peer delays need be configured for each
> port.
> 
> ptp4l in turn expects to do the forwarding in software (for the P2P_TC clock
> configuration). For this, no hardware configuration of the peer delay is
> necessary. But due to limitations of currently available hardware, this TC
> forwarding is currently only supported for 2 step clocks, as a one-step clock
> would probably fully replace the originTimestamp field (similar as a BC, but
> not as a TC).
> 
> Vladimir suggested to configure an ACL in the KSZ switch to block forwarding
> of PTP messages between the user ports and to run ptp4l as BC. My idea is to
> simply block forwarding of UDP messages with destination ports 319+320 and
> L2 messages with the PTP Ether-Type.
> 
> I installed the following ACL (for UDP) in the Port ACL Access registers 0-F:
> |_0__1__2__3__4__5__6__7__8__9__A__B__C__D__E__F
> | 00 39 01 40 01 3F 42 22 00 00 00 60 00 00 00 01
> ACL index: 0
> 
> Match: 
> - MD=11 (L4)
> - ENB=10 (UDP ports)
> - S/D=0 (dst)
> - EQ=1 (equal)
> - MAX_PORT=320
> - MIN_PORT=319
> - PC=01 (min or max)
> - PRO=17 (UDP, don't care?)
> - FME=0 (disabled)
> 
> Action:
> - PM=0 (disabled)
> - P=0 (don't care)
> - RPE=0 (disabled)
> - RP=0 (don't care)
> - MM=11 (replace)
> - PORT_FWD_MAP: all ports to 0
> 
> Processing entry:
> - Ruleset=0x0001
> - FRN=0
> 
> Unfortunately, with this configuration PTP messages are still forwarded from
> port 1 to port 2. Although I was successful in blocking other communication
> (e.g. by MAC address), the matching rules above seem not to work. Is there an
> error in the ACL, or is forwarding of PTP traffic independent of configured
> ACLs?
> 
> regards
> Christian
> 





^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x
  2020-11-26 16:53         ` Christian Eggers
@ 2020-11-30 21:01           ` Tristram.Ha
  2020-11-30 22:28             ` Richard Cochran
  0 siblings, 1 reply; 32+ messages in thread
From: Tristram.Ha @ 2020-11-30 21:01 UTC (permalink / raw)
  To: ceggers
  Cc: olteanv, kuba, andrew, richardcochran, robh+dt, vivien.didelot,
	davem, kurt.kanzenbach, george.mccollister, marex, helmut.grohne,
	pbarker, Codrin.Ciubotariu, Woojung.Huh, UNGLinuxDriver, netdev,
	devicetree, linux-kernel

> Hi Microchip,
> 
> as ACL based blocking of PTP traffic seems not to work, I tried to install MAC
> based static lookup rules on the switch I successfully managed to block other
> non-PTP traffic, but for PTP the lookup table entry (see below) seems not to
> work. Incoming SYNC messages on port are still forwarded to port 2.
> 
> The table entry is based on the multicast MAC used for PTP. With PTP
> domains!=0
> there could be 128 possible MAC addresses that needs to blocked (but the
> switch
> has only 16 entries in the static table). Is there any way to block the whole
> PTP multicast address range (01:00:5E:00:01:81-01:00:5E:00:01:ff)? The data
> sheet
> mentions that the static address table can be used for multicast addresses,
> so there should be a way.
> 
> Alternatively, is there a hidden "disable TC" setting which disables the
> transparent clock entirely?

The 1588 PTP engine in the KSZ switches was designed to be controlled closely by
a PTP stack, so it is a little difficult to use when there is a layer of kernel support
between the application and the driver.

The default mode to use should be 1-step E2E where the switch acts as an E2E
Transparent Clock.

The 16-bit register 0x514 specifies the basic operation mode of the switch.

Bit 0 is for 1-step clock mode.
Bit 1 is for master mode, which should be off when the clock is acting as a master.
Bit 2 is for P2P mode.
Bit 7 stops the automatic forwarding and every PTP message goes to the host port.
This is the mode to use when the switch acts as a Boundary Clock or 2-step Clock.

When master mode is on Delay_Resp will not be forwarded to the host port.
When master mode is off Delay_Req will not be forwarded to the host port.

When P2P mode is off Pdelay_Req/Pdelay_Resp/Pdelay_Resp_Follow_Up will not
be forwarded to the host port.
When P2P mode is on those messages can be sent and received even though the port
Is closed for normal communication.

Bit 5 recognizes L2 PTP messages and the switch acts accordingly.
Bit 4 is for UDPv4 while bit 3 is for UDPv6.

It is rather pointless to actively filter certain PTP messages through other means.
It is better to leave the kernel PTP receive filter as coarse as possible.

When using P2P in 1-step clock mode the port id in the PTP header is automatically
changed by hardware to be the same as the real port, so it is useless to arbitrarily
use a different port id.  The original intent is to send 1 Pdelay_Req and receive
several Pdelay_Resp in each port.

The calculated peer delay from the port needs to be programmed to the port
register so that the Sync message can be compensated correctly while it travels
through the switches.  This poses a problem as the driver normally does not do the
calculation.

The 2-step clock mode avoids some of the mentioned issues.  However there are some
hardware bugs associated with this operation mode and it is not recommended to use.

For some profiles that require 2-step operation like gPTP there are ways to workaround.

For Sync it is quite simple to send Follow_Up after it even though the Sync contains the
transmit timestamp.  The Follow_Up just repeats that information.

For 2-step Pdelay_Resp it is harder as the hardware puts the turnaround time in
the correction field.  The driver workaround is to report the transmit timestamp
differently such that it is the same as Pdelay_Req receive timestamp so that the net
calculation of the peer delay is just the same as receiving 1-step Pdelay_Resp.

I will try my own implementation to see how these steps can be done.


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x
  2020-11-30 21:01           ` Tristram.Ha
@ 2020-11-30 22:28             ` Richard Cochran
  0 siblings, 0 replies; 32+ messages in thread
From: Richard Cochran @ 2020-11-30 22:28 UTC (permalink / raw)
  To: Tristram.Ha
  Cc: ceggers, olteanv, kuba, andrew, robh+dt, vivien.didelot, davem,
	kurt.kanzenbach, george.mccollister, marex, helmut.grohne,
	pbarker, Codrin.Ciubotariu, Woojung.Huh, UNGLinuxDriver, netdev,
	devicetree, linux-kernel

On Mon, Nov 30, 2020 at 09:01:25PM +0000, Tristram.Ha@microchip.com wrote:
> The 1588 PTP engine in the KSZ switches was designed to be controlled closely by
> a PTP stack, so it is a little difficult to use when there is a layer of kernel support
> between the application and the driver.

Are you saying that linuxptp is not a PTP stack?

Maybe it would be wiser to design your HW so that it can work under Linux?

Nah, nobody cares about Linux support these days.

^ permalink raw reply	[flat|nested] 32+ messages in thread

end of thread, other threads:[~2020-11-30 22:29 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-18 20:30 [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x Christian Eggers
2020-11-18 20:30 ` [PATCH net-next v3 01/12] dt-bindings: net: dsa: convert ksz bindings document to yaml Christian Eggers
2020-11-19 13:42   ` Rob Herring
2020-11-19 13:48   ` Rob Herring
2020-11-19 20:22     ` Christian Eggers
2020-11-18 20:30 ` [PATCH net-next v3 02/12] net: dsa: microchip: support for "ethernet-ports" node Christian Eggers
2020-11-18 20:30 ` [PATCH net-next v3 03/12] net: dsa: microchip: rename ksz9477.c to ksz9477_main.c Christian Eggers
2020-11-18 20:30 ` [PATCH net-next v3 04/12] dt-bindings: net: dsa: microchip,ksz: add interrupt property Christian Eggers
2020-11-18 20:30 ` [PATCH net-next v3 05/12] net: dsa: microchip: ksz9477: move chip reset to ksz9477_switch_init() Christian Eggers
2020-11-20 22:49   ` Vladimir Oltean
2020-11-18 20:30 ` [PATCH net-next v3 06/12] net: dsa: microchip: ksz9477: basic interrupt support Christian Eggers
2020-11-20 23:03   ` Vladimir Oltean
2020-11-18 20:30 ` [PATCH net-next v3 07/12] net: dsa: microchip: ksz9477: add Posix clock support for chip PTP clock Christian Eggers
2020-11-20 23:14   ` Vladimir Oltean
2020-11-18 20:30 ` [PATCH net-next v3 08/12] net: ptp: add helper for one-step P2P clocks Christian Eggers
2020-11-18 20:30 ` [PATCH net-next v3 09/12] net: dsa: microchip: ksz9477: initial hardware time stamping support Christian Eggers
2020-11-20 23:27   ` Vladimir Oltean
2020-11-18 20:30 ` [PATCH net-next v3 10/12] net: dsa: microchip: ksz9477: remaining " Christian Eggers
2020-11-18 20:30 ` [PATCH net-next v3 11/12] net: dsa: microchip: ksz9477: add Pulse Per Second (PPS) support Christian Eggers
2020-11-18 20:30 ` [PATCH net-next v3 12/12] net: dsa: microchip: ksz9477: add periodic output support Christian Eggers
2020-11-20 23:48   ` Vladimir Oltean
2020-11-18 23:40 ` [PATCH net-next v3 00/12] net: dsa: microchip: PTP support for KSZ956x Vladimir Oltean
2020-11-19  5:28   ` Christian Eggers
2020-11-19 18:51     ` Tristram.Ha
2020-11-19 20:16       ` Christian Eggers
2020-11-21  1:26       ` Vladimir Oltean
2020-11-22  1:36         ` Richard Cochran
2020-11-22  1:53         ` Richard Cochran
2020-11-25 21:08       ` Christian Eggers
2020-11-26 16:53         ` Christian Eggers
2020-11-30 21:01           ` Tristram.Ha
2020-11-30 22:28             ` Richard Cochran

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).