[PATCH net-next v2 00/11] net: dsa: microchip: PTP support for KSZ956x

[PATCH net-next v2 00/11] net: dsa: microchip: PTP support for KSZ956x

[Christian Eggers]

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 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")

[PATCH net-next v2 01/11] dt-bindings: net: dsa: convert ksz bindings document to yaml

[Christian Eggers]

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       | 150 ++++++++++++++++++
 MAINTAINERS                                   |   2 +-
 3 files changed, 151 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..431ca5c498a8
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
@@ -0,0 +1,150 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%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
+
+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 1e7d1d71c125..3d173fcbf119 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11518,7 +11518,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

Re: [PATCH net-next v2 01/11] dt-bindings: net: dsa: convert ksz bindings document to yaml

[Vladimir Oltean]

On Thu, Nov 12, 2020 at 04:35:27PM +0100, Christian Eggers wrote:
> 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..431ca5c498a8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> @@ -0,0 +1,150 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause

Where is the "GPL-2.0-only OR BSD-2-Clause" license coming from? I see
that the Microchip KSZ driver is GPL-2.0, and the previous bindings
document had no license, which would also make it implicitly GPL I
believe.

Re: [PATCH net-next v2 01/11] dt-bindings: net: dsa: convert ksz bindings document to yaml

[Rob Herring]

On Thu, 12 Nov 2020 16:35:27 +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       | 150 ++++++++++++++++++
>  MAINTAINERS                                   |   2 +-
>  3 files changed, 151 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.yaml: 'oneOf' conditional failed, one must be fixed:
	'unevaluatedProperties' is a required property
	'additionalProperties' is a required property
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml: ignoring, error in schema: 
warning: no schema found in file: ./Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml


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

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.

Re: [PATCH net-next v2 01/11] dt-bindings: net: dsa: convert ksz bindings document to yaml

[Christian Eggers]

On Monday, 16 November 2020, 15:37:20 CET, Rob Herring wrote:
> On Thu, 12 Nov 2020 16:35:27 +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       | 150 ++++++++++++++++++
> >  MAINTAINERS                                   |   2 +-
> >  3 files changed, 151 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.yaml: 'oneOf' conditional failed, one must be fixed:
> 'unevaluatedProperties' is a required property
> 	'additionalProperties' is a required property
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/dsa
> /microchip,ksz.yaml: ignoring, error in schema: warning: no schema found in
> file: ./Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> 
> 
> See https://patchwork.ozlabs.org/patch/1399036
> 
> 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.

with the latest dtschema I get further warnings:

/home/.../build-net-next/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: /home/.../Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
/home/.../build-net-next/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: /home/.../Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml

Which schema requires the regex 'pinctrl-[0-9]+'? I have tried to add pinctrl-0
properties to the switch@0 and switch@1 nodes, but that didn't help.

Current version of microchip,ksz.yaml is below.

regards
Christian


# 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

  interrupts:
    description:
      Interrupt specifier for the INTRP_N line from the device.
    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>
    #include <dt-bindings/interrupt-controller/irq.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>;
            interrupts-extended = <&gpio5 1 IRQ_TYPE_LEVEL_LOW>;  /* INTRP_N line */

            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;
                    };
                };
            };
        };
    };
...

Re: [PATCH net-next v2 01/11] dt-bindings: net: dsa: convert ksz bindings document to yaml

[Rob Herring]

On Thu, Nov 12, 2020 at 04:35:27PM +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       | 150 ++++++++++++++++++
>  MAINTAINERS                                   |   2 +-
>  3 files changed, 151 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/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> new file mode 100644
> index 000000000000..431ca5c498a8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> @@ -0,0 +1,150 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%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"

Don't need quotes.

> +
> +  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
> +
> +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 1e7d1d71c125..3d173fcbf119 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11518,7 +11518,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
> 

[PATCH net-next v2 02/11] net: dsa: microchip: support for "ethernet-ports" node

[Christian Eggers]

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

Signed-off-by: Christian Eggers <ceggers@arri.de>
---
 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

Re: [PATCH net-next v2 02/11] net: dsa: microchip: support for "ethernet-ports" node

[Vladimir Oltean]

On Thu, Nov 12, 2020 at 04:35:28PM +0100, Christian Eggers wrote:
> The dsa.yaml device tree binding allows "ethernet-ports" (preferred) and
> "ports".
>
> Signed-off-by: Christian Eggers <ceggers@arri.de>
> ---
>  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");

Man, I didn't think there could be something as uninspired as naming the
private structure of your driver "dev"...

>  		if (ports)
>  			for_each_available_child_of_node(ports, port) {
>  				if (of_property_read_u32(port, "reg",
> --

Either way:

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

[PATCH net-next v2 03/11] net: dsa: microchip: split ksz_common.h

[Christian Eggers]

Parts of ksz_common.h (struct ksz_device) will be required in
net/dsa/tag_ksz.c soon. So move the relevant parts into a new header
file.

Signed-off-by: Christian Eggers <ceggers@arri.de>
---
 MAINTAINERS                            |  1 +
 drivers/net/dsa/microchip/ksz_common.h | 81 +---------------------
 include/linux/dsa/ksz_common.h         | 96 ++++++++++++++++++++++++++
 3 files changed, 98 insertions(+), 80 deletions(-)
 create mode 100644 include/linux/dsa/ksz_common.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 3d173fcbf119..de7e2d80426a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11520,6 +11520,7 @@ L:	netdev@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
 F:	drivers/net/dsa/microchip/*
+F:	include/linux/dsa/microchip/ksz_common.h
 F:	include/linux/platform_data/microchip-ksz.h
 F:	net/dsa/tag_ksz.c
 
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index cf866e48ff66..5735374b5bc3 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -7,92 +7,13 @@
 #ifndef __KSZ_COMMON_H
 #define __KSZ_COMMON_H
 
+#include <linux/dsa/ksz_common.h>
 #include <linux/etherdevice.h>
-#include <linux/kernel.h>
-#include <linux/mutex.h>
-#include <linux/phy.h>
-#include <linux/regmap.h>
-#include <net/dsa.h>
 
 struct vlan_table {
 	u32 table[3];
 };
 
-struct ksz_port_mib {
-	struct mutex cnt_mutex;		/* structure access */
-	u8 cnt_ptr;
-	u64 *counters;
-};
-
-struct ksz_port {
-	u16 member;
-	u16 vid_member;
-	int stp_state;
-	struct phy_device phydev;
-
-	u32 on:1;			/* port is not disabled by hardware */
-	u32 phy:1;			/* port has a PHY */
-	u32 fiber:1;			/* port is fiber */
-	u32 sgmii:1;			/* port is SGMII */
-	u32 force:1;
-	u32 read:1;			/* read MIB counters in background */
-	u32 freeze:1;			/* MIB counter freeze is enabled */
-
-	struct ksz_port_mib mib;
-	phy_interface_t interface;
-};
-
-struct ksz_device {
-	struct dsa_switch *ds;
-	struct ksz_platform_data *pdata;
-	const char *name;
-
-	struct mutex dev_mutex;		/* device access */
-	struct mutex regmap_mutex;	/* regmap access */
-	struct mutex alu_mutex;		/* ALU access */
-	struct mutex vlan_mutex;	/* vlan access */
-	const struct ksz_dev_ops *dev_ops;
-
-	struct device *dev;
-	struct regmap *regmap[3];
-
-	void *priv;
-
-	struct gpio_desc *reset_gpio;	/* Optional reset GPIO */
-
-	/* chip specific data */
-	u32 chip_id;
-	int num_vlans;
-	int num_alus;
-	int num_statics;
-	int cpu_port;			/* port connected to CPU */
-	int cpu_ports;			/* port bitmap can be cpu port */
-	int phy_port_cnt;
-	int port_cnt;
-	int reg_mib_cnt;
-	int mib_cnt;
-	int mib_port_cnt;
-	int last_port;			/* ports after that not used */
-	phy_interface_t compat_interface;
-	u32 regs_size;
-	bool phy_errata_9477;
-	bool synclko_125;
-
-	struct vlan_table *vlan_cache;
-
-	struct ksz_port *ports;
-	struct delayed_work mib_read;
-	unsigned long mib_read_interval;
-	u16 br_member;
-	u16 member;
-	u16 mirror_rx;
-	u16 mirror_tx;
-	u32 features;			/* chip specific features */
-	u32 overrides;			/* chip functions set by user */
-	u16 host_mask;
-	u16 port_mask;
-};
-
 struct alu_struct {
 	/* entry 1 */
 	u8	is_static:1;
diff --git a/include/linux/dsa/ksz_common.h b/include/linux/dsa/ksz_common.h
new file mode 100644
index 000000000000..3b22380d85c5
--- /dev/null
+++ b/include/linux/dsa/ksz_common.h
@@ -0,0 +1,96 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/* Included by drivers/net/dsa/microchip/ksz_common.h and net/dsa/tag_ksz.c */
+
+#ifndef _NET_DSA_KSZ_COMMON_H_
+#define _NET_DSA_KSZ_COMMON_H_
+
+#include <linux/gpio/consumer.h>
+#include <linux/kernel.h>
+#include <linux/mutex.h>
+#include <linux/phy.h>
+#include <linux/regmap.h>
+#include <linux/timer.h>
+#include <linux/workqueue.h>
+#include <net/dsa.h>
+
+struct ksz_platform_data;
+struct ksz_dev_ops;
+struct vlan_table;
+
+struct ksz_port_mib {
+	struct mutex cnt_mutex;		/* structure access */
+	u8 cnt_ptr;
+	u64 *counters;
+};
+
+struct ksz_port {
+	u16 member;
+	u16 vid_member;
+	int stp_state;
+	struct phy_device phydev;
+
+	u32 on:1;			/* port is not disabled by hardware */
+	u32 phy:1;			/* port has a PHY */
+	u32 fiber:1;			/* port is fiber */
+	u32 sgmii:1;			/* port is SGMII */
+	u32 force:1;
+	u32 read:1;			/* read MIB counters in background */
+	u32 freeze:1;			/* MIB counter freeze is enabled */
+
+	struct ksz_port_mib mib;
+	phy_interface_t interface;
+};
+
+struct ksz_device {
+	struct dsa_switch *ds;
+	struct ksz_platform_data *pdata;
+	const char *name;
+
+	struct mutex dev_mutex;		/* device access */
+	struct mutex regmap_mutex;	/* regmap access */
+	struct mutex alu_mutex;		/* ALU access */
+	struct mutex vlan_mutex;	/* vlan access */
+	const struct ksz_dev_ops *dev_ops;
+
+	struct device *dev;
+	struct regmap *regmap[3];
+
+	void *priv;
+
+	struct gpio_desc *reset_gpio;	/* Optional reset GPIO */
+
+	/* chip specific data */
+	u32 chip_id;
+	int num_vlans;
+	int num_alus;
+	int num_statics;
+	int cpu_port;			/* port connected to CPU */
+	int cpu_ports;			/* port bitmap can be cpu port */
+	int phy_port_cnt;
+	int port_cnt;
+	int reg_mib_cnt;
+	int mib_cnt;
+	int mib_port_cnt;
+	int last_port;			/* ports after that not used */
+	phy_interface_t compat_interface;
+	u32 regs_size;
+	bool phy_errata_9477;
+	bool synclko_125;
+
+	struct vlan_table *vlan_cache;
+
+	struct ksz_port *ports;
+	struct delayed_work mib_read;
+	unsigned long mib_read_interval;
+	u16 br_member;
+	u16 member;
+	u16 mirror_rx;
+	u16 mirror_tx;
+	u32 features;			/* chip specific features */
+	u32 overrides;			/* chip functions set by user */
+	u16 host_mask;
+	u16 port_mask;
+};
+
+#endif /* _NET_DSA_KSZ_COMMON_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

Re: [PATCH net-next v2 03/11] net: dsa: microchip: split ksz_common.h

[Vladimir Oltean]

On Thu, Nov 12, 2020 at 04:35:29PM +0100, Christian Eggers wrote:
> Parts of ksz_common.h (struct ksz_device) will be required in
> net/dsa/tag_ksz.c soon. So move the relevant parts into a new header
> file.
> 
> Signed-off-by: Christian Eggers <ceggers@arri.de>
> ---

I had to skip ahead to see what you're going to use struct ksz_port and
struct ksz_device for. It looks like you need:

	struct ksz_port::tstamp_rx_latency_ns
	struct ksz_device::ptp_clock_lock
	struct ksz_device::ptp_clock_time

Not more.

Why don't you go the other way around, i.e. exporting some functions
from your driver, and calling them from the tagger? You could even move
the entire ksz9477_tstamp_to_clock() into the driver as-is, as far as I
can see.

Re: [PATCH net-next v2 03/11] net: dsa: microchip: split ksz_common.h

[Christian Eggers]

On Friday, 13 November 2020, 00:02:54 CET, Vladimir Oltean wrote:
> On Thu, Nov 12, 2020 at 04:35:29PM +0100, Christian Eggers wrote:
> > Parts of ksz_common.h (struct ksz_device) will be required in
> > net/dsa/tag_ksz.c soon. So move the relevant parts into a new header
> > file.
> > 
> > Signed-off-by: Christian Eggers <ceggers@arri.de>
> > ---
> 
> I had to skip ahead to see what you're going to use struct ksz_port and
> struct ksz_device for. It looks like you need:
> 
> 	struct ksz_port::tstamp_rx_latency_ns
> 	struct ksz_device::ptp_clock_lock
> 	struct ksz_device::ptp_clock_time
> 
> Not more.
> 
> Why don't you go the other way around, i.e. exporting some functions
> from your driver, and calling them from the tagger? 

Good question... But as for as I can see, there are a single tagger and 
multiple device drivers (currently KSZ8795 and KSZ9477). 

Moving the KSZ9477 specific stuff, which is required by the tagger, into the 
KSZ9477 device driver, would make the tagger dependent on the driver(s). 
Currently, no tagger seems to have this direction of dependency (at least I 
cannot find this in net/dsa/Kconfig).

If I shall change this anyway, I would use #ifdefs within the tag_ksz driver 
in order to avoid unnecessary dependencies to the KSZ9477 driver for the case 
only KSZ8795 is selected.

> You could even move
> the entire ksz9477_tstamp_to_clock() into the driver as-is, as far as I
> can see.

Re: [PATCH net-next v2 03/11] net: dsa: microchip: split ksz_common.h

[Christian Eggers]

On Friday, 13 November 2020, 17:56:34 CET, Christian Eggers wrote:
> On Friday, 13 November 2020, 00:02:54 CET, Vladimir Oltean wrote:
> > On Thu, Nov 12, 2020 at 04:35:29PM +0100, Christian Eggers wrote:
> > > Parts of ksz_common.h (struct ksz_device) will be required in
> > > net/dsa/tag_ksz.c soon. So move the relevant parts into a new header
> > > file.
> > > 
> > > Signed-off-by: Christian Eggers <ceggers@arri.de>
> > > ---
> > 
> > I had to skip ahead to see what you're going to use struct ksz_port and
> > 
> > struct ksz_device for. It looks like you need:
> > 	struct ksz_port::tstamp_rx_latency_ns
> > 	struct ksz_device::ptp_clock_lock
> > 	struct ksz_device::ptp_clock_time
> > 
> > Not more.
I have tried to put these members into separate structs:

include/linux/dsa/ksz_common.h:
struct ksz_port_ptp_shared {
	u16 tstamp_rx_latency_ns;   /* rx delay from wire to tstamp unit */
};

struct ksz_device_ptp_shared {
	spinlock_t ptp_clock_lock; /* for ptp_clock_time */
	/* approximated current time, read once per second from hardware */
	struct timespec64 ptp_clock_time;
};

drivers/net/dsa/microchip/ksz_common.h:
...
#include <linux/dsa/ksz_common.h>
...
struct ksz_port {
...
#if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ9477_PTP)
	struct ksz_port_ptp_shared ptp_shared;	/* shared with tag_ksz.c */
	u16 tstamp_tx_latency_ns;	/* tx delay from tstamp unit to wire */
	struct hwtstamp_config tstamp_config;
	struct sk_buff *tstamp_tx_xdelay_skb;
	unsigned long tstamp_state;
#endif
};
...
struct ksz_device {
...
#if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ9477_PTP)
	struct ptp_clock *ptp_clock;
	struct ptp_clock_info ptp_caps;
	struct mutex ptp_mutex;
	struct ksz_device_ptp_shared ptp_shared;   /* shared with tag_ksz.c */
#endif
};

The problem with such technique is, that I still need to dereference
struct ksz_device in tag_ksz.c:

static void ksz9477_rcv_timestamp(struct sk_buff *skb, u8 *tag,
				  struct net_device *dev, unsigned int port)
{
...
	struct dsa_switch *ds = dev->dsa_ptr->ds;
	struct ksz_device *ksz = ds->priv;
	struct ksz_port *prt = &ksz->ports[port];
...
}

As struct dsa_switch::priv is already occupied by the pointer to
struct ksz_device, I see no way accessing the ptp specific device/port
information in tag_ksz.c.

> > 
> > Why don't you go the other way around, i.e. exporting some functions
> > from your driver, and calling them from the tagger?
> 
> Good question... But as for as I can see, there are a single tagger and
> multiple device drivers (currently KSZ8795 and KSZ9477).
> 
> Moving the KSZ9477 specific stuff, which is required by the tagger, into the
> KSZ9477 device driver, would make the tagger dependent on the driver(s).
> Currently, no tagger seems to have this direction of dependency (at least I
> cannot find this in net/dsa/Kconfig).
> 
> If I shall change this anyway, I would use #ifdefs within the tag_ksz driver
> in order to avoid unnecessary dependencies to the KSZ9477 driver for the
> case only KSZ8795 is selected.
> 
> > You could even move
> > the entire ksz9477_tstamp_to_clock() into the driver as-is, as far as I
> > can see.

regards
Christian

Re: [PATCH net-next v2 03/11] net: dsa: microchip: split ksz_common.h

[Vladimir Oltean]

On Mon, Nov 16, 2020 at 10:21:14AM +0100, Christian Eggers wrote:
> On Friday, 13 November 2020, 17:56:34 CET, Christian Eggers wrote:
> > On Friday, 13 November 2020, 00:02:54 CET, Vladimir Oltean wrote:
> > > On Thu, Nov 12, 2020 at 04:35:29PM +0100, Christian Eggers wrote:
> > > > Parts of ksz_common.h (struct ksz_device) will be required in
> > > > net/dsa/tag_ksz.c soon. So move the relevant parts into a new header
> > > > file.
> > > >
> > > > Signed-off-by: Christian Eggers <ceggers@arri.de>
> > > > ---
> > >
> > > I had to skip ahead to see what you're going to use struct ksz_port and
> > >
> > > struct ksz_device for. It looks like you need:
> > >     struct ksz_port::tstamp_rx_latency_ns
> > >     struct ksz_device::ptp_clock_lock
> > >     struct ksz_device::ptp_clock_time
> > >
> > > Not more.
> I have tried to put these members into separate structs:
>
> include/linux/dsa/ksz_common.h:
> struct ksz_port_ptp_shared {
>         u16 tstamp_rx_latency_ns;   /* rx delay from wire to tstamp unit */
> };
>
> struct ksz_device_ptp_shared {
>         spinlock_t ptp_clock_lock; /* for ptp_clock_time */
>         /* approximated current time, read once per second from hardware */
>         struct timespec64 ptp_clock_time;
> };
>
> drivers/net/dsa/microchip/ksz_common.h:
> ...
> #include <linux/dsa/ksz_common.h>
> ...
> struct ksz_port {
> ...
> #if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ9477_PTP)
>         struct ksz_port_ptp_shared ptp_shared;  /* shared with tag_ksz.c */
>         u16 tstamp_tx_latency_ns;       /* tx delay from tstamp unit to wire */
>         struct hwtstamp_config tstamp_config;
>         struct sk_buff *tstamp_tx_xdelay_skb;
>         unsigned long tstamp_state;
> #endif
> };
> ...
> struct ksz_device {
> ...
> #if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ9477_PTP)
>         struct ptp_clock *ptp_clock;
>         struct ptp_clock_info ptp_caps;
>         struct mutex ptp_mutex;
>         struct ksz_device_ptp_shared ptp_shared;   /* shared with tag_ksz.c */
> #endif
> };
>
> The problem with such technique is, that I still need to dereference
> struct ksz_device in tag_ksz.c:
>
> static void ksz9477_rcv_timestamp(struct sk_buff *skb, u8 *tag,
>                                   struct net_device *dev, unsigned int port)
> {
> ...
>         struct dsa_switch *ds = dev->dsa_ptr->ds;
>         struct ksz_device *ksz = ds->priv;
>         struct ksz_port *prt = &ksz->ports[port];
> ...
> }
>
> As struct dsa_switch::priv is already occupied by the pointer to
> struct ksz_device, I see no way accessing the ptp specific device/port
> information in tag_ksz.c.

There is a dp->priv that you could use to hold a reference to your
PTP-specific substructure (struct ksz_port_ptp_shared) of
struct ksz_port.

Then, in that PTP-specific per-port substructure, you could hold another
pointer to a common struct ksz_device_ptp_shared.

Re: [PATCH net-next v2 03/11] net: dsa: microchip: split ksz_common.h

[Vladimir Oltean]

On Thu, Nov 12, 2020 at 04:35:29PM +0100, Christian Eggers wrote:
> Parts of ksz_common.h (struct ksz_device) will be required in
> net/dsa/tag_ksz.c soon. So move the relevant parts into a new header
> file.
> 
> Signed-off-by: Christian Eggers <ceggers@arri.de>
> ---
>  MAINTAINERS                            |  1 +
>  drivers/net/dsa/microchip/ksz_common.h | 81 +---------------------
>  include/linux/dsa/ksz_common.h         | 96 ++++++++++++++++++++++++++
>  3 files changed, 98 insertions(+), 80 deletions(-)
>  create mode 100644 include/linux/dsa/ksz_common.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3d173fcbf119..de7e2d80426a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11520,6 +11520,7 @@ L:	netdev@vger.kernel.org
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
>  F:	drivers/net/dsa/microchip/*
> +F:	include/linux/dsa/microchip/ksz_common.h

Ah, I almost forgot. This path is wrong, it has an extra "microchip".

[PATCH net-next v2 04/11] net: dsa: microchip: rename ksz9477.c to ksz9477_main.o

[Christian Eggers]

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

Signed-off-by: Christian Eggers <ceggers@arri.de>
---
 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

Re: [PATCH net-next v2 04/11] net: dsa: microchip: rename ksz9477.c to ksz9477_main.o

[Vladimir Oltean]

On Thu, Nov 12, 2020 at 04:35:30PM +0100, Christian Eggers wrote:
> 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>

Re: [PATCH net-next v2 04/11] net: dsa: microchip: rename ksz9477.c to ksz9477_main.o

[Vladimir Oltean]

On Thu, Nov 12, 2020 at 04:35:30PM +0100, Christian Eggers wrote:
> PTP functionality will be built into a separate source file
> (ksz9477_ptp.c).
> 
> Signed-off-by: Christian Eggers <ceggers@arri.de>
> ---

You might consider editing the title. "ksz9477_main.c".

[PATCH net-next v2 05/11] dt-bindings: net: dsa: microchip,ksz: add interrupt property

[Christian Eggers]

The devices have an optional interrupt line.

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

diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
index 431ca5c498a8..b2613d6c97cf 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:
@@ -47,6 +52,7 @@ required:
 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 {
@@ -68,6 +74,8 @@ examples:
             compatible = "microchip,ksz9477";
             reg = <0>;
             reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
+            interrupt-parent = <&gpio5>;
+            interrupts = <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

Re: [PATCH net-next v2 05/11] dt-bindings: net: dsa: microchip,ksz: add interrupt property

[Vladimir Oltean]

On Thu, Nov 12, 2020 at 04:35:31PM +0100, Christian Eggers wrote:
> The devices have an optional interrupt line.
>
> Signed-off-by: Christian Eggers <ceggers@arri.de>
> ---
>  .../devicetree/bindings/net/dsa/microchip,ksz.yaml        | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> index 431ca5c498a8..b2613d6c97cf 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:
> @@ -47,6 +52,7 @@ required:
>  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 {
> @@ -68,6 +74,8 @@ examples:
>              compatible = "microchip,ksz9477";
>              reg = <0>;
>              reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
> +            interrupt-parent = <&gpio5>;
> +            interrupts = <1 IRQ_TYPE_LEVEL_LOW>;  /* INTRP_N line */

Isn't it preferable to use this syntax?

		interrupts-extended = <&gpio5 1 IRQ_TYPE_LEVEL_LOW>;  /* INTRP_N line */

Re: [PATCH net-next v2 05/11] dt-bindings: net: dsa: microchip,ksz: add interrupt property

[Christian Eggers]

On Friday, 13 November 2020, 00:07:32 CET, Vladimir Oltean wrote:
> On Thu, Nov 12, 2020 at 04:35:31PM +0100, Christian Eggers wrote:
> > The devices have an optional interrupt line.
> > 
> > Signed-off-by: Christian Eggers <ceggers@arri.de>
> > ---
> > 
> >  .../devicetree/bindings/net/dsa/microchip,ksz.yaml        | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml index
> > 431ca5c498a8..b2613d6c97cf 100644
> > --- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> > 
...
> > +            interrupt-parent = <&gpio5>;
> > +            interrupts = <1 IRQ_TYPE_LEVEL_LOW>;  /* INTRP_N line */
> 
> Isn't it preferable to use this syntax?
> 
> 		interrupts-extended = <&gpio5 1 IRQ_TYPE_LEVEL_LOW>;  /* INTRP_N line */

After reading Documentation/devicetree/bindings/interrupt-controller/interrupts.txt,
I would say that "interrupts-extended" is more flexible as it allows different
interrupt parents for the case there is more than one interrupt line. Although
there is only one line on the KSZ, I will change this.

[PATCH net-next v2 06/11] net: dsa: microchip: ksz9477: basic interrupt support

[Christian Eggers]

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 actually enabled nor handled.

ksz9477_reset_switch() must be called before requesting the IRQ (in
ksz9477_init() instead of ksz9477_setup()).

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

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 abfd3802bb51..6b5a981fb21f 100644
--- a/drivers/net/dsa/microchip/ksz9477_main.c
+++ b/drivers/net/dsa/microchip/ksz9477_main.c
@@ -7,7 +7,9 @@
 
 #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>
@@ -1345,19 +1347,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);
@@ -1535,12 +1530,84 @@ 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;
+	irqreturn_t result = IRQ_NONE;
+
+	/* Read global port interrupt status register */
+	ret = ksz_read32(dev, REG_SW_PORT_INT_STATUS__4, &data);
+	if (ret)
+		return result;
+
+	for (port = 0; port < dev->port_cnt; port++) {
+		if (data & BIT(port)) {
+			u8 data8;
+
+			/* Read port interrupt status register */
+			ret = ksz_read8(dev, PORT_CTRL_ADDR(port, REG_PORT_INT_STATUS),
+					&data8);
+			if (ret)
+				return result;
+
+			/* ToDo: Add specific handling of port interrupts */
+		}
+	}
+
+	return result;
+}
+
+static int ksz9477_enable_port_interrupts(struct ksz_device *dev)
+{
+	u32 data;
+	int ret;
+
+	ret = ksz_read32(dev, REG_SW_PORT_INT_MASK__4, &data);
+	if (ret)
+		return ret;
+
+	/* Enable port interrupts (0 means enabled) */
+	data &= ~((1 << dev->port_cnt) - 1);
+	ret = ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ksz9477_disable_port_interrupts(struct ksz_device *dev)
+{
+	u32 data;
+	int ret;
+
+	ret = ksz_read32(dev, REG_SW_PORT_INT_MASK__4, &data);
+	if (ret)
+		return ret;
+
+	/* Disable port interrupts (1 means disabled) */
+	data |= ((1 << dev->port_cnt) - 1);
+	ret = ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 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];
 
@@ -1584,12 +1651,32 @@ 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) {
+		unsigned long irqflags = irqd_get_trigger_type(irq_get_irq_data(dev->irq));
+
+		irqflags |= IRQF_ONESHOT;
+		ret = devm_request_threaded_irq(dev->dev, dev->irq, NULL,
+						ksz9477_switch_irq_thread,
+						irqflags,
+						dev_name(dev->dev),
+						dev);
+		if (ret) {
+			dev_err(dev->dev, "failed to request IRQ.\n");
+			return ret;
+		}
+
+		ret = ksz9477_enable_port_interrupts(dev);
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }
 
 static void ksz9477_switch_exit(struct ksz_device *dev)
 {
+	if (dev->irq > 0)
+		ksz9477_disable_port_interrupts(dev);
 	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/include/linux/dsa/ksz_common.h b/include/linux/dsa/ksz_common.h
index 3b22380d85c5..bf57ba4b2132 100644
--- a/include/linux/dsa/ksz_common.h
+++ b/include/linux/dsa/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

Re: [PATCH net-next v2 06/11] net: dsa: microchip: ksz9477: basic interrupt support

[Vladimir Oltean]

On Thu, Nov 12, 2020 at 04:35:32PM +0100, Christian Eggers wrote:
> Interrupts are required for TX time stamping. Probably they could also
> be used for PHY connection status.

Do the KSZ switches have an internal PHY? And there's a single interrupt
line, shared between the PTP timestamping engine, and the internal PHY
that is driver by phylib?

> This patch only adds the basic infrastructure for interrupts, no
> interrupts are actually enabled nor handled.
>
> ksz9477_reset_switch() must be called before requesting the IRQ (in
> ksz9477_init() instead of ksz9477_setup()).

A patch can never be "too simple". Maybe you could factor out that code
movement into a separate patch.

> Signed-off-by: Christian Eggers <ceggers@arri.de>
> ---
>  drivers/net/dsa/microchip/ksz9477_i2c.c  |   2 +
>  drivers/net/dsa/microchip/ksz9477_main.c | 103 +++++++++++++++++++++--
>  drivers/net/dsa/microchip/ksz9477_spi.c  |   2 +
>  include/linux/dsa/ksz_common.h           |   1 +
>  4 files changed, 100 insertions(+), 8 deletions(-)
>
> 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 abfd3802bb51..6b5a981fb21f 100644
> --- a/drivers/net/dsa/microchip/ksz9477_main.c
> +++ b/drivers/net/dsa/microchip/ksz9477_main.c
> @@ -7,7 +7,9 @@
>
>  #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>
> @@ -1345,19 +1347,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);
> @@ -1535,12 +1530,84 @@ 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;
> +	irqreturn_t result = IRQ_NONE;

Please keep local variable declaration sorted in the reverse order of
line length. But....

> +
> +	/* Read global port interrupt status register */
> +	ret = ksz_read32(dev, REG_SW_PORT_INT_STATUS__4, &data);
> +	if (ret)
> +		return result;

...Is there any point at all in keeping the "result" variable?

> +
> +	for (port = 0; port < dev->port_cnt; port++) {
> +		if (data & BIT(port)) {

You can reduce the indentation level by 1 here using:

		if (!(data & BIT(port)))
			continue;

> +			u8 data8;
> +
> +			/* Read port interrupt status register */
> +			ret = ksz_read8(dev, PORT_CTRL_ADDR(port, REG_PORT_INT_STATUS),
> +					&data8);
> +			if (ret)
> +				return result;
> +
> +			/* ToDo: Add specific handling of port interrupts */

Buggy? Please return IRQ_HANDLED, otherwise the system, when bisected to
this commit exactly, will emit interrupts and complain that nobody cared.

> +		}
> +	}
> +
> +	return result;
> +}
> +
> +static int ksz9477_enable_port_interrupts(struct ksz_device *dev)
> +{
> +	u32 data;
> +	int ret;
> +
> +	ret = ksz_read32(dev, REG_SW_PORT_INT_MASK__4, &data);
> +	if (ret)
> +		return ret;
> +
> +	/* Enable port interrupts (0 means enabled) */
> +	data &= ~((1 << dev->port_cnt) - 1);

And what's the " - 1" for?

> +	ret = ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

	return ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data);

> +}
> +
> +static int ksz9477_disable_port_interrupts(struct ksz_device *dev)
> +{
> +	u32 data;
> +	int ret;
> +
> +	ret = ksz_read32(dev, REG_SW_PORT_INT_MASK__4, &data);
> +	if (ret)
> +		return ret;
> +
> +	/* Disable port interrupts (1 means disabled) */
> +	data |= ((1 << dev->port_cnt) - 1);
> +	ret = ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

same comments as above.

Also, it's almost as if you want to implement these in the same
function, with a "bool enable"?

> +}
> +
>  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];
>
> @@ -1584,12 +1651,32 @@ 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) {
> +		unsigned long irqflags = irqd_get_trigger_type(irq_get_irq_data(dev->irq));

What is irqd_get_trigger_type and what does it have to do with the
"irqflags" argument of request_threaded_irq? Where else have you even
seen this?

> +
> +		irqflags |= IRQF_ONESHOT;

And shared maybe?

> +		ret = devm_request_threaded_irq(dev->dev, dev->irq, NULL,
> +						ksz9477_switch_irq_thread,
> +						irqflags,
> +						dev_name(dev->dev),
> +						dev);
> +		if (ret) {
> +			dev_err(dev->dev, "failed to request IRQ.\n");
> +			return ret;
> +		}
> +
> +		ret = ksz9477_enable_port_interrupts(dev);
> +		if (ret)
> +			return ret;

Could you also clear pending interrupts before enabling the line?

> +	}
>
>  	return 0;
>  }
>
>  static void ksz9477_switch_exit(struct ksz_device *dev)
>  {
> +	if (dev->irq > 0)
> +		ksz9477_disable_port_interrupts(dev);

I think it'd look a bit nicer if you moved this condition into
ksz9477_disable_port_interrupts:

	if (!dev->irq)
		return;

>  	ksz9477_reset_switch(dev);
>  }
>

Re: [PATCH net-next v2 06/11] net: dsa: microchip: ksz9477: basic interrupt support

[Christian Eggers]

On Friday, 13 November 2020, 00:26:17 CET, Vladimir Oltean wrote:
> On Thu, Nov 12, 2020 at 04:35:32PM +0100, Christian Eggers wrote:
> > Interrupts are required for TX time stamping. Probably they could also
> > be used for PHY connection status.
> 
> Do the KSZ switches have an internal PHY? And there's a single interrupt
> line, shared between the PTP timestamping engine, and the internal PHY
> that is driver by phylib?
The device has only one interrupt line (INTRP_N), although there may be
applications which use additionally the GPIO (PPS/PEROUT) output as an
interrupt.

I assume that the PHY driver currently uses polling (as the KSZ9477 driver
used to have no interrupt functionality. Maybe this can be changed in future,
as the KSZ hardware has hierarchical interrupt enable/status registers.

> > This patch only adds the basic infrastructure for interrupts, no
> > interrupts are actually enabled nor handled.
> > 
> > ksz9477_reset_switch() must be called before requesting the IRQ (in
> > ksz9477_init() instead of ksz9477_setup()).
> 
> A patch can never be "too simple". Maybe you could factor out that code
> movement into a separate patch.
I haven't checked yet, but I'll try.

[...]

> > +static irqreturn_t ksz9477_switch_irq_thread(int irq, void *dev_id)
> > +{
> > +	struct ksz_device *dev = dev_id;
> > +	u32 data;
> > +	int port;
> > +	int ret;
> > +	irqreturn_t result = IRQ_NONE;
> 
> Please keep local variable declaration sorted in the reverse order of
> line length. But....
> 
> > +
> > +	/* Read global port interrupt status register */
> > +	ret = ksz_read32(dev, REG_SW_PORT_INT_STATUS__4, &data);
> > +	if (ret)
> > +		return result;
> 
> ...Is there any point at all in keeping the "result" variable?
> 
> > +
> > +	for (port = 0; port < dev->port_cnt; port++) {
> > +		if (data & BIT(port)) {
> 
> You can reduce the indentation level by 1 here using:
> 
> 		if (!(data & BIT(port)))
> 			continue;
> 
> > +			u8 data8;
> > +
> > +			/* Read port interrupt status register */
> > +			ret = ksz_read8(dev, PORT_CTRL_ADDR(port, REG_PORT_INT_STATUS),
> > +					&data8);
> > +			if (ret)
> > +				return result;
> > +
> > +			/* ToDo: Add specific handling of port interrupts */
> 
> Buggy? Please return IRQ_HANDLED, otherwise the system, when bisected to
> this commit exactly, will emit interrupts and complain that nobody cared.
Probably this can be kept as it is. The hardware will only emit interrupts
if these have been explicitly enabled. Although the *port* interrupts are
enabled here (and all bits in the "Port Interrupt Mask Register" (section
5.2.1.12) are active after reset), actually no interrupts should be raised as
the ports sub units (PTP, PHY and ACL) don't emit interrupt after reset:
- PHY (section 5.2.2.19): All interrupts are disabled after reset
- PTP (section 5.2.11.11): dito
- ACL (not found): I got never interrupts from here

> 
> > +		}
> > +	}
> > +
> > +	return result;
> > +}
> > +
> > +static int ksz9477_enable_port_interrupts(struct ksz_device *dev)
> > +{
> > +	u32 data;
> > +	int ret;
> > +
> > +	ret = ksz_read32(dev, REG_SW_PORT_INT_MASK__4, &data);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Enable port interrupts (0 means enabled) */
> > +	data &= ~((1 << dev->port_cnt) - 1);
> 
> And what's the " - 1" for?
I build a bitmask where the bits 0..(dev->port_cnt-1) are set... I'll whether
GENMASK() can be used with variable data as argument.
> 
> > +	ret = ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> 
> 	return ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data);
> 
> > +}
> > +
> > +static int ksz9477_disable_port_interrupts(struct ksz_device *dev)
> > +{
> > +	u32 data;
> > +	int ret;
> > +
> > +	ret = ksz_read32(dev, REG_SW_PORT_INT_MASK__4, &data);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Disable port interrupts (1 means disabled) */
> > +	data |= ((1 << dev->port_cnt) - 1);
> > +	ret = ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> 
> same comments as above.
> 
> Also, it's almost as if you want to implement these in the same
> function, with a "bool enable"?
You are right.

> 
> > +}
> > +
> > 
> >  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];
> > 
> > @@ -1584,12 +1651,32 @@ 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) {
> > +		unsigned long irqflags =
> > irqd_get_trigger_type(irq_get_irq_data(dev->irq));
> What is irqd_get_trigger_type and what does it have to do with the
> "irqflags" argument of request_threaded_irq? Where else have you even
> seen this?
No idea where I originally found this. It's some time ago when I wrote this.

> 
> > +
> > +		irqflags |= IRQF_ONESHOT;
> 
> And shared maybe?
I don't need it. Is there a rule when to add shared? At least the KSZ should 
be able to tell whether it has raised an IRQ or not.

> 
> > +		ret = devm_request_threaded_irq(dev->dev, dev->irq, NULL,
> > +						ksz9477_switch_irq_thread,
> > +						irqflags,
> > +						dev_name(dev->dev),
> > +						dev);
> > +		if (ret) {
> > +			dev_err(dev->dev, "failed to request IRQ.\n");
> > +			return ret;
> > +		}
> > +
> > +		ret = ksz9477_enable_port_interrupts(dev);
> > +		if (ret)
> > +			return ret;
> 
> Could you also clear pending interrupts before enabling the line?
As the device has just been reset and no concrete interrupts have been enabled,
there should be no need for this.

> 
> > +	}
> > 
> >  	return 0;
> >  
> >  }
> >  
> >  static void ksz9477_switch_exit(struct ksz_device *dev)
> >  {
> > 
> > +	if (dev->irq > 0)
> > +		ksz9477_disable_port_interrupts(dev);
> 
> I think it'd look a bit nicer if you moved this condition into
> ksz9477_disable_port_interrupts:
> 
> 	if (!dev->irq)
> 		return;
> 
> >  	ksz9477_reset_switch(dev);
> >  
> >  }

regards
Christian

[PATCH net-next v2 07/11] net: dsa: microchip: ksz9477: add Posix clock support for chip PTP clock

[Christian Eggers]

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        |   9 +
 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  | 301 +++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz9477_ptp.h  |  27 ++
 drivers/net/dsa/microchip/ksz9477_spi.c  |   2 +-
 drivers/net/dsa/microchip/ksz_common.h   |   1 +
 include/linux/dsa/ksz_common.h           |   7 +
 9 files changed, 365 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..71cc910e5941 100644
--- a/drivers/net/dsa/microchip/Kconfig
+++ b/drivers/net/dsa/microchip/Kconfig
@@ -24,6 +24,15 @@ 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"
+	default n
+	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 6b5a981fb21f..7d623400139f 100644
--- a/drivers/net/dsa/microchip/ksz9477_main.c
+++ b/drivers/net/dsa/microchip/ksz9477_main.c
@@ -18,6 +18,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)
@@ -1719,10 +1720,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..44d7bbdea518
--- /dev/null
+++ b/drivers/net/dsa/microchip/ksz9477_ptp.c
@@ -0,0 +1,301 @@
+// 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;
+
+	if (scaled_ppm) {
+		/* 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) */
+		s64 ppb, adj;
+		u32 data32;
+
+		/* 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)
+			return ret;
+	}
+
+	ret = ksz_read16(dev, REG_PTP_CLK_CTRL, &data16);
+	if (ret)
+		return ret;
+
+	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)
+		return ret;
+
+	return 0;
+}
+
+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 hw */
+	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 -ENOTTY;
+}
+
+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;
+	ret = ksz_write16(dev, REG_PTP_CLK_CTRL, data);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+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 5735374b5bc3..96808d2585d9 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -67,6 +67,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);
diff --git a/include/linux/dsa/ksz_common.h b/include/linux/dsa/ksz_common.h
index bf57ba4b2132..4d5b6cc9429a 100644
--- a/include/linux/dsa/ksz_common.h
+++ b/include/linux/dsa/ksz_common.h
@@ -9,6 +9,7 @@
 #include <linux/kernel.h>
 #include <linux/mutex.h>
 #include <linux/phy.h>
+#include <linux/ptp_clock_kernel.h>
 #include <linux/regmap.h>
 #include <linux/timer.h>
 #include <linux/workqueue.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;
+#endif
 };
 
 #endif /* _NET_DSA_KSZ_COMMON_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

Re: [PATCH net-next v2 07/11] net: dsa: microchip: ksz9477: add Posix clock support for chip PTP clock

[Vladimir Oltean]

On Thu, Nov 12, 2020 at 04:35:33PM +0100, Christian Eggers wrote:
> diff --git a/drivers/net/dsa/microchip/Kconfig b/drivers/net/dsa/microchip/Kconfig
> index 4ec6a47b7f72..71cc910e5941 100644
> --- a/drivers/net/dsa/microchip/Kconfig
> +++ b/drivers/net/dsa/microchip/Kconfig
> @@ -24,6 +24,15 @@ 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"
> +	default n

"default n" is implicit, please remove this

> +	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 6b5a981fb21f..7d623400139f 100644
> --- a/drivers/net/dsa/microchip/ksz9477_main.c
> +++ b/drivers/net/dsa/microchip/ksz9477_main.c
> @@ -18,6 +18,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)
> @@ -1719,10 +1720,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..44d7bbdea518
> --- /dev/null
> +++ b/drivers/net/dsa/microchip/ksz9477_ptp.c
> @@ -0,0 +1,301 @@
> +// 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;
> +
> +	if (scaled_ppm) {
> +		/* 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) */
> +		s64 ppb, adj;
> +		u32 data32;

Don't you want to move these declarations right beneath the "if" line?

> +
> +		/* 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)
> +			return ret;
> +	}
> +
> +	ret = ksz_read16(dev, REG_PTP_CLK_CTRL, &data16);
> +	if (ret)
> +		return ret;
> +
> +	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)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +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);

You're skipping the mutex in ksz9477_ptp_adjfine, is that intentional or
just a proof that it's useless? You should add a comment at its
declaration site about what it's meant to protect.

> +
> +	/* do not use ns_to_timespec64(), both sec and nsec are subtracted by hw */
> +	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 -ENOTTY;
> +}

How about -EOPNOTSUPP? I think -ENOTTY is reserved for "invalid ioctl on
the PTP clock", you wouldn't want to confuse a user into thinking that
they got the ioctls wrong (such as with the recent introduction of
PTP_PEROUT_REQUEST2 etc) when in fact the error comes from the driver.

[PATCH net-next v2 08/11] net: ptp: add helper for one-step P2P clocks

[Christian Eggers]

This function subtracts the ingress hardware time stamp from 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.

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

diff --git a/include/linux/ptp_classify.h b/include/linux/ptp_classify.h
index 56b2d7d66177..f27b512e1abd 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 */
@@ -118,6 +122,91 @@ 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_onestep_p2p_move_t2_to_correction - Update PTP header's correction field
+ * @skb: packet buffer
+ * @type: type of the packet (see ptp_classify_raw())
+ * @hdr: ptp header
+ * @t2: ingress hardware time stamp
+ *
+ * This function subtracts the ingress hardware time stamp from 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_onestep_p2p_move_t2_to_correction(struct sk_buff *skb,
+					   unsigned int type,
+					   struct ptp_header *hdr,
+					   ktime_t t2)
+{
+	u8 *ptr = skb_mac_header(skb);
+	struct udphdr *uhdr = NULL;
+	s64 ns = ktime_to_ns(t2);
+	__be64 correction_old;
+	s64 correction;
+
+	/* previous correction value is required for checksum update. */
+	memcpy(&correction_old,  &hdr->correction, sizeof(correction_old));
+	correction = (s64)be64_to_cpu(correction_old);
+
+	/* PTP correction field consists of 32 bit nanoseconds and 16 bit
+	 * fractional nanoseconds.  Avoid shifting negative numbers.
+	 */
+	if (ns >= 0)
+		correction -= ns << 16;
+	else
+		correction += -ns << 16;
+
+	/* write new correction value */
+	put_unaligned_be64((u64)correction, &hdr->correction);
+
+	/* locate udp header */
+	if (type & PTP_CLASS_VLAN)
+		ptr += VLAN_HLEN;
+
+	ptr += ETH_HLEN;
+
+	switch (type & PTP_CLASS_PMASK) {
+	case PTP_CLASS_IPV4:
+		ptr += ((struct iphdr *)ptr)->ihl << 2;
+		uhdr = (struct udphdr *)ptr;
+		break;
+	case PTP_CLASS_IPV6:
+		ptr += IP6_HLEN;
+		uhdr = (struct udphdr *)ptr;
+		break;
+	}
+
+	if (!uhdr)
+		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)
@@ -140,5 +229,13 @@ static inline u8 ptp_get_msgtype(const struct ptp_header *hdr,
 	 */
 	return 0;
 }
+
+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

Re: [PATCH net-next v2 08/11] net: ptp: add helper for one-step P2P clocks

[Vladimir Oltean]

On Thu, Nov 12, 2020 at 04:35:34PM +0100, Christian Eggers wrote:
> This function subtracts the ingress hardware time stamp from 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

Please close the parenthesis.

> that does not already modify the correction field of Pdelay_Req event
> messages on ingress.
> 
> Signed-off-by: Christian Eggers <ceggers@arri.de>

Please add more verbiage here, giving the reader as much context as
possible. You are establishing a de-facto approach for one-step peer
delay timestamping for hardware like yours, you need to explain why it
is done this way, for people to understand just by looking at git blame.

> ---
>  include/linux/ptp_classify.h | 97 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 97 insertions(+)
> 
> diff --git a/include/linux/ptp_classify.h b/include/linux/ptp_classify.h
> index 56b2d7d66177..f27b512e1abd 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 */
> @@ -118,6 +122,91 @@ 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_onestep_p2p_move_t2_to_correction - Update PTP header's correction field
> + * @skb: packet buffer
> + * @type: type of the packet (see ptp_classify_raw())
> + * @hdr: ptp header
> + * @t2: ingress hardware time stamp
> + *
> + * This function subtracts the ingress hardware time stamp from 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_onestep_p2p_move_t2_to_correction(struct sk_buff *skb,
> +					   unsigned int type,
> +					   struct ptp_header *hdr,
> +					   ktime_t t2)

The way this function is coded up right now, there's no reason to call it:
- onestep
- p2p
- move_t2_to_correction
As it is, it would be better named ptp_header_update_correction.

> +{
> +	u8 *ptr = skb_mac_header(skb);
> +	struct udphdr *uhdr = NULL;
> +	s64 ns = ktime_to_ns(t2);
> +	__be64 correction_old;
> +	s64 correction;
> +
> +	/* previous correction value is required for checksum update. */
> +	memcpy(&correction_old,  &hdr->correction, sizeof(correction_old));
> +	correction = (s64)be64_to_cpu(correction_old);
> +
> +	/* PTP correction field consists of 32 bit nanoseconds and 16 bit

48 bit nanoseconds

> +	 * fractional nanoseconds.  Avoid shifting negative numbers.
> +	 */
> +	if (ns >= 0)
> +		correction -= ns << 16;
> +	else
> +		correction += -ns << 16;

Again? Why? There's nothing wrong with left-shifting negative numbers,
two's complement works the same (note that right-shifting is a different
story, but that's not the case here).

> +
> +	/* write new correction value */
> +	put_unaligned_be64((u64)correction, &hdr->correction);
> +
> +	/* locate udp header */
> +	if (type & PTP_CLASS_VLAN)
> +		ptr += VLAN_HLEN;

Can't you just go back from the struct ptp_header pointer and avoid this
check?

> +	ptr += ETH_HLEN;
> +
> +	switch (type & PTP_CLASS_PMASK) {
> +	case PTP_CLASS_IPV4:
> +		ptr += ((struct iphdr *)ptr)->ihl << 2;
> +		uhdr = (struct udphdr *)ptr;
> +		break;
> +	case PTP_CLASS_IPV6:
> +		ptr += IP6_HLEN;
> +		uhdr = (struct udphdr *)ptr;
> +		break;
> +	}
> +
> +	if (!uhdr)
> +		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;
> +}
> +
> 

Re: [PATCH net-next v2 08/11] net: ptp: add helper for one-step P2P clocks

[Christian Eggers]

On Friday, 13 November 2020, 01:51:24 CET, Vladimir Oltean wrote:
> On Thu, Nov 12, 2020 at 04:35:34PM +0100, Christian Eggers wrote:
> > This function subtracts the ingress hardware time stamp from 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
> 
> Please close the parenthesis.
> 
> > that does not already modify the correction field of Pdelay_Req event
> > messages on ingress.
> > 
> > Signed-off-by: Christian Eggers <ceggers@arri.de>
> 
> Please add more verbiage here, giving the reader as much context as
> possible. You are establishing a de-facto approach for one-step peer
> delay timestamping for hardware like yours, you need to explain why it
> is done this way, for people to understand just by looking at git blame.
> 
> > ---
> > 
> >  include/linux/ptp_classify.h | 97 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 97 insertions(+)
> > 
> > diff --git a/include/linux/ptp_classify.h b/include/linux/ptp_classify.h
> > index 56b2d7d66177..f27b512e1abd 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 */
> > 
> > @@ -118,6 +122,91 @@ 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_onestep_p2p_move_t2_to_correction - Update PTP header's correction
> > field + * @skb: packet buffer
> > + * @type: type of the packet (see ptp_classify_raw())
> > + * @hdr: ptp header
> > + * @t2: ingress hardware time stamp
> > + *
> > + * This function subtracts the ingress hardware time stamp from 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_onestep_p2p_move_t2_to_correction(struct sk_buff *skb,
> > +					   unsigned int type,
> > +					   struct ptp_header *hdr,
> > +					   ktime_t t2)
> 
> The way this function is coded up right now, there's no reason to call it:
> - onestep
> - p2p
> - move_t2_to_correction
> As it is, it would be better named ptp_header_update_correction.

Do you want to provide the verbatim value of the correction field instead of t2?
I already considered this prototype as long as I wanted to move the "negative
correction back to the tail tag".

Providing the verbatim correction value would make this function more flexible
for other user but would move reading the old correction value to the caller.

> 
> > +{
> > +	u8 *ptr = skb_mac_header(skb);
> > +	struct udphdr *uhdr = NULL;
> > +	s64 ns = ktime_to_ns(t2);
> > +	__be64 correction_old;
> > +	s64 correction;
> > +
> > +	/* previous correction value is required for checksum update. */
> > +	memcpy(&correction_old,  &hdr->correction, sizeof(correction_old));
> > +	correction = (s64)be64_to_cpu(correction_old);
> > +
> > +	/* PTP correction field consists of 32 bit nanoseconds and 16 bit
> 
> 48 bit nanoseconds
> 
> > +	 * fractional nanoseconds.  Avoid shifting negative numbers.
> > +	 */
> > +	if (ns >= 0)
> > +		correction -= ns << 16;
> > +	else
> > +		correction += -ns << 16;
> 
> Again? Why? There's nothing wrong with left-shifting negative numbers,
> two's complement works the same (note that right-shifting is a different
> story, but that's not the case here).
I had the same in mind, but googling for this gave another result:
https://stackoverflow.com/questions/3784996/why-does-left-shift-operation-invoke-undefined-behaviour-when-the-left-side-oper

Did I understand something wrong?

But without "moving negative correction values to the tail tag", there should
be no negative values for t2.

> 
> > +
> > +	/* write new correction value */
> > +	put_unaligned_be64((u64)correction, &hdr->correction);
> > +
> > +	/* locate udp header */
> > +	if (type & PTP_CLASS_VLAN)
> > +		ptr += VLAN_HLEN;
> 
> Can't you just go back from the struct ptp_header pointer and avoid this
> check?
This should also remove the distinction between IPV4 and IPV6. Knowing
that it's any of these should be enough.

> 
> > +	ptr += ETH_HLEN;
> > +
> > +	switch (type & PTP_CLASS_PMASK) {
> > +	case PTP_CLASS_IPV4:
> > +		ptr += ((struct iphdr *)ptr)->ihl << 2;
> > +		uhdr = (struct udphdr *)ptr;
> > +		break;
> > +	case PTP_CLASS_IPV6:
> > +		ptr += IP6_HLEN;
> > +		uhdr = (struct udphdr *)ptr;
> > +		break;
> > +	}
> > +
> > +	if (!uhdr)
> > +		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;
> > +}
> > +

[PATCH net-next v2 09/11] net: dsa: microchip: ksz9477: add hardware time stamping support

[Christian Eggers]

Add 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. 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.

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/

One step TX time stamping of PDelay_Resp requires the RX time stamp from
the associated PDelay_Req message. linuxptp assumes that the RX time
stamp has already been subtracted from the PDelay_Req correction field
(as done by the TI PHYTER). linuxptp 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 doesn't support negative values on this field.
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 |   9 +-
 drivers/net/dsa/microchip/ksz9477_ptp.c  | 544 +++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz9477_ptp.h  |  27 ++
 include/linux/dsa/ksz_common.h           |  47 ++
 net/dsa/tag_ksz.c                        | 121 ++++-
 5 files changed, 740 insertions(+), 8 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477_main.c b/drivers/net/dsa/microchip/ksz9477_main.c
index 7d623400139f..42cd17c8c25d 100644
--- a/drivers/net/dsa/microchip/ksz9477_main.c
+++ b/drivers/net/dsa/microchip/ksz9477_main.c
@@ -1388,6 +1388,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,
@@ -1408,6 +1409,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          = ksz9477_ptp_port_txtstamp,
+	/* never defer rx delivery, tstamping is done via tail tagging */
+	.port_rxtstamp          = NULL,
 };
 
 static u32 ksz9477_get_port_addr(int port, int offset)
@@ -1554,7 +1560,8 @@ static irqreturn_t ksz9477_switch_irq_thread(int irq, void *dev_id)
 			if (ret)
 				return result;
 
-			/* ToDo: Add specific handling of port interrupts */
+			if (data8 & PORT_PTP_INT)
+				result |= ksz9477_ptp_port_interrupt(dev, port);
 		}
 	}
 
diff --git a/drivers/net/dsa/microchip/ksz9477_ptp.c b/drivers/net/dsa/microchip/ksz9477_ptp.c
index 44d7bbdea518..12698568b68b 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"
@@ -71,8 +74,10 @@ 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 timespec64 delta64 = ns_to_timespec64(delta);
 	s32 sec, nsec;
 	u16 data16;
+	unsigned long flags;
 	int ret;
 
 	mutex_lock(&dev->ptp_mutex);
@@ -103,6 +108,10 @@ static int ksz9477_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
 	if (ret)
 		goto error_return;
 
+	spin_lock_irqsave(&dev->ptp_clock_lock, flags);
+	dev->ptp_clock_time = timespec64_add(dev->ptp_clock_time, delta64);
+	spin_unlock_irqrestore(&dev->ptp_clock_lock, flags);
+
 error_return:
 	mutex_unlock(&dev->ptp_mutex);
 	return ret;
@@ -160,6 +169,7 @@ static int ksz9477_ptp_settime(struct ptp_clock_info *ptp, struct timespec64 con
 {
 	struct ksz_device *dev = container_of(ptp, struct ksz_device, ptp_caps);
 	u16 data16;
+	unsigned long flags;
 	int ret;
 
 	mutex_lock(&dev->ptp_mutex);
@@ -198,6 +208,10 @@ static int ksz9477_ptp_settime(struct ptp_clock_info *ptp, struct timespec64 con
 	if (ret)
 		goto error_return;
 
+	spin_lock_irqsave(&dev->ptp_clock_lock, flags);
+	dev->ptp_clock_time = *ts;
+	spin_unlock_irqrestore(&dev->ptp_clock_lock, flags);
+
 error_return:
 	mutex_unlock(&dev->ptp_mutex);
 	return ret;
@@ -208,9 +222,27 @@ static int ksz9477_ptp_enable(struct ptp_clock_info *ptp, struct ptp_clock_reque
 	return -ENOTTY;
 }
 
+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;
+	unsigned long flags;
+
+	mutex_lock(&dev->ptp_mutex);
+	_ksz9477_ptp_gettime(dev, &ts);
+	mutex_unlock(&dev->ptp_mutex);
+
+	spin_lock_irqsave(&dev->ptp_clock_lock, flags);
+	dev->ptp_clock_time = ts;
+	spin_unlock_irqrestore(&dev->ptp_clock_lock, flags);
+
+	return HZ;  /* reschedule in 1 second */
+}
+
 static int ksz9477_ptp_start_clock(struct ksz_device *dev)
 {
 	u16 data;
+	unsigned long flags;
 	int ret;
 
 	ret = ksz_read16(dev, REG_PTP_CLK_CTRL, &data);
@@ -230,6 +262,11 @@ static int ksz9477_ptp_start_clock(struct ksz_device *dev)
 	if (ret)
 		return ret;
 
+	spin_lock_irqsave(&dev->ptp_clock_lock, flags);
+	dev->ptp_clock_time.tv_sec = 0;
+	dev->ptp_clock_time.tv_nsec = 0;
+	spin_unlock_irqrestore(&dev->ptp_clock_lock, flags);
+
 	return 0;
 }
 
@@ -251,11 +288,249 @@ static int ksz9477_ptp_stop_clock(struct ksz_device *dev)
 	return 0;
 }
 
+/* Time stamping support */
+
+static int ksz9477_ptp_enable_mode(struct ksz_device *dev)
+{
+	u16 data;
+	int ret;
+
+	ret = ksz_read16(dev, REG_PTP_MSG_CONF1, &data);
+	if (ret)
+		return ret;
+
+	/* Enable PTP mode */
+	data |= PTP_ENABLE;
+	ret = ksz_write16(dev, REG_PTP_MSG_CONF1, data);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ksz9477_ptp_disable_mode(struct ksz_device *dev)
+{
+	u16 data;
+	int ret;
+
+	ret = ksz_read16(dev, REG_PTP_MSG_CONF1, &data);
+	if (ret)
+		return ret;
+
+	/* Disable PTP mode */
+	data &= ~PTP_ENABLE;
+	ret = ksz_write16(dev, REG_PTP_MSG_CONF1, data);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ksz9477_ptp_enable_port_ptp_interrupts(struct ksz_device *dev, int port)
+{
+	u32 addr = PORT_CTRL_ADDR(port, REG_PORT_INT_MASK);
+	u8 data;
+	int ret;
+
+	ret = ksz_read8(dev, addr, &data);
+	if (ret)
+		return ret;
+
+	/* Enable port PTP interrupt (0 means enabled) */
+	data &= ~PORT_PTP_INT;
+	ret = ksz_write8(dev, addr, data);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ksz9477_ptp_disable_port_ptp_interrupts(struct ksz_device *dev, int port)
+{
+	u32 addr = PORT_CTRL_ADDR(port, REG_PORT_INT_MASK);
+	u8 data;
+	int ret;
+
+	ret = ksz_read8(dev, addr, &data);
+	if (ret)
+		return ret;
+
+	/* Enable port PTP interrupt (1 means disabled) */
+	data |= PORT_PTP_INT;
+	ret = ksz_write8(dev, addr, data);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ksz9477_ptp_enable_port_egress_interrupts(struct ksz_device *dev, int port)
+{
+	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;
+
+	/* Enable port xdelay egress timestamp interrupt (1 means enabled) */
+	data |= PTP_PORT_XDELAY_REQ_INT;
+	ret = ksz_write16(dev, addr, data);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ksz9477_ptp_disable_port_egress_interrupts(struct ksz_device *dev, int port)
+{
+	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;
+
+	/* Disable port xdelay egress timestamp interrupts (0 means disabled) */
+	data &= PTP_PORT_XDELAY_REQ_INT;
+	ret = ksz_write16(dev, addr, data);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int ksz9477_ptp_port_init(struct ksz_device *dev, int port)
+{
+	struct ksz_port *prt = &dev->ports[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),
+			 &prt->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) {
+		ret = ksz9477_ptp_enable_port_ptp_interrupts(dev, port);
+		if (ret)
+			return ret;
+
+		ret = ksz9477_ptp_enable_port_egress_interrupts(dev, port);
+		if (ret)
+			goto error_disable_port_ptp_interrupts;
+	}
+
+	return 0;
+
+error_disable_port_ptp_interrupts:
+	if (port != dev->cpu_port)
+		ksz9477_ptp_disable_port_ptp_interrupts(dev, port);
+	return ret;
+}
+
+static void ksz9477_ptp_port_deinit(struct ksz_device *dev, int port)
+{
+	if (port != dev->cpu_port) {
+		ksz9477_ptp_disable_port_egress_interrupts(dev, port);
+		ksz9477_ptp_disable_port_ptp_interrupts(dev, port);
+	}
+}
+
+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:
+	for (--port; port >= 0; --port)
+		ksz9477_ptp_port_deinit(dev, port);
+	return ret;
+}
+
+static void ksz9477_ptp_ports_deinit(struct ksz_device *dev)
+{
+	int port;
+
+	for (port = dev->port_cnt - 1; port >= 0; --port)
+		ksz9477_ptp_port_deinit(dev, port);
+}
+
+/* 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;
+
+	ret = ksz_write16(dev, REG_PTP_MSG_CONF1, data);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+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;
+
+	ret = ksz_write16(dev, REG_PTP_MSG_CONF1, data);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 int ksz9477_ptp_init(struct ksz_device *dev)
 {
 	int ret;
 
 	mutex_init(&dev->ptp_mutex);
+	spin_lock_init(&dev->ptp_clock_lock);
 
 	/* PTP clock properties */
 
@@ -275,6 +550,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);
@@ -287,8 +563,44 @@ 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);
+	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
+	 * 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_ports_deinit;
+
 	return 0;
 
+error_ports_deinit:
+	ksz9477_ptp_ports_deinit(dev);
+error_disable_mode:
+	ksz9477_ptp_disable_mode(dev);
+error_unregister_clock:
+	ptp_clock_unregister(dev->ptp_clock);
 error_stop_clock:
 	ksz9477_ptp_stop_clock(dev);
 	return ret;
@@ -296,6 +608,238 @@ int ksz9477_ptp_init(struct ksz_device *dev)
 
 void ksz9477_ptp_deinit(struct ksz_device *dev)
 {
+	ksz9477_ptp_ports_deinit(dev);
+	ksz9477_ptp_disable_mode(dev);
 	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];
+	irqreturn_t result = IRQ_NONE;
+	u16 data;
+	int ret;
+
+	ret = ksz_read16(dev, addr, &data);
+	if (ret)
+		return IRQ_NONE;
+
+	if ((data & PTP_PORT_XDELAY_REQ_INT) && prt->tstamp_tx_xdelay_skb) {
+		/* Timestamp for Pdelay_Req / Delay_Req */
+		u32 tstamp_raw;
+		ktime_t tstamp;
+		struct skb_shared_hwtstamps shhwtstamps;
+		struct sk_buff *tmp_skb;
+
+		/* 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 result;
+
+		tstamp = ksz9477_decode_tstamp(tstamp_raw, prt->tstamp_tx_latency_ns);
+		memset(&shhwtstamps, 0, sizeof(shhwtstamps));
+		shhwtstamps.hwtstamp = ksz9477_tstamp_to_clock(dev, tstamp);
+
+		/* skb_complete_tx_timestamp() will free up the client to make
+		 * another timestamp-able transmit. We have to be ready for it
+		 * -- by clearing the ps->tx_skb "flag" -- beforehand.
+		 */
+
+		tmp_skb = prt->tstamp_tx_xdelay_skb;
+		prt->tstamp_tx_xdelay_skb = NULL;
+		clear_bit_unlock(KSZ_HWTSTAMP_TX_XDELAY_IN_PROGRESS, &prt->tstamp_state);
+		skb_complete_tx_timestamp(tmp_skb, &shhwtstamps);
+	}
+
+	/* 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 __always_unused,
+			    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];
+	bool tstamp_enable = false;
+
+	/* Prevent the TX/RX paths from trying to interact with the
+	 * timestamp hardware while we reconfigure it.
+	 */
+	clear_bit_unlock(KSZ_HWTSTAMP_ENABLED, &prt->tstamp_state);
+
+	/* reserved for future extensions */
+	if (config->flags)
+		return -EINVAL;
+
+	switch (config->tx_type) {
+	case HWTSTAMP_TX_OFF:
+		tstamp_enable = false;
+		break;
+	case HWTSTAMP_TX_ONESTEP_P2P:
+		tstamp_enable = true;
+		break;
+	default:
+		return -ERANGE;
+	}
+
+	switch (config->rx_filter) {
+	case HWTSTAMP_FILTER_NONE:
+		tstamp_enable = false;
+		break;
+	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
+		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L4_EVENT;  /* UDPv4/UDPv6 */
+		break;
+	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
+		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;  /* 802.3 ether */
+		break;
+	case HWTSTAMP_FILTER_PTP_V2_EVENT:
+	case HWTSTAMP_FILTER_PTP_V2_SYNC:
+	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
+		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;     /* UDP / 802.3 */
+		break;
+	case HWTSTAMP_FILTER_ALL:
+	default:
+		config->rx_filter = HWTSTAMP_FILTER_NONE;
+		return -ERANGE;
+	}
+
+	/* Once hardware has been configured, enable timestamp checks
+	 * in the RX/TX paths.
+	 */
+	if (tstamp_enable)
+		set_bit(KSZ_HWTSTAMP_ENABLED, &prt->tstamp_state);
+
+	return 0;
+}
+
+int ksz9477_ptp_port_hwtstamp_get(struct dsa_switch *ds, int port, struct ifreq *ifr)
+{
+	struct ksz_device *dev = ds->priv;
+	struct hwtstamp_config *port_tstamp_config = &dev->ports[port].tstamp_config;
+
+	return copy_to_user(ifr->ifr_data,
+			    port_tstamp_config, sizeof(*port_tstamp_config)) ? -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 *port_tstamp_config = &dev->ports[port].tstamp_config;
+	struct hwtstamp_config config;
+	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(port_tstamp_config, &config, sizeof(config));
+
+	return copy_to_user(ifr->ifr_data, &config, sizeof(config)) ? -EFAULT : 0;
+}
+
+/* Returns a pointer to the PTP header if the caller should time stamp,
+ * or NULL if the caller should not.
+ */
+static struct ptp_header *ksz9477_ptp_should_tstamp(struct ksz_port *port, struct sk_buff *skb,
+						    unsigned int type)
+{
+	enum ksz9477_ptp_event_messages msg_type;
+	struct ptp_header *hdr;
+
+	if (!test_bit(KSZ_HWTSTAMP_ENABLED, &port->tstamp_state))
+		return NULL;
+
+	hdr = ptp_parse_header(skb, type);
+	if (!hdr)
+		return NULL;
+
+	msg_type = ptp_get_msgtype(hdr, type);
+
+	switch (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_Event_Message_Pdelay_Req:
+		break;
+	default:
+		return NULL;
+	}
+
+	return hdr;
+}
+
+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;
+
+	/* KSZ9563 supports PTPv2 only */
+	if (!(type & PTP_CLASS_V2))
+		return false;
+
+	/* Should already been tested in dsa_skb_tx_timestamp()? */
+	if (!(skb_shinfo(clone)->tx_flags & SKBTX_HW_TSTAMP))
+		return false;
+
+	hdr = ksz9477_ptp_should_tstamp(prt, clone, type);
+	if (!hdr)
+		return false;
+
+	if (test_and_set_bit_lock(KSZ_HWTSTAMP_TX_XDELAY_IN_PROGRESS,
+				  &prt->tstamp_state))
+		return false;  /* free cloned skb */
+
+	prt->tstamp_tx_xdelay_skb = clone;
+
+	return true;  /* keep cloned skb */
+}
diff --git a/drivers/net/dsa/microchip/ksz9477_ptp.h b/drivers/net/dsa/microchip/ksz9477_ptp.h
index 0076538419fa..e8d50a086311 100644
--- a/drivers/net/dsa/microchip/ksz9477_ptp.h
+++ b/drivers/net/dsa/microchip/ksz9477_ptp.h
@@ -10,6 +10,9 @@
 #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"
 
 #if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ9477_PTP)
@@ -17,11 +20,35 @@
 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; }
+
+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; }
+
+static inline bool ksz9477_ptp_port_txtstamp(struct dsa_switch *ds, int port,
+					     struct sk_buff *clone, unsigned int type)
+	{ return false; }
+
 #endif
 
 #endif /* DRIVERS_NET_DSA_MICROCHIP_KSZ9477_PTP_H_ */
diff --git a/include/linux/dsa/ksz_common.h b/include/linux/dsa/ksz_common.h
index 4d5b6cc9429a..bfe8873b1636 100644
--- a/include/linux/dsa/ksz_common.h
+++ b/include/linux/dsa/ksz_common.h
@@ -5,16 +5,27 @@
 #ifndef _NET_DSA_KSZ_COMMON_H_
 #define _NET_DSA_KSZ_COMMON_H_
 
+#include <linux/bitfield.h>
+#include <linux/bits.h>
 #include <linux/gpio/consumer.h>
 #include <linux/kernel.h>
+#include <linux/ktime.h>
 #include <linux/mutex.h>
+#include <linux/net_tstamp.h>
 #include <linux/phy.h>
 #include <linux/ptp_clock_kernel.h>
+#include <linux/spinlock.h>
 #include <linux/regmap.h>
 #include <linux/timer.h>
 #include <linux/workqueue.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_platform_data;
 struct ksz_dev_ops;
 struct vlan_table;
@@ -25,6 +36,12 @@ struct ksz_port_mib {
 	u64 *counters;
 };
 
+/* state flags for ksz_port::tstamp_state */
+enum {
+	KSZ_HWTSTAMP_ENABLED,
+	KSZ_HWTSTAMP_TX_XDELAY_IN_PROGRESS,
+};
+
 struct ksz_port {
 	u16 member;
 	u16 vid_member;
@@ -41,6 +58,13 @@ struct ksz_port {
 
 	struct ksz_port_mib mib;
 	phy_interface_t interface;
+#if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ9477_PTP)
+	u16 tstamp_rx_latency_ns;   /* rx delay from wire to tstamp unit */
+	u16 tstamp_tx_latency_ns;   /* tx delay from tstamp unit to wire */
+	struct hwtstamp_config tstamp_config;
+	struct sk_buff *tstamp_tx_xdelay_skb;
+	unsigned long tstamp_state;
+#endif
 };
 
 struct ksz_device {
@@ -98,7 +122,30 @@ struct ksz_device {
 	struct ptp_clock *ptp_clock;
 	struct ptp_clock_info ptp_caps;
 	struct mutex ptp_mutex;
+	spinlock_t ptp_clock_lock; /* for ptp_clock_time */
+	/* approximated current time, read once per second from hardware */
+	struct timespec64 ptp_clock_time;
 #endif
 };
 
+/* KSZ9563 will only timestamp event messages */
+enum ksz9477_ptp_event_messages {
+	PTP_Event_Message_Sync        = 0x0,
+	PTP_Event_Message_Delay_Req   = 0x1,
+	PTP_Event_Message_Pdelay_Req  = 0x2,
+	PTP_Event_Message_Pdelay_Resp = 0x3,
+};
+
+/* 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_to_clock(struct ksz_device *ksz, ktime_t tstamp);
+
 #endif /* _NET_DSA_KSZ_COMMON_H_ */
diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
index 4820dbcedfa2..c16eb9eae19c 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,125 @@ 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)
+{
+	/* We send always 0 in the tail tag.  For PDelay_Resp, the ingress
+	 * time stamp of the PDelay_Req message has already been subtracted from
+	 * the correction field on rx.
+	 */
+	put_unaligned_be32(0, skb_put(skb, KSZ9477_PTP_TAG_LEN));
+}
+
+ktime_t ksz9477_tstamp_to_clock(struct ksz_device *ksz, ktime_t tstamp)
+{
+	struct timespec64 ts = ktime_to_timespec64(tstamp);
+	struct timespec64 ptp_clock_time;
+	struct timespec64 diff;
+	unsigned long flags;
+
+	spin_lock_irqsave(&ksz->ptp_clock_lock, flags);
+	ptp_clock_time = ksz->ptp_clock_time;
+	spin_unlock_irqrestore(&ksz->ptp_clock_lock, flags);
+
+	/* 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_to_clock);
+
+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);
+	u8 *tstamp_raw = tag - KSZ9477_PTP_TAG_LEN;
+	enum ksz9477_ptp_event_messages msg_type;
+	struct dsa_switch *ds = dev->dsa_ptr->ds;
+	struct ksz_device *ksz = ds->priv;
+	struct ksz_port *prt = &ksz->ports[port];
+	struct ptp_header *ptp_hdr;
+	unsigned int ptp_type;
+	ktime_t tstamp;
+
+	/* convert time stamp and write to skb */
+	tstamp = ksz9477_decode_tstamp(get_unaligned_be32(tstamp_raw),
+				       -prt->tstamp_rx_latency_ns);
+	memset(hwtstamps, 0, sizeof(*hwtstamps));
+	hwtstamps->hwtstamp = ksz9477_tstamp_to_clock(ksz, 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;
+
+	msg_type = ptp_get_msgtype(ptp_hdr, ptp_type);
+	if (msg_type != PTP_Event_Message_Pdelay_Req)
+		return;
+
+	/* Only subtract 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.
+	 */
+	ptp_onestep_p2p_move_t2_to_correction(skb, ptp_type, ptp_hdr, tstamp);
+}
+#else   /* IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ9477_PTP) */
+static void ksz9477_xmit_timestamp(struct sk_buff *skb __maybe_unused)
+{
+}
+
+static void ksz9477_rcv_timestamp(struct sk_buff *skb __maybe_unused, u8 *tag __maybe_unused,
+				  struct net_device *dev __maybe_unused,
+				  unsigned int port __maybe_unused)
+{
+}
+#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 +219,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);
 
@@ -138,8 +242,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 +255,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 +273,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);
 
@@ -183,7 +290,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

Re: [PATCH net-next v2 09/11] net: dsa: microchip: ksz9477: add hardware time stamping support

[Vladimir Oltean]

On Thu, Nov 12, 2020 at 04:35:35PM +0100, Christian Eggers wrote:
> Add 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. 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.
>
> 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/
>
> One step TX time stamping of PDelay_Resp requires the RX time stamp from
> the associated PDelay_Req message. linuxptp assumes that the RX time
> stamp has already been subtracted from the PDelay_Req correction field
> (as done by the TI PHYTER). linuxptp will echo back the value of the

No, not TI PHYTER, this was discussed before, but the ZHAW InES PTP time
stamping IP core.

> correction field in the PDelay_Resp message.

Misleading. Not just linuxptp will do that. Any PTP stack must do that.

> 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 doesn't support negative values on this field.

Old comment.

> 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 |   9 +-
>  drivers/net/dsa/microchip/ksz9477_ptp.c  | 544 +++++++++++++++++++++++
>  drivers/net/dsa/microchip/ksz9477_ptp.h  |  27 ++
>  include/linux/dsa/ksz_common.h           |  47 ++
>  net/dsa/tag_ksz.c                        | 121 ++++-
>  5 files changed, 740 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/dsa/microchip/ksz9477_main.c b/drivers/net/dsa/microchip/ksz9477_main.c
> index 7d623400139f..42cd17c8c25d 100644
> --- a/drivers/net/dsa/microchip/ksz9477_main.c
> +++ b/drivers/net/dsa/microchip/ksz9477_main.c
> @@ -1388,6 +1388,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,
> @@ -1408,6 +1409,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          = ksz9477_ptp_port_txtstamp,
> +	/* never defer rx delivery, tstamping is done via tail tagging */
> +	.port_rxtstamp          = NULL,
>  };
>
>  static u32 ksz9477_get_port_addr(int port, int offset)
> @@ -1554,7 +1560,8 @@ static irqreturn_t ksz9477_switch_irq_thread(int irq, void *dev_id)
>  			if (ret)
>  				return result;
>
> -			/* ToDo: Add specific handling of port interrupts */
> +			if (data8 & PORT_PTP_INT)
> +				result |= ksz9477_ptp_port_interrupt(dev, port);

No... why are you making assumptions about the value of IRQ_NONE?

>  		}
>  	}
>
> diff --git a/drivers/net/dsa/microchip/ksz9477_ptp.c b/drivers/net/dsa/microchip/ksz9477_ptp.c
> index 44d7bbdea518..12698568b68b 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"
> @@ -71,8 +74,10 @@ 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 timespec64 delta64 = ns_to_timespec64(delta);
>  	s32 sec, nsec;
>  	u16 data16;
> +	unsigned long flags;

Reverse Christmas tree variable declaration, please.

>  	int ret;
>
>  	mutex_lock(&dev->ptp_mutex);
> @@ -103,6 +108,10 @@ static int ksz9477_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
>  	if (ret)
>  		goto error_return;
>
> +	spin_lock_irqsave(&dev->ptp_clock_lock, flags);

I believe that spin_lock_irqsave is unnecessary, since there is no code
that runs in hardirq context.

> +	dev->ptp_clock_time = timespec64_add(dev->ptp_clock_time, delta64);
> +	spin_unlock_irqrestore(&dev->ptp_clock_lock, flags);
> +
>  error_return:
>  	mutex_unlock(&dev->ptp_mutex);
>  	return ret;
> @@ -160,6 +169,7 @@ static int ksz9477_ptp_settime(struct ptp_clock_info *ptp, struct timespec64 con
>  {
>  	struct ksz_device *dev = container_of(ptp, struct ksz_device, ptp_caps);
>  	u16 data16;
> +	unsigned long flags;
>  	int ret;
>
>  	mutex_lock(&dev->ptp_mutex);
> @@ -198,6 +208,10 @@ static int ksz9477_ptp_settime(struct ptp_clock_info *ptp, struct timespec64 con
>  	if (ret)
>  		goto error_return;
>
> +	spin_lock_irqsave(&dev->ptp_clock_lock, flags);
> +	dev->ptp_clock_time = *ts;
> +	spin_unlock_irqrestore(&dev->ptp_clock_lock, flags);
> +
>  error_return:
>  	mutex_unlock(&dev->ptp_mutex);
>  	return ret;
> @@ -208,9 +222,27 @@ static int ksz9477_ptp_enable(struct ptp_clock_info *ptp, struct ptp_clock_reque
>  	return -ENOTTY;
>  }
>
> +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;
> +	unsigned long flags;
> +
> +	mutex_lock(&dev->ptp_mutex);
> +	_ksz9477_ptp_gettime(dev, &ts);
> +	mutex_unlock(&dev->ptp_mutex);
> +
> +	spin_lock_irqsave(&dev->ptp_clock_lock, flags);
> +	dev->ptp_clock_time = ts;
> +	spin_unlock_irqrestore(&dev->ptp_clock_lock, flags);
> +
> +	return HZ;  /* reschedule in 1 second */
> +}
> +
>  static int ksz9477_ptp_start_clock(struct ksz_device *dev)
>  {
>  	u16 data;
> +	unsigned long flags;
>  	int ret;
>
>  	ret = ksz_read16(dev, REG_PTP_CLK_CTRL, &data);
> @@ -230,6 +262,11 @@ static int ksz9477_ptp_start_clock(struct ksz_device *dev)
>  	if (ret)
>  		return ret;
>
> +	spin_lock_irqsave(&dev->ptp_clock_lock, flags);
> +	dev->ptp_clock_time.tv_sec = 0;
> +	dev->ptp_clock_time.tv_nsec = 0;
> +	spin_unlock_irqrestore(&dev->ptp_clock_lock, flags);
> +
>  	return 0;
>  }
>
> @@ -251,11 +288,249 @@ static int ksz9477_ptp_stop_clock(struct ksz_device *dev)
>  	return 0;
>  }
>
> +/* Time stamping support */
> +
> +static int ksz9477_ptp_enable_mode(struct ksz_device *dev)
> +{
> +	u16 data;
> +	int ret;
> +
> +	ret = ksz_read16(dev, REG_PTP_MSG_CONF1, &data);
> +	if (ret)
> +		return ret;
> +
> +	/* Enable PTP mode */
> +	data |= PTP_ENABLE;
> +	ret = ksz_write16(dev, REG_PTP_MSG_CONF1, data);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

	return ksz_write16(dev, REG_PTP_MSG_CONF1, data);

> +}
> +
> +static int ksz9477_ptp_disable_mode(struct ksz_device *dev)
> +{
> +	u16 data;
> +	int ret;
> +
> +	ret = ksz_read16(dev, REG_PTP_MSG_CONF1, &data);
> +	if (ret)
> +		return ret;
> +
> +	/* Disable PTP mode */
> +	data &= ~PTP_ENABLE;
> +	ret = ksz_write16(dev, REG_PTP_MSG_CONF1, data);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

Please merge this with ksz9477_ptp_enable_mode.

> +
> +static int ksz9477_ptp_enable_port_ptp_interrupts(struct ksz_device *dev, int port)
> +{
> +	u32 addr = PORT_CTRL_ADDR(port, REG_PORT_INT_MASK);
> +	u8 data;
> +	int ret;
> +
> +	ret = ksz_read8(dev, addr, &data);
> +	if (ret)
> +		return ret;
> +
> +	/* Enable port PTP interrupt (0 means enabled) */
> +	data &= ~PORT_PTP_INT;
> +	ret = ksz_write8(dev, addr, data);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int ksz9477_ptp_disable_port_ptp_interrupts(struct ksz_device *dev, int port)
> +{
> +	u32 addr = PORT_CTRL_ADDR(port, REG_PORT_INT_MASK);
> +	u8 data;
> +	int ret;
> +
> +	ret = ksz_read8(dev, addr, &data);
> +	if (ret)
> +		return ret;
> +
> +	/* Enable port PTP interrupt (1 means disabled) */
> +	data |= PORT_PTP_INT;
> +	ret = ksz_write8(dev, addr, data);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

Same comments as above.

> +
> +static int ksz9477_ptp_enable_port_egress_interrupts(struct ksz_device *dev, int port)

Could we make this line shorter?

> +{
> +	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;
> +
> +	/* Enable port xdelay egress timestamp interrupt (1 means enabled) */
> +	data |= PTP_PORT_XDELAY_REQ_INT;
> +	ret = ksz_write16(dev, addr, data);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int ksz9477_ptp_disable_port_egress_interrupts(struct ksz_device *dev, int port)
> +{
> +	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;
> +
> +	/* Disable port xdelay egress timestamp interrupts (0 means disabled) */
> +	data &= PTP_PORT_XDELAY_REQ_INT;
> +	ret = ksz_write16(dev, addr, data);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

Don't you get tired of copy-pasting code? :)

> +
> +static int ksz9477_ptp_port_init(struct ksz_device *dev, int port)
> +{
> +	struct ksz_port *prt = &dev->ports[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),
> +			 &prt->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) {

Just return early for the CPU port.
	if (port == dev->cpu_port)
		return 0;

> +		ret = ksz9477_ptp_enable_port_ptp_interrupts(dev, port);
> +		if (ret)
> +			return ret;
> +
> +		ret = ksz9477_ptp_enable_port_egress_interrupts(dev, port);
> +		if (ret)
> +			goto error_disable_port_ptp_interrupts;
> +	}
> +
> +	return 0;
> +
> +error_disable_port_ptp_interrupts:
> +	if (port != dev->cpu_port)
> +		ksz9477_ptp_disable_port_ptp_interrupts(dev, port);

The "if" condition here is completely unnecessary since the only goto
that can reach this code has already satisfied that condition.

> +	return ret;
> +}
> +
> +static void ksz9477_ptp_port_deinit(struct ksz_device *dev, int port)
> +{
> +	if (port != dev->cpu_port) {

Return early?

> +		ksz9477_ptp_disable_port_egress_interrupts(dev, port);
> +		ksz9477_ptp_disable_port_ptp_interrupts(dev, port);
> +	}
> +}
> +
> +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:
> +	for (--port; port >= 0; --port)

Is that idiom runtime-tested? If not, or you're unsure if it works or
not, you can use:
	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 = dev->port_cnt - 1; port >= 0; --port)

Nice, but also probably not worth the effort?

> +		ksz9477_ptp_port_deinit(dev, port);
> +}
> +
> +/* 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;
> +
> +	ret = ksz_write16(dev, REG_PTP_MSG_CONF1, data);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

	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;
> +
> +	ret = ksz_write16(dev, REG_PTP_MSG_CONF1, data);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  int ksz9477_ptp_init(struct ksz_device *dev)
>  {
>  	int ret;
>
>  	mutex_init(&dev->ptp_mutex);
> +	spin_lock_init(&dev->ptp_clock_lock);
>
>  	/* PTP clock properties */
>
> @@ -275,6 +550,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);
> @@ -287,8 +563,44 @@ 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);
> +	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
> +	 * 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_ports_deinit;
> +
>  	return 0;
>
> +error_ports_deinit:
> +	ksz9477_ptp_ports_deinit(dev);
> +error_disable_mode:
> +	ksz9477_ptp_disable_mode(dev);
> +error_unregister_clock:
> +	ptp_clock_unregister(dev->ptp_clock);
>  error_stop_clock:
>  	ksz9477_ptp_stop_clock(dev);
>  	return ret;
> @@ -296,6 +608,238 @@ int ksz9477_ptp_init(struct ksz_device *dev)
>
>  void ksz9477_ptp_deinit(struct ksz_device *dev)
>  {
> +	ksz9477_ptp_ports_deinit(dev);
> +	ksz9477_ptp_disable_mode(dev);
>  	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];
> +	irqreturn_t result = IRQ_NONE;
> +	u16 data;
> +	int ret;
> +
> +	ret = ksz_read16(dev, addr, &data);
> +	if (ret)
> +		return IRQ_NONE;
> +
> +	if ((data & PTP_PORT_XDELAY_REQ_INT) && prt->tstamp_tx_xdelay_skb) {

It's almost as if you just want the "if" condition for the IRQ being
asserted, and WARN_ON(!prt->tstamp_tx_xdelay_skb). Otherwise, it would
mean that you received a TX timestamp interrupt for a packet that the
kernel has no clue of whatsoever, and you just keep chugging along.
Of course, you would still need to take care that you don't access a
NULL pointer, and you would still need to clear the IRQ, as you do now.

> +		/* Timestamp for Pdelay_Req / Delay_Req */
> +		u32 tstamp_raw;
> +		ktime_t tstamp;
> +		struct skb_shared_hwtstamps shhwtstamps;
> +		struct sk_buff *tmp_skb;

Reverse Christmas notation.

> +
> +		/* 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 result;

No reason whatsoever to keep IRQ_NONE in a variable.

> +
> +		tstamp = ksz9477_decode_tstamp(tstamp_raw, prt->tstamp_tx_latency_ns);
> +		memset(&shhwtstamps, 0, sizeof(shhwtstamps));
> +		shhwtstamps.hwtstamp = ksz9477_tstamp_to_clock(dev, tstamp);
> +
> +		/* skb_complete_tx_timestamp() will free up the client to make
> +		 * another timestamp-able transmit. We have to be ready for it
> +		 * -- by clearing the ps->tx_skb "flag" -- beforehand.
> +		 */
> +
> +		tmp_skb = prt->tstamp_tx_xdelay_skb;
> +		prt->tstamp_tx_xdelay_skb = NULL;
> +		clear_bit_unlock(KSZ_HWTSTAMP_TX_XDELAY_IN_PROGRESS, &prt->tstamp_state);
> +		skb_complete_tx_timestamp(tmp_skb, &shhwtstamps);
> +	}
> +
> +	/* 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 __always_unused,
> +			    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 =

Odd that you didn't continue on the same line? (same above)

> +			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];
> +	bool tstamp_enable = false;
> +
> +	/* Prevent the TX/RX paths from trying to interact with the
> +	 * timestamp hardware while we reconfigure it.
> +	 */
> +	clear_bit_unlock(KSZ_HWTSTAMP_ENABLED, &prt->tstamp_state);
> +
> +	/* reserved for future extensions */
> +	if (config->flags)
> +		return -EINVAL;
> +
> +	switch (config->tx_type) {
> +	case HWTSTAMP_TX_OFF:
> +		tstamp_enable = false;
> +		break;
> +	case HWTSTAMP_TX_ONESTEP_P2P:
> +		tstamp_enable = true;
> +		break;
> +	default:
> +		return -ERANGE;
> +	}
> +
> +	switch (config->rx_filter) {
> +	case HWTSTAMP_FILTER_NONE:
> +		tstamp_enable = false;
> +		break;
> +	case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
> +	case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
> +	case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
> +		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L4_EVENT;  /* UDPv4/UDPv6 */

I don't think the comments are really necessary. Additionally, you
exceed the 80 characters limit.

> +		break;
> +	case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
> +	case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
> +	case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:

I don't think you want to accept DELAY_REQ though? These rx_filters are
a bit arbitrary to begin with, and you might be one of the first to
differentiate here, but you really can't timestamp delay request/response.

> +		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT;  /* 802.3 ether */
> +		break;
> +	case HWTSTAMP_FILTER_PTP_V2_EVENT:
> +	case HWTSTAMP_FILTER_PTP_V2_SYNC:
> +	case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
> +		config->rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;     /* UDP / 802.3 */
> +		break;
> +	case HWTSTAMP_FILTER_ALL:
> +	default:
> +		config->rx_filter = HWTSTAMP_FILTER_NONE;
> +		return -ERANGE;
> +	}
> +
> +	/* Once hardware has been configured, enable timestamp checks
> +	 * in the RX/TX paths.
> +	 */
> +	if (tstamp_enable)
> +		set_bit(KSZ_HWTSTAMP_ENABLED, &prt->tstamp_state);
> +
> +	return 0;
> +}
> +
> +int ksz9477_ptp_port_hwtstamp_get(struct dsa_switch *ds, int port, struct ifreq *ifr)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	struct hwtstamp_config *port_tstamp_config = &dev->ports[port].tstamp_config;
> +
> +	return copy_to_user(ifr->ifr_data,
> +			    port_tstamp_config, sizeof(*port_tstamp_config)) ? -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 *port_tstamp_config = &dev->ports[port].tstamp_config;
> +	struct hwtstamp_config config;
> +	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(port_tstamp_config, &config, sizeof(config));
> +
> +	return copy_to_user(ifr->ifr_data, &config, sizeof(config)) ? -EFAULT : 0;
> +}
> +
> +/* Returns a pointer to the PTP header if the caller should time stamp,
> + * or NULL if the caller should not.
> + */
> +static struct ptp_header *ksz9477_ptp_should_tstamp(struct ksz_port *port, struct sk_buff *skb,
> +						    unsigned int type)

The fact that others have a function called "should_tstamp" which
returns a pointer to a struct ptp_header doesn't mean that this is a
good model to copy. Also, you exceeded 80 characters by quite a bit.

> +{
> +	enum ksz9477_ptp_event_messages msg_type;
> +	struct ptp_header *hdr;
> +
> +	if (!test_bit(KSZ_HWTSTAMP_ENABLED, &port->tstamp_state))
> +		return NULL;
> +
> +	hdr = ptp_parse_header(skb, type);
> +	if (!hdr)
> +		return NULL;
> +
> +	msg_type = ptp_get_msgtype(hdr, type);
> +
> +	switch (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_Event_Message_Pdelay_Req:
> +		break;
> +	default:
> +		return NULL;
> +	}
> +
> +	return hdr;
> +}
> +
> +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;
> +
> +	/* KSZ9563 supports PTPv2 only */
> +	if (!(type & PTP_CLASS_V2))
> +		return false;

It should be sufficient that you specified this in the
config->rx_filters from ksz9477_set_hwtstamp_config? I'm not even sure
who uses PTP v1 anyway.

> +
> +	/* Should already been tested in dsa_skb_tx_timestamp()? */
> +	if (!(skb_shinfo(clone)->tx_flags & SKBTX_HW_TSTAMP))
> +		return false;

Yeah, should have...
What do you think about this one though:
https://lore.kernel.org/netdev/20201104015834.mcn2eoibxf6j3ksw@skbuf/

> +
> +	hdr = ksz9477_ptp_should_tstamp(prt, clone, type);
> +	if (!hdr)
> +		return false;
> +
> +	if (test_and_set_bit_lock(KSZ_HWTSTAMP_TX_XDELAY_IN_PROGRESS,
> +				  &prt->tstamp_state))
> +		return false;  /* free cloned skb */
> +
> +	prt->tstamp_tx_xdelay_skb = clone;

Who do you think will guarantee you that a second timestampable packet
may not come in before the TX timestamp interrupt for the first one has
fired?

Either the hardware supports matching a TX timestamp to a PTP event
message (by sequenceId or whatnot), case in which you need more complex
logic in the IRQ handler, or the hardware can take a single TX timestamp
at a time, case in which you'll need an skb_queue and a process context
to wait for the TX timestamp of the previous PTP message before calling
dsa_enqueue_skb for the next PTP event message. There are already
implementations of both models in DSA that you can look at.

> +
> +	return true;  /* keep cloned skb */
> +}
> diff --git a/drivers/net/dsa/microchip/ksz9477_ptp.h b/drivers/net/dsa/microchip/ksz9477_ptp.h
> index 0076538419fa..e8d50a086311 100644
> --- a/drivers/net/dsa/microchip/ksz9477_ptp.h
> +++ b/drivers/net/dsa/microchip/ksz9477_ptp.h
> @@ -10,6 +10,9 @@
>  #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"
>
>  #if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ9477_PTP)
> @@ -17,11 +20,35 @@
>  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; }
> +
> +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; }
> +
> +static inline bool ksz9477_ptp_port_txtstamp(struct dsa_switch *ds, int port,
> +					     struct sk_buff *clone, unsigned int type)
> +	{ return false; }
> +

In networking we still have the 80-characters rule, please follow it.

>  #endif
>
>  #endif /* DRIVERS_NET_DSA_MICROCHIP_KSZ9477_PTP_H_ */
> diff --git a/include/linux/dsa/ksz_common.h b/include/linux/dsa/ksz_common.h
> index 4d5b6cc9429a..bfe8873b1636 100644
> --- a/include/linux/dsa/ksz_common.h
> +++ b/include/linux/dsa/ksz_common.h
> @@ -5,16 +5,27 @@
>  #ifndef _NET_DSA_KSZ_COMMON_H_
>  #define _NET_DSA_KSZ_COMMON_H_
>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/kernel.h>
> +#include <linux/ktime.h>
>  #include <linux/mutex.h>
> +#include <linux/net_tstamp.h>
>  #include <linux/phy.h>
>  #include <linux/ptp_clock_kernel.h>
> +#include <linux/spinlock.h>
>  #include <linux/regmap.h>
>  #include <linux/timer.h>
>  #include <linux/workqueue.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_platform_data;
>  struct ksz_dev_ops;
>  struct vlan_table;
> @@ -25,6 +36,12 @@ struct ksz_port_mib {
>  	u64 *counters;
>  };
>
> +/* state flags for ksz_port::tstamp_state */
> +enum {
> +	KSZ_HWTSTAMP_ENABLED,
> +	KSZ_HWTSTAMP_TX_XDELAY_IN_PROGRESS,
> +};
> +
>  struct ksz_port {
>  	u16 member;
>  	u16 vid_member;
> @@ -41,6 +58,13 @@ struct ksz_port {
>
>  	struct ksz_port_mib mib;
>  	phy_interface_t interface;
> +#if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ9477_PTP)
> +	u16 tstamp_rx_latency_ns;   /* rx delay from wire to tstamp unit */
> +	u16 tstamp_tx_latency_ns;   /* tx delay from tstamp unit to wire */
> +	struct hwtstamp_config tstamp_config;
> +	struct sk_buff *tstamp_tx_xdelay_skb;
> +	unsigned long tstamp_state;
> +#endif
>  };
>
>  struct ksz_device {
> @@ -98,7 +122,30 @@ struct ksz_device {
>  	struct ptp_clock *ptp_clock;
>  	struct ptp_clock_info ptp_caps;
>  	struct mutex ptp_mutex;
> +	spinlock_t ptp_clock_lock; /* for ptp_clock_time */
> +	/* approximated current time, read once per second from hardware */
> +	struct timespec64 ptp_clock_time;
>  #endif
>  };
>
> +/* KSZ9563 will only timestamp event messages */
> +enum ksz9477_ptp_event_messages {
> +	PTP_Event_Message_Sync        = 0x0,
> +	PTP_Event_Message_Delay_Req   = 0x1,
> +	PTP_Event_Message_Pdelay_Req  = 0x2,
> +	PTP_Event_Message_Pdelay_Resp = 0x3,
> +};
> +
> +/* 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_to_clock(struct ksz_device *ksz, ktime_t tstamp);
> +
>  #endif /* _NET_DSA_KSZ_COMMON_H_ */
> diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
> index 4820dbcedfa2..c16eb9eae19c 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,125 @@ 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)
> +{
> +	/* We send always 0 in the tail tag.  For PDelay_Resp, the ingress
> +	 * time stamp of the PDelay_Req message has already been subtracted from
> +	 * the correction field on rx.
> +	 */
> +	put_unaligned_be32(0, skb_put(skb, KSZ9477_PTP_TAG_LEN));
> +}

On TX you don't need the "PTP time stamp" field at all, can't you
disable it?

> +
> +ktime_t ksz9477_tstamp_to_clock(struct ksz_device *ksz, ktime_t tstamp)
> +{
> +	struct timespec64 ts = ktime_to_timespec64(tstamp);
> +	struct timespec64 ptp_clock_time;
> +	struct timespec64 diff;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ksz->ptp_clock_lock, flags);
> +	ptp_clock_time = ksz->ptp_clock_time;
> +	spin_unlock_irqrestore(&ksz->ptp_clock_lock, flags);
> +
> +	/* 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_to_clock);

It should be noted that I tried to find fault with this simplistic
implementation, where you just reconstruct the partial timestamp with
whatever PTP time you've got laying around, but I couldn't (or at least
I couldn't prove it).

In principle there should be a problem when the current PTP time wraps
around before you get the chance to reconstruct the partial timestamp.
That one you can detect by ensuring that the partial timestamp is larger
than the lower bits of the current PTP time. But that imposes the
restriction that the current PTP time must be collected after the
partial timestamp was taken by the MAC. And that means that PTP
timestamping must be done in process context, because it's accessing
SPI/I2C. Which means a very convoluted implementation, a nightmare
frankly.

The way you seem to be avoiding this, while still detecting the
wraparound case, is that you're just patching in the partial timestamp
into the "current" (i.e. at most 1 second old) PTP time, and then you
take a look at how well it fits. If the diff is larger than half the
wraparound interval, and positive (like: the "current" PTP time was
collected by the driver after the MAC took the partial timestamp), then
the current PTP time is too far ahead and must have wrapped around. If
the diff is large but negative (like: the partial timestamp, which is in
the "current" PTP time's future, has wrapped around), then the "current"
PTP time needs to be manually boosted.

This all seems to work because you have a somewhat workable budget of 4
seconds wraparound time. I am not sure that reading the PTP time once
per second is desirable under all circumstances if avoidable, and
there's also an even bigger assumption, which is that the PTP worker
will in fact get scheduled with a delay no larger than 2 seconds. I
suppose that is an academic only concern though.

So good for you that you can use a function so simple for timestamp
reconstruction. By the way, what about the name ksz9477_tstamp_reconstruct?
I don't exactly understand where does the "tstamp_to_clock" name come
from.

> +
> +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);
> +	u8 *tstamp_raw = tag - KSZ9477_PTP_TAG_LEN;
> +	enum ksz9477_ptp_event_messages msg_type;
> +	struct dsa_switch *ds = dev->dsa_ptr->ds;
> +	struct ksz_device *ksz = ds->priv;
> +	struct ksz_port *prt = &ksz->ports[port];
> +	struct ptp_header *ptp_hdr;
> +	unsigned int ptp_type;
> +	ktime_t tstamp;
> +
> +	/* convert time stamp and write to skb */
> +	tstamp = ksz9477_decode_tstamp(get_unaligned_be32(tstamp_raw),
> +				       -prt->tstamp_rx_latency_ns);
> +	memset(hwtstamps, 0, sizeof(*hwtstamps));
> +	hwtstamps->hwtstamp = ksz9477_tstamp_to_clock(ksz, 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;

And what does the comment have to do with the code?

> +
> +	__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;
> +
> +	msg_type = ptp_get_msgtype(ptp_hdr, ptp_type);
> +	if (msg_type != PTP_Event_Message_Pdelay_Req)
> +		return;

Would you be so generous to also modify ptp_get_msgtype to return this
enum of yours? There is also some opportunity for reuse with drivers/ptp/ptp_ines.c.

> +
> +	/* Only subtract 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.
> +	 */
> +	ptp_onestep_p2p_move_t2_to_correction(skb, ptp_type, ptp_hdr, tstamp);
> +}
> +#else   /* IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ9477_PTP) */
> +static void ksz9477_xmit_timestamp(struct sk_buff *skb __maybe_unused)
> +{
> +}
> +
> +static void ksz9477_rcv_timestamp(struct sk_buff *skb __maybe_unused, u8 *tag __maybe_unused,
> +				  struct net_device *dev __maybe_unused,
> +				  unsigned int port __maybe_unused)

Where did you see __maybe_unused being utilized in this way? And what's
so "maybe" about it? They are absolutely unused, and the compiler should
not complain. Please remove these variable attributes.

> +{
> +}
> +#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 +219,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);
>
> @@ -138,8 +242,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 +255,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 +273,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);
>
> @@ -183,7 +290,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,
>  };
>
> --

Long and exhausting patch... Could you split it up into a patch for the
control path and another one for the data path?

Re: [PATCH net-next v2 09/11] net: dsa: microchip: ksz9477: add hardware time stamping support

[Richard Cochran]

On Fri, Nov 13, 2020 at 04:40:20AM +0200, Vladimir Oltean wrote:
> > @@ -103,6 +108,10 @@ static int ksz9477_ptp_adjtime(struct ptp_clock_info *ptp, s64 delta)
> >  	if (ret)
> >  		goto error_return;
> >
> > +	spin_lock_irqsave(&dev->ptp_clock_lock, flags);
> 
> I believe that spin_lock_irqsave is unnecessary, since there is no code
> that runs in hardirq context.

The .adjtime method is in a system call context.  If all of the code
that manipulates dev->ptp_clock_time can sleep, then you can use a
mutex.  Otherwise spin_lock_irqsave is the safe choice.

Thanks,
Richard

Re: [PATCH net-next v2 09/11] net: dsa: microchip: ksz9477: add hardware time stamping support

[Christian Eggers]

On Friday, 13 November 2020, 03:40:20 CET, Vladimir Oltean wrote:
> On Thu, Nov 12, 2020 at 04:35:35PM +0100, Christian Eggers wrote:
[...]
> > @@ -103,6 +108,10 @@ static int ksz9477_ptp_adjtime(struct ptp_clock_info
> > *ptp, s64 delta)> 
> >  	if (ret)
> >  	
> >  		goto error_return;
> > 
> > +	spin_lock_irqsave(&dev->ptp_clock_lock, flags);
> 
> I believe that spin_lock_irqsave is unnecessary, since there is no code
> that runs in hardirq context.
I'll check this again. Originally I had only a mutex for everything, but later
it turned out that for ptp_clock_time a spinlock is required. Maybe this has
changed since starting of my work on the driver.

> 
> > +	dev->ptp_clock_time = timespec64_add(dev->ptp_clock_time, delta64);
> > +	spin_unlock_irqrestore(&dev->ptp_clock_lock, flags);
> > +

[...]

> Could we make this line shorter?
...
> Additionally, you exceed the 80 characters limit.
...
> Also, you exceeded 80 characters by quite a bit.
...
> In networking we still have the 80-characters rule, please follow it.
Can this be added to the netdev-FAQ (just below the section about 
"comment style convention")?

> > +static void ksz9477_ptp_ports_deinit(struct ksz_device *dev)
> > +{
> > +	int port;
> > +
> > +	for (port = dev->port_cnt - 1; port >= 0; --port)
> 
> Nice, but also probably not worth the effort?
What do you mean. Shall I used forward direction?

> 
> > +		ksz9477_ptp_port_deinit(dev, port);
> > +}
> 

[...]

> > +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;
> > +
> > +	/* KSZ9563 supports PTPv2 only */
> > +	if (!(type & PTP_CLASS_V2))
> > +		return false;
> 
> It should be sufficient that you specified this in the
> config->rx_filters from ksz9477_set_hwtstamp_config? I'm not even sure
> who uses PTP v1 anyway.
> 
> > +
> > +	/* Should already been tested in dsa_skb_tx_timestamp()? */
> > +	if (!(skb_shinfo(clone)->tx_flags & SKBTX_HW_TSTAMP))
> > +		return false;
> 
> Yeah, should have...
> What do you think about this one though:
> https://lore.kernel.org/netdev/20201104015834.mcn2eoibxf6j3ksw@skbuf/
I am not an expert for performance stuff. But for me it looks obvious that
cheaper checks should be performed first. What about also moving checking
for ops->port_txtstamp above ptp_classify_raw()?

Is there any reason why this isn't already applied?

> 
> > +
> > +	hdr = ksz9477_ptp_should_tstamp(prt, clone, type);
> > +	if (!hdr)
> > +		return false;
> > +
> > +	if (test_and_set_bit_lock(KSZ_HWTSTAMP_TX_XDELAY_IN_PROGRESS,
> > +				  &prt->tstamp_state))
> > +		return false;  /* free cloned skb */
> > +
> > +	prt->tstamp_tx_xdelay_skb = clone;
> 
> Who do you think will guarantee you that a second timestampable packet
> may not come in before the TX timestamp interrupt for the first one has
> fired?
> 
> Either the hardware supports matching a TX timestamp to a PTP event
> message (by sequenceId or whatnot),
it doesn't

> case in which you need more complex
> logic in the IRQ handler, or the hardware can take a single TX timestamp
> at a time, 
yes

> case in which you'll need an skb_queue and a process context
> to wait for the TX timestamp of the previous PTP message before calling
> dsa_enqueue_skb for the next PTP event message. There are already
> implementations of both models in DSA that you can look at.
In the past I got sometimes a "timeout waiting for hw timestamp" (or similar)
message from ptp4l. I am not sure whether this is still the case, but this may
explain this type of problems.

> 
> > +
> > +	return true;  /* keep cloned skb */
> > +}
> 

[...]

> > diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
> > index 4820dbcedfa2..c16eb9eae19c 100644
> > --- a/net/dsa/tag_ksz.c
> > +++ b/net/dsa/tag_ksz.c

[...]

> > 
> > +#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)
> > +{
> > +	/* We send always 0 in the tail tag.  For PDelay_Resp, the ingress
> > +	 * time stamp of the PDelay_Req message has already been subtracted from
> > +	 * the correction field on rx.
> > +	 */
> > +	put_unaligned_be32(0, skb_put(skb, KSZ9477_PTP_TAG_LEN));
> > +}
> 
> On TX you don't need the "PTP time stamp" field at all, can't you
> disable it?

No :-( 

From the data sheet, section 4.4.9:

"In the host-to-switch direction, the 4-byte timestamp field is always present when PTP is enabled, as shown in Figure 4-
6. This is true for all packets sent by the host, including IBA packets. The host uses this field to insert the receive time-
stamp from PTP Pdelay_Req messages into the Pdelay_Resp messages. For all other traffic and PTP message types,
the host should populate the timestamp field with zeros."

> 
> > +
> > +ktime_t ksz9477_tstamp_to_clock(struct ksz_device *ksz, ktime_t tstamp)
> > +{
> > +	struct timespec64 ts = ktime_to_timespec64(tstamp);
> > +	struct timespec64 ptp_clock_time;
> > +	struct timespec64 diff;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&ksz->ptp_clock_lock, flags);
> > +	ptp_clock_time = ksz->ptp_clock_time;
> > +	spin_unlock_irqrestore(&ksz->ptp_clock_lock, flags);
> > +
> > +	/* 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_to_clock);
> 
> It should be noted that I tried to find fault with this simplistic
> implementation, where you just reconstruct the partial timestamp with
> whatever PTP time you've got laying around, but I couldn't (or at least
> I couldn't prove it).
> 
> In principle there should be a problem when the current PTP time wraps
> around before you get the chance to reconstruct the partial timestamp.
> That one you can detect by ensuring that the partial timestamp is larger
> than the lower bits of the current PTP time. But that imposes the
> restriction that the current PTP time must be collected after the
> partial timestamp was taken by the MAC. And that means that PTP
> timestamping must be done in process context, because it's accessing
> SPI/I2C. Which means a very convoluted implementation, a nightmare
> frankly.
> 
> The way you seem to be avoiding this, while still detecting the
> wraparound case, is that you're just patching in the partial timestamp
> into the "current" (i.e. at most 1 second old) PTP time, and then you
> take a look at how well it fits. If the diff is larger than half the
> wraparound interval, and positive (like: the "current" PTP time was
> collected by the driver after the MAC took the partial timestamp), then
> the current PTP time is too far ahead and must have wrapped around. If
> the diff is large but negative (like: the partial timestamp, which is in
> the "current" PTP time's future, has wrapped around), then the "current"
> PTP time needs to be manually boosted.
> 
> This all seems to work because you have a somewhat workable budget of 4
> seconds wraparound time. I am not sure that reading the PTP time once
> per second is desirable under all circumstances if avoidable, and
> there's also an even bigger assumption, which is that the PTP worker
> will in fact get scheduled with a delay no larger than 2 seconds. I
> suppose that is an academic only concern though.
> 
> So good for you that you can use a function so simple for timestamp
> reconstruction. 
You already told me that another hardware has much less budget than 4 seconds.
How is timestamp reconstruction done there? Is there any code which I should
reuse?

> By the way, what about the name ksz9477_tstamp_reconstruct?
it's ok.

> I don't exactly understand where does the "tstamp_to_clock" name come
> from.
Invented by me. The function "converts" a time stamp into the "clock" time. 
But "reconstruct" fits better than "convert".

> 
> > +
> > +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);
> > +	u8 *tstamp_raw = tag - KSZ9477_PTP_TAG_LEN;
> > +	enum ksz9477_ptp_event_messages msg_type;
> > +	struct dsa_switch *ds = dev->dsa_ptr->ds;
> > +	struct ksz_device *ksz = ds->priv;
> > +	struct ksz_port *prt = &ksz->ports[port];
> > +	struct ptp_header *ptp_hdr;
> > +	unsigned int ptp_type;
> > +	ktime_t tstamp;
> > +
> > +	/* convert time stamp and write to skb */
> > +	tstamp = ksz9477_decode_tstamp(get_unaligned_be32(tstamp_raw),
> > +				       -prt->tstamp_rx_latency_ns);
> > +	memset(hwtstamps, 0, sizeof(*hwtstamps));
> > +	hwtstamps->hwtstamp = ksz9477_tstamp_to_clock(ksz, 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;
> 
> And what does the comment have to do with the code?
The comment if for the whole remaining part of the function, not for the single line.
>
> > +
> > +	__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;
> > +
> > +	msg_type = ptp_get_msgtype(ptp_hdr, ptp_type);
> > +	if (msg_type != PTP_Event_Message_Pdelay_Req)
> > +		return;
> 
> Would you be so generous to also modify ptp_get_msgtype to return this
> enum of yours? There is also some opportunity for reuse with
> drivers/ptp/ptp_ines.c.
no problem.

> > +
> > +	/* Only subtract 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.
> > +	 */
> > +	ptp_onestep_p2p_move_t2_to_correction(skb, ptp_type, ptp_hdr, tstamp);
> > +}
> > +#else   /* IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ9477_PTP) */
> > +static void ksz9477_xmit_timestamp(struct sk_buff *skb __maybe_unused)
> > +{
> > +}
> > +
> > +static void ksz9477_rcv_timestamp(struct sk_buff *skb __maybe_unused, u8
> > *tag __maybe_unused, +				  struct net_device *dev __maybe_unused,
> > +				  unsigned int port __maybe_unused)
> 
> Where did you see __maybe_unused being utilized in this way? And what's
> so "maybe" about it? They are absolutely unused, and the compiler should
> not complain. Please remove these variable attributes.
ok, __always_unused would fit.

I added the attributes due to Documentation/process/4.Coding.rst:

"Code submitted for review should, as a rule, not produce any compiler
warnings." [...] "Note that not all compiler warnings are enabled by default.  Build the
kernel with "make EXTRA_CFLAGS=-W" to get the full set."

I assumed that reducing the number of warnings raised by "-W" should be reduced
as a long term goal. Is this wrong.

Side note: Documentation/kbuild/makefiles.rst declares usage of EXTRA_CFLAGS as
deprecated.

[...]
> 
> Long and exhausting patch... Could you split it up into a patch for the
> control path and another one for the data path?
I am not sure whether the result will exactly look as you expect, but I'll try.

Thanks a lot for the fast and comprehensive review! As soon as I get your
answer regarding patch 3/11 (split ksz_common.h), I will perform the changes.

Have a nice weekend
Christian

Re: [PATCH net-next v2 09/11] net: dsa: microchip: ksz9477: add hardware time stamping support

[Vladimir Oltean]

On Fri, Nov 13, 2020 at 07:57:32PM +0100, Christian Eggers wrote:
> On Friday, 13 November 2020, 03:40:20 CET, Vladimir Oltean wrote:
> > On Thu, Nov 12, 2020 at 04:35:35PM +0100, Christian Eggers wrote:
> [...]
> > > @@ -103,6 +108,10 @@ static int ksz9477_ptp_adjtime(struct ptp_clock_info
> > > *ptp, s64 delta)>
> > >     if (ret)
> > >
> > >             goto error_return;
> > >
> > > +   spin_lock_irqsave(&dev->ptp_clock_lock, flags);
> >
> > I believe that spin_lock_irqsave is unnecessary, since there is no code
> > that runs in hardirq context.
> I'll check this again. Originally I had only a mutex for everything, but later
> it turned out that for ptp_clock_time a spinlock is required. Maybe this has
> changed since starting of my work on the driver.

Yes, it's called from the networking softirq.
The typical assumption is that the networking data path can run in
both hardirq and softirq context (or, well, in process context if it
gets picked up by ksoftirqd), so one would think that _irqsave would be
justified. But the hardirq stuff is only used by netpoll, for
netconsole. So you would never hit that condition for PTP timestamping.

> >
> > > +   dev->ptp_clock_time = timespec64_add(dev->ptp_clock_time, delta64);
> > > +   spin_unlock_irqrestore(&dev->ptp_clock_lock, flags);
> > > +
>
> [...]
>
> > Could we make this line shorter?
> ...
> > Additionally, you exceed the 80 characters limit.
> ...
> > Also, you exceeded 80 characters by quite a bit.
> ...
> > In networking we still have the 80-characters rule, please follow it.
> Can this be added to the netdev-FAQ (just below the section about
> "comment style convention")?
>
> > > +static void ksz9477_ptp_ports_deinit(struct ksz_device *dev)
> > > +{
> > > +   int port;
> > > +
> > > +   for (port = dev->port_cnt - 1; port >= 0; --port)
> >
> > Nice, but also probably not worth the effort?
> What do you mean. Shall I used forward direction?

Yes, that's what I meant.

> > > +
> > > +   /* Should already been tested in dsa_skb_tx_timestamp()? */
> > > +   if (!(skb_shinfo(clone)->tx_flags & SKBTX_HW_TSTAMP))
> > > +           return false;
> >
> > Yeah, should have...
> > What do you think about this one though:
> > https://lore.kernel.org/netdev/20201104015834.mcn2eoibxf6j3ksw@skbuf/
> I am not an expert for performance stuff. But for me it looks obvious that
> cheaper checks should be performed first. What about also moving checking
> for ops->port_txtstamp above ptp_classify_raw()?

I am no expert either. Also, it looks like I'm not even keeping on top
of things lately. I'll try to return to that investigation during this
weekend.

>
> Is there any reason why this isn't already applied?

Probably because nobody sent a patch for it?

> > case in which you'll need an skb_queue and a process context
> > to wait for the TX timestamp of the previous PTP message before calling
> > dsa_enqueue_skb for the next PTP event message. There are already
> > implementations of both models in DSA that you can look at.
> In the past I got sometimes a "timeout waiting for hw timestamp" (or similar)
> message from ptp4l. I am not sure whether this is still the case, but this may
> explain this type of problems.

Yeah, well, the default tx_timestamp_timeout value of 1 ms chosen by
ptp4l is not going to be enough in general for DSA. If you schedule a
workqueue for timestamping, that delay will only get worse, but luckily
you can increase the timestamp timeout value and all should be fine.

> > So good for you that you can use a function so simple for timestamp
> > reconstruction.
> You already told me that another hardware has much less budget than 4 seconds.

sja1105 has 24 bits of partial timestamp (and 1 bit measures 8 ns). So
it wraps around in 135 ms. You can imagine that periodically reading the
PTP clock over SPI is not an option there :)

> How is timestamp reconstruction done there? Is there any code which I should
> reuse?

No, I wasn't suggesting you reuse that logic, since it's very
error-prone. If you can get away with reconstruction done on-the-fly,
great. But just for reference:
- In drivers/net/dsa/sja1105/, the actual transmission of the PTP
  packets is done synchronously, from process context, and an interrupt
  is not even used. See sja1105_ptp_txtstamp_skb and
  sja1105_tstamp_reconstruct. Actually, more interesting would be the RX
  timestamping case, since we have a worse problem there: the partial
  PTP timestamp is obtained in softirq context, and we need process
  context for the current PTP time. For that, see sja1105_port_rxtstamp
  and sja1105_rxtstamp_work.
- In drivers/net/dsa/ocelot/, the reconstruction is done in IRQ context,
  since it is a memory-mapped switch and therefore, reading the PTP time
  is "cheap". See ocelot_get_txtstamp and ocelot_get_hwtimestamp.
The point is that both these drivers read the full PTP current time
_after_ the partial timestamp was obtained. That's what gives you a
solid guarantee that the "partial_timestamp > current_ptp_time & lower_bits"
condition means "wraparound occurred" and not something else.

> > > +static void ksz9477_rcv_timestamp(struct sk_buff *skb __maybe_unused, u8
> > > *tag __maybe_unused, +                                struct net_device *dev __maybe_unused,
> > > +                             unsigned int port __maybe_unused)
> >
> > Where did you see __maybe_unused being utilized in this way? And what's
> > so "maybe" about it? They are absolutely unused, and the compiler should
> > not complain. Please remove these variable attributes.
> ok, __always_unused would fit.
>
> I added the attributes due to Documentation/process/4.Coding.rst:
>
> "Code submitted for review should, as a rule, not produce any compiler
> warnings." [...] "Note that not all compiler warnings are enabled by default.  Build the
> kernel with "make EXTRA_CFLAGS=-W" to get the full set."
>
> I assumed that reducing the number of warnings raised by "-W" should be reduced
> as a long term goal. Is this wrong.

Uhm, I don't know of any compiler flag that would cause warnings for
unused arguments of a function. And the kernel uses this stub function
scheme for so many things, that even if that flag existed, the chance of
it getting enabled in the kernel's Makefile would be zero. It would
flood everybody in useless warning messages.

So please don't add __maybe_unused or __always_unused or whatever. Just
do what everybody else does.

> Side note: Documentation/kbuild/makefiles.rst declares usage of EXTRA_CFLAGS as
> deprecated.

Yeah, if you can build-test with "make W=1" and "make W=2" you should be
safe, no need to overthink anything. Beware though, there's going to be
a lot of output coming your way... There's also "make C=1" for sparse,
if you want to be proactive and avoid later patches from the static
analysis crew. But you'll need to use a source-built sparse binary
there, the one packaged by typical distributions won't cut it, and will
give up saying "error: too many
errors".

[PATCH net-next v2 10/11] net: dsa: microchip: ksz9477: add Pulse Per Second (PPS) support

[Christian Eggers]

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 |   2 +
 include/linux/dsa/ksz_common.h          |   6 +
 3 files changed, 257 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477_ptp.c b/drivers/net/dsa/microchip/ksz9477_ptp.c
index 12698568b68b..004bf43d33bf 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)
@@ -71,6 +191,8 @@ static int ksz9477_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
 	return 0;
 }
 
+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);
@@ -108,6 +230,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_irqsave(&dev->ptp_clock_lock, flags);
 	dev->ptp_clock_time = timespec64_add(dev->ptp_clock_time, delta64);
 	spin_unlock_irqrestore(&dev->ptp_clock_lock, flags);
@@ -208,6 +344,20 @@ static int ksz9477_ptp_settime(struct ptp_clock_info *ptp, struct timespec64 con
 	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_irqsave(&dev->ptp_clock_lock, flags);
 	dev->ptp_clock_time = *ts;
 	spin_unlock_irqrestore(&dev->ptp_clock_lock, flags);
@@ -217,9 +367,106 @@ static int ksz9477_ptp_settime(struct ptp_clock_info *ptp, struct timespec64 con
 	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 -ENOTTY;
+	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)
@@ -544,7 +791,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 e8d50a086311..03058e795678 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, struct ethtool_ts_info *ts);
@@ -33,6 +34,7 @@ bool ksz9477_ptp_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *
 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/include/linux/dsa/ksz_common.h b/include/linux/dsa/ksz_common.h
index bfe8873b1636..375f484ddb31 100644
--- a/include/linux/dsa/ksz_common.h
+++ b/include/linux/dsa/ksz_common.h
@@ -67,6 +67,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;
@@ -125,6 +130,7 @@ struct ksz_device {
 	spinlock_t ptp_clock_lock; /* for ptp_clock_time */
 	/* approximated current time, read once per second from hardware */
 	struct timespec64 ptp_clock_time;
+	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

Re: [PATCH net-next v2 10/11] net: dsa: microchip: ksz9477: add Pulse Per Second (PPS) support

[Vladimir Oltean]

On Thu, Nov 12, 2020 at 04:35:36PM +0100, Christian Eggers wrote:
>  static int ksz9477_ptp_enable(struct ptp_clock_info *ptp, struct ptp_clock_request *req, int on)
>  {
> -	return -ENOTTY;
> +	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;
>  }

Richard, do you think we can clarify the intended usage of PTP_CLK_REQ_PPS
in the documentation? It doesn't appear to be written anywhere that
PTP_ENABLE_PPS is supposed to enable event generation for the drivers/pps
subsystem. You would sort of have to know before you could find out...

Re: [PATCH net-next v2 10/11] net: dsa: microchip: ksz9477: add Pulse Per Second (PPS) support

[Richard Cochran]

On Fri, Nov 13, 2020 at 04:53:11AM +0200, Vladimir Oltean wrote:

> Richard, do you think we can clarify the intended usage of PTP_CLK_REQ_PPS
> in the documentation? It doesn't appear to be written anywhere that
> PTP_ENABLE_PPS is supposed to enable event generation for the drivers/pps
> subsystem. You would sort of have to know before you could find out...

Yes, please!

The poor naming is a source of eternal confusion.  I think that the
"hard pps" thing from NTP is not used very often, maybe never, but I
didn't know that when I first drafted the whole PHC subsystem.

Naturally developers of PHC device drivers think that this the PPS
that they need to implement.  After all, the name matches!

(Actually, at the time I thought that this would be the way to
synchronize the system clock to the PHC, but it turned out that
Miroslav's generic method in phc2sys worked very well, and so the hard
pps thing has little, if any, practical value.)

The documentation is vague, yes, but I think even more important
would be to remove the word PPS from the C-language identifiers.

I'm open to suggestions/patches on this...

Thanks,
Richard

[PATCH net-next v2 11/11] net: dsa: microchip: ksz9477: add periodic output support

[Christian Eggers]

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 +++++++++++++++++++++++-
 include/linux/dsa/ksz_common.h          |   6 +
 2 files changed, 202 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/microchip/ksz9477_ptp.c b/drivers/net/dsa/microchip/ksz9477_ptp.c
index 004bf43d33bf..1babacb9ee0d 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;
@@ -191,6 +205,7 @@ static int ksz9477_ptp_adjfine(struct ptp_clock_info *ptp, long scaled_ppm)
 	return 0;
 }
 
+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)
@@ -234,6 +249,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");
 
@@ -348,6 +372,15 @@ static int ksz9477_ptp_settime(struct ptp_clock_info *ptp, struct timespec64 con
 	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");
 
@@ -367,6 +400,159 @@ static int ksz9477_ptp_settime(struct ptp_clock_info *ptp, struct timespec64 con
 	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)
@@ -457,6 +643,15 @@ static int ksz9477_ptp_enable(struct ptp_clock_info *ptp, struct ptp_clock_reque
 	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);
@@ -790,7 +985,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/include/linux/dsa/ksz_common.h b/include/linux/dsa/ksz_common.h
index 375f484ddb31..f5c3db50b60a 100644
--- a/include/linux/dsa/ksz_common.h
+++ b/include/linux/dsa/ksz_common.h
@@ -16,6 +16,7 @@
 #include <linux/ptp_clock_kernel.h>
 #include <linux/spinlock.h>
 #include <linux/regmap.h>
+#include <linux/time64.h>
 #include <linux/timer.h>
 #include <linux/workqueue.h>
 #include <net/dsa.h>
@@ -69,6 +70,7 @@ struct ksz_port {
 
 enum ksz_ptp_tou_mode {
 	KSZ_PTP_TOU_IDLE,
+	KSZ_PTP_TOU_PEROUT,
 	KSZ_PTP_TOU_PPS,
 };
 
@@ -131,6 +133,10 @@ struct ksz_device {
 	/* approximated current time, read once per second from hardware */
 	struct timespec64 ptp_clock_time;
 	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