| Message ID | 20221025135243.4038706-2-camel.guo@axis.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | DSA driver draft for MaxLinear's gsw1xx series switch | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 13 of 13 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 160 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
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
> > + - 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
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
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.
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
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.
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 = <ð0>; > + phy-mode = "rgmii-id"; > + > + fixed-link { > + speed = <1000>; > + full-duplex; > + pause; > + }; > + }; > + };
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
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).
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 = <ð0>; + 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
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