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