[RFC net-next 0/2] DSA driver draft for MaxLinear's gsw1xx series switch

[RFC net-next 0/2] DSA driver draft for MaxLinear's gsw1xx series switch

[Camel Guo]

Hi netdev,

Recently I had been working on implementing DSA driver for MaxLinear GSW145
switch. Here is my initial draft. Before proceeding futher, I am hoping to
get comments and cooperation as early/many as possible.

On my hand, I have an arm64 board with one GSW145 chip being connected via
MDIO management interface. According to the public datasheet of GSW145, the
chip itself on my board is in self-start (PS_NOWAIT=1b), managed switch
(PS_OP_MD=11) mode. On this board, only two slave ethernet ports (port0,
port1) of this chip are enabled.

As its datasheet says, this chip supports MDIO, SPI, UART management
interfaces in many operation modes (self-start/non-self-start,
managed/standalone). Unfortunately it is impossible for me to run this
draft on all of these combinations. It will be very helpful if anyone can
help to run it on different hardware setups and send the feedback back to
me.

I also noticed that lantiq_gswip.c looks similar as gsw145. I have no
access to the datasheet of the chip this file supports. Maybe they can be
merged together.

Best Regards
Camel Guo

Camel Guo (2):
  dt-bindings: net: dsa: add bindings for GSW Series switches
  net: dsa: Add driver for Maxlinear GSW1XX switch

 .../devicetree/bindings/net/dsa/mxl,gsw.yaml  | 140 +++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS                                   |   9 +
 drivers/net/dsa/Kconfig                       |  16 +
 drivers/net/dsa/Makefile                      |   2 +
 drivers/net/dsa/gsw1xx.h                      |  27 +
 drivers/net/dsa/gsw1xx_core.c                 | 823 ++++++++++++++++++
 drivers/net/dsa/gsw1xx_mdio.c                 | 128 +++
 8 files changed, 1147 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/dsa/mxl,gsw.yaml
 create mode 100644 drivers/net/dsa/gsw1xx.h
 create mode 100644 drivers/net/dsa/gsw1xx_core.c
 create mode 100644 drivers/net/dsa/gsw1xx_mdio.c


base-commit: 6143eca3578f486e4d58fe6fb5e96a5699c86fbc
-- 
2.30.2

[RFC net-next 1/2] dt-bindings: net: dsa: add bindings for GSW Series switches

[Camel Guo]

Add documentation and an example for Maxlinear's GSW Series Ethernet
switches.

Signed-off-by: Camel Guo <camel.guo@axis.com>
---
 .../devicetree/bindings/net/dsa/mxl,gsw.yaml  | 140 ++++++++++++++++++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS                                   |   6 +
 3 files changed, 148 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/dsa/mxl,gsw.yaml

diff --git a/Documentation/devicetree/bindings/net/dsa/mxl,gsw.yaml b/Documentation/devicetree/bindings/net/dsa/mxl,gsw.yaml
new file mode 100644
index 000000000000..8e124b7ec58c
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/dsa/mxl,gsw.yaml
@@ -0,0 +1,140 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/dsa/mxl,gsw.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Maxlinear GSW Series Switch Device Tree Bindings
+
+allOf:
+  - $ref: dsa.yaml#
+
+maintainers:
+  - Camel Guo <camel.guo@axis.com>
+
+description:
+  The Maxlinear's GSW Series Ethernet Switch is a highly integrated, low-power,
+  non-blocking Gigabit Ethernet Switch.
+
+properties:
+  compatible:
+    oneOf:
+      - enum:
+          - mxl,gsw145-mdio
+
+  reg:
+    maxItems: 1
+
+  mdio:
+    type: object
+
+    description:
+      Container of ethernet phy devices on the MDIO bus of GSW switch
+
+    properties:
+      '#address-cells':
+        const: 1
+      '#size-cells':
+        const: 0
+
+    allOf:
+      - $ref: "http://devicetree.org/schemas/net/ethernet-phy.yaml#"
+
+required:
+  - compatible
+  - reg
+  - mdio
+
+additionalProperties: true
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    mdio {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        switch@0 {
+            compatible = "mxl,gsw145-mdio";
+            #address-cells = <1>;
+            #size-cells = <0>;
+            reg = <0x0>;
+
+            ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                port@0 {
+                    reg = <0>;
+                    label = "lan0";
+                    phy-mode = "internal";
+                    phy-handle = <&phy0>;
+                };
+
+                port@1 {
+                    reg = <1>;
+                    label = "lan1";
+                    phy-mode = "internal";
+                    phy-handle = <&phy1>;
+                };
+
+                port@2 {
+                    reg = <2>;
+                    label = "lan2";
+                    phy-mode = "internal";
+                    phy-handle = <&phy2>;
+                    status = "disabled";
+                };
+
+                port@3 {
+                    reg = <3>;
+                    label = "lan3";
+                    phy-mode = "internal";
+                    phy-handle = <&phy3>;
+                    status = "disabled";
+                };
+
+                port@4 {
+                    reg = <4>;
+                    label = "lan4";
+                    phy-mode = "sgmii";
+                    status = "disabled";
+                };
+
+                port@5 {
+                    reg = <5>;
+                    label = "cpu";
+                    ethernet = <&eth0>;
+                    phy-mode = "rgmii-id";
+
+                    fixed-link {
+                        speed = <1000>;
+                        full-duplex;
+                        pause;
+                    };
+                };
+            };
+
+            mdio {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                phy0: ethernet-phy@0 {
+                    reg = <0>;
+                };
+
+                phy1: ethernet-phy@1 {
+                    reg = <1>;
+                };
+
+                phy2: ethernet-phy@2 {
+                    reg = <2>;
+                };
+
+                phy3: ethernet-phy@3 {
+                    reg = <3>;
+                };
+            };
+        };
+    };
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 6e323a380294..1d209115692c 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -863,6 +863,8 @@ patternProperties:
     deprecated: true
   "^mxicy,.*":
     description: Macronix International Co., Ltd.
+  "^mxl,.*":
+    description: MaxLinear, Inc.
   "^myir,.*":
     description: MYIR Tech Limited
   "^national,.*":
diff --git a/MAINTAINERS b/MAINTAINERS
index 657b223ed6b0..df88faabdb53 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12585,6 +12585,12 @@ L:	netdev@vger.kernel.org
 S:	Supported
 F:	drivers/net/phy/mxl-gpy.c
 
+MAXLINEAR GSW1XX SERIES ETHERNET SWITCH DRIVER
+M:	Camel Guo <camel.guo@axis.com>
+L:	netdev@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/net/dsa/mxl,gsw.yaml
+
 MCBA MICROCHIP CAN BUS ANALYZER TOOL DRIVER
 R:	Yasushi SHOJI <yashi@spacecubics.com>
 L:	linux-can@vger.kernel.org
-- 
2.30.2

Re: [RFC net-next 1/2] dt-bindings: net: dsa: add bindings for GSW Series switches

[Krzysztof Kozlowski]

On 25/10/2022 09:52, Camel Guo wrote:
> Add documentation and an example for Maxlinear's GSW Series Ethernet
> switches.
> 
> Signed-off-by: Camel Guo <camel.guo@axis.com>
> ---
>  .../devicetree/bindings/net/dsa/mxl,gsw.yaml  | 140 ++++++++++++++++++
>  .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
>  MAINTAINERS                                   |   6 +
>  3 files changed, 148 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/dsa/mxl,gsw.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/mxl,gsw.yaml b/Documentation/devicetree/bindings/net/dsa/mxl,gsw.yaml
> new file mode 100644
> index 000000000000..8e124b7ec58c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/dsa/mxl,gsw.yaml

Filename based on compatible, so mxl,gsw145-mdio.yaml. But see below.

> @@ -0,0 +1,140 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/dsa/mxl,gsw.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Maxlinear GSW Series Switch Device Tree Bindings

Drop "Device Tree Bindings"

> +
> +allOf:
> +  - $ref: dsa.yaml#
> +
> +maintainers:
> +  - Camel Guo <camel.guo@axis.com>
> +
> +description:
> +  The Maxlinear's GSW Series Ethernet Switch is a highly integrated, low-power,
> +  non-blocking Gigabit Ethernet Switch.
> +
> +properties:
> +  compatible:
> +    oneOf:

You do not have multiple choices, so no need for oneOf

> +      - enum:
> +          - mxl,gsw145-mdio

Why "mdio" suffix?

> +
> +  reg:
> +    maxItems: 1
> +
> +  mdio:
> +    type: object
> +
> +    description:
> +      Container of ethernet phy devices on the MDIO bus of GSW switch
> +
> +    properties:
> +      '#address-cells':
> +        const: 1
> +      '#size-cells':
> +        const: 0
> +
> +    allOf:

No need for allOf

> +      - $ref: "http://devicetree.org/schemas/net/ethernet-phy.yaml#"

That's not an URL. Open other schemas using ethernet-phy and check how
they are doing.

You miss:

    unevaluatedProperties: false

> +
> +required:
> +  - compatible
> +  - reg
> +  - mdio
> +
> +additionalProperties: true

This cannot be true. Again - open existing bindings and do like they are
doing, not differently.

You wanted here unevaluatedProperties: false.

> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    mdio {

Hmmm... switch with MDIO is part of MDIO?

> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        switch@0 {
> +            compatible = "mxl,gsw145-mdio";
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +            reg = <0x0>;

reg is a second property

> +
> +            ports {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +

Best regards,
Krzysztof

Re: [RFC net-next 1/2] dt-bindings: net: dsa: add bindings for GSW Series switches

[Andrew Lunn]

> > +      - enum:
> > +          - mxl,gsw145-mdio
> 
> Why "mdio" suffix?

I wondered about that as well. At some point in the future, there
could be an SPI version of this driver, and a UART version. Would they
all use the same compatible, and then context it used to determine the
correct binding? I think the kernel would be happy to do that, but i
don't know if the YAML tools can support that?

> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +
> > +    mdio {
> 
> Hmmm... switch with MDIO is part of MDIO?

Happens a lot. Nothing wrong with this.

	Andrew

Re: [RFC net-next 1/2] dt-bindings: net: dsa: add bindings for GSW Series switches

[Krzysztof Kozlowski]

On 25/10/2022 11:01, Andrew Lunn wrote:
>>> +      - enum:
>>> +          - mxl,gsw145-mdio
>>
>> Why "mdio" suffix?
> 
> I wondered about that as well. At some point in the future, there
> could be an SPI version of this driver, and a UART version. Would they
> all use the same compatible, and then context it used to determine the
> correct binding? I think the kernel would be happy to do that, but i
> don't know if the YAML tools can support that?

In general the bus should not be encoded in the device compatible. On
which bus this device sits, is determined from the parent, not from the
device compatible. As you wrote the context is used to determine
properties. There are few exceptions, though, but I think this is not a
candidate for such.

> 
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/gpio/gpio.h>
>>> +
>>> +    mdio {
>>
>> Hmmm... switch with MDIO is part of MDIO?
> 
> Happens a lot. Nothing wrong with this.

OK, everyday learning :)

Best regards,
Krzysztof

Re: [RFC net-next 1/2] dt-bindings: net: dsa: add bindings for GSW Series switches

[Krzysztof Kozlowski]

On 27/10/2022 02:34, Camel Guo wrote:
> On 10/25/22 16:27, Krzysztof Kozlowski wrote:
>  > On 25/10/2022 09:52, Camel Guo wrote:
>  >> Add documentation and an example for Maxlinear's GSW Series Ethernet
>  >> switches.
>  >>
>  >> Signed-off-by: Camel Guo <camel.guo@axis.com>
>  >> ---
>  >>  .../devicetree/bindings/net/dsa/mxl,gsw.yaml  | 140 ++++++++++++++++++
>  >>  .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
>  >>  MAINTAINERS                                   |   6 +
>  >>  3 files changed, 148 insertions(+)
>  >>  create mode 100644 
> Documentation/devicetree/bindings/net/dsa/mxl,gsw.yaml
>  >>
>  >> diff --git a/Documentation/devicetree/bindings/net/dsa/mxl,gsw.yaml 
> b/Documentation/devicetree/bindings/net/dsa/mxl,gsw.yaml
>  >> new file mode 100644
>  >> index 000000000000..8e124b7ec58c
>  >> --- /dev/null
>  >> +++ b/Documentation/devicetree/bindings/net/dsa/mxl,gsw.yaml
>  >
>  > Filename based on compatible, so mxl,gsw145-mdio.yaml. But see below.
> 
> I hope in future more gsw1xx chips (e.g: GSW150, GSW120) can be added
> and more management modes (e.g: uart, spi) can be supported. 

Maybe you will add support, maybe not. If these compatibles were
mentioned now, would be different topic, but there are not.

> So I think
> maybe mxl.gsw.yaml is more generic. Otherwise maybe in future someone
> has to add files like mxl,gsw150-uart.yaml, mxl,gsw120-spi.yaml

No, they can be added here, with or without renaming the file.

> 
>  >
>  >> @@ -0,0 +1,140 @@
>  >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>  >> +%YAML 1.2
>  >> +---
>  >> +$id: http://devicetree.org/schemas/net/dsa/mxl,gsw.yaml#
>  > <http://devicetree.org/schemas/net/dsa/mxl,gsw.yaml#>
>  >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>  > <http://devicetree.org/meta-schemas/core.yaml#>
>  >> +
>  >> +title: Maxlinear GSW Series Switch Device Tree Bindings
>  >
>  > Drop "Device Tree Bindings"
> 
> Ack. will do in v2
> 
>  >
>  >> +
>  >> +allOf:
>  >> +  - $ref: dsa.yaml#
>  >> +
>  >> +maintainers:
>  >> +  - Camel Guo <camel.guo@axis.com>
>  >> +
>  >> +description:
>  >> +  The Maxlinear's GSW Series Ethernet Switch is a highly 
> integrated, low-power,
>  >> +  non-blocking Gigabit Ethernet Switch.
>  >> +
>  >> +properties:
>  >> +  compatible:
>  >> +    oneOf:
>  >
>  > You do not have multiple choices, so no need for oneOf >
> 
> Currently that is true. But according to the datasheet of gsw1xx, it
> could be easily to expand to other gsw1xx chips and other management
> interfaces (e.g: spi, uart). If so, we could add something like:
> mxl,gsw145-spi
> mxl,gsw150-mdio


So expand it now... Anyway enum allows you to add new types, so why why
oneOf?

> ...
> 
> 
>  >> +      - enum:
>  >> +          - mxl,gsw145-mdio
>  >
>  > Why "mdio" suffix?
> 
> Inspired by others dsa chips.
> lan9303.txt:  - "smsc,lan9303-mdio" for mdio managed mode
> lantiq-gswip.txt:- compatible   : "lantiq,xrx200-mdio" for the MDIO bus
> inside the GSWIP
> nxp,sja1105.yaml:                  - nxp,sja1110-base-t1-mdio

As I replied to Andrew, this is discouraged.

Best regards,
Krzysztof

Re: [RFC net-next 1/2] dt-bindings: net: dsa: add bindings for GSW Series switches

[Vladimir Oltean]

Hi Camel,

On Thu, Oct 27, 2022 at 08:46:27AM -0400, Krzysztof Kozlowski wrote:
> >  >> +      - enum:
> >  >> +          - mxl,gsw145-mdio
> >  >
> >  > Why "mdio" suffix?
> > 
> > Inspired by others dsa chips.
> > lan9303.txt:  - "smsc,lan9303-mdio" for mdio managed mode
> > lantiq-gswip.txt:- compatible   : "lantiq,xrx200-mdio" for the MDIO bus
> > inside the GSWIP
> > nxp,sja1105.yaml:                  - nxp,sja1110-base-t1-mdio
> 
> As I replied to Andrew, this is discouraged.

Let's compare apples to apples, shall we?
"nxp,sja1110-base-t1-mdio" is the 100Base-T1 MDIO controller of the
NXP SJA1110 switch, hence the name. It is not a SJA1110 switch connected
over MDIO.

Re: [RFC net-next 1/2] dt-bindings: net: dsa: add bindings for GSW Series switches

[Krzysztof Kozlowski]

On 27/10/2022 09:57, Vladimir Oltean wrote:
> Hi Camel,
> 
> On Thu, Oct 27, 2022 at 08:46:27AM -0400, Krzysztof Kozlowski wrote:
>>>  >> +      - enum:
>>>  >> +          - mxl,gsw145-mdio
>>>  >
>>>  > Why "mdio" suffix?
>>>
>>> Inspired by others dsa chips.
>>> lan9303.txt:  - "smsc,lan9303-mdio" for mdio managed mode
>>> lantiq-gswip.txt:- compatible   : "lantiq,xrx200-mdio" for the MDIO bus
>>> inside the GSWIP
>>> nxp,sja1105.yaml:                  - nxp,sja1110-base-t1-mdio
>>
>> As I replied to Andrew, this is discouraged.
> 
> Let's compare apples to apples, shall we?
> "nxp,sja1110-base-t1-mdio" is the 100Base-T1 MDIO controller of the
> NXP SJA1110 switch, hence the name. It is not a SJA1110 switch connected
> over MDIO.

Thanks for clarifying. Then this could be fine. Let me then explain what
is discouraged:
1. Adding bus suffixes to the compatible, so for example foo,bar LED
controller is on I2C bus, so you call it "foo,bar-i2c".

2. Adding device types to the compatible, if this is the only
function/variant of the device, so for example calling foo,bar LED
controller "foo,bar-led". This makes sense in case of multi functional
devices (PMICs, SoCs), but not standalone ones.

So what do we have here? Is it one of the cases above?

Best regards,
Krzysztof

Re: [RFC net-next 1/2] dt-bindings: net: dsa: add bindings for GSW Series switches

[Vladimir Oltean]

On Thu, Oct 27, 2022 at 12:08:06PM -0400, Krzysztof Kozlowski wrote:
> On 27/10/2022 09:57, Vladimir Oltean wrote:
> > Hi Camel,
> > 
> > On Thu, Oct 27, 2022 at 08:46:27AM -0400, Krzysztof Kozlowski wrote:
> >>>  >> +      - enum:
> >>>  >> +          - mxl,gsw145-mdio
> >>>  >
> >>>  > Why "mdio" suffix?
> >>>
> >>> Inspired by others dsa chips.
> >>> lan9303.txt:  - "smsc,lan9303-mdio" for mdio managed mode
> >>> lantiq-gswip.txt:- compatible   : "lantiq,xrx200-mdio" for the MDIO bus
> >>> inside the GSWIP
> >>> nxp,sja1105.yaml:                  - nxp,sja1110-base-t1-mdio
> >>
> >> As I replied to Andrew, this is discouraged.
> > 
> > Let's compare apples to apples, shall we?
> > "nxp,sja1110-base-t1-mdio" is the 100Base-T1 MDIO controller of the
> > NXP SJA1110 switch, hence the name. It is not a SJA1110 switch connected
> > over MDIO.
> 
> Thanks for clarifying. Then this could be fine. Let me then explain what
> is discouraged:
> 1. Adding bus suffixes to the compatible, so for example foo,bar LED
> controller is on I2C bus, so you call it "foo,bar-i2c".
> 
> 2. Adding device types to the compatible, if this is the only
> function/variant of the device, so for example calling foo,bar LED
> controller "foo,bar-led". This makes sense in case of multi functional
> devices (PMICs, SoCs), but not standalone ones.
> 
> So what do we have here? Is it one of the cases above?

We are in agreement about SJA1110 (it's in case 2), this is what I said
about Camel's comparison not being apples to apples. "mxl,gsw145-mdio"
is in case 1, and I've also been recommending people to not add such
suffixes to compatible strings (also see the discussion with Colin
Foster about "mscc,vsc7512-switch" vs "mscc,vsc7512-ext-switch" to
denote an "external" switch which is otherwise the exact same hw but on
a different bus).

Re: [RFC net-next 1/2] dt-bindings: net: dsa: add bindings for GSW Series switches

[Rob Herring]

On Tue, 25 Oct 2022 15:52:40 +0200, Camel Guo wrote:
> Add documentation and an example for Maxlinear's GSW Series Ethernet
> switches.
> 
> Signed-off-by: Camel Guo <camel.guo@axis.com>
> ---
>  .../devicetree/bindings/net/dsa/mxl,gsw.yaml  | 140 ++++++++++++++++++
>  .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
>  MAINTAINERS                                   |   6 +
>  3 files changed, 148 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/dsa/mxl,gsw.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/dsa/mxl,gsw.yaml: properties:mdio:allOf:0:$ref: 'http://devicetree.org/schemas/net/ethernet-phy.yaml#' should not be valid under {'pattern': '^https?://'}
	from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/dsa/mxl,gsw.example.dtb: switch@0: mdio: 'reg' is a required property
	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/dsa/mxl,gsw.yaml

doc reference errors (make refcheckdocs):

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

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

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: [RFC net-next 1/2] dt-bindings: net: dsa: add bindings for GSW Series switches

[Vladimir Oltean]

Hi Camel,

On Tue, Oct 25, 2022 at 03:52:40PM +0200, Camel Guo wrote:
> +additionalProperties: true

I don't think the switch schema should have additionalProperties: true.
Only shared schemas should. WHat should be here is "unevaluatedProperties: false".

> +                port@5 {
> +                    reg = <5>;
> +                    label = "cpu";

Please drop label = "cpu" for the CPU port, it is not needed/not parsed.

> +                    ethernet = <&eth0>;
> +                    phy-mode = "rgmii-id";
> +
> +                    fixed-link {
> +                        speed = <1000>;
> +                        full-duplex;
> +                        pause;
> +                    };
> +                };
> +            };

[RFC net-next 2/2] net: dsa: Add driver for Maxlinear GSW1XX switch

[Camel Guo]

Add initial framework for Maxlinear's GSW1xx switch and
currently only GSW145 in MDIO managed mode is supported.

Signed-off-by: Camel Guo <camel.guo@axis.com>
---
 MAINTAINERS                   |   3 +
 drivers/net/dsa/Kconfig       |  16 +
 drivers/net/dsa/Makefile      |   2 +
 drivers/net/dsa/gsw1xx.h      |  27 ++
 drivers/net/dsa/gsw1xx_core.c | 823 ++++++++++++++++++++++++++++++++++
 drivers/net/dsa/gsw1xx_mdio.c | 128 ++++++
 6 files changed, 999 insertions(+)
 create mode 100644 drivers/net/dsa/gsw1xx.h
 create mode 100644 drivers/net/dsa/gsw1xx_core.c
 create mode 100644 drivers/net/dsa/gsw1xx_mdio.c

diff --git a/MAINTAINERS b/MAINTAINERS
index df88faabdb53..40371dc9e2dd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12590,6 +12590,9 @@ M:	Camel Guo <camel.guo@axis.com>
 L:	netdev@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/net/dsa/mxl,gsw.yaml
+F:	drivers/net/dsa/gsw1xx.h
+F:	drivers/net/dsa/gsw1xx_core.c
+F:	drivers/net/dsa/gsw1xx_mdio.c
 
 MCBA MICROCHIP CAN BUS ANALYZER TOOL DRIVER
 R:	Yasushi SHOJI <yashi@spacecubics.com>
diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
index 07507b4820d7..af57b92f786a 100644
--- a/drivers/net/dsa/Kconfig
+++ b/drivers/net/dsa/Kconfig
@@ -122,4 +122,20 @@ config NET_DSA_VITESSE_VSC73XX_PLATFORM
 	  This enables support for the Vitesse VSC7385, VSC7388, VSC7395
 	  and VSC7398 SparX integrated ethernet switches, connected over
 	  a CPU-attached address bus and work in memory-mapped I/O mode.
+
+config NET_DSA_MXL_GSW1XX
+	tristate
+	select REGMAP
+	help
+	  This enables support for the Maxlinear GSW1XX integrated ethernet
+	  switch chips.
+
+config NET_DSA_MXL_GSW1XX_MDIO
+	tristate "MaxLinear GSW1XX ethernet switch in MDIO managed mode"
+	select NET_DSA_MXL_GSW1XX
+	select FIXED_PHY
+	help
+	  This enables access functions if the MaxLinear GSW1XX is configured
+	  for MDIO managed mode.
+
 endmenu
diff --git a/drivers/net/dsa/Makefile b/drivers/net/dsa/Makefile
index 16eb879e0cb4..022fc661107b 100644
--- a/drivers/net/dsa/Makefile
+++ b/drivers/net/dsa/Makefile
@@ -15,6 +15,8 @@ obj-$(CONFIG_NET_DSA_SMSC_LAN9303_MDIO) += lan9303_mdio.o
 obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX) += vitesse-vsc73xx-core.o
 obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX_PLATFORM) += vitesse-vsc73xx-platform.o
 obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX_SPI) += vitesse-vsc73xx-spi.o
+obj-$(CONFIG_NET_DSA_MXL_GSW1XX) += gsw1xx_core.o
+obj-$(CONFIG_NET_DSA_MXL_GSW1XX_MDIO) += gsw1xx_mdio.o
 obj-y				+= b53/
 obj-y				+= hirschmann/
 obj-y				+= microchip/
diff --git a/drivers/net/dsa/gsw1xx.h b/drivers/net/dsa/gsw1xx.h
new file mode 100644
index 000000000000..08b2975e1267
--- /dev/null
+++ b/drivers/net/dsa/gsw1xx.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef GSW1XX_H
+#define GSW1XX_H
+
+#include <linux/regmap.h>
+#include <linux/device.h>
+#include <net/dsa.h>
+
+struct gsw1xx_hw_info {
+	int max_ports;
+	int cpu_port;
+};
+
+struct gsw1xx_priv {
+	struct device *dev;
+	struct regmap *regmap;
+	struct dsa_switch *ds;
+	const struct gsw1xx_hw_info *hw_info;
+};
+
+extern const struct regmap_access_table gsw1xx_register_set;
+
+int gsw1xx_probe(struct gsw1xx_priv *priv, struct device *dev);
+void gsw1xx_remove(struct gsw1xx_priv *priv);
+void gsw1xx_shutdown(struct gsw1xx_priv *priv);
+
+#endif /* GSW1XX_H */
diff --git a/drivers/net/dsa/gsw1xx_core.c b/drivers/net/dsa/gsw1xx_core.c
new file mode 100644
index 000000000000..1b3cbee4addc
--- /dev/null
+++ b/drivers/net/dsa/gsw1xx_core.c
@@ -0,0 +1,823 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/delay.h>
+#include <linux/etherdevice.h>
+#include <linux/if_bridge.h>
+#include <linux/if_vlan.h>
+#include <linux/module.h>
+#include <linux/of_mdio.h>
+#include <linux/of_net.h>
+#include <linux/of_platform.h>
+#include <linux/phy.h>
+#include <linux/phylink.h>
+#include <linux/regmap.h>
+#include <net/dsa.h>
+
+#include "gsw1xx.h"
+
+/* GSW1XX MDIO Registers */
+#define GSW1XX_MDIO_BASE_ADDR		0xF400
+#define GSW1XX_MDIO_REG_LEN		0x0422
+#define GSW1XX_MDIO_GLOB		0x00
+#define  GSW1XX_MDIO_GLOB_ENABLE	BIT(15)
+#define GSW1XX_MDIO_CTRL		0x08
+#define  GSW1XX_MDIO_CTRL_BUSY		BIT(12)
+#define  GSW1XX_MDIO_CTRL_RD		BIT(11)
+#define  GSW1XX_MDIO_CTRL_WR		BIT(10)
+#define  GSW1XX_MDIO_CTRL_PHYAD_MASK	0x1F
+#define  GSW1XX_MDIO_CTRL_PHYAD_SHIFT	5
+#define  GSW1XX_MDIO_CTRL_REGAD_MASK	0x1F
+#define GSW1XX_MDIO_READ		0x09
+#define GSW1XX_MDIO_WRITE		0x0A
+#define GSW1XX_MDIO_MDC_CFG0		0x0B
+#define GSW1XX_MDIO_PHYp(p)		(0x15 - (p))
+#define  GSW1XX_MDIO_PHY_LINK_MASK	0x6000
+#define  GSW1XX_MDIO_PHY_LINK_AUTO	0x0000
+#define  GSW1XX_MDIO_PHY_LINK_DOWN	0x4000
+#define  GSW1XX_MDIO_PHY_LINK_UP	0x2000
+#define  GSW1XX_MDIO_PHY_SPEED_MASK	0x1800
+#define  GSW1XX_MDIO_PHY_SPEED_AUTO	0x1800
+#define  GSW1XX_MDIO_PHY_SPEED_M10	0x0000
+#define  GSW1XX_MDIO_PHY_SPEED_M100	0x0800
+#define  GSW1XX_MDIO_PHY_SPEED_G1	0x1000
+#define  GSW1XX_MDIO_PHY_FDUP_MASK	0x0600
+#define  GSW1XX_MDIO_PHY_FDUP_AUTO	0x0000
+#define  GSW1XX_MDIO_PHY_FDUP_EN	0x0200
+#define  GSW1XX_MDIO_PHY_FDUP_DIS	0x0600
+#define  GSW1XX_MDIO_PHY_FCONTX_MASK	0x0180
+#define  GSW1XX_MDIO_PHY_FCONTX_AUTO	0x0000
+#define  GSW1XX_MDIO_PHY_FCONTX_EN	0x0100
+#define  GSW1XX_MDIO_PHY_FCONTX_DIS	0x0180
+#define  GSW1XX_MDIO_PHY_FCONRX_MASK	0x0060
+#define  GSW1XX_MDIO_PHY_FCONRX_AUTO	0x0000
+#define  GSW1XX_MDIO_PHY_FCONRX_EN	0x0020
+#define  GSW1XX_MDIO_PHY_FCONRX_DIS	0x0060
+#define  GSW1XX_MDIO_PHY_ADDR_MASK	0x001F
+#define  GSW1XX_MDIO_PHY_MASK		(GSW1XX_MDIO_PHY_ADDR_MASK | \
+					 GSW1XX_MDIO_PHY_FCONRX_MASK | \
+					 GSW1XX_MDIO_PHY_FCONTX_MASK | \
+					 GSW1XX_MDIO_PHY_LINK_MASK | \
+					 GSW1XX_MDIO_PHY_SPEED_MASK | \
+					 GSW1XX_MDIO_PHY_FDUP_MASK)
+
+/* GSW1XX_IP Core Registers */
+#define GSW1XX_IP_BASE_ADDR		0xE000
+#define GSW1XX_IP_REG_LEN		0x0D46
+#define GSW1XX_IP_SWRES			0x000
+#define  GSW1XX_IP_SWRES_R1		BIT(1)	/* Software reset */
+#define  GSW1XX_IP_SWRES_R0		BIT(0)	/* Hardware reset */
+#define GSW1XX_IP_VERSION		0x013
+#define  GSW1XX_IP_VERSION_REV_SHIFT	0
+#define  GSW1XX_IP_VERSION_REV_MASK	GENMASK(7, 0)
+#define  GSW1XX_IP_VERSION_MOD_SHIFT	8
+#define  GSW1XX_IP_VERSION_MOD_MASK	GENMASK(15, 8)
+#define   GSW1XX_IP_VERSION_2_3		0x023
+
+#define GSW1XX_IP_BM_RAM_VAL(x)		(0x043 - (x))
+#define GSW1XX_IP_BM_RAM_ADDR		0x044
+#define GSW1XX_IP_BM_RAM_CTRL		0x045
+#define  GSW1XX_IP_BM_RAM_CTRL_BAS		BIT(15)
+#define  GSW1XX_IP_BM_RAM_CTRL_OPMOD		BIT(5)
+#define  GSW1XX_IP_BM_RAM_CTRL_ADDR_MASK	GENMASK(4, 0)
+#define GSW1XX_IP_BM_QUEUE_GCTRL		0x04A
+#define  GSW1XX_IP_BM_QUEUE_GCTRL_GL_MOD	BIT(10)
+/* buffer management Port Configuration Register */
+#define GSW1XX_IP_BM_PCFGp(p)		(0x080 + ((p) * 2))
+#define  GSW1XX_IP_BM_PCFG_CNTEN	BIT(0)	/* RMON Counter Enable */
+/* PCE */
+#define GSW1XX_IP_PCE_PMAP2		0x454	/* Default Multicast port map */
+#define GSW1XX_IP_PCE_PMAP3		0x455	/* Default Unknown Unicast port map */
+#define GSW1XX_IP_PCE_GCTRL_0		0x456
+#define  GSW1XX_IP_PCE_GCTRL_0_MTFL	BIT(0)  /* MAC Table Flushing */
+#define  GSW1XX_IP_PCE_GCTRL_0_MC_VALID	BIT(3)
+#define GSW1XX_IP_PCE_PCTRL_0p(p)	(0x480 + ((p) * 0xA))
+#define  GSW1XX_IP_PCE_PCTRL_0_PSTATE_LISTEN		0x0
+#define  GSW1XX_IP_PCE_PCTRL_0_PSTATE_RX		0x1
+#define  GSW1XX_IP_PCE_PCTRL_0_PSTATE_TX		0x2
+#define  GSW1XX_IP_PCE_PCTRL_0_PSTATE_LEARNING		0x3
+#define  GSW1XX_IP_PCE_PCTRL_0_PSTATE_FORWARDING	0x7
+#define  GSW1XX_IP_PCE_PCTRL_0_PSTATE_MASK		GENMASK(2, 0)
+
+#define GSW1XX_IP_MAC_FLEN			0x8C5
+#define GSW1XX_IP_MAC_CTRL_0p(p)		(0x903 + ((p) * 0xC))
+#define  GSW1XX_IP_MAC_CTRL_0_PADEN		BIT(8)
+#define  GSW1XX_IP_MAC_CTRL_0_FCS_EN		BIT(7)
+#define  GSW1XX_IP_MAC_CTRL_0_FCON_MASK		0x0070
+#define  GSW1XX_IP_MAC_CTRL_0_FCON_AUTO		0x0000
+#define  GSW1XX_IP_MAC_CTRL_0_FCON_RX		0x0010
+#define  GSW1XX_IP_MAC_CTRL_0_FCON_TX		0x0020
+#define  GSW1XX_IP_MAC_CTRL_0_FCON_RXTX		0x0030
+#define  GSW1XX_IP_MAC_CTRL_0_FCON_NONE		0x0040
+#define  GSW1XX_IP_MAC_CTRL_0_FDUP_MASK		0x000C
+#define  GSW1XX_IP_MAC_CTRL_0_FDUP_AUTO		0x0000
+#define  GSW1XX_IP_MAC_CTRL_0_FDUP_EN		0x0004
+#define  GSW1XX_IP_MAC_CTRL_0_FDUP_DIS		0x000C
+#define  GSW1XX_IP_MAC_CTRL_0_GMII_MASK		0x0003
+#define  GSW1XX_IP_MAC_CTRL_0_GMII_AUTO		0x0000
+#define  GSW1XX_IP_MAC_CTRL_0_GMII_MII		0x0001
+#define  GSW1XX_IP_MAC_CTRL_0_GMII_RGMII	0x0002
+#define GSW1XX_IP_MAC_CTRL_2p(p)		(0x905 + ((p) * 0xC))
+#define GSW1XX_IP_MAC_CTRL_2_MLEN		BIT(3) /* Maximum Untagged Frame Lnegth */
+#define GSW1XX_IP_MAC_CTRL_4p(p)		(0x907 + ((p) * 0xC))
+#define  GSW1XX_IP_MAC_CTRL_4_LPIEN		BIT(7)
+
+/* Ethernet Switch Fetch DMA Port Control Register */
+#define GSW1XX_IP_FDMA_PCTRLp(p)		(0xA80 + ((p) * 0x6))
+#define  GSW1XX_IP_FDMA_PCTRL_EN		BIT(0)	/* FDMA Port Enable */
+
+/* Ethernet Switch Store DMA Port Control Register */
+#define GSW1XX_IP_SDMA_PCTRLp(p)		(0xBC0 + ((p) * 0x6))
+#define  GSW1XX_IP_SDMA_PCTRL_EN		BIT(0)	/* SDMA Port Enable */
+#define  GSW1XX_IP_SDMA_PCTRL_FCEN		BIT(1)	/* Flow Control Enable */
+#define  GSW1XX_IP_SDMA_PCTRL_PAUFWD		BIT(3)	/* Pause Frame Forwarding */
+
+struct gsw1xx_rmon_cnt_desc {
+	unsigned int size;
+	unsigned int offset;
+	const char *name;
+};
+
+#define MIB_DESC(_size, _offset, _name)                                        \
+	{                                                                      \
+		.size = _size, .offset = _offset, .name = _name                \
+	}
+
+static const struct gsw1xx_rmon_cnt_desc gsw1xx_rmon_cnt[] = {
+	/** Receive Packet Count (only packets that are accepted and not discarded). */
+	MIB_DESC(1, 0x1F, "RxGoodPkts"),
+	MIB_DESC(1, 0x23, "RxUnicastPkts"),
+	MIB_DESC(1, 0x22, "RxMulticastPkts"),
+	MIB_DESC(1, 0x21, "RxFCSErrorPkts"),
+	MIB_DESC(1, 0x1D, "RxUnderSizeGoodPkts"),
+	MIB_DESC(1, 0x1E, "RxUnderSizeErrorPkts"),
+	MIB_DESC(1, 0x1B, "RxOversizeGoodPkts"),
+	MIB_DESC(1, 0x1C, "RxOversizeErrorPkts"),
+	MIB_DESC(1, 0x20, "RxGoodPausePkts"),
+	MIB_DESC(1, 0x1A, "RxAlignErrorPkts"),
+	MIB_DESC(1, 0x12, "Rx64BytePkts"),
+	MIB_DESC(1, 0x13, "Rx127BytePkts"),
+	MIB_DESC(1, 0x14, "Rx255BytePkts"),
+	MIB_DESC(1, 0x15, "Rx511BytePkts"),
+	MIB_DESC(1, 0x16, "Rx1023BytePkts"),
+	/** Receive Size 1024-1522 (or more, if configured) Packet Count. */
+	MIB_DESC(1, 0x17, "RxMaxBytePkts"),
+	MIB_DESC(1, 0x18, "RxDroppedPkts"),
+	MIB_DESC(1, 0x19, "RxFilteredPkts"),
+	MIB_DESC(2, 0x24, "RxGoodBytes"),
+	MIB_DESC(2, 0x26, "RxBadBytes"),
+	MIB_DESC(1, 0x11, "TxAcmDroppedPkts"),
+	MIB_DESC(1, 0x0C, "TxGoodPkts"),
+	MIB_DESC(1, 0x06, "TxUnicastPkts"),
+	MIB_DESC(1, 0x07, "TxMulticastPkts"),
+	MIB_DESC(1, 0x00, "Tx64BytePkts"),
+	MIB_DESC(1, 0x01, "Tx127BytePkts"),
+	MIB_DESC(1, 0x02, "Tx255BytePkts"),
+	MIB_DESC(1, 0x03, "Tx511BytePkts"),
+	MIB_DESC(1, 0x04, "Tx1023BytePkts"),
+	/** Transmit Size 1024-1522 (or more, if configured) Packet Count. */
+	MIB_DESC(1, 0x05, "TxMaxBytePkts"),
+	MIB_DESC(1, 0x08, "TxSingleCollCount"),
+	MIB_DESC(1, 0x09, "TxMultCollCount"),
+	MIB_DESC(1, 0x0A, "TxLateCollCount"),
+	MIB_DESC(1, 0x0B, "TxExcessCollCount"),
+	MIB_DESC(1, 0x0D, "TxPauseCount"),
+	MIB_DESC(1, 0x10, "TxDroppedPkts"),
+	MIB_DESC(2, 0x0E, "TxGoodBytes"),
+};
+
+static u32 gsw1xx_switch_r(struct gsw1xx_priv *priv, u32 offset)
+{
+	int ret = 0;
+	u32 val = 0;
+
+	ret = regmap_read(priv->regmap, GSW1XX_IP_BASE_ADDR + offset, &val);
+
+	return ret < 0 ? (u32)ret : val;
+}
+
+static void gsw1xx_switch_w(struct gsw1xx_priv *priv, u32 val, u32 offset)
+{
+	regmap_write(priv->regmap, GSW1XX_IP_BASE_ADDR + offset, val);
+}
+
+static u32 gsw1xx_mdio_r(struct gsw1xx_priv *priv, u32 offset)
+{
+	int ret = 0;
+	u32 val = 0;
+
+	ret = regmap_read(priv->regmap, GSW1XX_MDIO_BASE_ADDR + offset, &val);
+
+	return ret < 0 ? (u32)ret : val;
+}
+
+static void gsw1xx_mdio_w(struct gsw1xx_priv *priv, u32 val, u32 offset)
+{
+	regmap_write(priv->regmap, GSW1XX_MDIO_BASE_ADDR + offset, val);
+}
+
+static void gsw1xx_mdio_mask(struct gsw1xx_priv *priv, u32 clear, u32 set,
+			     u32 offset)
+{
+	u32 val = gsw1xx_mdio_r(priv, offset);
+
+	val &= ~(clear);
+	val |= set;
+	gsw1xx_mdio_w(priv, val, offset);
+}
+
+static void gsw1xx_switch_mask(struct gsw1xx_priv *priv, u32 clear, u32 set,
+			       u32 offset)
+{
+	u32 val = gsw1xx_switch_r(priv, offset);
+
+	val &= ~(clear);
+	val |= set;
+	gsw1xx_switch_w(priv, val, offset);
+}
+
+static u32 gsw1xx_switch_r_timeout(struct gsw1xx_priv *priv, u32 offset,
+				   u32 cleared)
+{
+	u32 val;
+
+	return read_poll_timeout(gsw1xx_switch_r, val, (val & cleared) == 0, 20,
+				 50000, true, priv, offset);
+}
+
+static int gsw1xx_mdio_poll(struct gsw1xx_priv *priv)
+{
+	int cnt = 100;
+
+	while (likely(cnt--)) {
+		u32 ctrl = gsw1xx_mdio_r(priv, GSW1XX_MDIO_CTRL);
+
+		if ((ctrl & GSW1XX_MDIO_CTRL_BUSY) == 0)
+			return 0;
+		usleep_range(20, 40);
+	}
+
+	return -ETIMEDOUT;
+}
+
+static int gsw1xx_mdio_wr(struct mii_bus *bus, int addr, int reg, u16 val)
+{
+	struct gsw1xx_priv *priv = bus->priv;
+	int err;
+
+	err = gsw1xx_mdio_poll(priv);
+	if (err) {
+		dev_err(&bus->dev, "timeout while waiting for MDIO bus\n");
+		return err;
+	}
+
+	gsw1xx_mdio_w(priv, val, GSW1XX_MDIO_WRITE);
+	gsw1xx_mdio_w(priv,
+		      GSW1XX_MDIO_CTRL_WR |
+			      ((addr & GSW1XX_MDIO_CTRL_PHYAD_MASK)
+			       << GSW1XX_MDIO_CTRL_PHYAD_SHIFT) |
+			      (reg & GSW1XX_MDIO_CTRL_REGAD_MASK),
+		      GSW1XX_MDIO_CTRL);
+
+	return 0;
+}
+
+static int gsw1xx_mdio_rd(struct mii_bus *bus, int addr, int reg)
+{
+	struct gsw1xx_priv *priv = bus->priv;
+	int err;
+
+	err = gsw1xx_mdio_poll(priv);
+	if (err) {
+		dev_err(&bus->dev, "timeout while waiting for MDIO bus\n");
+		return err;
+	}
+
+	gsw1xx_mdio_w(priv,
+		      GSW1XX_MDIO_CTRL_RD |
+			      ((addr & GSW1XX_MDIO_CTRL_PHYAD_MASK)
+			       << GSW1XX_MDIO_CTRL_PHYAD_SHIFT) |
+			      (reg & GSW1XX_MDIO_CTRL_REGAD_MASK),
+		      GSW1XX_MDIO_CTRL);
+
+	err = gsw1xx_mdio_poll(priv);
+	if (err) {
+		dev_err(&bus->dev, "timeout while waiting for MDIO bus\n");
+		return err;
+	}
+
+	return gsw1xx_mdio_r(priv, GSW1XX_MDIO_READ);
+}
+
+static int gsw1xx_mdio(struct gsw1xx_priv *priv, struct device_node *mdio_np)
+{
+	struct dsa_switch *ds = priv->ds;
+	int err;
+
+	ds->slave_mii_bus = mdiobus_alloc();
+	if (!ds->slave_mii_bus)
+		return -ENOMEM;
+
+	ds->slave_mii_bus->priv = priv;
+	ds->slave_mii_bus->read = gsw1xx_mdio_rd;
+	ds->slave_mii_bus->write = gsw1xx_mdio_wr;
+	ds->slave_mii_bus->name = "mxl,gsw1xx-mdio";
+	snprintf(ds->slave_mii_bus->id, MII_BUS_ID_SIZE, "%s-mii",
+		 dev_name(priv->dev));
+	ds->slave_mii_bus->parent = priv->dev;
+	ds->slave_mii_bus->phy_mask = ~ds->phys_mii_mask;
+
+	err = of_mdiobus_register(ds->slave_mii_bus, mdio_np);
+	if (err)
+		mdiobus_free(ds->slave_mii_bus);
+
+	return err;
+}
+
+static int gsw1xx_port_enable(struct dsa_switch *ds, int port,
+			      struct phy_device *phydev)
+{
+	struct gsw1xx_priv *priv = ds->priv;
+
+	if (!dsa_is_user_port(ds, port))
+		return 0;
+
+	/* RMON Counter Enable for port */
+	gsw1xx_switch_w(priv, GSW1XX_IP_BM_PCFG_CNTEN,
+			GSW1XX_IP_BM_PCFGp(port));
+
+	/* enable port fetch/store dma */
+	gsw1xx_switch_mask(priv, 0, GSW1XX_IP_FDMA_PCTRL_EN,
+			   GSW1XX_IP_FDMA_PCTRLp(port));
+	gsw1xx_switch_mask(priv, 0, GSW1XX_IP_SDMA_PCTRL_EN,
+			   GSW1XX_IP_SDMA_PCTRLp(port));
+
+	if (!dsa_is_cpu_port(ds, port)) {
+		u32 mdio_phy = 0;
+
+		if (phydev)
+			mdio_phy =
+				phydev->mdio.addr & GSW1XX_MDIO_PHY_ADDR_MASK;
+
+		gsw1xx_mdio_mask(priv, GSW1XX_MDIO_PHY_ADDR_MASK, mdio_phy,
+				 GSW1XX_MDIO_PHYp(port));
+	}
+
+	return 0;
+}
+
+static void gsw1xx_port_disable(struct dsa_switch *ds, int port)
+{
+	struct gsw1xx_priv *priv = ds->priv;
+
+	if (!dsa_is_user_port(ds, port))
+		return;
+
+	gsw1xx_switch_mask(priv, GSW1XX_IP_FDMA_PCTRL_EN, 0,
+			   GSW1XX_IP_FDMA_PCTRLp(port));
+	gsw1xx_switch_mask(priv, GSW1XX_IP_SDMA_PCTRL_EN, 0,
+			   GSW1XX_IP_SDMA_PCTRLp(port));
+}
+
+static int gsw1xx_setup(struct dsa_switch *ds)
+{
+	struct gsw1xx_priv *priv = ds->priv;
+	unsigned int cpu_port = priv->hw_info->cpu_port;
+	int i;
+	int err;
+
+	gsw1xx_switch_w(priv, GSW1XX_IP_SWRES_R0, GSW1XX_IP_SWRES);
+	usleep_range(5000, 10000);
+	gsw1xx_switch_w(priv, 0, GSW1XX_IP_SWRES);
+
+	/* disable port fetch/store dma on all ports */
+	for (i = 0; i < priv->hw_info->max_ports; i++)
+		gsw1xx_port_disable(ds, i);
+
+	/* enable Switch */
+	gsw1xx_mdio_mask(priv, 0, GSW1XX_MDIO_GLOB_ENABLE, GSW1XX_MDIO_GLOB);
+
+	gsw1xx_switch_w(priv, 0x7F, GSW1XX_IP_PCE_PMAP2);
+	gsw1xx_switch_w(priv, 0x7F, GSW1XX_IP_PCE_PMAP3);
+
+	/* Deactivate MDIO PHY auto polling since it affects mmd read/write.
+	 */
+	gsw1xx_mdio_w(priv, 0x0, GSW1XX_MDIO_MDC_CFG0);
+
+	gsw1xx_switch_mask(priv, 1, GSW1XX_IP_MAC_CTRL_2_MLEN,
+			   GSW1XX_IP_MAC_CTRL_2p(cpu_port));
+	gsw1xx_switch_mask(priv, 0, GSW1XX_IP_BM_QUEUE_GCTRL_GL_MOD,
+			   GSW1XX_IP_BM_QUEUE_GCTRL);
+
+	/* Flush MAC Table */
+	gsw1xx_switch_mask(priv, 0, GSW1XX_IP_PCE_GCTRL_0_MTFL,
+			   GSW1XX_IP_PCE_GCTRL_0);
+	err = gsw1xx_switch_r_timeout(priv, GSW1XX_IP_PCE_GCTRL_0,
+				      GSW1XX_IP_PCE_GCTRL_0_MTFL);
+	if (err) {
+		dev_err(priv->dev, "MAC flushing didn't finish\n");
+		return err;
+	}
+
+	gsw1xx_port_enable(ds, cpu_port, NULL);
+
+	return 0;
+}
+
+static enum dsa_tag_protocol gsw1xx_get_tag_protocol(struct dsa_switch *ds,
+						     int port,
+						     enum dsa_tag_protocol mp)
+{
+	return DSA_TAG_PROTO_NONE;
+}
+
+static void gsw1xx_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
+{
+	struct gsw1xx_priv *priv = ds->priv;
+	u32 stp_state;
+
+	switch (state) {
+	case BR_STATE_DISABLED:
+		gsw1xx_switch_mask(priv, GSW1XX_IP_SDMA_PCTRL_EN, 0,
+				   GSW1XX_IP_SDMA_PCTRLp(port));
+		return;
+	case BR_STATE_BLOCKING:
+	case BR_STATE_LISTENING:
+		stp_state = GSW1XX_IP_PCE_PCTRL_0_PSTATE_LISTEN;
+		break;
+	case BR_STATE_LEARNING:
+		stp_state = GSW1XX_IP_PCE_PCTRL_0_PSTATE_LEARNING;
+		break;
+	case BR_STATE_FORWARDING:
+		stp_state = GSW1XX_IP_PCE_PCTRL_0_PSTATE_FORWARDING;
+		break;
+	default:
+		dev_err(priv->dev, "invalid STP state: %d\n", state);
+		return;
+	}
+
+	gsw1xx_switch_mask(priv, 0, GSW1XX_IP_SDMA_PCTRL_EN,
+			   GSW1XX_IP_SDMA_PCTRLp(port));
+	gsw1xx_switch_mask(priv, GSW1XX_IP_PCE_PCTRL_0_PSTATE_MASK, stp_state,
+			   GSW1XX_IP_PCE_PCTRL_0p(port));
+}
+
+static void gsw1xx_port_set_link(struct gsw1xx_priv *priv, int port, bool link)
+{
+	u32 mdio_phy;
+
+	if (link)
+		mdio_phy = GSW1XX_MDIO_PHY_LINK_UP;
+	else
+		mdio_phy = GSW1XX_MDIO_PHY_LINK_DOWN;
+
+	gsw1xx_mdio_mask(priv, GSW1XX_MDIO_PHY_LINK_MASK, mdio_phy,
+			 GSW1XX_MDIO_PHYp(port));
+}
+
+static void gsw1xx_port_set_speed(struct gsw1xx_priv *priv, int port, int speed,
+				  phy_interface_t interface)
+{
+	u32 mdio_phy = 0, mac_ctrl_0 = 0;
+
+	switch (speed) {
+	case SPEED_10:
+		mdio_phy = GSW1XX_MDIO_PHY_SPEED_M10;
+		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_GMII_MII;
+		break;
+
+	case SPEED_100:
+		mdio_phy = GSW1XX_MDIO_PHY_SPEED_M100;
+		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_GMII_MII;
+		break;
+
+	case SPEED_1000:
+		mdio_phy = GSW1XX_MDIO_PHY_SPEED_G1;
+		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_GMII_RGMII;
+		break;
+	}
+
+	gsw1xx_mdio_mask(priv, GSW1XX_MDIO_PHY_SPEED_MASK, mdio_phy,
+			 GSW1XX_MDIO_PHYp(port));
+	gsw1xx_switch_mask(priv, GSW1XX_IP_MAC_CTRL_0_GMII_MASK, mac_ctrl_0,
+			   GSW1XX_IP_MAC_CTRL_0p(port));
+}
+
+static void gsw1xx_port_set_duplex(struct gsw1xx_priv *priv, int port,
+				   int duplex)
+{
+	u32 mac_ctrl_0, mdio_phy;
+
+	if (duplex == DUPLEX_FULL) {
+		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_FDUP_EN;
+		mdio_phy = GSW1XX_MDIO_PHY_FDUP_EN;
+	} else {
+		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_FDUP_DIS;
+		mdio_phy = GSW1XX_MDIO_PHY_FDUP_DIS;
+	}
+
+	gsw1xx_switch_mask(priv, GSW1XX_IP_MAC_CTRL_0_FDUP_MASK, mac_ctrl_0,
+			   GSW1XX_IP_MAC_CTRL_0p(port));
+	gsw1xx_mdio_mask(priv, GSW1XX_MDIO_PHY_FDUP_MASK, mdio_phy,
+			 GSW1XX_MDIO_PHYp(port));
+}
+
+static void gsw1xx_port_set_pause(struct gsw1xx_priv *priv, int port,
+				  bool tx_pause, bool rx_pause)
+{
+	u32 mac_ctrl_0, mdio_phy;
+
+	if (tx_pause && rx_pause) {
+		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_FCON_RXTX;
+		mdio_phy =
+			GSW1XX_MDIO_PHY_FCONTX_EN | GSW1XX_MDIO_PHY_FCONRX_EN;
+	} else if (tx_pause) {
+		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_FCON_TX;
+		mdio_phy =
+			GSW1XX_MDIO_PHY_FCONTX_EN | GSW1XX_MDIO_PHY_FCONRX_DIS;
+	} else if (rx_pause) {
+		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_FCON_RX;
+		mdio_phy =
+			GSW1XX_MDIO_PHY_FCONTX_DIS | GSW1XX_MDIO_PHY_FCONRX_EN;
+	} else {
+		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_FCON_NONE;
+		mdio_phy =
+			GSW1XX_MDIO_PHY_FCONTX_DIS | GSW1XX_MDIO_PHY_FCONRX_DIS;
+	}
+
+	gsw1xx_switch_mask(priv, GSW1XX_IP_MAC_CTRL_0_FCON_MASK, mac_ctrl_0,
+			   GSW1XX_IP_MAC_CTRL_0p(port));
+	gsw1xx_mdio_mask(priv,
+			 GSW1XX_MDIO_PHY_FCONTX_MASK | GSW1XX_MDIO_PHY_FCONRX_MASK,
+			 mdio_phy, GSW1XX_MDIO_PHYp(port));
+}
+
+static void gsw1xx_phylink_mac_link_down(struct dsa_switch *ds, int port,
+					 unsigned int mode,
+					 phy_interface_t interface)
+{
+	struct gsw1xx_priv *priv = ds->priv;
+
+	if (!dsa_is_cpu_port(ds, port))
+		gsw1xx_port_set_link(priv, port, false);
+}
+
+static void gsw1xx_phylink_mac_link_up(struct dsa_switch *ds, int port,
+				       unsigned int mode,
+				       phy_interface_t interface,
+				       struct phy_device *phydev, int speed,
+				       int duplex, bool tx_pause, bool rx_pause)
+{
+	struct gsw1xx_priv *priv = ds->priv;
+
+	if (!dsa_is_cpu_port(ds, port)) {
+		gsw1xx_port_set_link(priv, port, true);
+		gsw1xx_port_set_speed(priv, port, speed, interface);
+		gsw1xx_port_set_duplex(priv, port, duplex);
+		gsw1xx_port_set_pause(priv, port, tx_pause, rx_pause);
+	}
+}
+
+static void gsw1xx_get_strings(struct dsa_switch *ds, int port, u32 stringset,
+			       uint8_t *data)
+{
+	int i;
+
+	if (stringset != ETH_SS_STATS)
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(gsw1xx_rmon_cnt); i++)
+		strncpy(data + i * ETH_GSTRING_LEN, gsw1xx_rmon_cnt[i].name,
+			ETH_GSTRING_LEN);
+}
+
+static u32 gsw1xx_bcm_ram_entry_read(struct gsw1xx_priv *priv, u32 port,
+				     u32 index)
+{
+	u32 result;
+	int err;
+
+	gsw1xx_switch_w(priv, index, GSW1XX_IP_BM_RAM_ADDR);
+	gsw1xx_switch_mask(priv,
+			   GSW1XX_IP_BM_RAM_CTRL_ADDR_MASK | GSW1XX_IP_BM_RAM_CTRL_OPMOD,
+			   port | GSW1XX_IP_BM_RAM_CTRL_BAS,
+			   GSW1XX_IP_BM_RAM_CTRL);
+
+	err = gsw1xx_switch_r_timeout(priv, GSW1XX_IP_BM_RAM_CTRL,
+				      GSW1XX_IP_BM_RAM_CTRL_BAS);
+	if (err) {
+		dev_err(priv->dev,
+			"timeout while reading entry: %u from RMON table for port: %u",
+			index, port);
+		return 0;
+	}
+
+	result = gsw1xx_switch_r(priv, GSW1XX_IP_BM_RAM_VAL(0));
+	result |= gsw1xx_switch_r(priv, GSW1XX_IP_BM_RAM_VAL(1)) << 16;
+
+	return result;
+}
+
+static void gsw1xx_get_ethtool_stats(struct dsa_switch *ds, int port,
+				     uint64_t *data)
+{
+	struct gsw1xx_priv *priv = ds->priv;
+	const struct gsw1xx_rmon_cnt_desc *rmon_cnt;
+	int i;
+	u64 high;
+
+	for (i = 0; i < ARRAY_SIZE(gsw1xx_rmon_cnt); i++) {
+		rmon_cnt = &gsw1xx_rmon_cnt[i];
+
+		data[i] =
+			gsw1xx_bcm_ram_entry_read(priv, port, rmon_cnt->offset);
+		if (rmon_cnt->size == 2) {
+			high = gsw1xx_bcm_ram_entry_read(priv, port,
+							 rmon_cnt->offset + 1);
+			data[i] |= high << 32;
+		}
+	}
+}
+
+static int gsw1xx_get_sset_count(struct dsa_switch *ds, int port, int sset)
+{
+	if (sset != ETH_SS_STATS)
+		return 0;
+
+	return ARRAY_SIZE(gsw1xx_rmon_cnt);
+}
+
+static int gsw1xx_get_mac_eee(struct dsa_switch *ds, int port,
+			      struct ethtool_eee *e)
+{
+	struct gsw1xx_priv *priv = (struct gsw1xx_priv *)ds->priv;
+	u32 val = 0;
+
+	val = gsw1xx_switch_r(priv, GSW1XX_IP_MAC_CTRL_4p(port));
+	e->tx_lpi_enabled = !!(val & GSW1XX_IP_MAC_CTRL_4_LPIEN);
+
+	e->tx_lpi_timer = 20;
+
+	return 0;
+}
+
+static int gsw1xx_set_mac_eee(struct dsa_switch *ds, int port,
+			      struct ethtool_eee *e)
+{
+	struct gsw1xx_priv *priv = (struct gsw1xx_priv *)ds->priv;
+
+	if (e->tx_lpi_enabled) {
+		gsw1xx_switch_mask(priv, 0, GSW1XX_IP_MAC_CTRL_4_LPIEN,
+				   GSW1XX_IP_MAC_CTRL_4p(port));
+	} else {
+		gsw1xx_switch_mask(priv, GSW1XX_IP_MAC_CTRL_4_LPIEN, 0,
+				   GSW1XX_IP_MAC_CTRL_4p(port));
+	}
+
+	return 0;
+}
+
+static const struct dsa_switch_ops gsw1xx_switch_ops = {
+	.get_tag_protocol	= gsw1xx_get_tag_protocol,
+	.setup			= gsw1xx_setup,
+	.set_mac_eee		= gsw1xx_set_mac_eee,
+	.get_mac_eee		= gsw1xx_get_mac_eee,
+	.port_enable		= gsw1xx_port_enable,
+	.port_disable		= gsw1xx_port_disable,
+	.port_stp_state_set	= gsw1xx_port_stp_state_set,
+	.phylink_mac_link_down	= gsw1xx_phylink_mac_link_down,
+	.phylink_mac_link_up	= gsw1xx_phylink_mac_link_up,
+	.get_strings		= gsw1xx_get_strings,
+	.get_ethtool_stats	= gsw1xx_get_ethtool_stats,
+	.get_sset_count		= gsw1xx_get_sset_count,
+};
+
+int gsw1xx_probe(struct gsw1xx_priv *priv, struct device *dev)
+{
+	struct device_node *np, *mdio_np;
+	int err;
+	u32 version;
+
+	if (!priv->regmap || IS_ERR(priv->regmap))
+		return -EINVAL;
+
+	priv->hw_info = of_device_get_match_data(dev);
+	if (!priv->hw_info)
+		return -EINVAL;
+
+	priv->ds = devm_kzalloc(dev, sizeof(*priv->ds), GFP_KERNEL);
+	if (!priv->ds)
+		return -ENOMEM;
+
+	priv->ds->dev = dev;
+	priv->ds->num_ports = priv->hw_info->max_ports;
+	priv->ds->priv = priv;
+	priv->ds->ops = &gsw1xx_switch_ops;
+	priv->dev = dev;
+	version = gsw1xx_switch_r(priv, GSW1XX_IP_VERSION);
+
+	np = dev->of_node;
+	switch (version) {
+	case GSW1XX_IP_VERSION_2_3:
+		if (!of_device_is_compatible(np, "mxl,gsw145-mdio"))
+			return -EINVAL;
+		break;
+	default:
+		dev_err(dev, "unknown GSW1XX_IP version: 0x%x", version);
+		return -ENOENT;
+	}
+
+	/* bring up the mdio bus */
+	mdio_np = of_get_child_by_name(np, "mdio");
+	if (!mdio_np) {
+		dev_err(dev, "missing child mdio node\n");
+		return -EINVAL;
+	}
+
+	err = gsw1xx_mdio(priv, mdio_np);
+	if (err) {
+		dev_err(dev, "mdio probe failed\n");
+		goto put_mdio_node;
+	}
+
+	err = dsa_register_switch(priv->ds);
+	if (err) {
+		dev_err(dev, "dsa switch register failed: %i\n", err);
+		goto mdio_bus;
+	}
+	if (!dsa_is_cpu_port(priv->ds, priv->hw_info->cpu_port)) {
+		dev_err(dev,
+			"wrong CPU port defined, HW only supports port: %i",
+			priv->hw_info->cpu_port);
+		err = -EINVAL;
+		goto disable_switch;
+	}
+
+	dev_set_drvdata(dev, priv);
+
+	return 0;
+
+disable_switch:
+	gsw1xx_mdio_mask(priv, GSW1XX_MDIO_GLOB_ENABLE, 0, GSW1XX_MDIO_GLOB);
+	dsa_unregister_switch(priv->ds);
+mdio_bus:
+	if (mdio_np) {
+		mdiobus_unregister(priv->ds->slave_mii_bus);
+		mdiobus_free(priv->ds->slave_mii_bus);
+	}
+put_mdio_node:
+	of_node_put(mdio_np);
+	return err;
+}
+EXPORT_SYMBOL(gsw1xx_probe);
+
+void gsw1xx_remove(struct gsw1xx_priv *priv)
+{
+	if (!priv)
+		return;
+
+	/* disable the switch */
+	gsw1xx_mdio_mask(priv, GSW1XX_MDIO_GLOB_ENABLE, 0, GSW1XX_MDIO_GLOB);
+
+	dsa_unregister_switch(priv->ds);
+
+	if (priv->ds->slave_mii_bus) {
+		mdiobus_unregister(priv->ds->slave_mii_bus);
+		of_node_put(priv->ds->slave_mii_bus->dev.of_node);
+		mdiobus_free(priv->ds->slave_mii_bus);
+	}
+
+	dev_set_drvdata(priv->dev, NULL);
+}
+EXPORT_SYMBOL(gsw1xx_remove);
+
+void gsw1xx_shutdown(struct gsw1xx_priv *priv)
+{
+	if (!priv)
+		return;
+
+	/* disable the switch */
+	gsw1xx_mdio_mask(priv, GSW1XX_MDIO_GLOB_ENABLE, 0, GSW1XX_MDIO_GLOB);
+
+	dsa_switch_shutdown(priv->ds);
+
+	dev_set_drvdata(priv->dev, NULL);
+}
+EXPORT_SYMBOL(gsw1xx_shutdown);
+
+static const struct regmap_range gsw1xx_valid_regs[] = {
+	/* GSWIP Core Registers */
+	regmap_reg_range(GSW1XX_IP_BASE_ADDR,
+			 GSW1XX_IP_BASE_ADDR + GSW1XX_IP_REG_LEN),
+	/* Top Level PDI Registers, MDIO Master Reigsters */
+	regmap_reg_range(GSW1XX_MDIO_BASE_ADDR,
+			 GSW1XX_MDIO_BASE_ADDR + GSW1XX_MDIO_REG_LEN),
+};
+
+const struct regmap_access_table gsw1xx_register_set = {
+	.yes_ranges = gsw1xx_valid_regs,
+	.n_yes_ranges = ARRAY_SIZE(gsw1xx_valid_regs),
+};
+EXPORT_SYMBOL(gsw1xx_register_set);
+
+MODULE_AUTHOR("Camel Guo <camel.guo@axis.com>");
+MODULE_DESCRIPTION("Core Driver for MaxLinear GSM1XX ethernet switch");
+MODULE_LICENSE("GPL");
diff --git a/drivers/net/dsa/gsw1xx_mdio.c b/drivers/net/dsa/gsw1xx_mdio.c
new file mode 100644
index 000000000000..8328001041ed
--- /dev/null
+++ b/drivers/net/dsa/gsw1xx_mdio.c
@@ -0,0 +1,128 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * MaxLinear switch driver for GSW1XX in MDIO managed mode
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mdio.h>
+#include <linux/phy.h>
+#include <linux/of.h>
+
+#include "gsw1xx.h"
+
+#define GSW1XX_SMDIO_TARGET_BASE_ADDR_REG	0x1F
+
+static int gsw1xx_mdio_write(void *ctx, uint32_t reg, uint32_t val)
+{
+	struct mdio_device *mdiodev = (struct mdio_device *)ctx;
+	int ret = 0;
+
+	mutex_lock_nested(&mdiodev->bus->mdio_lock, MDIO_MUTEX_NESTED);
+
+	ret = mdiodev->bus->write(mdiodev->bus, mdiodev->addr,
+				  GSW1XX_SMDIO_TARGET_BASE_ADDR_REG, reg);
+	if (ret < 0)
+		goto out;
+
+	ret = mdiodev->bus->write(mdiodev->bus, mdiodev->addr, 0, val);
+
+out:
+	mutex_unlock(&mdiodev->bus->mdio_lock);
+
+	return ret;
+}
+
+static int gsw1xx_mdio_read(void *ctx, uint32_t reg, uint32_t *val)
+{
+	struct mdio_device *mdiodev = (struct mdio_device *)ctx;
+	int ret = 0;
+
+	mutex_lock_nested(&mdiodev->bus->mdio_lock, MDIO_MUTEX_NESTED);
+
+	ret = mdiodev->bus->write(mdiodev->bus, mdiodev->addr,
+				  GSW1XX_SMDIO_TARGET_BASE_ADDR_REG, reg);
+	if (ret < 0)
+		goto out;
+
+	*val = mdiodev->bus->read(mdiodev->bus, mdiodev->addr, 0);
+
+out:
+	mutex_unlock(&mdiodev->bus->mdio_lock);
+
+	return ret;
+}
+
+static const struct regmap_config gsw1xx_mdio_regmap_config = {
+	.reg_bits = 16,
+	.val_bits = 16,
+	.reg_stride = 1,
+
+	.disable_locking = true,
+
+	.volatile_table = &gsw1xx_register_set,
+	.wr_table = &gsw1xx_register_set,
+	.rd_table = &gsw1xx_register_set,
+
+	.reg_read = gsw1xx_mdio_read,
+	.reg_write = gsw1xx_mdio_write,
+
+	.cache_type = REGCACHE_NONE,
+};
+
+static int gsw1xx_mdio_probe(struct mdio_device *mdiodev)
+{
+	struct gsw1xx_priv *priv;
+	struct device *dev = &mdiodev->dev;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->regmap = devm_regmap_init(dev, NULL, mdiodev,
+					&gsw1xx_mdio_regmap_config);
+	if (IS_ERR(priv->regmap)) {
+		ret = PTR_ERR(priv->regmap);
+		dev_err(dev, "regmap init failed: %d\n", ret);
+		return ret;
+	}
+
+	return gsw1xx_probe(priv, dev);
+}
+
+static void gsw1xx_mdio_remove(struct mdio_device *mdiodev)
+{
+	gsw1xx_remove(dev_get_drvdata(&mdiodev->dev));
+}
+
+static void gsw1xx_mdio_shutdown(struct mdio_device *mdiodev)
+{
+	gsw1xx_shutdown(dev_get_drvdata(&mdiodev->dev));
+}
+
+static const struct gsw1xx_hw_info gsw145_hw_info = {
+	.max_ports = 6,
+	.cpu_port = 5,
+};
+
+static const struct of_device_id gsw1xx_mdio_of_match[] = {
+	{ .compatible = "mxl,gsw145-mdio", .data = &gsw145_hw_info },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, gsw1xx_mdio_of_match);
+
+static struct mdio_driver gsw1xx_mdio_driver = {
+	.probe  = gsw1xx_mdio_probe,
+	.remove = gsw1xx_mdio_remove,
+	.shutdown = gsw1xx_mdio_shutdown,
+	.mdiodrv.driver = {
+		.name = "GSW1XX_MDIO",
+		.of_match_table = of_match_ptr(gsw1xx_mdio_of_match),
+	},
+};
+
+mdio_module_driver(gsw1xx_mdio_driver);
+
+MODULE_AUTHOR("Camel Guo <camel.guo@axis.com>");
+MODULE_DESCRIPTION("Driver for MaxLinear GSM1XX ethernet switch in MDIO managed mode");
+MODULE_LICENSE("GPL");
-- 
2.30.2

Re: [RFC net-next 2/2] net: dsa: Add driver for Maxlinear GSW1XX switch

[Krzysztof Kozlowski]

On 25/10/2022 09:52, Camel Guo wrote:
> Add initial framework for Maxlinear's GSW1xx switch and
> currently only GSW145 in MDIO managed mode is supported.
> 
> Signed-off-by: Camel Guo <camel.guo@axis.com>
> ---

(...)

> +	priv->ds->dev = dev;
> +	priv->ds->num_ports = priv->hw_info->max_ports;
> +	priv->ds->priv = priv;
> +	priv->ds->ops = &gsw1xx_switch_ops;
> +	priv->dev = dev;
> +	version = gsw1xx_switch_r(priv, GSW1XX_IP_VERSION);
> +
> +	np = dev->of_node;
> +	switch (version) {
> +	case GSW1XX_IP_VERSION_2_3:
> +		if (!of_device_is_compatible(np, "mxl,gsw145-mdio"))
> +			return -EINVAL;
> +		break;
> +	default:
> +		dev_err(dev, "unknown GSW1XX_IP version: 0x%x", version);
> +		return -ENOENT;
> +	}
> +
> +	/* bring up the mdio bus */
> +	mdio_np = of_get_child_by_name(np, "mdio");
> +	if (!mdio_np) {
> +		dev_err(dev, "missing child mdio node\n");
> +		return -EINVAL;
> +	}
> +
> +	err = gsw1xx_mdio(priv, mdio_np);
> +	if (err) {
> +		dev_err(dev, "mdio probe failed\n");

dev_err_probe()

> +		goto put_mdio_node;
> +	}
> +
> +	err = dsa_register_switch(priv->ds);
> +	if (err) {
> +		dev_err(dev, "dsa switch register failed: %i\n", err);


dev_err_probe()

> +		goto mdio_bus;
> +	}
> +	if (!dsa_is_cpu_port(priv->ds, priv->hw_info->cpu_port)) {
> +		dev_err(dev,
> +			"wrong CPU port defined, HW only supports port: %i",
> +			priv->hw_info->cpu_port);
> +		err = -EINVAL;
> +		goto disable_switch;
> +	}
> +
> +	dev_set_drvdata(dev, priv);
> +
> +	return 0;
> +
> +disable_switch:
> +	gsw1xx_mdio_mask(priv, GSW1XX_MDIO_GLOB_ENABLE, 0, GSW1XX_MDIO_GLOB);
> +	dsa_unregister_switch(priv->ds);
> +mdio_bus:
> +	if (mdio_np) {
> +		mdiobus_unregister(priv->ds->slave_mii_bus);
> +		mdiobus_free(priv->ds->slave_mii_bus);
> +	}
> +put_mdio_node:
> +	of_node_put(mdio_np);
> +	return err;
> +}
> +EXPORT_SYMBOL(gsw1xx_probe);
> +
> +void gsw1xx_remove(struct gsw1xx_priv *priv)
> +{
> +	if (!priv)
> +		return;
> +
> +	/* disable the switch */
> +	gsw1xx_mdio_mask(priv, GSW1XX_MDIO_GLOB_ENABLE, 0, GSW1XX_MDIO_GLOB);
> +
> +	dsa_unregister_switch(priv->ds);
> +
> +	if (priv->ds->slave_mii_bus) {
> +		mdiobus_unregister(priv->ds->slave_mii_bus);
> +		of_node_put(priv->ds->slave_mii_bus->dev.of_node);
> +		mdiobus_free(priv->ds->slave_mii_bus);
> +	}
> +
> +	dev_set_drvdata(priv->dev, NULL);
> +}
> +EXPORT_SYMBOL(gsw1xx_remove);
> +
> +void gsw1xx_shutdown(struct gsw1xx_priv *priv)
> +{
> +	if (!priv)
> +		return;
> +
> +	/* disable the switch */
> +	gsw1xx_mdio_mask(priv, GSW1XX_MDIO_GLOB_ENABLE, 0, GSW1XX_MDIO_GLOB);
> +
> +	dsa_switch_shutdown(priv->ds);
> +
> +	dev_set_drvdata(priv->dev, NULL);
> +}
> +EXPORT_SYMBOL(gsw1xx_shutdown);

1. EXPORT_SYMBOL_GPL
2. Why do you do it in the first place? It's one driver, no need for
building two modules. Same applies to other places.

> +
> +static const struct regmap_range gsw1xx_valid_regs[] = {
> +	/* GSWIP Core Registers */
> +	regmap_reg_range(GSW1XX_IP_BASE_ADDR,
> +			 GSW1XX_IP_BASE_ADDR + GSW1XX_IP_REG_LEN),
> +	/* Top Level PDI Registers, MDIO Master Reigsters */
> +	regmap_reg_range(GSW1XX_MDIO_BASE_ADDR,
> +			 GSW1XX_MDIO_BASE_ADDR + GSW1XX_MDIO_REG_LEN),
> +};
> +
> +const struct regmap_access_table gsw1xx_register_set = {
> +	.yes_ranges = gsw1xx_valid_regs,
> +	.n_yes_ranges = ARRAY_SIZE(gsw1xx_valid_regs),
> +};
> +EXPORT_SYMBOL(gsw1xx_register_set);
> +
> +MODULE_AUTHOR("Camel Guo <camel.guo@axis.com>");
> +MODULE_DESCRIPTION("Core Driver for MaxLinear GSM1XX ethernet switch");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/net/dsa/gsw1xx_mdio.c b/drivers/net/dsa/gsw1xx_mdio.c
> new file mode 100644
> index 000000000000..8328001041ed
> --- /dev/null
> +++ b/drivers/net/dsa/gsw1xx_mdio.c
> @@ -0,0 +1,128 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * MaxLinear switch driver for GSW1XX in MDIO managed mode
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mdio.h>
> +#include <linux/phy.h>
> +#include <linux/of.h>
> +
> +#include "gsw1xx.h"
> +
> +#define GSW1XX_SMDIO_TARGET_BASE_ADDR_REG	0x1F
> +
> +static int gsw1xx_mdio_write(void *ctx, uint32_t reg, uint32_t val)
> +{
> +	struct mdio_device *mdiodev = (struct mdio_device *)ctx;
> +	int ret = 0;
> +
> +	mutex_lock_nested(&mdiodev->bus->mdio_lock, MDIO_MUTEX_NESTED);
> +
> +	ret = mdiodev->bus->write(mdiodev->bus, mdiodev->addr,
> +				  GSW1XX_SMDIO_TARGET_BASE_ADDR_REG, reg);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = mdiodev->bus->write(mdiodev->bus, mdiodev->addr, 0, val);
> +
> +out:
> +	mutex_unlock(&mdiodev->bus->mdio_lock);
> +
> +	return ret;
> +}
> +
> +static int gsw1xx_mdio_read(void *ctx, uint32_t reg, uint32_t *val)
> +{
> +	struct mdio_device *mdiodev = (struct mdio_device *)ctx;
> +	int ret = 0;
> +
> +	mutex_lock_nested(&mdiodev->bus->mdio_lock, MDIO_MUTEX_NESTED);
> +
> +	ret = mdiodev->bus->write(mdiodev->bus, mdiodev->addr,
> +				  GSW1XX_SMDIO_TARGET_BASE_ADDR_REG, reg);
> +	if (ret < 0)
> +		goto out;
> +
> +	*val = mdiodev->bus->read(mdiodev->bus, mdiodev->addr, 0);
> +
> +out:
> +	mutex_unlock(&mdiodev->bus->mdio_lock);
> +
> +	return ret;
> +}
> +
> +static const struct regmap_config gsw1xx_mdio_regmap_config = {
> +	.reg_bits = 16,
> +	.val_bits = 16,
> +	.reg_stride = 1,
> +
> +	.disable_locking = true,
> +
> +	.volatile_table = &gsw1xx_register_set,
> +	.wr_table = &gsw1xx_register_set,
> +	.rd_table = &gsw1xx_register_set,
> +
> +	.reg_read = gsw1xx_mdio_read,
> +	.reg_write = gsw1xx_mdio_write,
> +
> +	.cache_type = REGCACHE_NONE,
> +};
> +
> +static int gsw1xx_mdio_probe(struct mdio_device *mdiodev)
> +{
> +	struct gsw1xx_priv *priv;
> +	struct device *dev = &mdiodev->dev;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->regmap = devm_regmap_init(dev, NULL, mdiodev,
> +					&gsw1xx_mdio_regmap_config);
> +	if (IS_ERR(priv->regmap)) {
> +		ret = PTR_ERR(priv->regmap);
> +		dev_err(dev, "regmap init failed: %d\n", ret);
> +		return ret;

return dev_err_probe().

> +	}
> +
> +	return gsw1xx_probe(priv, dev);
> +}
> +
> +static void gsw1xx_mdio_remove(struct mdio_device *mdiodev)
> +{
> +	gsw1xx_remove(dev_get_drvdata(&mdiodev->dev));
> +}
> +
> +static void gsw1xx_mdio_shutdown(struct mdio_device *mdiodev)
> +{
> +	gsw1xx_shutdown(dev_get_drvdata(&mdiodev->dev));
> +}
> +
> +static const struct gsw1xx_hw_info gsw145_hw_info = {
> +	.max_ports = 6,
> +	.cpu_port = 5,
> +};
> +
> +static const struct of_device_id gsw1xx_mdio_of_match[] = {
> +	{ .compatible = "mxl,gsw145-mdio", .data = &gsw145_hw_info },
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, gsw1xx_mdio_of_match);
> +
> +static struct mdio_driver gsw1xx_mdio_driver = {
> +	.probe  = gsw1xx_mdio_probe,
> +	.remove = gsw1xx_mdio_remove,
> +	.shutdown = gsw1xx_mdio_shutdown,
> +	.mdiodrv.driver = {
> +		.name = "GSW1XX_MDIO",
> +		.of_match_table = of_match_ptr(gsw1xx_mdio_of_match),

of_match_ptr requires maybe_unused. Or just drop it.

Best regards,
Krzysztof

Re: [RFC net-next 2/2] net: dsa: Add driver for Maxlinear GSW1XX switch

[Andrew Lunn]

> > +EXPORT_SYMBOL(gsw1xx_shutdown);
> 
> 1. EXPORT_SYMBOL_GPL
> 2. Why do you do it in the first place? It's one driver, no need for
> building two modules. Same applies to other places.

At some point, there is likely to be SPI and UART support. The
communication with the chip and the core driver will then be in
separate modules. But i agree this is not needed at the moment when it
is all linked into one.

   Andrew

Re: [RFC net-next 2/2] net: dsa: Add driver for Maxlinear GSW1XX switch

[Camel Guo]

On 10/25/22 16:56, Andrew Lunn wrote:
>> > +EXPORT_SYMBOL(gsw1xx_shutdown);
>> 
>> 1. EXPORT_SYMBOL_GPL
>> 2. Why do you do it in the first place? It's one driver, no need for
>> building two modules. Same applies to other places.
> 
> At some point, there is likely to be SPI and UART support. The
> communication with the chip and the core driver will then be in
> separate modules. But i agree this is not needed at the moment when it
> is all linked into one.

Do you suggest that currently we put the content of gsw1xx_core.c and 
gsw1xx_mdio.c into one file and split them later at the time when 
another management mode (e,g: spi) is added?

Actually I kinda hope this piece of code (gsw1xx_core.c) can be reused 
in lantiq_gswip in short future.

I tried to use the logic in lantiq_gswip directly on the gsw145 chip. 
Unfortunately it did not work. It seems that the GSWIP part changes a lot.

> 
>     Andrew

Re: [RFC net-next 2/2] net: dsa: Add driver for Maxlinear GSW1XX switch

[Andrew Lunn]

On Thu, Oct 27, 2022 at 08:35:17AM +0200, Camel Guo wrote:
> On 10/25/22 16:56, Andrew Lunn wrote:
> > > > +EXPORT_SYMBOL(gsw1xx_shutdown);
> > > 
> > > 1. EXPORT_SYMBOL_GPL
> > > 2. Why do you do it in the first place? It's one driver, no need for
> > > building two modules. Same applies to other places.
> > 
> > At some point, there is likely to be SPI and UART support. The
> > communication with the chip and the core driver will then be in
> > separate modules. But i agree this is not needed at the moment when it
> > is all linked into one.
> 
> Do you suggest that currently we put the content of gsw1xx_core.c and
> gsw1xx_mdio.c into one file and split them later at the time when another
> management mode (e,g: spi) is added?

No, keep them separate. But you can remove the module exports, and
just link them together at compile time into one module. For forward
compatibility, call that module gsw1xx_mdio.ko. In the future,
somebody can then split it apart again, add gsw1xx_spi.ko, and module
dependencies should cause the gsw1xx_core.ko to be loaded first.

     Andrew

Re: [RFC net-next 2/2] net: dsa: Add driver for Maxlinear GSW1XX switch

[Krzysztof Kozlowski]

On 27/10/2022 02:35, Camel Guo wrote:
utdown(priv->ds);
>  >> +
>  >> +     dev_set_drvdata(priv->dev, NULL);
>  >> +}
>  >> +EXPORT_SYMBOL(gsw1xx_shutdown);
>  >
>  > 1. EXPORT_SYMBOL_GPL
> 
> Will update in v2
> 
>  > 2. Why do you do it in the first place? It's one driver, no need for
>  > building two modules. Same applies to other places.
> 
> All stuff in drivers/net/dsa/gsw1xx_core.c is supposed to be generic and
> totally independent of the actual management interface (mdio, spi, uart,
> maybe memory-mapped IO). This way, I think the gsw1xx_core.ko can be 
> reused in
> gsw1xx_spi.ko, gsw1xx_uart.ko and so on.
> 
> I don't how similar the chips that lantiq_gswip.c supports are due to
> no datasheet. If not too much, maybe someone the gsw1xx_core.ko can also
> be reused in lantiq_gswip as well.

Keep the files separate but there is no need to make two modules and
exprt this. Your patch should stand on its own, not prepare for some
imaginary future work which might or might not bring more modules.

Once these future modules appear, it will be easy to change existing
file to a module.

Best regards,
Krzysztof

Re: [RFC net-next 2/2] net: dsa: Add driver for Maxlinear GSW1XX switch

[Andrew Lunn]

> +++ b/drivers/net/dsa/Kconfig
> @@ -122,4 +122,20 @@ config NET_DSA_VITESSE_VSC73XX_PLATFORM
>  	  This enables support for the Vitesse VSC7385, VSC7388, VSC7395
>  	  and VSC7398 SparX integrated ethernet switches, connected over
>  	  a CPU-attached address bus and work in memory-mapped I/O mode.
> +
> +config NET_DSA_MXL_GSW1XX
> +	tristate
> +	select REGMAP
> +	help
> +	  This enables support for the Maxlinear GSW1XX integrated ethernet
> +	  switch chips.
> +
> +config NET_DSA_MXL_GSW1XX_MDIO
> +	tristate "MaxLinear GSW1XX ethernet switch in MDIO managed mode"

Please keep this file sorted on the tristate text.

In general, it is wrong to insert at the end. That causes the most
conflicts. By keeping lists like this sorted, inserts tend to be
separated, and so don't cause conflicts. Also, keeping it sorted helps
users actually find the configuration option they want.

> +	select NET_DSA_MXL_GSW1XX
> +	select FIXED_PHY
> +	help
> +	  This enables access functions if the MaxLinear GSW1XX is configured
> +	  for MDIO managed mode.
> +
>  endmenu
> diff --git a/drivers/net/dsa/Makefile b/drivers/net/dsa/Makefile
> index 16eb879e0cb4..022fc661107b 100644
> --- a/drivers/net/dsa/Makefile
> +++ b/drivers/net/dsa/Makefile
> @@ -15,6 +15,8 @@ obj-$(CONFIG_NET_DSA_SMSC_LAN9303_MDIO) += lan9303_mdio.o
>  obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX) += vitesse-vsc73xx-core.o
>  obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX_PLATFORM) += vitesse-vsc73xx-platform.o
>  obj-$(CONFIG_NET_DSA_VITESSE_VSC73XX_SPI) += vitesse-vsc73xx-spi.o
> +obj-$(CONFIG_NET_DSA_MXL_GSW1XX) += gsw1xx_core.o
> +obj-$(CONFIG_NET_DSA_MXL_GSW1XX_MDIO) += gsw1xx_mdio.o

This file is sorted as well.

> diff --git a/drivers/net/dsa/gsw1xx.h b/drivers/net/dsa/gsw1xx.h

If you think there is going to be a gsw1xx_spi.c and gsw1xx_uart.c, i
would suggest you move into a subdirectory.

> +static u32 gsw1xx_switch_r(struct gsw1xx_priv *priv, u32 offset)
> +{
> +	int ret = 0;
> +	u32 val = 0;
> +
> +	ret = regmap_read(priv->regmap, GSW1XX_IP_BASE_ADDR + offset, &val);
> +
> +	return ret < 0 ? (u32)ret : val;

A negative error code becomes positive? So how do you then know it is
an error code?

The general pattern is you pass the error code and the register value
in two separate ways. Generally, the error code as the return value,
as an int, and the register value via a pointer. Just as regmap_read()
does.

> +}
> +
> +static void gsw1xx_switch_w(struct gsw1xx_priv *priv, u32 val, u32 offset)
> +{
> +	regmap_write(priv->regmap, GSW1XX_IP_BASE_ADDR + offset, val);
> +}

Return the error code from regmap_write().

In general, don't ignore errors. Return them up the call stack.

> +static u32 gsw1xx_switch_r_timeout(struct gsw1xx_priv *priv, u32 offset,
> +				   u32 cleared)
> +{
> +	u32 val;
> +
> +	return read_poll_timeout(gsw1xx_switch_r, val, (val & cleared) == 0, 20,
> +				 50000, true, priv, offset);
> +}
> +
> +static int gsw1xx_mdio_poll(struct gsw1xx_priv *priv)
> +{
> +	int cnt = 100;
> +
> +	while (likely(cnt--)) {
> +		u32 ctrl = gsw1xx_mdio_r(priv, GSW1XX_MDIO_CTRL);
> +
> +		if ((ctrl & GSW1XX_MDIO_CTRL_BUSY) == 0)
> +			return 0;
> +		usleep_range(20, 40);
> +	}
> +
> +	return -ETIMEDOUT;

It looks like this could be implemented using read_poll_timeout() as
well?

> +}
> +
> +static int gsw1xx_mdio_wr(struct mii_bus *bus, int addr, int reg, u16 val)
> +{
> +	struct gsw1xx_priv *priv = bus->priv;
> +	int err;

Please check for C45 and return -EOPNOTSUPP.

> +
> +	err = gsw1xx_mdio_poll(priv);
> +	if (err) {
> +		dev_err(&bus->dev, "timeout while waiting for MDIO bus\n");
> +		return err;
> +	}
> +
> +	gsw1xx_mdio_w(priv, val, GSW1XX_MDIO_WRITE);
> +	gsw1xx_mdio_w(priv,
> +		      GSW1XX_MDIO_CTRL_WR |
> +			      ((addr & GSW1XX_MDIO_CTRL_PHYAD_MASK)
> +			       << GSW1XX_MDIO_CTRL_PHYAD_SHIFT) |
> +			      (reg & GSW1XX_MDIO_CTRL_REGAD_MASK),
> +		      GSW1XX_MDIO_CTRL);
> +
> +	return 0;
> +}
> +
> +static int gsw1xx_mdio_rd(struct mii_bus *bus, int addr, int reg)
> +{
> +	struct gsw1xx_priv *priv = bus->priv;
> +	int err;

Same here.

> +static int gsw1xx_port_enable(struct dsa_switch *ds, int port,
> +			      struct phy_device *phydev)
> +{
> +	struct gsw1xx_priv *priv = ds->priv;
> +
> +	if (!dsa_is_user_port(ds, port))
> +		return 0;
> +
> +	/* RMON Counter Enable for port */
> +	gsw1xx_switch_w(priv, GSW1XX_IP_BM_PCFG_CNTEN,
> +			GSW1XX_IP_BM_PCFGp(port));
> +
> +	/* enable port fetch/store dma */
> +	gsw1xx_switch_mask(priv, 0, GSW1XX_IP_FDMA_PCTRL_EN,
> +			   GSW1XX_IP_FDMA_PCTRLp(port));
> +	gsw1xx_switch_mask(priv, 0, GSW1XX_IP_SDMA_PCTRL_EN,
> +			   GSW1XX_IP_SDMA_PCTRLp(port));
> +
> +	if (!dsa_is_cpu_port(ds, port)) {

How can this be true given the previous check for dsa_is_user_port()?


> +static int gsw1xx_setup(struct dsa_switch *ds)
> +{
> +	struct gsw1xx_priv *priv = ds->priv;
> +	unsigned int cpu_port = priv->hw_info->cpu_port;
> +	int i;
> +	int err;

Reverse christmass tree, which means you need to delay assigning
cpu_port into the body of the function.

> +int gsw1xx_probe(struct gsw1xx_priv *priv, struct device *dev)
> +{
> +	struct device_node *np, *mdio_np;
> +	int err;
> +	u32 version;

Reverse christmass tree.

> +
> +	if (!priv->regmap || IS_ERR(priv->regmap))
> +		return -EINVAL;
> +
> +	priv->hw_info = of_device_get_match_data(dev);
> +	if (!priv->hw_info)
> +		return -EINVAL;
> +
> +	priv->ds = devm_kzalloc(dev, sizeof(*priv->ds), GFP_KERNEL);
> +	if (!priv->ds)
> +		return -ENOMEM;
> +
> +	priv->ds->dev = dev;
> +	priv->ds->num_ports = priv->hw_info->max_ports;
> +	priv->ds->priv = priv;
> +	priv->ds->ops = &gsw1xx_switch_ops;
> +	priv->dev = dev;
> +	version = gsw1xx_switch_r(priv, GSW1XX_IP_VERSION);
> +
> +	np = dev->of_node;
> +	switch (version) {
> +	case GSW1XX_IP_VERSION_2_3:
> +		if (!of_device_is_compatible(np, "mxl,gsw145-mdio"))
> +			return -EINVAL;
> +		break;
> +	default:
> +		dev_err(dev, "unknown GSW1XX_IP version: 0x%x", version);
> +		return -ENOENT;

I think ENODEV is more appropriate.

I noticed there is no tagging protocol defined. How are frames
direction out a specific port?

I've also not yet looked at the overlap with lantiq_gswip.c.

     Andrew

Re: [RFC net-next 2/2] net: dsa: Add driver for Maxlinear GSW1XX switch

[Andrew Lunn]

> >> +}
> >> +
> >> +static int gsw1xx_mdio_wr(struct mii_bus *bus, int addr, int reg, u16 val)
> >> +{
> >> +     struct gsw1xx_priv *priv = bus->priv;
> >> +     int err;
> >
> > Please check for C45 and return -EOPNOTSUPP.
> 
> Maybe not need. According to the datasheet of gsw145 "The interface uses the
> serial protocol defined by IEEE 802.3, clause 22.",  I think it is enough to
> add "ds->slave_mii_bus->probe_capabilities = MDIOBUS_C22" into gsw1xx_mdio. It
> probe_capabilities is static and never changes.

probe_capabilities only limits probing the bus when it is
registered. It does not prevent C45 transfers from being requested by
PHY drivers. And there are some funky PHY drivers which mix C22 and
C45. They can probe via C22 and then use C45. So it is much better to
check and return an error if requested to do something which the
hardware cannot do. Also, if you don't check, and convert a C45
request into a C22 request, you often end up with really odd accesses,
depending on the hardware, reads could become writes, etc.

> > I noticed there is no tagging protocol defined. How are frames
> > direction out a specific port?
> 
> Yes, this chip supports Special Tags which should be enabled, but unfortunately
> I have no make it work.

You need to make this work. You added support to set the port spanning
tree status. But that makes no sense if you cannot send/receive bridge
PDUs out specific ports, etc.

> The chip in my dev board works in self-start, managed switch mode. So far, it
> works fine on this board.
> 
> >
> > I've also not yet looked at the overlap with lantiq_gswip.c.
> 
> The version of GSWIP changes and also the management interface of
> it is memory-mapped io. I tried to use the same logic in my gsw145 chip
> (with mdio interface update), lots of parts (e.g: fdb, vlan) don't work
> at all.

There has been past attempts at a driver for this hardware and it was
argued that they are sufficiently different that a new driver was
needed. As i said, i've not compared the code yet, so i cannot comment
on that yet.

   Andrew

Re: [RFC net-next 2/2] net: dsa: Add driver for Maxlinear GSW1XX switch

[Vladimir Oltean]

Hi Camel,

I took a very superficial look. I'm only interested in the API
perspective for now. Some comments.

On Tue, Oct 25, 2022 at 03:52:41PM +0200, Camel Guo wrote:
> +static int gsw1xx_port_enable(struct dsa_switch *ds, int port,
> +			      struct phy_device *phydev)
> +{
> +	struct gsw1xx_priv *priv = ds->priv;
> +
> +	if (!dsa_is_user_port(ds, port))
> +		return 0;
> +
> +	/* RMON Counter Enable for port */
> +	gsw1xx_switch_w(priv, GSW1XX_IP_BM_PCFG_CNTEN,
> +			GSW1XX_IP_BM_PCFGp(port));
> +
> +	/* enable port fetch/store dma */
> +	gsw1xx_switch_mask(priv, 0, GSW1XX_IP_FDMA_PCTRL_EN,
> +			   GSW1XX_IP_FDMA_PCTRLp(port));
> +	gsw1xx_switch_mask(priv, 0, GSW1XX_IP_SDMA_PCTRL_EN,
> +			   GSW1XX_IP_SDMA_PCTRLp(port));
> +
> +	if (!dsa_is_cpu_port(ds, port)) {
> +		u32 mdio_phy = 0;
> +
> +		if (phydev)
> +			mdio_phy =
> +				phydev->mdio.addr & GSW1XX_MDIO_PHY_ADDR_MASK;
> +
> +		gsw1xx_mdio_mask(priv, GSW1XX_MDIO_PHY_ADDR_MASK, mdio_phy,
> +				 GSW1XX_MDIO_PHYp(port));
> +	}
> +
> +	return 0;
> +}
> +
> +static void gsw1xx_port_disable(struct dsa_switch *ds, int port)
> +{
> +	struct gsw1xx_priv *priv = ds->priv;
> +
> +	if (!dsa_is_user_port(ds, port))
> +		return;
> +
> +	gsw1xx_switch_mask(priv, GSW1XX_IP_FDMA_PCTRL_EN, 0,
> +			   GSW1XX_IP_FDMA_PCTRLp(port));
> +	gsw1xx_switch_mask(priv, GSW1XX_IP_SDMA_PCTRL_EN, 0,
> +			   GSW1XX_IP_SDMA_PCTRLp(port));
> +}
> +
> +static int gsw1xx_setup(struct dsa_switch *ds)
> +{
> +	struct gsw1xx_priv *priv = ds->priv;
> +	unsigned int cpu_port = priv->hw_info->cpu_port;
> +	int i;
> +	int err;
> +
> +	gsw1xx_switch_w(priv, GSW1XX_IP_SWRES_R0, GSW1XX_IP_SWRES);
> +	usleep_range(5000, 10000);
> +	gsw1xx_switch_w(priv, 0, GSW1XX_IP_SWRES);
> +
> +	/* disable port fetch/store dma on all ports */
> +	for (i = 0; i < priv->hw_info->max_ports; i++)
> +		gsw1xx_port_disable(ds, i);
> +
> +	/* enable Switch */
> +	gsw1xx_mdio_mask(priv, 0, GSW1XX_MDIO_GLOB_ENABLE, GSW1XX_MDIO_GLOB);
> +
> +	gsw1xx_switch_w(priv, 0x7F, GSW1XX_IP_PCE_PMAP2);
> +	gsw1xx_switch_w(priv, 0x7F, GSW1XX_IP_PCE_PMAP3);
> +
> +	/* Deactivate MDIO PHY auto polling since it affects mmd read/write.
> +	 */
> +	gsw1xx_mdio_w(priv, 0x0, GSW1XX_MDIO_MDC_CFG0);
> +
> +	gsw1xx_switch_mask(priv, 1, GSW1XX_IP_MAC_CTRL_2_MLEN,
> +			   GSW1XX_IP_MAC_CTRL_2p(cpu_port));
> +	gsw1xx_switch_mask(priv, 0, GSW1XX_IP_BM_QUEUE_GCTRL_GL_MOD,
> +			   GSW1XX_IP_BM_QUEUE_GCTRL);
> +
> +	/* Flush MAC Table */
> +	gsw1xx_switch_mask(priv, 0, GSW1XX_IP_PCE_GCTRL_0_MTFL,
> +			   GSW1XX_IP_PCE_GCTRL_0);
> +	err = gsw1xx_switch_r_timeout(priv, GSW1XX_IP_PCE_GCTRL_0,
> +				      GSW1XX_IP_PCE_GCTRL_0_MTFL);
> +	if (err) {
> +		dev_err(priv->dev, "MAC flushing didn't finish\n");
> +		return err;
> +	}
> +
> +	gsw1xx_port_enable(ds, cpu_port, NULL);

DSA automatically calls this on the CPU port. You have this code which
ignores the call:

	if (!dsa_is_user_port(ds, port)) // which the CPU port isn't
		return 0;

so why do it?!

> +
> +	return 0;
> +}
> +
> +static enum dsa_tag_protocol gsw1xx_get_tag_protocol(struct dsa_switch *ds,
> +						     int port,
> +						     enum dsa_tag_protocol mp)
> +{
> +	return DSA_TAG_PROTO_NONE;

Nope, this won't fly for new drivers, please write a protocol driver for
your switch, or use something based on tag_8021q.c if the switch doesn't
support something natively.

> +}
> +
> +static void gsw1xx_port_stp_state_set(struct dsa_switch *ds, int port, u8 state)
> +{
> +	struct gsw1xx_priv *priv = ds->priv;
> +	u32 stp_state;
> +
> +	switch (state) {
> +	case BR_STATE_DISABLED:
> +		gsw1xx_switch_mask(priv, GSW1XX_IP_SDMA_PCTRL_EN, 0,
> +				   GSW1XX_IP_SDMA_PCTRLp(port));
> +		return;
> +	case BR_STATE_BLOCKING:
> +	case BR_STATE_LISTENING:
> +		stp_state = GSW1XX_IP_PCE_PCTRL_0_PSTATE_LISTEN;
> +		break;
> +	case BR_STATE_LEARNING:
> +		stp_state = GSW1XX_IP_PCE_PCTRL_0_PSTATE_LEARNING;
> +		break;
> +	case BR_STATE_FORWARDING:
> +		stp_state = GSW1XX_IP_PCE_PCTRL_0_PSTATE_FORWARDING;
> +		break;
> +	default:
> +		dev_err(priv->dev, "invalid STP state: %d\n", state);
> +		return;
> +	}
> +
> +	gsw1xx_switch_mask(priv, 0, GSW1XX_IP_SDMA_PCTRL_EN,
> +			   GSW1XX_IP_SDMA_PCTRLp(port));
> +	gsw1xx_switch_mask(priv, GSW1XX_IP_PCE_PCTRL_0_PSTATE_MASK, stp_state,
> +			   GSW1XX_IP_PCE_PCTRL_0p(port));
> +}
> +
> +static const struct dsa_switch_ops gsw1xx_switch_ops = {
> +	.get_tag_protocol	= gsw1xx_get_tag_protocol,
> +	.setup			= gsw1xx_setup,
> +	.set_mac_eee		= gsw1xx_set_mac_eee,
> +	.get_mac_eee		= gsw1xx_get_mac_eee,
> +	.port_enable		= gsw1xx_port_enable,
> +	.port_disable		= gsw1xx_port_disable,
> +	.port_stp_state_set	= gsw1xx_port_stp_state_set,

No need to implement .port_stp_state_set() if .port_bridge_join() is
absent. First get the support for standalone port mode right (hint, need
to disable address learning and forwarding between ports for standalone mode).

Look inside tools/testing/selftests/drivers/net/dsa/, a bunch of tests
should pass even with software forwarding. First make sure that switch
ports can ping each other through a cable in loopback. Then there's
no_forwarding.sh, a must have. I think bridge_vlan_aware.sh and
bridge_vlan_unaware.sh should also pass.

As far as local_termination.sh, that also tests for some optional
optimizations. You can run it, and the goal should be to get "OK" for
all tests. "FAIL" is also ok, if the reason is "reception succeeded,
but should have failed". What is not ok is "FAIL" with the reason
"reception failed". Something like this would be ok:

    TEST: br0: Unicast IPv4 to primary MAC address                      [ OK ]
    TEST: br0: Unicast IPv4 to macvlan MAC address                      [ OK ]
    TEST: br0: Unicast IPv4 to unknown MAC address                      [FAIL]
            reception succeeded, but should have failed
    TEST: br0: Unicast IPv4 to unknown MAC address, promisc             [ OK ]
    TEST: br0: Unicast IPv4 to unknown MAC address, allmulti            [FAIL]
            reception succeeded, but should have failed
    TEST: br0: Multicast IPv4 to joined group                           [ OK ]
    TEST: br0: Multicast IPv4 to unknown group                          [FAIL]
            reception succeeded, but should have failed
    TEST: br0: Multicast IPv4 to unknown group, promisc                 [ OK ]
    TEST: br0: Multicast IPv4 to unknown group, allmulti                [ OK ]
    TEST: br0: Multicast IPv6 to joined group                           [ OK ]
    TEST: br0: Multicast IPv6 to unknown group                          [FAIL]
            reception succeeded, but should have failed
    TEST: br0: Multicast IPv6 to unknown group, promisc                 [ OK ]
    TEST: br0: Multicast IPv6 to unknown group, allmulti                [ OK ]

The selftest results for unoffloaded mode will stand as a future guide
for regression testing when you add later support for offloading various
features. So please try to spend some time running them now, ask
questions if needed.

> +	.phylink_mac_link_down	= gsw1xx_phylink_mac_link_down,
> +	.phylink_mac_link_up	= gsw1xx_phylink_mac_link_up,
> +	.get_strings		= gsw1xx_get_strings,
> +	.get_ethtool_stats	= gsw1xx_get_ethtool_stats,
> +	.get_sset_count		= gsw1xx_get_sset_count,

New DSA drivers should first add support for:

	.get_stats64
	.get_pause_stats
	.get_rmon_stats
	.get_eth_ctrl_stats
	.get_eth_mac_stats
	.get_eth_phy_stats

Only if there remains something uncovered do we start talking about
unstructured stats.

> +};
> +
> +void gsw1xx_remove(struct gsw1xx_priv *priv)
> +{
> +	if (!priv)
> +		return;
> +
> +	/* disable the switch */
> +	gsw1xx_mdio_mask(priv, GSW1XX_MDIO_GLOB_ENABLE, 0, GSW1XX_MDIO_GLOB);
> +
> +	dsa_unregister_switch(priv->ds);
> +
> +	if (priv->ds->slave_mii_bus) {
> +		mdiobus_unregister(priv->ds->slave_mii_bus);
> +		of_node_put(priv->ds->slave_mii_bus->dev.of_node);
> +		mdiobus_free(priv->ds->slave_mii_bus);
> +	}
> +
> +	dev_set_drvdata(priv->dev, NULL);

dev_set_drvdata() is no longer required from remove().
Please keep it in shutdown() though.

> +}
> +EXPORT_SYMBOL(gsw1xx_remove);
> +static const struct regmap_range gsw1xx_valid_regs[] = {
> +	/* GSWIP Core Registers */
> +	regmap_reg_range(GSW1XX_IP_BASE_ADDR,
> +			 GSW1XX_IP_BASE_ADDR + GSW1XX_IP_REG_LEN),
> +	/* Top Level PDI Registers, MDIO Master Reigsters */

s/Reigsters/Registers/

> +	regmap_reg_range(GSW1XX_MDIO_BASE_ADDR,
> +			 GSW1XX_MDIO_BASE_ADDR + GSW1XX_MDIO_REG_LEN),
> +};

Re: [RFC net-next 2/2] net: dsa: Add driver for Maxlinear GSW1XX switch

[Arun.Ramadoss]

Hi Camel,
On Tue, 2022-10-25 at 15:52 +0200, Camel Guo wrote:
> Add initial framework for Maxlinear's GSW1xx switch and
> currently only GSW145 in MDIO managed mode is supported.
> 
> Signed-off-by: Camel Guo <camel.guo@axis.com>
> ---
>  MAINTAINERS                   |   3 +
>  drivers/net/dsa/Kconfig       |  16 +
>  drivers/net/dsa/Makefile      |   2 +
>  drivers/net/dsa/gsw1xx.h      |  27 ++
>  drivers/net/dsa/gsw1xx_core.c | 823
> ++++++++++++++++++++++++++++++++++
>  drivers/net/dsa/gsw1xx_mdio.c | 128 ++++++
>  6 files changed, 999 insertions(+)
>  create mode 100644 drivers/net/dsa/gsw1xx.h
>  create mode 100644 drivers/net/dsa/gsw1xx_core.c
>  create mode 100644 drivers/net/dsa/gsw1xx_mdio.c
> 
> 
> diff --git a/drivers/net/dsa/gsw1xx.h b/drivers/net/dsa/gsw1xx.h
> new file mode 100644
> index 000000000000..08b2975e1267
> --- /dev/null
> +++ b/drivers/net/dsa/gsw1xx.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef GSW1XX_H
> +#define GSW1XX_H
> +
> +#include <linux/regmap.h>
> +#include <linux/device.h>

include headers in sorted manner.

> +#include <net/dsa.h>
> +
> +struct gsw1xx_hw_info {
> +	int max_ports;
> +	int cpu_port;
> +};
> +
> +struct gsw1xx_priv {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct dsa_switch *ds;
> +	const struct gsw1xx_hw_info *hw_info;
> +};
> +
> +extern const struct regmap_access_table gsw1xx_register_set;
> +
> +int gsw1xx_probe(struct gsw1xx_priv *priv, struct device *dev);
> +void gsw1xx_remove(struct gsw1xx_priv *priv);
> +void gsw1xx_shutdown(struct gsw1xx_priv *priv);
> +
> +#endif /* GSW1XX_H */
> diff --git a/drivers/net/dsa/gsw1xx_core.c
> b/drivers/net/dsa/gsw1xx_core.c
> new file mode 100644
> index 000000000000..1b3cbee4addc
> --- /dev/null
> +++ b/drivers/net/dsa/gsw1xx_core.c
> @@ -0,0 +1,823 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/delay.h>
> +#include <linux/etherdevice.h>
> +#include <linux/if_bridge.h>
> +#include <linux/if_vlan.h>
> +#include <linux/module.h>
> +#include <linux/of_mdio.h>
> +#include <linux/of_net.h>
> +#include <linux/of_platform.h>
> +#include <linux/phy.h>
> +#include <linux/phylink.h>
> +#include <linux/regmap.h>
> +#include <net/dsa.h>
> +
> +#include "gsw1xx.h"
> +
> +/* GSW1XX MDIO Registers */
> +#define GSW1XX_MDIO_BASE_ADDR		0xF400
> +#define GSW1XX_MDIO_REG_LEN		0x0422
> +#define GSW1XX_MDIO_GLOB		0x00
> +#define  GSW1XX_MDIO_GLOB_ENABLE	BIT(15)
> +#define GSW1XX_MDIO_CTRL		0x08
> +#define  GSW1XX_MDIO_CTRL_BUSY		BIT(12)
> +#define  GSW1XX_MDIO_CTRL_RD		BIT(11)
> +#define  GSW1XX_MDIO_CTRL_WR		BIT(10)
> +#define  GSW1XX_MDIO_CTRL_PHYAD_MASK	0x1F
> +#define  GSW1XX_MDIO_CTRL_PHYAD_SHIFT	5
> +#define  GSW1XX_MDIO_CTRL_REGAD_MASK	0x1F
> +#define GSW1XX_MDIO_READ		0x09
> +#define GSW1XX_MDIO_WRITE		0x0A
> +#define GSW1XX_MDIO_MDC_CFG0		0x0B
> +#define GSW1XX_MDIO_PHYp(p)		(0x15 - (p))

All Capital letters in Macro will be good.

> +#define  GSW1XX_MDIO_PHY_LINK_MASK	0x6000
> +#define  GSW1XX_MDIO_PHY_LINK_AUTO	0x0000
> +#define  GSW1XX_MDIO_PHY_LINK_DOWN	0x4000
> +#define  GSW1XX_MDIO_PHY_LINK_UP	0x2000
> +#define  GSW1XX_MDIO_PHY_SPEED_MASK	0x1800
> +#define  GSW1XX_MDIO_PHY_SPEED_AUTO	0x1800
> +#define  GSW1XX_MDIO_PHY_SPEED_M10	0x0000
> +#define  GSW1XX_MDIO_PHY_SPEED_M100	0x0800
> +#define  GSW1XX_MDIO_PHY_SPEED_G1	0x1000
> +#define  GSW1XX_MDIO_PHY_FDUP_MASK	0x0600
> +#define  GSW1XX_MDIO_PHY_FDUP_AUTO	0x0000
> +#define  GSW1XX_MDIO_PHY_FDUP_EN	0x0200
> +#define  GSW1XX_MDIO_PHY_FDUP_DIS	0x0600
> +#define  GSW1XX_MDIO_PHY_FCONTX_MASK	0x0180
> +#define  GSW1XX_MDIO_PHY_FCONTX_AUTO	0x0000
> +#define  GSW1XX_MDIO_PHY_FCONTX_EN	0x0100
> +#define  GSW1XX_MDIO_PHY_FCONTX_DIS	0x0180
> +#define  GSW1XX_MDIO_PHY_FCONRX_MASK	0x0060
> +#define  GSW1XX_MDIO_PHY_FCONRX_AUTO	0x0000
> +#define  GSW1XX_MDIO_PHY_FCONRX_EN	0x0020
> +#define  GSW1XX_MDIO_PHY_FCONRX_DIS	0x0060
> +#define  GSW1XX_MDIO_PHY_ADDR_MASK	0x001F
> +#define  GSW1XX_MDIO_PHY_MASK		(GSW1XX_MDIO_PHY_ADDR_M
> ASK | \
> +					 GSW1XX_MDIO_PHY_FCONRX_MASK |
> \
> +					 GSW1XX_MDIO_PHY_FCONTX_MASK |
> \
> +					 GSW1XX_MDIO_PHY_LINK_MASK | \
> +					 GSW1XX_MDIO_PHY_SPEED_MASK | \
> +					 GSW1XX_MDIO_PHY_FDUP_MASK)
> +
> 
> +struct gsw1xx_rmon_cnt_desc {
> +	unsigned int size;
> +	unsigned int offset;
> +	const char *name;
> +};
> +
> +#define MIB_DESC(_size, _offset,
> _name)                                        \
> +	{                                                              
>         \
> +		.size = _size, .offset = _offset, .name =
> _name                \
> +	}
> +
> +static const struct gsw1xx_rmon_cnt_desc gsw1xx_rmon_cnt[] = {
> +	/** Receive Packet Count (only packets that are accepted and
> not discarded). */
> +	MIB_DESC(1, 0x1F, "RxGoodPkts"),
> +	MIB_DESC(1, 0x23, "RxUnicastPkts"),
> +	MIB_DESC(1, 0x22, "RxMulticastPkts"),
> +	MIB_DESC(1, 0x21, "RxFCSErrorPkts"),
> +	MIB_DESC(1, 0x1D, "RxUnderSizeGoodPkts"),
> +	MIB_DESC(1, 0x1E, "RxUnderSizeErrorPkts"),
> +	MIB_DESC(1, 0x1B, "RxOversizeGoodPkts"),
> +	MIB_DESC(1, 0x1C, "RxOversizeErrorPkts"),
> +	MIB_DESC(1, 0x20, "RxGoodPausePkts"),
> +	MIB_DESC(1, 0x1A, "RxAlignErrorPkts"),
> +	MIB_DESC(1, 0x12, "Rx64BytePkts"),
> +	MIB_DESC(1, 0x13, "Rx127BytePkts"),
> +	MIB_DESC(1, 0x14, "Rx255BytePkts"),
> +	MIB_DESC(1, 0x15, "Rx511BytePkts"),
> +	MIB_DESC(1, 0x16, "Rx1023BytePkts"),
> +	/** Receive Size 1024-1522 (or more, if configured) Packet
> Count. */
> +	MIB_DESC(1, 0x17, "RxMaxBytePkts"),
> +	MIB_DESC(1, 0x18, "RxDroppedPkts"),
> +	MIB_DESC(1, 0x19, "RxFilteredPkts"),
> +	MIB_DESC(2, 0x24, "RxGoodBytes"),
> +	MIB_DESC(2, 0x26, "RxBadBytes"),
> +	MIB_DESC(1, 0x11, "TxAcmDroppedPkts"),
> +	MIB_DESC(1, 0x0C, "TxGoodPkts"),
> +	MIB_DESC(1, 0x06, "TxUnicastPkts"),
> +	MIB_DESC(1, 0x07, "TxMulticastPkts"),
> +	MIB_DESC(1, 0x00, "Tx64BytePkts"),
> +	MIB_DESC(1, 0x01, "Tx127BytePkts"),
> +	MIB_DESC(1, 0x02, "Tx255BytePkts"),
> +	MIB_DESC(1, 0x03, "Tx511BytePkts"),
> +	MIB_DESC(1, 0x04, "Tx1023BytePkts"),
> +	/** Transmit Size 1024-1522 (or more, if configured) Packet
> Count. */
> +	MIB_DESC(1, 0x05, "TxMaxBytePkts"),
> +	MIB_DESC(1, 0x08, "TxSingleCollCount"),
> +	MIB_DESC(1, 0x09, "TxMultCollCount"),
> +	MIB_DESC(1, 0x0A, "TxLateCollCount"),
> +	MIB_DESC(1, 0x0B, "TxExcessCollCount"),
> +	MIB_DESC(1, 0x0D, "TxPauseCount"),
> +	MIB_DESC(1, 0x10, "TxDroppedPkts"),
> +	MIB_DESC(2, 0x0E, "TxGoodBytes"),
> +};
> +
> +static u32 gsw1xx_switch_r(struct gsw1xx_priv *priv, u32 offset)
> +{
> +	int ret = 0;

In the below apis you have used *err* variable for storing the return
values. Here *ret* is used. It will be good to have either ret or err
in all the apis.
And Do we need to explicitly assign ret  to zero. since it is assigned
return value of regmap_read in the next statement itself.

> +	u32 val = 0;
> +
> +	ret = regmap_read(priv->regmap, GSW1XX_IP_BASE_ADDR + offset,
> &val);
> +
> +	return ret < 0 ? (u32)ret : val;
> +}
> +
> 
> +
> 
> +static u32 gsw1xx_switch_r_timeout(struct gsw1xx_priv *priv, u32
> offset,
> +				   u32 cleared)
> +{
> +	u32 val;
> +
> +	return read_poll_timeout(gsw1xx_switch_r, val, (val & cleared)
> == 0, 20,
> +				 50000, true, priv, offset);
> +}
> +
> +static int gsw1xx_mdio_poll(struct gsw1xx_priv *priv)
> +{
> +	int cnt = 100;

Is it possible to use u8 instead of int since the value is less than
255.

> +
> +	while (likely(cnt--)) {
> +		u32 ctrl = gsw1xx_mdio_r(priv, GSW1XX_MDIO_CTRL);
> +
> +		if ((ctrl & GSW1XX_MDIO_CTRL_BUSY) == 0)
> +			return 0;
> +		usleep_range(20, 40);
> +	}
> +
> +	return -ETIMEDOUT;
> +}
> +
> 
> 
> +static int gsw1xx_mdio(struct gsw1xx_priv *priv, struct device_node
> *mdio_np)
> +{
> +	struct dsa_switch *ds = priv->ds;
> +	int err;
> +
> +	ds->slave_mii_bus = mdiobus_alloc();
> +	if (!ds->slave_mii_bus)
> +		return -ENOMEM;

Is it possible to use devm_mdiobus_alloc api for memory allocation
which doesn't require mdiobus_free.

> +
> +	ds->slave_mii_bus->priv = priv;
> +	ds->slave_mii_bus->read = gsw1xx_mdio_rd;
> +	ds->slave_mii_bus->write = gsw1xx_mdio_wr;
> +	ds->slave_mii_bus->name = "mxl,gsw1xx-mdio";
> +	snprintf(ds->slave_mii_bus->id, MII_BUS_ID_SIZE, "%s-mii",
> +		 dev_name(priv->dev));

If you have struct mii_bus *bus and assign ds->slave_mii_bus = bus,
then all the above statement can fit in single line.
snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(priv->dev));

> +	ds->slave_mii_bus->parent = priv->dev;
> +	ds->slave_mii_bus->phy_mask = ~ds->phys_mii_mask;
> +
> +	err = of_mdiobus_register(ds->slave_mii_bus, mdio_np);

Simillarly here devm_of_mdiobus_register

> +	if (err)
> +		mdiobus_free(ds->slave_mii_bus);
> +
> +	return err;
> +}
> +
> +static int gsw1xx_setup(struct dsa_switch *ds)
> +{
> +	struct gsw1xx_priv *priv = ds->priv;
> +	unsigned int cpu_port = priv->hw_info->cpu_port;

Is it possible to use u8 instead of unsinged int.

> +	int i;
> +	int err;
> +
> +	gsw1xx_switch_w(priv, GSW1XX_IP_SWRES_R0, GSW1XX_IP_SWRES);
> +	usleep_range(5000, 10000);
> +	gsw1xx_switch_w(priv, 0, GSW1XX_IP_SWRES);
> +
> +	/* disable port fetch/store dma on all ports */
> +	for (i = 0; i < priv->hw_info->max_ports; i++)
> +		gsw1xx_port_disable(ds, i);
> +
> +	/* enable Switch */
> +	gsw1xx_mdio_mask(priv, 0, GSW1XX_MDIO_GLOB_ENABLE,
> GSW1XX_MDIO_GLOB);
> +
> +	gsw1xx_switch_w(priv, 0x7F, GSW1XX_IP_PCE_PMAP2);
> +	gsw1xx_switch_w(priv, 0x7F, GSW1XX_IP_PCE_PMAP3);
> +
> +	/* Deactivate MDIO PHY auto polling since it affects mmd
> read/write.
> +	 */
> +	gsw1xx_mdio_w(priv, 0x0, GSW1XX_MDIO_MDC_CFG0);
> +
> +	gsw1xx_switch_mask(priv, 1, GSW1XX_IP_MAC_CTRL_2_MLEN,
> +			   GSW1XX_IP_MAC_CTRL_2p(cpu_port));
> +	gsw1xx_switch_mask(priv, 0, GSW1XX_IP_BM_QUEUE_GCTRL_GL_MOD,
> +			   GSW1XX_IP_BM_QUEUE_GCTRL);
> +
> +	/* Flush MAC Table */
> +	gsw1xx_switch_mask(priv, 0, GSW1XX_IP_PCE_GCTRL_0_MTFL,
> +			   GSW1XX_IP_PCE_GCTRL_0);
> +	err = gsw1xx_switch_r_timeout(priv, GSW1XX_IP_PCE_GCTRL_0,
> +				      GSW1XX_IP_PCE_GCTRL_0_MTFL);
> +	if (err) {
> +		dev_err(priv->dev, "MAC flushing didn't finish\n");
> +		return err;
> +	}
> +
> +	gsw1xx_port_enable(ds, cpu_port, NULL);
> +
> +	return 0;
> +}
> +
> +static void gsw1xx_port_set_link(struct gsw1xx_priv *priv, int port,
> bool link)
> +{
> +	u32 mdio_phy;
> +
> +	if (link)
> +		mdio_phy = GSW1XX_MDIO_PHY_LINK_UP;
> +	else
> +		mdio_phy = GSW1XX_MDIO_PHY_LINK_DOWN;
> +
> +	gsw1xx_mdio_mask(priv, GSW1XX_MDIO_PHY_LINK_MASK, mdio_phy,
> +			 GSW1XX_MDIO_PHYp(port));
> +}
> +
> +static void gsw1xx_port_set_speed(struct gsw1xx_priv *priv, int
> port, int speed,
> +				  phy_interface_t interface)
> +{
> +	u32 mdio_phy = 0, mac_ctrl_0 = 0;

Initialize variable in different line to uniformity in the file.

> +
> +	switch (speed) {
> +	case SPEED_10:
> +		mdio_phy = GSW1XX_MDIO_PHY_SPEED_M10;
> +		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_GMII_MII;
> +		break;
> +
> +	case SPEED_100:
> +		mdio_phy = GSW1XX_MDIO_PHY_SPEED_M100;
> +		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_GMII_MII;
> +		break;
> +
> +	case SPEED_1000:
> +		mdio_phy = GSW1XX_MDIO_PHY_SPEED_G1;
> +		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_GMII_RGMII;
> +		break;
> +	}
> +
> +	gsw1xx_mdio_mask(priv, GSW1XX_MDIO_PHY_SPEED_MASK, mdio_phy,
> +			 GSW1XX_MDIO_PHYp(port));
> +	gsw1xx_switch_mask(priv, GSW1XX_IP_MAC_CTRL_0_GMII_MASK,
> mac_ctrl_0,
> +			   GSW1XX_IP_MAC_CTRL_0p(port));
> +}
> +
> +static void gsw1xx_port_set_duplex(struct gsw1xx_priv *priv, int
> port,
> +				   int duplex)
> +{
> +	u32 mac_ctrl_0, mdio_phy;

Simillarly here. In the above function, you have initialized the values
to zero. But not here.

> +
> +	if (duplex == DUPLEX_FULL) {
> +		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_FDUP_EN;
> +		mdio_phy = GSW1XX_MDIO_PHY_FDUP_EN;
> +	} else {
> +		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_FDUP_DIS;
> +		mdio_phy = GSW1XX_MDIO_PHY_FDUP_DIS;
> +	}
> +
> +	gsw1xx_switch_mask(priv, GSW1XX_IP_MAC_CTRL_0_FDUP_MASK,
> mac_ctrl_0,
> +			   GSW1XX_IP_MAC_CTRL_0p(port));
> +	gsw1xx_mdio_mask(priv, GSW1XX_MDIO_PHY_FDUP_MASK, mdio_phy,
> +			 GSW1XX_MDIO_PHYp(port));
> +}
> +
> +static void gsw1xx_port_set_pause(struct gsw1xx_priv *priv, int
> port,
> +				  bool tx_pause, bool rx_pause)
> +{
> +	u32 mac_ctrl_0, mdio_phy;

Simillarly here.

> +
> +	if (tx_pause && rx_pause) {
> +		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_FCON_RXTX;
> +		mdio_phy =
> +			GSW1XX_MDIO_PHY_FCONTX_EN |
> GSW1XX_MDIO_PHY_FCONRX_EN;
> +	} else if (tx_pause) {
> +		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_FCON_TX;
> +		mdio_phy =
> +			GSW1XX_MDIO_PHY_FCONTX_EN |
> GSW1XX_MDIO_PHY_FCONRX_DIS;
> +	} else if (rx_pause) {
> +		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_FCON_RX;
> +		mdio_phy =
> +			GSW1XX_MDIO_PHY_FCONTX_DIS |
> GSW1XX_MDIO_PHY_FCONRX_EN;
> +	} else {
> +		mac_ctrl_0 = GSW1XX_IP_MAC_CTRL_0_FCON_NONE;
> +		mdio_phy =
> +			GSW1XX_MDIO_PHY_FCONTX_DIS |
> GSW1XX_MDIO_PHY_FCONRX_DIS;
> +	}
> +
> +	gsw1xx_switch_mask(priv, GSW1XX_IP_MAC_CTRL_0_FCON_MASK,
> mac_ctrl_0,
> +			   GSW1XX_IP_MAC_CTRL_0p(port));
> +	gsw1xx_mdio_mask(priv,
> +			 GSW1XX_MDIO_PHY_FCONTX_MASK |
> GSW1XX_MDIO_PHY_FCONRX_MASK,
> +			 mdio_phy, GSW1XX_MDIO_PHYp(port));
> +}
> +
> 
> 
> 
> +
> +static void gsw1xx_get_ethtool_stats(struct dsa_switch *ds, int
> port,
> +				     uint64_t *data)
> +{
> +	struct gsw1xx_priv *priv = ds->priv;
> +	const struct gsw1xx_rmon_cnt_desc *rmon_cnt;
> +	int i;
> +	u64 high;

Reverse christmas tree

> +
> +	for (i = 0; i < ARRAY_SIZE(gsw1xx_rmon_cnt); i++) {
> +		rmon_cnt = &gsw1xx_rmon_cnt[i];
> +
> +		data[i] =
> +			gsw1xx_bcm_ram_entry_read(priv, port, rmon_cnt-
> >offset);
> +		if (rmon_cnt->size == 2) {
> +			high = gsw1xx_bcm_ram_entry_read(priv, port,
> +							 rmon_cnt-
> >offset + 1);
> +			data[i] |= high << 32;
> +		}
> +	}
> +}
> +
> 
> +static int gsw1xx_get_mac_eee(struct dsa_switch *ds, int port,
> +			      struct ethtool_eee *e)
> +{
> +	struct gsw1xx_priv *priv = (struct gsw1xx_priv *)ds->priv;
> +	u32 val = 0;

Val initialized to zero.

> +
> +	val = gsw1xx_switch_r(priv, GSW1XX_IP_MAC_CTRL_4p(port));
> +	e->tx_lpi_enabled = !!(val & GSW1XX_IP_MAC_CTRL_4_LPIEN);
> +
> +	e->tx_lpi_timer = 20;
> +
> +	return 0;
> +}
> +
> 
> 
> +int gsw1xx_probe(struct gsw1xx_priv *priv, struct device *dev)
> +{
> +	struct device_node *np, *mdio_np;
> +	int err;
> +	u32 version;
> +
> +	if (!priv->regmap || IS_ERR(priv->regmap))
> +		return -EINVAL;
> +
> +	priv->hw_info = of_device_get_match_data(dev);
> +	if (!priv->hw_info)
> +		return -EINVAL;
> +
> +	priv->ds = devm_kzalloc(dev, sizeof(*priv->ds), GFP_KERNEL);
> +	if (!priv->ds)
> +		return -ENOMEM;
> +
> +	priv->ds->dev = dev;
> +	priv->ds->num_ports = priv->hw_info->max_ports;
> +	priv->ds->priv = priv;
> +	priv->ds->ops = &gsw1xx_switch_ops;
> +	priv->dev = dev;
> +	version = gsw1xx_switch_r(priv, GSW1XX_IP_VERSION);
> +
> +	np = dev->of_node;
> +	switch (version) {
> +	case GSW1XX_IP_VERSION_2_3:
> +		if (!of_device_is_compatible(np, "mxl,gsw145-mdio"))
> +			return -EINVAL;
> +		break;
> +	default:
> +		dev_err(dev, "unknown GSW1XX_IP version: 0x%x",
> version);
> +		return -ENOENT;
> +	}
> +
> +	/* bring up the mdio bus */
> +	mdio_np = of_get_child_by_name(np, "mdio");
> +	if (!mdio_np) {
> +		dev_err(dev, "missing child mdio node\n");
> +		return -EINVAL;
> +	}
> +
> +	err = gsw1xx_mdio(priv, mdio_np);
> +	if (err) {
> +		dev_err(dev, "mdio probe failed\n");
> +		goto put_mdio_node;
> +	}
> +
> +	err = dsa_register_switch(priv->ds);
> +	if (err) {
> +		dev_err(dev, "dsa switch register failed: %i\n", err);
> +		goto mdio_bus;

To have readability, can be renamed to mdio_bus_unregister.

> +	}
> +	if (!dsa_is_cpu_port(priv->ds, priv->hw_info->cpu_port)) {
> +		dev_err(dev,
> +			"wrong CPU port defined, HW only supports port:
> %i",
> +			priv->hw_info->cpu_port);
> +		err = -EINVAL;
> +		goto disable_switch;
> +	}
> +
> +	dev_set_drvdata(dev, priv);
> +
> +	return 0;
> +
> +disable_switch:
> +	gsw1xx_mdio_mask(priv, GSW1XX_MDIO_GLOB_ENABLE, 0,
> GSW1XX_MDIO_GLOB);
> +	dsa_unregister_switch(priv->ds);
> +mdio_bus:
> +	if (mdio_np) {
> +		mdiobus_unregister(priv->ds->slave_mii_bus);
> +		mdiobus_free(priv->ds->slave_mii_bus);
> +	}
> +put_mdio_node:
> +	of_node_put(mdio_np);
> +	return err;
> +}
> +EXPORT_SYMBOL(gsw1xx_probe);
> +
> 
> 
> 
> diff --git a/drivers/net/dsa/gsw1xx_mdio.c
> b/drivers/net/dsa/gsw1xx_mdio.c
> new file mode 100644
> index 000000000000..8328001041ed
> --- /dev/null
> +++ b/drivers/net/dsa/gsw1xx_mdio.c
> @@ -0,0 +1,128 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * MaxLinear switch driver for GSW1XX in MDIO managed mode
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mdio.h>
> +#include <linux/phy.h>
> +#include <linux/of.h>
                                                          
Header file in sorted order.

> +
> +#include "gsw1xx.h"
> +
> +#define GSW1XX_SMDIO_TARGET_BASE_ADDR_REG	0x1F
> +
>