[RFC,net-next,1/2] dt-bindings: net: dsa: add bindings for GSW Series switches
Commit Message
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
Comments
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).
new file mode 100644
@@ -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>;
+ };
+ };
+ };
+ };
@@ -863,6 +863,8 @@ patternProperties:
deprecated: true
"^mxicy,.*":
description: Macronix International Co., Ltd.
+ "^mxl,.*":
+ description: MaxLinear, Inc.
"^myir,.*":
description: MYIR Tech Limited
"^national,.*":
@@ -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